Skip to content

Commit 05b4beb

Browse files
committed
Reconcile stale configured OAuth clients
Removing a delegate client-auth association from config never cleaned up its durable storage record, leaving orphaned operator-owned clients indistinguishable from live ones. Track configured-client ownership through eviction, reconcile the full desired set against durable storage at startup, and for Redis renew a lease in the background so a stale replica cannot prune active configuration out from under a healthy one. SPIFFE static clients are deliberately excluded from this reconciliation: they are already durably claimed as inert placeholders by decorateStorageForSPIFFE/preflightDurableCollisions, and feeding the real, token-exchange-capable client into this reconciler would fight that placeholder for the same row and could persist a usable client where only an inert stand-in must ever exist. Stale SPIFFE placeholder cleanup is left for follow-up once SPIFFE client-auth enforcement lands. ReleaseConfiguredClientLease no longer sweeps with an empty desired set on release: doing so treated the sole live replica shutting down as "nothing is desired," pruning every durably reconciled client on a routine restart. Releasing now only relinquishes the caller's lease; cleanup of anything genuinely stale happens on the next reconciliation call, which supplies a real desired set. Fixes #6477 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
1 parent 720209d commit 05b4beb

9 files changed

Lines changed: 661 additions & 16 deletions

File tree

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,26 @@ behavior (their stored row has no auth method and reads back as a bare
251251
`*fosite.DefaultClient`). This brings the Redis backend in line with the
252252
in-memory backend, which has always enforced the pinned method.
253253

254-
## Configuration
254+
## Configured-client reconciliation
255+
256+
Operator-declared delegate and SPIFFE clients are reconciled as one desired set at
257+
embedded auth-server startup. Redis rows written by this path carry a
258+
`configured` ownership marker. Rows without that marker are legacy data and are
259+
never pruned; DCR-issued rows are also excluded.
260+
261+
Redis replicas publish their desired client IDs and logical fingerprints under a
262+
short-lived lease. A live lease protects its claimed IDs during rolling
263+
updates. Reconciliation uses cursor-based `SCAN` over the client namespace and
264+
rechecks candidate rows before deletion, so concurrent registration or
265+
reconciliation cannot be removed from a stale read. Conflicting fingerprints
266+
for the same client ID fail reconciliation instead of silently overwriting one
267+
replica's configuration.
268+
269+
The server performs an initial reconciliation before serving requests, renews
270+
its lease while running, and releases it before closing storage. Memory storage
271+
uses the same desired-set semantics under its existing mutex and treats lease
272+
operations as process-local no-ops.
273+
255274

256275
### CRD Configuration
257276

‎pkg/authserver/server_impl.go‎

Lines changed: 115 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"time"
1414

1515
josev3 "github.com/go-jose/go-jose/v3"
16+
"github.com/google/uuid"
1617
"github.com/ory/fosite"
1718
"github.com/ory/fosite/compose"
1819

@@ -49,6 +50,11 @@ type server struct {
4950
// TrustedIssuers are configured (nil otherwise). Held here so Close can shut
5051
// down its per-issuer JWKS refresh worker pools; nothing else releases them.
5152
trustedIssuerValidator *tokenexchange.MultiIssuerTokenValidator
53+
configuredReconciler storage.ConfiguredClientReconciler
54+
configuredClients []fosite.Client
55+
configuredLeaseID string
56+
configuredCancel context.CancelFunc
57+
configuredDone chan struct{}
5258
}
5359

5460
// DefaultUpstreamFactory creates the production upstream provider based on type.
@@ -172,15 +178,11 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage) (_ *server
172178
return nil, fmt.Errorf("storage backend %T does not implement storage.DCRCredentialStore", baseStore)
173179
}
174180

175-
stor, err := decorateStorageForSPIFFE(ctx, cfg, stor)
181+
stor, configuredReconciler, configuredLeaseID, desiredClients, err := setupConfiguredClients(ctx, cfg, stor)
176182
if err != nil {
177183
return nil, err
178184
}
179185

180-
if err := registerDelegateClients(ctx, stor, cfg.DelegateClients); err != nil {
181-
return nil, err
182-
}
183-
184186
slog.Debug("creating OAuth2 configuration")
185187

186188
// Get signing key from KeyProvider
@@ -279,14 +281,111 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage) (_ *server
279281
"issuer", cfg.Issuer,
280282
)
281283

