feat: support multiple client secrets per OIDC client (#1679)

backend/internal/oidc/client.go

@@ -19,8 +19,28 @@ func (c Client) GetID() string {
 	return c.ID
 }
 
+// GetHashedSecret returns the hash of the client's most recently created secret that is still active
+// Fosite checks this one first and then falls back to GetRotatedHashes for the others
 func (c Client) GetHashedSecret() []byte {
-	return []byte(c.Secret)
+	active := c.Credentials.ActiveSecrets()
+	if len(active) == 0 {
+		return nil
+	}
+	return active[0].EncodedHash()
+}
+
+// GetRotatedHashes returns the hashes of every active secret except the one returned by GetHashedSecret
+func (c Client) GetRotatedHashes() [][]byte {
+	active := c.Credentials.ActiveSecrets()
+	if len(active) <= 1 {
+		return nil
+	}
+
+	hashes := make([][]byte, len(active)-1)
+	for i, secret := range active[1:] {
+		hashes[i] = secret.EncodedHash()
+	}
+	return hashes
 }
 
 func (c Client) GetRedirectURIs() []string {

backend/internal/oidc/client_secret_hasher.go

@@ -0,0 +1,49 @@
+package oidc
+
+import (
+	"context"
+	"crypto/sha256"
+	"crypto/subtle"
+	"encoding/hex"
+	"errors"
+	"strings"
+
+	"golang.org/x/crypto/bcrypt"
+
+	"github.com/pocket-id/pocket-id/backend/internal/model"
+)
+
+// errUnsupportedHashAlgorithm is returned when a stored hash carries an algorithm this build cannot verify
+var errUnsupportedHashAlgorithm = errors.New("unsupported client secret hash algorithm")
+
+// clientSecretHasher verifies client secrets against the hashes stored in the client's credentials document
+// Fosite hands it the encoded hashes produced by model.OidcClientSecret.EncodedHash, which carry the algorithm they were computed with
+type clientSecretHasher struct{}
+
+// Compare checks a presented client secret against a stored, algorithm-prefixed hash
+func (clientSecretHasher) Compare(_ context.Context, hash []byte, data []byte) error {
+	algorithm, encoded, found := strings.Cut(string(hash), ":")
+	if !found {
+		return errUnsupportedHashAlgorithm
+	}
+
+	switch model.OidcClientSecretHashAlgorithm(algorithm) {
+	case model.OidcClientSecretHashSHA256:
+		sum := sha256.Sum256(data)
+		if subtle.ConstantTimeCompare([]byte(hex.EncodeToString(sum[:])), []byte(encoded)) != 1 {
+			return errors.New("client secret does not match")
+		}
+		return nil
+	case model.OidcClientSecretHashBcrypt:
+		// Legacy hashes migrated from the single-secret column, which cannot be re-hashed because the secret's value is not recoverable
+		return bcrypt.CompareHashAndPassword([]byte(encoded), data)
+	default:
+		return errUnsupportedHashAlgorithm
+	}
+}
+
+// Hash is required by the fosite.Hasher interface but is never called, because Pocket ID hashes secrets when it stores them
+func (clientSecretHasher) Hash(_ context.Context, data []byte) ([]byte, error) {
+	sum := sha256.Sum256(data)
+	return []byte(string(model.OidcClientSecretHashSHA256) + ":" + hex.EncodeToString(sum[:])), nil
+}

backend/internal/oidc/client_secret_hasher_test.go

@@ -0,0 +1,99 @@
+package oidc
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/crypto/bcrypt"
+
+	"github.com/pocket-id/pocket-id/backend/internal/model"
+	"github.com/pocket-id/pocket-id/backend/internal/utils"
+)
+
+func TestClientSecretHasherCompare(t *testing.T) {
+	const value = "client-secret-value"
+
+	bcryptHash, err := bcrypt.GenerateFromPassword([]byte(value), bcrypt.MinCost)
+	require.NoError(t, err)
+
+	for _, test := range []struct {
+		name    string
+		hash    string
+		data    string
+		wantErr bool
+	}{
+		{
+			name: "matching SHA-256 hash",
+			hash: "sha256:" + utils.CreateSha256Hash(value),
+			data: value,
+		},
+		{
+			name:    "SHA-256 hash of another secret",
+			hash:    "sha256:" + utils.CreateSha256Hash(value),
+			data:    "some-other-value",
+			wantErr: true,
+		},
+		{
+			name: "bcrypt hash migrated from the single-secret column",
+			hash: "bcrypt:" + string(bcryptHash),
+			data: value,
+		},
+		{
+			name:    "bcrypt hash of another secret",
+			hash:    "bcrypt:" + string(bcryptHash),
+			data:    "some-other-value",
+			wantErr: true,
+		},
+		{
+			name:    "hash without an algorithm",
+			hash:    utils.CreateSha256Hash(value),
+			data:    value,
+			wantErr: true,
+		},
+		{
+			name:    "unknown algorithm",
+			hash:    "md5:" + utils.CreateSha256Hash(value),
+			data:    value,
+			wantErr: true,
+		},
+		{
+			name:    "empty hash, as returned for a client without secrets",
+			hash:    "",
+			data:    value,
+			wantErr: true,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			err := clientSecretHasher{}.Compare(t.Context(), []byte(test.hash), []byte(test.data))
+			if test.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestClientSecretHasherHash(t *testing.T) {
+	const value = "client-secret-value"
+
+	hash, err := clientSecretHasher{}.Hash(t.Context(), []byte(value))
+	require.NoError(t, err)
+	assert.Equal(t, "sha256:"+utils.CreateSha256Hash(value), string(hash))
+
+	// A hash produced by the hasher is accepted by its own comparison
+	err = clientSecretHasher{}.Compare(t.Context(), hash, []byte(value))
+	require.NoError(t, err)
+}
+
+func TestClientSecretHasherAcceptsModelEncoding(t *testing.T) {
+	const value = "client-secret-value"
+
+	secret := model.OidcClientSecret{
+		Algorithm: model.OidcClientSecretHashSHA256,
+		Hash:      utils.CreateSha256Hash(value),
+	}
+	err := clientSecretHasher{}.Compare(t.Context(), secret.EncodedHash(), []byte(value))
+	require.NoError(t, err)
+}

backend/internal/oidc/client_test.go

@@ -1,13 +1,17 @@
 package oidc
 
 import (
+	"strconv"
 	"testing"
 	"time"
 
 	"github.com/ory/fosite"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/pocket-id/pocket-id/backend/internal/model"
+	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
+	"github.com/pocket-id/pocket-id/backend/internal/utils"
 )
 
 // Interface assertions
@@ -15,8 +19,26 @@ var (
 	_ fosite.Client                         = (*Client)(nil)
 	_ fosite.ResponseModeClient             = (*Client)(nil)
 	_ fosite.ClientWithCustomTokenLifespans = (*Client)(nil)
+	_ fosite.ClientWithSecretRotation       = (*Client)(nil)
 )
 
+// testClientCredentials returns credentials with a single never-expiring secret with the given value
+func testClientCredentials(values ...string) model.OidcClientCredentials {
+	credentials := model.OidcClientCredentials{
+		Secrets: make([]model.OidcClientSecret, len(values)),
+	}
+	for i, value := range values {
+		credentials.Secrets[i] = model.OidcClientSecret{
+			ID:        "secret-" + strconv.Itoa(i),
+			Algorithm: model.OidcClientSecretHashSHA256,
+			Hash:      utils.CreateSha256Hash(value),
+			Prefix:    value[:model.OidcClientSecretPrefixLength],
+			CreatedAt: datatype.DateTime(time.Now()),
+		}
+	}
+	return credentials
+}
+
 func TestClientGetEffectiveLifespan(t *testing.T) {
 	client := Client{OidcClient: model.OidcClient{
 		AccessTokenDurationMinutes:  2 * 60,
@@ -51,3 +73,41 @@ func TestClientGetEffectiveLifespan(t *testing.T) {
 	require.Equal(t, fallback, client.GetEffectiveLifespan(fosite.GrantTypeAuthorizationCode, fosite.AccessToken, fallback))
 	require.Equal(t, fallback, client.GetEffectiveLifespan(fosite.GrantTypeAuthorizationCode, fosite.RefreshToken, fallback))
 }
+
+func TestClientSecretHashes(t *testing.T) {
+	past := datatype.DateTime(time.Now().Add(-time.Hour))
+	future := datatype.DateTime(time.Now().Add(time.Hour))
+	older := datatype.DateTime(time.Now().Add(-48 * time.Hour))
+	newer := datatype.DateTime(time.Now().Add(-time.Minute))
+
+	t.Run("client without secrets", func(t *testing.T) {
+		client := Client{}
+		assert.Nil(t, client.GetHashedSecret())
+		assert.Empty(t, client.GetRotatedHashes())
+	})
+
+	t.Run("expired secrets are never returned", func(t *testing.T) {
+		client := Client{OidcClient: model.OidcClient{Credentials: model.OidcClientCredentials{
+			Secrets: []model.OidcClientSecret{
+				{ID: "expired", Algorithm: model.OidcClientSecretHashSHA256, Hash: "expired-hash", ExpiresAt: &past},
+			},
+		}}}
+		assert.Nil(t, client.GetHashedSecret())
+		assert.Empty(t, client.GetRotatedHashes())
+	})
+
+	t.Run("the most recent active secret comes first", func(t *testing.T) {
+		client := Client{OidcClient: model.OidcClient{Credentials: model.OidcClientCredentials{
+			Secrets: []model.OidcClientSecret{
+				{ID: "older", Algorithm: model.OidcClientSecretHashSHA256, Hash: "older-hash", CreatedAt: older},
+				{ID: "expired", Algorithm: model.OidcClientSecretHashSHA256, Hash: "expired-hash", CreatedAt: newer, ExpiresAt: &past},
+				{ID: "newer", Algorithm: model.OidcClientSecretHashSHA256, Hash: "newer-hash", CreatedAt: newer, ExpiresAt: &future},
+			},
+		}}}
+
+		assert.Equal(t, "sha256:newer-hash", string(client.GetHashedSecret()))
+		rotated := client.GetRotatedHashes()
+		require.Len(t, rotated, 1)
+		assert.Equal(t, "sha256:older-hash", string(rotated[0]))
+	})
+}

backend/internal/oidc/provider.go

@@ -57,6 +57,7 @@ func newProvider(store *Store, authenticator *federatedClientAuthenticator, sign
 		GlobalSecret:                            secret,
 		JWTScopeClaimKey:                        jwt.JWTScopeFieldBoth,
 		ClientResolver:                          clientResolver,
+		ClientSecretsHasher:                     clientSecretHasher{},
 	}
 
 	keyGetter := func(context.Context) (interface{}, error) {

backend/internal/oidc/token_handler_test.go

@@ -20,10 +20,10 @@ import (
 	fositeoauth2 "github.com/ory/fosite/handler/oauth2"
 	fositejwt "github.com/ory/fosite/token/jwt"
 	"github.com/stretchr/testify/require"
-	"golang.org/x/crypto/bcrypt"
 	"gorm.io/gorm"
 
 	"github.com/pocket-id/pocket-id/backend/internal/model"
+	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
 	testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing"
 )
 
@@ -49,12 +49,10 @@ func TestTokenHandlerClientCredentialsGrant(t *testing.T) {
 	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	require.NoError(t, err)
 
-	hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost)
-	require.NoError(t, err)
 	require.NoError(t, db.Create(&model.OidcClient{
 		Base:                       model.Base{ID: clientID},
 		Name:                       "Client Credentials Client",
-		Secret:                     string(hashed),
+		Credentials:                testClientCredentials(clientPlain),
 		IsPublic:                   false,
 		AccessTokenDurationMinutes: 2 * 60,
 	}).Error)
@@ -111,13 +109,11 @@ func TestTokenHandlerClientCredentialsDropsIdentityScopes(t *testing.T) {
 	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	require.NoError(t, err)
 
-	hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost)
-	require.NoError(t, err)
 	require.NoError(t, db.Create(&model.OidcClient{
-		Base:     model.Base{ID: clientID},
-		Name:     "Client Credentials Client",
-		Secret:   string(hashed),
-		IsPublic: false,
+		Base:        model.Base{ID: clientID},
+		Name:        "Client Credentials Client",
+		Credentials: testClientCredentials(clientPlain),
+		IsPublic:    false,
 	}).Error)
 
 	provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{
@@ -166,13 +162,11 @@ func TestTokenHandlerClientCredentialsUsesClientSubjectGrants(t *testing.T) {
 	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	require.NoError(t, err)
 
-	hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost)
-	require.NoError(t, err)
 	require.NoError(t, db.Create(&model.OidcClient{
-		Base:     model.Base{ID: clientID},
-		Name:     "Client Credentials Client",
-		Secret:   string(hashed),
-		IsPublic: false,
+		Base:        model.Base{ID: clientID},
+		Name:        "Client Credentials Client",
+		Credentials: testClientCredentials(clientPlain),
+		IsPublic:    false,
 	}).Error)
 
 	apiAccess := fakeAPIAccess{allowed: map[string]map[SubjectType][]string{
@@ -239,13 +233,11 @@ func TestTokenHandlerClientCredentialsDefaultsResourceScopes(t *testing.T) {
 	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	require.NoError(t, err)
 
-	hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost)
-	require.NoError(t, err)
 	require.NoError(t, db.Create(&model.OidcClient{
-		Base:     model.Base{ID: clientID},
-		Name:     "Client Credentials Client",
-		Secret:   string(hashed),
-		IsPublic: false,
+		Base:        model.Base{ID: clientID},
+		Name:        "Client Credentials Client",
+		Credentials: testClientCredentials(clientPlain),
+		IsPublic:    false,
 	}).Error)
 
 	apiAccess := fakeAPIAccess{allowed: map[string]map[SubjectType][]string{
@@ -731,3 +723,124 @@ func TestTokenHandlerRefreshGrantPreservesAudienceAndScope(t *testing.T) {
 		require.Equal(t, "invalid_scope", body["error"])
 	})
 }
+
+// TestTokenHandlerClientSecretRotation drives the token endpoint with each of a client's secrets,
+// to check that every active secret authenticates the client while expired ones are rejected.
+func TestTokenHandlerClientSecretRotation(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+
+	const (
+		baseURL      = "https://issuer.example.com"
+		secret       = "test-secret"
+		clientID     = "rotation-client"
+		currentPlain = "current-secret-value"
+		olderPlain   = "older-secret-value"
+		expiredPlain = "expired-secret-value"
+	)
+
+	db := testutils.NewDatabaseForTest(t)
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	credentials := testClientCredentials(currentPlain, olderPlain, expiredPlain)
+	expiredAt := datatype.DateTime(time.Now().Add(-time.Hour))
+	credentials.Secrets[2].ExpiresAt = &expiredAt
+
+	require.NoError(t, db.Create(&model.OidcClient{
+		Base:        model.Base{ID: clientID},
+		Name:        "Secret Rotation Client",
+		Credentials: credentials,
+		IsPublic:    false,
+	}).Error)
+
+	provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{
+		BaseURL:      baseURL,
+		TokenBaseURL: baseURL,
+		Secret:       []byte(secret),
+	}, nil)
+	require.NoError(t, err)
+	handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil)
+
+	requestToken := func(t *testing.T, clientSecret string) map[string]any {
+		t.Helper()
+
+		form := url.Values{"grant_type": {"client_credentials"}}
+		req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode()))
+		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+		req.SetBasicAuth(clientID, clientSecret)
+
+		rec := httptest.NewRecorder()
+		c, _ := gin.CreateTestContext(rec)
+		c.Request = req
+		handler.token(c)
+
+		var body map[string]any
+		require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body))
+		return body
+	}
+
+	for _, test := range []struct {
+		name         string
+		clientSecret string
+		wantToken    bool
+	}{
+		{name: "most recently created secret", clientSecret: currentPlain, wantToken: true},
+		{name: "secret being rotated out", clientSecret: olderPlain, wantToken: true},
+		{name: "expired secret", clientSecret: expiredPlain},
+		{name: "secret that was never configured", clientSecret: "not-a-secret"},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			body := requestToken(t, test.clientSecret)
+			if test.wantToken {
+				require.NotEmpty(t, body["access_token"], "expected a token, got error: %v", body["error"])
+			} else {
+				require.Empty(t, body["access_token"])
+				require.Equal(t, "invalid_client", body["error"])
+			}
+		})
+	}
+}
+
+// TestTokenHandlerClientWithoutSecrets checks that a confidential client that has no secrets at all cannot authenticate.
+func TestTokenHandlerClientWithoutSecrets(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+
+	const (
+		baseURL  = "https://issuer.example.com"
+		secret   = "test-secret"
+		clientID = "no-secrets-client"
+	)
+
+	db := testutils.NewDatabaseForTest(t)
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	require.NoError(t, db.Create(&model.OidcClient{
+		Base:     model.Base{ID: clientID},
+		Name:     "Client Without Secrets",
+		IsPublic: false,
+	}).Error)
+
+	provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{
+		BaseURL:      baseURL,
+		TokenBaseURL: baseURL,
+		Secret:       []byte(secret),
+	}, nil)
+	require.NoError(t, err)
+	handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil)
+
+	form := url.Values{"grant_type": {"client_credentials"}}
+	req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.SetBasicAuth(clientID, "")
+
+	rec := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(rec)
+	c.Request = req
+	handler.token(c)
+
+	var body map[string]any
+	require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body))
+	require.Empty(t, body["access_token"])
+	require.Equal(t, "invalid_client", body["error"])
+}