diff --git a/docs/server/docs.go b/docs/server/docs.go index b9f55820b4..783de24b11 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -3516,6 +3516,10 @@ const docTemplate = `{ "pkg_api_v1.pushSkillRequest": { "description": "Request to push a built skill artifact", "properties": { + "identity_token": { + "description": "IdentityToken is a short-lived OIDC identity token used for keyless\nsigning, mutually exclusive with Key", + "type": "string" + }, "key": { "description": "Key is the path to a cosign private key used to sign the pushed\nartifact", "type": "string" diff --git a/docs/server/swagger.json b/docs/server/swagger.json index ee9a53ea29..ba528af39e 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -3509,6 +3509,10 @@ "pkg_api_v1.pushSkillRequest": { "description": "Request to push a built skill artifact", "properties": { + "identity_token": { + "description": "IdentityToken is a short-lived OIDC identity token used for keyless\nsigning, mutually exclusive with Key", + "type": "string" + }, "key": { "description": "Key is the path to a cosign private key used to sign the pushed\nartifact", "type": "string" diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 1d420d016f..926df5364f 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -3284,6 +3284,11 @@ components: pkg_api_v1.pushSkillRequest: description: Request to push a built skill artifact properties: + identity_token: + description: |- + IdentityToken is a short-lived OIDC identity token used for keyless + signing, mutually exclusive with Key + type: string key: description: |- Key is the path to a cosign private key used to sign the pushed diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index e6e67c1ba5..1e316aec70 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -301,9 +301,10 @@ 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, + Reference: req.Reference, + Key: req.Key, + IdentityToken: req.IdentityToken, + NoSign: req.NoSign, }); err != nil { return err } diff --git a/pkg/api/v1/skills_test.go b/pkg/api/v1/skills_test.go index 5ad7599923..c6b9a31732 100644 --- a/pkg/api/v1/skills_test.go +++ b/pkg/api/v1/skills_test.go @@ -488,6 +488,21 @@ func TestSkillsRouter(t *testing.T) { }, expectedStatus: http.StatusNoContent, }, + { + // Guards the DTO trap: identity_token must reach PushOptions, not + // just decode into the request struct and get dropped. + name: "push skill forwards identity token", + method: "POST", + path: "/push", + body: `{"reference":"ghcr.io/test/skill:v1","identity_token":"a.b.c"}`, + setupMock: func(svc *skillsmocks.MockSkillService, _ string) { + svc.EXPECT().Push(gomock.Any(), skills.PushOptions{ + Reference: "ghcr.io/test/skill:v1", + IdentityToken: "a.b.c", + }).Return(nil) + }, + expectedStatus: http.StatusNoContent, + }, { name: "push skill bad request", method: "POST", diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index 8150645b67..71bd6e254f 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -80,6 +80,9 @@ type pushSkillRequest struct { // Key is the path to a cosign private key used to sign the pushed // artifact Key string `json:"key,omitempty"` + // IdentityToken is a short-lived OIDC identity token used for keyless + // signing, mutually exclusive with Key + IdentityToken string `json:"identity_token,omitempty"` // NoSign pushes without signing NoSign bool `json:"no_sign,omitempty"` } diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index cd3c5eab64..909d26b5d8 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -284,7 +284,12 @@ 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, Key: opts.Key, NoSign: opts.NoSign} + body := pushRequest{ + Reference: opts.Reference, + Key: opts.Key, + IdentityToken: opts.IdentityToken, + NoSign: opts.NoSign, + } return c.doJSONRequest(ctx, http.MethodPost, "/push", nil, body, nil) } diff --git a/pkg/skills/client/client_test.go b/pkg/skills/client/client_test.go index bb4cde2aab..9a9ee6ee30 100644 --- a/pkg/skills/client/client_test.go +++ b/pkg/skills/client/client_test.go @@ -519,6 +519,20 @@ func TestPush(t *testing.T) { wantBody: pushRequest{Reference: "ghcr.io/org/my-skill:v1.0.0"}, statusCode: http.StatusNoContent, }, + { + // Guards the DTO trap: identity_token must reach the wire request, + // not just PushOptions. + name: "forwards identity token", + opts: skills.PushOptions{ + Reference: "ghcr.io/org/my-skill:v1.0.0", + IdentityToken: "a.b.c", + }, + wantBody: pushRequest{ + Reference: "ghcr.io/org/my-skill:v1.0.0", + IdentityToken: "a.b.c", + }, + statusCode: http.StatusNoContent, + }, { name: "not found", opts: skills.PushOptions{Reference: "ghcr.io/org/missing:v1"}, diff --git a/pkg/skills/client/dto.go b/pkg/skills/client/dto.go index 333ab3b6f8..62f39a4fec 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -32,7 +32,10 @@ type buildRequest struct { type pushRequest struct { Reference string `json:"reference"` Key string `json:"key,omitempty"` - NoSign bool `json:"no_sign,omitempty"` + // IdentityToken mirrors skills.PushOptions.IdentityToken; without it here + // the --identity-token flag would silently never reach the server. + IdentityToken string `json:"identity_token,omitempty"` + NoSign bool `json:"no_sign,omitempty"` } type listResponse struct { diff --git a/pkg/skills/options.go b/pkg/skills/options.go index fc63c6ca62..3590275d8b 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -239,9 +239,14 @@ 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. + // artifact (COSIGN_PASSWORD decrypts encrypted keys). Mutually exclusive + // with IdentityToken. One of Key, IdentityToken, or NoSign is required. Key string `json:"key,omitempty"` + // IdentityToken is a short-lived OIDC identity token (raw JWT) used for + // keyless signing: the server exchanges it with Fulcio for a short-lived + // signing certificate and records the signature in Rekor. Mutually + // exclusive with Key. One of Key, IdentityToken, or NoSign is required. + IdentityToken string `json:"identity_token,omitempty"` // NoSign pushes without signing. Consumers installing the artifact // project-scoped will need an explicit unsigned exception. NoSign bool `json:"no_sign,omitempty"` diff --git a/pkg/skills/skillsvc/build.go b/pkg/skills/skillsvc/build.go index 7a2a53f664..e469b8e1dc 100644 --- a/pkg/skills/skillsvc/build.go +++ b/pkg/skills/skillsvc/build.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "net/http" + "os" "path/filepath" "strings" @@ -21,6 +22,15 @@ import ( "github.com/stacklok/toolhive/pkg/skills" ) +// Environment variable overrides for the Fulcio/Rekor instances used by +// keyless signing. Unset means the sigstore public-good instances (core's +// defaults). Intended for E2E and staging use only — not a supported +// production configuration knob. +const ( + envFulcioURL = "TOOLHIVE_SIGSTORE_FULCIO_URL" + envRekorURL = "TOOLHIVE_SIGSTORE_REKOR_URL" +) + // Validate checks whether a skill definition is valid. func (*service) Validate(_ context.Context, path string) (*skills.ValidationResult, error) { if err := validateLocalPath(path); err != nil { @@ -113,11 +123,8 @@ 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, - ) + if err := validateSigningInputs(opts); err != nil { + return err } d, err := s.ociStore.Resolve(ctx, opts.Reference) @@ -139,7 +146,10 @@ func (s *service) Push(ctx context.Context, opts skills.PushOptions) error { // 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, + Key: opts.Key, + IdentityToken: opts.IdentityToken, + FulcioURL: os.Getenv(envFulcioURL), + RekorURL: os.Getenv(envRekorURL), }); err != nil { return httperr.WithCode(fmt.Errorf("signing pushed artifact: %w", err), http.StatusBadRequest) } @@ -232,6 +242,39 @@ func (s *service) DeleteBuild(ctx context.Context, tag string) error { return s.ociStore.DeleteBuild(ctx, tag) } +// validateSigningInputs enforces that a push declares exactly one signing +// method: a cosign key, an OIDC identity token for keyless signing, or an +// explicit opt-out. Ambiguous or absent input is rejected here, before the +// artifact is pushed, rather than surfacing as a signing failure afterward. +func validateSigningInputs(opts skills.PushOptions) error { + methods := 0 + if opts.Key != "" { + methods++ + } + if opts.IdentityToken != "" { + methods++ + } + switch { + case opts.NoSign && methods > 0: + return httperr.WithCode( + errors.New("no_sign (--no-sign) cannot be combined with key (--key) or identity_token (--identity-token)"), + http.StatusBadRequest, + ) + case !opts.NoSign && methods == 0: + return httperr.WithCode( + errors.New("signing credential required: set key (--key), identity_token (--identity-token) "+ + "for CI/OIDC keyless signing, or no_sign (--no-sign) to push unsigned"), + http.StatusBadRequest, + ) + case !opts.NoSign && methods > 1: + return httperr.WithCode( + errors.New("specify only one of key (--key) or identity_token (--identity-token)"), + http.StatusBadRequest, + ) + } + return nil +} + // validateLocalPath checks that a path is non-empty, absolute, and does not // contain ".." path traversal segments. This prevents API clients from // accessing arbitrary directories on the host filesystem via traversal. diff --git a/pkg/skills/skillsvc/build_verify_test.go b/pkg/skills/skillsvc/build_verify_test.go index 2f2b0fcf76..c450e489ac 100644 --- a/pkg/skills/skillsvc/build_verify_test.go +++ b/pkg/skills/skillsvc/build_verify_test.go @@ -33,17 +33,42 @@ func newPushFixture(t *testing.T) (*ocimocks.MockRegistryClient, *ociskills.Stor 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) { +// TestPushValidatesSigningInputs guards the RFC invariant that pushes are +// signed by default: exactly one of a key, an identity token, or an explicit +// no_sign must be given, before anything is pushed. +func TestPushValidatesSigningInputs(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)) + tests := []struct { + name string + opts skills.PushOptions + }{ + {name: "neither key, identity_token, nor no_sign", opts: skills.PushOptions{}}, + { + name: "both key and identity_token", + opts: skills.PushOptions{Key: "/tmp/cosign.key", IdentityToken: "tok"}, + }, + { + name: "no_sign combined with key", + opts: skills.PushOptions{NoSign: true, Key: "/tmp/cosign.key"}, + }, + { + name: "no_sign combined with identity_token", + opts: skills.PushOptions{NoSign: true, IdentityToken: "tok"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + reg, ociStore, _ := newPushFixture(t) + svc := New(&storage.NoopSkillStore{}, WithRegistryClient(reg), WithOCIStore(ociStore)) + + tc.opts.Reference = "my-tag" + err := svc.Push(t.Context(), tc.opts) + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) + }) + } } // TestPushSignsAfterPushing proves the pushed artifact is signed with the @@ -63,6 +88,29 @@ func TestPushSignsAfterPushing(t *testing.T) { require.NoError(t, err) } +// TestPushSignsKeylessWithIdentityToken proves an identity token pushes +// through keyless signing instead of a key, and that the Fulcio/Rekor URL +// env overrides reach core's signer.Options — the E2E/staging escape hatch. +// Not run in parallel: t.Setenv forbids it. +func TestPushSignsKeylessWithIdentityToken(t *testing.T) { + reg, ociStore, digest := newPushFixture(t) + t.Setenv(envFulcioURL, "https://fulcio.example.test") + t.Setenv(envRekorURL, "https://rekor.example.test") + + ms := signermocks.NewMockSigner(gomock.NewController(t)) + ms.EXPECT().SignOCI(gomock.Any(), "my-tag", digest, signer.Options{ + IdentityToken: "a.b.c", + FulcioURL: "https://fulcio.example.test", + RekorURL: "https://rekor.example.test", + }).Return(&signer.Result{Bundle: []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", IdentityToken: "a.b.c"}) + 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) {