From cd3c20b67e3b4e150eca65ad9802093b964e3bce Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 10:37:14 +0200 Subject: [PATCH 01/10] feat(oidc/client)!: hold every secret in a redacted Secret type 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. --- CHANGELOG.md | 20 +- LIMITATIONS.md | 25 +-- MIGRATION.md | 59 +++++- docs/observability.md | 4 +- docs/oidc-client.md | 12 +- docs/security-considerations.md | 17 +- examples/oidc-client/main.go | 16 +- examples/oidc-client/main_test.go | 18 +- examples/oidc-client/ratelimit_test.go | 5 +- .../interop/oidcrp/metadata_corpus_test.go | 6 +- .../interop/oidcrp/zitadel_options_test.go | 4 +- internal/interop/oidcrp/zitadel_test.go | 10 +- .../interop/oidcstack/concurrency_test.go | 2 +- .../interop/oidcstack/conformance/rp_test.go | 4 +- internal/interop/oidcstack/flow_test.go | 12 +- internal/interop/oidcstack/harness_test.go | 6 +- internal/interop/oidcstack/replay_test.go | 9 +- internal/interop/oidcstack/rotation_test.go | 2 +- .../interop/oidcstack/substitution_test.go | 12 +- oidc/client/begin.go | 24 +-- oidc/client/begin_test.go | 12 +- oidc/client/callback.go | 7 +- oidc/client/callback_test.go | 16 +- oidc/client/config.go | 57 ++---- oidc/client/config_test.go | 22 +-- oidc/client/discovery_test.go | 17 +- oidc/client/doc.go | 5 + oidc/client/fakeop_test.go | 14 +- oidc/client/format.go | 3 +- oidc/client/http.go | 6 +- oidc/client/identity.go | 28 +-- oidc/client/idtoken.go | 4 +- oidc/client/integrity_test.go | 10 +- oidc/client/params_test.go | 2 +- oidc/client/redaction_test.go | 55 ++++-- oidc/client/secret.go | 86 +++++++++ oidc/client/secret_test.go | 181 ++++++++++++++++++ oidc/client/secrets.go | 4 +- oidc/client/token.go | 10 +- oidc/client/token_test.go | 4 +- oidc/client/userinfo.go | 4 +- oidc/client/userinfo_test.go | 2 +- 42 files changed, 588 insertions(+), 228 deletions(-) create mode 100644 oidc/client/secret.go create mode 100644 oidc/client/secret_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a945951..e71b912d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1027,6 +1027,14 @@ legacy packages (`authentication/`, `authorization/`, the in-tree every 64 writes instead of keeping them for the process lifetime. `memory.New` takes options; `memory.WithClock` gives it the ServiceProvider's clock (`time.Now` by default). +- **`oidcclient.Secret`** (breaking): every secret of the `oidc/client` + API — `Config.ClientSecret`, `Config.TransactionKeys` (now `[]Secret`), + `AuthRequest.Binding`, `Callback.Binding`, `Tokens.AccessToken`, + `RefreshToken` and `IDToken`, `AuthOptions.IDTokenHint` — is a `Secret`: + `NewSecret(s)` builds one, `Reveal()` reads it, and it prints, logs and + encodes as `REDACTED` everywhere, `encoding/json` included: `Identity` + and `Tokens` no longer encode their tokens, which applications persist by + revealing them (see MIGRATION.md). - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in @@ -1536,8 +1544,8 @@ legacy packages (`authentication/`, `authorization/`, the in-tree redacted, transaction keys counted), whose encoding used to fail on its `func` fields only by accident. `UserInfo` printed its claims (personal data) with `%s` and `%q`; it now prints and logs its subject only. - `Identity` and `Tokens` still encode their tokens, deliberately, so that - applications can persist them (see MIGRATION.md). + `Identity` and `Tokens` encoded their tokens, for persistence, until they + became `oidcclient.Secret` values (see Changed). - `oidcclient.AuthOptions.ExtraParams` compared its names with the reserved parameters exactly: `Request`, `STATE`, `Nonce`, `request.uri`… passed, and a provider that ignores case (ASP.NET) or rewrites `.` as `_` @@ -2214,3 +2222,11 @@ legacy packages (`authentication/`, `authorization/`, the in-tree servers could not tell it from the client's `client_credentials` token (RFC 9068 §2.2, RFC 9700 §4.15). It now answers `server_error` (cause `oauth2.ErrSubjectIsClient`), nothing minted or persisted. +- `oidc/client` secrets printed in clear through `fmt` when an `oidcclient` + value sat in an unexported field of another value, or when a `Client` was + dereferenced (`%+v` of `*client`): fmt prints such values by reflection, + without calling their redacting methods, so a `Config`'s client secret + and transaction keys, an `AuthRequest`'s or `Callback`'s binding secret + and the tokens of an `Identity` reached logs — `slog`'s text handler + included. They are now `oidcclient.Secret` values (see Changed), which + print as an address there and `REDACTED` everywhere else, with every verb. diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 80f5fb30..6b95c3b5 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -683,10 +683,9 @@ PKCE. Not supported, and refused where a provider could select them: ## Logging - **What `fmt` cannot redact** — the types that hold secrets - (`oidcclient.Config` and `*Client`, `oauth2.DefaultClient`, the `jwtsec` - keys, signers and key sets, `session.Codec` and `session.Session`, the - `basic`, `bearer` and `session` authentications, `security.AttemptKey`…) - print redacted with + (`oauth2.DefaultClient`, the `jwtsec` keys, signers and key sets, + `session.Codec` and `session.Session`, the `basic`, `bearer` and + `session` authentications, `security.AttemptKey`…) print redacted with every `fmt` verb and in `slog` output — as do the `grpcsec` and `connectrpcsec` carriers, which print metadata key counts only — but fmt prints some values without @@ -694,15 +693,19 @@ PKCE. Not supported, and refused where a provider could select them: value held in an unexported field of your own types, and — for the verbs a pointer does not take (`%s`, `%q`…) — what a nested pointer points to. Log these types directly or through exported fields, with `%v`. + `oidc/client` holds every credential in an `oidcclient.Secret`, which + stays redacted even there; what such a path still prints of its values is + not a credential: a `Callback`'s parameters (the authorization code, + useless without the PKCE verifier derived from the binding secret, and + the state), an `AuthRequest`'s URL and state, an `Identity`'s claims. - **JSON** — `slog`'s JSON handler calls `LogValue` for attribute values only: a value nested in another one (a struct field, a map) is encoded - with `encoding/json`. `oidcclient.Config`, `AuthRequest` and `Callback`, - and `session.Session`, encode without their secrets, but - `oidcclient.Identity` and `Tokens` encode their tokens — deliberately, so - that applications can persist them — and `UserInfo` its claims: log these - as attributes of their own. Other reflection-based encoders - (`encoding/gob`, `encoding/xml`, YAML) write every exported field, an - `AuthRequest`'s binding secret included. + with `encoding/json`. `oidcclient` values and `session.Session` encode + without their secrets — an `oidcclient.Secret` encodes `REDACTED` — but + `oidcclient.Identity` encodes its claims and `UserInfo` its claims + (personal data): log these as attributes of their own. Other + reflection-based encoders (`encoding/gob`, `encoding/xml`, YAML) write + every exported field; an `oidcclient.Secret` has none. - **Credentials and tokens in flight are not redacted yet** — the `oauth2` records (`AuthorizationCode`, `AccessToken`, `RefreshToken`, the `GrantResponse` holding them) print the raw code and tokens they carry diff --git a/MIGRATION.md b/MIGRATION.md index 44e625d7..f1e68bfa 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -792,7 +792,8 @@ To migrate: (`openssl rand -hex 32`). It is a secret like a session key: keep it in your secret store, never in code or logs. 2. Give the same keys to every instance that may serve a callback, and set - `Config.TransactionKeys: [][]byte{key}` on every `oidcclient.Client` + `Config.TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(key))}` + on every `oidcclient.Client` (`examples/oidc-client` reads `OIDC_TRANSACTION_KEYS`, hex keys separated by commas). 3. Deploy knowing that the logins in progress at that moment fail once with @@ -918,7 +919,7 @@ like `Config`: | `Config` | an error (`json: unsupported type: func…`) | `{"ConnectionID":…,"Issuer":…,"ClientID":…,"ClientSecret":"REDACTED","AuthMethod":…,"RedirectURL":…,"ResponseMode":…,"TransactionKeys":}` | | a struct **embedding** `AuthRequest` or `Callback` | its own fields and the embedded ones | the promoted `MarshalJSON`: the embedded value's redacted document alone — the struct's other fields are not written; when another type embedded at the same depth also has a `MarshalJSON`, neither is promoted and the exported fields are written, the secrets excluded | | `json.Unmarshal` into `AuthRequest`, `Callback` | every field read | `AuthRequest.Binding` and `Callback`'s `URL`, `Params` and `Binding` are tagged `json:"-"`: never read nor written by a reflective encoding | -| `Identity`, `Tokens` | tokens and claims | unchanged, deliberately: JSON is how applications persist them | +| `Identity`, `Tokens` | tokens and claims | claims unchanged; tokens `REDACTED` since they are `oidcclient.Secret` values (see "every secret is an `oidcclient.Secret`" below) | | `UserInfo` | its claims | unchanged; `fmt` now prints its subject only | | `*Client` | `{}` | unchanged | @@ -934,10 +935,11 @@ To migrate: another embedded type with a `MarshalJSON`, as flattened fields without the secrets: name the field (`Login oidcclient.AuthRequest`) instead of embedding it. -4. Other reflection-based encoders (`encoding/gob`, `encoding/xml`, YAML) - still write the binding secret: do not use them on these types. -5. Keep logging an `Identity` or `Tokens` as an attribute of its own: nested - in another logged value, `slog`'s JSON handler writes their tokens. +4. Other reflection-based encoders never write the binding secret either: + it is an `oidcclient.Secret`, which `encoding/gob` refuses and + `encoding/xml` writes empty. +5. Keep logging an `Identity` as an attribute of its own: nested in another + logged value, `slog`'s JSON handler writes its claims (personal data). ### `oidc/client`: an authorization request never repeats a parameter @@ -1831,6 +1833,7 @@ separator, and the three hint lists are bounded: To migrate: nothing for clients sending ordinary values. A client sending more than 2048 bytes in one of these hints sends fewer: they never fail a request (OIDC Core §3.1.2.1, §15.1). + ### `oauth2`: request sizes and parameters are bounded The endpoints parsed requests of any size up to the 10 MB `net/http` form @@ -3012,3 +3015,47 @@ To migrate: keys through the metadata (RFC 9068 §4), lose them. 2. List the scopes clients may request in `ServerConfig.ScopesSupported` (optional, RECOMMENDED by RFC 8414 §2). + +### `oidc/client`: every secret is an `oidcclient.Secret` + +`fmt` prints a value it reaches through an unexported field of another +value by reflection, without calling any of its methods: an application +struct holding an `oidcclient.Config`, `AuthRequest`, `Callback`, +`Identity` or `Tokens` in an unexported field — or a dereferenced +`*Client` (`fmt.Sprintf("%+v", *client)`) — 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`. Every secret of the +package's API is now an `oidcclient.Secret`: `NewSecret(s)` builds one, +`Reveal()` returns its value, and every other rendering — every `fmt` verb, +`slog`, `encoding/json`, and a `Secret` reached through an unexported field, +whose value sits behind a pointer fmt prints as an address — shows +`REDACTED` (`""` when empty). `encoding/gob` refuses a `Secret` and +`encoding/xml` writes it empty; a `Secret` does not decode from JSON. + +| Field | Before | After | +| --- | --- | --- | +| `Config.ClientSecret` | `string` | `Secret` — `oidcclient.NewSecret(secret)` | +| `Config.TransactionKeys` | `[][]byte` | `[]Secret` — `oidcclient.NewSecret(string(key))` per key | +| `AuthRequest.Binding` | `string` | `Secret` — `ar.Binding.Reveal()` to hand it to the user agent | +| `Callback.Binding` | `string` | `Secret` — `oidcclient.NewSecret(cookie.Value)` | +| `Tokens.AccessToken`, `RefreshToken`, `IDToken` | `string` | `Secret` — `Reveal()` to call an API or store them | +| `AuthOptions.IDTokenHint` | `string` | `Secret` — `IDTokenHint: id.Tokens.IDToken` needs no conversion | +| `json.Marshal` of an `Identity` or `Tokens` | tokens written, for persistence | tokens `"REDACTED"`; `json.Unmarshal` of such a document into an `Identity` fails | +| `fmt` of a `Config`, `AuthRequest`, `Callback`, `Identity`, `Tokens` or `Client` reached through an unexported field, `%+v` of `*client` | secrets printed | secrets printed as an address | + +To migrate: + +1. Wrap the configured secrets: `ClientSecret: oidcclient.NewSecret(secret)` + and `TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(key))}`. + A public client leaves `ClientSecret` zero (`oidcclient.Secret{}`). +2. With `Begin`/`Complete` instead of the net/http helpers, put + `ar.Binding.Reveal()` in the user agent's cookie and pass + `oidcclient.NewSecret(cookie.Value)` as `Callback.Binding`. +3. Read the tokens with `Reveal()` where they are used (`Authorization: + Bearer`, a refresh request) and pass `id.Tokens.IDToken` as + `AuthOptions.IDTokenHint` unchanged. +4. An application persisting an `Identity` or its `Tokens` as JSON stores + the revealed tokens explicitly, in storage fit for credentials: the + encoding of `Identity` and `Tokens` now writes them `REDACTED`. +5. Compare two `Secret`s with `crypto/subtle.ConstantTimeCompare` on their + revealed values, never with `==` (which compares their identity). diff --git a/docs/observability.md b/docs/observability.md index 3658231c..22571b5d 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -32,8 +32,8 @@ through the OAuth2 error hook described below. The OpenID Connect client (`oidc/client`, its stores) and the core's `outbound` package open no spans either: every failure is returned to the caller as a sentinel error (`errors.Is`) that carries no secret. -`Config`, `*Client`, `Callback`, `AuthRequest`, `Identity` and `Tokens` -redact their secrets with every `fmt` verb and in `slog` output, and +Every credential of `oidc/client` is an `oidcclient.Secret`, which prints, +logs and encodes as `REDACTED` wherever it sits, and `Identity` and `UserInfo` log their identifiers only (`LogValue`). Log the error of `Complete` or `HandleCallback`. diff --git a/docs/oidc-client.md b/docs/oidc-client.md index b38406cd..15f8bf7a 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -90,12 +90,13 @@ client, err := oidcclient.New(ctx, oidcclient.Config{ ConnectionID: "acme", Issuer: "https://login.acme.example", ClientID: "my-client", - ClientSecret: secret, + ClientSecret: oidcclient.NewSecret(secret), AuthMethod: oidcclient.AuthMethodClientSecretBasic, RedirectURL: "https://app.example/oidc/acme/callback", Scopes: []string{"profile", "email"}, - Store: store, // sqlstore or redisstore with several instances - TransactionKeys: [][]byte{key}, // 32+ secret bytes, shared by every instance + Store: store, // sqlstore or redisstore with several instances + // 32+ secret bytes, shared by every instance + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(key))}, }) mux.HandleFunc("POST /login/acme", func(w http.ResponseWriter, r *http.Request) { @@ -156,6 +157,11 @@ at several issuers defeats the distinct redirect URIs). compares the URL the browser used) or call `Complete` with the public URL. - After a successful callback, create a fresh session (a new ID) and answer 303 to a clean URL, so the code leaves the browser history. +- Every credential — client secret, transaction keys, binding secret, + tokens — is an `oidcclient.Secret`: it prints, logs and encodes as + `REDACTED` wherever it sits (`TestSecretsNeverPrint`), and `Reveal()` + returns it where it is used. An application persisting tokens reveals + them explicitly: `Identity` and `Tokens` encode them `REDACTED`. - Show users a generic message on failure: `AuthorizationError` and `TokenError` carry the provider's untrusted text, and errors of `New` may name an allowed destination's address. Provider-controlled text an error diff --git a/docs/security-considerations.md b/docs/security-considerations.md index aaed59ae..92cf62d7 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1401,8 +1401,8 @@ are attacker-influenced. browser. The store is trusted for availability and single consumption only. The transaction keys are secrets like a session key: 32 bytes or more from a cryptographic source, shared by every instance serving - callbacks, never logged (`Config` and `*Client` redact them with every - `fmt` verb and in `slog`). Rotate them in three steps — add the new key last on every + callbacks, never logged (they are `oidcclient.Secret` values, which never + print, log nor encode). Rotate them in three steps — add the new key last on every instance, move it first, remove the old one once `TransactionTTL` has elapsed: the first key protects new transactions, every key is accepted. - **Single use** — `TransactionStore.Consume` removes the transaction @@ -1505,13 +1505,12 @@ are attacker-influenced. (discovery and JWK Set requests only within the origin requested); `UserInfo` sends an identity's access token only to the UserInfo endpoint of the connection that issued it, and returns claims only when - their `sub` is the identity's. Every `fmt` verb and `slog` redact - secrets (`%p` of a non-pointer value excepted: fmt prints the fields - before any method), and so does `encoding/json` for `Config`, - `AuthRequest` and `Callback` — which matters to `slog`'s JSON handler, - which encodes a nested value with `encoding/json`. `Identity` and - `Tokens` deliberately encode their tokens (applications persist them), - `UserInfo` its claims: never log them nested in another value. + their `sub` is the identity's. Every credential is an + `oidcclient.Secret`: every `fmt` verb, `slog` and `encoding/json` render + it `REDACTED` — also when fmt reaches it by reflection through an + unexported field, where it prints as an address — and `Reveal()` is the + only way to its value. `Identity` encodes its claims and `UserInfo` its + claims (personal data): never log them nested in another value. - **Caveats** — sending `acr_values` makes `acr` required, among the requested values; a public client (`none`) requires the provider to advertise S256. `github.com/coreos/go-oidc` was evaluated and rejected: diff --git a/examples/oidc-client/main.go b/examples/oidc-client/main.go index 7375ee41..e89926ef 100644 --- a/examples/oidc-client/main.go +++ b/examples/oidc-client/main.go @@ -86,7 +86,7 @@ type connectionConfig struct { ID string Issuer string ClientID string - ClientSecret string + ClientSecret oidcclient.Secret ResponseMode string // "" (query) or "form_post" } @@ -100,7 +100,7 @@ type config struct { Outbound outbound.Policy // TransactionKeys protect the pending logins // (oidcclient.Config.TransactionKeys): secret, shared by every instance. - TransactionKeys [][]byte + TransactionKeys []oidcclient.Secret // MaxPendingLogins bounds the logins in progress the store keeps // (defaultMaxPendingLogins when not positive). MaxPendingLogins int @@ -147,7 +147,7 @@ func loadConfig() (config, error) { ID: id, Issuer: os.Getenv(prefix + "ISSUER"), ClientID: os.Getenv(prefix + "CLIENT_ID"), - ClientSecret: os.Getenv(prefix + "CLIENT_SECRET"), + ClientSecret: oidcclient.NewSecret(os.Getenv(prefix + "CLIENT_SECRET")), ResponseMode: os.Getenv(prefix + "RESPONSE_MODE"), }) } @@ -211,7 +211,7 @@ var errKeyNotHex = errors.New("not hex-encoded") // transactionKeys decodes OIDC_TRANSACTION_KEYS: hex-encoded keys separated // by commas. Unset, it draws one random key — for a single development // instance only: the logins in progress do not survive a restart. -func transactionKeys(raw string) ([][]byte, error) { +func transactionKeys(raw string) ([]oidcclient.Secret, error) { if strings.TrimSpace(raw) == "" { key := make([]byte, oidcclient.MinTransactionKeySize) _, _ = rand.Read(key) // crypto/rand.Read never fails @@ -219,10 +219,10 @@ func transactionKeys(raw string) ([][]byte, error) { log.Print("oidc-client: OIDC_TRANSACTION_KEYS is not set: using a random per-process transaction key " + "(logins in progress fail on a restart and cannot span instances)") - return [][]byte{key}, nil + return []oidcclient.Secret{oidcclient.NewSecret(string(key))}, nil } - var keys [][]byte + var keys []oidcclient.Secret for part := range strings.SplitSeq(raw, ",") { key, err := hex.DecodeString(strings.TrimSpace(part)) @@ -231,7 +231,7 @@ func transactionKeys(raw string) ([][]byte, error) { return nil, fmt.Errorf("OIDC_TRANSACTION_KEYS: key %d: %w", len(keys)+1, errKeyNotHex) } - keys = append(keys, key) + keys = append(keys, oidcclient.NewSecret(string(key))) } return keys, nil @@ -332,7 +332,7 @@ func newServer(ctx context.Context, cfg config) (http.Handler, error) { for _, c := range cfg.Connections { method := oidcclient.AuthMethodClientSecretBasic - if c.ClientSecret == "" { + if c.ClientSecret.Reveal() == "" { method = oidcclient.AuthMethodNone } diff --git a/examples/oidc-client/main_test.go b/examples/oidc-client/main_test.go index 65ce2d3e..09759a67 100644 --- a/examples/oidc-client/main_test.go +++ b/examples/oidc-client/main_test.go @@ -238,11 +238,11 @@ func TestOIDCClientExample(t *testing.T) { handler, err := newServer(t.Context(), config{ BaseURL: base, Connections: []connectionConfig{ - {ID: "acme", Issuer: acme.issuer(), ClientID: "rp-acme", ClientSecret: "acme-secret"}, - {ID: "globex", Issuer: globex.issuer(), ClientID: "rp-globex", ClientSecret: "globex-secret"}, + {ID: "acme", Issuer: acme.issuer(), ClientID: "rp-acme", ClientSecret: oidcclient.NewSecret("acme-secret")}, + {ID: "globex", Issuer: globex.issuer(), ClientID: "rp-globex", ClientSecret: oidcclient.NewSecret("globex-secret")}, }, Outbound: outbound.Policy{AllowedPrefixes: outbound.LoopbackPrefixes(), RootCAs: pool}, - TransactionKeys: [][]byte{[]byte("an example transaction key of 32+ bytes")}, + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret("an example transaction key of 32+ bytes")}, }) require.NoError(t, err) @@ -372,11 +372,11 @@ func TestLoadConfig(t *testing.T) { require.NoError(t, err) assert.Equal(t, "http://localhost:8083", cfg.BaseURL) assert.Equal(t, []connectionConfig{ - {ID: "acme", Issuer: "https://acme.example.com", ClientID: "rp", ClientSecret: "s3cr3t"}, + {ID: "acme", Issuer: "https://acme.example.com", ClientID: "rp", ClientSecret: oidcclient.NewSecret("s3cr3t")}, {ID: "globex", Issuer: "https://globex.example.com", ClientID: "rp-public", ResponseMode: "form_post"}, }, cfg.Connections) require.Len(t, cfg.TransactionKeys, 1, "a random development key when none is configured") - assert.Len(t, cfg.TransactionKeys[0], 32) + assert.Len(t, cfg.TransactionKeys[0].Reveal(), 32) other, err := loadConfig() require.NoError(t, err) @@ -387,7 +387,9 @@ func TestLoadConfig(t *testing.T) { cfg, err = loadConfig() require.NoError(t, err) - assert.Equal(t, [][]byte{bytes.Repeat([]byte{0xab}, 32), bytes.Repeat([]byte{0xcd}, 48)}, cfg.TransactionKeys) + require.Len(t, cfg.TransactionKeys, 2) + assert.Equal(t, string(bytes.Repeat([]byte{0xab}, 32)), cfg.TransactionKeys[0].Reveal()) + assert.Equal(t, string(bytes.Repeat([]byte{0xcd}, 48)), cfg.TransactionKeys[1].Reveal()) // A malformed key is reported by its position: the decoder's error would // echo one of its characters into the log. @@ -405,8 +407,8 @@ func TestNewServerRefusesShortTransactionKeys(t *testing.T) { _, err := newServer(t.Context(), config{ BaseURL: "http://localhost:8083", - Connections: []connectionConfig{{ID: "acme", Issuer: "https://acme.example.com", ClientID: "rp", ClientSecret: "s3cr3t"}}, - TransactionKeys: [][]byte{[]byte("too short")}, + Connections: []connectionConfig{{ID: "acme", Issuer: "https://acme.example.com", ClientID: "rp", ClientSecret: oidcclient.NewSecret("s3cr3t")}}, + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret("too short")}, }) require.ErrorIs(t, err, oidcclient.ErrInvalidConfig) } diff --git a/examples/oidc-client/ratelimit_test.go b/examples/oidc-client/ratelimit_test.go index 0671f680..f74ac15d 100644 --- a/examples/oidc-client/ratelimit_test.go +++ b/examples/oidc-client/ratelimit_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + oidcclient "github.com/hyperscale-stack/security/oidc/client" "github.com/hyperscale-stack/security/outbound" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -33,9 +34,9 @@ func limitedServer(t *testing.T, maxPending, perMinute int) http.Handler { handler, err := newServer(t.Context(), config{ BaseURL: base, - Connections: []connectionConfig{{ID: "acme", Issuer: acme.issuer(), ClientID: "rp-acme", ClientSecret: "acme-secret"}}, + Connections: []connectionConfig{{ID: "acme", Issuer: acme.issuer(), ClientID: "rp-acme", ClientSecret: oidcclient.NewSecret("acme-secret")}}, Outbound: outbound.Policy{AllowedPrefixes: outbound.LoopbackPrefixes(), RootCAs: pool}, - TransactionKeys: [][]byte{[]byte("an example transaction key of 32+ bytes")}, + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret("an example transaction key of 32+ bytes")}, MaxPendingLogins: maxPending, LoginStartsPerMinute: perMinute, }) diff --git a/internal/interop/oidcrp/metadata_corpus_test.go b/internal/interop/oidcrp/metadata_corpus_test.go index 3f58a40b..a5bfe329 100644 --- a/internal/interop/oidcrp/metadata_corpus_test.go +++ b/internal/interop/oidcrp/metadata_corpus_test.go @@ -159,17 +159,17 @@ func corpusConfig(issuer string, p clientProfile, policy outbound.Policy) oidccl ConnectionID: "corpus", Issuer: issuer, ClientID: "rp", - ClientSecret: "rp-secret", + ClientSecret: oidcclient.NewSecret("rp-secret"), AuthMethod: p.method, RedirectURL: "https://rp.example.com/oidc/corpus/callback", ResponseMode: p.mode, Store: memory.New(), - TransactionKeys: [][]byte{transactionKey}, + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(transactionKey))}, Outbound: policy, } if p.method == oidcclient.AuthMethodNone { - cfg.ClientSecret = "" + cfg.ClientSecret = oidcclient.Secret{} } return cfg diff --git a/internal/interop/oidcrp/zitadel_options_test.go b/internal/interop/oidcrp/zitadel_options_test.go index e807d086..33614deb 100644 --- a/internal/interop/oidcrp/zitadel_options_test.go +++ b/internal/interop/oidcrp/zitadel_options_test.go @@ -57,7 +57,7 @@ func TestZitadelMaxAge(t *testing.T) { maxAge := time.Minute id, _, err := p.loginWith(t, rp, oidcclient.AuthOptions{MaxAge: &maxAge}) require.NoError(t, err) - assert.Contains(t, claimsOf(t, id.Tokens.IDToken), "auth_time") + assert.Contains(t, claimsOf(t, id.Tokens.IDToken.Reveal()), "auth_time") assert.WithinDuration(t, time.Now(), id.AuthTime, time.Minute) } @@ -79,6 +79,6 @@ func TestZitadelACRValues(t *testing.T) { id, _, err = p.login(t, rp) require.NoError(t, err) - assert.NotContains(t, claimsOf(t, id.Tokens.IDToken), "acr", "the provider emits no acr") + assert.NotContains(t, claimsOf(t, id.Tokens.IDToken.Reveal()), "acr", "the provider emits no acr") assert.Empty(t, id.ACR) } diff --git a/internal/interop/oidcrp/zitadel_test.go b/internal/interop/oidcrp/zitadel_test.go index a590bcb6..713eab85 100644 --- a/internal/interop/oidcrp/zitadel_test.go +++ b/internal/interop/oidcrp/zitadel_test.go @@ -103,12 +103,12 @@ func (p *provider) relyingParty(t *testing.T, connection string, mutators ...fun ConnectionID: connection, Issuer: p.issuer, ClientID: clientID, - ClientSecret: clientSecret, + ClientSecret: oidcclient.NewSecret(clientSecret), AuthMethod: oidcclient.AuthMethodClientSecretBasic, RedirectURL: redirectURL(connection), Scopes: []string{"openid", "profile", "email"}, Store: memory.New(), - TransactionKeys: [][]byte{transactionKey}, + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(transactionKey))}, Outbound: outbound.Policy{AllowedPrefixes: outbound.LoopbackPrefixes(), RootCAs: p.pool}, } @@ -282,7 +282,7 @@ func TestZitadelCodeFlow(t *testing.T) { cfg.AuthMethod = tc.method if tc.method == oidcclient.AuthMethodNone { - cfg.ClientSecret = "" + cfg.ClientSecret = oidcclient.Secret{} } }) @@ -292,8 +292,8 @@ func TestZitadelCodeFlow(t *testing.T) { assert.Equal(t, p.issuer, id.Issuer) assert.Equal(t, userID, id.Subject) assert.Equal(t, []string{clientID}, id.Audience) - assert.NotEmpty(t, id.Tokens.AccessToken) - assert.Contains(t, claimsOf(t, id.Tokens.IDToken), "at_hash", "at_hash was verified") + assert.NotEmpty(t, id.Tokens.AccessToken.Reveal()) + assert.Contains(t, claimsOf(t, id.Tokens.IDToken.Reveal()), "at_hash", "at_hash was verified") var claims struct { Email string `json:"email"` diff --git a/internal/interop/oidcstack/concurrency_test.go b/internal/interop/oidcstack/concurrency_test.go index f5d37f48..39d95a4d 100644 --- a/internal/interop/oidcstack/concurrency_test.go +++ b/internal/interop/oidcstack/concurrency_test.go @@ -94,7 +94,7 @@ func TestConcurrentCallbacksAcrossReplicas(t *testing.T) { store := clientmemory.New() starting := o.rp(t, o.basic(), func(cfg *oidcclient.Config) { cfg.Store = store }) completing := o.rp(t, o.basic(), func(cfg *oidcclient.Config) { - cfg.Store, cfg.TransactionKeys = store, [][]byte{[]byte("another replica's transaction key!!")} + cfg.Store, cfg.TransactionKeys = store, []oidcclient.Secret{oidcclient.NewSecret("another replica's transaction key!!")} }) ar, loc := o.begin(t, starting, oidcclient.AuthOptions{}) diff --git a/internal/interop/oidcstack/conformance/rp_test.go b/internal/interop/oidcstack/conformance/rp_test.go index bba1b3e4..fc669404 100644 --- a/internal/interop/oidcstack/conformance/rp_test.go +++ b/internal/interop/oidcstack/conformance/rp_test.go @@ -273,13 +273,13 @@ func loginWith(t *testing.T, plan rpPlan, module, issuer, secret string) (string cfg := oidcclient.Config{ ConnectionID: "conformance", Issuer: issuer, ClientID: "hyperscale-rp", AuthMethod: plan.method, RedirectURL: rpRedirectURI, Scopes: []string{"profile", "email", "address", "phone"}, - ResponseMode: plan.mode, Store: clientmemory.New(), TransactionKeys: [][]byte{key}, + ResponseMode: plan.mode, Store: clientmemory.New(), TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(key))}, Outbound: outbound.Policy{RootCAs: pool, AllowedPrefixes: outbound.LoopbackPrefixes()}, Now: clk.now, } if plan.method != oidcclient.AuthMethodNone { - cfg.ClientSecret = secret + cfg.ClientSecret = oidcclient.NewSecret(secret) } client, err := oidcclient.New(ctx, cfg) diff --git a/internal/interop/oidcstack/flow_test.go b/internal/interop/oidcstack/flow_test.go index 905038ef..19deaa19 100644 --- a/internal/interop/oidcstack/flow_test.go +++ b/internal/interop/oidcstack/flow_test.go @@ -61,7 +61,7 @@ func TestCodeFlowAgainstOurProvider(t *testing.T) { assert.Equal(t, []string{"pwd"}, id.AMR) assert.WithinDuration(t, o.clock.now(), id.AuthTime, 2*time.Second) - _, claims := jwsParts(t, id.Tokens.IDToken) + _, claims := jwsParts(t, id.Tokens.IDToken.Reveal()) assert.Contains(t, claims, "at_hash", "the client verified at_hash") assert.Contains(t, claims, "nonce") @@ -104,7 +104,7 @@ func TestCodeFlowSigningAlgorithms(t *testing.T) { id, err := o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) - header, _ := jwsParts(t, id.Tokens.IDToken) + header, _ := jwsParts(t, id.Tokens.IDToken.Reveal()) assert.Equal(t, string(alg), header["alg"]) assert.Equal(t, "root-"+name, header["kid"]) }) @@ -349,15 +349,15 @@ func TestRefreshOfAClientLogin(t *testing.T) { id, err := o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) - assert.Empty(t, id.Tokens.RefreshToken, "no consent dialog, no offline access") + assert.Empty(t, id.Tokens.RefreshToken.Reveal(), "no consent dialog, no offline access") id, err = o.login(t, c, oidcclient.AuthOptions{Prompt: []string{"consent"}}) require.NoError(t, err) - require.NotEmpty(t, id.Tokens.RefreshToken) + require.NotEmpty(t, id.Tokens.RefreshToken.Reveal()) o.clock.advance(time.Minute) - refreshed := o.refresh(t, basicID, basicSecret, id.Tokens.RefreshToken) + refreshed := o.refresh(t, basicID, basicSecret, id.Tokens.RefreshToken.Reveal()) require.Equal(t, http.StatusOK, refreshed.status, refreshed.body) raw, ok := refreshed.json["id_token"].(string) @@ -367,7 +367,7 @@ func TestRefreshOfAClientLogin(t *testing.T) { require.NoError(t, err) var before, after map[string]any - _, before = jwsParts(t, id.Tokens.IDToken) + _, before = jwsParts(t, id.Tokens.IDToken.Reveal()) require.NoError(t, json.Unmarshal(verified.Payload, &after)) for _, claim := range []string{"iss", "sub", "aud", "auth_time", "amr"} { diff --git a/internal/interop/oidcstack/harness_test.go b/internal/interop/oidcstack/harness_test.go index 3fb58d29..b89ea1b8 100644 --- a/internal/interop/oidcstack/harness_test.go +++ b/internal/interop/oidcstack/harness_test.go @@ -594,12 +594,12 @@ func (o *op) config(c connection, mutators ...func(*oidcclient.Config)) oidcclie ConnectionID: c.name, Issuer: c.issuer, ClientID: c.clientID, - ClientSecret: c.secret, + ClientSecret: oidcclient.NewSecret(c.secret), AuthMethod: c.method, RedirectURL: callback(c.name), Scopes: []string{"openid", "profile", "email"}, Store: clientmemory.New(), - TransactionKeys: [][]byte{transactionKey}, + TransactionKeys: []oidcclient.Secret{oidcclient.NewSecret(string(transactionKey))}, Outbound: outbound.Policy{AllowedPrefixes: outbound.LoopbackPrefixes(), RootCAs: o.pool(), Timeout: 2 * time.Second}, Now: o.clock.now, } @@ -654,7 +654,7 @@ func redirected(t *testing.T, resp *http.Response) *url.URL { // callbackOf is the callback the user agent delivers for the response at // loc, presenting binding. -func callbackOf(loc *url.URL, binding string) oidcclient.Callback { +func callbackOf(loc *url.URL, binding oidcclient.Secret) oidcclient.Callback { return oidcclient.Callback{Mode: oidcclient.ResponseModeQuery, URL: loc, Params: loc.Query(), Binding: binding} } diff --git a/internal/interop/oidcstack/replay_test.go b/internal/interop/oidcstack/replay_test.go index b75fc425..25d786cf 100644 --- a/internal/interop/oidcstack/replay_test.go +++ b/internal/interop/oidcstack/replay_test.go @@ -216,20 +216,21 @@ func TestRefreshReuseRevokesTheClientsTokens(t *testing.T) { id, err := o.login(t, c, oidcclient.AuthOptions{Prompt: []string{"consent"}}) require.NoError(t, err) - require.NotEmpty(t, id.Tokens.RefreshToken) + require.NotEmpty(t, id.Tokens.RefreshToken.Reveal()) o.clock.advance(time.Second) - rotated := o.refresh(t, basicID, basicSecret, id.Tokens.RefreshToken) + rotated := o.refresh(t, basicID, basicSecret, id.Tokens.RefreshToken.Reveal()) require.Equal(t, http.StatusOK, rotated.status, rotated.body) newest := *id - newest.Tokens.AccessToken, _ = rotated.json["access_token"].(string) + accessToken, _ := rotated.json["access_token"].(string) + newest.Tokens.AccessToken = oidcclient.NewSecret(accessToken) _, err = c.UserInfo(t.Context(), &newest) require.NoError(t, err, "the rotated access token works") - reuse := o.refresh(t, basicID, basicSecret, id.Tokens.RefreshToken) + reuse := o.refresh(t, basicID, basicSecret, id.Tokens.RefreshToken.Reveal()) assert.True(t, reuse.isCode("invalid_grant"), reuse.body) _, err = c.UserInfo(t.Context(), &newest) diff --git a/internal/interop/oidcstack/rotation_test.go b/internal/interop/oidcstack/rotation_test.go index 98369ae6..308241fd 100644 --- a/internal/interop/oidcstack/rotation_test.go +++ b/internal/interop/oidcstack/rotation_test.go @@ -65,7 +65,7 @@ func TestDocumentedRotationIsInvisibleToTheClient(t *testing.T) { id, err := o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) - header, _ := jwsParts(t, id.Tokens.IDToken) + header, _ := jwsParts(t, id.Tokens.IDToken.Reveal()) assert.Equal(t, "root-rs256-2", header["kid"]) assert.Equal(t, 2, o.hits(jwks), "no unknown-kid refresh was needed") diff --git a/internal/interop/oidcstack/substitution_test.go b/internal/interop/oidcstack/substitution_test.go index 71176aa3..5a02b70d 100644 --- a/internal/interop/oidcstack/substitution_test.go +++ b/internal/interop/oidcstack/substitution_test.go @@ -196,7 +196,7 @@ func TestTokenResponseSubstitution(t *testing.T) { earlier, err := o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) - o.tamperTokenResponse(o.base, func(resp map[string]any) { resp["access_token"] = earlier.Tokens.AccessToken }) + o.tamperTokenResponse(o.base, func(resp map[string]any) { resp["access_token"] = earlier.Tokens.AccessToken.Reveal() }) _, err = o.login(t, c, oidcclient.AuthOptions{}) require.ErrorIs(t, err, oidcclient.ErrAccessTokenHash) @@ -209,7 +209,7 @@ func TestTokenResponseSubstitution(t *testing.T) { other, err := o.login(t, o.rp(t, conn("other", o.base, otherID, secret, oidcclient.AuthMethodClientSecretBasic)), oidcclient.AuthOptions{}) require.NoError(t, err) - o.tamperTokenResponse(o.base, func(resp map[string]any) { resp["id_token"] = other.Tokens.IDToken }) + o.tamperTokenResponse(o.base, func(resp map[string]any) { resp["id_token"] = other.Tokens.IDToken.Reveal() }) _, err = o.login(t, o.rp(t, o.basic()), oidcclient.AuthOptions{}) require.ErrorIs(t, err, oidcclient.ErrIDTokenAudience) @@ -223,7 +223,7 @@ func TestTokenResponseSubstitution(t *testing.T) { earlier, err := o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) - o.tamperTokenResponse(o.base, func(resp map[string]any) { resp["id_token"] = earlier.Tokens.IDToken }) + o.tamperTokenResponse(o.base, func(resp map[string]any) { resp["id_token"] = earlier.Tokens.IDToken.Reveal() }) _, err = o.login(t, c, oidcclient.AuthOptions{}) require.ErrorIs(t, err, oidcclient.ErrIDTokenNonce) @@ -246,7 +246,7 @@ func TestTokenResponseSubstitution(t *testing.T) { b, err := o.login(t, o.rp(t, conn("shared-b", o.base+tenantB, sharedID, secret, oidcclient.AuthMethodClientSecretBasic)), oidcclient.AuthOptions{}) require.NoError(t, err) - o.tamperTokenResponse(o.base+tenantA, func(resp map[string]any) { resp["id_token"] = b.Tokens.IDToken }) + o.tamperTokenResponse(o.base+tenantA, func(resp map[string]any) { resp["id_token"] = b.Tokens.IDToken.Reveal() }) _, err = o.login(t, o.rp(t, conn("shared-a", o.base+tenantA, sharedID, secret, oidcclient.AuthMethodClientSecretBasic)), oidcclient.AuthOptions{}) require.ErrorIs(t, err, oidcclient.ErrIDTokenIssuer, "the signature verifies: iss is the only guard") @@ -294,7 +294,7 @@ func TestIDTokenUsedAsAnotherToken(t *testing.T) { req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, o.base+o.paths(o.base).UserInfo, nil) require.NoError(t, err) - req.Header.Set("Authorization", "Bearer "+own.Tokens.IDToken) + req.Header.Set("Authorization", "Bearer "+own.Tokens.IDToken.Reveal()) resp, err := o.srv.Client().Do(req) require.NoError(t, err) @@ -308,7 +308,7 @@ func TestIDTokenUsedAsAnotherToken(t *testing.T) { tenant, err := o.login(t, o.rp(t, o.tenant(tenantA, tenantAID)), oidcclient.AuthOptions{}) require.NoError(t, err) - for name, hint := range map[string]string{ + for name, hint := range map[string]oidcclient.Secret{ "the at+jwt access token": own.Tokens.AccessToken, "another client's ID Token": other.Tokens.IDToken, "another issuer's ID Token": tenant.Tokens.IDToken, diff --git a/oidc/client/begin.go b/oidc/client/begin.go index e19836a4..ccc49d4a 100644 --- a/oidc/client/begin.go +++ b/oidc/client/begin.go @@ -36,8 +36,10 @@ type AuthOptions struct { // LoginHint is passed as login_hint (at most 1024 bytes of UTF-8, // without control characters). LoginHint string - // IDTokenHint is passed as id_token_hint. - IDTokenHint string + // IDTokenHint is passed as id_token_hint: an ID Token the provider + // issued earlier, such as Identity.Tokens.IDToken. It never prints, + // logs nor encodes (see Secret). + IDTokenHint Secret // Prompt values are passed space-separated as prompt; "none" cannot // be combined with another value. Prompt []string @@ -82,12 +84,10 @@ type AuthRequest struct { // Binding is the browser-binding secret of the login. The user agent // that started the login MUST keep it — the net/http helpers put it in // an HttpOnly __Host- cookie — and present it at the callback. It never - // leaves the Relying Party and the user agent: never log it. fmt, - // log/slog and encoding/json leave it out — it is tagged json:"-" for - // the encodings that do not go through MarshalJSON — but other - // reflection-based encoders (encoding/gob, encoding/xml, YAML) write - // it: persist it explicitly, only where a secret belongs. - Binding string `json:"-"` + // leaves the Relying Party and the user agent: it is a Secret, which + // never prints, logs nor encodes; Reveal it only to hand it to the user + // agent. + Binding Secret `json:"-"` // ExpiresAt is when the login expires. ExpiresAt time.Time } @@ -154,7 +154,7 @@ func (c *Client) Begin(ctx context.Context, opts AuthOptions) (*AuthRequest, err CreatedAt: created, ExpiresAt: created.Add(c.cfg.TransactionTTL), } - tx.Integrity = integrityFor(c.cfg.TransactionKeys[0], secret, tx) + tx.Integrity = integrityFor([]byte(c.cfg.TransactionKeys[0].Reveal()), secret, tx) if err := c.cfg.Store.Save(ctx, tx); err != nil { return nil, fmt.Errorf("oidcclient: save login transaction: %w", err) @@ -163,7 +163,7 @@ func (c *Client) Begin(ctx context.Context, opts AuthOptions) (*AuthRequest, err return &AuthRequest{ URL: c.authorizationURL(state, secret, opts), State: state, - Binding: binding, + Binding: NewSecret(binding), ExpiresAt: tx.ExpiresAt, }, nil } @@ -195,7 +195,7 @@ func (c *Client) authorizationURL(state string, secret []byte, opts AuthOptions) } setNonEmpty(params, "login_hint", opts.LoginHint) - setNonEmpty(params, "id_token_hint", opts.IDTokenHint) + setNonEmpty(params, "id_token_hint", opts.IDTokenHint.Reveal()) setNonEmpty(params, "prompt", strings.Join(opts.Prompt, " ")) setNonEmpty(params, "acr_values", strings.Join(opts.ACRValues, " ")) setNonEmpty(params, "ui_locales", strings.Join(opts.UILocales, " ")) @@ -248,7 +248,7 @@ func (o AuthOptions) validate(endpointParams map[string]struct{}) error { return fmt.Errorf("%w: LoginHint must be at most 1024 bytes of UTF-8 without control characters", ErrInvalidAuthRequest) } - if o.IDTokenHint != "" && !isASCII(o.IDTokenHint, visibleASCII, maxIDTokenHintLength) { + if hint := o.IDTokenHint.Reveal(); hint != "" && !isASCII(hint, visibleASCII, maxIDTokenHintLength) { return fmt.Errorf("%w: IDTokenHint must be a compact JWT", ErrInvalidAuthRequest) } diff --git a/oidc/client/begin_test.go b/oidc/client/begin_test.go index 3e9551ba..d2e84a7f 100644 --- a/oidc/client/begin_test.go +++ b/oidc/client/begin_test.go @@ -117,7 +117,7 @@ func TestBeginParameters(t *testing.T) { assert.Regexp(t, b64url43, q.Get("state")) assert.Regexp(t, b64url43, q.Get("nonce")) assert.Regexp(t, b64url43, q.Get("code_challenge")) - assert.Regexp(t, b64url43, ar.Binding) + assert.Regexp(t, b64url43, ar.Binding.Reveal()) assert.NotContains(t, ar.URL, ar.Binding, "the binding secret never leaves the RP and the user agent") }) } @@ -140,7 +140,7 @@ func TestBeginValuesAreUnique(t *testing.T) { u, err := url.Parse(ar.URL) require.NoError(t, err) - for _, v := range []string{ar.State, ar.Binding, u.Query().Get("nonce"), u.Query().Get("code_challenge")} { + for _, v := range []string{ar.State, ar.Binding.Reveal(), u.Query().Get("nonce"), u.Query().Get("code_challenge")} { require.False(t, seen[v], "value reused") seen[v] = true } @@ -171,7 +171,7 @@ func TestBeginPersistsTransaction(t *testing.T) { tx := store.last(t) assert.Equal(t, sha256Hex(ar.State), tx.StateHash) assert.Regexp(t, `^[0-9a-f]{64}$`, tx.BindingHash) - assert.NotEqual(t, sha256Hex(ar.Binding), tx.BindingHash, "a keyed hash, not a plain digest of the secret") + assert.NotEqual(t, sha256Hex(ar.Binding.Reveal()), tx.BindingHash, "a keyed hash, not a plain digest of the secret") assert.Equal(t, testConnection, tx.ConnectionID) assert.Equal(t, op.issuer, tx.Issuer) assert.Equal(t, testClientID, tx.ClientID) @@ -186,7 +186,7 @@ func TestBeginPersistsTransaction(t *testing.T) { assert.Regexp(t, `^[0-9a-f]{64}$`, tx.Integrity) - for _, secret := range []string{ar.Binding, ar.State} { + for _, secret := range []string{ar.Binding.Reveal(), ar.State} { assert.NotContains(t, tx.BindingHash+tx.StateHash+tx.AppState+tx.ACRValues+tx.Integrity, secret) } @@ -258,7 +258,7 @@ func TestBeginHints(t *testing.T) { want map[string]string }{ {"login hint", oidcclient.AuthOptions{LoginHint: "jane@example.com"}, map[string]string{"login_hint": "jane@example.com"}}, - {"id token hint", oidcclient.AuthOptions{IDTokenHint: "eyJ.x.y"}, map[string]string{"id_token_hint": "eyJ.x.y"}}, + {"id token hint", oidcclient.AuthOptions{IDTokenHint: oidcclient.NewSecret("eyJ.x.y")}, map[string]string{"id_token_hint": "eyJ.x.y"}}, {"prompt", oidcclient.AuthOptions{Prompt: []string{"login", "consent"}}, map[string]string{"prompt": "login consent"}}, {"prompt none", oidcclient.AuthOptions{Prompt: []string{"none"}}, map[string]string{"prompt": "none"}}, {"max age zero", oidcclient.AuthOptions{MaxAge: &zero}, map[string]string{"max_age": "0"}}, @@ -307,7 +307,7 @@ func TestBeginRefusesInvalidOptions(t *testing.T) { "display with space": {Display: "pop up"}, "login hint too long": {LoginHint: strings.Repeat("a", 1025)}, "login hint with control": {LoginHint: "jane\r\n@example.com"}, - "id token hint too long": {IDTokenHint: strings.Repeat("a", 65537)}, + "id token hint too long": {IDTokenHint: oidcclient.NewSecret(strings.Repeat("a", 65537))}, "app state too long": {AppState: strings.Repeat("a", 4097)}, "app state invalid UTF-8": {AppState: "\xff"}, "extra param empty name": {ExtraParams: map[string]string{"": "x"}}, diff --git a/oidc/client/callback.go b/oidc/client/callback.go index 064f3be8..f271df12 100644 --- a/oidc/client/callback.go +++ b/oidc/client/callback.go @@ -43,8 +43,9 @@ type Callback struct { // Params are the response parameters (url.ParseQuery or r.PostForm). Params url.Values `json:"-"` // Binding is the browser-binding secret the user agent presented (the - // AuthRequest.Binding of the login, read from its cookie). - Binding string `json:"-"` + // AuthRequest.Binding of the login, read from its cookie): + // NewSecret(cookie.Value). + Binding Secret `json:"-"` } // String implements fmt.Stringer without the binding secret, the parameter @@ -129,7 +130,7 @@ func (c *Client) Complete(ctx context.Context, cb Callback) (*Identity, error) { return nil, err } - secret, ok := decodeSecret(cb.Binding) + secret, ok := decodeSecret(cb.Binding.Reveal()) if !ok { return nil, fmt.Errorf("%w: missing or malformed binding", ErrBindingMismatch) } diff --git a/oidc/client/callback_test.go b/oidc/client/callback_test.go index 5523fb91..b78807e0 100644 --- a/oidc/client/callback_test.go +++ b/oidc/client/callback_test.go @@ -43,12 +43,12 @@ func TestCompleteQuery(t *testing.T) { assert.Equal(t, testNow.Add(5*time.Minute), id.ExpiresAt) assert.True(t, id.AuthTime.IsZero()) assert.Equal(t, "return=/home", id.AppState) - assert.Equal(t, testAccessToken, id.Tokens.AccessToken) + assert.Equal(t, testAccessToken, id.Tokens.AccessToken.Reveal()) assert.Equal(t, "Bearer", id.Tokens.TokenType) - assert.Equal(t, "rt-1", id.Tokens.RefreshToken) + assert.Equal(t, "rt-1", id.Tokens.RefreshToken.Reveal()) assert.Equal(t, "openid profile", id.Tokens.Scope) assert.Equal(t, testNow.Add(time.Hour), id.Tokens.Expiry) - assert.NotEmpty(t, id.Tokens.IDToken) + assert.NotEmpty(t, id.Tokens.IDToken.Reveal()) var claims struct { Email string `json:"email"` @@ -109,10 +109,10 @@ func TestCompleteBinding(t *testing.T) { cases := map[string]func(victim, attacker *oidcclient.AuthRequest) string{ "missing": func(*oidcclient.AuthRequest, *oidcclient.AuthRequest) string { return "" }, - "malformed": func(v, _ *oidcclient.AuthRequest) string { return v.Binding[:42] }, - "padded": func(v, _ *oidcclient.AuthRequest) string { return v.Binding + "=" }, - "non-canonical encoding": func(v, _ *oidcclient.AuthRequest) string { return nonCanonical(v.Binding) }, - "another login's binding": func(_, a *oidcclient.AuthRequest) string { return a.Binding }, + "malformed": func(v, _ *oidcclient.AuthRequest) string { return v.Binding.Reveal()[:42] }, + "padded": func(v, _ *oidcclient.AuthRequest) string { return v.Binding.Reveal() + "=" }, + "non-canonical encoding": func(v, _ *oidcclient.AuthRequest) string { return nonCanonical(v.Binding.Reveal()) }, + "another login's binding": func(_, a *oidcclient.AuthRequest) string { return a.Binding.Reveal() }, } for name, bindingOf := range cases { @@ -127,7 +127,7 @@ func TestCompleteBinding(t *testing.T) { attacker, _ := op.begin(t, c, oidcclient.AuthOptions{}) cb := callbackFor(victim, code) - cb.Binding = bindingOf(victim, attacker) + cb.Binding = oidcclient.NewSecret(bindingOf(victim, attacker)) id, err := c.Complete(context.Background(), cb) require.ErrorIs(t, err, oidcclient.ErrBindingMismatch) diff --git a/oidc/client/config.go b/oidc/client/config.go index 697bf394..de30c5cc 100644 --- a/oidc/client/config.go +++ b/oidc/client/config.go @@ -5,7 +5,6 @@ package oidcclient import ( - "bytes" "encoding/json" "fmt" "log/slog" @@ -110,8 +109,9 @@ type Config struct { ClientID string // ClientSecret is the client secret, required for the client_secret_* - // methods and forbidden for AuthMethodNone. - ClientSecret string + // methods and forbidden for AuthMethodNone: NewSecret(secret). It never + // prints, logs nor encodes (see Secret). + ClientSecret Secret // AuthMethod is the token endpoint authentication method. It has no // default: choose it explicitly. @@ -142,7 +142,7 @@ type Config struct { // integrity MAC. REQUIRED: 1 to MaxTransactionKeys keys of at least // MinTransactionKeySize bytes from a cryptographic source, secret like // a session key and shared by every instance that may serve the - // callback. TransactionKeys[0] protects the transactions Begin stores; + // callback: NewSecret(string(key)). TransactionKeys[0] protects the transactions Begin stores; // Complete accepts any of the keys. The MAC key of a transaction // derives from both a transaction key and the login's binding secret, // so whoever can write to the Store — even from the browser of a login @@ -152,7 +152,7 @@ type Config struct { // instance, then move it first, then remove the old key once // TransactionTTL has elapsed. A key removed earlier fails the logins it // protected with ErrTransactionIntegrity. - TransactionKeys [][]byte + TransactionKeys []Secret // TransactionTTL bounds the time between Begin and the callback, in // whole seconds: DefaultTransactionTTL when zero, at most @@ -242,7 +242,7 @@ func (c Config) normalized() (Config, *url.URL, error) { } out.TrustedAudiences = slices.Clone(c.TrustedAudiences) - out.TransactionKeys = cloneKeys(c.TransactionKeys) + out.TransactionKeys = slices.Clone(c.TransactionKeys) out.Outbound.AllowedPrefixes = slices.Clone(c.Outbound.AllowedPrefixes) out.applyDefaults() @@ -277,11 +277,11 @@ func (c Config) validateClient() error { switch c.AuthMethod { case AuthMethodClientSecretBasic, AuthMethodClientSecretPost: - if c.ClientSecret == "" { + if c.ClientSecret.Reveal() == "" { return fmt.Errorf("%w: %s requires ClientSecret", ErrInvalidConfig, c.AuthMethod) } case AuthMethodNone: - if c.ClientSecret != "" { + if c.ClientSecret.Reveal() != "" { return fmt.Errorf("%w: a public client (none) has no ClientSecret", ErrInvalidConfig) } default: @@ -317,13 +317,13 @@ func (c Config) validateFlow() error { // validateTransactionKeys checks Config.TransactionKeys: 1 to // MaxTransactionKeys keys of at least MinTransactionKeySize bytes. The // errors never carry key material. -func validateTransactionKeys(keys [][]byte) error { +func validateTransactionKeys(keys []Secret) error { if len(keys) == 0 || len(keys) > MaxTransactionKeys { return fmt.Errorf("%w: TransactionKeys must hold 1 to %d keys", ErrInvalidConfig, MaxTransactionKeys) } for _, key := range keys { - if len(key) < MinTransactionKeySize { + if len(key.Reveal()) < MinTransactionKeySize { return fmt.Errorf("%w: every TransactionKeys entry must be at least %d bytes", ErrInvalidConfig, MinTransactionKeySize) } } @@ -331,23 +331,12 @@ func validateTransactionKeys(keys [][]byte) error { return nil } -// cloneKeys returns a deep copy of keys: the Client never shares key -// bytes with its caller. -func cloneKeys(keys [][]byte) [][]byte { - out := make([][]byte, len(keys)) - for i, key := range keys { - out[i] = bytes.Clone(key) - } - - return out -} - // String implements fmt.Stringer with the identifying fields only: the // client secret and the transaction keys are redacted. func (c Config) String() string { return fmt.Sprintf("oidcclient.Config{ConnectionID: %q, Issuer: %q, ClientID: %q, ClientSecret: %s, AuthMethod: %s, "+ "RedirectURL: %q, ResponseMode: %s, TransactionKeys: %d REDACTED}", - c.ConnectionID, c.Issuer, c.ClientID, redacted(c.ClientSecret), c.AuthMethod, c.RedirectURL, c.ResponseMode, + c.ConnectionID, c.Issuer, c.ClientID, c.ClientSecret.String(), c.AuthMethod, c.RedirectURL, c.ResponseMode, len(c.TransactionKeys)) } @@ -360,7 +349,7 @@ func (c Config) LogValue() slog.Value { slog.String("connection_id", c.ConnectionID), slog.String("issuer", c.Issuer), slog.String("client_id", c.ClientID), - slog.String("client_secret", redacted(c.ClientSecret)), + slog.String("client_secret", c.ClientSecret.String()), slog.String("auth_method", string(c.AuthMethod)), slog.String("redirect_url", c.RedirectURL), slog.String("response_mode", string(c.ResponseMode)), @@ -370,38 +359,22 @@ func (c Config) LogValue() slog.Value { // MarshalJSON implements json.Marshaler like String: the identifying // fields, the client secret redacted and the transaction keys counted. -// Without it, encoding/json would write the secret and the keys of a -// Config that held no func. func (c Config) MarshalJSON() ([]byte, error) { - clientSecret := "" - if c.ClientSecret != "" { - clientSecret = redactedMark - } - - return json.Marshal(struct { //nolint:wrapcheck // strings and an int only: never fails + return json.Marshal(struct { //nolint:wrapcheck // strings, a Secret and an int only: never fails ConnectionID string Issuer string ClientID string - ClientSecret string + ClientSecret Secret AuthMethod AuthMethod RedirectURL string ResponseMode ResponseMode TransactionKeys int }{ - c.ConnectionID, c.Issuer, c.ClientID, clientSecret, c.AuthMethod, c.RedirectURL, c.ResponseMode, + c.ConnectionID, c.Issuer, c.ClientID, c.ClientSecret, c.AuthMethod, c.RedirectURL, c.ResponseMode, len(c.TransactionKeys), }) } -// redacted renders a secret as REDACTED, or as "" when it is empty. -func redacted(secret string) string { - if secret == "" { - return `""` - } - - return redactedMark -} - // validateIssuer checks an issuer identifier (OpenID Connect Core §2: // scheme, host, optional port and path, no query or fragment). func validateIssuer(raw string, allowHTTP bool) error { diff --git a/oidc/client/config_test.go b/oidc/client/config_test.go index 1c3d0b19..20f79ca2 100644 --- a/oidc/client/config_test.go +++ b/oidc/client/config_test.go @@ -50,8 +50,8 @@ func TestNewRejectsInvalidConfig(t *testing.T) { "client with control": func(c *oidcclient.Config) { c.ClientID = "client\n1" }, "no auth method": func(c *oidcclient.Config) { c.AuthMethod = "" }, "unsupported auth method": func(c *oidcclient.Config) { c.AuthMethod = "private_key_jwt" }, - "basic without secret": func(c *oidcclient.Config) { c.ClientSecret = "" }, - "post without secret": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodClientSecretPost; c.ClientSecret = "" }, + "basic without secret": func(c *oidcclient.Config) { c.ClientSecret = oidcclient.Secret{} }, + "post without secret": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodClientSecretPost; c.ClientSecret = oidcclient.Secret{} }, "none with secret": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone }, "empty redirect": func(c *oidcclient.Config) { c.RedirectURL = "" }, "relative redirect": func(c *oidcclient.Config) { c.RedirectURL = testCallbackPath }, @@ -82,13 +82,13 @@ func TestNewRejectsInvalidConfig(t *testing.T) { "invalid outbound policy": func(c *oidcclient.Config) { c.Outbound.MaxRedirects = -1 }, "no transaction key": func(c *oidcclient.Config) { c.TransactionKeys = nil }, "short transaction key": func(c *oidcclient.Config) { - c.TransactionKeys = [][]byte{make([]byte, oidcclient.MinTransactionKeySize-1)} + c.TransactionKeys = secretKeys(make([]byte, oidcclient.MinTransactionKeySize-1)) }, "empty transaction key after a valid one": func(c *oidcclient.Config) { - c.TransactionKeys = [][]byte{testTransactionKey, nil} + c.TransactionKeys = secretKeys(testTransactionKey, nil) }, "too many transaction keys": func(c *oidcclient.Config) { - c.TransactionKeys = [][]byte{testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey} + c.TransactionKeys = secretKeys(testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey) }, } @@ -116,7 +116,7 @@ func TestNewAcceptsBoundaries(t *testing.T) { "redirect with app query": func(c *oidcclient.Config) { c.RedirectURL += "?tenant=acme" }, "one hour ttl": func(c *oidcclient.Config) { c.TransactionTTL = time.Hour }, "five minutes skew": func(c *oidcclient.Config) { c.ClockSkew = 5 * time.Minute }, - "public client": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone; c.ClientSecret = "" }, + "public client": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone; c.ClientSecret = oidcclient.Secret{} }, "post client": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodClientSecretPost }, "form post": func(c *oidcclient.Config) { c.ResponseMode = oidcclient.ResponseModeFormPost }, "single algorithm": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} }, @@ -125,9 +125,9 @@ func TestNewAcceptsBoundaries(t *testing.T) { "duplicated scopes": func(c *oidcclient.Config) { c.Scopes = []string{"email", "openid", "email"} }, "trusted audience": func(c *oidcclient.Config) { c.TrustedAudiences = []string{"https://api.example.com"} }, "four transaction keys": func(c *oidcclient.Config) { - c.TransactionKeys = [][]byte{testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey} + c.TransactionKeys = secretKeys(testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey) }, - "long transaction key": func(c *oidcclient.Config) { c.TransactionKeys = [][]byte{make([]byte, 64)} }, + "long transaction key": func(c *oidcclient.Config) { c.TransactionKeys = secretKeys(make([]byte, 64)) }, } for name, tweak := range cases { @@ -170,12 +170,12 @@ func TestNewCopiesConfigSlices(t *testing.T) { plain := newFakeOP(t) key := bytes.Clone(testTransactionKey) - keys := [][]byte{key} + keys := secretKeys(key) keyed := plain.client(t, func(cfg *oidcclient.Config) { cfg.TransactionKeys = keys }) ar, code := plain.begin(t, keyed, oidcclient.AuthOptions{}) - key[0] ^= 0xff - keys[0] = bytes.Repeat([]byte{0x33}, oidcclient.MinTransactionKeySize) + key[0] ^= 0xff // a Secret holds its own copy of the key bytes + keys[0] = oidcclient.NewSecret(string(bytes.Repeat([]byte{0x33}, oidcclient.MinTransactionKeySize))) _, err = keyed.Complete(context.Background(), callbackFor(ar, code)) require.NoError(t, err, "the Client kept its own copy of TransactionKeys and of their bytes") diff --git a/oidc/client/discovery_test.go b/oidc/client/discovery_test.go index 0fe7d8ac..bcf53d90 100644 --- a/oidc/client/discovery_test.go +++ b/oidc/client/discovery_test.go @@ -336,15 +336,24 @@ func TestCapabilities(t *testing.T) { "confidential, no pkce": {metadata: func(m map[string]any) { delete(m, "code_challenge_methods_supported") }, ok: true}, "public client, no pkce": { metadata: func(m map[string]any) { delete(m, "code_challenge_methods_supported") }, - config: func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone; c.ClientSecret = "" }, + config: func(c *oidcclient.Config) { + c.AuthMethod = oidcclient.AuthMethodNone + c.ClientSecret = oidcclient.Secret{} + }, }, "public client, plain only": { metadata: func(m map[string]any) { m["code_challenge_methods_supported"] = []string{"plain"} }, - config: func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone; c.ClientSecret = "" }, + config: func(c *oidcclient.Config) { + c.AuthMethod = oidcclient.AuthMethodNone + c.ClientSecret = oidcclient.Secret{} + }, }, "public client with S256": { - config: func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone; c.ClientSecret = "" }, - ok: true, + config: func(c *oidcclient.Config) { + c.AuthMethod = oidcclient.AuthMethodNone + c.ClientSecret = oidcclient.Secret{} + }, + ok: true, }, "only HMAC advertised": {metadata: func(m map[string]any) { m["id_token_signing_alg_values_supported"] = []string{"HS256", "none"} }}, "no common algorithm": { diff --git a/oidc/client/doc.go b/oidc/client/doc.go index 138a0bff..0a051b62 100644 --- a/oidc/client/doc.go +++ b/oidc/client/doc.go @@ -72,6 +72,11 @@ // Never route callbacks by the state value. // - Nothing is returned before the ID Token is fully validated: tokens of // a failed login are never exposed (RFC 9700 §4.5.3.2). +// - Every credential — the client secret, the transaction keys, the +// binding secret, the tokens — is a [Secret]: it prints, logs and +// encodes as REDACTED with every fmt verb, log/slog and encoding/json, +// also when fmt reaches it through an unexported field of another +// value; Secret.Reveal is the only way to its value. // - JSON — the ID Token's JOSE header included — is parsed strictly: // duplicate member names, two names that differ only by case, invalid // UTF-8 and unpaired surrogate escapes are refused at every nesting diff --git a/oidc/client/fakeop_test.go b/oidc/client/fakeop_test.go index 949a88dc..5c6396c0 100644 --- a/oidc/client/fakeop_test.go +++ b/oidc/client/fakeop_test.go @@ -42,6 +42,16 @@ var testNow = time.Unix(1_800_000_000, 0).UTC() // Clients (Config.TransactionKeys). var testTransactionKey = []byte("0123456789abcdef0123456789abcdef") +// secretKeys returns keys as Config.TransactionKeys. +func secretKeys(keys ...[]byte) []oidcclient.Secret { + out := make([]oidcclient.Secret, len(keys)) + for i, key := range keys { + out[i] = oidcclient.NewSecret(string(key)) + } + + return out +} + const ( testClientID = "client-1" testClientSecret = "s3cr3t-with-enough-entropy-for-hs256" @@ -692,12 +702,12 @@ func (op *fakeOP) config(mutators ...func(*oidcclient.Config)) oidcclient.Config ConnectionID: testConnection, Issuer: op.issuer, ClientID: testClientID, - ClientSecret: testClientSecret, + ClientSecret: oidcclient.NewSecret(testClientSecret), AuthMethod: oidcclient.AuthMethodClientSecretBasic, RedirectURL: testRedirectURL, Scopes: []string{"openid", "profile"}, Store: memory.New(), - TransactionKeys: [][]byte{testTransactionKey}, + TransactionKeys: secretKeys(testTransactionKey), Outbound: outbound.Policy{AllowedPrefixes: outbound.LoopbackPrefixes(), RootCAs: op.pool()}, Now: op.clock.Now, } diff --git a/oidc/client/format.go b/oidc/client/format.go index a0a6f5f3..804bfe07 100644 --- a/oidc/client/format.go +++ b/oidc/client/format.go @@ -79,7 +79,8 @@ func (c *Client) String() string { func (c *Client) GoString() string { return c.String() } // Format implements fmt.Formatter like String, for every verb: without it, -// fmt prints the unexported fields of a *Client, secrets included. +// fmt prints the unexported fields of a *Client (its secrets are Secrets, +// which print as an address there). func (c *Client) Format(f fmt.State, verb rune) { formatRedacted(f, verb, c) } // LogValue implements slog.LogValuer with the Client's configuration, its diff --git a/oidc/client/http.go b/oidc/client/http.go index eb886d56..47d5ab55 100644 --- a/oidc/client/http.go +++ b/oidc/client/http.go @@ -56,7 +56,7 @@ func (c *Client) StartLogin(w http.ResponseWriter, r *http.Request, opts AuthOpt } c.expireOldCookies(w, r) - http.SetCookie(w, c.bindingCookie(c.cookieName(ar.State), ar.Binding, int(c.cfg.TransactionTTL/time.Second))) + http.SetCookie(w, c.bindingCookie(c.cookieName(ar.State), ar.Binding.Reveal(), int(c.cfg.TransactionTTL/time.Second))) w.Header().Set("Cache-Control", "no-store") w.Header().Set("Referrer-Policy", "no-referrer") http.Redirect(w, r, ar.URL, http.StatusSeeOther) @@ -93,13 +93,13 @@ func (c *Client) HandleCallback(w http.ResponseWriter, r *http.Request) (*Identi return nil, err } - var binding string + var binding Secret if states := params[paramState]; len(states) == 1 { if _, ok := decodeSecret(states[0]); ok { name := c.cookieName(states[0]) if ck, err := r.Cookie(name); err == nil { - binding = ck.Value + binding = NewSecret(ck.Value) } http.SetCookie(w, c.bindingCookie(name, "", -1)) diff --git a/oidc/client/identity.go b/oidc/client/identity.go index cf786a4a..27f36afa 100644 --- a/oidc/client/identity.go +++ b/oidc/client/identity.go @@ -25,12 +25,12 @@ import ( // can assert the same value. With pairwise subject identifiers the same // person has a different Subject for every client. // -// Tokens holds credentials. fmt and log/slog print Identity without them, -// but encoding/json writes them — deliberately, so that an application can -// persist an Identity (the golang.org/x/oauth2 convention for tokens). An -// Identity nested in a value that log/slog's JSON handler logs (a struct -// field, a map) is therefore written in full, tokens and claims included: -// log it as an attribute of its own, which LogValue redacts. +// Tokens holds credentials, as Secrets: fmt, log/slog and encoding/json +// print an Identity without them, wherever it sits — an application +// persisting tokens reveals them explicitly (Secret.Reveal). Its claims are +// not secrets: an Identity nested in a value that log/slog's JSON handler +// logs (a struct field, a map) is written with them; log it as an +// attribute of its own, which LogValue reduces to its identifiers. type Identity struct { // ConnectionID is the Config.ConnectionID of the Client. ConnectionID string @@ -88,20 +88,20 @@ func (id *Identity) DecodeClaims(v any) error { return nil } -// Tokens are the tokens of a login. They are credentials: String, GoString, -// Format and LogValue redact them, but encoding/json writes them — -// deliberately, for persistence — including when log/slog's JSON handler -// encodes Tokens nested in another value. +// Tokens are the tokens of a login. They are credentials, held as Secrets: +// they never print, log nor encode — encoding/json writes them REDACTED. +// Reveal them to use them (an API call, the application's own token +// storage). type Tokens struct { // AccessToken authorizes calls to the provider's APIs and UserInfo. - AccessToken string + AccessToken Secret // TokenType is the token_type, Bearer. TokenType string - // RefreshToken is the refresh token, "" when none was issued. The + // RefreshToken is the refresh token, empty when none was issued. The // application redeems it itself. - RefreshToken string + RefreshToken Secret // IDToken is the compact ID Token the Identity was validated from. - IDToken string + IDToken Secret // Scope is the granted scope when the response states it. Scope string // Expiry is when the access token expires, zero when unstated. diff --git a/oidc/client/idtoken.go b/oidc/client/idtoken.go index 226790a6..2c6b0ec2 100644 --- a/oidc/client/idtoken.go +++ b/oidc/client/idtoken.go @@ -69,7 +69,7 @@ type verifiedTimes struct { // verifyLogin validates the ID Token of tokens and builds the Identity. func (c *Client) verifyLogin(ctx context.Context, tokens Tokens, tx *Transaction, nonce string) (*Identity, error) { - alg, payload, err := c.verifyIDToken(ctx, tokens.IDToken) + alg, payload, err := c.verifyIDToken(ctx, tokens.IDToken.Reveal()) if err != nil { return nil, err } @@ -93,7 +93,7 @@ func (c *Client) verifyLogin(ctx context.Context, tokens Tokens, tx *Transaction return nil, err } - if claims.ATHash != nil && subtle.ConstantTimeCompare([]byte(*claims.ATHash), []byte(accessTokenHash(alg, tokens.AccessToken))) != 1 { + if claims.ATHash != nil && subtle.ConstantTimeCompare([]byte(*claims.ATHash), []byte(accessTokenHash(alg, tokens.AccessToken.Reveal()))) != 1 { return nil, ErrAccessTokenHash } diff --git a/oidc/client/integrity_test.go b/oidc/client/integrity_test.go index 7e3bc22a..8dba7573 100644 --- a/oidc/client/integrity_test.go +++ b/oidc/client/integrity_test.go @@ -131,7 +131,7 @@ func TestStoreWriterCannotForgeIntegrity(t *testing.T) { c := op.client(t, func(cfg *oidcclient.Config) { cfg.Store = store }) ar, code := op.begin(t, c, step.opts) - store.binding = ar.Binding + store.binding = ar.Binding.Reveal() id, err := c.Complete(context.Background(), callbackFor(ar, code)) require.ErrorIs(t, err, oidcclient.ErrTransactionIntegrity) @@ -150,7 +150,7 @@ func TestStoreWriterCannotForgeIntegrity(t *testing.T) { c := op.client(t, func(cfg *oidcclient.Config) { cfg.Store = store }) ar, code := op.begin(t, c, step.opts) - store.binding = ar.Binding + store.binding = ar.Binding.Reveal() id, err := c.Complete(context.Background(), callbackFor(ar, code)) require.NoError(t, err, "the forgery with the real key is the package's own MAC") @@ -190,8 +190,10 @@ func TestTransactionKeyRotation(t *testing.T) { op := newFakeOP(t) store := memory.New() - starting := op.client(t, func(cfg *oidcclient.Config) { cfg.Store, cfg.TransactionKeys = store, tc.begin }) - completing := op.client(t, func(cfg *oidcclient.Config) { cfg.Store, cfg.TransactionKeys = store, tc.complete }) + starting := op.client(t, func(cfg *oidcclient.Config) { cfg.Store, cfg.TransactionKeys = store, secretKeys(tc.begin...) }) + completing := op.client(t, func(cfg *oidcclient.Config) { + cfg.Store, cfg.TransactionKeys = store, secretKeys(tc.complete...) + }) ar, code := op.begin(t, starting, oidcclient.AuthOptions{}) diff --git a/oidc/client/params_test.go b/oidc/client/params_test.go index 03a16ce1..a644bfb0 100644 --- a/oidc/client/params_test.go +++ b/oidc/client/params_test.go @@ -125,7 +125,7 @@ func TestBeginSendsEveryParameterOnce(t *testing.T) { ar, err := c.Begin(context.Background(), oidcclient.AuthOptions{ LoginHint: "jane@example.com", - IDTokenHint: "eyJ.x.y", + IDTokenHint: oidcclient.NewSecret("eyJ.x.y"), Prompt: []string{"login"}, MaxAge: &maxAge, ACRValues: []string{"urn:mfa"}, diff --git a/oidc/client/redaction_test.go b/oidc/client/redaction_test.go index 2349f42b..1458deba 100644 --- a/oidc/client/redaction_test.go +++ b/oidc/client/redaction_test.go @@ -95,15 +95,15 @@ func TestSecretsAreRedacted(t *testing.T) { c := op.client(t) ar, code := op.begin(t, c, oidcclient.AuthOptions{}) - assertRedacted(t, *ar, ar.Binding) - assertRedacted(t, ar, ar.Binding) + assertRedacted(t, *ar, ar.Binding.Reveal()) + assertRedacted(t, ar, ar.Binding.Reveal()) assert.Contains(t, logged(*ar), "REDACTED") cb := callbackFor(ar, code) require.Contains(t, cb.URL.RawQuery, code, "the received URL carries the response in query mode") - assertRedacted(t, cb, ar.Binding, code, ar.State) - assertRedacted(t, &cb, ar.Binding, code, ar.State) + assertRedacted(t, cb, ar.Binding.Reveal(), code, ar.State) + assertRedacted(t, &cb, ar.Binding.Reveal(), code, ar.State) assert.Contains(t, logged(cb), testRedirectURL, "the URL is logged without its query") assert.Contains(t, logged(oidcclient.Callback{}), `URL: ""`, "a callback without URL") @@ -111,7 +111,7 @@ func TestSecretsAreRedacted(t *testing.T) { require.NoError(t, err) for _, v := range []any{id.Tokens, &id.Tokens, *id, id} { - assertRedacted(t, v, id.Tokens.AccessToken, id.Tokens.IDToken) + assertRedacted(t, v, id.Tokens.AccessToken.Reveal(), id.Tokens.IDToken.Reveal()) } assert.Contains(t, logged(id), id.Subject, "an identity stays recognizable") @@ -164,7 +164,9 @@ func TestConfigRedactsSecrets(t *testing.T) { assert.Contains(t, logged(v), testConnection, "the identifying fields stay readable") } - public := op.config(func(c *oidcclient.Config) { c.AuthMethod, c.ClientSecret = oidcclient.AuthMethodNone, "" }) + public := op.config(func(c *oidcclient.Config) { + c.AuthMethod, c.ClientSecret = oidcclient.AuthMethodNone, oidcclient.Secret{} + }) assert.Contains(t, public.String(), `ClientSecret: ""`, "a public client has no secret to redact") var none *oidcclient.Client @@ -225,7 +227,7 @@ func TestLoginSecretsStayOutOfJSON(t *testing.T) { cb := callbackFor(ar, code) for _, v := range []any{*ar, ar} { - assertEncodedRedacted(t, v, ar.Binding) + assertEncodedRedacted(t, v, ar.Binding.Reveal()) b, err := json.Marshal(v) require.NoError(t, err) @@ -234,7 +236,7 @@ func TestLoginSecretsStayOutOfJSON(t *testing.T) { } for _, v := range []any{cb, &cb} { - assertEncodedRedacted(t, v, ar.Binding, code, ar.State) + assertEncodedRedacted(t, v, ar.Binding.Reveal(), code, ar.State) b, err := json.Marshal(v) require.NoError(t, err) @@ -258,7 +260,9 @@ func TestLoginSecretsStayOutOfJSON(t *testing.T) { testConnection, op.issuer, testClientID, testRedirectURL), string(b)) } - public := op.config(func(c *oidcclient.Config) { c.AuthMethod, c.ClientSecret = oidcclient.AuthMethodNone, "" }) + public := op.config(func(c *oidcclient.Config) { + c.AuthMethod, c.ClientSecret = oidcclient.AuthMethodNone, oidcclient.Secret{} + }) b, err = json.Marshal(public) require.NoError(t, err) assert.Contains(t, string(b), `"ClientSecret":""`, "a public client has no secret to redact") @@ -276,7 +280,7 @@ func TestLoginSecretsStayOutOfJSON(t *testing.T) { func TestEmbeddingPromotesTheRedactedEncoding(t *testing.T) { t.Parallel() - ar := oidcclient.AuthRequest{URL: "https://op.example/authorize", State: "s", Binding: "secret-binding"} + ar := oidcclient.AuthRequest{URL: "https://op.example/authorize", State: "s", Binding: oidcclient.NewSecret("secret-binding")} type loginState struct { oidcclient.AuthRequest @@ -319,7 +323,7 @@ func TestEmbeddingPromotesTheRedactedEncoding(t *testing.T) { require.NoError(t, err) cb := oidcclient.Callback{ - Mode: oidcclient.ResponseModeQuery, URL: received, Binding: "callback-binding", + Mode: oidcclient.ResponseModeQuery, URL: received, Binding: oidcclient.NewSecret("callback-binding"), Params: url.Values{"code": {"code-xyz"}, "state": {"state-xyz"}}, } @@ -352,10 +356,11 @@ type stamp struct{ At string } func (s stamp) MarshalJSON() ([]byte, error) { return json.Marshal(s.At) } -// TestIdentityAndTokensStayJSONSerialisable pins the documented exception: -// an Identity — its Tokens included — encodes and decodes in full, so that -// an application can persist it (the golang.org/x/oauth2 convention). -func TestIdentityAndTokensStayJSONSerialisable(t *testing.T) { +// TestIdentityEncodesWithoutItsTokens checks that an Identity encodes to +// JSON with its tokens REDACTED (they are Secrets) and its other fields as +// they are, and that the document does not decode back into an Identity: +// an application persists tokens by revealing them explicitly. +func TestIdentityEncodesWithoutItsTokens(t *testing.T) { t.Parallel() op := newFakeOP(t) @@ -365,11 +370,23 @@ func TestIdentityAndTokensStayJSONSerialisable(t *testing.T) { b, err := json.Marshal(id) require.NoError(t, err) + var doc struct { + Subject string + Claims json.RawMessage + Tokens map[string]any + } + + require.NoError(t, json.Unmarshal(b, &doc)) + assert.Equal(t, id.Subject, doc.Subject) + assert.JSONEq(t, string(id.Claims), string(doc.Claims), "claims are not secrets") + assert.Equal(t, "REDACTED", doc.Tokens["AccessToken"]) + assert.Equal(t, "REDACTED", doc.Tokens["RefreshToken"]) + assert.Equal(t, "REDACTED", doc.Tokens["IDToken"]) + assert.Equal(t, "Bearer", doc.Tokens["TokenType"]) + assert.NotContains(t, string(b), id.Tokens.IDToken.Reveal()) + var back oidcclient.Identity - require.NoError(t, json.Unmarshal(b, &back)) - assert.Equal(t, id.Tokens, back.Tokens) - assert.Equal(t, id.Subject, back.Subject) - assert.JSONEq(t, string(id.Claims), string(back.Claims)) + require.Error(t, json.Unmarshal(b, &back), "a redacted document never decodes back into credentials") } // TestUserInfoIsRedacted checks that UserInfo prints and logs its subject diff --git a/oidc/client/secret.go b/oidc/client/secret.go new file mode 100644 index 00000000..6e4e30eb --- /dev/null +++ b/oidc/client/secret.go @@ -0,0 +1,86 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oidcclient + +import ( + "encoding/json" + "fmt" + "log/slog" +) + +// Secret is a credential of the Relying Party: the client secret, a +// transaction key, the binding secret of a login, a token. Reveal returns +// its value; every other rendering prints REDACTED — "" for an empty +// Secret — so that no log, error or dump carries it: +// +// - fmt, with every verb, flag and width (Format), String and GoString; +// - log/slog, as an attribute (LogValue) or nested in a struct that the +// text handler prints with fmt; +// - encoding/json (MarshalJSON), log/slog's JSON handler included. A +// Secret does not decode from JSON: a REDACTED document never turns +// back into a credential; +// - a Secret reached through an unexported field of another value, which +// fmt prints by reflection without calling any method: its value sits +// behind a pointer to a string, which fmt then renders as an address, +// whatever the verb. encoding/gob refuses a Secret (it has no exported +// field) and encoding/xml writes it empty. +// +// The zero Secret is empty. A Secret is immutable and safe to copy and +// share between goroutines. Compare two Secrets with crypto/subtle on their +// revealed values, never with ==, which compares their identity. Reveal a +// Secret only where the credential is used: in a request to the provider, +// in a cookie, in the application's own secure storage. +type Secret struct { + // v points to a string — never a struct, slice or map, which fmt + // expands when it reports a bad verb for a value it reached by + // reflection, printing their content. nil for an empty Secret. + v *string +} + +// NewSecret returns the Secret holding value; the zero Secret for "". +func NewSecret(value string) Secret { + if value == "" { + return Secret{} + } + + return Secret{v: &value} +} + +// Reveal returns the value of the Secret, "" for an empty one. +func (s Secret) Reveal() string { + if s.v == nil { + return "" + } + + return *s.v +} + +// String implements fmt.Stringer: REDACTED, or "" when the Secret is empty. +func (s Secret) String() string { + if s.Reveal() == "" { + return `""` + } + + return redactedMark +} + +// GoString implements fmt.GoStringer like String. +func (s Secret) GoString() string { return s.String() } + +// Format implements fmt.Formatter like String, for every verb. +func (s Secret) Format(f fmt.State, verb rune) { formatRedacted(f, verb, s) } + +// LogValue implements slog.LogValuer like String. +func (s Secret) LogValue() slog.Value { return slog.StringValue(s.String()) } + +// MarshalJSON implements json.Marshaler: "REDACTED", or "" when the Secret +// is empty. +func (s Secret) MarshalJSON() ([]byte, error) { + if s.Reveal() == "" { + return []byte(`""`), nil + } + + return json.Marshal(redactedMark) //nolint:wrapcheck // a constant string: never fails +} diff --git a/oidc/client/secret_test.go b/oidc/client/secret_test.go new file mode 100644 index 00000000..f6a081c6 --- /dev/null +++ b/oidc/client/secret_test.go @@ -0,0 +1,181 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oidcclient_test + +import ( + "bytes" + "context" + "encoding/gob" + "encoding/hex" + "encoding/json" + "encoding/xml" + "fmt" + "log/slog" + "strings" + "testing" + + oidcclient "github.com/hyperscale-stack/security/oidc/client" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// everyVerb is every fmt verb, with flags and widths, an application may +// print a value with — including the verbs a struct or a pointer does not +// take, for which fmt reports a bad verb and prints the value again. +var everyVerb = []string{ + "%v", "%+v", "%#v", "%s", "%q", "%x", "%X", "%d", "%t", "%e", "%c", "%U", "%b", "%o", "%p", "%10s", "%-q", "%08.3f", +} + +// secretForms are the ways fmt may spell secret: the text itself, its hex +// encodings and the per-byte lists fmt prints for a byte slice. +func secretForms(secret string) []string { + forms := []string{secret, hex.EncodeToString([]byte(secret)), strings.ToUpper(hex.EncodeToString([]byte(secret)))} + + for _, verb := range []string{"%d", "%o", "%b", "%c", "%U"} { + forms = append(forms, strings.Trim(fmt.Sprintf(verb, []byte(secret)), "[]")) + } + + return forms +} + +// everyRendering renders v with every verb, log/slog's text and JSON +// handlers, encoding/json, encoding/gob and encoding/xml. +func everyRendering(v any) map[string]string { + out := map[string]string{} + + for _, verb := range everyVerb { + out[verb] = fmt.Sprintf(verb, v) + } + + var text, js strings.Builder + + slog.New(slog.NewTextHandler(&text, nil)).Info("value", "v", v) + slog.New(slog.NewJSONHandler(&js, nil)).Info("value", "v", v) + out["slog text"], out["slog JSON"] = text.String(), js.String() + + encoded, _ := json.Marshal(v) + out["encoding/json"] = string(encoded) + + var g bytes.Buffer + + _ = gob.NewEncoder(&g).Encode(v) + out["encoding/gob"] = g.String() + + encoded, _ = xml.Marshal(v) + out["encoding/xml"] = string(encoded) + + return out +} + +// TestSecretsNeverPrint checks every secret of the package's values — the +// client secret, the transaction keys, the binding secret, the tokens — +// under every verb and encoder, wherever the value sits: printed directly, +// in a slice or a map, in an exported field, and in an unexported field of +// an application's struct, which fmt prints by reflection without calling +// any method of the value. +func TestSecretsNeverPrint(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + op.set(func(op *fakeOP) { + op.accessToken = "an-access-token-long-enough-to-be-unmistakable" + op.tokenHook = func(_ *grant, body map[string]any) { + body["refresh_token"] = "a-refresh-token-long-enough-to-be-unmistakable" + } + }) + cfg := op.config() + c := op.client(t) + + ar, code := op.begin(t, c, oidcclient.AuthOptions{}) + cb := callbackFor(ar, code) + + id, err := c.Complete(context.Background(), cb) + require.NoError(t, err) + require.NotEmpty(t, id.Tokens.RefreshToken) + + configSecrets := []string{testClientSecret, string(testTransactionKey)} + tokens := []string{id.Tokens.AccessToken.Reveal(), id.Tokens.RefreshToken.Reveal(), id.Tokens.IDToken.Reveal()} + hint := oidcclient.AuthOptions{IDTokenHint: id.Tokens.IDToken} + + secret := oidcclient.NewSecret(testClientSecret) + + shapes := []struct { + name string + value any + secrets []string + }{ + {"Secret", secret, configSecrets}, + {"pointer to a Secret", &secret, configSecrets}, + {"slice of Secrets", []oidcclient.Secret{secret}, configSecrets}, + {"Secret in a map", map[string]oidcclient.Secret{"k": secret}, configSecrets}, + {"Secret in an unexported field", struct{ s oidcclient.Secret }{secret}, configSecrets}, + {"pointer to a Secret in an unexported field", struct{ s *oidcclient.Secret }{&secret}, configSecrets}, + {"Client value", *c, configSecrets}, + {"Client in an unexported field", struct{ c oidcclient.Client }{*c}, configSecrets}, + {"slice of Client values", []oidcclient.Client{*c}, configSecrets}, + {"Config", cfg, configSecrets}, + {"Config in an unexported field", struct{ cfg oidcclient.Config }{cfg}, configSecrets}, + {"Config in a map", map[string]oidcclient.Config{"acme": cfg}, configSecrets}, + {"AuthRequest in an unexported field", struct{ ar oidcclient.AuthRequest }{*ar}, []string{ar.Binding.Reveal()}}, + {"Callback in an unexported field", struct{ cb oidcclient.Callback }{cb}, []string{ar.Binding.Reveal()}}, + {"Identity in an unexported field", struct{ id oidcclient.Identity }{*id}, tokens}, + {"Identity in an exported field", struct{ ID oidcclient.Identity }{*id}, tokens}, + {"Tokens in an unexported field", struct{ t oidcclient.Tokens }{id.Tokens}, tokens}, + {"Tokens in an exported field", struct{ T *oidcclient.Tokens }{&id.Tokens}, tokens}, + {"AuthOptions with an ID Token hint", hint, tokens[2:]}, + {"AuthOptions in an unexported field", struct{ o oidcclient.AuthOptions }{hint}, tokens[2:]}, + } + + for _, shape := range shapes { + t.Run(shape.name, func(t *testing.T) { + t.Parallel() + + for way, rendered := range everyRendering(shape.value) { + for _, secret := range shape.secrets { + for _, form := range secretForms(secret) { + assert.NotContains(t, rendered, form, "%s reveals a secret", way) + } + } + } + }) + } +} + +// TestSecret checks the Secret type itself: its value, its renderings and +// its encodings, empty and not. +func TestSecret(t *testing.T) { + t.Parallel() + + secret := oidcclient.NewSecret("s3cr3t") + assert.Equal(t, "s3cr3t", secret.Reveal()) + assert.Equal(t, oidcclient.Secret{}, oidcclient.NewSecret(""), "the empty Secret is the zero Secret") + assert.Empty(t, oidcclient.Secret{}.Reveal()) + + for _, verb := range everyVerb { + if verb == "%p" { // fmt handles %p before any method: the address of the value + continue + } + + assert.Equal(t, "REDACTED", fmt.Sprintf(verb, secret), verb) + assert.Equal(t, `""`, fmt.Sprintf(verb, oidcclient.Secret{}), verb) + } + + assert.Equal(t, "REDACTED", secret.LogValue().String()) + assert.Equal(t, `""`, oidcclient.Secret{}.LogValue().String()) + + var text strings.Builder + + slog.New(slog.NewTextHandler(&text, nil)).Info("m", "secret", secret) + assert.Contains(t, text.String(), "secret=REDACTED") + + encoded, err := json.Marshal(struct{ A, B oidcclient.Secret }{secret, oidcclient.Secret{}}) + require.NoError(t, err) + assert.JSONEq(t, `{"A":"REDACTED","B":""}`, string(encoded)) + + var back struct{ A oidcclient.Secret } + require.Error(t, json.Unmarshal(encoded, &back), "a Secret never decodes from JSON") + + require.Error(t, gob.NewEncoder(&bytes.Buffer{}).Encode(secret), "encoding/gob refuses a Secret") +} diff --git a/oidc/client/secrets.go b/oidc/client/secrets.go index dde2dd0a..b9395742 100644 --- a/oidc/client/secrets.go +++ b/oidc/client/secrets.go @@ -129,9 +129,9 @@ func integrityFor(serverKey, secret []byte, tx *Transaction) string { // verifyIntegrity reports whether tx.Integrity is the MAC of tx under one // of the transaction keys, comparing in constant time. -func verifyIntegrity(keys [][]byte, secret []byte, tx *Transaction) bool { +func verifyIntegrity(keys []Secret, secret []byte, tx *Transaction) bool { for _, key := range keys { - if hmac.Equal([]byte(integrityFor(key, secret, tx)), []byte(tx.Integrity)) { + if hmac.Equal([]byte(integrityFor([]byte(key.Reveal()), secret, tx)), []byte(tx.Integrity)) { return true } } diff --git a/oidc/client/token.go b/oidc/client/token.go index 63d84e37..9b03cc32 100644 --- a/oidc/client/token.go +++ b/oidc/client/token.go @@ -59,7 +59,7 @@ func (c *Client) exchange(ctx context.Context, code, verifier string) (Tokens, e switch c.cfg.AuthMethod { case AuthMethodClientSecretPost: form.Set(paramClientID, c.cfg.ClientID) - form.Set(paramClientSecret, c.cfg.ClientSecret) + form.Set(paramClientSecret, c.cfg.ClientSecret.Reveal()) case AuthMethodNone: form.Set(paramClientID, c.cfg.ClientID) case AuthMethodClientSecretBasic: @@ -76,7 +76,7 @@ func (c *Client) exchange(ctx context.Context, code, verifier string) (Tokens, e if c.cfg.AuthMethod == AuthMethodClientSecretBasic { // RFC 6749 §2.3.1: identifier and secret are form-urlencoded // (Appendix B) before the Basic encoding. - req.SetBasicAuth(url.QueryEscape(c.cfg.ClientID), url.QueryEscape(c.cfg.ClientSecret)) + req.SetBasicAuth(url.QueryEscape(c.cfg.ClientID), url.QueryEscape(c.cfg.ClientSecret.Reveal())) } resp, err := c.send.Do(req) //nolint:gosec // G107/G704: endpoint from validated metadata, outbound policy enforced @@ -153,10 +153,10 @@ func parseTokenResponse(body []byte, now time.Time) (Tokens, error) { } tokens := Tokens{ - AccessToken: r.AccessToken, + AccessToken: NewSecret(r.AccessToken), TokenType: r.TokenType, - RefreshToken: r.RefreshToken, - IDToken: r.IDToken, + RefreshToken: NewSecret(r.RefreshToken), + IDToken: NewSecret(r.IDToken), } seconds, present, err := parseExpiresIn(r.ExpiresIn) diff --git a/oidc/client/token_test.go b/oidc/client/token_test.go index 0e2eabaa..e6048ec2 100644 --- a/oidc/client/token_test.go +++ b/oidc/client/token_test.go @@ -68,7 +68,7 @@ func TestTokenClientAuthentication(t *testing.T) { }) c := op.client(t, func(cfg *oidcclient.Config) { cfg.ClientID = "client:1" - cfg.ClientSecret = "a b:c%d+é" + cfg.ClientSecret = oidcclient.NewSecret("a b:c%d+é") }) _, err := op.login(t, c) @@ -106,7 +106,7 @@ func TestTokenClientAuthentication(t *testing.T) { op.set(func(op *fakeOP) { op.authMethod = oidcclient.AuthMethodNone }) c := op.client(t, func(cfg *oidcclient.Config) { cfg.AuthMethod = oidcclient.AuthMethodNone - cfg.ClientSecret = "" + cfg.ClientSecret = oidcclient.Secret{} }) _, err := op.login(t, c) diff --git a/oidc/client/userinfo.go b/oidc/client/userinfo.go index 4efa3826..d9967b81 100644 --- a/oidc/client/userinfo.go +++ b/oidc/client/userinfo.go @@ -78,7 +78,7 @@ func (c *Client) UserInfo(ctx context.Context, id *Identity) (*UserInfo, error) return nil, fmt.Errorf("%w: %w", ErrUserInfo, err) } - req.Header.Set("Authorization", "Bearer "+id.Tokens.AccessToken) + req.Header.Set("Authorization", "Bearer "+id.Tokens.AccessToken.Reveal()) req.Header.Set("Accept", mediaJSON) resp, err := c.send.Do(req) //nolint:gosec // G107/G704: endpoint from validated metadata, outbound policy enforced @@ -124,7 +124,7 @@ func (c *Client) checkUserInfoRequest(id *Identity) error { return fmt.Errorf("%w: no identity", ErrUserInfo) case id.ConnectionID != c.cfg.ConnectionID || id.Issuer != c.md.Issuer: return fmt.Errorf("%w: identity of another connection", ErrUserInfo) - case id.Subject == "" || id.Tokens.AccessToken == "": + case id.Subject == "" || id.Tokens.AccessToken.Reveal() == "": return fmt.Errorf("%w: identity without subject or access token", ErrUserInfo) case c.md.UserInfoEndpoint == "": return fmt.Errorf("%w: the provider has no UserInfo endpoint", ErrUserInfo) diff --git a/oidc/client/userinfo_test.go b/oidc/client/userinfo_test.go index 435c3a4c..263152c7 100644 --- a/oidc/client/userinfo_test.go +++ b/oidc/client/userinfo_test.go @@ -159,7 +159,7 @@ func TestUserInfoRefusesForeignIdentity(t *testing.T) { "other connection": func(id *oidcclient.Identity) { id.ConnectionID = "other" }, "other issuer": func(id *oidcclient.Identity) { id.Issuer = "https://other.example.com" }, "no subject": func(id *oidcclient.Identity) { id.Subject = "" }, - "no access token": func(id *oidcclient.Identity) { id.Tokens.AccessToken = "" }, + "no access token": func(id *oidcclient.Identity) { id.Tokens.AccessToken = oidcclient.Secret{} }, } for name, mutate := range cases { From 29196eef9d73b8261bf2e8758f511478ef3de3fd Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 10:41:33 +0200 Subject: [PATCH 02/10] fix(oidc/client)!: refuse InsecureCookies unless the redirect is loopback 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. --- CHANGELOG.md | 12 +++ MIGRATION.md | 22 ++++++ docs/security-considerations.md | 5 +- oidc/client/config.go | 15 +++- oidc/client/config_test.go | 134 ++++++++++++++++++-------------- oidc/client/fakeop_test.go | 7 +- oidc/client/http.go | 2 +- oidc/client/http_test.go | 49 ++++++++++-- 8 files changed, 172 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e71b912d..07ae8fa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1035,6 +1035,10 @@ legacy packages (`authentication/`, `authorization/`, the in-tree encodes as `REDACTED` everywhere, `encoding/json` included: `Identity` and `Tokens` no longer encode their tokens, which applications persist by revealing them (see MIGRATION.md). +- `oidcclient.Config.InsecureCookies` is refused (`ErrInvalidConfig`) + unless `RedirectURL` is plain http, which is accepted on a loopback host + only (breaking for a configuration combining it with an https redirect + URI; see MIGRATION.md). - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in @@ -2230,3 +2234,11 @@ legacy packages (`authentication/`, `authorization/`, the in-tree and the tokens of an `Identity` reached logs — `slog`'s text handler included. They are now `oidcclient.Secret` values (see Changed), which print as an address there and `REDACTED` everywhere else, with every verb. +- `oidcclient.Config.InsecureCookies`, documented for local development, + was accepted with a public https `RedirectURL`: the binding cookie then + lost its `__Host-` prefix and `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 — signing the victim in as the attacker + (login CSRF, RFC 9700 §4.7; rfc6265bis §4.1.3.2). `New` now refuses it + unless the redirect URI is http on a loopback host. diff --git a/MIGRATION.md b/MIGRATION.md index f1e68bfa..13a5e7fd 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3059,3 +3059,25 @@ To migrate: encoding of `Identity` and `Tokens` now writes them `REDACTED`. 5. Compare two `Secret`s with `crypto/subtle.ConstantTimeCompare` on their revealed values, never with `==` (which compares their identity). + +### `oidc/client`: `InsecureCookies` only with a loopback http redirect URI + +`Config.InsecureCookies` sets the binding cookie without the `__Host-` +prefix and the `Secure` attribute, for local development over plain http. +It was accepted with an https `RedirectURL` too, where it removes the +protection of the cookie against a sibling subdomain or a network attacker +planting one (login CSRF, RFC 9700 §4.7; rfc6265bis §4.1.3.2). + +| `InsecureCookies: true` with | Before | After | +| --- | --- | --- | +| an http `RedirectURL` (loopback host: `localhost`, `127.0.0.1`, `[::1]`) | accepted | unchanged | +| an https `RedirectURL`, loopback host included | accepted; plain `oidc-…` cookie without `Secure` | `New` fails with `ErrInvalidConfig` | +| `ResponseMode: form_post` | `ErrInvalidConfig` | unchanged | + +To migrate: + +1. Set `InsecureCookies` only where the application is served over plain + http on the loopback interface (`examples/oidc-client` sets it when + `BASE_URL` is http). +2. Over https — including `https://localhost` — drop it: the `__Host-` + cookie works on any https origin. diff --git a/docs/security-considerations.md b/docs/security-considerations.md index 92cf62d7..8af21b97 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1466,8 +1466,9 @@ are attacker-influenced. sharing the prefix. SameSite is Lax for the `query` response mode — the callback is a top-level GET — and None for `form_post`, whose cross-site POST carries no Lax cookie; an opt-in headless-Chrome test checks both. - `InsecureCookies` drops Secure for local http development only, and is - refused with `form_post`. + `InsecureCookies` drops the prefix and Secure for local http development + only: `New` refuses it unless the redirect URI is http — on a loopback + host, the only http redirect URI accepted — and with `form_post`. - **CSRF** — exempt the callback route from CSRF middleware in `form_post` mode: the cross-site POST is the protocol, the binding cookie its defense. Start logins on a user action from the application's own diff --git a/oidc/client/config.go b/oidc/client/config.go index de30c5cc..ae1c0ac7 100644 --- a/oidc/client/config.go +++ b/oidc/client/config.go @@ -203,8 +203,14 @@ type Config struct { // InsecureCookies makes the net/http helpers set the binding cookie // without the __Host- prefix and the Secure attribute, for local - // development over plain http. Refused with ResponseModeFormPost: - // browsers refuse SameSite=None cookies that are not Secure. + // development over plain http. New refuses it unless RedirectURL is + // http — on a loopback host, the only http RedirectURL accepted: on an + // https site, the prefix and Secure are what keep a sibling subdomain, + // or a network attacker answering for the http origin, from planting a + // binding cookie of its own login and signing the victim in as the + // attacker (rfc6265bis-22 §4.1.3.2, §8.6; RFC 9700 §4.7). Refused with + // ResponseModeFormPost too: browsers refuse SameSite=None cookies that + // are not Secure. InsecureCookies bool // Now is the clock; time.Now when nil. @@ -227,6 +233,11 @@ func (c Config) normalized() (Config, *url.URL, error) { return Config{}, nil, err } + // parseRedirectURL admits plain http on a loopback host only. + if c.InsecureCookies && redirect.Scheme != schemeHTTP { + return Config{}, nil, fmt.Errorf("%w: InsecureCookies requires an http RedirectURL on a loopback host", ErrInvalidConfig) + } + if err := c.validateFlow(); err != nil { return Config{}, nil, err } diff --git a/oidc/client/config_test.go b/oidc/client/config_test.go index 20f79ca2..4d2cc9df 100644 --- a/oidc/client/config_test.go +++ b/oidc/client/config_test.go @@ -30,57 +30,66 @@ func TestNewRejectsInvalidConfig(t *testing.T) { }) cases := map[string]func(*oidcclient.Config){ - "empty connection": func(c *oidcclient.Config) { c.ConnectionID = "" }, - "connection with space": func(c *oidcclient.Config) { c.ConnectionID = "acme okta" }, - "connection too long": func(c *oidcclient.Config) { c.ConnectionID = strings.Repeat("c", 256) }, - "empty issuer": func(c *oidcclient.Config) { c.Issuer = "" }, - "http issuer": func(c *oidcclient.Config) { c.Issuer = strings.Replace(c.Issuer, "https://", "http://", 1) }, - "relative issuer": func(c *oidcclient.Config) { c.Issuer = "/tenants/acme" }, - "issuer with query": func(c *oidcclient.Config) { c.Issuer += "?tenant=acme" }, - "issuer with empty query": func(c *oidcclient.Config) { c.Issuer += "?" }, - "issuer with fragment": func(c *oidcclient.Config) { c.Issuer += "#x" }, - "issuer with userinfo": func(c *oidcclient.Config) { c.Issuer = strings.Replace(c.Issuer, "https://", "https://u:p@", 1) }, - "issuer non-ASCII": func(c *oidcclient.Config) { c.Issuer += "/é" }, - "issuer unparsable": func(c *oidcclient.Config) { c.Issuer = "https://[::1" }, - "issuer opaque": func(c *oidcclient.Config) { c.Issuer = "https:idp.example.com" }, - "redirect unparsable": func(c *oidcclient.Config) { c.RedirectURL = "https://[::1/cb" }, - "redirect opaque": func(c *oidcclient.Config) { c.RedirectURL = "https:rp.example.com/cb" }, - "redirect malformed query": func(c *oidcclient.Config) { c.RedirectURL += "?a=%zz" }, - "empty client": func(c *oidcclient.Config) { c.ClientID = "" }, - "client with control": func(c *oidcclient.Config) { c.ClientID = "client\n1" }, - "no auth method": func(c *oidcclient.Config) { c.AuthMethod = "" }, - "unsupported auth method": func(c *oidcclient.Config) { c.AuthMethod = "private_key_jwt" }, - "basic without secret": func(c *oidcclient.Config) { c.ClientSecret = oidcclient.Secret{} }, - "post without secret": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodClientSecretPost; c.ClientSecret = oidcclient.Secret{} }, - "none with secret": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone }, - "empty redirect": func(c *oidcclient.Config) { c.RedirectURL = "" }, - "relative redirect": func(c *oidcclient.Config) { c.RedirectURL = testCallbackPath }, - "http redirect": func(c *oidcclient.Config) { c.RedirectURL = "http://rp.example.com" + testCallbackPath }, - "redirect with fragment": func(c *oidcclient.Config) { c.RedirectURL += "#x" }, - "redirect with userinfo": func(c *oidcclient.Config) { c.RedirectURL = "https://u@rp.example.com/cb" }, - "redirect query with state": func(c *oidcclient.Config) { c.RedirectURL += "?state=x" }, - "redirect query with code": func(c *oidcclient.Config) { c.RedirectURL += "?code=x" }, - "redirect query with iss": func(c *oidcclient.Config) { c.RedirectURL += "?iss=x" }, - "redirect query with error": func(c *oidcclient.Config) { c.RedirectURL += "?error=x" }, - "redirect too long": func(c *oidcclient.Config) { c.RedirectURL += strings.Repeat("a", 2048) }, - "scope with space": func(c *oidcclient.Config) { c.Scopes = []string{"openid profile"} }, - "scope with quote": func(c *oidcclient.Config) { c.Scopes = []string{`pro"file`} }, - "empty scope": func(c *oidcclient.Config) { c.Scopes = []string{""} }, - "fragment mode": func(c *oidcclient.Config) { c.ResponseMode = "fragment" }, - "unknown mode": func(c *oidcclient.Config) { c.ResponseMode = "web_message" }, - "no store": func(c *oidcclient.Config) { c.Store = nil }, - "negative ttl": func(c *oidcclient.Config) { c.TransactionTTL = -time.Second }, - "ttl over an hour": func(c *oidcclient.Config) { c.TransactionTTL = time.Hour + time.Second }, - "sub-second ttl": func(c *oidcclient.Config) { c.TransactionTTL = 1500 * time.Millisecond }, - "HS256 allowed": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{"HS256"} }, - "none allowed": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{"none"} }, - "unknown algorithm": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{"RS1"} }, - "empty trusted audience": func(c *oidcclient.Config) { c.TrustedAudiences = []string{""} }, - "negative skew": func(c *oidcclient.Config) { c.ClockSkew = -time.Second }, - "skew over five minutes": func(c *oidcclient.Config) { c.ClockSkew = 5*time.Minute + time.Second }, - "insecure cookies in form_post": func(c *oidcclient.Config) { c.InsecureCookies = true; c.ResponseMode = oidcclient.ResponseModeFormPost }, - "invalid outbound policy": func(c *oidcclient.Config) { c.Outbound.MaxRedirects = -1 }, - "no transaction key": func(c *oidcclient.Config) { c.TransactionKeys = nil }, + "empty connection": func(c *oidcclient.Config) { c.ConnectionID = "" }, + "connection with space": func(c *oidcclient.Config) { c.ConnectionID = "acme okta" }, + "connection too long": func(c *oidcclient.Config) { c.ConnectionID = strings.Repeat("c", 256) }, + "empty issuer": func(c *oidcclient.Config) { c.Issuer = "" }, + "http issuer": func(c *oidcclient.Config) { c.Issuer = strings.Replace(c.Issuer, "https://", "http://", 1) }, + "relative issuer": func(c *oidcclient.Config) { c.Issuer = "/tenants/acme" }, + "issuer with query": func(c *oidcclient.Config) { c.Issuer += "?tenant=acme" }, + "issuer with empty query": func(c *oidcclient.Config) { c.Issuer += "?" }, + "issuer with fragment": func(c *oidcclient.Config) { c.Issuer += "#x" }, + "issuer with userinfo": func(c *oidcclient.Config) { c.Issuer = strings.Replace(c.Issuer, "https://", "https://u:p@", 1) }, + "issuer non-ASCII": func(c *oidcclient.Config) { c.Issuer += "/é" }, + "issuer unparsable": func(c *oidcclient.Config) { c.Issuer = "https://[::1" }, + "issuer opaque": func(c *oidcclient.Config) { c.Issuer = "https:idp.example.com" }, + "redirect unparsable": func(c *oidcclient.Config) { c.RedirectURL = "https://[::1/cb" }, + "redirect opaque": func(c *oidcclient.Config) { c.RedirectURL = "https:rp.example.com/cb" }, + "redirect malformed query": func(c *oidcclient.Config) { c.RedirectURL += "?a=%zz" }, + "empty client": func(c *oidcclient.Config) { c.ClientID = "" }, + "client with control": func(c *oidcclient.Config) { c.ClientID = "client\n1" }, + "no auth method": func(c *oidcclient.Config) { c.AuthMethod = "" }, + "unsupported auth method": func(c *oidcclient.Config) { c.AuthMethod = "private_key_jwt" }, + "basic without secret": func(c *oidcclient.Config) { c.ClientSecret = oidcclient.Secret{} }, + "post without secret": func(c *oidcclient.Config) { + c.AuthMethod = oidcclient.AuthMethodClientSecretPost + c.ClientSecret = oidcclient.Secret{} + }, + "none with secret": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone }, + "empty redirect": func(c *oidcclient.Config) { c.RedirectURL = "" }, + "relative redirect": func(c *oidcclient.Config) { c.RedirectURL = testCallbackPath }, + "http redirect": func(c *oidcclient.Config) { c.RedirectURL = "http://rp.example.com" + testCallbackPath }, + "redirect with fragment": func(c *oidcclient.Config) { c.RedirectURL += "#x" }, + "redirect with userinfo": func(c *oidcclient.Config) { c.RedirectURL = "https://u@rp.example.com/cb" }, + "redirect query with state": func(c *oidcclient.Config) { c.RedirectURL += "?state=x" }, + "redirect query with code": func(c *oidcclient.Config) { c.RedirectURL += "?code=x" }, + "redirect query with iss": func(c *oidcclient.Config) { c.RedirectURL += "?iss=x" }, + "redirect query with error": func(c *oidcclient.Config) { c.RedirectURL += "?error=x" }, + "redirect too long": func(c *oidcclient.Config) { c.RedirectURL += strings.Repeat("a", 2048) }, + "scope with space": func(c *oidcclient.Config) { c.Scopes = []string{"openid profile"} }, + "scope with quote": func(c *oidcclient.Config) { c.Scopes = []string{`pro"file`} }, + "empty scope": func(c *oidcclient.Config) { c.Scopes = []string{""} }, + "fragment mode": func(c *oidcclient.Config) { c.ResponseMode = "fragment" }, + "unknown mode": func(c *oidcclient.Config) { c.ResponseMode = "web_message" }, + "no store": func(c *oidcclient.Config) { c.Store = nil }, + "negative ttl": func(c *oidcclient.Config) { c.TransactionTTL = -time.Second }, + "ttl over an hour": func(c *oidcclient.Config) { c.TransactionTTL = time.Hour + time.Second }, + "sub-second ttl": func(c *oidcclient.Config) { c.TransactionTTL = 1500 * time.Millisecond }, + "HS256 allowed": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{"HS256"} }, + "none allowed": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{"none"} }, + "unknown algorithm": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{"RS1"} }, + "empty trusted audience": func(c *oidcclient.Config) { c.TrustedAudiences = []string{""} }, + "negative skew": func(c *oidcclient.Config) { c.ClockSkew = -time.Second }, + "skew over five minutes": func(c *oidcclient.Config) { c.ClockSkew = 5*time.Minute + time.Second }, + "insecure cookies in form_post": func(c *oidcclient.Config) { + c.InsecureCookies, c.RedirectURL, c.ResponseMode = true, testLocalRedirectURL, oidcclient.ResponseModeFormPost + }, + "insecure cookies with an https redirect": func(c *oidcclient.Config) { c.InsecureCookies = true }, + "insecure cookies with an https loopback redirect": func(c *oidcclient.Config) { + c.InsecureCookies, c.RedirectURL = true, "https://localhost:8443/cb" + }, + "invalid outbound policy": func(c *oidcclient.Config) { c.Outbound.MaxRedirects = -1 }, + "no transaction key": func(c *oidcclient.Config) { c.TransactionKeys = nil }, "short transaction key": func(c *oidcclient.Config) { c.TransactionKeys = secretKeys(make([]byte, oidcclient.MinTransactionKeySize-1)) }, @@ -116,14 +125,21 @@ func TestNewAcceptsBoundaries(t *testing.T) { "redirect with app query": func(c *oidcclient.Config) { c.RedirectURL += "?tenant=acme" }, "one hour ttl": func(c *oidcclient.Config) { c.TransactionTTL = time.Hour }, "five minutes skew": func(c *oidcclient.Config) { c.ClockSkew = 5 * time.Minute }, - "public client": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodNone; c.ClientSecret = oidcclient.Secret{} }, - "post client": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodClientSecretPost }, - "form post": func(c *oidcclient.Config) { c.ResponseMode = oidcclient.ResponseModeFormPost }, - "single algorithm": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} }, - "insecure cookies query mode": func(c *oidcclient.Config) { c.InsecureCookies = true }, - "no explicit scopes": func(c *oidcclient.Config) { c.Scopes = nil }, - "duplicated scopes": func(c *oidcclient.Config) { c.Scopes = []string{"email", "openid", "email"} }, - "trusted audience": func(c *oidcclient.Config) { c.TrustedAudiences = []string{"https://api.example.com"} }, + "public client": func(c *oidcclient.Config) { + c.AuthMethod = oidcclient.AuthMethodNone + c.ClientSecret = oidcclient.Secret{} + }, + "post client": func(c *oidcclient.Config) { c.AuthMethod = oidcclient.AuthMethodClientSecretPost }, + "form post": func(c *oidcclient.Config) { c.ResponseMode = oidcclient.ResponseModeFormPost }, + "single algorithm": func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} }, + "insecure cookies on localhost": func(c *oidcclient.Config) { c.InsecureCookies, c.RedirectURL = true, testLocalRedirectURL }, + "insecure cookies on 127.0.0.1": func(c *oidcclient.Config) { + c.InsecureCookies, c.RedirectURL = true, "http://127.0.0.1:8080/cb" + }, + "insecure cookies on ::1": func(c *oidcclient.Config) { c.InsecureCookies, c.RedirectURL = true, "http://[::1]:8080/cb" }, + "no explicit scopes": func(c *oidcclient.Config) { c.Scopes = nil }, + "duplicated scopes": func(c *oidcclient.Config) { c.Scopes = []string{"email", "openid", "email"} }, + "trusted audience": func(c *oidcclient.Config) { c.TrustedAudiences = []string{"https://api.example.com"} }, "four transaction keys": func(c *oidcclient.Config) { c.TransactionKeys = secretKeys(testTransactionKey, testTransactionKey, testTransactionKey, testTransactionKey) }, diff --git a/oidc/client/fakeop_test.go b/oidc/client/fakeop_test.go index 5c6396c0..b02e36b9 100644 --- a/oidc/client/fakeop_test.go +++ b/oidc/client/fakeop_test.go @@ -58,8 +58,11 @@ const ( testConnection = "acme" testCallbackPath = "/oidc/acme/callback" testRedirectURL = "https://rp.example.com" + testCallbackPath - testSubject = "248289761001" - testAccessToken = "at-1" + // testLocalRedirectURL is a plain-http loopback redirect URI, the only + // kind Config.InsecureCookies accepts. + testLocalRedirectURL = "http://localhost" + testCallbackPath + testSubject = "248289761001" + testAccessToken = "at-1" ) // testClock is a settable clock shared by a fake OP and its clients. diff --git a/oidc/client/http.go b/oidc/client/http.go index 47d5ab55..fde9f9d3 100644 --- a/oidc/client/http.go +++ b/oidc/client/http.go @@ -187,7 +187,7 @@ func (c *Client) bindingCookie(name, value string, maxAge int) *http.Cookie { sameSite = http.SameSiteNoneMode } - return &http.Cookie{ //nolint:gosec // G124: Secure is false only with Config.InsecureCookies, the explicit local-development opt-in + return &http.Cookie{ //nolint:gosec // G124: Secure is false only with Config.InsecureCookies, which New admits for a loopback http RedirectURL only Name: name, Value: value, Path: "/", diff --git a/oidc/client/http_test.go b/oidc/client/http_test.go index b2e7039e..7df47402 100644 --- a/oidc/client/http_test.go +++ b/oidc/client/http_test.go @@ -113,7 +113,7 @@ func TestStartLoginCookie(t *testing.T) { t.Parallel() op := newFakeOP(t) - c := op.client(t, func(cfg *oidcclient.Config) { cfg.InsecureCookies = true }) + c := op.client(t, func(cfg *oidcclient.Config) { cfg.InsecureCookies, cfg.RedirectURL = true, testLocalRedirectURL }) rec := startLogin(t, c) ck := cookieNamed(t, rec, expectedCookieName("oidc-", loginState(t, rec))) @@ -193,7 +193,11 @@ func TestStartLoginLeavesApplicationCookies(t *testing.T) { t.Parallel() op := newFakeOP(t) - c := op.client(t, func(cfg *oidcclient.Config) { cfg.InsecureCookies = tc.insecure }) + c := op.client(t, func(cfg *oidcclient.Config) { + if tc.insecure { + cfg.InsecureCookies, cfg.RedirectURL = true, testLocalRedirectURL + } + }) var cookies, bindings []*http.Cookie @@ -466,6 +470,17 @@ func TestHandleCallbackRefuses(t *testing.T) { func newRP(t *testing.T, op *fakeOP, mode oidcclient.ResponseMode) (*httptest.Server, *http.Client) { t.Helper() + return serveRP(t, op, httptest.NewTLSServer, func(cfg *oidcclient.Config) { cfg.ResponseMode = mode }) +} + +// serveRP serves StartLogin and HandleCallback for a Client of op on the +// server serve starts, its RedirectURL on that server, modified by the +// mutators. +func serveRP(t *testing.T, op *fakeOP, serve func(http.Handler) *httptest.Server, + mutators ...func(*oidcclient.Config), +) (*httptest.Server, *http.Client) { + t.Helper() + var c *oidcclient.Client mux := http.NewServeMux() @@ -485,21 +500,20 @@ func newRP(t *testing.T, op *fakeOP, mode oidcclient.ResponseMode) (*httptest.Se _, _ = io.WriteString(w, "hello "+id.Subject) }) - rp := httptest.NewTLSServer(mux) + rp := serve(mux) t.Cleanup(rp.Close) - c = op.client(t, func(cfg *oidcclient.Config) { + c = op.client(t, append([]func(*oidcclient.Config){func(cfg *oidcclient.Config) { cfg.RedirectURL = rp.URL + testCallbackPath - cfg.ResponseMode = mode - }) + }}, mutators...)...) jar, err := cookiejar.New(nil) require.NoError(t, err) - browser := rp.Client() // trusts the httptest certificate shared by the RP and the OP + browser := *op.srv.Client() // a copy: trusts the httptest certificate shared by the RP and the OP browser.Jar = jar - return rp, browser + return rp, &browser } func TestEndToEndQuery(t *testing.T) { @@ -516,6 +530,25 @@ func TestEndToEndQuery(t *testing.T) { assert.Equal(t, "hello "+testSubject, string(body)) } +// TestEndToEndInsecureCookiesOnLoopback runs a login over plain http on the +// loopback interface — the local development setup InsecureCookies is for — +// through a browser's cookie jar, which keeps a cookie without Secure on +// http. +func TestEndToEndInsecureCookiesOnLoopback(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + rp, browser := serveRP(t, op, httptest.NewServer, func(cfg *oidcclient.Config) { cfg.InsecureCookies = true }) + require.True(t, strings.HasPrefix(rp.URL, "http://127.0.0.1:")) + + resp, err := browser.Get(rp.URL + "/login") + require.NoError(t, err) + + body := readAll(t, resp.Body) + assert.Equal(t, http.StatusOK, resp.StatusCode, string(body)) + assert.Equal(t, "hello "+testSubject, string(body)) +} + func TestEndToEndFormPost(t *testing.T) { t.Parallel() From 96798abc6248a8653be6187442a3a12ff9ec3adb Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 10:46:46 +0200 Subject: [PATCH 03/10] fix(oidc/client)!: expect RS256 ID Tokens unless Config.Algorithms is 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. --- CHANGELOG.md | 8 +++ LIMITATIONS.md | 5 ++ MIGRATION.md | 23 ++++++ docs/oidc-client.md | 2 +- docs/oidc-conformance.md | 2 +- docs/security-considerations.md | 4 +- .../interop/oidcstack/conformance/rp_test.go | 8 +++ internal/interop/oidcstack/flow_test.go | 8 ++- .../interop/oidcstack/substitution_test.go | 4 +- oidc/client/config.go | 23 ++++-- oidc/client/discovery.go | 3 +- oidc/client/discovery_test.go | 8 +++ oidc/client/doc.go | 7 +- oidc/client/errors.go | 6 +- oidc/client/idtoken_test.go | 71 ++++++++++++++----- 15 files changed, 147 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07ae8fa4..72f41a2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1039,6 +1039,14 @@ legacy packages (`authentication/`, `authorization/`, the in-tree unless `RedirectURL` is plain http, which is accepted on a loopback host only (breaking for a configuration combining it with an https redirect URI; see MIGRATION.md). +- `oidc/client` expects ID Tokens signed with RS256 unless + `Config.Algorithms` names another algorithm (breaking): an empty + `Algorithms` used to accept every asymmetric algorithm the provider + listed. OpenID Connect Core §3.1.3.7 step 7: the algorithm "SHOULD be the + default of RS256 or the algorithm sent by the Client in the + id_token_signed_response_alg parameter during Registration". A provider + signing with ES256, PS256, EdDSA… needs `Config.Algorithms` set to that + algorithm; the errors say so. - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 6b95c3b5..6f02c0f8 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -295,6 +295,11 @@ PKCE. Not supported, and refused where a provider could select them: run time. One exception: a public client (`none`) requires the provider to advertise S256 in `code_challenge_methods_supported`, which some providers omit although they support it. +- **ID Tokens are expected in RS256** — the default of OpenID Connect Core + §3.1.3.7 step 7: a provider signing with another algorithm fails every + login (`ErrIDTokenAlgorithm`, or `ErrInvalidMetadata` at `New` when it + does not list RS256) until `Config.Algorithms` names the algorithm + registered for the client. - **`acr_values` makes `acr` required** — when sent, the ID Token must carry one of the requested values; list every acceptable level. - **No OpenTelemetry spans** — `oidc/client` and `outbound` return errors diff --git a/MIGRATION.md b/MIGRATION.md index 13a5e7fd..34dbc327 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3081,3 +3081,26 @@ To migrate: `BASE_URL` is http). 2. Over https — including `https://localhost` — drop it: the `__Host-` cookie works on any https origin. + +### `oidc/client`: ID Tokens are expected in RS256 unless `Config.Algorithms` says otherwise + +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". An empty +`Config.Algorithms` used to accept every asymmetric algorithm the provider +listed; it now means RS256, which every provider supports (Discovery §3). + +| Provider signing ID Tokens with | `Config.Algorithms` | Before | After | +| --- | --- | --- | --- | +| RS256 | empty | logins succeed | unchanged | +| ES256, PS256, EdDSA, RS384… | empty | logins succeed | every login fails with `ErrIDTokenAlgorithm` (`New` fails with `ErrInvalidMetadata` when the provider does not list RS256) | +| any | the algorithm(s) listed | only those | unchanged | + +To migrate: + +1. Find the algorithm each provider signs your client's ID Tokens with — + the `id_token_signed_response_alg` of its registration, or the `alg` of + an ID Token it issued. +2. For any algorithm but RS256, set `Config.Algorithms` to it, for example + `[]oidcclient.Algorithm{oidcclient.ES256}`. List two only while the + provider moves from one to the other. diff --git a/docs/oidc-client.md b/docs/oidc-client.md index 15f8bf7a..89c1ef8e 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -62,7 +62,7 @@ Test names refer to `oidc/client` unless prefixed with `stack:` | Client authentication | Core §9, RFC 6749 §2.3.1 | `client_secret_basic` (form-urlencoded first), `client_secret_post`, `none`; `private_key_jwt`, `client_secret_jwt`, mTLS not supported | `TestTokenClientAuthentication`, stack: `TestCodeFlowAgainstOurProvider` | | Token response | RFC 6749 §5.1, Core §3.1.3.3 | Parsed strictly; error bodies of 400 and 401 answers become `TokenError`; redirects never followed | `TestTokenResponse`, `TestTokenRedirectNotFollowed`, stack: `TestTokenResponseSubstitution` | | ID Token validation | Core §3.1.3.7, RFC 8725 | Signature, `iss`, `sub`, `aud` (further audiences only when trusted), `azp`, `exp`, `iat`, `nbf`, `nonce`, `auth_time`, `acr`, `at_hash` when present, `typ` absent or `JWT`; before go-jose parses it, the JOSE header is strict JSON of at most 4 KiB without `jwk`, `x5c`, `jku` or `x5u` (Core §2) | `TestIDTokenKeyBearingHeaders`, `TestIDTokenHeaderStrict`, `TestCheckRawHeaderRefusesBeforeParsing`, `TestIDTokenIssuerAndSubject`, `TestIDTokenAudience`, `TestIDTokenTimes`, `TestIDTokenAccessTokenHash`, `TestIDTokenHeader`, stack: `TestResignedIDTokensAreRefused` | -| Signing algorithms | Core §15.1, RFC 8725 §3.1 | RS256/384/512, PS256/384/512, ES256/384/512, EdDSA (Ed25519); HMAC and `none` refused | `TestIDTokenAlgorithms`, stack: `TestCodeFlowSigningAlgorithms` | +| Signing algorithms | Core §15.1, §3.1.3.7 step 7, RFC 8725 §3.1 | RS256 by default; RS384/512, PS256/384/512, ES256/384/512, EdDSA (Ed25519) once `Config.Algorithms` names the client's registered algorithm; HMAC and `none` refused | `TestDefaultIDTokenAlgorithmIsRS256`, `TestIDTokenAlgorithms`, stack: `TestCodeFlowSigningAlgorithms` | | Token confusion | RFC 9068 §5, RFC 8725 §3.11 | An `at+jwt` access token is never accepted as an ID Token | `TestAccessTokenNotAcceptedAsIDToken`, stack: `TestIDTokenUsedAsAnotherToken` | | Encrypted ID Tokens | Core §10.2 | Not supported, refused | `TestIDTokenSerialization` | | UserInfo | Core §5.3, RFC 6750 §2.1 | Bearer token in the `Authorization` header, to the connection's endpoint only; `sub` must equal the ID Token's | `TestUserInfo`, `TestUserInfoRefusesForeignIdentity`, stack: `TestUserInfoSubstitution` | diff --git a/docs/oidc-conformance.md b/docs/oidc-conformance.md index a3c578e6..f6836554 100644 --- a/docs/oidc-conformance.md +++ b/docs/oidc-conformance.md @@ -62,7 +62,7 @@ names an `e2e/oidc` file, `suite:` a module of the OpenID Foundation suite. | R7 | Client authentication: basic (form-urlencoded), post, none (Core §9, RFC 6749 §2.3.1) | oauth2: `TestBasicDecodesFormURLEncodedCredentials` | `TestTokenClientAuthentication` | stack: `TestCodeFlowAgainstOurProvider`; interop: `TestInteropClientSecretPost`; e2e: `provider_errors`; suite: `oidcc-server-client-secret-post`, `oidcc-client-test-client-secret-basic` | | R8 | ID Token claims (Core §2, §3.1.3.6) | `TestIDTokenClaims`, `TestIDTokenCarriesTheAuthenticationEvent`, `TestProtocolClaimsCannotBeAliasedByCaseVariants` | `TestIDTokenIssuerAndSubject`, `TestIDTokenAudience`, `TestIDTokenTimes`, `TestIDTokenAccessTokenHash` | stack: `TestCodeFlowAgainstOurProvider`; interop: `TestInteropCaseVariantClaimsNeverReachTheRP`; suite: `oidcc-client-test-missing-sub`, `-missing-iat`, `-invalid-aud`, `-invalid-iss` | | R9 | ID Token validation (Core §3.1.3.7, RFC 8725 §3.1, §3.11) | — | `TestIDTokenAlgorithms`, `TestIDTokenHeader`, `TestIDTokenKeySelection`, `TestIDTokenSignature`, `TestIDTokenClaimsJSON` | stack: `TestResignedIDTokensAreRefused`, `TestUnknownKeysTriggerABoundedRefresh`; suite: `oidcc-client-test-invalid-sig-rs256`, `-invalid-sig-hs256`, `-invalid-sig-es256`, `-kid-absent-*` | -| R10 | Signing algorithms, RS256 mandatory, never `none` or HMAC (Core §15.1, §16.14) | `TestIDTokenSigningAlgorithms`, `TestKeyRingValidate`, `TestClientWithoutAnAlgorithmGetsRS256` | `TestIDTokenAlgorithms` | stack: `TestCodeFlowSigningAlgorithms`; interop: `TestInteropSigningAlgorithms`; suite: `oidcc-client-test-idtoken-sig-rs256`, `-idtoken-sig-none` | +| R10 | Signing algorithms, RS256 mandatory, never `none` or HMAC (Core §15.1, §16.14) | `TestIDTokenSigningAlgorithms`, `TestKeyRingValidate`, `TestClientWithoutAnAlgorithmGetsRS256` | `TestIDTokenAlgorithms`, `TestDefaultIDTokenAlgorithmIsRS256` (RS256 unless `Config.Algorithms` names the registered algorithm, Core §3.1.3.7 step 7) | stack: `TestCodeFlowSigningAlgorithms`; interop: `TestInteropSigningAlgorithms`; suite: `oidcc-client-test-idtoken-sig-rs256`, `-idtoken-sig-none` | | R11 | `prompt` (Core §3.1.2.1, §3.1.2.6) | `TestPromptParsing`, `TestPromptNoneIsSilent`, `TestPromptNoneRefusals`, `TestRequestCheck` | `TestBeginRefusesInvalidOptions`, `TestCompleteAuthorizationError` | stack: `TestPromptNoneThroughTheClient`, `TestRefusalsRevealNoSession`; interop: `TestInteropPromptNone`; e2e: `provider_prompt`; suite: `oidcc-prompt-login`, `oidcc-prompt-none-*` | | R12 | `max_age` and `auth_time` (Core §3.1.2.1) | `TestMaxAgeParsing`, `TestFreshnessIsEnforcedByTheProvider` | `TestIDTokenAuthTime` | stack: `TestReauthenticationThroughTheClient`; interop: `TestInteropReauthentication`; e2e: `provider_prompt`; suite: `oidcc-max-age-1`, `oidcc-max-age-10000` | | R13 | `id_token_hint`, `login_hint`, locales, `display`, `acr_values` (Core §3.1.2.1) | `TestIDTokenHint`, `TestIDTokenHintDesignatesTheEndUser`, `TestPassthroughParameters`, `TestVoluntaryACRIsNotEnforced` | `TestBeginHints`, `TestIDTokenACR` | stack: `TestHintsReachTheProvider`, `TestACRValuesThroughTheClient`; interop: `TestInteropIDTokenHint`; rp: `TestZitadelACRValues`; suite: `oidcc-id-token-hint`, `oidcc-login-hint`, `oidcc-ui-locales`, `oidcc-claims-locales`, `oidcc-display-*` | diff --git a/docs/security-considerations.md b/docs/security-considerations.md index 8af21b97..c1b1648b 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1424,7 +1424,9 @@ are attacker-influenced. header the proxy forwards: preserve it, or give `Complete` the public URL. - **ID Token** — validated per OpenID Connect Core §3.1.3.7 before anything - is returned: asymmetric algorithms only (HMAC and `none` refused), `typ` + is returned: RS256 — or the algorithm registered for the client, named in + `Config.Algorithms` (OpenID Connect Core §3.1.3.7 step 7) — asymmetric + algorithms only (HMAC and `none` refused), `typ` absent or `JWT` (an RFC 9068 `at+jwt` access token is refused), keys from the configured issuer's `jwks_uri` only (`jku`, `x5u`, `jwk` and `x5c` are refused, see below) through a bounded cache that refetches once, rate diff --git a/internal/interop/oidcstack/conformance/rp_test.go b/internal/interop/oidcstack/conformance/rp_test.go index fc669404..65d04b8b 100644 --- a/internal/interop/oidcstack/conformance/rp_test.go +++ b/internal/interop/oidcstack/conformance/rp_test.go @@ -282,6 +282,14 @@ func loginWith(t *testing.T, plan rpPlan, module, issuer, secret string) (string cfg.ClientSecret = oidcclient.NewSecret(secret) } + // The ID Token algorithm of the static registration + // (id_token_signed_response_alg): RS256, the default, unless the module + // signs with another one — the client then verifies it (OpenID Connect + // Core §3.1.3.7 step 7) instead of refusing the algorithm. + if strings.HasSuffix(module, "-es256") { + cfg.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} + } + client, err := oidcclient.New(ctx, cfg) if err != nil { return outcomeOf(err), err.Error() diff --git a/internal/interop/oidcstack/flow_test.go b/internal/interop/oidcstack/flow_test.go index 19deaa19..0e46f136 100644 --- a/internal/interop/oidcstack/flow_test.go +++ b/internal/interop/oidcstack/flow_test.go @@ -83,8 +83,9 @@ func TestCodeFlowAgainstOurProvider(t *testing.T) { // TestCodeFlowSigningAlgorithms checks every ID Token algorithm a client // may register (Registration §2 id_token_signed_response_alg): our provider -// signs with the issuer's key of that algorithm, our client verifies the -// signature and the at_hash computed with the algorithm's hash (Core +// signs with the issuer's key of that algorithm, our client — configured +// with that algorithm (Config.Algorithms, Core §3.1.3.7 step 7) — verifies +// the signature and the at_hash computed with the algorithm's hash (Core // §3.1.3.6). func TestCodeFlowSigningAlgorithms(t *testing.T) { t.Parallel() @@ -99,7 +100,8 @@ func TestCodeFlowSigningAlgorithms(t *testing.T) { t.Parallel() name := strings.ToLower(string(alg)) - c := o.rp(t, conn(name, o.base, "rp-"+name, secret, oidcclient.AuthMethodClientSecretBasic)) + c := o.rp(t, conn(name, o.base, "rp-"+name, secret, oidcclient.AuthMethodClientSecretBasic), + func(cfg *oidcclient.Config) { cfg.Algorithms = []oidcclient.Algorithm{oidcclient.Algorithm(alg)} }) id, err := o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) diff --git a/internal/interop/oidcstack/substitution_test.go b/internal/interop/oidcstack/substitution_test.go index 5a02b70d..6e03c5ce 100644 --- a/internal/interop/oidcstack/substitution_test.go +++ b/internal/interop/oidcstack/substitution_test.go @@ -112,7 +112,9 @@ func TestResignedIDTokensAreRefused(t *testing.T) { }, oidcclient.ErrIDTokenType, true, nil}, {"PS256 with the RS256 key (one algorithm per key)", oidcclient.AuthOptions{}, func(t *testing.T, _ *op, tok string) string { return resign(t, tok, "PS256", rootKey(), nil, nil) - }, oidcclient.ErrUnknownSigningKey, true, nil}, + }, oidcclient.ErrUnknownSigningKey, true, func(cfg *oidcclient.Config) { + cfg.Algorithms = []oidcclient.Algorithm{oidcclient.RS256, oidcclient.PS256} // PS256 accepted, never with the RS256 key + }}, {"kid of another issuer's key", oidcclient.AuthOptions{}, func(t *testing.T, _ *op, tok string) string { return resign(t, tok, "RS256", rsaKeys()[6], func(h map[string]any) { h["kid"] = "tenant-a" }, nil) }, oidcclient.ErrUnknownSigningKey, true, nil}, diff --git a/oidc/client/config.go b/oidc/client/config.go index ae1c0ac7..c7a055e9 100644 --- a/oidc/client/config.go +++ b/oidc/client/config.go @@ -37,9 +37,14 @@ const ( EdDSA Algorithm = "EdDSA" ) -// supportedAlgorithms is the default Config.Algorithms. +// supportedAlgorithms are the algorithms Config.Algorithms may list. var supportedAlgorithms = []Algorithm{RS256, RS384, RS512, PS256, PS384, PS512, ES256, ES384, ES512, EdDSA} +// defaultAlgorithms is the default Config.Algorithms: RS256, the algorithm +// of a client that registered none (OpenID Connect Core §3.1.3.7 step 7), +// which every provider supports (Discovery §3). +var defaultAlgorithms = []Algorithm{RS256} + // AuthMethod is a token endpoint client authentication method (OpenID // Connect Core §9). type AuthMethod string @@ -159,9 +164,17 @@ type Config struct { // MaxTransactionTTL. TransactionTTL time.Duration - // Algorithms restricts the ID Token signature algorithms; empty means - // every asymmetric algorithm. The effective set is its intersection - // with the provider's id_token_signing_alg_values_supported. + // Algorithms are the ID Token signature algorithms accepted: the + // id_token_signed_response_alg registered for the client — RS256 when + // empty. 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". A + // provider signing its ID Tokens with another algorithm (ES256, PS256, + // EdDSA…) needs it listed here, else its logins fail with + // ErrIDTokenAlgorithm; list several only while the provider moves from + // one to another. The effective set is the intersection with the + // provider's id_token_signing_alg_values_supported: New fails with + // ErrInvalidMetadata when it is empty. // // One algorithm per key (RFC 8725 §3.1): an RSA JWK without "alg" is // used for RS256 only — or for the single RSA algorithm of the @@ -478,7 +491,7 @@ func isScopeToken(s string) bool { func normalizeAlgorithms(in []Algorithm) ([]Algorithm, error) { if len(in) == 0 { - return slices.Clone(supportedAlgorithms), nil + return slices.Clone(defaultAlgorithms), nil } out := make([]Algorithm, 0, len(in)) diff --git a/oidc/client/discovery.go b/oidc/client/discovery.go index 65ae42ba..5684dfb9 100644 --- a/oidc/client/discovery.go +++ b/oidc/client/discovery.go @@ -266,7 +266,8 @@ func (md *providerMetadata) negotiate(cfg Config) ([]Algorithm, error) { } if len(algs) == 0 { - return nil, fmt.Errorf("%w: no ID Token signature algorithm in common", ErrInvalidMetadata) + return nil, fmt.Errorf("%w: no ID Token signature algorithm in common with Config.Algorithms "+ + "(RS256 by default: set it to the algorithm registered for the client)", ErrInvalidMetadata) } return algs, nil diff --git a/oidc/client/discovery_test.go b/oidc/client/discovery_test.go index bcf53d90..55c87e7d 100644 --- a/oidc/client/discovery_test.go +++ b/oidc/client/discovery_test.go @@ -360,6 +360,14 @@ func TestCapabilities(t *testing.T) { metadata: func(m map[string]any) { m["id_token_signing_alg_values_supported"] = []string{"RS256"} }, config: func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} }, }, + "ES256 only, RS256 by default": { + metadata: func(m map[string]any) { m["id_token_signing_alg_values_supported"] = []string{"ES256"} }, + }, + "ES256 only, ES256 configured": { + metadata: func(m map[string]any) { m["id_token_signing_alg_values_supported"] = []string{"ES256"} }, + config: func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} }, + ok: true, + }, "iss parameter flag wrong type": {metadata: func(m map[string]any) { m["authorization_response_iss_parameter_supported"] = "true" }}, "response modes wrong type": {metadata: func(m map[string]any) { m["response_modes_supported"] = "query" }}, "pkce methods wrong type": {metadata: func(m map[string]any) { m["code_challenge_methods_supported"] = []int{1} }}, diff --git a/oidc/client/doc.go b/oidc/client/doc.go index 0a051b62..15338f5c 100644 --- a/oidc/client/doc.go +++ b/oidc/client/doc.go @@ -27,9 +27,10 @@ // Supported: response modes query and form_post; client authentication // client_secret_basic (identifier and secret form-urlencoded first, RFC 6749 // §2.3.1), client_secret_post and none (public clients, which require the -// provider to advertise S256); ID Tokens signed with RS256/384/512, -// PS256/384/512, ES256/384/512 or EdDSA (Ed25519); RFC 9207 authorization -// response issuer identification; JWK Set rollover. +// provider to advertise S256); ID Tokens signed with RS256 — the default +// (OpenID Connect Core §3.1.3.7 step 7) — or, once Config.Algorithms names +// it, RS384/512, PS256/384/512, ES256/384/512 or EdDSA (Ed25519); RFC 9207 +// authorization response issuer identification; JWK Set rollover. // // Not supported, and refused: implicit and hybrid flows, the fragment // response mode, HMAC-signed (HS256/384/512) or unsigned ID Tokens, diff --git a/oidc/client/errors.go b/oidc/client/errors.go index 526bb5d9..79fea6e8 100644 --- a/oidc/client/errors.go +++ b/oidc/client/errors.go @@ -97,8 +97,10 @@ var ( // x5u member (refused before the token is parsed). ErrIDTokenMalformed = idTokenError("malformed") // ErrIDTokenAlgorithm reports a signature algorithm outside the - // effective allow-list ("none" and HMAC algorithms are never allowed). - ErrIDTokenAlgorithm = idTokenError("algorithm not allowed") + // effective allow-list ("none" and HMAC algorithms are never allowed): + // Config.Algorithms, RS256 by default — a provider signing with another + // algorithm needs it set to the algorithm registered for the client. + ErrIDTokenAlgorithm = idTokenError("algorithm not allowed (Config.Algorithms)") // ErrIDTokenType reports a JOSE header naming another kind of token // (typ, e.g. an RFC 9068 at+jwt access token), a nested JWT (cty) or an // extension (crit, b64). diff --git a/oidc/client/idtoken_test.go b/oidc/client/idtoken_test.go index 72863ca6..cd0e3c9e 100644 --- a/oidc/client/idtoken_test.go +++ b/oidc/client/idtoken_test.go @@ -111,6 +111,21 @@ func baseClaims(op *fakeOP, g *grant) string { func itoa(n int64) string { return strconv.FormatInt(n, 10) } +// onlyAlgorithm configures alg as the client's registered ID Token +// algorithm (Config.Algorithms). +func onlyAlgorithm(alg jose.SignatureAlgorithm) func(*oidcclient.Config) { + return func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.Algorithm(alg)} } +} + +// everyAlgorithm configures every supported algorithm, for the tests of +// key selection across key types. +func everyAlgorithm(c *oidcclient.Config) { + c.Algorithms = []oidcclient.Algorithm{ + oidcclient.RS256, oidcclient.RS384, oidcclient.RS512, oidcclient.PS256, oidcclient.PS384, oidcclient.PS512, + oidcclient.ES256, oidcclient.ES384, oidcclient.ES512, oidcclient.EdDSA, + } +} + func signWith(key signingKey) func(op *fakeOP) { return func(op *fakeOP) { op.signer = key @@ -118,20 +133,41 @@ func signWith(key signingKey) func(op *fakeOP) { } } +// TestDefaultIDTokenAlgorithmIsRS256 checks the default of +// Config.Algorithms: RS256 only (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"), even from a provider listing every algorithm and +// publishing the keys; another algorithm verifies once configured. +func TestDefaultIDTokenAlgorithmIsRS256(t *testing.T) { + t.Parallel() + + es256 := func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.ES256} } + + runIDCases(t, []idCase{ + {name: "RS256 by default", setup: signWith(keyRSA(jose.RS256, "k"))}, + {name: "ES256 refused by default", setup: signWith(keyEC(jose.ES256, "k")), err: oidcclient.ErrIDTokenAlgorithm}, + {name: "PS256 refused by default", setup: signWith(keyRSA(jose.PS256, "k")), err: oidcclient.ErrIDTokenAlgorithm}, + {name: "EdDSA refused by default", setup: signWith(keyEd("k")), err: oidcclient.ErrIDTokenAlgorithm}, + {name: "ES256 once configured", setup: signWith(keyEC(jose.ES256, "k")), config: es256}, + {name: "RS256 refused once ES256 is configured", setup: signWith(keyRSA(jose.RS256, "k")), config: es256, err: oidcclient.ErrIDTokenAlgorithm}, + }) +} + func TestIDTokenAlgorithms(t *testing.T) { t.Parallel() runIDCases(t, []idCase{ {name: "RS256", setup: signWith(keyRSA(jose.RS256, "k"))}, - {name: "RS384", setup: signWith(keyRSA(jose.RS384, "k"))}, - {name: "RS512", setup: signWith(keyRSA(jose.RS512, "k"))}, - {name: "PS256", setup: signWith(keyRSA(jose.PS256, "k"))}, - {name: "PS384", setup: signWith(keyRSA(jose.PS384, "k"))}, - {name: "PS512", setup: signWith(keyRSA(jose.PS512, "k"))}, - {name: "ES256", setup: signWith(keyEC(jose.ES256, "k"))}, - {name: "ES384", setup: signWith(keyEC(jose.ES384, "k"))}, - {name: "ES512", setup: signWith(keyEC(jose.ES512, "k"))}, - {name: "EdDSA", setup: signWith(keyEd("k"))}, + {name: "RS384", setup: signWith(keyRSA(jose.RS384, "k")), config: onlyAlgorithm(jose.RS384)}, + {name: "RS512", setup: signWith(keyRSA(jose.RS512, "k")), config: onlyAlgorithm(jose.RS512)}, + {name: "PS256", setup: signWith(keyRSA(jose.PS256, "k")), config: onlyAlgorithm(jose.PS256)}, + {name: "PS384", setup: signWith(keyRSA(jose.PS384, "k")), config: onlyAlgorithm(jose.PS384)}, + {name: "PS512", setup: signWith(keyRSA(jose.PS512, "k")), config: onlyAlgorithm(jose.PS512)}, + {name: "ES256", setup: signWith(keyEC(jose.ES256, "k")), config: onlyAlgorithm(jose.ES256)}, + {name: "ES384", setup: signWith(keyEC(jose.ES384, "k")), config: onlyAlgorithm(jose.ES384)}, + {name: "ES512", setup: signWith(keyEC(jose.ES512, "k")), config: onlyAlgorithm(jose.ES512)}, + {name: "EdDSA", setup: signWith(keyEd("k")), config: onlyAlgorithm(jose.EdDSA)}, { name: "alg none", err: oidcclient.ErrIDTokenAlgorithm, setup: func(op *fakeOP) { @@ -162,7 +198,8 @@ func TestIDTokenAlgorithms(t *testing.T) { }, { name: "algorithm not advertised", err: oidcclient.ErrIDTokenAlgorithm, - setup: func(op *fakeOP) { op.metadata["id_token_signing_alg_values_supported"] = []string{"ES256"} }, + setup: func(op *fakeOP) { op.metadata["id_token_signing_alg_values_supported"] = []string{"ES256"} }, + config: func(c *oidcclient.Config) { c.Algorithms = []oidcclient.Algorithm{oidcclient.RS256, oidcclient.ES256} }, }, { name: "algorithm not configured", err: oidcclient.ErrIDTokenAlgorithm, @@ -311,7 +348,7 @@ func TestIDTokenKeySelection(t *testing.T) { {name: "JWK alg differs from header alg", err: oidcclient.ErrUnknownSigningKey, setup: func(op *fakeOP) { op.jwks = []jose.JSONWebKey{keyRSA(jose.RS384, "rsa-1").jwk()} }}, - {name: "JWK declaring an alg its key cannot verify", err: oidcclient.ErrUnknownSigningKey, setup: func(op *fakeOP) { + {name: "JWK declaring an alg its key cannot verify", err: oidcclient.ErrUnknownSigningKey, config: everyAlgorithm, setup: func(op *fakeOP) { op.signer = keyEC(jose.ES256, "rsa-1") k := keyRSA(jose.RS256, "rsa-1").jwk() k.Algorithm = string(jose.ES256) @@ -320,11 +357,11 @@ func TestIDTokenKeySelection(t *testing.T) { {name: "key type does not fit the alg", err: oidcclient.ErrUnknownSigningKey, setup: func(op *fakeOP) { op.jwks = []jose.JSONWebKey{keyEC(jose.ES256, "rsa-1").bareJWK()} }}, - {name: "curve does not fit the alg", err: oidcclient.ErrUnknownSigningKey, setup: func(op *fakeOP) { + {name: "curve does not fit the alg", err: oidcclient.ErrUnknownSigningKey, config: everyAlgorithm, setup: func(op *fakeOP) { op.signer = keyEC(jose.ES384, "ec") op.jwks = []jose.JSONWebKey{keyEC(jose.ES256, "ec").bareJWK()} }}, - {name: "alg-less RSA key is RS256 only", err: oidcclient.ErrUnknownSigningKey, setup: func(op *fakeOP) { + {name: "alg-less RSA key is RS256 only", err: oidcclient.ErrUnknownSigningKey, config: everyAlgorithm, setup: func(op *fakeOP) { op.signer = keyRSA(jose.PS256, "rsa-1") op.jwks = []jose.JSONWebKey{op.signer.bareJWK()} }}, @@ -354,7 +391,7 @@ func TestIDTokenKeySelection(t *testing.T) { op.jwks = []jose.JSONWebKey{op.signer.bareJWK()} }, }, - {name: "alg-less EC key bound by its curve", setup: func(op *fakeOP) { + {name: "alg-less EC key bound by its curve", config: everyAlgorithm, setup: func(op *fakeOP) { op.signer = keyEC(jose.ES384, "ec") op.jwks = []jose.JSONWebKey{op.signer.bareJWK()} }}, @@ -685,9 +722,9 @@ func TestIDTokenAccessTokenHash(t *testing.T) { op.claimsHook = func(_ *grant, claims map[string]any) { claims["at_hash"] = leftHalfHash(jose.RS256, testAccessToken) } op.accessToken = "attacker-access-token" }}, - {name: "ES384 uses SHA-384", setup: signWith(keyEC(jose.ES384, "k"))}, - {name: "EdDSA uses SHA-512", setup: signWith(keyEd("k"))}, - {name: "EdDSA with a SHA-256 at_hash", err: oidcclient.ErrAccessTokenHash, setup: func(op *fakeOP) { + {name: "ES384 uses SHA-384", setup: signWith(keyEC(jose.ES384, "k")), config: onlyAlgorithm(jose.ES384)}, + {name: "EdDSA uses SHA-512", setup: signWith(keyEd("k")), config: onlyAlgorithm(jose.EdDSA)}, + {name: "EdDSA with a SHA-256 at_hash", err: oidcclient.ErrAccessTokenHash, config: onlyAlgorithm(jose.EdDSA), setup: func(op *fakeOP) { signWith(keyEd("k"))(op) op.claimsHook = func(_ *grant, claims map[string]any) { claims["at_hash"] = leftHalfHash(jose.RS256, testAccessToken) } }}, From ccdb5c3e3036cad2b032d4f6fb52499f89abf29f Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 10:51:09 +0200 Subject: [PATCH 04/10] feat(oidc/client)!: enforce the RFC 9207 iss policy, add RequireIssParameter 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. --- CHANGELOG.md | 15 ++++ LIMITATIONS.md | 8 ++ MIGRATION.md | 34 ++++++++ docs/oidc-client.md | 30 ++++--- docs/security-considerations.md | 13 ++- internal/interop/oidcstack/attacker_test.go | 12 +++ internal/interop/oidcstack/mixup_test.go | 74 ++++++++++++++--- oidc/client/callback.go | 37 +++++---- oidc/client/callback_test.go | 88 +++++++++++++++++---- oidc/client/config.go | 24 ++++++ oidc/client/doc.go | 7 +- oidc/client/errors.go | 6 +- 12 files changed, 288 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72f41a2c..40168474 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -559,6 +559,15 @@ legacy packages (`authentication/`, `authorization/`, the in-tree receives the value of every security-pipeline panic the interceptors recover (the value may hold a credential: it is the application's to log or report); a panic of the handler is ignored. + the error classes sees the new `too_many_attempts` class. +- **`oidcclient.Config.RequireIssParameter`**: declares that the provider + sends the RFC 9207 `iss` parameter in every authorization response + (RFC 9207 §2.4, "according to the client's configuration"): a response + without it is refused (`ErrIssuerParameter`). Set on every connection a + tenant configures, it closes the mix-up that distinct redirect URIs + leave open (RFC 9700 §4.4.2.2) for applications working only with + providers that send `iss`; it also admits a provider that sends `iss` + without advertising it. - `oauth2.ServerConfig.AllowInsecureLoopbackIssuer` (development only: an http issuer on localhost or a loopback IP address), the @@ -1047,6 +1056,12 @@ legacy packages (`authentication/`, `authorization/`, the in-tree id_token_signed_response_alg parameter during Registration". A provider signing with ES256, PS256, EdDSA… needs `Config.Algorithms` set to that algorithm; the errors say so. +- `oidc/client` discards an authorization response carrying `iss` from a + provider that does not advertise + `authorization_response_iss_parameter_supported` (`ErrIssuerParameter`, + RFC 9207 §2.4: "Clients SHOULD discard…"); a matching `iss` used to be + accepted. Breaking for a provider that sends `iss` without advertising + it: set `Config.RequireIssParameter` (see MIGRATION.md). - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 6f02c0f8..a1162165 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -295,6 +295,14 @@ PKCE. Not supported, and refused where a provider could select them: run time. One exception: a public client (`none`) requires the provider to advertise S256 in `code_challenge_methods_supported`, which some providers omit although they support it. +- **RFC 9207 `iss` policy** — a response carrying `iss` from a provider + that does not advertise `authorization_response_iss_parameter_supported` + is discarded (`ErrIssuerParameter`, RFC 9207 §2.4) unless + `Config.RequireIssParameter` declares that the provider sends it. Without + `RequireIssParameter` on every connection, distinct redirect URIs remain + the only mix-up defense against a provider that sends no `iss`, with the + gap RFC 9700 §4.4.2.2 names (docs/oidc-client.md, "Connections and + redirect URIs"). - **ID Tokens are expected in RS256** — the default of OpenID Connect Core §3.1.3.7 step 7: a provider signing with another algorithm fails every login (`ErrIDTokenAlgorithm`, or `ErrInvalidMetadata` at `New` when it diff --git a/MIGRATION.md b/MIGRATION.md index 34dbc327..ab48dad4 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3104,3 +3104,37 @@ To migrate: 2. For any algorithm but RS256, set `Config.Algorithms` to it, for example `[]oidcclient.Algorithm{oidcclient.ES256}`. List two only while the provider moves from one to the other. + +### `oidc/client`: the RFC 9207 `iss` policy and `Config.RequireIssParameter` + +RFC 9207 §2.4: "Clients SHOULD discard authorization responses with the iss +parameter from authorization servers that do not indicate their support for +the parameter", and they "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 had no configuration +saying that a provider sends `iss`. + +| Authorization response | Provider advertises RFC 9207 | `RequireIssParameter` | Before | After | +| --- | --- | --- | --- | --- | +| `iss` = issuer | yes | — | accepted | unchanged | +| `iss` = issuer | no | false (default) | accepted | `ErrIssuerParameter` | +| `iss` = issuer | no | true | — | accepted | +| no `iss` | yes | — | `ErrIssuerParameter` | unchanged | +| no `iss` | no | false (default) | accepted | unchanged | +| no `iss` | no | true | — | `ErrIssuerParameter` | +| `iss` ≠ issuer | any | any | `ErrIssuerParameter` | unchanged | + +To migrate: + +1. Nothing for a provider that advertises RFC 9207, or that sends no `iss`. +2. Symptom: every login of a connection fails with `ErrIssuerParameter` + and the text "iss from a provider that does not advertise + authorization_response_iss_parameter_supported": the provider sends + `iss` without advertising it. Set `RequireIssParameter: true` on that + connection. +3. An application whose tenants configure providers sets + `RequireIssParameter: true` on every tenant connection — its own + setting, never one the tenant chooses — to close the mix-up residual of + RFC 9700 §4.4.2.2; it then works only with providers that send `iss`. diff --git a/docs/oidc-client.md b/docs/oidc-client.md index 89c1ef8e..43a4c571 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -51,8 +51,8 @@ Test names refer to `oidc/client` unless prefixed with `stack:` | No parameter twice | RFC 6749 §3.1 | The authorization endpoint's query is kept; `ExtraParams` may not repeat a parameter the client sends, one of that query or each other — names compared case-insensitively, `.` read as `_` (servers such as ASP.NET and PHP read them so); `New` refuses an endpoint query that does | `TestBeginSendsEveryParameterOnce`, `TestBeginRefusesOtherSpellingsOfReservedParams`, `TestBeginRefusesParamsOfTheEndpointQuery`, `TestEndpointURLs` | | `acr_values` | Core §3.1.2.1, §2 | Sent as given; the ID Token's `acr` must then be one of them (a provider ignoring them fails the login) | `TestIDTokenACR`, stack: `TestACRValuesThroughTheClient`, rp: `TestZitadelACRValues` | | Authorization errors | RFC 6749 §4.1.2.1, Core §3.1.2.6 | `AuthorizationError` with the provider's code; nothing redeemed | `TestCompleteAuthorizationError`, stack: `TestPromptNoneThroughTheClient`, `TestConsentDeniedThroughTheClient` | -| RFC 9207 `iss` | RFC 9207 | Compared when present; required when the provider advertises it | `TestCompleteIssParameter`, stack: `TestMixUpBetweenIssuersOfOneProvider` | -| Mix-up defense | RFC 9700 §4.4 | The response must arrive on the connection's own redirect URI, for a transaction of that connection, issuer, client, redirect URI and response mode | `TestCompleteMixUp`, `TestCompleteTransactionMismatch`, stack: `TestMixUpWithAnAttackerProvider`, `TestMixUpResidualWithoutRFC9207` | +| RFC 9207 `iss` | RFC 9207 §2.4 | Required and compared when the provider advertises it or `Config.RequireIssParameter` is set; from another provider a response carrying `iss` is discarded | `TestCompleteIssParameter`, stack: `TestMixUpBetweenIssuersOfOneProvider` | +| Mix-up defense | RFC 9700 §4.4 | The response must arrive on the connection's own redirect URI, for a transaction of that connection, issuer, client, redirect URI and response mode; `RequireIssParameter` closes the §4.4.2.2 residual | `TestCompleteMixUp`, `TestCompleteTransactionMismatch`, stack: `TestMixUpWithAnAttackerProvider`, `TestMixUpResidualWithoutRFC9207`, `TestRequireIssParameterOnTheReceivingConnectionClosesTheResidual` | | Implicit and hybrid flows, PAR, request objects, `claims` parameter | Core §3.2, §3.3, §6, §5.5; RFC 9126 | Not supported | — | ### Tokens and claims @@ -138,15 +138,23 @@ connection's — this is the mix-up defense (RFC 9700 §4.4.2): `Complete` refuses a response received on another URL, or carrying a transaction of another connection, issuer or client (`ErrTransactionMismatch`). Never route callbacks by the state value. A provider advertising RFC 9207 is -additionally checked on `iss` (`ErrIssuerParameter`). One gap remains, the -one RFC 9700 §4.4.2.2 names: when the honest provider does not send `iss` -and an attacker registered, at that provider, a client whose redirect URI is -the attacker connection's callback, nothing tells the responses apart — -`stack: TestMixUpResidualWithoutRFC9207` shows the honest code reaching the -attacker's token endpoint. Prefer providers that support RFC 9207, never -register a connection's redirect URI at another provider, and on a -multi-tenant provider use the tenant's own issuer (one registration valid -at several issuers defeats the distinct redirect URIs). +additionally checked on `iss` (`ErrIssuerParameter`); a response carrying +`iss` from a provider that does not advertise it is discarded (RFC 9207 +§2.4), unless `Config.RequireIssParameter` declares that the provider sends +it. Distinct redirect URIs leave the gap RFC 9700 §4.4.2.2 names: when the +honest provider does not send `iss` and an attacker registered, at that +provider, a client whose redirect URI is the attacker connection's +callback, the response lands on the attacker connection and nothing tells +it apart — `stack: TestMixUpResidualWithoutRFC9207` shows the honest code +reaching the attacker's token endpoint. `Config.RequireIssParameter` closes +it on the connection that receives the response +(`stack: TestRequireIssParameterOnTheReceivingConnectionClosesTheResidual`): +an application whose tenants configure providers sets it on every tenant +connection — its own setting, never the tenant's — and then works only with +providers that send `iss`. Otherwise, never register a connection's +redirect URI at another provider, and on a multi-tenant provider use the +tenant's own issuer (one registration valid at several issuers defeats the +distinct redirect URIs). ### Callback and session diff --git a/docs/security-considerations.md b/docs/security-considerations.md index c1b1648b..07e9538f 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1415,12 +1415,17 @@ are attacker-influenced. port, path or query parameter): `Complete` refuses a response received on another URL than the connection's redirect URI, and a transaction of another connection, issuer, client, redirect URI or response mode. A - provider advertising RFC 9207 must also send the `iss` parameter, - compared with its issuer. Never route callbacks by the state value. + provider advertising RFC 9207 — or declared to send it with + `Config.RequireIssParameter` — must also send the `iss` parameter, + compared with its issuer; a response carrying `iss` from another provider + is discarded (RFC 9207 §2.4). Never route callbacks by the state value. Distinct redirect URIs leave the gap RFC 9700 §4.4.2.2 names — an attacker registering, at the honest provider, a client with the redirect - URI of its own connection; RFC 9207 closes it, so prefer providers that - support it. Behind a reverse proxy, `HandleCallback` compares the Host + URI of its own connection, so that the honest response lands on the + attacker's connection. `RequireIssParameter` closes it on the connection + that receives the response: set it on every connection a tenant + configures (the application's setting, never the tenant's), and work only + with providers that send `iss`. Behind a reverse proxy, `HandleCallback` compares the Host header the proxy forwards: preserve it, or give `Complete` the public URL. - **ID Token** — validated per OpenID Connect Core §3.1.3.7 before anything diff --git a/internal/interop/oidcstack/attacker_test.go b/internal/interop/oidcstack/attacker_test.go index 132cf3ed..ad428684 100644 --- a/internal/interop/oidcstack/attacker_test.go +++ b/internal/interop/oidcstack/attacker_test.go @@ -29,6 +29,14 @@ type attackerAS struct { func newAttackerAS(t *testing.T, divert func(q url.Values) string) *attackerAS { t.Helper() + return newAttackerASAdvertising(t, false, divert) +} + +// newAttackerASAdvertising is newAttackerAS whose metadata advertises RFC +// 9207 (authorization_response_iss_parameter_supported) when iss is true. +func newAttackerASAdvertising(t *testing.T, iss bool, divert func(q url.Values) string) *attackerAS { + t.Helper() + a := &attackerAS{divert: divert} mux := http.NewServeMux() @@ -41,6 +49,10 @@ func newAttackerAS(t *testing.T, divert func(q url.Values) string) *attackerAS { "code_challenge_methods_supported": []string{"S256"}, } + if iss { + doc["authorization_response_iss_parameter_supported"] = true + } + w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(doc) }) diff --git a/internal/interop/oidcstack/mixup_test.go b/internal/interop/oidcstack/mixup_test.go index f73c8752..a7ab79ab 100644 --- a/internal/interop/oidcstack/mixup_test.go +++ b/internal/interop/oidcstack/mixup_test.go @@ -141,20 +141,24 @@ func (o *op) enduserRequests() int { // connection — ErrTransactionMismatch; // - (c) A registered its own client at H with its connection's callback // (§4.4.2.2's residual risk): the response lands on A's callback with -// iss = H — ErrIssuerParameter (RFC 9207). +// iss = H — ErrIssuerParameter (RFC 9207 §2.4): compared with A's +// issuer when A advertises RFC 9207, discarded as an iss A never +// announced when it does not. func TestMixUpWithAnAttackerProvider(t *testing.T) { t.Parallel() cases := []struct { - name string - clientID string - redirect string - deliver string // the connection whose callback receives the response; "" when H refuses - want error + name string + clientID string + redirect string + deliver string // the connection whose callback receives the response; "" when H refuses + want error + attackIss bool // A advertises RFC 9207 }{ - {"(a) honest registration, attacker's redirect URI", basicID, callback("attacker"), "", nil}, - {"(b) honest registration and redirect URI", basicID, callback("basic"), "basic", oidcclient.ErrTransactionMismatch}, - {"(c) attacker's registration at the honest provider", "attacker-x", callback("attacker"), "attacker", oidcclient.ErrIssuerParameter}, + {"(a) honest registration, attacker's redirect URI", basicID, callback("attacker"), "", nil, false}, + {"(b) honest registration and redirect URI", basicID, callback("basic"), "basic", oidcclient.ErrTransactionMismatch, false}, + {"(c) attacker's registration at the honest provider, A advertising RFC 9207", "attacker-x", callback("attacker"), "attacker", oidcclient.ErrIssuerParameter, true}, + {"(c) attacker's registration at the honest provider, A silent on RFC 9207", "attacker-x", callback("attacker"), "attacker", oidcclient.ErrIssuerParameter, false}, } for _, tc := range cases { @@ -164,7 +168,7 @@ func TestMixUpWithAnAttackerProvider(t *testing.T) { o := newOP(t) o.clients.add(registration{id: "attacker-x", secret: secret, method: "client_secret_basic", issuers: []string{o.base}, redirects: []string{callback("attacker")}}) - a := newAttackerAS(t, func(q url.Values) string { + a := newAttackerASAdvertising(t, tc.attackIss, func(q url.Values) string { q.Set("client_id", tc.clientID) q.Set("redirect_uri", tc.redirect) @@ -214,7 +218,9 @@ func TestMixUpWithAnAttackerProvider(t *testing.T) { // response came from H — the honest code reaches the attacker's token // endpoint. Never register a connection's redirect URI at another provider; // with providers that do not send iss, distinct redirect URIs are the only -// mix-up defense. +// mix-up defense — Config.RequireIssParameter on the receiving connection +// closes the gap, for applications that work only with providers sending +// iss (TestRequireIssParameterOnTheReceivingConnectionClosesTheResidual). func TestMixUpResidualWithoutRFC9207(t *testing.T) { t.Parallel() @@ -252,6 +258,52 @@ func TestMixUpResidualWithoutRFC9207(t *testing.T) { assert.Equal(t, []string{loc.Query().Get("code")}, a.received(), "the honest code leaked to the attacker") } +// TestRequireIssParameterOnTheReceivingConnectionClosesTheResidual closes +// the residual of TestMixUpResidualWithoutRFC9207: the same attack — H +// without iss, the attacker's client registered at H with the attacker +// connection's callback — against an attacker connection configured with +// Config.RequireIssParameter, as an application sets it on every tenant +// connection. The response without iss is refused there (RFC 9207 §2.4), +// before any token request: the honest code never reaches the attacker. +func TestRequireIssParameterOnTheReceivingConnectionClosesTheResidual(t *testing.T) { + t.Parallel() + + o := newOP(t) + o.clients.add(registration{id: "attacker-x", secret: secret, method: "client_secret_basic", issuers: []string{o.base}, redirects: []string{callback("attacker")}}) + + o.faults.set(o.paths(o.base).Authorize, rule{location: func(u *url.URL) { + q := u.Query() + q.Del("iss") + u.RawQuery = q.Encode() + }}) + + a := newAttackerAS(t, func(q url.Values) string { + q.Set("client_id", "attacker-x") + + return o.base + o.paths(o.base).Authorize + "?" + q.Encode() + }) + + attacker := o.rp(t, conn("attacker", a.srv.URL, "rp-at-attacker", secret, oidcclient.AuthMethodClientSecretBasic), + func(cfg *oidcclient.Config) { cfg.RequireIssParameter = true }) + + ar, err := attacker.Begin(t.Context(), oidcclient.AuthOptions{}) + require.NoError(t, err) + + resp, err := o.browser().Get(ar.URL) + require.NoError(t, err) + _ = resp.Body.Close() + + loc := redirected(t, o.authorize(t, redirected(t, resp).String())) + require.NotEmpty(t, loc.Query().Get("code")) + require.Empty(t, loc.Query().Get("iss")) + + id, err := attacker.Complete(t.Context(), callbackOf(loc, ar.Binding)) + require.ErrorIs(t, err, oidcclient.ErrIssuerParameter) + assert.Nil(t, id) + assert.Empty(t, a.received(), "the honest code never reaches the attacker's token endpoint") + assert.Zero(t, o.hits(o.paths(o.base).Token)) +} + // TestMixUpBetweenIssuersOfOneProvider: the victim's request to tenant-a is // diverted to tenant-b, which answers with its own iss: the client refuses // the response (RFC 9207 §2.4). Defense in depth: tenant-a's token endpoint diff --git a/oidc/client/callback.go b/oidc/client/callback.go index f271df12..755c53d4 100644 --- a/oidc/client/callback.go +++ b/oidc/client/callback.go @@ -114,8 +114,10 @@ type authorizationResponse struct { // used — else ErrTransactionIntegrity; // 4. it must belong to this connection, issuer, client, redirect URI and // response mode — else ErrTransactionMismatch — and be unexpired; -// 5. the RFC 9207 iss parameter must be the issuer when present, and is -// required when the provider advertises it — else ErrIssuerParameter; +// 5. the RFC 9207 iss parameter (§2.4) is required, and must be the +// issuer, when the provider advertises it or Config.RequireIssParameter +// is set; from another provider a response carrying iss is discarded — +// else ErrIssuerParameter; // 6. an error response becomes an *AuthorizationError; // 7. the code is redeemed with the PKCE verifier (RFC 7636 §4.5), the // token response parsed strictly, the ID Token validated (OpenID Connect @@ -306,22 +308,27 @@ func (c *Client) checkTransaction(tx *Transaction, now time.Time) error { return nil } -// checkIssuerParameter applies RFC 9207 §2.4: when present, iss must be -// the issuer (simple string comparison), errors included; it is required -// when the provider advertises authorization_response_iss_parameter_supported. -// A matching iss from a provider that does not advertise it is accepted — -// the local policy §2.4 leaves open. +// checkIssuerParameter applies RFC 9207 §2.4, errors included. The +// provider supports iss when its metadata advertises +// authorization_response_iss_parameter_supported or Config.RequireIssParameter +// says so ("according to the client's configuration"): iss is then +// required and must be the issuer (simple string comparison). From a +// provider that does not, a response carrying iss is discarded ("Clients +// SHOULD discard authorization responses with the iss parameter from +// authorization servers that do not indicate their support"). func (c *Client) checkIssuerParameter(resp authorizationResponse) error { - if resp.hasIss { - if resp.iss != c.md.Issuer { - return fmt.Errorf("%w: iss of another issuer", ErrIssuerParameter) - } - - return nil - } + supported := c.md.AuthorizationResponseIssParameterSupported || c.cfg.RequireIssParameter - if c.md.AuthorizationResponseIssParameterSupported { + switch { + case !resp.hasIss && supported: return fmt.Errorf("%w: iss missing although the provider sends it", ErrIssuerParameter) + case !resp.hasIss: + return nil + case !supported: + return fmt.Errorf("%w: iss from a provider that does not advertise authorization_response_iss_parameter_supported "+ + "(set Config.RequireIssParameter if it sends iss in every response)", ErrIssuerParameter) + case resp.iss != c.md.Issuer: + return fmt.Errorf("%w: iss of another issuer", ErrIssuerParameter) } return nil diff --git a/oidc/client/callback_test.go b/oidc/client/callback_test.go index b78807e0..81413b62 100644 --- a/oidc/client/callback_test.go +++ b/oidc/client/callback_test.go @@ -812,21 +812,33 @@ func TestCompleteRefusesMalformedCallbacks(t *testing.T) { func TestCompleteIssParameter(t *testing.T) { t.Parallel() + equal := func(op *fakeOP) []string { return []string{op.issuer} } + absent := func(*fakeOP) []string { return nil } + empty := func(*fakeOP) []string { return []string{""} } + other := func(*fakeOP) []string { return []string{"https://other.example.com"} } + cases := []struct { name string advertised bool + required bool // Config.RequireIssParameter iss func(op *fakeOP) []string ok bool }{ - {"advertised and equal", true, func(op *fakeOP) []string { return []string{op.issuer} }, true}, - {"advertised and missing", true, func(*fakeOP) []string { return nil }, false}, - {"advertised and empty", true, func(*fakeOP) []string { return []string{""} }, false}, - {"advertised and different", true, func(*fakeOP) []string { return []string{"https://other.example.com"} }, false}, - {"advertised, trailing slash", true, func(op *fakeOP) []string { return []string{op.issuer + "/"} }, false}, - {"not advertised and absent", false, func(*fakeOP) []string { return nil }, true}, - {"not advertised and empty", false, func(*fakeOP) []string { return []string{""} }, true}, - {"not advertised and equal", false, func(op *fakeOP) []string { return []string{op.issuer} }, true}, - {"not advertised and different", false, func(*fakeOP) []string { return []string{"https://other.example.com"} }, false}, + {"advertised and equal", true, false, equal, true}, + {"advertised and missing", true, false, absent, false}, + {"advertised and empty", true, false, empty, false}, + {"advertised and different", true, false, other, false}, + {"advertised, trailing slash", true, false, func(op *fakeOP) []string { return []string{op.issuer + "/"} }, false}, + {"not advertised and absent", false, false, absent, true}, + {"not advertised and empty", false, false, empty, true}, + {"not advertised and equal: discarded (RFC 9207 §2.4)", false, false, equal, false}, + {"not advertised and different", false, false, other, false}, + {"required, not advertised, equal", false, true, equal, true}, + {"required, not advertised, absent", false, true, absent, false}, + {"required, not advertised, empty", false, true, empty, false}, + {"required, not advertised, different", false, true, other, false}, + {"required and advertised, equal", true, true, equal, true}, + {"required and advertised, absent", true, true, absent, false}, } for _, tc := range cases { @@ -835,7 +847,7 @@ func TestCompleteIssParameter(t *testing.T) { op := newFakeOP(t) op.set(func(op *fakeOP) { op.metadata["authorization_response_iss_parameter_supported"] = tc.advertised }) - c := op.client(t) + c := op.client(t, func(cfg *oidcclient.Config) { cfg.RequireIssParameter = tc.required }) ar, code := op.begin(t, c, oidcclient.AuthOptions{}) cb := callbackFor(ar, code) @@ -843,7 +855,7 @@ func TestCompleteIssParameter(t *testing.T) { cb.Params["iss"] = iss } - _, err := c.Complete(context.Background(), cb) + id, err := c.Complete(context.Background(), cb) if tc.ok { require.NoError(t, err) @@ -851,10 +863,35 @@ func TestCompleteIssParameter(t *testing.T) { } require.ErrorIs(t, err, oidcclient.ErrIssuerParameter) + assert.Nil(t, id) assert.Zero(t, op.hitCount("/token")) }) } + t.Run("the refusal of an unadvertised iss names the option", func(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + c := op.client(t) + ar, code := op.begin(t, c, oidcclient.AuthOptions{}) + + cb := callbackFor(ar, code) + cb.Params["iss"] = []string{op.issuer} + + _, err := c.Complete(context.Background(), cb) + require.ErrorIs(t, err, oidcclient.ErrIssuerParameter) + assert.ErrorContains(t, err, "RequireIssParameter") + }) + + errorResponse := func(ar *oidcclient.AuthRequest, iss ...string) oidcclient.Callback { + params := url.Values{"error": {"access_denied"}, "state": {ar.State}} + if len(iss) > 0 { + params["iss"] = iss + } + + return oidcclient.Callback{Mode: oidcclient.ResponseModeQuery, URL: receivedOn(testRedirectURL), Binding: ar.Binding, Params: params} + } + t.Run("error response without iss when advertised", func(t *testing.T) { t.Parallel() @@ -863,12 +900,33 @@ func TestCompleteIssParameter(t *testing.T) { c := op.client(t) ar, _ := op.begin(t, c, oidcclient.AuthOptions{}) - _, err := c.Complete(context.Background(), oidcclient.Callback{ - Mode: oidcclient.ResponseModeQuery, URL: receivedOn(testRedirectURL), Binding: ar.Binding, - Params: url.Values{"error": {"access_denied"}, "state": {ar.State}}, - }) + _, err := c.Complete(context.Background(), errorResponse(ar)) require.ErrorIs(t, err, oidcclient.ErrIssuerParameter, "RFC 9207 §2.4: an error is not trusted either") }) + + t.Run("error response without iss when required", func(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + c := op.client(t, func(cfg *oidcclient.Config) { cfg.RequireIssParameter = true }) + ar, _ := op.begin(t, c, oidcclient.AuthOptions{}) + + _, err := c.Complete(context.Background(), errorResponse(ar)) + require.ErrorIs(t, err, oidcclient.ErrIssuerParameter) + assert.NotErrorIs(t, err, oidcclient.ErrAuthorizationFailed) + }) + + t.Run("error response with an unadvertised matching iss", func(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + c := op.client(t) + ar, _ := op.begin(t, c, oidcclient.AuthOptions{}) + + _, err := c.Complete(context.Background(), errorResponse(ar, op.issuer)) + require.ErrorIs(t, err, oidcclient.ErrIssuerParameter, "the iss check comes before the error response") + assert.NotErrorIs(t, err, oidcclient.ErrAuthorizationFailed) + }) } // TestCompleteAuthorizationError checks the error response of RFC 6749 diff --git a/oidc/client/config.go b/oidc/client/config.go index c7a055e9..a57e8593 100644 --- a/oidc/client/config.go +++ b/oidc/client/config.go @@ -214,6 +214,30 @@ type Config struct { // tenant, log these errors and show the tenant a generic message. Outbound outbound.Policy + // RequireIssParameter declares that the provider sends the RFC 9207 iss + // parameter in every authorization response, whatever its metadata + // says — RFC 9207 §2.4, "according to the client's configuration". + // Complete then refuses a response without iss, errors included, with + // ErrIssuerParameter, and compares the iss of the others with the + // issuer. Without it, iss is required when the provider advertises + // authorization_response_iss_parameter_supported, and a response + // carrying iss from a provider that does not is refused (RFC 9207 §2.4: + // "Clients SHOULD discard authorization responses with the iss + // parameter from authorization servers that do not indicate their + // support"): set it for a provider that sends iss without advertising + // it. + // + // It protects the connection that receives the response. The mix-up + // that distinct redirect URIs leave open (RFC 9700 §4.4.2.2) sends the + // honest provider's response, without iss, to the callback of a + // connection whose provider is the attacker's: only that connection + // requiring iss refuses it. An application whose tenants configure + // providers therefore sets it on every tenant connection — the + // application's setting, never the tenant's — and then works only with + // providers that send iss (RFC 9207 §2.4: clients "MAY … exclusively + // support authorization servers that provide the iss parameter"). + RequireIssParameter bool + // InsecureCookies makes the net/http helpers set the binding cookie // without the __Host- prefix and the Secure attribute, for local // development over plain http. New refuses it unless RedirectURL is diff --git a/oidc/client/doc.go b/oidc/client/doc.go index 15338f5c..7a644c82 100644 --- a/oidc/client/doc.go +++ b/oidc/client/doc.go @@ -69,8 +69,11 @@ // redirect URI — distinct from every other connection's by scheme, // host, port, path or query parameter — and Complete refuses a response // received on another URL than the connection's. Providers that -// advertise RFC 9207 are additionally checked on the iss parameter. -// Never route callbacks by the state value. +// advertise RFC 9207, or that Config.RequireIssParameter declares, are +// additionally checked on the iss parameter, which closes the residual +// of distinct redirect URIs (RFC 9700 §4.4.2.2) on the connection that +// receives the response; an iss from another provider is discarded +// (RFC 9207 §2.4). Never route callbacks by the state value. // - Nothing is returned before the ID Token is fully validated: tokens of // a failed login are never exposed (RFC 9700 §4.5.3.2). // - Every credential — the client secret, the transaction keys, the diff --git a/oidc/client/errors.go b/oidc/client/errors.go index 79fea6e8..ab7b4a0c 100644 --- a/oidc/client/errors.go +++ b/oidc/client/errors.go @@ -71,8 +71,10 @@ var ( // ErrTransactionExpired reports a transaction past its expiry. ErrTransactionExpired = fmt.Errorf("oidcclient: login transaction expired: %w", security.ErrInvalidCredentials) // ErrIssuerParameter reports an authorization response whose RFC 9207 - // iss parameter is missing while the provider advertises it, or does - // not equal the provider's issuer. + // iss parameter (§2.4) is missing while the provider advertises it or + // Config.RequireIssParameter is set, does not equal the provider's + // issuer, or comes from a provider that does not advertise it without + // Config.RequireIssParameter. ErrIssuerParameter = fmt.Errorf("oidcclient: authorization response issuer mismatch: %w", security.ErrInvalidCredentials) // ErrAuthorizationFailed is matched by every *AuthorizationError: the // provider answered the authorization request with an error. From e79cda30e70915eeeb13a517ce906f0bedeeda77 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 10:53:34 +0200 Subject: [PATCH 05/10] fix(oidc/client)!: bound the in-memory transaction store by default 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. --- CHANGELOG.md | 5 +++ LIMITATIONS.md | 9 +++--- MIGRATION.md | 22 ++++++++++++++ docs/oidc-client.md | 5 +-- oidc/client/storage/memory/memory.go | 37 ++++++++++++++--------- oidc/client/storage/memory/memory_test.go | 26 ++++++++++++++++ 6 files changed, 84 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40168474..318eec1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1062,6 +1062,11 @@ legacy packages (`authentication/`, `authorization/`, the in-tree RFC 9207 §2.4: "Clients SHOULD discard…"); a matching `iss` used to be accepted. Breaking for a provider that sends `iss` without advertising it: set `Config.RequireIssParameter` (see MIGRATION.md). +- `oidc/client/storage/memory` is bounded by default: `New()` keeps at most + `memory.DefaultCapacity` (10 000) pending logins and refuses the next + with `memory.ErrFull`; it used to grow without bound unless built with + `memory.WithCapacity`, while login starts are unauthenticated (breaking + above 10 000 pending logins; see MIGRATION.md). - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in diff --git a/LIMITATIONS.md b/LIMITATIONS.md index a1162165..b87486f1 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -325,10 +325,11 @@ PKCE. Not supported, and refused where a provider could select them: logins. Removing a transaction key before `TransactionTTL` has elapsed fails the logins it protected. `oidc/client/storage/memory` keeps transactions in the process: use the SQL or Redis store when several instances serve - callbacks. It is unbounded unless built with `memory.WithCapacity(n)`: - login starts are unauthenticated, so size it — about 0.5 KB per pending - login plus its `AppState` (up to 4 KiB), kept for `TransactionTTL` — and - rate-limit login starts, since a full store refuses every new login + callbacks. It keeps at most `memory.DefaultCapacity` (10 000) pending + logins, or the bound of `memory.WithCapacity(n)`: login starts are + unauthenticated, so size it — about 0.5 KB per pending login plus its + `AppState` (up to 4 KiB), kept for `TransactionTTL` — and rate-limit + login starts, since a full store refuses every new login (`memory.ErrFull`) until transactions expire or are consumed. `oidc/client/store/sql` ships `PurgeExpired`, a single unbatched `DELETE` the operator schedules (index `expires_at`, see its diff --git a/MIGRATION.md b/MIGRATION.md index ab48dad4..a1a3549b 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3138,3 +3138,25 @@ To migrate: `RequireIssParameter: true` on every tenant connection — its own setting, never one the tenant chooses — to close the mix-up residual of RFC 9700 §4.4.2.2; it then works only with providers that send `iss`. + +### `oidc/client/storage/memory`: bounded by default + +Login starts are unauthenticated, and `memory.New()` kept every pending +login until it expired: a flood of starts grew the store until the process +ran out of memory, unless the application passed `memory.WithCapacity`. A +store built without it now keeps at most `memory.DefaultCapacity` +transactions. + +| `memory.New(…)` | Before | After | +| --- | --- | --- | +| without `WithCapacity` | unbounded | at most `DefaultCapacity` (10 000) live transactions; the next `Save` fails with `memory.ErrFull` after dropping the expired ones | +| `WithCapacity(n)` | at most `n` | unchanged | + +To migrate: + +1. Nothing below 10 000 pending logins — about 16 login starts a second + over the default 10-minute `TransactionTTL`. +2. Above, pass `memory.WithCapacity(n)` sized from the login rate and the + TTL (about 0.5 KB per transaction plus its `AppState`), map + `memory.ErrFull` to `503 Service Unavailable` (as `examples/oidc-client` + does), and rate-limit login starts — or use the SQL or Redis store. diff --git a/docs/oidc-client.md b/docs/oidc-client.md index 43a4c571..6bd79912 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -115,8 +115,9 @@ mux.HandleFunc("GET /oidc/acme/callback", func(w http.ResponseWriter, r *http.Re - Start logins on a user action from the application's own origin (a POST behind `http.CrossOriginProtection` or a CSRF token) and rate limit them: each start writes a transaction, and non-browser clients pass - `CrossOriginProtection`. With `storage/memory`, bound what a flood can - hold with `memory.WithCapacity(n)` (a full store answers `memory.ErrFull`; + `CrossOriginProtection`. `storage/memory` keeps at most + `memory.DefaultCapacity` (10 000) pending logins, or the bound of + `memory.WithCapacity(n)` (a full store answers `memory.ErrFull`; `examples/oidc-client` maps it to 503 and rate-limits starts per client and per network — an IPv4 /24, an IPv6 /48 — so that one network cannot fill it). The capacity bounds memory, not availability: a flood from diff --git a/oidc/client/storage/memory/memory.go b/oidc/client/storage/memory/memory.go index b9ca0770..e9de2b7c 100644 --- a/oidc/client/storage/memory/memory.go +++ b/oidc/client/storage/memory/memory.go @@ -9,11 +9,13 @@ // MUST use a shared store (oidc/client/store/sql or oidc/client/store/redis). // // Expired transactions — abandoned logins — are swept every 64 operations -// once one may have expired, so the store stays bounded by the login rate -// times the transaction TTL. Login starts are unauthenticated: without -// WithCapacity, a flood of them grows the store until it is rate limited -// or the process runs out of memory. One mutex serializes every operation; -// it is the linearization point of the single-consumption guarantee. +// once one may have expired. Login starts are unauthenticated: the store +// keeps at most DefaultCapacity live transactions — WithCapacity sets +// another bound — and refuses the next with ErrFull, so that a flood of +// starts cannot exhaust the process's memory; rate-limit them too, since a +// full store fails every new login until transactions expire. One mutex +// serializes every operation; it is the linearization point of the +// single-consumption guarantee. package memory import ( @@ -28,16 +30,22 @@ import ( ) // ErrFull reports a Save refused because the store holds its capacity of -// transactions (WithCapacity): the login fails to start; no pending login -// is evicted to make room. +// transactions (DefaultCapacity, or WithCapacity): the login fails to +// start; no pending login is evicted to make room. var ErrFull = errors.New("memory: transaction store full") +// DefaultCapacity is the number of live transactions a Store built without +// WithCapacity keeps: 10 000 pending logins — about 16 login starts a +// second over the default 10-minute TransactionTTL, 2.8 over the maximum +// of one hour — at most about 46 MB with 4 KiB AppStates. +const DefaultCapacity = 10_000 + // Option configures a Store. type Option func(*Store) -// WithCapacity bounds the transactions the store retains to n, at least 1: -// it panics otherwise, so that a bound computed as zero never silently -// disables itself. At capacity, Save first drops the expired transactions, +// WithCapacity bounds the transactions the store retains to n instead of +// DefaultCapacity, at least 1: it panics otherwise, so that a bound +// computed as zero never silently disables itself. At capacity, Save first drops the expired transactions, // then refuses with ErrFull; a pending login is never evicted. A full store // refuses in constant time — it walks its transactions at most once per // expiry, not once per refused login. Size it from the login rate and the @@ -59,7 +67,7 @@ const sweepEvery = 64 // Store is an in-memory oidcclient.TransactionStore, safe for concurrent // use. The zero value is unusable; build one with New. type Store struct { - capacity int // 0: no bound + capacity int mu sync.Mutex txs map[string]oidcclient.Transaction @@ -71,9 +79,10 @@ type Store struct { sweeps int // the walks of txs, for the tests } -// New returns an empty Store, unbounded unless WithCapacity says otherwise. +// New returns an empty Store keeping at most DefaultCapacity transactions, +// unless WithCapacity says otherwise. func New(opts ...Option) *Store { - s := &Store{txs: make(map[string]oidcclient.Transaction)} + s := &Store{capacity: DefaultCapacity, txs: make(map[string]oidcclient.Transaction)} for _, opt := range opts { opt(s) } @@ -98,7 +107,7 @@ func (s *Store) Save(_ context.Context, tx *oidcclient.Transaction) error { return oidcclient.ErrTransactionExists } - if s.capacity > 0 && len(s.txs) >= s.capacity { + if len(s.txs) >= s.capacity { s.sweepExpired(tx.CreatedAt) if len(s.txs) >= s.capacity { diff --git a/oidc/client/storage/memory/memory_test.go b/oidc/client/storage/memory/memory_test.go index 1288d383..9bd51c9f 100644 --- a/oidc/client/storage/memory/memory_test.go +++ b/oidc/client/storage/memory/memory_test.go @@ -110,6 +110,32 @@ func TestCapacity(t *testing.T) { assert.Equal(t, 3, s.Len()) } +// TestStoreIsBoundedByDefault checks that a Store built without +// WithCapacity keeps at most DefaultCapacity live transactions: login +// starts are unauthenticated, so an unbounded default would let a flood of +// them exhaust the process's memory. +func TestStoreIsBoundedByDefault(t *testing.T) { + t.Parallel() + + const capacity = 10_000 + + require.Equal(t, capacity, memory.DefaultCapacity) + + s := memory.New() + now := time.Unix(1_800_000_000, 0).UTC() + + for i := range capacity { + require.NoError(t, s.Save(context.Background(), transaction("live-"+strconv.Itoa(i), now, 10*time.Minute))) + } + + require.ErrorIs(t, s.Save(context.Background(), transaction("refused", now, 10*time.Minute)), memory.ErrFull) + assert.Equal(t, capacity, s.Len()) + + require.NoError(t, s.Save(context.Background(), transaction("later", now.Add(10*time.Minute), 10*time.Minute)), + "once the transactions expire, the store takes new ones") + assert.Equal(t, 1, s.Len()) +} + // TestCapacityDropsExpiredFirst checks that a full store drops its expired // transactions before refusing — without waiting for the periodic sweep. func TestCapacityDropsExpiredFirst(t *testing.T) { From 4a37be8c604ab9ef5b8599b7288b977156774800 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 10:57:11 +0200 Subject: [PATCH 06/10] fix(oidc/client): keep the state out of AuthRequest renderings 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. --- CHANGELOG.md | 5 ++ LIMITATIONS.md | 8 +++ docs/oidc-client.md | 5 ++ docs/security-considerations.md | 10 ++- oidc/client/begin.go | 43 +++++++++---- oidc/client/http_test.go | 109 ++++++++++++++++++++++++++++++++ oidc/client/redaction_test.go | 42 ++++++++++++ oidc/client/transaction.go | 4 +- 8 files changed, 211 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 318eec1c..08861161 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1067,6 +1067,11 @@ legacy packages (`authentication/`, `authorization/`, the in-tree with `memory.ErrFull`; it used to grow without bound unless built with `memory.WithCapacity`, while login starts are unauthenticated (breaking above 10 000 pending logins; see MIGRATION.md). +- `oidcclient.AuthRequest` prints and logs (`String`, `GoString`, `Format`, + `LogValue`) without its state nor the query of its URL — hints included — + with a leaked state, a request to the callback through the victim's own + browser burns the pending login. Its JSON encoding, the hand-off of the + request to a front end, is unchanged. - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in diff --git a/LIMITATIONS.md b/LIMITATIONS.md index b87486f1..152d07c2 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -295,6 +295,14 @@ PKCE. Not supported, and refused where a provider could select them: run time. One exception: a public client (`none`) requires the provider to advertise S256 in `code_challenge_methods_supported`, which some providers omit although they support it. +- **A leaked state burns the pending login** — an authorization response is + not authenticated: any request to the callback carrying the state, + through the victim's own browser (which presents the binding cookie on a + top-level navigation), 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. It cannot complete a login for the + attacker. `AuthRequest` prints and logs without the state; its JSON + encoding, the hand-off to a front end, keeps it. - **RFC 9207 `iss` policy** — a response carrying `iss` from a provider that does not advertise `authorization_response_iss_parameter_supported` is discarded (`ErrIssuerParameter`, RFC 9207 §2.4) unless diff --git a/docs/oidc-client.md b/docs/oidc-client.md index 6bd79912..96701923 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -162,6 +162,11 @@ distinct redirect URIs). - In `form_post` mode exempt the callback route from CSRF middleware: the cross-site POST is the protocol, the binding cookie its defense. Leave the body unread before `HandleCallback`, or let it reuse `r.PostForm`. +- A leaked state lets anyone burn the pending login: any request to the + callback carrying it, through the victim's browser, expires the binding + cookie (`TestLeakedStateBurnsThePendingLoginThroughTheVictimBrowser`). + The user starts over; keep the state and the authorization URL out of + logs (`AuthRequest` prints and logs without them). - Behind a reverse proxy, preserve the Host header (`HandleCallback` compares the URL the browser used) or call `Complete` with the public URL. - After a successful callback, create a fresh session (a new ID) and diff --git a/docs/security-considerations.md b/docs/security-considerations.md index 07e9538f..b22d6ac7 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1408,8 +1408,14 @@ are attacker-influenced. - **Single use** — `TransactionStore.Consume` removes the transaction atomically, unknown first, then expired whatever the binding, then a binding mismatch, which keeps the transaction so that a leaked state - cannot burn the victim's login. A replayed callback finds nothing and - never reaches the token endpoint. + presented from another browser cannot burn the victim's login. A + replayed callback finds nothing and never reaches the token endpoint. + An authorization response is not authenticated, though: whoever learns + the state can send the victim's own browser — which presents the + binding cookie on a top-level navigation — to the callback with it, and + that request expires the cookie (and, with an error or a bogus code, + consumes the transaction): the login fails and the user starts over. Keep + the state out of logs — `AuthRequest` prints and logs without it. - **Mix-up attacks** (RFC 9700 §4.4) — give every connection its own redirect URI, distinct from every other connection's (by scheme, host, port, path or query parameter): `Complete` refuses a response received on diff --git a/oidc/client/begin.go b/oidc/client/begin.go index ccc49d4a..989e06b7 100644 --- a/oidc/client/begin.go +++ b/oidc/client/begin.go @@ -74,8 +74,13 @@ type AuthOptions struct { AppState string } -// AuthRequest is a started login. It prints, logs and encodes to JSON -// without its binding secret (String, LogValue, MarshalJSON). +// AuthRequest is a started login. It prints and logs (String, GoString, +// Format, LogValue) without its binding secret, its state nor the query of +// its URL — with a leaked state, a request to the callback through the +// victim's own browser burns the pending login. It encodes to JSON +// (MarshalJSON), the hand-off of the request to a front end, with its URL +// and state but without its binding secret: never log it nested in another +// value under log/slog's JSON handler. type AuthRequest struct { // URL is the authorization request: send the user agent to it. URL string @@ -92,23 +97,39 @@ type AuthRequest struct { ExpiresAt time.Time } -// String implements fmt.Stringer without the binding secret. +// String implements fmt.Stringer without the binding secret, the state +// nor the URL's query. func (r AuthRequest) String() string { - return fmt.Sprintf("oidcclient.AuthRequest{URL: %q, ExpiresAt: %s, Binding: REDACTED}", r.URL, r.ExpiresAt.Format(time.RFC3339)) + return fmt.Sprintf("oidcclient.AuthRequest{URL: %q, State: REDACTED, ExpiresAt: %s, Binding: REDACTED}", + requestURLForLogs(r.URL), r.ExpiresAt.Format(time.RFC3339)) } -// GoString implements fmt.GoStringer without the binding secret. +// GoString implements fmt.GoStringer like String. func (r AuthRequest) GoString() string { return r.String() } -// LogValue implements slog.LogValuer without the binding secret. +// LogValue implements slog.LogValuer like String. func (r AuthRequest) LogValue() slog.Value { - return slog.GroupValue(slog.String("url", r.URL), slog.Time("expires_at", r.ExpiresAt), slog.String("binding", redactedMark)) + return slog.GroupValue(slog.String("url", requestURLForLogs(r.URL)), slog.String("state", redactedMark), + slog.Time("expires_at", r.ExpiresAt), slog.String("binding", redactedMark)) } -// MarshalJSON implements json.Marshaler without the binding secret, which -// the default encoding would write — and log/slog's JSON handler encodes -// a value nested in another one (a struct field, a map) with -// encoding/json, LogValue applying to attribute values only. A struct +// requestURLForLogs renders an authorization request URL without its query +// — state, nonce, hints — nor fragment or userinfo; REDACTED when it does +// not parse. +func requestURLForLogs(raw string) string { + u, err := url.Parse(raw) + if err != nil { + return redactedMark + } + + return withoutQuery(u) +} + +// MarshalJSON implements json.Marshaler with the URL and the state — the +// encoding hands the request to a front end — but without the binding +// secret. log/slog's JSON handler encodes a value nested in another one (a +// struct field, a map) with encoding/json, LogValue applying to attribute +// values only: log an AuthRequest as an attribute of its own. A struct // embedding AuthRequest is promoted this method and encodes as the // AuthRequest alone — or, when another type embedded at the same depth // also has a MarshalJSON, neither is promoted and encoding/json writes the diff --git a/oidc/client/http_test.go b/oidc/client/http_test.go index 7df47402..c321c4d8 100644 --- a/oidc/client/http_test.go +++ b/oidc/client/http_test.go @@ -14,6 +14,7 @@ import ( "net/http/httptest" "net/url" "strings" + "sync" "testing" oidcclient "github.com/hyperscale-stack/security/oidc/client" @@ -530,6 +531,114 @@ func TestEndToEndQuery(t *testing.T) { assert.Equal(t, "hello "+testSubject, string(body)) } +// TestLeakedStateBurnsThePendingLoginThroughTheVictimBrowser characterizes +// a residual risk, and documents it: an authorization response is not +// authenticated, so whoever learned the state of a pending login can send +// the victim's own browser — which presents the binding cookie, SameSite=Lax +// on a top-level navigation — to the callback with that state. The response +// to that request expires the binding cookie, and with an error (or a +// bogus code) the transaction is consumed: the genuine callback then fails +// (ErrBindingMismatch, the cookie gone) and the user starts over. A +// request carrying the state alone — no code, no error — burns the login +// through the cookie only. Keep the state out of logs (AuthRequest prints +// without it). +func TestLeakedStateBurnsThePendingLoginThroughTheVictimBrowser(t *testing.T) { + t.Parallel() + + for name, forged := range map[string]url.Values{ + "forged error response": {"error": {"access_denied"}}, + "state alone": {}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + + var ( + mu sync.Mutex + lastErr error + ) + + var c *oidcclient.Client + + mux := http.NewServeMux() + mux.HandleFunc("GET /login", func(w http.ResponseWriter, r *http.Request) { + assert.NoError(t, c.StartLogin(w, r, oidcclient.AuthOptions{})) + }) + mux.HandleFunc(testCallbackPath, func(w http.ResponseWriter, r *http.Request) { + _, err := c.HandleCallback(w, r) + + mu.Lock() + lastErr = err + mu.Unlock() + }) + + rp := httptest.NewTLSServer(mux) + t.Cleanup(rp.Close) + + c = op.client(t, func(cfg *oidcclient.Config) { cfg.RedirectURL = rp.URL + testCallbackPath }) + rpURL, err := url.Parse(rp.URL) + require.NoError(t, err) + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + victim := *op.srv.Client() + victim.Jar = jar + victim.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse } + + get := func(target string) error { + resp, err := victim.Get(target) + require.NoError(t, err) + _ = resp.Body.Close() + + mu.Lock() + defer mu.Unlock() + + return lastErr + } + + resp, err := victim.Get(rp.URL + "/login") + require.NoError(t, err) + _ = resp.Body.Close() + + authURL := resp.Header.Get("Location") + loc, err := url.Parse(authURL) + require.NoError(t, err) + + state := loc.Query().Get("state") + require.Len(t, jar.Cookies(rpURL), 1, "the binding cookie of the pending login") + binding := jar.Cookies(rpURL)[0].Value + code := op.authorize(t, authURL) + + // The attacker, knowing the state, makes the victim's browser + // request the callback. + forged.Set("state", state) + require.Error(t, get(rp.URL+testCallbackPath+"?"+forged.Encode())) + assert.Empty(t, jar.Cookies(rpURL), "the response expired the binding cookie") + + // The genuine response arrives without the cookie. + err = get(rp.URL + testCallbackPath + "?" + url.Values{"state": {state}, "code": {code}}.Encode()) + require.ErrorIs(t, err, oidcclient.ErrBindingMismatch) + assert.Zero(t, op.hitCount("/token")) + + // The store: an error response consumed the transaction; the + // state alone, refused before the store, left it. + received := receivedOn(rp.URL + testCallbackPath) + received.RawQuery = url.Values{"state": {state}, "code": {code}}.Encode() + _, err = c.Complete(context.Background(), oidcclient.Callback{ + Mode: oidcclient.ResponseModeQuery, URL: received, Params: received.Query(), Binding: oidcclient.NewSecret(binding), + }) + + if forged.Has("error") { + require.ErrorIs(t, err, oidcclient.ErrTransactionNotFound) + } else { + require.NoError(t, err) + } + }) + } +} + // TestEndToEndInsecureCookiesOnLoopback runs a login over plain http on the // loopback interface — the local development setup InsecureCookies is for — // through a browser's cookie jar, which keeps a cookie without Secure on diff --git a/oidc/client/redaction_test.go b/oidc/client/redaction_test.go index 1458deba..f6a2e489 100644 --- a/oidc/client/redaction_test.go +++ b/oidc/client/redaction_test.go @@ -117,6 +117,48 @@ func TestSecretsAreRedacted(t *testing.T) { assert.Contains(t, logged(id), id.Subject, "an identity stays recognizable") } +// TestAuthRequestRenderingsOmitTheState checks that an AuthRequest prints +// and logs without its state — with a leaked state, a request to the +// callback through the victim's browser burns the pending login — nor the +// rest of the authorization request's query (hints included): the URL is +// rendered without its query. Its JSON encoding, the hand-off of the +// request to a front end, keeps them. +func TestAuthRequestRenderingsOmitTheState(t *testing.T) { + t.Parallel() + + op := newFakeOP(t) + c := op.client(t) + + ar, err := c.Begin(context.Background(), oidcclient.AuthOptions{LoginHint: "jane@example.com"}) + require.NoError(t, err) + + endpoint, err := url.Parse(ar.URL) + require.NoError(t, err) + + endpoint.RawQuery = "" + + for _, v := range []any{*ar, ar} { + for way, rendered := range renderings(v) { + assert.NotContains(t, rendered, ar.State, "%s reveals the state", way) + assert.NotContains(t, rendered, "&state=", way) + assert.NotContains(t, rendered, "?", "%s renders the URL with its query", way) + assert.NotContains(t, rendered, "jane", "%s reveals the login hint", way) + assert.NotContains(t, rendered, ar.Binding.Reveal(), way) + } + + assert.Contains(t, logged(v), endpoint.String(), "the authorization endpoint stays readable") + assert.Contains(t, fmt.Sprint(v), "State: REDACTED") + } + + b, err := json.Marshal(ar) + require.NoError(t, err) + assert.Contains(t, string(b), ar.State, "the JSON hand-off keeps the state") + + unparsable := oidcclient.AuthRequest{URL: "https://op.example/authorize?state=secret-state\x7f\n%zz"} + assert.Contains(t, unparsable.String(), `URL: "REDACTED"`) + assert.NotContains(t, logged(unparsable), "secret-state") +} + func TestDefaultClock(t *testing.T) { t.Parallel() diff --git a/oidc/client/transaction.go b/oidc/client/transaction.go index dd794d39..e6fe5f9f 100644 --- a/oidc/client/transaction.go +++ b/oidc/client/transaction.go @@ -175,8 +175,8 @@ type TransactionStore interface { // - expired (now is not before ExpiresAt) → ErrTransactionNotFound, // whatever the binding; the store may delete it; // - BindingHash different from bindingHash → ErrBindingMismatch, and - // the transaction stays stored: a leaked state alone cannot burn a - // login; + // the transaction stays stored: a leaked state alone, presented from + // another browser, cannot burn a login; // - otherwise the transaction, removed so that every later or // concurrent call gets ErrTransactionNotFound. // From c57b0c31f296a9e901bae9190e0431a450492b44 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 11:00:13 +0200 Subject: [PATCH 07/10] fix(oidc/client): refresh the provider JWK Set every five minutes 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. --- CHANGELOG.md | 7 ++++++- LIMITATIONS.md | 9 +++++++++ MIGRATION.md | 19 +++++++++++++++++++ docs/oidc-client.md | 10 +++++----- docs/oidc-provider.md | 10 +++++----- docs/security-considerations.md | 8 +++++--- internal/interop/oidcstack/rotation_test.go | 16 ++++++++-------- oidc/client/keyset.go | 10 +++++++--- oidc/client/keyset_test.go | 6 +++--- 9 files changed, 67 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08861161..82e89792 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -228,7 +228,7 @@ legacy packages (`authentication/`, `authorization/`, the in-tree audiences only if trusted), azp, exp/iat/nbf with a bounded skew, nonce, auth_time against `max_age`, acr against the requested `acr_values`, and at_hash. Keys come from the issuer's `jwks_uri` only, through a bounded - cache (1 h TTL, one rate-limited refresh for an unknown `kid`, + cache (5 min TTL, one rate-limited refresh for an unknown `kid`, single-flight, 24 h stale limit). Every JSON document is parsed strictly (duplicate or case-variant member names refused at every level). Token and UserInfo requests never follow redirects. `UserInfo` returns claims @@ -1072,6 +1072,11 @@ legacy packages (`authentication/`, `authorization/`, the in-tree with a leaked state, a request to the callback through the victim's own browser burns the pending login. Its JSON encoding, the hand-off of the request to a front end, is unchanged. +- `oidc/client` caches the provider's JWK Set for five minutes instead of + one hour — the TTL of the `jwt` module's remote key sets — so a key the + provider withdraws stops verifying ID Tokens within five minutes while + the JWK Set endpoint answers; the 24-hour staleness limit while fetches + fail is unchanged. - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 152d07c2..5305eff5 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -295,6 +295,15 @@ PKCE. Not supported, and refused where a provider could select them: run time. One exception: a public client (`none`) requires the provider to advertise S256 in `code_challenge_methods_supported`, which some providers omit although they support it. +- **A withdrawn provider key stays trusted for a while** — the provider's + JWK Set is cached five minutes: a key the provider withdrew (a leaked + one) keeps verifying ID Tokens until the next refresh. While the JWK Set + fetches fail, the cached keys serve for up to 24 hours, so an attacker + holding a withdrawn key and able to make those fetches fail (a network + position, a denial of service on the `jwks_uri`) keeps it trusted that + long — the availability trade-off of the `jwt` module's remote key sets. + Rebuilding the `Client` drops the cache at once (OpenID Connect Core + §10.1.1 sets no bound). - **A leaked state burns the pending login** — an authorization response is not authenticated: any request to the callback carrying the state, through the victim's own browser (which presents the binding cookie on a diff --git a/MIGRATION.md b/MIGRATION.md index a1a3549b..e17d577e 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3160,3 +3160,22 @@ To migrate: TTL (about 0.5 KB per transaction plus its `AppState`), map `memory.ErrFull` to `503 Service Unavailable` (as `examples/oidc-client` does), and rate-limit login starts — or use the SQL or Redis store. + +### `oidc/client`: the provider's JWK Set is cached five minutes + +A key the provider withdrew — a leaked one — kept verifying ID Tokens until +the client's cached JWK Set was an hour old. The cache now lives five +minutes, like the `jwt` module's remote key sets (`DefaultCacheTTL`). + +| Event | Before | After | +| --- | --- | --- | +| A withdrawn key keeps verifying | up to one hour | up to five minutes | +| A newly published key is learned without an unknown-`kid` refetch | within one hour | within five minutes | +| JWK Set requests | one per hour and `Client`, plus unknown-`kid` refetches | one per five minutes and `Client` with logins, plus unknown-`kid` refetches | +| Cached keys while fetches fail | up to 24 hours | unchanged | + +To migrate: + +1. Nothing in the application. +2. A provider with a strict rate limit on its `jwks_uri` sees up to twelve + requests an hour per `Client` instead of one. diff --git a/docs/oidc-client.md b/docs/oidc-client.md index 96701923..a3afcf35 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -75,7 +75,7 @@ Test names refer to `oidc/client` unless prefixed with `stack:` | Capability | Spec | Status | Tests | | ---------------------------- | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------- | | Discovery | Discovery §4 | Required; the metadata issuer must equal the configured one byte for byte; templated multi-tenant issuers refused; WebFinger and static metadata not supported | `TestDiscoveryURL`, `TestIssuerMismatch`, `TestRequiredMetadata`, `TestStrictMetadataJSON`, rp: `TestProviderMetadataCorpus` | -| JWK Set | Core §10.1.1, RFC 7517 | Cached one hour; an unknown `kid` refetches at most every 10 s; stale keys serve at most 24 h while the endpoint fails; never refetched on a bad signature | `TestKeySetTTL`, `TestUnknownKidIsRateLimited`, `TestKeySetStaleOnFailure`, `TestNoRefetchOnBadSignature`, stack: `TestDocumentedRotationIsInvisibleToTheClient`, `TestKeyWithdrawalWindow` | +| JWK Set | Core §10.1.1, RFC 7517 | Cached five minutes; an unknown `kid` refetches at most every 10 s; stale keys serve at most 24 h while the endpoint fails; never refetched on a bad signature | `TestKeySetTTL`, `TestUnknownKidIsRateLimited`, `TestKeySetStaleOnFailure`, `TestNoRefetchOnBadSignature`, stack: `TestDocumentedRotationIsInvisibleToTheClient`, `TestKeyWithdrawalWindow` | | Outbound requests | Discovery §4.1, §7.2 | Through `outbound`: https, public destinations, bounded time and size; no redirects for token and UserInfo; discovery and JWK Set redirects only with `MaxRedirects`, and never to another origin than the one requested | `TestDiscoveryHTTP`, `TestUserInfoTransport`, `TestDiscoveryRedirectStaysOnTheIssuerOrigin`, `TestKeySetRedirectStaysOnTheJWKSOrigin` | | Transaction stores | — | `storage/memory`, `store/sql`, `store/redis`; atomic single consumption; integrity keyed by `Config.TransactionKeys` | `TestMemoryStoreConformance`, `TestStoreWriterCannotForgeIntegrity`, `TestTransactionKeyRotation`, stack: `TestConcurrentCallbacksAcrossReplicas`, `TestRealBackendCallbackRace` | | Failures | RFC 9700 §4.5.3.2 | Fail closed: no identity, no token of a failed login | stack: `TestProviderStorageFailuresReachTheClient`, `TestTokenResponseLostAfterCommit`, `TestJWKSOutage`, `TestDiscoveryOutage` | @@ -199,17 +199,17 @@ a transaction altered in it fails its login (`ErrTransactionIntegrity`). ### Provider keys and rotation -The client caches the provider's JWK Set for one hour, refetches it at most +The client caches the provider's JWK Set for five minutes, refetches it at most every 10 seconds when an ID Token names an unknown `kid`, and keeps using the cached keys for at most 24 hours while the JWK Set endpoint fails. What it means for the provider's operators: | Event | Effect on this client | | --------------------------------------- | -------------------------------------------------------------------------------------------------------------- | -| A key is published before it signs | Learned at the next refresh (at most one hour), or at the first ID Token naming it — rotation is invisible | +| A key is published before it signs | Learned at the next refresh (at most five minutes), or at the first ID Token naming it — rotation is invisible | | A key signs right after being published | Logins within 10 s of the previous refetch fail closed (`ErrUnknownSigningKey`), the next ones succeed | -| A key is withdrawn (leaked) | ID Tokens it signs keep verifying until the cached set is an hour old; rebuild the `Client` to drop it at once | -| The JWK Set endpoint is down | Cached keys keep verifying for up to 24 hours; a key the cache lacks cannot be learned | +| A key is withdrawn (leaked) | ID Tokens it signs keep verifying until the cached set is five minutes old; rebuild the `Client` to drop it at once | +| The JWK Set endpoint is down | Cached keys keep verifying for up to 24 hours — a withdrawn key too, so an attacker holding it and able to make the fetches fail keeps it trusted that long; a key the cache lacks cannot be learned | `stack: TestDocumentedRotationIsInvisibleToTheClient`, `TestAbruptRotationWithinTheRefreshInterval` and `TestKeyWithdrawalWindow` diff --git a/docs/oidc-provider.md b/docs/oidc-provider.md index 4bf0d7a6..aebdd168 100644 --- a/docs/oidc-provider.md +++ b/docs/oidc-provider.md @@ -319,15 +319,15 @@ Set publishes. Rotate without breaking a relying party: (`Config.IDTokenTTL`, one hour by default) and no `id_token_hint` needs it. How long step 1 waits depends on the relying parties' caches. The JWK Set -answers with `Cache-Control: max-age=300`; `oidc/client` keeps a set for an -hour and refetches it at most every 10 seconds on an unknown `kid` +answers with `Cache-Control: max-age=300`; `oidc/client` keeps a set for five +minutes and refetches it at most every 10 seconds on an unknown `kid` ([oidc-client.md](oidc-client.md#provider-keys-and-rotation)); go-oidc -refetches on any unknown `kid`. Wait at least an hour — more if a relying -party you know caches longer — between steps 1 and 2: a key that signs +refetches on any unknown `kid`. Wait at least five minutes — more if a +relying party you know caches longer — between steps 1 and 2: a key that signs before a relying party learned it fails that relying party's logins until it refetches (stack: `TestAbruptRotationWithinTheRefreshInterval`). Withdrawing a leaked key is not instantaneous either: `oidc/client` keeps -accepting it until its cached set is an hour old (stack: +accepting it until its cached set is five minutes old (stack: `TestKeyWithdrawalWindow`); ask the relying parties to rebuild their clients. diff --git a/docs/security-considerations.md b/docs/security-considerations.md index b22d6ac7..473d7caa 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -2015,9 +2015,11 @@ span catalog. - [ ] Start logins with a same-origin POST and rate limit them; exempt only the `form_post` callback routes from CSRF middleware. - [ ] Rotate an OpenID Provider's keys on its relying parties' timetable: - publish the next key at least an hour before it signs; after a leak, - withdraw the key and ask relying parties to rebuild their clients - (`oidc/client` keeps a withdrawn key for up to an hour). + publish the next key at least five minutes before it signs (more for + relying parties caching longer); after a leak, withdraw the key and + ask relying parties to rebuild their clients (`oidc/client` keeps a + withdrawn key for up to five minutes, up to 24 hours while its JWK + Set fetches fail). - [ ] Prefer providers that send the RFC 9207 `iss` parameter; never register a connection's redirect URI at another provider. - [ ] Serialize the refreshes of one grant across instances: providers diff --git a/internal/interop/oidcstack/rotation_test.go b/internal/interop/oidcstack/rotation_test.go index 308241fd..e9b5830b 100644 --- a/internal/interop/oidcstack/rotation_test.go +++ b/internal/interop/oidcstack/rotation_test.go @@ -36,7 +36,7 @@ func rotatedRing(t *testing.T, signing jwtsec.PrivateKey, published ...jwtsec.Pr // TestDocumentedRotationIsInvisibleToTheClient follows the rotation the // provider documents (KeyRing, OpenID Connect Core §10.1.1) with our // client: (1) publish the next key and wait for relying parties to refresh -// their cached sets — an hour for our client (its key-set TTL); (2) sign +// their cached sets — five minutes for our client (its key-set TTL); (2) sign // with it, the former key still published; (3) drop the former key once // what it signed has expired. Every login succeeds, and the client never // needs an unknown-kid refresh. @@ -53,7 +53,7 @@ func TestDocumentedRotationIsInvisibleToTheClient(t *testing.T) { // (1) The next key is published. o.keys.set(o.base, rotatedRing(t, oldRS256(), newRS256())) - o.clock.advance(time.Hour + time.Minute) + o.clock.advance(5*time.Minute + time.Second) _, err = o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err) @@ -104,9 +104,9 @@ func TestAbruptRotationWithinTheRefreshInterval(t *testing.T) { // TestKeyWithdrawalWindow measures how long our client keeps accepting a // key the provider withdrew — a leaked key, say: ID Tokens forged with it -// verify until the client's cached key set is an hour old, and fail after -// (OpenID Connect Core §10.1.1). Building a new Client empties the cache at -// once. +// verify until the client's cached key set is five minutes old, and fail +// after (OpenID Connect Core §10.1.1). Building a new Client empties the +// cache at once. func TestKeyWithdrawalWindow(t *testing.T) { t.Parallel() @@ -123,7 +123,7 @@ func TestKeyWithdrawalWindow(t *testing.T) { resp["id_token"] = resign(t, resp["id_token"].(string), "RS256", rootKey(), func(h map[string]any) { h["kid"] = "root-rs256" }, nil) //nolint:forcetypeassert // test }) - o.clock.advance(30 * time.Minute) + o.clock.advance(4 * time.Minute) _, err = o.login(t, c, oidcclient.AuthOptions{}) require.NoError(t, err, "within the cache lifetime the withdrawn key still verifies") @@ -131,8 +131,8 @@ func TestKeyWithdrawalWindow(t *testing.T) { _, err = o.login(t, o.rp(t, o.basic()), oidcclient.AuthOptions{}) require.ErrorIs(t, err, oidcclient.ErrUnknownSigningKey, "a new Client fetches the current set") - o.clock.advance(31 * time.Minute) + o.clock.advance(time.Minute) _, err = o.login(t, c, oidcclient.AuthOptions{}) - require.ErrorIs(t, err, oidcclient.ErrUnknownSigningKey, "after an hour the cached set is refreshed") + require.ErrorIs(t, err, oidcclient.ErrUnknownSigningKey, "after five minutes the cached set is refreshed") } diff --git a/oidc/client/keyset.go b/oidc/client/keyset.go index 79dca73d..e1728b2e 100644 --- a/oidc/client/keyset.go +++ b/oidc/client/keyset.go @@ -16,14 +16,18 @@ import ( // JWK Set cache policy. const ( // keySetTTL is the age after which the next use fetches the set again: - // it bounds how long a key the provider withdrew stays accepted. - keySetTTL = time.Hour + // it bounds how long a key the provider withdrew stays accepted while + // the JWK Set endpoint answers — the DefaultCacheTTL of the jwt + // module's remote key sets. + keySetTTL = 5 * time.Minute // keySetMinRefresh is the minimum time between two fetch attempts: a // stream of unknown kids — a hostile tenant-configured provider — // cannot make the Client fetch more often. keySetMinRefresh = 10 * time.Second // keySetMaxStale is the age after which a cached set is no longer used - // while fetches fail: verification then fails closed. + // while fetches fail: verification then fails closed. Until then, an + // attacker holding a key the provider withdrew and able to make the + // fetches fail keeps it trusted; rebuilding the Client drops it. keySetMaxStale = 24 * time.Hour ) diff --git a/oidc/client/keyset_test.go b/oidc/client/keyset_test.go index e6132e0b..fae18d55 100644 --- a/oidc/client/keyset_test.go +++ b/oidc/client/keyset_test.go @@ -183,15 +183,15 @@ func TestKeySetTTL(t *testing.T) { op.set(func(op *fakeOP) { op.jwks = []jose.JSONWebKey{keyRSAB("rsa-2").jwk()} }) - op.clock.Advance(59 * time.Minute) + op.clock.Advance(5*time.Minute - time.Second) _, err = op.login(t, c) require.NoError(t, err, "still cached") - op.clock.Advance(time.Minute) + op.clock.Advance(time.Second) _, err = op.login(t, c) - require.ErrorIs(t, err, oidcclient.ErrUnknownSigningKey, "refreshed after an hour: rsa-1 is gone") + require.ErrorIs(t, err, oidcclient.ErrUnknownSigningKey, "refreshed after five minutes: rsa-1 is gone") } // TestKeySetStaleOnFailure checks that a failing JWK Set endpoint does not From 7b22050fa3225e574f0241fc14c51683ce98a230 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 11:15:14 +0200 Subject: [PATCH 08/10] fix(oidc/client): strict PSS salt length and Ed25519 key checks 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). --- CHANGELOG.md | 12 +++ MIGRATION.md | 16 ++++ docs/oidc-client.md | 2 +- docs/security-considerations.md | 4 +- oidc/client/ed25519key.go | 75 +++++++++++++++ oidc/client/idtoken.go | 9 +- oidc/client/idtoken_keys_test.go | 154 ++++++++++++++++++++++++++++++ oidc/client/jwk.go | 17 ++++ oidc/client/keys_internal_test.go | 114 ++++++++++++++++++++++ oidc/client/pss.go | 48 ++++++++++ 10 files changed, 445 insertions(+), 6 deletions(-) create mode 100644 oidc/client/ed25519key.go create mode 100644 oidc/client/idtoken_keys_test.go create mode 100644 oidc/client/keys_internal_test.go create mode 100644 oidc/client/pss.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 82e89792..c7cfae69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1077,6 +1077,12 @@ legacy packages (`authentication/`, `authorization/`, the in-tree provider withdraws stops verifying ID Tokens within five minutes while the JWK Set endpoint answers; the 24-hour staleness limit while fetches fail is unchanged. +- `oidc/client` verifies PS256/384/512 ID Tokens only with a salt as long + as the hash output (RFC 7518 §3.5), and ignores JWK Set Ed25519 keys that + are not canonically encoded (RFC 8032 §5.1.3) or of small order (RFC 8725 + §3.4) — the rules of the `jwt` module. A provider signing PS* tokens with + another salt length (the maximum, say) now fails with + `ErrIDTokenSignature` (see MIGRATION.md). - A typed-nil dependency — an interface holding a nil pointer, `bearer.VerifierFunc(nil)` — now panics at construction in @@ -2280,3 +2286,9 @@ legacy packages (`authentication/`, `authorization/`, the in-tree the callback with its own code — signing the victim in as the attacker (login CSRF, RFC 9700 §4.7; rfc6265bis §4.1.3.2). `New` now refuses it unless the redirect URI is http on a loopback host. +- `oidc/client` verified ID Tokens through go-jose directly, which accepts + an RSASSA-PSS signature of any salt length (RFC 7518 §3.5 requires the + hash size) and checks an Ed25519 JWK only against a list of weak + encodings, so a non-canonical encoding (y ≥ p, whose decoding RFC 8032 + §5.1.3 makes fail) was used as a key. PS* signatures are now verified with + a hash-size salt and such keys are ignored, as in the `jwt` module. diff --git a/MIGRATION.md b/MIGRATION.md index e17d577e..9f9433bf 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3179,3 +3179,19 @@ To migrate: 1. Nothing in the application. 2. A provider with a strict rate limit on its `jwks_uri` sees up to twelve requests an hour per `Client` instead of one. + +### `oidc/client`: PS* salts and Ed25519 keys are checked strictly + +| ID Token or key | Before | After | +| --- | --- | --- | +| PS256/384/512 signature with a salt as long as the hash (RFC 7518 §3.5) | verified | unchanged | +| PS* signature with another salt length (the maximum, as `crypto/rsa` signs with `PSSSaltLengthAuto`, some older OpenSSL defaults) | verified | `ErrIDTokenSignature` | +| JWK Set Ed25519 key encoded non-canonically (y ≥ p) or of small order | used (small-order ones already refused by go-jose) | ignored: an ID Token naming it fails with `ErrUnknownSigningKey` | + +To migrate: + +1. Nothing for providers signing with RS*, ES* or EdDSA, or with PS* and a + hash-size salt (key management services do). +2. A provider whose PS* ID Tokens now fail with `ErrIDTokenSignature` must + sign with a salt as long as the hash (RFC 7518 §3.5), or the client use + another algorithm it offers (`Config.Algorithms`). diff --git a/docs/oidc-client.md b/docs/oidc-client.md index a3afcf35..d43b9901 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -62,7 +62,7 @@ Test names refer to `oidc/client` unless prefixed with `stack:` | Client authentication | Core §9, RFC 6749 §2.3.1 | `client_secret_basic` (form-urlencoded first), `client_secret_post`, `none`; `private_key_jwt`, `client_secret_jwt`, mTLS not supported | `TestTokenClientAuthentication`, stack: `TestCodeFlowAgainstOurProvider` | | Token response | RFC 6749 §5.1, Core §3.1.3.3 | Parsed strictly; error bodies of 400 and 401 answers become `TokenError`; redirects never followed | `TestTokenResponse`, `TestTokenRedirectNotFollowed`, stack: `TestTokenResponseSubstitution` | | ID Token validation | Core §3.1.3.7, RFC 8725 | Signature, `iss`, `sub`, `aud` (further audiences only when trusted), `azp`, `exp`, `iat`, `nbf`, `nonce`, `auth_time`, `acr`, `at_hash` when present, `typ` absent or `JWT`; before go-jose parses it, the JOSE header is strict JSON of at most 4 KiB without `jwk`, `x5c`, `jku` or `x5u` (Core §2) | `TestIDTokenKeyBearingHeaders`, `TestIDTokenHeaderStrict`, `TestCheckRawHeaderRefusesBeforeParsing`, `TestIDTokenIssuerAndSubject`, `TestIDTokenAudience`, `TestIDTokenTimes`, `TestIDTokenAccessTokenHash`, `TestIDTokenHeader`, stack: `TestResignedIDTokensAreRefused` | -| Signing algorithms | Core §15.1, §3.1.3.7 step 7, RFC 8725 §3.1 | RS256 by default; RS384/512, PS256/384/512, ES256/384/512, EdDSA (Ed25519) once `Config.Algorithms` names the client's registered algorithm; HMAC and `none` refused | `TestDefaultIDTokenAlgorithmIsRS256`, `TestIDTokenAlgorithms`, stack: `TestCodeFlowSigningAlgorithms` | +| Signing algorithms | Core §15.1, §3.1.3.7 step 7, RFC 8725 §3.1 | RS256 by default; RS384/512, PS256/384/512, ES256/384/512, EdDSA (Ed25519) once `Config.Algorithms` names the client's registered algorithm; HMAC and `none` refused; PS* with a hash-size salt (RFC 7518 §3.5); Ed25519 keys canonical and not of small order | `TestDefaultIDTokenAlgorithmIsRS256`, `TestIDTokenAlgorithms`, `TestIDTokenPSSSaltLength`, `TestIDTokenEd25519KeyValidation`, stack: `TestCodeFlowSigningAlgorithms` | | Token confusion | RFC 9068 §5, RFC 8725 §3.11 | An `at+jwt` access token is never accepted as an ID Token | `TestAccessTokenNotAcceptedAsIDToken`, stack: `TestIDTokenUsedAsAnotherToken` | | Encrypted ID Tokens | Core §10.2 | Not supported, refused | `TestIDTokenSerialization` | | UserInfo | Core §5.3, RFC 6750 §2.1 | Bearer token in the `Authorization` header, to the connection's endpoint only; `sub` must equal the ID Token's | `TestUserInfo`, `TestUserInfoRefusesForeignIdentity`, stack: `TestUserInfoSubstitution` | diff --git a/docs/security-considerations.md b/docs/security-considerations.md index 473d7caa..b8c75953 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1437,7 +1437,9 @@ are attacker-influenced. - **ID Token** — validated per OpenID Connect Core §3.1.3.7 before anything is returned: RS256 — or the algorithm registered for the client, named in `Config.Algorithms` (OpenID Connect Core §3.1.3.7 step 7) — asymmetric - algorithms only (HMAC and `none` refused), `typ` + algorithms only (HMAC and `none` refused; PS256/384/512 with a salt as + long as the hash, RFC 7518 §3.5; Ed25519 keys canonical and not of small + order, RFC 8032 §5.1.3, RFC 8725 §3.4), `typ` absent or `JWT` (an RFC 9068 `at+jwt` access token is refused), keys from the configured issuer's `jwks_uri` only (`jku`, `x5u`, `jwk` and `x5c` are refused, see below) through a bounded cache that refetches once, rate diff --git a/oidc/client/ed25519key.go b/oidc/client/ed25519key.go new file mode 100644 index 00000000..830513ec --- /dev/null +++ b/oidc/client/ed25519key.go @@ -0,0 +1,75 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oidcclient + +import ( + "bytes" + "crypto/ed25519" +) + +// smallOrderY are the y-coordinates, little-endian, of the Ed25519 points +// of order dividing 8 (0, 1, p−1 and the two of order 8): with the sign bit +// cleared, every encoding of a small-order point is one of them, or a +// non-canonical encoding of 0 or 1 (p, p+1). +var smallOrderY = [][]byte{ + make([]byte, ed25519.PublicKeySize), + append([]byte{0x01}, make([]byte, ed25519.PublicKeySize-1)...), + { + 0xec, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f, + }, + { + 0x26, 0xe8, 0x95, 0x8f, 0xc2, 0xb2, 0x27, 0xb0, 0x45, 0xc3, 0xf4, 0x89, 0xf2, 0xef, 0x98, 0xf0, + 0xd5, 0xdf, 0xac, 0x05, 0xd3, 0xc6, 0x33, 0x39, 0xb1, 0x38, 0x02, 0x88, 0x6d, 0x53, 0xfc, 0x05, + }, + { + 0xc7, 0x17, 0x6a, 0x70, 0x3d, 0x4d, 0xd8, 0x4f, 0xba, 0x3c, 0x0b, 0x76, 0x0d, 0x10, 0x67, 0x0f, + 0x2a, 0x20, 0x53, 0xfa, 0x2c, 0x39, 0xcc, 0xc6, 0x4e, 0xc7, 0xfd, 0x77, 0x92, 0xac, 0x03, 0x7a, + }, +} + +// validEd25519PublicKey reports whether key can verify Ed25519 signatures: +// 32 bytes, a canonical encoding — y, the key with its sign bit (bit 255) +// cleared, below p = 2^255 − 19 (RFC 8032 §5.1.3 step 1: decoding fails +// otherwise, and the signature is invalid, §5.1.7 step 1), which +// crypto/ed25519 does not check — and not a point of small order, under +// which forged signatures verify (RFC 8725 §3.4). A key that is not on the +// curve verifies nothing: crypto/ed25519 fails its decoding. +func validEd25519PublicKey(key ed25519.PublicKey) bool { + if len(key) != ed25519.PublicKeySize { + return false + } + + y := bytes.Clone(key) + y[ed25519.PublicKeySize-1] &= 0x7f + + if !canonicalY(y) { + return false + } + + for _, small := range smallOrderY { + if bytes.Equal(y, small) { + return false + } + } + + return true +} + +// canonicalY reports whether y (32 bytes, little-endian, bit 255 cleared) +// is below p = 2^255 − 19, whose encoding is ed ff … ff 7f. +func canonicalY(y []byte) bool { + if y[ed25519.PublicKeySize-1] != 0x7f { + return true + } + + for _, b := range y[1 : ed25519.PublicKeySize-1] { + if b != 0xff { + return true + } + } + + return y[0] < 0xed +} diff --git a/oidc/client/idtoken.go b/oidc/client/idtoken.go index 2c6b0ec2..7f0da911 100644 --- a/oidc/client/idtoken.go +++ b/oidc/client/idtoken.go @@ -231,7 +231,7 @@ func (c *Client) verifySignature(ctx context.Context, jws *jose.JSONWebSignature return nil, err } - payload, outcome := verifyWith(jws, candidates(keys, kid, alg, c.algs)) + payload, outcome := verifyWith(jws, alg, candidates(keys, kid, alg, c.algs)) switch { case outcome == verified: @@ -246,7 +246,7 @@ func (c *Client) verifySignature(ctx context.Context, jws *jose.JSONWebSignature return nil, err } - payload, outcome = verifyWith(jws, candidates(c.keys.snapshot(), kid, alg, c.algs)) + payload, outcome = verifyWith(jws, alg, candidates(c.keys.snapshot(), kid, alg, c.algs)) switch outcome { case verified: @@ -259,7 +259,8 @@ func (c *Client) verifySignature(ctx context.Context, jws *jose.JSONWebSignature return nil, ErrUnknownSigningKey } -func verifyWith(jws *jose.JSONWebSignature, keys []verificationKey) ([]byte, verification) { +// verifyWith verifies jws, signed with alg, with the single key of keys. +func verifyWith(jws *jose.JSONWebSignature, alg Algorithm, keys []verificationKey) ([]byte, verification) { switch len(keys) { case 0: return nil, noKey @@ -268,7 +269,7 @@ func verifyWith(jws *jose.JSONWebSignature, keys []verificationKey) ([]byte, ver return nil, ambiguousKey } - payload, err := jws.Verify(keys[0].key) + payload, err := jws.Verify(keys[0].verifier(alg)) if err != nil { return nil, badSignature } diff --git a/oidc/client/idtoken_keys_test.go b/oidc/client/idtoken_keys_test.go new file mode 100644 index 00000000..ac9784e7 --- /dev/null +++ b/oidc/client/idtoken_keys_test.go @@ -0,0 +1,154 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oidcclient_test + +import ( + "crypto" + "crypto/rand" + "crypto/rsa" + "encoding/base64" + "encoding/hex" + "encoding/json" + "strconv" + "testing" + + jose "github.com/go-jose/go-jose/v4" + oidcclient "github.com/hyperscale-stack/security/oidc/client" + "github.com/stretchr/testify/require" +) + +// pssSigned makes the OP sign its ID Tokens with RSASSA-PSS (alg, hash) +// and a salt of saltLength bytes (rsa.PSSSaltLengthAuto: the maximum — Go +// cannot sign with an empty salt), its key published for alg. +func pssSigned(alg jose.SignatureAlgorithm, hash crypto.Hash, saltLength int) func(op *fakeOP) { + return func(op *fakeOP) { + signWith(keyRSA(alg, "k"))(op) + + op.idTokenHook = func(g *grant) string { + header, err := json.Marshal(map[string]string{"alg": string(alg), "kid": "k", "typ": "JWT"}) + require.NoError(op.t, err) + + payload, err := json.Marshal(op.idClaims(g)) + require.NoError(op.t, err) + + input := base64.RawURLEncoding.EncodeToString(header) + "." + base64.RawURLEncoding.EncodeToString(payload) + + h := hash.New() + h.Write([]byte(input)) + + priv, ok := op.signer.priv.(*rsa.PrivateKey) + require.True(op.t, ok) + + sig, err := rsa.SignPSS(rand.Reader, priv, hash, h.Sum(nil), &rsa.PSSOptions{SaltLength: saltLength, Hash: hash}) + require.NoError(op.t, err) + + return input + "." + base64.RawURLEncoding.EncodeToString(sig) + } + } +} + +// TestIDTokenPSSSaltLength checks that a PS256/384/512 signature verifies +// only with a salt as long as the hash output — RFC 7518 §3.5: "The size of +// the salt value is the same size as the hash function output" — not with +// the other lengths RSASSA-PSS (RFC 8017 §9.1) admits. +func TestIDTokenPSSSaltLength(t *testing.T) { + t.Parallel() + + var cases []idCase + + for _, ps := range []struct { + alg jose.SignatureAlgorithm + hash crypto.Hash + }{{jose.PS256, crypto.SHA256}, {jose.PS384, crypto.SHA384}, {jose.PS512, crypto.SHA512}} { + size := ps.hash.Size() + + for _, salt := range []int{1, size - 1, size, size + 1, rsa.PSSSaltLengthAuto} { + name := string(ps.alg) + " salt " + strconv.Itoa(salt) + if salt == rsa.PSSSaltLengthAuto { + name = string(ps.alg) + " maximum salt" + } + + tc := idCase{name: name, setup: pssSigned(ps.alg, ps.hash, salt), config: onlyAlgorithm(ps.alg)} + if salt != size { + tc.err = oidcclient.ErrIDTokenSignature + } + + cases = append(cases, tc) + } + } + + runIDCases(t, cases) +} + +// mustHex decodes a hex constant. +func mustHex(s string) []byte { + b, err := hex.DecodeString(s) + if err != nil { + panic(err) + } + + return b +} + +// TestIDTokenEd25519KeyValidation checks that a JWK Set key whose Ed25519 +// encoding is not canonical — y not below p, RFC 8032 §5.1.3 step 1, which +// makes its decoding fail (§5.1.7 step 1) — or that is a point of small +// order (RFC 8725 §3.4: validate cryptographic inputs) is ignored, as an +// unusable key (RFC 7517 §5): an ID Token naming it fails with +// ErrUnknownSigningKey, never reaching a signature check. A small-order key +// verifies forged signatures: S = 0 and R = the identity point verify any +// message under the identity key. +func TestIDTokenEd25519KeyValidation(t *testing.T) { + t.Parallel() + + identityForgery := func(op *fakeOP, g *grant) string { + header, err := json.Marshal(map[string]string{"alg": "EdDSA", "kid": "ed", "typ": "JWT"}) + require.NoError(op.t, err) + + payload, err := json.Marshal(op.idClaims(g)) + require.NoError(op.t, err) + + sig := append(mustHex("0100000000000000000000000000000000000000000000000000000000000000"), make([]byte, 32)...) + + return base64.RawURLEncoding.EncodeToString(header) + "." + base64.RawURLEncoding.EncodeToString(payload) + + "." + base64.RawURLEncoding.EncodeToString(sig) + } + + // publish serves a JWK Set of the one Ed25519 key, written by hand: + // go-jose refuses to encode some of these keys. + publish := func(key []byte) func(op *fakeOP) { + return func(op *fakeOP) { + signWith(keyEd("ed"))(op) + + raw, err := json.Marshal(map[string]any{"keys": []map[string]string{{ + "kty": "OKP", "crv": "Ed25519", "x": base64.RawURLEncoding.EncodeToString(key), + "kid": "ed", "alg": "EdDSA", "use": "sig", + }}}) + require.NoError(op.t, err) + + op.jwksRaw = raw + op.idTokenHook = func(g *grant) string { return identityForgery(op, g) } + } + } + + runIDCases(t, []idCase{ + { + name: "identity key: a forged signature", err: oidcclient.ErrUnknownSigningKey, config: onlyAlgorithm(jose.EdDSA), + setup: publish(mustHex("0100000000000000000000000000000000000000000000000000000000000000")), + }, + { + name: "non-canonical encoding p+3", err: oidcclient.ErrUnknownSigningKey, config: onlyAlgorithm(jose.EdDSA), + setup: publish(mustHex("f0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f")), + }, + { + name: "non-canonical encoding p+3, sign bit set", err: oidcclient.ErrUnknownSigningKey, config: onlyAlgorithm(jose.EdDSA), + setup: publish(mustHex("f0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")), + }, + { + name: "a valid key verifies its own signatures", config: onlyAlgorithm(jose.EdDSA), + setup: signWith(keyEd("ed")), + }, + }) +} diff --git a/oidc/client/jwk.go b/oidc/client/jwk.go index 7a108446..9c8b6765 100644 --- a/oidc/client/jwk.go +++ b/oidc/client/jwk.go @@ -146,6 +146,10 @@ func parseVerificationKey(raw []byte, members map[string]josejson.RawMessage) (v k.kind, k.implied = kindEC, alg case ed25519.PublicKey: + if !validEd25519PublicKey(pub) { + return verificationKey{}, false + } + k.kind, k.implied = kindEd25519, EdDSA default: return verificationKey{}, false @@ -223,6 +227,19 @@ func implicitRSAAlgorithm(effective []Algorithm) Algorithm { return only } +// verifier returns what verifies a signature made with alg under k: the +// public key, or for PS256/384/512 a pssVerifier, which requires the salt +// length RFC 7518 §3.5 prescribes. +func (k verificationKey) verifier(alg Algorithm) any { + if pub, ok := k.key.(*rsa.PublicKey); ok { + if _, pss := pssHashes[jose.SignatureAlgorithm(alg)]; pss { + return pssVerifier{pub: pub} + } + } + + return k.key +} + // candidates returns the keys that may verify a signature made with alg // and designated by kid ("" designates every key). func candidates(keys []verificationKey, kid string, alg Algorithm, effective []Algorithm) []verificationKey { diff --git a/oidc/client/keys_internal_test.go b/oidc/client/keys_internal_test.go new file mode 100644 index 00000000..58002695 --- /dev/null +++ b/oidc/client/keys_internal_test.go @@ -0,0 +1,114 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oidcclient + +import ( + "crypto/ecdsa" + "crypto/ed25519" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "encoding/hex" + "testing" + + jose "github.com/go-jose/go-jose/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func fromHex(t *testing.T, s string) []byte { + t.Helper() + + b, err := hex.DecodeString(s) + require.NoError(t, err) + + return b +} + +// TestValidEd25519PublicKey checks the Ed25519 key rule: 32 bytes, a +// canonical y (below p, RFC 8032 §5.1.3 step 1), not a point of small order +// — every one of go-jose's weak encodings included — whatever the sign bit. +func TestValidEd25519PublicKey(t *testing.T) { + t.Parallel() + + refused := map[string]string{ + "y = 0": "0000000000000000000000000000000000000000000000000000000000000000", + "y = 0, sign bit": "0000000000000000000000000000000000000000000000000000000000000080", + "identity (y = 1)": "0100000000000000000000000000000000000000000000000000000000000000", + "y = 1, sign bit": "0100000000000000000000000000000000000000000000000000000000000080", + "order 8 (a)": "26e8958fc2b227b045c3f489f2ef98f0d5dfac05d3c63339b13802886d53fc05", + "order 8 (a), sign bit": "26e8958fc2b227b045c3f489f2ef98f0d5dfac05d3c63339b13802886d53fc85", + "order 8 (b)": "c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac037a", + "order 8 (b), sign bit": "c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac03fa", + "y = p - 1": "ecffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f", + "y = p - 1, sign bit": "ecffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + "y = p (non-canonical 0)": "edffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f", + "y = p, sign bit": "edffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + "y = p + 1 (non-canonical 1)": "eeffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f", + "y = p + 1, sign bit": "eeffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + "y = p + 3 (non-canonical)": "f0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f", + "y = p + 3, sign bit": "f0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + "y = 2^255 - 1": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f", + } + + for name, key := range refused { + assert.False(t, validEd25519PublicKey(fromHex(t, key)), name) + } + + for _, n := range []int{0, 31, 33} { + assert.False(t, validEd25519PublicKey(make([]byte, n)), "length %d", n) + } + + for range 1000 { + pub, _, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + require.True(t, validEd25519PublicKey(pub), "%x", []byte(pub)) + } +} + +// TestCanonicalY checks the y < p boundary byte by byte. +func TestCanonicalY(t *testing.T) { + t.Parallel() + + cases := map[string]bool{ + "ebffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f": true, // p - 2 + "ecffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f": true, // p - 1 + "edffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f": false, // p + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7e": true, // below by its top byte + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f7f": true, // below by a middle byte + "fffeffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f": true, // below by its second byte + } + + for y, want := range cases { + assert.Equal(t, want, canonicalY(fromHex(t, y)), y) + } +} + +// TestPSSVerifier checks the verifier selection and the defensive branch of +// pssVerifier: PS* keys get the strict verifier, other algorithms their key. +func TestPSSVerifier(t *testing.T) { + t.Parallel() + + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + ecKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + rsaPub := verificationKey{kind: kindRSA, key: &rsaKey.PublicKey} + + for _, alg := range []Algorithm{PS256, PS384, PS512} { + assert.Equal(t, pssVerifier{pub: &rsaKey.PublicKey}, rsaPub.verifier(alg), alg) + } + + for _, alg := range []Algorithm{RS256, RS384, RS512} { + assert.Same(t, &rsaKey.PublicKey, rsaPub.verifier(alg), alg) + } + + assert.Same(t, &ecKey.PublicKey, verificationKey{kind: kindEC, key: &ecKey.PublicKey}.verifier(ES256)) + + err = pssVerifier{pub: &rsaKey.PublicKey}.VerifyPayload([]byte("input"), make([]byte, 256), jose.RS256) + require.ErrorIs(t, err, errNotPSS) +} diff --git a/oidc/client/pss.go b/oidc/client/pss.go new file mode 100644 index 00000000..11b1d2ca --- /dev/null +++ b/oidc/client/pss.go @@ -0,0 +1,48 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oidcclient + +import ( + "crypto" + "crypto/rsa" + "errors" + + jose "github.com/go-jose/go-jose/v4" +) + +// errNotPSS reports a pssVerifier asked to verify another algorithm than +// PS256, PS384 or PS512. +var errNotPSS = errors.New("not an RSASSA-PSS algorithm") + +// pssHashes are the hash functions of the RSASSA-PSS algorithms (RFC 7518 +// §3.5), used for the message digest and for MGF1 alike. +var pssHashes = map[jose.SignatureAlgorithm]crypto.Hash{ + jose.PS256: crypto.SHA256, + jose.PS384: crypto.SHA384, + jose.PS512: crypto.SHA512, +} + +// pssVerifier verifies PS256, PS384 and PS512 signatures with a salt as +// long as the hash output, as RFC 7518 §3.5 requires ("The size of the salt +// value is the same size as the hash function output"): go-jose verifies +// them with rsa.PSSSaltLengthAuto, any salt length. It implements +// jose.OpaqueVerifier, so go-jose still parses the JWS and hands over its +// signing input. +type pssVerifier struct { + pub *rsa.PublicKey +} + +// VerifyPayload implements jose.OpaqueVerifier. +func (v pssVerifier) VerifyPayload(payload, signature []byte, alg jose.SignatureAlgorithm) error { + hash, ok := pssHashes[alg] + if !ok { + return errNotPSS + } + + h := hash.New() + h.Write(payload) + + return rsa.VerifyPSS(v.pub, hash, h.Sum(nil), signature, &rsa.PSSOptions{SaltLength: rsa.PSSSaltLengthEqualsHash}) //nolint:wrapcheck // mapped to ErrIDTokenSignature by the caller +} From 4008a7f345578463a7dd1e28951859836e059787 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 12:38:16 +0200 Subject: [PATCH 09/10] fix(oidc/client): bind the PSS verifier to its key's algorithm 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). --- oidc/client/jwk.go | 2 +- oidc/client/keys_internal_test.go | 23 ++++++++++++++++++----- oidc/client/pss.go | 8 +++++--- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/oidc/client/jwk.go b/oidc/client/jwk.go index 9c8b6765..4daaec74 100644 --- a/oidc/client/jwk.go +++ b/oidc/client/jwk.go @@ -233,7 +233,7 @@ func implicitRSAAlgorithm(effective []Algorithm) Algorithm { func (k verificationKey) verifier(alg Algorithm) any { if pub, ok := k.key.(*rsa.PublicKey); ok { if _, pss := pssHashes[jose.SignatureAlgorithm(alg)]; pss { - return pssVerifier{pub: pub} + return pssVerifier{pub: pub, alg: jose.SignatureAlgorithm(alg)} } } diff --git a/oidc/client/keys_internal_test.go b/oidc/client/keys_internal_test.go index 58002695..4fd7d7f5 100644 --- a/oidc/client/keys_internal_test.go +++ b/oidc/client/keys_internal_test.go @@ -5,11 +5,13 @@ package oidcclient import ( + "crypto" "crypto/ecdsa" "crypto/ed25519" "crypto/elliptic" "crypto/rand" "crypto/rsa" + "crypto/sha256" "encoding/hex" "testing" @@ -86,8 +88,9 @@ func TestCanonicalY(t *testing.T) { } } -// TestPSSVerifier checks the verifier selection and the defensive branch of -// pssVerifier: PS* keys get the strict verifier, other algorithms their key. +// TestPSSVerifier checks the verifier selection and the defensive branches +// of pssVerifier: PS* keys get the strict verifier bound to the algorithm +// it was chosen for, other algorithms their key. func TestPSSVerifier(t *testing.T) { t.Parallel() @@ -100,7 +103,7 @@ func TestPSSVerifier(t *testing.T) { rsaPub := verificationKey{kind: kindRSA, key: &rsaKey.PublicKey} for _, alg := range []Algorithm{PS256, PS384, PS512} { - assert.Equal(t, pssVerifier{pub: &rsaKey.PublicKey}, rsaPub.verifier(alg), alg) + assert.Equal(t, pssVerifier{pub: &rsaKey.PublicKey, alg: jose.SignatureAlgorithm(alg)}, rsaPub.verifier(alg), alg) } for _, alg := range []Algorithm{RS256, RS384, RS512} { @@ -109,6 +112,16 @@ func TestPSSVerifier(t *testing.T) { assert.Same(t, &ecKey.PublicKey, verificationKey{kind: kindEC, key: &ecKey.PublicKey}.verifier(ES256)) - err = pssVerifier{pub: &rsaKey.PublicKey}.VerifyPayload([]byte("input"), make([]byte, 256), jose.RS256) - require.ErrorIs(t, err, errNotPSS) + err = pssVerifier{pub: &rsaKey.PublicKey, alg: jose.RS256}.VerifyPayload([]byte("input"), make([]byte, 256), jose.RS256) + require.ErrorIs(t, err, errNotPSS, "not a PSS algorithm") + + // A valid PS256 signature, offered for PS384 to a verifier bound to + // PS256 — or for PS256 to one bound to PS384 — is refused. + digest := sha256.Sum256([]byte("input")) + sig, err := rsa.SignPSS(rand.Reader, rsaKey, crypto.SHA256, digest[:], &rsa.PSSOptions{SaltLength: rsa.PSSSaltLengthEqualsHash}) + require.NoError(t, err) + + require.NoError(t, pssVerifier{pub: &rsaKey.PublicKey, alg: jose.PS256}.VerifyPayload([]byte("input"), sig, jose.PS256)) + require.ErrorIs(t, pssVerifier{pub: &rsaKey.PublicKey, alg: jose.PS256}.VerifyPayload([]byte("input"), sig, jose.PS384), errNotPSS) + require.ErrorIs(t, pssVerifier{pub: &rsaKey.PublicKey, alg: jose.PS384}.VerifyPayload([]byte("input"), sig, jose.PS256), errNotPSS) } diff --git a/oidc/client/pss.go b/oidc/client/pss.go index 11b1d2ca..35c18887 100644 --- a/oidc/client/pss.go +++ b/oidc/client/pss.go @@ -13,7 +13,7 @@ import ( ) // errNotPSS reports a pssVerifier asked to verify another algorithm than -// PS256, PS384 or PS512. +// the PS256, PS384 or PS512 it is bound to. var errNotPSS = errors.New("not an RSASSA-PSS algorithm") // pssHashes are the hash functions of the RSASSA-PSS algorithms (RFC 7518 @@ -29,15 +29,17 @@ var pssHashes = map[jose.SignatureAlgorithm]crypto.Hash{ // value is the same size as the hash function output"): go-jose verifies // them with rsa.PSSSaltLengthAuto, any salt length. It implements // jose.OpaqueVerifier, so go-jose still parses the JWS and hands over its -// signing input. +// signing input. It is bound to one algorithm, the one its key verifies +// (RFC 8725 §3.1), and refuses any other. type pssVerifier struct { pub *rsa.PublicKey + alg jose.SignatureAlgorithm } // VerifyPayload implements jose.OpaqueVerifier. func (v pssVerifier) VerifyPayload(payload, signature []byte, alg jose.SignatureAlgorithm) error { hash, ok := pssHashes[alg] - if !ok { + if !ok || alg != v.alg { return errNotPSS } From 2cf2da10739cc1eee598e86bc8a39a6c6a46d642 Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Wed, 21 Oct 2026 12:38:16 +0200 Subject: [PATCH 10/10] docs(oidc/client): state the limits of Secret and the persistence break 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. --- CHANGELOG.md | 11 ++++--- LIMITATIONS.md | 24 ++++++++++---- MIGRATION.md | 48 ++++++++++++++++++++++------ docs/observability.md | 4 ++- docs/oidc-client.md | 19 ++++++++--- docs/security-considerations.md | 4 ++- oidc/client/begin.go | 12 +++++-- oidc/client/doc.go | 7 ++-- oidc/client/identity.go | 7 ++-- oidc/client/redaction_test.go | 17 +++++++--- oidc/client/secret.go | 23 ++++++++----- oidc/client/storage/memory/memory.go | 3 +- 12 files changed, 132 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7cfae69..cd354d78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1040,10 +1040,13 @@ legacy packages (`authentication/`, `authorization/`, the in-tree API — `Config.ClientSecret`, `Config.TransactionKeys` (now `[]Secret`), `AuthRequest.Binding`, `Callback.Binding`, `Tokens.AccessToken`, `RefreshToken` and `IDToken`, `AuthOptions.IDTokenHint` — is a `Secret`: - `NewSecret(s)` builds one, `Reveal()` reads it, and it prints, logs and - encodes as `REDACTED` everywhere, `encoding/json` included: `Identity` - and `Tokens` no longer encode their tokens, which applications persist by - revealing them (see MIGRATION.md). + `NewSecret(s)` builds one, `Reveal()` reads it, and every `fmt` verb, + `slog` and `encoding/json` render it `REDACTED` (`fmt` an address when it + reaches it through an unexported field): `Identity` and `Tokens` no + longer encode their tokens, which applications persist by revealing + them. `encoding/gob` refuses any value holding a `Secret` (a gob session + holding an `Identity` fails to save), YAML writes it empty, and documents + persisted before the upgrade no longer decode (see MIGRATION.md). - `oidcclient.Config.InsecureCookies` is refused (`ErrInvalidConfig`) unless `RedirectURL` is plain http, which is accepted on a loopback host only (breaking for a configuration combining it with an https redirect diff --git a/LIMITATIONS.md b/LIMITATIONS.md index 5305eff5..4fbf4fac 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -725,18 +725,30 @@ PKCE. Not supported, and refused where a provider could select them: a pointer does not take (`%s`, `%q`…) — what a nested pointer points to. Log these types directly or through exported fields, with `%v`. `oidc/client` holds every credential in an `oidcclient.Secret`, which - stays redacted even there; what such a path still prints of its values is - not a credential: a `Callback`'s parameters (the authorization code, - useless without the PKCE verifier derived from the binding secret, and - the state), an `AuthRequest`'s URL and state, an `Identity`'s claims. + `fmt` prints as an address there; what such a path still prints of its + values is not a `Secret`: a `Callback`'s parameters (the authorization + code, useless without the PKCE verifier derived from the binding secret, + and the state), an `AuthRequest`'s URL — every request parameter in + clear: the state, the `login_hint` (personal data) and the + `id_token_hint` (an ID Token) — and an `Identity`'s claims. +- **Reflection dumpers** — a dumper that walks unexported fields and + dereferences pointers without calling methods prints every value it + reaches, an `oidcclient.Secret`'s included: go-spew with + `DisableMethods`, which testify's failure diffs use. Dump these values + that way in tests only, and mind that test output lands in CI logs. A + struct embedding an `oidcclient.Secret` is promoted its methods and + prints and encodes as `REDACTED` as a whole: name the field. - **JSON** — `slog`'s JSON handler calls `LogValue` for attribute values only: a value nested in another one (a struct field, a map) is encoded with `encoding/json`. `oidcclient` values and `session.Session` encode without their secrets — an `oidcclient.Secret` encodes `REDACTED` — but `oidcclient.Identity` encodes its claims and `UserInfo` its claims (personal data): log these as attributes of their own. Other - reflection-based encoders (`encoding/gob`, `encoding/xml`, YAML) write - every exported field; an `oidcclient.Secret` has none. + reflection-based encoders write every exported field: `encoding/gob` + refuses any value holding an `oidcclient.Secret` (a gob-encoded session + holding an `Identity` fails to save), and `encoding/xml` and YAML write + it empty. Persist the identifiers and the revealed tokens in a struct of + your own. - **Credentials and tokens in flight are not redacted yet** — the `oauth2` records (`AuthorizationCode`, `AccessToken`, `RefreshToken`, the `GrantResponse` holding them) print the raw code and tokens they carry diff --git a/MIGRATION.md b/MIGRATION.md index 9f9433bf..740282bc 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -3026,11 +3026,14 @@ struct holding an `oidcclient.Config`, `AuthRequest`, `Callback`, transaction keys, the binding secret or the tokens, and so did `slog`'s text handler, which prints such a struct with `fmt`. Every secret of the package's API is now an `oidcclient.Secret`: `NewSecret(s)` builds one, -`Reveal()` returns its value, and every other rendering — every `fmt` verb, -`slog`, `encoding/json`, and a `Secret` reached through an unexported field, -whose value sits behind a pointer fmt prints as an address — shows -`REDACTED` (`""` when empty). `encoding/gob` refuses a `Secret` and -`encoding/xml` writes it empty; a `Secret` does not decode from JSON. +`Reveal()` returns its value, every `fmt` verb, `slog` and `encoding/json` +show `REDACTED` (`""` when empty), and `fmt` prints an address for a +`Secret` it reaches through an unexported field, whose value sits behind a +pointer. `encoding/gob` refuses any value holding a `Secret`, YAML and +`encoding/xml` write it empty, and a `Secret` does not decode from JSON — +values persisted with those encoders break, see the table. A reflection +dumper that follows pointers without calling methods (testify's failure +diffs) still prints the value. | Field | Before | After | | --- | --- | --- | @@ -3041,6 +3044,10 @@ whose value sits behind a pointer fmt prints as an address — shows | `Tokens.AccessToken`, `RefreshToken`, `IDToken` | `string` | `Secret` — `Reveal()` to call an API or store them | | `AuthOptions.IDTokenHint` | `string` | `Secret` — `IDTokenHint: id.Tokens.IDToken` needs no conversion | | `json.Marshal` of an `Identity` or `Tokens` | tokens written, for persistence | tokens `"REDACTED"`; `json.Unmarshal` of such a document into an `Identity` fails | +| `json.Unmarshal` into an `Identity` or `Tokens` of a document written before the upgrade (string tokens) | decoded | fails: `cannot unmarshal string into … oidcclient.Secret` | +| `encoding/gob` of any value holding an `Identity`, `Tokens`, `Config`, `AuthRequest` or `Callback` — gob-encoded sessions (gorilla/sessions, scs) | encoded, secrets included | fails: `gob: type oidcclient.Secret has no exported fields`; gob data written before the upgrade no longer decodes either | +| YAML (`go.yaml.in/yaml/v3`) of such a value | tokens written | each `Secret` written `{}`, without error; a string does not decode into a `Secret` | +| Configuration decoded into an `oidcclient.Config` (YAML, envconfig, mapstructure, viper) | `ClientSecret`, `TransactionKeys` filled | not filled: decode into strings, then `NewSecret` | | `fmt` of a `Config`, `AuthRequest`, `Callback`, `Identity`, `Tokens` or `Client` reached through an unexported field, `%+v` of `*client` | secrets printed | secrets printed as an address | To migrate: @@ -3054,10 +3061,33 @@ To migrate: 3. Read the tokens with `Reveal()` where they are used (`Authorization: Bearer`, a refresh request) and pass `id.Tokens.IDToken` as `AuthOptions.IDTokenHint` unchanged. -4. An application persisting an `Identity` or its `Tokens` as JSON stores - the revealed tokens explicitly, in storage fit for credentials: the - encoding of `Identity` and `Tokens` now writes them `REDACTED`. -5. Compare two `Secret`s with `crypto/subtle.ConstantTimeCompare` on their +4. An application persisting an `Identity` or its `Tokens` — in a session + (gob, JSON), a database, a cache — stores the identifiers and the + revealed tokens in a struct of its own, of strings, in storage fit for + credentials: + + ```go + type storedLogin struct { + ConnectionID, Issuer, Subject string + AccessToken, RefreshToken, IDToken string + Expiry time.Time + } + + s := storedLogin{ + ConnectionID: id.ConnectionID, Issuer: id.Issuer, Subject: id.Subject, + AccessToken: id.Tokens.AccessToken.Reveal(), RefreshToken: id.Tokens.RefreshToken.Reveal(), + IDToken: id.Tokens.IDToken.Reveal(), Expiry: id.Tokens.Expiry, + } + ``` + + Read back, wrap the tokens again with `oidcclient.NewSecret` where an + `oidcclient` API takes them (`AuthOptions.IDTokenHint`). +5. Expect the sessions that hold an `Identity` or `Tokens`, written before + the upgrade, to fail to decode at deploy: drop them, and their users + sign in again. +6. Decode configuration (YAML, environment) into strings and build the + `Config` with `NewSecret`. +7. Compare two `Secret`s with `crypto/subtle.ConstantTimeCompare` on their revealed values, never with `==` (which compares their identity). ### `oidc/client`: `InsecureCookies` only with a loopback http redirect URI diff --git a/docs/observability.md b/docs/observability.md index 22571b5d..49eafcb8 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -33,7 +33,9 @@ The OpenID Connect client (`oidc/client`, its stores) and the core's `outbound` package open no spans either: every failure is returned to the caller as a sentinel error (`errors.Is`) that carries no secret. Every credential of `oidc/client` is an `oidcclient.Secret`, which prints, -logs and encodes as `REDACTED` wherever it sits, and +logs and encodes as `REDACTED` — an address when `fmt` reaches it through +an unexported field; a reflection dumper that follows pointers without +calling methods (testify's failure diffs) prints its value — and `Identity` and `UserInfo` log their identifiers only (`LogValue`). Log the error of `Complete` or `HandleCallback`. diff --git a/docs/oidc-client.md b/docs/oidc-client.md index d43b9901..39cd6e3b 100644 --- a/docs/oidc-client.md +++ b/docs/oidc-client.md @@ -172,10 +172,21 @@ distinct redirect URIs). - After a successful callback, create a fresh session (a new ID) and answer 303 to a clean URL, so the code leaves the browser history. - Every credential — client secret, transaction keys, binding secret, - tokens — is an `oidcclient.Secret`: it prints, logs and encodes as - `REDACTED` wherever it sits (`TestSecretsNeverPrint`), and `Reveal()` - returns it where it is used. An application persisting tokens reveals - them explicitly: `Identity` and `Tokens` encode them `REDACTED`. + tokens — is an `oidcclient.Secret`: every `fmt` verb, `slog` and + `encoding/json` render it `REDACTED`, `fmt` an address when it reaches + it through an unexported field (`TestSecretsNeverPrint`), and `Reveal()` + returns it where it is used. `encoding/gob` refuses any value holding + one: a gob-encoded session holding an `Identity` fails to save. An + application persisting tokens reveals them into its own struct of + strings: `Identity` and `Tokens` encode them `REDACTED`. A reflection + dumper that follows pointers without calling methods (go-spew with + `DisableMethods`, testify's failure diffs) prints a `Secret`'s value. +- `AuthRequest.URL` is the whole authorization request: the state, the + `login_hint` (personal data) and the `id_token_hint` (an ID Token) are + in it, in clear — it must carry them to the provider. `AuthRequest` + prints and logs it without its query, but its JSON hand-off writes it, + and `fmt` prints it when it reaches an `AuthRequest` through an + unexported field. - Show users a generic message on failure: `AuthorizationError` and `TokenError` carry the provider's untrusted text, and errors of `New` may name an allowed destination's address. Provider-controlled text an error diff --git a/docs/security-considerations.md b/docs/security-considerations.md index b8c75953..eec90d61 100644 --- a/docs/security-considerations.md +++ b/docs/security-considerations.md @@ -1525,7 +1525,9 @@ are attacker-influenced. `oidcclient.Secret`: every `fmt` verb, `slog` and `encoding/json` render it `REDACTED` — also when fmt reaches it by reflection through an unexported field, where it prints as an address — and `Reveal()` is the - only way to its value. `Identity` encodes its claims and `UserInfo` its + only way to its value; a reflection dumper that follows pointers without + calling methods (testify's failure diffs) prints it, and `encoding/gob` + refuses any value holding one. `Identity` encodes its claims and `UserInfo` its claims (personal data): never log them nested in another value. - **Caveats** — sending `acr_values` makes `acr` required, among the requested values; a public client (`none`) requires the provider to diff --git a/oidc/client/begin.go b/oidc/client/begin.go index 989e06b7..32609e52 100644 --- a/oidc/client/begin.go +++ b/oidc/client/begin.go @@ -37,8 +37,9 @@ type AuthOptions struct { // without control characters). LoginHint string // IDTokenHint is passed as id_token_hint: an ID Token the provider - // issued earlier, such as Identity.Tokens.IDToken. It never prints, - // logs nor encodes (see Secret). + // issued earlier, such as Identity.Tokens.IDToken. AuthOptions prints, + // logs and encodes without it (see Secret), but the request must carry + // it to the provider: AuthRequest.URL holds it in clear. IDTokenHint Secret // Prompt values are passed space-separated as prompt; "none" cannot // be combined with another value. @@ -82,7 +83,12 @@ type AuthOptions struct { // and state but without its binding secret: never log it nested in another // value under log/slog's JSON handler. type AuthRequest struct { - // URL is the authorization request: send the user agent to it. + // URL is the authorization request: send the user agent to it. It + // carries every request parameter in clear — the state, the + // login_hint (personal data), the id_token_hint (an ID Token) — which + // the provider must receive: the renderings of AuthRequest leave out + // its query, but its JSON encoding writes it, and so does fmt when it + // reaches an AuthRequest through an unexported field of another value. URL string // State is the state parameter, also carried by URL. State string diff --git a/oidc/client/doc.go b/oidc/client/doc.go index 7a644c82..a6c17fd2 100644 --- a/oidc/client/doc.go +++ b/oidc/client/doc.go @@ -78,9 +78,10 @@ // a failed login are never exposed (RFC 9700 §4.5.3.2). // - Every credential — the client secret, the transaction keys, the // binding secret, the tokens — is a [Secret]: it prints, logs and -// encodes as REDACTED with every fmt verb, log/slog and encoding/json, -// also when fmt reaches it through an unexported field of another -// value; Secret.Reveal is the only way to its value. +// encodes as REDACTED with every fmt verb, log/slog and encoding/json — +// an address when fmt reaches it through an unexported field of another +// value — and Secret.Reveal is the only way to its value (see Secret +// for the reflection dumpers it cannot cover). // - JSON — the ID Token's JOSE header included — is parsed strictly: // duplicate member names, two names that differ only by case, invalid // UTF-8 and unpaired surrogate escapes are refused at every nesting diff --git a/oidc/client/identity.go b/oidc/client/identity.go index 27f36afa..ab3d83dd 100644 --- a/oidc/client/identity.go +++ b/oidc/client/identity.go @@ -25,9 +25,10 @@ import ( // can assert the same value. With pairwise subject identifiers the same // person has a different Subject for every client. // -// Tokens holds credentials, as Secrets: fmt, log/slog and encoding/json -// print an Identity without them, wherever it sits — an application -// persisting tokens reveals them explicitly (Secret.Reveal). Its claims are +// Tokens holds credentials, as Secrets, which fmt, log/slog and +// encoding/json never print (see Secret for what they do not cover) and +// encoding/gob refuses: an application persisting tokens reveals them +// explicitly (Secret.Reveal), into its own struct of strings. Its claims are // not secrets: an Identity nested in a value that log/slog's JSON handler // logs (a struct field, a map) is written with them; log it as an // attribute of its own, which LogValue reduces to its identifiers. diff --git a/oidc/client/redaction_test.go b/oidc/client/redaction_test.go index f6a2e489..cfa3f73e 100644 --- a/oidc/client/redaction_test.go +++ b/oidc/client/redaction_test.go @@ -120,18 +120,25 @@ func TestSecretsAreRedacted(t *testing.T) { // TestAuthRequestRenderingsOmitTheState checks that an AuthRequest prints // and logs without its state — with a leaked state, a request to the // callback through the victim's browser burns the pending login — nor the -// rest of the authorization request's query (hints included): the URL is -// rendered without its query. Its JSON encoding, the hand-off of the -// request to a front end, keeps them. +// rest of the authorization request's query, the login_hint and the +// id_token_hint (an ID Token) included: the URL is rendered without its +// query. Its JSON encoding, the hand-off of the request to a front end, +// keeps them. func TestAuthRequestRenderingsOmitTheState(t *testing.T) { t.Parallel() op := newFakeOP(t) c := op.client(t) - ar, err := c.Begin(context.Background(), oidcclient.AuthOptions{LoginHint: "jane@example.com"}) + first, err := op.login(t, c) require.NoError(t, err) + hint := first.Tokens.IDToken.Reveal() + + ar, err := c.Begin(context.Background(), oidcclient.AuthOptions{LoginHint: "jane@example.com", IDTokenHint: first.Tokens.IDToken}) + require.NoError(t, err) + require.Contains(t, ar.URL, hint, "the request carries the hint to the provider") + endpoint, err := url.Parse(ar.URL) require.NoError(t, err) @@ -143,6 +150,7 @@ func TestAuthRequestRenderingsOmitTheState(t *testing.T) { assert.NotContains(t, rendered, "&state=", way) assert.NotContains(t, rendered, "?", "%s renders the URL with its query", way) assert.NotContains(t, rendered, "jane", "%s reveals the login hint", way) + assert.NotContains(t, rendered, hint, "%s reveals the ID Token hint", way) assert.NotContains(t, rendered, ar.Binding.Reveal(), way) } @@ -153,6 +161,7 @@ func TestAuthRequestRenderingsOmitTheState(t *testing.T) { b, err := json.Marshal(ar) require.NoError(t, err) assert.Contains(t, string(b), ar.State, "the JSON hand-off keeps the state") + assert.Contains(t, string(b), hint, "and the whole request, the ID Token hint included") unparsable := oidcclient.AuthRequest{URL: "https://op.example/authorize?state=secret-state\x7f\n%zz"} assert.Contains(t, unparsable.String(), `URL: "REDACTED"`) diff --git a/oidc/client/secret.go b/oidc/client/secret.go index 6e4e30eb..8423da67 100644 --- a/oidc/client/secret.go +++ b/oidc/client/secret.go @@ -12,20 +12,27 @@ import ( // Secret is a credential of the Relying Party: the client secret, a // transaction key, the binding secret of a login, a token. Reveal returns -// its value; every other rendering prints REDACTED — "" for an empty -// Secret — so that no log, error or dump carries it: +// its value. The renderings of the standard library print REDACTED — "" +// for an empty Secret: // // - fmt, with every verb, flag and width (Format), String and GoString; // - log/slog, as an attribute (LogValue) or nested in a struct that the // text handler prints with fmt; // - encoding/json (MarshalJSON), log/slog's JSON handler included. A // Secret does not decode from JSON: a REDACTED document never turns -// back into a credential; -// - a Secret reached through an unexported field of another value, which -// fmt prints by reflection without calling any method: its value sits -// behind a pointer to a string, which fmt then renders as an address, -// whatever the verb. encoding/gob refuses a Secret (it has no exported -// field) and encoding/xml writes it empty. +// back into a credential. +// +// fmt prints a Secret it reaches through an unexported field of another +// value by reflection, without calling any method: its value sits behind a +// pointer to a string, which fmt renders there as an address, whatever the +// verb. encoding/gob refuses any value holding a Secret (it has no +// exported field); encoding/xml and YAML encoders write it empty. Not +// covered: a reflection-based dumper that dereferences pointers without +// calling methods — go-spew with DisableMethods, as in testify's failure +// diffs — prints its value; dump these values that way in tests only, and +// mind that test output lands in CI logs. Name the field that holds a +// Secret: a struct embedding one is promoted its methods, and prints and +// encodes as REDACTED as a whole. // // The zero Secret is empty. A Secret is immutable and safe to copy and // share between goroutines. Compare two Secrets with crypto/subtle on their diff --git a/oidc/client/storage/memory/memory.go b/oidc/client/storage/memory/memory.go index e9de2b7c..64ad7bb3 100644 --- a/oidc/client/storage/memory/memory.go +++ b/oidc/client/storage/memory/memory.go @@ -37,7 +37,8 @@ var ErrFull = errors.New("memory: transaction store full") // DefaultCapacity is the number of live transactions a Store built without // WithCapacity keeps: 10 000 pending logins — about 16 login starts a // second over the default 10-minute TransactionTTL, 2.8 over the maximum -// of one hour — at most about 46 MB with 4 KiB AppStates. +// of one hour — about 46 MB with 4 KiB AppStates, more with long +// acr_values (up to about 66 MB). const DefaultCapacity = 10_000 // Option configures a Store.