fix(security): give each cosign attest retry its own timeout, classify Rekor conflicts per stream - #373
Merged
Merged
Conversation
A Rekor createLogEntryConflict (HTTP 409) aborted `sc sbom attach` and `sc provenance attach`, failing the whole Pulumi update. Because both run after the workload resources, deploys went red with the new revision already rolled out and healthy — the attestation step was the only casualty, but operators had no way to tell that from the run status. cosign retries its Rekor upload on a timeout or 5xx. When the first attempt already committed server-side, the retry replays a byte-identical body and Rekor answers 409. Concurrent deploy jobs attesting against the public-good instance make that likely. `cosign sign` already handled this via runCosignSign; attest did not. Lift the conflict detector and the retry loop into RetryOnRekorConflict and use it from all three call sites. Retry rather than treating 409 as success: cosign uploads to Rekor before it pushes to the registry, so a tlog entry does not prove the attestation was attached. A fresh keyless invocation mints a new ephemeral certificate, so the replayed body differs and the conflict clears; deterministic keys reproduce the same signature and correctly exhaust the loop. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Cre-eD
requested review from
Laboratory,
smecsia and
universe-ops
as code owners
August 8, 2026 09:34
Semgrep Scan ResultsRepository:
Scanned at 2026-08-19 12:16 UTC |
Security Scan ResultsRepository:
Scanned at 2026-08-19 12:16 UTC |
📊 Statement coverageMeasured on the documented included set (see
Baseline: |
…tream Review of the first pass found the retry did not actually work. Both attest call sites built the timeout context OUTSIDE the retry closure, so all three attempts shared one a.Timeout budget. That breaks precisely the scenario the fix targets: the 409 exists because cosign timed out client-side and retried internally, so the conflicting attempt is the one that eats the wall clock. Attempt 2 was then SIGKILLed with empty stderr, the conflict was not classified, and the loop bailed after one invocation surfacing "signal: killed" — losing the createLogEntryConflict string the troubleshooting docs tell operators to search for. Verified by mutation: restoring the shared deadline fails the new regression tests. runCosignSign never had this bug because tools.ExecCommand derives a fresh context.WithTimeout per call. Rather than patch the divergence twice, collapse both attest sites onto one helper that owns the invocation: signing.RunCosignWithRetry(ctx, label, args, env, timeout) (string, error) Built on tools.ExecCommand, so a per-attempt deadline is structural. This also removes ~14 duplicated lines of exec plumbing per call site and the closure-captured variable runCosignSign needed to smuggle stdout out. Other review findings fixed: - Classify stdout and stderr separately again. Concatenating them let the loose "409" AND "/api/v1/log/entries" arm match across the seam, so a registry 409 (immutable-tag overwrite, concurrent blob upload) plus the tlog URL cosign prints read as a retryable conflict. Anchor that arm on the swagger shape [POST /api/v1/log/entries][409], which an image reference cannot satisfy. - Add jittered backoff and context cancellation, mirroring requestOIDCTokenWithRetry. The conflict is a contention artifact, so retrying instantly maximizes re-collision. - Report the first conflict on exhaustion, not the last error, so an incidental later failure cannot erase the diagnosis. - Keep isRekorConflict and maxCosignAttempts unexported; neither had a cross-package production consumer, and this is a published module. - Drop the claim that deterministic keys reproduce a signature. cosign signs with ecdsa.SignASN1, whose nonce is randomized, so a fresh invocation always produces a new signature. - Bump the cosign install target 3.0.2 -> 3.1.3. GHSA-whqx-f9j3-ch6m (medium, <=3.0.3) is "verification accepts any valid Rekor entry under certain conditions" — the same subsystem this change touches. - Extract the fake-cosign harness into pkg/security/tools/cosigntest. Three near-identical shell stubs had accumulated; the shared one quotes paths (a spaced TMPDIR broke the old ones) and fails loudly when the counter file is missing instead of reporting zero calls. - Document the attest paths and the retry policy in the troubleshooting page, which scoped both to sc image sign. Tests close the mutations that survived the first pass: per-attempt timeout budget, conflict on stdout, cross-stream false positive, first conflict preserved over a later error, success on the final allowed attempt, conflict text on a successful run, cancelled context, key-based attest retry, and a literal pin on the attempt bound. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
govulncheck went red on this branch without any dependency change: go1.26.6 shipped fixes for advisories the reachability pass now traces into our call graph, all of them 'Found in: <pkg>@go1.26.5, Fixed in: @go1.26.6'. GO-2026-6091 crypto/tls GO-2026-6089 net/http GO-2026-6090 crypto/tls GO-2026-6088 encoding/xml GO-2026-5972 encoding/asn1 GO-2026-5026 golang.org/x/net/idna (7th trace shares the net/http finding) Unrelated to the cosign retry work in this branch — it is calendar drift that will fail every open PR until the directive moves. Bumping the go directive is the whole fix; govulncheck reports 0 reachable vulnerabilities afterwards and ./pkg/... stays green. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Folding the deferred findings in rather than leaving them to a later PR. 1. Cached-SBOM attach ignored SBOM.Required. The generated-SBOM and provenance paths downgrade an attach failure to a warning when required is false; the cache-hit branch returned unconditionally. A transparency log outage was therefore fatal on a cache hit and merely a warning on a cache miss, and the documented sbom.required=false escape hatch had no effect on the branch operators actually hit. 2. sc sbom attach had no OIDC-token guard. sc provenance attach resolves the token and fails loudly without it, because keyless cosign 3.x can exit 0 while uploading no attestation. The SBOM CLI neither resolved nor validated it, so the Pulumi path could report a successful attach for an image that carries no SBOM attestation. Mirrors the provenance guard verbatim. 3. One configured output.local was shared by every image in a stack. Security operations run once per image, so two images wrote the same file: a concurrent sbom-att could attach the other image's SBOM, and scan results and the PR comment body overwrote each other. Each resolver now disambiguates by image via appendPathSuffix, the idiom already used for per-tool scan output. Nothing outside the runner reads these paths — no workflow globs them, only client.yaml declares them — so the rename is contained. The resolvers had zero test coverage, which is why the collision went unnoticed; they are pinned now. 4. Scanner pins disagreed with themselves. registry.go floored grype at 0.106.0 and trivy at 0.68.2 while the scanners pinned 0.111.0 and 0.70.0, so bumping one silently left the other behind. Centralize all four in pkg/security/tools/versions.go as the single source of truth, have both consumers derive from it, and bump to current: syft 1.51.0, grype 0.117.0, trivy 0.74.0. Version-boundary test fixtures now reference the constants instead of hard-coded literals, so the next bump cannot leave them stale. Design and requirements docs refreshed to match. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
smecsia
approved these changes
Aug 19, 2026
universe-ops
approved these changes
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A Rekor
createLogEntryConflict(HTTP 409) fails the whole deploy:The damaging part is where this lands.
sbom-att-*andprov-att-*run afterthe workload resources, so Pulumi reports
update failedwith the new revisionalready rolled out and healthy:
Deploymentupdated, revision N→N+1,updatedReplicas: 1✅prov-att-*→ Rekor 409 ❌sbom-att-*→ Rekor 409 →error: update failedPulumi still summarises
~ 8 updated, 9 changes, 25 unchanged. The rolloutsucceeded; only the attestation step died. Nothing in the run status conveys
that, so a red deploy sends operators hunting for a rollout problem that isn't
there.
Cause
cosign retries its Rekor upload on a client timeout or 5xx. When the first
attempt already committed server-side, the retry replays a byte-identical body
and Rekor answers 409 — its dedup response. Several deploy jobs attesting
against the public-good instance in the same window makes that likely.
cosign signalready handled this.cosign attestdid not, in either the SBOMor the provenance attacher.
Change
One helper owns the cosign invocation, the per-attempt deadline, and the retry:
It is built on
tools.ExecCommand, which derives a freshcontext.WithTimeoutper call, so each attempt getting its own full budget is structural rather
than something each call site has to remember.
runCosignSignbecomes aone-line wrapper; both attest call sites collapse from ~14 lines of exec
plumbing to a single call.
Retry, not treat-as-success. A 409 means the entry is in the tlog, which is
tempting to call done, but cosign uploads to Rekor before it pushes to the
registry — a tlog entry does not prove the attestation was attached. Swallowing
the 409 would report success on an image with no attestation, which is worse
than a red build. A fresh invocation always produces a new signature, so it
cannot conflict with itself; exhausting the loop means a persistent server-side
condition, and surfacing it is correct.
Also in this change:
and the loose
409+/api/v1/log/entriesarm is anchored on the swaggershape
[POST /api/v1/log/entries][409]. Concatenating the streams let themarker be assembled across the seam, so a registry 409 (immutable-tag
overwrite, concurrent blob upload) plus the tlog URL cosign prints on success
read as a retryable conflict. The anchored form is also not satisfiable by an
image reference, which is caller-controlled.
requestOIDCTokenWithRetryin
pkg/security/context.go. The conflict is a contention artifact, soretrying instantly maximizes re-collision against a rate-limited service.
cannot erase the
createLogEntryConflictstring the troubleshooting pagetells operators to search for.
isRekorConflict/maxCosignAttemptsstay unexported — neither had across-package production consumer, and this is a published module.
fixed 3.0.4) is "verification accepts any valid Rekor entry under certain
conditions" — the same subsystem this change touches. 3.0.2 shipped
2025-10-10; 3.1.3 is current.
pkg/security/tools/cosigntestreplaces three near-identical fake-cosignshell harnesses. It shell-quotes paths (a spaced
TMPDIRsilently broke theold ones) and fails loudly when the counter file is missing, instead of
reporting zero invocations.
sc image sign; it now covers the attest paths, the Pulumi-side symptom, theretry policy, and the
required: falseescape hatch.What review caught
The first pass built the timeout context outside the retry closure, so all
three attempts shared one
a.Timeout. That breaks exactly the scenario the fixtargets — the 409 exists because cosign burned the clock retrying internally, so
attempt 2 was SIGKILLed with empty stderr, the conflict went unclassified, and
the loop bailed after one invocation surfacing
signal: killed. It looked fixedand was not. Restoring the shared deadline fails the new regression tests.
A related claim in the first pass was wrong and is gone: cosign signs with
ecdsa.SignASN1, whose nonce is randomized, so key-based signing does notreproduce a signature and is not structurally stuck in a conflict loop.
Tests
go test ./pkg/...green;gofmt,go vetclean.Every test below was checked against a mutated implementation to confirm it
fails for the property it names:
EachRetryGetsItsOwnTimeoutBudget(sbom + provenance)ClassifiesConflictOnStdout,RetriesRekorConflictOnStdoutRegistryConflictPlusRekorURLIsNotAConflictSurfacesFirstConflictNotLastErrorSucceedsOnFinalAllowedAttemptConflictTextOnSuccessIsIgnoredStopsOnCancelledContextMaxCosignAttempts_IsPinnedKeyBasedRetriesRekorConflictThe
DelayEachstub option is what makes the timeout guard discriminating: eachinvocation fits inside its own window but two cannot fit in one shared window.
Verified on live infrastructure
A preview build of this branch was deployed against a real consumer stack. A
dry-run alone cannot prove the fix: it skips the image push, so nothing is ever
signed or attested. So this was a real staging deploy pinned to the preview.
Target was chosen deliberately: that stack builds two images, so it exercises
both the per-image artifact-path fix and two attestations racing within one
stack — the contention shape that produced the original 409.
The conflict actually occurred, and the deploy stayed green:
The provenance attest hit the conflict twice and succeeded on the third attempt.
Under the previous code the first of those aborted
sc provenance attachandfailed the whole
pulumi up— the reported incident exactly. Final state:✓ Image signed successfully with keyless OIDC signing×2✓ Image signature verified successfully×2✓ SBOM attached successfully×2✓ Provenance attached successfully×2error: update failed; pod rolled out, both containers ready, 0 restartsPer-image artifact paths rendered as intended — two distinct files where one was
shared before:
That also corrects an assumption from the review write-up: the multi-image case is
not hypothetical. The stack renders a full
sbom-gen/sbom-att/prov-att/scan/security-reportset per image, so thesbom.jsoncollision wasreachable in production — two
sbom-genruns wrote one file andsbom-attcouldattach the other image's SBOM. Identical pod image digests had masked this; the
digests match only because the two builds are byte-identical, while the image
names — and therefore the paths — differ.
Zero-mutation preview beforehand showed update-only plans with no resource
deletions (23 update / 25 unchanged on the two-image stack; 13 update / 28
unchanged on a custom-env stack). The update count is expected: changing the
artifact paths changes the rendered command strings, so the security commands
re-run once.
Warning: … attempt 3/3, retryingnever appears — the final-attempt guard holds.The throwaway branch used for this has been deleted.
Also in this PR
Four findings the review deferred, folded in rather than left to a later pass.
Cached-SBOM attach ignored
SBOM.Required. The generated-SBOM and provenancepaths downgrade an attach failure to a warning when
requiredis false; thecache-hit branch returned unconditionally. So a transparency-log outage was fatal
on a cache hit and a warning on a cache miss, and the documented
sbom.required=falseescape hatch had no effect on the branch operators actuallyhit.
sc sbom attachhad no OIDC-token guard.sc provenance attachresolves thetoken and fails loudly without it, because keyless cosign 3.x can exit 0 while
uploading no attestation. The SBOM CLI neither resolved nor validated it, so the
Pulumi path could report a successful attach for an image carrying no SBOM
attestation. The provenance guard is mirrored verbatim.
One configured
output.localwas shared by every image in a stack. Securityoperations run once per image, so two images wrote the same file: a concurrent
sbom-attcould attach the other image's SBOM, and scan results and the PRcomment body overwrote each other. Each resolver now disambiguates by image via
appendPathSuffix, the idiom already used for per-tool scan output. Nothingoutside the runner reads these paths — no workflow globs them, only
client.yamldeclares them — so the rename is contained. The resolvers had zero test
coverage, which is why the collision went unnoticed; they are pinned now.
Scanner pins disagreed with themselves.
registry.gofloored grype at0.106.0 and trivy at 0.68.2 while the scanners pinned 0.111.0 and 0.70.0, so
bumping one silently left the other behind. All four tool versions move to
pkg/security/tools/versions.goas the single source of truth, both consumersderive from it, and the pins go to current: syft 1.51.0, grype 0.117.0, trivy
0.74.0. Version-boundary fixtures now reference the constants instead of literals
so the next bump cannot leave them stale. Design and requirements docs refreshed.
Unrelated, but blocking the gate
go 1.26.5→1.26.6. govulncheck went red on this branch with no dependencychange: go1.26.6 shipped fixes for seven advisories the reachability pass traces
into our call graph (
crypto/tls,net/http,encoding/xml,encoding/asn1,x/net/idna). Calendar drift that will fail every open PR until the directivemoves; bumping it is the whole fix.