public/headscale
1
0
Fork 0
mirror of https://github.com/juanfont/headscale.git synced 2026-09-10 10:41:32 +09:00
Code Issues Packages Projects Releases Wiki Activity

oidc: harden reloadable confirmation flow

Browse Source 
Updates #3365
This commit is contained in:
Kristoffer Dalby
2026-09-04 13:29:59 +00:00
parent 6d377b5348
commit 10dd38fcef
5 changed files with 430 additions and 363 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hscontrol/handlers.go
+15 -8
View File
@@ -40,18 +40,23 @@ func httpError(w http.ResponseWriter, err error) {
// an actionable message derived from the HTTP status code.
func httpUserError(w http.ResponseWriter, err error) {
code := http.StatusInternalServerError
userMsg := ""
if herr, ok := errors.AsType[HTTPError](err); ok {
if herr.Code != 0 {
code = herr.Code
}
userMsg = herr.UserMsg
log.Error().Err(herr.Err).Int("code", code).Msgf("user msg: %s", herr.Msg)
} else {
log.Error().Err(err).Int("code", code).Msg("http internal server error")
}
userMsg := userMessageForStatusCode(code)
if userMsg == "" {
userMsg = userMessageForStatusCode(code)
}
w.Header().Set("Content-Type", "text/html; charset=utf-8")
w.WriteHeader(code)
@@ -73,10 +78,7 @@ func userMessageForStatusCode(code int) string {
case code == http.StatusUnauthorized || code == http.StatusForbidden:
return "You are not authorized. Please contact your administrator."
case code == http.StatusGone:
// Overwhelmingly a reload or a back button on a link the user
// already used, not a failure, so lead with that.
return "This link has already been used or has expired. " +
"If your device is connected you are done; otherwise start the login again."
return "Your session has expired. Please try again."
case code >= 400 && code < 500:
return "The request could not be processed. Please try again."
default:
@@ -86,9 +88,10 @@ func userMessageForStatusCode(code int) string {
// HTTPError represents an error that is surfaced to the user via web.
type HTTPError struct {
Code int // HTTP response code to send to client; 0 means 500
Msg string // Response body to send to client
Err error // Detailed error to log on the server
Code int // HTTP response code to send to client; 0 means 500
Msg string // Response body to send to non-browser clients
Err error // Detailed error to log on the server
UserMsg string // Optional safe message for browser-facing error pages
}
func (e HTTPError) Error() string { return fmt.Sprintf("http error[%d]: %s, %s", e.Code, e.Msg, e.Err) }
@@ -99,6 +102,10 @@ func NewHTTPError(code int, msg string, err error) HTTPError {
return HTTPError{Code: code, Msg: msg, Err: err}
}
func newHTTPUserError(code int, msg, userMsg string, err error) HTTPError {
return HTTPError{Code: code, Msg: msg, Err: err, UserMsg: userMsg}
}
var errMethodNotAllowed = NewHTTPError(http.StatusMethodNotAllowed, "method not allowed", nil)
var ErrRegisterMethodCLIDoesNotSupportExpire = errors.New(
hscontrol/handlers_test.go
+13 -1
View File
@@ -202,9 +202,21 @@ func TestHttpUserError(t *testing.T) {
name: "gone_renders_session_expired",
err: NewHTTPError(http.StatusGone, "login session expired, try again", nil),
wantCode: http.StatusGone,
wantContains: "This link has already been used or has expired.",
wantContains: "Your session has expired. Please try again.",
wantNotContain: "login session expired",
},
{
name: "gone_with_user_message_renders_specific_guidance",
err: newHTTPUserError(
http.StatusGone,
"registration link already used or expired",
"This link has already been used or has expired.",
nil,
),
wantCode: http.StatusGone,
wantContains: "This link has already been used or has expired.",
wantNotContain: "registration link already used or expired",
},
{
name: "bad_request_renders_generic_retry",
err: NewHTTPError(http.StatusBadRequest, "state not found", nil),
hscontrol/oidc.go
+44 -14
View File
@@ -97,7 +97,7 @@ func NewAuthProviderOIDC(
ClientID: cfg.ClientID,
ClientSecret: cfg.ClientSecret,
Endpoint: oidcProvider.Endpoint(),
RedirectURL: strings.TrimSuffix(serverURL, "/") + "/oidc/callback",
RedirectURL: oidcCallbackURL(serverURL),
Scopes: cfg.Scope,
}
@@ -127,6 +127,18 @@ func (a *AuthProviderOIDC) cookiesSecure() bool {
return strings.HasPrefix(a.serverURL, "https://")
}
func oidcCallbackURL(serverURL string) string {
return strings.TrimSuffix(serverURL, "/") + "/oidc/callback"
}
func (a *AuthProviderOIDC) oidcCallbackPath() string {
if u, err := url.Parse(oidcCallbackURL(a.serverURL)); err == nil { //nolint:noinlineerr
return u.Path
}
return "/oidc/callback"
}
func (a *AuthProviderOIDC) AuthURL(authID types.AuthID) string {
return authPathURL(a.serverURL, "auth", authID)
}
@@ -166,10 +178,10 @@ func (a *AuthProviderOIDC) authHandler(
}
// Set the state and nonce cookies to protect against CSRF attacks
state := setCSRFCookie(writer, req, "state", a.cookiesSecure())
state := a.setCSRFCookie(writer, req, "state")
// Set the state and nonce cookies to protect against CSRF attacks
nonce := setCSRFCookie(writer, req, "nonce", a.cookiesSecure())
nonce := a.setCSRFCookie(writer, req, "nonce")
registrationInfo := AuthInfo{
AuthID: authID,
@@ -275,8 +287,8 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler(
// 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)
a.clearOIDCCallbackCookie(writer, stateCookieName)
a.clearOIDCCallbackCookie(writer, nonceCookieName)
nodeExpiry := a.determineNodeExpiry(idToken.Expiry)
@@ -674,7 +686,15 @@ const registerConfirmCSRFCookie = "headscale_register_confirm"
// from [userMessageForStatusCode].
const registrationLinkSpentMsg = "registration link already used or expired"
var errRegistrationLinkSpent = NewHTTPError(http.StatusGone, registrationLinkSpentMsg, nil)
const registrationLinkSpentUserMsg = "This link has already been used or has expired. " +
"If your device is connected you are done; otherwise start the login again."
var errRegistrationLinkSpent = newHTTPUserError(
http.StatusGone,
registrationLinkSpentMsg,
registrationLinkSpentUserMsg,
nil,
)
// registerConfirmURL is the browser-facing URL of the confirmation page.
// It is built from server_url, like [AuthProviderOIDC.RegisterURL] and the
@@ -930,7 +950,12 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
newNode, err := a.handleRegistration(user, authID, pending.NodeExpiry)
if err != nil {
if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) {
httpUserError(writer, NewHTTPError(http.StatusGone, registrationLinkSpentMsg, err))
httpUserError(writer, newHTTPUserError(
http.StatusGone,
registrationLinkSpentMsg,
registrationLinkSpentUserMsg,
err,
))
return
}
@@ -1037,27 +1062,32 @@ func getCookieName(baseName, value string) string {
return fmt.Sprintf("%s_%s", baseName, value[:n])
}
// 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) {
// clearOIDCCallbackCookie expires an OIDC callback cookie by name. Matching
// the browser-facing path the cookie was set with is required for the browser
// to drop it.
func (a *AuthProviderOIDC) 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",
Path: a.oidcCallbackPath(),
MaxAge: -1,
})
}
func setCSRFCookie(w http.ResponseWriter, r *http.Request, name string, secure bool) string {
func (a *AuthProviderOIDC) setCSRFCookie(
w http.ResponseWriter,
r *http.Request,
name string,
) 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",
Path: a.oidcCallbackPath(),
Name: getCookieName(name, val),
Value: val,
MaxAge: int(time.Hour.Seconds()),
Secure: secure || r.TLS != nil,
Secure: a.cookiesSecure() || 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
hscontrol/oidc_callback_reload_test.go
-336
View File
@@ -1,336 +0,0 @@
package hscontrol
import (
"context"
"io"
"net/http"
"net/http/cookiejar"
"net/http/httptest"
"net/url"
"regexp"
"strings"
"testing"
"github.com/juanfont/headscale/hscontrol/types"
"github.com/oauth2-proxy/mockoidc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"tailscale.com/types/key"
)
// oidcBrowser drives the interactive OIDC registration flow the way a
// browser does: over real HTTP against the real route table, through a
// cookie jar that honours Path and expiry, following redirects.
//
// Both halves matter for this bug. The route table is where the
// confirmation page's missing GET route lives, and cookie Path decides
// whether the state cookie the callback deletes is actually gone on the
// next hit.
type oidcBrowser struct {
app *Headscale
idp *mockoidc.MockOIDC
srv *httptest.Server
client *http.Client
}
func newOIDCBrowser(t *testing.T) *oidcBrowser {
t.Helper()
idp, err := mockoidc.Run()
require.NoError(t, err)
t.Cleanup(func() {
_ = idp.Shutdown()
})
app := createTestApp(t)
// The provider derives its OIDC redirect_uri from the server URL, and
// the server serves the router the provider is registered on, so bind
// the router late to break the cycle.
var router http.Handler
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
router.ServeHTTP(w, r)
}))
t.Cleanup(srv.Close)
provider, err := NewAuthProviderOIDC(
context.Background(),
app,
srv.URL,
&types.OIDCConfig{
Issuer: idp.Issuer(),
ClientID: idp.ClientID,
ClientSecret: idp.ClientSecret,
Scope: []string{"openid", "profile", "email"},
},
)
require.NoError(t, err)
app.authProvider = provider
router = app.createRouter(nil, nil)
jar, err := cookiejar.New(nil)
require.NoError(t, err)
return &oidcBrowser{
app: app,
idp: idp,
srv: srv,
client: &http.Client{Jar: jar},
}
}
// get fetches a URL, follows redirects, and returns the status, the URL
// the browser ended up on, and the page body.
func (b *oidcBrowser) get(t *testing.T, rawURL string) (int, *url.URL, string) {
t.Helper()
resp, err := b.client.Get(rawURL) //nolint:noctx,bodyclose // test client; closed below
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
return resp.StatusCode, resp.Request.URL, string(body)
}
// pendingNode mints a pending node registration and returns the URL the
// tailscale client would print for the user to open.
func (b *oidcBrowser) pendingNode(t *testing.T) (types.AuthID, string) {
t.Helper()
authID := types.MustAuthID()
b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{
MachineKey: key.NewMachine().Public(),
NodeKey: key.NewNode().Public(),
Hostname: "reload-victim",
}))
return authID, b.srv.URL + "/register/" + authID.String()
}
var (
csrfInputRe = regexp.MustCompile(
`name="` + registerConfirmCSRFCookie + `"[^>]*value="([^"]+)"`,
)
formActionRe = regexp.MustCompile(`action="([^"]+)"`)
)
// TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL reproduces
// https://github.com/juanfont/headscale/issues/3365.
//
// The interactive OIDC flow ends with the browser sitting on the
// confirmation interstitial, waiting for the user to click. Today that
// interstitial is written as the body of the /oidc/callback response, so
// the URL the browser is parked on is the one carrying the single-use
// OAuth authorization code. Reloading it re-enters the callback, which
// has already spent the code and deleted the state cookie, and the error
// page paints over the interstitial. The node is never registered.
//
// Adblock Plus is only the loudest trigger — it calls
// window.location.reload() to apply element-hiding filters. The back
// button, mobile pull-to-refresh and browser prerendering all aim at the
// same URL.
//
// The property asserted here is the one that fixes the whole class,
// stated without naming an implementation: wherever the flow leaves the
// browser, that URL must be free of the authorization code and safe to
// load again.
func TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL(t *testing.T) {
b := newOIDCBrowser(t)
_, registerURL := b.pendingNode(t)
status, landed, body := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status, "the login flow must reach a page")
require.Contains(t, body, "Confirm node registration",
"the flow must end on the confirmation interstitial")
assert.Empty(t, landed.Query().Get("code"),
"the browser must not be left parked on the URL carrying the one-time "+
"OAuth code; any reload of it re-enters the spent callback")
reloadedStatus, _, reloadedBody := b.get(t, landed.String())
require.Equal(t, http.StatusOK, reloadedStatus,
"reloading the page the flow left the browser on must re-render it")
assert.Contains(t, reloadedBody, "Confirm node registration",
"the reload must show the confirmation interstitial, not an error page")
}
// TestOIDCLoginCompletesAfterReload is the reporters' scenario end to
// end: the confirmation page is reloaded before the user clicks, and the
// registration must still complete. One deployment measured login
// completion falling from 100% to 61-73% across this exact step.
func TestOIDCLoginCompletesAfterReload(t *testing.T) {
b := newOIDCBrowser(t)
_, registerURL := b.pendingNode(t)
status, landed, _ := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status)
// The spurious reload, on whatever URL the flow parked the browser on.
reloadedStatus, reloadedURL, body := b.get(t, landed.String())
require.Equal(t, http.StatusOK, reloadedStatus,
"the page the user is sitting on must survive a reload")
csrf := csrfInputRe.FindStringSubmatch(body)
require.Len(t, csrf, 2, "the reloaded page must still carry a usable confirm form")
action := formActionRe.FindStringSubmatch(body)
require.Len(t, action, 2, "the reloaded page must still carry a form action")
confirmURL, err := reloadedURL.Parse(action[1])
require.NoError(t, err)
//nolint:noctx,bodyclose // test client; closed below
confirmed, err := b.client.PostForm(confirmURL.String(), url.Values{
registerConfirmCSRFCookie: {csrf[1]},
})
require.NoError(t, err)
defer confirmed.Body.Close()
confirmedBody, err := io.ReadAll(confirmed.Body)
require.NoError(t, err)
require.Equal(t, http.StatusOK, confirmed.StatusCode,
"confirming after a reload must register the node")
assert.Contains(t, strings.ToLower(string(confirmedBody)), "registered",
"the user must get the registration success page")
}
// TestRegisterConfirmGETIsNotADeadEnd covers the second, independent way
// a registration is lost, reported with no ad blocker involved: the
// confirmation endpoint is POST-only in the route table, so a user who
// refreshes or navigates back to it gets a bare 405 from the router with
// no way to recover, while the pending registration sits in the cache
// unreachable until it expires.
//
// This is a route-table gap, so it can only be observed through the real
// router — calling the handler directly cannot see it.
func TestRegisterConfirmGETIsNotADeadEnd(t *testing.T) {
b := newOIDCBrowser(t)
authID, registerURL := b.pendingNode(t)
// Complete the OIDC leg so there is a pending confirmation to render.
status, _, _ := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status)
confirmStatus, _, body := b.get(t, b.srv.URL+"/register/confirm/"+authID.String())
require.NotEqual(t, http.StatusMethodNotAllowed, confirmStatus,
"GET on the confirmation URL must not be a dead end for a user who "+
"refreshes or goes back")
require.Equal(t, http.StatusOK, confirmStatus,
"the confirmation page must be reachable by GET")
assert.Contains(t, body, "Confirm node registration")
}
// TestRegisterConfirmNeedsTheCallbackCookie locks the reason the
// confirmation step exists. The node being registered knows its own auth
// ID, so the auth ID alone must never be enough to view the device
// details or to finalise the registration — only the browser that
// completed the OIDC login holds the cookie the callback set, and holding
// it is what authorises the confirm.
//
// Without this, an attacker could hand a victim a /register/{auth_id}
// link for the attacker's own node, let the victim's IdP silently sign
// in, and then confirm the registration themselves under the victim's
// identity.
func TestRegisterConfirmNeedsTheCallbackCookie(t *testing.T) {
b := newOIDCBrowser(t)
authID, registerURL := b.pendingNode(t)
status, _, body := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status)
csrf := csrfInputRe.FindStringSubmatch(body)
require.Len(t, csrf, 2)
// A second browser that knows the auth ID, and even the token from the
// rendered page, but never completed the OIDC login.
jar, err := cookiejar.New(nil)
require.NoError(t, err)
attacker := &http.Client{Jar: jar}
confirmURL := b.srv.URL + "/register/confirm/" + authID.String()
//nolint:noctx,bodyclose // test client; closed below
viewed, err := attacker.Get(confirmURL)
require.NoError(t, err)
defer viewed.Body.Close()
assert.Equal(t, http.StatusForbidden, viewed.StatusCode,
"the confirmation page must not render without the callback cookie")
//nolint:noctx,bodyclose // test client; closed below
submitted, err := attacker.PostForm(confirmURL, url.Values{
registerConfirmCSRFCookie: {csrf[1]},
})
require.NoError(t, err)
defer submitted.Body.Close()
assert.Equal(t, http.StatusForbidden, submitted.StatusCode,
"the registration must not finalise without the callback cookie")
cached, ok := b.app.state.GetAuthCacheEntry(authID)
require.True(t, ok, "the pending registration must survive the attempt")
assert.NotNil(t, cached.PendingConfirmation(),
"the pending registration must still be waiting for the real user")
}
// TestSetRegisterConfirmCookieSameSite pins SameSite=Lax. Strict is
// withheld by browsers that evaluate the whole redirect chain, and this
// cookie now has to survive the callback's redirect to the confirmation
// page — a chain that begins cross-site at the identity provider. Lax is
// still never attached to a cross-site POST, so the confirm submission
// keeps its protection.
func TestSetRegisterConfirmCookieSameSite(t *testing.T) {
a := &AuthProviderOIDC{serverURL: "https://hs.example.com"}
authID := types.MustAuthID()
rec := httptest.NewRecorder()
a.setRegisterConfirmCookie(rec,
httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil),
authID, "token", 900)
cookies := rec.Result().Cookies()
require.Len(t, cookies, 1)
assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite,
"the confirm cookie must survive the callback redirect")
assert.True(t, cookies[0].Secure, "https server_url must set Secure")
assert.Equal(t, "/register/confirm/"+authID.String(), cookies[0].Path)
}
// TestRegisterConfirmURLFollowsServerURLPrefix covers the deployment
// where a reverse proxy serves Headscale under a path prefix. The
// redirect target, the form action and the cookie scope are all seen by
// the browser, so they carry the prefix even though the routed path does
// not.
func TestRegisterConfirmURLFollowsServerURLPrefix(t *testing.T) {
a := &AuthProviderOIDC{serverURL: "https://example.com/hs"}
authID := types.MustAuthID()
assert.Equal(t, "https://example.com/hs/register/confirm/"+authID.String(),
a.registerConfirmURL(authID))
rec := httptest.NewRecorder()
a.setRegisterConfirmCookie(rec,
httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil),
authID, "token", 900)
cookies := rec.Result().Cookies()
require.Len(t, cookies, 1)
assert.Equal(t, "/hs/register/confirm/"+authID.String(), cookies[0].Path,
"the cookie must be scoped to the path the browser sees")
}
hscontrol/oidc_test.go
+358 -4
View File
@@ -1,15 +1,22 @@
package hscontrol
import (
"context"
"io"
"net/http"
"net/http/cookiejar"
"net/http/httptest"
"net/url"
"regexp"
"testing"
"time"
"github.com/hashicorp/golang-lru/v2/expirable"
"github.com/juanfont/headscale/hscontrol/types"
"github.com/oauth2-proxy/mockoidc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"tailscale.com/types/key"
)
func TestDoOIDCAuthorization(t *testing.T) {
@@ -185,10 +192,11 @@ func TestDoOIDCAuthorization(t *testing.T) {
// previously set no SameSite (despite a comment claiming it did), leaving
// browsers that do not default to Lax sending it on cross-site requests.
func TestSetCSRFCookieSameSite(t *testing.T) {
a := &AuthProviderOIDC{serverURL: "http://hs.example.com"}
w := httptest.NewRecorder()
r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil)
setCSRFCookie(w, r, "state", false)
a.setCSRFCookie(w, r, "state")
cookies := w.Result().Cookies()
require.Len(t, cookies, 1)
@@ -233,12 +241,14 @@ func TestGetAuthInfoFromStateSingleUse(t *testing.T) {
// 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) {
a := &AuthProviderOIDC{serverURL: "https://hs.example.com/prefix"}
w := httptest.NewRecorder()
clearOIDCCallbackCookie(w, "state_abcdef")
a.clearOIDCCallbackCookie(w, "state_abcdef")
cookies := w.Result().Cookies()
require.Len(t, cookies, 1)
assert.Equal(t, "state_abcdef", cookies[0].Name)
assert.Equal(t, "/prefix/oidc/callback", cookies[0].Path)
assert.Negative(t, cookies[0].MaxAge, "deletion cookie must have negative MaxAge")
}
@@ -250,14 +260,358 @@ func TestSetCSRFCookieSecure(t *testing.T) {
r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil)
secureRec := httptest.NewRecorder()
setCSRFCookie(secureRec, r, "state", true)
secureProvider := &AuthProviderOIDC{serverURL: "https://hs.example.com"}
secureProvider.setCSRFCookie(secureRec, r, "state")
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)
plainProvider := &AuthProviderOIDC{serverURL: "http://hs.example.com"}
plainProvider.setCSRFCookie(plainRec, r, "state")
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")
}
// oidcBrowser drives the interactive OIDC registration flow the way a
// browser does: over real HTTP against the real route table, through a
// cookie jar that honours Path and expiry, following redirects.
//
// Both halves matter for this bug. The route table is where the
// confirmation page's missing GET route lives, and cookie Path decides
// whether the state cookie the callback deletes is actually gone on the
// next hit.
type oidcBrowser struct {
app *Headscale
idp *mockoidc.MockOIDC
srv *httptest.Server
publicURL string
client *http.Client
}
func newOIDCBrowser(t *testing.T) *oidcBrowser {
t.Helper()
return newOIDCBrowserWithPrefix(t, "")
}
func newOIDCBrowserWithPrefix(t *testing.T, prefix string) *oidcBrowser {
t.Helper()
idp, err := mockoidc.Run()
require.NoError(t, err)
t.Cleanup(func() {
_ = idp.Shutdown()
})
app := createTestApp(t)
// The provider derives its OIDC redirect_uri from the server URL, and
// the server serves the router the provider is registered on, so bind
// the router late to break the cycle.
var router http.Handler
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if prefix == "" {
router.ServeHTTP(w, r)
return
}
http.StripPrefix(prefix, router).ServeHTTP(w, r)
}))
t.Cleanup(srv.Close)
publicURL := srv.URL + prefix
provider, err := NewAuthProviderOIDC(
context.Background(),
app,
publicURL,
&types.OIDCConfig{
Issuer: idp.Issuer(),
ClientID: idp.ClientID,
ClientSecret: idp.ClientSecret,
Scope: []string{"openid", "profile", "email"},
},
)
require.NoError(t, err)
app.authProvider = provider
router = app.createRouter(nil, nil)
jar, err := cookiejar.New(nil)
require.NoError(t, err)
return &oidcBrowser{
app: app,
idp: idp,
srv: srv,
publicURL: publicURL,
client: &http.Client{Jar: jar},
}
}
// get fetches a URL, follows redirects, and returns the status, the URL
// the browser ended up on, and the page body.
func (b *oidcBrowser) get(t *testing.T, rawURL string) (int, *url.URL, string) {
t.Helper()
resp, err := b.client.Get(rawURL) //nolint:noctx,bodyclose // test client; closed below
require.NoError(t, err)
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
return resp.StatusCode, resp.Request.URL, string(body)
}
// pendingNode mints a pending node registration and returns the URL the
// tailscale client would print for the user to open.
func (b *oidcBrowser) pendingNode(t *testing.T) (types.AuthID, string) {
t.Helper()
authID := types.MustAuthID()
b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{
MachineKey: key.NewMachine().Public(),
NodeKey: key.NewNode().Public(),
Hostname: "reload-victim",
}))
return authID, b.publicURL + "/register/" + authID.String()
}
var (
csrfInputRe = regexp.MustCompile(
`name="` + registerConfirmCSRFCookie + `"[^>]*value="([^"]+)"`,
)
formActionRe = regexp.MustCompile(`action="([^"]+)"`)
)
// TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL reproduces
// https://github.com/juanfont/headscale/issues/3365.
//
// Before this regression was fixed, the interactive OIDC flow wrote the
// confirmation interstitial as the body of the /oidc/callback response.
// The browser was therefore parked on the URL carrying the single-use
// OAuth authorization code. Reloading it re-entered the callback, which
// had already spent the code and deleted the state cookie, and the error
// page painted over the interstitial. The node was never registered.
//
// Adblock Plus is only the loudest trigger — it calls
// window.location.reload() to apply element-hiding filters. The back
// button, mobile pull-to-refresh and browser prerendering all aim at the
// same URL.
//
// The property asserted here is the one that fixes the whole class,
// stated without naming an implementation: wherever the flow leaves the
// browser, that URL must be free of the authorization code and safe to
// load again.
func TestOIDCLoginDoesNotParkTheBrowserOnTheCodeURL(t *testing.T) {
b := newOIDCBrowser(t)
_, registerURL := b.pendingNode(t)
status, landed, body := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status, "the login flow must reach a page")
require.Contains(t, body, "Confirm node registration",
"the flow must end on the confirmation interstitial")
assert.Empty(t, landed.Query().Get("code"),
"the browser must not be left parked on the URL carrying the one-time "+
"OAuth code; any reload of it re-enters the spent callback")
reloadedStatus, _, reloadedBody := b.get(t, landed.String())
require.Equal(t, http.StatusOK, reloadedStatus,
"reloading the page the flow left the browser on must re-render it")
assert.Contains(t, reloadedBody, "Confirm node registration",
"the reload must show the confirmation interstitial, not an error page")
}
// TestOIDCLoginCompletesAfterReload is the reporters' scenario end to
// end: the confirmation page is reloaded before the user clicks, and the
// registration must still complete. One deployment measured login
// completion falling from 100% to 61-73% across this exact step.
func TestOIDCLoginCompletesAfterReload(t *testing.T) {
b := newOIDCBrowser(t)
authID, registerURL := b.pendingNode(t)
status, landed, _ := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status)
// The spurious reload, on whatever URL the flow parked the browser on.
reloadedStatus, reloadedURL, body := b.get(t, landed.String())
require.Equal(t, http.StatusOK, reloadedStatus,
"the page the user is sitting on must survive a reload")
csrf := csrfInputRe.FindStringSubmatch(body)
require.Len(t, csrf, 2, "the reloaded page must still carry a usable confirm form")
action := formActionRe.FindStringSubmatch(body)
require.Len(t, action, 2, "the reloaded page must still carry a form action")
confirmURL, err := reloadedURL.Parse(action[1])
require.NoError(t, err)
//nolint:noctx,bodyclose // test client; closed below
confirmed, err := b.client.PostForm(confirmURL.String(), url.Values{
registerConfirmCSRFCookie: {csrf[1]},
})
require.NoError(t, err)
defer confirmed.Body.Close()
confirmedBody, err := io.ReadAll(confirmed.Body)
require.NoError(t, err)
require.Equal(t, http.StatusOK, confirmed.StatusCode,
"confirming after a reload must register the node")
assert.Contains(t, string(confirmedBody), "Node registered",
"the user must get the registration success page")
_, cached := b.app.state.GetAuthCacheEntry(authID)
assert.False(t, cached, "a completed registration must consume the auth session")
assert.True(t, b.app.state.ListNodes().ContainsFunc(func(node types.NodeView) bool {
return node.Hostname() == "reload-victim"
}), "the pending node must be persisted after confirmation")
}
// TestRegisterConfirmGETIsNotADeadEnd covers the second, independent way
// a registration is lost, reported with no ad blocker involved: the
// confirmation endpoint used to be POST-only in the route table, so a
// user who refreshed or navigated back to it got a bare 405 from the
// router with no way to recover, while the pending registration sat in
// the cache unreachable until it expired.
//
// This is a route-table gap, so it can only be observed through the real
// router — calling the handler directly cannot see it.
func TestRegisterConfirmGETIsNotADeadEnd(t *testing.T) {
b := newOIDCBrowser(t)
authID, registerURL := b.pendingNode(t)
// Complete the OIDC leg so there is a pending confirmation to render.
status, _, _ := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status)
confirmStatus, _, body := b.get(t, b.srv.URL+"/register/confirm/"+authID.String())
require.NotEqual(t, http.StatusMethodNotAllowed, confirmStatus,
"GET on the confirmation URL must not be a dead end for a user who "+
"refreshes or goes back")
require.Equal(t, http.StatusOK, confirmStatus,
"the confirmation page must be reachable by GET")
assert.Contains(t, body, "Confirm node registration")
}
// TestRegisterConfirmNeedsTheCallbackCookie locks the reason the
// confirmation step exists. The node being registered knows its own auth
// ID, so the auth ID alone must never be enough to view the device
// details or to finalise the registration — only the browser that
// completed the OIDC login holds the cookie the callback set, and holding
// it is what authorises the confirm.
//
// Without this, an attacker could hand a victim a /register/{auth_id}
// link for the attacker's own node, let the victim's IdP silently sign
// in, and then confirm the registration themselves under the victim's
// identity.
func TestRegisterConfirmNeedsTheCallbackCookie(t *testing.T) {
b := newOIDCBrowser(t)
authID, registerURL := b.pendingNode(t)
status, _, body := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status)
csrf := csrfInputRe.FindStringSubmatch(body)
require.Len(t, csrf, 2)
// A second browser that knows the auth ID, and even the token from the
// rendered page, but never completed the OIDC login.
jar, err := cookiejar.New(nil)
require.NoError(t, err)
attacker := &http.Client{Jar: jar}
confirmURL := b.srv.URL + "/register/confirm/" + authID.String()
//nolint:noctx,bodyclose // test client; closed below
viewed, err := attacker.Get(confirmURL)
require.NoError(t, err)
defer viewed.Body.Close()
assert.Equal(t, http.StatusForbidden, viewed.StatusCode,
"the confirmation page must not render without the callback cookie")
//nolint:noctx,bodyclose // test client; closed below
submitted, err := attacker.PostForm(confirmURL, url.Values{
registerConfirmCSRFCookie: {csrf[1]},
})
require.NoError(t, err)
defer submitted.Body.Close()
assert.Equal(t, http.StatusForbidden, submitted.StatusCode,
"the registration must not finalise without the callback cookie")
cached, ok := b.app.state.GetAuthCacheEntry(authID)
require.True(t, ok, "the pending registration must survive the attempt")
assert.NotNil(t, cached.PendingConfirmation(),
"the pending registration must still be waiting for the real user")
}
// TestSetRegisterConfirmCookieSameSite pins SameSite=Lax. Strict is
// withheld by browsers that evaluate the whole redirect chain, and this
// cookie now has to survive the callback's redirect to the confirmation
// page — a chain that begins cross-site at the identity provider. Lax is
// still never attached to a cross-site POST, so the confirm submission
// keeps its protection.
func TestSetRegisterConfirmCookieSameSite(t *testing.T) {
a := &AuthProviderOIDC{serverURL: "https://hs.example.com"}
authID := types.MustAuthID()
rec := httptest.NewRecorder()
a.setRegisterConfirmCookie(rec,
httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/oidc/callback", nil),
authID, "token", 900)
cookies := rec.Result().Cookies()
require.Len(t, cookies, 1)
assert.Equal(t, http.SameSiteLaxMode, cookies[0].SameSite,
"the confirm cookie must survive the callback redirect")
assert.True(t, cookies[0].Secure, "https server_url must set Secure")
assert.Equal(t, "/register/confirm/"+authID.String(), cookies[0].Path)
}
// TestRegisterConfirmURLFollowsServerURLPrefix covers the deployment
// where a reverse proxy serves Headscale under a path prefix. The
// redirect target, the form action and the cookie scope are all seen by
// the browser, so they carry the prefix even though the routed path does
// not.
func TestRegisterConfirmURLFollowsServerURLPrefix(t *testing.T) {
b := newOIDCBrowserWithPrefix(t, "/hs")
authID, registerURL := b.pendingNode(t)
status, landed, body := b.get(t, registerURL)
require.Equal(t, http.StatusOK, status,
"the prefixed callback must receive its state and nonce cookies")
assert.Equal(t, "/hs/register/confirm/"+authID.String(), landed.Path)
assert.Contains(t, body, "Confirm node registration")
}
func TestRouterMethodNotAllowedIncludesAllow(t *testing.T) {
b := newOIDCBrowser(t)
//nolint:noctx,bodyclose // test client; closed below
resp, err := b.client.PostForm(b.srv.URL+"/health", nil)
require.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode)
assert.Equal(t, []string{http.MethodGet}, resp.Header.Values("Allow"))
}