fix(oauth2): refuse client authentication with an empty secret

CHANGELOG.md

@@ -312,3 +312,26 @@ legacy packages (`authentication/`, `authorization/`, the in-tree
   `grant.Config.OnError`. JWT access tokens validated offline stay valid
   until they expire. Previously the tokens issued from a replayed code
   stayed valid.
+- OAuth2 `client_secret_basic` no longer authenticates a client with an
+  empty password (RFC 6749 §2.3, §10.1). It handed the empty secret of
+  `Authorization: Basic base64("<client_id>:")` to the client's
+  `SecretMatcher`, and `DefaultClient.SecretMatches` matched it against an
+  empty `Secret`: anyone knowing the `client_id` of a client registered
+  without a secret authenticated as it at `/token` — obtaining tokens,
+  `client_credentials` included — and at `/revoke`, whatever the client's
+  type, and at `/introspect` when the client was confidential, reading the
+  validity and claims of the tokens they held. `client_secret_basic` now
+  answers a missing `client_id` or an empty password with
+  `401 invalid_client` (`missing client_id or client_secret`) before
+  loading the client, with the guard `client_secret_post`'s `Authenticate`
+  already has: no store lookup, one answer for every client.
+  `DefaultClient.SecretMatches` never matches an empty `Secret`, and the
+  `oauth2.SecretMatcher` contract now says implementations MUST NOT match
+  an empty secret: callers of `DefaultClient` are covered, custom matchers
+  are told to follow suit. Public clients still authenticate with `none`,
+  by design; through it, a public client can still use any grant its
+  `GrantTypeValues` allow, `client_credentials` included (RFC 6749 §4.4 is
+  not enforced yet; see [MIGRATION.md](MIGRATION.md)). This breaks public
+  clients that sent an empty Basic password — they must use `none` — and
+  clients registered as confidential or untyped that authenticated with an
+  empty Basic password. See [MIGRATION.md](MIGRATION.md).

LIMITATIONS.md

@@ -10,6 +10,11 @@ than future refactor phases.
 
 - **`private_key_jwt` client authentication (RFC 7523)** — not implemented.
   `client_secret_basic`, `client_secret_post`, and `none` are.
+- **RFC 6749 §4.4 is not enforced yet** — the `client_credentials` grant
+  "MUST only be used by confidential clients", but it is not refused to
+  public clients (authenticated with `none`), and an empty
+  `GrantTypeValues` allows every grant: restrict `GrantTypeValues` for
+  public clients.
 - **`/.well-known/jwks.json` endpoint** — not exposed. JWKS publication
   depends on a server-side public-key store; the `jwtsec` module already
   provides the building blocks (`NewStaticJWKS`).

MIGRATION.md

@@ -295,3 +295,55 @@ Unknown tokens and other clients' tokens still answer `200 OK`. Every
 underlying failure still reaches `ServerConfig.OnError` exactly once, as a
 `server_error` carrying its cause (a failed lookup is now reported too, as
 `revoke: token lookup failed`); the 503 itself is not reported again.
+
+### `oauth2`: `client_secret_basic` refuses an empty secret (#94)
+
+`client_secret_basic` decoded `Authorization: Basic base64("<client_id>:")`
+into an empty secret and handed it to the client's `SecretMatcher`, and
+`DefaultClient.SecretMatches` matched it against an empty `Secret`: anyone
+knowing the `client_id` of a client registered without a secret
+authenticated as it at `/token` and `/revoke`, and at `/introspect` when the
+client was confidential. An empty secret is no credential. RFC 6749 §2.3
+lets the authorization server accept the client authentication "meeting its
+security requirements" and forbids relying on a public client's
+authentication to identify it, and §10.1 forbids issuing client passwords
+to native or user-agent-based clients. RFC 6749 §2.3.1 and Appendix A.2
+(`client-secret = *VSCHAR`) allow an empty secret on the wire, but they do
+not oblige the server to accept one: Basic is required "for authenticating
+clients that were issued a client password", and a client registered
+without a secret was issued none.
+
+`client_secret_basic` now refuses a missing `client_id` or an empty
+password with `401 invalid_client` (`missing client_id or client_secret`)
+before loading the client, as `client_secret_post`'s `Authenticate`
+already did (a form with an empty `client_secret` is a `none` request),
+and `DefaultClient.SecretMatches` never matches an empty `Secret`:
+
+| Request                                                        | Before                                                   | After                           |
+| -------------------------------------------------------------- | -------------------------------------------------------- | ------------------------------- |
+| Basic, empty password, client registered without a secret      | authenticated (`/introspect`: confidential clients only) | `401 invalid_client`            |
+| Basic, empty password, client with a secret, or unknown client | `401`, description depends on the client                 | `401`, same description for all |
+| Basic/Post with a real secret; `none` for public clients       | authenticated                                            | unchanged                       |
+| `DefaultClient{Secret: ""}.SecretMatches("")`                  | `true`                                                   | `false`                         |
+
+To migrate:
+
+1. **Public clients that sent an empty Basic password** use `none`
+   instead: `client_id` in the form body, no `Authorization` header and no
+   `client_secret` (RFC 6749 §3.2.1). Register them with
+   `TypeValue: oauth2.ClientPublic`, add `clientauth.NewNone()` to
+   `ServerConfig.ClientAuth` (`examples/oauth2` does not wire it) and, if
+   the client sets `AuthMethodValues`, include `"none"`.
+2. **Confidential or untyped clients registered without a secret** can no
+   longer authenticate through `client_secret_basic` or
+   `client_secret_post` (and `none` refuses non-public clients). Give each
+   one a high-entropy secret, or authenticate it with a custom
+   `ClientAuthenticator` using another credential (e.g. `private_key_jwt`,
+   mTLS). Register a client as public only if it cannot keep a secret — a
+   browser or native app using `authorization_code` + PKCE — and then set
+   its `GrantTypeValues`: an empty list allows every grant, and the server
+   does not yet refuse `client_credentials` to public clients (RFC 6749
+   §4.4).
+3. **Custom `SecretMatcher`s** must return `false` when no secret is
+   registered or the presented one is empty, whatever they compare
+   against: a bcrypt hash of `""` matches `""`.

docs/security-considerations.md

@@ -123,8 +123,16 @@ configurable clock skew, and resolves keys by `kid` against a JWKS provider
   additionally refuses the `password` grant. Legacy grants are opt-in and
   refused outright under the stricter profile.
 - **Client authentication** — `client_secret_basic` / `client_secret_post`
-  compare secrets in constant time. Public clients use `none` and MUST use
-  PKCE. `/introspect` answers `ClientConfidential` callers only: a public
+  compare secrets in constant time and never accept an empty one. Basic
+  answers an empty password or a missing `client_id` with
+  `401 invalid_client` before loading the client, the same answer whether
+  or not the client exists; a form with an empty or absent `client_secret`
+  is a `none` request, which only public clients may make. Custom
+  `SecretMatcher`s MUST NOT match an empty secret; `DefaultClient` never
+  matches an empty `Secret`. A public client has no secret: it
+  authenticates with `none` (RFC 7591 §2) and MUST use PKCE. Register as
+  public a client that cannot keep its credentials confidential (RFC 6749
+  §2.1). `/introspect` answers `ClientConfidential` callers only: a public
   client (whatever the method, `none` included) or an untyped client gets
   `401 invalid_client`.
 - **Token introspection** — RFC 7662 §4 requires the authorization server
@@ -134,20 +142,20 @@ configurable clock skew, and resolves keys by `kid` against a JWKS provider
   MUST NOT rely on a public client's authentication — even with a secret —
   to identify it (RFC 6749 §2.3). Accepting one would let anyone read the
   validity and claims of a token they held — through `none`, or through
-  Basic/Post with the client's registered secret (Basic: even an empty
-  one) — so `/introspect` refuses every caller that is not a confidential
-  client. By default, any authenticated confidential client may then
-  introspect any token. RFC 7662 §4 adds that protected resources SHOULD be
-  specifically authorized, and that when a token can be used only at certain
-  resource servers the authorization server MUST determine whether it can be
-  used at the caller: that decision is `ServerConfig.IntrospectionPolicy`,
-  which sees the authenticated caller and the token's metadata before
-  anything is disclosed. Refuse refresh tokens to resource servers — they are
-  meant for the client alone, and a resource server that ignores
-  `token_type` could accept one as a bearer token. A withheld token and a
-  failed policy answer the same bytes as an unknown token
-  (`{"active":false}`), so the response discloses nothing (its latency may
-  still differ); a policy failure is reported to `OnError`.
+  Basic/Post with the client's registered secret — so `/introspect` refuses
+  every caller that is not a confidential client. By default, any
+  authenticated confidential client may then introspect any token.
+  RFC 7662 §4 adds that protected resources SHOULD be specifically
+  authorized, and that when a token can be used only at certain resource
+  servers the authorization server MUST determine whether it can be used at
+  the caller: that decision is `ServerConfig.IntrospectionPolicy`, which
+  sees the authenticated caller and the token's metadata before anything is
+  disclosed. Refuse refresh tokens to resource servers — they are meant for
+  the client alone, and a resource server that ignores `token_type` could
+  accept one as a bearer token. A withheld token and a failed policy answer
+  the same bytes as an unknown token (`{"active":false}`), so the response
+  discloses nothing (its latency may still differ); a policy failure is
+  reported to `OnError`.
 
 ### Token-family revocation (storage guarantees)
 
