Require a discovery capability for key signing

[samuv]

Summary

POST /api/v1beta/skills/push accepts key, a path to a cosign private key that the thv serve process opens and signs with. Without independent authorization, the management API can become a signing oracle: an untrusted caller can ask the server to publish an artifact signed by any readable key.

Checking only the backend peer address is not sufficient. A public reverse proxy can forward an untrusted request over loopback or a UNIX socket, making the caller appear local to the API server.

This change:

• Generates an independent 128-bit key-signing capability at server startup. It is separate from the discovery nonce, which /health intentionally exposes.
• Stores the capability only in the owner-protected server discovery file and requires it for every key-bearing push, using a constant-time comparison and failing closed when it is missing or misconfigured.
• Has automatically discovered skill clients attach the capability only to key-bearing pushes. TOOLHIVE_API_URL and manually constructed clients do not inherit it.
• Protects the capability as a bearer credential: it is not logged or returned by /health, is refused over an unprotected remote transport, and is never followed across redirects.
• Documents the conditional header and 403 response in OpenAPI, CLI help, and the skills architecture guide.

This was found while reviewing #6528, which restores the same --key path for plugins. The plugin branch will need to adopt the same capability after rebasing.

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)
• Generated API and CLI documentation (task docs)

Regression coverage includes fail-closed capability checks, a real reverse proxy whose backend sees a loopback peer, discovery round trips, health-header non-disclosure, client header scoping, and redirect refusal.

Changes

Area	Change
API server and discovery	Generate, persist, and verify the protected key-signing capability
Skills API client	Read the capability only from local discovery and attach it only to key pushes
Transport security	Refuse insecure delivery and redirects for signing credentials
Tests	Cover proxy bypass, missing/wrong capability, non-disclosure, and redirect leakage
Docs	Document the capability header, 403 response, CLI constraint, and architecture

Does this introduce a user-facing change?

Yes. thv skill push --key now requires the automatically discovered local ToolHive server. A key-bearing push through TOOLHIVE_API_URL, a manually constructed client, or a caller without the protected discovery capability receives 403. Keyless signing with an OIDC identity token and unsigned pushes are unchanged. The 403 body names the runnable alternatives: drop --key (the CLI acquires an identity token itself) or pass --identity-token; direct API callers are told to send identity_token in place of key.

Special notes for reviewers

• Loopback and IPC are intentionally not authorization boundaries because a reverse proxy can make a remote caller appear local.
• The discovery nonce is intentionally not reused: /health returns it publicly for instance verification.
• Existing clients remain compatible for non-key operations. Old clients talking to the new server receive 403 only when they attempt a key-bearing push without the capability.
• The capability header is documented as optional because it is required only when request.key is set.
• Package-level change in pkg/server/discovery: ServerInfo gains a KeySigningCapability field. Every literal in this repo is keyed, and go vet's composites check already rejects unkeyed literals of a struct imported from another package, so any downstream code this could break is code go vet refuses today. A separate discovery-file DTO would keep the exported shape byte-identical but would also mean a second Write/Read signature to thread the capability through; I judged that a worse trade for a field addition. Migration, if anyone does hit it: switch the literal to keyed fields.
• Review round (2026-09-10): the 403 remediation originally said "sign keylessly with identity_token", the JSON field, while the CLI relays the server body verbatim and only knows --identity-token. Reworded in 9d52481 (originally 668364a before the rebase onto current main) so a terminal user gets a runnable path first; the test now pins --identity-token, drop --key, and identity_token all appearing.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 82.53968% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.83%. Comparing base (1f87350) to head (9d52481).

Files with missing lines	Patch %	Lines
pkg/api/server.go	63.63%	8 Missing ⚠️
pkg/skills/client/client.go	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6560      +/-   ##
==========================================
- Coverage   78.89%   78.83%   -0.06%     
==========================================
  Files         778      779       +1     
  Lines       77598    77641      +43     
==========================================
- Hits        61219    61209      -10     
- Misses      16374    16427      +53     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[samuv]

Context for reviewers: this came out of the review on #6528, which restores thv ai-plugin push --key. The finding there was that a key-bearing push turns the API into a signing oracle — correct, but not introduced by that PR: /skills/push already accepts a caller-nominated private-key path on merged main. Guarding only the plugin side would have left the live exposure open here, so the guard is introduced once, next to the endpoint that ships it today, and #6528 (stacked above) adds the one-line call for /plugins/push.

Worth a second opinion on two judgement calls, both argued in the PR body:

