diff --git a/internal/clientmetadata/clientmetadata.go b/internal/clientmetadata/clientmetadata.go index 45b19136c..a819b3983 100644 --- a/internal/clientmetadata/clientmetadata.go +++ b/internal/clientmetadata/clientmetadata.go @@ -29,6 +29,7 @@ import ( "io" "net/http" "net/url" + "strconv" "strings" "sync" "time" @@ -248,7 +249,22 @@ func (p *Provider) fetchViaClient(ctx context.Context, clientID string, client * } req.Header.Set("Accept", "application/json") - resp, err := client.Do(req) + // Refuse redirects, matching the JWKS and OIDC-discovery fetches. The + // document must be AT the client_id URL, so a hop away from it is a document + // for a different identifier, and SafeHTTPClient pins the dial to the + // validated IP — a redirect elsewhere would re-issue the request against + // that same address carrying someone else's Host header rather than + // reaching the named host at all. + // + // Set on a COPY, not on the caller's client: fetch() builds a fresh one per + // request, but SetHTTPClientForTest injects a client the test owns, and + // mutating a caller's value is a side effect regardless of which path is + // hotter. The copy costs nothing and keeps both paths on the same policy, so + // the test seam actually exercises what production does. + noRedirect := *client + noRedirect.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse } + + resp, err := noRedirect.Do(req) if err != nil { return nil, 0, fmt.Errorf("could not fetch client metadata document") } @@ -322,8 +338,9 @@ func cacheTTL(header string) time.Duration { for _, part := range strings.Split(header, ",") { part = strings.ToLower(strings.TrimSpace(part)) if v, ok := strings.CutPrefix(part, "max-age="); ok { - var secs int - if _, err := fmt.Sscanf(v, "%d", &secs); err == nil && secs > 0 { + // strconv.Atoi, not fmt.Sscanf: Sscanf stops at the first non-digit + // and still reports success, so "max-age=60junk" parsed as 60. + if secs, err := strconv.Atoi(v); err == nil && secs > 0 { ttl = time.Duration(secs) * time.Second } } diff --git a/internal/clientmetadata/clientmetadata_test.go b/internal/clientmetadata/clientmetadata_test.go index 9627c4026..0b00a1331 100644 --- a/internal/clientmetadata/clientmetadata_test.go +++ b/internal/clientmetadata/clientmetadata_test.go @@ -164,6 +164,15 @@ func TestCacheTTLIsClamped(t *testing.T) { {"max-age=31536000", maxCacheTTL}, {"public, max-age=600", 600 * time.Second}, {"MAX-AGE=600", 600 * time.Second}, + // fmt.Sscanf stopped at the first non-digit and still reported success, + // so these parsed as 600 / 60 rather than being rejected. The clamp made + // every outcome safe, which is why it went unnoticed — a header this + // malformed should still fall back to the floor rather than be half-read. + {"max-age=600junk", minCacheTTL}, + {"max-age=600 junk", minCacheTTL}, + {"max-age=", minCacheTTL}, + {"max-age=-5", minCacheTTL}, + {"max-age=abc", minCacheTTL}, } for _, tc := range cases { t.Run(tc.header, func(t *testing.T) { @@ -301,6 +310,56 @@ func TestFetchValidatesDocumentContent(t *testing.T) { }) } +// TestFetchRefusesRedirects pins the redirect policy. +// +// The document must be AT the client_id URL — that identity binding is the whole +// mechanism CIMD rests on — so a hop away from it serves a document for a +// different identifier. Following one is also useless in production: the +// SSRF-hardened client pins the dial to the IP validated for the ORIGINAL host, +// so a redirect elsewhere re-issues the request against that same address +// carrying a foreign Host header instead of reaching the named host. +// +// The redirect target here serves a perfectly valid document for its own URL, so +// nothing but the redirect policy itself can make this fail. +func TestFetchRefusesRedirects(t *testing.T) { + var targetURL string + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"client_id":%q,"client_name":"Moved Client","redirect_uris":["https://app.example.com/cb"]}`, targetURL) + })) + defer target.Close() + targetURL = target.URL + "/moved.json" + + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, targetURL, http.StatusFound) + })) + defer redirector.Close() + + _, _, err := testProvider(t).fetchViaClient( + context.Background(), redirector.URL+"/client.json", redirector.Client()) + require.Error(t, err, "a redirected document MUST NOT resolve") + // The 302 is surfaced as the final response rather than followed. + assert.Contains(t, err.Error(), "status 302") +} + +// TestFetchViaClientDoesNotMutateTheCallersClient guards the mechanism behind +// TestFetchRefusesRedirects. The policy is applied to a copy, because the +// injected-client seam (SetHTTPClientForTest) hands in a client the caller owns. +// Setting the field in place would work and still be a side effect on someone +// else's value. +func TestFetchViaClientDoesNotMutateTheCallersClient(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + client := srv.Client() + require.Nil(t, client.CheckRedirect, "precondition: the caller's client has no redirect policy") + + _, _, _ = testProvider(t).fetchViaClient(context.Background(), srv.URL+"/c.json", client) + + assert.Nil(t, client.CheckRedirect, "fetchViaClient MUST NOT mutate the client it was handed") +} + // TestIsMetadataClientIDForExcludesTheReservedClient guards against a // configuration-triggered change of identity source. // diff --git a/internal/events/events.go b/internal/events/events.go index 76ac00abb..44aba49f2 100644 --- a/internal/events/events.go +++ b/internal/events/events.go @@ -215,6 +215,15 @@ func (p *provider) deliver(ctx context.Context, log *zerolog.Logger, webhook *sc // allow-list, DNS-rebinding host pinning, TLS SNI) is unchanged either way. Mirrors // internal/http_handlers/oauth_sso.go's ssoHTTPClient, kept independent so the two // escape hatches can never relax each other. +// +// Redirects are deliberately still FOLLOWED here, unlike the CIMD document and +// backchannel-logout fetches which refuse them. Those two retrieve a +// spec-defined resource that must be AT the URL named; a webhook endpoint is an +// operator's own URL, and a same-host path redirect is followed today with the +// final status recorded in WebhookLog.HttpStatus. Refusing them would change +// that recorded status for existing deployments to close nothing: SafeHTTPClient +// pins the dial to the validated IP, so a redirect cannot reach a different host +// regardless. Do not "make this consistent" without a compatibility note. func webhookHTTPClient(ctx context.Context, rawURL string, timeout time.Duration, allowPrivate bool) (*http.Client, error) { if allowPrivate { return validators.SafeHTTPClientAllowPrivate(ctx, rawURL, timeout) diff --git a/internal/service/admin_webhooks.go b/internal/service/admin_webhooks.go index e26cb9133..6edc1f2b2 100644 --- a/internal/service/admin_webhooks.go +++ b/internal/service/admin_webhooks.go @@ -363,6 +363,12 @@ func (p *provider) TestEndpoint(ctx context.Context, meta RequestMetadata, param if skipSSRF { client = &http.Client{Timeout: testEndpointHTTPTimeout} } else { + // Redirects are deliberately still followed here — this must behave + // exactly like a real delivery, and delivery follows them (see + // events.webhookHTTPClient for why, and why the CIMD and + // backchannel-logout fetches differ). A "test endpoint" that applied a + // stricter policy than delivery would report a failure the live webhook + // would not have. client, err = validators.SafeHTTPClient(ctx, params.Endpoint, testEndpointHTTPTimeout) if err != nil { log.Debug().Err(err).Str("endpoint", params.Endpoint).Msg("endpoint URL rejected by SSRF filter") diff --git a/internal/token/backchannel_logout.go b/internal/token/backchannel_logout.go index 49f269c1c..d88651b1f 100644 --- a/internal/token/backchannel_logout.go +++ b/internal/token/backchannel_logout.go @@ -86,6 +86,13 @@ func (p *provider) NotifyBackchannelLogout(ctx context.Context, uri string, cfg if err != nil { return fmt.Errorf("backchannel logout SSRF check: %w", err) } + // Refuse redirects. Go re-issues a 302'd POST as a GET with no body, so an RP + // whose backchannel_logout_uri redirects was already receiving a bodyless GET + // and silently not processing the logout — this turns that into a visible + // non-2xx rather than a success that did nothing. SafeHTTPClient also pins the + // dial to the validated IP, so a redirect could never reach the named host + // anyway. Same policy as the JWKS and OIDC-discovery fetches. + client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse } resp, err := client.Do(req) if err != nil { return err