mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-12 06:38:53 +00:00
fix(auth): SEC-001 — gate OIDC discovery through SafeHTTPDialContext + ValidateSafeURL
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.
This commit is contained in:
@@ -7,6 +7,8 @@ import (
|
|||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
|
"github.com/certctl-io/certctl/internal/validation"
|
||||||
)
|
)
|
||||||
|
|
||||||
// Coverage fill — v2.1.0 release gate Phase 3.
|
// 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
|
// TestTestDiscovery_DiscoveryFailure_ReturnsErrorsSlice points
|
||||||
// TestDiscovery at a URL that doesn't serve a discovery doc; the
|
// TestDiscovery at a URL that doesn't serve a discovery doc; the
|
||||||
// function MUST return res with DiscoverySucceeded=false and a
|
// function MUST return res with DiscoverySucceeded=false and a
|
||||||
|
|||||||
@@ -22,6 +22,7 @@ import (
|
|||||||
"time"
|
"time"
|
||||||
|
|
||||||
authdomain "github.com/certctl-io/certctl/internal/domain/auth"
|
authdomain "github.com/certctl-io/certctl/internal/domain/auth"
|
||||||
|
"github.com/certctl-io/certctl/internal/validation"
|
||||||
)
|
)
|
||||||
|
|
||||||
// OIDCProvider describes a configured OpenID Connect identity provider
|
// 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 {
|
if _, err := url.Parse(p.IssuerURL); err != nil {
|
||||||
return fmt.Errorf("oidc: issuer_url is not a valid URL: %w", err)
|
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) == "" {
|
if strings.TrimSpace(p.ClientID) == "" {
|
||||||
return ErrOIDCEmptyClientID
|
return ErrOIDCEmptyClientID
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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) {
|
func TestOIDCProvider_Validate_RejectsEmptyClientID(t *testing.T) {
|
||||||
p := validProvider()
|
p := validProvider()
|
||||||
p.ClientID = ""
|
p.ClientID = ""
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -1063,7 +1063,14 @@ func (s *Service) getOrLoad(ctx context.Context, providerID string) (*providerEn
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Fetch + cache the discovery doc + JWKS via go-oidc.
|
// 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 {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("oidc: discovery fetch failed for %s: %w", providerID, err)
|
return nil, fmt.Errorf("oidc: discovery fetch failed for %s: %w", providerID, err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -29,4 +29,14 @@ func init() {
|
|||||||
Timeout: 10 * time.Second,
|
Timeout: 10 * time.Second,
|
||||||
Transport: http.DefaultTransport,
|
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 }
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -58,11 +58,31 @@ type TestDiscoveryResult struct {
|
|||||||
func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDiscoveryResult, error) {
|
func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDiscoveryResult, error) {
|
||||||
res := &TestDiscoveryResult{}
|
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
|
// Step 1 — discovery. gooidc.NewProvider fetches
|
||||||
// `<issuer>/.well-known/openid-configuration` and runs the iss
|
// `<issuer>/.well-known/openid-configuration` and runs the iss
|
||||||
// match check internally; on failure it returns a fmt-style
|
// match check internally; on failure it returns a fmt-style
|
||||||
// wrapped error.
|
// 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 {
|
if err != nil {
|
||||||
res.Errors = append(res.Errors, fmt.Sprintf("discovery fetch failed: %v", err))
|
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.
|
return res, nil // Non-fatal at this layer; the response carries the per-leg failure.
|
||||||
|
|||||||
Reference in New Issue
Block a user