282-
return &server{
284+
srv := &server{
283285
handler: router,
284286
storage: stor,
285287
dcrStore: dcrStore,
286288
upstreams: upstreams,
287289
upstreamRefresher: refresher,
288290
trustedIssuerValidator: trustedIssuerValidator,
289-
}, nil
291+
configuredReconciler: configuredReconciler,
292+
configuredClients: desiredClients,
293+
configuredLeaseID: configuredLeaseID,
294+
}
295+
if configuredReconciler != nil {
296+
srv.startConfiguredClientLeaseRenewal()
297+
}
298+
return srv, nil
299+
}
300+
301+
// startConfiguredClientLeaseRenewal starts a background worker that renews
302+
// s.configuredLeaseID on a ticker so a healthy replica's lease does not
303+
// expire and let another replica prune its still-active configuration. The
304+
// worker stops, and its done channel closes, once s.configuredCancel is
305+
// called (from Close).
306+
func (s *server) startConfiguredClientLeaseRenewal() {
307+
renewCtx, cancel := context.WithCancel(context.Background())
308+
s.configuredCancel = cancel
309+
s.configuredDone = make(chan struct{})
310+
done := s.configuredDone
311+
go func() {
312+
ticker := time.NewTicker(10 * time.Second)
313+
defer ticker.Stop()
314+
defer close(done)
315+
for {
316+
select {
317+
case <-renewCtx.Done():
318+
return
319+
case <-ticker.C:
320+
err := s.configuredReconciler.ReconcileConfiguredClients(renewCtx, s.configuredLeaseID, s.configuredClients)
321+
if err != nil {
322+
slog.Warn("configured client lease renewal failed", "error", err)
323+
}
324+
}
325+
}
326+
}()
327+
}
328+
329+
// setupConfiguredClients decorates storage for SPIFFE, then reconciles the
330+
// desired set of operator-configured (delegate) clients against durable
331+
// storage if the backend supports it, falling back to one-shot delegate
332+
// registration otherwise. It returns the (possibly wrapped) storage, the
333+
// reconciler the caller must use for lease renewal and release (nil if the
334+
// backend doesn't support reconciliation), the lease ID that reconciliation
335+
// claimed, and the desired client set for later renewal calls.
336+
func setupConfiguredClients(ctx context.Context, cfg Config, stor storage.Storage) (
337+
storage.Storage, storage.ConfiguredClientReconciler, string, []fosite.Client, error,
338+
) {
339+
desiredClients, err := configuredClients(cfg)
340+
if err != nil {
341+
return nil, nil, "", nil, err
342+
}
343+
stor, err = decorateStorageForSPIFFE(ctx, cfg, stor)
344+
if err != nil {
345+
return nil, nil, "", nil, err
346+
}
347+
348+
baseStore := storage.Unwrap(stor)
349+
reconciler, ok := baseStore.(storage.ConfiguredClientReconciler)
350+
if !ok {
351+
if err := registerDelegateClients(ctx, stor, cfg.DelegateClients); err != nil {
352+
return nil, nil, "", nil, err
353+
}
354+
return stor, nil, "", desiredClients, nil
355+
}
356+
357+
leaseID := uuid.NewString()
358+
if err := reconciler.ReconcileConfiguredClients(ctx, leaseID, desiredClients); err != nil {
359+
return nil, nil, "", nil, fmt.Errorf("reconcile configured clients: %w", err)
360+
}
361+
return stor, reconciler, leaseID, desiredClients, nil
362+
}
363+
364+
// configuredClients builds the desired set of operator-declared clients for
365+
// ClientRegistry reconciliation. SPIFFE static clients are deliberately
366+
// excluded: they are durably claimed as inert placeholders by
367+
// decorateStorageForSPIFFE/preflightDurableCollisions, never as the real,
368+
// token-exchange-capable client this function would otherwise build via
369+
// registry.staticClients(). Reconciling the real client here would fight
370+
// that placeholder for the same row (mismatched fingerprint, since the
371+
// placeholder carries no grant types) and, if it ever won, would durably
372+
// persist a usable client where only an inert stand-in must ever exist.
373+
func configuredClients(cfg Config) ([]fosite.Client, error) {
374+
clients := make([]fosite.Client, 0, len(cfg.DelegateClients))
375+
for _, delegateClient := range cfg.DelegateClients {
376+
client, err := registration.NewStaticDelegateClient(registration.Config{
377+
ID: delegateClient.ClientID,
378+
Secret: delegateClient.ClientSecret,
379+
GrantTypes: []string{oauthproto.GrantTypeTokenExchange},
380+
Scopes: delegateClient.Scopes,
381+
Audience: delegateClient.Audiences,
382+
})
383+
if err != nil {
384+
return nil, fmt.Errorf("failed to create delegate client %q: %w", delegateClient.ClientID, err)
385+
}
386+
clients = append(clients, client)
387+
}
388+
return clients, nil
290389
}
291390

292391
func registerDelegateClients(ctx context.Context, stor storage.Storage, delegateClients []DelegateClient) error {
@@ -537,6 +636,15 @@ func (s *server) Close() error {
537636
slog.Debug("closing OAuth authorization server")
538637
s.CloseIdleConnections()
539638
var errs []error
639+
if s.configuredCancel != nil {
640+
s.configuredCancel()
641+
<-s.configuredDone
642+
}
643+
if s.configuredReconciler != nil && s.configuredLeaseID != "" {
644+
if err := s.configuredReconciler.ReleaseConfiguredClientLease(context.Background(), s.configuredLeaseID); err != nil {
645+
errs = append(errs, fmt.Errorf("failed to release configured client lease: %w", err))
646+
}
647+
}
540648
if s.trustedIssuerValidator != nil {
541649
if err := s.trustedIssuerValidator.Close(); err != nil {
542650
errs = append(errs, fmt.Errorf("failed to shut down trusted-issuer validator: %w", err))

‎pkg/authserver/storage/memory.go‎

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ type MemoryStorage struct {
8383
// marker is needed here.
8484
clients map[string]fosite.Client
8585

86+
// configuredClients records rows explicitly owned by operator configuration.
87+
// It is separate from the client value so SPIFFE placeholder behavior is not
88+
// altered by an ownership marker.
89+
configuredClients map[string]struct{}
90+
8691
// clientOrder is the least-recently-proven-used order of clients: a
8792
// registration starts at the back, and RenewClientTTL moves a DCR-issued
8893
// client to the back again on proven use (a successful token
@@ -235,6 +240,7 @@ func WithMinClientAge(d time.Duration) MemoryStorageOption {
235240
func NewMemoryStorage(opts ...MemoryStorageOption) *MemoryStorage {
236241
s := &MemoryStorage{
237242
clients: make(map[string]fosite.Client),
243+
configuredClients: make(map[string]struct{}),
238244
authCodes: make(map[string]*timedEntry[fosite.Requester]),
239245
accessTokens: make(map[string]*timedEntry[fosite.Requester]),
240246
refreshTokens: make(map[string]*timedEntry[fosite.Requester]),
@@ -505,9 +511,62 @@ func (s *MemoryStorage) UpsertDCRIssuedClient(_ context.Context, client fosite.C
505511
s.clientOrder = slices.DeleteFunc(s.clientOrder, func(e clientOrderEntry) bool { return e.id == id })
506512
s.clientOrder = append(s.clientOrder, clientOrderEntry{id: id, touchedAt: time.Now()})
507513
s.clients[id] = client
514+
delete(s.configuredClients, id)
515+
return nil
516+
}
517+
518+
// ReconcileConfiguredClients atomically applies the complete desired configured
519+
// client set and removes stale explicitly configured clients.
520+
func (s *MemoryStorage) ReconcileConfiguredClients(_ context.Context, _ string, clients []fosite.Client) error {
521+
desired := make(map[string]fosite.Client, len(clients))
522+
for _, client := range clients {
523+
if client == nil {
524+
return fmt.Errorf("configured client is required")
525+
}
526+
if registration.DCRIssued(client) {
527+
return fmt.Errorf("configured client %q must not carry the DCR-issued marker", client.GetID())
528+
}
529+
if err := ValidateRegisterableClientID(client.GetID()); err != nil {
530+
return err
531+
}
532+
if _, exists := desired[client.GetID()]; exists {
533+
return fmt.Errorf("duplicate configured client %q", client.GetID())
534+
}
535+
desired[client.GetID()] = client
536+
}
537+
538+
s.mu.Lock()
539+
defer s.mu.Unlock()
540+
for id, client := range desired {
541+
if existing, exists := s.clients[id]; exists {
542+
if registration.DCRIssued(existing) || !fingerprintOfClient(existing).equal(fingerprintOfClient(client)) {
543+
return fmt.Errorf("%w: client %q is already registered", ErrAlreadyExists, id)
544+
}
545+
}
546+
}
547+
for id := range s.configuredClients {
548+
if _, keep := desired[id]; !keep {
549+
delete(s.clients, id)
550+
delete(s.configuredClients, id)
551+
}
552+
}
553+
for id, client := range desired {
554+
if _, exists := s.clients[id]; exists {
555+
s.clientOrder = slices.DeleteFunc(s.clientOrder, func(e clientOrderEntry) bool { return e.id == id })
556+
} else if err := s.insertClientLocked(id, client); err != nil {
557+
return err
558+
}
559+
s.clients[id] = client
560+
s.configuredClients[id] = struct{}{}
561+
s.clientOrder = slices.DeleteFunc(s.clientOrder, func(e clientOrderEntry) bool { return e.id == id })
562+
s.clientOrder = append(s.clientOrder, clientOrderEntry{id: id, touchedAt: time.Now()})
563+
}
508564
return nil
509565
}
510566

567+
// ReleaseConfiguredClientLease is a no-op for process-local storage.
568+
func (*MemoryStorage) ReleaseConfiguredClientLease(_ context.Context, _ string) error { return nil }
569+
511570
// ReconcileConfiguredClient applies an operator-declared client: creates it
512571
// if absent, idempotently replaces a matching-fingerprint configured client
513572
// (the restart-with-unchanged-config case), or refuses with ErrAlreadyExists
@@ -527,7 +586,11 @@ func (s *MemoryStorage) ReconcileConfiguredClient(_ context.Context, client fosi
527586

528587
existing, exists := s.clients[id]
529588
if !exists {
530-
return s.insertClientLocked(id, client)
589+
if err := s.insertClientLocked(id, client); err != nil {
590+
return err
591+
}
592+
s.configuredClients[id] = struct{}{}
593+
return nil
531594
}
532595
if registration.DCRIssued(existing) {
533596
return fmt.Errorf("%w: client %q is DCR-issued, refusing to overwrite with a configured client",
@@ -543,6 +606,7 @@ func (s *MemoryStorage) ReconcileConfiguredClient(_ context.Context, client fosi
543606
s.clientOrder = slices.DeleteFunc(s.clientOrder, func(e clientOrderEntry) bool { return e.id == id })
544607
s.clientOrder = append(s.clientOrder, clientOrderEntry{id: id, touchedAt: time.Now()})
545608
s.clients[id] = client
609+
s.configuredClients[id] = struct{}{}
546610
return nil
547611
}
548612

@@ -556,6 +620,7 @@ func (s *MemoryStorage) insertClientLocked(id string, client fosite.Client) erro
556620
victim := s.clientOrder[idx].id
557621
s.clientOrder = append(s.clientOrder[:idx], s.clientOrder[idx+1:]...)
558622
delete(s.clients, victim)
623+
delete(s.configuredClients, victim)
559624
slog.Debug("evicted oldest DCR-issued client registration at capacity",
560625
"client_id", victim, "max_clients", s.maxClients)
561626
} else {

‎pkg/authserver/storage/memory_test.go‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -380,7 +380,29 @@ func TestMemoryStorage_ReconcileConfiguredClient(t *testing.T) {
380380
})
381381
}
382382

383-
// TestMemoryStorage_UpsertDCRIssuedClient covers the create/replace/reject
383+
func TestMemoryStorage_ReconcileConfiguredClientsPrunesStaleRows(t *testing.T) {
384+
t.Parallel()
385+
386+
ctx := t.Context()
387+
s := NewMemoryStorage()
388+
defer s.Close()
389+
390+
keep := &mockClient{id: "keep", scopes: []string{"openid"}, public: false}
391+
stale := &mockClient{id: "stale", scopes: []string{"openid"}, public: false}
392+
dcr := dcrClient(t, "dcr")
393+
require.NoError(t, s.ReconcileConfiguredClient(ctx, keep))
394+
require.NoError(t, s.ReconcileConfiguredClient(ctx, stale))
395+
require.NoError(t, s.RegisterClient(ctx, dcr))
396+
397+
require.NoError(t, s.ReconcileConfiguredClients(ctx, "memory", []fosite.Client{keep}))
398+
_, err := s.GetClient(ctx, "stale")
399+
assert.ErrorIs(t, err, ErrNotFound)
400+
_, err = s.GetClient(ctx, "keep")
401+
require.NoError(t, err)
402+
_, err = s.GetClient(ctx, "dcr")
403+
require.NoError(t, err)
404+
}
405+
384406
// matrix UpsertDCRIssuedClient must implement: create when absent, replace
385407
// (and renew the eviction position) when the existing record is itself
386408
// DCR-issued, refuse with ErrAlreadyExists when the existing record is NOT

0 commit comments

Comments
 (0)