@@ -284,6 +292,10 @@ span catalog.
 - [ ] Keep the JWT allowlist asymmetric unless you truly need HMAC.
 - [ ] Use `Profile20BCP` or stricter; do not enable the `implicit` /
       `password` legacy grants without a documented reason.
+- [ ] Give every confidential client that authenticates with a secret a
+      high-entropy one. Register a client that cannot keep a secret as
+      public (`none` + PKCE), and list its `GrantTypeValues`: an empty
+      list allows every grant, `client_credentials` included.
 - [ ] Register the resource servers calling `/introspect` as confidential
       clients, and set `ServerConfig.IntrospectionPolicy` when several
       resource servers or tenants share the authorization server.

oauth2/client.go

@@ -53,9 +53,14 @@ type Client interface {
 // authentication methods (client_secret_basic, client_secret_post) to
 // verify the registered secret without exposing it. Implementations MUST
 // use constant-time comparison (or a hashed-secret scheme).
+//
+// An empty secret is never a credential: implementations MUST NOT match
+// it — not even against a hash of the empty string — nor anything when no
+// secret is registered.
 type SecretMatcher interface {
 	// SecretMatches returns true when secret matches the registered one.
-	// Implementations MUST use constant-time comparison.
+	// Implementations MUST use constant-time comparison, and MUST return
+	// false when secret is empty or no secret is registered.
 	SecretMatches(secret string) bool
 }
 
@@ -74,6 +79,11 @@ type DefaultClient struct {
 	// Secret is the cleartext client secret. DefaultClient stores it
 	// verbatim for dev/test convenience; production deployments wrap a
 	// hashed-secret store and implement SecretMatches themselves.
+	//
+	// Leave it empty for a client that does not authenticate with a secret
+	// (a public client using "none", or a client authenticated by another
+	// method): an empty Secret matches nothing, so client_secret_basic and
+	// client_secret_post never authenticate it.
 	Secret            string //nolint:gosec // dev/test convenience
 	TypeValue         ClientType
 	RedirectURIValues []string
@@ -101,10 +111,15 @@ func (c *DefaultClient) Scopes() []string { return c.ScopeValues }
 func (c *DefaultClient) AuthMethods() []string { return c.AuthMethodValues }
 
 // SecretMatches implements [SecretMatcher] using constant-time comparison.
-// The DefaultClient stores secrets in cleartext for development convenience;
-// production deployments SHOULD wrap a hashed-secret store and implement
-// SecretMatches themselves.
+// An empty Secret registers no secret and matches nothing, the empty string
+// included. The DefaultClient stores secrets in cleartext for development
+// convenience; production deployments SHOULD wrap a hashed-secret store and
+// implement SecretMatches themselves.
 func (c *DefaultClient) SecretMatches(secret string) bool {
+	if c.Secret == "" {
+		return false
+	}
+
 	return subtle.ConstantTimeCompare([]byte(c.Secret), []byte(secret)) == 1
 }
 

oauth2/clientauth/basic.go

@@ -14,6 +14,12 @@ import (
 // NewBasic returns a client_secret_basic authenticator. The client_id and
 // client_secret are read from the HTTP Basic Authorization header per
 // RFC 6749 §2.3.1.
+//
+// A header without a client_id or with an empty client_secret, such as
+// Basic base64("<client_id>:"), is refused with invalid_client before the
+// client is loaded, as [NewPost] refuses an empty client_secret. A public
+// client authenticates with [NewNone] instead (RFC 7591 §2: "none" is the
+// method of a public client that has no secret).
 func NewBasic() ClientAuthenticator { return basicAuth{} }
 
 type basicAuth struct{}
@@ -43,6 +49,14 @@ func (basicAuth) Authenticate(ctx context.Context, r *http.Request, store oauth2
 		return nil, oauth2.ErrInvalidClient.WithDescription("malformed Basic Authorization header")
 	}
 
+	// A missing client_id or an empty password is no credential: refuse it
+	// before the store is consulted, like client_secret_post, so the answer
+	// does not depend on the client existing (RFC 6749 §2.3 lets the server
+	// choose the client authentication it accepts).
+	if id == "" || secret == "" {
+		return nil, oauth2.ErrInvalidClient.WithDescription("missing client_id or client_secret")
+	}
+
 	client, err := store.LoadClient(ctx, id)
 	if err != nil {
 		return nil, errInvalid(err)

oauth2/clientauth/clientauth_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"strings"
+	"sync/atomic"
 	"testing"
 
 	"github.com/hyperscale-stack/security/oauth2"
@@ -34,6 +35,18 @@ func (s fakeStore) LoadClient(_ context.Context, id string) (oauth2.Client, erro
 	return s.clients[id], nil
 }
 
+// countingStore is a fakeStore that counts its LoadClient calls.
+type countingStore struct {
+	fakeStore
+	calls atomic.Int32
+}
+
+func (s *countingStore) LoadClient(ctx context.Context, id string) (oauth2.Client, error) {
+	s.calls.Add(1)
+
+	return s.fakeStore.LoadClient(ctx, id)
+}
+
 // noMatcherClient implements oauth2.Client but NOT oauth2.SecretMatcher.
 type noMatcherClient struct {
 	id      string
@@ -299,6 +312,79 @@ func TestPostAuthenticate(t *testing.T) {
 	})
 }
 
+// --- secret-based methods ------------------------------------------------
+
+// TestSecretMethodsRefuseMissingCredentials pins that client_secret_basic
+// and client_secret_post refuse a missing client_id or an empty secret
+// before the client store is consulted, with the same answer whatever the
+// client: an empty secret is no credential, even for a client registered
+// without one (#94).
+func TestSecretMethodsRefuseMissingCredentials(t *testing.T) {
+	t.Parallel()
+
+	clients := map[string]oauth2.Client{
+		"c1":        confidentialClient(),
+		"no-secret": &oauth2.DefaultClient{IDValue: "no-secret", TypeValue: oauth2.ClientConfidential},
+		"public":    &oauth2.DefaultClient{IDValue: "public", TypeValue: oauth2.ClientPublic},
+	}
+
+	methods := []struct {
+		name string
+		auth ClientAuthenticator
+		req  func(id, secret string) *http.Request
+	}{
+		{
+			name: "basic",
+			auth: NewBasic(),
+			req: func(id, secret string) *http.Request {
+				r := httptest.NewRequest(http.MethodPost, "/", nil)
+				r.Header.Set("Authorization", basicHeader(id, secret))
+
+				return r
+			},
+		},
+		{
+			name: "post",
+			auth: NewPost(),
+			req: func(id, secret string) *http.Request {
+				return postReq(url.Values{"client_id": {id}, "client_secret": {secret}})
+			},
+		},
+	}
+
+	cases := []struct {
+		name       string
+		id, secret string
+	}{
+		{"client with a secret", "c1", ""},
+		{"confidential client without a secret", "no-secret", ""},
+		{"public client without a secret", "public", ""},
+		{"unknown client", "ghost", ""},
+		{"empty client_id", "", "s3cr3t"},
+		{"empty client_id and secret", "", ""},
+	}
+
+	for _, m := range methods {
+		for _, tc := range cases {
+			t.Run(m.name+"/"+tc.name, func(t *testing.T) {
+				t.Parallel()
+
+				// A store per subtest, so that the count is this request's.
+				store := &countingStore{fakeStore: fakeStore{clients: clients}}
+
+				c, err := m.auth.Authenticate(context.Background(), m.req(tc.id, tc.secret), store)
+				assert.Nil(t, c)
+				assertInvalidClient(t, err)
+
+				var oe *oauth2.Error
+				require.ErrorAs(t, err, &oe)
+				assert.Equal(t, "missing client_id or client_secret", oe.Description)
+				assert.Zero(t, store.calls.Load(), "the client store must not be consulted")
+			})
+		}
+	}
+}
+
 // --- none ----------------------------------------------------------------
 
 func TestNoneMethodAndMatch(t *testing.T) {