[neilLasrado]

⛔ Requires human security-engineer sign-off before merge

This is authentication code gating production access. It was written by an AI agent and adversarially reviewed by a second one. Neither of those is a substitute for a human reading it. The reviewer's suggested reading order is at the bottom — it is about 40 lines that matter most.

Slice 3 of 6. Stacked on #3 (feat/oidc-slice1-delete-auth-request) because it needs DeleteOIDCAuthRequest. Merge #3 first. Nothing registers the service yet, so merging this changes no behaviour.

What it does

Implements auth.OIDCService for Google Workspace, restricted to one hosted domain.

The authorization decision is checkClaims — require hd present and exactly equal to the configured domain (ASCII-lowercase comparison), and email_verified == true. A personal Gmail carries no hd claim at all, so absence is an explicit rejection, never a default-allow. The domain is never derived from email; a suffix match there is forgeable via example.com.evil.net.

Before any claim is trusted, the ID token is fully verified — signature, iss, aud, exp, nonce — with signing algorithms pinned to RS256. The nonce is bound to the state token as hex(SHA-256(state)). State is single-use: deleted on read, failing closed if the delete errors.

Two guards protect existing accounts. A Google identity cannot take over a user created by a different connector (upstream's rule), and cannot inherit a user whose stored OIDC sub differs — Google never reuses sub, but an email address can be reissued to a new person.

checkConnector refuses five settings this implementation doesn't enforce, rather than ignoring them: non-Google issuer, allow_unverified_email, max_age, Workspace directory sync, mfa.enabled. Silently ignoring an operator's setting is how a control quietly stops working.

Independent review

A second agent verified every security property against the source with line numbers, and confirmed each: hd handling, no email-derived domain, email_verified, verify-before-trust, algorithm pinning, state single-use, nonce derivation, both collision guards, SSOTestFlow writing no user, audit events on success and failure, fail-closed constructor. Verdict: ready for a human, with the items below.

Known items, carried deliberately

1. ErrUnauthorizedIdentity does not survive the gRPC boundary. The original comment claimed the proxy would match it with errors.Is to show an "unauthorized" page. It can't: the callback runs on Auth, and a trace error crossing gRPC is flattened to a code plus message and rebuilt client-side, so an errors.New sentinel never matches. (ErrGithubNoTeams works only because it is a trace.BadParameter, whose Is compares message strings.) The comment is corrected in this PR. Effect is a UX shortfall — unauthorized identities get the generic failure page — not a security one. The rejection still happens on Auth and the audit event still carries the reason. Follow-up needed for the distinct page.

2. ValidateClientRedirect is skipped when CreateWebSession is true. This replicates lib/auth/github.go:145-158 exactly. Reviewer confirmed it is not attacker-influenced: auth_with_roles.go:4560-4562 restricts CreateWebSession to callers holding RoleProxy, and the proxy passes the redirect through httplib.OriginLocalRedirectURI, which strips it to a relative path. Certificates never reach that URL. This is the highest-consequence judgement call in the slice and deserves a human signing off on the inherited guarantee.

3. Get + Delete of the auth request are not atomic. Two concurrent callbacks could both pass the Get. In practice the loser's Delete returns NotFound and fails closed, and both would hold the same code, which Google rejects on second exchange. A compare-and-delete would close it fully; that is a change to #3, deliberately not made here.

4. ConnectorSpec including the client secret is persisted for SSOTestFlow. Upstream behaviour — OIDCAuthRequest.Check() requires it — and only a connector admin can trigger it. Flagged as a conscious inheritance rather than an accident.

5. Test gaps. 64 tests pass, but there is no negative test for a non-pinned signing algorithm: the fake issuer signs ES256 and tests override the pin to match, so WithSupportedSigningAlgorithms is never exercised negatively. Also missing: hd present-but-empty, multi-audience with bad azp, and a regression test that hd actually unmarshals (see below).

One trap worth knowing about

googleClaims deliberately does not embed zoidc.IDTokenClaims. That type defines its own UnmarshalJSON, which Go would promote onto the outer struct, leaving every declared field silently zero — hd permanently empty. It fails closed (everyone rejected), but a future "tidy-up" would break login with no obvious cause. There is currently no test that would catch it.

Verification

GOFLAGS=-p=2 GOMAXPROCS=2, scoped, never ./...:

• go build ./lib/auth/oidcgoogle/ — OK
• go test ./lib/auth/oidcgoogle/ -count=1 — 64/64 pass
• go test ./lib/oidc/... ./lib/kube/token/ ./lib/join/ — pass (existing users of the fakeissuer helper, which is extended additively)
• go vet ./lib/auth/ — clean

lib/oidc/fakeissuer needed extending: as shipped its discovery document advertises no token endpoint and its only signing method is hardcoded to Kubernetes claims. SetEndpoints and SignClaims are additive and no-ops for existing callers. Discovery, JWKS and signature verification in the tests are real; only the token endpoint is hosted by the test.

Also written in-package: claims→traits conversion, because services.OIDCClaimsToTraits is Enterprise-only and not in this tree.

Suggested reading order for the human reviewer

1. checkClaims and claims.go — the entire authorization decision, ~40 lines
2. exchangeAndVerify — confirm for yourself that omitting rp.WithNonce would assert an empty nonce rather than skip the check. Reviewer verified this against rp/verifier.go:76-80,119-121; it is load-bearing and it fails closed.
3. createUser — the two collision guards. Decide whether a user with an empty stored UserID should be silently upgradeable.
4. Item 2 above — read auth_with_roles.go:4560-4562 and httplib.go:300-318 once and sign off.
5. Skip makeAuthResponse — a faithful mirror of makeGithubAuthResponse.

🤖 Generated with Claude Code