From e6cfd756ac85d766a1077cf916b6888d8a88946d Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 16 May 2026 03:31:42 +0000 Subject: [PATCH] =?UTF-8?q?fix(auth):=20SEC-001=20=E2=80=94=20gate=20OIDC?= =?UTF-8?q?=20discovery=20through=20SafeHTTPDialContext=20+=20ValidateSafe?= =?UTF-8?q?URL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sprint 1 unified-master-audit closure. Two OIDC discovery call sites passed the bare request context to gooidc.NewProvider: - internal/auth/oidc/test_discovery.go:65 (dry-run validator) - internal/auth/oidc/service.go:1066 (runtime cache load) gooidc.NewProvider derives its HTTP client from the context via oidc.ClientContext; with no override it falls through to http.DefaultClient — no SSRF guard. An admin with auth.oidc.create could induce server-side HTTPS egress to loopback (127.0.0.1, ::1), RFC 1918, link-local (169.254.169.254 — cloud-instance metadata), and IPv6 link-local (fe80::/10). The companion JWKS reachability probe was already routed through SafeHTTPDialContext via the Bundle 5 R6 closure; the discovery + claims path bypassed that. Fix: - New internal/auth/oidc/safehttp.go: oidcDiscoveryClient (Transport DialContext = validation.SafeHTTPDialContext) + SafeOIDCContext helper. Both call sites now wrap ctx through SafeOIDCContext before NewProvider runs. - Defense-in-depth: OIDCProvider.Validate calls validation.ValidateSafeURL on the IssuerURL after the existing https/parse checks, refusing reserved-address issuers at provider-creation time. - TestDiscovery surfaces the SSRF policy error via the result's Errors slice up-front (early-fail UX rail) before invoking NewProvider. Test seams: - setup_test.go swaps oidcDiscoveryClient + validateIssuerSSRF for httptest loopback compatibility, mirroring the existing jwksProbeClient pattern. Regression coverage: - internal/auth/oidc/domain/types_test.go: 5-case table pinning loopback v4/v6, cloud metadata, link-local v4/v6 rejection. - internal/auth/oidc/coverage_fill_test.go: same 5 cases against Service.TestDiscovery via temporarily restoring the production gate. Closes SEC-001. --- internal/auth/oidc/coverage_fill_test.go | 50 ++++++++++++ internal/auth/oidc/domain/types.go | 11 +++ internal/auth/oidc/domain/types_test.go | 35 +++++++++ internal/auth/oidc/safehttp.go | 97 ++++++++++++++++++++++++ internal/auth/oidc/service.go | 9 ++- internal/auth/oidc/setup_test.go | 10 +++ internal/auth/oidc/test_discovery.go | 22 +++++- 7 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 internal/auth/oidc/safehttp.go diff --git a/internal/auth/oidc/coverage_fill_test.go b/internal/auth/oidc/coverage_fill_test.go index 10302ee..24bdc9f 100644 --- a/internal/auth/oidc/coverage_fill_test.go +++ b/internal/auth/oidc/coverage_fill_test.go @@ -7,6 +7,8 @@ import ( "net/http/httptest" "strings" "testing" + + "github.com/certctl-io/certctl/internal/validation" ) // Coverage fill — v2.1.0 release gate Phase 3. @@ -59,6 +61,54 @@ func TestJWKSStatus_ReturnsSnapshot_AfterAuthRequestPopulatesEntry(t *testing.T) } } +// TestTestDiscovery_RejectsSSRFIssuer_AtEarlyFailRail pins the +// SEC-001 closure (Sprint 1, 2026-05-16): TestDiscovery refuses +// reserved-address issuers up-front via validateIssuerSSRF, surfacing +// a clean "issuer_url failed SSRF policy" error in the result's +// Errors slice without ever hitting the dial path. The package-wide +// setup_test.go init() swaps validateIssuerSSRF to a no-op so the +// other tests can use httptest loopback servers; this test temporarily +// restores the production gate (validation.ValidateSafeURL) and +// asserts the rejection fires. +func TestTestDiscovery_RejectsSSRFIssuer_AtEarlyFailRail(t *testing.T) { + prev := validateIssuerSSRF + validateIssuerSSRF = validation.ValidateSafeURL + defer func() { validateIssuerSSRF = prev }() + + svc := newServiceForUnitTest(t) + cases := []struct { + name string + issuer string + }{ + {"loopback_v4", "https://127.0.0.1/realms/certctl"}, + {"loopback_v6", "https://[::1]/realms/certctl"}, + {"cloud_metadata", "https://169.254.169.254/latest/meta-data/"}, + {"link_local_v4", "https://169.254.10.5/realms/certctl"}, + {"link_local_v6", "https://[fe80::1]/realms/certctl"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res, err := svc.TestDiscovery(context.Background(), tc.issuer) + if err != nil { + t.Fatalf("TestDiscovery (non-fatal): %v", err) + } + if res == nil { + t.Fatalf("expected non-nil result") + } + if res.DiscoverySucceeded { + t.Errorf("expected DiscoverySucceeded=false for SSRF issuer; got true") + } + if len(res.Errors) == 0 { + t.Fatalf("expected non-empty Errors slice") + } + joined := strings.Join(res.Errors, "|") + if !strings.Contains(joined, "SSRF policy") { + t.Errorf("expected 'SSRF policy' in errors; got %v", res.Errors) + } + }) + } +} + // TestTestDiscovery_DiscoveryFailure_ReturnsErrorsSlice points // TestDiscovery at a URL that doesn't serve a discovery doc; the // function MUST return res with DiscoverySucceeded=false and a diff --git a/internal/auth/oidc/domain/types.go b/internal/auth/oidc/domain/types.go index 9d4cb1d..b8dbc00 100644 --- a/internal/auth/oidc/domain/types.go +++ b/internal/auth/oidc/domain/types.go @@ -22,6 +22,7 @@ import ( "time" authdomain "github.com/certctl-io/certctl/internal/domain/auth" + "github.com/certctl-io/certctl/internal/validation" ) // OIDCProvider describes a configured OpenID Connect identity provider @@ -160,6 +161,16 @@ func (p *OIDCProvider) Validate() error { if _, err := url.Parse(p.IssuerURL); err != nil { return fmt.Errorf("oidc: issuer_url is not a valid URL: %w", err) } + // SEC-001 closure (Sprint 1, 2026-05-16): reject reserved-address + // issuers (loopback / RFC 1918 / link-local / cloud metadata) at + // provider-creation time. Defense-in-depth alongside + // oidc.SafeOIDCContext, which is the authoritative dial-time + // re-resolution + reject. The static URL check stops the obvious + // case ("https://169.254.169.254/...") before the row is persisted + // or the dry-run validator runs. + if err := validation.ValidateSafeURL(p.IssuerURL); err != nil { + return fmt.Errorf("oidc: issuer_url failed SSRF policy: %w", err) + } if strings.TrimSpace(p.ClientID) == "" { return ErrOIDCEmptyClientID } diff --git a/internal/auth/oidc/domain/types_test.go b/internal/auth/oidc/domain/types_test.go index 5edc977..b3dd073 100644 --- a/internal/auth/oidc/domain/types_test.go +++ b/internal/auth/oidc/domain/types_test.go @@ -82,6 +82,41 @@ func TestOIDCProvider_Validate_RejectsNonHTTPSIssuer(t *testing.T) { } } +// SEC-001 closure (Sprint 1, 2026-05-16). The IssuerURL Validate gate +// now refuses reserved-address issuers (loopback, RFC 1918, +// link-local, IPv6 loopback, IPv6 link-local, cloud metadata) so a +// row claiming https://127.0.0.1/... or https://169.254.169.254/... +// never makes it to the persistence layer or the runtime discovery +// dial. Authoritative dial-time rejection lives in +// internal/validation.SafeHTTPDialContext (DNS-rebinding-safe); this +// test pins the static URL gate that surfaces the policy violation +// with a clean error before any network I/O. +func TestOIDCProvider_Validate_RejectsSSRFIssuer(t *testing.T) { + cases := []struct { + name string + issuer string + }{ + {"loopback_v4", "https://127.0.0.1/realms/certctl"}, + {"loopback_v6", "https://[::1]/realms/certctl"}, + {"cloud_metadata", "https://169.254.169.254/latest/meta-data/"}, + {"link_local_v4", "https://169.254.10.5/realms/certctl"}, + {"link_local_v6", "https://[fe80::1]/realms/certctl"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := validProvider() + p.IssuerURL = tc.issuer + err := p.Validate() + if err == nil { + t.Fatalf("issuer=%q: Validate returned nil; want SSRF policy rejection", tc.issuer) + } + if !strings.Contains(err.Error(), "SSRF policy") { + t.Errorf("issuer=%q: err=%v; want error mentioning SSRF policy", tc.issuer, err) + } + }) + } +} + func TestOIDCProvider_Validate_RejectsEmptyClientID(t *testing.T) { p := validProvider() p.ClientID = "" diff --git a/internal/auth/oidc/safehttp.go b/internal/auth/oidc/safehttp.go new file mode 100644 index 0000000..03012b4 --- /dev/null +++ b/internal/auth/oidc/safehttp.go @@ -0,0 +1,97 @@ +// Copyright 2026 certctl LLC. All rights reserved. +// SPDX-License-Identifier: BUSL-1.1 + +package oidc + +// SEC-001 closure (Sprint 1, 2026-05-16). Pre-fix, two OIDC discovery +// call sites passed the bare request context to gooidc.NewProvider: +// +// - test_discovery.go:65 (dry-run validator from the GUI) +// - service.go:1066 (runtime provider load on first cache miss) +// +// 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 +// with `auth.oidc.create` could induce server-side HTTPS egress to +// loopback (127.0.0.1, ::1), RFC 1918 (10/8 / 172.16/12 / 192.168/16), +// link-local (169.254.169.254 — cloud-instance metadata), and IPv6 +// link-local (fe80::/10). +// +// The companion JWKS reachability probe (jwksReachable + jwksProbeClient +// in this package) was already routed through SafeHTTPDialContext via +// the Bundle 5 R6 closure; the discovery + claims path bypassed that +// guard. +// +// This file adds the symmetric guard for the discovery leg: +// +// - oidcDiscoveryClient — an *http.Client wrapping a Transport whose +// DialContext is SafeHTTPDialContext, sized to the same outbound +// budget as jwksProbeClient (oidcOutboundTimeout = 10s). +// - SafeOIDCContext(ctx) — returns a context that gooidc.NewProvider +// and the resulting Verifier will use for every outbound call. +// +// The two call sites above are rewritten to thread their context through +// SafeOIDCContext before NewProvider runs. The fail-closed posture is +// owned by validation.SafeHTTPDialContext — DNS-rebinding-safe by +// re-resolving at dial time and rejecting any reserved address that +// surfaces in the resolution. +// +// Defense-in-depth: domain/types.go.Validate also calls +// validation.ValidateSafeURL on the persisted IssuerURL at provider- +// creation time so reserved-address issuers fail before they ever reach +// the cache + dial path. + +import ( + "context" + "net/http" + "time" + + gooidc "github.com/coreos/go-oidc/v3/oidc" + + "github.com/certctl-io/certctl/internal/validation" +) + +// oidcDiscoveryClient is the *http.Client gooidc.NewProvider uses for +// the discovery doc fetch + the per-Verifier JWKS read it issues +// internally on first sig-verify. Routed through SafeHTTPDialContext +// so the dial-time guard re-resolves the issuer host and rejects +// loopback / link-local / private / cloud-metadata before any HTTP +// byte goes out. Mirrors jwksProbeClient (test_discovery.go) so both +// outbound paths share an identical SSRF posture. +// +// Package-level var so the test suite can swap it for an +// SSRF-guard-bypassed client when exercising the discovery code path +// against httptest.NewServer (which binds to 127.0.0.1 and would +// otherwise be refused). Mirrors the webhook/slack/teams test-seam +// pattern. Production code never reassigns this var. +var oidcDiscoveryClient = &http.Client{ + Timeout: oidcOutboundTimeout, + Transport: &http.Transport{ + DialContext: validation.SafeHTTPDialContext(oidcOutboundTimeout), + MaxIdleConns: 10, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + }, +} + +// SafeOIDCContext returns a derived context that carries the SSRF-safe +// discovery http.Client. Pass the result to gooidc.NewProvider so that +// the discovery doc fetch + the internal JWKS fetch the resulting +// Verifier issues both run through SafeHTTPDialContext. +// +// Callers SHOULD use this wrapper for every gooidc.NewProvider call +// site; the package's own callers (service.go runtime load, +// test_discovery.go dry-run validator) do this unconditionally. +func SafeOIDCContext(ctx context.Context) context.Context { + return gooidc.ClientContext(ctx, oidcDiscoveryClient) +} + +// validateIssuerSSRF is the package-level seam tests substitute for the +// static issuer-URL SSRF gate. Production callers always run through +// validation.ValidateSafeURL; tests using httptest.NewServer (which +// binds to 127.0.0.1) swap this to a no-op in setup_test.go so the +// loopback URL doesn't trip the early-fail rail. Mirrors the +// jwksProbeClient / oidcDiscoveryClient test-seam pattern. Production +// code MUST NOT reassign this var. +var validateIssuerSSRF = validation.ValidateSafeURL diff --git a/internal/auth/oidc/service.go b/internal/auth/oidc/service.go index c4d922b..681b8be 100644 --- a/internal/auth/oidc/service.go +++ b/internal/auth/oidc/service.go @@ -1063,7 +1063,14 @@ func (s *Service) getOrLoad(ctx context.Context, providerID string) (*providerEn } // Fetch + cache the discovery doc + JWKS via go-oidc. - provider, err := gooidc.NewProvider(ctx, cfgRow.IssuerURL) + // + // SEC-001 closure (Sprint 1, 2026-05-16): the bare `ctx` is wrapped + // in SafeOIDCContext so the discovery fetch + every subsequent + // Verifier-issued JWKS fetch run through validation.SafeHTTPDialContext. + // Pre-fix this path used http.DefaultClient and could be aimed at + // loopback / RFC 1918 / link-local / cloud-metadata addresses via the + // admin-supplied issuer URL. See safehttp.go for the full closure note. + provider, err := gooidc.NewProvider(SafeOIDCContext(ctx), cfgRow.IssuerURL) if err != nil { return nil, fmt.Errorf("oidc: discovery fetch failed for %s: %w", providerID, err) } diff --git a/internal/auth/oidc/setup_test.go b/internal/auth/oidc/setup_test.go index 79cb9fe..3d7b2f0 100644 --- a/internal/auth/oidc/setup_test.go +++ b/internal/auth/oidc/setup_test.go @@ -29,4 +29,14 @@ func init() { Timeout: 10 * time.Second, Transport: http.DefaultTransport, } + // SEC-001 closure companion: same SSRF-bypass for the discovery + // fetch's http.Client + the static issuer-URL gate. Tests using + // httptest.NewServer get a loopback URL; the production + // SafeHTTPDialContext + validateIssuerSSRF would reject these. + // Production code never reassigns either var. + oidcDiscoveryClient = &http.Client{ + Timeout: 10 * time.Second, + Transport: http.DefaultTransport, + } + validateIssuerSSRF = func(string) error { return nil } } diff --git a/internal/auth/oidc/test_discovery.go b/internal/auth/oidc/test_discovery.go index 1edb01c..de1d0d2 100644 --- a/internal/auth/oidc/test_discovery.go +++ b/internal/auth/oidc/test_discovery.go @@ -58,11 +58,31 @@ type TestDiscoveryResult struct { func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDiscoveryResult, error) { res := &TestDiscoveryResult{} + // SEC-001 closure (Sprint 1, 2026-05-16): refuse reserved-address + // issuers up-front so operators see a clear policy error instead + // of the lower-level dial-rejection wrap from SafeHTTPDialContext. + // The dial-time guard remains the authoritative DNS-rebinding-safe + // defense; this is the early-fail UX rail. Routed through the + // validateIssuerSSRF package-level seam so tests using + // httptest.NewServer can swap it for a no-op (see setup_test.go). + if vErr := validateIssuerSSRF(issuerURL); vErr != nil { + res.Errors = append(res.Errors, fmt.Sprintf("issuer_url failed SSRF policy: %v", vErr)) + return res, nil + } + // Step 1 — discovery. gooidc.NewProvider fetches // `/.well-known/openid-configuration` and runs the iss // match check internally; on failure it returns a fmt-style // wrapped error. - provider, err := gooidc.NewProvider(ctx, issuerURL) + // + // SEC-001 closure (Sprint 1, 2026-05-16): the bare `ctx` is wrapped + // in SafeOIDCContext so the discovery fetch + the resulting + // Verifier's internal JWKS fetch both run through a transport + // whose DialContext is validation.SafeHTTPDialContext. Pre-fix the + // default HTTP client could be aimed at loopback / RFC 1918 / + // link-local / cloud-metadata addresses via the admin-supplied + // issuer URL. See safehttp.go for the full closure note. + provider, err := gooidc.NewProvider(SafeOIDCContext(ctx), issuerURL) if err != nil { res.Errors = append(res.Errors, fmt.Sprintf("discovery fetch failed: %v", err)) return res, nil // Non-fatal at this layer; the response carries the per-leg failure.