Skip to content

Commit 913534a

Browse files
committed
refactor(auth): stateless OAuth2 state via securecookie
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)
1 parent 11f5a11 commit 913534a

3 files changed

Lines changed: 44 additions & 59 deletions

File tree

go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ require (
5252
github.com/go-openapi/swag/typeutils v0.25.4 // indirect
5353
github.com/go-openapi/swag/yamlutils v0.25.4 // indirect
5454
github.com/google/gnostic-models v0.7.0 // indirect
55+
github.com/gorilla/securecookie v1.1.2 // indirect
5556
github.com/huandu/xstrings v1.5.0 // indirect
5657
github.com/json-iterator/go v1.1.12 // indirect
5758
github.com/klauspost/compress v1.18.0 // indirect

go.sum

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ github.com/google/pprof v0.0.0-20250403155104-27863c87afa6 h1:BHT72Gu3keYf3ZEu2J
9898
github.com/google/pprof v0.0.0-20250403155104-27863c87afa6/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA=
9999
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
100100
github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
101+
github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA=
102+
github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo=
101103
github.com/huandu/xstrings v1.5.0 h1:2ag3IFq9ZDANvthTwTiqSSZLjDc+BedvHPAp5tJy2TI=
102104
github.com/huandu/xstrings v1.5.0/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE=
103105
github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y=

internal/auth/oidc.go

Lines changed: 41 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,26 @@ package auth
22

33
import (
44
"context"
5-
"crypto/rand"
6-
"encoding/base64"
75
"fmt"
86
"net/url"
97
"strings"
10-
"sync"
118
"time"
129

1310
"github.com/AYDEV-FR/dploy/internal/config"
1411
"github.com/AYDEV-FR/dploy/internal/logger"
1512
"github.com/coreos/go-oidc/v3/oidc"
1613
"github.com/gofiber/fiber/v2"
14+
"github.com/gorilla/securecookie"
1715
"golang.org/x/oauth2"
1816
)
1917

20-
// StateData holds a one-time state token's expiry + the URL to redirect back
21-
// to after a successful callback. In-memory map keyed by state; lost on pod
22-
// restart (in-flight logins fail back to /auth/login).
23-
type StateData struct {
24-
Expiry time.Time
18+
// stateBlob is what we encode + sign into the OAuth2 `state` parameter.
19+
// Carrying the data inside the signed token keeps things stateless — no
20+
// server-side map, no cleanup goroutine — so the only failure mode left
21+
// is "browser took longer than stateTTL to come back", which is intended.
22+
type stateBlob struct {
2523
ReturnURL string
24+
Expiry int64 // unix seconds
2625
}
2726

2827
// OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the
@@ -33,14 +32,11 @@ type StateData struct {
3332
type OIDCHandler struct {
3433
config *config.Config
3534
oauth2Config *oauth2.Config
36-
37-
statesMu sync.RWMutex
38-
states map[string]*StateData
35+
sc *securecookie.SecureCookie // signs+encodes the OAuth2 state blob
3936
}
4037

4138
const (
4239
stateTTL = 10 * time.Minute
43-
stateCleanupEvery = 5 * time.Minute
4440
discoveryTimeout = 10 * time.Second
4541
discoveryAttempts = 5
4642
)
@@ -78,22 +74,26 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) {
7874
"internal", internalBase, "public", publicBase, "authURL", authURL)
7975
}
8076

81-
h := &OIDCHandler{
77+
// Keys are random per process: an in-flight login that straddles a pod
78+
// restart fails closed (same as the in-memory map this replaces). Stable
79+
// keys via env/secret would survive restarts — easy follow-up if needed.
80+
sc := securecookie.New(securecookie.GenerateRandomKey(64), securecookie.GenerateRandomKey(32))
81+
sc.MaxAge(int(stateTTL.Seconds()))
82+
83+
return &OIDCHandler{
8284
config: cfg,
8385
oauth2Config: &oauth2.Config{
8486
ClientID: cfg.OIDCClientID,
8587
ClientSecret: cfg.OIDCClientSecret,
8688
RedirectURL: cfg.OIDCRedirectURL,
8789
Scopes: []string{oidc.ScopeOpenID, "email", "profile"},
8890
Endpoint: oauth2.Endpoint{
89-
AuthURL: authURL, // public — browser redirects here
90-
TokenURL: endpoint.TokenURL, // internal — backend POSTs here
91+
AuthURL: authURL, // public — browser redirects here
92+
TokenURL: endpoint.TokenURL, // internal — backend POSTs here
9193
},
9294
},
93-
states: make(map[string]*StateData),
94-
}
95-
go h.cleanupStates()
96-
return h, nil
95+
sc: sc,
96+
}, nil
9797
}
9898

