fix(oidc): SEC-020 — wrap fetchUserinfoGroups via SafeOIDCContext

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.
This commit is contained in:
shankar0123
2026-05-16 16:41:05 +00:00
parent c4ed3da30b
commit 5d7bc86451
3 changed files with 145 additions and 2 deletions
+25
View File
@@ -9,6 +9,31 @@ package oidc
// - test_discovery.go:65 (dry-run validator from the GUI) // - test_discovery.go:65 (dry-run validator from the GUI)
// - service.go:1066 (runtime provider load on first cache miss) // - 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 // gooidc.NewProvider derives its HTTP client from the context via
// oidc.ClientContext; with no override it falls through to // oidc.ClientContext; with no override it falls through to
// http.DefaultClient. The default client has no SSRF guard, so an admin // http.DefaultClient. The default client has no SSRF guard, so an admin
+13 -2
View File
@@ -948,8 +948,19 @@ func (s *Service) fetchUserinfoGroups(
if entry.provider.UserInfoEndpoint() == "" { if entry.provider.UserInfoEndpoint() == "" {
return nil, fmt.Errorf("oidc: userinfo fallback configured but provider has no userinfo endpoint") return nil, fmt.Errorf("oidc: userinfo fallback configured but provider has no userinfo endpoint")
} }
ts := entry.oauthConfig.TokenSource(ctx, token) // Acquisition-audit SEC-020 closure (Sprint 1 follow-up to SEC-001,
uinfo, err := entry.provider.UserInfo(ctx, ts) // 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 { if err != nil {
return nil, fmt.Errorf("oidc: userinfo fetch: %w", err) return nil, fmt.Errorf("oidc: userinfo fetch: %w", err)
} }
+107
View File
@@ -19,11 +19,15 @@ import (
"github.com/go-jose/go-jose/v4" "github.com/go-jose/go-jose/v4"
"github.com/go-jose/go-jose/v4/jwt" "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" oidcdomain "github.com/certctl-io/certctl/internal/auth/oidc/domain"
userdomain "github.com/certctl-io/certctl/internal/auth/user/domain" userdomain "github.com/certctl-io/certctl/internal/auth/user/domain"
cryptopkg "github.com/certctl-io/certctl/internal/crypto" cryptopkg "github.com/certctl-io/certctl/internal/crypto"
"github.com/certctl-io/certctl/internal/repository" "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). // 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) 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 <ip>". 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)
}
}