dcr: support RFC 8414 §3.1 path-insertion in discovery-URL → issuer derivation#5395
Conversation
…erivation `deriveExpectedIssuerFromDiscoveryURL` recovers the issuer the upstream is expected to claim in its discovery document. It already handled the suffix-append form (e.g. https://mcp.atlassian.com/.well-known/oauth-authorization-server → https://mcp.atlassian.com) and the issuer-suffix multi-tenant style (.../tenants/acme/.well-known/openid-configuration → .../tenants/acme), but the comment block explicitly opted out of the RFC 8414 §3.1 path-insertion form — operators on that pattern had to fall back to `dcr_config.registration_endpoint` to bypass discovery entirely. That gap rejects providers that publish a path-component issuer per the letter of the RFC. Datadog's MCP authorization server is one such provider: its discovery URL `https://mcp.us5.datadoghq.com/.well-known/oauth-authorization-server/v1/mcp` declares issuer `https://mcp.us5.datadoghq.com/v1/mcp`, and DCR discovery aborts with: issuer mismatch (RFC 8414 §3.3): expected "https://mcp.us5.datadoghq.com", got "https://mcp.us5.datadoghq.com/v1/mcp" Recognise the path-insertion form by checking for the well-known segment as a path *prefix* followed by a tenant path (HasPrefix(path, suffix+"/")), trimming just the well-known segment to recover origin + tenant path. Disambiguated from the existing suffix-append case by position: the well-known segment at the end of the path is suffix-append; at the start with more path following is path-insertion. The two cases cannot both match a single URL. Tests cover the new branch for both the OAuth and OIDC suffix variants plus a multi-segment tenant. All existing cases continue to pass. Per RFC 8414 §3 (the well-known URI is formed by inserting the well-known suffix between host and path of the issuer) and RFC 8615 (well-known URI conventions). Signed-off-by: Juzer Patanwala <juzer.patanwala@project44.com>
juzerpatanwala
requested review from
ChrisJBurns,
JAORMX,
jhrozek and
yrobla
as code owners
tgrunnagle
left a comment
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-agent review summary
Tight, well-scoped bug fix that closes the RFC 8414 §3.1 path-insertion gap in deriveExpectedIssuerFromDiscoveryURL for the operator-configured discoveryUrl branch. The approach matches the issue's suggested fix exactly — two HasPrefix(path, suffix+"/") arms ordered after the suffix arms, with the trailing-/ guard disambiguating suffix-append from path-insertion. The well-known segment is matched as a full path component (leading / in the constant + trailing / guard), so substrings can't false-match, and any mis-derivation fails safe against the exact-match §3.3 check. Core defect resolved; 0 HIGH-severity findings.
| # | Finding | Severity | Consensus |
|---|---|---|---|
| A | Trailing-slash bare well-known now derives issuer with spurious / (regression + false comment) |
LOW | 8/10 |
| B | Missing operator-configured DiscoveryURL e2e regression test the issue required |
MEDIUM | 9/10 |
B — Missing e2e regression test (MEDIUM)
Issue #5390 lists two test deliverables: unit cases on the helper (✅ added) and an e2e regression test in pkg/auth/discovery/dcr_resolver_test.go mirroring TestHandleDynamicRegistration_NonRootIssuerRFC8414PathInsertion (#5357) but driving the operator-configured DiscoveryURL branch. Only the unit cases landed. The helper is verified in isolation, but nothing proves the path-bearing issuer survives the full discovery flow and clears the §3.3 check in the DiscoveryURL branch — which is exactly where the original error surfaced.
Suggestion: add an httptest-based test serving metadata at /.well-known/oauth-authorization-server/v1/mcp with issuer = server.URL+"/v1/mcp", drive the DiscoveryURL branch, and assert DCR succeeds end-to-end. If you consciously decided the unit cases suffice, a note in "Special notes for reviewers" would capture that, since it deviates from the linked issue.
3 specialist agents (RFC 8414 correctness, Go quality, test coverage). Codex cross-review skipped — CLI not installed.
🤖 Generated with Claude Code
…ack) Per @tgrunnagle's review on stacklok#5395: the path-insertion arms introduced in the previous commit accidentally regress one edge case. For an input where the path ends `/.well-known/oauth-authorization-server/` (trailing slash, no tenant), the suffix arms don't match (suffix test sees the trailing "/"), so the HasPrefix arm fires and TrimPrefix leaves `u.Path = "/"` → spurious issuer `https://host/` that fails the §3.3 byte-equality check against the upstream's declared `https://host`. Before this PR the same input hit the `default` arm and produced `https://host` correctly. Fix: after TrimPrefix in each path-insertion arm, collapse a lone `/` back to empty so the trailing-slash form converges on the same origin issuer the bare-suffix and `default` arms produce. Tightens the inline comment to describe both shapes that reach the HasPrefix arm (real tenant suffix and trailing-slash). Adds two table-driven cases — one each for the oauth and oidc trailing-slash forms — to lock in the expected origin output. Signed-off-by: Juzer Patanwala <juzer.patanwala@project44.com>
tgrunnagle
left a comment
tgrunnagle
left a comment
There was a problem hiding this comment.
Re-reviewed after 6d1d7cc. The trailing-slash regression (finding A) is cleanly resolved — u.Path == "/" is collapsed back to "" in both path-insertion arms, the misleading comment is corrected to describe both shapes that reach the branch, and two new table cases (oauth + oidc trailing-slash → origin) pin the behavior. No new issues introduced; the normalization fires only on a bare /, so real tenant paths are untouched. Core fix is spec-faithful and fails safe against the §3.3 check.
Approving. The operator-configured DiscoveryURL e2e regression test (finding B) is a non-blocking suggestion rather than a merge blocker — the added unit cases now cover the path-insertion and trailing-slash derivations directly, so the helper's §3.3-relevant output is well pinned. An end-to-end test driving the full DiscoveryURL discovery flow would still be a nice follow-up to guard the derive→§3.3 round-trip, but it's optional given the unit coverage.
🤖 Generated with Claude Code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5395 +/- ##
==========================================
+ Coverage 68.76% 68.78% +0.02%
==========================================
Files 629 629
Lines 63922 63930 +8
==========================================
+ Hits 43958 43977 +19
+ Misses 16707 16698 -9
+ Partials 3257 3255 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
deriveExpectedIssuerFromDiscoveryURL(inpkg/auth/dcr/resolver.go) recovers the issuer the upstream is expected to claim in its discovery document, so the RFC 8414 §3.3 equality check inoauthproto.FetchAuthorizationServerMetadataFromURLcan be applied.It already handled:
https://mcp.atlassian.com/.well-known/oauth-authorization-server→https://mcp.atlassian.comhttps://idp.example.com/tenants/acme/.well-known/openid-configuration→https://idp.example.com/tenants/acmeThe function explicitly opted out of the RFC 8414 §3.1 / RFC 8615 path-insertion form — where the well-known segment is inserted between the host and the issuer's tenant path. The existing comment block punted to
dcr_config.registration_endpointas the workaround.That gap rejects providers that publish a path-component issuer per the letter of the RFC. Datadog's MCP authorization server is one such provider: its discovery URL
https://mcp.us5.datadoghq.com/.well-known/oauth-authorization-server/v1/mcpdeclaresissuer: https://mcp.us5.datadoghq.com/v1/mcp, and DCR fails with:This PR closes that gap.
Type of change
How
Add two new branches to the
switchinderiveExpectedIssuerFromDiscoveryURLthat recognise the well-known segment as a path prefix (HasPrefix(path, suffix+"/")) and trim just that segment to recover origin + tenant path:Disambiguation from the existing suffix-append case is positional: well-known at the end of the path is suffix-append; at the start with more path following is path-insertion. The two cases cannot both match a single URL — guarded by the trailing
/so a bare suffix (already handled above) doesn't take this branch.The opt-out comment in the function docstring is replaced with a third "recognised convention" entry describing the new behaviour.
Test plan
go test ./pkg/auth/dcr/... ./pkg/oauthproto/...(all packages pass)task test-e2e) — not run locally; covered by repo CItask lint-fix) —gofmt -lclean;go vetclean;golangci-lintnot available locally, relying on CINew unit cases (added to the existing table-driven
TestDeriveExpectedIssuerFromDiscoveryURL):https://mcp.us5.datadoghq.com/.well-known/oauth-authorization-server/v1/mcphttps://mcp.us5.datadoghq.com/v1/mcphttps://idp.example.com/.well-known/oauth-authorization-server/tenants/acmehttps://idp.example.com/tenants/acmehttps://idp.example.com/.well-known/openid-configuration/tenants/acmehttps://idp.example.com/tenants/acmeAll eight pre-existing cases still pass.
API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Pure function-internal change. Pre-existing values entries (issuer-suffix and bare-suffix forms) continue to derive the same issuer they did before.
Changes
pkg/auth/dcr/resolver.goderiveExpectedIssuerFromDiscoveryURLrecognises the path-insertion form; opt-out comment replaced with a third recognised conventionpkg/auth/dcr/resolver_test.goDoes this introduce a user-facing change?
Yes — operators with a
dcr_config.discovery_urlpointing at an RFC 8414 §3.1 path-insertion discovery document (e.g. Datadog's MCP atmcp.us5.datadoghq.com) now succeed instead of failing the issuer-equality check. No values changes are required.