fix(security): dial-time SSRF guard on shared OIDC HTTP client (bookshelf-qapga) #1405
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-qapga"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Follow-up to #1396 (bookshelf-qapga). Ports cover's dial-time SSRF guard onto
internal/users' shared
oidcHTTPClientto close a DNS-rebinding/TOCTOU gap: ahostname-based
jwks_uri(or any OIDC discovery/JWKS request) that resolvesto a private/loopback/metadata IP only at connect time was not caught by the
existing pre-fetch
validateFetchableDiscoveryURLhost check.safeDialContext/safeDialContextWithResolver/safeTransport/isPrivateIPinto a new sharedinternal/netguardpackage (resolve hostname once, validate every returned IP, dial the validated literal directly — no re-resolution TOCTOU).internal/cover/download.gonow delegates to it with no behavior change.netguard.SafeTransport()ontooidcHTTPClient(internal/users/oidc_jwks.go). SinceDiscoverOIDCMeta/FetchJWKSshare this one client, the fix applies symmetrically to both the admin Test Connection diagnostic and the production login-time JWKS fetch (newOIDCJWKSCache -> FetchJWKS, wired inwire.go) — the login path previously had no host-validation guard at all.oidcHTTPClientis also used for real OAuth2 discovery/token/userinfo against trusted issuers, and most existing tests point it athttptest.Serveron 127.0.0.1 via the existingSetOIDCHTTPClientescape hatch.users_suite_test.gonow installs a package-wide test-mode client (plain transport, sameCheckRedirectpolicy) via a globalBeforeEach, while capturing the real production client inBeforeSuite. A newoidc_ssrf_transport_test.gore-installs that captured production client to prove the loopback/private-IP block, the DNS-rebinding-shaped case (localhost), the realCheckRedirectclosure, and that the escape hatch still works.Extraction vs. inline port: chose extraction into
internal/netguard(project convention favors DRY reuse over duplicating ~90 lines of dial-guard logic across two packages); the alternative (duplicating the guard directly intooidc_jwks.go) was rejected as it would need to be kept in sync with cover's copy by hand.Test plan
internal/netguardnew black-box suite, 100% coverage (rebinding-style resolver stub rejected, public-IP resolver allowed, CGNAT/loopback/link-local ranges,SafeTransportfallback path,SetOIDCHTTPClientescape hatch).internal/coverexisting suite passes unmodified (delegates via export_test.go aliases).internal/usersexisting suite passes (1032+ specs) after installing the suite-wide test-mode client.oidc_ssrf_transport_test.go: production wiring rejects loopback IP literal, rejects a DNS-rebinding-shaped hostname, realCheckRedirectclosure still refuses redirects,SetOIDCHTTPClientescape hatch still bypasses the guard.make build,make test,make lint(scoped to changed packages — a stale sibling worktree pollutes the repo-widegolangci-lintrun, unrelated to this change),./scripts/check-coverage.shall green locally.Closes bead bookshelf-qapga on merge.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Security Review — PR #1405 (bd-bookshelf-qapga)
Reviewed both commits (
02378b044dial-time SSRF guard,1316b5691loopback exemption fix) against the diff basec833c0e23.TOCTOU / DNS-rebinding closure: Confirmed closed.
netguard.dialWithResolverAndBlocklistresolves the hostname exactly once via the injected resolver, validates every returnednet.IPAddragainst the blocklist, then dialsnet.JoinHostPort(addrs[0].IP.String(), port)— the validated IP literal, never the hostname. Verified no code path re-passes the original hostname to the dialer. Redirects:internal/cover'ssafeCheckRedirectallows up to 5 hops but every redirect's TCP dial goes back through the sameSafeDialContext-backed transport (validated per-hop, sincehttp.Transport.DialContextis invoked again on every redirect connection) — good. OIDC'soidcHTTPClient.CheckRedirectrefuses all redirects outright, closing the redirect-to-internal-target vector entirely for OIDC. Confirmed viagit diffthat production login (newOIDCJWKSCache → FetchJWKS, wired inwire.go:184) and the admin Test Connection diagnostic (DiscoverOIDCMeta/FetchJWKS) share the singleoidcHTTPClientpackage var — so the production login-time JWKS fetch (the bead's key gap) genuinely gets the guard, not just the diagnostic. Before this PR,oidcHTTPClienthad noTransportfield set at all (plain zero-valuehttp.Client{}, i.e.http.DefaultTransport, fully unguarded) — confirmed via diff.Blocklist completeness: Verified programmatically (
net.ParseCIDR/net.IPbehavior) that IPv4-mapped IPv6 addresses correctly collapse vianet.IP.To4()before bothnet.IPNet.Containsandnet.IP.IsLinkLocalUnicast()/IsPrivate(), so::ffff:169.254.169.254and::ffff:10.1.1.1are correctly caught (ipnet.ContainsandIsPrivateboth returnedtruein a standalone repro). RFC1918, loopback (v4+v6), link-local incl. metadata 169.254.169.254, CGNAT 100.64.0.0/10, IPv6 ULA fc00::/7, ::1, 0.0.0.0/8, ::/128 are all present inprivateRanges+ stdlib checks. No missing range found.Loopback exemption (OIDC only):
IsPrivateIPExceptLoopbackexempts ONLY loopback (!ip.IsLoopback() && IsPrivateIP(ip)) — RFC-1918, link-local/metadata, CGNAT, ULA, unspecified remain blocked for OIDC.internal/covercontinues to use the strictnetguard.SafeTransport()(no exemption) — confirmed cover/download.go does not reference theAllowingLoopbackvariants. The exemption is justified in-repo byinternal/settings.validateIssuerURI's pre-existing accepted carve-out for a self-hostedhttp://localhostIdP, and is exercised end-to-end by the e2e fake-IdP journey per the commit message.Escape hatch:
SetOIDCHTTPClientlives only ininternal/users/export_test.go, a_test.gofile excluded from the production binary by the Go toolchain — not reachable in prod.Test coverage:
internal/netguardis properly wired intoUNIT_PKGS(Makefile) andcheck-coverage.sh.oidc_ssrf_transport_test.goexercises both the RFC-1918/metadata-IP rejection and the loopback-allow path against the real production client, plus the CheckRedirect closure and the escape hatch.One documentation defect found:
[MINOR] internal/settings/oidc_settings.go:590 — stale comment says wrong transport function
The comment states "oidcHTTPClient ... is wired with netguard.SafeTransport()", but per the second commit (
1316b5691) the actual wiring isnetguard.SafeTransportAllowingLoopback(). This comment was written in the first commit (02378b044) and not updated when the loopback exemption was added in the follow-up commit, so it now misstates the security posture at the exact point most likely to be read by a future reviewer evaluatingvalidateIssuerURI's risk model (a reader could wrongly conclude loopback is blocked at the transport layer here too). Fix: update the comment to saySafeTransportAllowingLoopback()and note the loopback exemption, matching the accurate doc comment already present ininternal/users/oidc_jwks.go.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Code Review — PR #1405 (bd-bookshelf-qapga)
Phase 1 — Spec compliance
Extraction into
internal/netguardis faithful (see Phase 2 below); wiring covers bothDiscoverOIDCMeta/FetchJWKScall sites (Test Connection diagnostic + productionnewOIDCJWKSCache) since both shareoidcHTTPClient. Docs: N/A is correctly justified (internal security hardening, no user-facing surface).Phase 2 — Findings
[BLOCKER] internal/users/oidc_jwks.go:78 — loopback exemption reopens the exact discovery-derived-URL SSRF class this PR's sibling code says must stay closed
oidcHTTPClientis the single shared client for: (1)DiscoverOIDCMetaagainst the admin-configured, trusted issuer, AND (2) every subsequent IdP-supplied endpoint pulled out of that discovery document —jwks_uri(FetchJWKS, called from bothcheckOIDCJWKSin internal/settings/oidc_settings.go:805 and the productionnewOIDCJWKSCache → FetchJWKSpath wired in internal/users/wire.go:95), plustoken_endpoint/userinfo_endpoint. Those discovery-response fields are explicitly documented as untrusted/attacker-influenceable —internal/settings/oidc_settings.go:582-583: "discovery response fields are never admin-typed trusted config, so we hold them to the stricter rule", andvalidateFetchableDiscoveryURL's own doc-comment says "there is no http/loopback exception here".But the actual backstop for hostname-based (non-literal-IP) discovery-response URLs — which
validateFetchableDiscoveryURLexplicitly defers to the transport layer for (oidc_settings.go:583-584, "Hostnames that are not raw IP literals are allowed through this check (DNS resolution happens at fetch time)") — isnetguard.SafeTransportAllowingLoopback()(oidc_jwks.go:78), which does carve out a loopback exception (IsPrivateIPExceptLoopback). So a malicious or compromised IdP can returnjwks_uri(ortoken_endpoint/userinfo_endpoint) as a hostname that resolves to127.0.0.1/::1, and:validateFetchableDiscoveryURLcheck does not catch it (only literal IPs are checked at that layer).Net effect: an IdP the admin trusts as an OIDC provider can pivot an SSRF into the pergamum host's own loopback-bound services (e.g.
/debug/pprof,/metrics, any other localhost-only admin surface) via a craftedjwks_uri/token_endpoint/userinfo_endpoint— every login, not just the Test Connection diagnostic, since the production path (newOIDCJWKSCache → FetchJWKS) has no pre-fetch host check at all and relies 100% on this transport.This is exactly the failure mode
validateIssuerURI's loopback carve-out was scoped to avoid extending — it exists only for the admin-typed, trusted issuer URL, not for provider-supplied response fields. The extraction/wiring in this PR silently widened that carve-out to cover every request the shared client makes.Fix: don't reuse one loopback-permissive client for both trust levels. Options: (a) use
netguard.SafeTransport()(no loopback exception) forFetchJWKS/token/userinfo requests and reserveSafeTransportAllowingLoopback()only for the initialDiscoverOIDCMetarequest to the admin-configured issuer; (b) splitoidcHTTPClientinto two clients (discovery-only allows loopback, everything IdP-supplied does not); (c) at minimum, if the local-self-hosted-IdP scenario is expected to also serve JWKS/token/userinfo on loopback (which it plausibly does, since it's the same host), validate that the resolved IP forjwks_uri/etc. matches the resolved IP of the already-validated issuer, not merely "any loopback address is fine."[MINOR] internal/settings/oidc_settings.go:590-591 / internal/users/oidc_jwks.go:66,76-78 — doc comments say
netguard.SafeTransport(), code wiresnetguard.SafeTransportAllowingLoopback()Both the updated doc-comment in
validateFetchableDiscoveryURL("is wired with netguard.SafeTransport()...") and the PR description ("Wirednetguard.SafeTransport()ontooidcHTTPClient") describe the strict (no-loopback) helper, while the code actually installsSafeTransportAllowingLoopback(). This is a symptom of the BLOCKER above — the comment describes the behavior the authors intended/believed they shipped. Once the BLOCKER is fixed, reconcile the comment with whichever transport is actually used at each call site.What's solid
internal/cover/download.goextraction is a true behavior-preserving move:ErrPrivateAddress,privateRanges(all 11 CIDRs incl. CGNAT/this-network),isPrivateIP's exact boolean logic, the IP-literal fast path, the resolve-once/validate-every-IP/dial-the-literal flow, thelen(addrs)==0guard, andsafeTransport'shttp.DefaultTransportclone-and-fallback are all present verbatim ininternal/netguard/dial.go, just renamed to exported symbols. Cover's own test suite (download_test.go,export_test.go) is untouched in the diff and still compiles/passes via the new local var-aliases (internal/cover/download.go:39-44), confirming zero behavior drift on the cover path.netguard.SafeTransport()/SafeDialContext(no loopback exemption) — the loopback carve-out is scoped only to the OIDC variant, as intended for the cover path.internal/netguard/dial_test.gois properly black-box (package netguard_test), usesDescribeTable/Itwith one assertion perIt, and exercises all documented branches (CIDR panics, empty-resolver-result, mixed-IP-list, DNS-rebinding-shaped resolver stub,SafeTransport's non-*http.Transportfallback).internal/users/export_test.go's newExportOIDCHTTPClientis consistent with the pre-existing, allowlisted white-box exception forinternal/users(nnb9.30 black-box conversion still pending; file is already inscripts/test_policy_check/allowlist.txtand the diff doesn't touch the allowlist). The new spec fileoidc_ssrf_transport_test.goitself is proper black-box (package users_test), using only exportedSetOIDCHTTPClient/ExportOIDCHTTPClient/DiscoverOIDCMeta/FetchJWKS.MakefileUNIT_PKGS andscripts/check-coverage.shboth add./internal/netguard/...— coverage wiring is correct.REVIEW VERDICT: 1 blocker, 0 major, 1 minor
Security RE-REVIEW — PR #1405 (bd-bookshelf-qapga), SHA
5e75a7c2Verifying the fix for the BLOCKER in comment #17278 (loopback-exemption pivot via discovery-supplied
jwks_uri/token_endpoint/userinfo_endpoint).Verification performed
Issuer provenance at every threaded call site — traced all uses of
oidcClientForIssuer(issuer):DiscoverOIDCMeta(ctx, issuer)→ issuer is the raw admin-configured value (fromTestOIDCConnectionResult'sissuerURIparam, orcfg.Issuerin the login callback). Nevermeta.Issuer.FetchJWKS(ctx, jwksURI, issuer)→ every caller passescfg.Issuer(oidcVerifyAndProvision,fetchBackchannelJWKS) or the validatedissuerURI(checkOIDCJWKSininternal/settings/oidc_settings.go), nevermeta.JWKSURI/discovery-derived data.oidcExchangeCode(ctx, ..., issuer, ...)→ called withcfg.Issuer(internal/users/oidc_service.go).fetchUserinfoClaims(ctx, userinfoURL, ..., issuer, ...)→ called withcfg.Issuer.oidcJWKSCache.Fetch/Refresh(ctx, jwksURI, issuer)and the backchanneljwksCache.getJWKS→issuerthreaded through unchanged ascfg.Issuer, survives caching/singleflight keyed onjwksURI(notissuer, which is correct since jwksURI is the cache key and issuer is only used for the transport decision on each underlying fetch).issuerargument. Gate is sound end-to-end.Threat matrix — verified via
internal/users/oidc_ssrf_transport_test.go(real end-to-end specs, not mocks) plus source review ofinternal/netguard/dial.go:jwks_uri(IP literal127.0.0.1or hostnamelocalhost) → BLOCKED (oidc_ssrf_transport_test.go: "blocks FetchJWKS when jwks_uri resolves to loopback").jwks_uri→ allowed (self-hosted local IdP case preserved, matchesvalidateIssuerURI's existing accepted exception).169.254.169.254) → blocked regardless of issuer (loopback-allowing transport only exemptsIsPrivateIPExceptLoopback, metadata IP is link-local, still denied).netguard.dialWithResolverAndBlocklistresolves the hostname exactly once, validates every returned IP, dials the validated literal directly (never re-resolves at connect time) — closes the TOCTOU windowvalidateFetchableDiscoveryURL's doc comment explicitly defers to the transport layer.refuseOIDCRedirect, shared by both clients) — closes the "validated URL redirects to a restricted target" bypass.isLoopbackHost/isLoopbackIssuer(internal/users/oidc_jwks.go): handles"localhost"(case-insensitive), IP-literal loopback vianet.ParseIP(h).IsLoopback()(covers 127.0.0.0/8,::1, and IPv4-mapped IPv6 loopback since Go'sIsLoopbackchecksTo4()first). Fails CLOSED on an unparseable issuer (url.Parseerror →isLoopbackIssuerreturnsfalse→ strict client) and on an empty issuer (empty hostname matches neither branch →false→ strict client). Verified by the "issuer is not a parseable URL... fails closed" spec.SetOIDCHTTPClientescape hatch:oidcClientForIssuerchecksoidcHTTPClient != defaultOIDCHTTPClient(pointer inequality against the value captured at package init) and honors an override unconditionally when set — test-only, since production code never callsSetOIDCHTTPClient(it's declared inexport_test.go). No production bypass introduced; confirmed no non-test file referencesSetOIDCHTTPClient.Test hygiene:
internal/netguard/dial_test.go,netguard_suite_test.go,internal/users/oidc_ssrf_transport_test.go,internal/users/users_suite_test.goare allpackage <pkg>_test(black-box), no new.golangci.ymlexclusions added.internal/users/export_test.go's new exports (ExportOIDCHTTPClient, updatedExportFetchJWKS/ExportDiscoverOIDCMetasignatures) are consistent with the pre-existing allowlisted white-box exception for that package (nnb9.30) and don't leak the SSRF gate itself (they expose the client/functions, not a way to force the loopback branch outside the documented issuer-based selection).Doc-comment MINOR from the original review — resolved:
validateFetchableDiscoveryURL's comment (oidc_settings.go:589) now correctly describesnetguard.SafeTransport()as the default with the loopback-allowing sibling scoped to a loopback issuer, matching the actual wiring.Findings
None. The fix closes the BLOCKER: the loopback-allowing transport is now selected only when the trusted, admin-configured issuer is itself a loopback host, and every threaded
issuerparameter traces back to admin-configured config, never a discovery-response field. CI is green (SHA5e75a7c2,success) and the PR is mergeable.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code RE-REVIEW of the BLOCKER fix on PR #1405 (SHA
5e75a7c2). Diff-review only, focused on commits34a08b926and5e75a7c24layered on the prior netguard extraction.Summary of what was verified:
oidcClientForIssuerselection:oidcHTTPClient != defaultOIDCHTTPClientpointer-inequality correctly detects aSetOIDCHTTPClienttest override and takes priority over the loopback-issuer check;SetOIDCHTTPClient's restore closure correctly resets the pointer toprev, so nesting/cleanup works. Not "always returns one client" — confirmed both branches (strictoidcHTTPClientvsoidcHTTPClientLoopback) are reachable and covered byoidc_ssrf_transport_test.go.isLoopbackHost/isLoopbackIssuer: correctly matches"localhost"(case-insensitive) and IP-literalIsLoopback(); on aurl.Parseerror, returnsfalse→ fails closed to the strict (loopback-blocking) client. Verifiedhttp://[::1(unterminated bracket) is a genuineurl.Parseerror, confirming the "unparseable issuer fails closed" test case is real, not a false negative.DiscoverOIDCMeta(issuer)→oidcClientForIssuer(issuer)uses the issuer itself (correct — discovery target IS the trusted issuer);FetchJWKS(jwksURI, issuer)called fromoidcVerifyAndProvisionwithcfg.Issuer(oidc_service.go:293/302), fromfetchBackchannelJWKSwith the backchannel'sissuerparam (oidc_backchannel.go:238), and fromcheckOIDCJWKS/buildOIDCChecks/TestOIDCConnectionResultwith the admin-typedissuerURIunder test (oidc_settings.go).fetchUserinfoClaims(..., cfg.Issuer, ...)(oidc_service.go:316) andoidcExchangeCode(..., cfg.Issuer, ...)(oidc_service.go:478) both correctly use the trusted admin-configured issuer, never the untrusted discovery-supplied endpoint URL. No call site found passing a mislabeled string (e.g. jwksURI as issuer) in production code — the one test that does (oidc_jwks_test.go:123,ExportFetchJWKS(GinkgoT(), jwksURI, jwksURI)) runs under the suite-wideBeforeEachtest-mode client override (users_suite_test.go), which bypasses the loopback decision entirely, so the mismatch is inert.oidcJWKSCache/newOIDCJWKSCache/Fetch/Refreshsignatures consistently updated to(ctx, jwksURI, issuer); cache is keyed byjwksURIonly (notissuer), but since a cache hit never triggers a new dial, this isn't a new SSRF vector — worst case is stale-but-already-legitimately-fetched bytes served under a different issuer's cache key collision, not a fresh network request steered anywhere new..golangci.ymlexclusions (diffed clean vs origin/main), no coverage-script exclusion additions, all new/changed test files remainpackage users_test/package settings_test(black-box), one-Expect-per-It maintained in the newoidc_ssrf_transport_test.gospecs.export_test.gochanges are signature updates to already-existing exported test hooks (ExportFetchJWKS,ExportNewOIDCJWKSCache), not new test-only exports — no nnb9 concern.No blockers or majors found in this fix. One minor style note below.
[MINOR] internal/users/oidc_cache.go:120 — JWKS cache keyed by URI only, not (issuer, URI)
oidcJWKSCache.entriesis keyed solely byjwksURI. Not a security issue today (a cache hit never re-dials, so no new SSRF surface), but it's a latent footgun: if two different admin-configured issuers ever legitimately share ajwks_uristring (unusual but not impossible for multi-tenant discovery documents), the second issuer's fetch would silently reuse the first's cached bytes without re-validating via its ownoidcClientForIssuerdecision. Consider keying by(issuer, jwksURI)if multi-issuer support is ever added; not worth blocking this PR over.REVIEW VERDICT: 0 blocker, 0 major, 1 minor