From 14dfd1858f157b880cd792eaf37827deaf630751 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Fri, 22 May 2026 00:19:51 +0400 Subject: [PATCH 1/4] fix(provenance): align attest/verify predicate type, plumb keyless OIDC token, enforce tool MinVersion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/clouds/pulumi/docker/build_and_push.go | 4 +-- .../pulumi/docker/build_and_push_test.go | 26 +++++++++++++++ pkg/clouds/pulumi/docker/security_helpers.go | 13 ++++++++ pkg/cmd/cmd_provenance/attach.go | 25 +++++++++++++-- pkg/security/provenance/provenance.go | 23 +++++++++++-- pkg/security/provenance/provenance_test.go | 27 ++++++++++++++-- pkg/security/tools/installer.go | 9 ++++-- pkg/security/tools/installer_test.go | 32 +++++++++++++++++++ 8 files changed, 147 insertions(+), 12 deletions(-) diff --git a/pkg/clouds/pulumi/docker/build_and_push.go b/pkg/clouds/pulumi/docker/build_and_push.go index c40b6ab1..22255aa9 100644 --- a/pkg/clouds/pulumi/docker/build_and_push.go +++ b/pkg/clouds/pulumi/docker/build_and_push.go @@ -462,9 +462,7 @@ func createProvenanceCommands(ctx *sdk.Context, security *api.SecurityDescriptor } provVerifyCmd, err := newSecurityCommand(ctx, fmt.Sprintf("verify-provenance-%s", imageName), imageRef, func(img string) []string { - args := []string{"cosign", "verify-attestation", "--type", "https://slsa.dev/provenance/v1"} - args = append(args, verifyIdentityArgs(security.Signing)...) - return append(args, img) + return verifyProvenanceArgs(security.Signing, img) }, "> /dev/null", verifyCommandEnvironment(security.Signing), []sdk.Resource{provCmd}) if err != nil { return nil, errors.Wrapf(err, "failed to create provenance attestation verification for image %q", imageName) diff --git a/pkg/clouds/pulumi/docker/build_and_push_test.go b/pkg/clouds/pulumi/docker/build_and_push_test.go index b4c56670..c862627e 100644 --- a/pkg/clouds/pulumi/docker/build_and_push_test.go +++ b/pkg/clouds/pulumi/docker/build_and_push_test.go @@ -12,6 +12,7 @@ import ( sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/security/provenance" ) func TestCanUseMergedScanCommand(t *testing.T) { @@ -281,6 +282,31 @@ func TestWriteDockerConfigScript(t *testing.T) { Expect(script).To(ContainSubstring(".docker/config.json")) } +func TestVerifyProvenanceArgs(t *testing.T) { + RegisterTestingT(t) + + signing := &api.SigningDescriptor{ + Keyless: true, + Verify: &api.VerifyDescriptor{ + OIDCIssuer: "https://token.actions.githubusercontent.com", + IdentityRegexp: "^https://github.com/integrail/.*$", + }, + } + args := verifyProvenanceArgs(signing, "registry.example.com/img@sha256:"+strings.Repeat("a", 64)) + + Expect(args[0]).To(Equal("cosign")) + Expect(args[1]).To(Equal("verify-attestation")) + Expect(args[2]).To(Equal("--type")) + // Must equal what `Attacher.Attach` passes to `cosign attest --type` so + // the DSSE envelope's predicateType is byte-identical to the verifier's + // match string. Asymmetry here is the root cause this PR fixes. + Expect(args[3]).To(Equal(provenance.PredicateTypeSLSAV10)) + Expect(args[3]).To(Equal("https://slsa.dev/provenance/v1")) + Expect(args[len(args)-1]).To(HavePrefix("registry.example.com/img@sha256:")) + Expect(args).To(ContainElement("--certificate-oidc-issuer")) + Expect(args).To(ContainElement("--certificate-identity-regexp")) +} + func TestVerifyAttestationStdoutRedirect(t *testing.T) { RegisterTestingT(t) diff --git a/pkg/clouds/pulumi/docker/security_helpers.go b/pkg/clouds/pulumi/docker/security_helpers.go index 4c3d2eac..1359d50d 100644 --- a/pkg/clouds/pulumi/docker/security_helpers.go +++ b/pkg/clouds/pulumi/docker/security_helpers.go @@ -10,6 +10,7 @@ import ( sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/security/provenance" ) // --- Shell utilities --- @@ -123,6 +124,18 @@ func signingCLIArgs(cfg *api.SigningDescriptor) []string { return nil } +// verifyProvenanceArgs builds the cosign verify-attestation command for a +// SLSA-v1 provenance attestation. The --type value MUST match what +// `Attacher.Attach` passes to `cosign attest --type` (see +// pkg/security/provenance/attestationType). The two sides used to drift — +// short alias on attest vs. URL on verify — and cosign 3.x's alias rewrite +// turned that drift into a silent attach→verify failure. +func verifyProvenanceArgs(signing *api.SigningDescriptor, img string) []string { + args := []string{"cosign", "verify-attestation", "--type", provenance.PredicateTypeSLSAV10} + args = append(args, verifyIdentityArgs(signing)...) + return append(args, img) +} + // verifyIdentityArgs returns cosign verify/verify-attestation args for identity // checking. For keyless: --certificate-oidc-issuer + --certificate-identity-regexp. // For key-based: --key. diff --git a/pkg/cmd/cmd_provenance/attach.go b/pkg/cmd/cmd_provenance/attach.go index b60223b3..044df203 100644 --- a/pkg/cmd/cmd_provenance/attach.go +++ b/pkg/cmd/cmd_provenance/attach.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/cobra" + "github.com/simple-container-com/api/pkg/security" "github.com/simple-container-com/api/pkg/security/provenance" "github.com/simple-container-com/api/pkg/security/signing" ) @@ -64,12 +65,32 @@ func runAttach(ctx context.Context, opts *attachOptions) error { return fmt.Errorf("cannot specify both --keyless and --key") } - attacher := provenance.NewAttacher(&signing.Config{ + signingCfg := &signing.Config{ Enabled: true, Keyless: useKeyless, PrivateKey: opts.key, Password: opts.password, - }) + } + + // Keyless cosign attest needs an OIDC identity token. Without it cosign 3.x + // can exit 0 while uploading no attestation, leaving the verify step to + // fail with "none of the attestations matched the predicate type". Mirror + // the CLI sign path: pull from SIGSTORE_ID_TOKEN or the GitHub Actions + // OIDC request endpoint, and fail loudly if neither is available. + if useKeyless { + execCtx, err := security.NewExecutionContext(ctx) + if err != nil { + return fmt.Errorf("creating execution context: %w", err) + } + if execCtx.OIDCToken == "" { + return fmt.Errorf("OIDC token not available for keyless provenance attestation. " + + "Ensure you're running in a CI environment with OIDC configured " + + "(id-token: write permission on GitHub Actions), or set SIGSTORE_ID_TOKEN") + } + signingCfg.OIDCToken = execCtx.OIDCToken + } + + attacher := provenance.NewAttacher(signingCfg) if err := attacher.Attach(ctx, statement, opts.image); err != nil { return fmt.Errorf("attaching provenance: %w", err) } diff --git a/pkg/security/provenance/provenance.go b/pkg/security/provenance/provenance.go index ab88ce53..71002c1b 100644 --- a/pkg/security/provenance/provenance.go +++ b/pkg/security/provenance/provenance.go @@ -18,7 +18,20 @@ import ( ) const ( - // Cosign attestation types for supported provenance predicate schemas. + // PredicateTypeSLSAV10 is the canonical in-toto predicateType URI for SLSA v1.0 + // provenance. Pass this to `cosign attest --type` AND `cosign verify-attestation + // --type` so the value written into the DSSE envelope is byte-identical to the + // value the verifier matches against, irrespective of cosign's alias mappings + // (which changed between cosign 2.x and 3.x). + PredicateTypeSLSAV10 = "https://slsa.dev/provenance/v1" + // PredicateTypeSLSAV02 is the canonical predicateType URI for the legacy SLSA + // v0.2 schema. Retained for envelope detection on existing attestations. + PredicateTypeSLSAV02 = "https://slsa.dev/provenance/v0.2" + + // CosignAttestationTypeV10 / CosignAttestationTypeV02 are the cosign short-form + // aliases. Deprecated: prefer the PredicateType* URI constants — cosign 3.x + // dropped some short-form aliases on verify-attestation, and asymmetry between + // attest and verify silently breaks the attach→verify cycle. CosignAttestationTypeV10 = "slsaprovenance1" CosignAttestationTypeV02 = "slsaprovenance02" ) @@ -381,14 +394,18 @@ func matchesExpectedEnvelope(format Format, actualType, actualPredicateType stri } } +// attestationType returns the canonical predicateType URI for the given format. +// Both `cosign attest --type` and `cosign verify-attestation --type` accept URI +// values; using the URI on both sides keeps the envelope predicateType byte- +// identical to the verifier's match string across cosign 2.x and 3.x. func attestationType(format Format) string { switch format { case FormatSLSAV02: - return CosignAttestationTypeV02 + return PredicateTypeSLSAV02 case FormatSLSAV10: fallthrough default: - return CosignAttestationTypeV10 + return PredicateTypeSLSAV10 } } diff --git a/pkg/security/provenance/provenance_test.go b/pkg/security/provenance/provenance_test.go index 52ee27cf..31224614 100644 --- a/pkg/security/provenance/provenance_test.go +++ b/pkg/security/provenance/provenance_test.go @@ -39,8 +39,31 @@ func TestStatementPredicateFromBarePredicate(t *testing.T) { func TestAttestationType(t *testing.T) { RegisterTestingT(t) - Expect(attestationType(FormatSLSAV10)).To(Equal(CosignAttestationTypeV10)) - Expect(attestationType(FormatSLSAV02)).To(Equal(CosignAttestationTypeV02)) + // Canonical URI form so the value `cosign attest --type ...` writes into + // the DSSE envelope's predicateType is byte-identical to what + // `cosign verify-attestation --type ...` matches against. Asymmetry here + // silently breaks the attach→verify cycle (see PR fixing the SLSA-v1 + // verify-provenance regression). + Expect(attestationType(FormatSLSAV10)).To(Equal(PredicateTypeSLSAV10)) + Expect(attestationType(FormatSLSAV02)).To(Equal(PredicateTypeSLSAV02)) + Expect(attestationType(FormatSLSAV10)).To(Equal("https://slsa.dev/provenance/v1")) + Expect(attestationType(FormatSLSAV02)).To(Equal("https://slsa.dev/provenance/v0.2")) +} + +func TestAttachAndVerifyUseSamePredicateType(t *testing.T) { + RegisterTestingT(t) + + // Regression guard: the verify-attestation step in + // pkg/clouds/pulumi/docker/build_and_push.go hard-coded + // "https://slsa.dev/provenance/v1" while Attacher.Attach passed the + // short alias "slsaprovenance1" to `cosign attest --type`. Cosign 3.x + // changed how aliases resolve on the verify side; only matching URIs + // are guaranteed to round-trip. Both sides MUST use the same constant. + attachType := attestationType(FormatSLSAV10) + verifyType := PredicateTypeSLSAV10 + Expect(attachType).To(Equal(verifyType), + "attest --type must equal verify-attestation --type (got attest=%q verify=%q)", + attachType, verifyType) } func TestMatchesExpectedEnvelope(t *testing.T) { diff --git a/pkg/security/tools/installer.go b/pkg/security/tools/installer.go index 749a0061..4e66f7c0 100644 --- a/pkg/security/tools/installer.go +++ b/pkg/security/tools/installer.go @@ -36,10 +36,15 @@ func (i *ToolInstaller) CheckInstalled(ctx context.Context, toolName string) err return nil } -// InstallIfMissing checks if a tool is installed and auto-installs it if not. +// InstallIfMissing checks if a tool is installed at the registered MinVersion +// and auto-installs the pinned version if it is missing OR present-but-stale. +// A bare PATH-only check is unsafe: e.g., a Blacksmith runner shipping cosign +// 2.x would be accepted even though `MinVersion = 3.0.2`, and cosign 3.x +// changed several attestation-related defaults — silent acceptance lets the +// runner-installed binary drive behavior instead of the SC-pinned version. // Supports: cosign, syft, grype, trivy. func (i *ToolInstaller) InstallIfMissing(ctx context.Context, toolName string) error { - if i.IsToolAvailable(ctx, toolName) { + if err := i.CheckInstalledWithVersion(ctx, toolName); err == nil { return nil } diff --git a/pkg/security/tools/installer_test.go b/pkg/security/tools/installer_test.go index fa629969..7ae08cde 100644 --- a/pkg/security/tools/installer_test.go +++ b/pkg/security/tools/installer_test.go @@ -207,6 +207,38 @@ func TestToolRegistryRegisterAndUnregister(t *testing.T) { Expect(registry.HasTool("custom-tool")).To(BeFalse()) } +func TestInstallIfMissingChecksVersion(t *testing.T) { + RegisterTestingT(t) + + installer := NewToolInstaller() + ctx := context.Background() + + // Register a fake tool that resolves to a binary almost certainly already + // on PATH ("sh") but with a MinVersion the GNU coreutils-ish output won't + // satisfy. Prior to the fix, InstallIfMissing returned nil because the + // command was found in PATH — version was never compared. With the fix + // it must reject and attempt install (which then fails because there's + // no install script for "fake-versioned-tool" — exactly the surfacing + // behavior we want). + installer.registry.Register(ToolMetadata{ + Name: "fake-versioned-tool", + Command: "sh", + MinVersion: "999.999.999", + InstallURL: "https://example.com/never-installed", + Description: "version-gate regression guard", + VersionFlag: "--version", + }) + defer installer.registry.Unregister("fake-versioned-tool") + + err := installer.InstallIfMissing(ctx, "fake-versioned-tool") + // We expect a non-nil error: either the version-check rejection + // propagates, or it falls through to the install branch and the + // "unknown tool" install script returns its own error. The point of the + // assertion is that we no longer get a silent nil return. + Expect(err).To(HaveOccurred(), + "InstallIfMissing must NOT silently accept a tool that does not meet MinVersion") +} + func TestInstallScriptVersionValidation(t *testing.T) { RegisterTestingT(t) From 115c0b034fc170097fedd2d228d4d5c4371eb0d2 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Fri, 22 May 2026 00:26:18 +0400 Subject: [PATCH 2/4] =?UTF-8?q?fix:=20address=20codex=20P2s=20=E2=80=94=20?= =?UTF-8?q?local-dev=20SIGSTORE=5FID=5FTOKEN=20+=20installer=20PATH=20prec?= =?UTF-8?q?edence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/security/context.go | 13 +++++-- pkg/security/context_test.go | 58 ++++++++++++++++++++++++++++ pkg/security/tools/installer.go | 42 +++++++++++--------- pkg/security/tools/installer_test.go | 57 +++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 22 deletions(-) create mode 100644 pkg/security/context_test.go diff --git a/pkg/security/context.go b/pkg/security/context.go index b50e673e..16a9d35e 100644 --- a/pkg/security/context.go +++ b/pkg/security/context.go @@ -31,10 +31,15 @@ func NewExecutionContext(ctx context.Context) (*ExecutionContext, error) { execCtx := &ExecutionContext{} execCtx.DetectCI() - if execCtx.IsCI { - if err := execCtx.GetOIDCToken(ctx); err != nil { - // Non-fatal: OIDC token is optional (only needed for keyless signing). - // Log to stderr so callers that need the token get diagnostic info. + // Always attempt OIDC resolution — SIGSTORE_ID_TOKEN may be set by a + // developer running `sc image sign --keyless` or `sc provenance attach + // --keyless` locally (outside any detected CI provider). GetOIDCToken + // checks the env var first, then falls back to the GitHub-Actions + // request endpoint when applicable. A failure here is non-fatal: + // OIDC is only needed for keyless flows, and those will return their + // own clearer error when execCtx.OIDCToken is empty. + if err := execCtx.GetOIDCToken(ctx); err != nil { + if execCtx.IsCI { fmt.Fprintf(os.Stderr, "OIDC token not acquired: %v\n", err) } } diff --git a/pkg/security/context_test.go b/pkg/security/context_test.go new file mode 100644 index 00000000..e4f52739 --- /dev/null +++ b/pkg/security/context_test.go @@ -0,0 +1,58 @@ +package security + +import ( + "context" + "testing" + + . "github.com/onsi/gomega" +) + +func TestNewExecutionContextHonoursSIGSTOREIDTokenOutsideCI(t *testing.T) { + RegisterTestingT(t) + + // Codex review caught a regression in keyless attach / sign: when a + // developer runs `sc provenance attach --keyless` locally with + // SIGSTORE_ID_TOKEN already exported, NewExecutionContext used to skip + // OIDC resolution entirely because DetectCI returned false. The CLI + // then surfaced "OIDC token not available" even though the env var was + // set, blocking local keyless workflows. NewExecutionContext now + // always tries GetOIDCToken (which checks SIGSTORE_ID_TOKEN first), + // regardless of IsCI. + clearCIEnv(t) + t.Setenv("SIGSTORE_ID_TOKEN", "test-token-not-real") + + execCtx, err := NewExecutionContext(context.Background()) + Expect(err).ToNot(HaveOccurred()) + Expect(execCtx.IsCI).To(BeFalse(), "test setup: must not be detected as CI") + Expect(execCtx.OIDCToken).To(Equal("test-token-not-real"), + "NewExecutionContext must surface SIGSTORE_ID_TOKEN even outside CI") +} + +func TestNewExecutionContextNoTokenOutsideCIIsNonFatal(t *testing.T) { + RegisterTestingT(t) + + // Sanity guard: with neither SIGSTORE_ID_TOKEN nor a CI provider, the + // constructor must still return successfully — keyless callers raise + // their own clearer error from the empty OIDCToken. + clearCIEnv(t) + t.Setenv("SIGSTORE_ID_TOKEN", "") + + execCtx, err := NewExecutionContext(context.Background()) + Expect(err).ToNot(HaveOccurred()) + Expect(execCtx.OIDCToken).To(BeEmpty()) +} + +func clearCIEnv(t *testing.T) { + t.Helper() + // t.Setenv("", "") would panic; explicitly unset each var so detection + // falls through to the local-dev branch regardless of the surrounding + // shell (e.g., running this test from within a GitHub Actions runner). + for _, key := range []string{ + "GITHUB_ACTIONS", + "GITLAB_CI", + "ACTIONS_ID_TOKEN_REQUEST_URL", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN", + } { + t.Setenv(key, "") + } +} diff --git a/pkg/security/tools/installer.go b/pkg/security/tools/installer.go index 4e66f7c0..b537ca6b 100644 --- a/pkg/security/tools/installer.go +++ b/pkg/security/tools/installer.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "regexp" + "strings" ) // ToolInstaller checks tool availability and auto-installs missing tools. @@ -69,31 +70,36 @@ func (i *ToolInstaller) InstallIfMissing(ctx context.Context, toolName string) e return fmt.Errorf("auto-install of %s failed: %w — install manually from %s", toolName, err, tool.InstallURL) } - // Ensure the install directory is in PATH for this process and subprocesses. - // Inside Docker containers, ~/.local/bin may not be in the default PATH. - // Use filepath.SplitList + exact match to avoid substring false positives - // (e.g., "/usr/local/binutils" should not match "/usr/local/bin"). - currentPath := os.Getenv("PATH") - found := false - for _, dir := range filepath.SplitList(currentPath) { - if dir == installDir { - found = true - break - } - } - if !found { - os.Setenv("PATH", installDir+":"+currentPath) - } + os.Setenv("PATH", prependToPath(installDir, os.Getenv("PATH"))) - // Verify installation succeeded - if err := i.CheckInstalled(ctx, toolName); err != nil { - return fmt.Errorf("%s installed but not found in PATH — check %s", toolName, installDir) + // Verify installation succeeded AND meets the pinned MinVersion — a + // bare presence check would silently accept a stale binary that still + // shadows the install (e.g., kernel/glibc mismatch leaves the new + // download non-executable and PATH falls back to the old one). + if err := i.CheckInstalledWithVersion(ctx, toolName); err != nil { + return fmt.Errorf("%s install verification failed: %w — check %s", toolName, err, installDir) } fmt.Fprintf(os.Stderr, "Tool %s installed successfully\n", toolName) return nil } +// prependToPath returns currentPath with installDir moved to the front, +// removing any prior occurrence so the freshly-installed binary always wins +// over a stale copy earlier in PATH (e.g., system-package cosign 2.x at +// /usr/bin/cosign vs. our pinned 3.x at ~/.local/bin/cosign). Empty path +// entries are dropped. Uses filepath.SplitList + exact match so +// "/usr/local/binutils" does NOT match "/usr/local/bin". +func prependToPath(installDir, currentPath string) string { + parts := []string{installDir} + for _, dir := range filepath.SplitList(currentPath) { + if dir != installDir && dir != "" { + parts = append(parts, dir) + } + } + return strings.Join(parts, string(os.PathListSeparator)) +} + // resolveInstallDir returns a writable bin directory. // Prefers /usr/local/bin when writable (root or writable dir), falls back to ~/.local/bin. func resolveInstallDir() string { diff --git a/pkg/security/tools/installer_test.go b/pkg/security/tools/installer_test.go index 7ae08cde..165d14b2 100644 --- a/pkg/security/tools/installer_test.go +++ b/pkg/security/tools/installer_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "os" "testing" . "github.com/onsi/gomega" @@ -239,6 +240,62 @@ func TestInstallIfMissingChecksVersion(t *testing.T) { "InstallIfMissing must NOT silently accept a tool that does not meet MinVersion") } +func TestPrependToPath(t *testing.T) { + RegisterTestingT(t) + + // Codex review caught that the prior "append installDir if missing" + // logic left stale binaries winning when installDir was already on + // PATH after the stale location. After the fix, installDir must + // always end up at the FRONT of PATH, with any prior occurrence + // removed (not duplicated). + sep := string(os.PathListSeparator) + + tests := []struct { + name string + installDir string + currentPath string + want string + }{ + { + name: "installDir not on PATH — prepended", + installDir: "/home/u/.local/bin", + currentPath: "/usr/local/bin" + sep + "/usr/bin", + want: "/home/u/.local/bin" + sep + "/usr/local/bin" + sep + "/usr/bin", + }, + { + name: "installDir already last — moved to front (regression guard)", + installDir: "/home/u/.local/bin", + currentPath: "/usr/bin" + sep + "/home/u/.local/bin", + want: "/home/u/.local/bin" + sep + "/usr/bin", + }, + { + name: "installDir already first — kept first, no duplication", + installDir: "/usr/local/bin", + currentPath: "/usr/local/bin" + sep + "/usr/bin", + want: "/usr/local/bin" + sep + "/usr/bin", + }, + { + name: "empty entries dropped", + installDir: "/usr/local/bin", + currentPath: sep + "/usr/bin" + sep + sep, + want: "/usr/local/bin" + sep + "/usr/bin", + }, + { + name: "substring lookalike NOT collapsed (/usr/local/binutils preserved)", + installDir: "/usr/local/bin", + currentPath: "/usr/local/binutils" + sep + "/usr/local/bin", + want: "/usr/local/bin" + sep + "/usr/local/binutils", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + Expect(prependToPath(tt.installDir, tt.currentPath)).To(Equal(tt.want)) + }) + } +} + func TestInstallScriptVersionValidation(t *testing.T) { RegisterTestingT(t) From dfdee3c814879c0553e983c3ad93a6a822da40b5 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Fri, 22 May 2026 08:57:19 +0400 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20address=20gemini=20findings=20?= =?UTF-8?q?=E2=80=94=20PATH=20mutation=20race=20+=20dedup=20edge=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/cmd/cmd_provenance/attach.go | 4 ++-- pkg/security/tools/installer.go | 25 +++++++++++++++++++++---- pkg/security/tools/installer_test.go | 16 ++++++++++++++-- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/cmd_provenance/attach.go b/pkg/cmd/cmd_provenance/attach.go index 044df203..50a4b324 100644 --- a/pkg/cmd/cmd_provenance/attach.go +++ b/pkg/cmd/cmd_provenance/attach.go @@ -84,8 +84,8 @@ func runAttach(ctx context.Context, opts *attachOptions) error { } if execCtx.OIDCToken == "" { return fmt.Errorf("OIDC token not available for keyless provenance attestation. " + - "Ensure you're running in a CI environment with OIDC configured " + - "(id-token: write permission on GitHub Actions), or set SIGSTORE_ID_TOKEN") + "Set SIGSTORE_ID_TOKEN, or run from a CI provider that supplies one " + + "(e.g. GitHub Actions with `permissions: id-token: write`)") } signingCfg.OIDCToken = execCtx.OIDCToken } diff --git a/pkg/security/tools/installer.go b/pkg/security/tools/installer.go index b537ca6b..8523e2cc 100644 --- a/pkg/security/tools/installer.go +++ b/pkg/security/tools/installer.go @@ -8,8 +8,15 @@ import ( "path/filepath" "regexp" "strings" + "sync" ) +// pathMu serializes PATH mutation in InstallIfMissing. os.Setenv is +// process-global, so concurrent installer calls (e.g., parallel tool +// pre-flight) would otherwise race on the read-modify-write of $PATH and +// silently drop newly-installed install dirs. +var pathMu sync.Mutex + // ToolInstaller checks tool availability and auto-installs missing tools. type ToolInstaller struct { registry *ToolRegistry @@ -70,7 +77,9 @@ func (i *ToolInstaller) InstallIfMissing(ctx context.Context, toolName string) e return fmt.Errorf("auto-install of %s failed: %w — install manually from %s", toolName, err, tool.InstallURL) } + pathMu.Lock() os.Setenv("PATH", prependToPath(installDir, os.Getenv("PATH"))) + pathMu.Unlock() // Verify installation succeeded AND meets the pinned MinVersion — a // bare presence check would silently accept a stale binary that still @@ -87,15 +96,23 @@ func (i *ToolInstaller) InstallIfMissing(ctx context.Context, toolName string) e // prependToPath returns currentPath with installDir moved to the front, // removing any prior occurrence so the freshly-installed binary always wins // over a stale copy earlier in PATH (e.g., system-package cosign 2.x at -// /usr/bin/cosign vs. our pinned 3.x at ~/.local/bin/cosign). Empty path -// entries are dropped. Uses filepath.SplitList + exact match so -// "/usr/local/binutils" does NOT match "/usr/local/bin". +// /usr/bin/cosign vs. our pinned 3.x at ~/.local/bin/cosign). Comparison is +// done on filepath.Clean'd values so a trailing slash on an existing entry +// (e.g., "/usr/local/bin/") doesn't defeat dedup. POSIX-meaningful empty +// entries (which denote "current directory") are preserved, not stripped. func prependToPath(installDir, currentPath string) string { + target := filepath.Clean(installDir) parts := []string{installDir} for _, dir := range filepath.SplitList(currentPath) { - if dir != installDir && dir != "" { + if dir == "" { + // Empty entry == CWD in POSIX PATH semantics; preserve. parts = append(parts, dir) + continue + } + if filepath.Clean(dir) == target { + continue } + parts = append(parts, dir) } return strings.Join(parts, string(os.PathListSeparator)) } diff --git a/pkg/security/tools/installer_test.go b/pkg/security/tools/installer_test.go index 165d14b2..2d9eb7f9 100644 --- a/pkg/security/tools/installer_test.go +++ b/pkg/security/tools/installer_test.go @@ -275,10 +275,10 @@ func TestPrependToPath(t *testing.T) { want: "/usr/local/bin" + sep + "/usr/bin", }, { - name: "empty entries dropped", + name: "POSIX empty entries (CWD) preserved, not stripped", installDir: "/usr/local/bin", currentPath: sep + "/usr/bin" + sep + sep, - want: "/usr/local/bin" + sep + "/usr/bin", + want: "/usr/local/bin" + sep + sep + "/usr/bin" + sep + sep, }, { name: "substring lookalike NOT collapsed (/usr/local/binutils preserved)", @@ -286,6 +286,18 @@ func TestPrependToPath(t *testing.T) { currentPath: "/usr/local/binutils" + sep + "/usr/local/bin", want: "/usr/local/bin" + sep + "/usr/local/binutils", }, + { + name: "trailing slash on existing PATH entry still deduped", + installDir: "/usr/local/bin", + currentPath: "/usr/bin" + sep + "/usr/local/bin/", + want: "/usr/local/bin" + sep + "/usr/bin", + }, + { + name: "trailing slash on installDir still deduped", + installDir: "/usr/local/bin/", + currentPath: "/usr/bin" + sep + "/usr/local/bin", + want: "/usr/local/bin/" + sep + "/usr/bin", + }, } for _, tt := range tests { From c82e2b4c12c2b813acdc847737dc25bb6e2e5c9b Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Fri, 22 May 2026 14:20:11 +0400 Subject: [PATCH 4/4] =?UTF-8?q?chore(deps):=20bump=20golang.org/x/crypto?= =?UTF-8?q?=20v0.51.0=20=E2=86=92=20v0.52.0=20(govulncheck)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- go.mod | 6 +++--- go.sum | 8 ++++---- pkg/api/git/mocks/git_mock.go | 2 +- pkg/clouds/pulumi/mocks/pulumi_mock.go | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 8c81bab1..842a6530 100644 --- a/go.mod +++ b/go.mod @@ -55,7 +55,8 @@ require ( github.com/vektra/mockery/v2 v2.53.6 go.mongodb.org/mongo-driver v1.17.9 go.uber.org/atomic v1.11.0 - golang.org/x/crypto v0.51.0 + gocloud.dev v0.37.0 + golang.org/x/crypto v0.52.0 golang.org/x/oauth2 v0.36.0 golang.org/x/sync v0.20.0 golang.org/x/term v0.43.0 @@ -464,14 +465,13 @@ require ( go.uber.org/zap v1.27.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect - gocloud.dev v0.37.0 // indirect gocloud.dev/secrets/hashivault v0.37.0 // indirect golang.org/x/arch v0.11.0 // indirect golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f // indirect golang.org/x/exp/typeparams v0.0.0-20250210185358-939b2ce775ac // indirect golang.org/x/mod v0.35.0 // indirect golang.org/x/net v0.54.0 // indirect - golang.org/x/sys v0.44.0 // indirect + golang.org/x/sys v0.45.0 // indirect golang.org/x/telemetry v0.0.0-20260409153401-be6f6cb8b1fa // indirect golang.org/x/time v0.15.0 // indirect golang.org/x/tools v0.44.0 // indirect diff --git a/go.sum b/go.sum index c6271783..22b844c4 100644 --- a/go.sum +++ b/go.sum @@ -1117,8 +1117,8 @@ golang.org/x/crypto v0.13.0/go.mod h1:y6Z2r+Rw4iayiXXAIxJIDAJ1zMW4yaTpebo8fPOliY golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4= golang.org/x/crypto v0.18.0/go.mod h1:R0j02AL6hcrfOiy9T4ZYp/rcWeMxM3L6QYxlOuEG1mg= golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= -golang.org/x/crypto v0.51.0 h1:IBPXwPfKxY7cWQZ38ZCIRPI50YLeevDLlLnyC5wRGTI= -golang.org/x/crypto v0.51.0/go.mod h1:8AdwkbraGNABw2kOX6YFPs3WM22XqI4EXEd8g+x7Oc8= +golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= +golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f h1:W3F4c+6OLc6H2lb//N1q4WpJkhzJCK5J6kUi1NTVXfM= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f/go.mod h1:J1xhfL/vlindoeF/aINzNzt2Bket5bjo9sdOYzOsU80= @@ -1220,8 +1220,8 @@ golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= -golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= +golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/telemetry v0.0.0-20260409153401-be6f6cb8b1fa h1:efT73AJZfAAUV7SOip6pWGkwJDzIGiKBZGVzHYa+ve4= golang.org/x/telemetry v0.0.0-20260409153401-be6f6cb8b1fa/go.mod h1:kHjTxDEnAu6/Nl9lDkzjWpR+bmKfxeiRuSDlsMb70gE= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= diff --git a/pkg/api/git/mocks/git_mock.go b/pkg/api/git/mocks/git_mock.go index 8420a743..73abb936 100644 --- a/pkg/api/git/mocks/git_mock.go +++ b/pkg/api/git/mocks/git_mock.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.53.4. DO NOT EDIT. +// Code generated by mockery v2.53.6. DO NOT EDIT. package git_mocks diff --git a/pkg/clouds/pulumi/mocks/pulumi_mock.go b/pkg/clouds/pulumi/mocks/pulumi_mock.go index d7330078..d2fd143e 100644 --- a/pkg/clouds/pulumi/mocks/pulumi_mock.go +++ b/pkg/clouds/pulumi/mocks/pulumi_mock.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.52.2. DO NOT EDIT. +// Code generated by mockery v2.53.6. DO NOT EDIT. package pulumi_mocks