ci: harden checkout credential handling for INC-7027

[leiicamundi]

Workflow credential-handling hardening.

Tracked under INC-7027 — rationale, analysis and review notes are in the incident ticket and are intentionally not repeated here.

Not tested. YAML and step ordering were validated; the runtime path has not been exercised. Please confirm on a throwaway run before merging.

Questions to the incident channel or the ticket, please — not this thread.

[Copilot]

Pull request overview

This PR hardens two GitHub Actions workflows against token exfiltration by ensuring the Distro CI GitHub App token is not persisted in .git/config while unreviewed branch content (local actions, npm install, go mod tidy, make targets) is executed, and by moving the actual git push into a dedicated final step.

Changes:

• Set actions/checkout persist-credentials: false in both workflows to prevent persisting the App token in .git/config.
• Configure EndBug/add-and-commit to not push (push: false) and add an id so its committed output can gate follow-up behavior.
• Add a final Push changes step that injects the App token only for the push and only when a commit was created.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
.github/workflows/renovate-post-upgrade.yaml	Prevents checkout credential persistence and moves pushing to a final gated git push step (Renovate branch post-upgrade chores).
.github/workflows/chart-chores.yaml	Prevents checkout credential persistence and moves pushing to a final gated git push step (PR chart chores automation).

[leiicamundi]

Good catch, and verified against AGENTS.md line 11 ("NEVER write reasoning/'why'/narration comments — comments explain only non-obvious HOW ... tactical rationale ... goes in the PR body or commit message"). Fixed in 4252245.

The blocks are now one line each, limited to the non-obvious HOW — namely the coupling a future editor could otherwise miss: persist-credentials: false on the checkout and push: false on add-and-commit only work as a pair with the trailing Push changes step. Removing either one in isolation breaks the job. The full incident rationale lives in the PR body.

[leiicamundi]

Good catch, and verified against AGENTS.md line 11 ("NEVER write reasoning/'why'/narration comments — comments explain only non-obvious HOW ... tactical rationale ... goes in the PR body or commit message"). Fixed in 4252245.

The blocks are now one line each, limited to the non-obvious HOW — namely the coupling a future editor could otherwise miss: persist-credentials: false on the checkout and push: false on add-and-commit only work as a pair with the trailing Push changes step. Removing either one in isolation breaks the job. The full incident rationale lives in the PR body.

[eamonnmoloney]

I have concerns about both verification and the security boundary here:

• The successful Chart Chores run did not exercise the new push path: Push changes was skipped because no commit was created. The Renovate post-upgrade path was not exercised either, so CI has not yet shown that push: false followed by the explicit authenticated git push preserves the existing behavior.
• More importantly, moving the token to a final step in the same job does not isolate it from code executed earlier on that runner. Earlier PR/dependency-controlled code can persist state for later steps, for example by installing .git/hooks/pre-push, appending a malicious git wrapper to $GITHUB_PATH, or setting BASH_ENV through $GITHUB_ENV. That code would then run when the final step exposes GH_APP_TOKEN. persist-credentials: false prevents passive extraction from .git/config, but the fresh-job boundary is what prevents earlier code from intercepting a later credential.

Could you please clarify how you intend to validate the commit-and-push path and whether the privileged token generation/push can be moved to a separate trusted job on a fresh runner?

[leiicamundi]

You're right on both counts, and the second one is the important one — thanks for pushing on it.

On the security boundary. I over-claimed. Ordering the credential after the untrusted steps only closes passive extraction from .git/config. It does not close active interception, and your three examples are exactly right: a pre-push hook, a git shim prepended via $GITHUB_PATH, or BASH_ENV set through $GITHUB_ENV all survive into the later step and fire when the token is exposed. I've since added core.hooksPath redirection in the sibling PRs, but that addresses one of your three vectors and leaves the other two open. Same-runner ordering is defence-in-depth, not a boundary. The fresh-job boundary is the actual control, as you say.

On validation. You're also right that nothing has exercised the push. Both runs so far ended with a clean tree, so Push changes was skipped and committed: false — the credential steps ran, the push did not. I did not want to overstate that, which is why the PR body still says untested, but it deserves to be stated plainly rather than left in a caveat.

To answer your two questions directly:

1. Validation — I'd propose forcing the path on a throwaway branch: introduce a deliberate formatting violation so chart-chores actually produces a commit, confirm the push lands and the loop guard behaves, then drop the branch. I'm happy to do that, or to leave it to whoever owns the workflow if you'd rather it ran under your eyes. It cannot be validated from a PR that changes no chart content.

2. Job split — yes, it can and I think it should. The shape would be: an unprivileged job checks out the branch, runs the chores/post-upgrade tasks, and uploads the resulting diff as an artifact; a second job on a fresh runner mints the token, downloads the artifact, applies it, commits and pushes. No repository-supplied code ever executes on the runner that holds the credential, which is the boundary you're describing. Applying a patch is data handling rather than code execution, so the residual is much smaller.

That is a bigger change to your workflow than the one in front of you, so I'd rather not push it unannounced. Two options, your call:

• I convert this PR to the split now and you review the whole thing, or
• this PR lands as the incremental step it is, honestly labelled as not a boundary, and the split goes in as a follow-up tracked on INC-7027.

