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/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/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 diff --git a/pkg/cmd/cmd_provenance/attach.go b/pkg/cmd/cmd_provenance/attach.go index b60223b3..50a4b324 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. " + + "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 + } + + 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/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/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..8523e2cc 100644 --- a/pkg/security/tools/installer.go +++ b/pkg/security/tools/installer.go @@ -7,8 +7,16 @@ import ( "os/exec" "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 @@ -36,10 +44,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 } @@ -64,31 +77,46 @@ 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) - } + pathMu.Lock() + os.Setenv("PATH", prependToPath(installDir, os.Getenv("PATH"))) + pathMu.Unlock() - // 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). 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 == "" { + // 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)) +} + // 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 fa629969..2d9eb7f9 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" @@ -207,6 +208,106 @@ 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 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: "POSIX empty entries (CWD) preserved, not stripped", + installDir: "/usr/local/bin", + currentPath: sep + "/usr/bin" + sep + sep, + want: "/usr/local/bin" + sep + sep + "/usr/bin" + sep + sep, + }, + { + 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", + }, + { + 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 { + 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)