fix(provenance): align attest/verify predicate type, plumb keyless OIDC token, enforce tool MinVersion#287
Merged
Conversation
…DC token, enforce tool MinVersion Three latent bugs in the provenance attach→verify cycle that combined to fail storage-service deploys with "none of the attestations matched the predicate type: https://slsa.dev/provenance/v1, found: https://sigstore.dev/cosign/sign/v1" even though `sc provenance attach` reported success. 1. Predicate-type asymmetry. `Attacher.Attach` passed cosign's short-form alias `slsaprovenance1` to `cosign attest --type`, while the Pulumi verify step hard-coded the URI `https://slsa.dev/provenance/v1`. Cosign 3.x changed how short aliases resolve on the verify side; only matching URI values are guaranteed to round-trip across cosign minor versions. Both call sites now read from a single exported constant (`provenance.PredicateTypeSLSAV10`), and `attestationType()` returns the URI form so the DSSE envelope's predicateType is byte-identical to the verifier's match string. The verify call site is factored into a `verifyProvenanceArgs` helper so the symmetry is testable. 2. Missing OIDC token in keyless attach. `cmd_provenance/attach.go` built a `signing.Config{Keyless: true}` but never populated `OIDCToken`. The `Attacher.buildSigningEnv` consequently never emitted `SIGSTORE_ID_TOKEN`, leaving cosign 3.x to attempt ambient-credential discovery. In some keyless attest paths this exits 0 without uploading the attestation — matching the observed "attach success, verify can't find SLSA" symptom. The fix mirrors `cmd_image/sign.go`: pull from `SIGSTORE_ID_TOKEN` or the GitHub Actions OIDC request endpoint via `security.NewExecutionContext`, and fail loudly if neither is available. 3. Installer MinVersion never enforced. `ToolInstaller.InstallIfMissing` gated on `IsToolAvailable` (PATH lookup only) so a runner-installed cosign 2.x was silently accepted even though the registry pins 3.0.2. This is what let the cosign version drift on Blacksmith runners change SC behavior without anyone noticing. The gate now uses `CheckInstalledWithVersion`, so present-but-stale tools trigger re-install of the pinned version. Tests: - `TestAttestationType` / `TestAttachAndVerifyUseSamePredicateType`: assert canonical URI return and pin the attach/verify type-string contract. - `TestVerifyProvenanceArgs`: assert the verify-attestation arg builder uses the same constant as the attacher. - `TestInstallIfMissingChecksVersion`: registers a fake tool with an unreachable MinVersion and asserts the installer no longer silently accepts a present-but-stale binary. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Semgrep Scan ResultsRepository:
Scanned at 2026-05-22 10:23 UTC |
Security Scan ResultsRepository:
Scanned at 2026-05-22 10:23 UTC |
… precedence
Two regressions / latent gaps surfaced by codex review on the prior
commit:
1. NewExecutionContext now always attempts OIDC token resolution, not
only when IsCI is true. Previously a developer running
`sc image sign --keyless` or `sc provenance attach --keyless` locally
with SIGSTORE_ID_TOKEN already exported was rejected with "OIDC token
not available" because the new attach guard fired before the env-var
lookup ran. GetOIDCToken still checks SIGSTORE_ID_TOKEN first, so the
local-dev path now works the same as in CI; failures stay non-fatal
and the CI-vs-local stderr policy is preserved.
2. InstallIfMissing now moves installDir to the FRONT of PATH (and
removes any duplicate occurrence) instead of only appending it when
missing. The previous logic let a stale system-package binary at
/usr/bin/cosign keep winning when installDir was already on PATH
after the stale location — defeating the MinVersion enforcement the
prior commit introduced. Post-install verification now uses
CheckInstalledWithVersion so we fail loud if the freshly-installed
binary still doesn't satisfy MinVersion (e.g., shadowed by an even
newer broken copy, or downloaded but non-executable).
Tests:
- TestNewExecutionContextHonoursSIGSTOREIDTokenOutsideCI /
TestNewExecutionContextNoTokenOutsideCIIsNonFatal: pin the local-dev
contract and the no-token non-fatal path.
- TestPrependToPath: table-driven coverage of the front-of-PATH move,
duplicate removal, empty-entry handling, and the substring-lookalike
guard ("/usr/local/binutils" must not collapse into "/usr/local/bin").
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Gemini review on the prior commit surfaced one valid P0 and two P2s on
the installer + CLI:
1. P0: data race on PATH mutation. `InstallIfMissing` rewrites a
process-global env var via `os.Setenv("PATH", …)`. If two installer
calls overlap (parallel pre-flight, future fan-out), the get-modify-
set sequence races and one of the newly-installed directories can be
dropped. The mutation predates this PR but the new front-of-PATH move
makes it more frequent. Add a package-level `sync.Mutex` (`pathMu`)
serialising the read-compute-write block.
2. P2: trailing-slash dedup failure. `prependToPath` compared raw
strings, so `/usr/local/bin/` in $PATH and `/usr/local/bin` as the
install dir produced a duplicate. Switch to `filepath.Clean` for
comparisons.
3. P2: POSIX empty-entry semantics. The previous helper dropped empty
PATH segments, but in POSIX an empty entry denotes the current
directory. Preserve empties verbatim — the prepend still wins for
our installed binary; we just no longer silently rewrite the
developer's `.:` shadow.
4. P2: error-message accuracy. The keyless-attach error referenced only
GitHub Actions. Generalise to "set SIGSTORE_ID_TOKEN, or run from a
CI provider that supplies one (e.g. GH Actions …)".
Tests:
- `TestPrependToPath` gains coverage for trailing-slash dedup (both
sides), POSIX empty-entry preservation, and substring-lookalike
preservation. The previous "empty entries dropped" assertion is
inverted to match POSIX-correct behaviour.
Gemini's P1 ("swallow errors outside CI") is by design: the only
GetOIDCToken error outside CI is the generic "OIDC token not available"
fall-through, which would be one stderr line per `sc` invocation for
non-keyless flows. The higher-level keyless-attach error in attach.go
gives an actionable message; the lower-level error adds no signal.
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
smecsia
previously approved these changes
May 22, 2026
Six new SSH-related CVEs disclosed in golang.org/x/crypto between main's last green (2026-05-21T18:19Z) and this PR's first govulncheck run (2026-05-22T05:01Z): GO-2026-5015, 5017, 5018, 5019, 5020, 5021. All fixed in x/crypto v0.52.0. None reachable from any code touched by the provenance fix — bundled here purely to clear the failing `govulncheck (reachability-aware)` gate so the PR can merge. Local `govulncheck -mode source -test ./...` after the bump: === Symbol Results === No vulnerabilities found. Your code is affected by 0 vulnerabilities. Collateral: - `go mod tidy` promoted gocloud.dev from indirect to direct (it is imported in pkg/clouds/pulumi/create_stack.go via `gocloud.dev/gcerrors` — prior classification was stale). - `golang.org/x/sys` v0.44.0 → v0.45.0 came along as an x/crypto transitive. - `go generate -tags tools` refreshed two `Code generated by mockery` header comments from v2.53.4 to v2.53.6 (the version pinned in go.mod since #273); content unchanged. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
smecsia
approved these changes
May 24, 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
2 participants
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.
Summary
Fixes the production
verify-provenancefailure observed on storage-service deploy (Integrail/storage-service#26245635924, 2026-05-21):sc provenance attachreports success, but the image carries only a cosign-sign attestation — no SLSA-provenance attestation. Three latent bugs combined to cause this; the cosign-on-runner version drift (2.x → 3.x on Blacksmith) is what made the latent issues surface.Root causes (all addressed)
1. Predicate-type asymmetry between attach and verify
Attacher.Attach(pkg/security/provenance/provenance.go:194) passed cosign's short-form aliasslsaprovenance1tocosign attest --type.https://slsa.dev/provenance/v1.verify-attestation; only matching URI values are guaranteed to round-trip across cosign minor versions. The SBOM path is symmetric and works — provenance was the only asymmetric one.Fix: both call sites now read from a single exported constant
provenance.PredicateTypeSLSAV10, andattestationType()returns the URI form so the DSSE envelope's predicateType is byte-identical to the verifier's match string. The verify call site is factored into averifyProvenanceArgshelper so the symmetry is unit-testable.2. Missing OIDC token in keyless
sc provenance attachsigning.Config{Keyless: true}but never populatedOIDCToken.Attacher.buildSigningEnvnever emittedSIGSTORE_ID_TOKEN.Fix: mirror pkg/cmd/cmd_image/sign.go's pattern — pull the token from
SIGSTORE_ID_TOKENor the GitHub Actions OIDC request endpoint viasecurity.NewExecutionContext, and fail loudly with a specific error message if neither is available.3. Installer
MinVersionnever enforcedToolInstaller.InstallIfMissing(pkg/security/tools/installer.go:42) gated onIsToolAvailable(PATH lookup only).CheckInstalledWithVersionalready existed but was unused.MinVersion = 3.0.2— which is exactly how cosign version drift on the runner started driving SC behavior without anyone noticing.Fix: gate on
CheckInstalledWithVersionso present-but-stale tools trigger re-install of the pinned version. Re-install overwrites viamvand the existing PATH-prepending logic inInstallIfMissingensures the new binary takes precedence.Tests added / updated
TestAttestationType(updated) and newTestAttachAndVerifyUseSamePredicateTypein pkg/security/provenance/provenance_test.go: canonicalise the URI return value and pin the attach/verify type-string contract.TestVerifyProvenanceArgsin pkg/clouds/pulumi/docker/build_and_push_test.go: asserts the verify-attestation args use the same constant as the attacher.TestInstallIfMissingChecksVersionin pkg/security/tools/installer_test.go: registers a fake tool with an unreachable MinVersion and assertsInstallIfMissingno longer silently accepts a present-but-stale binary.Test plan
go test ./pkg/security/... ./pkg/clouds/pulumi/docker/... ./pkg/cmd/...— all green locallygo vetclean on touched packagesgofmtclean on touched filescosign tree <image>shows bothhttps://sigstore.dev/cosign/sign/v1ANDhttps://slsa.dev/provenance/v1attestations.Out-of-band follow-ups (NOT in this PR)
cosign versionin a job step) for incident notes.gh releaseflow to publish a release-note fragment whenever the cosignMinVersionconstant changes, since version drift is the actual trigger here.