9999
// discoverWithRetry rides out the post-startup network-identity window
@@ -137,51 +137,30 @@ func extractBaseURL(rawURL string) string {
137137
return fmt.Sprintf("%s://%s", parsed.Scheme, parsed.Host)
138138
}
139139

140-
// generateState mints a fresh 32-byte CSRF token and stashes the returnURL
141-
// alongside it for the callback to pick up.
142-
func (h *OIDCHandler) generateState(returnURL string) string {
143-
b := make([]byte, 32)
144-
if _, err := rand.Read(b); err != nil {
145-
// crypto/rand failure is exceedingly rare; fall back to a time-based
146-
// state rather than panicking. Lower entropy but still single-use.
147-
b = []byte(fmt.Sprintf("dploy-state-%d", time.Now().UnixNano()))
148-
}
149-
state := base64.URLEncoding.EncodeToString(b)
150-
h.statesMu.Lock()
151-
h.states[state] = &StateData{Expiry: time.Now().Add(stateTTL), ReturnURL: returnURL}
152-
h.statesMu.Unlock()
153-
return state
140+
// generateState signs the (returnURL, expiry) blob into a self-contained
141+
// OAuth2 state parameter. The signing key is process-random — an attacker
142+
// can't forge a valid blob, so the CSRF guarantee holds without any
143+
// server-side bookkeeping.
144+
func (h *OIDCHandler) generateState(returnURL string) (string, error) {
145+
return h.sc.Encode("dploy-state", stateBlob{
146+
ReturnURL: returnURL,
147+
Expiry: time.Now().Add(stateTTL).Unix(),
148+
})
154149
}
155150

156-
// consumeState looks up + deletes the state in one critical section.
157-
// Returns (data, true) on a valid first-use, (nil, false) on miss or expiry.
158-
func (h *OIDCHandler) consumeState(state string) (*StateData, bool) {
159-
h.statesMu.Lock()
160-
defer h.statesMu.Unlock()
161-
data, ok := h.states[state]
162-
if !ok {
151+
// consumeState verifies the signature, decodes the blob and checks the
152+
// embedded expiry. Replay within the TTL is theoretically possible (no
153+
// nonce store), but mitigated by the IdP's own one-time-code semantics
154+
// and the short TTL — acceptable for the threat model.
155+
func (h *OIDCHandler) consumeState(state string) (*stateBlob, bool) {
156+
var blob stateBlob
157+
if err := h.sc.Decode("dploy-state", state, &blob); err != nil {
163158
return nil, false
164159
}
165-
delete(h.states, state)
166-
if time.Now().After(data.Expiry) {
160+
if time.Now().Unix() > blob.Expiry {
167161
return nil, false
168162
}
169-
return data, true
170-
}
171-
172-
func (h *OIDCHandler) cleanupStates() {
173-
ticker := time.NewTicker(stateCleanupEvery)
174-
defer ticker.Stop()
175-
for range ticker.C {
176-
h.statesMu.Lock()
177-
now := time.Now()
178-
for state, data := range h.states {
179-
if now.After(data.Expiry) {
180-
delete(h.states, state)
181-
}
182-
}
183-
h.statesMu.Unlock()
184-
}
163+
return &blob, true
185164
}
186165

187166
// Login initiates the Authorization Code flow.
@@ -192,7 +171,10 @@ func (h *OIDCHandler) Login(c *fiber.Ctx) error {
192171
logger.Warn("OIDC login: invalid returnUrl, defaulting to /", "returnUrl", returnURL)
193172
returnURL = "/"
194173
}
195-
state := h.generateState(returnURL)
174+
state, err := h.generateState(returnURL)
175+
if err != nil {
176+
return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"})
177+
}
196178
logger.Debug("OIDC login redirect", "returnUrl", returnURL)
197179
return c.Redirect(h.oauth2Config.AuthCodeURL(state), fiber.StatusFound)
198180
}

0 commit comments

Comments
 (0)