Found during the review of #81 (PR #90). Audited revision: d8ccbb5 (master).
Problem
client_secret_basic authenticates a client whose registered secret is empty when the request presents an empty password:
oauth2/clientauth/basic.go: decodeBasic accepts Authorization: Basic base64("<client_id>:") and Authenticate never checks that the presented secret is non-empty.oauth2/client.go: DefaultClient.SecretMatches is subtle.ConstantTimeCompare([]byte(c.Secret), []byte(secret)) == 1, which returns true for two empty slices, so an unset Secret matches an empty password.oauth2/clientauth/post.go already refuses an empty client_secret (in Match and in Authenticate), so the two secret-based methods are inconsistent.
Impact: anyone who knows the client_id of a client registered without a secret (a public client registered with an empty Secret, or a misconfigured confidential client) authenticates as that client through client_secret_basic at /token and /revoke. At /introspect, #81's ClientConfidential allowlist already refuses public and untyped clients, but a confidential client with an empty registered secret still passes. An empty secret is no credential: RFC 6749 §2.3 says the authorization server MUST NOT rely on public client authentication to identify the client, and §10.1 asks for client credentials that actually authenticate the client.
Implementation
Secret-based client authentication must never succeed without a secret:
client_secret_basic refuses an empty presented secret with invalid_client, as client_secret_post already does (no client-store lookup needed for that case).DefaultClient.SecretMatches never matches when its registered Secret is empty (defense in depth for direct callers).- Document on
oauth2.SecretMatcher that implementations MUST NOT match an empty secret, and document the behavior change (CHANGELOG, MIGRATION).
Acceptance criteria
Found during the review of #81 (PR #90). Audited revision: d8ccbb5 (master).
Problem
client_secret_basicauthenticates a client whose registered secret is empty when the request presents an empty password:oauth2/clientauth/basic.go:decodeBasicacceptsAuthorization: Basic base64("<client_id>:")andAuthenticatenever checks that the presented secret is non-empty.oauth2/client.go:DefaultClient.SecretMatchesissubtle.ConstantTimeCompare([]byte(c.Secret), []byte(secret)) == 1, which returns true for two empty slices, so an unsetSecretmatches an empty password.oauth2/clientauth/post.goalready refuses an emptyclient_secret(inMatchand inAuthenticate), so the two secret-based methods are inconsistent.Impact: anyone who knows the
client_idof a client registered without a secret (a public client registered with an emptySecret, or a misconfigured confidential client) authenticates as that client throughclient_secret_basicat/tokenand/revoke. At/introspect, #81'sClientConfidentialallowlist already refuses public and untyped clients, but a confidential client with an empty registered secret still passes. An empty secret is no credential: RFC 6749 §2.3 says the authorization server MUST NOT rely on public client authentication to identify the client, and §10.1 asks for client credentials that actually authenticate the client.Implementation
Secret-based client authentication must never succeed without a secret:
client_secret_basicrefuses an empty presented secret withinvalid_client, asclient_secret_postalready does (no client-store lookup needed for that case).DefaultClient.SecretMatchesnever matches when its registeredSecretis empty (defense in depth for direct callers).oauth2.SecretMatcherthat implementations MUST NOT match an empty secret, and document the behavior change (CHANGELOG, MIGRATION).Acceptance criteria
Basic base64("id:")is refused with 401invalid_client(+WWW-Authenticate) at/token,/revokeand/introspect, whatever the registered secret.DefaultClient{Secret: ""}.SecretMatches("")is false; non-empty secrets keep constant-time comparison.nonekeep working.