Skip to content

Add CIMD document fetch/validate and extend SSRF protections#5320

Merged
amirejaz merged 7 commits into
mainfrom
cimd-phase2-pr1-fetch-validate
May 20, 2026
Merged

Add CIMD document fetch/validate and extend SSRF protections#5320
amirejaz merged 7 commits into
mainfrom
cimd-phase2-pr1-fetch-validate

Conversation

@amirejaz
Copy link
Copy Markdown
Contributor

Summary

Phase 2 PR 1 of CIMD embedded AS support — closes part of #4825.

The embedded authorization server (Phase 2) will need to fetch and validate Client ID Metadata Documents when MCP clients present HTTPS URLs as client_id. This PR adds that foundation as a pure library layer with no server wiring — wiring comes in PR 2 (storage decorator) and PR 3 (config + discovery).

  • FetchClientMetadataDocument fetches a CIMD document with the security controls required by the spec: HTTPS-only, 5 s timeout, 10 KB body cap, one-hop redirect limit, per-dial SSRF protection, Content-Type validation, and strict self-referential binding (client_id must exactly equal the serving URL).
  • ValidateClientMetadataDocument checks required fields and allowed redirect URI schemes.
  • ClientMetadataDocument struct matches draft-ietf-oauth-client-id-metadata-document fields.
  • SSRF range extensions in pkg/networking: RFC6598 CGN (100.64.0.0/10) and RFC5737 documentation ranges added to the private IP block list.
  • WithDisableKeepAlives on HttpClientBuilder so callers can prevent connection reuse from bypassing per-dial SSRF checks.

The SSRF check in cimd.go is implemented inline rather than via pkg/networking to preserve the oauthproto leaf-package invariant (documented in doc.go).

Type of change

  • New feature (non-breaking)

