From b2d891c9d1d6dcfabcbfb79a1b92094d3f6c1657 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Tue, 12 May 2026 21:05:18 +0000 Subject: [PATCH] policy/v2: address review on sshTests engine Empty-dst-nodes fails loudly. Compat runner uses per-capture topology so capture IPs match. Test rows tightened; stale godoc and goto fixed. --- hscontrol/policy/v2/sshtest.go | 68 +++++++++---- hscontrol/policy/v2/sshtest_test.go | 96 ++++++++++++------- hscontrol/policy/v2/sshtester_compat_test.go | 19 ++-- .../v2/tailscale_ssh_data_compat_test.go | 55 ----------- hscontrol/policy/v2/test.go | 9 +- 5 files changed, 128 insertions(+), 119 deletions(-) diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index 4af62101..6a6a203c 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -264,7 +264,7 @@ func runSSHPolicyTest( return res } - dstNodes, err := resolveSSHTestDestNodes(test.Dst, pol, users, nodes, srcUserID) + dstNodes, emptyDsts, err := resolveSSHTestDestNodes(test.Dst, pol, users, nodes, srcUserID) if err != nil { res.Passed = false res.Errors = append(res.Errors, @@ -273,6 +273,20 @@ func runSSHPolicyTest( return res } + // SaaS treats a dst alias that resolves to no nodes as a failure + // when the entry has anything to assert. Without this branch, the + // per-assertion loops below run zero iterations and the test + // passes silently — wrong shape, missed regression. + for _, dst := range emptyDsts { + res.Passed = false + res.Errors = append(res.Errors, + fmt.Sprintf("dst alias %q resolved to no nodes", dst)) + } + + if len(dstNodes) == 0 { + return res + } + for _, user := range test.Accept { evaluateAssertion( pol, users, nodes, cache, @@ -334,6 +348,7 @@ func evaluateAssertion( kind sshAssertion, res *SSHPolicyTestResult, ) { +dstLoop: for _, dst := range dstNodes { dstPol, err := compiledSSHPolicy(pol, users, nodes, cache, dst) if err != nil { @@ -347,8 +362,8 @@ func evaluateAssertion( dstLabel := dst.Hostname() - // reachableAccept covers "any matching accept-or-check rule"; - // reachableCheck restricts to check-action matches only. + // acceptHit covers "any matching accept-or-check rule"; + // checkHit restricts to check-action matches only. acceptHit := false checkHit := false @@ -371,14 +386,14 @@ func evaluateAssertion( res.Passed = false res.AcceptFail = appendUserDst(res.AcceptFail, user, dstLabel) - goto nextDst + continue dstLoop } case assertDeny: if a { res.Passed = false res.DenyFail = appendUserDst(res.DenyFail, user, dstLabel) - goto nextDst + continue dstLoop } case assertCheck: if !c { @@ -392,7 +407,7 @@ func evaluateAssertion( res.AcceptOK = appendUserDst(res.AcceptOK, user, dstLabel) } - goto nextDst + continue dstLoop } } } @@ -409,8 +424,6 @@ func evaluateAssertion( res.CheckOK = appendUserDst(res.CheckOK, user, dstLabel) } } - - nextDst: } } @@ -482,25 +495,32 @@ func resolveSSHTestDestNodes( users []types.User, nodes views.Slice[types.NodeView], srcUserID uint, -) ([]types.NodeView, error) { +) ([]types.NodeView, []string, error) { seen := make(map[types.NodeID]struct{}) - var out []types.NodeView + var ( + out []types.NodeView + emptyDsts []string + ) for _, dst := range dsts { alias, err := parseAlias(dst) if err != nil { - return nil, fmt.Errorf("invalid destination %q: %w", dst, err) + return nil, nil, fmt.Errorf("invalid destination %q: %w", dst, err) } + matched := false + if ag, ok := alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { // autogroup:self → destinations are the non-tagged // nodes owned by the same user as src. A tagged or // IP-only src has no user identity, so the dst set - // is empty (matches SaaS, which treats this as a - // no-op assertion that the engine then reports as a - // failure via the empty-dst-nodes branch below). + // is empty and the caller surfaces it as a failure + // (matches SaaS, which treats a no-node dst as a + // failing assertion). if srcUserID == 0 { + emptyDsts = append(emptyDsts, dst) + continue } @@ -517,6 +537,8 @@ func resolveSSHTestDestNodes( continue } + matched = true + if _, dup := seen[n.ID()]; dup { continue } @@ -525,15 +547,21 @@ func resolveSSHTestDestNodes( out = append(out, n) } + if !matched { + emptyDsts = append(emptyDsts, dst) + } + continue } ips, err := alias.Resolve(pol, users, nodes) if err != nil { - return nil, fmt.Errorf("resolving destination %q: %w", dst, err) + return nil, nil, fmt.Errorf("resolving destination %q: %w", dst, err) } if ips == nil || ips.Empty() { + emptyDsts = append(emptyDsts, dst) + continue } @@ -542,7 +570,7 @@ func resolveSSHTestDestNodes( // the resolved prefixes. set, err := prefixesToIPSet(ips.Prefixes()) if err != nil { - return nil, fmt.Errorf("building IPSet for %q: %w", dst, err) + return nil, nil, fmt.Errorf("building IPSet for %q: %w", dst, err) } for _, n := range nodes.All() { @@ -550,6 +578,8 @@ func resolveSSHTestDestNodes( continue } + matched = true + if _, dup := seen[n.ID()]; dup { continue } @@ -557,9 +587,13 @@ func resolveSSHTestDestNodes( seen[n.ID()] = struct{}{} out = append(out, n) } + + if !matched { + emptyDsts = append(emptyDsts, dst) + } } - return out, nil + return out, emptyDsts, nil } // prefixesToIPSet builds a netipx.IPSet from a slice of prefixes. The diff --git a/hscontrol/policy/v2/sshtest_test.go b/hscontrol/policy/v2/sshtest_test.go index b76e54ef..98253d4a 100644 --- a/hscontrol/policy/v2/sshtest_test.go +++ b/hscontrol/policy/v2/sshtest_test.go @@ -1,6 +1,7 @@ package v2 import ( + "encoding/json" "strings" "testing" @@ -8,7 +9,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gorm.io/gorm" - "tailscale.com/tailcfg" ) // sshTestUsers/sshTestNodes are reused across the table below to keep @@ -563,24 +563,6 @@ func TestRunSSHTests(t *testing.T) { }`, wantPass: true, }, - { - name: "tag-as-dst-single-node", - policy: `{ - "tagOwners": { "tag:server": ["alice@headscale.net"] }, - "ssh": [{ - "action": "accept", - "src": ["alice@headscale.net"], - "dst": ["tag:server"], - "users": ["root"] - }], - "sshTests": [{ - "src": "alice@headscale.net", - "dst": ["tag:server"], - "accept": ["root"] - }] - }`, - wantPass: true, - }, { name: "host-alias-as-dst", policy: `{ @@ -618,9 +600,19 @@ func TestRunSSHTests(t *testing.T) { wantErrSub: []string{"alice@headscale.net", "root", "expected ALLOWED"}, }, { + // ACL grants only TCP:80 to alice; no rule grants TCP:22. + // SSH rule independently allows root@tag:server. The + // sshTests assertion must pass on the SSH layer alone, + // proving the engine does not require an ACL packet- + // filter rule for the SSH port. name: "acl-denies-tcp22-ssh-rule-allows", policy: `{ "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "acls": [{ + "action": "accept", + "src": ["alice@headscale.net"], + "dst": ["tag:server:80"] + }], "ssh": [{ "action": "accept", "src": ["alice@headscale.net"], @@ -704,6 +696,40 @@ func TestRunSSHTests(t *testing.T) { wantPass: false, wantErrSub: []string{"alice@headscale.net", "expected ALLOWED"}, }, + { + // tag:empty has an owner but no tagged nodes, so the dst + // alias resolves to no nodes. Without the empty-dst guard + // the per-assertion loop runs zero iterations and the + // test silently passes — exactly the regression the + // guard exists to catch. + name: "dst-tag-with-no-tagged-nodes-fails", + policy: `{ + "tagOwners": { "tag:empty": ["alice@headscale.net"] }, + "sshTests": [{ + "src": "alice@headscale.net", + "dst": ["tag:empty"], + "accept": ["root"] + }] + }`, + wantPass: false, + wantErrSub: []string{"tag:empty", "resolved to no nodes"}, + }, + { + // autogroup:self from a tag src has no user identity to + // scope to, so the dst alias resolves to no nodes. Same + // empty-dst guard, distinct trigger path. + name: "dst-autogroup-self-from-tag-src-fails", + policy: `{ + "tagOwners": { "tag:prod": ["alice@headscale.net"] }, + "sshTests": [{ + "src": "tag:prod", + "dst": ["autogroup:self"], + "accept": ["root"] + }] + }`, + wantPass: false, + wantErrSub: []string{"autogroup:self", "resolved to no nodes"}, + }, } for _, tt := range tests { @@ -813,12 +839,19 @@ func TestSetPolicyRejectsFailingSSHTests(t *testing.T) { // Snapshot SSHPolicy output for alice-laptop before the rejected // write — the live PolicyManager state must still describe the - // previous (good) rules afterwards. + // previous (good) rules afterwards. JSON-marshal the snapshot so + // the comparison sees rule content, not just object identity: a + // hypothetical mutation that preserves the slice length but + // rewrites principals or SSHUsers would slip past a count-only + // assertion. aliceView := nodes.ViewSlice().At(0) beforePol, err := pm.SSHPolicy("", aliceView) require.NoError(t, err) + beforeJSON, err := json.Marshal(beforePol) + require.NoError(t, err) + changed, err := pm.SetPolicy([]byte(bad)) require.Error(t, err, "SetPolicy must reject a policy whose sshTests fail") require.False(t, changed, "SetPolicy must report no change when rejected") @@ -827,7 +860,11 @@ func TestSetPolicyRejectsFailingSSHTests(t *testing.T) { afterPol, err := pm.SSHPolicy("", aliceView) require.NoError(t, err) - require.Equal(t, sshRuleCount(beforePol), sshRuleCount(afterPol), + + afterJSON, err := json.Marshal(afterPol) + require.NoError(t, err) + + require.JSONEq(t, string(beforeJSON), string(afterJSON), "live SSH policy must not change after a rejected SetPolicy") } @@ -903,6 +940,12 @@ func TestSetPolicyAggregatesACLAndSSHTestFailures(t *testing.T) { "aggregated error must include the ACL failure message") assert.Contains(t, body, "bob@headscale.net", "aggregated error must include the bob src") + // The SSH renderer emits "src/user -> dst" form; the ACL renderer + // emits "src -> dst". Substring "/root -> " is unique to the SSH + // body, so finding it inside the aggregated error proves the SSH + // failure rendering was concatenated alongside the ACL body. + assert.Contains(t, body, "/root -> ", + "aggregated error must include the SSH-shape src/user -> dst rendering") } // TestNewPolicyManagerWarnsOnSSHTestsFailure asserts the boot path does @@ -977,14 +1020,3 @@ func TestSSHPolicyTestResultsErrorsRendering(t *testing.T) { assert.Equal(t, 3, strings.Count(rendered, "\n")+1, "expected one line per failing assertion") } - -// sshRuleCount returns the number of rules in p, treating nil as 0. -// Used by TestSetPolicyRejectsFailingSSHTests to verify a rejected -// SetPolicy leaves the live SSH policy untouched. -func sshRuleCount(p *tailcfg.SSHPolicy) int { - if p == nil { - return 0 - } - - return len(p.Rules) -} diff --git a/hscontrol/policy/v2/sshtester_compat_test.go b/hscontrol/policy/v2/sshtester_compat_test.go index 8d7dbe57..63558a04 100644 --- a/hscontrol/policy/v2/sshtester_compat_test.go +++ b/hscontrol/policy/v2/sshtester_compat_test.go @@ -32,16 +32,6 @@ import ( // disagree on whether a policy is accepted. Each entry should describe // the engine area a follow-up PR needs to touch. var knownSSHTesterDivergences = map[string]string{ - // SaaS resolves an IP-literal sshTests src to the owning node's - // user identity and matches it against compiled SSH principals; - // headscale's resolveSSHTestSource returns srcUserID=0 for any - // non-Username alias and only consults principal NodeIPs, so an - // IP that names a user-owned node misses every accept rule keyed - // by that user. Fix in hscontrol/policy/v2/sshtest.go — extend - // resolveSSHTestSource to recover the owning user when the src - // resolves to exactly one user-owned node. - "sshtest-ip-literal-src": "headscale does not map an IP-literal sshTests src to the owning node's user; SaaS does", - // SaaS rejects `users: ["*"]` on an `ssh` rule at policy-parse // time with `user "*" is not valid`; headscale accepts the // wildcard and proceeds to evaluate sshTests against it. Fix in @@ -61,7 +51,6 @@ func TestSSHTesterCompat(t *testing.T) { } users := setupSSHDataCompatUsers() - nodes := setupSSHDataCompatNodes(users) for _, file := range files { c, err := testcapture.Read(file) @@ -74,6 +63,14 @@ func TestSSHTesterCompat(t *testing.T) { t.Skip(reason) } + // Per-capture nodes mean the topology IPs (which a + // policy `hosts` mapping references by literal IP) + // resolve to real nodes in the test fixture. Without + // this the static fixture's IPs do not overlap with + // the captures and host-alias dsts resolve to no + // nodes — that path is now a load-bearing failure. + nodes := buildGrantsNodesFromCapture(users, c) + policyJSON := []byte(c.Input.FullPolicy) pm, parseErr := NewPolicyManager(policyJSON, users, nodes.ViewSlice()) diff --git a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go index 33b24c23..12d533ce 100644 --- a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go @@ -61,61 +61,6 @@ func setupSSHDataCompatUsers() types.Users { } } -// setupSSHDataCompatNodes returns the test nodes for SSH data-driven -// compatibility tests. Node GivenNames match the anonymized pokémon names: -// - bulbasaur (owned by odin) -// - ivysaur (owned by thor) -// - venusaur (owned by freya) -// - beedrill (tag:server) -// - kakuna (tag:prod) -func setupSSHDataCompatNodes(users types.Users) types.Nodes { - return types.Nodes{ - &types.Node{ - ID: 1, - GivenName: "bulbasaur", - User: &users[0], - UserID: &users[0].ID, - IPv4: ptrAddr("100.90.199.68"), - IPv6: ptrAddr("fd7a:115c:a1e0::2d01:c747"), - Hostinfo: &tailcfg.Hostinfo{}, - }, - &types.Node{ - ID: 2, - GivenName: "ivysaur", - User: &users[1], - UserID: &users[1].ID, - IPv4: ptrAddr("100.110.121.96"), - IPv6: ptrAddr("fd7a:115c:a1e0::1737:7960"), - Hostinfo: &tailcfg.Hostinfo{}, - }, - &types.Node{ - ID: 3, - GivenName: "venusaur", - User: &users[2], - UserID: &users[2].ID, - IPv4: ptrAddr("100.103.90.82"), - IPv6: ptrAddr("fd7a:115c:a1e0::9e37:5a52"), - Hostinfo: &tailcfg.Hostinfo{}, - }, - &types.Node{ - ID: 4, - GivenName: "beedrill", - IPv4: ptrAddr("100.108.74.26"), - IPv6: ptrAddr("fd7a:115c:a1e0::b901:4a87"), - Tags: []string{"tag:server"}, - Hostinfo: &tailcfg.Hostinfo{}, - }, - &types.Node{ - ID: 5, - GivenName: "kakuna", - IPv4: ptrAddr("100.103.8.15"), - IPv6: ptrAddr("fd7a:115c:a1e0::5b37:80f"), - Tags: []string{"tag:prod"}, - Hostinfo: &tailcfg.Hostinfo{}, - }, - } -} - // loadSSHTestFile loads and parses a single SSH capture HuJSON file. func loadSSHTestFile(t *testing.T, path string) *testcapture.Capture { t.Helper() diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index c2b5f9b5..93887401 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -82,10 +82,11 @@ type SSHPolicyTest struct { // Src connects to each entry in Dst. Deny []string `json:"deny,omitempty"` - // Check lists SSH login users that must be allowed by an action:check - // rule specifically. action:accept matches do not satisfy a check - // assertion. Engine evaluation is not implemented yet; parse-time - // validation accepts the field so policies can be authored ahead of it. + // Check lists SSH login users that must reach every dst via an + // action:check rule specifically (the HoldAndDelegate signal on the + // compiled SSH policy). An action:accept rule alone does not satisfy + // a check assertion — SaaS keeps the two categories distinct so + // policy authors can pin sensitive logins to check rules. Check []string `json:"check,omitempty"` }