Restore capability-gated --key signing on thv ai-plugin push - #6528
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6528 +/- ##
==========================================
- Coverage 78.94% 78.94% -0.01%
==========================================
Files 781 782 +1
Lines 77784 77814 +30
==========================================
+ Hits 61410 61431 +21
- Misses 16369 16378 +9
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
935df4f to
3f1c3c4
Compare
3f1c3c4 to
55be810
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Reviewed 55be810 against its exact stacked base abda3b6. This direct range consistently restores --key through CLI, client/DTO, API, validation, and signer options; the CLI correctly states that the signing host (thv serve) must access an encrypted key/password. No blocking findings in the PR-owned changes. This approval does not waive the separately requested changes on stacked #6525. Current CI includes an author-owned failing lifecycle job and remaining in-progress checks; it is not part of this review decision.
55be810 to
c58156f
Compare
c58156f to
9dc7307
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Fresh review of exact head ea4f6bb against exact stacked base #6560 at 11c47dd. This review attributes only the PR-owned range.\n\nThe prior private-key signing-oracle blocker is resolved by the inherited protected discovery capability and this PR's pre-dispatch plugin-route integration (pkg/api/v1/plugins.go:342-355; pkg/api/server.go:461-465). The capability is sourced only through protected local discovery, sent only for --key pushes, and protected from plaintext remote transport and redirects (pkg/plugins/client/client.go:301-324). The direct range correctly carries --key through CLI, client/DTO, API, service, and signer; exactly-one signing-mode validation and stage → sign → promote remain intact.\n\nNo merge-blocking findings in the direct range. The CLI help accurately states that COSIGN_PASSWORD is read by thv serve; the generated API schema's shorter description is non-blocking.\n\nCI is separate from this decision: all checks are successful except Helm Charts / Lint and Test Helm Charts, which is author-owned and does not establish a reviewed-code defect.
ea4f6bb to
e5526a7
Compare
e5526a7 to
a58c244
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Follow-up panel review of the direct range 9d5248151d36df661b923b5b63caf82e92ce9b27...a58c2442925ecfacff028f80b1430a0a269e3950.
Blocking — direct API requests can bypass signing-mode validation
pushPlugin only validates reference before it dispatches to PluginService (pkg/api/v1/plugins.go:335-354), though the endpoint's documented contract requires exactly one of key, identity_token, or no_sign (pkg/api/v1/plugins_types.go:81-105). A request containing only reference therefore succeeds with a service implementation that does not repeat the concrete pluginsvc validation; the route test explicitly expects that invalid request to return 204 (pkg/api/v1/plugins_test.go:466-474). This makes the API contract dependent on a particular service implementation and permits generated/direct clients to issue a request that the real service rejects.
Validate the signing choice at the HTTP trust boundary before dispatch, and change the route test to expect 400. The capability guard can remain scoped to the key arm after that validation.
The previous signing-oracle finding is addressed by the inherited discovery capability plus this PR's route/client wiring. Exact-head CI is green; this correctness issue is independently blocking.
a58c244 to
5c7a52a
Compare
|
Addressed in 5c7a52a. Verified the finding first:
Also rebased onto One thing outside this PR's scope: |
Plugin push was made keyless-only because a key-signed artifact could not be installed project-scoped: verification found the bundle, failed it as key-signed, and --allow-unsigned did not apply. The flag could only publish plugins nobody could consume, so it was removed rather than documented as a trap. Install-time key verification now exists. A key-signed plugin installs with --public-key, the key is pinned in the lock entry, sync re-verifies the stored bundle against it offline, and upgrade applies it to the candidate — so publishing one is no longer a dead end and the flag can come back. PushOptions is an alias of skills.PushOptions again; the distinct struct existed only to omit Key. The push DTO, the Go client, and validateSigningInputs carry the key arm the skills surface already has, and the endpoint keeps rejecting unknown fields with key now among the known ones. Fixes #6442. Signed-off-by: Samuele Verzi <samu@stacklok.com>
The path is opened by the thv serve process that signs, not by the CLI, so against a remote server the advertised command fails unless the key file and COSIGN_PASSWORD are both provisioned on that host. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Same guard as skills/push, which the commit below adds: a private-key path is resolved by the thv serve process, so a remote caller naming one would be asking the server to sign with a key it never supplied. Signed-off-by: Samuele Verzi <samu@stacklok.com>
Carry the protected discovery capability through the plugin API client and require it before dispatching a key-bearing push. Keep the capability off non-key requests and protect it from plaintext remote transports and redirects. Update the CLI, OpenAPI, architecture docs, and regression coverage to reflect that server-side keys are available only through automatic local discovery. Signed-off-by: Samuele Verzi <samu@stacklok.com>
The push endpoint documents that exactly one of key, identity_token, or no_sign is required, but the handler only checked reference before dispatching. The contract was held by pluginsvc alone, so a request naming only a reference was a 204 against any other PluginService implementation, and the route test pinned that behavior. Lift the validator into pkg/plugins as ValidatePushSigning and run it in the handler before the capability guard and dispatch. The service keeps calling it for in-process callers. The reference-only route test now expects 400, and a conflicting-choice case is added alongside it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Summary
thv ai-plugin push --keywas removed in #6438 because ToolHive could publish a key-signed plugin but could not verify and install it project-scoped. With the install-time public-key verification and lock-file support from #6524 and #6525, publishers can safely use key signing again and consumers can pin the matching public key on first install. This closes #6442.Restoring the flag also reopens the server-side private-key trust boundary addressed by #6560: the
thv serveprocess resolves and opens the requested key path. A peer-address check is insufficient because a public reverse proxy can make an untrusted request appear to arrive over loopback or IPC. This PR therefore extends #6560's protected discovery capability to plugin pushes rather than treating transport locality as authorization.This change:
--keyand theKeyfield across the plugin CLI, API DTO, client, and service, while retaining strict signing-input validation.key,identity_token,no_sign) in the handler before the capability guard and dispatch, so the endpoint's contract holds independent of whichPluginServiceimplementation is wired in. The validator lives inpkg/pluginsand the service calls the same function for in-process callers.--keyrequires the locally discovered ToolHive server; remote or manually configured API URLs must use keyless signing.Fixes #6442
Type of change
Test plan
task test)task lint-fix)task test-e2e)task docs)Regression coverage includes:
Changes
--key, load the discovery capability, and protect credential transportkey, validate the signing choice and require the capability before dispatch, and document 403API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.keyand the conditional capability header are additive. Existing keyless and--no-signcallers are unchanged.Does this introduce a user-facing change?
Yes.
thv ai-plugin push --key <cosign.key>works again when the CLI uses automatic local server discovery. Consumers installing the resulting plugin project-scoped must passthv ai-plugin install --public-keyon first use; the key is pinned intoolhive.lock.yamland reused bysyncandupgradethereafter. Remote and manually configured API URLs must use keyless signing.Special notes for reviewers
mainafter Require a discovery capability for key signing #6560 merged. The rebase also reconciled Document AI plugin support #6627's documentation rewrite, which recorded that plugin push has no--key: the architecture doc, CLI help, and thetoolhive-cli-userskill reference now describe the restored flag and its local-discovery requirement.pushSkillinpkg/api/v1/skills.gohas the same handler-level gap (signing choice enforced only byskillsvc). Left for a separate PR to keep this one scoped to plugins.--keypush signing has no install-time verification path (plugins) #6442.--public-keyremains install-only.upgradeandsyncreuse the lock-file anchor, and in-place re-anchoring remains out of scope.🤖 Generated with Claude Code