Test plan

  • Unit tests: go test ./pkg/oauthproto/... ./pkg/networking/... — all pass
  • Fetch/validate tests cover: valid document, non-HTTPS rejection, non-200 status, 10 KB cap, Content-Type validation, application/*+json subtype, client_id mismatch, missing redirect_uris, SSRF private IP rejection
  • New IP range tests: CGN and documentation ranges verified as private

Special notes for reviewers

The duplicate SSRF block list in cimd.go is intentional — see the comment referencing doc.go. If the leaf-package invariant is ever relaxed, cimdPrivateBlocks can be replaced with a call to networking.IsPrivateIP.

Generated with Claude Code

Phase 2 PR 1 of CIMD embedded AS support (issue #4825).

- pkg/oauthproto/cimd.go: add ClientMetadataDocument struct,
  FetchClientMetadataDocument (HTTPS-only, 10 KB cap, 5 s timeout,
  1-hop redirect limit, per-dial SSRF check, Content-Type validation,
  strict self-referential binding), ValidateClientMetadataDocument.
  SSRF check implemented inline to preserve the oauthproto leaf-package
  invariant (no import of pkg/networking).

- pkg/networking/utilities.go: add RFC6598 CGN (100.64.0.0/10) and
  RFC5737 documentation ranges (192.0.2.0/24, 198.51.100.0/24,
  203.0.113.0/24) to the private IP block list.

- pkg/networking/http_client.go: add WithDisableKeepAlives option to
  HttpClientBuilder so callers can prevent keep-alive connection reuse
  from bypassing per-dial SSRF checks.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/L Large PR: 600-999 lines changed label May 19, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 71.42857% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.40%. Comparing base (8d22ac5) to head (9b4a77b).

Files with missing lines Patch % Lines
pkg/oauthproto/cimd/fetch.go 65.38% 16 Missing and 11 partials ⚠️
pkg/networking/http_client.go 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5320      +/-   ##
==========================================
- Coverage   68.41%   68.40%   -0.02%     
==========================================
  Files         621      622       +1     
  Lines       63278    63371      +93     
==========================================
+ Hits        43293    43349      +56     
- Misses      16757    16784      +27     
- Partials     3228     3238      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amirejaz amirejaz marked this pull request as draft May 19, 2026 10:04
Verified against both draft-ietf-oauth-client-id-metadata-document and
internal RFC-0071:

- validateCIMDClientURL enforces §3 URL requirements: non-empty path
  component, no fragment, no userinfo, no dot-segments; removes the
  TODO that was deferring this validation to Phase 2

- ValidateClientMetadataDocument rejects symmetric auth methods
  (client_secret_post, client_secret_basic, client_secret_jwt) per §4.1
  of the CIMD draft

- Add IPv4 multicast (224.0.0.0/4) and IPv6 multicast (ff00::/8) to
  both pkg/networking and the oauthproto inline SSRF block list

- Update tests to use meaningful URL paths (/metadata.json); bare-root
  paths (/) now correctly fail the §3 path requirement

Note: custom CA cert support (RFC-0071 §4 server-side) is deferred to
PR 2 — FetchClientMetadataDocument will accept an optional *http.Client
allowing the storage decorator to pass a CA-aware client.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels May 19, 2026
@amirejaz amirejaz marked this pull request as ready for review May 19, 2026 10:28
@amirejaz amirejaz requested a review from Copilot May 19, 2026 10:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds foundational library support for Client ID Metadata Documents (CIMD) by introducing fetch + validation helpers in pkg/oauthproto, and extends SSRF protections in pkg/networking (additional blocked ranges + an option to disable HTTP keep-alives so per-dial SSRF checks can’t be bypassed via connection reuse).

Changes:

  • Add ClientMetadataDocument model plus FetchClientMetadataDocument (HTTP fetch with guardrails) and ValidateClientMetadataDocument (field + redirect URI validation).
  • Extend private/IP-block detection (CGN + RFC5737 documentation ranges + multicast) and add HttpClientBuilder.WithDisableKeepAlives.
  • Add unit tests for CIMD fetch/validation behavior and for new private IP ranges.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pkg/oauthproto/cimd.go Adds CIMD document type, URL validation, SSRF-guarded fetcher, and document validation logic.
pkg/oauthproto/cimd_test.go Adds tests for CIMD fetch/validation scenarios (status codes, content-type, body cap, redirect URIs, etc.).
pkg/networking/utilities.go Extends private IP block list to include CGN + RFC5737 documentation ranges + multicast.
pkg/networking/utilities_test.go Adds unit tests asserting the new private IP ranges are classified as private.
pkg/networking/http_client.go Adds builder option to disable keep-alives (supports per-dial SSRF enforcement).
Comments suppressed due to low confidence (3)

pkg/oauthproto/cimd.go:313

func validateRedirectURI(uri string) error {
	switch {
	case strings.HasPrefix(uri, "https://"):
		return nil
	case strings.HasPrefix(uri, "http://localhost"),
		strings.HasPrefix(uri, "http://127.0.0.1"),
		strings.HasPrefix(uri, "http://[::1]"):
		return nil

pkg/oauthproto/cimd.go:230

  • Loopback addresses are explicitly allowed in the SSRF guard (!ip.IsLoopback()), meaning an HTTPS client_id whose hostname resolves to 127.0.0.1/::1 will be permitted. This contradicts the stated goal of blocking loopback/private ranges and makes localhost SSRF possible via DNS. If loopback should only be allowed for explicit loopback development URLs, gate it on the parsed URL host being loopback/localhost rather than allowing all loopback resolutions.
				// Loopback is allowed — it is already gated by the URL scheme check
				// (HTTP to loopback is explicitly permitted for development). All other
				// private ranges are rejected to prevent SSRF.
				if !ip.IsLoopback() && isPrivateForCIMD(ip) {
					return nil, fmt.Errorf("cimd: refusing connection to private address %s", ipStr)
				}

pkg/oauthproto/cimd.go:189

  • Using io.LimitReader(resp.Body, maxBodyBytes) does not reliably enforce the 10KB cap: if valid JSON is fully contained within the first 10KB, the decoder will succeed and any remaining bytes will be ignored (and currently later drained). Consider limiting to maxBodyBytes+1 and returning an explicit error when the response exceeds the cap, or read with io.ReadAll using a max+1 limit.
	const maxBodyBytes = 10 * 1024
	limited := io.LimitReader(resp.Body, maxBodyBytes)
	var doc ClientMetadataDocument
	if err := json.NewDecoder(limited).Decode(&doc); err != nil {
		return nil, fmt.Errorf("failed to decode client metadata document: %w", err)
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd_test.go Outdated
1. isLoopbackHTTP bypass: prefix matching (e.g. "http://localhost")
   matched "http://localhost.evil.com/". Now parses the URL and checks
   the hostname exactly against loopback addresses.

2. DNS TOCTOU: DialContext resolved the hostname then dialled by name,
   leaving a window where the second resolution could return a private
   IP. Now picks the first valid IP, validates it, and dials by IP
   literal to eliminate the race.

3. Redirect target not validated: CheckRedirect only counted hops.
   A redirect from HTTPS to HTTP or to a private IP was not caught.
   Now calls validateCIMDClientURL on each redirect target.

Also:
- Drop io.Copy drain after decode; keep-alive connections are disabled
  so draining for connection reuse is unnecessary and would defeat the
  10KB cap by reading the remainder of the body
- Add clarifying comment that only symmetric auth methods are forbidden;
  asymmetric methods (private_key_jwt, tls_client_auth, none) are valid
- Add tests: localhost subdomain bypass, dial-guard SSRF via HTTPS URL,
  renamed scheme-check test to accurately describe what it exercises

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels May 19, 2026
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code itself is good, but I think we should reduce the code duplication

Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Comment thread pkg/oauthproto/cimd.go Outdated
Addresses jhrozek feedback on duplication:

- Create pkg/oauthproto/cimd sub-package which may import pkg/networking,
  eliminating three cases of duplicated logic:
  1. validateRedirectURI → now delegates to oauthproto.ValidateRedirectURI
     with RedirectURIPolicyStrict (RFC 8252 §8.4)
  2. isLoopbackHTTP → now uses oauthproto.IsLoopbackHost(parsed.Hostname())
     via URL.Parse, closing the localhost.evil.com prefix-match bypass
  3. cimdPrivateBlocks/isPrivateForCIMD → now uses networking.IsPrivateIP,
     eliminating the duplicated IP block list init

- pkg/oauthproto/cimd.go retains only ToolHiveClientMetadataDocumentURL
  and IsClientIDMetadataDocumentURL (Phase 1 code depends on these)

- HttpClientBuilder.WithPrivateIPs(false) is not used in newCIMDHTTPClient
  because it also blocks loopback (127.0.0.0/8), breaking the intentional
  http://localhost development exception. The custom DialContext is kept
  but uses networking.IsPrivateIP instead of the duplicated block list.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels May 20, 2026
@amirejaz amirejaz requested a review from jhrozek May 20, 2026 11:57
Follow-up to jhrozek's review of the FetchJSON comment:

pkg/networking/fetch.go:
- Add WithMaxResponseSize(int64) FetchOption so callers can enforce
  tighter limits; CIMD uses 10 KB vs the 1 MB default
- Fix Content-Type validation bug: old strings.Contains check rejected
  valid application/*+json subtypes (e.g. ld+json). Replace with
  mime.ParseMediaType + RFC 6839 prefix/suffix check, which is the
  same logic CIMD already used correctly

pkg/oauthproto/cimd/fetch.go:
- FetchClientMetadataDocument delegates to networking.FetchJSON with
  WithMaxResponseSize(10*1024); removes the manual HTTP fetch loop,
  body-drain defer, and the now-unused isJSONSubtype helper (~30 lines)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels May 20, 2026
jhrozek
jhrozek previously approved these changes May 20, 2026
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FetchJSON refactor in ab8a5b8 cleaned up the duplication I was about to ask about — nice. Three small nits below, all non-blocking. Approve.

Comment thread pkg/oauthproto/cimd/fetch.go Outdated
Comment thread pkg/oauthproto/cimd/fetch.go Outdated
Comment thread pkg/oauthproto/cimd/fetch.go
- CheckRedirect: remove dead code. `len(via) >= 1` fired on the first
  redirect (via already contains the original request), making the
  validateCIMDClientURL call unreachable. Per CIMD §4 the AS MUST NOT
  automatically follow redirects, so simplify to unconditional
  ErrUseLastResponse with the spec quote in the comment.

- validateCIMDClientURL: use parsed.Scheme instead of strings.HasPrefix
  for the HTTPS check. URI schemes are case-insensitive per RFC 3986 §3.1;
  HTTPS://example.com/ is valid but the prefix check rejected it with a
  misleading error. The loopback check already used parsed.Scheme correctly.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels May 20, 2026
@amirejaz amirejaz requested a review from jhrozek May 20, 2026 15:04
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels May 20, 2026
@amirejaz amirejaz merged commit 9a5d0c2 into main May 20, 2026
45 checks passed
@amirejaz amirejaz deleted the cimd-phase2-pr1-fetch-validate branch May 20, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants