From 5d7bc864515a900ccfcd431cba006b5813d1466e Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 16 May 2026 16:41:05 +0000 Subject: [PATCH] =?UTF-8?q?fix(oidc):=20SEC-020=20=E2=80=94=20wrap=20fetch?= =?UTF-8?q?UserinfoGroups=20via=20SafeOIDCContext?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acquisition-audit Sprint 1 follow-up to SEC-001 (2026-05-16). The original SEC-001 sweep routed two OIDC discovery legs (test_discovery.go dry-run + service.go runtime provider load) through validation.SafeHTTPDialContext via the SafeOIDCContext(ctx) helper. This commit closes one of the two adjacent call sites the sweep missed: the userinfo-fallback path at service.go::fetchUserinfoGroups. Pre-fix: func (s *Service) fetchUserinfoGroups(ctx, entry, token, path) { ... ts := entry.oauthConfig.TokenSource(ctx, token) uinfo, err := entry.provider.UserInfo(ctx, ts) ... } go-oidc/v3 Provider.UserInfo (oidc.go:351-374) derives its http.Client from ctx via getClient(ctx) (oidc.go:61-65). Without an override, the internal doRequest (oidc.go:87-92) falls through to http.DefaultClient — no SSRF guard, no DNS-rebinding re-resolve at dial time. An IdP whose discovery doc advertises a userinfo_endpoint pointing at a reserved address (loopback / link-local / 169.254.169.254 cloud-metadata) would trigger an unguarded HTTPS egress at userinfo-fetch time. Operator opt-in to fetch_userinfo=true turns the gap on; the leg fires whenever the ID token doesn't surface the configured groups claim. Post-fix: safeCtx := SafeOIDCContext(ctx) ts := entry.oauthConfig.TokenSource(safeCtx, token) uinfo, err := entry.provider.UserInfo(safeCtx, ts) Context-key shape: gooidc.ClientContext is implemented as context.WithValue(ctx, oauth2.HTTPClient, client) (go-oidc v3.18.0 oidc.go:57-59). Both go-oidc's getClient AND golang.org/x/oauth2's internal.ContextClient read the same oauth2.HTTPClient key, so the SINGLE SafeOIDCContext wrap covers go-oidc-driven HTTP calls (Provider.UserInfo / Verifier JWKS) AND oauth2-driven HTTP calls (Config.TokenSource refresh / Exchange). No additional context.WithValue(ctx, oauth2.HTTPClient, ...) is required. Files touched: internal/auth/oidc/service.go — wrap ctx in fetchUserinfoGroups internal/auth/oidc/safehttp.go — extend SEC-001 header comment block to enumerate the two newly-patched sites (SEC-020 here + SEC-021 in the next commit) and the oauth2.HTTPClient key-sharing rationale, so future audits don't re-flag the design as confused internal/auth/oidc/service_test.go — new test TestFetchUserinfoGroups_SSRF_BlocksReservedAddress that stands up a loopback discovery server whose discovery doc advertises userinfo_endpoint = http://169.254.169.254/userinfo, constructs *gooidc.Provider via the test-bypassed oidcDiscoveryClient (setup_test.go's init() pattern), then RESTORES the production SafeHTTPDialContext-backed client just before the fetchUserinfoGroups call. Asserts the error wraps SafeHTTPDialContext's 'refusing to dial reserved address' rejection rather than a generic connect-refused. Companion to the TestDefaultBCLVerifier_SSRF_BlocksReservedAddress that SEC-021 (next commit) adds. Verified: gofmt -l internal/ docs/ (clean) go vet ./... (clean) go test -race -short ./internal/auth/oidc/... (all green) TestFetchUserinfoGroups_SSRF_BlocksReservedAddress (new; green) All 4 cited CI guards pass (openapi-handler-parity, openapi-codegen-drift, no-sh-c-in-connectors, skip-inventory-drift) Acceptance grep: internal/auth/oidc/service.go:963: uinfo, err := entry.provider.UserInfo(safeCtx, ts) internal/auth/oidc/service.go:1084: provider, err := gooidc.NewProvider(SafeOIDCContext(ctx), cfgRow.IssuerURL) No bare-ctx UserInfo / NewProvider remains in service.go. Closes acquisition-audit SEC-020. SEC-021 (BCL discovery re-fetch) lands in the next commit. --- internal/auth/oidc/safehttp.go | 25 +++++++ internal/auth/oidc/service.go | 15 +++- internal/auth/oidc/service_test.go | 107 +++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 2 deletions(-) diff --git a/internal/auth/oidc/safehttp.go b/internal/auth/oidc/safehttp.go index 03012b4..f72100c 100644 --- a/internal/auth/oidc/safehttp.go +++ b/internal/auth/oidc/safehttp.go @@ -9,6 +9,31 @@ package oidc // - test_discovery.go:65 (dry-run validator from the GUI) // - service.go:1066 (runtime provider load on first cache miss) // +// Acquisition-audit follow-up SEC-020 + SEC-021 (Sprint 1 follow-up, +// 2026-05-16) extended the same wrap to two adjacent call sites that +// the original SEC-001 sweep missed: +// +// - service.go::fetchUserinfoGroups (~L948-961, SEC-020 closure) — +// the userinfo-fallback path called entry.provider.UserInfo(ctx, ts) +// with bare ctx. go-oidc/v3 Provider.UserInfo derives its HTTP +// client from the context via getClient(ctx) (oidc.go:61-65); +// without an override, the internal doRequest falls through to +// http.DefaultClient. +// - internal/api/handler/auth_session_oidc_bcl.go::Verify (~L125, +// SEC-021 closure) — the back-channel-logout verifier performs a +// per-request discovery re-fetch via gooidc.NewProvider(ctx, ...) +// with bare ctx; SafeOIDCContext now wraps before the call. +// +// Context-key shape: gooidc.ClientContext is implemented as +// context.WithValue(ctx, oauth2.HTTPClient, client) +// (go-oidc v3.18.0 oidc.go:57-59). Both go-oidc's getClient AND +// golang.org/x/oauth2's internal.ContextClient read oauth2.HTTPClient, +// so the SINGLE SafeOIDCContext wrap covers go-oidc-driven HTTP calls +// (Provider.UserInfo / NewProvider discovery / Verifier JWKS) AND +// oauth2-driven HTTP calls (Config.TokenSource refresh / Exchange). +// No additional context.WithValue(ctx, oauth2.HTTPClient, ...) is +// required alongside the wrap. +// // gooidc.NewProvider derives its HTTP client from the context via // oidc.ClientContext; with no override it falls through to // http.DefaultClient. The default client has no SSRF guard, so an admin diff --git a/internal/auth/oidc/service.go b/internal/auth/oidc/service.go index 681b8be..551838f 100644 --- a/internal/auth/oidc/service.go +++ b/internal/auth/oidc/service.go @@ -948,8 +948,19 @@ func (s *Service) fetchUserinfoGroups( if entry.provider.UserInfoEndpoint() == "" { return nil, fmt.Errorf("oidc: userinfo fallback configured but provider has no userinfo endpoint") } - ts := entry.oauthConfig.TokenSource(ctx, token) - uinfo, err := entry.provider.UserInfo(ctx, ts) + // Acquisition-audit SEC-020 closure (Sprint 1 follow-up to SEC-001, + // 2026-05-16). Wrap ctx via SafeOIDCContext before TokenSource + + // UserInfo so the SSRF guard owned by validation.SafeHTTPDialContext + // re-resolves the userinfo endpoint at dial time and refuses reserved + // addresses (loopback / link-local / cloud-metadata). The single wrap + // covers both legs because gooidc.ClientContext and oauth2.TokenSource + // both read the same oauth2.HTTPClient context key (see go-oidc/v3 + // oidc.go:57-65 and golang.org/x/oauth2 oauth2.go:339-341). Production + // provider-load paths in this package already use SafeOIDCContext; the + // userinfo fallback was missed in the SEC-001 sweep. + safeCtx := SafeOIDCContext(ctx) + ts := entry.oauthConfig.TokenSource(safeCtx, token) + uinfo, err := entry.provider.UserInfo(safeCtx, ts) if err != nil { return nil, fmt.Errorf("oidc: userinfo fetch: %w", err) } diff --git a/internal/auth/oidc/service_test.go b/internal/auth/oidc/service_test.go index 2c60dcf..6b1c1e2 100644 --- a/internal/auth/oidc/service_test.go +++ b/internal/auth/oidc/service_test.go @@ -19,11 +19,15 @@ import ( "github.com/go-jose/go-jose/v4" "github.com/go-jose/go-jose/v4/jwt" + "golang.org/x/oauth2" + + gooidc "github.com/coreos/go-oidc/v3/oidc" oidcdomain "github.com/certctl-io/certctl/internal/auth/oidc/domain" userdomain "github.com/certctl-io/certctl/internal/auth/user/domain" cryptopkg "github.com/certctl-io/certctl/internal/crypto" "github.com/certctl-io/certctl/internal/repository" + "github.com/certctl-io/certctl/internal/validation" ) // sha384New returns a SHA-384 hash via crypto/sha512 (Go stdlib). @@ -2400,3 +2404,106 @@ func TestService_UpsertUser_ValidateErrorOnEmptyEmail(t *testing.T) { t.Errorf("err = %v; want validate wrap", err) } } + +// Acquisition-audit SEC-020 closure (Sprint 1 follow-up to SEC-001, +// 2026-05-16). fetchUserinfoGroups previously called +// entry.provider.UserInfo(ctx, ts) with the bare request context. go-oidc +// /v3's Provider.UserInfo derives its http.Client from ctx via +// getClient(ctx) (oidc.go:61-65); without an override the internal +// doRequest falls through to http.DefaultClient — an unwrapped client +// with no SSRF guard. The fix wraps ctx via SafeOIDCContext so the +// dial-time SafeHTTPDialContext guard re-resolves the userinfo +// endpoint and rejects reserved-address answers. +// +// This test exercises the wrap end-to-end: +// +// 1. Stand up a discovery httptest server (loopback) whose discovery +// doc advertises userinfo_endpoint = "http://169.254.169.254/userinfo" +// (link-local cloud-metadata range — rejected by +// validation.SafeHTTPDialContext.isReservedIPForDial). +// 2. Construct the *gooidc.Provider via the test-bypassed +// oidcDiscoveryClient (setup_test.go's init() leaves it bypassed for +// the package). +// 3. Restore the production-shape oidcDiscoveryClient (the one whose +// Transport.DialContext is validation.SafeHTTPDialContext) BEFORE +// calling fetchUserinfoGroups, so the SafeOIDCContext wrap inside +// the function captures the production guard at ctx-wrap time. +// 4. Call fetchUserinfoGroups and assert the resulting error wraps the +// dial-time reserved-address rejection (substring "refusing to +// dial" / "reserved address"), not a generic transport error. +// +// The test does NOT use t.Parallel() — it mutates the package-level +// oidcDiscoveryClient and must run serially against any other test that +// reads the same var. +func TestFetchUserinfoGroups_SSRF_BlocksReservedAddress(t *testing.T) { + // Stand up a loopback discovery server. Discovery doc's + // userinfo_endpoint points at the link-local cloud-metadata IP so + // the subsequent UserInfo dial trips SafeHTTPDialContext. + var discoveryURL string + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + doc := map[string]interface{}{ + "issuer": discoveryURL, + "authorization_endpoint": discoveryURL + "/authorize", + "token_endpoint": discoveryURL + "/token", + "jwks_uri": discoveryURL + "/jwks", + "userinfo_endpoint": "http://169.254.169.254/userinfo", + "id_token_signing_alg_values_supported": []string{"RS256"}, + "response_types_supported": []string{"code"}, + "subject_types_supported": []string{"public"}, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(doc) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + discoveryURL = srv.URL + + // Build the *gooidc.Provider using the test-bypassed discovery + // client (setup_test.go init() already swapped oidcDiscoveryClient + // to a DefaultTransport-backed client so the httptest loopback URL + // resolves cleanly). + ctx := context.Background() + provider, err := gooidc.NewProvider(SafeOIDCContext(ctx), discoveryURL) + if err != nil { + t.Fatalf("NewProvider against loopback discovery server: %v", err) + } + if got := provider.UserInfoEndpoint(); got != "http://169.254.169.254/userinfo" { + t.Fatalf("provider.UserInfoEndpoint() = %q; want link-local override", got) + } + + // Restore the production-shape SafeHTTPDialContext-backed client + // just before the call. SafeOIDCContext inside fetchUserinfoGroups + // will pick THIS client up because gooidc.ClientContext reads the + // package-level var at wrap time. + saved := oidcDiscoveryClient + t.Cleanup(func() { oidcDiscoveryClient = saved }) + oidcDiscoveryClient = &http.Client{ + Timeout: oidcOutboundTimeout, + Transport: &http.Transport{ + DialContext: validation.SafeHTTPDialContext(oidcOutboundTimeout), + }, + } + + entry := &providerEntry{ + provider: provider, + oauthConfig: &oauth2.Config{ + ClientID: "test-client", + ClientSecret: "test-secret", + Endpoint: oauth2.Endpoint{TokenURL: discoveryURL + "/token"}, + }, + } + svc := &Service{} + _, err = svc.fetchUserinfoGroups(ctx, entry, &oauth2.Token{AccessToken: "test-access-token"}, "groups") + if err == nil { + t.Fatal("fetchUserinfoGroups against link-local userinfo endpoint: expected SSRF reject; got nil") + } + msg := err.Error() + // SafeHTTPDialContext emits one of two messages for the literal-IP + // case: "refusing to dial reserved address ". Either is the + // load-bearing signal we want — a generic connect-refused / EOF + // would mean the guard didn't fire. + if !strings.Contains(msg, "refusing to dial") && !strings.Contains(msg, "reserved address") { + t.Errorf("fetchUserinfoGroups err = %q; want SafeHTTPDialContext reserved-address rejection", msg) + } +}