2 Commits

Author	SHA1	Message	Date
shankar0123	2e9262cfb7	fix(handler): SEC-021 — wrap BCL provider re-fetch via SafeOIDCContext ... 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":"<loopback url>"} 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).	2026-05-16 16:41:39 +00:00
shankar0123	cd374b243e	refactor(handler): split auth_session_oidc.go by handler-section (Phase 9, 11 of N) ... Phase 9 ARCH-M2 closure Sprint 11. Splits internal/api/handler/auth_session_oidc.go (was 1577 LOC, the fifth-largest backend hotspot from the original audit) via the Option B sibling-file pattern — new files stay in `package handler` so every external caller of `handler.AuthSessionOIDCHandler.{LoginInitiate, LoginCallback, BackChannelLogout, Logout, ListSessions, RevokeSession, RevokeAllExceptCurrent, ListProviders, CreateProvider, UpdateProvider, DeleteProvider, TestProvider, RefreshProvider, ListGroupMappings, AddGroupMapping, RemoveGroupMapping}` and `handler.{DefaultBCLVerifier, NewDefaultBCLVerifier, DefaultBCLVerifierMaxAge}` resolves the same way. Pure mechanical relocation; no signature, no behavior, no import-graph change. Section-based split (Option B + audit's verb prescription) ========================================================== The audit's Tasks-Deferred row prescribed splitting "per handler verb (login / callback / refresh / logout / backchannel)." The file itself documents a three-section layout in its package doc-comment: 1. Public OIDC handshake (auth-exempt) 2. Session management (RBAC-gated) 3. OIDC provider + group-mapping CRUD (RBAC-gated) Going strictly verb-by-verb would have: - mis-grouped RefreshProvider (which is an ADMIN op on a provider's signing-key cache, not a session refresh — same auth.oidc.edit permission as Update/Delete); - split LoginInitiate + LoginCallback into separate files despite them sharing the state cookie + pre-login row flow; - left the other 9 handlers (Sessions, Provider CRUD, Group Mappings) with no obvious home. Sprint 11 follows the file's own self-described section split plus a fourth file for the DefaultBCLVerifier, which the original file already kept under a separate banner. What moved ========== New `internal/api/handler/auth_session_oidc_handshake.go` (391 LOC) — Section 1 / Public OIDC handshake handlers (auth-exempt): - LoginInitiate (GET /auth/oidc/login?provider=<id>) - LoginCallback (GET /auth/oidc/callback?code=...&state=...) - BackChannelLogout (POST /auth/oidc/back-channel-logout) - Logout (POST /auth/logout) New `internal/api/handler/auth_session_oidc_sessions.go` (208 LOC) — Section 2 / Session-management handlers (RBAC-gated): - sessionResponse projection type + sessionToResponse mapper - ListSessions (GET /api/v1/auth/sessions) - RevokeSession (DELETE /api/v1/auth/sessions/{id}) - RevokeAllExceptCurrent (DELETE /api/v1/auth/sessions/all-except-current) New `internal/api/handler/auth_session_oidc_crud.go` (470 LOC) — Section 3 / OIDC provider + group-mapping CRUD (RBAC-gated): - oidcProviderResponse + oidcProviderRequest projection types, providerToResponse mapper - ListProviders / CreateProvider / UpdateProvider / DeleteProvider / TestProvider / RefreshProvider - groupMappingResponse + groupMappingRequest projection types, mappingToResponse mapper - ListGroupMappings / AddGroupMapping / RemoveGroupMapping New `internal/api/handler/auth_session_oidc_bcl.go` (225 LOC) — DefaultBCLVerifier (handler's default implementation of the BackChannelLogoutVerifier interface declared in auth_session_oidc.go): - DefaultBCLVerifierMaxAge constant - DefaultBCLVerifier struct + NewDefaultBCLVerifier - WithMaxAge builder - Verify (the OpenID Connect Back-Channel Logout 1.0 §2.6 verification: events claim, iat window, algorithm allowlist, audience match, sub/sid/jti decode) - peekIssuer unexported helper What stays in auth_session_oidc.go (452 LOC, down from 1577) ============================================================ - Package + import block. - Service-layer interface projections (OIDCAuthHandshaker, SessionMinter, BackChannelLogoutVerifier) — declared once and consumed by every section. - SessionCookieAttrs config struct. - AuthSessionOIDCHandler struct + permissionChecker / BCLReplayConsumer / AuditRecorder interfaces + NewAuthSession- OIDCHandler constructor + the WithPermissionChecker / WithBCLReplayConsumer builder methods. - The shared helpers consumed across multiple sections: encryptClientSecret, recordAudit, clearPreLoginCookie, clearSessionCookies, clientIPFromRequest, classifyOIDCFailure, randomB64URLForHandler, defaultIfBlank, defaultIntIfZero. Side-effect import cleanup ========================== Four imports drop from auth_session_oidc.go as a clean side effect of the cut: - "encoding/json" (used only in CRUD + BCL — moved out) - "fmt" (used only in BCL — moved out) - gooidc "github.com/coreos/go-oidc/v3/oidc" (used only in BCL — moved out) - oidcdomain "github.com/certctl-io/certctl/internal/auth/oidc/domain" (used in handshake + CRUD + BCL — moved out) Per-import audit on every new sibling file is in the commit's diff: each carries only the imports its extracted code actually consumes. Net effect ========== auth_session_oidc.go: 1577 → 452 LOC (-1,125 = -71.3%). Four new sibling files at 1,294 LOC total (1,125 moved + ~169 of header + Phase 9 doc-comment overhead). The original hotspot drops below the cmd/agent/main.go target for Sprint 12 (1489 LOC). Cumulative Phase 9 progress (top 5 hotspots) ============================================ config.go 3403 → 1342 (-60.6%, Sprints 1-7) cmd/server/main.go 2966 → 2260 (-23.8%, Sprints 8 + 8b) service/acme.go 1965 → 1162 (-40.9%, Sprints 9 + 9b) mcp/tools.go 1867 → 109 (-94.2%, Sprint 10) auth_session_oidc 1577 → 452 (-71.3%, Sprint 11) TOTAL across 5 files: 11,778 → 5,325 LOC = -6,453 (-54.8%) Behavior preservation contract ============================== 1. gofmt -l clean across all 5 affected files. 2. go vet ./internal/api/handler/... — no findings. 3. staticcheck ./internal/api/handler/... — no findings. 4. go test -short -count=1 ./internal/api/handler/... — green (includes the 1,439-line auth_session_oidc_test.go suite that pins every moved handler's behavior including BCL replay, CSRF rotation, audit emission, and the Phase-5 RBAC path). 5. Broader-importer build green: go build ./... . 6. Broader-importer tests green: go test -short -count=1 ./cmd/server/... ./internal/api/router/... . cmd/server/main.go consumes handler.DefaultBCLVerifier + handler.NewDefaultBCLVerifier + handler.DefaultBCLVerifierMaxAge across three call sites; all three resolve unchanged through Go's same-package public-export mechanism (the type + constructor moved to a sibling file in the same `handler` package). The mcp/tools_auth_bundle2.go comment string referencing "oidcProviderRequest" is descriptive prose, not an import. What remains for Phase 9 ======================== One sibling-file split queued: - Sprint 12: cmd/agent/main.go (1489 LOC) → main + poll + deploy + register sibling files in same cmd/agent package (mirrors the cmd/server pattern from Sprints 8 + 8b). Refs: ARCH-M2 (god-files), Phase 9 audit. Sprint 11 closes the auth-session-OIDC handler hotspot from the audit's top-5 list.	2026-05-14 10:22:33 +00:00