From 913534ab19cc6a7baf09c8a9b02eb824bf98609d Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Fri, 5 Jun 2026 08:18:23 +0200 Subject: [PATCH] refactor(auth): stateless OAuth2 state via securecookie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops the in-memory state map, the RWMutex protecting it and the cleanup goroutine. The OAuth2 `state` parameter is now a self-contained signed blob — `securecookie.Encode` packs (returnURL, expiry) into the string we hand to the IdP, `Decode` verifies the signature and pulls them back out on callback. No server-side bookkeeping. Removed: - states map[string]*StateData + statesMu sync.RWMutex - cleanupStates goroutine + stateCleanupEvery constant - StateData struct (replaced by tiny private stateBlob) - crypto/rand + encoding/base64 + sync imports Added: - github.com/gorilla/securecookie (battle-tested; the lib handles all the HMAC + AES details and the MaxAge bookkeeping) Trade-offs vs the in-memory map: - Pod restart: same behaviour. Both the in-memory map AND the process-random securecookie keys are lost on restart, so in-flight logins fail closed either way. Stable keys (env or k8s secret) is a one-line follow-up that would also let multiple replicas share state. - Replay within TTL: theoretically possible with the new code (no nonce store to detect reuse), where the old code deleted on first use. Mitigated by the IdP's own one-time-code semantics and the 10-min TTL. Acceptable for the threat model; can add a nonce later if needed. Stats: - oidc.go: 252 -> 234 lines (-18) - Combined with phases A+B: 655 -> 319 (-336, -51% of pre-refactor auth code) --- go.mod | 1 + go.sum | 2 + internal/auth/oidc.go | 100 +++++++++++++++++------------------------- 3 files changed, 44 insertions(+), 59 deletions(-) diff --git a/go.mod b/go.mod index e88296d..b9f2c56 100644 --- a/go.mod +++ b/go.mod @@ -52,6 +52,7 @@ require ( github.com/go-openapi/swag/typeutils v0.25.4 // indirect github.com/go-openapi/swag/yamlutils v0.25.4 // indirect github.com/google/gnostic-models v0.7.0 // indirect + github.com/gorilla/securecookie v1.1.2 // indirect github.com/huandu/xstrings v1.5.0 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/klauspost/compress v1.18.0 // indirect diff --git a/go.sum b/go.sum index f984147..dd6462e 100644 --- a/go.sum +++ b/go.sum @@ -98,6 +98,8 @@ github.com/google/pprof v0.0.0-20250403155104-27863c87afa6 h1:BHT72Gu3keYf3ZEu2J github.com/google/pprof v0.0.0-20250403155104-27863c87afa6/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA= +github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo= github.com/huandu/xstrings v1.5.0 h1:2ag3IFq9ZDANvthTwTiqSSZLjDc+BedvHPAp5tJy2TI= github.com/huandu/xstrings v1.5.0/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 93da04c..9efdae0 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -2,27 +2,26 @@ package auth import ( "context" - "crypto/rand" - "encoding/base64" "fmt" "net/url" "strings" - "sync" "time" "github.com/AYDEV-FR/dploy/internal/config" "github.com/AYDEV-FR/dploy/internal/logger" "github.com/coreos/go-oidc/v3/oidc" "github.com/gofiber/fiber/v2" + "github.com/gorilla/securecookie" "golang.org/x/oauth2" ) -// StateData holds a one-time state token's expiry + the URL to redirect back -// to after a successful callback. In-memory map keyed by state; lost on pod -// restart (in-flight logins fail back to /auth/login). -type StateData struct { - Expiry time.Time +// stateBlob is what we encode + sign into the OAuth2 `state` parameter. +// Carrying the data inside the signed token keeps things stateless — no +// server-side map, no cleanup goroutine — so the only failure mode left +// is "browser took longer than stateTTL to come back", which is intended. +type stateBlob struct { ReturnURL string + Expiry int64 // unix seconds } // OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the @@ -33,14 +32,11 @@ type StateData struct { type OIDCHandler struct { config *config.Config oauth2Config *oauth2.Config - - statesMu sync.RWMutex - states map[string]*StateData + sc *securecookie.SecureCookie // signs+encodes the OAuth2 state blob } const ( stateTTL = 10 * time.Minute - stateCleanupEvery = 5 * time.Minute discoveryTimeout = 10 * time.Second discoveryAttempts = 5 ) @@ -78,7 +74,13 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { "internal", internalBase, "public", publicBase, "authURL", authURL) } - h := &OIDCHandler{ + // Keys are random per process: an in-flight login that straddles a pod + // restart fails closed (same as the in-memory map this replaces). Stable + // keys via env/secret would survive restarts — easy follow-up if needed. + sc := securecookie.New(securecookie.GenerateRandomKey(64), securecookie.GenerateRandomKey(32)) + sc.MaxAge(int(stateTTL.Seconds())) + + return &OIDCHandler{ config: cfg, oauth2Config: &oauth2.Config{ ClientID: cfg.OIDCClientID, @@ -86,14 +88,12 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { RedirectURL: cfg.OIDCRedirectURL, Scopes: []string{oidc.ScopeOpenID, "email", "profile"}, Endpoint: oauth2.Endpoint{ - AuthURL: authURL, // public — browser redirects here - TokenURL: endpoint.TokenURL, // internal — backend POSTs here + AuthURL: authURL, // public — browser redirects here + TokenURL: endpoint.TokenURL, // internal — backend POSTs here }, }, - states: make(map[string]*StateData), - } - go h.cleanupStates() - return h, nil + sc: sc, + }, nil } // discoverWithRetry rides out the post-startup network-identity window @@ -137,51 +137,30 @@ func extractBaseURL(rawURL string) string { return fmt.Sprintf("%s://%s", parsed.Scheme, parsed.Host) } -// generateState mints a fresh 32-byte CSRF token and stashes the returnURL -// alongside it for the callback to pick up. -func (h *OIDCHandler) generateState(returnURL string) string { - b := make([]byte, 32) - if _, err := rand.Read(b); err != nil { - // crypto/rand failure is exceedingly rare; fall back to a time-based - // state rather than panicking. Lower entropy but still single-use. - b = []byte(fmt.Sprintf("dploy-state-%d", time.Now().UnixNano())) - } - state := base64.URLEncoding.EncodeToString(b) - h.statesMu.Lock() - h.states[state] = &StateData{Expiry: time.Now().Add(stateTTL), ReturnURL: returnURL} - h.statesMu.Unlock() - return state +// generateState signs the (returnURL, expiry) blob into a self-contained +// OAuth2 state parameter. The signing key is process-random — an attacker +// can't forge a valid blob, so the CSRF guarantee holds without any +// server-side bookkeeping. +func (h *OIDCHandler) generateState(returnURL string) (string, error) { + return h.sc.Encode("dploy-state", stateBlob{ + ReturnURL: returnURL, + Expiry: time.Now().Add(stateTTL).Unix(), + }) } -// consumeState looks up + deletes the state in one critical section. -// Returns (data, true) on a valid first-use, (nil, false) on miss or expiry. -func (h *OIDCHandler) consumeState(state string) (*StateData, bool) { - h.statesMu.Lock() - defer h.statesMu.Unlock() - data, ok := h.states[state] - if !ok { +// consumeState verifies the signature, decodes the blob and checks the +// embedded expiry. Replay within the TTL is theoretically possible (no +// nonce store), but mitigated by the IdP's own one-time-code semantics +// and the short TTL — acceptable for the threat model. +func (h *OIDCHandler) consumeState(state string) (*stateBlob, bool) { + var blob stateBlob + if err := h.sc.Decode("dploy-state", state, &blob); err != nil { return nil, false } - delete(h.states, state) - if time.Now().After(data.Expiry) { + if time.Now().Unix() > blob.Expiry { return nil, false } - return data, true -} - -func (h *OIDCHandler) cleanupStates() { - ticker := time.NewTicker(stateCleanupEvery) - defer ticker.Stop() - for range ticker.C { - h.statesMu.Lock() - now := time.Now() - for state, data := range h.states { - if now.After(data.Expiry) { - delete(h.states, state) - } - } - h.statesMu.Unlock() - } + return &blob, true } // Login initiates the Authorization Code flow. @@ -192,7 +171,10 @@ func (h *OIDCHandler) Login(c *fiber.Ctx) error { logger.Warn("OIDC login: invalid returnUrl, defaulting to /", "returnUrl", returnURL) returnURL = "/" } - state := h.generateState(returnURL) + state, err := h.generateState(returnURL) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"}) + } logger.Debug("OIDC login redirect", "returnUrl", returnURL) return c.Redirect(h.oauth2Config.AuthCodeURL(state), fiber.StatusFound) }