diff --git a/cmd/thv/app/skill_push.go b/cmd/thv/app/skill_push.go index 9edd1c3af3..8607a3db20 100644 --- a/cmd/thv/app/skill_push.go +++ b/cmd/thv/app/skill_push.go @@ -9,6 +9,11 @@ import ( "github.com/stacklok/toolhive/pkg/skills" ) +var ( + skillPushKey string + skillPushNoSign bool +) + var skillPushCmd = &cobra.Command{ Use: "push [reference]", Short: "Push a built skill", @@ -19,6 +24,10 @@ var skillPushCmd = &cobra.Command{ func init() { skillCmd.AddCommand(skillPushCmd) + skillPushCmd.Flags().StringVar(&skillPushKey, "key", "", + "Path to a cosign private key to sign the pushed artifact (COSIGN_PASSWORD decrypts encrypted keys)") + skillPushCmd.Flags().BoolVar(&skillPushNoSign, "no-sign", false, + "Push without signing (consumers will need an explicit unsigned exception to install project-scoped)") } func skillPushCmdFunc(cmd *cobra.Command, args []string) error { @@ -26,6 +35,8 @@ func skillPushCmdFunc(cmd *cobra.Command, args []string) error { err := c.Push(cmd.Context(), skills.PushOptions{ Reference: args[0], + Key: skillPushKey, + NoSign: skillPushNoSign, }) if err != nil { return formatSkillError("push skill", err) diff --git a/cmd/thv/app/skill_sync.go b/cmd/thv/app/skill_sync.go index 1c52f9faf8..a412048fbc 100644 --- a/cmd/thv/app/skill_sync.go +++ b/cmd/thv/app/skill_sync.go @@ -25,12 +25,9 @@ var ( var skillSyncCmd = &cobra.Command{ Use: "sync", - Short: "Restore project skills to match the lock file (experimental)", + Short: "Restore project skills to match the lock file", Long: `Restore a project's installed skills to match toolhive.lock.yaml. -Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive -server while the lock file feature rolls out. - Missing or drifted skills are reinstalled at their pinned digest. Use --check to report drift without installing anything (suitable for CI). Use --adopt to record lock entries for existing unmanaged installs, and diff --git a/cmd/thv/app/skill_upgrade.go b/cmd/thv/app/skill_upgrade.go index 850f439826..6f8bc988e3 100644 --- a/cmd/thv/app/skill_upgrade.go +++ b/cmd/thv/app/skill_upgrade.go @@ -25,12 +25,9 @@ var ( var skillUpgradeCmd = &cobra.Command{ Use: "upgrade [skill-name...]", - Short: "Upgrade project skills to newer pinned content (experimental)", + Short: "Upgrade project skills to newer pinned content", Long: `Re-resolve a project's lock entries and install newer content where available. -Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive -server while the lock file feature rolls out. - Skills pinned to an immutable reference (an OCI digest or a full git commit hash) are reported not-upgradable — there is nothing newer to resolve to. Use --preview to see what would change without persisting anything (OCI diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index 9e6786e4ee..11280dface 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -329,11 +329,15 @@ oci_tags table (reserved; not currently populated) RFC [THV-0080](https://github.com/stacklok/toolhive-rfcs/blob/main/rfcs/THV-0080-skills-lock-file.md) adds a project-level `toolhive.lock.yaml`, committed at the project root, that pins the exact content of every project-scoped skill install — the same guarantee `package-lock.json`, `Cargo.lock`, and `go.sum` provide elsewhere. Two teammates (or a CI runner) cloning the same repo restore identical skill content via `thv skill sync`, rather than whatever the source currently resolves to. -**Trust model, stated plainly:** until the RFC's Sigstore signing/verification stack lands, the lock file provides *reproducibility and drift detection over a repository-editable file* — not verified integrity. A committed digest is an unauthenticated trust root: anyone who can change `toolhive.lock.yaml` in the repository controls what `sync` installs. Reviewing lock-file diffs (especially `digest` and `resolvedReference` changes) carries the same weight as reviewing the AI-executed skill content itself. Signature verification on consume is what upgrades this from drift detection to integrity, and is why the whole feature ships gated until that half exists. +**Trust model, stated plainly:** project-scoped installs are verified against Sigstore signatures, and the lock file records the trust decisions those verifications produce. On first install of a signed skill the observed signer identity is recorded (trust on first use) as the entry's `provenance:` block and **displayed to the user**; every later install, sync, and upgrade enforces that identity *inside* the Sigstore verification policy — OCI artifacts through their attached signature bundles, git commits through gitsign signature-and-chain verification (currently `provisional: true`: the transparency-log proof of signing time is not yet validated, so the replay window is unbounded until that lands). Sync additionally re-verifies each entry's stored signature bundle offline (embedded trust root, no network) before counting it current, and upgrade refuses to move to an artifact signed by a different identity — or unsigned — without an explicit `--allow-signer-change`. -### Rollout +What is still trusted on faith, deliberately and visibly: -The feature is gated behind the `TOOLHIVE_SKILLS_LOCK_ENABLED` environment variable (`skills.LockFileFeatureEnabled()`) while it lands across a stack of PRs, following the existing `TOOLHIVE_DEV` precedent for staged rollouts (`pkg/skills/gitresolver/reference.go`). With the flag unset, project-scope installs behave exactly as they did before this RFC — no lock file is written, no `toolhive.requires` materialization happens, `sync`/`upgrade` refuse with a clear "experimental" error. +- **Unsigned skills** install only with an explicit `--allow-unsigned`, recorded as `unsigned: true` in the lock entry. That entry is a standing exception: lock-driven operations (sync restores, upgrade re-pins) honor it without re-asking. +- **The lock file itself** remains a repository-editable policy document. A diff converting a `provenance:` block to `unsigned: true` is a trust downgrade that sync will honor — it cannot happen without a lock file edit, which is exactly what lock-file review must catch. Reviewing `provenance`, `unsigned`, `digest`, and `resolvedReference` changes carries the same weight as reviewing the AI-executed skill content itself. +- **First use** anchors trust to whatever identity signed the artifact at that moment; verify the printed identity is the publisher you expect. + +Publishing is signed by default: `thv skill push` requires either `--key` (a cosign private key; the signature manifest is attached next to the artifact and the bundle is retrievable at install) or an explicit `--no-sign`. ### Schema diff --git a/docs/cli/thv_skill.md b/docs/cli/thv_skill.md index 2717cddc63..a97336a789 100644 --- a/docs/cli/thv_skill.md +++ b/docs/cli/thv_skill.md @@ -38,8 +38,8 @@ The skill command provides subcommands to manage skills. * [thv skill install](thv_skill_install.md) - Install a skill * [thv skill list](thv_skill_list.md) - List installed skills * [thv skill push](thv_skill_push.md) - Push a built skill -* [thv skill sync](thv_skill_sync.md) - Restore project skills to match the lock file (experimental) +* [thv skill sync](thv_skill_sync.md) - Restore project skills to match the lock file * [thv skill uninstall](thv_skill_uninstall.md) - Uninstall a skill -* [thv skill upgrade](thv_skill_upgrade.md) - Upgrade project skills to newer pinned content (experimental) +* [thv skill upgrade](thv_skill_upgrade.md) - Upgrade project skills to newer pinned content * [thv skill validate](thv_skill_validate.md) - Validate a skill definition diff --git a/docs/cli/thv_skill_push.md b/docs/cli/thv_skill_push.md index b370b1b715..9d712f63a2 100644 --- a/docs/cli/thv_skill_push.md +++ b/docs/cli/thv_skill_push.md @@ -24,7 +24,9 @@ thv skill push [reference] [flags] ### Options ``` - -h, --help help for push + -h, --help help for push + --key string Path to a cosign private key to sign the pushed artifact (COSIGN_PASSWORD decrypts encrypted keys) + --no-sign Push without signing (consumers will need an explicit unsigned exception to install project-scoped) ``` ### Options inherited from parent commands diff --git a/docs/cli/thv_skill_sync.md b/docs/cli/thv_skill_sync.md index 8cf0f6beb1..d2f2f39bfd 100644 --- a/docs/cli/thv_skill_sync.md +++ b/docs/cli/thv_skill_sync.md @@ -11,15 +11,12 @@ mdx: ## thv skill sync -Restore project skills to match the lock file (experimental) +Restore project skills to match the lock file ### Synopsis Restore a project's installed skills to match toolhive.lock.yaml. -Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive -server while the lock file feature rolls out. - Missing or drifted skills are reinstalled at their pinned digest. Use --check to report drift without installing anything (suitable for CI). Use --adopt to record lock entries for existing unmanaged installs, and diff --git a/docs/cli/thv_skill_upgrade.md b/docs/cli/thv_skill_upgrade.md index 42118fbcf3..29d2eea178 100644 --- a/docs/cli/thv_skill_upgrade.md +++ b/docs/cli/thv_skill_upgrade.md @@ -11,15 +11,12 @@ mdx: ## thv skill upgrade -Upgrade project skills to newer pinned content (experimental) +Upgrade project skills to newer pinned content ### Synopsis Re-resolve a project's lock entries and install newer content where available. -Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive -server while the lock file feature rolls out. - Skills pinned to an immutable reference (an OCI digest or a full git commit hash) are reported not-upgradable — there is nothing newer to resolve to. Use --preview to see what would change without persisting anything (OCI diff --git a/docs/server/docs.go b/docs/server/docs.go index 9fe8ee5b15..18ea43a155 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -3377,6 +3377,14 @@ const docTemplate = `{ "pkg_api_v1.pushSkillRequest": { "description": "Request to push a built skill artifact", "properties": { + "key": { + "description": "Key is the path to a cosign private key used to sign the pushed\nartifact", + "type": "string" + }, + "no_sign": { + "description": "NoSign pushes without signing", + "type": "boolean" + }, "reference": { "description": "OCI reference to push", "type": "string" diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 400a66af5f..a85c9af9fa 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -3370,6 +3370,14 @@ "pkg_api_v1.pushSkillRequest": { "description": "Request to push a built skill artifact", "properties": { + "key": { + "description": "Key is the path to a cosign private key used to sign the pushed\nartifact", + "type": "string" + }, + "no_sign": { + "description": "NoSign pushes without signing", + "type": "boolean" + }, "reference": { "description": "OCI reference to push", "type": "string" diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 540e55361c..f87f8a3406 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -3032,6 +3032,14 @@ components: pkg_api_v1.pushSkillRequest: description: Request to push a built skill artifact properties: + key: + description: |- + Key is the path to a cosign private key used to sign the pushed + artifact + type: string + no_sign: + description: NoSign pushes without signing + type: boolean reference: description: OCI reference to push type: string diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index 9b760a0dfb..bc8ca25a9a 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -290,6 +290,8 @@ func (s *SkillsRoutes) pushSkill(w http.ResponseWriter, r *http.Request) error { if err := s.skillService.Push(r.Context(), skills.PushOptions{ Reference: req.Reference, + Key: req.Key, + NoSign: req.NoSign, }); err != nil { return err } diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index 755b06d2d6..ba9b29428e 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -71,6 +71,11 @@ type buildSkillRequest struct { type pushSkillRequest struct { // OCI reference to push Reference string `json:"reference"` + // Key is the path to a cosign private key used to sign the pushed + // artifact + Key string `json:"key,omitempty"` + // NoSign pushes without signing + NoSign bool `json:"no_sign,omitempty"` } // syncSkillsRequest represents the request to sync a project's skills. diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index 038415dd4e..3d94cc4c75 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -240,7 +240,7 @@ func (c *Client) Build(ctx context.Context, opts skills.BuildOptions) (*skills.B // Push pushes a built skill artifact to a remote registry. func (c *Client) Push(ctx context.Context, opts skills.PushOptions) error { - body := pushRequest{Reference: opts.Reference} + body := pushRequest{Reference: opts.Reference, Key: opts.Key, NoSign: opts.NoSign} return c.doJSONRequest(ctx, http.MethodPost, "/push", nil, body, nil) } diff --git a/pkg/skills/client/dto.go b/pkg/skills/client/dto.go index c92dad6417..6e6445103e 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -31,6 +31,8 @@ type buildRequest struct { type pushRequest struct { Reference string `json:"reference"` + Key string `json:"key,omitempty"` + NoSign bool `json:"no_sign,omitempty"` } type listResponse struct { diff --git a/pkg/skills/feature_gate.go b/pkg/skills/feature_gate.go deleted file mode 100644 index 8efff9d00a..0000000000 --- a/pkg/skills/feature_gate.go +++ /dev/null @@ -1,27 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package skills - -import ( - "os" - "strings" -) - -// LockFileEnvVar gates the project-level skills lock file feature (RFC -// THV-0080) while it lands across multiple PRs. The feature is inert on -// main until every PR in the stack — lock file, sync, upgrade, and Sigstore -// signing/verification — has merged; this keeps each PR mergeable on its own -// without exposing partial, unsigned-by-default behavior in between. -// -// This is intentionally a plain env var, not persisted config or a CLI flag: -// it exists only for the duration of the rollout and is expected to be -// removed once the feature ships, matching the existing TOOLHIVE_DEV / -// TOOLHIVE_REMOTE_HEALTHCHECKS precedent for staged/dev-only behavior. -const LockFileEnvVar = "TOOLHIVE_SKILLS_LOCK_ENABLED" - -// LockFileFeatureEnabled reports whether the project-level skills lock file -// feature is enabled for this process. -func LockFileFeatureEnabled() bool { - return strings.EqualFold(os.Getenv(LockFileEnvVar), "true") -} diff --git a/pkg/skills/feature_gate_test.go b/pkg/skills/feature_gate_test.go deleted file mode 100644 index cac0d8e4d9..0000000000 --- a/pkg/skills/feature_gate_test.go +++ /dev/null @@ -1,33 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package skills - -import "testing" - -func TestLockFileFeatureEnabled(t *testing.T) { - tests := []struct { - name string - value string - want bool - }{ - {name: "unset defaults to disabled", value: "", want: false}, - {name: "true enables", value: "true", want: true}, - {name: "mixed case true enables", value: "True", want: true}, - {name: "false stays disabled", value: "false", want: false}, - {name: "arbitrary value stays disabled", value: "1", want: false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.value == "" { - t.Setenv(LockFileEnvVar, "") - } else { - t.Setenv(LockFileEnvVar, tt.value) - } - if got := LockFileFeatureEnabled(); got != tt.want { - t.Errorf("LockFileFeatureEnabled() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/skills/options.go b/pkg/skills/options.go index bc77188f8e..4ee30cc30e 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -230,6 +230,13 @@ type BuildResult struct { type PushOptions struct { // Reference is the OCI reference to push. Reference string `json:"reference"` + // Key is the path to a cosign PEM private key used to sign the pushed + // artifact (COSIGN_PASSWORD decrypts encrypted keys). Empty with + // NoSign false is an error: unsigned pushes must be explicit. + Key string `json:"key,omitempty"` + // NoSign pushes without signing. Consumers installing the artifact + // project-scoped will need an explicit unsigned exception. + NoSign bool `json:"no_sign,omitempty"` } // SyncOptions configures the behavior of the Sync operation. diff --git a/pkg/skills/signer/mocks/mock_signer.go b/pkg/skills/signer/mocks/mock_signer.go new file mode 100644 index 0000000000..3aada3d2cb --- /dev/null +++ b/pkg/skills/signer/mocks/mock_signer.go @@ -0,0 +1,57 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: signer.go +// +// Generated by this command: +// +// mockgen -destination=mocks/mock_signer.go -package=mocks -source=signer.go Signer +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + reflect "reflect" + + signer "github.com/stacklok/toolhive/pkg/skills/signer" + gomock "go.uber.org/mock/gomock" +) + +// MockSigner is a mock of Signer interface. +type MockSigner struct { + ctrl *gomock.Controller + recorder *MockSignerMockRecorder + isgomock struct{} +} + +// MockSignerMockRecorder is the mock recorder for MockSigner. +type MockSignerMockRecorder struct { + mock *MockSigner +} + +// NewMockSigner creates a new mock instance. +func NewMockSigner(ctrl *gomock.Controller) *MockSigner { + mock := &MockSigner{ctrl: ctrl} + mock.recorder = &MockSignerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockSigner) EXPECT() *MockSignerMockRecorder { + return m.recorder +} + +// SignOCI mocks base method. +func (m *MockSigner) SignOCI(ctx context.Context, ref, digest string, opts signer.Options) ([]byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SignOCI", ctx, ref, digest, opts) + ret0, _ := ret[0].([]byte) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// SignOCI indicates an expected call of SignOCI. +func (mr *MockSignerMockRecorder) SignOCI(ctx, ref, digest, opts any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SignOCI", reflect.TypeOf((*MockSigner)(nil).SignOCI), ctx, ref, digest, opts) +} diff --git a/pkg/skills/signer/signer.go b/pkg/skills/signer/signer.go index 245ff69e38..d252f6a3b4 100644 --- a/pkg/skills/signer/signer.go +++ b/pkg/skills/signer/signer.go @@ -19,6 +19,8 @@ import ( "github.com/sigstore/sigstore-go/pkg/sign" ) +//go:generate mockgen -destination=mocks/mock_signer.go -package=mocks -source=signer.go Signer + // ErrKeyRequired indicates no signing key was provided. Keyless (OIDC) // signing is not implemented yet, so a cosign private key is the only // supported signing method. diff --git a/pkg/skills/skillsvc/build.go b/pkg/skills/skillsvc/build.go index eb89da02a1..2839450aeb 100644 --- a/pkg/skills/skillsvc/build.go +++ b/pkg/skills/skillsvc/build.go @@ -16,7 +16,9 @@ import ( "github.com/stacklok/toolhive-core/httperr" ociskills "github.com/stacklok/toolhive-core/oci/skills" + "github.com/stacklok/toolhive/pkg/container/images" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/signer" ) // Validate checks whether a skill definition is valid. @@ -111,6 +113,12 @@ func (s *service) Push(ctx context.Context, opts skills.PushOptions) error { http.StatusBadRequest, ) } + if opts.Key == "" && !opts.NoSign { + return httperr.WithCode( + errors.New("signing key required: set key (--key), or no_sign (--no-sign) to push unsigned"), + http.StatusBadRequest, + ) + } d, err := s.ociStore.Resolve(ctx, opts.Reference) if err != nil { @@ -125,9 +133,28 @@ func (s *service) Push(ctx context.Context, opts skills.PushOptions) error { return fmt.Errorf("pushing to registry: %w", err) } + if opts.NoSign { + return nil + } + // Sign the pushed artifact and attach the signature manifest next to + // it, so project-scoped installs can verify it (RFC THV-0080). + if _, err := s.artifactSigner().SignOCI(ctx, opts.Reference, d.String(), signer.Options{ + Key: opts.Key, + }); err != nil { + return httperr.WithCode(fmt.Errorf("signing pushed artifact: %w", err), http.StatusBadRequest) + } return nil } +// artifactSigner returns the configured signer, defaulting to the Sigstore +// signer with the composite registry keychain. +func (s *service) artifactSigner() signer.Signer { + if s.sigSigner != nil { + return s.sigSigner + } + return signer.NewDefault(images.NewCompositeKeychain()) +} + // ListBuilds returns all locally-built OCI skill artifacts in the local store. // Tags are filtered by the local-build descriptor annotation (set by Build), // so artifacts pulled into the store by install or the content API for diff --git a/pkg/skills/skillsvc/build_test.go b/pkg/skills/skillsvc/build_test.go index ae53da68bd..b7f9e2fbc1 100644 --- a/pkg/skills/skillsvc/build_test.go +++ b/pkg/skills/skillsvc/build_test.go @@ -347,7 +347,7 @@ func TestPush(t *testing.T) { }{ { name: "nil registry returns 500", - opts: skills.PushOptions{Reference: "ghcr.io/test/skill:v1"}, + opts: skills.PushOptions{Reference: "ghcr.io/test/skill:v1", NoSign: true}, setup: func(_ *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store) { return nil, nil }, @@ -365,7 +365,7 @@ func TestPush(t *testing.T) { }, { name: "resolve not found returns 404", - opts: skills.PushOptions{Reference: "nonexistent"}, + opts: skills.PushOptions{Reference: "nonexistent", NoSign: true}, setup: func(ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store) { ociStore, err := ociskills.NewStore(t.TempDir()) require.NoError(t, err) @@ -375,7 +375,7 @@ func TestPush(t *testing.T) { }, { name: "registry push error propagates", - opts: skills.PushOptions{Reference: "my-tag"}, + opts: skills.PushOptions{Reference: "my-tag", NoSign: true}, setup: func(ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store) { ociStore, err := ociskills.NewStore(t.TempDir()) require.NoError(t, err) @@ -393,7 +393,7 @@ func TestPush(t *testing.T) { }, { name: "successful push", - opts: skills.PushOptions{Reference: "my-tag"}, + opts: skills.PushOptions{Reference: "my-tag", NoSign: true}, setup: func(ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store) { ociStore, err := ociskills.NewStore(t.TempDir()) require.NoError(t, err) diff --git a/pkg/skills/skillsvc/build_verify_test.go b/pkg/skills/skillsvc/build_verify_test.go new file mode 100644 index 0000000000..ebc377e662 --- /dev/null +++ b/pkg/skills/skillsvc/build_verify_test.go @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive-core/httperr" + ociskills "github.com/stacklok/toolhive-core/oci/skills" + ocimocks "github.com/stacklok/toolhive-core/oci/skills/mocks" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/signer" + signermocks "github.com/stacklok/toolhive/pkg/skills/signer/mocks" + "github.com/stacklok/toolhive/pkg/storage" +) + +// newPushFixture builds an OCI store with a manifest tagged "my-tag" and a +// registry mock, mirroring TestPush's setup. +func newPushFixture(t *testing.T) (*ocimocks.MockRegistryClient, *ociskills.Store, string) { + t.Helper() + ctrl := gomock.NewController(t) + ociStore, err := ociskills.NewStore(t.TempDir()) + require.NoError(t, err) + d, err := ociStore.PutManifest(t.Context(), []byte(`{"schemaVersion":2}`)) + require.NoError(t, err) + require.NoError(t, ociStore.Tag(t.Context(), d, "my-tag")) + return ocimocks.NewMockRegistryClient(ctrl), ociStore, d.String() +} + +// TestPushRequiresExplicitSigningDecision guards the RFC invariant that +// pushes are signed by default: no key and no explicit no_sign is a 400, +// before anything is pushed. +func TestPushRequiresExplicitSigningDecision(t *testing.T) { + t.Parallel() + reg, ociStore, _ := newPushFixture(t) + svc := New(&storage.NoopSkillStore{}, WithRegistryClient(reg), WithOCIStore(ociStore)) + + err := svc.Push(t.Context(), skills.PushOptions{Reference: "my-tag"}) + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) +} + +// TestPushSignsAfterPushing proves the pushed artifact is signed with the +// provided key, pinned to the digest that was pushed. +func TestPushSignsAfterPushing(t *testing.T) { + t.Parallel() + reg, ociStore, digest := newPushFixture(t) + + ms := signermocks.NewMockSigner(gomock.NewController(t)) + ms.EXPECT().SignOCI(gomock.Any(), "my-tag", digest, signer.Options{Key: "/tmp/cosign.key"}). + Return([]byte(`{"bundle":true}`), nil) + reg.EXPECT().Push(gomock.Any(), gomock.Any(), gomock.Any(), "my-tag").Return(nil) + + svc := New(&storage.NoopSkillStore{}, + WithRegistryClient(reg), WithOCIStore(ociStore), WithSigner(ms)) + err := svc.Push(t.Context(), skills.PushOptions{Reference: "my-tag", Key: "/tmp/cosign.key"}) + require.NoError(t, err) +} + +// TestPushSigningFailurePropagates: a failed signing is a failed push — the +// artifact must not be silently published unsigned. +func TestPushSigningFailurePropagates(t *testing.T) { + t.Parallel() + reg, ociStore, _ := newPushFixture(t) + + ms := signermocks.NewMockSigner(gomock.NewController(t)) + ms.EXPECT().SignOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, signer.ErrKeyRequired) + reg.EXPECT().Push(gomock.Any(), gomock.Any(), gomock.Any(), "my-tag").Return(nil) + + svc := New(&storage.NoopSkillStore{}, + WithRegistryClient(reg), WithOCIStore(ociStore), WithSigner(ms)) + err := svc.Push(t.Context(), skills.PushOptions{Reference: "my-tag", Key: "/bad/key"}) + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) +} diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index e493d9f61c..ae13d126bb 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -217,7 +217,7 @@ func (s *service) registerSkillInGroup(ctx context.Context, groupName string, sk // installAndRegister registers the just-installed skill in the target group // and, for project-scope installs with the lock file feature enabled (see -// skills.LockFileFeatureEnabled), records it — and any toolhive.requires +// records it — and any toolhive.requires // dependencies — in the project's toolhive.lock.yaml. If group registration // or the lock write fails, the DB record and lock entry are rolled back to // their pre-install state: restored when this call updated a pre-existing @@ -233,7 +233,7 @@ func (s *service) installAndRegister( skillName string, scope skills.Scope, ) (*skills.InstallResult, error) { - lockScoped := scope == skills.ScopeProject && skills.LockFileFeatureEnabled() + lockScoped := scope == skills.ScopeProject // Surface the verification decision on the result so callers can show // what trust state this install recorded. result.Provenance = opts.Provenance diff --git a/pkg/skills/skillsvc/list.go b/pkg/skills/skillsvc/list.go index 7d2d8a5290..a52ee22567 100644 --- a/pkg/skills/skillsvc/list.go +++ b/pkg/skills/skillsvc/list.go @@ -83,7 +83,7 @@ func (s *service) Info(ctx context.Context, opts skills.InfoOptions) (*skills.Sk } // Project-scoped, lock-managed skills carry the lock file's recorded // trust state so callers can display what installs are checked against. - if scope == skills.ScopeProject && projectRoot != "" && skills.LockFileFeatureEnabled() { + if scope == skills.ScopeProject && projectRoot != "" { if expected, expectUnsigned, trustErr := expectedLockTrust(projectRoot, opts.Name); trustErr == nil { info.Provenance = provenanceInfoFromLock(expected) info.Unsigned = expectUnsigned diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go index 7fe341c3ba..393470d15c 100644 --- a/pkg/skills/skillsvc/lock.go +++ b/pkg/skills/skillsvc/lock.go @@ -23,7 +23,7 @@ var errLockWrite = errors.New("lock file write failed") // materialized entries for any toolhive.requires dependencies declared in // its SKILL.md. It also marks sk as lock-managed in the store. Callers must // only invoke this for project-scope installs with the lock file feature -// enabled (see skills.LockFileFeatureEnabled) — sk is returned updated so the +// enabled — sk is returned updated so the // caller can reflect the Managed flag back to its own result. func (s *service) recordLockState( ctx context.Context, diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index b859e3fd0c..98a17f1832 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -35,7 +35,6 @@ import ( // Only the git resolver and path resolver are test doubles. func newLockTestService(t *testing.T, gr *gitmocks.MockResolver, extra ...Option) (skills.SkillService, string) { t.Helper() - t.Setenv(skills.LockFileEnvVar, "true") dbPath := filepath.Join(t.TempDir(), "test.db") db, err := sqlite.Open(t.Context(), dbPath) @@ -191,23 +190,6 @@ func readLockfile(t *testing.T, projectRoot string) *lockfile.Lockfile { return lf } -//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestInstallProjectScope_LockFileDisabled_NoLockFileWritten(t *testing.T) { - gr, fx := newGitResolverMock(t) - fx.register("my-skill", gitSkill("my-skill")) - svc, projectRoot := newLockTestService(t, gr) - t.Setenv(skills.LockFileEnvVar, "false") // override newLockTestService's default - - ref, _ := gitRef("my-skill") - _, err := svc.Install(t.Context(), skills.InstallOptions{ - Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, - }) - require.NoError(t, err) - - _, err = os.Stat(filepath.Join(projectRoot, lockfile.FileName)) - assert.True(t, os.IsNotExist(err), "lock file must not be written when the feature is disabled") -} - //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestInstallProjectScope_RecordsExplicitEntry(t *testing.T) { gr, fx := newGitResolverMock(t) diff --git a/pkg/skills/skillsvc/service.go b/pkg/skills/skillsvc/service.go index 656adf4644..ac3609ca65 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/signer" "github.com/stacklok/toolhive/pkg/skills/verifier" "github.com/stacklok/toolhive/pkg/storage" ) @@ -120,6 +121,15 @@ type service struct { skillLookup SkillLookup gitResolver gitresolver.Resolver sigVerifier verifier.Verifier + sigSigner signer.Signer +} + +// WithSigner sets the artifact signer used by Push. Defaults to the +// Sigstore signer with the composite registry keychain. +func WithSigner(sg signer.Signer) Option { + return func(s *service) { + s.sigSigner = sg + } } // WithVerifier sets the signature verifier used for install-time diff --git a/pkg/skills/skillsvc/sync.go b/pkg/skills/skillsvc/sync.go index 4e0b2ce867..e3f6c1c9fa 100644 --- a/pkg/skills/skillsvc/sync.go +++ b/pkg/skills/skillsvc/sync.go @@ -25,9 +25,6 @@ import ( // the lock file are reported (or removed with Prune). Check performs the // same reconciliation read-only: nothing is installed, written, or removed. func (s *service) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) { - if !skills.LockFileFeatureEnabled() { - return nil, errExperimentalLockFeature - } _, projectRoot, err := normalizeProjectRoot(skills.ScopeProject, opts.ProjectRoot) if err != nil { @@ -68,13 +65,6 @@ func (s *service) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.Sy return result, nil } -// errExperimentalLockFeature is returned by Sync/Upgrade while the lock file -// feature is behind its rollout gate (skills.LockFileFeatureEnabled). -var errExperimentalLockFeature = httperr.WithCode( - fmt.Errorf("skills lock file support is experimental; set %s=true to use it", skills.LockFileEnvVar), - http.StatusForbidden, -) - // syncLockedEntry reconciles one lock file entry against installed state, // appending its outcome to result. Missing (dbOK false) and drifted (digest // or contentDigest mismatch) entries are reinstalled at the pinned reference diff --git a/pkg/skills/skillsvc/sync_test.go b/pkg/skills/skillsvc/sync_test.go index c7261a1679..0609c9cf19 100644 --- a/pkg/skills/skillsvc/sync_test.go +++ b/pkg/skills/skillsvc/sync_test.go @@ -4,7 +4,6 @@ package skillsvc import ( - "net/http" "os" "path/filepath" "testing" @@ -13,7 +12,6 @@ import ( "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" skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" @@ -21,17 +19,6 @@ import ( "github.com/stacklok/toolhive/pkg/storage/sqlite" ) -//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestSync_FeatureDisabledReturnsForbidden(t *testing.T) { - gr, _ := newGitResolverMock(t) - svc, projectRoot := newLockTestService(t, gr) - t.Setenv(skills.LockFileEnvVar, "false") - - _, err := svc.(*service).Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert - require.Error(t, err) - assert.Equal(t, http.StatusForbidden, httperr.Code(err)) -} - //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestSync_ReportsUpToDateWhenNothingChanged(t *testing.T) { gr, fx := newGitResolverMock(t) @@ -289,16 +276,26 @@ func TestSync_AdoptsUnmanagedInstall(t *testing.T) { fx.register("unmanaged-skill", gitSkill("unmanaged-skill")) svc, projectRoot := newLockTestService(t, gr) - // Disable the feature for the initial install so it lands unmanaged - // (no lock entry, Managed=false) — simulating a pre-existing install - // from before the lock feature was ever enabled. - t.Setenv(skills.LockFileEnvVar, "false") + // Simulate a pre-existing install from before lock tracking: install + // normally, then strip the lock entry and managed flag — the state a + // legacy install would be in. ref, _ := gitRef("unmanaged-skill") _, err := svc.Install(t.Context(), skills.InstallOptions{ - Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, }) require.NoError(t, err) - t.Setenv(skills.LockFileEnvVar, "true") + root := mustOpenRoot(t, projectRoot) + require.NoError(t, lockfile.Update(root, func(lf *lockfile.Lockfile) error { + lf.Remove("unmanaged-skill") + return nil + })) + syncSvc := svc.(*service) //nolint:forcetypeassert + legacySk, err := syncSvc.store.Get(t.Context(), "unmanaged-skill", skills.ScopeProject, projectRoot) + require.NoError(t, err) + legacySk.Managed = false + legacySk.SigstoreBundle = nil + require.NoError(t, syncSvc.store.Update(t.Context(), legacySk)) syncer := svc.(*service) //nolint:forcetypeassert result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) @@ -366,13 +363,11 @@ func TestSync_PrunesRemovedFromLock(t *testing.T) { // client's copy would leave tampering with any other client's materialized // files invisible to --check — and which directory got checked would depend // on install order. -// -//nolint:paralleltest // uses t.Setenv, incompatible with t.Parallel func TestSync_CheckDetectsTamperInAnyClientDir(t *testing.T) { + t.Parallel() gr, fx := newGitResolverMock(t) fx.register("multi-skill", gitSkill("multi-skill")) - t.Setenv(skills.LockFileEnvVar, "true") dbPath := filepath.Join(t.TempDir(), "test.db") db, err := sqlite.Open(t.Context(), dbPath) require.NoError(t, err) diff --git a/pkg/skills/skillsvc/uninstall.go b/pkg/skills/skillsvc/uninstall.go index 9c52241957..893cfdc092 100644 --- a/pkg/skills/skillsvc/uninstall.go +++ b/pkg/skills/skillsvc/uninstall.go @@ -18,7 +18,7 @@ import ( ) // Uninstall removes an installed skill and cleans up files for all clients. -// For a project-scope, lock-managed skill (see skills.LockFileFeatureEnabled), +// For a project-scope, lock-managed skill, // it also removes the skill's lock entry and cascades to any dependency that // loses its last requiring parent as a result. func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) error { @@ -64,7 +64,7 @@ func (s *service) uninstallOne(ctx context.Context, opts skills.UninstallOptions visited[opts.Name] = struct{}{} var cascadeCandidates []string - if scope == skills.ScopeProject && existing.Managed && skills.LockFileFeatureEnabled() { + if scope == skills.ScopeProject && existing.Managed { cascadeCandidates, err = removeLockEntry(opts) if err != nil { return fmt.Errorf("updating project lock file: %w", err) diff --git a/pkg/skills/skillsvc/upgrade.go b/pkg/skills/skillsvc/upgrade.go index cdda95d6ee..7db7d4a579 100644 --- a/pkg/skills/skillsvc/upgrade.go +++ b/pkg/skills/skillsvc/upgrade.go @@ -29,9 +29,6 @@ var _ skills.SkillLockService = (*service)(nil) // pinned to an immutable reference (an OCI digest or a full git commit hash) // are reported not-upgradable: there is nothing newer to resolve to. func (s *service) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, error) { - if !skills.LockFileFeatureEnabled() { - return nil, errExperimentalLockFeature - } _, projectRoot, err := normalizeProjectRoot(skills.ScopeProject, opts.ProjectRoot) if err != nil { diff --git a/pkg/skills/skillsvc/upgrade_test.go b/pkg/skills/skillsvc/upgrade_test.go index 90f7146624..71f8aef13c 100644 --- a/pkg/skills/skillsvc/upgrade_test.go +++ b/pkg/skills/skillsvc/upgrade_test.go @@ -14,17 +14,6 @@ import ( "github.com/stacklok/toolhive/pkg/skills" ) -//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestUpgrade_FeatureDisabledReturnsForbidden(t *testing.T) { - gr, _ := newGitResolverMock(t) - svc, projectRoot := newLockTestService(t, gr) - t.Setenv(skills.LockFileEnvVar, "false") - - _, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert - require.Error(t, err) - assert.Equal(t, http.StatusForbidden, httperr.Code(err)) -} - //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestUpgrade_ReportsUpToDateWhenSourceUnchanged(t *testing.T) { gr, fx := newGitResolverMock(t) diff --git a/pkg/skills/skillsvc/verify.go b/pkg/skills/skillsvc/verify.go index 7726a1459a..6d9894c670 100644 --- a/pkg/skills/skillsvc/verify.go +++ b/pkg/skills/skillsvc/verify.go @@ -26,11 +26,10 @@ func (s *service) artifactVerifier() verifier.Verifier { } // 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. +// applies: project-scope installs. 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() + return scope == skills.ScopeProject && opts.ProjectRoot != "" } // provenanceDecision is the outcome of install-time verification: either a diff --git a/test/e2e/api_skills_test.go b/test/e2e/api_skills_test.go index 3dbd668dc8..f4bd1ef358 100644 --- a/test/e2e/api_skills_test.go +++ b/test/e2e/api_skills_test.go @@ -204,7 +204,9 @@ func buildAndInstallSkill(server *e2e.Server, skillName, description string) { } func pushSkill(server *e2e.Server, reference string) *http.Response { - reqBody := pushSkillRequest{Reference: reference} + // E2E artifacts are pushed unsigned (no signing infrastructure in the + // suite); project-scoped installs of them pass allow_unsigned. + reqBody := pushSkillRequest{Reference: reference, NoSign: true} jsonData, err := json.Marshal(reqBody) ExpectWithOffset(1, err).ToNot(HaveOccurred()) @@ -219,6 +221,7 @@ func pushSkill(server *e2e.Server, reference string) *http.Response { type pushSkillRequest struct { Reference string `json:"reference"` + NoSign bool `json:"no_sign,omitempty"` } // createUpstreamRegistryWithSkill creates a JSON file in the upstream registry @@ -1089,10 +1092,6 @@ func buildAndPushSkill(server *e2e.Server, ociRegistry *httptest.Server, skillNa return ociRef } -// This RFC THV-0080 feature is gated behind TOOLHIVE_SKILLS_LOCK_ENABLED -// while it lands across a stack of PRs (see skills.LockFileFeatureEnabled), -// so this Describe block runs its own server with the gate turned on rather -// than sharing the "Skills API" block's default-off server above. var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", "api-registry", "skills", "skills-lock", "e2e"), func() { var ( config *e2e.ServerConfig @@ -1101,7 +1100,6 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " BeforeEach(func() { config = e2e.NewServerConfig() - config.ExtraEnv = []string{"TOOLHIVE_SKILLS_LOCK_ENABLED=true"} apiServer = e2e.StartServer(config) }) diff --git a/test/e2e/cli_skills_lock_test.go b/test/e2e/cli_skills_lock_test.go index 66ca0983f2..98ad41cd28 100644 --- a/test/e2e/cli_skills_lock_test.go +++ b/test/e2e/cli_skills_lock_test.go @@ -18,10 +18,6 @@ import ( "github.com/stacklok/toolhive/test/e2e" ) -// This RFC THV-0080 feature is gated behind TOOLHIVE_SKILLS_LOCK_ENABLED -// while it lands across a stack of PRs (see skills.LockFileFeatureEnabled), -// so this Describe block runs its own server with the gate turned on rather -// than sharing the default-off server other CLI skills tests use. var _ = Describe("Skills CLI lock file exit codes (RFC THV-0080)", Label("api", "cli", "skills", "skills-lock", "e2e"), func() { var ( config *e2e.ServerConfig @@ -31,7 +27,6 @@ var _ = Describe("Skills CLI lock file exit codes (RFC THV-0080)", Label("api", BeforeEach(func() { config = e2e.NewServerConfig() - config.ExtraEnv = []string{"TOOLHIVE_SKILLS_LOCK_ENABLED=true"} apiServer = e2e.StartServer(config) thvConfig = e2e.NewTestConfig() })