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
2026-10-10 08:40:07 +09:00 · 2026-10-02 09:10:45 +00:00
parent 03e723c611
commit dceb584c89
4 changed files with 208 additions and 50 deletions
@@ -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
 }
@@ -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
@@ -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)
+			}
+		})
+	}
+}
@@ -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...)