Skip to content

Concurrent re-approvals of the EKS rendered-surface fingerprint silently invalidate each other #3740

Description

@devantler

🤖 Generated by the Agentic Engineer

Evidence

scripts/validate-eks-ci-role-policy/main.go pins expectedRenderedSurfaceSHA, the aggregate
fingerprint of the rendered EKS authorization surface. Any change that moves a selected auth
document must re-approve it, with a conservation proof.

Three branches currently in flight each re-approved it, and all three declare the same base:

ref expectedRenderedSurfaceSHA declared Previous aggregate
main @491d5ef4 (#3677) 814debc4fdfaf76b… 5b85be735c3d7d32…
#3735 @67df9df3d1 43a3ac6748ec8adc… 5b85be735c3d7d32…
#3736 @bebf2723fa 413036a9623179f8… 5b85be735c3d7d32…

Read directly from the three trees. Identical base, three different results — so each aggregate
describes a tree containing its own delta and none of the others.

Why this is a security problem and not just a merge annoyance

The fingerprint exists to make an unreviewed change to the EKS authorization surface fail loudly.
Its guarantee is "this exact rendered surface was reviewed and approved". Concurrent re-approval
breaks that guarantee in a way that is invisible at exactly the moment someone is most likely to get
it wrong:

  1. Whichever branch merges first invalidates the other two, because their aggregates no longer
    describe a tree that includes the merged delta.
  2. The result surfaces as an ordinary merge conflict in a constant, and the natural resolutions —
    take ours, take theirs, or splice the two comment blocks together — all yield a fingerprint that
    does not describe the merged tree. The conflict markers give the resolver no signal that both
    values are stale; the only evidence is the Previous aggregate lines, which sit outside the
    conflicted hunk and read as prose.
  3. The failure mode is fail-open in appearance: a wrong-but-plausible constant looks exactly like
    a correct one. It is caught today only because CI re-renders and compares — so the control's real
    integrity currently rests on CI, not on the approval, while the approval is what carries the
    human-reviewed conservation narrative.

This nearly happened on #3735 today. It is DIRTY, GitHub cannot auto-update it, and the sole
conflicted file is this one.

Expected behaviour

A re-approval that was computed against a base which is no longer the merge base should be
detectable mechanically, rather than depending on a resolver noticing two hex strings in a
comment.

Acceptance criteria

  • The approval carries its base in a machine-readable form rather than only in prose — the
    aggregate it supersedes, next to the aggregate it asserts.
  • The validator fails when the recorded base is not the current base — i.e. when an approval was
    computed against a tree that has since moved — with a message that says re-derive the
    aggregate against the current base
    , not merely that the digests differ. A guardrail that
    blocks without naming the fix is a friction tax and gets routed around.
  • Proven RED/GREEN: a re-approval carrying a stale base is rejected, and the same approval
    with a current base passes. The RED case is the one that matters — assert it fails for the
    right reason, not merely that it is non-zero.
  • A negative control: an unrelated change that legitimately moves nothing must not start failing.

Note on where the digest can be derived

Worth recording for whoever picks this up, because it shapes the fix: the aggregate cannot be
computed on the agent host
. The file's own comments record that this host renders with kubectl
v1.36.1 / kustomize v5.8.1, which the validator rejects as unapproved, so approvals here are taken
from CI's failure output. Any proposed fix that assumes local re-derivation will not work in the
environment that actually has to run it.

Related: #3735, #3736 are both blocked behind exactly this.

Rough size: small-to-medium — the detection is a few lines; the care is all in the RED/GREEN proof.

Activity

  1. moved this to 📥 Backlog in 🌊 Project Boardon Sep 12, 2026
  2. devantler commented on Sep 12, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Two more concurrent re-approvals, both off the same base

    Two open PRs now carry a re-approved fingerprint computed against the same main aggregate:

    PR head expectedRenderedSurfaceSHA declared previous
    #3736 becc4d8d a5304bbb… 814debc4…
    #3741 65e833d4 ce959e8f… 814debc4…

    Each value was read from CI on its own branch, so each is correct only for a tree that holds its own delta. Whichever PR merges second needs its fingerprint re-derived from CI against the new main. Resolving the conflict in the constant by hand cannot produce a correct value. Both PRs name this ordering in their descriptions.

  3. devantler commented on Sep 13, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Merge-order progress (2026-09-13)

    The Wedding database recovery succeeded at 02:03Z, so the production deploy slot is free and the fingerprint-moving PRs are going through in order.

    PR Moves the EKS fingerprint? State
    #3741 yes merged 02:26Z at its reviewed head; main's fingerprint is now 525e04ee
    #3732 no merged 02:39Z
    #3751 no in the merge queue (running its checks)
    #3736 yes main merged in (87a682d3); the constant stays on the 525e04ee base until CI reports the rendered fingerprint, then it gets pinned and reviewed
    #3735 yes next: merge main and re-pin once #3736 has landed

    Each re-pin uses the fingerprint CI's approved renderer reports for that PR's merged tree, never a local value.

  4. devantler commented on Sep 13, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Merge order complete (2026-09-13)

    All three PRs that change the EKS fingerprint have merged, each at the exact commit that was reviewed and re-pinned:

    PR Merged head Merge commit Fingerprint after merge
    #3741 11691a0f 61a8c311 525e04ee
    #3736 854c262d f274ea26 e36a3db7
    #3735 7d1b460d d84c2503 59f51f17

    main now reads 59f51f1775bcded62ab018a6389ef11420b696a3b00357fc4424c4ab83935dba. Each re-pin took its value from what CI reported for that PR's merged tree, with no missing or duplicate resources.

    #3736 and #3735 each needed another merge of main and a new pin once the PR ahead of them landed. That rework is the cost this issue describes.

  5. self-assigned this
    on Sep 13, 2026
  6. moved this from 📥 Backlog to 🏃🏻‍♂️ In Progress in 🌊 Project Boardon Sep 13, 2026
  7. added a commit that references this issue on Sep 13, 2026
  8. moved this from 🏃🏻‍♂️ In Progress to 👀 In Review in 🌊 Project Boardon Sep 13, 2026
  9. moved this from 👀 In Review to ✅ Done in 🌊 Project Boardon Sep 13, 2026
  10. moved this from ✅ Done to 📊 Verifying in 🌊 Project Boardon Sep 13, 2026
  11. devantler commented on Oct 11, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Verification read 2026-10-11 — outcome decided, card moved to Done.

    Delivered by #3759 (merged 2026-09-13): the approval records the base it was computed against, and the validator rejects a re-approval whose base has moved, with tests for both the stale and the current case. The required checks on main are green at the current head, so those tests still pass. I did not find a later pull request that hit the stale-base refusal for real, so the refusal message has been seen only in tests.

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Fields

No fields configured for Security.

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions