diff --git a/pkg/authserver/runner/dcr.go b/pkg/authserver/runner/dcr.go index 092af89911..b097bb7d5a 100644 --- a/pkg/authserver/runner/dcr.go +++ b/pkg/authserver/runner/dcr.go @@ -5,8 +5,6 @@ package runner import ( "context" - "crypto/sha256" - "encoding/hex" "errors" "fmt" "log/slog" @@ -15,13 +13,13 @@ import ( "regexp" "runtime/debug" "slices" - "sort" "strings" "time" "golang.org/x/sync/singleflight" "github.com/stacklok/toolhive/pkg/authserver" + "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/authserver/upstream" "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" @@ -229,34 +227,12 @@ func applyResolutionToOAuth2Config(cfg *upstream.OAuth2Config, res *DCRResolutio cfg.ClientSecret = res.ClientSecret } -// scopesHash returns the SHA-256 hex digest of the canonical scope set. -// -// Canonicalisation: -// 1. Sort ascending so the digest is order-insensitive — e.g. -// []string{"openid", "profile"} and []string{"profile", "openid"} hash to -// the same value. -// 2. Deduplicate so that []string{"openid"} and []string{"openid", "openid"} -// hash to the same value. An OAuth scope set is a set, not a multiset -// (RFC 6749 §3.3), and without deduplication a caller that accidentally -// duplicated a scope would miss cache entries and trigger redundant -// RFC 7591 registrations. -// 3. Join with newlines (a character not valid in OAuth scope tokens per -// RFC 6749 §3.3) to avoid collision between e.g. ["ab", "c"] and -// ["a", "bc"]. -func scopesHash(scopes []string) string { - sorted := slices.Clone(scopes) - sort.Strings(sorted) - sorted = slices.Compact(sorted) - - h := sha256.New() - for i, s := range sorted { - if i > 0 { - _, _ = h.Write([]byte("\n")) - } - _, _ = h.Write([]byte(s)) - } - return hex.EncodeToString(h.Sum(nil)) -} +// scopesHash is a runner-package shorthand for storage.ScopesHash, kept so the +// resolver and its tests can reference the canonical hash function without an +// explicit storage. qualifier on every call site. The canonical implementation +// lives in the storage package next to DCRKey so any future backend hashes +// keys identically. +var scopesHash = storage.ScopesHash // Step identifiers for structured error logs emitted by the caller of // resolveDCRCredentials. These values flow through the "step" attribute so diff --git a/pkg/authserver/runner/dcr_store.go b/pkg/authserver/runner/dcr_store.go index 74b6009082..3fa1707057 100644 --- a/pkg/authserver/runner/dcr_store.go +++ b/pkg/authserver/runner/dcr_store.go @@ -8,43 +8,23 @@ import ( "fmt" "sync" "time" + + "github.com/stacklok/toolhive/pkg/authserver/storage" ) // dcrStaleAgeThreshold is the age beyond which a cached DCR resolution is // considered stale and logged as such by higher-level wiring. The store itself // does not expire or evict entries — RFC 7591 client registrations are -// long-lived and are only purged by explicit RFC 7592 deregistration. This -// threshold is consumed by Step 2g observability logs introduced in the next -// PR in the DCR stack (sub-issue C, #5039); 5042 only defines the constant -// so the consumer can land without a cross-PR cycle. -// -//nolint:unused // consumed by lookupCachedResolution in #5039 +// long-lived and are only purged by explicit RFC 7592 deregistration. const dcrStaleAgeThreshold = 90 * 24 * time.Hour -// DCRKey is the canonical lookup key for a DCR resolution. The tuple is -// designed so a future Redis-backed store can serialise it into a single key -// segment (Phase 3) without redefining the canonical form. ScopesHash rather -// than the raw scope slice is used so the key is comparable and order- -// insensitive. -type DCRKey struct { - // Issuer is *this* auth server's issuer identifier — the local issuer - // of the embedded authorization server that performed the registration, - // NOT the upstream's. The cache is keyed by this value because two - // different local issuers registering against the same upstream are - // distinct OAuth clients and must not share credentials. The upstream's - // issuer is used only for RFC 8414 §3.3 metadata verification inside - // the resolver and is not part of the cache key. - Issuer string - - // RedirectURI is the redirect URI registered with the upstream - // authorization server. Lives on the local issuer's origin since it is - // where the upstream sends the user back to us after authentication. - RedirectURI string - - // ScopesHash is the SHA-256 hex digest of the sorted scope list. - // See scopesHash in dcr.go for the canonical form. - ScopesHash string -} +// DCRKey is a re-export of storage.DCRKey, kept as a package-local alias so +// existing runner-side callers continue to compile against runner.DCRKey +// while the canonical definition lives in pkg/authserver/storage. The +// canonical form (and its ScopesHash constructor) MUST live in a single place +// so any future Redis backend hashes keys identically to the in-memory +// backend; see storage.DCRKey for the field documentation. +type DCRKey = storage.DCRKey // DCRCredentialStore caches RFC 7591 Dynamic Client Registration resolutions // keyed by the (Issuer, RedirectURI, ScopesHash) tuple. Implementations must diff --git a/pkg/authserver/runner/dcr_store_test.go b/pkg/authserver/runner/dcr_store_test.go index ca40d51f8c..b3209e8834 100644 --- a/pkg/authserver/runner/dcr_store_test.go +++ b/pkg/authserver/runner/dcr_store_test.go @@ -168,75 +168,11 @@ func TestInMemoryDCRCredentialStore_GetReturnsDefensiveCopy(t *testing.T) { assert.Equal(t, "orig", refetched.ClientID) } -func TestScopesHash_StableAcrossPermutation(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - a, b []string - }{ - { - name: "two-element permutation", - a: []string{"openid", "profile"}, - b: []string{"profile", "openid"}, - }, - { - name: "three-element permutation", - a: []string{"openid", "profile", "email"}, - b: []string{"email", "openid", "profile"}, - }, - { - // OAuth scope sets are sets, not multisets (RFC 6749 §3.3). - // scopesHash deduplicates before hashing so a caller who - // accidentally repeats a scope still hits the cache entry - // keyed under the canonical set. - name: "single element equals double element duplicate", - a: []string{"openid"}, - b: []string{"openid", "openid"}, - }, - { - name: "three-element with duplicate equals two-element unique", - a: []string{"openid", "profile", "openid"}, - b: []string{"openid", "profile"}, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - assert.Equal(t, scopesHash(tc.a), scopesHash(tc.b)) - }) - } -} - -func TestScopesHash_DistinctForDistinctScopes(t *testing.T) { - t.Parallel() - - a := scopesHash([]string{"openid"}) - b := scopesHash([]string{"openid", "profile"}) - c := scopesHash([]string{"profile"}) - d := scopesHash(nil) - e := scopesHash([]string{}) - - // Non-empty distinct sets produce distinct hashes. - assert.NotEqual(t, a, b) - assert.NotEqual(t, a, c) - assert.NotEqual(t, b, c) - assert.NotEqual(t, a, d) - // nil and empty slice canonicalise to the same hash (both sort-then-join - // to the empty canonical form). - assert.Equal(t, d, e) -} - -func TestScopesHash_NoCollisionFromBoundaryJoin(t *testing.T) { - t.Parallel() - - // Without a delimiter that cannot appear inside a scope value, - // ["ab", "c"] and ["a", "bc"] would collide. This test exists to - // prevent a regression if the canonical form is ever simplified. - h1 := scopesHash([]string{"ab", "c"}) - h2 := scopesHash([]string{"a", "bc"}) - assert.NotEqual(t, h1, h2) -} +// Tests for the canonical scopes-hash form live next to the canonical +// implementation in pkg/authserver/storage/memory_test.go (TestScopesHash_*). +// The runner-package binding `scopesHash = storage.ScopesHash` would only +// re-exercise the same code, so duplicating the suite here would be redundant +// per .claude/rules/testing.md. // TestInMemoryDCRCredentialStore_ConcurrentAccess fans out N goroutines // performing alternating Put / Get against overlapping and disjoint keys, diff --git a/pkg/authserver/storage/memory.go b/pkg/authserver/storage/memory.go index 3d5a222e59..2f69de391e 100644 --- a/pkg/authserver/storage/memory.go +++ b/pkg/authserver/storage/memory.go @@ -94,6 +94,15 @@ type MemoryStorage struct { // This enables O(1) lookup during authentication callbacks. providerIdentities map[string]*ProviderIdentity + // dcrCredentials maps DCRKey -> DCRCredentials for RFC 7591 Dynamic Client + // Registration credentials. Entries are intentionally excluded from the + // periodic cleanupExpired loop: DCR registrations are long-lived and the + // authoritative expiry signal is RFC 7591 client_secret_expires_at, which + // is honored at read time by callers (and by the future Redis backend's + // SetEX TTL). The map is bounded by the operator-configured upstream + // count, so unbounded growth is not a concern. + dcrCredentials map[DCRKey]*DCRCredentials + // cleanupInterval is how often the background cleanup runs cleanupInterval time.Duration @@ -129,6 +138,7 @@ func NewMemoryStorage(opts ...MemoryStorageOption) *MemoryStorage { clientAssertionJWTs: make(map[string]time.Time), users: make(map[string]*User), providerIdentities: make(map[string]*ProviderIdentity), + dcrCredentials: make(map[DCRKey]*DCRCredentials), cleanupInterval: DefaultCleanupInterval, stopCleanup: make(chan struct{}), cleanupDone: make(chan struct{}), @@ -1187,6 +1197,73 @@ func (s *MemoryStorage) GetUserProviderIdentities(_ context.Context, userID stri return identities, nil } +// ----------------------- +// DCR Credentials Storage +// ----------------------- + +// cloneDCRCredentials returns a field-by-field copy of c, or nil if c is nil. +// All fields are values (no slices, maps, or pointers), so a shallow copy is +// sufficient — adding a new reference-typed field requires updating this +// helper to deep-copy that field. +func cloneDCRCredentials(c *DCRCredentials) *DCRCredentials { + if c == nil { + return nil + } + cp := *c + return &cp +} + +// StoreDCRCredentials persists DCR credentials for the given key. +// The credentials are stored under their own Key field; callers must populate +// it before calling. A defensive copy is made so subsequent caller mutations +// do not affect persisted state. +// +// Overwrites any existing entry for the same Key. The in-memory backend +// applies no native TTL — DCR registrations are long-lived and bounded by +// the operator-configured upstream count, and ClientSecretExpiresAt is +// retained verbatim for callers to re-check on read (see the interface +// docstring's "TTL handling" section). +// +// Rejects nil creds and an unpopulated Key (empty Issuer or empty +// RedirectURI). An empty ScopesHash is permitted because ScopesHash("") +// produces a non-empty canonical digest, and the empty-scope case +// (no scopes configured or discovered) is a real, valid registration. +func (s *MemoryStorage) StoreDCRCredentials(_ context.Context, creds *DCRCredentials) error { + if creds == nil { + return fosite.ErrInvalidRequest.WithHint("dcr credentials cannot be nil") + } + if creds.Key.Issuer == "" { + return fosite.ErrInvalidRequest.WithHint("dcr credentials key issuer cannot be empty") + } + if creds.Key.RedirectURI == "" { + return fosite.ErrInvalidRequest.WithHint("dcr credentials key redirect_uri cannot be empty") + } + + s.mu.Lock() + defer s.mu.Unlock() + + s.dcrCredentials[creds.Key] = cloneDCRCredentials(creds) + return nil +} + +// GetDCRCredentials retrieves DCR credentials by key. +// Returns a defensive copy; returns ErrNotFound (wrapped) on miss. +func (s *MemoryStorage) GetDCRCredentials(_ context.Context, key DCRKey) (*DCRCredentials, error) { + s.mu.RLock() + defer s.mu.RUnlock() + + entry, ok := s.dcrCredentials[key] + if !ok { + slog.Debug("dcr credentials not found", + "issuer", key.Issuer, + "redirect_uri", key.RedirectURI, + ) + return nil, fmt.Errorf("%w: %w", ErrNotFound, fosite.ErrNotFound.WithHint("DCR credentials not found")) + } + + return cloneDCRCredentials(entry), nil +} + // ----------------------- // Metrics/Stats (for testing and monitoring) // ----------------------- @@ -1204,6 +1281,7 @@ type Stats struct { ClientAssertionJWTs int Users int ProviderIdentities int + DCRCredentials int } // Stats returns current statistics about storage contents. @@ -1224,6 +1302,7 @@ func (s *MemoryStorage) Stats() Stats { ClientAssertionJWTs: len(s.clientAssertionJWTs), Users: len(s.users), ProviderIdentities: len(s.providerIdentities), + DCRCredentials: len(s.dcrCredentials), } } @@ -1234,4 +1313,5 @@ var ( _ ClientRegistry = (*MemoryStorage)(nil) _ UpstreamTokenStorage = (*MemoryStorage)(nil) _ UserStorage = (*MemoryStorage)(nil) + _ DCRCredentialStore = (*MemoryStorage)(nil) ) diff --git a/pkg/authserver/storage/memory_test.go b/pkg/authserver/storage/memory_test.go index c0791acdc8..47ff0a17f3 100644 --- a/pkg/authserver/storage/memory_test.go +++ b/pkg/authserver/storage/memory_test.go @@ -1717,3 +1717,236 @@ func TestMemoryStorage_ConcurrentAccess(t *testing.T) { }) }) } + +func TestMemoryStorage_DCRCredentials_RoundTrip(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + key := DCRKey{ + Issuer: "https://thv.example.com", + RedirectURI: "https://thv.example.com/oauth/callback", + ScopesHash: ScopesHash([]string{"openid", "profile"}), + } + creds := &DCRCredentials{ + Key: key, + ProviderName: "atlassian", + ClientID: "client-abc", + ClientSecret: "secret-xyz", + TokenEndpointAuthMethod: "client_secret_basic", + RegistrationAccessToken: "rat-123", + RegistrationClientURI: "https://idp.example.com/register/client-abc", + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + CreatedAt: time.Date(2025, 5, 1, 12, 0, 0, 0, time.UTC), + ClientSecretExpiresAt: time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC), + } + + require.NoError(t, s.StoreDCRCredentials(ctx, creds)) + + got, err := s.GetDCRCredentials(ctx, key) + require.NoError(t, err) + require.NotNil(t, got) + + // Every field round-trips, including the embedded Key. + assert.Equal(t, *creds, *got) + }) +} + +func TestMemoryStorage_DCRCredentials_DistinctKeysDoNotCollide(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + mkKey := func(issuer, redirect string, scopes []string) DCRKey { + return DCRKey{ + Issuer: issuer, + RedirectURI: redirect, + ScopesHash: ScopesHash(scopes), + } + } + entries := []*DCRCredentials{ + {Key: mkKey("https://idp-a.example.com", "https://x/cb", []string{"openid"}), ClientID: "a"}, + {Key: mkKey("https://idp-b.example.com", "https://x/cb", []string{"openid"}), ClientID: "b"}, + {Key: mkKey("https://idp-a.example.com", "https://y/cb", []string{"openid"}), ClientID: "c"}, + {Key: mkKey("https://idp-a.example.com", "https://x/cb", []string{"openid", "email"}), ClientID: "d"}, + } + for _, e := range entries { + require.NoError(t, s.StoreDCRCredentials(ctx, e)) + } + + for _, want := range entries { + got, err := s.GetDCRCredentials(ctx, want.Key) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, want.ClientID, got.ClientID) + } + }) +} + +func TestMemoryStorage_DCRCredentials_OverwriteSemantics(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + key := DCRKey{Issuer: "https://idp.example.com", RedirectURI: "https://x/cb"} + + require.NoError(t, s.StoreDCRCredentials(ctx, &DCRCredentials{Key: key, ClientID: "first"})) + require.NoError(t, s.StoreDCRCredentials(ctx, &DCRCredentials{Key: key, ClientID: "second"})) + + got, err := s.GetDCRCredentials(ctx, key) + require.NoError(t, err) + assert.Equal(t, "second", got.ClientID) + }) +} + +func TestMemoryStorage_DCRCredentials_NotFound(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + _, err := s.GetDCRCredentials(ctx, DCRKey{Issuer: "https://unknown.example.com"}) + requireNotFoundError(t, err) + }) +} + +// TestMemoryStorage_DCRCredentials_StoreInvalidInputRejected pins the +// fail-loud-on-invalid-input contract: nil creds, an empty Key.Issuer, or +// an empty Key.RedirectURI must be rejected with fosite.ErrInvalidRequest +// rather than producing a working-looking write under a partially-populated +// key. +func TestMemoryStorage_DCRCredentials_StoreInvalidInputRejected(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + creds *DCRCredentials + }{ + { + name: "nil creds", + creds: nil, + }, + { + name: "empty issuer", + creds: &DCRCredentials{ + Key: DCRKey{Issuer: "", RedirectURI: "https://x/cb"}, + }, + }, + { + name: "empty redirect_uri", + creds: &DCRCredentials{ + Key: DCRKey{Issuer: "https://idp.example.com", RedirectURI: ""}, + }, + }, + } + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + err := s.StoreDCRCredentials(ctx, tc.creds) + require.Error(t, err) + assert.ErrorIs(t, err, fosite.ErrInvalidRequest) + // Confirm the rejection did not partially populate the store. + assert.Equal(t, 0, s.Stats().DCRCredentials, + "rejected Store must not leave any entry behind") + }) + }) + } +} + +// TestMemoryStorage_DCRCredentials_GetReturnsDefensiveCopy pins the +// defensive-copy contract: a caller mutating the returned record must not +// affect persisted state. +func TestMemoryStorage_DCRCredentials_GetReturnsDefensiveCopy(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + key := DCRKey{Issuer: "https://idp.example.com", RedirectURI: "https://x/cb"} + require.NoError(t, s.StoreDCRCredentials(ctx, &DCRCredentials{Key: key, ClientID: "orig"})) + + got, err := s.GetDCRCredentials(ctx, key) + require.NoError(t, err) + got.ClientID = "tampered-by-caller" + + refetched, err := s.GetDCRCredentials(ctx, key) + require.NoError(t, err) + assert.Equal(t, "orig", refetched.ClientID) + }) +} + +// TestMemoryStorage_DCRCredentials_StoreCopyIsolatesCaller pins the +// store-side defensive-copy contract: a caller mutating the input *after* +// Store must not affect persisted state. +func TestMemoryStorage_DCRCredentials_StoreCopyIsolatesCaller(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + key := DCRKey{Issuer: "https://idp.example.com", RedirectURI: "https://x/cb"} + input := &DCRCredentials{Key: key, ClientID: "orig"} + require.NoError(t, s.StoreDCRCredentials(ctx, input)) + + input.ClientID = "tampered-after-store" + + got, err := s.GetDCRCredentials(ctx, key) + require.NoError(t, err) + assert.Equal(t, "orig", got.ClientID) + }) +} + +// TestMemoryStorage_DCRCredentials_ExcludedFromCleanupExpired pins the +// invariant that DCR entries are NOT swept by cleanupExpired. The Redis +// backend applies TTL via SetEX; the in-memory backend keeps entries for the +// process lifetime. +func TestMemoryStorage_DCRCredentials_ExcludedFromCleanupExpired(t *testing.T) { + withStorage(t, func(ctx context.Context, s *MemoryStorage) { + key := DCRKey{Issuer: "https://idp.example.com", RedirectURI: "https://x/cb"} + require.NoError(t, s.StoreDCRCredentials(ctx, &DCRCredentials{ + Key: key, + ClientID: "abc", + CreatedAt: time.Now().Add(-365 * 24 * time.Hour), + })) + + s.cleanupExpired() + + got, err := s.GetDCRCredentials(ctx, key) + require.NoError(t, err) + assert.Equal(t, "abc", got.ClientID) + }) +} + +// TestScopesHash_StableAcrossPermutationAndDuplicates pins the canonical-form +// invariants of ScopesHash. +func TestScopesHash_StableAcrossPermutationAndDuplicates(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + a, b []string + }{ + {"two-element permutation", []string{"openid", "profile"}, []string{"profile", "openid"}}, + {"three-element permutation", []string{"openid", "profile", "email"}, []string{"email", "openid", "profile"}}, + // OAuth scope sets are sets, not multisets (RFC 6749 §3.3); duplicates + // must canonicalise to the same hash. + {"single equals double duplicate", []string{"openid"}, []string{"openid", "openid"}}, + {"three with duplicate equals two unique", []string{"openid", "profile", "openid"}, []string{"openid", "profile"}}, + } + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, ScopesHash(tc.a), ScopesHash(tc.b)) + }) + } +} + +// TestScopesHash_NoCollisionFromBoundaryJoin guards against a regression in +// the canonical form: ["ab", "c"] and ["a", "bc"] must produce different +// hashes. The newline delimiter is what prevents the collision; this test +// exists to fail loudly if the canonical form is ever simplified. +func TestScopesHash_NoCollisionFromBoundaryJoin(t *testing.T) { + t.Parallel() + assert.NotEqual(t, ScopesHash([]string{"ab", "c"}), ScopesHash([]string{"a", "bc"})) +} + +// TestScopesHash_DistinctForDistinctScopes pins that distinct non-empty +// scope sets hash to distinct values, while nil and empty slice canonicalise +// to the same value (both reduce to the empty canonical form). +func TestScopesHash_DistinctForDistinctScopes(t *testing.T) { + t.Parallel() + + a := ScopesHash([]string{"openid"}) + b := ScopesHash([]string{"openid", "profile"}) + c := ScopesHash([]string{"profile"}) + d := ScopesHash(nil) + e := ScopesHash([]string{}) + + assert.NotEqual(t, a, b) + assert.NotEqual(t, a, c) + assert.NotEqual(t, b, c) + assert.NotEqual(t, a, d) + assert.Equal(t, d, e) +} diff --git a/pkg/authserver/storage/mocks/mock_storage.go b/pkg/authserver/storage/mocks/mock_storage.go index 4bb0f83dab..3654b7b784 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,ClientRegistry,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage +// mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,ClientRegistry,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage,DCRCredentialStore // // Package mocks is a generated GoMock package. @@ -19,6 +19,59 @@ import ( gomock "go.uber.org/mock/gomock" ) +// MockDCRCredentialStore is a mock of DCRCredentialStore interface. +type MockDCRCredentialStore struct { + ctrl *gomock.Controller + recorder *MockDCRCredentialStoreMockRecorder + isgomock struct{} +} + +// MockDCRCredentialStoreMockRecorder is the mock recorder for MockDCRCredentialStore. +type MockDCRCredentialStoreMockRecorder struct { + mock *MockDCRCredentialStore +} + +// NewMockDCRCredentialStore creates a new mock instance. +func NewMockDCRCredentialStore(ctrl *gomock.Controller) *MockDCRCredentialStore { + mock := &MockDCRCredentialStore{ctrl: ctrl} + mock.recorder = &MockDCRCredentialStoreMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockDCRCredentialStore) EXPECT() *MockDCRCredentialStoreMockRecorder { + return m.recorder +} + +// GetDCRCredentials mocks base method. +func (m *MockDCRCredentialStore) GetDCRCredentials(ctx context.Context, key storage.DCRKey) (*storage.DCRCredentials, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetDCRCredentials", ctx, key) + ret0, _ := ret[0].(*storage.DCRCredentials) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetDCRCredentials indicates an expected call of GetDCRCredentials. +func (mr *MockDCRCredentialStoreMockRecorder) GetDCRCredentials(ctx, key any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDCRCredentials", reflect.TypeOf((*MockDCRCredentialStore)(nil).GetDCRCredentials), ctx, key) +} + +// StoreDCRCredentials mocks base method. +func (m *MockDCRCredentialStore) StoreDCRCredentials(ctx context.Context, creds *storage.DCRCredentials) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "StoreDCRCredentials", ctx, creds) + ret0, _ := ret[0].(error) + return ret0 +} + +// StoreDCRCredentials indicates an expected call of StoreDCRCredentials. +func (mr *MockDCRCredentialStoreMockRecorder) StoreDCRCredentials(ctx, creds any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StoreDCRCredentials", reflect.TypeOf((*MockDCRCredentialStore)(nil).StoreDCRCredentials), ctx, creds) +} + // MockPendingAuthorizationStorage is a mock of PendingAuthorizationStorage interface. type MockPendingAuthorizationStorage struct { ctrl *gomock.Controller diff --git a/pkg/authserver/storage/types.go b/pkg/authserver/storage/types.go index f57d752ef9..3c422c3085 100644 --- a/pkg/authserver/storage/types.go +++ b/pkg/authserver/storage/types.go @@ -16,11 +16,15 @@ // OAuth authorization server. package storage -//go:generate mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,ClientRegistry,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage +//go:generate mockgen -destination=mocks/mock_storage.go -package=mocks -source=types.go Storage,PendingAuthorizationStorage,ClientRegistry,UpstreamTokenStorage,UpstreamTokenRefresher,UserStorage,DCRCredentialStore import ( "context" + "crypto/sha256" + "encoding/hex" "errors" + "slices" + "sort" "time" "github.com/ory/fosite" @@ -107,6 +111,167 @@ func (t *UpstreamTokens) IsExpired(now time.Time) bool { return !t.ExpiresAt.IsZero() && now.After(t.ExpiresAt) } +// DCRKey is the canonical lookup key for a DCR registration. The tuple is +// designed so that any backend (in-memory or Redis) serialises it identically +// without redefining the canonical form. ScopesHash is used rather than a raw +// scope slice so the key is comparable, fixed-size, and order-insensitive. +// +// The key lives in the storage package because both MemoryStorage and the +// future Redis backend must hash keys identically; keeping the canonical form +// next to the persistence implementations prevents drift. +type DCRKey struct { + // Issuer is *this* auth server's issuer identifier — the local issuer + // of the embedded authorization server that performed the registration, + // NOT the upstream's. The cache is keyed by this value because two + // different local issuers registering against the same upstream are + // distinct OAuth clients and must not share credentials. + Issuer string + + // RedirectURI is the redirect URI registered with the upstream + // authorization server. Lives on the local issuer's origin since it is + // where the upstream sends the user back after authentication. + RedirectURI string + + // ScopesHash is the SHA-256 hex digest of the sorted, deduplicated scope + // list. Use ScopesHash() to compute this value — do NOT hash scopes by + // hand at call sites; the canonical form must be a single source of truth + // so the key matches across processes and backends. + ScopesHash string +} + +// ScopesHash returns the SHA-256 hex digest of the canonical OAuth scope set, +// suitable for use as DCRKey.ScopesHash. +// +// Canonicalisation: +// 1. Sort ascending so the digest is order-insensitive — e.g. +// []string{"openid", "profile"} and []string{"profile", "openid"} hash to +// the same value. +// 2. Deduplicate so that []string{"openid"} and []string{"openid", "openid"} +// hash to the same value. An OAuth scope set is a set, not a multiset +// (RFC 6749 §3.3), and without deduplication a caller that accidentally +// duplicated a scope would miss cache entries and trigger redundant +// RFC 7591 registrations. +// 3. Join with newlines (a character not valid in OAuth scope tokens per +// RFC 6749 §3.3) to avoid collision between e.g. ["ab", "c"] and +// ["a", "bc"]. +// +// nil and empty slice both canonicalise to the same hash. +func ScopesHash(scopes []string) string { + sorted := slices.Clone(scopes) + sort.Strings(sorted) + sorted = slices.Compact(sorted) + + h := sha256.New() + for i, s := range sorted { + if i > 0 { + _, _ = h.Write([]byte("\n")) + } + _, _ = h.Write([]byte(s)) + } + return hex.EncodeToString(h.Sum(nil)) +} + +// DCRCredentials is the persisted form of an RFC 7591 Dynamic Client +// Registration result. All fields are populated from the upstream's DCR +// response. The RFC 7592 management fields (RegistrationAccessToken, +// RegistrationClientURI) are preserved verbatim so future rotation / +// management flows can use them. +// +// # Defensive copy +// +// Callers receive a defensive copy from the store. Mutations on the returned +// value do not affect persisted state, and mutations on a value passed to +// StoreDCRCredentials are not observed by subsequent reads. This matches the +// UpstreamTokens contract. +// +// # Lifetime +// +// Entries are long-lived — RFC 7591 client registrations do not expire unless +// the upstream asserts client_secret_expires_at. The in-memory backend +// retains entries for the process lifetime and is intentionally excluded from +// the periodic cleanup loop. The Redis backend (future sub-issue) applies +// TTL via SetEX when ClientSecretExpiresAt is non-zero. +type DCRCredentials struct { + // Key is the canonical cache key: (Issuer, RedirectURI, ScopesHash). + Key DCRKey + + // ProviderName is the upstream's UpstreamRunConfig.Name. Debug / audit + // only — never used as a primary key. Two upstreams with different + // ProviderName but identical Key share one credential record. + ProviderName string + + ClientID string + ClientSecret string //nolint:gosec // G117: field legitimately holds sensitive data + TokenEndpointAuthMethod string + + // RegistrationAccessToken and RegistrationClientURI are RFC 7592 fields + // captured for future management operations (rotation, deletion). + RegistrationAccessToken string //nolint:gosec // G117: field legitimately holds sensitive data + RegistrationClientURI string + + AuthorizationEndpoint string + TokenEndpoint string + + // CreatedAt is the wall-clock time at which the registration completed. + // Used to compute staleness for observability — the cache itself does + // not expire entries based on age (see ClientSecretExpiresAt for the + // authoritative expiry signal). + CreatedAt time.Time + + // ClientSecretExpiresAt is the RFC 7591 §3.2.1 client_secret_expires_at + // value converted from int64 epoch seconds to time.Time. The wire + // convention is that 0 means "the secret does not expire"; this struct + // represents that as the zero time.Time so callers can use IsZero() + // rather than special-casing 0. + // + // When non-zero, this is the authoritative signal a backend uses to TTL + // the persisted entry: the Redis backend (sub-issue 2) plumbs it through + // SetEX so the row evicts before the upstream rejects the secret at the + // token endpoint. The in-memory backend ignores this field — entries + // persist for the process lifetime and the resolver re-checks the + // expiry on read. + ClientSecretExpiresAt time.Time +} + +// DCRCredentialStore is a narrow, segregated interface for persisting +// dynamic-client-registration credentials. Both MemoryStorage and a future +// Redis-backed store implement it; an authserver backed by Redis shares DCR +// credentials across replicas and restarts. +// +// # Cross-replica limitation +// +// Sharing DCR credentials does NOT imply cross-replica session / token +// delivery. Callers that need that must still route through the proxy runner +// and (if applicable) pin sessions to a replica. +// +// # Defensive copy +// +// Implementations MUST defensively copy on both Store and Get so caller +// mutations cannot reach persisted state and vice versa, mirroring the +// UpstreamTokens contract. +// +// # TTL handling +// +// Implementations SHOULD honor a non-zero DCRCredentials.ClientSecretExpiresAt +// as a backend-level TTL when the underlying store supports one (e.g. Redis +// SetEX) so an entry evicts before the upstream rejects the secret at the +// token endpoint. Backends without a native TTL (e.g. the in-memory backend) +// retain the field verbatim and rely on the caller — typically the runner's +// resolver — to re-check expiry on read; see MemoryStorage.GetDCRCredentials. +// A zero ClientSecretExpiresAt means the upstream did not assert an expiry +// and no TTL is applied. +type DCRCredentialStore interface { + // GetDCRCredentials returns the credentials for the given key. + // Returns ErrNotFound (wrapped) if no entry exists for the key. + // The returned value is a defensive copy. + GetDCRCredentials(ctx context.Context, key DCRKey) (*DCRCredentials, error) + + // StoreDCRCredentials persists the credentials, overwriting any existing + // entry for the same Key. See the interface-level "TTL handling" section + // for the contract on ClientSecretExpiresAt. + StoreDCRCredentials(ctx context.Context, creds *DCRCredentials) error +} + // User represents a user account in the authorization server. // A user can have multiple linked provider identities. // The User.ID is used as the "sub" claim in JWTs issued by ToolHive,