From 8ef092713fc7810c2354611176f11335f54660d4 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 17:33:25 -0400 Subject: [PATCH] Address Gadfly review on #5: OIDC identity guard, verified-email, timeouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from the PR #24 adversarial review (graded 23 real / 1 false positive): Security / correctness - LinkOIDC no longer overwrites a different stored identity: the UPDATE matches only when the row has no identity yet or already carries this exact one, so a second IdP asserting the same verified email can't hijack or lock out an account (returns ErrOIDCIdentityConflict). Uses a single UPDATE...RETURNING (also fixes the ignored-RowsAffected / misleading-ErrNotFound path and the round-trip). - Provisioning now requires a verified email for BOTH linking and JIT creation (was: linking only), so an unverified-email identity can't create an account — nor become the first admin on a fresh instance, nor squat an email a real user later owns. - OIDC-identity collisions surface as the dedicated ErrOIDCIdentityConflict instead of the email-specific ErrEmailTaken. Robustness - readOIDCTxCookie requires a non-empty nonce (an empty one would make the callback's nonce check pass vacuously). - Callback token exchange + verify run under a 15s context timeout so a slow IdP can't outlast the server write timeout. - ensure() performs discovery outside the mutex, so concurrent cold-start requests don't serialize behind one another's full timeout. - setOIDCTxCookie returns its marshal error; oidcLogin aborts rather than redirecting to the IdP with no tx cookie. Maintainability - redirectAuthError helper dedups the ~dozen callback redirects (and the empty-code path now logs like the rest). - Distinct login error codes (no_email / email_unverified / oidc_conflict) for the UI; writeServiceError maps the OIDC sentinels; shared test issuer const. Tests: unverified email refused for both link and JIT; identity-overwrite refused while the original identity keeps working. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- internal/api/auth.go | 6 +++ internal/api/oidc.go | 96 ++++++++++++++++++++++++++--------- internal/domain/domain.go | 11 ++-- internal/service/auth.go | 26 ++++++---- internal/service/auth_test.go | 48 +++++++++++++++--- internal/store/users.go | 31 +++++++---- 6 files changed, 165 insertions(+), 53 deletions(-) diff --git a/internal/api/auth.go b/internal/api/auth.go index 8f27483..6372928 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -223,6 +223,12 @@ func writeServiceError(c *gin.Context, err error) { writeAPIError(c, http.StatusForbidden, "REGISTRATION_CLOSED", "registration is closed") case errors.Is(err, domain.ErrLocalAuthDisabled): writeAPIError(c, http.StatusForbidden, "LOCAL_AUTH_DISABLED", "local authentication is disabled") + case errors.Is(err, domain.ErrOIDCNoEmail): + writeAPIError(c, http.StatusBadRequest, "OIDC_NO_EMAIL", "the identity provider returned no email") + case errors.Is(err, domain.ErrOIDCEmailUnverified): + writeAPIError(c, http.StatusForbidden, "OIDC_EMAIL_UNVERIFIED", "the identity provider's email is not verified") + case errors.Is(err, domain.ErrOIDCIdentityConflict): + writeAPIError(c, http.StatusConflict, "OIDC_IDENTITY_CONFLICT", "this identity conflicts with an existing account") case errors.Is(err, domain.ErrInvalidInput): writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input") default: diff --git a/internal/api/oidc.go b/internal/api/oidc.go index ba516b8..d264ffb 100644 --- a/internal/api/oidc.go +++ b/internal/api/oidc.go @@ -6,6 +6,7 @@ import ( "crypto/subtle" "encoding/base64" "encoding/json" + "errors" "log/slog" "net/http" "sync" @@ -16,6 +17,7 @@ import ( "golang.org/x/oauth2" "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" ) @@ -30,6 +32,10 @@ const ( // oidcDiscoveryTimeout bounds a single lazy discovery attempt so a hung IdP // can't wedge a request goroutine. oidcDiscoveryTimeout = 10 * time.Second + // oidcExchangeTimeout bounds the callback's token exchange + ID-token + // verification (which also fetches JWKS) so a slow IdP can't outlast the + // server's write timeout and leave the user on a blank page. + oidcExchangeTimeout = 15 * time.Second ) // oidcClient lazily performs OIDC discovery and holds the derived verifier and @@ -59,11 +65,14 @@ func newOIDCClient(cfg *config.Config) *oidcClient { // ensure performs discovery once (idempotent) and builds the verifier + oauth2 // config. Safe for concurrent callers; a failure leaves the client uninitialized -// so the next request retries. +// so the next request retries. The network discovery runs OUTSIDE the mutex, so +// concurrent cold-start (or IdP-outage) requests don't serialize behind one +// another's full timeout; the first to finish installs the result. func (o *oidcClient) ensure(ctx context.Context) error { o.mu.Lock() - defer o.mu.Unlock() - if o.provider != nil { + ready := o.provider != nil + o.mu.Unlock() + if ready { return nil } @@ -74,6 +83,12 @@ func (o *oidcClient) ensure(ctx context.Context) error { if err != nil { return err } + + o.mu.Lock() + defer o.mu.Unlock() + if o.provider != nil { + return nil // another goroutine won the race; keep its result. + } o.provider = provider o.verifier = provider.Verifier(&oidc.Config{ClientID: o.clientID}) o.oauth = &oauth2.Config{ @@ -96,12 +111,19 @@ type oidcTx struct { Nonce string `json:"n"` } +// redirectAuthError sends the browser back to the login page with an error code +// the UI (#6) can render. Codes: oidc_unavailable, state, no_email, +// email_unverified, oidc_conflict, oidc (generic). +func redirectAuthError(c *gin.Context, code string) { + c.Redirect(http.StatusFound, "/login?error="+code) +} + // oidcLogin starts the authorization-code + PKCE flow: it stashes fresh state, // PKCE verifier, and nonce in a short-lived cookie, then redirects to the IdP. func (h *handlers) oidcLogin(c *gin.Context) { if err := h.oidc.ensure(c.Request.Context()); err != nil { slog.Error("api: oidc discovery failed", "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc_unavailable") + redirectAuthError(c, "oidc_unavailable") return } @@ -109,12 +131,16 @@ func (h *handlers) oidcLogin(c *gin.Context) { nonce, err2 := randToken() if err1 != nil || err2 != nil { slog.Error("api: oidc token generation failed", "state_err", err1, "nonce_err", err2) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } verifier := oauth2.GenerateVerifier() - h.setOIDCTxCookie(c, oidcTx{State: state, Verifier: verifier, Nonce: nonce}) + if err := h.setOIDCTxCookie(c, oidcTx{State: state, Verifier: verifier, Nonce: nonce}); err != nil { + slog.Error("api: set oidc tx cookie", "error", err) + redirectAuthError(c, "oidc") + return + } authURL := h.oidc.oauth.AuthCodeURL(state, oauth2.S256ChallengeOption(verifier), @@ -128,15 +154,13 @@ func (h *handlers) oidcLogin(c *gin.Context) { // pansy session. Every failure clears the tx cookie and redirects to the login // page with an error code rather than leaking details to the browser. func (h *handlers) oidcCallback(c *gin.Context) { - ctx := c.Request.Context() - tx, haveTx := h.readOIDCTxCookie(c) h.clearOIDCTxCookie(c) // A provider-side error (e.g. user denied consent) comes back as ?error=. if e := c.Query("error"); e != "" { slog.Warn("api: oidc provider returned error", "error", e) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } @@ -145,45 +169,51 @@ func (h *handlers) oidcCallback(c *gin.Context) { state := c.Query("state") if !haveTx || state == "" || subtle.ConstantTimeCompare([]byte(state), []byte(tx.State)) != 1 { slog.Warn("api: oidc state mismatch or missing transaction") - c.Redirect(http.StatusFound, "/login?error=state") + redirectAuthError(c, "state") return } + // Bound the network work (token exchange + JWKS fetch during verify) so a slow + // IdP can't outlast the server's write timeout. + ctx, cancel := context.WithTimeout(c.Request.Context(), oidcExchangeTimeout) + defer cancel() + if err := h.oidc.ensure(ctx); err != nil { slog.Error("api: oidc discovery failed on callback", "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc_unavailable") + redirectAuthError(c, "oidc_unavailable") return } code := c.Query("code") if code == "" { - c.Redirect(http.StatusFound, "/login?error=oidc") + slog.Warn("api: oidc callback missing code") + redirectAuthError(c, "oidc") return } token, err := h.oidc.oauth.Exchange(ctx, code, oauth2.VerifierOption(tx.Verifier)) if err != nil { slog.Error("api: oidc code exchange failed", "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } rawIDToken, ok := token.Extra("id_token").(string) if !ok || rawIDToken == "" { slog.Error("api: oidc token response missing id_token") - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } idToken, err := h.oidc.verifier.Verify(ctx, rawIDToken) if err != nil { slog.Error("api: oidc id_token verification failed", "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1 { slog.Warn("api: oidc nonce mismatch") - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } @@ -195,7 +225,7 @@ func (h *handlers) oidcCallback(c *gin.Context) { } if err := idToken.Claims(&claims); err != nil { slog.Error("api: oidc claims decode failed", "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } name := claims.Name @@ -213,31 +243,47 @@ func (h *handlers) oidcCallback(c *gin.Context) { }) if err != nil { slog.Warn("api: oidc provisioning failed", "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, oidcProvisionErrorCode(err)) return } if err := h.startSession(c, user.ID); err != nil { slog.Error("api: oidc session start failed", "user_id", user.ID, "error", err) - c.Redirect(http.StatusFound, "/login?error=oidc") + redirectAuthError(c, "oidc") return } c.Redirect(http.StatusFound, "/gardens") } +// oidcProvisionErrorCode maps a LoginOIDC failure to a login-page error code so +// the UI can explain what went wrong instead of showing a generic message. +func oidcProvisionErrorCode(err error) string { + switch { + case errors.Is(err, domain.ErrOIDCNoEmail): + return "no_email" + case errors.Is(err, domain.ErrOIDCEmailUnverified): + return "email_unverified" + case errors.Is(err, domain.ErrOIDCIdentityConflict): + return "oidc_conflict" + default: + return "oidc" + } +} + // setOIDCTxCookie writes the login transaction as a compact base64 JSON cookie. -func (h *handlers) setOIDCTxCookie(c *gin.Context, tx oidcTx) { +// It returns an error rather than swallowing a marshal failure, so oidcLogin +// never redirects to the IdP with no way to complete the callback. +func (h *handlers) setOIDCTxCookie(c *gin.Context, tx oidcTx) error { b, err := json.Marshal(tx) if err != nil { - // tx holds only our own generated strings, so this cannot fail in practice. - slog.Error("api: marshal oidc tx", "error", err) - return + return err // tx holds only our own strings, so this can't happen in practice. } value := base64.RawURLEncoding.EncodeToString(b) // SameSite=Lax so the cookie survives the IdP's top-level redirect back to the // callback (Strict would drop it on that cross-site navigation). c.SetSameSite(http.SameSiteLaxMode) c.SetCookie(oidcTxCookie, value, int(oidcTxMaxAge.Seconds()), "/", "", h.cookieSecure(), true) + return nil } func (h *handlers) readOIDCTxCookie(c *gin.Context) (oidcTx, bool) { @@ -250,7 +296,9 @@ func (h *handlers) readOIDCTxCookie(c *gin.Context) (oidcTx, bool) { return oidcTx{}, false } var tx oidcTx - if err := json.Unmarshal(raw, &tx); err != nil || tx.State == "" || tx.Verifier == "" { + // All three fields must be present: an empty nonce would make the callback's + // nonce check pass vacuously against an ID token that omitted the claim. + if err := json.Unmarshal(raw, &tx); err != nil || tx.State == "" || tx.Verifier == "" || tx.Nonce == "" { return oidcTx{}, false } return tx, true diff --git a/internal/domain/domain.go b/internal/domain/domain.go index cfef925..145a168 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -39,10 +39,15 @@ var ( // ErrOIDCNoEmail means the IdP returned no email claim, so no account can be // provisioned (email is the account's unique key). The email scope is required. ErrOIDCNoEmail = errors.New("oidc identity has no email") - // ErrOIDCEmailUnverified means the IdP's email is unverified and it collides - // with an existing account; auto-linking it would enable account takeover, so - // it is refused. + // ErrOIDCEmailUnverified means the IdP asserted an email it hasn't verified; + // pansy won't provision or link on an unverified email (it would enable + // account takeover / squatting). ErrOIDCEmailUnverified = errors.New("oidc email not verified") + // ErrOIDCIdentityConflict means the OIDC identity can't be attached: either + // the target account already carries a different identity (refusing to + // overwrite it prevents lockout/takeover) or the (issuer, subject) pair is + // already bound to another account. Mapped to 409. + ErrOIDCIdentityConflict = errors.New("oidc identity conflict") ) // Enumerated string values mirrored from the schema CHECK constraints. diff --git a/internal/service/auth.go b/internal/service/auth.go index e0c93a9..2e59958 100644 --- a/internal/service/auth.go +++ b/internal/service/auth.go @@ -57,19 +57,23 @@ func (s *Service) Providers() Providers { // issuing no session itself (the caller does). The IdP has already gated access, // so PANSY_REGISTRATION does not apply. Resolution order: // 1. an existing user with the same (issuer, subject) — a returning OIDC user; -// 2. else an existing user with the same *verified* email — linked to this -// identity (so one person isn't split across a local and an OIDC account); +// 2. else an existing user with the same email — linked to this identity (so +// one person isn't split across a local and an OIDC account); // 3. else a new just-in-time account stamped with the identity. // -// An unverified email that collides with an existing account is refused (it -// would let anyone who can assert that email at the IdP take over the account). -// An identity with no email can't be provisioned (email is the account key). +// Steps 2 and 3 require a verified email: linking on an unverified email would +// let anyone who can assert that email at an IdP take over an account, and +// JIT-creating on one would let them squat an email a real user later owns +// (then the real user's different identity would be refused by LinkOIDC). An +// identity with no email can't be provisioned at all (email is the account key). +// Returning users (step 1) skip the email check — their identity is already +// proven. func (s *Service) LoginOIDC(ctx context.Context, id OIDCIdentity) (*domain.User, error) { if id.Issuer == "" || id.Subject == "" { return nil, domain.ErrInvalidInput } - // 1. Returning OIDC user. + // 1. Returning OIDC user (identity already proven; email state irrelevant). u, err := s.store.GetUserByOIDC(ctx, id.Issuer, id.Subject) if err == nil { return u, nil @@ -78,18 +82,20 @@ func (s *Service) LoginOIDC(ctx context.Context, id OIDCIdentity) (*domain.User, return nil, err } + // Provisioning (link or create) requires a verified email. email := normalizeEmail(id.Email) if email == "" { return nil, domain.ErrOIDCNoEmail } + if !id.EmailVerified { + return nil, domain.ErrOIDCEmailUnverified + } - // 2. Link to an existing account by email — but only a verified one. + // 2. Link to an existing account by email. LinkOIDC refuses to overwrite a + // different stored identity (returns ErrOIDCIdentityConflict). existing, err := s.store.GetUserByEmail(ctx, email) switch { case err == nil: - if !id.EmailVerified { - return nil, domain.ErrOIDCEmailUnverified - } return s.store.LinkOIDC(ctx, existing.ID, id.Issuer, id.Subject) case !errors.Is(err, domain.ErrNotFound): return nil, err diff --git a/internal/service/auth_test.go b/internal/service/auth_test.go index 4140b9d..60b67d8 100644 --- a/internal/service/auth_test.go +++ b/internal/service/auth_test.go @@ -290,7 +290,7 @@ func TestProvidersReflectsConfig(t *testing.T) { // OIDC configured with a base URL → reported ready. ready := openConfig() ready.BaseURL = "https://pansy.example.com" - ready.OIDC = config.OIDCConfig{Issuer: "https://idp.example", ClientID: "cid", ButtonLabel: "Sign in with Authentik"} + ready.OIDC = config.OIDCConfig{Issuer: testOIDCIssuer, ClientID: "cid", ButtonLabel: "Sign in with Authentik"} if got := newTestService(t, ready).Providers(); !got.OIDC || got.OIDCLabel != "Sign in with Authentik" { t.Errorf("providers = %+v, want oidc=true with Authentik label", got) } @@ -303,8 +303,11 @@ func TestProvidersReflectsConfig(t *testing.T) { } } +// testOIDCIssuer is the issuer URL used across the OIDC service tests. +const testOIDCIssuer = "https://idp.example" + func oidcIdentity(sub, email, name string, verified bool) OIDCIdentity { - return OIDCIdentity{Issuer: "https://idp.example", Subject: sub, Email: email, EmailVerified: verified, Name: name} + return OIDCIdentity{Issuer: testOIDCIssuer, Subject: sub, Email: email, EmailVerified: verified, Name: name} } func TestLoginOIDCJITProvisionsThenReturnsSameUser(t *testing.T) { @@ -351,14 +354,47 @@ func TestLoginOIDCLinksExistingLocalAccount(t *testing.T) { } } -func TestLoginOIDCRefusesUnverifiedEmailCollision(t *testing.T) { +func TestLoginOIDCRefusesUnverifiedEmail(t *testing.T) { s := newTestService(t, openConfig()) - mustRegister(t, s, "carol@example.com", "Carol", "password123") - _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3", "carol@example.com", "Carol", false)) - if !errors.Is(err, domain.ErrOIDCEmailUnverified) { + // Unverified email colliding with an existing account is refused (takeover). + mustRegister(t, s, "carol@example.com", "Carol", "password123") + if _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3", "carol@example.com", "Carol", false)); !errors.Is(err, domain.ErrOIDCEmailUnverified) { t.Errorf("unverified collision err = %v, want ErrOIDCEmailUnverified", err) } + + // Unverified email with NO collision is also refused (can't JIT-provision on + // an unverified email — it could squat an address a real user later owns, and + // could make an unverified identity the first admin). + if _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3b", "fresh@example.com", "Fresh", false)); !errors.Is(err, domain.ErrOIDCEmailUnverified) { + t.Errorf("unverified JIT err = %v, want ErrOIDCEmailUnverified", err) + } +} + +func TestLoginOIDCRefusesOverwritingDifferentIdentity(t *testing.T) { + s := newTestService(t, openConfig()) + + // A user links identity A. + first, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-A", "dana@example.com", "Dana", true)) + if err != nil { + t.Fatalf("initial link: %v", err) + } + + // A different identity asserting the same verified email must NOT overwrite + // the stored identity (that would hijack/lock out the account). + _, err = s.LoginOIDC(context.Background(), oidcIdentity("sub-B", "dana@example.com", "Dana", true)) + if !errors.Is(err, domain.ErrOIDCIdentityConflict) { + t.Fatalf("overwrite attempt err = %v, want ErrOIDCIdentityConflict", err) + } + + // The original identity still works and still points at the same account. + again, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-A", "dana@example.com", "Dana", true)) + if err != nil || again.ID != first.ID { + t.Errorf("original identity broken: user=%v err=%v", again, err) + } + if again.OIDCSubject == nil || *again.OIDCSubject != "sub-A" { + t.Errorf("stored identity was overwritten: %v", again.OIDCSubject) + } } func TestLoginOIDCRequiresEmail(t *testing.T) { diff --git a/internal/store/users.go b/internal/store/users.go index 740475d..f208f3d 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -120,26 +120,37 @@ func (d *DB) GetUserByOIDC(ctx context.Context, issuer, subject string) (*domain } // LinkOIDC stamps an OIDC identity onto an existing user (first OIDC login for a -// pre-existing local account) and returns the updated row. A collision with -// another user's identity pair trips the UNIQUE index and maps to ErrEmailTaken -// as a generic conflict (it shouldn't happen — the caller looks up by identity -// first — but the index is the backstop). +// pre-existing local account) and returns the updated row. +// +// The UPDATE only matches when the account carries no identity yet, or already +// carries this exact one (idempotent) — it will NOT overwrite a different stored +// identity, which would let anyone asserting the same email at a second IdP +// hijack or lock out the account. A no-match (different identity, or the row is +// gone) and a UNIQUE (issuer, subject) collision with another account both +// surface as domain.ErrOIDCIdentityConflict. RETURNING folds the read-back into +// the same statement. func (d *DB) LinkOIDC(ctx context.Context, userID int64, issuer, subject string) (*domain.User, error) { - _, err := d.sql.ExecContext(ctx, + u, err := scanUser(d.sql.QueryRowContext(ctx, `UPDATE users SET oidc_issuer = ?, oidc_subject = ?, version = version + 1, updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now') - WHERE id = ?`, - issuer, subject, userID, - ) + WHERE id = ? + AND (oidc_subject IS NULL OR (oidc_issuer = ? AND oidc_subject = ?)) + RETURNING `+userColumns, + issuer, subject, userID, issuer, subject, + )) + if errors.Is(err, sql.ErrNoRows) { + // The account already has a different identity (or no longer exists). + return nil, domain.ErrOIDCIdentityConflict + } if err != nil { if isUniqueViolation(err) { - return nil, domain.ErrEmailTaken + return nil, domain.ErrOIDCIdentityConflict } return nil, fmt.Errorf("store: link oidc: %w", err) } - return d.GetUserByID(ctx, userID) + return u, nil } // CountUsers returns the number of user rows. Used to decide first-user-is-admin