From 6fd3dbd0a2eaa2bf2ff085f8ba3cccd633117127 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 14:03:33 +0000 Subject: [PATCH] policy/v2: evaluate sshTests sources through the nodes they cover A src was expanded to every address it names, so "*" or ::/0 never finished. Principals are node addresses: keep those plus one stand-in. --- hscontrol/policy/v2/sshtest.go | 37 +++++++++- hscontrol/policy/v2/sshtest_test.go | 105 ++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index 5d01d29d6..cd932fd70 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -385,6 +385,12 @@ func appendUserDst(m map[string][]string, user, dst string) map[string][]string // resolveSSHTestSource returns the src's principal addresses and, for // user-shaped sources, the user ID (so autogroup:self can scope to it). // [Tag], [Host], and IP sources return userID 0. +// +// SSH principals are always node addresses, so a src is represented by +// the node addresses it covers plus one address standing in for all +// the others, which no rule can match. The result is bounded by the +// node count; a CIDR, `*`, or autogroup:internet src is never walked +// address by address. func resolveSSHTestSource( src Alias, pol *Policy, @@ -412,7 +418,36 @@ func resolveSSHTestSource( return nil, 0, nil } - out := slices.Collect(addrs.Iter()) + var ( + out []netip.Addr + rest netipx.IPSetBuilder + ) + + for _, p := range addrs.Prefixes() { + rest.AddPrefix(p) + } + + for _, n := range nodes.All() { + for _, ip := range n.IPs() { + if addrs.Contains(ip) { + out = append(out, ip) + rest.Remove(ip) + } + } + } + + restSet, err := rest.IPSet() + if err != nil { + return nil, 0, fmt.Errorf("resolving: %w", err) + } + + if ranges := restSet.Ranges(); len(ranges) > 0 { + out = append(out, ranges[0].From()) + } + + // Address order, as a full walk has: the first failing address + // decides how a check failure is rendered. + slices.SortFunc(out, netip.Addr.Compare) var userID uint diff --git a/hscontrol/policy/v2/sshtest_test.go b/hscontrol/policy/v2/sshtest_test.go index a4973b94c..d2d5595e6 100644 --- a/hscontrol/policy/v2/sshtest_test.go +++ b/hscontrol/policy/v2/sshtest_test.go @@ -2,6 +2,7 @@ package v2 import ( "encoding/json" + "net/netip" "strings" "testing" @@ -747,6 +748,88 @@ func TestRunSSHTests(t *testing.T) { wantPass: false, wantErrSub: []string{"autogroup:self", "resolved to no nodes"}, }, + { + // A src naming address space is evaluated through the node + // addresses inside it, never address by address: ::/0 + // holds alice's IPv6, which the rule allows. + name: "src-cidr-deny-fails-on-covered-node", + policy: `{ + "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "ssh": [{ + "action": "accept", + "src": ["alice@headscale.net"], + "dst": ["tag:server"], + "users": ["root"] + }], + "sshTests": [{ + "src": "::/0", + "dst": ["tag:server"], + "deny": ["root"] + }] + }`, + wantPass: false, + wantErrSub: []string{"::/0", "expected DENIED, got ALLOWED"}, + }, + { + name: "src-address-space-deny-pass", + policy: `{ + "hosts": { "lan": "10.0.0.0/8" }, + "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "ssh": [{ + "action": "accept", + "src": ["alice@headscale.net"], + "dst": ["tag:server"], + "users": ["root"] + }], + "sshTests": [ + { "src": "*", "dst": ["tag:server"], "deny": ["ubuntu"] }, + { "src": "autogroup:danger-all", "dst": ["tag:server"], "deny": ["ubuntu"] }, + { "src": "autogroup:internet", "dst": ["tag:server"], "deny": ["root"] }, + { "src": "lan", "dst": ["tag:server"], "deny": ["root"] } + ] + }`, + wantPass: true, + }, + { + // Every node is allowed, but `*` also covers addresses no + // node holds, and all src addresses must agree. + name: "src-wildcard-accept-fails-on-unassigned-address", + policy: `{ + "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "ssh": [{ + "action": "accept", + "src": ["autogroup:member", "autogroup:tagged"], + "dst": ["tag:server"], + "users": ["root"] + }], + "sshTests": [{ + "src": "*", + "dst": ["tag:server"], + "accept": ["root"] + }] + }`, + wantPass: false, + wantErrSub: []string{"expected ALLOWED, got DENIED"}, + }, + { + // 100.64.0.4/31 is exactly alice-tablet and thor-laptop. + name: "src-cidr-fully-assigned-accept-pass", + policy: `{ + "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "ssh": [{ + "action": "accept", + "src": ["autogroup:member"], + "dst": ["tag:server"], + "users": ["root"] + }], + "sshTests": [{ + "src": "100.64.0.4/31", + "dst": ["tag:server"], + "accept": ["root"] + }] + }`, + wantPass: true, + }, } for _, tt := range tests { @@ -772,6 +855,28 @@ func TestRunSSHTests(t *testing.T) { } } +// TestResolveSSHTestSourceIsBoundedByNodes pins that a src naming address +// space resolves to the node addresses it covers plus one address standing +// in for the rest, not to every address in it. +func TestResolveSSHTestSourceIsBoundedByNodes(t *testing.T) { + users := sshTestUsers() + nodes := sshTestNodes(users) + + src := Prefix(netip.MustParsePrefix("100.64.0.0/16")) + + got, _, err := resolveSSHTestSource(&src, nil, users, nodes.ViewSlice()) + require.NoError(t, err) + + want := make([]netip.Addr, 0, 1+len(nodes)) + want = append(want, netip.MustParseAddr("100.64.0.0")) + + for _, n := range nodes { + want = append(want, *n.IPv4) + } + + require.Equal(t, want, got) +} + // TestRunSSHTestsBothTestsPassSSHTestsFail captures the distinction the // caller cares about: a passing ACL `tests` block plus a failing // `sshTests` block returns errSSHPolicyTestsFailed and NOT