gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-07 16:01:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(oidc/bcl): resolve sub→actor_id via users.GetByOIDCSubject (CRIT-2 closure)

Browse Source 
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
This commit is contained in:
shankar0123
2026-05-10 20:07:29 +00:00
parent 68ca42fef1
commit ca1e135aa3
3 changed files with 299 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/api/handler/auth_session_oidc.go
+81 -9
View File
@@ -103,6 +103,7 @@ type AuthSessionOIDCHandler struct {
providerRepo repository.OIDCProviderRepository
mappingRepo repository.GroupRoleMappingRepository
sessionRepo repository.SessionRepository
userRepo repository.UserRepository // CRIT-2: BCL sub→actor_id lookup
audit AuditRecorder
encryptionKey string
cookieAttrs SessionCookieAttrs
@@ -116,6 +117,11 @@ type AuditRecorder interface {
}
// NewAuthSessionOIDCHandler constructs the handler.
//
// userRepo is load-bearing for the BCL sub→actor_id resolution
// (CRIT-2 of the 2026-05-10 audit). Passing nil here is only valid in
// tests that exercise non-BCL paths; production wiring in
// cmd/server/main.go MUST inject a non-nil repository.
func NewAuthSessionOIDCHandler(
oidcSvc OIDCAuthHandshaker,
sessionSvc SessionMinter,
@@ -123,6 +129,7 @@ func NewAuthSessionOIDCHandler(
providerRepo repository.OIDCProviderRepository,
mappingRepo repository.GroupRoleMappingRepository,
sessionRepo repository.SessionRepository,
userRepo repository.UserRepository,
audit AuditRecorder,
encryptionKey, tenantID, postLoginURL string,
cookieAttrs SessionCookieAttrs,
@@ -137,6 +144,7 @@ func NewAuthSessionOIDCHandler(
providerRepo: providerRepo,
mappingRepo: mappingRepo,
sessionRepo: sessionRepo,
userRepo: userRepo,
audit: audit,
encryptionKey: encryptionKey,
cookieAttrs: cookieAttrs,
@@ -314,16 +322,80 @@ func (h *AuthSessionOIDCHandler) BackChannelLogout(w http.ResponseWriter, r *htt
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", "anonymous", domain.ActorTypeSystem, sid,
map[string]interface{}{"sub_or_sid": "sid", "issuer": issuer, "session_id": sid})
} else if sub != "" {
// Phase 5 simplification: revoke ALL sessions belonging to a User
// actor with this oidc_subject. The full subject->actor_id lookup
// is a 1-row select on users; for v1 we treat sub as the actor_id
// directly (this matches the user.id seeding pattern in Phase 3
// upsertUser, which uses oidc_subject as the actor_id stem).
if rerr := h.sessionSvc.RevokeAllForActor(r.Context(), sub, "User"); rerr != nil {
_ = rerr
// CRIT-2 closure of the 2026-05-10 audit. Pre-fix this branch called
// RevokeAllForActor(sub, "User") under the false assumption that
// the OIDC subject was used as the actor_id stem. In reality,
// internal/auth/oidc/service.go::upsertUser mints
// u.ID = "u-" + randomB64URL(16) and stores the OIDC subject in
// a separate column, so the pre-fix lookup never found a session
// row and the error was silently swallowed. BCL silently revoked
// nothing — CWE-613.
//
// The fix resolves the IdP-signed `iss` claim back to a provider
// row via providerRepo.List + IssuerURL filter, then resolves
// sub → user.ID via userRepo.GetByOIDCSubject, then revokes all
// sessions for that actor. Outcome categories audited:
// - revoked (happy path)
// - issuer_unknown (iss doesn't match any configured provider)
// - user_unknown (provider matched, but no user.id seeded for this subject)
// - revoke_failed (DB hiccup at the revoke step)
// - provider_lookup_failed / user_lookup_failed → 503 (transient; IdP retries)
// All success-shaped outcomes return 200 + Cache-Control: no-store
// per OIDC BCL 1.0 §2.7. Transient errors return 503 so the IdP
// follows its own retry semantics.
providers, plerr := h.providerRepo.List(r.Context(), h.tenantID)
if plerr != nil {
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", "anonymous", domain.ActorTypeSystem, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub, "outcome": "provider_lookup_failed"})
http.Error(w, "transient", http.StatusServiceUnavailable)
return
}
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", "anonymous", domain.ActorTypeSystem, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub})
var matched *oidcdomain.OIDCProvider
for _, p := range providers {
if p.IssuerURL == issuer {
matched = p
break
}
}
if matched == nil {
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", "anonymous", domain.ActorTypeSystem, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub, "outcome": "issuer_unknown"})
// Idempotent — return 200 per spec.
w.Header().Set("Cache-Control", "no-store")
w.WriteHeader(http.StatusOK)
return
}
user, uerr := h.userRepo.GetByOIDCSubject(r.Context(), matched.ID, sub)
if uerr != nil {
if errors.Is(uerr, repository.ErrUserNotFound) {
// Idempotent: nothing to revoke. IdP may BCL a user we
// never logged in. RFC compliance: still 200.
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", "anonymous", domain.ActorTypeSystem, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub, "outcome": "user_unknown"})
w.Header().Set("Cache-Control", "no-store")
w.WriteHeader(http.StatusOK)
return
}
// Transient — let the IdP retry.
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", "anonymous", domain.ActorTypeSystem, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub, "outcome": "user_lookup_failed"})
http.Error(w, "transient", http.StatusServiceUnavailable)
return
}
if rerr := h.sessionSvc.RevokeAllForActor(r.Context(), user.ID, string(domain.ActorTypeUser)); rerr != nil {
// Revoke failed — BCL is best-effort per §2.8; still 200,
// audit the failure.
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", user.ID, domain.ActorTypeUser, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub, "outcome": "revoke_failed"})
w.Header().Set("Cache-Control", "no-store")
w.WriteHeader(http.StatusOK)
return
}
h.recordAudit(r.Context(), "auth.oidc_back_channel_logout", user.ID, domain.ActorTypeUser, sub,
map[string]interface{}{"sub_or_sid": "sub", "issuer": issuer, "subject": sub, "outcome": "revoked"})
}
// Per spec §2.7 — Cache-Control: no-store on success.
w.Header().Set("Cache-Control", "no-store")