Skip to content
Open
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
28 changes: 16 additions & 12 deletions cmd/thv/app/skill_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import (
)

var (
skillSyncProjectRoot string
skillSyncClientsRaw string
skillSyncCheck bool
skillSyncAdopt bool
skillSyncPrune bool
skillSyncYes bool
skillSyncFormat string
skillSyncProjectRoot string
skillSyncClientsRaw string
skillSyncCheck bool
skillSyncAdopt bool
skillSyncPrune bool
skillSyncYes bool
skillSyncAllowUnsigned bool
skillSyncFormat string
)

var skillSyncCmd = &cobra.Command{
Expand Down Expand Up @@ -59,6 +60,8 @@ func init() {
"Remove installs no longer present in the lock file")
skillSyncCmd.Flags().BoolVar(&skillSyncYes, "yes", false,
"Skip the confirmation prompt (required when not running interactively)")
skillSyncCmd.Flags().BoolVar(&skillSyncAllowUnsigned, "allow-unsigned", false,
"Allow adopting skills whose signature state cannot be established (recorded as unsigned)")
AddFormatFlag(skillSyncCmd, &skillSyncFormat)
}

Expand All @@ -84,11 +87,12 @@ func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error {

c := newSkillClient(cmd.Context())
result, err := c.Sync(cmd.Context(), skills.SyncOptions{
ProjectRoot: projectRoot,
Clients: parseSkillInstallClients(skillSyncClientsRaw),
Check: skillSyncCheck,
Adopt: skillSyncAdopt,
Prune: skillSyncPrune,
ProjectRoot: projectRoot,
Clients: parseSkillInstallClients(skillSyncClientsRaw),
Check: skillSyncCheck,
Adopt: skillSyncAdopt,
Prune: skillSyncPrune,
AllowUnsigned: skillSyncAllowUnsigned,
})
if err != nil {
return formatSkillError("sync skills", err)
Expand Down
1 change: 1 addition & 0 deletions docs/cli/thv_skill_sync.md

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/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.

11 changes: 6 additions & 5 deletions pkg/api/v1/skills.go
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,12 @@ func (s *SkillsRoutes) syncSkills(w http.ResponseWriter, r *http.Request) error
}

result, err := s.lockService.Sync(r.Context(), skills.SyncOptions{
ProjectRoot: req.ProjectRoot,
Clients: req.Clients,
Prune: req.Prune,
Check: req.Check,
Adopt: req.Adopt,
ProjectRoot: req.ProjectRoot,
Clients: req.Clients,
Prune: req.Prune,
Check: req.Check,
Adopt: req.Adopt,
AllowUnsigned: req.AllowUnsigned,
})
if err != nil {
return err
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 @@ -88,6 +88,9 @@ type syncSkillsRequest struct {
Check bool `json:"check,omitempty"`
// Adopt writes lock entries for existing unmanaged project-scope installs
Adopt bool `json:"adopt,omitempty"`
// AllowUnsigned permits adopting skills whose signature state cannot be
// established, recording them as unsigned
AllowUnsigned bool `json:"allow_unsigned,omitempty"`
}

// upgradeSkillsRequest represents the request to upgrade a project's skills.
Expand Down
11 changes: 6 additions & 5 deletions pkg/skills/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,12 @@ func (c *Client) GetContent(ctx context.Context, opts skills.ContentOptions) (*s
// Sync restores a project's installed skills to match its lock file.
func (c *Client) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) {
body := syncRequest{
ProjectRoot: opts.ProjectRoot,
Clients: opts.Clients,
Prune: opts.Prune,
Check: opts.Check,
Adopt: opts.Adopt,
ProjectRoot: opts.ProjectRoot,
Clients: opts.Clients,
Prune: opts.Prune,
Check: opts.Check,
Adopt: opts.Adopt,
AllowUnsigned: opts.AllowUnsigned,
}

var result skills.SyncResult
Expand Down
22 changes: 22 additions & 0 deletions pkg/skills/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -766,3 +766,25 @@ func TestInstallCarriesAllowUnsigned(t *testing.T) {
require.NoError(t, err)
assert.True(t, got.AllowUnsigned, "allow_unsigned must reach the server")
}

// TestSyncCarriesAllowUnsigned mirrors TestInstallCarriesAllowUnsigned for
// the sync/adopt path.
func TestSyncCarriesAllowUnsigned(t *testing.T) {
t.Parallel()

var got syncRequest
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")
_ = json.NewEncoder(w).Encode(skills.SyncResult{})
}))
t.Cleanup(srv.Close)

_, err := newTestClient(t, srv).Sync(t.Context(), skills.SyncOptions{
ProjectRoot: "/tmp/project",
Adopt: true,
AllowUnsigned: true,
})
require.NoError(t, err)
assert.True(t, got.AllowUnsigned, "allow_unsigned must reach the server")
}
2 changes: 2 additions & 0 deletions pkg/skills/client/dto.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ type syncRequest struct {
Prune bool `json:"prune,omitempty"`
Check bool `json:"check,omitempty"`
Adopt bool `json:"adopt,omitempty"`
// AllowUnsigned mirrors skills.SyncOptions.AllowUnsigned for adoption.
AllowUnsigned bool `json:"allow_unsigned,omitempty"`
}

