Skip to content

ci: Add guarded step-5 bucket IAM removal workflow - #515

Draft
Pigbibi wants to merge 1 commit into
mainfrom
ci/step5-bucket-iam-gha-r2
Draft

Pigbibi wants to merge 1 commit into
mainfrom
ci/step5-bucket-iam-gha-r2

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

AI assistance

Codex (GPT-6, host-selected configuration).

What changed

Add one manual, default-dry-run workflow for the owner-approved Firstrade step-5 grant removal. It removes only the configured deployment service account's unconditional storage.objectCreator and storage.objectViewer grants from the configured bucket. No other files or workflows change.

Configuration is required before execution. The repository currently has zero GitHub Environments, and no repository variable FIRSTRADE_STEP5_ENVIRONMENT. Existing FT workflows hardcode their WIF bindings; this workflow does not reuse those literals. This PR does not create an Environment or change secrets, variables, IAM, or production settings.

Required configuration:

  • Repository variable FIRSTRADE_STEP5_ENVIRONMENT: the owner-selected, existing GitHub Environment name.
  • Secrets in that Environment: GCP_PROJECT_ID, GCP_WORKLOAD_IDENTITY_PROVIDER, GCP_WORKLOAD_IDENTITY_SERVICE_ACCOUNT, FIRSTRADE_STEP5_BUCKET, and FIRSTRADE_STEP5_SERVICE_ACCOUNT.
  • The configured bucket and target service account must be the exact pair in the existing step-5 approval. No real resource identifiers are published here.

The Environment WIF principal needs bucket IAM read/write permission. Authentication uses short-lived tokens without credential files. The default-branch and configuration checks precede authentication. Missing, duplicate, conditional, malformed, or unexpected grants fail closed. The write carries the freshly read etag, never retries, and compares the returned policy and fresh readback against the exact planned policy. Logs/artifacts contain masked policies and explicit two-removal evidence only. A workflow-level concurrency group serializes this operation; the etag protects against other writers.

How verified

  • Offline IAM safety probes: 12 passed (fake fixtures only), including exact removals, no unrelated edits, conditional/missing/duplicate grants, missing etag, stale-policy drift, conflict without retry, fresh-etag PUT, and recursive masking.
  • Baseline has no workflow/entrypoint: the isolated baseline probe fails with one missing-entrypoint import error; the 12 safety probes are unavailable there. This is feature absence, not 12 separately observed pre-existing defects.
  • Full Python suite: 1813 passed and 118 subtests passed, with NO_GCE_CHECK=true. The first unconfigured run was interrupted after 1098 passes because an existing notification test attempted GCE metadata discovery; the supported google-auth setting disables only that local discovery. No tests were removed or skipped.
  • Ruff, documentation check (one non-blocking existing advisory), package build, dependency/lock checks, 11 Dot review Node tests, actionlint, and staged diff checks passed.
  • Nonpublic known-identifier matches: zero. The generic scanner's two findings are standard id-token permission and add-mask command syntax; scanner rules were not changed.
  • No cloud IAM read, dry-run dispatch, apply dispatch, or runtime acceptance occurred.

Why

Execute the existing bounded owner approval through Environment-backed GitHub Actions without copying production credentials to the box or publishing resource identifiers.

Risk

High: this adds a privileged manual IAM workflow. Owner approval is limited to the existing exact two-grant removal. Required native checks and review/merge policy remain intact. No labels or approvals are changed by this job.

Execution blocked on missing Environment configuration. The worker stops after PR publication under the task's missing-configuration rule. The established review-and-merge bot owns review and merge. After configuration and merge, resume with a default-branch dry_run=true dispatch, inspect exact-two-removal evidence, then and only then dispatch dry_run=false and verify the masked before/after policy diff. No dispatch from an unmerged branch.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@Pigbibi

Pigbibi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

@codex review for the current head: correctness, IAM/bucket-removal safety, and any regression introduced by this change.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T16:34:40.032756Z c36c29c Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c36c29c508

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


def plan(before, member):
require(isinstance(before, dict), "Invalid policy")
require(set(before) <= {"bindings", "etag", "version"}, "Unknown policy fields")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Accept the Storage API's documented policy metadata

The workflow reads the Cloud Storage JSON API directly, whose documented getIamPolicy response includes kind: "storage#policy" and resourceId alongside the allowed fields (API reference). This allowlist therefore rejects a valid bucket policy with Unknown policy fields, preventing both dry-run evidence generation and grant removal even with correct Environment configuration. I reproduced this using the extracted plan() function and a synthetic response matching the documented schema. Accept and validate these metadata fields, or consistently strip them before planning and readback comparison.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants