policy/v2: revoke stale SSH access and recheck authentication

Updates #3517, #3508

(cherry picked from commit 4fd4da75f2)

hscontrol/servertest/policy_test.go

@@ -1,6 +1,7 @@
 package servertest_test
 
 import (
+	"fmt"
 	"net/netip"
 	"testing"
 	"time"
@@ -129,6 +130,69 @@ func TestPolicyChanges(t *testing.T) {
 		}
 	})
 
+	// https://github.com/juanfont/headscale/issues/3508
+	t.Run("ssh_removal_clears_client_ssh_policy", func(t *testing.T) {
+		t.Parallel()
+		h := servertest.NewHarness(t, 2)
+
+		h.ChangePolicy(t, []byte(`{
+			"acls": [],
+			"ssh": [{
+				"action": "accept",
+				"src":    ["autogroup:member"],
+				"dst":    ["autogroup:self"],
+				"users":  ["root"]
+			}]
+		}`))
+
+		h.Client(0).WaitForCondition(t, "SSH rules present", 10*time.Second,
+			func(nm *netmap.NetworkMap) bool {
+				return nm.SSHPolicy != nil && len(nm.SSHPolicy.Rules) > 0
+			})
+
+		h.ChangePolicy(t, []byte(`{"acls": []}`))
+
+		h.Client(0).WaitForCondition(t, "SSH rules cleared", 10*time.Second,
+			func(nm *netmap.NetworkMap) bool {
+				return nm.SSHPolicy != nil && len(nm.SSHPolicy.Rules) == 0
+			})
+	})
+
+	// An unchanged SSHPolicy is dropped from policy frames; the client
+	// must keep the rules it holds.
+	t.Run("unrelated_policy_change_keeps_client_ssh_rules", func(t *testing.T) {
+		t.Parallel()
+		h := servertest.NewHarness(t, 2)
+
+		withSSH := func(group string) []byte {
+			return fmt.Appendf(nil, `{
+				"groups": {%q: []},
+				"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}],
+				"ssh": [{
+					"action": "accept",
+					"src":    ["autogroup:member"],
+					"dst":    ["autogroup:self"],
+					"users":  ["root"]
+				}]
+			}`, "group:"+group)
+		}
+
+		h.ChangePolicy(t, withSSH("a"))
+		h.Client(0).WaitForCondition(t, "SSH rules present", 10*time.Second,
+			func(nm *netmap.NetworkMap) bool {
+				return nm.SSHPolicy != nil && len(nm.SSHPolicy.Rules) > 0
+			})
+
+		countBefore := h.Client(0).UpdateCount()
+
+		h.ChangePolicy(t, withSSH("b"))
+		h.Client(0).WaitForCondition(t, "policy update with SSH rules kept", 10*time.Second,
+			func(nm *netmap.NetworkMap) bool {
+				return h.Client(0).UpdateCount() > countBefore &&
+					nm.SSHPolicy != nil && len(nm.SSHPolicy.Rules) > 0
+			})
+	})
+
 	t.Run("policy_with_multiple_users", func(t *testing.T) {
 		t.Parallel()
 

hscontrol/servertest/sshcheck_test.go

@@ -6,14 +6,17 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"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"
+	"tailscale.com/types/netmap"
 )
 
 // TestSSHCheckReDelegatesWhenSessionMissing exercises the fix for
@@ -44,7 +47,7 @@ func TestSSHCheckReDelegatesWhenSessionMissing(t *testing.T) {
 
 	// Sanity: the policy must actually subject this pair to a check, otherwise
 	// the test would pass for the wrong reason.
-	_, checkFound := h.Server.State().SSHCheckParams(srcID, dstID)
+	_, checkFound := h.Server.State().SSHCheckParams(srcID, dstID, sshCheckLocalUser)
 	require.True(t, checkFound, "test setup: (src, dst) must be subject to an SSH check")
 
 	// The dst node's first poll yields a real HoldAndDelegate carrying a real,
@@ -71,6 +74,9 @@ func TestSSHCheckReDelegatesWhenSessionMissing(t *testing.T) {
 		"re-delegation must mint a fresh auth_id")
 }
 
+// sshCheckLocalUser is the non-root local user the SSH-check polls log in as.
+const sshCheckLocalUser = "alice"
+
 // pollSSHAction issues an /machine/ssh/action poll from the given node over its
 // real Noise connection, as tailscaled does. An empty authID is the initial
 // poll; a non-empty one is a follow-up.
@@ -83,11 +89,20 @@ func pollSSHAction(
 ) tailcfg.SSHAction {
 	t.Helper()
 
-	actionURL := fmt.Sprintf("%s/machine/ssh/action/%d/to/%d", serverURL, srcID, dstID)
+	actionURL := fmt.Sprintf("%s/machine/ssh/action/%d/to/%d?local_user=%s",
+		serverURL, srcID, dstID, sshCheckLocalUser)
 	if authID != "" {
-		actionURL += "?auth_id=" + authID
+		actionURL += "&auth_id=" + authID
 	}
 
+	return pollSSHActionURL(t, node, actionURL)
+}
+
+// pollSSHActionURL polls actionURL from the given node over its real Noise
+// connection.
+func pollSSHActionURL(t *testing.T, node *servertest.TestClient, actionURL string) tailcfg.SSHAction {
+	t.Helper()
+
 	// Noise requests are addressed with the https scheme; the control client
 	// routes them over the established Noise connection (mirroring how
 	// controlclient issues its own register/map calls).
@@ -125,3 +140,108 @@ 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, sshCheckLocalUser)
+			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)
+			}
+		})
+	}
+}
+
+// expandHoldURL substitutes a HoldAndDelegate URL's variables literally, as
+// tailssh does before polling it.
+func expandHoldURL(holdURL string, srcID, dstID types.NodeID, localUser string) string {
+	return strings.NewReplacer(
+		"$SRC_NODE_ID", strconv.FormatUint(srcID.Uint64(), 10),
+		"$DST_NODE_ID", strconv.FormatUint(dstID.Uint64(), 10),
+		"$SSH_USER", url.QueryEscape(localUser),
+		"$LOCAL_USER", url.QueryEscape(localUser),
+	).Replace(holdURL)
+}
+
+// TestSSHCheckFollowsReturnedHoldURL walks a check the way tailssh does:
+// expand the HoldAndDelegate URL from the netmap, poll it, then poll the
+// URL the server returns. The follow-up must still carry the login user,
+// or a root-only rule denies a user who already authenticated.
+// https://github.com/juanfont/headscale/issues/3508
+func TestSSHCheckFollowsReturnedHoldURL(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(`{"ssh": [{
+		"action": "check",
+		"src":    ["harness-default@"],
+		"dst":    ["autogroup:self"],
+		"users":  ["root"]
+	}]}`))
+
+	var ruleURL string
+
+	h.Client(1).WaitForCondition(t, "check rule in netmap", 10*time.Second,
+		func(nm *netmap.NetworkMap) bool {
+			if nm.SSHPolicy == nil || len(nm.SSHPolicy.Rules) == 0 ||
+				nm.SSHPolicy.Rules[0].Action == nil {
+				return false
+			}
+
+			ruleURL = nm.SSHPolicy.Rules[0].Action.HoldAndDelegate
+
+			return ruleURL != ""
+		})
+
+	initial := pollSSHActionURL(t, h.Client(1), expandHoldURL(ruleURL, srcID, dstID, "root"))
+	require.NotEmpty(t, initial.HoldAndDelegate, "check must hold, got %+v", initial)
+
+	auth, ok := h.Server.State().GetAuthCacheEntry(authIDFromHoldURL(t, initial.HoldAndDelegate))
+	require.True(t, ok)
+	auth.FinishAuth(types.AuthVerdict{})
+
+	followUp := pollSSHActionURL(t, h.Client(1),
+		expandHoldURL(initial.HoldAndDelegate, srcID, dstID, "root"))
+	assert.True(t, followUp.Accept, "authenticated root login must be accepted, got %+v", followUp)
+}