SRE-915: Resolve the Vault target from the job's environment - #9161
SRE-915: Resolve the Vault target from the job's environment#9161TimDiekmann wants to merge 4 commits into
Conversation
The exported Vault token has no consumers — VAULT_TOKEN appears only as VAULT_TOKEN: "" in five places, clearing it again. Dropping exportToken removes the job-environment path, which was the only Vault path the payload could have walked on a GitHub runner. secrets: inherit passed the full secret set to a reusable workflow that reads no secrets.*, and contents: write was never used because Renovate commits with the App token.
…coped role The credentials move to ci/data/sccache/r2 behind a single-path policy, read by the ci-hash-sccache role instead of the broad dev role. The endpoint sits behind Cloudflare Access, so vault-action sends a service-token pair scoped to this repo and this environment. The sccache gate moves from vault_address to the Cloudflare client secret, which is the value that is actually required and is still empty on fork pull requests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
area/infra
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## t/sre-901-harden-the-centralized-renovate-workflow-against-compromised #9161 +/- ##
=======================================================================================================
Coverage 59.57% 59.57%
=======================================================================================================
Files 1413 1413
Lines 138053 138053
Branches 6510 6510
=======================================================================================================
Hits 82240 82240
Misses 54771 54771
Partials 1042 1042 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview sccache ( Sentry ( Legacy Vault stays inline where tier vars would be wrong: Reviewed by Cursor Bugbot for commit 29e65af. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Moves sccache credential retrieval to the staging Vault’s dedicated CI mount and scoped role.
Changes:
- Adds Cloudflare Access authentication to the sccache action.
- Uses the scoped
ci-hash-sccacherole andci/data/sccache/r2secrets. - Propagates staging Vault credentials through 20 workflow call sites.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.github/actions/install-sccache/action.yml |
Configures scoped Vault and Cloudflare authentication. |
.github/actions/install-tools/action.yml |
Adds and forwards the new authentication inputs. |
.github/workflows/ai-pr-review.yml |
Supplies staging Vault configuration. |
.github/workflows/bench.yml |
Supplies staging Vault configuration. |
.github/workflows/canary-release.yml |
Supplies staging Vault configuration. |
.github/workflows/codspeed.yml |
Supplies staging Vault configuration. |
.github/workflows/deploy.yml |
Supplies staging Vault configuration. |
.github/workflows/lint.yml |
Supplies staging Vault configuration. |
.github/workflows/publish-blocks-to-preview.yml |
Supplies staging Vault configuration. |
.github/workflows/release.yml |
Supplies staging Vault configuration. |
.github/workflows/test.yml |
Supplies staging Vault configuration. |
Merging this PR will degrade performance by 15.38%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | bit_matrix/dense/iter_row[64] |
140.8 ns | 170 ns | -17.16% |
| ❌ | bit_matrix/dense/iter_row[200] |
185.8 ns | 215 ns | -13.57% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing t/sre-915-narrow-vault-dev-role-housekeeping-workflow (29e65af) with t/sre-901-harden-the-centralized-renovate-workflow-against-compromised (c5953fd)
The job picked its Vault role from an expression about its own trigger, and nothing checked it. The role now binds the environment claim and, for main, the ref, so a pull-request branch cannot reach the main-tier token. The tokens move to ci/sentry/{main,pull-request} in the staging Vault.
GitHub Actions
Inactive
GitHub Actions
Inactive
TimDiekmann
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/deploy.yml:264
- The PR description explicitly says that only sccache moves and that
deploykeeps reading its own secrets from the other Vault, but this block also migratesSENTRY_AUTH_TOKENto the stagingci/mount and new role/environment contract. Please either revert the Sentry migration so the code matches the stated rollout boundary, or update the PR and its infrastructure verification to include this additional migration.
url: ${{ vars.VAULT_STAGE_ADDR }}
method: jwt
role: ci-hash-sentry-${{ env.VAULT_TIER }}
extraHeaders: |
CF-Access-Client-Id: ${{ vars.CF_ACCESS_CI_CLIENT_ID }}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a7283dd. Configure here.
The Sentry sourcemap job declares a GitHub environment, and the Vault address and Cloudflare Access credentials resolve from it: pull-request points at the staging instance, main at production. Each instance holds only its own tier's token, so the role name and the secret path carry no tier. sccache is deliberately exempt. It runs in twenty jobs that declare no environment, and a shared build cache should serve pull requests and main alike, so it keeps explicitly pinned names for the staging instance. Its install step also becomes non-fatal: a cache must not be able to fail the build. The two consumers still on the legacy Vault take its address inline, because they have no environment and would otherwise resolve to the wrong instance.
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/actions/install-tools/action.yml:59
continue-on-errorconverts every Vault authentication, secret lookup, installation, or server-start failure insideinstall-sccacheinto a successful tools setup. A wrong role/path or an unavailable Vault will therefore leave Rust jobs compiling without the configured cache while CI stays green, contrary to the stated verification that failures surface atRetrieve secrets. The existingifalready skips fork PRs when the secret is empty, so remove this suppression and let failures on configured runs propagate.
continue-on-error: true
.github/workflows/deploy.yml:264
- This implementation does not match the Vault contract described in the PR: the PR says both Sentry tiers are in the staging Vault at
ci/data/sentry/{main,pull-request}under rolesci-hash-sentry-{tier}, whereas this selects the production Vault formainand uses oneci-hash-sentryrole plusci/data/sentry/sourcemaps. Because the Vault state is already applied, please reconcile the workflow with that applied role/path layout (or update the PR contract if this newer per-instance layout is the applied one) before merging.
# Address and Access credentials come from the job's environment:
# `pull-request` resolves to the staging Vault, `main` to production.
# Each instance holds only its own tier's token, so the role name and
# the path carry no tier — the instance is the tier.
url: ${{ vars.VAULT_ADDR }}
🌟 What is the purpose of this PR?
Two CI secrets move to a dedicated
ci/mount, each read by a role scoped to a single path: the sccache build cache and the Sentry sourcemap token.Stacked on #9160. The Vault side is already applied.
🔗 Related links
internal-infra#319🚫 Blocked by
🔍 What does this change?
Sentry — the
sourcemapsjob declares a GitHub environment, and both the Vault address and the Cloudflare Access credentials resolve from it. Each instance holds only its own tier's token, so the role name and the path carry no tier. The environment is keyed on the ref, so aworkflow_dispatchonmain— a publishing run elsewhere in this workflow — gets the same tier as a push.sccache —
install-sccachereadsci/data/sccache/r2withrole: ci-hash-sccacheinstead ofrole: dev, and sends Access headers.install-toolsgainedvault_address,cf_access_client_idandcf_access_client_secret; the sccache gate moved to the client secret, which keeps it skipped on fork pull requests.sccache stays on explicitly pinned names rather than the environment-resolved ones: it runs in twenty jobs that declare no environment, and a shared cache should serve pull requests and
mainalike. Its install step is nowcontinue-on-error— a cache must not be able to fail the build.Legacy —
bench.ymlandrelease.ymlstill read their own secrets from the old Vault and take its address inline, because they have no environment and would otherwise resolve to the wrong instance.test.ymllost aVAULT_ADDRit no longer read.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
sourcemapsjob now produces a deployment record per run, pull requests included. Accepted for one job; the same on the sccache path would mean ~20 per run.account_idandbucketsit in Vault alongside the two R2 keys although neither is a secret, so both are masked in logs and the sccache endpoint reads ashttps://***.r2.cloudflarestorage.com. Moving them out means two more inputs across 20 call sites, which is not worth it until endpoint debugging actually hurts.benchandreleasestill read from the old Vault. Retiring it needs those two moved first.🐾 Next steps
VAULT_ADDRvariable and secret are unreferenced and can go.🛡 What tests cover this?
install-toolswith Rust enabled exercises it. Withcontinue-on-errora Vault failure no longer fails the job, so check the step's own outcome rather than the job's.deploy.ymlcovers one tier, a push tomainthe other.❓ How to test this?
Install toolsstep with Rust enabled —Retrieve secretssucceeds andStart sccache serverconnectsdeploy.ymlrun with sourcemaps in the matrix —sentry-cli loginsucceedsrust: falsestill skips sccache📹 Demo
Not applicable, CI-only change.