1. Loopback TCP is permitted. A loopback bind gets an Origin allowlist, so a cross-origin page cannot drive it; what remains is local processes that could read the key directly anyway. IPC-only would break thv serve on 127.0.0.1:8080 for no gain — but if you'd rather be stricter, that's a one-line change.
2. The behaviour change is user-visible: thv skill push --key against a remote server now 403s. That path could not work regardless (the key must exist on the server's filesystem), so this trades a confusing failure for an explicit one — but it is a behaviour change on a shipped flag.

[JAORMX]

Fresh review of exact head 11c47dd against main 5b66987.\n\nThe private-key signing-oracle fix is sound. The capability is independently generated with crypto/rand, written only to the owner-protected discovery record, never disclosed by /health, and checked in constant time before the key-bearing skills push reaches the service. It fails closed for missing, wrong, or unconfigured capability; a loopback or IPC-facing backend is deliberately not treated as authorization, covering the reverse-proxy bypass.\n\nDiscovery-derived clients attach the capability only for --key pushes, never inherit it for TOOLHIVE_API_URL/manual endpoints, reject unsafe plaintext remote transport, and suppress redirects so the bearer credential cannot be replayed. Existing keyless and unsigned pushes remain unaffected. Architecture, API/transport, UX, and security panel checks found no merge-blocking issue.\n\nNo blocking findings. Exact-head CI is green (the superseded cancelled size check is non-actionable) and is reported separately from this approval.

[JAORMX]

Panel review of 11c47dda071c431af4bb3aab688ef4deafab3853 against main 5b669870017ee4b1ea39ea1b95a03f4c5b47aeab.

Blocking — the 403 remediation names an API field, not a usable CLI option

pkg/api/v1/key_signing_transport.go:63-67 directs a rejected key-signing push to use identity_token. The CLI relays this server response unchanged (pkg/skills/client/client.go:451-452), but its supported option is --identity-token (cmd/thv/app/skill_push.go:43-47). A thv skill push --key user who follows the error is sent to a nonexistent option/argument rather than a working safe path. Use CLI-valid wording such as --identity-token / omit --key for automatic keyless signing, while documenting the JSON identity_token field separately for direct API consumers.

Non-blocking — exported discovery struct change is source-incompatible

Adding KeySigningCapability in the middle of exported discovery.ServerInfo (pkg/server/discovery/discovery.go:31-52) breaks downstream unkeyed composite literals. Consider retaining the public struct's shape and serializing the capability through an internal discovery-file DTO or a backward-compatible accessor; otherwise document the package-level breaking change and migration.

The security, architecture, protocol, reuse, and duplication reviewers found the authorization boundary itself sound: the capability is generated independently, restricted to discovery, constant-time checked, withheld from health/logs, and protected from insecure/redirected transport. CI is green at this head.

[samuv]

Both points taken; the blocker is fixed in 668364a (now 9d52481 after the stack was rebased onto current main, same content) and #6528 is restacked on it.

Blocking — 403 named the JSON field

Agreed, and the mechanism was exactly as you describe: the CLI relays the server body verbatim, so identity_token reached a terminal where only --identity-token exists. The refusal now leads with the two runnable paths and keeps the request field for direct API callers:

Push through the locally discovered ToolHive server, or sign keylessly instead: drop --key and the CLI acquires a short-lived identity token itself, or hands one over with --identity-token; a direct API caller sends identity_token in place of key

"Drop --key" is listed first because it is the zero-configuration path — thv skill push without --key already acquires a token from ambient CI OIDC or a browser sign-in. TestRequireKeySigningCapability now pins all three tokens (--identity-token, drop --key, identity_token) so a future rewording cannot drift back to API-only vocabulary. This is the shared guard, so the plugin route on #6528 inherits the wording.

Non-blocking — ServerInfo field addition

Kept as a field, documented rather than hidden behind a DTO. Reasoning, in case you want to push back:

• Every ServerInfo literal in this repo is keyed, and go vet's composites check rejects unkeyed literals of a struct imported from another package, so any downstream code this would break is code go vet already refuses.
• A discovery-file DTO would keep the exported shape byte-identical but needs a second Write/Read signature (or an accessor) to thread the capability through — a larger surface change than the field, for a break that vet says nobody can be relying on.

The PR body now records this under "Special notes for reviewers" with the one-line migration (switch the literal to keyed fields).

task lint-fix clean, task test green under -race on both branches. #6528 rebased cleanly onto it (head now a58c244) and also passes.

[JAORMX]

Follow-up panel review completed against 9d5248151d36df661b923b5b63caf82e92ce9b27.

The prior blocker is resolved: the 403 path now names the runnable CLI alternatives (--identity-token or dropping --key) while retaining identity_token guidance for direct API callers, and tests cover all three terms. Security and API panel checks found no new merge blockers in the capability generation, discovery-only delivery, constant-time enforcement, or redirect/transport protections.

Exact-head CI is green.