I have no preference beyond wanting it reviewed by someone who owns this workflow. Given your point stands, I'd understand if you'd rather not merge the incremental version at all — it is strictly better than the current state but it is not the fix.

[leiicamundi]

Handing the architectural fix to this team rather than attempting it myself.

The change in this PR is an incremental one: it keeps the credential out of .git/config and out of process arguments. It is not a boundary. As raised on the sibling PR, code that runs earlier in the same job can plant a pre-push hook, prepend a git shim via $GITHUB_PATH, or set BASH_ENV through $GITHUB_ENV, and any of those fire later when the credential appears. Those are per-job runner state, so the only reliable isolation is a second job on a fresh runner.

Four workflows on INC-7027 have been converted to that shape, and two are worth reading as references:

• ci: harden checkout credential handling for INC-7027 infraex-common-config#546 — .github/workflows/lint-global.yml. Closest analogue: pre-commit hooks run unprivileged, emit a patch, and a separate job mints the Vault token and pushes.
• camunda/team-fleetmanagement#210 — .github/workflows/lint.yml. Smallest version of the same pattern.

The shape is:

1. Unprivileged job — permissions: contents: read, references no secret at all. Runs the existing steps unchanged. Produces git diff --binary > autofix.patch, sets a has_changes output, uploads the patch.
2. Privileged job — needs: the first, gated on has_changes, fresh runner. Mints the credential, checks out the branch, downloads the patch, validates it against a path allowlist, applies it, commits and pushes with a per-command credential helper.

Two details that are easy to miss:

• The patch is untrusted input. Validate it with git apply --numstat and reject anything outside what the workflow legitimately edits — otherwise autofix becomes a way to rewrite CI. In lint-global.yml anything under .github/ is refused outright.
• If the privileged job runs a local composite action (uses: ./.github/...), pin that checkout to the base ref. Otherwise PR-supplied code executes in the privileged job and the split achieves nothing. saas-argocd-apps deps: update github/codeql-action action to v3.29.10 #3955 has a worked example.

I did not attempt the conversion here because this workflow is materially more complex than the ones above and I would be guessing at behaviour I cannot exercise. Happy to draft it if you would rather review a proposal than write it — just say so.

Your call whether this PR lands as the incremental step or waits for the split. If you would rather not merge the incremental version at all, that is a reasonable position and I would not argue against it.

[eamonnmoloney]

Note on the 233 golden files that briefly appeared here, and the workflow bug behind them

Between 7ab9998 and d5ce4165 this PR briefly carried a distro-ci[bot] commit (6b65e05, "chore: chart chores") rewriting 233 golden files across camunda-platform-8.3, 8.4, 8.5 and 8.6. That commit has been dropped and the PR is back to two workflow files. Recording the cause here because the bug is in chart-chores.yaml itself and will recur on any PR that touches that workflow without touching a chart.

What happened. chart-chores.yaml triggers on charts/** or on .github/workflows/chart-chores.yaml. This PR matched the second path, so the workflow ran on a PR with no chart changes. ct list-changed returned nothing, and the empty value was passed straight through:

chartPath="${CHANGED_CHARTS}" make go.update-golden-only

Makefile:4 then does:

chartPath := $(if $(chartPath),$(chartPath),charts/camunda-platform-*)

Because chartPath arrives as a shell environment variable rather than a make command-line override, the makefile assignment still applies and the $(if ...) fallback expands to every chart version. Verified directly:

chartPath=""                             make -n → find charts/camunda-platform-*
chartPath="charts/camunda-platform-8.10" make -n → find charts/camunda-platform-8.10

So all chart versions were regenerated, the four oldest produced a diff, add-and-commit committed it, and the push step sent it back to the PR branch.

The diff was not meaningful change. 479 insertions / 233 deletions, of which 246 are blank lines and 4 are whitespace-only lines. Files went from interval: 10s with no EOF newline to interval: 10s\n\n. This is an artefact of the \s+helm.sh/chart:\s+.* → "" substitution in utils/goldenfiles.go, which consumes the preceding newline and indentation and leaves the trailing one behind.

Fix applied in d5ce4165. The three update steps are now guarded on a non-empty CHANGED_CHARTS, matching the pattern already used in renovate-post-upgrade.yaml. The end-of-support exclusion from that workflow is deliberately not copied — regenerating an out-of-support chart is correct when someone actually edits it.

Confirmed working in production on run 32011351565:

Update golden files → "No chart changes, skipping golden file update."
Update README       → "No chart changes, skipping README update."
Update Schema       → "No chart changes, skipping schema update."
add-and-commit      → "> Working tree clean. Nothing to commit."
Push changes        → skipped

Two things worth separating out from this PR.

1. That bot commit is the only production evidence the hardened push path works — it was produced by the credential-helper push step with core.hooksPath=/dev/null and fetch: false in place. Dropping it removes that evidence from the branch, though run 32011352361 and its predecessor remain in Actions history. The guard now makes it impossible for chart-chores to produce a commit on this PR, so the push path is unproven on this branch again.

2. The goldens for 8.3–8.6 on main do not match current generator output. Since goldenfiles.go compares with an exact Require().Equal, those tests would fail if they ran, which suggests they are not exercised for out-of-support versions. Real issue, but it belongs in its own change rather than here.