From 2e9262cfb789395006085649cba6d32cdde281fd Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 16 May 2026 16:41:39 +0000 Subject: [PATCH] =?UTF-8?q?fix(handler):=20SEC-021=20=E2=80=94=20wrap=20BC?= =?UTF-8?q?L=20provider=20re-fetch=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). Companion to SEC-020 (prior commit). Closes the second of the two adjacent OIDC call sites the original SEC-001 sweep missed: the per-request discovery re-fetch in DefaultBCLVerifier.Verify. Pre-fix: func (v *DefaultBCLVerifier) Verify(ctx, logoutToken) { ... provider, perr := gooidc.NewProvider(ctx, matched.IssuerURL) ... } Same shape as service.go::fetchUserinfoGroups (closed in the prior commit) and service.go:1084 (closed by SEC-001 itself). go-oidc's NewProvider derives its http.Client from ctx; bare ctx falls through to http.DefaultClient at the discovery-doc + JWKS-fetch dial. An IdP whose registered IssuerURL resolves to a reserved address (or is rebinding to one at logout time) would trigger an unguarded HTTPS egress on every back-channel-logout request. Post-fix: provider, perr := gooidc.NewProvider( oidcsvc.SafeOIDCContext(ctx), matched.IssuerURL) The 'oidcsvc' alias for github.com/certctl-io/certctl/internal/auth/oidc is added to the import block (matches the canonical alias used in cmd/server/main.go:29). SafeOIDCContext routes the dial through validation.SafeHTTPDialContext, which re-resolves the issuer host at dial time and refuses reserved-address answers (loopback / link-local / 169.254.169.254 cloud-metadata). Files touched: internal/api/handler/auth_session_oidc_bcl.go — add oidcsvc import + wrap ctx at the NewProvider call site internal/api/handler/auth_session_oidc_bcl_test.go — NEW FILE. TestDefaultBCLVerifier_SSRF_BlocksReservedAddress constructs a stubProviderRepo with IssuerURL='http://127.0.0.1:1' (literal loopback — the IP-literal class that SafeHTTPDialContext. isReservedIPForDial refuses up-front, before any DNS resolution). Hand-rolls a 3-segment JWT whose payload base64url-decodes to {"iss":""} so peekIssuer extracts the matching issuer and provs.List() returns the seeded provider. Calls Verify and asserts the error wraps the dial-time reserved-address rejection (substring match on 'refusing to dial' / 'reserved address') AND that it's wrapped through the 'provider discovery:' prefix that distinguishes a discovery-time dial failure from a signature-verification failure. docs/operator/auth-threat-model.md — NEW subsection 'Userinfo + BCL SSRF parity (post-SEC-001 follow-up)' under '### Back-channel logout'. Documents both SEC-020 and SEC-021 closures, the context-key shape (why a single SafeOIDCContext wrap covers both go-oidc and oauth2 legs), and the out-of-scope RFC 1918 carve-out (covered separately by acquisition-audit Sprint 5 RED-005). Cross- references the two pinning tests by name so future audits can locate the load-bearing enforcement. Verified: gofmt -l internal/ docs/ (clean) go vet ./... (clean) go test -race -short ./internal/api/handler/... (all green) TestDefaultBCLVerifier_SSRF_BlocksReservedAddress (new; green) All 4 cited CI guards pass. Acceptance grep on the BCL handler: internal/api/handler/auth_session_oidc_bcl.go:132: provider, perr := gooidc.NewProvider(oidcsvc.SafeOIDCContext(ctx), matched.IssuerURL) No bare-ctx NewProvider remains in the BCL verifier. Combined with the SEC-020 commit, every gooidc.NewProvider + Provider.UserInfo call site in the production OIDC + BCL surface now routes through SafeOIDCContext. Closes acquisition-audit SEC-021. Sprint 1 ACQ is complete (2/2 findings). The single sprint shipped as two operator-authored commits (per-finding, mirrors the project's commit cadence for closures). --- docs/operator/auth-threat-model.md | 58 ++++++++++++++ internal/api/handler/auth_session_oidc_bcl.go | 9 ++- .../api/handler/auth_session_oidc_bcl_test.go | 77 +++++++++++++++++++ 3 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 internal/api/handler/auth_session_oidc_bcl_test.go diff --git a/docs/operator/auth-threat-model.md b/docs/operator/auth-threat-model.md index dc788c1..fb512c9 100644 --- a/docs/operator/auth-threat-model.md +++ b/docs/operator/auth-threat-model.md @@ -300,6 +300,64 @@ constant, router-level no-rbacGate-wraps-protocol-paths). attacks where an attacker captures a logout JWT and replays it. - **Cache-Control: no-store** on the response per spec §2.5. +### Userinfo + BCL SSRF parity (post-SEC-001 follow-up) + +The original SEC-001 closure (Sprint 1, 2026-05-16) routed two OIDC +discovery legs — `test_discovery.go` dry-run and `service.go` runtime +provider load — through `validation.SafeHTTPDialContext` via the +`SafeOIDCContext(ctx)` helper at +[`internal/auth/oidc/safehttp.go`](../../internal/auth/oidc/safehttp.go). +The acquisition-audit follow-up (2026-05-16) flagged two adjacent +call sites the sweep missed; both are now wrapped identically. + +- **SEC-020 — Userinfo fallback (`fetchUserinfoGroups`).** + `internal/auth/oidc/service.go` previously called + `entry.provider.UserInfo(ctx, ts)` with the bare request context + on the userinfo-fallback leg (operator opt-in when an IdP doesn't + surface groups in the ID token). 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` — 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, cloud-metadata `169.254.169.254`, RFC 1918) would + trigger an unguarded egress at userinfo-fetch time. Fixed by + wrapping `ctx` via `SafeOIDCContext(ctx)` before both + `oauthConfig.TokenSource` and `provider.UserInfo`. Pinned by + `TestFetchUserinfoGroups_SSRF_BlocksReservedAddress`. + +- **SEC-021 — Back-channel logout discovery re-fetch.** + `internal/api/handler/auth_session_oidc_bcl.go::Verify` performs + a per-request `gooidc.NewProvider(ctx, matched.IssuerURL)` to + fetch the JWKS for verifying the BCL token's signature. Same + bare-ctx shape — an IdP whose registered `IssuerURL` resolves to + a reserved address (or that is rebinding to one at logout time) + would dial an unguarded HTTPS egress. Fixed by wrapping via + `oidcsvc.SafeOIDCContext(ctx)` before `NewProvider`. Pinned by + `TestDefaultBCLVerifier_SSRF_BlocksReservedAddress`. + +- **Context-key shape (why a single wrap covers both legs).** + `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 (Provider.UserInfo, NewProvider + discovery, Verifier JWKS) AND oauth2-driven HTTP + (Config.TokenSource refresh, Config.Exchange). No additional + `context.WithValue(ctx, oauth2.HTTPClient, ...)` is required. + +- **Out-of-scope: RFC 1918.** Per the `IsReservedIP` policy + documented at [`internal/validation/ssrf.go:15-32`](../../internal/validation/ssrf.go), + RFC 1918 ranges are NOT treated as reserved by the SSRF guard. + certctl is designed to manage certificates inside private + networks; filtering 10/8 + 172.16/12 + 192.168/16 would break + the primary use case. Operators on hosted IaaS who want + RFC 1918 treated as reserved can opt in via the future + `CERTCTL_BLOCK_RFC1918_OUTBOUND` toggle (see acquisition-audit + Sprint 5 RED-005). The Sprint 1 SSRF parity fix above closes + the loopback / link-local / cloud-metadata leg only. + ### OIDC first-admin bootstrap - **Coexists with the env-var-token bootstrap path.** Both can be diff --git a/internal/api/handler/auth_session_oidc_bcl.go b/internal/api/handler/auth_session_oidc_bcl.go index ffbc018..7c7c9c5 100644 --- a/internal/api/handler/auth_session_oidc_bcl.go +++ b/internal/api/handler/auth_session_oidc_bcl.go @@ -14,6 +14,7 @@ import ( gooidc "github.com/coreos/go-oidc/v3/oidc" + oidcsvc "github.com/certctl-io/certctl/internal/auth/oidc" oidcdomain "github.com/certctl-io/certctl/internal/auth/oidc/domain" "github.com/certctl-io/certctl/internal/repository" ) @@ -122,7 +123,13 @@ func (v *DefaultBCLVerifier) Verify(ctx context.Context, logoutToken string) (is if v.verifyOverride != nil { idToken, err = v.verifyOverride(ctx, matched.IssuerURL, logoutToken) } else { - provider, perr := gooidc.NewProvider(ctx, matched.IssuerURL) + // Acquisition-audit SEC-021 closure (Sprint 1 follow-up to SEC-001, + // 2026-05-16). Per-request discovery re-fetch threaded through + // SafeOIDCContext so the dial-time SSRF guard + // (validation.SafeHTTPDialContext) re-resolves the issuer host and + // refuses reserved-address answers — matching the SEC-001 sweep + // over the runtime + dry-run discovery legs in internal/auth/oidc. + provider, perr := gooidc.NewProvider(oidcsvc.SafeOIDCContext(ctx), matched.IssuerURL) if perr != nil { return "", "", "", "", 0, fmt.Errorf("provider discovery: %w", perr) } diff --git a/internal/api/handler/auth_session_oidc_bcl_test.go b/internal/api/handler/auth_session_oidc_bcl_test.go new file mode 100644 index 0000000..246e4f4 --- /dev/null +++ b/internal/api/handler/auth_session_oidc_bcl_test.go @@ -0,0 +1,77 @@ +// Copyright 2026 certctl LLC. All rights reserved. +// SPDX-License-Identifier: BUSL-1.1 + +package handler + +import ( + "context" + "encoding/base64" + "strings" + "testing" + + oidcdomain "github.com/certctl-io/certctl/internal/auth/oidc/domain" +) + +// Acquisition-audit SEC-021 closure (Sprint 1 follow-up to SEC-001, +// 2026-05-16). DefaultBCLVerifier.Verify performs a per-request +// discovery re-fetch via gooidc.NewProvider(ctx, matched.IssuerURL). +// Pre-fix, the bare ctx fell through to http.DefaultClient at the dial +// layer — no SSRF guard, no DNS-rebinding re-resolve. The fix wraps +// ctx via oidcsvc.SafeOIDCContext so the dial-time +// validation.SafeHTTPDialContext refuses reserved-address answers +// (loopback / link-local / cloud-metadata). +// +// This test pins the wrap end-to-end: +// +// 1. Construct a stubProviderRepo with one provider whose IssuerURL is +// a literal-loopback http:// URL (the literal-IP class that +// SafeHTTPDialContext.isReservedIPForDial refuses up-front, before +// any DNS resolution attempt). +// 2. Hand-roll a 3-segment JWT whose payload base64url-decodes to +// {"iss":""} so peekIssuer extracts the matching +// issuer and provs.List() returns the seeded provider. +// 3. Call Verify. The discovery NewProvider call now routes through +// SafeOIDCContext; SafeHTTPDialContext sees the literal 127.0.0.1 +// and refuses with "refusing to dial reserved address ". +// 4. Assert the returned error wraps that rejection (substring match +// on "refusing to dial" / "reserved address") rather than a +// generic connect-refused or "did not respond" wrap. +// +// Companion to TestFetchUserinfoGroups_SSRF_BlocksReservedAddress in +// internal/auth/oidc/service_test.go which exercises the same wrap on +// the userinfo-fallback leg. Together they pin the post-SEC-001 sweep. +func TestDefaultBCLVerifier_SSRF_BlocksReservedAddress(t *testing.T) { + // Literal-loopback issuer URL. Port :1 keeps the URL syntactically + // valid; SafeHTTPDialContext refuses on the literal-IP check before + // the dial-time TCP connect, so the port choice is moot. + const reservedIssuer = "http://127.0.0.1:1" + + provs := &stubProviderRepo{ + provs: []*oidcdomain.OIDCProvider{ + {ID: "op-loopback", IssuerURL: reservedIssuer, ClientID: "test-client"}, + }, + } + v := NewDefaultBCLVerifier(provs, "t-default", nil) + + // Hand-roll the JWT. peekIssuer (see auth_session_oidc_bcl.go) parses + // only the iss claim from the 2nd segment (payload), so the header + + // signature segments only need to be syntactically present. + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"RS256"}`)) + payload := base64.RawURLEncoding.EncodeToString([]byte(`{"iss":"` + reservedIssuer + `"}`)) + logoutToken := header + "." + payload + ".sig" + + _, _, _, _, _, err := v.Verify(context.Background(), logoutToken) + if err == nil { + t.Fatal("Verify against literal-loopback issuer URL: expected SSRF reject; got nil") + } + msg := err.Error() + if !strings.Contains(msg, "refusing to dial") && !strings.Contains(msg, "reserved address") { + t.Errorf("Verify err = %q; want SafeHTTPDialContext reserved-address rejection", msg) + } + // Also confirm the error is wrapped through the Verify "provider + // discovery:" prefix so callers can distinguish a discovery-time + // dial failure from a signature-verification failure. + if !strings.Contains(msg, "provider discovery") { + t.Errorf("Verify err = %q; want \"provider discovery:\" wrap", msg) + } +}