From 0aafb0af1839f3e83d413d5bd71363f17bad1724 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 29 Jul 2026 09:47:15 +0200 Subject: [PATCH 1/2] Verify skill signatures at install time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Project-scoped installs now verify artifact signatures before anything is extracted or recorded (RFC THV-0080): OCI artifacts through the Sigstore keyless flow, git commits through gitsign verification, both against the identity recorded in the project's lock file. On first use the observed identity is recorded (trust on first use); later installs enforce it inside the verifier. Verification runs under the per-skill mutex so concurrent first installs cannot race their TOFU anchors. Unsigned artifacts are rejected unless the caller sets allow_unsigned, which records an explicit "unsigned: true" exception in the lock entry; an entry locked to a signer identity refuses unsigned or local-build replacements outright. Lock-driven operations (sync restores, upgrade re-pins) honor the trust state the entry already records — a lock diff converting provenance to unsigned is therefore a reviewable trust downgrade, called out in the code. Verified installs persist the Sigstore bundle with the DB record for offline re-verification during sync. The installers receive install options by pointer so the verification decision reaches the lock-recording step directly, with no parallel result fields to forget. Failures classify to typed reasons via errors.Is on the verifier's sentinels. Part of #5899. Co-Authored-By: Claude Fable 5 --- pkg/skills/options.go | 16 +- pkg/skills/skillsvc/install.go | 19 +- pkg/skills/skillsvc/install_extraction.go | 15 +- pkg/skills/skillsvc/install_git.go | 20 +- pkg/skills/skillsvc/install_oci.go | 18 +- pkg/skills/skillsvc/lock.go | 9 + pkg/skills/skillsvc/lock_test.go | 16 +- pkg/skills/skillsvc/service.go | 11 + pkg/skills/skillsvc/sync.go | 3 + pkg/skills/skillsvc/sync_test.go | 6 +- pkg/skills/skillsvc/verify.go | 293 ++++++++++++++++++++ pkg/skills/skillsvc/verify_test.go | 321 ++++++++++++++++++++++ test/e2e/api_skills_test.go | 49 +++- 13 files changed, 765 insertions(+), 31 deletions(-) create mode 100644 pkg/skills/skillsvc/verify.go create mode 100644 pkg/skills/skillsvc/verify_test.go 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)) From 86f35b106fa7a019af1010ba042089686c540470 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 29 Jul 2026 09:47:16 +0200 Subject: [PATCH 2/2] Wire allow-unsigned through CLI, client, and API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unsigned-install exception must reach the service from every surface: the CLI flag, the HTTP client DTO (without which the flag would silently never reach the server — pinned by a round-trip test), and the API request type, with error messages worded for both surfaces. Co-Authored-By: Claude Fable 5 --- cmd/thv/app/skill_install.go | 26 +++++++++++++++----------- docs/cli/thv_skill_install.md | 1 + docs/server/docs.go | 10 ++++++++++ docs/server/swagger.json | 10 ++++++++++ docs/server/swagger.yaml | 12 ++++++++++++ pkg/api/v1/skills.go | 15 ++++++++------- pkg/api/v1/skills_types.go | 4 ++++ pkg/skills/client/client.go | 15 ++++++++------- pkg/skills/client/client_test.go | 26 ++++++++++++++++++++++++++ pkg/skills/client/dto.go | 3 +++ 10 files changed, 97 insertions(+), 25 deletions(-) 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 {