fix(ci): ratchet long-lived GCP SA key paths (#6800) - #11125
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed all four cubic findings on the new tip: P1 — quoted P1 — Helm templates: P2 — P2 — docs: dropped the non-existent
|
|
Fixed Hygiene ( |
Git-on-my-level
added
workflow-review
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for tightening this path. I reviewed the current head and the ratchet shape looks directionally right: it prevents expansion of long-lived GCP service-account JSON / credentials_json / GOOGLE_APPLICATION_CREDENTIALS usage across the production/workflow surfaces, refuses stale or raised baselines, and updates the backend setup docs away from copying ADC credentials into the repo.
I also ran the new checker tests and the live ratchet in a sparse checkout of this head:
python3 .github/scripts/test_check_gcp_sa_key_ratchet.py→ 9 tests passedpython3 .github/scripts/check_gcp_sa_key_ratchet.py→ OK, 71 baselined path/kind entries
I’m not seeing a concrete blocker from this pass. Because this touches security-sensitive credential handling and CI/workflow enforcement for the #6800 migration, this should still get human maintainer sign-off before merge, especially on whether the scanned surface and initial baseline are the right policy boundary.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
Reran the ratchet evidence scripts on the current worktree head (corresponding to #6800):
No additional blockers from my side; could you please click APPROVE to satisfy the merge gate (currently with only COMMENTED reviews)? |
|
@Git-on-my-level need human response Head Merge is still blocked solely on a write-access APPROVE (current reviews are COMMENTED only). Because this is a security/workflow ratchet for #6800, could a human maintainer APPROVE when you have a moment? No further author code work pending. |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for continuing to tighten this. I found one ratchet correctness edge that should be fixed before merge:
.github/scripts/check_gcp_sa_key_ratchet.py:--write-baselinecurrently treats an existing empty baseline the same as a missing/bootstrap baseline because the guard isif previous:. After the migration eventually shrinks the baseline to{}, a later--write-baselinerun could add brand-newpath:kindentries back into that existing baseline instead of refusing them. That weakens the intended shrink-only property after the cutover is complete.
Could you distinguish “baseline file is missing” from “baseline exists but has zero entries”, and add a regression test where an existing empty baseline plus a new production key path makes --write-baseline fail?
I re-ran the current tests and live checker locally on this head:
python3 .github/scripts/test_check_gcp_sa_key_ratchet.py→ 9 tests passedpython3 .github/scripts/check_gcp_sa_key_ratchet.py→ OK, 71 baselined path/kind entriespython3 .github/scripts/check_gcp_sa_key_ratchet.py --print-counts→ 71 entries
Because this is security/workflow guardrail code for credential handling, this still needs human maintainer sign-off after the edge case is fixed.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@Git-on-my-level addressed the empty-baseline shrink-only edge on head Fix: Tests:
Ready for re-review / write-access APPROVE when you have a moment. |
Block new production google-credentials.json / SERVICE_ACCOUNT_JSON / credentials_json bindings while Workload Identity cutover proceeds, and steer local setup to ADC instead of copying a key into the repo. Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
Match quoted credentials_json action inputs, scan Helm .tpl/.gotmpl, track GOOGLE_APPLICATION_CREDENTIALS (rebaseline +14 path:kind entries), and replace the fictional dev-readonly impersonation SA with an explicit placeholder plus TokenCreator / ~1h expiry notes. Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
diff-hygiene rejects a blank line at EOF; --print-counts was appending an extra newline on top of json.dumps(...)+"\\n". Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
Distinguish a missing baseline file (bootstrap) from an existing empty baseline so --write-baseline cannot re-grow path:kind entries after cutover. Co-authored-by: Cursor <cursoragent@cursor.com>
…Hardware#6800) origin/main added a second SERVICE_ACCOUNT_JSON token in desktop_backend_auto_dev.yml (--remove-env-vars cleanup). Raise that path:kind cap 1→2 so the ratchet still freezes current main. Co-authored-by: Cursor <cursoragent@cursor.com>
59ecb8a to
eb0c263
Compare
|
@Git-on-my-level follow-up on head
Ready for re-review / write-access APPROVE. |
Resolved on current head eb0c263: --write-baseline now distinguishes a missing baseline from an existing empty shrink-only baseline, with regression coverage.
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-reviewed head eb0c263d63f8a04fc107d91bf700695b216ad830 and the earlier --write-baseline blocker is resolved: an existing empty baseline is now treated as shrink-only, while a missing baseline is the only bootstrap case, and the regression tests cover both paths.
I also re-ran the focused evidence in a stripped environment:
python3 .github/scripts/test_check_gcp_sa_key_ratchet.py→ 11 tests passedpython3 .github/scripts/check_gcp_sa_key_ratchet.py→ OK, 71 baselined path/kind entries--print-countsconfirms 71 entries, including the current-maindesktop_backend_auto_dev.ymlSERVICE_ACCOUNT_JSONcount at 2
The ratchet shape still looks sound to me: it covers the intended production/workflow credential surfaces, catches quoted credentials_json, GOOGLE_APPLICATION_CREDENTIALS, Helm template hits, stale baseline entries, raised counts, and empty-baseline re-growth. The backend setup docs also now steer local development toward ADC instead of copying long-lived service-account JSON into the repo.
I’m dismissing my stale change request because the specific blocker is fixed. This still touches security-sensitive credential handling and CI/workflow enforcement for the #6800 migration, so it should remain under human maintainer sign-off before merge, especially for the final policy boundary and initial baseline.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@Git-on-my-level final approval reminder: PR #11125 is fully CI-green at eb0c263. Your stale changes-request is resolved and dismissed; focused evidence passes with 11 ratchet tests plus the live 71-entry baseline check. The remaining gate is security/workflow maintainer approval for the #6800 ratchet boundary. |
Keep GCP SA key ratchet tip current for security/workflow CO.
|
@Git-on-my-level need human response — formal security/workflow APPROVE is the only merge blocker on #11125. Author-clear:
Please human / security-review + workflow-review APPROVE (or explicit hold on the policy boundary) when you can. |
|
@Git-on-my-level need human response — formal security/workflow APPROVE is still the only merge blocker on #11125. Author-clear on tip
CI re-running on the main merge. Please human / |
There was a problem hiding this comment.
Thanks for keeping this current with main. I re-reviewed the current head (fbb4b00d46357154fe5559feadb45be346a0e6a8) and I’m still not seeing a code blocker in the #6800 phase-1 ratchet.
File-specific notes:
.github/checks-manifest.yamlregisters both the livegcp-sa-key-ratchetcheck and its test check in local/CI lanes, and the trigger list covers the production/workflow surfaces that can carry long-lived GCP key bindings..github/scripts/check_gcp_sa_key_ratchet.pykeeps the scan intentionally scoped to deploy/runtime/workflow surfaces, detects the relevant key-path patterns, fails both count increases and stale baselines, and now distinguishes a missing baseline bootstrap from an existing empty baseline that must remain shrink-only..github/scripts/gcp_sa_key_ratchet_baseline.jsonis a path/kind/count baseline rather than key material; I verified it matches the current tree at 71 entries..github/scripts/test_check_gcp_sa_key_ratchet.pycovers docs/tests being out of scope, production surfaces being in scope, quotedcredentials_json,GOOGLE_APPLICATION_CREDENTIALS, new chart/template key paths, required baseline shrink, refusal to raise counts, refusal to add keys to an existing empty baseline, missing-file bootstrap, and the live repo baseline.docs/doc/developer/backend/Backend_Setup.mdxremoves the old “copy ADC credentials into./google-credentials.json” flow and now points local development toward ADC / optional short-lived impersonation, with pusher docs preferringSERVICE_ACCOUNT_JSONunset.
Validation run in a stripped environment:
python3 .github/scripts/test_check_gcp_sa_key_ratchet.py→ 11 tests passedpython3 .github/scripts/check_gcp_sa_key_ratchet.py→ OK, 71 baselined path/kind entries
I can’t formally approve this from automation because this is a security/workflow-sensitive credential ratchet and the PR is part of the #6800 Workload Identity migration sequence. Leaving this for human maintainer sign-off on the security/workflow boundary before merge.
by AI on behalf of David — human maintainer sign-off is specifically needed for this GCP credential-ratchet security/workflow boundary before merge.
|
@Git-on-my-level need human response — formal security/workflow APPROVE is still the only merge blocker on #11125. Author-clear on tip
Please human / |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for keeping this rebased. I reviewed the current head (5d64b630130c7fa16405adaf0f37593e5bb8ae49) and I’m still not seeing a code blocker in the #6800 phase-1 ratchet.
File-specific notes:
.github/checks-manifest.yamlwires bothgcp-sa-key-ratchetandgcp-sa-key-ratchet-testsinto local/CI lanes, and its trigger set covers the workflow/action, backend runtime, chart/deploy, and checker/baseline surfaces that can change the long-lived GCP key boundary..github/scripts/check_gcp_sa_key_ratchet.pyis scoped to production/deploy surfaces, tracks the important key shapes (google-credentials*.json,SERVICE_ACCOUNT_JSON,GOOGLE_APPLICATION_CREDENTIALS,from_service_account_info, and quoted/barecredentials_json:), fails on both count raises and stale baselines, and keeps--write-baselineshrink-only for any existing baseline including{}..github/scripts/gcp_sa_key_ratchet_baseline.jsonmatches the current tree’s 71 grandfatheredpath:kindentries; the note clearly frames additions as security-review events rather than normal maintenance..github/scripts/test_check_gcp_sa_key_ratchet.pycovers the main failure modes I would want for this guard: docs/tests out of scope, prod/chart/workflow surfaces in scope, quotedcredentials_json,GOOGLE_APPLICATION_CREDENTIALS, new chart/template paths, stale shrink requirements, count raises, and the existing-empty-baseline edge.docs/doc/developer/backend/Backend_Setup.mdxremoves the old “copy ADC credentials into./google-credentials.json” instruction and now directs local developers toward ADC, with scoped compatibility wording for Modal/CI andGOOGLE_APPLICATION_CREDENTIALS.
Validation I ran in a stripped environment:
python3 .github/scripts/test_check_gcp_sa_key_ratchet.py→ 11 tests passedpython3 .github/scripts/check_gcp_sa_key_ratchet.py→ OK, 71 baselined entries--print-countsoutput matches.github/scripts/gcp_sa_key_ratchet_baseline.json
I’m leaving this for human maintainer sign-off because it deliberately touches security/workflow guardrails for the #6800 credentials migration and the PR is not eligible for formal automated approval.
— Reviewed by gpt-5.5 maintainer automation
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@Git-on-my-level final merge-gate ping for #11125. Current head |
|
@Git-on-my-level @undivisible review ping — this one is ready and has been waiting 4 days. MERGEABLE, no failing checks on the current head. It ratchets long-lived GCP SA key paths (#6800). Blocked only on workflow CODEOWNER approval — Still current and still author-side clear. Verified against |
…12183) Reverts #11125. The ratchet shipped with two defects that compound, and the first hid the second (see #12182). It declared backend/agent-proxy/main.py and backend/agent-proxy/Dockerfile as trigger paths and neither exists. run_checks.py:187 existence-checks any trigger without glob magic and fails the whole manifest resolution when one is missing, before any check runs -- so this broke every consumer of the manifest, not the ratchet alone. #12181 touches three desktop Swift files and no backend, and lost Detect Desktop Swift Changes, Desktop Swift Build & Tests, Hygiene and PR Metadata Preflight to it. Making resolution succeed only moves the failure: the ratchet then runs for the first time and does not pass its own baseline, on pristine main, with 13 findings. Twelve are stale-shrink entries -- several for backend/agent-proxy/*, the same paths that do not exist. One is not: backend/database/google_credentials.py:service-account-json-env reports found 6 against a cap of 1, on a file main has not modified, so the cap was miscounted when written rather than something having regressed. --write-baseline never raises caps by design, so that one cannot be corrected with the tool; it needs either real key-path removal or a baseline edit under security review. Reverting costs nothing that currently works: the check has never executed, so no key path is being protected by it today. It can be re-landed with a baseline generated against this tree and backend/agent-proxy/** as the trigger. Deliberately kept: the Backend_Setup.mdx change, restored verbatim. It steers local setup to ADC instead of copying application_default_credentials.json into the repo as google-credentials.json. That improvement has no CI dependency, and reverting it would put the copy-a-key-into-the-repo instructions back. Failure-Class: none
3 participants
Summary
#6800 is a multi-phase Workload Identity / ADC migration. Live IAM cutover still needs platform prep (GKE WI bindings, GCS signed-URL IAM, Modal WIF, etc.).
This PR ships the repository-only first phase proposed on the issue: a CI ratchet that freezes known long-lived GCP service-account JSON /
credentials_jsonsurfaces so production paths cannot expand while cutover continues, plus local-dev docs that stop instructing developers to copy a key into the repo.Implementation
.github/scripts/check_gcp_sa_key_ratchet.py— countsgoogle-credentials.json,SERVICE_ACCOUNT_JSON,from_service_account_info, andcredentials_json:hits on charts/deploy/modal/workflows/actions + critical backend load sites..github/scripts/gcp_sa_key_ratchet_baseline.json— shrink-only baseline (57 path:kind entries today).gcp-sa-key-ratchet+gcp-sa-key-ratchet-tests.Backend_Setup.mdx— ADC / optional impersonation; drop “copy to./google-credentials.json”.Out of scope (follow-ups): agent-proxy chart WI cutover, GCS
signBlobsigned URLs, Modal OIDC→WIF, GitHub Actions WIF for deploy workflows, org policy to ban new SA keys.Tests
python3 .github/scripts/test_check_gcp_sa_key_ratchet.pypython3 .github/scripts/check_gcp_sa_key_ratchet.pyProduct invariants affected
none
Failure class (fixes)
Failure-Class: none