OIDC login via Authentik: PKCE, JIT provisioning, email linking (#5) #24
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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=
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -215,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:
|
||||
|
||||
@@ -0,0 +1,319 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/rand"
|
||||
"crypto/subtle"
|
||||
"encoding/base64"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"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/domain"
|
||||
"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
|
||||
// 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
|
||||
// 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. 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()
|
||||
ready := o.provider != nil
|
||||
o.mu.Unlock()
|
||||
if ready {
|
||||
return nil
|
||||
}
|
||||
|
||||
dctx, cancel := context.WithTimeout(ctx, oidcDiscoveryTimeout)
|
||||
defer cancel()
|
||||
|
||||
provider, err := oidc.NewProvider(dctx, o.issuer)
|
||||
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{
|
||||
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"`
|
||||
}
|
||||
|
||||
// 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)
|
||||
redirectAuthError(c, "oidc_unavailable")
|
||||
return
|
||||
}
|
||||
|
||||
state, err1 := randToken()
|
||||
|
gitea-actions
commented
🟠 oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper maintainability · flagged by 2 models
🪰 Gadfly · advisory 🟠 **oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper**
_maintainability · flagged by 2 models_
- **`oidcCallback` is a long, repetitive function with ~11 near-identical failure-redirect sites** (`internal/api/oidc.go:130-226`). Eight are `c.Redirect(http.StatusFound, "/login?error=oidc"); return` preceded by an `slog.*` call with slightly varying keys. A small helper like `oidcFail(c, code string, logMsg string, logAttrs ...any)` would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a gl…
<sub>🪰 Gadfly · advisory</sub>
|
||||
nonce, err2 := randToken()
|
||||
if err1 != nil || err2 != nil {
|
||||
slog.Error("api: oidc token generation failed", "state_err", err1, "nonce_err", err2)
|
||||
redirectAuthError(c, "oidc")
|
||||
return
|
||||
}
|
||||
verifier := oauth2.GenerateVerifier()
|
||||
|
||||
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),
|
||||
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) {
|
||||
tx, haveTx := h.readOIDCTxCookie(c)
|
||||
h.clearOIDCTxCookie(c)
|
||||
|
gitea-actions
commented
⚪ empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above maintainability · flagged by 1 model
🪰 Gadfly · advisory ⚪ **empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above**
_maintainability · flagged by 1 model_
- `internal/api/oidc.go:130-226` — `oidcCallback` repeats the same three-line shape (`slog.X(...)`; `c.Redirect(http.StatusFound, "/login?error=...")`; `return`) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception,…
<sub>🪰 Gadfly · advisory</sub>
|
||||
|
||||
// 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)
|
||||
redirectAuthError(c, "oidc")
|
||||
return
|
||||
|
gitea-actions
commented
🔴 oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout. error-handling · flagged by 2 models
🪰 Gadfly · advisory 🔴 **oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout.**
_error-handling · flagged by 2 models_
- **`internal/api/oidc.go:164` — token exchange and ID-token verification run on the unbounded request context.** `ctx := c.Request.Context()` (line 131) is passed directly to `h.oidc.oauth.Exchange(ctx, ...)` (line 164) and `h.oidc.verifier.Verify(ctx, rawIDToken)` (line 178). Only `ensure()` gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server's `WriteTimeout` is 30s (`cmd/pansy/main.go:65`), and the tx cookie is already cleared at line 134 befo…
<sub>🪰 Gadfly · advisory</sub>
|
||||
}
|
||||
|
||||
// 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")
|
||||
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)
|
||||
redirectAuthError(c, "oidc_unavailable")
|
||||
return
|
||||
}
|
||||
|
||||
code := c.Query("code")
|
||||
if code == "" {
|
||||
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)
|
||||
redirectAuthError(c, "oidc")
|
||||
return
|
||||
}
|
||||
|
||||
rawIDToken, ok := token.Extra("id_token").(string)
|
||||
if !ok || rawIDToken == "" {
|
||||
slog.Error("api: oidc token response missing id_token")
|
||||
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)
|
||||
redirectAuthError(c, "oidc")
|
||||
return
|
||||
}
|
||||
if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1 {
|
||||
slog.Warn("api: oidc nonce mismatch")
|
||||
|
gitea-actions
commented
🟡 All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render maintainability · flagged by 1 model
🪰 Gadfly · advisory 🟡 **All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render**
_maintainability · flagged by 1 model_
- **`internal/api/oidc.go:215` — `LoginOIDC` provisioning failures all collapse to `error=oidc`.** `LoginOIDC` returns distinct sentinels (`ErrOIDCNoEmail`, `ErrOIDCEmailUnverified`, `ErrInvalidInput` — confirmed in `domain/domain.go:39-45`) that the UI could meaningfully distinguish, but the callback maps every one to the generic `?error=oidc` at line 216. This mirrors the existing `writeServiceError` mapping philosophy but discards information that would help the login page show e.g. "email no…
<sub>🪰 Gadfly · advisory</sub>
|
||||
redirectAuthError(c, "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)
|
||||
redirectAuthError(c, "oidc")
|
||||
return
|
||||
|
gitea-actions
commented
🟡 OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector error-handling, maintainability, security · flagged by 2 models
🪰 Gadfly · advisory 🟡 **OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector**
_error-handling, maintainability, security · flagged by 2 models_
- **`internal/api/oidc.go:229-241` — tx cookie is not integrity-protected (low severity).** The `pansy_oidc_tx` cookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't *read* the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residua…
<sub>🪰 Gadfly · advisory</sub>
|
||||
}
|
||||
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)
|
||||
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)
|
||||
redirectAuthError(c, "oidc")
|
||||
return
|
||||
|
gitea-actions
commented
🔴 readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "". correctness, error-handling, maintainability · flagged by 4 models
🪰 Gadfly · advisory 🔴 **readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "".**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`internal/api/oidc.go:253` — `readOIDCTxCookie` does not require `tx.Nonce`.** The validity check at line 253 is `tx.State == "" || tx.Verifier == ""`; `Nonce` is not required. A hand-crafted or stale cookie yielding a tx with empty `Nonce` passes the check, and the nonce compare at line 184 (`ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))`) succeeds whenever the IdP also returns an empty nonce (many IdPs omit the `nonce` claim, leaving `idToken.Nonce == ""`), so the comparison…
<sub>🪰 Gadfly · advisory</sub>
|
||||
}
|
||||
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"
|
||||
|
gitea-actions
commented
🟡 randToken duplicates newSessionToken token-generation pattern maintainability · flagged by 1 model
🪰 Gadfly · advisory 🟡 **randToken duplicates newSessionToken token-generation pattern**
_maintainability · flagged by 1 model_
- **`internal/api/oidc.go:265` — `randToken` duplicates existing token-generation pattern** `randToken` is a near-exact clone of `newSessionToken` in `internal/service/service.go:62`: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper in `service` or a new `internal/util` package) so entropy/size requirements don't drift between the two call sites.
<sub>🪰 Gadfly · advisory</sub>
|
||||
case errors.Is(err, domain.ErrOIDCIdentityConflict):
|
||||
return "oidc_conflict"
|
||||
default:
|
||||
return "oidc"
|
||||
}
|
||||
}
|
||||
|
||||
// setOIDCTxCookie writes the login transaction as a compact base64 JSON cookie.
|
||||
// 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 {
|
||||
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) {
|
||||
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
|
||||
// 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
|
||||
}
|
||||
|
||||
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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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"),
|
||||
}
|
||||
|
||||
@@ -35,6 +35,19 @@ 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 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.
|
||||
|
||||
@@ -30,17 +30,90 @@ 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 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.
|
||||
//
|
||||
// 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
|
||||
|
gitea-actions
commented
🟡 TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding error-handling · flagged by 1 model
🪰 Gadfly · advisory 🟡 **TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding**
_error-handling · flagged by 1 model_
- `internal/service/auth.go:67-109` (`LoginOIDC`) — the returning-user-vs-JIT-create decision is a check-then-act race: `GetUserByOIDC` (a `SELECT`, line 73) and `CreateUser` (an `INSERT`, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new `(issuer, subject)` both miss the lookup and both call `CreateUser`; the loser trips the `idx_users_oidc` UNIQUE index (`internal/store/migrations/0001_init.sql:27`), which `CreateUser` maps to `domain.ErrEmailTak…
<sub>🪰 Gadfly · advisory</sub>
|
||||
// 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 (identity already proven; email state irrelevant).
|
||||
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
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
|
||||
|
gitea-actions
commented
🔴 LinkOIDC silently overwrites existing OIDC identity on email-linked user correctness · flagged by 2 models
🪰 Gadfly · advisory 🔴 **LinkOIDC silently overwrites existing OIDC identity on email-linked user**
_correctness · flagged by 2 models_
* **`internal/service/auth.go:93` — `LinkOIDC` silently overwrites an existing OIDC identity** `LoginOIDC` resolves a user by email and unconditionally calls `LinkOIDC`, which executes an `UPDATE` that stomps any existing `oidc_issuer` / `oidc_subject` on that row. The comment on `LinkOIDC` says it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issue…
<sub>🪰 Gadfly · advisory</sub>
|
||||
// 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:
|
||||
return s.store.LinkOIDC(ctx, existing.ID, id.Issuer, id.Subject)
|
||||
case !errors.Is(err, domain.ErrNotFound):
|
||||
return nil, err
|
||||
}
|
||||
|
||||
|
gitea-actions
commented
🔴 LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking. error-handling, security · flagged by 2 models
🪰 Gadfly · advisory 🔴 **LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking.**
_error-handling, security · flagged by 2 models_
- **`internal/service/auth.go:103` — `LoginOIDC` JIT-creates an account (which may become the first/admin user) when `email_verified` is false and the email doesn't collide.** The email-collision branch (lines 88-96) correctly requires `id.EmailVerified` before linking, but the JIT-creation fallthrough (lines 98-108) does not. With `PANSY_LOCAL_AUTH=false`, an identity whose unverified email matches no existing account is provisioned, and `store.CreateUser` makes it admin iff the table is empty…
<sub>🪰 Gadfly · advisory</sub>
|
||||
// 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 +281,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
|
||||
}
|
||||
|
||||
@@ -280,15 +280,148 @@ 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: testOIDCIssuer, ClientID: "cid", ButtonLabel: "Sign in with Authentik"}
|
||||
|
gitea-actions
commented
⚪ Duplicated issuer literal bypasses the oidcIdentity test helper maintainability · flagged by 1 model
🪰 Gadfly · advisory ⚪ **Duplicated issuer literal bypasses the oidcIdentity test helper**
_maintainability · flagged by 1 model_
- **Duplicated issuer string `"https://idp.example"` in `TestProvidersReflectsConfig`** (`internal/service/auth_test.go:293,300`) — the same constant is also hardcoded inside the `oidcIdentity` helper (line 307). The helper exists to centralize this; the two extra inlines bypass it.
<sub>🪰 Gadfly · advisory</sub>
|
||||
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")
|
||||
}
|
||||
}
|
||||
|
||||
// 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: testOIDCIssuer, 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", "[email protected]", "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", "[email protected]", "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, "[email protected]", "Bob", "password123")
|
||||
|
||||
linked, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-2", "[email protected]", "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(), "[email protected]", "password123"); err != nil {
|
||||
t.Errorf("local login after linking failed: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoginOIDCRefusesUnverifiedEmail(t *testing.T) {
|
||||
s := newTestService(t, openConfig())
|
||||
|
||||
// Unverified email colliding with an existing account is refused (takeover).
|
||||
mustRegister(t, s, "[email protected]", "Carol", "password123")
|
||||
if _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3", "[email protected]", "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", "[email protected]", "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", "[email protected]", "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", "[email protected]", "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", "[email protected]", "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) {
|
||||
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", "[email protected]", "", 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", "[email protected]", "Erin", true)); err != nil {
|
||||
t.Errorf("LoginOIDC with local auth disabled: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -105,6 +105,54 @@ 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.
|
||||
//
|
||||
// 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
|
||||
|
gitea-actions
commented
🟠 LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error. error-handling, performance · flagged by 5 models
🪰 Gadfly · advisory 🟠 **LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error.**
_error-handling, performance · flagged by 5 models_
- **`internal/store/users.go:127` — `LinkOIDC` ignores `RowsAffected() == 0`.** `ExecContext` (lines 128-135) returns no error and `RowsAffected()` is never checked; the function then calls `GetUserByID(ctx, userID)` (line 142), which returns `domain.ErrNotFound` for a non-existent userID. A `LinkOIDC` against a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: check `RowsAffected()` and return a…
<sub>🪰 Gadfly · advisory</sub>
|
||||
// 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) {
|
||||
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')
|
||||
|
gitea-actions
commented
🟡 LinkOIDC returns ErrEmailTaken for OIDC identity collision maintainability · flagged by 4 models
🪰 Gadfly · advisory 🟡 **LinkOIDC returns ErrEmailTaken for OIDC identity collision**
_maintainability · flagged by 4 models_
- **`internal/store/users.go:137` — `LinkOIDC` returns semantically wrong error on OIDC identity collision** A UNIQUE violation on `(oidc_issuer, oidc_subject)` is mapped to `domain.ErrEmailTaken`, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicated `ErrOIDCIdentityTaken` (or similar) would make the code self-documenting and prevent future confusion i…
<sub>🪰 Gadfly · advisory</sub>
|
||||
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.ErrOIDCIdentityConflict
|
||||
}
|
||||
return nil, fmt.Errorf("store: link oidc: %w", err)
|
||||
}
|
||||
return u, nil
|
||||
}
|
||||
|
||||
// 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) {
|
||||
|
||||
🟠 Discovery mutex serializes all OIDC login/callback requests behind a full 10s timeout during IdP outages, with no negative caching/backoff
performance · flagged by 2 models
internal/api/oidc.go:63-76—oidcClient.ensureholdso.mufor the entire duration ofoidc.NewProvider, a network call bounded tooidcDiscoveryTimeout(10s). Concurrent/oidc/loginand/oidc/callbackrequests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: theif o.provider != nil { return nil }fast-path (line 66) only…🪰 Gadfly · advisory