oidc: harden callback CSRF cookies, state reuse, and issuer config

Defence-in-depth fixes to the OIDC login flow. Set the state/nonce cookie Secure flag from the configured server_url scheme rather than req.TLS, so the cookies stay Secure behind a TLS-terminating reverse proxy where the proxy-to-Headscale hop is plain HTTP. Deriving it from config avoids trusting a spoofable X-Forwarded-Proto header. Make the OIDC state single-use: consume it from the cache on the callback and clear the state/nonce cookies once validated, so a replayed callback cannot resolve the same session and the cookies do not linger until expiry. Bound OIDC discovery to the caller's context so a slow or unreachable issuer fails startup within the timeout instead of hanging, and validate the issuer URL and required client_id/client_secret at config load so an unworkable setup fails fast. (cherry picked from commit 622e08f5e6)
2026-09-26 02:04:53 +09:00 · 2026-06-24 12:45:47 +00:00
parent 1e528f7f52
commit 2b28f5756b
4 changed files with 271 additions and 46 deletions
@@ -20,6 +20,7 @@ import (
 	"github.com/juanfont/headscale/hscontrol/util"
 	"github.com/rs/zerolog/log"
 	"golang.org/x/oauth2"
+	"tailscale.com/util/rands"
 )

 const (
@@ -83,9 +84,9 @@ func NewAuthProviderOIDC(
 	serverURL string,
 	cfg *types.OIDCConfig,
 ) (*AuthProviderOIDC, error) {
-	var err error
-	// grab oidc config if it hasn't been already
-	oidcProvider, err := oidc.NewProvider(context.Background(), cfg.Issuer) //nolint:contextcheck
+	// Use the caller's context (bounded, see app.go) so a slow or unreachable
+	// issuer fails discovery within the timeout instead of hanging startup.
+	oidcProvider, err := oidc.NewProvider(ctx, cfg.Issuer)
 	if err != nil {
 		return nil, fmt.Errorf("creating OIDC provider from issuer config: %w", err)
 	}
@@ -115,6 +116,15 @@ func NewAuthProviderOIDC(
 	}, nil
 }

+// cookiesSecure reports whether the OIDC cookies should carry the Secure flag.
+// It keys off the configured server_url scheme, not req.TLS, so cookies stay
+// Secure behind a TLS-terminating reverse proxy (where the proxy→Headscale hop
+// is plain HTTP and req.TLS is nil). Deriving it from config avoids trusting a
+// spoofable X-Forwarded-Proto header.
+func (a *AuthProviderOIDC) cookiesSecure() bool {
+	return strings.HasPrefix(a.serverURL, "https://")
+}
+
 func (a *AuthProviderOIDC) AuthURL(authID types.AuthID) string {
 	return fmt.Sprintf(
 		"%s/auth/%s",
@@ -160,18 +170,10 @@ func (a *AuthProviderOIDC) authHandler(
 	}

 	// Set the state and nonce cookies to protect against CSRF attacks
-	state, err := setCSRFCookie(writer, req, "state")
-	if err != nil {
-		httpUserError(writer, err)
-		return
-	}
+	state := setCSRFCookie(writer, req, "state", a.cookiesSecure())

 	// Set the state and nonce cookies to protect against CSRF attacks
-	nonce, err := setCSRFCookie(writer, req, "nonce")
-	if err != nil {
-		httpUserError(writer, err)
-		return
-	}
+	nonce := setCSRFCookie(writer, req, "nonce", a.cookiesSecure())

 	registrationInfo := AuthInfo{
 		AuthID:       authID,
@@ -269,6 +271,11 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler(
 		return
 	}

+	// The state/nonce cookies have served their CSRF purpose; clear them so a
+	// single-use pair does not linger in the browser until MaxAge.
+	clearOIDCCallbackCookie(writer, stateCookieName)
+	clearOIDCCallbackCookie(writer, nonceCookieName)
+
 	nodeExpiry := a.determineNodeExpiry(idToken.Expiry)

 	var claims types.OIDCClaims
@@ -597,13 +604,17 @@ func doOIDCAuthorization(
 	return nil
 }

-// getAuthInfoFromState retrieves the registration ID from the state.
+// getAuthInfoFromState retrieves and consumes the auth info for a state. The
+// entry is removed on read so a state is single-use: a replayed callback cannot
+// resolve the same auth session twice, even within the cache TTL.
 func (a *AuthProviderOIDC) getAuthInfoFromState(state string) *AuthInfo {
 	authInfo, ok := a.authCache.Get(state)
 	if !ok {
 		return nil
 	}

+	a.authCache.Remove(state)
+
 	return &authInfo
 }

@@ -657,6 +668,29 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim(
 // browser do not collide.
 const registerConfirmCSRFCookie = "headscale_register_confirm"

+// setRegisterConfirmCookie writes the per-session register-confirm CSRF
+// cookie. Pass the CSRF token and authCacheExpiration seconds to set it;
+// pass ("", -1) to clear it after the registration is finalised.
+func setRegisterConfirmCookie(
+	writer http.ResponseWriter,
+	req *http.Request,
+	authID types.AuthID,
+	value string,
+	maxAge int,
+	secure bool,
+) {
+	//nolint:gosec // G124: Secure from server_url scheme or req.TLS; HttpOnly + SameSite already set
+	http.SetCookie(writer, &http.Cookie{
+		Name:     registerConfirmCSRFCookie,
+		Value:    value,
+		Path:     "/register/confirm/" + authID.String(),
+		MaxAge:   maxAge,
+		Secure:   secure || req.TLS != nil,
+		HttpOnly: true,
+		SameSite: http.SameSiteStrictMode,
+	})
+}
+
 // renderRegistrationConfirmInterstitial captures the resolved OIDC
 // identity and node expiry into the cached [types.AuthRequest], sets the CSRF
 // cookie, and renders the confirmation page that the user must
@@ -698,16 +732,7 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
 		CSRF:       csrf,
 	})

-	//nolint:gosec // G124: Secure set conditionally via req.TLS; HttpOnly + SameSite already set
-	http.SetCookie(writer, &http.Cookie{
-		Name:     registerConfirmCSRFCookie,
-		Value:    csrf,
-		Path:     "/register/confirm/" + authID.String(),
-		MaxAge:   int(authCacheExpiration.Seconds()),
-		Secure:   req.TLS != nil,
-		HttpOnly: true,
-		SameSite: http.SameSiteStrictMode,
-	})
+	setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds()), a.cookiesSecure())

 	regData := authReq.RegistrationData()

@@ -824,16 +849,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
 	}

 	// Clear the CSRF cookie now that the registration is final.
