Safeguard configured client reconciliation

Author: jhrozek

docs/arch/11-auth-server-storage.md

@@ -253,23 +253,29 @@ in-memory backend, which has always enforced the pinned method.
 
 ## Configured-client reconciliation
 
-Operator-declared delegate and SPIFFE clients are reconciled as one desired set at
-embedded auth-server startup. Redis rows written by this path carry a
-`configured` ownership marker. Rows without that marker are legacy data and are
-never pruned; DCR-issued rows are also excluded.
-
-Redis replicas publish their desired client IDs and logical fingerprints under a
-short-lived lease. A live lease protects its claimed IDs during rolling
-updates. Reconciliation uses cursor-based `SCAN` over the client namespace and
-rechecks candidate rows before deletion, so concurrent registration or
-reconciliation cannot be removed from a stale read. Conflicting fingerprints
-for the same client ID fail reconciliation instead of silently overwriting one
-replica's configuration.
-
-The server performs an initial reconciliation before serving requests, renews
-its lease while running, and releases it before closing storage. Memory storage
-uses the same desired-set semantics under its existing mutex and treats lease
-operations as process-local no-ops.
+Operator-declared delegate clients are reconciled as one desired set at embedded
+auth-server startup. SPIFFE associations are not part of that delegate desired
+set: their durable rows are inert, reserved placeholders, not usable OAuth
+clients. Redis rows written by either configured path carry a `configured`
+ownership marker. Rows without that marker are legacy data and are never pruned;
+DCR-issued rows are also excluded.
+
+Redis atomically validates every desired client collision and every live lease
+claim before publishing or renewing its lease and writing any configured rows.
+A live lease protects its claimed IDs during rolling updates, and stale-row
+sweeps make their final keep/delete decision atomically against the current
+lease index. `SCAN` only discovers candidates; it never decides ownership.
+The running server heartbeat renews only its existing lease and never rewrites
+client material. If a same-ID configured client changes its secret, token
+endpoint authentication material, or authorization configuration, the new
+reconcile fails closed while any old-material lease is live. Operators must
+drain/release the old replicas (or wait for the 30-second lease TTL) before
+starting a replica with the changed same-ID configuration. This smaller-scope
+coordination intentionally does not provide zero-downtime ordered generations.
+
+Memory storage applies the same desired-set pruning under its mutex but has no
+distributed lease coordination, so it may atomically replace an already-owned
+configured client. DCR-issued and legacy/unowned rows remain protected.
 
 
 ### CRD Configuration

pkg/authserver/server_impl.go

@@ -51,7 +51,6 @@ type server struct {
 	// down its per-issuer JWKS refresh worker pools; nothing else releases them.
 	trustedIssuerValidator *tokenexchange.MultiIssuerTokenValidator
 	configuredReconciler   storage.ConfiguredClientReconciler
-	configuredClients      []fosite.Client
 	configuredLeaseID      string
 	configuredCancel       context.CancelFunc
 	configuredDone         chan struct{}
@@ -178,7 +177,7 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage) (_ *server
 		return nil, fmt.Errorf("storage backend %T does not implement storage.DCRCredentialStore", baseStore)
 	}
 
-	stor, configuredReconciler, configuredLeaseID, desiredClients, err := setupConfiguredClients(ctx, cfg, stor)
+	stor, configuredReconciler, configuredLeaseID, err := setupConfiguredClients(ctx, cfg, stor)
 	if err != nil {
 		return nil, err
 	}
