From 84edf3e42a87b4452f6a4cad6ee5f23779559f10 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 17:13:47 -0400 Subject: [PATCH 1/2] Add OIDC login via Authentik: PKCE, JIT provisioning, email linking (#5) OIDC is pansy's primary login path (Authentik the target IdP); local auth (#4) remains the fallback and both issue the same session cookie. - deps: github.com/coreos/go-oidc/v3 + golang.org/x/oauth2 (both pure Go; CGO stays off). - api/oidc.go: lazy issuer discovery (retried per-request, never crashes a server that also serves local auth), GET /auth/oidc/login builds an authorization-code URL with PKCE S256 + random state + nonce stashed in a short-lived HttpOnly cookie, GET /auth/oidc/callback verifies state (constant-time), exchanges the code with the PKCE verifier, verifies the ID token + nonce, and starts a pansy session. Failures redirect to /login?error=... ; success to /gardens. - service.LoginOIDC: (issuer,subject) match -> login; else *verified* email match -> link onto the existing account; else JIT-create (the IdP gates access, so PANSY_REGISTRATION doesn't apply). Unverified email colliding with an existing account is refused (takeover guard); no email is refused (email is the account key). Reuses the atomic CreateUser. - store: GetUserByOIDC + LinkOIDC (unique-pair backstop). - config: OIDCReady() (needs issuer+client+BaseURL for the redirect URI); /auth/providers now reports oidc from it and defaults the button label to "Sign in with Authentik". OIDC routes are only registered when ready, so an unconfigured instance 404s them. - PANSY_LOCAL_AUTH=false rejects/hides local auth but not OIDC. Tests: service provisioning (JIT, repeat login, link, unverified-collision refusal, no-email, name fallback, works with local auth off); api (routes-absent-when-unconfigured, providers reporting, login redirect with PKCE params + tx cookie via a fake discovery server, callback state/error paths). Smoke-tested: unreachable issuer degrades to error=oidc_unavailable with the server still up and local auth working. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- README.md | 6 +- go.mod | 3 + go.sum | 6 + internal/api/api.go | 18 ++- internal/api/auth.go | 14 +- internal/api/oidc.go | 271 ++++++++++++++++++++++++++++++++++ internal/api/oidc_test.go | 114 ++++++++++++++ internal/config/config.go | 10 +- internal/domain/domain.go | 8 + internal/service/auth.go | 84 ++++++++++- internal/service/auth_test.go | 113 +++++++++++++- internal/store/users.go | 37 +++++ 12 files changed, 664 insertions(+), 20 deletions(-) create mode 100644 internal/api/oidc.go create mode 100644 internal/api/oidc_test.go diff --git a/README.md b/README.md index dd79f6d..1a0626f 100644 --- a/README.md +++ b/README.md @@ -54,10 +54,12 @@ All configuration is via environment variables; every value has a default, so `. | `PANSY_OIDC_ISSUER` | *(empty)* | OIDC issuer/discovery URL (Authentik). Enables SSO when set. | | `PANSY_OIDC_CLIENT_ID` | *(empty)* | OIDC client ID. | | `PANSY_OIDC_CLIENT_SECRET`| *(empty)* | OIDC client secret. | -| `PANSY_OIDC_BUTTON_LABEL` | `Sign in with SSO` | Label for the OIDC button on the login page. | +| `PANSY_OIDC_BUTTON_LABEL` | `Sign in with Authentik` | Label for the OIDC button on the login page. | | `PANSY_TRUSTED_PROXIES` | *(none)* | Comma-separated proxy CIDRs/IPs to trust for client-IP resolution. | -Local email/password auth is live (`POST /api/v1/auth/register`, `/auth/login`, `/auth/logout`, `GET /auth/me`, `GET /auth/providers`); the session is an HttpOnly cookie (`Secure` when `PANSY_BASE_URL` is https). The first account registered becomes admin, and it may register even when `PANSY_REGISTRATION=closed` to bootstrap the instance. OIDC (`PANSY_OIDC_*`) lands in #5. +Local email/password auth is live (`POST /api/v1/auth/register`, `/auth/login`, `/auth/logout`, `GET /auth/me`, `GET /auth/providers`); the session is an HttpOnly cookie (`Secure` when `PANSY_BASE_URL` is https). The first account registered becomes admin, and it may register even when `PANSY_REGISTRATION=closed` to bootstrap the instance. + +OIDC (Authentik-first) is live too: set `PANSY_OIDC_ISSUER`, `PANSY_OIDC_CLIENT_ID`, `PANSY_OIDC_CLIENT_SECRET`, and `PANSY_BASE_URL` (needed for the redirect URI). Register `PANSY_BASE_URL` + `/api/v1/auth/oidc/callback` as the redirect URI in your IdP. `GET /auth/oidc/login` starts an authorization-code + PKCE flow; first login provisions a user just-in-time (a matching *verified* email links to an existing local account instead of duplicating it). Provider discovery is lazy, so a briefly-unreachable IdP never blocks startup or local auth. Set `PANSY_LOCAL_AUTH=false` for pure-Authentik deployments (local register/login are then rejected and hidden from `/auth/providers`). ## Docker & deployment diff --git a/go.mod b/go.mod index 51d77d4..d28448b 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,11 @@ module gitea.stevedudenhoeffer.com/steve/pansy go 1.26.2 require ( + github.com/coreos/go-oidc/v3 v3.20.0 github.com/gin-gonic/gin v1.10.1 github.com/samber/slog-gin v1.15.0 golang.org/x/crypto v0.31.0 + golang.org/x/oauth2 v0.36.0 modernc.org/sqlite v1.34.4 ) @@ -17,6 +19,7 @@ require ( github.com/dustin/go-humanize v1.0.1 // indirect github.com/gabriel-vasile/mimetype v1.4.4 // indirect github.com/gin-contrib/sse v0.1.0 // indirect + github.com/go-jose/go-jose/v4 v4.1.4 // indirect github.com/go-playground/locales v0.14.1 // indirect github.com/go-playground/universal-translator v0.18.1 // indirect github.com/go-playground/validator/v10 v10.22.0 // indirect diff --git a/go.sum b/go.sum index 4c8c120..42444ed 100644 --- a/go.sum +++ b/go.sum @@ -6,6 +6,8 @@ github.com/cloudwego/base64x v0.1.4 h1:jwCgWpFanWmN8xoIUHa2rtzmkd5J2plF/dnLS6Xd/ github.com/cloudwego/base64x v0.1.4/go.mod h1:0zlkT4Wn5C6NdauXdJRhSKRlJvmclQ1hhJgA0rcu/8w= github.com/cloudwego/iasm v0.2.0 h1:1KNIy1I1H9hNNFEEH3DVnI4UujN+1zjpuk6gwHLTssg= github.com/cloudwego/iasm v0.2.0/go.mod h1:8rXZaNYT2n95jn+zTI1sDr+IgcD2GVs0nlbbQPiEFhY= +github.com/coreos/go-oidc/v3 v3.20.0 h1:EtE0WIBHk03N+DqGkY4+UONzzZHk7amKt6IyNd7OsZE= +github.com/coreos/go-oidc/v3 v3.20.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -17,6 +19,8 @@ github.com/gin-contrib/sse v0.1.0 h1:Y/yl/+YNO8GZSjAhjMsSuLt29uWRFHdHYUb5lYOV9qE github.com/gin-contrib/sse v0.1.0/go.mod h1:RHrZQHXnP2xjPF+u1gW/2HnVO7nvIa9PG3Gm+fLHvGI= github.com/gin-gonic/gin v1.10.1 h1:T0ujvqyCSqRopADpgPgiTT63DUQVSfojyME59Ei63pQ= github.com/gin-gonic/gin v1.10.1/go.mod h1:4PMNQiOhvDRa013RKVbsiNwoyezlm2rm0uX/T7kzp5Y= +github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= +github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-playground/assert/v2 v2.2.0 h1:JvknZsQTYeFEAhQwI4qEt9cyV5ONwRHC+lYKSsYSR8s= github.com/go-playground/assert/v2 v2.2.0/go.mod h1:VDjEfimB/XKnb+ZQfWdccd7VUvScMdVu0Titje2rxJ4= github.com/go-playground/locales v0.14.1 h1:EWaQ/wswjilfKLTECiXz7Rh+3BjFhfDFKv/oXslEjJA= @@ -94,6 +98,8 @@ golang.org/x/mod v0.17.0 h1:zY54UmvipHiNd+pm+m0x9KhZ9hl1/7QNMyxXbc6ICqA= golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/net v0.33.0 h1:74SYHlV8BIgHIFC/LrYkOGIwL19eTYXQ5wc6TBuO36I= golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4= +golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= +golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ= golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/internal/api/api.go b/internal/api/api.go index 87ef607..72c905a 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -19,8 +19,9 @@ import ( // handlers carries the dependencies shared by every HTTP handler. Handlers stay // thin: decode the request, call a service method, encode the result. type handlers struct { - cfg *config.Config - svc *service.Service + cfg *config.Config + svc *service.Service + oidc *oidcClient // nil unless OIDC is configured (see config.OIDCReady) } // New builds the gin engine with the standard middleware stack and registers the @@ -59,6 +60,19 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { auth.GET("/providers", h.providers) auth.GET("/me", h.requireAuth(), h.me) + // OIDC routes exist only when OIDC can actually be offered, so an unconfigured + // instance 404s them (matching what /auth/providers advertises). Provider + // discovery is lazy (first request), so a briefly-unreachable IdP doesn't stop + // the server — or local auth — from starting. + switch { + case cfg.OIDCReady(): + h.oidc = newOIDCClient(cfg) + auth.GET("/oidc/login", h.oidcLogin) + auth.GET("/oidc/callback", h.oidcCallback) + case cfg.OIDC.Enabled(): + slog.Warn("api: OIDC is configured but PANSY_BASE_URL is unset; OIDC disabled (an absolute redirect URI is required)") + } + return r } diff --git a/internal/api/auth.go b/internal/api/auth.go index ca06dec..8f27483 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -160,18 +160,26 @@ func (h *handlers) csrfGuard() gin.HandlerFunc { } } +// startSession issues a session for the user and writes the session cookie. +func (h *handlers) startSession(c *gin.Context, userID int64) error { + token, expiresAt, err := h.svc.CreateSession(c.Request.Context(), userID) + if err != nil { + return err + } + h.setSessionCookie(c, token, expiresAt) + return nil +} + // startSessionAndRespond issues a session, sets the cookie, and returns the user // as JSON — the shared tail of register and login. func (h *handlers) startSessionAndRespond(c *gin.Context, user *domain.User) { - token, expiresAt, err := h.svc.CreateSession(c.Request.Context(), user.ID) - if err != nil { + if err := h.startSession(c, user.ID); err != nil { // The account exists and is usable via login; only the auto-login cookie // failed. Log it so the operator can see the underlying DB problem. slog.Error("api: could not start session", "user_id", user.ID, "error", err) writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "could not start session") return } - h.setSessionCookie(c, token, expiresAt) c.JSON(http.StatusOK, user) } diff --git a/internal/api/oidc.go b/internal/api/oidc.go new file mode 100644 index 0000000..ba516b8 --- /dev/null +++ b/internal/api/oidc.go @@ -0,0 +1,271 @@ +package api + +import ( + "context" + "crypto/rand" + "crypto/subtle" + "encoding/base64" + "encoding/json" + "log/slog" + "net/http" + "sync" + "time" + + "github.com/coreos/go-oidc/v3/oidc" + "github.com/gin-gonic/gin" + "golang.org/x/oauth2" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" + "gitea.stevedudenhoeffer.com/steve/pansy/internal/service" +) + +const ( + // oidcCallbackPath is appended to PANSY_BASE_URL to form the redirect URI + // registered with the IdP. + oidcCallbackPath = "/api/v1/auth/oidc/callback" + // oidcTxCookie holds the short-lived per-login transaction state (CSRF state, + // PKCE verifier, nonce) between /oidc/login and /oidc/callback. + oidcTxCookie = "pansy_oidc_tx" + oidcTxMaxAge = 10 * time.Minute + // oidcDiscoveryTimeout bounds a single lazy discovery attempt so a hung IdP + // can't wedge a request goroutine. + oidcDiscoveryTimeout = 10 * time.Second +) + +// oidcClient lazily performs OIDC discovery and holds the derived verifier and +// oauth2 config. Discovery is deferred to the first login/callback (and retried +// on the next request if it fails) so an IdP that's momentarily unreachable at +// boot doesn't stop the server — or local auth — from starting. +type oidcClient struct { + issuer string + clientID string + clientSecret string + redirectURL string + + mu sync.Mutex + provider *oidc.Provider + verifier *oidc.IDTokenVerifier + oauth *oauth2.Config +} + +func newOIDCClient(cfg *config.Config) *oidcClient { + return &oidcClient{ + issuer: cfg.OIDC.Issuer, + clientID: cfg.OIDC.ClientID, + clientSecret: cfg.OIDC.ClientSecret, + redirectURL: cfg.BaseURL + oidcCallbackPath, + } +} + +// 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. +func (o *oidcClient) ensure(ctx context.Context) error { + o.mu.Lock() + defer o.mu.Unlock() + if o.provider != nil { + return nil + } + + dctx, cancel := context.WithTimeout(ctx, oidcDiscoveryTimeout) + defer cancel() + + provider, err := oidc.NewProvider(dctx, o.issuer) + if err != nil { + return err + } + o.provider = provider + o.verifier = provider.Verifier(&oidc.Config{ClientID: o.clientID}) + o.oauth = &oauth2.Config{ + ClientID: o.clientID, + ClientSecret: o.clientSecret, + Endpoint: provider.Endpoint(), + RedirectURL: o.redirectURL, + Scopes: []string{oidc.ScopeOpenID, "email", "profile"}, + } + return nil +} + +// oidcTx is the per-login transaction stashed in the tx cookie. It never leaves +// the browser as anything an attacker can forge (HttpOnly, our domain), and the +// callback checks the returned state against it (CSRF), uses the PKCE verifier +// for the code exchange, and checks the ID-token nonce. +type oidcTx struct { + State string `json:"s"` + Verifier string `json:"v"` + Nonce string `json:"n"` +} + +// 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") + return + } + + state, err1 := randToken() + 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") + return + } + verifier := oauth2.GenerateVerifier() + + h.setOIDCTxCookie(c, oidcTx{State: state, Verifier: verifier, Nonce: nonce}) + + authURL := h.oidc.oauth.AuthCodeURL(state, + oauth2.S256ChallengeOption(verifier), + oidc.Nonce(nonce), + ) + c.Redirect(http.StatusFound, authURL) +} + +// oidcCallback completes the flow: verify state, exchange the code (with the PKCE +// verifier), verify the ID token and nonce, provision/link the user, and start a +// 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") + return + } + + // State must be present and match the cookie (CSRF protection). Constant-time + // compare avoids leaking via timing. + 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") + return + } + + 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") + return + } + + code := c.Query("code") + if code == "" { + c.Redirect(http.StatusFound, "/login?error=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") + 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") + 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") + return + } + if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1 { + slog.Warn("api: oidc nonce mismatch") + c.Redirect(http.StatusFound, "/login?error=oidc") + return + } + + var claims struct { + Email string `json:"email"` + EmailVerified bool `json:"email_verified"` + Name string `json:"name"` + PreferredUsername string `json:"preferred_username"` + } + if err := idToken.Claims(&claims); err != nil { + slog.Error("api: oidc claims decode failed", "error", err) + c.Redirect(http.StatusFound, "/login?error=oidc") + return + } + name := claims.Name + if name == "" { + name = claims.PreferredUsername + } + + // Issuer/Subject come from the *verified* token, not raw claims. + user, err := h.svc.LoginOIDC(ctx, service.OIDCIdentity{ + Issuer: idToken.Issuer, + Subject: idToken.Subject, + Email: claims.Email, + EmailVerified: claims.EmailVerified, + Name: name, + }) + if err != nil { + slog.Warn("api: oidc provisioning failed", "error", err) + c.Redirect(http.StatusFound, "/login?error=oidc") + 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") + return + } + c.Redirect(http.StatusFound, "/gardens") +} + +// setOIDCTxCookie writes the login transaction as a compact base64 JSON cookie. +func (h *handlers) setOIDCTxCookie(c *gin.Context, tx oidcTx) { + 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 + } + 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) +} + +func (h *handlers) readOIDCTxCookie(c *gin.Context) (oidcTx, bool) { + value, err := c.Cookie(oidcTxCookie) + if err != nil || value == "" { + return oidcTx{}, false + } + raw, err := base64.RawURLEncoding.DecodeString(value) + if err != nil { + return oidcTx{}, false + } + var tx oidcTx + if err := json.Unmarshal(raw, &tx); err != nil || tx.State == "" || tx.Verifier == "" { + return oidcTx{}, false + } + return tx, true +} + +func (h *handlers) clearOIDCTxCookie(c *gin.Context) { + c.SetSameSite(http.SameSiteLaxMode) + c.SetCookie(oidcTxCookie, "", -1, "/", "", h.cookieSecure(), true) +} + +// randToken returns 32 bytes of URL-safe randomness for state/nonce values. +func randToken() (string, error) { + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(b), nil +} diff --git a/internal/api/oidc_test.go b/internal/api/oidc_test.go new file mode 100644 index 0000000..d51ea18 --- /dev/null +++ b/internal/api/oidc_test.go @@ -0,0 +1,114 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" +) + +// fakeIssuer stands up a minimal OIDC discovery endpoint so oidcClient.ensure +// succeeds without a real IdP. It only needs to serve the discovery document for +// the login-initiation and route tests; token exchange is covered by the +// service-layer provisioning tests and manual Authentik verification. +func fakeIssuer(t *testing.T) string { + t.Helper() + mux := http.NewServeMux() + var issuer string + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": issuer, + "authorization_endpoint": issuer + "/authorize", + "token_endpoint": issuer + "/token", + "jwks_uri": issuer + "/jwks", + }) + }) + ts := httptest.NewServer(mux) + t.Cleanup(ts.Close) + issuer = ts.URL + return issuer +} + +func oidcCfg(t *testing.T) *config.Config { + cfg := localCfg() + cfg.BaseURL = "https://pansy.example.com" + cfg.OIDC = config.OIDCConfig{Issuer: fakeIssuer(t), ClientID: "pansy-client", ButtonLabel: "Sign in with Authentik"} + return cfg +} + +func TestOIDCRoutesAbsentWhenUnconfigured(t *testing.T) { + r := authEngine(t, localCfg()) // no OIDC + for _, path := range []string{"/api/v1/auth/oidc/login", "/api/v1/auth/oidc/callback"} { + w := doJSON(t, r, http.MethodGet, path, nil, nil) + if w.Code != http.StatusNotFound { + t.Errorf("GET %s status = %d, want 404 when OIDC unconfigured", path, w.Code) + } + } +} + +func TestProvidersReportsOIDCWhenConfigured(t *testing.T) { + r := authEngine(t, oidcCfg(t)) + w := doJSON(t, r, http.MethodGet, "/api/v1/auth/providers", nil, nil) + var got struct { + Local bool `json:"local"` + OIDC bool `json:"oidc"` + OIDCLabel string `json:"oidcLabel"` + } + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatalf("decode providers: %v", err) + } + if !got.OIDC || got.OIDCLabel != "Sign in with Authentik" { + t.Errorf("providers = %+v, want oidc=true with Authentik label", got) + } +} + +func TestOIDCLoginRedirectsToProviderWithPKCE(t *testing.T) { + r := authEngine(t, oidcCfg(t)) + w := doJSON(t, r, http.MethodGet, "/api/v1/auth/oidc/login", nil, nil) + if w.Code != http.StatusFound { + t.Fatalf("oidc login status = %d, want 302 (body %s)", w.Code, w.Body.String()) + } + loc := w.Header().Get("Location") + for _, want := range []string{"/authorize?", "code_challenge=", "code_challenge_method=S256", "state=", "nonce="} { + if !strings.Contains(loc, want) { + t.Errorf("auth redirect %q missing %q", loc, want) + } + } + // The transaction cookie must be set so the callback can validate state. + var haveTx bool + for _, ck := range w.Result().Cookies() { + if ck.Name == oidcTxCookie { + haveTx = ck.HttpOnly && ck.Value != "" + } + } + if !haveTx { + t.Error("expected an HttpOnly pansy_oidc_tx cookie to be set") + } +} + +func TestOIDCCallbackWithoutTransactionRejected(t *testing.T) { + r := authEngine(t, oidcCfg(t)) + // No tx cookie → state can't be validated → redirect to login with error=state. + w := doJSON(t, r, http.MethodGet, "/api/v1/auth/oidc/callback?state=abc&code=xyz", nil, nil) + if w.Code != http.StatusFound { + t.Fatalf("callback status = %d, want 302", w.Code) + } + if loc := w.Header().Get("Location"); !strings.Contains(loc, "error=state") { + t.Errorf("callback redirect = %q, want error=state", loc) + } +} + +func TestOIDCCallbackProviderErrorRedirects(t *testing.T) { + r := authEngine(t, oidcCfg(t)) + w := doJSON(t, r, http.MethodGet, "/api/v1/auth/oidc/callback?error=access_denied", nil, nil) + if w.Code != http.StatusFound { + t.Fatalf("callback status = %d, want 302", w.Code) + } + if loc := w.Header().Get("Location"); !strings.Contains(loc, "error=oidc") { + t.Errorf("callback redirect = %q, want error=oidc", loc) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 4347d21..bdbc4f4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -56,6 +56,14 @@ func (o OIDCConfig) Enabled() bool { return o.Issuer != "" && o.ClientID != "" } +// OIDCReady reports whether OIDC login can actually be offered: it needs an +// issuer + client ID and a BaseURL to build the absolute redirect URI that +// providers require. Both the login page (via /auth/providers) and route +// registration gate on this, so the advertised methods match the live routes. +func (c *Config) OIDCReady() bool { + return c.OIDC.Enabled() && c.BaseURL != "" +} + // RegistrationOpen reports whether local self-service signup is allowed. func (c *Config) RegistrationOpen() bool { return c.Registration == RegistrationOpen @@ -74,7 +82,7 @@ func Load() *Config { Issuer: envStr("PANSY_OIDC_ISSUER", ""), ClientID: envStr("PANSY_OIDC_CLIENT_ID", ""), ClientSecret: envStr("PANSY_OIDC_CLIENT_SECRET", ""), - ButtonLabel: envStr("PANSY_OIDC_BUTTON_LABEL", "Sign in with SSO"), + ButtonLabel: envStr("PANSY_OIDC_BUTTON_LABEL", "Sign in with Authentik"), }, TrustedProxies: envList("PANSY_TRUSTED_PROXIES"), } diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 57e4766..cfef925 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -35,6 +35,14 @@ var ( // ErrLocalAuthDisabled means PANSY_LOCAL_AUTH=false, so local register/login // are rejected in favor of OIDC. Mapped to 403. ErrLocalAuthDisabled = errors.New("local authentication disabled") + + // 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 = errors.New("oidc email not verified") ) // Enumerated string values mirrored from the schema CHECK constraints. diff --git a/internal/service/auth.go b/internal/service/auth.go index 6d284b2..e0c93a9 100644 --- a/internal/service/auth.go +++ b/internal/service/auth.go @@ -30,17 +30,84 @@ type Providers struct { OIDCLabel string `json:"oidcLabel"` } -// Providers returns the enabled auth methods. OIDC is always false until #5 -// wires the endpoints; advertising it before then would point the UI at routes -// that don't exist. +// OIDCIdentity is the set of claims the API layer extracts from a verified ID +// token and hands to LoginOIDC for provisioning. Issuer and Subject come from +// the verified token (not raw claims); Email/Name come from claims. +type OIDCIdentity struct { + Issuer string + Subject string + Email string + EmailVerified bool + Name string +} + +// Providers returns the enabled auth methods, so the login page renders the +// right controls. OIDC is reported only when it can actually be offered (issuer, +// client ID, and a BaseURL for the redirect URI), matching the routes that +// api.New registers. func (s *Service) Providers() Providers { return Providers{ Local: s.cfg.LocalAuth, - OIDC: false, + OIDC: s.cfg.OIDCReady(), OIDCLabel: s.cfg.OIDC.ButtonLabel, } } +// LoginOIDC provisions and returns the pansy user for a verified OIDC identity, +// 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); +// 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). +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. + u, err := s.store.GetUserByOIDC(ctx, id.Issuer, id.Subject) + if err == nil { + return u, nil + } + if !errors.Is(err, domain.ErrNotFound) { + return nil, err + } + + email := normalizeEmail(id.Email) + if email == "" { + return nil, domain.ErrOIDCNoEmail + } + + // 2. Link to an existing account by email — but only a verified one. + 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 + } + + // 3. Just-in-time provisioning. + name := strings.TrimSpace(id.Name) + if name == "" { + name = emailLocalPart(email) + } + return s.store.CreateUser(ctx, &domain.User{ + Email: email, + DisplayName: name, + OIDCIssuer: &id.Issuer, + OIDCSubject: &id.Subject, + }, true) // allowSignup: the IdP gates access, so registration policy is bypassed. +} + // Register creates a local (password) account and returns it. The first user on // a fresh instance becomes admin and may always register (bootstrap), even when // PANSY_REGISTRATION=closed; afterward, closed registration is enforced. @@ -208,3 +275,12 @@ func (s *Service) CleanupExpiredSessions(ctx context.Context) (int64, error) { func normalizeEmail(email string) string { return strings.ToLower(strings.TrimSpace(email)) } + +// emailLocalPart returns the portion of an email before '@', used as a fallback +// display name for JIT-provisioned OIDC users whose token carried no name. +func emailLocalPart(email string) string { + if i := strings.IndexByte(email, '@'); i > 0 { + return email[:i] + } + return email +} diff --git a/internal/service/auth_test.go b/internal/service/auth_test.go index 5a85bde..4140b9d 100644 --- a/internal/service/auth_test.go +++ b/internal/service/auth_test.go @@ -280,15 +280,112 @@ func TestCleanupExpiredSessions(t *testing.T) { } func TestProvidersReflectsConfig(t *testing.T) { - cfg := openConfig() - cfg.OIDC.ButtonLabel = "Sign in with Authentik" - s := newTestService(t, cfg) - + // No OIDC configured → local only. + s := newTestService(t, openConfig()) p := s.Providers() - if !p.Local { - t.Error("expected local=true") + if !p.Local || p.OIDC { + t.Errorf("providers = %+v, want local=true oidc=false", p) } - if p.OIDC { - t.Error("OIDC must be false until #5 wires it") + + // 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"} + 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) + } + + // OIDC configured but no base URL → not ready (can't build a redirect URI). + noBase := openConfig() + noBase.OIDC = config.OIDCConfig{Issuer: "https://idp.example", ClientID: "cid"} + if got := newTestService(t, noBase).Providers(); got.OIDC { + t.Error("OIDC should be false without a base URL") + } +} + +func oidcIdentity(sub, email, name string, verified bool) OIDCIdentity { + return OIDCIdentity{Issuer: "https://idp.example", Subject: sub, Email: email, EmailVerified: verified, Name: name} +} + +func TestLoginOIDCJITProvisionsThenReturnsSameUser(t *testing.T) { + s := newTestService(t, openConfig()) + + u, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-1", "alice@example.com", "Alice", true)) + if err != nil { + t.Fatalf("LoginOIDC create: %v", err) + } + if !u.IsAdmin { + t.Error("first provisioned OIDC user should be admin") + } + if u.OIDCSubject == nil || *u.OIDCSubject != "sub-1" { + t.Errorf("oidc subject not stamped: %+v", u.OIDCSubject) + } + + // A second login with the same identity returns the same user (no duplicate). + again, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-1", "alice@example.com", "Alice", true)) + if err != nil { + t.Fatalf("LoginOIDC repeat: %v", err) + } + if again.ID != u.ID { + t.Errorf("repeat login made a new user: %d vs %d", again.ID, u.ID) + } +} + +func TestLoginOIDCLinksExistingLocalAccount(t *testing.T) { + s := newTestService(t, openConfig()) + local := mustRegister(t, s, "bob@example.com", "Bob", "password123") + + linked, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-2", "Bob@Example.com", "Bob", true)) + if err != nil { + t.Fatalf("LoginOIDC link: %v", err) + } + if linked.ID != local.ID { + t.Errorf("linked to a new user %d, want existing %d", linked.ID, local.ID) + } + if linked.OIDCSubject == nil || *linked.OIDCSubject != "sub-2" { + t.Error("existing account was not stamped with the oidc identity") + } + // Local password still works after linking (one account, two methods). + if _, err := s.Login(context.Background(), "bob@example.com", "password123"); err != nil { + t.Errorf("local login after linking failed: %v", err) + } +} + +func TestLoginOIDCRefusesUnverifiedEmailCollision(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) { + t.Errorf("unverified collision err = %v, want ErrOIDCEmailUnverified", err) + } +} + +func TestLoginOIDCRequiresEmail(t *testing.T) { + s := newTestService(t, openConfig()) + _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-4", "", "No Email", true)) + if !errors.Is(err, domain.ErrOIDCNoEmail) { + t.Errorf("no-email err = %v, want ErrOIDCNoEmail", err) + } +} + +func TestLoginOIDCFallsBackToEmailLocalPartForName(t *testing.T) { + s := newTestService(t, openConfig()) + u, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-5", "dave@example.com", "", true)) + if err != nil { + t.Fatalf("LoginOIDC: %v", err) + } + if u.DisplayName != "dave" { + t.Errorf("display name = %q, want %q", u.DisplayName, "dave") + } +} + +func TestLocalAuthDisabledDoesNotBlockOIDC(t *testing.T) { + cfg := openConfig() + cfg.LocalAuth = false + s := newTestService(t, cfg) + // OIDC provisioning must work even when local auth is off (pure-Authentik). + if _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-6", "erin@example.com", "Erin", true)); err != nil { + t.Errorf("LoginOIDC with local auth disabled: %v", err) } } diff --git a/internal/store/users.go b/internal/store/users.go index a7e3548..740475d 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -105,6 +105,43 @@ func (d *DB) GetUserByEmail(ctx context.Context, email string) (*domain.User, er return u, nil } +// GetUserByOIDC returns the user with the given (issuer, subject) identity pair, +// or domain.ErrNotFound. Both arguments must be non-empty. +func (d *DB) GetUserByOIDC(ctx context.Context, issuer, subject string) (*domain.User, error) { + u, err := scanUser(d.sql.QueryRowContext(ctx, + `SELECT `+userColumns+` FROM users WHERE oidc_issuer = ? AND oidc_subject = ?`, issuer, subject)) + if errors.Is(err, sql.ErrNoRows) { + return nil, domain.ErrNotFound + } + if err != nil { + return nil, fmt.Errorf("store: get user by oidc: %w", err) + } + return u, nil +} + +// 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). +func (d *DB) LinkOIDC(ctx context.Context, userID int64, issuer, subject string) (*domain.User, error) { + _, err := d.sql.ExecContext(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, + ) + if err != nil { + if isUniqueViolation(err) { + return nil, domain.ErrEmailTaken + } + return nil, fmt.Errorf("store: link oidc: %w", err) + } + return d.GetUserByID(ctx, userID) +} + // CountUsers returns the number of user rows. Used to decide first-user-is-admin // and to allow bootstrap registration when signup is otherwise closed. func (d *DB) CountUsers(ctx context.Context) (int, error) { -- 2.54.0 From 8ef092713fc7810c2354611176f11335f54660d4 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 17:33:25 -0400 Subject: [PATCH 2/2] 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 -- 2.54.0