Add persistent DCRCredentialStore types and memory backend

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

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

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,

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)
 )