-	//nolint:gosec // G124: Secure set conditionally via req.TLS; HttpOnly + SameSite already set
-	http.SetCookie(writer, &http.Cookie{
-		Name:     registerConfirmCSRFCookie,
-		Value:    "",
-		Path:     "/register/confirm/" + authID.String(),
-		MaxAge:   -1,
-		Secure:   req.TLS != nil,
-		HttpOnly: true,
-		SameSite: http.SameSiteStrictMode,
-	})
+	setRegisterConfirmCookie(writer, req, authID, "", -1, a.cookiesSecure())

 	content := renderRegistrationSuccessTemplate(user, newNode)

@@ -929,19 +945,27 @@ func getCookieName(baseName, value string) string {
 	return fmt.Sprintf("%s_%s", baseName, value[:n])
 }

-func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string) (string, error) {
-	val, err := util.GenerateRandomStringURLSafe(64)
-	if err != nil {
-		return val, err
-	}
+// clearOIDCCallbackCookie expires a /oidc/callback cookie by name. Matching the
+// path the cookie was set with is required for the browser to drop it.
+func clearOIDCCallbackCookie(w http.ResponseWriter, name string) {
+	//nolint:gosec // G124: a deletion cookie (empty value, MaxAge<0); security attributes are moot
+	http.SetCookie(w, &http.Cookie{
+		Name:   name,
+		Path:   "/oidc/callback",
+		MaxAge: -1,
+	})
+}

-	//nolint:gosec // G124: Secure set conditionally via r.TLS; HttpOnly + SameSite set below
+func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string, secure bool) string {
+	val := rands.HexString(64)
+
+	//nolint:gosec // G124: Secure from server_url scheme or req.TLS; HttpOnly + SameSite set below
 	c := &http.Cookie{
 		Path:     "/oidc/callback",
 		Name:     getCookieName(name, val),
 		Value:    val,
 		MaxAge:   int(time.Hour.Seconds()),
-		Secure:   r.TLS != nil,
+		Secure:   secure || r.TLS != nil,
 		HttpOnly: true,
 		// Lax, not Strict: the OIDC callback is a cross-site top-level GET
 		// redirect from the IdP that must still carry this cookie. Strict
@@ -952,5 +976,5 @@ func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string) (string,
 	}
 	http.SetCookie(w, c)

-	return val, nil
+	return val
 }
@@ -4,7 +4,9 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"

+	"github.com/hashicorp/golang-lru/v2/expirable"
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -186,11 +188,76 @@ func TestSetCSRFCookieSameSite(t *testing.T) {
 	w := httptest.NewRecorder()
 	r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil)

