Add private_key_jwt DCR client authentication - #6427
Merged
Merged
Conversation
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
jerm-dro,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
August 25, 2026 08:47
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6427 +/- ##
==========================================
+ Coverage 77.81% 77.84% +0.03%
==========================================
Files 760 761 +1
Lines 73133 73317 +184
==========================================
+ Hits 56908 57076 +168
- Misses 16220 16236 +16
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
samuv
requested changes
Aug 25, 2026
samuv
left a comment
Contributor
There was a problem hiding this comment.
Summary
This PR adds opt-in private_key_jwt dynamic client registration with CRD/runtime plumbing, persistence, discovery metadata, documentation, and end-to-end coverage. The overall wiring is coherent, but the current implementation exposes client authentication with two blocking security problems: replay consumption is not atomic, and assertion exp/jti are unbounded so a registered caller can grow replay state indefinitely. It also advertises EdDSA even though the pinned Fosite runtime rejects EdDSA client assertions. Requesting changes before merge.
Checklist
- Tests: Broad unit/integration/E2E coverage and CI is green, but missing end-to-end EdDSA, concurrent replay, assertion-bound, mislabeled-key, and weak-RSA cases.
- Docs: Extensive, but currently overclaim EdDSA support and contradict the loopback override field contract.
- Registry impact: None.
- Security: Blocking replay and storage-growth issues; RSA key-strength validation is also missing.
- Backwards compatibility: Additive and default-off; no existing-client compatibility regression identified.
Thread a new AllowPrivateKeyJWTRegistration flag through the CRD (EmbeddedAuthServerConfig), RunConfig/Config, and AuthorizationServerConfig, and add ValidatePrivateKeyJWTRegistrationTransport to reject combining it with insecureAllowHTTP or a plain-HTTP issuer. private_key_jwt DCR needs its own on/off switch, kept independent of AllowConfidentialClientRegistration: it authenticates clients by key possession rather than a shared secret, so it carries a different threat model (no secret to leak, but also none to revoke). This commit only carries the capability through the config pipeline — the flag is inert until the next commit teaches DCR validation to act on it.
Teach ValidateDCRRequest to accept token_endpoint_auth_method= private_key_jwt when the new flag is set: require a non-empty inline JWKS containing a valid public signing key whose algorithm matches the declared token_endpoint_auth_signing_alg, reject jwks_uri, and pin grant_types to exactly the token-exchange grant for these clients. RFC 7523 only defines how a client asserts its identity at the token endpoint once registered — DCR still has to decide what registration metadata is acceptable. Without these checks, a client could register a key/algorithm pair it doesn't actually hold, or metadata that would only surface as a confusing failure later at token-exchange time instead of at registration. Grant types are pinned because private_key_jwt registrations exist here for the delegation/ token-exchange flow, not authorization-code; accepting other grants would register clients for capabilities nothing downstream wires up.
Extend buildDCRClient and registration.New to construct a fosite client carrying the validated JWKS and signing algorithm instead of a generated client secret, via a new privateKeyJWTClient type, and echo jwks/jwks_uri/token_endpoint_auth_signing_alg back in the DCR response. Validation alone doesn't make a private_key_jwt client usable: fosite's client-authentication strategy needs the key material attached to the client record it looks up at the token endpoint, and RFC 7591 §3.2.1 requires the registration response to reflect the metadata the server actually stored so the caller can confirm what got registered.
Add JSONWebKeys/TokenEndpointAuthSigningAlgorithm fields to the Redis-backed storedClient, stripping to public-only key material via a new publicJSONWebKeySet helper before writing. A private_key_jwt client's key material has to survive a server restart or a Redis-backed multi-replica deployment the same way client_secret_* clients already do, or every registered client would stop working the moment the process serving it restarted. Stripping to public keys before persisting matters because a caller could mistakenly submit a JWK with a private half; storage should never retain private key material it has no business holding, even transiently.
Add token_endpoint_auth_signing_alg_values_supported to the discovery document, advertise private_key_jwt in token_endpoint_auth_methods_supported when the flag is set, and factor the signing-algorithm allowlist out into an exported SupportedSigningAlgorithms so DCR validation and discovery share one list. RFC 8414 clients choose an auth method from what discovery advertises, so leaving private_key_jwt out of the metadata would mean a client could register successfully but have no spec-compliant way to discover that it's supported. Sharing the algorithm allowlist with validation closes a gap where discovery could claim support for an algorithm registration would then reject — exactly the kind of self-inconsistency a client can't work around on its own.
Wire allowPrivateKeyJWTRegistration through the integration test harness, tighten ValidatePrivateKeyJWTRegistrationTransport to only exempt a genuine loopback issuer when InsecureAllowConfidentialOverLoopbackHTTP is set (matching the confidential-client transport check it was modeled on), and add integration coverage that registers a private_key_jwt client and carries it through the RFC 8693 token-exchange grant end to end. The transport guard's original plain-HTTP check didn't distinguish a genuine local loopback issuer from any other HTTP issuer, so the in-process test server had no legitimate way to opt into the combination it needs — the same problem AllowConfidentialClientRegistration already solved with the same flag. Reusing it instead of inventing a parallel opt-in keeps both registration paths' "is this issuer trustworthy enough for plaintext" answer consistent. The integration test then closes the real gap: everything so far validated registration and storage in isolation, but nothing had proven a registered private_key_jwt client could sign an assertion and actually receive a token.
Add an e2e test that registers a client via private_key_jwt and performs the delegated token exchange through a real VirtualMCPServer/EmbeddedAuthServerConfig deployment on a kind cluster. The integration test in the previous commit proves the auth-server logic in isolation, but the feature is only reachable in production through the operator's CRD wiring (applySimpleAuthServerConfigFields, admission CEL rules), which unit and integration tests never exercise. Per this repo's standing convention, a feature that only reaches users through the operator isn't verified until it's proven through a real cluster and real CRDs, not just RunConfig construction.
Document the private_key_jwt storage schema in docs/arch/11-auth-server-storage.md, and the registration/ token-exchange flow — including the RFC 7523 §2.2 vs §2.1 distinction — in docs/arch/17-token-exchange-delegation.md. This is the second client-authentication method and the first key-based one the storage layer and delegation flow support; nothing in the existing architecture docs explained how a client authenticates without a secret or how that interacts with delegation. The RFC 7523 §2.1/§2.2 distinction is called out specifically because both the already-documented JWT-bearer grant and this feature involve a JWT and cite RFC 7523, and someone skimming the code without that distinction spelled out could plausibly conflate two unrelated mechanisms.
Registration validation had two gaps: DCR requests for other auth methods could echo back and retain attacker-supplied JWKS/JWKS URI/signing-alg fields even though those fields are meaningless outside private_key_jwt, and several edge cases around grant types, redirect URIs, and JWK content weren't rejected. Zero the key fields unless the client actually registers with private_key_jwt, tighten the remaining validation, and drop the now-unused duplicate cloneJWKS helper in favor of cloneJSONWebKeySet. Also document the use:"sig" requirement on registered JWKs, which callers would otherwise only discover via a rejection error.
Add ValidateAlgorithmForPublicKey and rsaAlgorithmsForClientKeys, and fix a nil-signer panic in the pre-existing ValidateAlgorithmForKey by extracting both into a shared validateAlgorithmForPublicKeyValue parameterized on which RSA algorithms each caller accepts. Registered private_key_jwt clients submit an RSA/EC/Ed25519 public key plus a claimed signing algorithm; nothing was checking that the two are actually compatible (e.g. an EC key claiming RS256), which would otherwise only surface as an opaque signature-verification failure at token-exchange time instead of a clear rejection at registration. Extracting the shared EC/Ed25519 logic — rather than duplicating it a second time for client keys — also exposed that the existing signing-key validator dereferenced a typed-nil crypto.Signer before checking it, which is now guarded explicitly per key type instead of relying on a comment that no longer matched what the code actually did.
registration.SupportedSigningAlgorithms() hand-listed RS/PS/ES algorithms separately from crypto.rsaAlgorithmsForClientKeys, which validateAlgorithmForPublicKeyValue already used to check key/algorithm compatibility. The two lists had to be kept in sync by hand, and had already drifted: the hand-listed copy was missing EdDSA entirely, silently rejecting every Ed25519 private_key_jwt registration at the token_endpoint_auth_signing_alg allowlist check, even though DeriveAlgorithm, ValidateAlgorithmForKey, and validateAlgorithmForPublicKeyValue all fully support Ed25519 keys. Nothing caught this because no test exercised Ed25519/EdDSA registration at all. Export crypto.SupportedClientKeyAlgorithms as the single source of truth (RSA algorithms plus the three EC algorithms and EdDSA), and have registration.SupportedSigningAlgorithms delegate to it instead of maintaining a parallel copy. Switch the two RSA-algorithm allowlists from map[string]bool to []string plus slices.Contains, since a deterministic order is required now that one of them is echoed directly in the discovery document's token_endpoint_auth_signing_alg_values_supported. Add regression tests at both layers: crypto asserts EdDSA is present in the shared list, registration asserts an Ed25519 key actually registers successfully end-to-end.
Adding AllowPrivateKeyJWTRegistration to RunConfig required
regenerating the checked-in swagger docs (docs/server/{docs.go,
swagger.json,swagger.yaml}); the CI docs-verification job caught the
omission.
Review found three gaps in private_key_jwt key validation: - SupportedClientKeyAlgorithms advertised and accepted EdDSA, but fosite v0.49.0's client-assertion verification (client_authentication.go) only switches on the RS*/ES*/PS*/HS* families. An EdDSA client could register successfully and then never be able to authenticate. Excluded it, with regression tests locking in the exclusion and matching doc updates. - validatePrivateKeyJWTKey returned early once a JWK's own alg label matched the requested algorithm, without checking that the label actually matched the key's type/curve. An EC key mislabeled "RS256" passed registration despite never being able to sign or verify with it. Now always runs ValidateAlgorithmForPublicKey's structural check. - The RSA branch of validateAlgorithmForPublicKeyValue checked algorithm compatibility but never modulus size or exponent, despite MinRSAKeyBits already being defined for the server's own signing key. Existing tests even registered a toy N=3,E=3 key. Now rejects keys below MinRSAKeyBits and non-odd/degenerate exponents; toy-key fixtures replaced with real 2048-bit generated keys.
Two related review findings on the private_key_jwt client-assertion replay path: - fosite calls Storage.ClientAssertionJWTValid (check) and SetClientAssertionJWT (record) as two separate operations, and both backends only overwrote the replay marker on the second call. Two concurrent requests presenting the same assertion could both pass the check before either recorded it. ClientAssertionJWTValid is now the atomic checkpoint (a write-lock check-and-insert in memory, Redis SET NX), reserving the jti for a short fixed window; SetClientAssertionJWT unconditionally extends that reservation to the assertion's real expiry, since only the caller that won the reservation reaches it. Added a 20-goroutine concurrent test per backend proving exactly one validator wins. - A registered client could submit an assertion with an arbitrary future exp and an arbitrarily long jti; fosite persists the replay marker keyed on both before the rest of validation completes. Redis's TTL is computed from that client-controlled exp, so a client could set exp decades out and get an effectively permanent key; memory's lazy sweep never reaches an entry that never expires. Added MaxAssertionLifespan (5 minutes) and MaxAssertionJTILength (256 bytes), enforced in both backends before persisting. ConsumeAssertionJWT, the unrelated RFC 8693 actor-token replay path, is deliberately left untouched.
Both the DCR registration and token-exchange requests used http.DefaultClient with a background context, so a stalled port-forward or half-open connection could hang the suite indefinitely. Share a 30s-timeout http.Client between them, matching the convention already used elsewhere in this test directory.
Review flagged that this PR's ValidatePrivateKeyJWTRegistrationTransport reused InsecureAllowConfidentialOverLoopbackHTTP, a flag documented as having no effect without confidential-client registration, silently breaking that contract. The suggested fixes were either to document the reuse or give private_key_jwt its own loopback flag. Both fixes assumed the underlying gate was justified. It isn't: unlike confidential registration, which mints and returns a client_secret, private_key_jwt registration never returns anything secret — the client submits its own public key and gets back a client_id and the same JWKS it sent. There is nothing here for cleartext HTTP to expose. ToolHive's own DCR validation already treats private_key_jwt at the public-client tier (validateAuthMethod applies no HTTPS or redirect-URI restriction to it, same as "none"); the transport gate was the only place it got confidential-grade treatment, copied from the confidential-client check without re-deriving whether the threat model transfers. RFC 7591 §5's blanket TLS requirement applies uniformly regardless of auth method, so nothing in the spec singles out private_key_jwt for stricter treatment than "none" either. Removes ValidatePrivateKeyJWTRegistrationTransport, its two call sites, and the CRD field/CEL rule/flag this PR had added to work around it (InsecureAllowPrivateKeyJWTOverLoopbackHTTP). private_key_jwt registration is now governed only by the issuer's general insecureAllowHTTP policy, the same as any public client.
The rebase onto origin/main conflicted on files main had independently evolved (delegate-client loopback CEL rules, malformed-issuer error handling). The generated artifacts were resolved by taking one side during conflict resolution as a placeholder; this regenerates them from the merged source so they match it exactly.
jhrozek
force-pushed
the
dcr-private-key-jwt
branch
from
August 25, 2026 21:38
41929e4 to
0235071
Compare
Both TestMemoryStorage_ClientAssertionJWT_ConcurrentReplay and
TestRedisStorage_ClientAssertionJWT_ConcurrentReplay called
t.Parallel() themselves and then passed t into withStorage/
withRedisStorage, which also call t.Parallel() internally — every
other top-level test using these helpers directly relies on the
helper for this and doesn't call it a second time. The double call
panics ("t.Parallel called multiple times"), which aborted the whole
storage package's test binary in CI.
TestValidateConfidentialClientTransport's loop asserted every error contains "cleartext HTTP", which was true before upstream's malformed- issuer cases landed (#6426) — those fail with "confidential clients require a valid issuer URL" instead, from a different validation branch. The rebase merged in the new cases without updating this assertion. Match upstream's own fix: assert on "confidential clients", the substring actually common to every error path in ValidateConfidentialClientTransport.
samuv
approved these changes
Aug 26, 2026
samuv
left a comment
Contributor
There was a problem hiding this comment.
Summary
Follow-up on the previous request-changes review. The blocking items are addressed: EdDSA is no longer advertised, client-assertion replay consumption is atomic on both backends, assertion exp/jti are bounded before persist, mislabeled keys and weak RSA material are rejected at DCR, and the e2e HTTP calls now have a timeout.
Removing ValidatePrivateKeyJWTRegistrationTransport is a reasonable resolution of the loopback-flag contract issue — private_key_jwt DCR never returns a secret, so treating it like a public client under the issuer's general insecureAllowHTTP policy is consistent. Approving on that basis.
Non-blocking leftover: rejection tests for MaxAssertionLifespan / MaxAssertionJTILength (and dedicated weak-RSA negative cases) would still be useful follow-up so those checks cannot regress silently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Delegate clients currently authenticate to the embedded OAuth authorization server either with
none(public, PKCE-only) or with aclient_secretToolHive mints and hands back — meaning a shared secret has to be received and stored by the calling agent. This adds RFC 7523 §2.2private_key_jwtas a third, independently-gated Dynamic Client Registration (RFC 7591) auth method: the client generates its own keypair, registers with the public half, and thereafter authenticates by signing aclient_assertionJWT with the private key it never has to transmit or store server-side. This is unrelated to the RFC 7523 §2.1 JWT-bearer grant already supported (#6391) — that's a different mechanism (a JWT used directly as the grant) on a different code path.What changed:
AllowPrivateKeyJWTRegistrationflag threaded through the CRD (EmbeddedAuthServerConfig) →RunConfig/Config→AuthorizationServerConfig, independent ofAllowConfidentialClientRegistration, with its own transport guard rejecting the combination with cleartext HTTP.private_key_jwt: requires a non-empty inline JWKS (nojwks_uri— see Special notes) with a valid public signing key whose algorithm matches the declaredtoken_endpoint_auth_signing_alg; grant types pinned to exactly the token-exchange grant.token_endpoint_auth_methods_supported,token_endpoint_auth_signing_alg_values_supported) for the new method.crypto.SupportedClientKeyAlgorithms), replacing a hand-maintained copy that had already drifted and silently excluded Ed25519/EdDSA.docs/arch/11-auth-server-storage.md,docs/arch/17-token-exchange-delegation.md) and an operator-level e2e test against a real kind cluster.Related: #6425 (not fixed here — see Special notes).
Type of change
Test plan
task test)task test-e2e)task lint-fix)Manually verified during review:
go build ./pkg/authserver/...andgo testfor thecrypto,registration, andhandlerspackages (including new EdDSA regression tests). CI is expected to run the fulltask test/task lint-fix/task test-e2esuite, including the kind-cluster e2e test added in this branch (virtualmcp_private_key_jwt_test.go), which was passing as of when it was added but has not been re-run locally after the later fixup commits.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.AllowPrivateKeyJWTRegistrationis a new, optional, default-falsefield onEmbeddedAuthServerConfig— purely additive.Changes
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.goAllowPrivateKeyJWTRegistrationCRD field + CEL validationcmd/thv-operator/pkg/controllerutil/authserver.goRunConfigpkg/authserver/config.goRunConfig/Configfields +ValidatePrivateKeyJWTRegistrationTransportpkg/authserver/server/provider.goAuthorizationServerConfigpkg/authserver/server/registration/dcr.goprivate_key_jwt; signing-algorithm allowlistpkg/authserver/server/registration/client.goprivateKeyJWTClientconstructionpkg/authserver/server/handlers/dcr.gopkg/authserver/server/handlers/discovery.gopkg/authserver/server/crypto/keys.goSupportedClientKeyAlgorithms(single source of truth), nil-signer fixpkg/authserver/storage/redis.gopkg/oauthproto/{constants,dcr,discovery}.godocs/arch/{11-auth-server-storage,17-token-exchange-delegation}.mdtest/e2e/thv-operator/virtualmcp/virtualmcp_private_key_jwt_test.go*_test.go(throughout)Does this introduce a user-facing change?
Yes. Operators can opt a
VirtualMCPServer'sEmbeddedAuthServerConfigintoallowPrivateKeyJWTRegistration: true, letting delegate clients register and authenticate viaprivate_key_jwtinstead of a ToolHive-issuedclient_secret. Off by default; no behavior change for existing configurations.Special notes for reviewers
jwks_uriis deliberately not supported — only inlinejwks. An unauthenticated registration endpoint that makes the AS fetch an attacker-controlled URL is an SSRF surface; nothing in this feature needs it.RegisterClientHandler//oauth/registerhas no registration-time authentication gate at all (predates this branch, Add POST /oauth/register handler for dynamic client registration #3428) —AllowPrivateKeyJWTRegistration/AllowConfidentialClientRegistrationcontrol what a client can register as, not who may register. Not specific toprivate_key_jwt, not a blocker for this PR, but worth reviewers' awareness.token_endpoint_auth_signing_alg_values_supportedis currently gated purely byAllowPrivateKeyJWTRegistration, with no equivalent toHasStaticDelegateClients's OR-clause for a future statically-provisionedprivate_key_jwtclient path (e.g. SPIFFE-issued). Nothing breaks today; revisit when static/SPIFFE-provisionedprivate_key_jwtclients become possible without DCR.crypto.SupportedClientKeyAlgorithms) instead of a hand-maintained copy, with regression tests at both layers. Note: this closes the gap at the DCR-validation and discovery layers; an end-to-end EdDSA token-exchange test (proving fosite's assertion verification actually accepts an EdDSA-signedclient_assertion) was not added and would be good follow-up scrutiny.client_secret.Generated with Claude Code