Allow loopback delegate clients - #6426
Merged
Merged
Conversation
The CEL rule on EmbeddedAuthServerConfig rejected every http:// issuer whenever delegateClients were configured. That blocked the supported local and Kind token-exchange path, where the embedded auth server issues on http://127.0.0.1 and the operator has already opted in through insecureAllowConfidentialOverLoopbackHTTP. Replace the categorical rule with one that admits an http:// issuer only when that opt-in is explicitly set, and leave the loopback-host check to the shared Go validator. CEL has no URL parser, so it cannot separate http://127.0.0.1 from http://auth.example.com without a regex that risks admitting a non-loopback host. Tighten ValidateConfidentialClientTransport to carry that weight. It previously short-circuited to nil as soon as the loopback opt-in was set, and returned nil for a plain-HTTP non-loopback issuer, deferring both to validateIssuerURL at pod startup. It now parses the issuer once and rejects a non-loopback plain-HTTP issuer whether or not the opt-in is set, so the misconfiguration surfaces as a reconcile error instead of a crashloop. Nothing that previously reconciled cleanly is newly rejected. Drop the envtest that asserted the removed CEL message, since the same combination is now rejected at reconcile rather than at admission. Fixes #6423
jhrozek
requested review from
ChrisJBurns,
JAORMX,
blkt,
jerm-dro,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
August 25, 2026 08:23
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6426 +/- ##
==========================================
+ Coverage 77.76% 77.78% +0.02%
==========================================
Files 759 760 +1
Lines 72979 73013 +34
==========================================
+ Hits 56750 56793 +43
+ Misses 16224 16215 -9
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rdimitrov
previously approved these changes
Aug 25, 2026
samuv
requested changes
Aug 25, 2026
samuv
left a comment
Contributor
There was a problem hiding this comment.
Summary
This PR correctly opens the intended loopback delegate-client path, and the well-formed loopback/non-loopback host policy is sound. However, malformed HTTP issuers can pass admission and reconciliation before failing inside the workload, while the VirtualMCPServer validation path recommends enabling a flag that the confidential-client policy forbids. The public field documentation and negative admission coverage also need to be updated, so I am requesting changes.
Checklist
- Tests: CI is green, but negative coverage for the new delegate CEL rule and malformed-issuer reconciliation is missing.
- Docs: Generated artifacts are current, but the source and generated flag descriptions still document the previous behavior.
- Registry impact: None.
- Security: Well-formed loopback enforcement is sound; malformed issuer handling currently fails open during reconciliation.
- Backwards compatibility: The schema relaxation is additive, and CRD compatibility checks pass.
samuv
reviewed
Aug 25, 2026
samuv
left a comment
Contributor
There was a problem hiding this comment.
Follow-up summary
The author addressed the VirtualMCP validation ordering, stale source/generated documentation, and missing negative CEL coverage. The malformed-issuer fix is only partial: it closes the exact parse-error path, but wrapping the raw net/url error can expose URL credentials, and parseable issuers that violate the runtime contract can still reconcile successfully before crashing at startup. The existing changes-requested verdict remains appropriate.
Checklist
- Tests: The required Go test job currently fails because two new assertions expect
invalid issuer URLwhile production returnsrequire a valid issuer URL. - Docs: Addressed and regenerated.
- Registry impact: None.
- Security: Well-formed loopback enforcement remains sound; raw parse-error propagation must be sanitized.
- Backwards compatibility: No new schema-compatibility issue found.
samuv
approved these changes
Aug 25, 2026
samuv
left a comment
Contributor
There was a problem hiding this comment.
The latest commit addresses my previously requested changes: malformed issuer errors are now sanitized, the loopback HTTP opt-in path applies full issuer validation, and regression coverage exercises both behaviors. The updated Go test job passes.
jhrozek
added a commit
that referenced
this pull request
Aug 26, 2026
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.
jhrozek
added a commit
that referenced
this pull request
Aug 26, 2026
* Add private-key JWT registration gate
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.
* Validate private-key JWT registration
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.
* Build private-key JWT DCR clients
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.
* Persist private-key JWT client metadata
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.
* Advertise private-key JWT authentication
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.
* Test private-key JWT token exchange
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.
* Exercise private-key JWT operator flow
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 private-key JWT registration
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.
* Harden private-key JWT DCR validation
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.
* Validate registered-client key algorithms
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.
* Unify the client-key signing-algorithm allowlist
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.
* Regenerate swagger docs for the new DCR config field
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.
* Reject EdDSA, mislabeled, and weak client keys at DCR
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.
* Make client-assertion replay atomic and bound its cost
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.
* Bound the private_key_jwt e2e test's HTTP calls
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.
* Remove the private_key_jwt registration transport gate
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.
* Regenerate CRD/swagger docs after rebasing onto main
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.
* Fix double t.Parallel() panic in concurrent replay tests
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.
* Fix stale error-substring assertion in transport test
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.
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
3 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
Type of change
Test plan
task test)task test-e2e)task lint-fix)Verified the branch diff with
git diff --check origin/main...HEAD; it completed without whitespace errors.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.gopkg/authserver/config.gocmd/thv-operator/api/v1beta1/*_test.go,pkg/authserver/config_test.gocmd/thv-operator/config/crd/bases/*,deploy/charts/operator/crds/*docs/operator/crd-api.mdDoes this introduce a user-facing change?
Yes. Configurations with delegate clients may now use a loopback HTTP issuer only when
insecureAllowConfidentialOverLoopbackHTTPis explicitly enabled; non-loopback HTTP issuers remain rejected.Special notes for reviewers
The CEL rule deliberately admits only the explicit opt-in. The shared Go transport validator performs URL parsing and the precise loopback-host validation, which CEL cannot safely express.
Generated with Claude Code