policy/v2: match the login user in SSHCheckParams

The client picks the check rule by login user; the server took the
first rule for the node pair, so a root login could get a 12h
localpart period instead of "always", or be approved after its rule
was removed while another user's rule remained. Hold URLs now carry
the concrete user: tailssh never expanded the encoded $LOCAL_USER.

Updates #3508

hscontrol/servertest/sshcheck_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -15,6 +16,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"tailscale.com/tailcfg"
+	"tailscale.com/types/netmap"
 )
 
 // TestSSHCheckReDelegatesWhenSessionMissing exercises the fix for
@@ -45,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,
@@ -72,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.
@@ -84,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).
@@ -163,7 +177,7 @@ func TestSSHCheckRejectedAfterRuleRemoved(t *testing.T) {
 
 			h.ChangePolicy(t, []byte(after))
 
-			_, checkFound := h.Server.State().SSHCheckParams(srcID, dstID)
+			_, 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()} {
@@ -174,3 +188,60 @@ func TestSSHCheckRejectedAfterRuleRemoved(t *testing.T) {
 		})
 	}
 }
+
+// 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)
+}