Repository navigation
fix!: harden JWKS rotation, typ, issuer/redirect URL and Client-Cert parsing, kid-less client keys - #777
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ptions JWKSIssuerKeySource, ClientJWKSSource and ephemeral.ClientKeySource rate-limited the unknown-kid forced refetch against the last fetch of any kind, with the interval defaulting to the cache TTL. Every TTL refresh reset the limit, so with the defaults an unknown kid never forced a refetch and a rotated-in key was refused until the cache expired (OIDC Core 10.1.1 expects the verifier to go back to the jwks_uri on an unfamiliar kid). The limit now counts only forced refetches, and defaults to min(cache TTL, 1 minute). A miss on a set fetched since the request arrived is negative-cached instead of refetched, so a cold start with an unknown kid still costs one fetch. Tests now cover the default options too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TypeIs lowercased typ with strings.ToLower, whose Unicode case mapping folds the Kelvin sign U+212A onto "k", so "trust-marK+jwt" matched "trust-mark+jwt". RFC 7515 4.1.9 compares typ as an RFC 2045 media type, case-insensitive in ASCII. Use canonical.LowerASCII, as the host and scheme comparisons already do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
v0.65.0 made fapi.ParseIssuerURL refuse an issuer with a query, as RFC 8414 section 2 and OpenID Connect Core section 2 require, but server.New and client.New accepted any fapi.URL as Config.Issuer. An issuer built with fapi.ParseEndpointURL, which allows a query, was still accepted in production, and the server published and signed it. Both constructors now refuse an issuer with a query, even an empty "?", at either assurance level. BREAKING CHANGE: server.New and client.New refuse a Config.Issuer with a query, including an empty "?". Parse the issuer with fapi.ParseIssuerURL and move any tenant selector into the path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RFC 3986 section 3.5 makes everything after the first "#" a fragment component, an empty one included, and RFC 8414 section 2 and RFC 6749 section 3.1.2 rule one out of an issuer and a redirect URI. The fapi.Parse*URL parsers refused a non-empty fragment but parsed a bare "#" away, silently rewriting the value, and storage.NewRegisteredClient checked a web client's redirect URIs for no fragment at all. The parsers now refuse any "#", and NewRegisteredClient refuses a redirect URI containing one. BREAKING CHANGE: fapi.ParseIssuerURL, ParseEndpointURL and ParseRedirectURL refuse a URL ending in a bare "#", and storage.NewRegisteredClient (and so sqlstore) refuses a registered redirect URI containing "#". Drop the "#" from configuration and from each client's registered redirect URIs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nt-Cert RFC 9440 section 2.2 makes Client-Cert a Byte Sequence, and RFC 9651 section 2.3 lets every Item carry Parameters as an extensibility mechanism, discouraging a field from treating an unrecognized one as an error. ForwardedClientCertificate refused any value with parameters, so a TLS-terminating proxy adding one had every mTLS request refused. It now parses the value as an RFC 9651 Item: a Byte Sequence followed by any valid parameters, which are ignored. Anything else is still ErrMalformedForwardedCertificate. The RFC 8941 citations now name RFC 9651, which obsoletes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A VerificationKey registered without a KeyID, given to NewStaticClientKeys or found in a client's JWK Set, matched only a request naming no kid. NewKeyManagerFromSigners derives a kid (the key's RFC 7638 thumbprint) for a key given without one, so a client assertion signed by such a key manager never matched the same key registered without a kid, and failed with invalid_client. A kid-less key now also matches a request naming its thumbprint, and no other kid. VerificationKey.KeyID now documents the matching rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
osanderson
force-pushed
the
fix/keys-urls-hardening
branch
from
November 2, 2026 07:01
afc9066 to
da49bdf
Compare
One case per way a parameter's bare item can be malformed: a sign with no digits, a bad or control character in a String, a non-base64 or unterminated Byte Sequence, a Boolean other than 0 or 1, and a Display String without its quote, with an uppercase or unterminated escape, or with a control character. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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
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.
Hardening of key resolution and URL parsing, one commit per fix. Two are breaking, with sections under
## v0.69.0in UPGRADING.md.Key rotation picked up with default options (keys). Before this change,
JWKSIssuerKeySource,ClientJWKSSourceandephemeral.ClientKeySourcechecked the unknown-kid forced refetch against the time of the last fetch of any kind. The interval also defaulted to the cache TTL. Every TTL refresh reset the limit, so with the defaults an unknown kid never forced a refetch, and a rotated-in key was refused until the cache expired (OIDC Core §10.1.1). Now:min(cache TTL, 1 minute).WithMinRefreshIntervaldocs are corrected, and tests cover the default options.ASCII-only
typcomparison (internal/jose).TypeIslowercased with Unicode case mapping, so the Kelvin sign U+212A matchedk. It now usescanonical.LowerASCII.Issuer with a query refused by
server.New/client.New(breaking). v0.65.0 madefapi.ParseIssuerURLrefuse a query, but both constructors accepted anyfapi.URL. An issuer built withParseEndpointURL("…?tenant=a")was still accepted in production, and the server published and signed it. Both constructors now refuse a query, even an empty?, at either assurance level (RFC 8414 §2, OIDC Core §2).Empty fragment refused (breaking). Before this change:
fapi.ParseIssuerURL,ParseEndpointURLandParseRedirectURLrefused a non-empty fragment but parsed a bare#away.storage.NewRegisteredClientdidn't check a web client's redirect URIs for a fragment at all.Now the parsers refuse any
#, andNewRegisteredClientrefuses a redirect URI that contains one (RFC 3986 §3.5, RFC 6749 §3.1.2, RFC 8414 §2).Client-Cert parameters (fapihttp). RFC 9651 §2.3 lets every Item carry Parameters.
ForwardedClientCertificatenow accepts and ignores valid parameters after the Byte Sequence. Anything that isn't a valid Item with a Byte Sequence bare item is still refused. The fuzz oracle gains an independent grammar for the parameters, and the RFC 8941 citations now name RFC 9651.Kid-less client keys (keys). Before this change, a
VerificationKeyregistered without aKeyID, directly or in a client's JWK Set, matched only a request naming no kid. Assertions fromNewKeyManagerFromSigners, which always derives a kid, therefore failed withinvalid_client. Such a key now also matches a request naming its RFC 7638 thumbprint, and no other kid.VerificationKey.KeyIDdocuments the rule.Each fix has regression tests that fail without it. Checked at go1.26.10:
go test -race ./...,go vet,golangci-lintand govulncheck are clean, every example module's tests pass, andcmd/...builds.🤖 Generated with Claude Code