diff --git a/docs/arch/11-auth-server-storage.md b/docs/arch/11-auth-server-storage.md index 70197d3859..54fce85d05 100644 --- a/docs/arch/11-auth-server-storage.md +++ b/docs/arch/11-auth-server-storage.md @@ -251,7 +251,16 @@ behavior (their stored row has no auth method and reads back as a bare `*fosite.DefaultClient`). This brings the Redis backend in line with the in-memory backend, which has always enforced the pinned method. -## Configuration +## Configured-client reconciliation + +Operator-declared delegate clients are reconciled as one desired set at embedded auth-server startup and on a periodic heartbeat while the server runs. Each reconcile writes every client in the desired set and marks any previously-configured row no longer in it as stale. A stale row is retained for a bounded grace period before pruning, so a replacement replica does not immediately remove a client still used by an older replica. 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 the configured path carry a `configured` ownership marker. A row without that marker is legacy data — adopted into the marker scheme only if its stored shape exactly matches the client being reconciled at that ID, otherwise reconciliation fails for that one entry as a genuine collision. DCR-issued and reserved rows are never touched by reconciliation, regardless of the desired set. + +Reconciliation is last-write-wins, with no cross-replica coordination beyond the shared Redis rows themselves: each replica's own periodic call unconditionally writes its own desired set and marks away whatever it does not want. During a rolling update, replicas running the old and new configuration briefly disagree, and a row can flip between old and new content until every old replica has been reconciled with (or replaced by) the new configuration. The stale-row grace period gives old replicas time to continue refreshing rows they still need; once no replica refreshes a removed row and the grace period elapses, a later reconcile prunes it. This is a bounded, self-healing transient and never blocks a replica's own startup or readiness. Same-ID material rotation remains last-write-wins during the rollout. + +Memory storage applies the same desired-set write-and-prune logic under its +mutex, single-process, so there is no cross-replica disagreement to consider. +DCR-issued and legacy/unowned rows remain protected there too. + ### CRD Configuration diff --git a/pkg/authserver/server_impl.go b/pkg/authserver/server_impl.go index ff7758c6ce..26ed490269 100644 --- a/pkg/authserver/server_impl.go +++ b/pkg/authserver/server_impl.go @@ -49,6 +49,13 @@ type server struct { // TrustedIssuers are configured (nil otherwise). Held here so Close can shut // down its per-issuer JWKS refresh worker pools; nothing else releases them. trustedIssuerValidator *tokenexchange.MultiIssuerTokenValidator + configuredReconciler storage.ConfiguredClientReconciler + // configuredClients is the desired operator-configured client set, + // re-asserted on every reconciliation-loop tick so a live replica keeps + // re-publishing (and sweeping toward) its own current configuration. + configuredClients []fosite.Client + configuredCancel context.CancelFunc + configuredDone chan struct{} } // DefaultUpstreamFactory creates the production upstream provider based on type. @@ -172,15 +179,11 @@ 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, err := decorateStorageForSPIFFE(ctx, cfg, stor) + stor, configuredReconciler, desiredConfiguredClients, err := setupConfiguredClients(ctx, cfg, stor) if err != nil { return nil, err } - if err := registerDelegateClients(ctx, stor, cfg.DelegateClients); err != nil { - return nil, err - } - slog.Debug("creating OAuth2 configuration") // Get signing key from KeyProvider @@ -279,14 +282,113 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage) (_ *server "issuer", cfg.Issuer, ) - return &server{ + srv := &server{ handler: router, storage: stor, dcrStore: dcrStore, upstreams: upstreams, upstreamRefresher: refresher, trustedIssuerValidator: trustedIssuerValidator, - }, nil + configuredReconciler: configuredReconciler, + configuredClients: desiredConfiguredClients, + } + if configuredReconciler != nil { + srv.startConfiguredClientReconciliationLoop() + } + return srv, nil +} + +// startConfiguredClientReconciliationLoop starts a background worker that +// periodically re-runs ReconcileConfiguredClients with this replica's own +// desired set on a ticker. Unlike a lease renewal, this is a full reconcile +// every tick: a live replica re-asserts (writes) every client it wants and +// sweeps away anything it doesn't, so a client removed from configuration +// converges to pruned once every replica has been reconciled with (or +// replaced by) the new configuration -- not just at startup. The worker +// stops, and its done channel closes, once s.configuredCancel is called +// (from Close). +func (s *server) startConfiguredClientReconciliationLoop() { + renewCtx, cancel := context.WithCancel(context.Background()) + s.configuredCancel = cancel + s.configuredDone = make(chan struct{}) + done := s.configuredDone + go func() { + ticker := time.NewTicker(10 * time.Second) + defer ticker.Stop() + defer close(done) + for { + select { + case <-renewCtx.Done(): + return + case <-ticker.C: + err := s.configuredReconciler.ReconcileConfiguredClients(renewCtx, s.configuredClients) + if err != nil { + slog.Warn("configured client reconciliation failed", "error", err) + } + } + } + }() +} + +// setupConfiguredClients decorates storage for SPIFFE, then reconciles the +// desired set of operator-configured (delegate) clients against durable +// storage if the backend supports it, falling back to one-shot delegate +// registration otherwise. It returns the (possibly wrapped) storage, the +// reconciler the caller must use for the periodic reconciliation loop (nil if +// the backend doesn't support reconciliation), and the desired client set the +// loop must keep re-asserting on every tick. +func setupConfiguredClients(ctx context.Context, cfg Config, stor storage.Storage) ( + storage.Storage, storage.ConfiguredClientReconciler, []fosite.Client, error, +) { + desiredClients, err := configuredClients(cfg) + if err != nil { + return nil, nil, nil, err + } + stor, err = decorateStorageForSPIFFE(ctx, cfg, stor) + if err != nil { + return nil, 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 stor, nil, nil, nil + } + + if err := reconciler.ReconcileConfiguredClients(ctx, desiredClients); err != nil { + return nil, nil, nil, fmt.Errorf("reconcile configured clients: %w", err) + } + return stor, reconciler, desiredClients, nil +} + +// configuredClients builds the desired set of operator-declared clients for +// ClientRegistry reconciliation. SPIFFE static clients are deliberately +// excluded: they are durably claimed as inert placeholders by +// decorateStorageForSPIFFE/preflightDurableCollisions, never as the real, +// token-exchange-capable client this function would otherwise build via +// registry.staticClients(). Reconciling the real client here would fight +// that placeholder for the same row (mismatched fingerprint, since the +// placeholder carries no grant types) and, if it ever won, would durably +// persist a usable client where only an inert stand-in must ever exist. +func configuredClients(cfg Config) ([]fosite.Client, error) { + clients := make([]fosite.Client, 0, len(cfg.DelegateClients)) + for _, delegateClient := range cfg.DelegateClients { + client, err := registration.NewStaticDelegateClient(registration.Config{ + ID: delegateClient.ClientID, + Secret: delegateClient.ClientSecret, + GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, + Scopes: delegateClient.Scopes, + Audience: delegateClient.Audiences, + }) + if err != nil { + return nil, fmt.Errorf("failed to create delegate client %q: %w", delegateClient.ClientID, err) + } + clients = append(clients, client) + } + return clients, nil } func registerDelegateClients(ctx context.Context, stor storage.Storage, delegateClients []DelegateClient) error { @@ -537,6 +639,10 @@ func (s *server) Close() error { slog.Debug("closing OAuth authorization server") s.CloseIdleConnections() var errs []error + if s.configuredCancel != nil { + s.configuredCancel() + <-s.configuredDone + } if s.trustedIssuerValidator != nil { if err := s.trustedIssuerValidator.Close(); err != nil { errs = append(errs, fmt.Errorf("failed to shut down trusted-issuer validator: %w", err)) diff --git a/pkg/authserver/storage/memory.go b/pkg/authserver/storage/memory.go index bb7e8965b5..54a7e61b24 100644 --- a/pkg/authserver/storage/memory.go +++ b/pkg/authserver/storage/memory.go @@ -83,6 +83,11 @@ type MemoryStorage struct { // marker is needed here. clients map[string]fosite.Client + // configuredClients records rows explicitly owned by operator configuration. + // It is separate from the client value so SPIFFE placeholder behavior is not + // altered by an ownership marker. + configuredClients map[string]struct{} + // clientOrder is the least-recently-proven-used order of clients: a // registration starts at the back, and RenewClientTTL moves a DCR-issued // client to the back again on proven use (a successful token @@ -235,6 +240,7 @@ func WithMinClientAge(d time.Duration) MemoryStorageOption { func NewMemoryStorage(opts ...MemoryStorageOption) *MemoryStorage { s := &MemoryStorage{ clients: make(map[string]fosite.Client), + configuredClients: make(map[string]struct{}), authCodes: make(map[string]*timedEntry[fosite.Requester]), accessTokens: make(map[string]*timedEntry[fosite.Requester]), refreshTokens: make(map[string]*timedEntry[fosite.Requester]), @@ -505,6 +511,85 @@ func (s *MemoryStorage) UpsertDCRIssuedClient(_ context.Context, client fosite.C s.clientOrder = slices.DeleteFunc(s.clientOrder, func(e clientOrderEntry) bool { return e.id == id }) s.clientOrder = append(s.clientOrder, clientOrderEntry{id: id, touchedAt: time.Now()}) s.clients[id] = client + delete(s.configuredClients, id) + return nil +} + +// ReconcileConfiguredClients atomically applies the complete desired configured +// client set and removes stale explicitly configured clients. +func (s *MemoryStorage) ReconcileConfiguredClients(_ context.Context, clients []fosite.Client) error { + desired := make(map[string]fosite.Client, len(clients)) + for _, client := range clients { + if client == nil { + return fmt.Errorf("configured client is required") + } + if registration.DCRIssued(client) { + return fmt.Errorf("configured client %q must not carry the DCR-issued marker", client.GetID()) + } + if err := ValidateRegisterableClientID(client.GetID()); err != nil { + return err + } + if _, exists := desired[client.GetID()]; exists { + return fmt.Errorf("duplicate configured client %q", client.GetID()) + } + desired[client.GetID()] = client + } + + s.mu.Lock() + defer s.mu.Unlock() + for id := range desired { + if existing, exists := s.clients[id]; exists { + _, configured := s.configuredClients[id] + if registration.DCRIssued(existing) || !configured { + return fmt.Errorf("%w: client %q is already registered", ErrAlreadyExists, id) + } + } + } + workingClients := make(map[string]fosite.Client, len(s.clients)) + for id, client := range s.clients { + workingClients[id] = client + } + workingConfigured := make(map[string]struct{}, len(s.configuredClients)) + for id := range s.configuredClients { + workingConfigured[id] = struct{}{} + } + working := &MemoryStorage{ + clients: workingClients, + configuredClients: workingConfigured, + clientOrder: slices.Clone(s.clientOrder), + maxClients: s.maxClients, + minClientAge: s.minClientAge, + } + if err := applyConfiguredClientsLocked(working, desired); err != nil { + return err + } + + s.clients = working.clients + s.configuredClients = working.configuredClients + s.clientOrder = working.clientOrder + return nil +} + +// applyConfiguredClientsLocked applies a desired set to isolated working state. +// The caller owns the live-state lock; working must not be shared with callers. +func applyConfiguredClientsLocked(working *MemoryStorage, desired map[string]fosite.Client) error { + for id := range working.configuredClients { + if _, keep := desired[id]; !keep { + delete(working.clients, id) + delete(working.configuredClients, id) + } + } + for id, client := range desired { + if _, exists := working.clients[id]; exists { + working.clientOrder = slices.DeleteFunc(working.clientOrder, func(e clientOrderEntry) bool { return e.id == id }) + } else if err := working.insertClientLocked(id, client); err != nil { + return err + } + working.clients[id] = client + working.configuredClients[id] = struct{}{} + working.clientOrder = slices.DeleteFunc(working.clientOrder, func(e clientOrderEntry) bool { return e.id == id }) + working.clientOrder = append(working.clientOrder, clientOrderEntry{id: id, touchedAt: time.Now()}) + } return nil } @@ -527,7 +612,11 @@ func (s *MemoryStorage) ReconcileConfiguredClient(_ context.Context, client fosi existing, exists := s.clients[id] if !exists { - return s.insertClientLocked(id, client) + if err := s.insertClientLocked(id, client); err != nil { + return err + } + s.configuredClients[id] = struct{}{} + return nil } if registration.DCRIssued(existing) { return fmt.Errorf("%w: client %q is DCR-issued, refusing to overwrite with a configured client", @@ -543,6 +632,7 @@ func (s *MemoryStorage) ReconcileConfiguredClient(_ context.Context, client fosi s.clientOrder = slices.DeleteFunc(s.clientOrder, func(e clientOrderEntry) bool { return e.id == id }) s.clientOrder = append(s.clientOrder, clientOrderEntry{id: id, touchedAt: time.Now()}) s.clients[id] = client + s.configuredClients[id] = struct{}{} return nil } @@ -556,6 +646,7 @@ func (s *MemoryStorage) insertClientLocked(id string, client fosite.Client) erro victim := s.clientOrder[idx].id s.clientOrder = append(s.clientOrder[:idx], s.clientOrder[idx+1:]...) delete(s.clients, victim) + delete(s.configuredClients, victim) slog.Debug("evicted oldest DCR-issued client registration at capacity", "client_id", victim, "max_clients", s.maxClients) } else { diff --git a/pkg/authserver/storage/memory_test.go b/pkg/authserver/storage/memory_test.go index 25eab0325a..ec96df4b8d 100644 --- a/pkg/authserver/storage/memory_test.go +++ b/pkg/authserver/storage/memory_test.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "net/url" + "slices" "sync" "sync/atomic" "testing" @@ -380,7 +381,90 @@ func TestMemoryStorage_ReconcileConfiguredClient(t *testing.T) { }) } -// TestMemoryStorage_UpsertDCRIssuedClient covers the create/replace/reject +func TestMemoryStorage_ReconcileConfiguredClientsPrunesStaleRows(t *testing.T) { + t.Parallel() + + ctx := t.Context() + s := NewMemoryStorage() + defer s.Close() + + keep := &mockClient{id: "keep", scopes: []string{"openid"}, public: false} + stale := &mockClient{id: "stale", scopes: []string{"openid"}, public: false} + dcr := dcrClient(t, "dcr") + require.NoError(t, s.ReconcileConfiguredClient(ctx, keep)) + require.NoError(t, s.ReconcileConfiguredClient(ctx, stale)) + require.NoError(t, s.RegisterClient(ctx, dcr)) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{keep})) + _, err := s.GetClient(ctx, "stale") + assert.ErrorIs(t, err, ErrNotFound) + _, err = s.GetClient(ctx, "keep") + require.NoError(t, err) + _, err = s.GetClient(ctx, "dcr") + require.NoError(t, err) +} + +func TestMemoryStorage_ReconcileConfiguredClientsLeavesStateUnchangedOnCapacityFailure(t *testing.T) { + t.Parallel() + + ctx := t.Context() + s := NewMemoryStorage(WithMaxClients(3)) + defer s.Close() + + keep := &mockClient{id: "keep", scopes: []string{"openid"}, public: false} + stale := &mockClient{id: "stale", scopes: []string{"openid"}, public: false} + legacy := &mockClient{id: "legacy", scopes: []string{"openid"}, public: false} + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{keep, stale})) + require.NoError(t, s.RegisterClient(ctx, legacy)) + + s.mu.RLock() + beforeClients := make(map[string]fosite.Client, len(s.clients)) + for id, client := range s.clients { + beforeClients[id] = client + } + beforeConfigured := make(map[string]struct{}, len(s.configuredClients)) + for id := range s.configuredClients { + beforeConfigured[id] = struct{}{} + } + beforeOrder := slices.Clone(s.clientOrder) + s.mu.RUnlock() + + // The desired set removes stale and adds two clients. The first addition can + // fit after pruning, but the second cannot because legacy is not evictable. + newOne := &mockClient{id: "new-one", scopes: []string{"openid"}, public: false} + newTwo := &mockClient{id: "new-two", scopes: []string{"openid"}, public: false} + err := s.ReconcileConfiguredClients(ctx, []fosite.Client{keep, newOne, newTwo}) + require.ErrorIs(t, err, ErrClientCapacity) + + s.mu.RLock() + assert.Equal(t, beforeClients, s.clients) + assert.Equal(t, beforeConfigured, s.configuredClients) + assert.Equal(t, beforeOrder, s.clientOrder) + s.mu.RUnlock() +} + +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, []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, []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 diff --git a/pkg/authserver/storage/mocks/mock_storage.go b/pkg/authserver/storage/mocks/mock_storage.go index 8ee47fdf71..74eb3e7ea7 100644 --- a/pkg/authserver/storage/mocks/mock_storage.go +++ b/pkg/authserver/storage/mocks/mock_storage.go @@ -3,7 +3,7 @@ // // Generated by this command: // -// mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,AssertionJWTConsumer,ClientRegistry,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage,DCRCredentialStore +// mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,AssertionJWTConsumer,ClientRegistry,ConfiguredClientReconciler,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage,DCRCredentialStore // // Package mocks is a generated GoMock package. @@ -301,6 +301,44 @@ func (mr *MockClientRegistryMockRecorder) UpsertDCRIssuedClient(ctx, client any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpsertDCRIssuedClient", reflect.TypeOf((*MockClientRegistry)(nil).UpsertDCRIssuedClient), ctx, client) } +// MockConfiguredClientReconciler is a mock of ConfiguredClientReconciler interface. +type MockConfiguredClientReconciler struct { + ctrl *gomock.Controller + recorder *MockConfiguredClientReconcilerMockRecorder + isgomock struct{} +} + +// MockConfiguredClientReconcilerMockRecorder is the mock recorder for MockConfiguredClientReconciler. +type MockConfiguredClientReconcilerMockRecorder struct { + mock *MockConfiguredClientReconciler +} + +// NewMockConfiguredClientReconciler creates a new mock instance. +func NewMockConfiguredClientReconciler(ctrl *gomock.Controller) *MockConfiguredClientReconciler { + mock := &MockConfiguredClientReconciler{ctrl: ctrl} + mock.recorder = &MockConfiguredClientReconcilerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockConfiguredClientReconciler) EXPECT() *MockConfiguredClientReconcilerMockRecorder { + return m.recorder +} + +// ReconcileConfiguredClients mocks base method. +func (m *MockConfiguredClientReconciler) ReconcileConfiguredClients(ctx context.Context, clients []fosite.Client) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ReconcileConfiguredClients", ctx, clients) + ret0, _ := ret[0].(error) + return ret0 +} + +// ReconcileConfiguredClients indicates an expected call of ReconcileConfiguredClients. +func (mr *MockConfiguredClientReconcilerMockRecorder) ReconcileConfiguredClients(ctx, clients any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReconcileConfiguredClients", reflect.TypeOf((*MockConfiguredClientReconciler)(nil).ReconcileConfiguredClients), ctx, clients) +} + // MockUpstreamTokenStorage is a mock of UpstreamTokenStorage interface. type MockUpstreamTokenStorage struct { ctrl *gomock.Controller diff --git a/pkg/authserver/storage/redis.go b/pkg/authserver/storage/redis.go index bc8c1dc86f..468ee0d61c 100644 --- a/pkg/authserver/storage/redis.go +++ b/pkg/authserver/storage/redis.go @@ -4,6 +4,7 @@ package storage import ( + "bytes" "context" "encoding/json" "errors" @@ -39,6 +40,12 @@ const nullMarker = "null" // confuse this row with a healthy long-lived registration. const pastExpiryDCRTTL = time.Second +// configuredClientStaleGracePeriod allows replicas from an older configuration +// to finish draining before a replacement replica removes their client row. +// The heartbeat runs every 10 seconds, so this covers several normal heartbeats +// without requiring cross-replica coordination. +const configuredClientStaleGracePeriod = 30 * time.Second + // maxDCRClaimRetries bounds StoreDCRCredentialsIfAbsent's WATCH/MULTI retry // loop. go-redis does not retry Watch internally: a concurrent write to the // watched key (another replica claiming, refreshing, or evicting the same @@ -219,6 +226,12 @@ type storedClient struct { // is not compensated for — it predates confidential DCR support entirely, // so it cannot be DCR-issued. DCRIssued bool `json:"dcr_issued,omitempty"` + // Configured is true only for rows explicitly owned by operator configuration. + // Missing values decode false so legacy rows remain exempt from pruning. + Configured bool `json:"configured,omitempty"` + // StaleSinceUnix is set when a configured row is first absent from a + // replica's desired set. It is cleared whenever a replica writes the row. + StaleSinceUnix int64 `json:"stale_since_unix,omitempty"` // Reserved is true when the row is a SPIFFE static-client durable // placeholder (see staticClientPlaceholder in spiffe_decorator.go) — // never a real, authenticatable client. It is checked before GrantTypes/ @@ -369,7 +382,7 @@ func clientFromStored(stored storedClient, hasTTL bool) fosite.Client { // clientFromStored treats the empty method as a legacy row. Do NOT substitute // a "none" fallback here — that would silently reclassify a confidential row // as public on read-back. -func buildStoredClient(client fosite.Client) storedClient { +func buildStoredClient(client fosite.Client, configured bool) storedClient { stored := storedClient{ ID: client.GetID(), Secret: client.GetHashedSecret(), @@ -379,6 +392,7 @@ func buildStoredClient(client fosite.Client) storedClient { Scopes: client.GetScopes(), Audience: client.GetAudience(), Public: client.IsPublic(), + Configured: configured, } stored.Reserved = isReservedPlaceholder(client) // Resources and IdentityFingerprint are only ever restored on read for a @@ -415,7 +429,7 @@ func (s *RedisStorage) RegisterClient(ctx context.Context, client fosite.Client) } key := redisKey(s.keyPrefix, KeyTypeClient, client.GetID()) - stored := buildStoredClient(client) + stored := buildStoredClient(client, false) data, err := json.Marshal(stored) //nolint:gosec // G117 - internal Redis storage serialization, not exposed to users if err != nil { @@ -471,7 +485,7 @@ func (s *RedisStorage) UpsertDCRIssuedClient(ctx context.Context, client fosite. } key := redisKey(s.keyPrefix, KeyTypeClient, client.GetID()) - stored := buildStoredClient(client) + stored := buildStoredClient(client, false) data, err := json.Marshal(stored) //nolint:gosec // G117 - internal Redis storage serialization, not exposed to users if err != nil { return fmt.Errorf("failed to marshal client: %w", err) @@ -578,7 +592,7 @@ func (s *RedisStorage) ReconcileConfiguredClient(ctx context.Context, client fos } key := redisKey(s.keyPrefix, KeyTypeClient, client.GetID()) - stored := buildStoredClient(client) + stored := buildStoredClient(client, true) data, err := json.Marshal(stored) //nolint:gosec // G117 - internal Redis storage serialization, not exposed to users if err != nil { return fmt.Errorf("failed to marshal client: %w", err) @@ -638,6 +652,199 @@ func (s *RedisStorage) ReconcileConfiguredClient(ctx context.Context, client fos return watchErr } +// maxConfiguredClients bounds the desired set ReconcileConfiguredClients will +// accept in one call: operator configuration is expected to hold at most a +// few hundred entries, never anywhere near this ceiling. Guards against an +// unbounded per-client write loop from a malformed or malicious config. +const maxConfiguredClients = 10_000 + +// ReconcileConfiguredClients validates the complete desired set, writes each +// client's row, then marks any previously-configured row no longer in the +// desired set for grace-period pruning. See the ConfiguredClientReconciler +// interface doc for the full last-write-wins contract. +func (s *RedisStorage) ReconcileConfiguredClients(ctx context.Context, clients []fosite.Client) error { + if len(clients) > maxConfiguredClients { + return fmt.Errorf("too many configured clients: %d exceeds limit of %d", len(clients), maxConfiguredClients) + } + desired := make(map[string]struct{}, len(clients)) + for _, client := range clients { + if client == nil { + return fmt.Errorf("configured client is required") + } + if registration.DCRIssued(client) { + return fmt.Errorf("configured client %q must not carry the DCR-issued marker", client.GetID()) + } + if err := ValidateRegisterableClientID(client.GetID()); err != nil { + return err + } + if _, exists := desired[client.GetID()]; exists { + return fmt.Errorf("duplicate configured client %q", client.GetID()) + } + desired[client.GetID()] = struct{}{} + } + + for _, client := range clients { + if err := s.writeConfiguredClient(ctx, client); err != nil { + return err + } + } + + return s.sweepConfiguredClients(ctx, desired) +} + +// writeConfiguredClient atomically writes a single configured client's row. +// An existing DCR-issued or Reserved row is never touched. An existing row +// that is not yet marked configured is adopted (and stamped configured) only +// if its stored shape exactly matches client; otherwise the write fails as a +// genuine collision. An existing row already marked configured is always +// overwritten (last-write-wins), regardless of whether its material matches. +func (s *RedisStorage) writeConfiguredClient(ctx context.Context, client fosite.Client) error { + key := redisKey(s.keyPrefix, KeyTypeClient, client.GetID()) + stored := buildStoredClient(client, true) + data, err := json.Marshal(stored) //nolint:gosec // G117 - internal Redis storage serialization, not exposed to users + if err != nil { + return fmt.Errorf("marshal configured client %q: %w", client.GetID(), err) + } + + setPipelined := func(tx *redis.Tx) error { + _, err := tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error { + pipe.Set(ctx, key, data, 0) + return nil + }) + return err + } + + txFn := func(tx *redis.Tx) error { + existingData, getErr := tx.Get(ctx, key).Bytes() + if errors.Is(getErr, redis.Nil) { + return setPipelined(tx) + } + if getErr != nil { + return fmt.Errorf("get existing client %q: %w", client.GetID(), getErr) + } + + var existingStored storedClient + if unmarshalErr := json.Unmarshal(existingData, &existingStored); unmarshalErr != nil { + return fmt.Errorf("unmarshal existing client %q: %w", client.GetID(), unmarshalErr) + } + if existingStored.DCRIssued || existingStored.Reserved { + return fmt.Errorf("%w: configured client %q collides with an existing non-configured registration", + ErrAlreadyExists, client.GetID()) + } + if !existingStored.Configured && !existingStored.fingerprint().equal(stored.fingerprint()) { + return fmt.Errorf("%w: configured client %q collides with an existing non-configured registration", + ErrAlreadyExists, client.GetID()) + } + + return setPipelined(tx) + } + + var watchErr error + for attempt := 0; attempt < maxConfiguredClientReconcileRetries; attempt++ { + watchErr = s.client.Watch(ctx, txFn, key) + if !errors.Is(watchErr, redis.TxFailedErr) { + return watchErr + } + } + return watchErr +} + +// sweepConfiguredClients marks configured rows absent from desired and deletes +// them only after a grace period. The mark is reset by writeConfiguredClient, +// allowing a replica that still desires the row to keep it alive without +// cross-replica coordination. SCAN only discovers candidates; the conditional +// update/delete makes the decision against the row's current content. +func (s *RedisStorage) sweepConfiguredClients(ctx context.Context, desired map[string]struct{}) error { + var cursor uint64 + pattern := redisKey(s.keyPrefix, KeyTypeClient, "*") + now := time.Now().Unix() + for { + keys, next, err := s.client.Scan(ctx, cursor, pattern, 100).Result() + if err != nil { + return fmt.Errorf("scan configured clients: %w", err) + } + for _, key := range keys { + data, err := s.client.Get(ctx, key).Bytes() + if errors.Is(err, redis.Nil) { + continue + } + if err != nil { + return fmt.Errorf("read configured client %q: %w", key, err) + } + var stored storedClient + if err := json.Unmarshal(data, &stored); err != nil { + slog.Warn("skipping unmarshalable row during configured-client sweep", "key", key, "error", err) + continue + } + if !stored.Configured || stored.DCRIssued || stored.Reserved { + continue + } + if _, keep := desired[stored.ID]; keep { + continue + } + if err := s.markOrDeleteStaleConfiguredClient(ctx, key, data, now); err != nil { + return err + } + } + cursor = next + if cursor == 0 { + return nil + } + } +} + +// markOrDeleteStaleConfiguredClient atomically marks a stale row or deletes it +// after the grace period. A concurrent configured write changes the row and +// therefore prevents this operation from clobbering it. +func (s *RedisStorage) markOrDeleteStaleConfiguredClient(ctx context.Context, key string, expected []byte, now int64) error { + var watchErr error + for attempt := 0; attempt < maxConfiguredClientReconcileRetries; attempt++ { + watchErr = s.client.Watch(ctx, func(tx *redis.Tx) error { + current, getErr := tx.Get(ctx, key).Bytes() + if errors.Is(getErr, redis.Nil) { + return nil + } + if getErr != nil { + return fmt.Errorf("read client %q: %w", key, getErr) + } + if !bytes.Equal(current, expected) { + return nil + } + var stored storedClient + if err := json.Unmarshal(current, &stored); err != nil { + return nil + } + if stored.StaleSinceUnix == 0 { + stored.StaleSinceUnix = now + data, err := json.Marshal(stored) //nolint:gosec // G117 - internal Redis storage serialization + if err != nil { + return fmt.Errorf("marshal stale configured client %q: %w", key, err) + } + _, err = tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error { + pipe.Set(ctx, key, data, 0) + return nil + }) + return err + } + if stored.StaleSinceUnix > now-int64(configuredClientStaleGracePeriod/time.Second) { + return nil + } + _, err := tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error { + pipe.Del(ctx, key) + return nil + }) + return err + }, key) + if !errors.Is(watchErr, redis.TxFailedErr) { + break + } + } + if watchErr != nil { + return fmt.Errorf("update stale configured client %q: %w", key, watchErr) + } + return nil +} + // GetClient loads the client by its ID. // // The value and the key's TTL are fetched together in one pipelined round diff --git a/pkg/authserver/storage/redis_test.go b/pkg/authserver/storage/redis_test.go index cee64a03c5..86ded44c99 100644 --- a/pkg/authserver/storage/redis_test.go +++ b/pkg/authserver/storage/redis_test.go @@ -769,6 +769,185 @@ func TestRedisStorage_ReconcileConfiguredClient(t *testing.T) { }) } +// TestRedisStorage_ReconcileConfiguredClients covers ReconcileConfiguredClients' +// last-write-wins bulk semantics: it writes every desired client, prunes a +// previously configured client no longer desired, and leaves legacy/DCR-issued/ +// reserved rows untouched regardless of the desired set. +func TestRedisStorage_ReconcileConfiguredClients(t *testing.T) { + t.Parallel() + + newConfigured := func(id string, scopes ...string) fosite.Client { + client, err := registration.NewStaticDelegateClient(registration.Config{ + ID: id, Secret: "secret", GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, + Scopes: scopes, Audience: []string{"https://mcp.example"}, + }) + require.NoError(t, err) + return client + } + + t.Run("repeated call with the same desired set is idempotent", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + client := newConfigured("stable", "openid") + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{client})) + key := redisKey(s.keyPrefix, KeyTypeClient, "stable") + before, err := s.client.Get(ctx, key).Bytes() + require.NoError(t, err) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{client})) + after, err := s.client.Get(ctx, key).Bytes() + require.NoError(t, err) + assert.Equal(t, before, after) + }) + }) + + t.Run("last write wins when a configured client's material changes", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{newConfigured("shared", "openid")})) + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{newConfigured("shared", "profile")})) + + retrieved, err := s.GetClient(ctx, "shared") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"profile"}, retrieved.GetScopes()) + }) + }) + + t.Run("marks stale rows before pruning them after the grace period", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + stale := newConfigured("stale", "openid") + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{stale})) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, nil)) + _, err := s.GetClient(ctx, "stale") + require.NoError(t, err, "a stale client must survive the initial grace period") + + key := redisKey(s.keyPrefix, KeyTypeClient, "stale") + data, err := s.client.Get(ctx, key).Bytes() + require.NoError(t, err) + var stored storedClient + require.NoError(t, json.Unmarshal(data, &stored)) + assert.NotZero(t, stored.StaleSinceUnix) + + stored.StaleSinceUnix = time.Now().Add(-configuredClientStaleGracePeriod).Unix() + data, err = json.Marshal(stored) + require.NoError(t, err) + require.NoError(t, s.client.Set(ctx, key, data, 0).Err()) + require.NoError(t, s.ReconcileConfiguredClients(ctx, nil)) + _, err = s.GetClient(ctx, "stale") + assert.ErrorIs(t, err, ErrNotFound) + }) + }) + + t.Run("never prunes a legacy row with no configured marker", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + require.NoError(t, s.RegisterClient(ctx, newConfigured("legacy", "openid"))) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, nil)) + + _, err := s.GetClient(ctx, "legacy") + require.NoError(t, err, "a row with no configured marker predates this feature and must never be pruned") + }) + }) + + t.Run("never prunes a DCR-issued row", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + require.NoError(t, s.UpsertDCRIssuedClient(ctx, newDCRClient(t, "dcr", oauthproto.TokenEndpointAuthMethodNone, ""))) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, nil)) + + _, err := s.GetClient(ctx, "dcr") + require.NoError(t, err) + }) + }) + + t.Run("a DCR-issued row is never overwritten by a configured client at the same ID", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + require.NoError(t, s.UpsertDCRIssuedClient(ctx, newDCRClient(t, "dcr", oauthproto.TokenEndpointAuthMethodNone, ""))) + + err := s.ReconcileConfiguredClients(ctx, []fosite.Client{newConfigured("dcr", "openid")}) + require.ErrorIs(t, err, ErrAlreadyExists) + }) + }) + + t.Run("refuses to replace a reserved configured row", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + reserved := storedClient{ID: "reserved", Configured: true, Reserved: true} + data, err := json.Marshal(reserved) + require.NoError(t, err) + require.NoError(t, s.client.Set(ctx, redisKey(s.keyPrefix, KeyTypeClient, "reserved"), data, 0).Err()) + + err = s.ReconcileConfiguredClients(ctx, []fosite.Client{newConfigured("reserved", "openid")}) + require.ErrorIs(t, err, ErrAlreadyExists) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, nil)) + assert.Equal(t, int64(1), s.client.Exists(ctx, redisKey(s.keyPrefix, KeyTypeClient, "reserved")).Val()) + }) + }) + + t.Run("adopts an unmarked legacy row whose shape matches", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + client := newConfigured("legacy-match", "openid") + require.NoError(t, s.RegisterClient(ctx, client)) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{client})) + + key := redisKey(s.keyPrefix, KeyTypeClient, "legacy-match") + data, err := s.client.Get(ctx, key).Bytes() + require.NoError(t, err) + var stored storedClient + require.NoError(t, json.Unmarshal(data, &stored)) + assert.True(t, stored.Configured) + }) + }) + + t.Run("rejects an unmarked legacy row whose shape differs as a genuine collision", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + require.NoError(t, s.RegisterClient(ctx, newConfigured("legacy-mismatch", "openid"))) + + err := s.ReconcileConfiguredClients(ctx, []fosite.Client{newConfigured("legacy-mismatch", "profile")}) + require.ErrorIs(t, err, ErrAlreadyExists) + }) + }) + + t.Run("rejects a duplicate client ID in the desired set", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + err := s.ReconcileConfiguredClients(ctx, + []fosite.Client{newConfigured("dup", "openid"), newConfigured("dup", "openid")}) + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate configured client") + }) + }) + + t.Run("rejects a desired set larger than the configured limit", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + clients := make([]fosite.Client, 10_001) + for i := range clients { + clients[i] = &mockClient{id: fmt.Sprintf("client-%d", i)} + } + err := s.ReconcileConfiguredClients(ctx, clients) + require.Error(t, err) + assert.Contains(t, err.Error(), "too many configured clients") + }) + }) + + t.Run("sweep skips a corrupt row without failing the whole reconcile", func(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + require.NoError(t, s.ReconcileConfiguredClients(ctx, + []fosite.Client{newConfigured("keep", "openid"), newConfigured("stale", "openid")})) + + corruptKey := redisKey(s.keyPrefix, KeyTypeClient, "corrupt") + require.NoError(t, s.client.Set(ctx, corruptKey, "not-json", 0).Err()) + + require.NoError(t, s.ReconcileConfiguredClients(ctx, []fosite.Client{newConfigured("keep", "openid")})) + + _, err := s.GetClient(ctx, "keep") + require.NoError(t, err) + _, err = s.GetClient(ctx, "stale") + require.NoError(t, err, "the stale row remains during its grace period") + assert.Equal(t, int64(1), s.client.Exists(ctx, corruptKey).Val(), "the corrupt row itself is left alone") + }) + }) +} + // TestRedisStorage_UpsertDCRIssuedClient covers the create/replace/reject // matrix UpsertDCRIssuedClient must implement: create when absent (with the // DCR TTL, not permanent), replace and renew the TTL when the existing row is diff --git a/pkg/authserver/storage/types.go b/pkg/authserver/storage/types.go index 61c3d15d3b..b25dac34f2 100644 --- a/pkg/authserver/storage/types.go +++ b/pkg/authserver/storage/types.go @@ -16,7 +16,7 @@ // OAuth authorization server. package storage -//go:generate mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,AssertionJWTConsumer,ClientRegistry,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage,DCRCredentialStore +//go:generate mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,AssertionJWTConsumer,ClientRegistry,ConfiguredClientReconciler,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage,DCRCredentialStore import ( "context" @@ -597,17 +597,19 @@ func ValidateRegisterableClientID(id string) error { return nil } +// canonicalStringSet returns a sorted, deduplicated copy of values. +// Canonicalisation mirrors ScopesHash's approach so every configured-client +// material comparison uses the same set semantics. +func canonicalStringSet(values []string) []string { + canonical := slices.Clone(values) + sort.Strings(canonical) + return slices.Compact(canonical) +} + // sameStringSet reports whether a and b contain the same elements as sets: -// order and duplicate count don't matter, only membership. Canonicalisation -// (sort, then dedup) mirrors ScopesHash's approach so the two stay consistent. +// order and duplicate count don't matter, only membership. func sameStringSet(a, b []string) bool { - as := slices.Clone(a) - bs := slices.Clone(b) - sort.Strings(as) - sort.Strings(bs) - as = slices.Compact(as) - bs = slices.Compact(bs) - return slices.Equal(as, bs) + return slices.Equal(canonicalStringSet(a), canonicalStringSet(b)) } // clientFingerprint is the identity of a configured client registration: the @@ -734,6 +736,29 @@ type ClientRegistry interface { RenewClientTTL(ctx context.Context, client fosite.Client) error } +// ConfiguredClientReconciler lets a storage backend durably reconcile the +// complete set of operator-configured (not dynamically registered) OAuth +// clients: unconditionally write every client in the desired set, and +// eventually prune any previously-configured row no longer in it. Redis +// implementations retain an undesired row for a bounded grace period so +// another replica can continue refreshing it during a rollout. It is +// last-write-wins by design — multiple replicas calling this concurrently +// with different desired sets (e.g. mid rolling-update) may transiently +// overwrite each other's view, +// but every replica's own periodic call converges the shared storage to its +// own current configuration, and the moment every "stale" replica has exited +// (or is next reconciled with the new configuration), the row settles. +// Implementations must never prune or overwrite a DCR-issued or SPIFFE +// reserved-placeholder row — those are owned by a different mechanism +// entirely. A row that predates this marker (no "configured" ownership tag +// at all) may be adopted into the marker scheme only if its stored shape +// exactly matches the desired client being written for that ID; otherwise it +// is treated as a genuine, unrelated collision and reconciliation fails for +// that entry. +type ConfiguredClientReconciler interface { + ReconcileConfiguredClients(ctx context.Context, clients []fosite.Client) error +} + // UpstreamTokenRowID is an opaque, process-local coordination key for one // logical upstream token row. Callers must not persist, log, or expose it. type UpstreamTokenRowID string