diff --git a/CHANGELOG.md b/CHANGELOG.md index 18b6fdc..1565071 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,13 +15,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `mark3labs` module: adapter for [`mark3labs/mcp-go`](https://github.com/mark3labs/mcp-go). Wraps `*authplanehttp.Adapter` so consumers get bearer + DPoP auth, RFC 9728 PRM, and `HTTPContextFunc` integration for the mark3labs HTTP server. `HTTPContextFunc` takes `WithForwardedContextKeys(keys ...any)` and `WithContextForwarding(fn)` options to propagate values (request IDs, tracing spans, …) from the upstream request context onto the per-tool-call MCP context. See `mark3labs/docs/user-guide.md`. - `core/resource`: `Resource.PRMURL()` returns the precomputed absolute Protected Resource Metadata URL as a single infallible source of truth. - `core/resource`: `Resource.PRMConfig()` returns the Protected Resource Metadata as a typed `PRMConfig` struct, for feeding into third-party PRM-serving handlers (e.g. mark3labs/mcp-go's `NewProtectedResourceMetadataHandler`) without going through the dynamic `PRMResponse` map. +- `core/authplane`: `TokenResponse`/`IntrospectionResponse` expose the RFC 9449 §6 confirmation — `Cnf json.RawMessage` (raw `cnf`, verbatim) and `CnfJkt string` (DPoP thumbprint from `cnf.jkt`, empty when unbound). Derived at parse time and preserved across `TokenCache` hits. ### Fixed - `core/resource/verifier`: `validateHTU` compares `EscapedPath()` instead of `Path`, so an encoded `%2F` is no longer treated as equivalent to a literal `/`. The previous comparison conflated distinct request targets, weakening the RFC 9449 §4.3 `htu` binding (RFC 3986 §6.2.2.2 only permits decoding *unreserved* characters when comparing URLs). - `core/resource/verifier`: `validateHTU` strips an explicit default port (`:80` for http, `:443` for https) on both sides before comparing (RFC 9110 §7.2), so a resource configured as `http://api.example.com:80/mcp` no longer mismatches every client that signs the port-less form. - `core/resource/verifier`: `validateHTU` collapses an empty path to `/` on both sides before comparing, so a client signing a bare-origin htu (e.g. `https://host`) no longer fails against a server where every inbound `r.URL.EscapedPath()` is at least `/`. +- `core/internal/cache`: `TokenCache.Set` distinguishes a missing `expires_in` from `expires_in: 0` (RFC 6749 §5.1) — nil applies the default TTL, `0` is refused as born-expired instead of cached for the default hour. +- `core/internal/dpop`: `NormalizePath` upper-cases percent-encoded hex triplets (RFC 3986 §6.2.2.1) on both comparison sides, so a proof signed with `%2f` matches a request reconstructed with `%2F`. ### Changed +- **BREAKING (pre-1.0)** `core/authplane`: `TokenResponse.ExpiresIn` changes from `int64` to `*int64`, so a response that omits `expires_in` is now `nil` rather than `0`. This distinguishes an absent field from an explicit `expires_in: 0` (RFC 6749 §5.1 permits a deliberately-expired one-shot token) and aligns the Go shape with the optional `expires_in` wire contract (absent vs. `0` vs. positive). **Migration:** any caller reading `resp.ExpiresIn` directly (arithmetic, comparison, formatting) must dereference and nil-check; treat `nil` as "apply your default" and `*v == 0` as "already expired". - **BREAKING (pre-1.0)** `http`: DPoP `htu` reconstruction in the `net/http` adapter no longer reads the inbound `Host` header, `r.TLS`, or `r.URL.RawQuery`. Both `Host` and `r.TLS` are proxy-controlled and would otherwise let a misconfigured edge — or an attacker forging `Host` — shift the `htu` binding to a different origin or downgrade `https` to `http` behind a TLS-terminating proxy. The adapter now sources scheme + authority from the operator-configured resource URI and contributes only the request path in raw `EscapedPath` form (query and fragment are dropped per RFC 9449 §4.3 #5). **Migration:** mount the middleware **before** any `http.StripPrefix` so `r.URL.EscapedPath()` still reflects the path the client signed; apps relying on `Host`-derived htu (or `r.TLS`-derived scheme) will see proof rejections until the resource URI is corrected to match the canonical origin. - `core/resource/verifier`: `(*VerifiedClaims).RequireScope` delegates to `RequireScopes`, so the singular helper also emits the enriched `required scope "X"; token has scopes: …` `error_description`. Behaviour (`errors.Is(err, ErrInsufficientScope)`, 403 status, `scope="…"` parameter) is unchanged; only the error string is enriched. - **BREAKING** `core/resource/verifier`: `DPoPContext` no longer exposes the raw proof through a public field — the previous `DPoPContext.Proof string` is replaced with an unexported slice plus the `(*DPoPContext).Proof()` accessor and the `NewDPoPContext` constructor. Route through the factory so RFC 9449 §4.3 #1 enforcement stays single-source and invalid (multi-proof) states stay unconstructable. diff --git a/core/authplane/client.go b/core/authplane/client.go index 356d624..e2e1f21 100644 --- a/core/authplane/client.go +++ b/core/authplane/client.go @@ -6,6 +6,7 @@ package authplane import ( "context" + "encoding/json" "errors" "fmt" "os" @@ -187,11 +188,26 @@ func (c *Client) currentMetadata(ctx context.Context) (*metadata.ASMetadata, err } func (c *Client) tokenResponseFromCache(entry *cache.CacheEntry) *TokenResponse { + // Clone Cnf so a caller mutating resp.Cnf bytes cannot corrupt the + // shared backing slice held in the cache entry. + var cnf json.RawMessage + if len(entry.Cnf) > 0 { + cnf = append(json.RawMessage(nil), entry.Cnf...) + } + // Same reason for ExpiresIn: a caller writing through `*resp.ExpiresIn` + // would otherwise mutate the cached entry's lifetime in place. + var expiresIn *int64 + if entry.ExpiresIn != nil { + v := *entry.ExpiresIn + expiresIn = &v + } return &TokenResponse{ AccessToken: entry.AccessToken, TokenType: entry.TokenType, - ExpiresIn: entry.ExpiresIn, + ExpiresIn: expiresIn, Scope: entry.Scope, + Cnf: cnf, + CnfJkt: entry.CnfJkt, } } @@ -237,7 +253,7 @@ func (c *Client) ClientCredentials(ctx context.Context, scopes, resources []stri } // Cache the token. - c.tokenCache.Set(cacheKey, resp.AccessToken, resp.TokenType, resp.ExpiresIn, resp.Scope) + c.tokenCache.Set(cacheKey, resp.AccessToken, resp.TokenType, resp.ExpiresIn, resp.Scope, resp.Cnf, resp.CnfJkt) return resp, nil } @@ -257,7 +273,7 @@ func (c *Client) TokenExchange(ctx context.Context, opts TokenExchangeInput) (*T return nil, err } - c.tokenCache.Set(cacheKey, resp.AccessToken, resp.TokenType, resp.ExpiresIn, resp.Scope) + c.tokenCache.Set(cacheKey, resp.AccessToken, resp.TokenType, resp.ExpiresIn, resp.Scope, resp.Cnf, resp.CnfJkt) return resp, nil } diff --git a/core/conformancetests/rfc6749_test.go b/core/conformancetests/rfc6749_test.go index 44f677f..68413ee 100644 --- a/core/conformancetests/rfc6749_test.go +++ b/core/conformancetests/rfc6749_test.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -37,8 +38,12 @@ func TestRFC6749ClientCredentialsSuccessResponse(t *testing.T) { if resp.TokenType != "Bearer" { t.Errorf("token_type = %q, want %q", resp.TokenType, "Bearer") } - if resp.ExpiresIn != 3600 { - t.Errorf("expires_in = %d, want %d", resp.ExpiresIn, 3600) + if resp.ExpiresIn == nil || *resp.ExpiresIn != 3600 { + got := "" + if resp.ExpiresIn != nil { + got = fmt.Sprintf("%d", *resp.ExpiresIn) + } + t.Errorf("expires_in = %s, want %d", got, 3600) } } diff --git a/core/internal/cache/token_cache.go b/core/internal/cache/token_cache.go index 212d0bc..d67b616 100644 --- a/core/internal/cache/token_cache.go +++ b/core/internal/cache/token_cache.go @@ -1,6 +1,7 @@ package cache import ( + "encoding/json" "sort" "strings" "sync" @@ -11,9 +12,27 @@ import ( type CacheEntry struct { //nolint:revive // CacheEntry is the established name used across the SDK AccessToken string TokenType string - ExpiresIn int64 - Scope string - ExpiresAt time.Time + + // ExpiresIn mirrors the wire field's tri-state (nil = AS omitted it, + // `*v > 0` = AS-issued lifetime). `*v == 0` never reaches the cache — + // Set refuses it — so callers reading from cache see a sentinel that + // always round-trips to the AS-issued shape. + ExpiresIn *int64 + + Scope string + ExpiresAt time.Time + + // Cnf is the raw RFC 9449 §6.1 confirmation object the AS bound to + // the token (when DPoP-sender-constrained). Persisted so callers + // served from cache see the same sender-constrained shape as on the + // first miss — without this, every cache hit silently degraded the + // apparent security model of a DPoP-bound token to bearer-only. + Cnf json.RawMessage + + // CnfJkt is the DPoP key thumbprint at `cnf.jkt`. Stored alongside + // `Cnf` so downstream code reading the convenience accessor cannot + // mistake a sender-constrained cache hit for an unbound bearer. + CnfJkt string } // TokenCache is a thread-safe in-memory cache for OAuth access tokens, keyed by @@ -56,12 +75,31 @@ func (c *TokenCache) Get(key string) *CacheEntry { return entry } -// Set stores a token in the cache under key. If the effective TTL (expiresIn minus -// ttlBuffer) is <= 0 the entry is not stored. -func (c *TokenCache) Set(key, accessToken, tokenType string, expiresIn int64, scope string) { - ttl := time.Duration(expiresIn) * time.Second - if expiresIn <= 0 { +// Set stores a token in the cache under key. +// +// `expiresIn` is tri-state: +// +// - nil ⇒ the AS omitted `expires_in`; apply `defaultTTL`. +// - `*v == 0` ⇒ RFC 6749 §5.1 explicit zero (one-shot, born-expired); +// refuse to store so the next Get is a miss instead of a stale hit. +// - `*v > 0` ⇒ honor `*v` seconds. +// +// The effective TTL (chosen value minus `ttlBuffer`) must be > 0; otherwise +// the entry is not stored. +// +// `cnf` and `cnfJkt` carry the RFC 9449 §6.1 confirmation through the cache so +// a DPoP-bound token retrieved from cache reports its binding to downstream +// callers instead of silently degrading to a bearer-only shape. Both may be +// zero-valued for bearer tokens. +func (c *TokenCache) Set(key, accessToken, tokenType string, expiresIn *int64, scope string, cnf json.RawMessage, cnfJkt string) { + var ttl time.Duration + switch { + case expiresIn == nil: ttl = c.defaultTTL + case *expiresIn == 0: + return + default: + ttl = time.Duration(*expiresIn) * time.Second } ttl -= c.ttlBuffer if ttl <= 0 { @@ -74,6 +112,8 @@ func (c *TokenCache) Set(key, accessToken, tokenType string, expiresIn int64, sc ExpiresIn: expiresIn, Scope: scope, ExpiresAt: time.Now().Add(ttl), + Cnf: cnf, + CnfJkt: cnfJkt, } c.mu.Unlock() } diff --git a/core/internal/cache/token_cache_test.go b/core/internal/cache/token_cache_test.go index 36f0d56..00cd5c5 100644 --- a/core/internal/cache/token_cache_test.go +++ b/core/internal/cache/token_cache_test.go @@ -1,13 +1,16 @@ package cache import ( + "encoding/json" "testing" "time" ) +func int64Ptr(v int64) *int64 { return &v } + func TestTokenCache_SetGet_RoundTrip(t *testing.T) { tc := NewTokenCache(30*time.Second, 3600*time.Second) - tc.Set("key1", "token-abc", "Bearer", 3600, "read write") + tc.Set("key1", "token-abc", "Bearer", int64Ptr(3600), "read write", nil, "") entry := tc.Get("key1") if entry == nil { t.Fatal("expected entry") @@ -26,7 +29,7 @@ func TestTokenCache_SetGet_RoundTrip(t *testing.T) { func TestTokenCache_Get_Expired(t *testing.T) { tc := NewTokenCache(0, 3600*time.Second) // Set with very short TTL - tc.Set("expiring", "token", "Bearer", 1, "") + tc.Set("expiring", "token", "Bearer", int64Ptr(1), "", nil, "") time.Sleep(1100 * time.Millisecond) if tc.Get("expiring") != nil { t.Error("expected nil for expired entry") @@ -36,7 +39,7 @@ func TestTokenCache_Get_Expired(t *testing.T) { func TestTokenCache_TTLBuffer(t *testing.T) { // Buffer of 50s, expires_in=60s → effective TTL = 10s tc := NewTokenCache(50*time.Second, 3600*time.Second) - tc.Set("buffered", "token", "Bearer", 60, "") + tc.Set("buffered", "token", "Bearer", int64Ptr(60), "", nil, "") entry := tc.Get("buffered") if entry == nil { t.Fatal("expected entry (10s effective TTL)") @@ -51,29 +54,45 @@ func TestTokenCache_TTLBuffer(t *testing.T) { func TestTokenCache_SkipCaching_TTLTooSmall(t *testing.T) { // Buffer of 100s, expires_in=50s → effective TTL = -50s → skip caching tc := NewTokenCache(100*time.Second, 3600*time.Second) - tc.Set("skip", "token", "Bearer", 50, "") + tc.Set("skip", "token", "Bearer", int64Ptr(50), "", nil, "") if tc.Get("skip") != nil { t.Error("should not cache when effective TTL <= 0") } } -func TestTokenCache_DefaultTTL(t *testing.T) { +// TestTokenCache_NilExpiresIn_HonorsDefaultTTL pins the tri-state contract: +// when the AS omits `expires_in` (decoded as nil), the cache applies +// `defaultTTL` minus the buffer. +func TestTokenCache_NilExpiresIn_HonorsDefaultTTL(t *testing.T) { tc := NewTokenCache(30*time.Second, 300*time.Second) - // expires_in=0 → use default TTL (300s) minus buffer (30s) = 270s - tc.Set("default", "token", "Bearer", 0, "") + tc.Set("default", "token", "Bearer", nil, "", nil, "") entry := tc.Get("default") if entry == nil { t.Fatal("expected entry with default TTL") } + if entry.ExpiresIn != nil { + t.Errorf("ExpiresIn: expected nil to round-trip, got %d", *entry.ExpiresIn) + } remaining := time.Until(entry.ExpiresAt) if remaining < 265*time.Second || remaining > 275*time.Second { t.Errorf("expected effective TTL ~270s, got %v", remaining) } } +// TestTokenCache_ZeroExpiresIn_RefusesToStore pins the RFC 6749 §5.1 +// one-shot contract: a token the AS deliberately marks `expires_in: 0` +// is born-expired and must not be returned on the next read. +func TestTokenCache_ZeroExpiresIn_RefusesToStore(t *testing.T) { + tc := NewTokenCache(30*time.Second, 3600*time.Second) + tc.Set("oneshot", "token", "Bearer", int64Ptr(0), "", nil, "") + if tc.Get("oneshot") != nil { + t.Error("expected nil for expires_in=0 (one-shot) — refuse to store") + } +} + func TestTokenCache_Delete(t *testing.T) { tc := NewTokenCache(30*time.Second, 3600*time.Second) - tc.Set("del", "token", "Bearer", 3600, "") + tc.Set("del", "token", "Bearer", int64Ptr(3600), "", nil, "") tc.Delete("del") if tc.Get("del") != nil { t.Error("expected nil after delete") @@ -128,9 +147,9 @@ func TestCacheKey_ScopeOnly(t *testing.T) { func TestTokenCache_DeleteByAccessToken(t *testing.T) { tc := NewTokenCache(30*time.Second, 3600*time.Second) - tc.Set("key1", "token-A", "Bearer", 3600, "read") - tc.Set("key2", "token-B", "Bearer", 3600, "write") - tc.Set("key3", "token-A", "Bearer", 3600, "admin") // same token, different key + tc.Set("key1", "token-A", "Bearer", int64Ptr(3600), "read", nil, "") + tc.Set("key2", "token-B", "Bearer", int64Ptr(3600), "write", nil, "") + tc.Set("key3", "token-A", "Bearer", int64Ptr(3600), "admin", nil, "") // same token, different key tc.DeleteByAccessToken("token-A") @@ -147,7 +166,7 @@ func TestTokenCache_DeleteByAccessToken(t *testing.T) { func TestTokenCache_DeleteByAccessToken_NoMatch(t *testing.T) { tc := NewTokenCache(30*time.Second, 3600*time.Second) - tc.Set("key1", "token-X", "Bearer", 3600, "read") + tc.Set("key1", "token-X", "Bearer", int64Ptr(3600), "read", nil, "") tc.DeleteByAccessToken("no-such-token") @@ -155,3 +174,44 @@ func TestTokenCache_DeleteByAccessToken_NoMatch(t *testing.T) { t.Error("key1 should still exist") } } + +// TestTokenCache_DpopBindingSurvivesRoundTrip pins the DPoP binding +// through the cache round-trip: a DPoP-bound token must report its +// binding (both raw `Cnf` and the convenience `CnfJkt`) when served +// from cache, not silently degrade to a bearer-only shape. +func TestTokenCache_DpopBindingSurvivesRoundTrip(t *testing.T) { + tc := NewTokenCache(30*time.Second, 3600*time.Second) + cnf := json.RawMessage(`{"jkt":"thumbprint-abc"}`) + tc.Set("dpop", "dpop-bound-token", "DPoP", int64Ptr(3600), "tools/echo", cnf, "thumbprint-abc") + + entry := tc.Get("dpop") + if entry == nil { + t.Fatal("expected entry") + } + if entry.CnfJkt != "thumbprint-abc" { + t.Errorf("CnfJkt: expected %q, got %q", "thumbprint-abc", entry.CnfJkt) + } + if string(entry.Cnf) != `{"jkt":"thumbprint-abc"}` { + t.Errorf("Cnf: expected raw object, got %q", string(entry.Cnf)) + } +} + +// TestTokenCache_BearerTokenDefaultsCnfToZero pins that callers +// caching a bearer-only token (no DPoP binding) keep `Cnf` nil and +// `CnfJkt` empty so downstream code reading them cannot mistake +// the entry for sender-constrained. +func TestTokenCache_BearerTokenDefaultsCnfToZero(t *testing.T) { + tc := NewTokenCache(30*time.Second, 3600*time.Second) + tc.Set("bearer", "bearer-tok", "Bearer", int64Ptr(3600), "read", nil, "") + + entry := tc.Get("bearer") + if entry == nil { + t.Fatal("expected entry") + } + if entry.Cnf != nil { + t.Errorf("Cnf: expected nil, got %q", string(entry.Cnf)) + } + if entry.CnfJkt != "" { + t.Errorf("CnfJkt: expected empty, got %q", entry.CnfJkt) + } +} diff --git a/core/internal/dpop/htu.go b/core/internal/dpop/htu.go index 829c843..1e0a522 100644 --- a/core/internal/dpop/htu.go +++ b/core/internal/dpop/htu.go @@ -40,23 +40,61 @@ func NormalizeHost(u *url.URL) string { return u.Host } -// NormalizePath collapses an empty path to "/" to match the ts and python -// reference SDKs (`parsed.pathname || "/"` / `parsed.path or "/"`). The Go -// net/http server hands every request a path of at least "/", so the -// asymmetry only bites when an htu is built from a bare-origin URL — a -// Go-signed proof for `https://host` would otherwise fail against a request -// the server sees as `https://host/`. +// NormalizePath collapses an empty path to "/" and upper-cases the hex +// digits of percent-encoded triplets per RFC 3986 §6.2.2.1. // -// Percent-encoded triplets are intentionally NOT case-folded. RFC 3986 -// §6.2.2.1 says that the hex digits within a triplet (`%2f` vs `%2F`) -// *should* be normalized to upper-case when comparing URIs, but the verifier -// compares the path byte-for-byte. Folding case unilaterally would let the -// verifier accept a proof that a byte-exact signer rejects (and vice-versa) -// on the exact path the §4.3 binding is meant to lock down. Holding the -// byte-equality contract until an aligned RFC §6.2.2.1 pass lands. +// Empty-path collapse: the Go net/http server hands every request a +// path of at least "/", so the asymmetry only bites when an htu is +// built from a bare-origin URL — a proof signed for `https://host` +// would otherwise fail against a request the server sees as +// `https://host/`. +// +// Hex upper-casing: RFC 3986 §6.2.2.1 treats `%2f` and `%2F` as +// equivalent and recommends upper-case as the canonical form. The +// verifier (`core/resource/verifier/dpop.go`) routes both the proof's +// htu and the reconstructed request URL through `NormalizePath` before +// comparing, so canonicalizing hex casing on both sides lets a proof +// signed with `%2f` match a request whose framework emits `%2F` (or +// vice versa) without requiring identical raw casing across frameworks. +// Note: neither Go's `net/url.EscapedPath()` nor WHATWG `URL.pathname` +// rewrites the case of existing `%XX` triplets — the byte-for-byte +// equality at the comparison site is produced *here*, not by the parser. func NormalizePath(p string) string { if p == "" { return "/" } - return p + return upperHexPercent(p) +} + +// upperHexPercent rewrites every well-formed `%XX` triplet so the two +// hex digits are upper-case, leaving bytes outside such triplets +// (including a bare `%` not followed by two hex chars) untouched. +func upperHexPercent(s string) string { + if !strings.Contains(s, "%") { + return s + } + b := []byte(s) + for i := 0; i+2 < len(b); i++ { + if b[i] != '%' { + continue + } + if !isHex(b[i+1]) || !isHex(b[i+2]) { + continue + } + b[i+1] = upperHex(b[i+1]) + b[i+2] = upperHex(b[i+2]) + i += 2 + } + return string(b) +} + +func isHex(c byte) bool { + return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') +} + +func upperHex(c byte) byte { + if c >= 'a' && c <= 'f' { + return c - ('a' - 'A') + } + return c } diff --git a/core/internal/dpop/htu_test.go b/core/internal/dpop/htu_test.go index 78241a9..e0f2945 100644 --- a/core/internal/dpop/htu_test.go +++ b/core/internal/dpop/htu_test.go @@ -38,21 +38,34 @@ func TestNormalizeHost(t *testing.T) { func TestNormalizePath(t *testing.T) { tests := []struct { - in, want string + name string + in string + want string }{ - {"", "/"}, - {"/", "/"}, - {"/foo", "/foo"}, - {"/foo/bar", "/foo/bar"}, - {"/foo%2Fbar", "/foo%2Fbar"}, - // Percent-encoded triplets are preserved byte-for-byte — no - // case-folding of the hex digits. RFC 3986 §6.2.2.1 would allow - // `%2f` == `%2F`, but the verifier compares byte-by-byte so the - // §4.3 htu binding stays exact. - {"/foo%2fbar", "/foo%2fbar"}, + {"empty path collapses to slash", "", "/"}, + {"slash root unchanged", "/", "/"}, + {"plain path unchanged", "/foo", "/foo"}, + {"plain nested path unchanged", "/foo/bar", "/foo/bar"}, + {"already-upper triplet preserved", "/foo%2Fbar", "/foo%2Fbar"}, + // Lower-case hex digits in `%XX` triplets are folded to + // upper-case per RFC 3986 §6.2.2.1. A proof signed with `%2f` + // and a request seen as `%2F` must compare equal on the htu + // path; without folding, the §4.3 binding rejected otherwise- + // equivalent URIs. + {"lowercase hex folded", "/foo%2fbar", "/foo%2Fbar"}, + {"mixed-case hex folded", "/foo%aAbar", "/foo%AAbar"}, + {"multiple triplets folded", "/path%2fwith%20space%aa", "/path%2Fwith%20space%AA"}, + // A bare `%` not followed by two hex digits is left alone — + // the byte is malformed per RFC 3986 §2.1, but the verifier + // still compares byte-for-byte so we don't synthesize a triplet + // the signer didn't emit. + {"bare percent at end preserved", "/foo%", "/foo%"}, + {"percent + one hex preserved", "/foo%a", "/foo%a"}, + {"percent + non-hex preserved", "/foo%g0", "/foo%g0"}, + {"percent + hex + non-hex preserved", "/foo%ag", "/foo%ag"}, } for _, tt := range tests { - t.Run(tt.in, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { if got := NormalizePath(tt.in); got != tt.want { t.Errorf("NormalizePath(%q) = %q, want %q", tt.in, got, tt.want) } diff --git a/core/internal/oauth/client.go b/core/internal/oauth/client.go index 435cb50..381f180 100644 --- a/core/internal/oauth/client.go +++ b/core/internal/oauth/client.go @@ -192,6 +192,8 @@ func Introspect(ctx context.Context, endpoint string, auth ClientAuthentication, return nil, fmt.Errorf("introspection: invalid JSON response: %w", err) } + NormalizeCnf(&result.Cnf, &result.CnfJkt) + // Parse extra fields. var rawMap map[string]any if err := json.Unmarshal(resp.Body, &rawMap); err == nil { @@ -201,6 +203,9 @@ func Introspect(ctx context.Context, endpoint string, auth ClientAuthentication, "exp": true, "iat": true, "jti": true, // AuthPlane extensions surfaced as first-class fields (RFC 9706). "agent_id": true, "agent_chain": true, + // RFC 9449 §6.2 — confirmation. Surfaced first-class above so + // it must not also land in Extra. + "cnf": true, "cnf_jkt": true, } extra := make(map[string]any) for k, v := range rawMap { @@ -373,13 +378,43 @@ func doTokenRequest(ctx context.Context, endpoint string, auth ClientAuthenticat } // RFC 6749 §5.1 ABNF: expires_in = 1*DIGIT — negative values are malformed. - if tokenResp.ExpiresIn < 0 { - return nil, fmt.Errorf("%w: expires_in must be non-negative, got %d", ErrProtocolError, tokenResp.ExpiresIn) + if tokenResp.ExpiresIn != nil && *tokenResp.ExpiresIn < 0 { + return nil, fmt.Errorf("%w: expires_in must be non-negative, got %d", ErrProtocolError, *tokenResp.ExpiresIn) } + NormalizeCnf(&tokenResp.Cnf, &tokenResp.CnfJkt) + return &tokenResp, nil } +// NormalizeCnf inspects the raw `cnf` JSON value: when it's a JSON object, +// derives the `cnf_jkt` thumbprint from `cnf.jkt`. When it's absent, null, +// or a non-object scalar, drops both fields so a malformed AS cannot +// pollute the typed confirmation-claim shape downstream. +// +// Exported so the package-external callers that build response structs +// from cached state (e.g. `core/authplane/client.go`'s +// `tokenResponseFromCache`) can apply the same derivation. +func NormalizeCnf(cnf *json.RawMessage, cnfJkt *string) { + *cnfJkt = "" + if len(*cnf) == 0 { + return + } + var obj map[string]json.RawMessage + if err := json.Unmarshal(*cnf, &obj); err != nil || obj == nil { + // Non-object scalar or explicit `null` — drop both fields so a + // malformed AS cannot pollute the typed shape downstream. + *cnf = nil + return + } + if jktRaw, ok := obj["jkt"]; ok { + var jkt string + if err := json.Unmarshal(jktRaw, &jkt); err == nil { + *cnfJkt = jkt + } + } +} + // parseErrorResponse parses an OAuth 2.0 error response body. func parseErrorResponse(body []byte, status int) error { var errResp struct { diff --git a/core/internal/oauth/client_test.go b/core/internal/oauth/client_test.go index 7dd6201..d48869f 100644 --- a/core/internal/oauth/client_test.go +++ b/core/internal/oauth/client_test.go @@ -242,6 +242,87 @@ func TestIntrospect_Success(t *testing.T) { } } +// TestIntrospect_ExposesCnfJkt pins the RFC 9449 §6.2 surface +// end-to-end: a DPoP-bound introspection response must expose +// the raw `cnf` object (so callers can read extension members +// such as `x5t#S256`) and the derived `cnf_jkt` convenience +// accessor, and `cnf` must not be duplicated into the catch-all +// `Extra` map. +func TestIntrospect_ExposesCnfJkt(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, http.StatusOK, map[string]any{ + "active": true, + "token_type": "DPoP", + "cnf": map[string]any{"jkt": "thumbprint-abc"}, + }) + })) + defer srv.Close() + + resp, err := Introspect(context.Background(), srv.URL, testClientAuth(), testFetchSettings(), "tok", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.CnfJkt != "thumbprint-abc" { + t.Fatalf("CnfJkt = %q, want %q", resp.CnfJkt, "thumbprint-abc") + } + if string(resp.Cnf) == "" { + t.Fatal("Cnf: expected non-empty raw object") + } + // cnf must not leak into Extra — it is surfaced first-class above. + if _, dup := resp.Extra["cnf"]; dup { + t.Errorf("cnf must not appear in Extra: %#v", resp.Extra) + } +} + +// TestIntrospect_DropsNonObjectCnf pins the defensive `cnf` shape: a +// malformed AS sending `cnf` as a non-object scalar must not pollute +// the typed shape. +func TestIntrospect_DropsNonObjectCnf(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, http.StatusOK, map[string]any{ + "active": true, + "cnf": "not-an-object", + }) + })) + defer srv.Close() + + resp, err := Introspect(context.Background(), srv.URL, testClientAuth(), testFetchSettings(), "tok", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.Cnf != nil { + t.Errorf("Cnf: expected nil, got %q", string(resp.Cnf)) + } + if resp.CnfJkt != "" { + t.Errorf("CnfJkt: expected empty, got %q", resp.CnfJkt) + } +} + +// TestClientCredentials_ExposesCnfJkt pins the same RFC 9449 §6.1 +// surface for the token endpoint half (TokenResponse). +func TestClientCredentials_ExposesCnfJkt(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, http.StatusOK, map[string]any{ + "access_token": "at", + "token_type": "DPoP", + "expires_in": 3600, + "cnf": map[string]any{"jkt": "thumbprint-token"}, + }) + })) + defer srv.Close() + + resp, err := ClientCredentials(context.Background(), srv.URL, testClientAuth(), testFetchSettings(), nil, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.CnfJkt != "thumbprint-token" { + t.Fatalf("CnfJkt = %q, want %q", resp.CnfJkt, "thumbprint-token") + } + if string(resp.Cnf) == "" { + t.Fatal("Cnf: expected non-empty raw object") + } +} + func TestRevoke_Success(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) diff --git a/core/internal/oauth/types.go b/core/internal/oauth/types.go index e3c520f..e1e8f78 100644 --- a/core/internal/oauth/types.go +++ b/core/internal/oauth/types.go @@ -41,12 +41,33 @@ func (s *StringOrSlice) UnmarshalJSON(data []byte) error { // TokenResponse represents an OAuth 2.0 token endpoint response. type TokenResponse struct { - AccessToken string `json:"access_token"` - TokenType string `json:"token_type"` - ExpiresIn int64 `json:"expires_in,omitempty"` + AccessToken string `json:"access_token"` + TokenType string `json:"token_type"` + + // ExpiresIn is `*int64` so the wire shape `expires_in: 0` (RFC 6749 + // §5.1 — a deliberately-expired one-shot token) is distinguishable + // from the field being absent. Cache callers honor the AS's intent: + // nil ⇒ apply the default TTL; `*v == 0` ⇒ refuse to store. + ExpiresIn *int64 `json:"expires_in,omitempty"` + Scope string `json:"scope,omitempty"` RefreshToken string `json:"refresh_token,omitempty"` IssuedTokenType string `json:"issued_token_type,omitempty"` + + // Cnf is the raw RFC 9449 §6.1 confirmation object from the token + // response when present. Preserves extension members (`x5t#S256`, + // future RFC 9449 additions) verbatim. Nil when the AS did not emit + // `cnf`, or when the value was a non-object scalar (NormalizeCnf + // drops malformed inputs to keep the typed shape honest). + Cnf json.RawMessage `json:"cnf,omitempty"` + + // CnfJkt is the convenience accessor for the DPoP key thumbprint at + // `cnf.jkt` (RFC 9449 §6.1). Empty string when the token is not + // DPoP-bound. Always derived from `cnf.jkt` at parse time (see + // NormalizeCnf), so a wire payload that pinned a top-level `cnf_jkt` + // disagreeing with its own `cnf.jkt` cannot mint a mismatched + // thumbprint. + CnfJkt string `json:"cnf_jkt,omitempty"` } // IntrospectionResponse represents an RFC 7662 introspection response. @@ -72,6 +93,15 @@ type IntrospectionResponse struct { // when the AS does not emit it. AgentChain []string `json:"agent_chain,omitempty"` + // Cnf is the raw RFC 9449 §6.2 / RFC 7662 confirmation object. + // Same semantics as TokenResponse.Cnf — preserved verbatim when the + // AS sent a JSON object; nil otherwise. + Cnf json.RawMessage `json:"cnf,omitempty"` + + // CnfJkt is the DPoP key thumbprint at `cnf.jkt` (RFC 9449 §6.2). + // Same derivation semantics as TokenResponse.CnfJkt. + CnfJkt string `json:"cnf_jkt,omitempty"` + // Extra captures unknown claims from the introspection response. Extra map[string]any `json:"-"` } diff --git a/core/internal/oauth/types_test.go b/core/internal/oauth/types_test.go index 982acab..0244d50 100644 --- a/core/internal/oauth/types_test.go +++ b/core/internal/oauth/types_test.go @@ -140,3 +140,140 @@ func TestSentinelErrors_AreDistinct(t *testing.T) { } } } + +// TestNormalizeCnf pins the parser's confirmation-claim shape: a JSON +// object `cnf` is preserved and its `jkt` is surfaced as `cnf_jkt`; +// absent / null / non-object `cnf` collapses both fields to zero values +// so a malformed AS cannot pollute the typed shape downstream. +func TestNormalizeCnf(t *testing.T) { + tests := []struct { + name string + in json.RawMessage + wantCnf json.RawMessage + wantCnfJkt string + }{ + { + name: "object_with_jkt_derives_thumbprint", + in: json.RawMessage(`{"jkt":"abc"}`), + wantCnf: json.RawMessage(`{"jkt":"abc"}`), + wantCnfJkt: "abc", + }, + { + name: "object_with_jkt_and_extension_member_preserves_both", + in: json.RawMessage(`{"jkt":"abc","x5t#S256":"hash"}`), + wantCnf: json.RawMessage(`{"jkt":"abc","x5t#S256":"hash"}`), + wantCnfJkt: "abc", + }, + { + name: "object_without_jkt_keeps_cnf_empties_jkt", + in: json.RawMessage(`{"x5t#S256":"hash"}`), + wantCnf: json.RawMessage(`{"x5t#S256":"hash"}`), + wantCnfJkt: "", + }, + { + name: "absent_cnf_collapses_to_zero", + in: nil, + wantCnf: nil, + wantCnfJkt: "", + }, + { + name: "null_cnf_drops_both", + in: json.RawMessage(`null`), + wantCnf: nil, + wantCnfJkt: "", + }, + { + name: "non_object_scalar_cnf_is_dropped", + in: json.RawMessage(`"not-an-object"`), + wantCnf: nil, + wantCnfJkt: "", + }, + { + name: "non_object_array_cnf_is_dropped", + in: json.RawMessage(`["a","b"]`), + wantCnf: nil, + wantCnfJkt: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cnf := tt.in + var cnfJkt string + NormalizeCnf(&cnf, &cnfJkt) + if string(cnf) != string(tt.wantCnf) { + t.Errorf("Cnf: got %q, want %q", string(cnf), string(tt.wantCnf)) + } + if cnfJkt != tt.wantCnfJkt { + t.Errorf("CnfJkt: got %q, want %q", cnfJkt, tt.wantCnfJkt) + } + }) + } +} + +// TestTokenResponse_ExpiresIn_WireTriState pins the round-trip semantics +// of the `*int64` `ExpiresIn` field so a cached response re-serializes to +// the exact shape the AS sent: +// +// - missing on the wire → nil → omitted on re-marshal (omitempty). +// - `"expires_in": 0` → non-nil zero → re-marshals as 0, NOT omitted. +// - positive integer → round-trips verbatim. +// +// Locks the contract Set / cache callers rely on to distinguish a +// one-shot (RFC 6749 §5.1) token from an AS-omitted lifetime. +func TestTokenResponse_ExpiresIn_WireTriState(t *testing.T) { + tests := []struct { + name string + wire string + wantNil bool + wantVal int64 + wantOut string + }{ + { + name: "absent on wire decodes to nil and re-marshals as omitted", + wire: `{"access_token":"t","token_type":"Bearer"}`, + wantNil: true, + wantOut: `{"access_token":"t","token_type":"Bearer"}`, + }, + { + name: "explicit zero decodes to non-nil zero and re-marshals as 0", + wire: `{"access_token":"t","token_type":"Bearer","expires_in":0}`, + wantNil: false, + wantVal: 0, + wantOut: `{"access_token":"t","token_type":"Bearer","expires_in":0}`, + }, + { + name: "positive value round-trips", + wire: `{"access_token":"t","token_type":"Bearer","expires_in":3600}`, + wantNil: false, + wantVal: 3600, + wantOut: `{"access_token":"t","token_type":"Bearer","expires_in":3600}`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got TokenResponse + if err := json.Unmarshal([]byte(tt.wire), &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if tt.wantNil { + if got.ExpiresIn != nil { + t.Errorf("ExpiresIn: got %d, want nil", *got.ExpiresIn) + } + } else { + if got.ExpiresIn == nil { + t.Fatalf("ExpiresIn: got nil, want %d", tt.wantVal) + } + if *got.ExpiresIn != tt.wantVal { + t.Errorf("ExpiresIn: got %d, want %d", *got.ExpiresIn, tt.wantVal) + } + } + out, err := json.Marshal(got) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != tt.wantOut { + t.Errorf("marshal: got %q, want %q", string(out), tt.wantOut) + } + }) + } +} diff --git a/core/resource/verifier/dpop_test.go b/core/resource/verifier/dpop_test.go index 20e5c8f..5ad471b 100644 --- a/core/resource/verifier/dpop_test.go +++ b/core/resource/verifier/dpop_test.go @@ -239,25 +239,20 @@ func TestValidateDPoPProof_EncodedSlashIsNotPlainSlash(t *testing.T) { } } -// Percent-encoded triplets MUST compare case-sensitively. RFC 3986 -// §6.2.2.1 *allows* `%2f` and `%2F` to be considered equivalent, but the -// verifier deliberately compares byte-for-byte — folding case unilaterally -// would let the verifier accept a proof that a byte-exact signer rejects. -// Pin the current contract here so a future "let's just uppercase the -// triplets" patch can't land without the question being answered. -func TestValidateDPoPProof_PercentEncodedCaseIsNotFolded(t *testing.T) { +// Percent-encoded triplets compare case-insensitively after the shared +// dpop.NormalizePath helper upper-cases their hex digits per RFC 3986 +// §6.2.2.1. A proof signed with `%2f` and a request seen as `%2F` +// produce the same normalized htu path and the binding compares equal. +func TestValidateDPoPProof_PercentEncodedHexFoldedToUpper(t *testing.T) { key := newTestECKey(t) method := "GET" // Proof's htu carries lower-case `%2f`; request path carries - // upper-case `%2F`. Same byte semantics under RFC 3986 §6.2.2.1, - // but the verifier intentionally treats them as distinct to keep - // the htu binding byte-exact. + // upper-case `%2F`. Both normalize to `%2F`, so the htu binding holds. proof := generateDPoPProof(t, key, method, "https://example.com/foo%2fbar", "", nil) v := newDPoPTestVerifier(t, nil) - _, err := v.validateDPoPProof(mustCtx(t, method, "https://example.com/foo%2Fbar", proof), "") - if !errors.Is(err, ErrDPoPInvalid) { - t.Fatalf("expected ErrDPoPInvalid for %%2f vs %%2F mismatch, got: %v", err) + if _, err := v.validateDPoPProof(mustCtx(t, method, "https://example.com/foo%2Fbar", proof), ""); err != nil { + t.Fatalf("expected validation to succeed for %%2f vs %%2F (both fold to %%2F), got: %v", err) } }