@@ -193,23 +192,23 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage) (_ *server
 
 	// Create OAuth2 config from authserver.Config
 	oauthParams := &oauthserver.AuthorizationServerParams{
-		Issuer:                              cfg.Issuer,
-		AccessTokenLifespan:                 cfg.AccessTokenLifespan,
-		RefreshTokenLifespan:                cfg.RefreshTokenLifespan,
-		AuthCodeLifespan:                    cfg.AuthCodeLifespan,
-		HMACSecrets:                         cfg.HMACSecrets,
-		SigningKeyID:                        signingKey.KeyID,
-		SigningKeyAlgorithm:                 signingKey.Algorithm,
-		SigningKey:                          signingKey.Key,
-		ScopesSupported:                     cfg.ScopesSupported,
-		BaselineClientScopes:                cfg.BaselineClientScopes,
-		AllowedAudiences:                    cfg.AllowedAudiences,
-		AuthorizationEndpointBaseURL:        cfg.AuthorizationEndpointBaseURL,
-		CIMDEnabled:                         cfg.CIMDEnabled,
-		AllowConfidentialClientRegistration: cfg.AllowConfidentialClientRegistration,
-		AllowPrivateKeyJWTRegistration:      cfg.AllowPrivateKeyJWTRegistration,
-		HasStaticDelegateClients:            len(cfg.DelegateClients) > 0,
-		InsecureAllowHTTP:                   cfg.InsecureAllowHTTP,
+		Issuer:                                    cfg.Issuer,
+		AccessTokenLifespan:                       cfg.AccessTokenLifespan,
+		RefreshTokenLifespan:                      cfg.RefreshTokenLifespan,
+		AuthCodeLifespan:                          cfg.AuthCodeLifespan,
+		HMACSecrets:                               cfg.HMACSecrets,
+		SigningKeyID:                              signingKey.KeyID,
+		SigningKeyAlgorithm:                       signingKey.Algorithm,
+		SigningKey:                                signingKey.Key,
+		ScopesSupported:                           cfg.ScopesSupported,
+		BaselineClientScopes:                      cfg.BaselineClientScopes,
+		AllowedAudiences:                          cfg.AllowedAudiences,
+		AuthorizationEndpointBaseURL:              cfg.AuthorizationEndpointBaseURL,
+		CIMDEnabled:                               cfg.CIMDEnabled,
+		AllowConfidentialClientRegistration:       cfg.AllowConfidentialClientRegistration,
+		AllowPrivateKeyJWTRegistration:            cfg.AllowPrivateKeyJWTRegistration,
+		HasStaticDelegateClients:                  len(cfg.DelegateClients) > 0,
+		InsecureAllowHTTP:                         cfg.InsecureAllowHTTP,
 		InsecureAllowConfidentialOverLoopbackHTTP: cfg.InsecureAllowConfidentialOverLoopbackHTTP,
 		ForceConfidentialRedirectURIs:             cfg.ForceConfidentialRedirectURIs,
 		DisableTokenExchange:                      cfg.DisableTokenExchange,
@@ -289,7 +288,6 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage) (_ *server
 		upstreamRefresher:      refresher,
 		trustedIssuerValidator: trustedIssuerValidator,
 		configuredReconciler:   configuredReconciler,
-		configuredClients:      desiredClients,
 		configuredLeaseID:      configuredLeaseID,
 	}
 	if configuredReconciler != nil {
@@ -317,7 +315,7 @@ func (s *server) startConfiguredClientLeaseRenewal() {
 			case <-renewCtx.Done():
 				return
 			case <-ticker.C:
-				err := s.configuredReconciler.ReconcileConfiguredClients(renewCtx, s.configuredLeaseID, s.configuredClients)
+				err := s.configuredReconciler.RenewConfiguredClientLease(renewCtx, s.configuredLeaseID)
 				if err != nil {
 					slog.Warn("configured client lease renewal failed", "error", err)
 				}
@@ -332,33 +330,34 @@ func (s *server) startConfiguredClientLeaseRenewal() {
 // registration otherwise. It returns the (possibly wrapped) storage, the
 // reconciler the caller must use for lease renewal and release (nil if the
 // backend doesn't support reconciliation), the lease ID that reconciliation
-// claimed, and the desired client set for later renewal calls.
+// claimed. The desired set is used only for initial reconciliation; heartbeat
+// renewal mutates the lease only.
 func setupConfiguredClients(ctx context.Context, cfg Config, stor storage.Storage) (
-	storage.Storage, storage.ConfiguredClientReconciler, string, []fosite.Client, error,
+	storage.Storage, storage.ConfiguredClientReconciler, string, error,
 ) {
 	desiredClients, err := configuredClients(cfg)
 	if err != nil {
-		return nil, nil, "", nil, err
+		return nil, nil, "", err
 	}
 	stor, err = decorateStorageForSPIFFE(ctx, cfg, stor)
 	if err != nil {
-		return nil, nil, "", nil, err
+		return nil, nil, "", err
 	}
 
 	baseStore := storage.Unwrap(stor)
 	reconciler, ok := baseStore.(storage.ConfiguredClientReconciler)
 	if !ok {
 		if err := registerDelegateClients(ctx, stor, cfg.DelegateClients); err != nil {
-			return nil, nil, "", nil, err
+			return nil, nil, "", err
 		}
-		return stor, nil, "", desiredClients, nil
+		return stor, nil, "", nil
 	}
 
 	leaseID := uuid.NewString()
 	if err := reconciler.ReconcileConfiguredClients(ctx, leaseID, desiredClients); err != nil {
-		return nil, nil, "", nil, fmt.Errorf("reconcile configured clients: %w", err)
+		return nil, nil, "", fmt.Errorf("reconcile configured clients: %w", err)
 	}
-	return stor, reconciler, leaseID, desiredClients, nil
+	return stor, reconciler, leaseID, nil
 }
 
 // configuredClients builds the desired set of operator-declared clients for

pkg/authserver/storage/configured_client_reconciliation_test.go

@@ -0,0 +1,36 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package storage
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestConfiguredClientFingerprintCanonicalizesSemanticSets(t *testing.T) {
+	t.Parallel()
+
+	first := storedClient{
+		ID: "client", Secret: []byte("hashed-secret"),
+		Scopes:        []string{"profile", "openid", "openid"},
+		Audience:      []string{"https://b.example", "https://a.example"},
+		GrantTypes:    []string{"urn:ietf:params:oauth:grant-type:token-exchange"},
+		ResponseTypes: []string{"code", "code"},
+	}
+	second := storedClient{
+		ID: "client", Secret: []byte("hashed-secret"),
+		Scopes:        []string{"openid", "profile"},
+		Audience:      []string{"https://a.example", "https://b.example", "https://a.example"},
+		GrantTypes:    []string{"urn:ietf:params:oauth:grant-type:token-exchange"},
+		ResponseTypes: []string{"code"},
+	}
+
+	assert.Equal(t, configuredClientFingerprint(first), configuredClientFingerprint(second))
+	second.Secret = []byte("rotated-secret")
+	assert.NotEqual(t, configuredClientFingerprint(first), configuredClientFingerprint(second))
+	second.Secret = first.Secret
+	second.TokenEndpointAuthMethod = "client_secret_post"
+	assert.NotEqual(t, configuredClientFingerprint(first), configuredClientFingerprint(second))
+}

pkg/authserver/storage/memory.go

@@ -537,9 +537,10 @@ func (s *MemoryStorage) ReconcileConfiguredClients(_ context.Context, _ string,
 
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	for id, client := range desired {
+	for id := range desired {
 		if existing, exists := s.clients[id]; exists {
-			if registration.DCRIssued(existing) || !fingerprintOfClient(existing).equal(fingerprintOfClient(client)) {
+			_, configured := s.configuredClients[id]
+			if registration.DCRIssued(existing) || !configured {
 				return fmt.Errorf("%w: client %q is already registered", ErrAlreadyExists, id)
 			}
 		}
@@ -564,6 +565,9 @@ func (s *MemoryStorage) ReconcileConfiguredClients(_ context.Context, _ string,
 	return nil
 }
 
+// RenewConfiguredClientLease is a no-op for process-local storage.
+func (*MemoryStorage) RenewConfiguredClientLease(_ context.Context, _ string) error { return nil }
+
 // ReleaseConfiguredClientLease is a no-op for process-local storage.
 func (*MemoryStorage) ReleaseConfiguredClientLease(_ context.Context, _ string) error { return nil }
 

pkg/authserver/storage/memory_test.go

@@ -403,6 +403,28 @@ func TestMemoryStorage_ReconcileConfiguredClientsPrunesStaleRows(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func TestMemoryStorage_ReconcileConfiguredClientsReplacesOwnedClient(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+	s := NewMemoryStorage()
+	defer s.Close()
+
+	first := &mockClient{id: "configured", scopes: []string{"openid"}, public: false}
+	second := &mockClient{id: "configured", scopes: []string{"profile"}, public: false}
+	require.NoError(t, s.ReconcileConfiguredClient(ctx, first))
+	require.NoError(t, s.ReconcileConfiguredClients(ctx, "memory", []fosite.Client{second}))
+
+	retrieved, err := s.GetClient(ctx, "configured")
+	require.NoError(t, err)
+	assert.Equal(t, second, retrieved)
+
+	legacy := &mockClient{id: "legacy", public: false}
+	require.NoError(t, s.RegisterClient(ctx, legacy))
+	err = s.ReconcileConfiguredClients(ctx, "memory", []fosite.Client{&mockClient{id: "legacy", scopes: []string{"openid"}}})
+	require.ErrorIs(t, err, ErrAlreadyExists)
+}
+
 // matrix UpsertDCRIssuedClient must implement: create when absent, replace
 // (and renew the eviction position) when the existing record is itself
 // DCR-issued, refuse with ErrAlreadyExists when the existing record is NOT