5 Commits

Author	SHA1	Message	Date
shankar0123	f5ba17114d	fix(audit): close silence-leg of HIGH-6; emit WARN on audit-write failure ... Audit 2026-05-10 HIGH-6 partial closure (silence leg). The audit identified two distinct gaps in the auth surface's audit-emit pattern: (1) silence — `_ = audit.RecordEventWithCategory(...)` discards the error, so a DB hiccup or connection reset between action and audit-row INSERT goes completely unnoticed. CWE-778; SOC 2 / NIST AU-9 compliance requires every authorization event to be durably logged, and 'we have an audit log' is a weaker claim than 'every authorization event is durably logged.' (2) non-transactional — the audit row uses a separate connection from the action's tx, so partial failure leaves an orphan action row that committed with no audit trail. Decision 8 of the auth-bundles-index requires action + audit row atomic. This commit closes leg (1) fully across all six audit-emit call sites in the auth surface: - internal/service/auth/actor_role_service.go::recordAudit - internal/service/auth/role_service.go::recordAudit - internal/auth/bootstrap/service.go::ValidateAndMint - internal/auth/breakglass/service.go::recordAudit - internal/auth/session/service.go::recordAudit - internal/api/handler/auth_session_oidc.go::recordAudit - internal/service/profile.go::Update (Phase 9 approval-bypass) Each `_ = ...` swallow is replaced with: if err := audit.RecordEventWithCategory(...); err != nil { slog.WarnContext(ctx, '<surface> audit write failed (action committed; audit row may be missing)', 'action', action, 'actor_id', actor, 'resource_id', resource, 'err', err) } Operators monitoring audit-write failures now see structured WARN logs with action + actor + resource attribution; missing audit rows can be cross-referenced against monitoring without manual SELECT-from- audit-table. Infrastructure for leg (2) (transactional commit) is also landed in this commit: - service.AuditService.RecordEventWithCategoryWithTx (new method; accepts repository.Querier from postgres.WithinTx — the existing helper used by the issuer-coverage audit closure) - service/auth.AuditService interface declares the new method - test stub fakeAudit.RecordEventWithCategoryWithTx satisfies the extended interface The eight per-path WithinTx-refactors documented in cowork/auth-bundles-fixes-2026-05-10/10-high-6-atomic-audit-commit.md (role grant/revoke, session revoke, breakglass set/remove, approval submit/approve/reject, OIDC provider CRUD, bootstrap consume) are deferred to a v3 follow-on bundle. Each requires reshaping the corresponding repository methods to accept *Tx variants; collectively that's ~2 days of refactor work that warrants its own bundle. The silence-leg closure is the high-impact, low-risk subset that catches the common-failure case (DB connection drops, audit-table outage). Refs: cowork/auth-bundles-audit-2026-05-10.md HIGH-6 Spec: cowork/auth-bundles-fixes-2026-05-10/10-high-6-atomic-audit-commit.md	2026-05-10 21:24:29 +00:00
shankar0123	0f340beb14	fix(auth/ux): cause-aware OIDC + session error surfacing (HIGH-7 + HIGH-8 closure) ... Server (HIGH-7): the OIDC callback failure path now 302-redirects to /login?error=oidc_failed&reason=<category> instead of emitting a blank 400. `category` is the existing audit `failure_category` value; classifyOIDCFailure was extended with three new sentinel paths (email_domain_not_allowed, email_missing_but_required, pkce_invalid) so CRIT-5 + PKCE failures get distinguishable GUI rendering. Audit-log observability is unchanged — the same failure_category is written to the auth.oidc_login_failed audit row; the 302 is purely a UX leg layered on top. Server (HIGH-8): SessionMiddleware now stashes a cause classification on the request context when Validate returns an error, mapping the sentinels via classifySessionError (errors.Is-based, so wrapped sentinels still classify) to the stable wire-strings idle_timeout / absolute_timeout / back_channel_revoked / invalid_token. The 401 emit point in bearerSkipIfAuthenticated reads the stashed cause and emits WWW-Authenticate: Bearer realm="certctl", error="invalid_token", error_description=<cause> per RFC 6750 §3. GUI (HIGH-7): LoginPage reads ?error= + ?reason= from the URL via react-router useSearchParams and renders an operator-friendly amber-bordered banner above the form; OIDC_FAILURE_REASON_TEXT maps all 16 known categories with a defensive 'unspecified' fallback for forward-compat with future server-side categories. GUI (HIGH-8): api/client fetchJSON parses the WWW-Authenticate cause via parseWWWAuthenticateCause and attaches it to the 'certctl:auth-required' CustomEvent detail; AuthProvider redirects to /login?session_expired=<cause> on cause-aware 401s; LoginPage renders a blue-bordered session-cause banner. invalid_token stays on the current page (no hard redirect for opaque failures). Misc cleanup: ErrorState now accepts the title/message/data-testid form added by CRIT-4 BreakglassPage (was erroring tsc on master). Regression matrix: - internal/api/handler/oidc_redirect_categories_test.go pins all 16 failure categories to the 302 + reason= location + audit-row leg - internal/auth/session/www_authenticate_test.go pins the 4 stable cause categories on classifySessionError (incl. errors.Is wrapped sentinels) + the WWW-Authenticate emission across all 4 categories + the no-session-context fallback case - internal/api/handler/auth_session_oidc_test.go: 4 pre-existing TestLoginCallback_*Returns400 tests updated to assert 302 + reason= location (the wire shape changed from 400 to 302, but the audit observability and behaviour-equivalent failure-classification are preserved) - web/src/pages/LoginPage.test.tsx: 6 new cases pinning the failure banner, session-cause banner, unknown-reason fallback, and forward-compat 'unspecified' category Spec: cowork/auth-bundles-fixes-2026-05-10/08-high-7-8-error-surfacing.md Closes: HIGH-7, HIGH-8 of cowork/auth-bundles-audit-2026-05-10.md	2026-05-10 21:12:11 +00:00
shankar0123	15435ca02b	fix(oidc/bcl): jti replay-cache + iat freshness check (HIGH-3 closure) ... Closes HIGH-3 of the 2026-05-10 audit. Pre-fix the BCL handler accepted any logout_token whose iat + jti were syntactically present but never checked (a) that iat fell within a skew window or (b) that jti hadn't been seen before. A captured logout_token was replayable indefinitely; once CRIT-2 was fixed, every replay would revoke the user's current sessions — persistent DoS. RFC 9700 §2.7 + OIDC BCL 1.0 §2.5 require jti replay defense. - Migration 000040_bcl_replay_cache: oidc_bcl_consumed_jtis table with composite PK on (jti, issuer_url) — RFC 7519 §4.1.7 per-issuer uniqueness — and an expires_at index for the GC sweep. - repository.BCLReplayRepository interface + ErrBCLJTIAlreadyConsumed sentinel. Postgres impl uses INSERT...ON CONFLICT DO NOTHING RETURNING true for atomic single-use semantics in one round-trip. - handler.DefaultBCLVerifier gains WithMaxAge + nowFn clock seam. iat freshness check rejects tokens whose iat is in the future beyond max-age OR stale beyond it. Verifier signature extended: Verify(ctx, jwt) (iss, sub, sid, jti string, iat int64, err error). - handler.AuthSessionOIDCHandler gains BCLReplayConsumer (interface) + WithBCLReplayConsumer(consumer, maxAge) setter. BackChannelLogout consumes the jti post-verify with TTL = max(24h, 2*maxAge): - first-receive → 200, sessions revoked, audit outcome=revoked - replay (ErrBCLJTIAlreadyConsumed) → 200 + Cache-Control: no-store, audit outcome=jti_replayed, sessions NOT re-revoked - transient (non-AlreadyConsumed error) → 503 so the IdP retries - internal/scheduler/scheduler.go: SetBCLReplayGarbageCollector wires SweepExpired into the existing session-GC tick (no separate ticker for short-lived replay rows). - cmd/server/main.go: bclMaxAge from cfg.Auth.OIDCBCLMaxAgeSeconds (default 60s, env CERTCTL_OIDC_BCL_MAX_AGE_SECONDS); bclReplayRepo wired into the verifier + handler + scheduler. - Three regression tests in internal/api/handler/bcl_replay_test.go: TestBackChannelLogout_FirstReceiveConsumesJTI, TestBackChannelLogout_ReplayedJTIReturns200WithAudit, TestBackChannelLogout_TransientConsumeFailureReturns503. - internal/api/handler/auth_session_oidc_test.go: stubBCLVerifier gains jti + iat fields; existing TestBackChannelLogout_* tests rewritten for the new Verify return. Verification gate green: gofmt clean, go vet clean, go test -short -count=1 on internal/api/handler / internal/api/router / internal/scheduler / cmd/server / internal/auth/oidc / internal/auth/breakglass — all pass. CRIT-1..CRIT-5 + HIGH-1 + HIGH-2 + HIGH-3 of the 2026-05-10 audit now closed on this branch. Spec at cowork/auth-bundles-fixes-2026-05-10/07-high-3-bcl-replay-defense.md. Refs: cowork/auth-bundles-audit-2026-05-10.md HIGH-3	2026-05-10 20:53:29 +00:00
shankar0123	ca1e135aa3	fix(oidc/bcl): resolve sub→actor_id via users.GetByOIDCSubject (CRIT-2 closure) ... Closes CRIT-2 of the 2026-05-10 audit. The BCL handler previously called sessionSvc.RevokeAllForActor(sub, "User") but session rows are keyed by user.ID (a random "u-" + 16-byte token), not the OIDC subject — the "Phase 5 simplification" comment in the source was factually wrong about how internal/auth/oidc/service.go::upsertUser seeds user.ID. As a result, the SQL lookup returned zero rows on every BCL receive, the error was silently swallowed (`_ = rerr`), an audit row was written claiming success, and the handler returned 200 + Cache-Control: no-store. OIDC BCL 1.0 §2.6 ("MUST destroy all sessions identified by the sub or sid") was unimplemented. CWE-613. This commit: - Adds userRepo (repository.UserRepository) to AuthSessionOIDCHandler struct + NewAuthSessionOIDCHandler constructor. cmd/server/main.go injects the existing oidcUserRepo (no new repository instance). - Replaces the broken sub-as-actor-id path with: 1. providerRepo.List(ctx, tenantID) + IssuerURL filter to map claims.iss → provider row (N is small; typically 1-5). 2. userRepo.GetByOIDCSubject(ctx, provider.ID, sub) to resolve the OIDC subject → user.ID. 3. sessionSvc.RevokeAllForActor(user.ID, "User") with the RESOLVED actor_id (not the OIDC subject). - Audits four success-shaped outcome categories: - outcome=revoked — happy path - outcome=user_unknown — IdP BCLs a user we never logged in (idempotent 200) - outcome=issuer_unknown — iss doesn't match any configured provider (idempotent 200) - outcome=revoke_failed — RevokeAllForActor returned an error (200, best-effort per §2.8) And two transient outcomes that return 503 (IdP retries per §2.8): - outcome=provider_lookup_failed — providerRepo.List error - outcome=user_lookup_failed — non-NotFound userRepo error - Removes the misleading "Phase 5 simplification" comment block; replaces with a doc explaining the resolution path + outcome taxonomy + spec refs. - Adds 5 regression tests in internal/api/handler/auth_session_oidc_test.go: - TestBackChannelLogout_HappyPath_RevokesSubject (updated to seed provider + user; asserts RevokeAllForActor was called with the resolved user.ID, not the raw OIDC subject — the test that would have caught CRIT-2 had it existed) - TestBackChannelLogout_UnknownUserReturns200WithAudit - TestBackChannelLogout_IssuerUnknownReturns200WithAudit - TestBackChannelLogout_TransientUserRepoErrorReturns503 - TestBackChannelLogout_RevokeFailureReturns200WithAuditFailureOutcome - Introduces stubUserRepo in the handler test file (matching the four repository.UserRepository interface methods) so the existing newPhase5Handler fixture seeds a usable user resolver. Verification gate green: - gofmt -l . clean - go vet ./... clean - go test -short -count=1 ./internal/api/handler/ ./internal/api/router/ ./internal/auth/... ./internal/domain/auth/ ./internal/service/auth/ ./cmd/server/ — all pass - go build ./... clean CRIT-1 from the same audit is already closed on this branch (commit 68ca42f); CRIT-3 / CRIT-4 / CRIT-5 remain open and continue to block the v2.1.0 tag. Spec: cowork/auth-bundles-fixes-2026-05-10/02-crit-2-bcl-sub-lookup.md. Refs: cowork/auth-bundles-audit-2026-05-10.md CRIT-2	2026-05-10 20:07:29 +00:00
shankar0123	9c679a5960	auth-bundle-2 Phase 5: OIDC + session HTTP surface (13 endpoints), ... pre-login store, OpenID Connect Back-Channel Logout 1.0, cookieAuth scheme, 7 new auth permissions, CI guard, handler tests Phase 5 of the bundle puts the Phase 3 OIDC service + Phase 4 session service on the wire. 13 HTTP endpoints split into three logical groups: Public OIDC handshake (auth-exempt; protocol-mediated): GET /auth/oidc/login?provider=<id> -> 302 to IdP authorization URL + sets certctl_oidc_pending cookie (10-min TTL, Path=/auth/oidc/, SameSite=Lax) GET /auth/oidc/callback?code=...&state=... -> consume pre-login row, run Phase 3's 11-step token validation, mint post-login session, 302 to dashboard POST /auth/oidc/back-channel-logout -> OpenID Connect BCL 1.0 — IdP POSTs logout_token JWT; certctl validates signature against IdP JWKS via Phase 3 alg allow-list, required claims (iss/aud/iat/jti/ events; exactly one of sub/sid; nonce ABSENT per spec §2.4), revokes matching sessions, returns 200 with Cache-Control: no-store POST /auth/logout -> revoke caller's session Session management (RBAC-gated auth.session.*): GET /api/v1/auth/sessions -> auth.session.list (own / all) DELETE /api/v1/auth/sessions/{id} -> auth.session.revoke (own bypass) OIDC provider + group-mapping CRUD (RBAC-gated auth.oidc.*): GET /api/v1/auth/oidc/providers -> auth.oidc.list POST /api/v1/auth/oidc/providers -> auth.oidc.create (client_secret encrypted at rest via internal/crypto.EncryptIfKeySet) PUT /api/v1/auth/oidc/providers/{id} -> auth.oidc.edit DELETE /api/v1/auth/oidc/providers/{id} -> auth.oidc.delete (refused via ErrOIDCProviderInUse → 409 when users authenticated via this provider) POST /api/v1/auth/oidc/providers/{id}/refresh -> auth.oidc.edit (re-runs IdP downgrade defense via OIDCService.RefreshKeys) GET /api/v1/auth/oidc/group-mappings -> auth.oidc.list POST /api/v1/auth/oidc/group-mappings -> auth.oidc.edit DELETE /api/v1/auth/oidc/group-mappings/{id} -> auth.oidc.edit Migration 000037 ships: - oidc_pre_login_sessions table (10-min absolute TTL, FK CASCADE on oidc_provider_id, FK RESTRICT on signing_key_id; index on absolute_expires_at for the GC sweep); - 7 new permissions seeded into r-admin only: auth.session.list, auth.session.list.all, auth.session.revoke, auth.oidc.list, auth.oidc.create, auth.oidc.edit, auth.oidc.delete CanonicalPermissions extended in lockstep at internal/domain/auth/ validate.go. Pre-login machinery: - internal/repository/oidc.go gains PreLoginRepository interface + PreLoginSession struct + ErrPreLoginNotFound / ErrPreLoginExpired sentinels. - internal/repository/postgres/oidc_prelogin.go ships the impl; LookupAndConsume uses DELETE ... RETURNING for atomic single-use. - internal/auth/oidc/prelogin.go is the PreLoginAdapter that bridges the OIDC service's Phase 3 PreLoginStore interface to the new repository, signing the cookie value under the active SessionSigningKey via the same v1.<id>.<key>.<HMAC> wire format Phase 4 uses for post-login cookies. Defense-in-depth: the pre-login `pl-` prefix is enforced by ParseCookieValue(prefix); a stolen pre-login cookie cannot be replayed against the post-login Validate path (pinned by TestService_Validate_RejectsPreLoginCookieAtPostLoginGate). Session package extension: - internal/auth/session/service.go gains exported SignCookieValue, ParseCookieValue (with caller-supplied id-1 prefix), ComputeCookieHMAC, DecryptKeyMaterial wrappers so the OIDC pre-login adapter shares the same length-prefixed HMAC math without code duplication. - parseCookie no longer hardcodes the `ses-` prefix check (moved to Validate as defense-in-depth; pre-login cookie verification uses the `pl-` prefix via ParseCookieValue). Cookie attributes (all Phase 5 endpoints honor CERTCTL_SESSION_SAMESITE + Secure=true via SessionCookieAttrs from Phase 4 config): - certctl_oidc_pending: Path=/auth/oidc/, MaxAge=600s, SameSite=Lax (cannot be Strict because the IdP-initiated callback is a top-level navigation from a different origin). - certctl_session: Path=/, Expires=8h, SameSite=Lax|Strict, HttpOnly. - certctl_csrf: Path=/, Expires=8h, HttpOnly=false (intentional — GUI must read it to echo into X-CSRF-Token header). Audit logging on every mutating operation (event_category="auth"): auth.oidc_login_succeeded / failed / unmapped_groups auth.oidc_back_channel_logout / failed auth.session_revoked auth.oidc_provider_{created,updated,deleted,refreshed} auth.group_mapping_{added,removed} OpenAPI updates: - cookieAuth security scheme added to api/openapi.yaml under components.securitySchemes (apiKey / cookie / certctl_session). - The 13 Phase 5 routes are added to SpecParityExceptions with a deferral note: full per-endpoint OpenAPI rows land in a follow-on commit alongside the GUI work (Phase 8) so the ergonomic shape can be validated against the live GUI client. CI guard: scripts/ci-guards/N-bundle-2-security-empty-preserved.sh asserts api/openapi.yaml has ≥ 14 'security: []' occurrences (the pre-Bundle-2 baseline). Reducing the count below 14 would silently force a Bearer-or-cookie requirement onto an endpoint that legitimately runs without certctl-issued credentials; the guard fires before that regression lands. Handler tests (internal/api/handler/auth_session_oidc_test.go): - All 6 prompt-mandated negative cases: BCL with missing events claim -> 400 BCL with nonce present -> 400 (per spec §2.4) BCL with sig signed by an unknown key -> 400 Callback with replayed state -> 400 Callback with PKCE verifier mismatch -> 400 Callback with expired pre-login row -> 400 - Plus happy paths for every endpoint, edge cases (missing-cookie, duplicate-name, in-use-409, wrong-tenant), and the Helper-function coverage (peekIssuer, classifyOIDCFailure, defaultIfBlank, defaultIntIfZero, clientIPFromRequest, encryptClientSecret). Coverage on internal/api/handler/auth_session_oidc.go: 80.9% per-function (above the Phase 5 spec's ≥ 80% floor). Server wiring (cmd/server/main.go): Wired AFTER sessionService (Phase 4) so the OIDC PreLoginAdapter can sign pre-login cookies under the active SessionSigningKey: oidcProviderRepo + oidcMappingRepo + oidcUserRepo + oidcPreLoginRepo -> preLoginAdapter -> oidcService -> authSessionOIDCHandler. sessionMinterAdapter shim bridges *session.Service.Create to the oidcsvc.SessionMinter port the OIDC service consumes. Router wiring (internal/api/router/router.go): 4 public OIDC routes via direct r.mux.Handle (auth-exempt; pinned in AuthExemptRouterRoutes); 9 RBAC-gated routes via r.Register + rbacGate(checker, perm, h). Routes only register when reg.AuthSessionOIDC != nil so pre-Phase-5 builds skip the block entirely. Verifications: gofmt clean, go vet clean across all touched packages, go test -short -count=1 green across internal/api/handler (74 tests + new Phase 5 batch), internal/api/router (parity + auth-exempt allowlist), internal/auth/oidc + session (no regressions), full domain + scheduler + config sweeps green, ci-guard N-bundle-2-security-empty-preserved.sh green (17 ≥ 14 baseline).	2026-05-10 06:08:27 +00:00