diff --git a/cmd/thv/app/skill_install.go b/cmd/thv/app/skill_install.go index 57663ec3c2..0bfbc69394 100644 --- a/cmd/thv/app/skill_install.go +++ b/cmd/thv/app/skill_install.go @@ -12,11 +12,12 @@ import ( ) var ( - skillInstallScope string - skillInstallClientsRaw string - skillInstallForce bool - skillInstallProjectRoot string - skillInstallGroup string + skillInstallScope string + skillInstallClientsRaw string + skillInstallForce bool + skillInstallProjectRoot string + skillInstallGroup string + skillInstallAllowUnsigned bool ) var skillInstallCmd = &cobra.Command{ @@ -42,18 +43,21 @@ func init() { skillInstallCmd.Flags().BoolVar(&skillInstallForce, "force", false, "Overwrite existing skill directory") skillInstallCmd.Flags().StringVar(&skillInstallProjectRoot, "project-root", "", "Project root path for project-scoped installs") skillInstallCmd.Flags().StringVar(&skillInstallGroup, "group", "", "Group to add the skill to after installation") + skillInstallCmd.Flags().BoolVar(&skillInstallAllowUnsigned, "allow-unsigned", false, + "Allow installing a project-scoped skill without a verified signature (recorded in the lock file)") } func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { c := newSkillClient(cmd.Context()) _, err := c.Install(cmd.Context(), skills.InstallOptions{ - Name: args[0], - Scope: skills.Scope(skillInstallScope), - Clients: parseSkillInstallClients(skillInstallClientsRaw), - Force: skillInstallForce, - ProjectRoot: skillInstallProjectRoot, - Group: skillInstallGroup, + Name: args[0], + Scope: skills.Scope(skillInstallScope), + Clients: parseSkillInstallClients(skillInstallClientsRaw), + Force: skillInstallForce, + ProjectRoot: skillInstallProjectRoot, + Group: skillInstallGroup, + AllowUnsigned: skillInstallAllowUnsigned, }) if err != nil { return formatSkillError("install skill", err) diff --git a/docs/cli/thv_skill_install.md b/docs/cli/thv_skill_install.md index 84ca857c8b..268829aadc 100644 --- a/docs/cli/thv_skill_install.md +++ b/docs/cli/thv_skill_install.md @@ -25,6 +25,7 @@ thv skill install [skill-name] [flags] ### Options ``` + --allow-unsigned Allow installing a project-scoped skill without a verified signature (recorded in the lock file) --clients string Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client --force Overwrite existing skill directory --group string Group to add the skill to after installation diff --git a/docs/server/docs.go b/docs/server/docs.go index 73626b56b0..7ce641e12d 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1299,6 +1299,9 @@ const docTemplate = `{ "digest-missing", "validation-rejected", "lock-write-failed", + "signature-invalid", + "signer-mismatch", + "unsigned-rejected", "unknown" ], "type": "string", @@ -1307,6 +1310,9 @@ const docTemplate = `{ "FailureReasonDigestMissing", "FailureReasonValidationRejected", "FailureReasonLockWriteFailed", + "FailureReasonSignatureInvalid", + "FailureReasonSignerMismatch", + "FailureReasonUnsignedRejected", "FailureReasonUnknown" ] }, @@ -2754,6 +2760,10 @@ const docTemplate = `{ "pkg_api_v1.installSkillRequest": { "description": "Request to install a skill", "properties": { + "allow_unsigned": { + "description": "AllowUnsigned permits installing a project-scoped skill without a\nverified signature; the exception is recorded in the project's lock\nfile.", + "type": "boolean" + }, "clients": { "description": "Clients lists target client identifiers (e.g., \"claude-code\"),\nor [\"all\"] to target every skill-supporting client.\nOmitting this field installs to all available clients.", "items": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 5138f191df..38f0dc82e7 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1292,6 +1292,9 @@ "digest-missing", "validation-rejected", "lock-write-failed", + "signature-invalid", + "signer-mismatch", + "unsigned-rejected", "unknown" ], "type": "string", @@ -1300,6 +1303,9 @@ "FailureReasonDigestMissing", "FailureReasonValidationRejected", "FailureReasonLockWriteFailed", + "FailureReasonSignatureInvalid", + "FailureReasonSignerMismatch", + "FailureReasonUnsignedRejected", "FailureReasonUnknown" ] }, @@ -2747,6 +2753,10 @@ "pkg_api_v1.installSkillRequest": { "description": "Request to install a skill", "properties": { + "allow_unsigned": { + "description": "AllowUnsigned permits installing a project-scoped skill without a\nverified signature; the exception is recorded in the project's lock\nfile.", + "type": "boolean" + }, "clients": { "description": "Clients lists target client identifiers (e.g., \"claude-code\"),\nor [\"all\"] to target every skill-supporting client.\nOmitting this field installs to all available clients.", "items": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 532253617b..2e90a119c3 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1383,6 +1383,9 @@ components: - digest-missing - validation-rejected - lock-write-failed + - signature-invalid + - signer-mismatch + - unsigned-rejected - unknown type: string x-enum-varnames: @@ -1390,6 +1393,9 @@ components: - FailureReasonDigestMissing - FailureReasonValidationRejected - FailureReasonLockWriteFailed + - FailureReasonSignatureInvalid + - FailureReasonSignerMismatch + - FailureReasonUnsignedRejected - FailureReasonUnknown github_com_stacklok_toolhive_pkg_skills.InstallStatus: description: Status is the current installation status. @@ -2536,6 +2542,12 @@ components: pkg_api_v1.installSkillRequest: description: Request to install a skill properties: + allow_unsigned: + description: |- + AllowUnsigned permits installing a project-scoped skill without a + verified signature; the exception is recorded in the project's lock + file. + type: boolean clients: description: |- Clients lists target client identifiers (e.g., "claude-code"), diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index 4f0105f68c..c0db536f4a 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -113,13 +113,14 @@ func (s *SkillsRoutes) installSkill(w http.ResponseWriter, r *http.Request) erro } result, err := s.skillService.Install(r.Context(), skills.InstallOptions{ - Name: req.Name, - Version: req.Version, - Scope: req.Scope, - ProjectRoot: req.ProjectRoot, - Clients: req.Clients, - Force: req.Force, - Group: req.Group, + Name: req.Name, + Version: req.Version, + Scope: req.Scope, + ProjectRoot: req.ProjectRoot, + Clients: req.Clients, + Force: req.Force, + Group: req.Group, + AllowUnsigned: req.AllowUnsigned, }) if err != nil { return err diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index 072cfb3ed8..65c2356c82 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -31,6 +31,10 @@ type installSkillRequest struct { Clients []string `json:"clients,omitempty"` // Force allows overwriting unmanaged skill directories Force bool `json:"force,omitempty"` + // AllowUnsigned permits installing a project-scoped skill without a + // verified signature; the exception is recorded in the project's lock + // file. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` // Group is the group name to add the skill to after installation Group string `json:"group,omitempty"` } diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index efa83dec9e..6b55191a05 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -164,13 +164,14 @@ func (c *Client) List(ctx context.Context, opts skills.ListOptions) ([]skills.In // Install installs a skill from a remote source. func (c *Client) Install(ctx context.Context, opts skills.InstallOptions) (*skills.InstallResult, error) { body := installRequest{ - Name: opts.Name, - Version: opts.Version, - Scope: opts.Scope, - ProjectRoot: opts.ProjectRoot, - Clients: opts.Clients, - Force: opts.Force, - Group: opts.Group, + Name: opts.Name, + Version: opts.Version, + Scope: opts.Scope, + ProjectRoot: opts.ProjectRoot, + Clients: opts.Clients, + Force: opts.Force, + Group: opts.Group, + AllowUnsigned: opts.AllowUnsigned, } var resp installResponse diff --git a/pkg/skills/client/client_test.go b/pkg/skills/client/client_test.go index db747aed8b..85dcdde295 100644 --- a/pkg/skills/client/client_test.go +++ b/pkg/skills/client/client_test.go @@ -740,3 +740,29 @@ type failReader struct{} func (*failReader) Read([]byte) (int, error) { return 0, errors.New("simulated read error") } + +// TestInstallCarriesAllowUnsigned round-trips the unsigned exception through +// the client's request body — without this, the CLI flag silently never +// reaches the server (every --allow-unsigned install would 403 telling the +// user to pass the flag they passed). +func TestInstallCarriesAllowUnsigned(t *testing.T) { + t.Parallel() + + var got installRequest + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&got)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(installResponse{}) + })) + t.Cleanup(srv.Close) + + _, err := newTestClient(t, srv).Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + Scope: skills.ScopeProject, + ProjectRoot: "/tmp/project", + AllowUnsigned: true, + }) + require.NoError(t, err) + assert.True(t, got.AllowUnsigned, "allow_unsigned must reach the server") +} diff --git a/pkg/skills/client/dto.go b/pkg/skills/client/dto.go index 400f17465a..f880aea099 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -15,6 +15,9 @@ type installRequest struct { Clients []string `json:"clients,omitempty"` Force bool `json:"force,omitempty"` Group string `json:"group,omitempty"` + // AllowUnsigned mirrors skills.InstallOptions.AllowUnsigned; without it + // here the CLI flag would silently never reach the server. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` } type validateRequest struct { diff --git a/pkg/skills/options.go b/pkg/skills/options.go index f861c031b9..c2819bdc17 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -76,6 +76,11 @@ type InstallOptions struct { // normal "same digest means content is already correct" fast path must // not apply. Internal use only — NOT exposed via HTTP API. SyncRestore bool `json:"-"` + // Unsigned records the trust decision that this install proceeded + // without a verified signature (via AllowUnsigned). Set internally by + // install-time verification; recorded as `unsigned: true` in the lock + // entry. + Unsigned bool `json:"-"` // Provenance carries the verified signer identity established during // install-time verification, for recording into the lock entry. Set by // the verification step, nil when the artifact is unsigned or @@ -240,7 +245,16 @@ const ( FailureReasonDigestMissing FailureReason = "digest-missing" FailureReasonValidationRejected FailureReason = "validation-rejected" FailureReasonLockWriteFailed FailureReason = "lock-write-failed" - FailureReasonUnknown FailureReason = "unknown" + // FailureReasonSignatureInvalid means the artifact carries signature + // material that failed cryptographic verification. + FailureReasonSignatureInvalid FailureReason = "signature-invalid" + // FailureReasonSignerMismatch means the artifact verifies, but against + // an identity other than the one recorded in the lock file. + FailureReasonSignerMismatch FailureReason = "signer-mismatch" + // FailureReasonUnsignedRejected means the artifact is unsigned and the + // operation did not permit unsigned installs. + FailureReasonUnsignedRejected FailureReason = "unsigned-rejected" + FailureReasonUnknown FailureReason = "unknown" ) // SyncFailure describes a single skill that failed to sync. diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index 3bbc4dfdda..f0ac760916 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -60,14 +60,14 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski // re-resolution uses, so the two cannot drift. return dispatchSource(ctx, s, opts.Name, sourceOps[*skills.InstallResult]{ git: func(ctx context.Context, _ string) (*skills.InstallResult, error) { - result, err := s.installFromGit(ctx, opts, scope) + result, err := s.installFromGit(ctx, &opts, scope) if err != nil { return nil, err } return s.installAndRegister(ctx, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) }, oci: func(ctx context.Context, ref nameref.Reference) (*skills.InstallResult, error) { - result, err := s.installFromOCI(ctx, opts, scope, ref) + result, err := s.installFromOCI(ctx, &opts, scope, ref) if err != nil { slog.Debug("OCI pull failed, registry fallback may apply", "name", opts.Name, "error", err) return nil, err @@ -124,6 +124,17 @@ func (s *service) installByName( // resolved: opts hydrated, fall through to installWithExtraction } + // Local-store artifacts and raw layer data carry no registry signature + // to verify — installing them project-scoped is an unsigned trust + // decision that must be explicit. + if shouldVerifyInstall(opts, scope) { + decision, verifyErr := verifyLocalInstall(opts, opts.Name) + if verifyErr != nil { + return nil, verifyErr + } + applyDecisionToOpts(&opts, decision) + } + result, err := s.installWithExtraction(ctx, opts, scope) if err != nil { return nil, err @@ -168,7 +179,7 @@ func (s *service) installFromResolvedRegistry( case resolved.OCIRef != nil: slog.Info("resolved skill from registry (OCI)", "name", opts.Name, "oci_reference", resolved.OCIRef.String()) opts.Name = resolved.OCIRef.String() - result, ociErr := s.installFromOCI(ctx, opts, scope, resolved.OCIRef) + result, ociErr := s.installFromOCI(ctx, &opts, scope, resolved.OCIRef) if ociErr != nil { return nil, ociErr } @@ -179,7 +190,7 @@ func (s *service) installFromResolvedRegistry( case resolved.GitURL != "": slog.Info("resolved skill from registry (git)", "name", opts.Name, "git_url", resolved.GitURL) opts.Name = resolved.GitURL - result, gitErr := s.installFromGit(ctx, opts, scope) + result, gitErr := s.installFromGit(ctx, &opts, scope) if gitErr != nil { return nil, gitErr } diff --git a/pkg/skills/skillsvc/install_extraction.go b/pkg/skills/skillsvc/install_extraction.go index 7093b08baa..4824a5d43e 100644 --- a/pkg/skills/skillsvc/install_extraction.go +++ b/pkg/skills/skillsvc/install_extraction.go @@ -220,12 +220,13 @@ func buildInstalledSkill( Name: opts.Name, Version: opts.Version, }, - Scope: scope, - ProjectRoot: opts.ProjectRoot, - Reference: opts.Reference, - Digest: opts.Digest, - Status: skills.InstallStatusInstalled, - InstalledAt: time.Now().UTC(), - Clients: clients, + Scope: scope, + ProjectRoot: opts.ProjectRoot, + Reference: opts.Reference, + Digest: opts.Digest, + Status: skills.InstallStatusInstalled, + InstalledAt: time.Now().UTC(), + Clients: clients, + SigstoreBundle: opts.SigstoreBundle, } } diff --git a/pkg/skills/skillsvc/install_git.go b/pkg/skills/skillsvc/install_git.go index 4c64d56e0c..5ea8e247a5 100644 --- a/pkg/skills/skillsvc/install_git.go +++ b/pkg/skills/skillsvc/install_git.go @@ -22,7 +22,7 @@ import ( // same-commit no-op and upgrade detection. func (s *service) installFromGit( ctx context.Context, - opts skills.InstallOptions, + opts *skills.InstallOptions, scope skills.Scope, ) (*skills.InstallResult, error) { if s.gitResolver == nil { @@ -70,6 +70,7 @@ func (s *service) installFromGit( opts.Name = resolved.SkillConfig.Name opts.Reference = gitURL opts.Digest = resolved.CommitHash + if opts.Version == "" && resolved.SkillConfig.Version != "" { opts.Version = resolved.SkillConfig.Version } @@ -77,12 +78,25 @@ func (s *service) installFromGit( unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() - clientTypes, clientDirs, err := s.resolveAndValidateClients(opts, opts.Name, scope, opts.ProjectRoot) + // Verify the commit signature before anything is written or recorded. + // This runs under the per-skill lock so concurrent first installs + // cannot both read an absent lock entry and race their TOFU anchors. + if shouldVerifyInstall(*opts, scope) { + decision, verifyErr := s.verifyGitInstall( + ctx, *opts, resolved.SkillConfig.Name, resolved.CommitPayload, resolved.CommitSignature, + ) + if verifyErr != nil { + return nil, verifyErr + } + applyDecisionToOpts(opts, decision) + } + + clientTypes, clientDirs, err := s.resolveAndValidateClients(*opts, opts.Name, scope, opts.ProjectRoot) if err != nil { return nil, err } - return s.applyGitInstall(ctx, opts, scope, clientTypes, clientDirs, resolved.Files) + return s.applyGitInstall(ctx, *opts, scope, clientTypes, clientDirs, resolved.Files) } // applyGitInstall handles the create/upgrade/no-op logic for a git-based skill diff --git a/pkg/skills/skillsvc/install_oci.go b/pkg/skills/skillsvc/install_oci.go index f77be955df..b902b18ade 100644 --- a/pkg/skills/skillsvc/install_oci.go +++ b/pkg/skills/skillsvc/install_oci.go @@ -32,7 +32,7 @@ const maxCompressedLayerSize int64 = 50 * 1024 * 1024 // 50 MB // metadata and layer data, then delegates to the standard extraction flow. func (s *service) installFromOCI( ctx context.Context, - opts skills.InstallOptions, + opts *skills.InstallOptions, scope skills.Scope, ref nameref.Reference, ) (*skills.InstallResult, error) { @@ -104,6 +104,7 @@ func (s *service) installFromOCI( opts.LayerData = layerData opts.Reference = ociRef opts.Digest = pulledDigest.String() + if opts.Version == "" && skillConfig.Version != "" { opts.Version = skillConfig.Version } @@ -112,7 +113,20 @@ func (s *service) installFromOCI( unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() - return s.installWithExtraction(ctx, opts, scope) + // Verify the artifact signature before anything is extracted or + // recorded; the decision (verified identity or explicit unsigned + // exception) travels on opts into the DB record and lock entry. This + // runs under the per-skill lock so concurrent first installs cannot + // both read an absent lock entry and race their TOFU anchors. + if shouldVerifyInstall(*opts, scope) { + decision, verifyErr := s.verifyOCIInstall(ctx, *opts, skillConfig.Name, ociRef, opts.Digest) + if verifyErr != nil { + return nil, verifyErr + } + applyDecisionToOpts(opts, decision) + } + + return s.installWithExtraction(ctx, *opts, scope) } // resolveFromLocalStore attempts to resolve a skill name against the local diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go index b589dd4c75..7fe341c3ba 100644 --- a/pkg/skills/skillsvc/lock.go +++ b/pkg/skills/skillsvc/lock.go @@ -51,6 +51,8 @@ func (s *service) recordLockState( ResolvedReference: resolvedReference, Digest: sk.Digest, ContentDigest: contentDigest, + Provenance: provenanceInfoToLock(opts.Provenance), + Unsigned: opts.Unsigned, RequiredByParent: opts.RequiredByParent, PreserveExplicit: opts.SyncRestore, }); err != nil { @@ -150,6 +152,11 @@ type lockEntryInput struct { // transitively materialized dependency. Empty means the entry is // explicit (a direct, user-requested install). RequiredByParent string + // Provenance is the verified signer identity to record, nil for + // unsigned or unverified entries. + Provenance *lockfile.Provenance + // Unsigned records the explicit unsigned-install exception. + Unsigned bool // PreserveExplicit keeps the existing entry's Explicit flag verbatim // instead of deriving it from RequiredByParent. Set by sync restores: a // restore is not a user install, so it must not promote a non-explicit @@ -175,6 +182,8 @@ func recordLockEntry(projectRoot string, in lockEntryInput) error { ResolvedReference: in.ResolvedReference, Digest: in.Digest, ContentDigest: in.ContentDigest, + Provenance: in.Provenance, + Unsigned: in.Unsigned, Explicit: in.RequiredByParent == "", } existing, exists := lf.Get(in.Name) diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index 3b2da5f44e..1ee64abe8c 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -25,6 +25,7 @@ import ( gitmocks "github.com/stacklok/toolhive/pkg/skills/gitresolver/mocks" "github.com/stacklok/toolhive/pkg/skills/lockfile" skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" + verifiermocks "github.com/stacklok/toolhive/pkg/skills/verifier/mocks" "github.com/stacklok/toolhive/pkg/storage/sqlite" ) @@ -32,7 +33,7 @@ import ( // store and the real default Installer, so extracted files and lock file // state can be inspected on disk exactly as they would be in production. // Only the git resolver and path resolver are test doubles. -func newLockTestService(t *testing.T, gr *gitmocks.MockResolver) (skills.SkillService, string) { +func newLockTestService(t *testing.T, gr *gitmocks.MockResolver, extra ...Option) (skills.SkillService, string) { t.Helper() t.Setenv(skills.LockFileEnvVar, "true") @@ -60,7 +61,18 @@ func newLockTestService(t *testing.T, gr *gitmocks.MockResolver) (skills.SkillSe // client" (see TestUpgrade_PreservesExistingClients). pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code", "cursor"}) - svc := New(store, WithPathResolver(pr), WithGitResolver(gr)) + // Default verifier: everything verifies as signed by a fixed test + // identity, so tests exercising lock mechanics don't trip install-time + // verification. Tests about verification pass their own WithVerifier + // via extra (later options win). + mv := verifiermocks.NewMockVerifier(ctrl) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + + opts := append([]Option{WithPathResolver(pr), WithGitResolver(gr), WithVerifier(mv)}, extra...) + svc := New(store, opts...) return svc, projectRoot } diff --git a/pkg/skills/skillsvc/service.go b/pkg/skills/skillsvc/service.go index ef87558eab..656adf4644 100644 --- a/pkg/skills/skillsvc/service.go +++ b/pkg/skills/skillsvc/service.go @@ -12,6 +12,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/skills/verifier" "github.com/stacklok/toolhive/pkg/storage" ) @@ -118,6 +119,16 @@ type service struct { registry ociskills.RegistryClient skillLookup SkillLookup gitResolver gitresolver.Resolver + sigVerifier verifier.Verifier +} + +// WithVerifier sets the signature verifier used for install-time +// verification. Defaults to the Sigstore verifier with the composite +// registry keychain. +func WithVerifier(v verifier.Verifier) Option { + return func(s *service) { + s.sigVerifier = v + } } // New creates a new SkillService backed by the given store. diff --git a/pkg/skills/skillsvc/sync.go b/pkg/skills/skillsvc/sync.go index 3d68c2df0a..e4c0eb4832 100644 --- a/pkg/skills/skillsvc/sync.go +++ b/pkg/skills/skillsvc/sync.go @@ -231,6 +231,9 @@ func classifySyncFailure(err error) skills.FailureReason { if errors.Is(err, errLockWrite) { return skills.FailureReasonLockWriteFailed } + if reason := classifySignatureError(err); reason != "" { + return reason + } switch httperr.Code(err) { case http.StatusNotFound: return skills.FailureReasonDigestMissing diff --git a/pkg/skills/skillsvc/sync_test.go b/pkg/skills/skillsvc/sync_test.go index f479823845..b5f372fc4c 100644 --- a/pkg/skills/skillsvc/sync_test.go +++ b/pkg/skills/skillsvc/sync_test.go @@ -17,6 +17,7 @@ import ( "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/lockfile" skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" + verifiermocks "github.com/stacklok/toolhive/pkg/skills/verifier/mocks" "github.com/stacklok/toolhive/pkg/storage/sqlite" ) @@ -377,7 +378,10 @@ func TestSync_CheckDetectsTamperInAnyClientDir(t *testing.T) { return filepath.Join(projectRoot, "."+client, "skills", skillName), nil }) pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code", "cursor"}) - svc := New(store, WithPathResolver(pr), WithGitResolver(gr)) + mv := verifiermocks.NewMockVerifier(ctrl) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + svc := New(store, WithPathResolver(pr), WithGitResolver(gr), WithVerifier(mv)) ref, _ := gitRef("multi-skill") _, err = svc.Install(t.Context(), skills.InstallOptions{ diff --git a/pkg/skills/skillsvc/verify.go b/pkg/skills/skillsvc/verify.go new file mode 100644 index 0000000000..fca6c6ff42 --- /dev/null +++ b/pkg/skills/skillsvc/verify.go @@ -0,0 +1,293 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/container/images" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/skills/verifier" +) + +// artifactVerifier returns the configured signature verifier, defaulting to +// the Sigstore verifier with the composite registry keychain. +func (s *service) artifactVerifier() verifier.Verifier { + if s.sigVerifier != nil { + return s.sigVerifier + } + return verifier.NewDefault(images.NewCompositeKeychain()) +} + +// shouldVerifyInstall reports whether install-time signature verification +// applies: project-scope installs with the lock file feature enabled. The +// lock file is where trust decisions are recorded, so verification is +// scoped to it. +func shouldVerifyInstall(opts skills.InstallOptions, scope skills.Scope) bool { + return scope == skills.ScopeProject && opts.ProjectRoot != "" && skills.LockFileFeatureEnabled() +} + +// provenanceDecision is the outcome of install-time verification: either a +// verified identity (with the bundle backing it) or an explicit unsigned +// exception. +type provenanceDecision struct { + provenance *skills.ProvenanceInfo + unsigned bool + bundle []byte +} + +// applyDecisionToOpts records the verification outcome on the install +// options, from where it flows into the installed-skill record and the lock +// entry. +func applyDecisionToOpts(opts *skills.InstallOptions, decision *provenanceDecision) { + if decision == nil { + return + } + opts.Provenance = decision.provenance + opts.Unsigned = decision.unsigned + opts.SigstoreBundle = decision.bundle +} + +// verifyOCIInstall verifies the signature of the OCI artifact at ref/digest +// before anything is extracted or recorded. The identity expected by the +// lock file (if any) is enforced inside the verifier's Sigstore policy; +// trust on first use records whatever identity verification observes. +func (s *service) verifyOCIInstall( + ctx context.Context, + opts skills.InstallOptions, + skillName, ref, digest string, +) (*provenanceDecision, error) { + expected, expectUnsigned, err := expectedLockTrust(opts.ProjectRoot, skillName) + if err != nil { + return nil, err + } + if expectUnsigned { + return unsignedLockedDecision(opts, skillName) + } + + result, verifyErr := s.artifactVerifier().VerifyOCI(ctx, ref, digest, expected) + if verifyErr != nil { + if isAllowedUnsigned(verifyErr, opts, expected) { + return &provenanceDecision{unsigned: true}, nil + } + return nil, classifyInstallVerifyError(verifyErr, skillName, expected) + } + return &provenanceDecision{ + provenance: provenanceInfoFromResult(result), + bundle: result.Bundle, + }, nil +} + +// verifyGitInstall verifies the gitsign signature on the resolved commit +// before anything is written or recorded. +func (s *service) verifyGitInstall( + ctx context.Context, + opts skills.InstallOptions, + skillName string, + payload []byte, + signature string, +) (*provenanceDecision, error) { + expected, expectUnsigned, err := expectedLockTrust(opts.ProjectRoot, skillName) + if err != nil { + return nil, err + } + if expectUnsigned { + return unsignedLockedDecision(opts, skillName) + } + + result, verifyErr := s.artifactVerifier().VerifyGit(ctx, payload, []byte(signature), expected) + if verifyErr != nil { + if isAllowedUnsigned(verifyErr, opts, expected) { + return &provenanceDecision{unsigned: true}, nil + } + return nil, classifyInstallVerifyError(verifyErr, skillName, expected) + } + return &provenanceDecision{ + provenance: provenanceInfoFromResult(result), + bundle: result.Bundle, + }, nil +} + +// verifyLocalInstall handles installs sourced from the local OCI store or +// raw layer data: there is no registry signature to verify, so the install +// is an unsigned trust decision. An entry already locked to a signer +// identity refuses a local replacement outright — swapping a verified +// artifact for a local build is exactly the substitution the lock exists to +// catch. +func verifyLocalInstall(opts skills.InstallOptions, skillName string) (*provenanceDecision, error) { + expected, expectUnsigned, err := expectedLockTrust(opts.ProjectRoot, skillName) + if err != nil { + return nil, err + } + if expected != nil { + return nil, httperr.WithCode( + fmt.Errorf("skill %q is locked to signer %q; a local build cannot satisfy it", + skillName, expected.SignerIdentity), + http.StatusForbidden, + ) + } + if expectUnsigned { + return unsignedLockedDecision(opts, skillName) + } + if !opts.AllowUnsigned { + return nil, httperr.WithCode( + fmt.Errorf("local build for %q is unsigned; set allow_unsigned (--allow-unsigned) to record an exception", + skillName), + http.StatusForbidden, + ) + } + return &provenanceDecision{unsigned: true}, nil +} + +// unsignedLockedDecision handles installs of entries the lock file already +// marks unsigned. A lock-driven operation honors the recorded decision (the +// lock file IS the policy it restores); a fresh user-driven install must +// repeat the explicit exception. +// +// SECURITY: this early return means an entry marked unsigned is installed +// without consulting the verifier at all — by design, but it makes a lock +// diff that converts `provenance:` to `unsigned: true` a trust DOWNGRADE +// that sync will honor silently. That conversion is exactly what lock file +// review must catch; it cannot happen without a lock file edit. +func unsignedLockedDecision(opts skills.InstallOptions, skillName string) (*provenanceDecision, error) { + if opts.AllowUnsigned || lockDrivenInstall(opts) { + return &provenanceDecision{unsigned: true}, nil + } + return nil, httperr.WithCode( + fmt.Errorf("skill %q is locked as unsigned; set allow_unsigned (--allow-unsigned) to reinstall it", skillName), + http.StatusForbidden, + ) +} + +// expectedLockTrust reads the trust state recorded in projectRoot's lock +// file for skillName: the expected signer identity (nil on first use — the +// TOFU case), or that the entry was recorded unsigned. +func expectedLockTrust(projectRoot, skillName string) (*lockfile.Provenance, bool, error) { + if projectRoot == "" { + return nil, false, nil + } + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return nil, false, err + } + lf, err := lockfile.Load(root) + if err != nil { + return nil, false, err + } + entry, ok := lf.Get(skillName) + if !ok { + return nil, false, nil + } + if entry.Unsigned { + return nil, true, nil + } + return entry.Provenance, false, nil +} + +// isAllowedUnsigned reports whether a verification failure is the unsigned +// case AND the caller may proceed: either the explicit --allow-unsigned +// exception, or a sync restore of an entry with no recorded trust state +// (entries created before verification existed) — a restore materializes +// what install once accepted, and the outcome is recorded as unsigned so +// the trust state stops being ambiguous. An entry locked to a signer +// identity is never replaceable by an unsigned artifact. +func isAllowedUnsigned(verifyErr error, opts skills.InstallOptions, expected *lockfile.Provenance) bool { + if !errors.Is(verifyErr, verifier.ErrUnsigned) || expected != nil { + return false + } + return opts.AllowUnsigned || lockDrivenInstall(opts) +} + +// lockDrivenInstall reports whether this install materializes an existing +// lock entry rather than making a new trust decision: sync restores and +// upgrade re-pins (both set internal-only options). Such operations honor +// the trust state the entry already records. +func lockDrivenInstall(opts skills.InstallOptions) bool { + return opts.SyncRestore || opts.LockResolvedReference != "" +} + +// classifyInstallVerifyError maps a verifier failure to the HTTP-coded +// error surfaced by the install API — always a 403; the allowed-unsigned +// path is handled before this is called. +func classifyInstallVerifyError( + verifyErr error, + skillName string, + expected *lockfile.Provenance, +) error { + switch { + case errors.Is(verifyErr, verifier.ErrUnsigned): + if expected != nil { + return httperr.WithCode( + fmt.Errorf("skill %q is locked to signer %q but the artifact is unsigned", + skillName, expected.SignerIdentity), + http.StatusForbidden, + ) + } + return httperr.WithCode( + fmt.Errorf("unsigned skill %q rejected; set allow_unsigned (--allow-unsigned) to record an exception", + skillName), + http.StatusForbidden, + ) + case errors.Is(verifyErr, verifier.ErrSignerMismatch): + return httperr.WithCode( + fmt.Errorf("signer identity mismatch for %q: %w"+ + " (if the signer change is intended, remove the skill's lock entry"+ + " and reinstall, or upgrade with allow_signer_change)", skillName, verifyErr), + http.StatusForbidden, + ) + default: + return httperr.WithCode( + fmt.Errorf("signature verification failed for %q: %w", skillName, verifyErr), + http.StatusForbidden, + ) + } +} + +// classifySignatureError maps verifier sentinels to typed failure reasons +// for sync/upgrade results. Returns "" when err is not a signature failure. +func classifySignatureError(err error) skills.FailureReason { + switch { + case errors.Is(err, verifier.ErrSignerMismatch): + return skills.FailureReasonSignerMismatch + case errors.Is(err, verifier.ErrUnsigned): + return skills.FailureReasonUnsignedRejected + case errors.Is(err, verifier.ErrSignatureInvalid): + return skills.FailureReasonSignatureInvalid + default: + return "" + } +} + +// provenanceInfoToLock converts the internal provenance shape to the lock +// file's. +func provenanceInfoToLock(p *skills.ProvenanceInfo) *lockfile.Provenance { + if p == nil { + return nil + } + return &lockfile.Provenance{ + SignerIdentity: p.SignerIdentity, + CertIssuer: p.CertIssuer, + RepositoryURI: p.RepositoryURI, + SigstoreURL: p.SigstoreURL, + } +} + +// provenanceInfoFromResult converts a verification result into the internal +// provenance shape recorded on install options. +func provenanceInfoFromResult(r *verifier.Result) *skills.ProvenanceInfo { + if r == nil || !r.Signed || r.SignerIdentity == "" { + return nil + } + return &skills.ProvenanceInfo{ + SignerIdentity: r.SignerIdentity, + CertIssuer: r.CertIssuer, + RepositoryURI: r.RepositoryURI, + SigstoreURL: r.SigstoreURL, + } +} diff --git a/pkg/skills/skillsvc/verify_test.go b/pkg/skills/skillsvc/verify_test.go new file mode 100644 index 0000000000..8c6e02bce4 --- /dev/null +++ b/pkg/skills/skillsvc/verify_test.go @@ -0,0 +1,321 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/skills/verifier" + verifiermocks "github.com/stacklok/toolhive/pkg/skills/verifier/mocks" +) + +const ( + testSignerIdentity = "/.github/workflows/release.yml" + testCertIssuer = "https://token.actions.githubusercontent.com" +) + +func signedResult() *verifier.Result { + return &verifier.Result{ + Signed: true, + SignerIdentity: testSignerIdentity, + CertIssuer: testCertIssuer, + RepositoryURI: "https://github.com/org/repo", + SigstoreURL: "https://rekor.sigstore.dev", + Bundle: []byte(`{"bundle":true}`), + } +} + +// loadLockEntry reads the lock entry for name from projectRoot. +func loadLockEntry(t *testing.T, projectRoot, name string) (lockfile.Entry, bool) { + t.Helper() + root, err := lockfile.OpenRoot(projectRoot) + require.NoError(t, err) + lf, err := lockfile.Load(root) + require.NoError(t, err) + return lf.Get(name) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallVerification_TOFURecordsProvenance(t *testing.T) { + gr, fixtures := newGitResolverMock(t) + ref, _ := gitRef("signed-skill") + fixtures.register("signed-skill", gitSkill("signed-skill")) + + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + // First install: no lock entry yet — trust on first use, nil expected. + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(signedResult(), nil) + + svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv)) + result, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + entry, ok := loadLockEntry(t, projectRoot, "signed-skill") + require.True(t, ok) + require.NotNil(t, entry.Provenance, "TOFU must record the observed identity") + assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) + assert.Equal(t, testCertIssuer, entry.Provenance.CertIssuer) + assert.False(t, entry.Unsigned) + assert.Equal(t, []byte(`{"bundle":true}`), result.Skill.SigstoreBundle, + "the bundle must be persisted with the install record") + + // Second install: the recorded identity must flow into the verifier as + // the expected identity. + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ any, _, _ []byte, expected *lockfile.Provenance) (*verifier.Result, error) { + require.NotNil(t, expected, "the second install must enforce the recorded identity") + assert.Equal(t, testSignerIdentity, expected.SignerIdentity) + return signedResult(), nil + }) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, + }) + require.NoError(t, err) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallVerification_UnsignedRejectedWithoutFlag(t *testing.T) { + gr, fixtures := newGitResolverMock(t) + ref, _ := gitRef("unsigned-skill") + fixtures.register("unsigned-skill", gitSkill("unsigned-skill")) + + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + + _, ok := loadLockEntry(t, projectRoot, "unsigned-skill") + assert.False(t, ok, "a rejected install must not write a lock entry") + _, err = svc.Info(t.Context(), skills.InfoOptions{ + Name: "unsigned-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "a rejected install must not create a DB record") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallVerification_UnsignedAcceptedWithFlag(t *testing.T) { + gr, fixtures := newGitResolverMock(t) + ref, _ := gitRef("unsigned-ok") + fixtures.register("unsigned-ok", gitSkill("unsigned-ok")) + + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + AllowUnsigned: true, + }) + require.NoError(t, err) + + entry, ok := loadLockEntry(t, projectRoot, "unsigned-ok") + require.True(t, ok) + assert.True(t, entry.Unsigned, "the unsigned exception must be recorded") + assert.Nil(t, entry.Provenance) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallVerification_SignerMismatchRejectedAndLockIntact(t *testing.T) { + gr, fixtures := newGitResolverMock(t) + ref, _ := gitRef("pinned-skill") + fixtures.register("pinned-skill", gitSkill("pinned-skill")) + + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(signedResult(), nil) + + svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + // The re-install is signed by someone else: the verifier reports a + // mismatch (the expected identity was bound into its policy). + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, verifier.ErrSignerMismatch) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, + }) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + + // The prior trusted state is untouched. + entry, ok := loadLockEntry(t, projectRoot, "pinned-skill") + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallVerification_LockedUnsignedRequiresFlagAgain(t *testing.T) { + gr, fixtures := newGitResolverMock(t) + ref, _ := gitRef("unsigned-locked") + fixtures.register("unsigned-locked", gitSkill("unsigned-locked")) + + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + AllowUnsigned: true, + }) + require.NoError(t, err) + + // Reinstall without the flag: the locked unsigned exception does not + // silently renew — the verifier is not even consulted. + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, + }) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallVerification_UserScopeSkipsVerification(t *testing.T) { + gr, fixtures := newGitResolverMock(t) + ref, _ := gitRef("user-skill") + fixtures.register("user-skill", gitSkill("user-skill")) + + // The mock has no expectations: any verifier call fails the test. + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + + svc, _ := newLockTestService(t, gr, WithVerifier(mv)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, + Scope: skills.ScopeUser, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) +} + +func TestVerifyLocalInstall(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + opts skills.InstallOptions + entry *lockfile.Entry + wantErr bool + unsigned bool + }{ + { + name: "no flag rejected", + opts: skills.InstallOptions{}, + wantErr: true, + }, + { + name: "flag records unsigned", + opts: skills.InstallOptions{AllowUnsigned: true}, + unsigned: true, + }, + { + name: "locked identity refuses local replacement even with flag", + opts: skills.InstallOptions{AllowUnsigned: true}, + entry: &lockfile.Entry{ + Name: "local-skill", + Source: "example.com/org/local-skill", + ResolvedReference: "example.com/org/local-skill:v1", + Digest: "sha256:" + strings.Repeat("a", 64), + Provenance: &lockfile.Provenance{ + SignerIdentity: testSignerIdentity, + CertIssuer: testCertIssuer, + }, + }, + wantErr: true, + }, + { + name: "locked unsigned honored with flag", + opts: skills.InstallOptions{AllowUnsigned: true}, + entry: &lockfile.Entry{ + Name: "local-skill", + Source: "example.com/org/local-skill", + ResolvedReference: "example.com/org/local-skill:v1", + Digest: "sha256:" + strings.Repeat("a", 64), + Unsigned: true, + }, + unsigned: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + projectRoot := makeProjectRoot(t) + if tc.entry != nil { + root, err := lockfile.OpenRoot(projectRoot) + require.NoError(t, err) + require.NoError(t, lockfile.Update(root, func(lf *lockfile.Lockfile) error { + lf.Upsert(*tc.entry) + return nil + })) + } + opts := tc.opts + opts.ProjectRoot = projectRoot + + decision, err := verifyLocalInstall(opts, "local-skill") + if tc.wantErr { + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + return + } + require.NoError(t, err) + assert.Equal(t, tc.unsigned, decision.unsigned) + }) + } +} + +func TestClassifySignatureError(t *testing.T) { + t.Parallel() + assert.Equal(t, skills.FailureReasonSignerMismatch, classifySignatureError(verifier.ErrSignerMismatch)) + assert.Equal(t, skills.FailureReasonUnsignedRejected, classifySignatureError(verifier.ErrUnsigned)) + assert.Equal(t, skills.FailureReasonSignatureInvalid, classifySignatureError(verifier.ErrSignatureInvalid)) + assert.Equal(t, skills.FailureReason(""), classifySignatureError(assert.AnError)) +} diff --git a/test/e2e/api_skills_test.go b/test/e2e/api_skills_test.go index 9dc93a52ad..3dbd668dc8 100644 --- a/test/e2e/api_skills_test.go +++ b/test/e2e/api_skills_test.go @@ -49,13 +49,14 @@ type skillMetadataResponse struct { } type installSkillRequest struct { - Name string `json:"name"` - Version string `json:"version,omitempty"` - Scope string `json:"scope,omitempty"` - ProjectRoot string `json:"project_root,omitempty"` - Client string `json:"client,omitempty"` - Force bool `json:"force,omitempty"` - Group string `json:"group,omitempty"` + Name string `json:"name"` + Version string `json:"version,omitempty"` + Scope string `json:"scope,omitempty"` + ProjectRoot string `json:"project_root,omitempty"` + Client string `json:"client,omitempty"` + Force bool `json:"force,omitempty"` + Group string `json:"group,omitempty"` + AllowUnsigned bool `json:"allow_unsigned,omitempty"` } type installSkillResponse struct { @@ -1115,7 +1116,7 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " By("Installing the skill into the project") installResp := installSkill(apiServer, installSkillRequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -1133,12 +1134,38 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " Expect(entry.Digest).ToNot(BeEmpty()) Expect(entry.ContentDigest).ToNot(BeEmpty()) Expect(entry.Explicit).To(BeTrue()) + Expect(entry.Unsigned).To(BeTrue(), "the unsigned exception must be recorded in the lock entry") By("Cleaning up") cleanupResp := uninstallScopedSkill(apiServer, skillName, projectRoot) defer cleanupResp.Body.Close() }) + It("rejects an unsigned project-scoped install without allow_unsigned", func() { + projectRoot := makeE2EProjectRoot() + skillName := "lock-e2e-unsigned-rejected" + + By("Starting an in-process OCI registry and pushing an unsigned test skill") + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "An unsigned skill that must be rejected") + + By("Installing without allow_unsigned and expecting a 403") + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusForbidden)) + + By("Verifying no lock entry was written") + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + lf, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + _, ok := lf.Get(skillName) + Expect(ok).To(BeFalse(), "a rejected install must not write a lock entry") + }) + It("removes the lock entry when the skill is uninstalled", func() { projectRoot := makeE2EProjectRoot() skillName := "lock-e2e-uninstall-skill" @@ -1150,7 +1177,7 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " By("Installing the skill into the project") installResp := installSkill(apiServer, installSkillRequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -1185,7 +1212,7 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " By("Installing the skill into the project") installResp := installSkill(apiServer, installSkillRequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -1234,7 +1261,7 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " By("Installing the skill into the project") installResp := installSkill(apiServer, installSkillRequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated))