Drop PublicClient from DCR cache and flight keys

The original PublicClient-in-keys fix was defense-in-depth against a
collision that today's two consumers cannot reach: the embedded
authserver registers on AS-origin redirect URIs and the CLI registers
on RFC 8252 loopback redirect URIs, and the two address spaces are
disjoint. RedirectURI alone separates the public-client and
confidential-client profiles at both the persistent-cache and
singleflight layers.

Encoding PublicClient additionally would invalidate every existing
Redis-cached entry across a deployment without buying additional
protection. Drop it from DCRKey and flightKeyOf and document the
RedirectURI-disjointness invariant alongside the migration condition
for a hypothetical future consumer that brings the two address spaces
into collision.

Author: tgrunnagle

pkg/auth/dcr/resolver.go

@@ -93,32 +93,33 @@ import (
 // The two current call sites do not collide by construction — the embedded
 // authserver's redirect URI lives on the AS origin, and the CLI flow's
 // redirect URI lives on a loopback (http://localhost:{port}/callback per
-// RFC 8252 §7.3). A future consumer that defaults its redirect URI into
-// either of those spaces would silently coalesce; if a third profile is
-// added the flight key MUST gain a consumer-identifier component so a
-// collision is impossible rather than improbable.
+// RFC 8252 §7.3), so the disjoint RedirectURI address spaces separate
+// the public-client and confidential-client profiles at both the
+// singleflight and the persistent-cache layers. A future consumer that
+// defaults its redirect URI into either of those spaces would silently
+// coalesce; if a third profile is added the flight key (and persisted
+// DCRKey) MUST gain a consumer-identifier component so a collision is
+// impossible rather than improbable.
 var dcrFlight singleflight.Group
 
 // flightKeyOf canonicalises a Key into the singleflight string used by
-// dcrFlight. The "\n" separator is safe because newline is not a valid byte
-// in any of the four components: URI reference characters in Issuer and
-// RedirectURI (RFC 3986 §2), hex digits in ScopesHash (the form
-// storage.ScopesHash always emits), and a single "1"/"0" character for
-// PublicClient. Exposed as a function so tests and future inspection
-// helpers can compute the exact key the resolver would route through
-// dcrFlight without re-implementing the concatenation.
+// dcrFlight. The "\n" separator is safe because newline is not a valid
+// byte in any of the three components: URI reference characters in
+// Issuer and RedirectURI (RFC 3986 §2), and hex digits in ScopesHash
+// (the form storage.ScopesHash always emits). Exposed as a function so
+// tests and future inspection helpers can compute the exact key the
+// resolver would route through dcrFlight without re-implementing the
+// concatenation.
 //
-// PublicClient is included to mirror the persisted DCRKey: a public PKCE
-// registration and a confidential-client registration that happened to
-// share an Issuer/RedirectURI/ScopesHash tuple must not coalesce through
-// the singleflight, because a follower would receive a registration of
-// the wrong client shape from the leader.
+// PublicClient is intentionally NOT part of the flight key — the
+// dcrFlight doc above explains why: today's two consumers register on
+// disjoint RedirectURI address spaces (AS-origin vs RFC 8252 loopback),
+// so the public-client and confidential-client profiles cannot share a
+// flight key by construction. The same property protects the persisted
+// DCRKey from cross-profile coalescence; the two layers are aligned
+// rather than asymmetric.
 func flightKeyOf(key Key) string {
-	publicFlag := "0"
-	if key.PublicClient {
-		publicFlag = "1"
-	}
-	return key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash + "\n" + publicFlag
+	return key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash
 }
 
 // defaultUpstreamRedirectPath is the redirect path derived from the issuer
@@ -326,10 +327,9 @@ func ResolveCredentials(
 
 	scopes := slices.Clone(req.Scopes)
 	key := Key{
-		Issuer:       req.Issuer,
-		RedirectURI:  redirectURI,
-		ScopesHash:   storage.ScopesHash(scopes),
-		PublicClient: req.PublicClient,
+		Issuer:      req.Issuer,
+		RedirectURI: redirectURI,
+		ScopesHash:  storage.ScopesHash(scopes),
 	}
 
 	// Cache lookup short-circuits before any network I/O.

pkg/authserver/storage/redis_keys.go

@@ -97,29 +97,31 @@ func redisProviderKey(prefix, providerID, providerSubject string) string {
 // redisDCRKey generates a Redis key for a DCR credential entry, identifying the
 // (Issuer, RedirectURI, ScopesHash) tuple that DCRKey canonicalises.
 //
-// Format: "{prefix}dcr:<len(issuer)>:<issuer>:<len(redirect_uri)>:<redirect_uri>:<scopes_hash>:<public>"
+// Format: "{prefix}dcr:<len(issuer)>:<issuer>:<len(redirect_uri)>:<redirect_uri>:<scopes_hash>"
 //
 // The first two segments are length-prefixed to handle colons in RedirectURI
 // (and, for symmetry, Issuer) without ambiguity, mirroring redisProviderKey.
 // ScopesHash is expected to be a SHA-256 hex digest produced by
 // storage.ScopesHash — only [0-9a-f] and never colon-bearing — so it is
-// appended without a length prefix. The trailing PublicClient flag is the
-// literal "1" or "0" so confidential and public registrations never share a
-// cache entry even when the leading components coincide. The format is
-// robust for that domain; validateDCRCredentialsForStore (called by every
-// Store path) already rejects an empty ScopesHash, and callers are required
-// to compute the hash via storage.ScopesHash. Length-prefix collision-safety
-// is preserved on the leading segments either way.
+// appended without a length prefix. The format is robust for that domain;
+// validateDCRCredentialsForStore (called by every Store path) already
+// rejects an empty ScopesHash, and callers are required to compute the hash
+// via storage.ScopesHash. Length-prefix collision-safety is preserved on
+// the leading segments either way.
+//
+// The public-vs-confidential client distinction is intentionally NOT
+// encoded here — see DCRKey's doc for the rationale. Today's two consumers
+// register on disjoint RedirectURI address spaces (AS-origin vs RFC 8252
+// loopback) so the persisted key cannot collide across profiles. A future
+// consumer that defaults its RedirectURI into either space would need to
+// add the distinguishing component back to the key format alongside an
+// explicit migration story for existing Redis-cached entries.
 func redisDCRKey(prefix string, key DCRKey) string {
-	publicFlag := "0"
-	if key.PublicClient {
-		publicFlag = "1"
-	}
-	return fmt.Sprintf("%s%s:%d:%s:%d:%s:%s:%s",
+	return fmt.Sprintf("%s%s:%d:%s:%d:%s:%s",
 		prefix, KeyTypeDCR,
 		len(key.Issuer), key.Issuer,
 		len(key.RedirectURI), key.RedirectURI,
-		key.ScopesHash, publicFlag)
+		key.ScopesHash)
 }
 
 // redisUpstreamKey generates a Redis key for a per-provider upstream token entry.

pkg/authserver/storage/redis_test.go

@@ -1951,7 +1951,7 @@ func TestRedisKeyGeneration(t *testing.T) {
 		assert.Equal(t, "test:auth:reqid:access:req-123", result)
 	})
 
-	t.Run("redisDCRKey confidential", func(t *testing.T) {
+	t.Run("redisDCRKey", func(t *testing.T) {
 		t.Parallel()
 		result := redisDCRKey("test:auth:", DCRKey{
 			Issuer:      "https://thv.example.com",
@@ -1960,20 +1960,7 @@ func TestRedisKeyGeneration(t *testing.T) {
 		})
 		// 23 = len("https://thv.example.com"), 38 = len("https://thv.example.com/oauth/callback")
 		assert.Equal(t,
-			"test:auth:dcr:23:https://thv.example.com:38:https://thv.example.com/oauth/callback:abc123:0",
-			result)
-	})
-
-	t.Run("redisDCRKey public", func(t *testing.T) {
-		t.Parallel()
-		result := redisDCRKey("test:auth:", DCRKey{
-			Issuer:       "https://thv.example.com",
-			RedirectURI:  "https://thv.example.com/oauth/callback",
-			ScopesHash:   "abc123",
-			PublicClient: true,
-		})
-		assert.Equal(t,
-			"test:auth:dcr:23:https://thv.example.com:38:https://thv.example.com/oauth/callback:abc123:1",
+			"test:auth:dcr:23:https://thv.example.com:38:https://thv.example.com/oauth/callback:abc123",
 			result)
 	})
 }
@@ -2024,20 +2011,6 @@ func TestRedisDCRKey_Distinct(t *testing.T) {
 			a:    mk("https://idp.example.com", "https://x.example.com:443/cb", "h1"),
 			b:    mk("https://idp.example.com", "https://x.example.com/cb:443", "h1"),
 		},
-		{
-			// Public-client and confidential-client registrations for the
-			// same upstream/scopes/redirect must not share a cache entry.
-			// Pins the trailing PublicClient flag in the Redis key.
-			name: "different PublicClient flag",
-			a: DCRKey{
-				Issuer: "https://idp.example.com", RedirectURI: "https://x/cb", ScopesHash: "h1",
-				PublicClient: false,
-			},
-			b: DCRKey{
-				Issuer: "https://idp.example.com", RedirectURI: "https://x/cb", ScopesHash: "h1",
-				PublicClient: true,
-			},
-		},
 	}
 
 	for _, tc := range tests {

pkg/authserver/storage/types.go

@@ -128,12 +128,20 @@ type DCRKey struct {
 	//     issuer, because the CLI has no separate local issuer of its own.
 	// The cache is keyed by this value because two different consumers
 	// registering against the same upstream are distinct OAuth clients and
-	// must not share credentials. The (Issuer, RedirectURI, ScopesHash,
-	// PublicClient) tuple keeps the two consumer profiles' entries apart
-	// via the RedirectURI component (the embedded authserver registers an
-	// AS-origin callback while the CLI registers a loopback callback), so a
-	// collision between profiles is impossible by construction even when
-	// the upstream is the same.
+	// must not share credentials. The (Issuer, RedirectURI, ScopesHash)
+	// tuple keeps the two consumer profiles' entries apart via the
+	// RedirectURI component (the embedded authserver registers an
+	// AS-origin callback while the CLI registers a loopback callback per
+	// RFC 8252 §7.3 — the two address spaces are disjoint), so a collision
+	// between profiles is impossible by construction even when the
+	// upstream is the same. Public-client vs confidential-client
+	// separation rides on that same RedirectURI disjointness at both the
+	// persistent-cache and in-process singleflight layers; encoding it on
+	// the key would invalidate every existing Redis-cached entry across a
+	// deployment without buying additional protection. If a future
+	// consumer brings the two address spaces into collision the key
+	// format must gain a consumer-identifier component alongside an
+	// explicit migration story.
 	Issuer string
 
 	// RedirectURI is the redirect URI registered with the upstream
@@ -148,19 +156,6 @@ type DCRKey struct {
 	// hand at call sites; the canonical form must be a single source of truth
 	// so the key matches across processes and backends.
 	ScopesHash string
-
-	// PublicClient indicates the registration was issued for a public
-	// (PKCE) client with token_endpoint_auth_method=none, as opposed to a
-	// confidential client with a shared secret. Including this in the key
-	// is defense-in-depth: it makes the "public vs confidential
-	// registrations must not share cache entries" property structural
-	// rather than relying on RedirectURI happening to differ between
-	// consumer profiles. Without this field, a future caller that defaulted
-	// its redirect URI into the same address space as an existing consumer
-	// (and asked for a public registration where the cached entry was
-	// confidential, or vice versa) would silently receive a wrong-shape
-	// resolution.
-	PublicClient bool
 }
 
 // ScopesHash returns the SHA-256 hex digest of the canonical OAuth scope set,