Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/server/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions docs/server/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions docs/server/swagger.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions pkg/api/v1/skills.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/api/v1/skills_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions pkg/api/v1/skills_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/skills/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/skills/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down
5 changes: 4 additions & 1 deletion pkg/skills/client/dto.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 7 additions & 2 deletions pkg/skills/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
55 changes: 49 additions & 6 deletions pkg/skills/skillsvc/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"log/slog"
"net/http"
"os"
"path/filepath"
"strings"

Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
Expand Down Expand Up @@ -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.
Expand Down
66 changes: 57 additions & 9 deletions pkg/skills/skillsvc/build_verify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down
Loading