diff --git a/CHANGELOG.md b/CHANGELOG.md index a848256..7f455dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- Resource and issuer identifiers are never rewritten (RFC 8414 §3.3 / RFC 9728 §3.3 require the advertised value to be *identical* to the configured one). The RFC 8414 metadata URL and the PRM well-known path are formed by pure insertion, preserving the identifier's path exactly — including any trailing slash and percent-encoded octets (escaped path) — and AS-metadata issuer comparison is now an exact string match. Identifiers are validated at construction via the new `verifier.ValidateIdentifier` (absolute http(s) URL with a host and no fragment); trailing slashes, host case, and explicit ports are legal and preserved. The OIDC discovery fallback deliberately keeps its terminating-slash trim — OIDC Discovery 1.0 §4 *concatenates* rather than inserts and mandates the trim. **Migration**: if your configured issuer or resource differs from your authorization server's actual identifier by a trailing slash, correct the config — the SDK no longer silently reconciles them. + +### Fixed +- `core/resource/verifier`: a configured issuer whose identifier legitimately ends in `/` no longer has every token rejected. The trailing slash was silently stripped by `NewTokenVerifier` and the stripped value compared against the token's `iss`, which RFC 9068 requires to carry the slash; RFC 8414 discovery now also resolves the well-known URL for such issuers correctly. + ## [0.2.0] - 2026-07-21 ### Added diff --git a/core/conformancetests/rfc8414_test.go b/core/conformancetests/rfc8414_test.go index 67efa5d..61d3e48 100644 --- a/core/conformancetests/rfc8414_test.go +++ b/core/conformancetests/rfc8414_test.go @@ -59,6 +59,30 @@ func TestRFC8414MetadataIssuerMustMatchConfiguredIssuer(t *testing.T) { if !strings.Contains(err.Error(), "issuer mismatch") { t.Errorf("expected issuer mismatch error, got: %v", err) } + + // Variant: a trailing-slash difference is equivalent per RFC 3986 §6.2.3 + // but not identical — RFC 8414 §3.3 requires identity, so it must be + // rejected. + tsSlash := metadataServerDynamic(t, func(issuer string) map[string]any { + return map[string]any{ + "issuer": issuer + "/", + "jwks_uri": "https://auth.example.com/jwks", + } + }) + + mcSlash := metadata.New(metadata.Config{ + IssuerURL: tsSlash.URL, + FetchSettings: ssrf.DevModeFetchSettings(), + }) + defer mcSlash.Close() + + _, err = mcSlash.Get(ctx) + if err == nil { + t.Fatal("expected error when metadata issuer differs only by a trailing slash") + } + if !strings.Contains(err.Error(), "issuer mismatch") { + t.Errorf("expected issuer mismatch error, got: %v", err) + } } func TestRFC8414JWKSURIRequiredForJWTValidation(t *testing.T) { diff --git a/core/conformancetests/rfc9068_test.go b/core/conformancetests/rfc9068_test.go index f803b3d..151d17b 100644 --- a/core/conformancetests/rfc9068_test.go +++ b/core/conformancetests/rfc9068_test.go @@ -129,6 +129,23 @@ func TestRFC9068IssuerMustMatch(t *testing.T) { if !errors.Is(err, verifier.ErrIssuerMismatch) { t.Errorf("expected ErrIssuerMismatch, got %v", err) } + + // Variant: a token whose iss is identical to a configured trailing-slash + // issuer must verify — the issuer is never rewritten (RFC 8414 §3.3 / + // RFC 9068 §4 identity, catalog accept variant). + slashIssuer := "https://auth.example.com/" + tvSlash, _ := newTestVerifier(t, key, "key-0", slashIssuer, "https://api.example.com") + + slashClaims := testutil.StandardClaims(slashIssuer, "https://api.example.com", "user123", "client456") + slashToken, _ := testutil.SignToken(slashClaims, key, jose.ES256, "key-0") + + verified, err := tvSlash.VerifyToken(ctx, slashToken, nil) + if err != nil { + t.Fatalf("identical trailing-slash iss should verify: %v", err) + } + if verified.Sub() != "user123" { + t.Errorf("sub = %q, want %q", verified.Sub(), "user123") + } } func TestRFC9068AudienceMustMatchResource(t *testing.T) { diff --git a/core/conformancetests/rfc9728_test.go b/core/conformancetests/rfc9728_test.go index 6216a35..94fa3aa 100644 --- a/core/conformancetests/rfc9728_test.go +++ b/core/conformancetests/rfc9728_test.go @@ -120,6 +120,8 @@ func TestRFC9728WellKnownPathMustDeriveFromResourceURI(t *testing.T) { {"https://api.example.com", "/.well-known/oauth-protected-resource"}, {"https://api.example.com/mcp", "/.well-known/oauth-protected-resource/mcp"}, {"https://api.example.com/v2/mcp", "/.well-known/oauth-protected-resource/v2/mcp"}, + // RFC 9728 §3 insertion preserves the path exactly, trailing slash included. + {"https://api.example.com/mcp/", "/.well-known/oauth-protected-resource/mcp/"}, } for _, tc := range cases { diff --git a/core/internal/metadata/metadata.go b/core/internal/metadata/metadata.go index 7e9dff9..03d9386 100644 --- a/core/internal/metadata/metadata.go +++ b/core/internal/metadata/metadata.go @@ -207,17 +207,22 @@ func buildOAuthMetadataURL(issuer string) string { return issuer + "/.well-known/oauth-authorization-server" } - path := strings.TrimRight(u.EscapedPath(), "/") - if path == "" { - u.Path = "/.well-known/oauth-authorization-server" - } else { - u.Path = "/.well-known/oauth-authorization-server" + path - } - u.RawPath = "" - return u.String() + // RFC 8414 §3 forms the URL by *inserting* the well-known segment between + // the authority and the issuer's path — a pure string insertion that + // preserves the path exactly, including any trailing slash. Contrast with + // buildOIDCDiscoveryURL below, which concatenates and trims per its own + // spec: the two constructions look alike but are deliberately different. + return u.Scheme + "://" + u.Host + "/.well-known/oauth-authorization-server" + u.EscapedPath() } func buildOIDCDiscoveryURL(issuer string) string { + // OIDC Discovery 1.0 §4 *concatenates* rather than inserts, and mandates + // this trim: "If the Issuer value contains a path component, any + // terminating / MUST be removed before appending + // /.well-known/openid-configuration." This is the opposite of the + // RFC 8414 §3 insertion rule used above — both are correct in their own + // context. Do NOT "fix" this TrimRight when sweeping for identifier + // normalisation; removing it silently breaks OIDC discovery. return strings.TrimRight(issuer, "/") + "/.well-known/openid-configuration" } @@ -256,10 +261,10 @@ func (mc *MetadataCache) parse(data []byte) (*ASMetadata, error) { if meta.Issuer == "" { return nil, fmt.Errorf("metadata: missing required field \"issuer\"") } - configuredIssuer := strings.TrimRight(mc.issuerURL, "/") - metaIssuer := strings.TrimRight(meta.Issuer, "/") - if metaIssuer != configuredIssuer { - return nil, fmt.Errorf("metadata: issuer mismatch: expected %q, got %q", configuredIssuer, metaIssuer) + // RFC 8414 §3.3 — the returned issuer MUST be identical to the configured + // one; simple string comparison, no normalisation. + if meta.Issuer != mc.issuerURL { + return nil, fmt.Errorf("metadata: issuer mismatch: expected %q, got %q", mc.issuerURL, meta.Issuer) } if meta.JWKSURI == "" { return nil, fmt.Errorf("metadata: missing required field \"jwks_uri\"") diff --git a/core/internal/metadata/urls_test.go b/core/internal/metadata/urls_test.go new file mode 100644 index 0000000..cd773ff --- /dev/null +++ b/core/internal/metadata/urls_test.go @@ -0,0 +1,46 @@ +package metadata + +import "testing" + +// The two discovery-URL builders look alike but follow opposite rules: +// RFC 8414 §3 *inserts* the well-known segment and preserves the issuer's +// path verbatim, while OIDC Discovery 1.0 §4 *concatenates* and mandates +// trimming a terminating slash. These tests pin both behaviors so neither +// is "fixed" into the other. + +func TestBuildOAuthMetadataURLPreservesPath(t *testing.T) { + cases := []struct { + issuer string + want string + }{ + {"https://auth.example.com", "https://auth.example.com/.well-known/oauth-authorization-server"}, + {"https://auth.example.com/tenant", "https://auth.example.com/.well-known/oauth-authorization-server/tenant"}, + // RFC 8414 §3 insertion preserves the path exactly, trailing slash included. + {"https://auth.example.com/", "https://auth.example.com/.well-known/oauth-authorization-server/"}, + {"https://auth.example.com/tenant/", "https://auth.example.com/.well-known/oauth-authorization-server/tenant/"}, + } + for _, tc := range cases { + if got := buildOAuthMetadataURL(tc.issuer); got != tc.want { + t.Errorf("buildOAuthMetadataURL(%q) = %q, want %q", tc.issuer, got, tc.want) + } + } +} + +func TestBuildOIDCDiscoveryURLTrimsTerminatingSlash(t *testing.T) { + // OIDC Discovery 1.0 §4: "any terminating / MUST be removed before + // appending /.well-known/openid-configuration". The opposite of the + // RFC 8414 rule above, and correct here. + cases := []struct { + issuer string + want string + }{ + {"https://auth.example.com", "https://auth.example.com/.well-known/openid-configuration"}, + {"https://auth.example.com/", "https://auth.example.com/.well-known/openid-configuration"}, + {"https://auth.example.com/tenant/", "https://auth.example.com/tenant/.well-known/openid-configuration"}, + } + for _, tc := range cases { + if got := buildOIDCDiscoveryURL(tc.issuer); got != tc.want { + t.Errorf("buildOIDCDiscoveryURL(%q) = %q, want %q", tc.issuer, got, tc.want) + } + } +} diff --git a/core/resource/resource.go b/core/resource/resource.go index e0a465e..c1618a1 100644 --- a/core/resource/resource.go +++ b/core/resource/resource.go @@ -112,11 +112,15 @@ func (r *Resource) WellKnownPRMPath() string { } func wellKnownPRMPath(resourceURI string) string { + // RFC 9728 §3 — pure string insertion between the authority and the + // resource's path. The path is preserved exactly (including a bare or + // trailing slash), using the escaped form so percent-encoded octets are + // kept as sent. u, err := url.Parse(resourceURI) - if err != nil || u.Path == "" || u.Path == "/" { + if err != nil || u.EscapedPath() == "" { return "/.well-known/oauth-protected-resource" } - return "/.well-known/oauth-protected-resource" + u.Path + return "/.well-known/oauth-protected-resource" + u.EscapedPath() } // New creates a new Resource. @@ -128,12 +132,8 @@ func wellKnownPRMPath(resourceURI string) string { // invariant that downstream consumers (the HTTP adapter, the PRM emitter) // rely on consistent. func New(uri, issuer string, jwksCache *verifier.JWKSCache, opts ...Option) (*Resource, error) { - parsed, err := url.ParseRequestURI(uri) - if err != nil { - return nil, fmt.Errorf("resource: invalid resource URI: %w", err) - } - if parsed.Scheme == "" || parsed.Host == "" { - return nil, fmt.Errorf("resource: resource URI must be absolute with scheme and host, got %q", uri) + if err := verifier.ValidateIdentifier(uri, "resource URI"); err != nil { + return nil, fmt.Errorf("resource: %w", err) } cfg := &resourceConfig{} @@ -244,9 +244,10 @@ func (r *Resource) buildPRM() { r.prmMap = prm r.prmJSON, _ = json.Marshal(prm) - // r.uri was validated by New (url.ParseRequestURI), so url.Parse cannot - // fail here — this is the single, infallible source of truth that - // adapters consume via PRMURL(). + // r.uri was validated by New, so url.Parse cannot fail here — this is + // the single, infallible source of truth that adapters consume via + // PRMURL(). Composed by string concatenation so the well-known path from + // wellKnownPRMPath is not re-escaped. u, _ := url.Parse(r.uri) - r.prmURL = u.ResolveReference(&url.URL{Path: wellKnownPRMPath(r.uri)}).String() + r.prmURL = u.Scheme + "://" + u.Host + wellKnownPRMPath(r.uri) } diff --git a/core/resource/verifier/identifiers.go b/core/resource/verifier/identifiers.go new file mode 100644 index 0000000..4bb38e4 --- /dev/null +++ b/core/resource/verifier/identifiers.go @@ -0,0 +1,32 @@ +package verifier + +import ( + "fmt" + "net/url" + "strings" +) + +// ValidateIdentifier checks that value is an absolute http(s) URL identifier +// with a host and no fragment (RFC 8707 §2 forbids fragments in resource +// identifiers). +// +// It never rewrites the value: RFC 8414 §3.3 and RFC 9728 §3.3 require the +// advertised issuer/resource to be identical to the configured one — a simple +// string comparison — so trailing slashes, host case, and explicit ports are +// all legal variations that are preserved verbatim. +func ValidateIdentifier(value, label string) error { + u, err := url.Parse(value) + if err != nil { + return fmt.Errorf("%s is not a valid URL: %q: %w", label, value, err) + } + if u.Scheme != "https" && u.Scheme != "http" { + return fmt.Errorf("%s must be an absolute http or https URL, got %q", label, value) + } + if u.Host == "" { + return fmt.Errorf("%s must include a host, got %q", label, value) + } + if strings.Contains(value, "#") { + return fmt.Errorf("%s must not contain a fragment, got %q", label, value) + } + return nil +} diff --git a/core/resource/verifier/identifiers_test.go b/core/resource/verifier/identifiers_test.go new file mode 100644 index 0000000..10d0125 --- /dev/null +++ b/core/resource/verifier/identifiers_test.go @@ -0,0 +1,37 @@ +package verifier_test + +import ( + "testing" + + "github.com/authplane/go-sdk/core/resource/verifier" +) + +func TestValidateIdentifierAcceptsLegalVariations(t *testing.T) { + // Trailing slashes, host case, and explicit ports are legal identifier + // variations — preserved, never rejected or rewritten. + for _, value := range []string{ + "https://auth.example.com", + "https://auth.example.com/", + "https://auth.example.com/tenant/", + "https://Auth.Example.com:443/t1", + "http://localhost:8080/issuer", + } { + if err := verifier.ValidateIdentifier(value, "issuer"); err != nil { + t.Errorf("ValidateIdentifier(%q) = %v, want nil", value, err) + } + } +} + +func TestValidateIdentifierRejectsStructurallyInvalid(t *testing.T) { + for _, value := range []string{ + "auth.example.com", // no scheme + "ftp://auth.example.com", // non-http(s) scheme + "https:example.com", // no authority + "https://api.example.com/mcp#frag", // fragment (RFC 8707 §2) + "/mcp", // absolute path only + } { + if err := verifier.ValidateIdentifier(value, "issuer"); err == nil { + t.Errorf("ValidateIdentifier(%q) = nil, want error", value) + } + } +} diff --git a/core/resource/verifier/verifier.go b/core/resource/verifier/verifier.go index 10f0b48..45bf512 100644 --- a/core/resource/verifier/verifier.go +++ b/core/resource/verifier/verifier.go @@ -3,9 +3,7 @@ package verifier import ( "context" "fmt" - "net/url" "slices" - "strings" "time" "github.com/go-jose/go-jose/v4" @@ -38,7 +36,10 @@ type resolvedInboundDPoP struct { // The JWKSCache is injected from outside (the facade manages its lifecycle). func NewTokenVerifier(issuer, audience string, jwksCache *JWKSCache, opts ...Option) (*TokenVerifier, error) { v := &TokenVerifier{ - issuer: strings.TrimRight(issuer, "/"), + // RFC 8414 §3.3 — the issuer is an opaque identifier compared with + // simple string equality (against the token's iss); it is validated + // below but never rewritten. + issuer: issuer, audience: audience, jwks: jwksCache, clockSkew: DefaultClockSkew, @@ -54,11 +55,11 @@ func NewTokenVerifier(issuer, audience string, jwksCache *JWKSCache, opts ...Opt v.algorithms = defaultAlgorithms } - if _, err := url.ParseRequestURI(v.issuer); err != nil { - return nil, fmt.Errorf("verifier: invalid issuer URI: %w", err) + if err := ValidateIdentifier(v.issuer, "issuer"); err != nil { + return nil, fmt.Errorf("verifier: %w", err) } - if _, err := url.ParseRequestURI(v.audience); err != nil { - return nil, fmt.Errorf("verifier: invalid audience URI: %w", err) + if err := ValidateIdentifier(v.audience, "audience"); err != nil { + return nil, fmt.Errorf("verifier: %w", err) } return v, nil diff --git a/core/resource/verifier/verifier_test.go b/core/resource/verifier/verifier_test.go index a97ceac..e32a0f3 100644 --- a/core/resource/verifier/verifier_test.go +++ b/core/resource/verifier/verifier_test.go @@ -389,7 +389,9 @@ func TestVerifyToken_Scopes(t *testing.T) { } func TestVerifyToken_IssuerTrailingSlash(t *testing.T) { - // Verifier configured with trailing slash should still match issuer without. + // RFC 8414 §3.3 / RFC 9068 — the issuer is compared verbatim. A verifier + // configured with a trailing-slash issuer accepts a token whose iss + // carries the slash, and rejects one whose iss differs only by it. key, err := testutil.GenerateES256Key() if err != nil { t.Fatalf("generate key: %v", err) @@ -412,18 +414,27 @@ func TestVerifyToken_IssuerTrailingSlash(t *testing.T) { t.Fatalf("create verifier: %v", err) } - token, err := testutil.SignTokenWithClaims(key, jose.ES256, testKID, testIssuer, testAudience, testSubject, testClientID, nil) + matching, err := testutil.SignTokenWithClaims(key, jose.ES256, testKID, testIssuer+"/", testAudience, testSubject, testClientID, nil) if err != nil { t.Fatalf("sign token: %v", err) } - claims, err := v.VerifyToken(context.Background(), token, nil) + claims, err := v.VerifyToken(context.Background(), matching, nil) if err != nil { - t.Fatalf("trailing slash should be trimmed: %v", err) + t.Fatalf("identical trailing-slash iss should verify: %v", err) } if claims.Sub() != testSubject { t.Errorf("sub = %q, want %q", claims.Sub(), testSubject) } + + stripped, err := testutil.SignTokenWithClaims(key, jose.ES256, testKID, testIssuer, testAudience, testSubject, testClientID, nil) + if err != nil { + t.Fatalf("sign token: %v", err) + } + + if _, err := v.VerifyToken(context.Background(), stripped, nil); !errors.Is(err, verifier.ErrIssuerMismatch) { + t.Errorf("err = %v, want ErrIssuerMismatch — equivalent is not identical", err) + } } func TestVerifyToken_GarbageToken(t *testing.T) {