Repository navigation
fix(oidc/client)!: close the security audit findings of the OIDC client - #125
Merged
Merged
Conversation
euskadi31
force-pushed
the
fix/audit2-oidc-client
branch
5 times, most recently
from
October 21, 2026 19:35
f42ecee to
8fdf04f
Compare
fmt prints a value it reaches through an unexported field of another
value by reflection, without calling its String, GoString or Format
methods. An application struct holding an oidcclient.Config,
AuthRequest, Callback, Identity or Tokens in an unexported field, or a
dereferenced *Client (%+v of *client), therefore printed the client
secret, the transaction keys, the binding secret or the tokens - and so
did slog's text handler, which prints such a struct with fmt. Holding
the configuration behind one pointer would not do: for the verbs a
pointer does not take (%s, %q, %t...) fmt reports a bad verb and prints
what the pointer points to.
Every secret of the package's API is now an oidcclient.Secret:
Config.ClientSecret, Config.TransactionKeys ([]Secret), the Binding of
AuthRequest and Callback, the three tokens of Tokens and
AuthOptions.IDTokenHint. NewSecret builds one and Reveal reads it; every
other rendering - every fmt verb, log/slog, encoding/json - is REDACTED
("" when empty). Its value sits behind a pointer to a string, which fmt
renders as an address when it reaches a Secret by reflection, whatever
the verb; encoding/gob refuses a Secret and encoding/xml writes it
empty; it does not decode from JSON. Identity and Tokens consequently no
longer encode their tokens: applications persist them by revealing
them.
BREAKING CHANGE: the fields above change type; see MIGRATION.md.
Audit: F-11.
…back http Config.InsecureCookies, meant for local development over plain http, was refused only together with form_post. With a public https RedirectURL it made the net/http helpers set the binding cookie without the __Host- prefix and the Secure attribute, so a sibling subdomain, or a network attacker answering for the site's http origin, could plant the binding cookie of a login it started itself and send the victim to the callback with its own code: the victim ended up signed in as the attacker (login CSRF, RFC 9700 section 4.7; rfc6265bis-22 sections 4.1.3.2 and 8.6). New now refuses InsecureCookies with ErrInvalidConfig, before any network access, unless RedirectURL is http - which parseRedirectURL already accepts on a loopback host only. A plain-http end-to-end test on 127.0.0.1 keeps the development setup covered. BREAKING CHANGE: InsecureCookies with an https RedirectURL now fails New. Audit: F-12.
… set An empty Config.Algorithms accepted every asymmetric algorithm the provider listed in id_token_signing_alg_values_supported. OpenID Connect Core section 3.1.3.7 step 7: "The alg value SHOULD be the default of RS256 or the algorithm sent by the Client in the id_token_signed_response_alg parameter during Registration". The key/algorithm binding already prevented an algorithm substitution, but the client accepted algorithms it never registered. An empty Algorithms now means RS256, which every provider supports (Discovery section 3); a provider signing with another algorithm needs Config.Algorithms set to the algorithm registered for the client. The errors name the option: ErrIDTokenAlgorithm at login, ErrInvalidMetadata at New when the provider does not list RS256. The stack and conformance tests that exercise other algorithms now configure them, so the ES256 conformance module still exercises ES256 verification. BREAKING CHANGE: ID Tokens signed with anything but RS256 are refused unless Config.Algorithms lists the algorithm. Audit: I-16.
…rameter RFC 9207 section 2.4: "Clients SHOULD discard authorization responses with the iss parameter from authorization servers that do not indicate their support for the parameter", and "MUST reject authorization responses without the iss parameter from authorization servers that do support the parameter according to the client's configuration". The client accepted a matching iss from a provider that did not advertise authorization_response_iss_parameter_supported, and offered no configuration to require iss. Config.RequireIssParameter declares that the provider sends iss in every response: Complete then refuses a response without it, errors included. Without it, iss stays required when the provider advertises it, and a response carrying iss from a provider that does not is now discarded with ErrIssuerParameter, whose text names the option. Distinct redirect URIs leave the mix-up gap RFC 9700 section 4.4.2.2 names: the honest provider, sending no iss, answers on the callback of the attacker's connection. The flag closes it on the connection that receives the response; an application whose tenants configure providers sets it on every tenant connection and then works only with providers that send iss. A stack test runs that attack against an attacker connection with the flag: the code never reaches the attacker. BREAKING CHANGE: a provider that sends iss without advertising RFC 9207 fails every login until RequireIssParameter is set. Audit: I-17, D-13.
oidc/client/storage/memory kept every pending login until it expired unless the application passed memory.WithCapacity, while login starts are unauthenticated: a flood of starts grew the store until the process ran out of memory. memory.New now keeps at most memory.DefaultCapacity (10 000) live transactions - about 16 login starts a second over the default 10-minute TransactionTTL, 46 MB at most with 4 KiB AppStates - and, like a store built with WithCapacity, drops the expired ones then refuses the next Save with memory.ErrFull, never evicting a pending login. WithCapacity still sets another bound. BREAKING CHANGE: a store built without WithCapacity refuses logins beyond 10 000 pending ones. Audit: D-12.
An authorization response is not authenticated: whoever learns the state of a pending login can send the victim's own browser, which presents the binding cookie on a top-level navigation, to the callback with it. That request expires the binding cookie and, with an error or a bogus code, consumes the transaction, so the genuine response fails and the user starts over. No protocol defense exists; the state must stay out of logs, yet AuthRequest printed and logged its full URL, state, nonce and hints included. String, GoString, Format and LogValue now render the URL without its query (REDACTED when it does not parse) and the state as REDACTED. The JSON encoding, the hand-off of the request to a front end, is unchanged. A cookie-jar characterization test pins the residual, which the docs now state instead of claiming that a leaked state cannot burn a login. Audit: I-14.
The client cached the provider's JWK Set for one hour: a key the provider withdrew - a leaked one - kept verifying ID Tokens for up to an hour while the jwks_uri answered, twelve times longer than the jwt module's remote key sets (DefaultCacheTTL) for the same threat. OpenID Connect Core section 10.1.1 sets no bound; the cache now lives five minutes. The 24-hour staleness limit while fetches fail is kept, the same availability trade-off as jwtsec.MaxKeySetStaleness, and the docs now state its exact risk: an attacker holding a withdrawn key and able to make the fetches fail keeps it trusted that long; rebuilding the Client drops it. A Redis failover losing a consumption stays documented, like for the other Redis stores: single use then rests on the provider's single-use code and on the binding secret, whose cookie the first callback expires. Audit: D-14.
The client verified ID Tokens through go-jose directly. go-jose verifies RSASSA-PSS with rsa.PSSSaltLengthAuto, so PS256/384/512 signatures of any salt length verified, while RFC 7518 section 3.5 says "The size of the salt value is the same size as the hash function output". It also checks an Ed25519 JWK only against a list of weak encodings: a non-canonical encoding (y not below p), whose decoding RFC 8032 section 5.1.3 makes fail, was used as a key. PS* signatures now go through a jose.OpaqueVerifier that calls rsa.VerifyPSS with PSSSaltLengthEqualsHash, the hash serving both the digest and MGF1; a JWK Set Ed25519 key must be canonical and not of small order (RFC 8725 section 3.4) or it is ignored. These are the rules of the jwt module, duplicated since oidc/client does not depend on it. The ID Token header pre-decoding is untouched. Audit: I-11, I-12 (oidc/client counterpart).
pssVerifier verified with whichever of PS256, PS384 and PS512 go-jose handed it. The algorithm comes from the JWS header, the verifier is chosen from that same header and candidates enforces one algorithm per key, so the two always agreed; binding the verifier to the algorithm it was chosen for (RFC 8725 section 3.1), as the jwt module does, makes that a property of the verifier rather than of its callers. Audit: I-12 (oidc/client counterpart).
The documentation of oidcclient.Secret overstated its reach and left out the runtime break of persisted values: - encoding/gob refuses any value holding a Secret, so a gob-encoded session holding an Identity fails to save; YAML writes a Secret empty; documents persisted before the upgrade no longer decode. MIGRATION gives the rows and a struct-of-strings recipe. - fmt prints a Secret it reaches through an unexported field as an address, not REDACTED, and a reflection dumper that dereferences pointers without calling methods (go-spew with DisableMethods, as in testify's failure diffs) prints its value; embedding a Secret makes the whole struct print REDACTED. - AuthRequest.URL carries every request parameter in clear, the id_token_hint included: its renderings drop the query, its JSON hand-off does not. The renderings test now covers the hint. The DefaultCapacity memory figure also counts acr_values. Audit: F-11, I-14, D-12.
euskadi31
force-pushed
the
fix/audit2-oidc-client
branch
from
October 21, 2026 20:57
8fdf04f to
2cf2da1
Compare
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.
Fixes the following findings of the internal security audit of 2026-10-21 (commit 1551aa6): F-11, F-12, I-14, I-16, I-17, D-12, D-13, D-14 — plus the
oidc/clientcounterpart of I-11/I-12 (PSS salt length, Ed25519 key checks), raised by thejwtPR.One commit per finding; every commit passed
make generate sync build test lint check-modules.F-11 — secrets printed through unexported fields and a dereferenced
*ClientProblem. fmt prints a value it reaches through an unexported field of another value by reflection, without calling its
Format/Stringmethods. An application struct holding anoidcclient.Config,AuthRequest,Callback,IdentityorTokensin an unexported field — or%+vof a dereferenced*Client— printed the client secret, the transaction keys, the binding secret or the tokens; slog's text handler too. Holding the configuration behind one pointer is not enough: for the verbs a pointer does not take (%s,%q…) fmt reports a bad verb and prints what the pointer points to.Fix (breaking).
oidcclient.Secret(NewSecret,Reveal) for every secret of the API:Config.ClientSecret,Config.TransactionKeys([]Secret),AuthRequest.Binding,Callback.Binding,Tokens.AccessToken/RefreshToken/IDToken,AuthOptions.IDTokenHint. Every fmt verb, slog andencoding/jsonrender itREDACTED; its value sits behind a pointer to a string, which fmt prints as an address when it reaches it by reflection (a depth-0 re-print never expands a pointer to a string). gob refuses it, xml/YAML write it empty, it does not decode from JSON.Identity/Tokenstherefore encode their tokensREDACTED. Not covered (documented): reflection dumpers that dereference pointers without calling methods (testify's diffs).Migration. MIGRATION "every secret is an
oidcclient.Secret": wrap configured secrets withNewSecret,Reveal()tokens where used; gob/YAML-persisted values and pre-upgrade JSON break — persist a struct of strings (recipe included).F-12 —
InsecureCookiesaccepted with an httpsRedirectURLProblem. With an https redirect URI the binding cookie lost
__Host-andSecure: a sibling subdomain or a network attacker answering for the http origin could plant the binding cookie of its own login and sign the victim in as the attacker (login CSRF).Fix (breaking).
NewrefusesInsecureCookies(ErrInvalidConfig, before any network access) unlessRedirectURLis http — accepted on a loopback host only. RFC 9700 §4.7; rfc6265bis-22 §4.1.3.2, §8.6.I-14 — a leaked state burns the pending login
Problem. An authorization response is not authenticated: with the state, a request to the callback through the victim's own browser expires the binding cookie (and with an error or a bogus code consumes the transaction). No protocol defense exists.
Fix.
AuthRequestString/GoString/Format/LogValue drop the state and the URL query (hints included); its JSON hand-off is unchanged. The residual is documented (LIMITATIONS, security-considerations) and pinned by a cookie-jar test.I-16 — every asymmetric algorithm accepted by default
Fix (breaking). An empty
Config.Algorithmsnow means RS256 — OpenID Connect Core §3.1.3.7 step 7: "The alg value SHOULD be the default of RS256 or the algorithm sent by the Client in the id_token_signed_response_alg parameter during Registration"; every provider supports RS256 (Discovery §3). Errors name the option. Migration: setConfig.Algorithmsfor a provider signing with another algorithm.I-17 + D-13 — RFC 9207
isspolicy; mix-up residual without RFC 9207Problem. A matching
issfrom a provider not advertisingauthorization_response_iss_parameter_supportedwas accepted (RFC 9207 §2.4: "SHOULD discard"), and nothing could requireiss; without it, distinct redirect URIs leave the gap RFC 9700 §4.4.2.2 names.Fix (breaking). New
Config.RequireIssParameter("according to the client's configuration", §2.4):issrequired and compared on every response. Without it,issfrom a non-advertising provider is discarded (ErrIssuerParameter, text names the option). It protects the connection that receives the response: an application whose tenants configure providers sets it on every tenant connection. A stack test runs the §4.4.2.2 attack against an attacker connection with the flag: the honest code never reaches the attacker. Migration: set the flag for a provider that sendsisswithout advertising it (which real providers do is unknown).D-12 — in-memory transaction store unbounded by default
Fix (breaking).
memory.DefaultCapacity = 10_000applied bymemory.New;WithCapacityoverrides; a full store refuses withErrFull, never evicting a pending login.D-14 — withdrawn provider key trusted for a while; Redis failover
Fix. JWK Set TTL 1 h → 5 min (the
jwtmodule'sDefaultCacheTTL; OpenID Connect Core §10.1.1 sets no bound). The 24 h staleness while fetches fail stays — thejwttrade-off — and its exact risk is documented with the remedy (rebuild theClient). Redis failover losing a consumption: documented, like the other Redis stores (single use then rests on the provider's single-use code and the binding secret).oidc/clientcounterpart of I-11 / I-12PS256/384/512 ID Tokens are verified with a salt as long as the hash (RFC 7518 §3.5) through a
jose.OpaqueVerifierbound to its algorithm; JWK Set Ed25519 keys that are not canonical (RFC 8032 §5.1.3) or of small order (RFC 8725 §3.4) are ignored. Same rules asjwt.Tests
TestSecretsNeverPrintleaked under%q/%x/gob,TestNewRejectsInvalidConfig/insecure_cookies_with_an_https_redirectgot nil,TestCompleteIssParameter/not_advertised_and_equal…got nil,TestStoreIsBoundedByDefaultstored the 10 001st,TestKeySetTTLstill cached at 5 min,TestIDTokenPSSSaltLengthverified every salt).!= "http"vs== "https", no third scheme admitted). The independent code review ran 31 more: all killed.Checks
make generate sync build test lint check-modules: green (lint 0 issues,make syncleaves no diff).oidc/clientoidc/client/storage/memoryoidc/client/storetestNot run:
make test-backends(no SQL/Redis store touched),make vuln(no dependency change),make conformance(opt-in OIDF suite; the ES256 module now configuresAlgorithms: [ES256], the static registration's algorithm, and keeps its expected outcome).Follow-ups
make conformance(RP plans) before the release.Notes for the maintainer
CLAUDE.md(OpenID Connect client invariants) becomes stale on the JSON encoding ofIdentity/Tokens; the proposed text is held for your consent.