mirror of
https://github.com/juanfont/headscale.git
synced 2026-10-04 13:53:37 +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.
0.29 lacks the authPathURL helper from main, so it is added here.
(cherry picked from commit 6d377b5348)
This commit is contained in:
committed by
Kristoffer Dalby
parent
2b28f5756b
commit
2da47a77d6
@@ -563,6 +563,7 @@ func (h *Headscale) createRouter(grpcMux *grpcRuntime.ServeMux) *chi.Mux {
|
|||||||
|
|
||||||
if provider, ok := h.authProvider.(*AuthProviderOIDC); ok {
|
if provider, ok := h.authProvider.(*AuthProviderOIDC); ok {
|
||||||
r.Get("/oidc/callback", provider.OIDCCallbackHandler)
|
r.Get("/oidc/callback", provider.OIDCCallbackHandler)
|
||||||
|
r.Get("/register/confirm/{auth_id}", provider.RegisterConfirmGetHandler)
|
||||||
r.Post("/register/confirm/{auth_id}", provider.RegisterConfirmHandler)
|
r.Post("/register/confirm/{auth_id}", provider.RegisterConfirmHandler)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+15
-1
@@ -73,7 +73,10 @@ func userMessageForStatusCode(code int) string {
|
|||||||
case code == http.StatusUnauthorized || code == http.StatusForbidden:
|
case code == http.StatusUnauthorized || code == http.StatusForbidden:
|
||||||
return "You are not authorized. Please contact your administrator."
|
return "You are not authorized. Please contact your administrator."
|
||||||
case code == http.StatusGone:
|
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:
|
case code >= 400 && code < 500:
|
||||||
return "The request could not be processed. Please try again."
|
return "The request could not be processed. Please try again."
|
||||||
default:
|
default:
|
||||||
@@ -296,6 +299,17 @@ func NewAuthProviderWeb(serverURL string) *AuthProviderWeb {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// authPathURL builds an auth-flow URL of the form
|
||||||
|
// "<serverURL>/<kind>/<id>", trimming a trailing slash from serverURL.
|
||||||
|
func authPathURL(serverURL, kind string, authID types.AuthID) string {
|
||||||
|
return fmt.Sprintf(
|
||||||
|
"%s/%s/%s",
|
||||||
|
strings.TrimSuffix(serverURL, "/"),
|
||||||
|
kind,
|
||||||
|
authID.String(),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
func (a *AuthProviderWeb) RegisterURL(authID types.AuthID) string {
|
func (a *AuthProviderWeb) RegisterURL(authID types.AuthID) string {
|
||||||
return fmt.Sprintf(
|
return fmt.Sprintf(
|
||||||
"%s/register/%s",
|
"%s/register/%s",
|
||||||
|
|||||||
@@ -135,7 +135,7 @@ func TestHttpUserError(t *testing.T) {
|
|||||||
name: "gone_renders_session_expired",
|
name: "gone_renders_session_expired",
|
||||||
err: NewHTTPError(http.StatusGone, "login session expired, try again", nil),
|
err: NewHTTPError(http.StatusGone, "login session expired, try again", nil),
|
||||||
wantCode: http.StatusGone,
|
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",
|
wantNotContain: "login session expired",
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
|
|||||||
+121
-23
@@ -7,6 +7,7 @@ import (
|
|||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"net/url"
|
||||||
"slices"
|
"slices"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
@@ -350,7 +351,7 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler(
|
|||||||
// /register/{auth_id} could silently complete a registration when
|
// /register/{auth_id} could silently complete a registration when
|
||||||
// the IdP allows silent SSO.
|
// the IdP allows silent SSO.
|
||||||
if authInfo.Registration {
|
if authInfo.Registration {
|
||||||
a.renderRegistrationConfirmInterstitial(writer, req, authInfo.AuthID, user, nodeExpiry)
|
a.beginRegistrationConfirmation(writer, req, authInfo.AuthID, user, nodeExpiry)
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -668,34 +669,70 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim(
|
|||||||
// browser do not collide.
|
// browser do not collide.
|
||||||
const registerConfirmCSRFCookie = "headscale_register_confirm"
|
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
|
// setRegisterConfirmCookie writes the per-session register-confirm CSRF
|
||||||
// cookie. Pass the CSRF token and authCacheExpiration seconds to set it;
|
// cookie. Pass the CSRF token and authCacheExpiration seconds to set it;
|
||||||
// pass ("", -1) to clear it after the registration is finalised.
|
// pass ("", -1) to clear it after the registration is finalised.
|
||||||
func setRegisterConfirmCookie(
|
func (a *AuthProviderOIDC) setRegisterConfirmCookie(
|
||||||
writer http.ResponseWriter,
|
writer http.ResponseWriter,
|
||||||
req *http.Request,
|
req *http.Request,
|
||||||
authID types.AuthID,
|
authID types.AuthID,
|
||||||
value string,
|
value string,
|
||||||
maxAge int,
|
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
|
//nolint:gosec // G124: Secure from server_url scheme or req.TLS; HttpOnly + SameSite already set
|
||||||
http.SetCookie(writer, &http.Cookie{
|
http.SetCookie(writer, &http.Cookie{
|
||||||
Name: registerConfirmCSRFCookie,
|
Name: registerConfirmCSRFCookie,
|
||||||
Value: value,
|
Value: value,
|
||||||
Path: "/register/confirm/" + authID.String(),
|
Path: path,
|
||||||
MaxAge: maxAge,
|
MaxAge: maxAge,
|
||||||
Secure: secure || req.TLS != nil,
|
Secure: a.cookiesSecure() || req.TLS != nil,
|
||||||
HttpOnly: true,
|
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
|
// beginRegistrationConfirmation captures the resolved OIDC identity and
|
||||||
// identity and node expiry into the cached [types.AuthRequest], sets the CSRF
|
// node expiry into the cached [types.AuthRequest], sets the CSRF cookie, and
|
||||||
// cookie, and renders the confirmation page that the user must
|
// redirects the browser to the confirmation page.
|
||||||
// explicitly submit before the registration is finalised.
|
//
|
||||||
func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
|
// 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,
|
writer http.ResponseWriter,
|
||||||
req *http.Request,
|
req *http.Request,
|
||||||
authID types.AuthID,
|
authID types.AuthID,
|
||||||
@@ -732,14 +769,78 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
|
|||||||
CSRF: csrf,
|
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()
|
regData := authReq.RegistrationData()
|
||||||
|
|
||||||
info := templates.RegisterConfirmInfo{
|
info := templates.RegisterConfirmInfo{
|
||||||
FormAction: "/register/confirm/" + authID.String(),
|
FormAction: a.registerConfirmURL(authID),
|
||||||
CSRFTokenName: registerConfirmCSRFCookie,
|
CSRFTokenName: registerConfirmCSRFCookie,
|
||||||
CSRFToken: csrf,
|
CSRFToken: pending.CSRF,
|
||||||
User: user.Display(),
|
User: user.Display(),
|
||||||
Hostname: regData.Hostname,
|
Hostname: regData.Hostname,
|
||||||
MachineKey: regData.MachineKey.ShortString(),
|
MachineKey: regData.MachineKey.ShortString(),
|
||||||
@@ -748,6 +849,9 @@ func (a *AuthProviderOIDC) renderRegistrationConfirmInterstitial(
|
|||||||
info.OS = regData.Hostinfo.OS
|
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.Header().Set("Content-Type", "text/html; charset=utf-8")
|
||||||
writer.WriteHeader(http.StatusOK)
|
writer.WriteHeader(http.StatusOK)
|
||||||
|
|
||||||
@@ -764,12 +868,6 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
|||||||
writer http.ResponseWriter,
|
writer http.ResponseWriter,
|
||||||
req *http.Request,
|
req *http.Request,
|
||||||
) {
|
) {
|
||||||
if req.Method != http.MethodPost {
|
|
||||||
httpUserError(writer, errMethodNotAllowed)
|
|
||||||
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
authID, err := authIDFromRequest(req)
|
authID, err := authIDFromRequest(req)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
httpUserError(writer, err)
|
httpUserError(writer, err)
|
||||||
@@ -810,7 +908,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
|||||||
|
|
||||||
authReq, ok := a.h.state.GetAuthCacheEntry(authID)
|
authReq, ok := a.h.state.GetAuthCacheEntry(authID)
|
||||||
if !ok {
|
if !ok {
|
||||||
httpUserError(writer, NewHTTPError(http.StatusGone, "registration session expired", nil))
|
httpUserError(writer, errRegistrationLinkSpent)
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -838,7 +936,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
|||||||
newNode, err := a.handleRegistration(user, authID, pending.NodeExpiry)
|
newNode, err := a.handleRegistration(user, authID, pending.NodeExpiry)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) {
|
if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) {
|
||||||
httpUserError(writer, NewHTTPError(http.StatusGone, "registration session expired", err))
|
httpUserError(writer, NewHTTPError(http.StatusGone, registrationLinkSpentMsg, err))
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -849,7 +947,7 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler(
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear the CSRF cookie now that the registration is final.
|
// 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)
|
content := renderRegistrationSuccessTemplate(user, newNode)
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,337 @@
|
|||||||
|
package hscontrol
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"io"
|
||||||
|
"net/http"
|
||||||
|
"net/http/cookiejar"
|
||||||
|
"net/http/httptest"
|
||||||
|
"net/url"
|
||||||
|
"regexp"
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
grpcRuntime "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
|
||||||
|
"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(grpcRuntime.NewServeMux())
|
||||||
|
|
||||||
|
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
@@ -1128,6 +1128,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 {
|
func copyCookie(c *http.Cookie) *http.Cookie {
|
||||||
cc := *c
|
cc := *c
|
||||||
return &cc
|
return &cc
|
||||||
@@ -1241,10 +1246,11 @@ func doLoginURLWithClient(hostname string, loginURL *url.URL, hc *http.Client, f
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// The OIDC registration flow now renders a confirmation interstitial
|
// The OIDC registration flow renders a confirmation interstitial
|
||||||
// (POST form) instead of completing immediately. Detect the form and
|
// (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.
|
// 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)
|
confirmBody, confirmURL, confirmErr := submitConfirmForm(hostname, body, resp, hc)
|
||||||
if confirmErr != nil {
|
if confirmErr != nil {
|
||||||
return body, redirectURL, confirmErr
|
return body, redirectURL, confirmErr
|
||||||
@@ -1283,7 +1289,7 @@ func submitConfirmForm(
|
|||||||
|
|
||||||
// Extract hidden CSRF input value. The rendered <input> has
|
// Extract hidden CSRF input value. The rendered <input> has
|
||||||
// attributes in name-type-value order so we grab the whole tag.
|
// 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 {
|
if !ok {
|
||||||
return "", nil, fmt.Errorf("%s confirm form: no CSRF input", hostname) //nolint:err113
|
return "", nil, fmt.Errorf("%s confirm form: no CSRF input", hostname) //nolint:err113
|
||||||
}
|
}
|
||||||
@@ -1309,18 +1315,17 @@ func submitConfirmForm(
|
|||||||
valEnd := strings.Index(inputTag[valStart:], `"`)
|
valEnd := strings.Index(inputTag[valStart:], `"`)
|
||||||
csrfToken := inputTag[valStart : valStart+valEnd]
|
csrfToken := inputTag[valStart : valStart+valEnd]
|
||||||
|
|
||||||
// Build the absolute POST URL from the response's request URL.
|
// Resolve the form action against the page it was served from, so an
|
||||||
base := prevResp.Request.URL
|
// absolute and a relative action both work.
|
||||||
confirmURL := &url.URL{
|
confirmURL, err := prevResp.Request.URL.Parse(formAction)
|
||||||
Scheme: base.Scheme,
|
if err != nil {
|
||||||
Host: base.Host,
|
return "", nil, fmt.Errorf("%s confirm form: resolving action %q: %w", hostname, formAction, err)
|
||||||
Path: formAction,
|
|
||||||
}
|
}
|
||||||
|
|
||||||
log.Printf("%s auto-submitting confirm form: %s", hostname, confirmURL)
|
log.Printf("%s auto-submitting confirm form: %s", hostname, confirmURL)
|
||||||
|
|
||||||
formData := url.Values{
|
formData := url.Values{
|
||||||
"headscale_register_confirm": {csrfToken},
|
registerConfirmCSRFField: {csrfToken},
|
||||||
}
|
}
|
||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
|
|||||||
Reference in New Issue
Block a user