From dceb584c890ac99a5af0333a481946a9b3497abb Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 2 Oct 2026 09:10:45 +0000 Subject: [PATCH] noise: reject SSH checks the policy no longer requires A stale check rule still held and accepted after login. Deny with a 200 Reject (tailssh retries errors); re-check after the verdict. Updates #3508 --- hscontrol/noise.go | 67 +++++++------ hscontrol/noise_test.go | 134 ++++++++++++++++++++++---- hscontrol/servertest/sshcheck_test.go | 49 ++++++++++ hscontrol/state/state.go | 8 ++ 4 files changed, 208 insertions(+), 50 deletions(-) diff --git a/hscontrol/noise.go b/hscontrol/noise.go index 3d90e75ef..cced5e1c2 100644 --- a/hscontrol/noise.go +++ b/hscontrol/noise.go @@ -457,12 +457,13 @@ func (ns *noiseServer) SSHActionHandler( // sshAction resolves the SSH action for the given request parameters. // It returns the action to send to the client, or an [HTTPError] on failure. // -// Three cases: -// 1. Initial request, auto-approved — source recently authenticated +// Four cases: +// 1. No check rule covers the pair — reject; the client's rule is stale. +// 2. Initial request, auto-approved — source recently authenticated // within the check period, accept immediately. -// 2. Initial request, needs auth — build a [tailcfg.SSHAction.HoldAndDelegate] URL and +// 3. Initial request, needs auth — build a [tailcfg.SSHAction.HoldAndDelegate] URL and // wait for the user to authenticate. -// 3. Follow-up request — an auth_id is present, wait for the auth +// 4. Follow-up request — an auth_id is present, wait for the auth // verdict and accept or reject. A session that is gone or whose // verdict another follow-up consumed is re-decided. func (ns *noiseServer) sshAction( @@ -483,17 +484,22 @@ func (ns *noiseServer) sshAction( srcNodeID, dstNodeID, ) + // Clients only call back for check rules they were sent. Without one in + // the current policy the client's copy is stale. + if !checkFound { + return sshActionDeny(reqLog, &action), nil + } + // Follow-up request with auth_id — wait for the auth verdict. if authIDStr != "" { return ns.sshActionFollowUp( ctx, reqLog, &action, authIDStr, srcNodeID, dstNodeID, - checkFound, ) } // Initial request — check if auto-approval applies. - if checkFound && checkPeriod > 0 { + if checkPeriod > 0 { if lastAuth, ok := ns.headscale.state.GetLastSSHAuth( srcNodeID, dstNodeID, ); ok && time.Since(lastAuth) < checkPeriod { @@ -512,6 +518,17 @@ func (ns *noiseServer) sshAction( return ns.sshActionHoldAndDelegate(reqLog, &action, srcNodeID, dstNodeID) } +// sshActionDeny rejects a check the current policy does not require. It is a +// 200 Reject, not an HTTP error: tailssh retries errors for up to 30 minutes. +func sshActionDeny(reqLog zerolog.Logger, action *tailcfg.SSHAction) *tailcfg.SSHAction { + action.Reject = true + action.Message = "# Headscale SSH: no check rule in the tailnet policy covers this connection.\n" + + reqLog.Info().Caller().Msg("SSH check denied: no matching check rule") + + return action +} + // sshActionHoldAndDelegate creates a new auth session bound to the // (src, dst) pair and returns a [tailcfg.SSHAction.HoldAndDelegate] action that directs the // client to authenticate. @@ -580,7 +597,6 @@ func (ns *noiseServer) sshActionFollowUp( action *tailcfg.SSHAction, authIDStr string, srcNodeID, dstNodeID types.NodeID, - checkFound bool, ) (*tailcfg.SSHAction, error) { authID, err := types.AuthIDFromString(authIDStr) if err != nil { @@ -593,25 +609,14 @@ func (ns *noiseServer) sshActionFollowUp( reqLog = reqLog.With().Str("auth_id", authID.String()).Logger() - // The session is gone (expired, evicted, or lost on a control-plane - // restart) or its verdict was already consumed. A bare error dead-ends - // the client: it keeps polling this now-defunct auth_id until the SSH - // connection times out. Re-delegate so a still-required check can - // complete instead. + // A missing session or consumed verdict must never approve access. + // The caller checked that the policy still requires authentication; + // re-delegate so the client can complete a fresh check. sessionGone := func(logMsg string) (*tailcfg.SSHAction, error) { - if checkFound { - reqLog.Info().Caller(). - Msg(logMsg) + reqLog.Info().Caller().Msg(logMsg) - return ns.sshActionHoldAndDelegate( - reqLog, action, srcNodeID, dstNodeID, - ) - } - - return nil, NewHTTPError( - http.StatusBadRequest, - "Invalid auth_id", - fmt.Errorf("%w: %s", ErrNoAuthSession, authID), + return ns.sshActionHoldAndDelegate( + reqLog, action, srcNodeID, dstNodeID, ) } @@ -684,15 +689,19 @@ func (ns *noiseServer) sshActionFollowUp( return action, nil } + // The policy may have changed while the user authenticated, and the + // client won't drop a connection still waiting on its check. + if _, ok := ns.headscale.state.SSHCheckParams(srcNodeID, dstNodeID); !ok { + return sshActionDeny(reqLog, action), nil + } + action.Accept = true // Record the successful auth for future auto-approval. - if checkFound { - ns.headscale.state.SetLastSSHAuth(srcNodeID, dstNodeID) + ns.headscale.state.SetLastSSHAuth(srcNodeID, dstNodeID) - reqLog.Trace().Caller(). - Msg("auth recorded for auto-approval") - } + reqLog.Trace().Caller(). + Msg("auth recorded for auto-approval") return action, nil } diff --git a/hscontrol/noise_test.go b/hscontrol/noise_test.go index acaa57907..3ec37ea89 100644 --- a/hscontrol/noise_test.go +++ b/hscontrol/noise_test.go @@ -21,6 +21,7 @@ import ( "github.com/juanfont/headscale/hscontrol/capver" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" @@ -476,6 +477,45 @@ func putTestNodeInStore(t *testing.T, app *Headscale, user *types.User, hostname return node } +// sshCheckPolicy subjects every same-user pair of userName's nodes to an +// SSH check. +func sshCheckPolicy(userName string) string { + return fmt.Sprintf(`{"ssh": [{ + "action": "check", + "src": [%q], + "dst": ["autogroup:self"], + "users": ["autogroup:nonroot"] + }]}`, userName+"@") +} + +// putSSHCheckNodes stages one node per hostname for a new user and sets +// [sshCheckPolicy], so the SSH action handler holds and delegates for any +// pair of them. +func putSSHCheckNodes(t *testing.T, app *Headscale, userName string, hostnames ...string) []*types.Node { + t.Helper() + + user := app.state.CreateUserForTest(userName) + require.NoError(t, app.state.UpdatePolicyManagerUsersForTest()) + + nodes := make([]*types.Node, 0, len(hostnames)) + for _, hostname := range hostnames { + node := app.state.CreateRegisteredNodeForTest(user, hostname) + node.User = user + app.state.PutNodeInStoreForTest(*node) + nodes = append(nodes, node) + } + + require.NoError(t, app.state.UpdatePolicyManagerNodesForTest()) + + _, err := app.state.SetPolicy([]byte(sshCheckPolicy(userName))) + require.NoError(t, err) + + _, checkFound := app.state.SSHCheckParams(nodes[0].ID, nodes[len(nodes)-1].ID) + require.True(t, checkFound, "test setup: nodes must be subject to an SSH check") + + return nodes +} + // TestSSHActionHandler_RejectsRogueMachineKey verifies that the SSH // check action endpoint rejects a Noise session whose machine key does // not match the dst node. @@ -540,12 +580,9 @@ func TestSSHActionFollowUp_RejectsBindingMismatch(t *testing.T) { t.Parallel() app := createTestApp(t) - user := app.state.CreateUserForTest("ssh-binding-user") - - srcCached := putTestNodeInStore(t, app, user, "src-cached") - dstCached := putTestNodeInStore(t, app, user, "dst-cached") - srcOther := putTestNodeInStore(t, app, user, "src-other") - dstOther := putTestNodeInStore(t, app, user, "dst-other") + nodes := putSSHCheckNodes(t, app, "ssh-binding-user", + "src-cached", "dst-cached", "src-other", "dst-other") + srcCached, dstCached, srcOther, dstOther := nodes[0], nodes[1], nodes[2], nodes[3] // Mint an SSH-check auth request bound to (srcCached, dstCached). authID := types.MustAuthID() @@ -616,9 +653,8 @@ func TestSSHActionHoldAndDelegate_PersistsAuthSession(t *testing.T) { t.Parallel() app := createTestApp(t) - user := app.state.CreateUserForTest("ssh-persist-user") - src := putTestNodeInStore(t, app, user, "src-node") - dst := putTestNodeInStore(t, app, user, "dst-node") + nodes := putSSHCheckNodes(t, app, "ssh-persist-user", "src-node", "dst-node") + src, dst := nodes[0], nodes[1] ns := &noiseServer{headscale: app, machineKey: dst.MachineKey} @@ -643,12 +679,13 @@ func TestSSHActionHoldAndDelegate_PersistsAuthSession(t *testing.T) { require.True(t, ok, "auth session %s must persist after HoldAndDelegate", authID) } -// TestSSHActionHandler_RejectsMissingSessionWithoutCheck verifies that without -// an SSH check covering the pair, a follow-up poll for an unknown auth_id is a -// genuinely bogus request and is rejected. The re-delegation behaviour for a -// missing session (issue #3305, exercised end to end with a real client in the -// servertest package) applies only when the pair is still subject to a check. -func TestSSHActionHandler_RejectsMissingSessionWithoutCheck(t *testing.T) { +// TestSSHActionHandler_RejectsWithoutCheck verifies that a pair no check rule +// covers is denied with a 200 Reject on both the initial and the follow-up +// poll. Such a call comes from a client holding a stale check rule; an HTTP +// error would make tailssh retry for up to 30 minutes. Re-delegation for a +// missing session (issue #3305) applies only while a check is required. +// https://github.com/juanfont/headscale/issues/3508 +func TestSSHActionHandler_RejectsWithoutCheck(t *testing.T) { t.Parallel() app := createTestApp(t) @@ -656,18 +693,73 @@ func TestSSHActionHandler_RejectsMissingSessionWithoutCheck(t *testing.T) { src := putTestNodeInStore(t, app, user, "src-node") dst := putTestNodeInStore(t, app, user, "dst-node") - // No SSH-check policy is set, so the pair is not subject to a check. _, checkFound := app.state.SSHCheckParams(src.ID, dst.ID) require.False(t, checkFound, "test setup: pair must not be subject to a check") ns := &noiseServer{headscale: app, machineKey: dst.MachineKey} - missing := types.MustAuthID() + for name, req := range map[string]*http.Request{ + "initial": newSSHActionRequest(t, src.ID, dst.ID), + "follow-up": newSSHActionFollowUpRequest(t, src.ID, dst.ID, types.MustAuthID()), + } { + rec := httptest.NewRecorder() + ns.SSHActionHandler(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "%s: body=%s", name, rec.Body.String()) - rec := httptest.NewRecorder() - ns.SSHActionHandler(rec, newSSHActionFollowUpRequest(t, src.ID, dst.ID, missing)) - require.Equal(t, http.StatusBadRequest, rec.Code, - "a bogus auth_id with no active check must be rejected, body=%s", rec.Body.String()) + var action tailcfg.SSHAction + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &action)) + assert.True(t, action.Reject, "%s: must reject, got %+v", name, action) + assert.Empty(t, action.HoldAndDelegate, "%s: must not delegate", name) + } +} + +// TestSSHActionFollowUp_RejectsVerdictAfterCheckRemoved verifies the check is +// re-evaluated once the user authenticates: a rule removed while the login +// prompt was open must not grant access. +// https://github.com/juanfont/headscale/issues/3508 +func TestSSHActionFollowUp_RejectsVerdictAfterCheckRemoved(t *testing.T) { + t.Parallel() + + for name, removeRule := range map[string]bool{ + "rule kept": false, + "rule removed": true, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + nodes := putSSHCheckNodes(t, app, "ssh-verdict-user", "src-node", "dst-node") + src, dst := nodes[0], nodes[1] + + authID := types.MustAuthID() + app.state.SetAuthCacheEntry(authID, types.NewSSHCheckAuthRequest(src.ID, dst.ID)) + + auth, ok := app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + auth.FinishAuth(types.AuthVerdict{}) + + if removeRule { + _, err := app.state.SetPolicy([]byte(`{}`)) + require.NoError(t, err) + } + + ns := &noiseServer{headscale: app, machineKey: dst.MachineKey} + + // Call the follow-up directly: the verdict is already in, as if + // the policy changed while the user was authenticating. + action, err := ns.sshActionFollowUp( + t.Context(), zerolog.Nop(), &tailcfg.SSHAction{}, + authID.String(), src.ID, dst.ID, + ) + require.NoError(t, err) + + _, recorded := app.state.GetLastSSHAuth(src.ID, dst.ID) + + assert.Equal(t, !removeRule, action.Accept, "accept, got %+v", action) + assert.Equal(t, removeRule, action.Reject, "reject, got %+v", action) + assert.Equal(t, !removeRule, recorded, "auth recorded for auto-approval") + }) + } } // TestTS2021Route_AcceptsGETAndPOST reproduces a regression where the diff --git a/hscontrol/servertest/sshcheck_test.go b/hscontrol/servertest/sshcheck_test.go index 4ff1ed317..73df0332a 100644 --- a/hscontrol/servertest/sshcheck_test.go +++ b/hscontrol/servertest/sshcheck_test.go @@ -12,6 +12,7 @@ import ( "github.com/juanfont/headscale/hscontrol/servertest" "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" ) @@ -125,3 +126,51 @@ func authIDFromHoldURL(t *testing.T, holdURL string) types.AuthID { return authID } + +// TestSSHCheckRejectedAfterRuleRemoved verifies that once a check rule is +// removed or turned into accept, a client still holding the check gets a +// Reject on both the initial and the follow-up poll, not a hold it could +// pass by authenticating. +// https://github.com/juanfont/headscale/issues/3508 +func TestSSHCheckRejectedAfterRuleRemoved(t *testing.T) { + t.Parallel() + + const check = `{"ssh": [{ + "action": "check", + "src": ["harness-default@"], + "dst": ["autogroup:self"], + "users": ["autogroup:nonroot"] + }]}` + + for name, after := range map[string]string{ + "rule removed": `{}`, + "check to accept": strings.Replace(check, `"check"`, `"accept"`, 1), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + h := servertest.NewHarness(t, 2) + + srcID := types.NodeID(h.Client(0).Netmap().SelfNode.ID()) //nolint:gosec + dstID := types.NodeID(h.Client(1).Netmap().SelfNode.ID()) //nolint:gosec + + h.ChangePolicy(t, []byte(check)) + + initial := pollSSHAction(t, h.Server.URL, h.Client(1), srcID, dstID, "") + require.NotEmpty(t, initial.HoldAndDelegate, "check must hold, got %+v", initial) + + authID := authIDFromHoldURL(t, initial.HoldAndDelegate) + + h.ChangePolicy(t, []byte(after)) + + _, checkFound := h.Server.State().SSHCheckParams(srcID, dstID) + require.False(t, checkFound, "test setup: check must be gone") + + for poll, id := range map[string]string{"initial": "", "follow-up": authID.String()} { + action := pollSSHAction(t, h.Server.URL, h.Client(1), srcID, dstID, id) + assert.True(t, action.Reject, "%s poll must reject, got %+v", poll, action) + assert.Empty(t, action.HoldAndDelegate, "%s poll must not delegate", poll) + } + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 6185701df..edc49174f 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1631,6 +1631,14 @@ func (s *State) WrapPolicyManagerForTest(wrap func(policy.PolicyManager) policy. s.polMan = wrap(s.polMan) } +// UpdatePolicyManagerNodesForTest syncs the policy manager with the +// [NodeStore], for tests that stage nodes with [State.PutNodeInStoreForTest]. +func (s *State) UpdatePolicyManagerNodesForTest() error { + _, err := s.polMan.SetNodes(s.ListNodes()) + + return err +} + // CreateRegisteredNodeForTest creates a test node with allocated IPs. This is a convenience wrapper around the database layer. func (s *State) CreateRegisteredNodeForTest(user *types.User, hostname ...string) *types.Node { return s.db.CreateRegisteredNodeForTest(user, hostname...)