Validate subject tokens from external OIDC issuers#5995
Conversation
Introduce a SubjectTokenValidator interface with a single Validate method, preparing for multi-issuer token validation where external OIDC tokens (e.g., Keycloak) need different JWKS resolution than self-issued tokens. Rename the existing concrete struct to SelfIssuedTokenValidator to clarify its role. The handler now depends on the interface, not the concrete type, following Go's "accept interfaces, return structs" pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implement MultiIssuerTokenValidator that routes JWT validation based on the token's issuer claim. Self-issued tokens delegate to the existing SelfIssuedTokenValidator. External tokens (e.g., Keycloak) are validated against the issuer's JWKS, fetched via OIDC discovery with caching. Security: JWKS URLs from OIDC discovery are validated to require HTTPS and reject private/loopback addresses (SSRF prevention). Discovered URLs are re-resolved when the JWKS cache expires. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address panel review feedback on the multi-issuer validator: NewMultiIssuerTokenValidator silently accepted a TrustedIssuer with no ExpectedAudience instead of rejecting it, and subject-token validation failures returned invalid_grant instead of invalid_request. Also drain response bodies before closing on non-200 discovery/JWKS fetches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address code-review findings on the multi-issuer subject-token validator. Fix the self-issued routing test, which handed the private JWKS to the validator instead of the public set. Require an "exp" claim on external tokens so the delegated token can be bounded. Validate that the OIDC discovery document's issuer matches the expected issuer. Harden JWKS fetching against SSRF: validate redirects and resolved IPs at dial time (blocking loopback, private, link-local and unspecified addresses), not just the pre-request URL string. Reject empty key sets and cap the accepted key count. Drop dead test helpers and route JWKS handlers through the existing public-JWKS helper. Add tests for the empty-audience constructor error, discovery failures, and kid mismatch. External-token delegation consent, error-code mapping and clock-skew leeway are deferred to #5989 and flagged with TODOs, since the validator is not yet wired into Factory.
JAORMX
left a comment
There was a problem hiding this comment.
Reviewed with a focus on the SSRF surface and token-validation correctness. The security-critical design is sound and — importantly — I confirmed the feature is genuinely inert: factory.go still constructs only NewSelfIssuedTokenValidator, and NewMultiIssuerTokenValidator is referenced nowhere in non-test code, so the external-issuer path is unreachable until the #5989 wiring lands. TODO(#5989) points at a tracked open issue. Approving on that basis.
Verified correct:
- DNS-rebinding defense is real —
DialContextresolves, rejects if any resolved IP is disallowed, then dials the vetted IP directly (no re-resolve/TOCTOU).isDisallowedIPcovers loopback/link-local/private/unspecified across IPv4+IPv6, so169.254.169.254is blocked. - Redirects re-check HTTPS per hop and re-run the IP check via
DialContext; body reads areio.LimitReader-capped on both discovery and JWKS;maxJWKSKeyscap + empty-set rejection. - Token path: signature verified against issuer JWKS before any claim is trusted; issuer-of-record from config (not the token body); audience enforced; exp/nbf checked;
allowedSignatureAlgorithmsexcludesnoneand HMAC (no alg-confusion). Fail-closed on emptyExpectedAudience, unknown issuer, and all error paths.
Two should-fix items before this is wired up in #5989 (safe to land here or there — not exploitable while inert):
- The SSRF controls have no test coverage. Every test sets
insecureSkipJWKSURLValidation = true, sovalidateJWKSURL,isDisallowedIP, the dial-time IP check, and the redirect scheme re-check are never exercised. Please add tests (flag leftfalse) asserting refusal of: anhttp://JWKS URL, a discovery doc whosejwks_uriresolves to loopback/link-local/IPv6, and a 302 redirect to a private IP. This is security-critical code that can currently regress silently. isDisallowedIPmisses CGNAT100.64.0.0/10.net.IP.IsPrivate()doesn't include it, so a hostile trusted-issuer discovery doc could pointjwks_uriat e.g. Alibaba metadata100.100.100.200. Add an explicit100.64.0.0/10check (consider192.0.0.0/24,198.18.0.0/15too).
Minor nits (optional): the initial hop (configured IssuerURL/preconfigured JWKSURL) isn't HTTPS-checked — only discovered URLs are — so an http:// issuer config downgrades to cleartext on the first request; and ips[0] at the end of DialContext would panic on the (practically unreachable) empty-non-error LookupIPAddr result — a len(ips)==0 guard is cheap.
Heads-up unrelated to this PR: main is currently red from a nil-panic in pkg/vmcp/client (a #5990 struct-literal test hitting the new #5980 resolveAuthStrategy path), so your Test Go Code gate may fail on that until main is fixed — it's not from anything here.
Summary
Token exchange (#5812, #5813) currently only accepts subject tokens that the embedded authorization server issued itself. To let agents act on behalf of users authenticated by an external IdP (Keycloak, Entra, Okta), the AS needs to validate subject tokens from trusted external OIDC issuers — the trust-model groundwork for the federated token-exchange path in #5194.
This PR adds that validation, but deliberately does not enable it in production:
factory.gostill constructs the self-issued validator, so external tokens remain unreachable until the delegation-consent gate (#5989) lands. ATODO(#5989)on the type documents the constraint.SubjectTokenValidatorinterface; rename the concrete self-issued type toSelfIssuedTokenValidator(accept interfaces, return structs).MultiIssuerTokenValidator: routes self-issued tokens to the existing validator, and validates external-issuer tokens by resolving the issuer's JWKS.JWKSURLis configured.TrustedIssueris configured without anExpectedAudience.Closes #5814
Type of change
Test plan
task test) — not run locally; run in CINew
multi_issuer_validator_test.gocovers issuer routing, JWKS caching/expiry, OIDC discovery fallback, SSRF rejection (private/loopback IPs, non-HTTPS), oversized responses, and the empty-ExpectedAudienceguard. Locallygo build ./...andgo vet ./pkg/authserver/...are green.Does this introduce a user-facing change?
No. The validator is not wired into the token endpoint in this PR — external subject tokens are not accepted until #5989 adds the consent model and enables it in
factory.go.Special notes for reviewers
factory.gohere — external tokens carry noclient_id, socheckDelegationConsentwould fail them closed, and enabling them without the consent gate is a confused-deputy (CWE-863) risk.NewMultiIssuerTokenValidator's HTTP client (DialContextIP checks +CheckRedirect), and the JWKS cache locking inresolveJWKS.🤖 Generated with Claude Code