type upgradeRequest struct {
Expand Down
4 changes: 4 additions & 0 deletions pkg/skills/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ type SyncOptions struct {
// Check verifies on-disk content against contentDigest without installing
// or writing anything.
Check bool `json:"check,omitempty"`
// AllowUnsigned permits adopting a skill whose signature state cannot
// be established (no stored bundle), recording an explicit unsigned
// exception — the same trust decision as an unsigned install.
AllowUnsigned bool `json:"allow_unsigned,omitempty"`
// Adopt writes lock entries for existing unmanaged project-scope installs.
Adopt bool `json:"adopt,omitempty"`
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/skills/skillsvc/lock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ func newLockTestService(t *testing.T, gr *gitmocks.MockResolver, extra ...Option
AnyTimes().Return(signedResult(), nil)
mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().Return(signedResult(), nil)
mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().Return(nil)
mv.EXPECT().ResultFromBundle(gomock.Any(), gomock.Any()).
AnyTimes().Return(signedResult(), nil)

opts := append([]Option{WithPathResolver(pr), WithGitResolver(gr), WithVerifier(mv)}, extra...)
svc := New(store, opts...)
Expand Down
67 changes: 64 additions & 3 deletions pkg/skills/skillsvc/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ import (
"context"
"errors"
"fmt"
"log/slog"
"net/http"
"strings"

"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"
"github.com/stacklok/toolhive/pkg/storage"
)

Expand Down Expand Up @@ -88,7 +91,19 @@ func (s *service) syncLockedEntry(
dbOK bool,
result *skills.SyncResult,
) {
if dbOK && entryMatchesInstalled(s.pathResolver, entry, sk) {
sigOK := true
if dbOK {
if sigErr := s.verifyStoredSignature(entry, sk); sigErr != nil {
// A failed offline re-verification is treated as drift: check
// mode reports it, apply mode reinstalls from the pinned
// reference — where install-time verification enforces the
// locked identity and, on success, heals the stored bundle.
sigOK = false
slog.Warn("stored signature failed offline re-verification",
"skill", entry.Name, "error", sigErr)
}
}
if dbOK && sigOK && entryMatchesInstalled(s.pathResolver, entry, sk) {
result.AlreadyCurrent = append(result.AlreadyCurrent, entry.Name)
return
}
Expand Down Expand Up @@ -170,7 +185,7 @@ func (s *service) syncUnlockedInstall(
if !sk.Managed {
result.NeverManaged = append(result.NeverManaged, sk.Metadata.Name)
if opts.Adopt && !opts.Check {
if err := s.adoptSkill(ctx, sk); err != nil {
if err := s.adoptSkill(ctx, opts, sk); err != nil {
result.Failed = append(result.Failed, skills.SyncFailure{
Name: sk.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(),
})
Expand All @@ -193,23 +208,69 @@ func (s *service) syncUnlockedInstall(
}
}

// verifyStoredSignature re-verifies the Sigstore bundle stored with an
// installed skill against the identity its lock entry records — entirely
// offline, via the embedded trust root. Entries recorded unsigned or with
// no provenance have nothing to verify. A recorded identity with no stored
// bundle fails closed for OCI installs (the bundle should exist); git
// installs never store a bundle — their signature lives on the commit and
// is re-verified when content is re-resolved.
func (s *service) verifyStoredSignature(entry lockfile.Entry, sk skills.InstalledSkill) error {
if entry.Unsigned || entry.Provenance == nil {
return nil
}
if len(sk.SigstoreBundle) == 0 {
if !strings.Contains(entry.Digest, ":") {
return nil // git install: no stored bundle by design
}
return fmt.Errorf("%w: lock entry records signer %q but no bundle is stored",
verifier.ErrSignatureInvalid, entry.Provenance.SignerIdentity)
}
return s.artifactVerifier().VerifyBundleOffline(sk.SigstoreBundle, entry.Digest, entry.Provenance)
}

// adoptSkill writes a lock entry for an existing, unmanaged project-scope
// install, pinning its current on-disk state. The install's own Reference is
// used as Source: an adopted install predates (or never went through) lock
// tracking, so the original user-typed request is not recoverable — the
// concrete resolved reference is the closest available fact to pin against.
func (s *service) adoptSkill(ctx context.Context, sk skills.InstalledSkill) error {
//
// Trust state is back-filled from the stored Sigstore bundle when one
// exists; otherwise adoption is the same trust decision as an unsigned
// install and requires the explicit AllowUnsigned exception.
func (s *service) adoptSkill(ctx context.Context, opts skills.SyncOptions, sk skills.InstalledSkill) error {
contentDigest, err := computeContentDigest(s.pathResolver, sk)
if err != nil {
return fmt.Errorf("computing content digest: %w", err)
}
var provenance *lockfile.Provenance
unsigned := false
if len(sk.SigstoreBundle) > 0 {
result, verifyErr := s.artifactVerifier().ResultFromBundle(sk.SigstoreBundle, sk.Digest)
if verifyErr != nil {
return fmt.Errorf("verifying stored bundle for adoption: %w", verifyErr)
}
provenance = provenanceInfoToLock(provenanceInfoFromResult(result))
}
if provenance == nil {
if !opts.AllowUnsigned {
return httperr.WithCode(
fmt.Errorf("%w: adopting %q records it as unsigned; pass --allow-unsigned to accept that",
verifier.ErrUnsigned, sk.Metadata.Name),
http.StatusForbidden,
)
}
unsigned = true
}
if err := recordLockEntry(sk.ProjectRoot, lockEntryInput{
Name: sk.Metadata.Name,
Version: sk.Metadata.Version,
Source: sk.Reference,
ResolvedReference: sk.Reference,
Digest: sk.Digest,
ContentDigest: contentDigest,
Provenance: provenance,
Unsigned: unsigned,
}); err != nil {
return fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err))
}
Expand Down
16 changes: 15 additions & 1 deletion pkg/skills/skillsvc/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,14 +305,26 @@ func TestSync_AdoptsUnmanagedInstall(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, []string{"unmanaged-skill"}, result.NeverManaged)

// Adoption of an install with no stored bundle is an unsigned trust
// decision: without the explicit exception it fails...
result, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Adopt: true})
require.NoError(t, err)
require.Len(t, result.Failed, 1)
assert.Equal(t, skills.FailureReasonUnsignedRejected, result.Failed[0].Reason)
lf := readLockfile(t, projectRoot)
_, ok := lf.Get("unmanaged-skill")
assert.False(t, ok, "adoption without the unsigned exception must not write a lock entry")

// ...and with it, the entry records the unsigned state.
result, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Adopt: true, AllowUnsigned: true})
require.NoError(t, err)
assert.Equal(t, []string{"unmanaged-skill"}, result.NeverManaged)

lf := readLockfile(t, projectRoot)
lf = readLockfile(t, projectRoot)
entry, ok := lf.Get("unmanaged-skill")
require.True(t, ok, "adopt must write a lock entry for the unmanaged install")
assert.NotEmpty(t, entry.ContentDigest)
assert.True(t, entry.Unsigned, "an adoption without verifiable trust must record unsigned")

sk, err := svc.Info(t.Context(), skills.InfoOptions{Name: "unmanaged-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot})
require.NoError(t, err)
Expand Down Expand Up @@ -381,6 +393,8 @@ func TestSync_CheckDetectsTamperInAnyClientDir(t *testing.T) {
mv := verifiermocks.NewMockVerifier(ctrl)
mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().Return(signedResult(), nil)
mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().Return(nil)
svc := New(store, WithPathResolver(pr), WithGitResolver(gr), WithVerifier(mv))

ref, _ := gitRef("multi-skill")
Expand Down
Loading
Loading