mirror of
https://github.com/juanfont/headscale.git
synced 2026-09-13 03:52:02 +09:00
oidc: serve the registration confirmation page from a reloadable URL
The interstitial was the body of /oidc/callback, the URL carrying the
single-use code, so any reload re-entered the spent exchange. Redirect to
GET /register/confirm/{auth_id}, also missing from the route table.
This commit is contained in:
committed by
Kristoffer Dalby
parent
475d3ae82c
commit
6d377b5348
@@ -478,6 +478,7 @@ func (h *Headscale) createRouter(apiV1Mux, apiV2Mux http.Handler) *chi.Mux {
|
||||
|
||||
if provider, ok := h.authProvider.(*AuthProviderOIDC); ok {
|
||||
r.Get("/oidc/callback", provider.OIDCCallbackHandler)
|
||||
r.Get("/register/confirm/{auth_id}", provider.RegisterConfirmGetHandler)
|
||||
r.Post("/register/confirm/{auth_id}", provider.RegisterConfirmHandler)
|
||||
}
|
||||
|
||||
|
||||
@@ -73,7 +73,10 @@ 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:
|
||||
return "Your session has expired. Please try again."
|
||||
// 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."
|
||||
case code >= 400 && code < 500:
|
||||
return "The request could not be processed. Please try again."
|
||||
default:
|
||||
|
||||
@@ -202,7 +202,7 @@ func TestHttpUserError(t *testing.T) {
|
||||
name: "gone_renders_session_expired",
|
||||
err: NewHTTPError(http.StatusGone, "login session expired, try again", nil),
|
||||
wantCode: http.StatusGone,
|
||||
wantContains: "Your session has expired. Please try again.",
|
||||
wantContains: "This link has already been used or has expired.",
|
||||
wantNotContain: "login session expired",
|
||||
},
|
||||
{
|
||||
|
||||
+121
-23
@@ -7,6 +7,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"slices"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -349,7 +350,7 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler(
|
||||
// /register/{auth_id} could silently complete a registration when
|
||||
// the IdP allows silent SSO.
|
||||
if authInfo.Registration {
|
||||
a.renderRegistrationConfirmInterstitial(writer, req, authInfo.AuthID, user, nodeExpiry)
|
||||
a.beginRegistrationConfirmation(writer, req, authInfo.AuthID, user, nodeExpiry)
|
||||
|
||||
return
|
||||
}
|
||||
@@ -667,34 +668,70 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim(
|
||||
// browser do not collide.
|
||||
const registerConfirmCSRFCookie = "headscale_register_confirm"
|
||||
|
||||
// registrationLinkSpentMsg is logged when a user returns to a
|
||||
// registration link whose session is gone, which is usually a reload or a
|
||||
// back button after they already confirmed. The page the user sees comes
|
||||
// from [userMessageForStatusCode].
|
||||
const registrationLinkSpentMsg = "registration link already used or expired"
|
||||
|
||||
var errRegistrationLinkSpent = NewHTTPError(http.StatusGone, registrationLinkSpentMsg, nil)
|
||||
|
||||
// registerConfirmURL is the browser-facing URL of the confirmation page.
|
||||
// It is built from server_url, like [AuthProviderOIDC.RegisterURL] and the
|
||||
// OIDC redirect URI, so a Headscale that a reverse proxy serves under a
|
||||
// path prefix hands the browser a URL that resolves.
|
||||
func (a *AuthProviderOIDC) registerConfirmURL(authID types.AuthID) string {
|
||||
return authPathURL(a.serverURL, "register/confirm", authID)
|
||||
}
|
||||
|
||||
// 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(
|
||||
func (a *AuthProviderOIDC) setRegisterConfirmCookie(
|
||||
writer http.ResponseWriter,
|
||||
req *http.Request,
|
||||
authID types.AuthID,
|
||||
value string,
|
||||
maxAge int,
|
||||
secure bool,
|
||||
) {
|
||||
// Scope the cookie to the browser-facing path, which carries the
|
||||
// reverse proxy's prefix; the routed path does not.
|
||||
path := "/register/confirm/" + authID.String()
|
||||
if u, err := url.Parse(a.registerConfirmURL(authID)); err == nil { //nolint:noinlineerr
|
||||
path = u.Path
|
||||
}
|
||||
|
||||
//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(),
|
||||
Path: path,
|
||||
MaxAge: maxAge,
|
||||
Secure: secure || req.TLS != nil,
|
||||
Secure: a.cookiesSecure() || req.TLS != nil,
|
||||
HttpOnly: true,
|
||||
SameSite: http.SameSiteStrictMode,
|
||||
// Lax, not Strict: the callback sets this cookie and immediately
|
||||
// redirects to the confirmation page. That hop ends a redirect
|
||||
// chain which began cross-site at the IdP, and Firefox evaluates
|
||||
// the whole chain, so a Strict cookie is withheld and the
|
||||
// confirmation page 403s. Lax still never rides a cross-site
|
||||
// POST, so the confirm submission stays protected.
|
||||
SameSite: http.SameSiteLaxMode,
|
||||
})
|
||||
}
|
||||
|
||||
// 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
|
||||
// explicitly submit before the registration is finalised.
|
||||
func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
|
||||
// beginRegistrationConfirmation captures the resolved OIDC identity and
|
||||
// node expiry into the cached [types.AuthRequest], sets the CSRF cookie, and
|
||||
// redirects the browser to the confirmation page.
|
||||
//
|
||||
// The interstitial is served from its own URL rather than written inline
|
||||
// here, because this request carries the single-use OAuth authorization
|
||||
// code. A page rendered on this response leaves the browser parked on the
|
||||
// code-bearing URL, and anything that reloads it — an extension calling
|
||||
// window.location.reload(), the back button, pull-to-refresh, a prerender
|
||||
// — re-enters the callback with a spent code and paints an error over the
|
||||
// interstitial. Redirecting keeps the code exchange one-shot and makes the
|
||||
// page the user waits on safe to reload.
|
||||
func (a *AuthProviderOIDC) beginRegistrationConfirmation(
|
||||
writer http.ResponseWriter,
|
||||
req *http.Request,
|
||||
authID types.AuthID,
|
||||
@@ -726,14 +763,78 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
|
||||
CSRF: csrf,
|
||||
})
|
||||
|
||||
setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds()), a.cookiesSecure())
|
||||
a.setRegisterConfirmCookie(writer, req, authID, csrf, int(authCacheExpiration.Seconds()))
|
||||
|
||||
// 303 See Other so the browser issues a fresh GET for the
|
||||
// confirmation page and leaves the code-bearing URL behind as a
|
||||
// transient hop rather than a history entry it can return to.
|
||||
http.Redirect(writer, req, a.registerConfirmURL(authID), http.StatusSeeOther)
|
||||
}
|
||||
|
||||
// RegisterConfirmGetHandler renders the OIDC registration confirmation
|
||||
// interstitial. It is reached via the redirect that
|
||||
// [AuthProviderOIDC.beginRegistrationConfirmation] issues from the OIDC
|
||||
// callback, and it is safe to reload: it only reads the pending
|
||||
// confirmation captured on the cached [types.AuthRequest] and never touches
|
||||
// the one-time code exchange.
|
||||
//
|
||||
// Listens in GET /register/confirm/:auth_id.
|
||||
func (a *AuthProviderOIDC) RegisterConfirmGetHandler(
|
||||
writer http.ResponseWriter,
|
||||
req *http.Request,
|
||||
) {
|
||||
authID, err := authIDFromRequest(req)
|
||||
if err != nil {
|
||||
httpUserError(writer, err)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
authReq, ok := a.h.state.GetAuthCacheEntry(authID)
|
||||
if !ok {
|
||||
httpUserError(writer, errRegistrationLinkSpent)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
pending := authReq.PendingConfirmation()
|
||||
if pending == nil {
|
||||
httpUserError(writer, NewHTTPError(http.StatusForbidden, "registration not OIDC-authorized", nil))
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// Only the browser that completed the OIDC flow holds this cookie, and
|
||||
// holding it is what authorises the confirm POST. Requiring it here too
|
||||
// keeps the device details, and the token that finalises the
|
||||
// registration, away from anyone who merely knows the auth ID — which
|
||||
// the node being registered does.
|
||||
cookie, err := req.Cookie(registerConfirmCSRFCookie)
|
||||
if err != nil {
|
||||
httpUserError(writer, NewHTTPError(http.StatusForbidden, "missing csrf cookie", err))
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
if cookie.Value != pending.CSRF {
|
||||
httpUserError(writer, NewHTTPError(http.StatusForbidden, "csrf token mismatch", nil))
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
user, err := a.h.state.GetUserByID(types.UserID(pending.UserID))
|
||||
if err != nil {
|
||||
httpUserError(writer, fmt.Errorf("looking up user: %w", err))
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
regData := authReq.RegistrationData()
|
||||
|
||||
info := templates.RegisterConfirmInfo{
|
||||
FormAction: "/register/confirm/" + authID.String(),
|
||||
FormAction: a.registerConfirmURL(authID),
|
||||
CSRFTokenName: registerConfirmCSRFCookie,
|
||||
CSRFToken: csrf,
|
||||
CSRFToken: pending.CSRF,
|
||||
User: user.Display(),
|
||||
Hostname: regData.Hostname,
|
||||
MachineKey: regData.MachineKey.ShortString(),
|
||||
@@ -742,6 +843,9 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
|
||||
info.OS = regData.Hostinfo.OS
|
||||
}
|
||||
|
||||
// The page carries the token that finalises the registration, so no
|
||||
// shared cache or history restore may serve it back.
|
||||
writer.Header().Set("Cache-Control", "no-store")
|
||||
writer.Header().Set("Content-Type", "text/html; charset=utf-8")
|
||||
writer.WriteHeader(http.StatusOK)
|
||||
|
||||
@@ -758,12 +862,6 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
||||
writer http.ResponseWriter,
|
||||
req *http.Request,
|
||||
) {
|
||||
if req.Method != http.MethodPost {
|
||||
httpUserError(writer, errMethodNotAllowed)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
authID, err := authIDFromRequest(req)
|
||||
if err != nil {
|
||||
httpUserError(writer, err)
|
||||
@@ -804,7 +902,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
||||
|
||||
authReq, ok := a.h.state.GetAuthCacheEntry(authID)
|
||||
if !ok {
|
||||
httpUserError(writer, NewHTTPError(http.StatusGone, "registration session expired", nil))
|
||||
httpUserError(writer, errRegistrationLinkSpent)
|
||||
|
||||
return
|
||||
}
|
||||
@@ -832,7 +930,7 @@ 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, "registration session expired", err))
|
||||
httpUserError(writer, NewHTTPError(http.StatusGone, registrationLinkSpentMsg, err))
|
||||
|
||||
return
|
||||
}
|
||||
@@ -843,7 +941,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
||||
}
|
||||
|
||||
// Clear the CSRF cookie now that the registration is final.
|
||||
setRegisterConfirmCookie(writer, req, authID, "", -1, a.cookiesSecure())
|
||||
a.setRegisterConfirmCookie(writer, req, authID, "", -1)
|
||||
|
||||
content := renderRegistrationSuccessTemplate(user, newNode)
|
||||
|
||||
|
||||
@@ -0,0 +1,336 @@
|
||||
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")
|
||||
}
|
||||
+16
-11
@@ -1112,6 +1112,11 @@ func (j *debugJar) Dump(w io.Writer) {
|
||||
}
|
||||
}
|
||||
|
||||
// registerConfirmCSRFField is the name of both the hidden CSRF form field
|
||||
// and the cookie on the OIDC registration confirmation interstitial. It
|
||||
// mirrors registerConfirmCSRFCookie in hscontrol, which is unexported.
|
||||
const registerConfirmCSRFField = "headscale_register_confirm"
|
||||
|
||||
func copyCookie(c *http.Cookie) *http.Cookie {
|
||||
cc := *c
|
||||
return &cc
|
||||
@@ -1225,10 +1230,11 @@ func doLoginURLWithClient(hostname string, loginURL *url.URL, hc *http.Client, f
|
||||
}
|
||||
}
|
||||
|
||||
// The OIDC registration flow now renders a confirmation interstitial
|
||||
// (POST form) instead of completing immediately. Detect the form and
|
||||
// The OIDC registration flow renders a confirmation interstitial
|
||||
// (POST form) instead of completing immediately. Detect the form by its
|
||||
// CSRF field, which does not move when the form action does, and
|
||||
// auto-submit it so integration tests behave like a real browser.
|
||||
if followRedirects && strings.Contains(body, `action="/register/confirm/`) {
|
||||
if followRedirects && strings.Contains(body, `name="`+registerConfirmCSRFField+`"`) {
|
||||
confirmBody, confirmURL, confirmErr := submitConfirmForm(hostname, body, resp, hc)
|
||||
if confirmErr != nil {
|
||||
return body, redirectURL, confirmErr
|
||||
@@ -1267,7 +1273,7 @@ func submitConfirmForm(
|
||||
|
||||
// Extract hidden CSRF input value. The rendered <input> has
|
||||
// attributes in name-type-value order so we grab the whole tag.
|
||||
before, _, ok := strings.Cut(htmlBody, `name="headscale_register_confirm"`)
|
||||
before, _, ok := strings.Cut(htmlBody, `name="`+registerConfirmCSRFField+`"`)
|
||||
if !ok {
|
||||
return "", nil, fmt.Errorf("%s confirm form: no CSRF input", hostname) //nolint:err113
|
||||
}
|
||||
@@ -1293,18 +1299,17 @@ func submitConfirmForm(
|
||||
valEnd := strings.Index(inputTag[valStart:], `"`)
|
||||
csrfToken := inputTag[valStart : valStart+valEnd]
|
||||
|
||||
// Build the absolute POST URL from the response's request URL.
|
||||
base := prevResp.Request.URL
|
||||
confirmURL := &url.URL{
|
||||
Scheme: base.Scheme,
|
||||
Host: base.Host,
|
||||
Path: formAction,
|
||||
// Resolve the form action against the page it was served from, so an
|
||||
// absolute and a relative action both work.
|
||||
confirmURL, err := prevResp.Request.URL.Parse(formAction)
|
||||
if err != nil {
|
||||
return "", nil, fmt.Errorf("%s confirm form: resolving action %q: %w", hostname, formAction, err)
|
||||
}
|
||||
|
||||
log.Printf("%s auto-submitting confirm form: %s", hostname, confirmURL)
|
||||
|
||||
formData := url.Values{
|
||||
"headscale_register_confirm": {csrfToken},
|
||||
registerConfirmCSRFField: {csrfToken},
|
||||
}
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
Reference in New Issue
Block a user