From 8b17c26d110f11be315973132429ea5b28bc83e3 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 24 Jun 2026 12:45:47 +0000 Subject: [PATCH] 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. --- hscontrol/oidc.go | 54 ++++++++++++++++++++------ hscontrol/oidc_test.go | 70 +++++++++++++++++++++++++++++++++- hscontrol/types/config.go | 39 +++++++++++++++++-- hscontrol/types/config_test.go | 67 ++++++++++++++++++++++++++++++++ 4 files changed, 213 insertions(+), 17 deletions(-) diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index d432a052..bd660e4d 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -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 diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index 2c581407..04d33164 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -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") +} diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 4ef0ba34..4f0b4ecf 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -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 } diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index d9acf253..d0859758 100644 --- a/hscontrol/types/config_test.go +++ b/hscontrol/types/config_test.go @@ -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