From f25cb126ca82571b233bf5b84719e38fd6254428 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Fri, 5 Jun 2026 19:34:25 +0200 Subject: [PATCH 01/14] fix(auth): harden OIDC flow against CTF-grade probing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four targeted fixes layered on top of the phase-A/B/C refactor. Every one uses an existing library helper — nothing hand-rolled. 1. Open-redirect via protocol-relative returnUrl (HIGH) The previous check `strings.HasPrefix(returnURL, "/")` accepted `//evil.com/x`, which c.Redirect then turns into a cross-origin redirect through the browser's protocol-relative URL rules. Add safeRelativePath: leading "/" plus url.Parse-confirmed empty scheme + empty host + nil userinfo. Covers //, /\\, http://, https://, javascript:, data:, userinfo tricks. Applied in both Login (before we sign the state) and Callback (before we honour the unwrapped blob). Test matrix in oidc_test.go locks the cases. 2. OIDC nonce (MEDIUM) The flow had no nonce, so a stolen id_token issued for our client_id could be replayed through /auth/callback. Add a 32-byte crypto/rand nonce baked into the (signed) state blob, passed to the IdP via oidc.Nonce(n) on AuthCodeURL, and checked against idToken.Nonce on return. 3. Server-side id_token verification (MEDIUM) We used to hand the raw id_token straight to the browser, trusting that the request-time JWT validator would catch a bad signature on the *next* call. Now Verify() runs at callback time — signature, iss, aud, exp, nbf, plus at_hash binding the id_token to the access_token it came with. Forged / unsigned tokens never reach the browser. 4. PKCE S256 (LOW) Adds the OAuth 2.1-recommended PKCE leg even though we're a confidential client. The code verifier is baked into the signed state, the S256 challenge is sent in AuthCodeURL via oauth2.S256ChallengeOption, and the verifier is passed to Exchange via oauth2.VerifierOption. An attacker who snags the auth code on the redirect leg can no longer redeem it. No new dependencies — all four leverage the go-oidc / oauth2 we already pulled in phases A/B. Public surface unchanged. --- internal/auth/oidc.go | 129 ++++++++++++++++++++++++++++--------- internal/auth/oidc_test.go | 38 +++++++++++ 2 files changed, 138 insertions(+), 29 deletions(-) create mode 100644 internal/auth/oidc_test.go diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 9efdae0..34a4739 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -2,6 +2,8 @@ package auth import ( "context" + "crypto/rand" + "encoding/base64" "fmt" "net/url" "strings" @@ -19,9 +21,27 @@ import ( // 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. +// +// Nonce binds the returned id_token to this specific login attempt (OIDC +// replay defense); PKCEVerifier proves to the IdP that the client redeeming +// the code is the same one that started the flow (defense against an +// intercepted auth code). type stateBlob struct { - ReturnURL string - Expiry int64 // unix seconds + ReturnURL string + Nonce string + PKCEVerifier string + Expiry int64 // unix seconds +} + +// safeRelativePath enforces "/path[?...][#...]" — no scheme, no host, no +// protocol-relative "//host/..." trick. `url.Parse` does the heavy lifting, +// we just inspect the result. +func safeRelativePath(s string) bool { + if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") { + return false + } + u, err := url.Parse(s) + return err == nil && u.Scheme == "" && u.Host == "" && u.User == nil } // OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the @@ -32,6 +52,7 @@ type stateBlob struct { type OIDCHandler struct { config *config.Config oauth2Config *oauth2.Config + idVerifier *oidc.IDTokenVerifier // verifies id_tokens at /auth/callback sc *securecookie.SecureCookie // signs+encodes the OAuth2 state blob } @@ -80,6 +101,12 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { sc := securecookie.New(securecookie.GenerateRandomKey(64), securecookie.GenerateRandomKey(32)) sc.MaxAge(int(stateTTL.Seconds())) + // Reuse the provider's KeySet for callback-time id_token verification. + // Same JWKS + same expected issuer as the request-time JWT validator; + // catches forged / unsigned id_tokens here instead of trusting them all + // the way down to the first API call. + idVerifier := provider.Verifier(&oidc.Config{ClientID: cfg.OIDCClientID}) + return &OIDCHandler{ config: cfg, oauth2Config: &oauth2.Config{ @@ -92,7 +119,8 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { TokenURL: endpoint.TokenURL, // internal — backend POSTs here }, }, - sc: sc, + idVerifier: idVerifier, + sc: sc, }, nil } @@ -137,14 +165,16 @@ func extractBaseURL(rawURL string) string { return fmt.Sprintf("%s://%s", parsed.Scheme, parsed.Host) } -// 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) { +// generateState signs the full blob (returnURL, nonce, PKCE verifier, expiry) +// 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, nonce, pkceVerifier string) (string, error) { return h.sc.Encode("dploy-state", stateBlob{ - ReturnURL: returnURL, - Expiry: time.Now().Add(stateTTL).Unix(), + ReturnURL: returnURL, + Nonce: nonce, + PKCEVerifier: pkceVerifier, + Expiry: time.Now().Add(stateTTL).Unix(), }) } @@ -163,20 +193,47 @@ func (h *OIDCHandler) consumeState(state string) (*stateBlob, bool) { return &blob, true } -// Login initiates the Authorization Code flow. +// Login initiates the Authorization Code flow with state + nonce + PKCE. +// Defense layering: +// - state — CSRF + carries the (signed) returnURL across the flow +// - nonce — binds the returned id_token to *this* login attempt +// - PKCE S256 — proves at token-exchange time that we're the same client +// that initiated the flow (defense against an intercepted auth code) +// +// returnURL is hardened against the protocol-relative open redirect +// (`//evil.com/x` would otherwise sail past a naive HasPrefix("/") check). func (h *OIDCHandler) Login(c *fiber.Ctx) error { returnURL := c.Query("returnUrl", "/") - // Open-redirect guard: only relative paths are accepted. - if !strings.HasPrefix(returnURL, "/") { - logger.Warn("OIDC login: invalid returnUrl, defaulting to /", "returnUrl", returnURL) + if !safeRelativePath(returnURL) { + logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", returnURL) returnURL = "/" } - state, err := h.generateState(returnURL) + nonce, err := randomToken() + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint nonce"}) + } + pkceVerifier := oauth2.GenerateVerifier() + state, err := h.generateState(returnURL, nonce, pkceVerifier) if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"}) } + authURL := h.oauth2Config.AuthCodeURL(state, + oidc.Nonce(nonce), + oauth2.S256ChallengeOption(pkceVerifier), + ) logger.Debug("OIDC login redirect", "returnUrl", returnURL) - return c.Redirect(h.oauth2Config.AuthCodeURL(state), fiber.StatusFound) + return c.Redirect(authURL, fiber.StatusFound) +} + +// randomToken returns a 32-byte URL-safe random string suitable for use as +// an OIDC nonce. crypto/rand is the source of truth — fall over loudly if +// the kernel can't provide entropy rather than degrade to a guessable value. +func randomToken() (string, error) { + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { + return "", fmt.Errorf("crypto/rand: %w", err) + } + return base64.RawURLEncoding.EncodeToString(b), nil } // Callback exchanges the code for tokens and bounces the browser to the @@ -197,32 +254,46 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "invalid or expired state parameter"}) } - token, err := h.oauth2Config.Exchange(c.Context(), code) + // Exchange the code with the PKCE verifier — the IdP rejects the call if + // it doesn't match the challenge we sent at Login. + token, err := h.oauth2Config.Exchange(c.Context(), code, oauth2.VerifierOption(stateData.PKCEVerifier)) if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ "error": fmt.Sprintf("failed to exchange code: %v", err), }) } - // OIDC flows put the id_token in Token.Extra; OAuth2-only IdPs only return - // an access_token. We prefer the id_token (it's what the JWT validator - // downstream expects), fall back to the access_token otherwise. - tokenToUse, _ := token.Extra("id_token").(string) - if tokenToUse == "" { - tokenToUse = token.AccessToken - } - if tokenToUse == "" { + rawIDToken, _ := token.Extra("id_token").(string) + if rawIDToken == "" { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ - "error": "no token received from OIDC provider", + "error": "no id_token in OIDC response", }) } + // Verify the id_token server-side (signature, iss, aud, exp, nbf, at_hash) + // before handing it to the browser. Catches forged / unsigned id_tokens + // here instead of trusting them as far as the next API call. + idToken, err := h.idVerifier.Verify(c.Context(), rawIDToken) + if err != nil { + logger.Warn("OIDC callback: id_token verification failed", "error", err.Error()) + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{ + "error": "id_token verification failed", + }) + } + // Replay defense: the nonce in the returned id_token must equal the one + // we baked into the (signed) state. Without this, an attacker who steals + // any valid id_token for our client_id can replay it through callback. + if idToken.Nonce != stateData.Nonce { + logger.Warn("OIDC callback: nonce mismatch — replay rejected") + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "nonce mismatch"}) + } + returnURL := "/" - if stateData != nil && strings.HasPrefix(stateData.ReturnURL, "/") { + if safeRelativePath(stateData.ReturnURL) { returnURL = stateData.ReturnURL } - logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(tokenToUse)) - return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, tokenToUse), fiber.StatusFound) + logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken)) + return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound) } // Logout currently just bounces the browser home — the SPA clears its diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go new file mode 100644 index 0000000..10bdf7f --- /dev/null +++ b/internal/auth/oidc_test.go @@ -0,0 +1,38 @@ +package auth + +import "testing" + +// TestSafeRelativePath covers the open-redirect surface a CTF participant is +// likely to probe through ?returnUrl=. Anything that isn't a plain +// "/path[?…][#…]" must be rejected before it reaches c.Redirect, where the +// browser would otherwise treat protocol-relative or backslash-prefixed +// inputs as cross-origin. +func TestSafeRelativePath(t *testing.T) { + cases := []struct { + in string + want bool + }{ + // happy paths + {"/", true}, + {"/foo", true}, + {"/foo/bar?x=1#frag", true}, + + // classic open-redirect tricks — all must be rejected + {"//evil.com/x", false}, // protocol-relative URL + {"/\\evil.com/x", false}, // backslash-prefixed (some browsers) + {"http://evil.com/x", false}, // absolute URL + {"https://evil.com/x", false}, // absolute URL https + {"javascript:alert(1)", false}, + {"data:text/html,x", false}, + {"//user@evil.com/x", false}, // userinfo trick + {"", false}, // empty + {"foo/bar", false}, // no leading slash + {" /foo", false}, // leading whitespace (Go parser doesn't strip it; we'd happily redirect) + } + for _, tc := range cases { + got := safeRelativePath(tc.in) + if got != tc.want { + t.Errorf("safeRelativePath(%q) = %v, want %v", tc.in, got, tc.want) + } + } +} From 8013320c20b501f592bc69b506c79a0a620f4325 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sun, 7 Jun 2026 18:24:25 +0200 Subject: [PATCH 02/14] fix(auth): address Copilot review on #39 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from the inline review: 1. at_hash binding was claimed but never executed The Verify() comment said it checks at_hash, but go-oidc/v3's Verify only does signature + iss + aud + exp + nbf. at_hash needs an explicit idToken.VerifyAccessToken(accessToken) call. Add it: when the IdP set the claim (Dex does), confirm the id_token was issued together with the access_token we received; skip with a warn when the claim is absent (the spec allows that). 2. Smuggled fragment in returnUrl broke the SPA hand-off safeRelativePath accepts "/foo#section" (the path is fine, the fragment is just metadata). On callback we then build "/foo#section#token=..." — two hashes, which the SPA's consumeHashToken() doesn't parse (it expects location.hash to start with "#token="). Strip the user-supplied fragment before appending the token hash. stripFragment helper + test cases pinning the behaviour. --- internal/auth/oidc.go | 32 +++++++++++++++++++++++++++++--- internal/auth/oidc_test.go | 17 +++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 34a4739..bc4acda 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -270,9 +270,9 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { }) } - // Verify the id_token server-side (signature, iss, aud, exp, nbf, at_hash) - // before handing it to the browser. Catches forged / unsigned id_tokens - // here instead of trusting them as far as the next API call. + // Verify the id_token server-side (signature, iss, aud, exp, nbf) before + // handing it to the browser. Catches forged / unsigned id_tokens here + // instead of trusting them as far as the next API call. idToken, err := h.idVerifier.Verify(c.Context(), rawIDToken) if err != nil { logger.Warn("OIDC callback: id_token verification failed", "error", err.Error()) @@ -287,15 +287,41 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { logger.Warn("OIDC callback: nonce mismatch — replay rejected") return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "nonce mismatch"}) } + // at_hash binding: when the IdP set it (Dex does for the code flow), + // confirm the id_token we just verified was issued together with this + // access_token. Defends against pairing a stolen access_token with a + // forged id_token. Optional in the spec, so we skip — with a warn — when + // the claim is absent. + if idToken.AccessTokenHash != "" { + if err := idToken.VerifyAccessToken(token.AccessToken); err != nil { + logger.Warn("OIDC callback: at_hash mismatch — token-pairing rejected", "error", err.Error()) + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "at_hash mismatch"}) + } + } else { + logger.Warn("OIDC callback: id_token has no at_hash, skipping access-token binding check") + } returnURL := "/" if safeRelativePath(stateData.ReturnURL) { returnURL = stateData.ReturnURL } + // Strip any fragment the user smuggled in via returnUrl (e.g. "/foo#section"): + // the SPA's consumeHashToken() expects "#token=..." to be the only hash + // on the final URL. Without this strip we'd produce "/foo#section#token=...", + // which the SPA fails to parse. + returnURL = stripFragment(returnURL) logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken)) return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound) } +// stripFragment returns the input URL without its #fragment, if any. Used at +// the callback to make sure the only hash on the final SPA redirect URL is +// the one carrying the token. +func stripFragment(s string) string { + before, _, _ := strings.Cut(s, "#") + return before +} + // Logout currently just bounces the browser home — the SPA clears its // localStorage token on this redirect. End-session at the IdP is opt-in // (the previous version didn't do it either; add an EndSessionEndpoint diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 10bdf7f..d229fbe 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -2,6 +2,23 @@ package auth import "testing" +func TestStripFragment(t *testing.T) { + for _, tc := range []struct { + in, want string + }{ + {"/foo", "/foo"}, + {"/foo?x=1", "/foo?x=1"}, + {"/foo#frag", "/foo"}, + {"/foo?x=1#frag", "/foo?x=1"}, + {"#frag", ""}, + {"", ""}, + } { + if got := stripFragment(tc.in); got != tc.want { + t.Errorf("stripFragment(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + // TestSafeRelativePath covers the open-redirect surface a CTF participant is // likely to probe through ?returnUrl=. Anything that isn't a plain // "/path[?…][#…]" must be rejected before it reaches c.Redirect, where the From d77606616937cf9e59c7c7eda5cdd0457fbd120e Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sun, 7 Jun 2026 18:37:03 +0200 Subject: [PATCH 03/14] refactor(auth): one net/url pass for returnUrl safety + canonicalisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the Copilot review. The previous fix had two helpers parsing the same URL twice: - safeRelativePath: url.Parse(s), reject if scheme/host/user set - stripFragment: strings.Cut(s, "#"), throw away the tail stripFragment was the embarrassing one — net/url already exposes url.URL.Fragment = "" + url.URL.String(), no string fiddling required. Collapse both helpers into a single sanitizeRelativePath(s) (string, bool) that returns the canonical fragment-stripped form alongside the safety verdict. One Parse, one set of checks, one rebuild. Test suite merges the two matrices into a single table — including fragment-stripping cases that the previous split missed (e.g. "/?x=1#frag" must come back as "/?x=1", not "" + stray strings.Cut output). --- internal/auth/oidc.go | 54 ++++++++++++++------------- internal/auth/oidc_test.go | 75 +++++++++++++++----------------------- 2 files changed, 59 insertions(+), 70 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index bc4acda..9f371af 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -33,15 +33,28 @@ type stateBlob struct { Expiry int64 // unix seconds } -// safeRelativePath enforces "/path[?...][#...]" — no scheme, no host, no -// protocol-relative "//host/..." trick. `url.Parse` does the heavy lifting, -// we just inspect the result. -func safeRelativePath(s string) bool { +// sanitizeRelativePath does two related jobs in one net/url pass: +// +// - validates that s is a safe relative URL — no scheme, no host, no +// userinfo, no protocol-relative ("//host/...") or backslash trick +// - returns the canonical form with any user-supplied #fragment dropped, +// because the SPA's consumeHashToken() expects the final redirect's +// hash to be exclusively "#token=..." (a leftover "/foo#section" would +// produce "/foo#section#token=..." which it can't parse) +// +// Both concerns are pure net/url plumbing — no string fiddling beyond the +// "//"/"/\\" prefix sniff that url.Parse can't catch (it parses them as +// scheme-less but still cross-origin). +func sanitizeRelativePath(s string) (string, bool) { if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") { - return false + return "", false } u, err := url.Parse(s) - return err == nil && u.Scheme == "" && u.Host == "" && u.User == nil + if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil { + return "", false + } + u.Fragment = "" + return u.String(), true } // OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the @@ -203,10 +216,11 @@ func (h *OIDCHandler) consumeState(state string) (*stateBlob, bool) { // returnURL is hardened against the protocol-relative open redirect // (`//evil.com/x` would otherwise sail past a naive HasPrefix("/") check). func (h *OIDCHandler) Login(c *fiber.Ctx) error { - returnURL := c.Query("returnUrl", "/") - if !safeRelativePath(returnURL) { - logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", returnURL) - returnURL = "/" + returnURL := "/" + if clean, ok := sanitizeRelativePath(c.Query("returnUrl", "/")); ok { + returnURL = clean + } else { + logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", c.Query("returnUrl")) } nonce, err := randomToken() if err != nil { @@ -301,27 +315,17 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { logger.Warn("OIDC callback: id_token has no at_hash, skipping access-token binding check") } + // Re-sanitize on the way out: the blob was signed by us, but defense in + // depth is cheap and this is the last chance to drop any fragment that + // would otherwise produce a "/foo#frag#token=..." URL the SPA can't read. returnURL := "/" - if safeRelativePath(stateData.ReturnURL) { - returnURL = stateData.ReturnURL + if clean, ok := sanitizeRelativePath(stateData.ReturnURL); ok { + returnURL = clean } - // Strip any fragment the user smuggled in via returnUrl (e.g. "/foo#section"): - // the SPA's consumeHashToken() expects "#token=..." to be the only hash - // on the final URL. Without this strip we'd produce "/foo#section#token=...", - // which the SPA fails to parse. - returnURL = stripFragment(returnURL) logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken)) return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound) } -// stripFragment returns the input URL without its #fragment, if any. Used at -// the callback to make sure the only hash on the final SPA redirect URL is -// the one carrying the token. -func stripFragment(s string) string { - before, _, _ := strings.Cut(s, "#") - return before -} - // Logout currently just bounces the browser home — the SPA clears its // localStorage token on this redirect. End-session at the IdP is opt-in // (the previous version didn't do it either; add an EndSessionEndpoint diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index d229fbe..7bfbd3e 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -2,54 +2,39 @@ package auth import "testing" -func TestStripFragment(t *testing.T) { +// TestSanitizeRelativePath pins both behaviours in one go: the relative-URL +// safety check (open-redirect surface) and the fragment-stripping canonical +// form the SPA's consumeHashToken() relies on. Both come out of one +// net/url.Parse pass — no string fiddling. +func TestSanitizeRelativePath(t *testing.T) { for _, tc := range []struct { - in, want string + in string + want string + wantOK bool }{ - {"/foo", "/foo"}, - {"/foo?x=1", "/foo?x=1"}, - {"/foo#frag", "/foo"}, - {"/foo?x=1#frag", "/foo?x=1"}, - {"#frag", ""}, - {"", ""}, - } { - if got := stripFragment(tc.in); got != tc.want { - t.Errorf("stripFragment(%q) = %q, want %q", tc.in, got, tc.want) - } - } -} + // happy paths — exact passthrough or fragment dropped + {"/", "/", true}, + {"/foo", "/foo", true}, + {"/foo?x=1", "/foo?x=1", true}, + {"/foo?x=1#frag", "/foo?x=1", true}, // fragment stripped + {"/foo#frag", "/foo", true}, + {"/#frag", "/", true}, -// TestSafeRelativePath covers the open-redirect surface a CTF participant is -// likely to probe through ?returnUrl=. Anything that isn't a plain -// "/path[?…][#…]" must be rejected before it reaches c.Redirect, where the -// browser would otherwise treat protocol-relative or backslash-prefixed -// inputs as cross-origin. -func TestSafeRelativePath(t *testing.T) { - cases := []struct { - in string - want bool - }{ - // happy paths - {"/", true}, - {"/foo", true}, - {"/foo/bar?x=1#frag", true}, - - // classic open-redirect tricks — all must be rejected - {"//evil.com/x", false}, // protocol-relative URL - {"/\\evil.com/x", false}, // backslash-prefixed (some browsers) - {"http://evil.com/x", false}, // absolute URL - {"https://evil.com/x", false}, // absolute URL https - {"javascript:alert(1)", false}, - {"data:text/html,x", false}, - {"//user@evil.com/x", false}, // userinfo trick - {"", false}, // empty - {"foo/bar", false}, // no leading slash - {" /foo", false}, // leading whitespace (Go parser doesn't strip it; we'd happily redirect) - } - for _, tc := range cases { - got := safeRelativePath(tc.in) - if got != tc.want { - t.Errorf("safeRelativePath(%q) = %v, want %v", tc.in, got, tc.want) + // classic open-redirect tricks — all rejected + {"//evil.com/x", "", false}, // protocol-relative URL + {"/\\evil.com/x", "", false}, // backslash-prefixed (some browsers) + {"http://evil.com/x", "", false}, // absolute URL + {"https://evil.com/x", "", false}, // absolute URL https + {"javascript:alert(1)", "", false}, + {"data:text/html,x", "", false}, + {"//user@evil.com/x", "", false}, // userinfo trick + {"", "", false}, // empty + {"foo/bar", "", false}, // no leading slash + {" /foo", "", false}, // leading whitespace + } { + got, ok := sanitizeRelativePath(tc.in) + if ok != tc.wantOK || got != tc.want { + t.Errorf("sanitizeRelativePath(%q) = (%q, %v), want (%q, %v)", tc.in, got, ok, tc.want, tc.wantOK) } } } From 840af77b0946798aa8bb14efa335a58986ac09b7 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sun, 7 Jun 2026 18:44:51 +0200 Subject: [PATCH 04/14] refactor(auth): delegate OIDC flow to zitadel/oidc RelyingParty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase D — the last chunk of hand-rolled auth code goes away. zitadel/oidc's RelyingParty owns the whole OAuth2/OIDC flow: discovery, state cookie (signed + encrypted), PKCE S256, nonce, code exchange, id_token verification (signature, iss, aud, exp, nbf, at_hash). Everything we implemented + hardened across phases A–C and #39 is now provided by the library, with a single import. What's left in oidc.go is the dploy-specific glue we still own: - split-horizon AuthURL substitution (in-cluster discovery, public-host browser redirect) - sanitizeRelativePath for returnUrl (open-redirect guard + #fragment strip) - retry-on-discovery at boot - Fiber adapter via fiber/middleware/adaptor - the "#token=..." hand-off the SPA expects What's gone: - Manual state map / cookie / encoding - Manual PKCE generation + Exchange-time VerifierOption wiring - Manual nonce generation + idToken.Nonce check - Manual idToken.VerifyAccessToken for at_hash - Manual id_token Verify call + error mapping - stateBlob struct, securecookie.SecureCookie setup, consumeState helper Stats: - oidc.go: 305 -> 184 lines (-121, -40%) - Whole auth package: 655 -> 269 lines (-386, -59% of pre-refactor) - Direct deps net: +zitadel/oidc/v3 - Direct deps gone: jwt.go's golang-jwt was already dropped in phase A --- go.mod | 14 +- go.sum | 33 ++++ internal/auth/oidc.go | 343 ++++++++++++------------------------------ 3 files changed, 141 insertions(+), 249 deletions(-) diff --git a/go.mod b/go.mod index b9f2c56..5adc3cf 100644 --- a/go.mod +++ b/go.mod @@ -10,8 +10,9 @@ require ( github.com/fluxcd/source-controller/api v1.8.4 github.com/gofiber/fiber/v2 v2.52.10 github.com/google/uuid v1.6.0 + github.com/gorilla/securecookie v1.1.2 + github.com/zitadel/oidc/v3 v3.47.5 go.uber.org/zap v1.27.1 - golang.org/x/oauth2 v0.36.0 k8s.io/api v0.36.1 k8s.io/apiextensions-apiserver v0.36.0 k8s.io/apimachinery v0.36.1 @@ -36,6 +37,7 @@ require ( github.com/fxamacker/cbor/v2 v2.9.0 // indirect github.com/go-jose/go-jose/v4 v4.1.4 // indirect github.com/go-logr/logr v1.4.3 // indirect + github.com/go-logr/stdr v1.2.2 // indirect github.com/go-logr/zapr v1.3.0 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect @@ -52,7 +54,6 @@ 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 @@ -63,6 +64,7 @@ require ( github.com/mitchellh/reflectwalk v1.0.2 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect + github.com/muhlemmer/gu v0.3.1 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/client_golang v1.23.2 // indirect @@ -71,17 +73,25 @@ require ( github.com/prometheus/procfs v0.19.2 // indirect github.com/rivo/uniseg v0.2.0 // indirect github.com/shopspring/decimal v1.4.0 // indirect + github.com/sirupsen/logrus v1.9.4 // indirect github.com/spf13/cast v1.7.0 // indirect github.com/spf13/pflag v1.0.10 // indirect github.com/valyala/bytebufferpool v1.0.0 // indirect github.com/valyala/fasthttp v1.51.0 // indirect github.com/valyala/tcplisten v1.0.0 // indirect github.com/x448/float16 v0.8.4 // indirect + github.com/zitadel/logging v0.7.0 // indirect + github.com/zitadel/schema v1.3.2 // indirect + go.opentelemetry.io/auto/sdk v1.2.1 // indirect + go.opentelemetry.io/otel v1.43.0 // indirect + go.opentelemetry.io/otel/metric v1.43.0 // indirect + go.opentelemetry.io/otel/trace v1.43.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.51.0 // indirect golang.org/x/net v0.55.0 // indirect + golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.45.0 // indirect golang.org/x/term v0.43.0 // indirect diff --git a/go.sum b/go.sum index dd6462e..25be5d2 100644 --- a/go.sum +++ b/go.sum @@ -10,6 +10,8 @@ github.com/andybalholm/brotli v1.1.0 h1:eLKJA0d02Lf0mVpIDgYnqXcUn0GqVmEFny3VuID1 github.com/andybalholm/brotli v1.1.0/go.mod h1:sms7XGricyQI9K10gOSf56VKKWS4oLer58Q+mhRPtnY= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= +github.com/bmatcuk/doublestar/v4 v4.10.0 h1:zU9WiOla1YA122oLM6i4EXvGW62DvKZVxIe6TYWexEs= +github.com/bmatcuk/doublestar/v4 v4.10.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/coreos/go-oidc/v3 v3.18.0 h1:V9orjXynvu5wiC9SemFTWnG4F45v403aIcjWo0d41+A= @@ -41,10 +43,15 @@ github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S github.com/fsnotify/fsnotify v1.9.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sapM= github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= +github.com/go-chi/chi/v5 v5.2.5 h1:Eg4myHZBjyvJmAFjFvWgrqDTXFyOzjj7YIm3L3mu6Ug= +github.com/go-chi/chi/v5 v5.2.5/go.mod h1:X7Gx4mteadT3eDOMTsXzmI4/rwUpOwBHLpAfupzFJP0= 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-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= +github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ= github.com/go-logr/zapr v1.3.0/go.mod h1:YKepepNBd1u/oyhd/yQmtjVXmm9uML4IXUgMOwR8/Gg= github.com/go-openapi/jsonpointer v0.19.6/go.mod h1:osyAmYz/mB/C3I+WsTTSgw1ONzaLJoLCyoi6/zppojs= @@ -102,6 +109,8 @@ github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kX 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/jeremija/gosubmit v0.2.8 h1:mmSITBz9JxVtu8eqbN+zmmwX7Ij2RidQxhcwRVI4wqA= +github.com/jeremija/gosubmit v0.2.8/go.mod h1:Ui+HS073lCFREXBbdfrJzMB57OI/bdxTiLtrDHHhFPI= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= @@ -134,6 +143,10 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFdJifH4BDsTlE89Zl93FEloxaWZfGcifgq8= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= +github.com/muhlemmer/gu v0.3.1 h1:7EAqmFrW7n3hETvuAdmFmn4hS8W+z3LgKtrnow+YzNM= +github.com/muhlemmer/gu v0.3.1/go.mod h1:YHtHR+gxM+bKEIIs7Hmi9sPT3ZDUvTN/i88wQpZkrdM= +github.com/muhlemmer/httpforwarded v0.1.0 h1:x4DLrzXdliq8mprgUMR0olDvHGkou5BJsK/vWUetyzY= +github.com/muhlemmer/httpforwarded v0.1.0/go.mod h1:yo9czKedo2pdZhoXe+yDkGVbU0TJ0q9oQ90BVoDEtw0= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/onsi/ginkgo/v2 v2.27.4 h1:fcEcQW/A++6aZAZQNUmNjvA9PSOzefMJBerHJ4t8v8Y= @@ -157,8 +170,12 @@ github.com/rivo/uniseg v0.2.0 h1:S1pD9weZBuJdFmowNwbpi7BJ8TNftyUImj/0WQi72jY= github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= +github.com/rs/cors v1.11.1 h1:eU3gRzXLRK57F5rKMGMZURNdIG4EoAmX8k94r9wXWHA= +github.com/rs/cors v1.11.1/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU= github.com/shopspring/decimal v1.4.0 h1:bxl37RwXBklmTi0C79JfXCEBD1cqqHt0bbgBAGFp81k= github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME= +github.com/sirupsen/logrus v1.9.4 h1:TsZE7l11zFCLZnZ+teH4Umoq5BhEIfIzfRDZ1Uzql2w= +github.com/sirupsen/logrus v1.9.4/go.mod h1:ftWc9WdOfJ0a92nsE2jF5u5ZwH8Bv2zdeOC42RjbV2g= github.com/spf13/cast v1.7.0 h1:ntdiHjuueXFgm5nzDRdOS4yfT43P5Fnud6DH50rz/7w= github.com/spf13/cast v1.7.0/go.mod h1:ancEpBxwJDODSW/UG4rDrAqiKolqNNh2DX3mk86cAdo= github.com/spf13/pflag v1.0.10 h1:4EBh2KAYBwaONj6b2Ye1GiHfwjqyROoF4RwYO+vPwFk= @@ -182,6 +199,20 @@ github.com/valyala/tcplisten v1.0.0 h1:rBHj/Xf+E1tRGZyWIWwJDiRY0zc1Js+CV5DqwacVS github.com/valyala/tcplisten v1.0.0/go.mod h1:T0xQ8SeCZGxckz9qRXTfG43PvQ/mcWh7FwZEA7Ioqkc= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= +github.com/zitadel/logging v0.7.0 h1:eugftwMM95Wgqwftsvj81isL0JK/hoScVqp/7iA2adQ= +github.com/zitadel/logging v0.7.0/go.mod h1:9A6h9feBF/3u0IhA4uffdzSDY7mBaf7RE78H5sFMINQ= +github.com/zitadel/oidc/v3 v3.47.5 h1:cR2z0oqa5XZkwpXQiPCUGqKtndrjHgEXb81y3oXocK4= +github.com/zitadel/oidc/v3 v3.47.5/go.mod h1:XxFh0666HRXycyrKmono+3gY0RACpYJLgy4r/+kliKY= +github.com/zitadel/schema v1.3.2 h1:gfJvt7dOMfTmxzhscZ9KkapKo3Nei3B6cAxjav+lyjI= +github.com/zitadel/schema v1.3.2/go.mod h1:IZmdfF9Wu62Zu6tJJTH3UsArevs3Y4smfJIj3L8fzxw= +go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= +go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= +go.opentelemetry.io/otel v1.43.0 h1:mYIM03dnh5zfN7HautFE4ieIig9amkNANT+xcVxAj9I= +go.opentelemetry.io/otel v1.43.0/go.mod h1:JuG+u74mvjvcm8vj8pI5XiHy1zDeoCS2LB1spIq7Ay0= +go.opentelemetry.io/otel/metric v1.43.0 h1:d7638QeInOnuwOONPp4JAOGfbCEpYb+K6DVWvdxGzgM= +go.opentelemetry.io/otel/metric v1.43.0/go.mod h1:RDnPtIxvqlgO8GRW18W6Z/4P462ldprJtfxHxyKd2PY= +go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= +go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= @@ -225,6 +256,8 @@ gopkg.in/evanphx/json-patch.v4 v4.13.0 h1:czT3CmqEaQ1aanPc5SdlgQrrEIb8w/wwCvWWnf gopkg.in/evanphx/json-patch.v4 v4.13.0/go.mod h1:p8EYWUEYMpynmqDbY58zCKCFZw8pRWMG4EsWvDvM72M= gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= +gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= +gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 9f371af..c9a6d83 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -1,165 +1,113 @@ +// Package auth handles JWT verification and the OIDC login flow for the dploy +// API. +// +// JWT verification (jwt.go) goes through the canonical go-oidc verifier; the +// OIDC login/callback/logout flow (this file) goes through zitadel/oidc's +// RelyingParty, which already handles state cookie, PKCE S256, nonce, +// at_hash, id_token signature/iss/aud/exp/nbf — all the gotchas that used to +// be hand-rolled here. What's left is dploy-specific glue: split-horizon +// AuthURL substitution, returnUrl sanitisation, retry-on-discovery at boot, +// the Fiber adapter, and the final "#token=..." hand-off the SPA expects. package auth import ( "context" - "crypto/rand" - "encoding/base64" "fmt" + "net/http" "net/url" "strings" "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/gofiber/fiber/v2/middleware/adaptor" "github.com/gorilla/securecookie" - "golang.org/x/oauth2" + "github.com/zitadel/oidc/v3/pkg/client/rp" + httphelper "github.com/zitadel/oidc/v3/pkg/http" + "github.com/zitadel/oidc/v3/pkg/oidc" ) -// 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. -// -// Nonce binds the returned id_token to this specific login attempt (OIDC -// replay defense); PKCEVerifier proves to the IdP that the client redeeming -// the code is the same one that started the flow (defense against an -// intercepted auth code). -type stateBlob struct { - ReturnURL string - Nonce string - PKCEVerifier string - Expiry int64 // unix seconds -} - -// sanitizeRelativePath does two related jobs in one net/url pass: -// -// - validates that s is a safe relative URL — no scheme, no host, no -// userinfo, no protocol-relative ("//host/...") or backslash trick -// - returns the canonical form with any user-supplied #fragment dropped, -// because the SPA's consumeHashToken() expects the final redirect's -// hash to be exclusively "#token=..." (a leftover "/foo#section" would -// produce "/foo#section#token=..." which it can't parse) -// -// Both concerns are pure net/url plumbing — no string fiddling beyond the -// "//"/"/\\" prefix sniff that url.Parse can't catch (it parses them as -// scheme-less but still cross-origin). -func sanitizeRelativePath(s string) (string, bool) { - if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") { - return "", false - } - u, err := url.Parse(s) - if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil { - return "", false - } - u.Fragment = "" - return u.String(), true -} - -// OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the -// configured IdP. Heavy lifting is delegated to golang.org/x/oauth2 (Code -// exchange, AuthCodeURL) and github.com/coreos/go-oidc (discovery). The only -// dploy-specific bits are split-horizon endpoint handling, a one-shot state -// map for CSRF protection, and the post-callback hash-fragment redirect. +// OIDCHandler is a thin wrapper around zitadel/oidc's RelyingParty plus the +// dploy-specific Fiber handlers. type OIDCHandler struct { - config *config.Config - oauth2Config *oauth2.Config - idVerifier *oidc.IDTokenVerifier // verifies id_tokens at /auth/callback - sc *securecookie.SecureCookie // signs+encodes the OAuth2 state blob + rp rp.RelyingParty } const ( - stateTTL = 10 * time.Minute discoveryTimeout = 10 * time.Second discoveryAttempts = 5 ) -// NewOIDCHandler discovers the IdP endpoints, wires an oauth2.Config and -// starts the state-cleanup goroutine. Returns an error only if discovery -// keeps failing after the retry budget — boot-time network races are the -// usual reason and 5 attempts with exponential backoff covers them in -// practice. +// NewOIDCHandler wires zitadel/oidc's RelyingParty (which handles state +// cookie, PKCE, nonce, at_hash + id_token verification) and substitutes the +// AuthorizationEndpoint with its public-issuer equivalent so browser redirects +// land on the user-facing IdP URL while backend code exchange + JWKS stay +// in-cluster. func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { - // Split-horizon: tokens carry the public issuer URL (what the browser - // sees), but we discover and call the IdP through the in-cluster URL. - // InsecureIssuerURLContext tells go-oidc which issuer to expect. - ctx := context.Background() - if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { - ctx = oidc.InsecureIssuerURLContext(ctx, cfg.OIDCPublicIssuer) - } + // Cookie security flag mirrors the redirect URL's scheme — HTTP for the + // dev/CTF cluster, HTTPS for production. Hash + block keys are + // process-random (in-flight logins fail closed on pod restart); promote + // to env/secret when running multiple replicas. + cookieOpts := []httphelper.CookieHandlerOpt{} + if strings.HasPrefix(cfg.OIDCRedirectURL, "http://") { + cookieOpts = append(cookieOpts, httphelper.WithUnsecure()) + } + cookieHandler := httphelper.NewCookieHandler( + securecookie.GenerateRandomKey(64), + securecookie.GenerateRandomKey(32), + cookieOpts..., + ) - provider, err := discoverWithRetry(ctx, cfg.OIDCIssuer) + relyingParty, err := newRelyingPartyWithRetry(context.Background(), + cfg.OIDCIssuer, cfg.OIDCClientID, cfg.OIDCClientSecret, cfg.OIDCRedirectURL, + []string{oidc.ScopeOpenID, "email", "profile"}, + rp.WithCookieHandler(cookieHandler), + rp.WithPKCE(cookieHandler), + ) if err != nil { - return nil, fmt.Errorf("failed to discover OIDC endpoints from %s: %w", cfg.OIDCIssuer, err) + return nil, fmt.Errorf("failed to build OIDC RelyingParty: %w", err) } - logger.Info("OIDC discovery completed", "issuer", cfg.OIDCIssuer) - // Browser redirects must hit the *public* authorization endpoint; backend - // code exchange stays on the *internal* token endpoint. Discovery gave us - // both with internal URLs — substitute the public base on AuthURL. - endpoint := provider.Endpoint() - authURL := endpoint.AuthURL + // Discovery served us in-cluster endpoint URLs (Dex emits them based on + // the request's Host header). Backend calls + JWKS stay internal; only + // the AuthorizationEndpoint needs to be rebased to public so browsers + // land on the user-facing IdP URL. if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) - authURL = strings.Replace(endpoint.AuthURL, internalBase, publicBase, 1) + ep := &relyingParty.OAuthConfig().Endpoint + ep.AuthURL = strings.Replace(ep.AuthURL, internalBase, publicBase, 1) logger.Info("OIDC auth endpoint rebased to public", - "internal", internalBase, "public", publicBase, "authURL", authURL) + "internal", internalBase, "public", publicBase, "authURL", ep.AuthURL) } - // 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())) - - // Reuse the provider's KeySet for callback-time id_token verification. - // Same JWKS + same expected issuer as the request-time JWT validator; - // catches forged / unsigned id_tokens here instead of trusting them all - // the way down to the first API call. - idVerifier := provider.Verifier(&oidc.Config{ClientID: cfg.OIDCClientID}) - - return &OIDCHandler{ - config: cfg, - oauth2Config: &oauth2.Config{ - ClientID: cfg.OIDCClientID, - ClientSecret: cfg.OIDCClientSecret, - 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 - }, - }, - idVerifier: idVerifier, - sc: sc, - }, nil + logger.Info("OIDC handler initialized", "issuer", cfg.OIDCIssuer) + return &OIDCHandler{rp: relyingParty}, nil } -// discoverWithRetry rides out the post-startup network-identity window +// newRelyingPartyWithRetry rides out the post-startup network-identity window // (Cilium et al.) where DNS / egress briefly returns EPERM. Same shape as the -// pre-refactor discoverOIDCWithRetry: 5 attempts, exponential backoff capped -// at 4 s. -func discoverWithRetry(ctx context.Context, issuer string) (*oidc.Provider, error) { +// previous discoverWithRetry: 5 attempts, exponential backoff capped at 4 s. +func newRelyingPartyWithRetry(ctx context.Context, issuer, clientID, clientSecret, redirectURI string, scopes []string, options ...rp.Option) (rp.RelyingParty, error) { delay := 500 * time.Millisecond var lastErr error for i := 1; i <= discoveryAttempts; i++ { attemptCtx, cancel := context.WithTimeout(ctx, discoveryTimeout) - provider, err := oidc.NewProvider(attemptCtx, issuer) + party, err := rp.NewRelyingPartyOIDC(attemptCtx, issuer, clientID, clientSecret, redirectURI, scopes, options...) cancel() if err == nil { if i > 1 { - logger.Info("OIDC discovery succeeded after retries", "issuer", issuer, "attempts", i) + logger.Info("OIDC discovery succeeded after retries", "attempts", i) } - return provider, nil + return party, nil } lastErr = err if i == discoveryAttempts { break } logger.Info("OIDC discovery attempt failed, retrying", - "issuer", issuer, "attempt", i, "nextDelay", delay, "error", err.Error()) + "attempt", i, "nextDelay", delay, "error", err.Error()) time.Sleep(delay) if delay < 4*time.Second { delay *= 2 @@ -168,8 +116,6 @@ func discoverWithRetry(ctx context.Context, issuer string) (*oidc.Provider, erro return nil, lastErr } -// extractBaseURL keeps only scheme + host (no path), so a public issuer with a -// trailing path component still substitutes cleanly. func extractBaseURL(rawURL string) string { parsed, err := url.Parse(rawURL) if err != nil { @@ -178,158 +124,61 @@ func extractBaseURL(rawURL string) string { return fmt.Sprintf("%s://%s", parsed.Scheme, parsed.Host) } -// generateState signs the full blob (returnURL, nonce, PKCE verifier, expiry) -// 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, nonce, pkceVerifier string) (string, error) { - return h.sc.Encode("dploy-state", stateBlob{ - ReturnURL: returnURL, - Nonce: nonce, - PKCEVerifier: pkceVerifier, - Expiry: time.Now().Add(stateTTL).Unix(), - }) -} - -// 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 +// sanitizeRelativePath validates that s is a safe relative URL (no scheme, +// host, userinfo, no protocol-relative or backslash trick) and returns the +// canonical form with any user-supplied #fragment dropped. The fragment-strip +// is required because the SPA's consumeHashToken() expects the final +// redirect's hash to be exclusively "#token=..." — a leftover "/foo#section" +// would otherwise produce "/foo#section#token=..." which can't be parsed. +func sanitizeRelativePath(s string) (string, bool) { + if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") { + return "", false } - if time.Now().Unix() > blob.Expiry { - return nil, false + u, err := url.Parse(s) + if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil { + return "", false } - return &blob, true + u.Fragment = "" + return u.String(), true } -// Login initiates the Authorization Code flow with state + nonce + PKCE. -// Defense layering: -// - state — CSRF + carries the (signed) returnURL across the flow -// - nonce — binds the returned id_token to *this* login attempt -// - PKCE S256 — proves at token-exchange time that we're the same client -// that initiated the flow (defense against an intercepted auth code) -// -// returnURL is hardened against the protocol-relative open redirect -// (`//evil.com/x` would otherwise sail past a naive HasPrefix("/") check). +// Login wraps zitadel/oidc's AuthURLHandler: it signs+encrypts the state into +// a cookie, hands it to the IdP as ?state=…, and gives it back to us after +// verifying. We use the sanitized returnUrl as the state value so it survives +// the round-trip without extra plumbing. func (h *OIDCHandler) Login(c *fiber.Ctx) error { + rawReturn := c.Query("returnUrl", "/") returnURL := "/" - if clean, ok := sanitizeRelativePath(c.Query("returnUrl", "/")); ok { + if clean, ok := sanitizeRelativePath(rawReturn); ok { returnURL = clean } else { - logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", c.Query("returnUrl")) - } - nonce, err := randomToken() - if err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint nonce"}) + logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", rawReturn) } - pkceVerifier := oauth2.GenerateVerifier() - state, err := h.generateState(returnURL, nonce, pkceVerifier) - if err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"}) - } - authURL := h.oauth2Config.AuthCodeURL(state, - oidc.Nonce(nonce), - oauth2.S256ChallengeOption(pkceVerifier), - ) - logger.Debug("OIDC login redirect", "returnUrl", returnURL) - return c.Redirect(authURL, fiber.StatusFound) + stateFn := func() string { return returnURL } + return adaptor.HTTPHandler(rp.AuthURLHandler(stateFn, h.rp))(c) } -// randomToken returns a 32-byte URL-safe random string suitable for use as -// an OIDC nonce. crypto/rand is the source of truth — fall over loudly if -// the kernel can't provide entropy rather than degrade to a guessable value. -func randomToken() (string, error) { - b := make([]byte, 32) - if _, err := rand.Read(b); err != nil { - return "", fmt.Errorf("crypto/rand: %w", err) - } - return base64.RawURLEncoding.EncodeToString(b), nil -} - -// Callback exchanges the code for tokens and bounces the browser to the -// stashed returnURL with the id_token in the URL hash (client-side only — -// never appears in server logs). +// Callback wraps zitadel/oidc's CodeExchangeHandler. The library does state +// cookie verification, PKCE-aware code exchange, and full id_token +// verification (signature, iss, aud, exp, nbf, nonce, at_hash). All we add is +// a defense-in-depth re-sanitisation of state-as-returnUrl and the SPA's +// hash-fragment token hand-off. func (h *OIDCHandler) Callback(c *fiber.Ctx) error { - if errorParam := c.Query("error"); errorParam != "" { - errorDesc := c.Query("error_description", errorParam) - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": errorDesc}) - } - code := c.Query("code") - state := c.Query("state") - if code == "" || state == "" { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing code or state parameter"}) - } - stateData, valid := h.consumeState(state) - if !valid { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "invalid or expired state parameter"}) - } - - // Exchange the code with the PKCE verifier — the IdP rejects the call if - // it doesn't match the challenge we sent at Login. - token, err := h.oauth2Config.Exchange(c.Context(), code, oauth2.VerifierOption(stateData.PKCEVerifier)) - if err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ - "error": fmt.Sprintf("failed to exchange code: %v", err), - }) - } - - rawIDToken, _ := token.Extra("id_token").(string) - if rawIDToken == "" { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ - "error": "no id_token in OIDC response", - }) - } - - // Verify the id_token server-side (signature, iss, aud, exp, nbf) before - // handing it to the browser. Catches forged / unsigned id_tokens here - // instead of trusting them as far as the next API call. - idToken, err := h.idVerifier.Verify(c.Context(), rawIDToken) - if err != nil { - logger.Warn("OIDC callback: id_token verification failed", "error", err.Error()) - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{ - "error": "id_token verification failed", - }) - } - // Replay defense: the nonce in the returned id_token must equal the one - // we baked into the (signed) state. Without this, an attacker who steals - // any valid id_token for our client_id can replay it through callback. - if idToken.Nonce != stateData.Nonce { - logger.Warn("OIDC callback: nonce mismatch — replay rejected") - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "nonce mismatch"}) - } - // at_hash binding: when the IdP set it (Dex does for the code flow), - // confirm the id_token we just verified was issued together with this - // access_token. Defends against pairing a stolen access_token with a - // forged id_token. Optional in the spec, so we skip — with a warn — when - // the claim is absent. - if idToken.AccessTokenHash != "" { - if err := idToken.VerifyAccessToken(token.AccessToken); err != nil { - logger.Warn("OIDC callback: at_hash mismatch — token-pairing rejected", "error", err.Error()) - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "at_hash mismatch"}) - } - } else { - logger.Warn("OIDC callback: id_token has no at_hash, skipping access-token binding check") - } + return adaptor.HTTPHandler(rp.CodeExchangeHandler(h.exchangeCallback, h.rp))(c) +} - // Re-sanitize on the way out: the blob was signed by us, but defense in - // depth is cheap and this is the last chance to drop any fragment that - // would otherwise produce a "/foo#frag#token=..." URL the SPA can't read. +func (h *OIDCHandler) exchangeCallback(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, _ rp.RelyingParty) { returnURL := "/" - if clean, ok := sanitizeRelativePath(stateData.ReturnURL); ok { + if clean, ok := sanitizeRelativePath(state); ok { returnURL = clean } - logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken)) - return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound) + logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(tokens.IDToken)) + http.Redirect(w, r, fmt.Sprintf("%s#token=%s", returnURL, tokens.IDToken), http.StatusFound) } -// Logout currently just bounces the browser home — the SPA clears its -// localStorage token on this redirect. End-session at the IdP is opt-in -// (the previous version didn't do it either; add an EndSessionEndpoint -// roundtrip here when an IdP demands SLO). +// Logout bounces the browser home — the SPA clears its localStorage token on +// the redirect. End-session at the IdP (RP-initiated logout) would be an +// rp.EndSessionEndpoint roundtrip; add it here when an IdP requires SLO. func (h *OIDCHandler) Logout(c *fiber.Ctx) error { return c.Redirect("/", fiber.StatusFound) } From bc52efc469e4cabd19ca1342931025b6b379632c Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sun, 7 Jun 2026 19:35:09 +0200 Subject: [PATCH 05/14] fix(auth): address second Copilot review on #39 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from the new inline comments. 1. CRITICAL — split-horizon broke after the zitadel/oidc switch zitadel/oidc validates that the discovery doc's `issuer` matches the one we passed to NewRelyingPartyOIDC. Dex always advertises its configured (public) issuer regardless of which URL the request came through, so the previous call — passing the *internal* issuer — would have failed at boot with ErrIssuerInvalid. Fix: pass the public issuer as `issuer` and use rp.WithCustomDiscoveryUrl(internalURL) to fetch discovery via the in-cluster endpoint. The AuthURL rebasing on OAuthConfig() still applies (endpoints are request-host-derived by Dex), but TokenURL + JWKS stay internal as intended. 2. HIGH — state = sanitized returnUrl was predictable For the vast majority of callers returnUrl is just "/", which means `state` was guessable. An attacker triggering /auth/login on a victim's browser then driving them to /auth/callback with the attacker's own code+state=/ would have bypassed CSRF / session-fixation protection: the cookie-state vs query-state check trivially matches when both are "/". Fix: state is now "<16-byte nonce>:". The nonce makes the state unguessable per login attempt; returnUrl piggybacks for free. The callback splits on the first ":" to recover the returnUrl (and re-sanitises it, defense in depth). 3. MEDIUM — process-random cookie keys broke multi-replica + rolling updates The previous code minted per-process hash/block keys, so a callback landing on a different pod than the one that started the login would fail to decrypt the state cookie. Fine for the current single-replica cluster, broken for any rolling update or replicaCount > 1. Fix: add OIDC_COOKIE_HASH_KEY / OIDC_COOKIE_BLOCK_KEY env vars (loaded via config), with a warn-and-random fallback when unset. Chart pipes them through the existing Secret, so values become `auth.oidcCookieHashKey` / `auth.oidcCookieBlockKey`. Documented in the values comment. --- charts/dploy/templates/secret.yaml | 10 ++++ charts/dploy/values.yaml | 8 +++ internal/auth/oidc.go | 78 ++++++++++++++++++++++-------- internal/config/config.go | 11 +++++ 4 files changed, 86 insertions(+), 21 deletions(-) diff --git a/charts/dploy/templates/secret.yaml b/charts/dploy/templates/secret.yaml index 8090b31..bdc4632 100644 --- a/charts/dploy/templates/secret.yaml +++ b/charts/dploy/templates/secret.yaml @@ -18,3 +18,13 @@ stringData: {{- if .Values.auth.oidcClientSecret }} OIDC_CLIENT_SECRET: {{ .Values.auth.oidcClientSecret | quote }} {{- end }} + {{/* Cookie keys for the zitadel/oidc state + PKCE cookies. When unset the + API generates per-process random keys (logs a warning) — fine for a + single replica; mandatory for replicaCount > 1 or rolling-update + resilience. Generate with `head -c 64 /dev/urandom | base64`. */}} + {{- if .Values.auth.oidcCookieHashKey }} + OIDC_COOKIE_HASH_KEY: {{ .Values.auth.oidcCookieHashKey | quote }} + {{- end }} + {{- if .Values.auth.oidcCookieBlockKey }} + OIDC_COOKIE_BLOCK_KEY: {{ .Values.auth.oidcCookieBlockKey | quote }} + {{- end }} diff --git a/charts/dploy/values.yaml b/charts/dploy/values.yaml index 36a1b4e..8820072 100644 --- a/charts/dploy/values.yaml +++ b/charts/dploy/values.yaml @@ -73,6 +73,14 @@ auth: oidcPublicIssuer: "" oidcRedirectURL: "" + # Cookie keys for the zitadel/oidc state + PKCE cookies. Leave empty for + # single-replica dev (API will mint per-process random keys + log a warn). + # MUST be set to stable secrets when running with replicaCount > 1 or + # tolerating rolling updates without breaking in-flight logins. + # Generate with: head -c 64 /dev/urandom | base64 + oidcCookieHashKey: "" + oidcCookieBlockKey: "" + service: type: ClusterIP port: 80 diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index c9a6d83..e321d75 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -12,6 +12,8 @@ package auth import ( "context" + "crypto/rand" + "encoding/base64" "fmt" "net/http" "net/url" @@ -46,33 +48,55 @@ const ( // in-cluster. func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { // Cookie security flag mirrors the redirect URL's scheme — HTTP for the - // dev/CTF cluster, HTTPS for production. Hash + block keys are - // process-random (in-flight logins fail closed on pod restart); promote - // to env/secret when running multiple replicas. + // dev/CTF cluster, HTTPS for production. cookieOpts := []httphelper.CookieHandlerOpt{} if strings.HasPrefix(cfg.OIDCRedirectURL, "http://") { cookieOpts = append(cookieOpts, httphelper.WithUnsecure()) } - cookieHandler := httphelper.NewCookieHandler( - securecookie.GenerateRandomKey(64), - securecookie.GenerateRandomKey(32), - cookieOpts..., - ) + // Cookie keys: prefer env-provided secrets so logins survive pod restarts + // and load-balance across replicas. Falling back to process-random is fine + // for single-replica dev but logs a loud warning. + hashKey := []byte(cfg.OIDCCookieHashKey) + blockKey := []byte(cfg.OIDCCookieBlockKey) + if len(hashKey) == 0 { + hashKey = securecookie.GenerateRandomKey(64) + logger.Warn("OIDC_COOKIE_HASH_KEY not set; using a process-random key — logins will break across pod restarts or replicas") + } + if len(blockKey) == 0 { + blockKey = securecookie.GenerateRandomKey(32) + } + cookieHandler := httphelper.NewCookieHandler(hashKey, blockKey, cookieOpts...) - relyingParty, err := newRelyingPartyWithRetry(context.Background(), - cfg.OIDCIssuer, cfg.OIDCClientID, cfg.OIDCClientSecret, cfg.OIDCRedirectURL, - []string{oidc.ScopeOpenID, "email", "profile"}, + // Split-horizon: tokens carry the public issuer URL (Dex's configured + // issuer is fixed regardless of request host), but we discover and call + // the IdP through the in-cluster URL. zitadel/oidc validates that the + // discovery doc's `issuer` matches the arg we pass, so we pass the + // public one and override the fetch URL via WithCustomDiscoveryUrl. + expectedIssuer := cfg.OIDCIssuer + internalDiscoveryURL := strings.TrimSuffix(cfg.OIDCIssuer, "/") + "/.well-known/openid-configuration" + opts := []rp.Option{ rp.WithCookieHandler(cookieHandler), rp.WithPKCE(cookieHandler), + } + if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { + expectedIssuer = cfg.OIDCPublicIssuer + opts = append(opts, rp.WithCustomDiscoveryUrl(internalDiscoveryURL)) + } + + relyingParty, err := newRelyingPartyWithRetry(context.Background(), + expectedIssuer, cfg.OIDCClientID, cfg.OIDCClientSecret, cfg.OIDCRedirectURL, + []string{oidc.ScopeOpenID, "email", "profile"}, + opts..., ) if err != nil { return nil, fmt.Errorf("failed to build OIDC RelyingParty: %w", err) } - // Discovery served us in-cluster endpoint URLs (Dex emits them based on - // the request's Host header). Backend calls + JWKS stay internal; only - // the AuthorizationEndpoint needs to be rebased to public so browsers - // land on the user-facing IdP URL. + // Endpoints from discovery are derived by Dex from the request Host + // header — fetching discovery via the in-cluster URL gives us internal + // endpoint URLs. Backend code exchange + JWKS stay internal; only the + // AuthorizationEndpoint must be rebased to public so browsers land on + // the user-facing IdP URL. if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) @@ -82,7 +106,7 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { "internal", internalBase, "public", publicBase, "authURL", ep.AuthURL) } - logger.Info("OIDC handler initialized", "issuer", cfg.OIDCIssuer) + logger.Info("OIDC handler initialized", "expectedIssuer", expectedIssuer) return &OIDCHandler{rp: relyingParty}, nil } @@ -144,8 +168,10 @@ func sanitizeRelativePath(s string) (string, bool) { // Login wraps zitadel/oidc's AuthURLHandler: it signs+encrypts the state into // a cookie, hands it to the IdP as ?state=…, and gives it back to us after -// verifying. We use the sanitized returnUrl as the state value so it survives -// the round-trip without extra plumbing. +// verifying. The state value is ":" — the nonce makes the +// state unguessable (defeats CSRF / session-fixation: an attacker who +// triggers /auth/login on a victim browser still can't predict the value +// stored in the cookie), while returnUrl piggybacks for free. func (h *OIDCHandler) Login(c *fiber.Ctx) error { rawReturn := c.Query("returnUrl", "/") returnURL := "/" @@ -154,7 +180,12 @@ func (h *OIDCHandler) Login(c *fiber.Ctx) error { } else { logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", rawReturn) } - stateFn := func() string { return returnURL } + nonceBytes := make([]byte, 16) + if _, err := rand.Read(nonceBytes); err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state nonce"}) + } + nonce := base64.RawURLEncoding.EncodeToString(nonceBytes) + stateFn := func() string { return nonce + ":" + returnURL } return adaptor.HTTPHandler(rp.AuthURLHandler(stateFn, h.rp))(c) } @@ -168,9 +199,14 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { } func (h *OIDCHandler) exchangeCallback(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, _ rp.RelyingParty) { + // state format set by Login: ":". The nonce is what + // zitadel/oidc already validated (cookie state == query state); we just + // need to recover the returnUrl. Anything else → default to "/". returnURL := "/" - if clean, ok := sanitizeRelativePath(state); ok { - returnURL = clean + if _, urlPart, ok := strings.Cut(state, ":"); ok { + if clean, ok := sanitizeRelativePath(urlPart); ok { + returnURL = clean + } } logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(tokens.IDToken)) http.Redirect(w, r, fmt.Sprintf("%s#token=%s", returnURL, tokens.IDToken), http.StatusFound) diff --git a/internal/config/config.go b/internal/config/config.go index 0434f20..6354703 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,6 +20,14 @@ type Config struct { OIDCClientSecret string OIDCRedirectURL string + // Cookie keys for the OIDC state / PKCE cookies (zitadel/oidc CookieHandler). + // Set both to stable random secrets (k8s Secret, env, etc.) when running + // with replicaCount > 1 or you'll get login failures on rolling updates, + // since requests can land on a pod that can't decrypt cookies set by + // another. Empty values fall back to process-random keys with a warn. + OIDCCookieHashKey string // 64 bytes recommended (HMAC key) + OIDCCookieBlockKey string // 32 bytes (AES key) + // Kubernetes: the namespace where DployTemplate and DployInstance CRs live. Namespace string @@ -69,6 +77,9 @@ func Load() (*Config, error) { OIDCClientSecret: getEnv("OIDC_CLIENT_SECRET", "dploy-secret"), OIDCRedirectURL: getEnv("OIDC_REDIRECT_URL", "http://localhost:8080/auth/callback"), + OIDCCookieHashKey: getEnv("OIDC_COOKIE_HASH_KEY", ""), + OIDCCookieBlockKey: getEnv("OIDC_COOKIE_BLOCK_KEY", ""), + // Kubernetes Namespace: getEnv("DPLOY_NAMESPACE", "dploy-system"), From 84738030457c36568d20519da26a973cf53ac9d0 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sun, 7 Jun 2026 21:53:30 +0200 Subject: [PATCH 06/14] docs(auth): mark the split-horizon path as opt-in The split-horizon issuer support (in-cluster discovery URL + public AuthURL rebase) was already gated on OIDCPublicIssuer being set and distinct from OIDCIssuer, but the code structure read like the default path. Reword the two comment blocks to say "this is optional, no-op when you use a single URL" and update the chart values.yaml so operators know oidcPublicIssuer is only for split-horizon and can be left empty for the common single-URL setup. No behaviour change. --- charts/dploy/values.yaml | 11 +++++++++++ internal/auth/oidc.go | 28 +++++++++++++++++----------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/charts/dploy/values.yaml b/charts/dploy/values.yaml index 8820072..695a5aa 100644 --- a/charts/dploy/values.yaml +++ b/charts/dploy/values.yaml @@ -69,8 +69,19 @@ auth: jwtUsernameClaim: preferred_username oidcClientID: dploy oidcClientSecret: "" + + # oidcIssuer is the URL the API uses for discovery, token exchange and + # JWKS — typically the in-cluster Service URL in a Kubernetes deployment. oidcIssuer: "" + + # oidcPublicIssuer is OPTIONAL — only set it for split-horizon deployments + # where the IdP is reached via two distinct URLs: an in-cluster one for + # backend traffic (oidcIssuer above) and a public one served via the + # ingress/gateway for browser redirects. When equal to oidcIssuer (or + # empty), the API uses a single URL for everything and the split-horizon + # code path is a no-op. oidcPublicIssuer: "" + oidcRedirectURL: "" # Cookie keys for the zitadel/oidc state + PKCE cookies. Leave empty for diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index e321d75..0841371 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -67,19 +67,24 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { } cookieHandler := httphelper.NewCookieHandler(hashKey, blockKey, cookieOpts...) - // Split-horizon: tokens carry the public issuer URL (Dex's configured - // issuer is fixed regardless of request host), but we discover and call - // the IdP through the in-cluster URL. zitadel/oidc validates that the - // discovery doc's `issuer` matches the arg we pass, so we pass the - // public one and override the fetch URL via WithCustomDiscoveryUrl. + // Optional split-horizon issuer support: when OIDCPublicIssuer is set and + // differs from OIDCIssuer, the IdP is reached via two URLs — the in-cluster + // one (low-latency for discovery/token/JWKS) and the public one (which is + // what tokens carry as `iss` and what browsers must redirect to). Most + // deployments expose the IdP on a single URL and can leave OIDCPublicIssuer + // empty; the block below + the AuthURL rebase further down are no-ops then. expectedIssuer := cfg.OIDCIssuer - internalDiscoveryURL := strings.TrimSuffix(cfg.OIDCIssuer, "/") + "/.well-known/openid-configuration" opts := []rp.Option{ rp.WithCookieHandler(cookieHandler), rp.WithPKCE(cookieHandler), } if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { + // zitadel/oidc validates discovery.Issuer == arg.issuer. Pass the + // public one (what Dex advertises) and override the fetch URL to the + // internal one via WithCustomDiscoveryUrl so the pod doesn't have to + // resolve the public host at boot. expectedIssuer = cfg.OIDCPublicIssuer + internalDiscoveryURL := strings.TrimSuffix(cfg.OIDCIssuer, "/") + "/.well-known/openid-configuration" opts = append(opts, rp.WithCustomDiscoveryUrl(internalDiscoveryURL)) } @@ -92,11 +97,12 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { return nil, fmt.Errorf("failed to build OIDC RelyingParty: %w", err) } - // Endpoints from discovery are derived by Dex from the request Host - // header — fetching discovery via the in-cluster URL gives us internal - // endpoint URLs. Backend code exchange + JWKS stay internal; only the - // AuthorizationEndpoint must be rebased to public so browsers land on - // the user-facing IdP URL. + // Second half of the optional split-horizon path. Dex derives the + // discovery doc's endpoint URLs from the request Host header, so fetching + // via the in-cluster URL gives us internal endpoints. Backend code + // exchange + JWKS stay internal (those are fine); only the + // AuthorizationEndpoint must be rebased to public so browsers actually + // land on the user-facing IdP URL. if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) From 66e2a7f25e19a2ce38a8252e926f694ec1488c9d Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sun, 7 Jun 2026 22:00:24 +0200 Subject: [PATCH 07/14] fix(auth): self-review fixes before re-running Copilot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things I caught re-reading the diff one more time: 1. AuthURL rebase was silently no-op when Dex's discovery returned an AuthURL whose base didn't match OIDCIssuer (config drift, port mismatch, etc.). strings.Replace just returned the input unchanged and we logged "rebased to public" anyway, so the browser would redirect to the in-cluster URL it can't reach. Now we Contains-check first and fail boot with a clear error. 2. Cookie key warn was inconsistent — only HASH absence triggered the "process-random, breaks across replicas" warning, BLOCK absence was silent. If you set HASH but forgot BLOCK, you'd ship to prod with one stable key and one random one. Symmetrical warn now. 3. The ":" state wire format had no test. Extracted decodeStateReturnURL (consumed by Callback, mirrors Login's encoding) and pinned it with 11 cases: empty / malformed / path-with-colon / fragment-strip / the three classic open-redirect tricks injected via the returnUrl half, plus a round-trip check that nonce never leaks into the redirect. --- internal/auth/oidc.go | 36 +++++++++++++++++++++------- internal/auth/oidc_test.go | 48 +++++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 0841371..8970a87 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -64,6 +64,7 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { } if len(blockKey) == 0 { blockKey = securecookie.GenerateRandomKey(32) + logger.Warn("OIDC_COOKIE_BLOCK_KEY not set; using a process-random key — logins will break across pod restarts or replicas") } cookieHandler := httphelper.NewCookieHandler(hashKey, blockKey, cookieOpts...) @@ -107,6 +108,13 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) ep := &relyingParty.OAuthConfig().Endpoint + if !strings.Contains(ep.AuthURL, internalBase) { + // Discovery returned an AuthURL we can't rebase (Dex emits a host + // that doesn't match OIDCIssuer — config drift). Bail loudly so + // the failure is visible at boot, not silently as "browser + // redirects to the in-cluster URL that it can't reach". + return nil, fmt.Errorf("OIDC AuthURL %q does not contain expected internal base %q; check OIDCIssuer / IdP issuer config", ep.AuthURL, internalBase) + } ep.AuthURL = strings.Replace(ep.AuthURL, internalBase, publicBase, 1) logger.Info("OIDC auth endpoint rebased to public", "internal", internalBase, "public", publicBase, "authURL", ep.AuthURL) @@ -205,19 +213,29 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { } func (h *OIDCHandler) exchangeCallback(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, _ rp.RelyingParty) { - // state format set by Login: ":". The nonce is what - // zitadel/oidc already validated (cookie state == query state); we just - // need to recover the returnUrl. Anything else → default to "/". - returnURL := "/" - if _, urlPart, ok := strings.Cut(state, ":"); ok { - if clean, ok := sanitizeRelativePath(urlPart); ok { - returnURL = clean - } - } + returnURL := decodeStateReturnURL(state) logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(tokens.IDToken)) http.Redirect(w, r, fmt.Sprintf("%s#token=%s", returnURL, tokens.IDToken), http.StatusFound) } +// decodeStateReturnURL recovers and re-sanitises the returnUrl half of the +// state minted in Login (":"). The nonce is already +// validated by zitadel/oidc (cookie state == query state); we only need the +// trailing returnUrl, and we re-run it through sanitizeRelativePath as +// defense-in-depth in case the cookie store's contents are ever trusted +// elsewhere. Anything malformed → "/". +func decodeStateReturnURL(state string) string { + _, urlPart, ok := strings.Cut(state, ":") + if !ok { + return "/" + } + clean, ok := sanitizeRelativePath(urlPart) + if !ok { + return "/" + } + return clean +} + // Logout bounces the browser home — the SPA clears its localStorage token on // the redirect. End-session at the IdP (RP-initiated logout) would be an // rp.EndSessionEndpoint roundtrip; add it here when an IdP requires SLO. diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 7bfbd3e..cc64e04 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -1,6 +1,9 @@ package auth -import "testing" +import ( + "strings" + "testing" +) // TestSanitizeRelativePath pins both behaviours in one go: the relative-URL // safety check (open-redirect surface) and the fragment-stripping canonical @@ -38,3 +41,46 @@ func TestSanitizeRelativePath(t *testing.T) { } } } + +// TestDecodeStateReturnURL pins the ":" wire format. Same +// helper produces (in Login) and consumes (in Callback) this string; if +// either side ever changes the separator, this test fails before the SPA +// login flow does. +func TestDecodeStateReturnURL(t *testing.T) { + for _, tc := range []struct { + name string + state string + want string + }{ + {"empty state", "", "/"}, + {"no separator", "abc123", "/"}, + {"nonce only", "abc123:", "/"}, // urlPart="" → reject → "/" + {"happy root", "abc123:/", "/"}, + {"happy path", "abc123:/dashboard", "/dashboard"}, + {"path with query", "abc123:/foo?x=1", "/foo?x=1"}, + {"path with colon — only first ':' splits", "abc123:/foo:bar", "/foo:bar"}, + {"fragment stripped (SPA hash hand-off must stay clean)", "abc123:/foo#section", "/foo"}, + {"open-redirect via protocol-relative URL", "abc123://evil.com/x", "/"}, + {"open-redirect via absolute URL", "abc123:http://evil.com/x", "/"}, + {"backslash trick", "abc123:/\\evil.com/x", "/"}, + } { + t.Run(tc.name, func(t *testing.T) { + got := decodeStateReturnURL(tc.state) + if got != tc.want { + t.Errorf("decodeStateReturnURL(%q) = %q, want %q", tc.state, got, tc.want) + } + }) + } + + // Round-trip sanity: what Login encodes, Callback decodes back. + for _, returnURL := range []string{"/", "/foo", "/foo?x=1", "/foo:bar"} { + state := "deadbeef" + ":" + returnURL + if got := decodeStateReturnURL(state); got != returnURL { + t.Errorf("round-trip for %q failed: got %q", returnURL, got) + } + // And verify nonce is *not* echoed into the return URL. + if strings.Contains(decodeStateReturnURL(state), "deadbeef") { + t.Errorf("nonce leaked into returnURL for state %q", state) + } + } +} From 3225bdda2a03cfe65f69af3cd839955b75765ab3 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Wed, 10 Jun 2026 15:47:11 +0200 Subject: [PATCH 08/14] refactor(auth): switch to the canonical go-oidc + oauth2 pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop zitadel/oidc, gorilla/securecookie and the derived-cookie-keys plumbing. The OIDC RP is now built from the two libraries every other Go project reaches for (coreos/go-oidc + golang.org/x/oauth2) and follows the official go-oidc README example: three short-lived HttpOnly, SameSite=Lax cookies carrying state / PKCE verifier / returnUrl across the login bounce, plus a state-mismatch check on the callback. What this removes: - the zitadel/oidc RelyingParty + CookieHandler dance and its custom discovery URL plumbing - the OIDC_COOKIE_HASH_KEY / OIDC_COOKIE_BLOCK_KEY env vars (and the matching Helm values, Secret entries, config fields, base64-decode helper, length validation, and tests) - the ":" state encoding (returnUrl now lives in its own cookie, no encoding gymnastics) - the fiber/middleware/adaptor bridge — handlers are native Fiber What this keeps: - split-horizon issuer support via oidc.InsecureIssuerURLContext + AuthURL rebase (gated on OIDCPublicIssuer, no-op when single URL) - the discovery retry-on-boot for Cilium-style network-identity races - sanitizeRelativePath + open-redirect tests - ID token verification (signature, iss, aud, exp, nbf) via the Provider's IDTokenVerifier CSRF protection comes from the state match (cookie vs query); replay protection from PKCE S256; cookie tampering surface is bounded by HttpOnly + Secure + SameSite=Lax + a /auth path scope. No shared key between replicas needed because the flow cookies travel with the user end-to-end. Net diff vs the previous PR head: ~170 fewer lines of auth code, two fewer direct deps, zero new operator config knobs. --- charts/dploy/templates/secret.yaml | 10 - charts/dploy/values.yaml | 8 - go.mod | 13 +- go.sum | 35 ---- internal/auth/oidc.go | 287 ++++++++++++++++------------- internal/auth/oidc_test.go | 54 ++---- internal/config/config.go | 11 -- 7 files changed, 172 insertions(+), 246 deletions(-) diff --git a/charts/dploy/templates/secret.yaml b/charts/dploy/templates/secret.yaml index bdc4632..8090b31 100644 --- a/charts/dploy/templates/secret.yaml +++ b/charts/dploy/templates/secret.yaml @@ -18,13 +18,3 @@ stringData: {{- if .Values.auth.oidcClientSecret }} OIDC_CLIENT_SECRET: {{ .Values.auth.oidcClientSecret | quote }} {{- end }} - {{/* Cookie keys for the zitadel/oidc state + PKCE cookies. When unset the - API generates per-process random keys (logs a warning) — fine for a - single replica; mandatory for replicaCount > 1 or rolling-update - resilience. Generate with `head -c 64 /dev/urandom | base64`. */}} - {{- if .Values.auth.oidcCookieHashKey }} - OIDC_COOKIE_HASH_KEY: {{ .Values.auth.oidcCookieHashKey | quote }} - {{- end }} - {{- if .Values.auth.oidcCookieBlockKey }} - OIDC_COOKIE_BLOCK_KEY: {{ .Values.auth.oidcCookieBlockKey | quote }} - {{- end }} diff --git a/charts/dploy/values.yaml b/charts/dploy/values.yaml index 695a5aa..e0de1fb 100644 --- a/charts/dploy/values.yaml +++ b/charts/dploy/values.yaml @@ -84,14 +84,6 @@ auth: oidcRedirectURL: "" - # Cookie keys for the zitadel/oidc state + PKCE cookies. Leave empty for - # single-replica dev (API will mint per-process random keys + log a warn). - # MUST be set to stable secrets when running with replicaCount > 1 or - # tolerating rolling updates without breaking in-flight logins. - # Generate with: head -c 64 /dev/urandom | base64 - oidcCookieHashKey: "" - oidcCookieBlockKey: "" - service: type: ClusterIP port: 80 diff --git a/go.mod b/go.mod index 5adc3cf..e88296d 100644 --- a/go.mod +++ b/go.mod @@ -10,9 +10,8 @@ require ( github.com/fluxcd/source-controller/api v1.8.4 github.com/gofiber/fiber/v2 v2.52.10 github.com/google/uuid v1.6.0 - github.com/gorilla/securecookie v1.1.2 - github.com/zitadel/oidc/v3 v3.47.5 go.uber.org/zap v1.27.1 + golang.org/x/oauth2 v0.36.0 k8s.io/api v0.36.1 k8s.io/apiextensions-apiserver v0.36.0 k8s.io/apimachinery v0.36.1 @@ -37,7 +36,6 @@ require ( github.com/fxamacker/cbor/v2 v2.9.0 // indirect github.com/go-jose/go-jose/v4 v4.1.4 // indirect github.com/go-logr/logr v1.4.3 // indirect - github.com/go-logr/stdr v1.2.2 // indirect github.com/go-logr/zapr v1.3.0 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect @@ -64,7 +62,6 @@ require ( github.com/mitchellh/reflectwalk v1.0.2 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect - github.com/muhlemmer/gu v0.3.1 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/client_golang v1.23.2 // indirect @@ -73,25 +70,17 @@ require ( github.com/prometheus/procfs v0.19.2 // indirect github.com/rivo/uniseg v0.2.0 // indirect github.com/shopspring/decimal v1.4.0 // indirect - github.com/sirupsen/logrus v1.9.4 // indirect github.com/spf13/cast v1.7.0 // indirect github.com/spf13/pflag v1.0.10 // indirect github.com/valyala/bytebufferpool v1.0.0 // indirect github.com/valyala/fasthttp v1.51.0 // indirect github.com/valyala/tcplisten v1.0.0 // indirect github.com/x448/float16 v0.8.4 // indirect - github.com/zitadel/logging v0.7.0 // indirect - github.com/zitadel/schema v1.3.2 // indirect - go.opentelemetry.io/auto/sdk v1.2.1 // indirect - go.opentelemetry.io/otel v1.43.0 // indirect - go.opentelemetry.io/otel/metric v1.43.0 // indirect - go.opentelemetry.io/otel/trace v1.43.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.51.0 // indirect golang.org/x/net v0.55.0 // indirect - golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.45.0 // indirect golang.org/x/term v0.43.0 // indirect diff --git a/go.sum b/go.sum index 25be5d2..f984147 100644 --- a/go.sum +++ b/go.sum @@ -10,8 +10,6 @@ github.com/andybalholm/brotli v1.1.0 h1:eLKJA0d02Lf0mVpIDgYnqXcUn0GqVmEFny3VuID1 github.com/andybalholm/brotli v1.1.0/go.mod h1:sms7XGricyQI9K10gOSf56VKKWS4oLer58Q+mhRPtnY= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/bmatcuk/doublestar/v4 v4.10.0 h1:zU9WiOla1YA122oLM6i4EXvGW62DvKZVxIe6TYWexEs= -github.com/bmatcuk/doublestar/v4 v4.10.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/coreos/go-oidc/v3 v3.18.0 h1:V9orjXynvu5wiC9SemFTWnG4F45v403aIcjWo0d41+A= @@ -43,15 +41,10 @@ github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S github.com/fsnotify/fsnotify v1.9.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sapM= github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= -github.com/go-chi/chi/v5 v5.2.5 h1:Eg4myHZBjyvJmAFjFvWgrqDTXFyOzjj7YIm3L3mu6Ug= -github.com/go-chi/chi/v5 v5.2.5/go.mod h1:X7Gx4mteadT3eDOMTsXzmI4/rwUpOwBHLpAfupzFJP0= 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-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= -github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= -github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ= github.com/go-logr/zapr v1.3.0/go.mod h1:YKepepNBd1u/oyhd/yQmtjVXmm9uML4IXUgMOwR8/Gg= github.com/go-openapi/jsonpointer v0.19.6/go.mod h1:osyAmYz/mB/C3I+WsTTSgw1ONzaLJoLCyoi6/zppojs= @@ -105,12 +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/jeremija/gosubmit v0.2.8 h1:mmSITBz9JxVtu8eqbN+zmmwX7Ij2RidQxhcwRVI4wqA= -github.com/jeremija/gosubmit v0.2.8/go.mod h1:Ui+HS073lCFREXBbdfrJzMB57OI/bdxTiLtrDHHhFPI= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= @@ -143,10 +132,6 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFdJifH4BDsTlE89Zl93FEloxaWZfGcifgq8= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= -github.com/muhlemmer/gu v0.3.1 h1:7EAqmFrW7n3hETvuAdmFmn4hS8W+z3LgKtrnow+YzNM= -github.com/muhlemmer/gu v0.3.1/go.mod h1:YHtHR+gxM+bKEIIs7Hmi9sPT3ZDUvTN/i88wQpZkrdM= -github.com/muhlemmer/httpforwarded v0.1.0 h1:x4DLrzXdliq8mprgUMR0olDvHGkou5BJsK/vWUetyzY= -github.com/muhlemmer/httpforwarded v0.1.0/go.mod h1:yo9czKedo2pdZhoXe+yDkGVbU0TJ0q9oQ90BVoDEtw0= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/onsi/ginkgo/v2 v2.27.4 h1:fcEcQW/A++6aZAZQNUmNjvA9PSOzefMJBerHJ4t8v8Y= @@ -170,12 +155,8 @@ github.com/rivo/uniseg v0.2.0 h1:S1pD9weZBuJdFmowNwbpi7BJ8TNftyUImj/0WQi72jY= github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= -github.com/rs/cors v1.11.1 h1:eU3gRzXLRK57F5rKMGMZURNdIG4EoAmX8k94r9wXWHA= -github.com/rs/cors v1.11.1/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU= github.com/shopspring/decimal v1.4.0 h1:bxl37RwXBklmTi0C79JfXCEBD1cqqHt0bbgBAGFp81k= github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME= -github.com/sirupsen/logrus v1.9.4 h1:TsZE7l11zFCLZnZ+teH4Umoq5BhEIfIzfRDZ1Uzql2w= -github.com/sirupsen/logrus v1.9.4/go.mod h1:ftWc9WdOfJ0a92nsE2jF5u5ZwH8Bv2zdeOC42RjbV2g= github.com/spf13/cast v1.7.0 h1:ntdiHjuueXFgm5nzDRdOS4yfT43P5Fnud6DH50rz/7w= github.com/spf13/cast v1.7.0/go.mod h1:ancEpBxwJDODSW/UG4rDrAqiKolqNNh2DX3mk86cAdo= github.com/spf13/pflag v1.0.10 h1:4EBh2KAYBwaONj6b2Ye1GiHfwjqyROoF4RwYO+vPwFk= @@ -199,20 +180,6 @@ github.com/valyala/tcplisten v1.0.0 h1:rBHj/Xf+E1tRGZyWIWwJDiRY0zc1Js+CV5DqwacVS github.com/valyala/tcplisten v1.0.0/go.mod h1:T0xQ8SeCZGxckz9qRXTfG43PvQ/mcWh7FwZEA7Ioqkc= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= -github.com/zitadel/logging v0.7.0 h1:eugftwMM95Wgqwftsvj81isL0JK/hoScVqp/7iA2adQ= -github.com/zitadel/logging v0.7.0/go.mod h1:9A6h9feBF/3u0IhA4uffdzSDY7mBaf7RE78H5sFMINQ= -github.com/zitadel/oidc/v3 v3.47.5 h1:cR2z0oqa5XZkwpXQiPCUGqKtndrjHgEXb81y3oXocK4= -github.com/zitadel/oidc/v3 v3.47.5/go.mod h1:XxFh0666HRXycyrKmono+3gY0RACpYJLgy4r/+kliKY= -github.com/zitadel/schema v1.3.2 h1:gfJvt7dOMfTmxzhscZ9KkapKo3Nei3B6cAxjav+lyjI= -github.com/zitadel/schema v1.3.2/go.mod h1:IZmdfF9Wu62Zu6tJJTH3UsArevs3Y4smfJIj3L8fzxw= -go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= -go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= -go.opentelemetry.io/otel v1.43.0 h1:mYIM03dnh5zfN7HautFE4ieIig9amkNANT+xcVxAj9I= -go.opentelemetry.io/otel v1.43.0/go.mod h1:JuG+u74mvjvcm8vj8pI5XiHy1zDeoCS2LB1spIq7Ay0= -go.opentelemetry.io/otel/metric v1.43.0 h1:d7638QeInOnuwOONPp4JAOGfbCEpYb+K6DVWvdxGzgM= -go.opentelemetry.io/otel/metric v1.43.0/go.mod h1:RDnPtIxvqlgO8GRW18W6Z/4P462ldprJtfxHxyKd2PY= -go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= -go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= @@ -256,8 +223,6 @@ gopkg.in/evanphx/json-patch.v4 v4.13.0 h1:czT3CmqEaQ1aanPc5SdlgQrrEIb8w/wwCvWWnf gopkg.in/evanphx/json-patch.v4 v4.13.0/go.mod h1:p8EYWUEYMpynmqDbY58zCKCFZw8pRWMG4EsWvDvM72M= gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= -gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= -gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 8970a87..c641fe8 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -1,144 +1,122 @@ // Package auth handles JWT verification and the OIDC login flow for the dploy // API. // -// JWT verification (jwt.go) goes through the canonical go-oidc verifier; the -// OIDC login/callback/logout flow (this file) goes through zitadel/oidc's -// RelyingParty, which already handles state cookie, PKCE S256, nonce, -// at_hash, id_token signature/iss/aud/exp/nbf — all the gotchas that used to -// be hand-rolled here. What's left is dploy-specific glue: split-horizon -// AuthURL substitution, returnUrl sanitisation, retry-on-discovery at boot, -// the Fiber adapter, and the final "#token=..." hand-off the SPA expects. +// Both JWT verification (jwt.go) and the OIDC login flow (this file) follow +// the canonical Go OIDC pattern used by Kubernetes, Argo CD and the official +// coreos/go-oidc README example: go-oidc for discovery + ID token +// verification, golang.org/x/oauth2 for the Authorization Code + PKCE flow, +// and three short-lived HttpOnly cookies to carry state/verifier/returnUrl +// across the browser bounce. No framework, no signed-cookie key management, +// nothing dploy-specific beyond the optional split-horizon issuer support +// and the SPA's "#token=..." hand-off. package auth import ( "context" "crypto/rand" + "crypto/subtle" "encoding/base64" "fmt" - "net/http" "net/url" "strings" "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/gofiber/fiber/v2/middleware/adaptor" - "github.com/gorilla/securecookie" - "github.com/zitadel/oidc/v3/pkg/client/rp" - httphelper "github.com/zitadel/oidc/v3/pkg/http" - "github.com/zitadel/oidc/v3/pkg/oidc" + "golang.org/x/oauth2" ) -// OIDCHandler is a thin wrapper around zitadel/oidc's RelyingParty plus the -// dploy-specific Fiber handlers. +// OIDCHandler wires the canonical go-oidc + oauth2 pair into Fiber handlers. type OIDCHandler struct { - rp rp.RelyingParty + oauth2Config *oauth2.Config + verifier *oidc.IDTokenVerifier + secureCookie bool } const ( discoveryTimeout = 10 * time.Second discoveryAttempts = 5 + + // flowCookieMaxAge bounds how long a user has to complete the IdP + // bounce. 10 min covers slow MFA prompts without leaving stale state + // cookies indefinitely. + flowCookieMaxAge = 10 * 60 + + cookieState = "dploy_oidc_state" + cookieVerifier = "dploy_oidc_verifier" + cookieReturn = "dploy_oidc_return" ) -// NewOIDCHandler wires zitadel/oidc's RelyingParty (which handles state -// cookie, PKCE, nonce, at_hash + id_token verification) and substitutes the -// AuthorizationEndpoint with its public-issuer equivalent so browser redirects -// land on the user-facing IdP URL while backend code exchange + JWKS stay -// in-cluster. +// NewOIDCHandler builds the RP from OIDC discovery. Optional split-horizon +// support: when OIDCPublicIssuer differs from OIDCIssuer, discovery is fetched +// via the in-cluster URL but the expected `iss` is the public one, and the +// browser-facing AuthURL is rebased to the public host. Single-URL setups +// leave OIDCPublicIssuer empty and the split-horizon branches are no-ops. func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { - // Cookie security flag mirrors the redirect URL's scheme — HTTP for the - // dev/CTF cluster, HTTPS for production. - cookieOpts := []httphelper.CookieHandlerOpt{} - if strings.HasPrefix(cfg.OIDCRedirectURL, "http://") { - cookieOpts = append(cookieOpts, httphelper.WithUnsecure()) - } - // Cookie keys: prefer env-provided secrets so logins survive pod restarts - // and load-balance across replicas. Falling back to process-random is fine - // for single-replica dev but logs a loud warning. - hashKey := []byte(cfg.OIDCCookieHashKey) - blockKey := []byte(cfg.OIDCCookieBlockKey) - if len(hashKey) == 0 { - hashKey = securecookie.GenerateRandomKey(64) - logger.Warn("OIDC_COOKIE_HASH_KEY not set; using a process-random key — logins will break across pod restarts or replicas") - } - if len(blockKey) == 0 { - blockKey = securecookie.GenerateRandomKey(32) - logger.Warn("OIDC_COOKIE_BLOCK_KEY not set; using a process-random key — logins will break across pod restarts or replicas") - } - cookieHandler := httphelper.NewCookieHandler(hashKey, blockKey, cookieOpts...) - - // Optional split-horizon issuer support: when OIDCPublicIssuer is set and - // differs from OIDCIssuer, the IdP is reached via two URLs — the in-cluster - // one (low-latency for discovery/token/JWKS) and the public one (which is - // what tokens carry as `iss` and what browsers must redirect to). Most - // deployments expose the IdP on a single URL and can leave OIDCPublicIssuer - // empty; the block below + the AuthURL rebase further down are no-ops then. + ctx := context.Background() expectedIssuer := cfg.OIDCIssuer - opts := []rp.Option{ - rp.WithCookieHandler(cookieHandler), - rp.WithPKCE(cookieHandler), - } - if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { - // zitadel/oidc validates discovery.Issuer == arg.issuer. Pass the - // public one (what Dex advertises) and override the fetch URL to the - // internal one via WithCustomDiscoveryUrl so the pod doesn't have to - // resolve the public host at boot. + splitHorizon := cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer + if splitHorizon { + // Tell go-oidc to expect tokens with iss == publicIssuer even + // though we hit the internal URL for the discovery doc. + ctx = oidc.InsecureIssuerURLContext(ctx, cfg.OIDCPublicIssuer) expectedIssuer = cfg.OIDCPublicIssuer - internalDiscoveryURL := strings.TrimSuffix(cfg.OIDCIssuer, "/") + "/.well-known/openid-configuration" - opts = append(opts, rp.WithCustomDiscoveryUrl(internalDiscoveryURL)) } - relyingParty, err := newRelyingPartyWithRetry(context.Background(), - expectedIssuer, cfg.OIDCClientID, cfg.OIDCClientSecret, cfg.OIDCRedirectURL, - []string{oidc.ScopeOpenID, "email", "profile"}, - opts..., - ) + provider, err := newProviderWithRetry(ctx, cfg.OIDCIssuer) if err != nil { - return nil, fmt.Errorf("failed to build OIDC RelyingParty: %w", err) + return nil, fmt.Errorf("OIDC discovery against %s: %w", cfg.OIDCIssuer, err) } - // Second half of the optional split-horizon path. Dex derives the - // discovery doc's endpoint URLs from the request Host header, so fetching - // via the in-cluster URL gives us internal endpoints. Backend code - // exchange + JWKS stay internal (those are fine); only the - // AuthorizationEndpoint must be rebased to public so browsers actually - // land on the user-facing IdP URL. - if cfg.OIDCPublicIssuer != "" && cfg.OIDCPublicIssuer != cfg.OIDCIssuer { + endpoint := provider.Endpoint() + if splitHorizon { + // Dex derives the discovery doc's endpoints from the request Host + // header, so fetching via the internal URL gives us internal + // endpoints. Backend code exchange + JWKS stay internal (fine); + // only the browser-facing AuthURL must be rebased so users land on + // the public IdP URL. internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) - ep := &relyingParty.OAuthConfig().Endpoint - if !strings.Contains(ep.AuthURL, internalBase) { - // Discovery returned an AuthURL we can't rebase (Dex emits a host - // that doesn't match OIDCIssuer — config drift). Bail loudly so - // the failure is visible at boot, not silently as "browser - // redirects to the in-cluster URL that it can't reach". - return nil, fmt.Errorf("OIDC AuthURL %q does not contain expected internal base %q; check OIDCIssuer / IdP issuer config", ep.AuthURL, internalBase) + if !strings.Contains(endpoint.AuthURL, internalBase) { + return nil, fmt.Errorf("OIDC AuthURL %q does not contain expected internal base %q; check OIDCIssuer / IdP config", endpoint.AuthURL, internalBase) } - ep.AuthURL = strings.Replace(ep.AuthURL, internalBase, publicBase, 1) + endpoint.AuthURL = strings.Replace(endpoint.AuthURL, internalBase, publicBase, 1) logger.Info("OIDC auth endpoint rebased to public", - "internal", internalBase, "public", publicBase, "authURL", ep.AuthURL) + "internal", internalBase, "public", publicBase, "authURL", endpoint.AuthURL) } - logger.Info("OIDC handler initialized", "expectedIssuer", expectedIssuer) - return &OIDCHandler{rp: relyingParty}, nil + h := &OIDCHandler{ + oauth2Config: &oauth2.Config{ + ClientID: cfg.OIDCClientID, + ClientSecret: cfg.OIDCClientSecret, + RedirectURL: cfg.OIDCRedirectURL, + Endpoint: endpoint, + Scopes: []string{oidc.ScopeOpenID, "email", "profile"}, + }, + verifier: provider.Verifier(&oidc.Config{ClientID: cfg.OIDCClientID}), + secureCookie: !strings.HasPrefix(cfg.OIDCRedirectURL, "http://"), + } + logger.Info("OIDC handler initialized", "expectedIssuer", expectedIssuer, "secureCookie", h.secureCookie) + return h, nil } -// newRelyingPartyWithRetry rides out the post-startup network-identity window -// (Cilium et al.) where DNS / egress briefly returns EPERM. Same shape as the -// previous discoverWithRetry: 5 attempts, exponential backoff capped at 4 s. -func newRelyingPartyWithRetry(ctx context.Context, issuer, clientID, clientSecret, redirectURI string, scopes []string, options ...rp.Option) (rp.RelyingParty, error) { +// newProviderWithRetry rides out the post-startup network-identity window +// (Cilium et al.) where DNS / egress briefly returns EPERM. 5 attempts with +// exponential backoff capped at 4 s. +func newProviderWithRetry(ctx context.Context, issuer string) (*oidc.Provider, error) { delay := 500 * time.Millisecond var lastErr error for i := 1; i <= discoveryAttempts; i++ { attemptCtx, cancel := context.WithTimeout(ctx, discoveryTimeout) - party, err := rp.NewRelyingPartyOIDC(attemptCtx, issuer, clientID, clientSecret, redirectURI, scopes, options...) + provider, err := oidc.NewProvider(attemptCtx, issuer) cancel() if err == nil { if i > 1 { logger.Info("OIDC discovery succeeded after retries", "attempts", i) } - return party, nil + return provider, nil } lastErr = err if i == discoveryAttempts { @@ -180,65 +158,114 @@ func sanitizeRelativePath(s string) (string, bool) { return u.String(), true } -// Login wraps zitadel/oidc's AuthURLHandler: it signs+encrypts the state into -// a cookie, hands it to the IdP as ?state=…, and gives it back to us after -// verifying. The state value is ":" — the nonce makes the -// state unguessable (defeats CSRF / session-fixation: an attacker who -// triggers /auth/login on a victim browser still can't predict the value -// stored in the cookie), while returnUrl piggybacks for free. +func randomURLSafe(n int) (string, error) { + b := make([]byte, n) + if _, err := rand.Read(b); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(b), nil +} + +func (h *OIDCHandler) setFlowCookie(c *fiber.Ctx, name, value string) { + c.Cookie(&fiber.Cookie{ + Name: name, + Value: value, + Path: "/auth", + HTTPOnly: true, + Secure: h.secureCookie, + SameSite: "Lax", + MaxAge: flowCookieMaxAge, + }) +} + +func (h *OIDCHandler) clearFlowCookie(c *fiber.Ctx, name string) { + c.Cookie(&fiber.Cookie{ + Name: name, + Value: "", + Path: "/auth", + HTTPOnly: true, + Secure: h.secureCookie, + SameSite: "Lax", + MaxAge: -1, + }) +} + +// Login follows the official go-oidc example: random state + PKCE verifier, +// each stored in an HttpOnly cookie, plus the AuthCodeURL redirect. The +// returnUrl piggybacks as a third cookie — no state encoding/decoding +// gymnastics needed. func (h *OIDCHandler) Login(c *fiber.Ctx) error { - rawReturn := c.Query("returnUrl", "/") returnURL := "/" + rawReturn := c.Query("returnUrl", "/") if clean, ok := sanitizeRelativePath(rawReturn); ok { returnURL = clean } else { logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", rawReturn) } - nonceBytes := make([]byte, 16) - if _, err := rand.Read(nonceBytes); err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state nonce"}) + + state, err := randomURLSafe(24) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"}) } - nonce := base64.RawURLEncoding.EncodeToString(nonceBytes) - stateFn := func() string { return nonce + ":" + returnURL } - return adaptor.HTTPHandler(rp.AuthURLHandler(stateFn, h.rp))(c) + verifier := oauth2.GenerateVerifier() + + h.setFlowCookie(c, cookieState, state) + h.setFlowCookie(c, cookieVerifier, verifier) + h.setFlowCookie(c, cookieReturn, returnURL) + + return c.Redirect( + h.oauth2Config.AuthCodeURL(state, oauth2.S256ChallengeOption(verifier)), + fiber.StatusFound, + ) } -// Callback wraps zitadel/oidc's CodeExchangeHandler. The library does state -// cookie verification, PKCE-aware code exchange, and full id_token -// verification (signature, iss, aud, exp, nbf, nonce, at_hash). All we add is -// a defense-in-depth re-sanitisation of state-as-returnUrl and the SPA's -// hash-fragment token hand-off. +// Callback exchanges the code for tokens, verifies the ID token, and bounces +// the browser to returnUrl with "#token=" so the SPA can pick it up. +// CSRF protection comes from the state match (cookie vs query); replay +// protection comes from PKCE. func (h *OIDCHandler) Callback(c *fiber.Ctx) error { - return adaptor.HTTPHandler(rp.CodeExchangeHandler(h.exchangeCallback, h.rp))(c) -} + state := c.Cookies(cookieState) + verifier := c.Cookies(cookieVerifier) + returnURL := c.Cookies(cookieReturn) + // One-shot: clear cookies before any failure path so a stale flow can't + // be retried by replaying the callback URL. + h.clearFlowCookie(c, cookieState) + h.clearFlowCookie(c, cookieVerifier) + h.clearFlowCookie(c, cookieReturn) -func (h *OIDCHandler) exchangeCallback(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, _ rp.RelyingParty) { - returnURL := decodeStateReturnURL(state) - logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(tokens.IDToken)) - http.Redirect(w, r, fmt.Sprintf("%s#token=%s", returnURL, tokens.IDToken), http.StatusFound) -} + if state == "" || verifier == "" { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing or expired login session"}) + } + if subtle.ConstantTimeCompare([]byte(c.Query("state")), []byte(state)) != 1 { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "state mismatch"}) + } -// decodeStateReturnURL recovers and re-sanitises the returnUrl half of the -// state minted in Login (":"). The nonce is already -// validated by zitadel/oidc (cookie state == query state); we only need the -// trailing returnUrl, and we re-run it through sanitizeRelativePath as -// defense-in-depth in case the cookie store's contents are ever trusted -// elsewhere. Anything malformed → "/". -func decodeStateReturnURL(state string) string { - _, urlPart, ok := strings.Cut(state, ":") - if !ok { - return "/" + token, err := h.oauth2Config.Exchange(c.Context(), c.Query("code"), oauth2.VerifierOption(verifier)) + if err != nil { + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed: " + err.Error()}) } - clean, ok := sanitizeRelativePath(urlPart) + rawIDToken, ok := token.Extra("id_token").(string) if !ok { - return "/" + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "no id_token in token response"}) + } + if _, err := h.verifier.Verify(c.Context(), rawIDToken); err != nil { + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "id_token verification failed: " + err.Error()}) + } + + // Re-sanitise the returnUrl from the cookie as defense-in-depth, even + // though Login already vetted it before setting the cookie. + if clean, ok := sanitizeRelativePath(returnURL); ok { + returnURL = clean + } else { + returnURL = "/" } - return clean + logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken)) + return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound) } -// Logout bounces the browser home — the SPA clears its localStorage token on -// the redirect. End-session at the IdP (RP-initiated logout) would be an -// rp.EndSessionEndpoint roundtrip; add it here when an IdP requires SLO. +// Logout bounces home — the SPA clears its localStorage token on the +// redirect. RP-initiated logout against the IdP would be an extra round-trip; +// add it when an IdP requires SLO. func (h *OIDCHandler) Logout(c *fiber.Ctx) error { return c.Redirect("/", fiber.StatusFound) } diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index cc64e04..4021bed 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -1,9 +1,6 @@ package auth -import ( - "strings" - "testing" -) +import "testing" // TestSanitizeRelativePath pins both behaviours in one go: the relative-URL // safety check (open-redirect surface) and the fragment-stripping canonical @@ -42,45 +39,22 @@ func TestSanitizeRelativePath(t *testing.T) { } } -// TestDecodeStateReturnURL pins the ":" wire format. Same -// helper produces (in Login) and consumes (in Callback) this string; if -// either side ever changes the separator, this test fails before the SPA -// login flow does. -func TestDecodeStateReturnURL(t *testing.T) { +// TestExtractBaseURL guards the split-horizon AuthURL rebase: if the helper +// strips path/query as expected, strings.Replace(authURL, internalBase, ...) +// hits exactly one match. The test cases mirror the two real-world Issuers +// (in-cluster Service URL with port, public URL via ingress). +func TestExtractBaseURL(t *testing.T) { for _, tc := range []struct { - name string - state string - want string + in, want string }{ - {"empty state", "", "/"}, - {"no separator", "abc123", "/"}, - {"nonce only", "abc123:", "/"}, // urlPart="" → reject → "/" - {"happy root", "abc123:/", "/"}, - {"happy path", "abc123:/dashboard", "/dashboard"}, - {"path with query", "abc123:/foo?x=1", "/foo?x=1"}, - {"path with colon — only first ':' splits", "abc123:/foo:bar", "/foo:bar"}, - {"fragment stripped (SPA hash hand-off must stay clean)", "abc123:/foo#section", "/foo"}, - {"open-redirect via protocol-relative URL", "abc123://evil.com/x", "/"}, - {"open-redirect via absolute URL", "abc123:http://evil.com/x", "/"}, - {"backslash trick", "abc123:/\\evil.com/x", "/"}, + {"http://dex.dex.svc.cluster.local:5556", "http://dex.dex.svc.cluster.local:5556"}, + {"http://dex.dex.svc.cluster.local:5556/", "http://dex.dex.svc.cluster.local:5556"}, + {"https://dex.dploy.ctf.local", "https://dex.dploy.ctf.local"}, + {"https://dex.dploy.ctf.local/dex", "https://dex.dploy.ctf.local"}, + {"https://dex.dploy.ctf.local/dex/auth?x=1", "https://dex.dploy.ctf.local"}, } { - t.Run(tc.name, func(t *testing.T) { - got := decodeStateReturnURL(tc.state) - if got != tc.want { - t.Errorf("decodeStateReturnURL(%q) = %q, want %q", tc.state, got, tc.want) - } - }) - } - - // Round-trip sanity: what Login encodes, Callback decodes back. - for _, returnURL := range []string{"/", "/foo", "/foo?x=1", "/foo:bar"} { - state := "deadbeef" + ":" + returnURL - if got := decodeStateReturnURL(state); got != returnURL { - t.Errorf("round-trip for %q failed: got %q", returnURL, got) - } - // And verify nonce is *not* echoed into the return URL. - if strings.Contains(decodeStateReturnURL(state), "deadbeef") { - t.Errorf("nonce leaked into returnURL for state %q", state) + if got := extractBaseURL(tc.in); got != tc.want { + t.Errorf("extractBaseURL(%q) = %q, want %q", tc.in, got, tc.want) } } } diff --git a/internal/config/config.go b/internal/config/config.go index 6354703..0434f20 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,14 +20,6 @@ type Config struct { OIDCClientSecret string OIDCRedirectURL string - // Cookie keys for the OIDC state / PKCE cookies (zitadel/oidc CookieHandler). - // Set both to stable random secrets (k8s Secret, env, etc.) when running - // with replicaCount > 1 or you'll get login failures on rolling updates, - // since requests can land on a pod that can't decrypt cookies set by - // another. Empty values fall back to process-random keys with a warn. - OIDCCookieHashKey string // 64 bytes recommended (HMAC key) - OIDCCookieBlockKey string // 32 bytes (AES key) - // Kubernetes: the namespace where DployTemplate and DployInstance CRs live. Namespace string @@ -77,9 +69,6 @@ func Load() (*Config, error) { OIDCClientSecret: getEnv("OIDC_CLIENT_SECRET", "dploy-secret"), OIDCRedirectURL: getEnv("OIDC_REDIRECT_URL", "http://localhost:8080/auth/callback"), - OIDCCookieHashKey: getEnv("OIDC_COOKIE_HASH_KEY", ""), - OIDCCookieBlockKey: getEnv("OIDC_COOKIE_BLOCK_KEY", ""), - // Kubernetes Namespace: getEnv("DPLOY_NAMESPACE", "dploy-system"), From ac8fedc00f8221ca9a3a49a7b30ae5baa4d8ae9e Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Wed, 10 Jun 2026 15:56:19 +0200 Subject: [PATCH 09/14] feat(auth): add OIDC nonce verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit State + PKCE cover CSRF and code-replay, but the OIDC spec recommends nonce as the third leg: it binds the *ID token* (not just the code) to this specific login attempt. The attack it closes is the one where both the auth code and the PKCE verifier leak together — the IdP would issue a token carrying the attacker's nonce, not ours. Mint a 16-byte random nonce per Login, ship it to the IdP via oidc.Nonce(...) on AuthCodeURL, store it alongside state/verifier in an HttpOnly cookie, then compare against idToken.Nonce in Callback with subtle.ConstantTimeCompare. Strict mode: a missing nonce cookie fails the flow with "missing or expired login session" same as missing state/verifier. --- internal/auth/oidc.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index c641fe8..2ffde33 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -46,6 +46,7 @@ const ( cookieState = "dploy_oidc_state" cookieVerifier = "dploy_oidc_verifier" + cookieNonce = "dploy_oidc_nonce" cookieReturn = "dploy_oidc_return" ) @@ -207,14 +208,19 @@ func (h *OIDCHandler) Login(c *fiber.Ctx) error { if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint state"}) } + nonce, err := randomURLSafe(16) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to mint nonce"}) + } verifier := oauth2.GenerateVerifier() h.setFlowCookie(c, cookieState, state) h.setFlowCookie(c, cookieVerifier, verifier) + h.setFlowCookie(c, cookieNonce, nonce) h.setFlowCookie(c, cookieReturn, returnURL) return c.Redirect( - h.oauth2Config.AuthCodeURL(state, oauth2.S256ChallengeOption(verifier)), + h.oauth2Config.AuthCodeURL(state, oauth2.S256ChallengeOption(verifier), oidc.Nonce(nonce)), fiber.StatusFound, ) } @@ -226,14 +232,16 @@ func (h *OIDCHandler) Login(c *fiber.Ctx) error { func (h *OIDCHandler) Callback(c *fiber.Ctx) error { state := c.Cookies(cookieState) verifier := c.Cookies(cookieVerifier) + nonce := c.Cookies(cookieNonce) returnURL := c.Cookies(cookieReturn) // One-shot: clear cookies before any failure path so a stale flow can't // be retried by replaying the callback URL. h.clearFlowCookie(c, cookieState) h.clearFlowCookie(c, cookieVerifier) + h.clearFlowCookie(c, cookieNonce) h.clearFlowCookie(c, cookieReturn) - if state == "" || verifier == "" { + if state == "" || verifier == "" || nonce == "" { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing or expired login session"}) } if subtle.ConstantTimeCompare([]byte(c.Query("state")), []byte(state)) != 1 { @@ -248,9 +256,17 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { if !ok { return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "no id_token in token response"}) } - if _, err := h.verifier.Verify(c.Context(), rawIDToken); err != nil { + idToken, err := h.verifier.Verify(c.Context(), rawIDToken) + if err != nil { return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "id_token verification failed: " + err.Error()}) } + // Nonce binds this ID token to *our* login attempt: even if both the + // auth code and PKCE verifier are intercepted, the IdP would return a + // token carrying the attacker's nonce, not ours. Constant-time compare + // because the nonce stays a one-shot secret until the cookie clears. + if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(nonce)) != 1 { + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "nonce mismatch"}) + } // Re-sanitise the returnUrl from the cookie as defense-in-depth, even // though Login already vetted it before setting the cookie. From 14abf8255a7b879f3abdb0abe32244fece019bc5 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Wed, 10 Jun 2026 21:17:29 +0200 Subject: [PATCH 10/14] fix(auth): handle IdP error redirect + missing code in callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round #4 caught two diagnostic gaps in the new go-oidc-style callback: 1. The IdP signals failure (user cancelled, consent denied, scope rejected, …) by redirecting back with ?error=& error_description= per OAuth 2.0 §4.1.2.1, NOT with a code. Previous code fell through to state validation and returned a "state mismatch" or "token exchange failed" error that hid the actual IdP reason. Now we short-circuit on ?error= with a 400 that carries through the IdP's error code and description. 2. An empty ?code= (IdP misbehaving, or someone hitting /auth/callback directly) reached oauth2.Exchange and surfaced as a 502 from the token endpoint. Now it's caught up front with a clean 400 "missing authorization code". Both checks happen after the one-shot cookie clear, so a failed flow can't be replayed by re-hitting the callback URL. --- internal/auth/oidc.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 2ffde33..abf2caa 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -241,14 +241,31 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { h.clearFlowCookie(c, cookieNonce) h.clearFlowCookie(c, cookieReturn) + // IdP-side failure (user cancelled, consent denied, scope rejected, …) + // arrives as ?error=&error_description= per OAuth 2.0 + // §4.1.2.1. Surface it as a clean 400 rather than letting the request + // fall through to a misleading "state mismatch" or token-exchange error. + if idpErr := c.Query("error"); idpErr != "" { + logger.Warn("OIDC callback: IdP returned error", + "error", idpErr, "description", c.Query("error_description")) + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{ + "error": "identity provider rejected the login: " + idpErr, + "error_description": c.Query("error_description"), + }) + } + if state == "" || verifier == "" || nonce == "" { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing or expired login session"}) } if subtle.ConstantTimeCompare([]byte(c.Query("state")), []byte(state)) != 1 { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "state mismatch"}) } + code := c.Query("code") + if code == "" { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing authorization code"}) + } - token, err := h.oauth2Config.Exchange(c.Context(), c.Query("code"), oauth2.VerifierOption(verifier)) + token, err := h.oauth2Config.Exchange(c.Context(), code, oauth2.VerifierOption(verifier)) if err != nil { return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed: " + err.Error()}) } From 8ddcae3d106527d96c98dd667c0bb1b9a5f8d13a Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Thu, 11 Jun 2026 12:47:13 +0200 Subject: [PATCH 11/14] fix(auth): keep token-exchange/verify error detail server-side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit oauth2 and go-oidc error strings embed the token endpoint and issuer URLs — internal cluster topology a CTF player shouldn't get for free. Log the detail, return a stable generic message to the browser. Addresses Copilot review round 5 (1/3 and 2/3); 3/3 (at_hash) is resolved in the PR description instead: the access token is discarded, so the at_hash binding has nothing to protect. --- internal/auth/oidc.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index abf2caa..6dddd80 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -241,7 +241,7 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { h.clearFlowCookie(c, cookieNonce) h.clearFlowCookie(c, cookieReturn) - // IdP-side failure (user cancelled, consent denied, scope rejected, …) + // IdP-side failure (user canceled, consent denied, scope rejected, …) // arrives as ?error=&error_description= per OAuth 2.0 // §4.1.2.1. Surface it as a clean 400 rather than letting the request // fall through to a misleading "state mismatch" or token-exchange error. @@ -267,7 +267,10 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { token, err := h.oauth2Config.Exchange(c.Context(), code, oauth2.VerifierOption(verifier)) if err != nil { - return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed: " + err.Error()}) + // Detail stays server-side: oauth2 errors embed the token endpoint + // URL, which would map internal cluster topology for the caller. + logger.Error("OIDC callback: token exchange failed", "error", err) + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed"}) } rawIDToken, ok := token.Extra("id_token").(string) if !ok { @@ -275,7 +278,8 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { } idToken, err := h.verifier.Verify(c.Context(), rawIDToken) if err != nil { - return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "id_token verification failed: " + err.Error()}) + logger.Error("OIDC callback: id_token verification failed", "error", err) + return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "id_token verification failed"}) } // Nonce binds this ID token to *our* login attempt: even if both the // auth code and PKCE verifier are intercepted, the IdP would return a @@ -285,7 +289,7 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "nonce mismatch"}) } - // Re-sanitise the returnUrl from the cookie as defense-in-depth, even + // Re-sanitize the returnUrl from the cookie as defense-in-depth, even // though Login already vetted it before setting the cookie. if clean, ok := sanitizeRelativePath(returnURL); ok { returnURL = clean From d6c5b0262686fed1d27df20c043c8f878121fd2d Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Thu, 11 Jun 2026 12:47:23 +0200 Subject: [PATCH 12/14] chore(lint): migrate golangci-lint config to v2 + fix repo-wide findings CI lint has been red on every branch since the action's 'latest' moved to golangci-lint v2, which rejects the v1 config schema (and the last v1 release can't even run against Go 1.26). Migrated with 'golangci-lint migrate', then tuned: - exhaustive: default-signifies-exhaustive (all flagged switches already had an explicit default) - drop goconst (pure noise: "true", "error", k8s condition strings) - govet: disable shadow ('if err :=' inside an err scope is idiomatic) - gocritic: disable hugeParam (controller-runtime passes specs by value) - gochecknoinits excluded for api/ and cmd/ (kubebuilder scheme registration pattern), gocyclo excluded for Reconcile loops - errcheck back to defaults (check-blank flagged deliberate discards) Code fixes: gofmt x4, misspell x4, QF1008 embedded selector, named results on JWTValidator.Validate, nolint on the scaffolded scheme.Builder. golangci-lint v2.12.2 now reports 0 issues. --- .golangci.yml | 207 +++++++++--------- api/v1alpha1/groupversion_info.go | 1 + cmd/api/main.go | 2 +- internal/auth/jwt.go | 12 +- internal/auth/oidc_test.go | 2 +- .../controller/dployinstance_controller.go | 18 +- internal/controller/flux.go | 2 +- internal/kube/client.go | 2 +- internal/models/responses.go | 8 +- internal/operatorconfig/resolver.go | 16 +- 10 files changed, 139 insertions(+), 131 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1709098..15b6399 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,40 +1,26 @@ -# golangci-lint configuration +# golangci-lint v2 configuration # https://golangci-lint.run/usage/configuration/ - +version: "2" run: - timeout: 5m issues-exit-code: 1 tests: true - output: formats: - - format: colored-line-number - print-issued-lines: true - print-linter-name: true - + text: + path: stdout + print-linter-name: true + print-issued-lines: true linters: enable: - # Default linters - - errcheck - - gosimple - - govet - - ineffassign - - staticcheck - - unused - # Additional linters - bodyclose - dogsled - dupl - errorlint - exhaustive - - exportloopref - gochecknoinits - - goconst - gocritic - gocyclo - godot - - gofmt - - goimports - goprintffuncname - gosec - misspell @@ -47,92 +33,113 @@ linters: - revive - rowserrcheck - sqlclosecheck - - stylecheck + - staticcheck - tparallel - unconvert - unparam - whitespace - -linters-settings: - errcheck: - check-type-assertions: true - check-blank: true - - govet: - enable-all: true - disable: - - fieldalignment - - gocyclo: - min-complexity: 15 - - goconst: - min-len: 3 - min-occurrences: 3 - - misspell: - locale: US - - revive: + settings: + dupl: + threshold: 150 + exhaustive: + # A switch with a default branch has made its catch-all explicit; + # don't also require every enum member to be spelled out. + default-signifies-exhaustive: true + gocritic: + disabled-checks: + - dupImport + - ifElseChain + - octalLiteral + - whyNoLint + - wrapperFunc + # controller-runtime passes spec structs by value all over; flagging + # every 160-byte struct copy is noise for an operator codebase. + - hugeParam + enabled-tags: + - diagnostic + - experimental + - opinionated + - performance + - style + gocyclo: + min-complexity: 20 + gosec: + excludes: + - G104 + - G304 + govet: + disable: + - fieldalignment + # `if err := f(); err != nil` inside a scope that already has an err + # is idiomatic Go; shadow flags all of them. + - shadow + enable-all: true + misspell: + locale: US + revive: + rules: + - name: blank-imports + - name: context-as-argument + - name: context-keys-type + - name: dot-imports + - name: error-return + - name: error-strings + - name: error-naming + - name: exported + - name: if-return + - name: increment-decrement + - name: var-naming + - name: var-declaration + - name: package-comments + - name: range + - name: receiver-naming + - name: time-naming + - name: unexported-return + - name: indent-error-flow + - name: errorf + exclusions: + generated: lax + presets: + - comments + - common-false-positives + - legacy + - std-error-handling rules: - - name: blank-imports - - name: context-as-argument - - name: context-keys-type - - name: dot-imports - - name: error-return - - name: error-strings - - name: error-naming - - name: exported - - name: if-return - - name: increment-decrement - - name: var-naming - - name: var-declaration - - name: package-comments - - name: range - - name: receiver-naming - - name: time-naming - - name: unexported-return - - name: indent-error-flow - - name: errorf - - gosec: - excludes: - - G104 # Audit errors not checked - - G304 # File path provided as taint input - - dupl: - threshold: 150 - - gocritic: - enabled-tags: - - diagnostic - - experimental - - opinionated - - performance - - style - disabled-checks: - - dupImport - - ifElseChain - - octalLiteral - - whyNoLint - - wrapperFunc - + - linters: + - dogsled + - dupl + - errcheck + - gocyclo + - gosec + path: _test\.go + # kubebuilder scaffolds init() for scheme registration in api packages + # and manager entrypoints — that's the supported pattern, not a smell. + - linters: + - gochecknoinits + path: ^(api|cmd)/ + # Reconcile loops are long state machines by design; the complexity + # budget elsewhere stays at the default. + - linters: + - gocyclo + path: ^internal/controller/ + - linters: + - all + path: (.*)\.gen\.go + paths: + - third_party$ + - builtin$ + - examples$ issues: - exclude-rules: - # Exclude some linters from running on tests files - - path: _test\.go - linters: - - gocyclo - - errcheck - - dupl - - gosec - - goconst - - # Exclude known issues in generated files - - path: "(.*)\\.gen\\.go" - linters: - - all - max-issues-per-linter: 0 max-same-issues: 0 new: false +formatters: + enable: + - gofmt + - goimports + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index 2f1c0cf..1051c57 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -13,6 +13,7 @@ var ( GroupVersion = schema.GroupVersion{Group: "dploy.dev", Version: "v1alpha1"} // SchemeBuilder is used to add go types to the GroupVersionKind scheme. + //nolint:staticcheck // scheme.Builder is the kubebuilder-scaffolded pattern; SA1019 is aimed at hand-written api packages. SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion} // AddToScheme adds the types in this group-version to the given scheme. diff --git a/cmd/api/main.go b/cmd/api/main.go index 78f8228..176a464 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -148,7 +148,7 @@ func main() { // Admin endpoints — gated by MANAGER_ENABLED + the admin claim/value pair. // 404 when disabled, 403 to non-admin requesters. Shared manager-gate - // middleware so both routes get the same 404 behaviour off-feature. + // middleware so both routes get the same 404 behavior off-feature. managerGate := func(c *fiber.Ctx) error { if !cfg.ManagerEnabled { return c.Status(fiber.StatusNotFound).JSON(models.ErrorResponse{Error: "manager disabled"}) diff --git a/internal/auth/jwt.go b/internal/auth/jwt.go index f9a62db..18b2e96 100644 --- a/internal/auth/jwt.go +++ b/internal/auth/jwt.go @@ -40,16 +40,16 @@ func NewJWTValidator(jwksURL, issuer, audience, usernameClaim string) *JWTValida return &JWTValidator{verifier: verifier, usernameClaim: usernameClaim} } -// Validate verifies the token and returns (sanitizedUsername, raw claims, err). -// All cryptographic and standard-claim checks live inside Verify; the only -// dploy-specific work is pulling the configured username claim and sanitizing -// it for use as a Kubernetes label. -func (v *JWTValidator) Validate(tokenString string) (string, map[string]any, error) { +// Validate verifies the token and returns the sanitized username plus the raw +// claims. All cryptographic and standard-claim checks live inside Verify; the +// only dploy-specific work is pulling the configured username claim and +// sanitizing it for use as a Kubernetes label. +func (v *JWTValidator) Validate(tokenString string) (username string, claims map[string]any, err error) { idToken, err := v.verifier.Verify(context.Background(), tokenString) if err != nil { return "", nil, fmt.Errorf("token parsing failed: %w", err) } - claims := map[string]any{} + claims = map[string]any{} if err := idToken.Claims(&claims); err != nil { return "", nil, fmt.Errorf("decode claims: %w", err) } diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 4021bed..c0456e5 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -2,7 +2,7 @@ package auth import "testing" -// TestSanitizeRelativePath pins both behaviours in one go: the relative-URL +// TestSanitizeRelativePath pins both behaviors in one go: the relative-URL // safety check (open-redirect surface) and the fragment-stripping canonical // form the SPA's consumeHashToken() relies on. Both come out of one // net/url.Parse pass — no string fiddling. diff --git a/internal/controller/dployinstance_controller.go b/internal/controller/dployinstance_controller.go index 1bd9799..af08995 100644 --- a/internal/controller/dployinstance_controller.go +++ b/internal/controller/dployinstance_controller.go @@ -301,15 +301,15 @@ func (r *DployInstanceReconciler) buildData(inst *dployv1alpha1.DployInstance, t } return &templating.Data{ - Owner: sanitize(owner), - UUID: inst.Status.UUID, - BaseDomain: eff.BaseDomain, - Host: defaultHost(inst.Spec.TemplateRef, inst.Status.UUID, eff.BaseDomain), - Namespace: targetNS, - Template: tmpl, - Params: params, - Claims: claims, - Config: templating.Config{Values: eff.Values}, + Owner: sanitize(owner), + UUID: inst.Status.UUID, + BaseDomain: eff.BaseDomain, + Host: defaultHost(inst.Spec.TemplateRef, inst.Status.UUID, eff.BaseDomain), + Namespace: targetNS, + Template: tmpl, + Params: params, + Claims: claims, + Config: templating.Config{Values: eff.Values}, }, nil } diff --git a/internal/controller/flux.go b/internal/controller/flux.go index 543e4b1..b702dda 100644 --- a/internal/controller/flux.go +++ b/internal/controller/flux.go @@ -12,8 +12,8 @@ import ( fluxmeta "github.com/fluxcd/pkg/apis/meta" sourcev1 "github.com/fluxcd/source-controller/api/v1" corev1 "k8s.io/api/core/v1" - apimeta "k8s.io/apimachinery/pkg/api/meta" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" diff --git a/internal/kube/client.go b/internal/kube/client.go index ed82f64..4094112 100644 --- a/internal/kube/client.go +++ b/internal/kube/client.go @@ -284,7 +284,7 @@ func (c *Client) ExtendInstance(ctx context.Context, inst *dployv1alpha1.DployIn return time.Time{}, fmt.Errorf("%w (%d)", ErrMaxExtends, maxExtends) } - newExpires := inst.Spec.ExpiresAt.Time.Add(time.Duration(extendSeconds) * time.Second) + newExpires := inst.Spec.ExpiresAt.Add(time.Duration(extendSeconds) * time.Second) patch := client.MergeFrom(inst.DeepCopy()) t := metav1.NewTime(newExpires) inst.Spec.ExpiresAt = &t diff --git a/internal/models/responses.go b/internal/models/responses.go index 0d42a8f..948926b 100644 --- a/internal/models/responses.go +++ b/internal/models/responses.go @@ -6,10 +6,10 @@ type AvailableEnvironmentResponse struct { Icon string `json:"icon"` Category string `json:"category,omitempty"` // TTL info - TTL int `json:"ttl"` // Initial TTL in seconds (-1 for unlimited) - ExtendTTL int `json:"extendTTL,omitempty"` // Seconds added per extension (0 = use default) - MaxExtends int `json:"maxExtends,omitempty"` // Max extensions allowed (0 = unlimited) - IsUnlimited bool `json:"isUnlimited"` // True if TTL is unlimited + TTL int `json:"ttl"` // Initial TTL in seconds (-1 for unlimited) + ExtendTTL int `json:"extendTTL,omitempty"` // Seconds added per extension (0 = use default) + MaxExtends int `json:"maxExtends,omitempty"` // Max extensions allowed (0 = unlimited) + IsUnlimited bool `json:"isUnlimited"` // True if TTL is unlimited } type UserEnvironmentResponse struct { diff --git a/internal/operatorconfig/resolver.go b/internal/operatorconfig/resolver.go index 468e831..36e051e 100644 --- a/internal/operatorconfig/resolver.go +++ b/internal/operatorconfig/resolver.go @@ -29,19 +29,19 @@ const ( // Effective is the merged, ready-to-use operator configuration. type Effective struct { - DefaultEngine dployv1alpha1.EngineType - FluxNamespace string - FluxServiceAccount string - FluxInterval time.Duration + DefaultEngine dployv1alpha1.EngineType + FluxNamespace string + FluxServiceAccount string + FluxInterval time.Duration BaseDomain string ConnectionURLTemplate string DefaultConnectionType dployv1alpha1.ConnectionType ConnectionMessageTemplate string TTLSeconds int64 - ExtendSeconds int64 - MaxExtends int - MaxInstancesPerUser int - Values map[string]any + ExtendSeconds int64 + MaxExtends int + MaxInstancesPerUser int + Values map[string]any } // Resolve reads the OperatorConfig named "default" and merges it over the From ebbd794b055b0ec69f61377cf96c56cb4fb750c8 Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Fri, 12 Jun 2026 18:12:13 +0200 Subject: [PATCH 13/14] fix(auth): don't fatal when the IdP already advertises public endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by deploying the PR build to the CTF cluster: the split-horizon AuthURL rebase required the discovery doc's AuthURL to be based on the internal issuer URL, and returned an error otherwise — crashlooping the pod. Dex doesn't derive endpoints from the request Host (the old comment claimed it does): it bakes its configured issuer into every endpoint, so fetching discovery via the internal URL still yields public endpoints, and there is nothing to rebase. AuthURL already on the public base -> leave it (the common Dex case); on the internal base -> rebase; neither -> warn and keep it, instead of refusing to boot a config the IdP considers valid. --- internal/auth/oidc.go | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 6dddd80..e7ff870 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -73,19 +73,26 @@ func NewOIDCHandler(cfg *config.Config) (*OIDCHandler, error) { endpoint := provider.Endpoint() if splitHorizon { - // Dex derives the discovery doc's endpoints from the request Host - // header, so fetching via the internal URL gives us internal - // endpoints. Backend code exchange + JWKS stay internal (fine); - // only the browser-facing AuthURL must be rebased so users land on - // the public IdP URL. + // What the discovery doc advertises depends on the IdP. Dex bakes + // its configured `issuer` into every endpoint, so fetching the doc + // via the internal URL still yields *public* endpoints — nothing to + // rebase (the backend reaches the public token endpoint via + // hostAliases/DNS). IdPs that derive endpoints from the request + // Host instead return internal URLs; for those, the browser-facing + // AuthURL must be rebased onto the public host. internalBase := extractBaseURL(cfg.OIDCIssuer) publicBase := extractBaseURL(cfg.OIDCPublicIssuer) - if !strings.Contains(endpoint.AuthURL, internalBase) { - return nil, fmt.Errorf("OIDC AuthURL %q does not contain expected internal base %q; check OIDCIssuer / IdP config", endpoint.AuthURL, internalBase) + switch { + case strings.HasPrefix(endpoint.AuthURL, publicBase): + logger.Info("OIDC auth endpoint already public, no rebase needed", "authURL", endpoint.AuthURL) + case strings.HasPrefix(endpoint.AuthURL, internalBase): + endpoint.AuthURL = strings.Replace(endpoint.AuthURL, internalBase, publicBase, 1) + logger.Info("OIDC auth endpoint rebased to public", + "internal", internalBase, "public", publicBase, "authURL", endpoint.AuthURL) + default: + logger.Warn("OIDC auth endpoint matches neither issuer base; leaving as-is", + "authURL", endpoint.AuthURL, "internal", internalBase, "public", publicBase) } - endpoint.AuthURL = strings.Replace(endpoint.AuthURL, internalBase, publicBase, 1) - logger.Info("OIDC auth endpoint rebased to public", - "internal", internalBase, "public", publicBase, "authURL", endpoint.AuthURL) } h := &OIDCHandler{ From 84af5fb86a378182eacc79d6d084e7e54a8354be Mon Sep 17 00:00:00 2001 From: AYDEV-FR Date: Sat, 20 Jun 2026 18:51:10 +0200 Subject: [PATCH 14/14] fix(auth): close encoded-backslash open-redirect + tidy callback findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the latest Copilot review on PR #39: - sanitizeRelativePath: reject a backslash in the decoded path. A percent-encoded backslash ("/%5cevil.com") survived the literal "/\\" prefix check, and user agents that normalize "\" to "/" would read the decoded "//evil.com" as a protocol-relative redirect. - Callback: return 400 (not 502) on nonce mismatch — a client/session issue or attack, not an upstream failure, mirroring the state path. - Fix the package comment: the flow now carries four flow cookies (state/verifier/nonce/returnUrl), not three. - Pin the bypass with percent-encoded backslash test cases. Claude-Session: https://claude.ai/code/session_01M4Dd5oASBGHwr6ts6wTnYn --- internal/auth/oidc.go | 13 +++++++++++-- internal/auth/oidc_test.go | 3 +++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index e7ff870..cc8137e 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -5,7 +5,7 @@ // the canonical Go OIDC pattern used by Kubernetes, Argo CD and the official // coreos/go-oidc README example: go-oidc for discovery + ID token // verification, golang.org/x/oauth2 for the Authorization Code + PKCE flow, -// and three short-lived HttpOnly cookies to carry state/verifier/returnUrl +// and four short-lived HttpOnly cookies to carry state/verifier/nonce/returnUrl // across the browser bounce. No framework, no signed-cookie key management, // nothing dploy-specific beyond the optional split-horizon issuer support // and the SPA's "#token=..." hand-off. @@ -162,6 +162,13 @@ func sanitizeRelativePath(s string) (string, bool) { if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil { return "", false } + // A percent-encoded backslash (e.g. "/%5cevil.com") survives the literal + // "/\\" prefix check above but decodes into u.Path; user agents that + // normalize "\" to "/" would then read "//evil.com" as protocol-relative. + // Reject any backslash in the decoded path to close that bypass. + if strings.Contains(u.Path, "\\") { + return "", false + } u.Fragment = "" return u.String(), true } @@ -293,7 +300,9 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error { // token carrying the attacker's nonce, not ours. Constant-time compare // because the nonce stays a one-shot secret until the cookie clears. if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(nonce)) != 1 { - return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "nonce mismatch"}) + // Client-side/session issue or an attack, not an upstream failure — + // mirror the state-mismatch path with a 400 rather than a 502. + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "nonce mismatch"}) } // Re-sanitize the returnUrl from the cookie as defense-in-depth, even diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index c0456e5..f920e8b 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -23,6 +23,9 @@ func TestSanitizeRelativePath(t *testing.T) { // classic open-redirect tricks — all rejected {"//evil.com/x", "", false}, // protocol-relative URL {"/\\evil.com/x", "", false}, // backslash-prefixed (some browsers) + {"/%5cevil.com/x", "", false}, // percent-encoded backslash + {"/%5Cevil.com/x", "", false}, // percent-encoded backslash (upper) + {"/foo/%5cbar", "", false}, // encoded backslash mid-path {"http://evil.com/x", "", false}, // absolute URL {"https://evil.com/x", "", false}, // absolute URL https {"javascript:alert(1)", "", false},