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.
2026-07-08 00:50:20 +09:00 · 2026-06-24 12:45:47 +00:00
parent 3fca03ad61
commit 8b17c26d11
4 changed files with 213 additions and 17 deletions
@@ -85,8 +85,8 @@ func NewAuthProviderOIDC(
 	serverURL string,
 	cfg *types.OIDCConfig,
 ) (*AuthProviderOIDC, error) {
-	var err error
-	// grab oidc config if it hasn't been already
+	// 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)
@@ -117,6 +117,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 authPathURL(a.serverURL, "auth", authID)
 }
@@ -156,10 +165,10 @@ func (a *AuthProviderOIDC) authHandler(
 	}

 	// Set the state and nonce cookies to protect against CSRF attacks
-	state := setCSRFCookie(writer, req, "state")
+	state := setCSRFCookie(writer, req, "state", a.cookiesSecure())

 	// Set the state and nonce cookies to protect against CSRF attacks
-	nonce := setCSRFCookie(writer, req, "nonce")
+	nonce := setCSRFCookie(writer, req, "nonce", a.cookiesSecure())

 	registrationInfo := AuthInfo{
 		AuthID:       authID,
@@ -263,6 +272,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
@@ -589,13 +603,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
 }

@@ -658,14 +676,15 @@ func setRegisterConfirmCookie(
 	authID types.AuthID,
 	value string,
 	maxAge int,
+	secure bool,
 ) {
-	//nolint:gosec // G124: Secure set conditionally via req.TLS; HttpOnly + SameSite already set
+	//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:   req.TLS != nil,
+		Secure:   secure || req.TLS != nil,
 		HttpOnly: true,
 		SameSite: http.SameSiteStrictMode,
 	})
@@ -707,7 +726,7 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
 		CSRF:       csrf,
 	})

-	setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds()))
+	setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds()), a.cookiesSecure())

 	regData := authReq.RegistrationData()

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

 	// Clear the CSRF cookie now that the registration is final.
-	setRegisterConfirmCookie(writer, req, authID, "", -1)
+	setRegisterConfirmCookie(writer, req, authID, "", -1, a.cookiesSecure())

 	content := renderRegistrationSuccessTemplate(user, newNode)

@@ -920,16 +939,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 {
+// 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,
+	})
+}
+
+func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string, secure bool) string {
 	val := rands.HexString(64)

-	//nolint:gosec // G124: Secure set conditionally via r.TLS; HttpOnly + SameSite set below
+	//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
@@ -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,10 +188,76 @@ func TestSetCSRFCookieSameSite(t *testing.T) {
 	w := httptest.NewRecorder()
 	r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil)

-	setCSRFCookie(w, r, "state")
+	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'")
@@ -363,6 +366,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 {
@@ -564,11 +596,10 @@ func validateServerConfig() error {
 	depr.fatalIfSet("oidc.expiry", "node.expiry")

 	// OIDC is activated by setting oidc.issuer (see app.go), not by a
-	// dedicated oidc.enabled key. Gate PKCE method validation on the real
-	// activation condition so an invalid method fails at startup instead of
-	// silently disabling PKCE at runtime.
+	// dedicated oidc.enabled key. Gate validation on the real activation
+	// condition so a misconfiguration fails at startup.
 	if viper.GetString("oidc.issuer") != "" {
-		err := validatePKCEMethod(viper.GetString("oidc.pkce.method"))
+		err := validateOIDCConfig()
 		if err != nil {
 			return err
 		}
@@ -444,6 +444,73 @@ oidc:
 	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