-	_, err := setCSRFCookie(w, r, "state")
-	require.NoError(t, err)
+	setCSRFCookie(w, r, "state", false)

 	cookies := w.Result().Cookies()
 	require.Len(t, cookies, 1)
 	assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite,
 		"OIDC CSRF cookie must explicitly set SameSite=Lax")
 }
+
+// TestExtractCodeAndStateParam covers the callback's first trust-boundary
+// checks: both params required, and a too-short state is rejected before
+// getCookieName can slice out of range.
+func TestExtractCodeAndStateParam(t *testing.T) {
+	_, _, err := extractCodeAndStateParamFromRequest(
+		httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil))
+	require.Error(t, err)
+
+	_, _, err = extractCodeAndStateParamFromRequest(
+		httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback?code=c&state=abc", nil))
+	require.ErrorIs(t, err, errOIDCStateTooShort)
+
+	code, state, err := extractCodeAndStateParamFromRequest(
+		httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback?code=c&state=abcdef0123", nil))
+	require.NoError(t, err)
+	assert.Equal(t, "c", code)
+	assert.Equal(t, "abcdef0123", state)
+}
+
+// TestGetAuthInfoFromStateSingleUse asserts a consumed OIDC state cannot be
+// resolved twice, so a replayed callback cannot re-bind the same session.
+func TestGetAuthInfoFromStateSingleUse(t *testing.T) {
+	a := &AuthProviderOIDC{
+		authCache: expirable.NewLRU[string, AuthInfo](16, nil, time.Minute),
+	}
+	a.authCache.Add("state-x", AuthInfo{Registration: true})
+
+	got := a.getAuthInfoFromState("state-x")
+	require.NotNil(t, got)
+	assert.True(t, got.Registration)
+
+	assert.Nil(t, a.getAuthInfoFromState("state-x"), "a consumed state must not resolve again")
+}
+
+// TestClearOIDCCallbackCookie asserts the cookie is expired (negative MaxAge) on
+// the same path it was set with, so the browser drops it.
+func TestClearOIDCCallbackCookie(t *testing.T) {
+	w := httptest.NewRecorder()
+	clearOIDCCallbackCookie(w, "state_abcdef")
+
+	cookies := w.Result().Cookies()
+	require.Len(t, cookies, 1)
+	assert.Equal(t, "state_abcdef", cookies[0].Name)
+	assert.Negative(t, cookies[0].MaxAge, "deletion cookie must have negative MaxAge")
+}
+
+// TestSetCSRFCookieSecure verifies the Secure flag is driven by the secure
+// argument (derived from the configured https server_url), not only req.TLS, so
+// cookies stay Secure behind a TLS-terminating reverse proxy where req.TLS is
+// nil.
+func TestSetCSRFCookieSecure(t *testing.T) {
+	r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil)
+
+	secureRec := httptest.NewRecorder()
+	setCSRFCookie(secureRec, r, "state", true)
+	require.Len(t, secureRec.Result().Cookies(), 1)
+	assert.True(t, secureRec.Result().Cookies()[0].Secure,
+		"https server_url must set Secure even when req.TLS is nil (proxy case)")
+
+	plainRec := httptest.NewRecorder()
+	setCSRFCookie(plainRec, r, "state", false)
+	require.Len(t, plainRec.Result().Cookies(), 1)
+	assert.False(t, plainRec.Result().Cookies()[0].Secure,
+		"plain-http server_url without req.TLS must not set Secure")
+}
@@ -33,6 +33,9 @@ const (

 var (
 	errOidcMutuallyExclusive     = errors.New("oidc_client_secret and oidc_client_secret_path are mutually exclusive")
+	errOIDCIssuerInvalid         = errors.New("oidc.issuer must be a valid http(s) URL")
+	errOIDCClientIDRequired      = errors.New("oidc.client_id is required when oidc.issuer is set")
+	errOIDCClientSecretRequired  = errors.New("oidc.client_secret or oidc.client_secret_path is required when oidc.issuer is set")
 	errServerURLSuffix           = errors.New("server_url cannot be part of base_domain in a way that could make the DERP and headscale server unreachable")
 	errServerURLSame             = errors.New("server_url cannot use the same domain as base_domain in a way that could make the DERP and headscale server unreachable")
 	errInvalidPKCEMethod         = errors.New("pkce.method must be either 'plain' or 'S256'")
@@ -356,6 +359,35 @@ func validatePKCEMethod(method string) error {
 	return nil
 }

+// validateOIDCConfig validates the OIDC settings, called when oidc.issuer is
+// set. It fails fast on a setup that cannot work: an invalid PKCE method, a
+// malformed issuer URL (which would otherwise surface as an opaque discovery
+// error or, worse, resolve to an unintended provider), or a missing client
+// id/secret.
+func validateOIDCConfig() error {
+	err := validatePKCEMethod(viper.GetString("oidc.pkce.method"))
+	if err != nil {
+		return err
+	}
+
+	issuer := viper.GetString("oidc.issuer")
+
+	u, err := url.Parse(issuer)
+	if err != nil || (u.Scheme != "https" && u.Scheme != "http") || u.Host == "" {
+		return fmt.Errorf("%w: got %q", errOIDCIssuerInvalid, issuer)
+	}
+
+	if viper.GetString("oidc.client_id") == "" {
+		return errOIDCClientIDRequired
+	}
+
+	if viper.GetString("oidc.client_secret") == "" && viper.GetString("oidc.client_secret_path") == "" {
+		return errOIDCClientSecretRequired
+	}
+
+	return nil
+}
+
 // Domain returns the hostname/domain part of the [Config.ServerURL].
 // If the [Config.ServerURL] is not a valid URL, it returns the [Config.BaseDomain].
 func (c *Config) Domain() string {
@@ -558,8 +590,11 @@ func validateServerConfig() error {
 	// Removed: oidc.expiry -> node.expiry
 	depr.fatalIfSet("oidc.expiry", "node.expiry")

-	if viper.GetBool("oidc.enabled") {
-		err := validatePKCEMethod(viper.GetString("oidc.pkce.method"))
+	// OIDC is activated by setting oidc.issuer (see app.go), not by a
+	// dedicated oidc.enabled key. Gate validation on the real activation
+	// condition so a misconfiguration fails at startup.
+	if viper.GetString("oidc.issuer") != "" {
+		err := validateOIDCConfig()
 		if err != nil {
 			return err
 		}
@@ -412,6 +412,105 @@ tls_letsencrypt_challenge_type: TLS-ALPN-01
 	require.NoError(t, err)
 }

+func TestPKCEMethodValidation(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	// OIDC is active (issuer set) with PKCE enabled and an invalid method.
+	// There is no oidc.enabled key. validateServerConfig must reject the
+	// invalid method rather than silently skipping the check.
+	configYaml := []byte(`---
+noise:
+  private_key_path: noise_private.key
+server_url: http://127.0.0.1:8080
+dns:
+  override_local_dns: false
+oidc:
+  issuer: https://idp.example.com
+  client_id: headscale
+  pkce:
+    enabled: true
+    method: S256-typo
+`)
+
+	configFilePath := filepath.Join(tmpDir, "config.yaml")
+	err := os.WriteFile(configFilePath, configYaml, 0o600)
+	require.NoError(t, err)
+
+	err = LoadConfig(tmpDir, false)
+	require.NoError(t, err)
+
+	err = validateServerConfig()
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), errInvalidPKCEMethod.Error())
+}
+
+// TestOIDCConfigValidation covers the issuer-URL and required-field checks that
+// fail an unworkable OIDC setup fast at config load.
+func TestOIDCConfigValidation(t *testing.T) {
+	tests := []struct {
+		name      string
+		oidcBlock string
+		wantErr   string
+	}{
+		{
+			name: "non-http issuer",
+			oidcBlock: `
+  issuer: ftp://idp.example.com
+  client_id: headscale
+  client_secret: sekret`,
+			wantErr: "valid http(s) URL",
+		},
+		{
+			name: "missing client_id",
+			oidcBlock: `
+  issuer: https://idp.example.com
+  client_secret: sekret`,
+			wantErr: "client_id is required",
+		},
+		{
+			name: "missing client_secret",
+			oidcBlock: `
+  issuer: https://idp.example.com
+  client_id: headscale`,
+			wantErr: "client_secret",
+		},
+		{
+			name: "valid",
+			oidcBlock: `
+  issuer: https://idp.example.com
+  client_id: headscale
+  client_secret: sekret`,
+			wantErr: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tmpDir := t.TempDir()
+			configYaml := []byte(`---
+noise:
+  private_key_path: noise_private.key
+server_url: http://127.0.0.1:8080
+dns:
+  override_local_dns: false
+oidc:` + tt.oidcBlock + "\n")
+
+			require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "config.yaml"), configYaml, 0o600))
+			require.NoError(t, LoadConfig(tmpDir, false))
+
+			err := validateServerConfig()
+			if tt.wantErr == "" {
+				require.NoError(t, err)
+
+				return
+			}
+
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), tt.wantErr)
+		})
+	}
+}
+
 // OK
 // server_url: headscale.com, base: clients.headscale.com
 // server_url: headscale.com, base: headscale.net