From fa6b46bfd970afc9cd06cb69628a138cbba56907 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 17:53:35 +0000 Subject: [PATCH] policy/v2: need every dst node for a tests accept, no range sources An accept passes only when each node behind the dst is reached, as on Tailscale SaaS; a deny keeps its rule. A CIDR, "*" or autogroup:tagged src names no user or host, so its test fails. --- .../policy/v2/policytester_compat_test.go | 10 --- hscontrol/policy/v2/test.go | 84 +++++++++++++++++-- hscontrol/policy/v2/test_test.go | 47 +++++++++++ hscontrol/servertest/policy_test.go | 4 - 4 files changed, 124 insertions(+), 21 deletions(-) diff --git a/hscontrol/policy/v2/policytester_compat_test.go b/hscontrol/policy/v2/policytester_compat_test.go index 0c2bcee0b..755fb0822 100644 --- a/hscontrol/policy/v2/policytester_compat_test.go +++ b/hscontrol/policy/v2/policytester_compat_test.go @@ -38,27 +38,17 @@ import ( // knownPolicyTesterDivergences lists checks where headscale's evaluator // differs from Tailscale SaaS, keyed /. var knownPolicyTesterDivergences = map[string]string{ //nolint:gosec // strings here are human-readable notes, not credentials - "policytest-accept-fail-autogroup-tagged-src-tag-dst/breakdown": "TEST_SRC_SHAPE: SaaS refuses autogroup:tagged as a tests source as an unknown user or host; headscale evaluates it", - "policytest-accept-fail-proto-icmp-not-allowed/breakdown": testEntryErrors, "policytest-proto-icmp/breakdown": testEntryErrors, "policytest-dst-unknown-group/breakdown": testUnknownAlias, "policytest-src-unknown-group/breakdown": testUnknownAlias, "policytest-src-unknown-user-email/breakdown": testUnknownAlias, - - "policytest-src-cidr-accept/verdict": testSrcShape, - "policytest-src-cidr-deny/breakdown": testSrcShape, - "policytest-src-wildcard-accept/breakdown": testSrcShape, - "policytest-src-wildcard-deny/verdict": testSrcShape, - - "policytest-dst-multinode-partial-accept/verdict": "TEST_DST_EVERY_NODE: SaaS accepts only when every node of the dst alias is reached; headscale when one is", } const ( testEntryErrors = "TEST_ENTRY_ERRORS: SaaS reports the first shape error of a tests entry, headscale every one" testUnknownAlias = "TEST_UNKNOWN_ALIAS: SaaS reports a test naming an unknown user or group as not reached, headscale as an evaluation error" - testSrcShape = "TEST_SRC_SHAPE: SaaS refuses a CIDR or \"*\" as a tests source as an unknown user or host; headscale evaluates it" ) // TestPolicyTesterCompat replays every capture under diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 3ad1e0be3..2f03657e4 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -10,6 +10,7 @@ import ( "github.com/go-json-experiment/json" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "go4.org/netipx" "tailscale.com/tailcfg" "tailscale.com/types/views" ) @@ -32,6 +33,7 @@ var ( errPolicyTestsFailed = errors.New("test(s) failed") errSSHPolicyTestsFailed = errors.New("test(s) failed") errTestDestinationNoIP = errors.New("destination resolved to no IP addresses") + errTestSourceUnknown = errors.New("unknown user or host") ) // PolicyTest is one entry in the policy's `tests` block. @@ -394,7 +396,7 @@ func runPolicyTest(test PolicyTest, pol *Policy, filter []tailcfg.FilterRule, us check := func(dsts []string, wantAllowed bool, ok, fail *[]string) { for _, dst := range dsts { - allowed, err := evalReachability(srcPrefixes, dst, test.Proto, pol, filter, users, nodes) + allowed, err := evalReachability(srcPrefixes, dst, test.Proto, pol, filter, users, nodes, wantAllowed) if err != nil { res.Passed = false res.Errors = append(res.Errors, fmt.Sprintf("error testing %q: %v", dst, err)) @@ -427,6 +429,21 @@ func resolveTestSource(src string, pol *Policy, users []types.User, nodes views. return nil, fmt.Errorf("invalid alias: %w", err) } + // Tailscale SaaS finds no user or host behind these, so a test from + // them fails, whether it accepts or denies. + unknown := isAddressRange(pol, alias) + + switch a := alias.(type) { + case Asterix: + unknown = true + case *AutoGroup: + unknown = a.Is(AutoGroupTagged) + } + + if unknown { + return nil, fmt.Errorf("%w: %q", errTestSourceUnknown, src) + } + // Rules tolerate unregistered users; a test naming one must still // fail (SaaS: policytest-src-unknown-user-email). if u, ok := alias.(*Username); ok { @@ -448,13 +465,25 @@ func resolveTestSource(src string, pol *Policy, users []types.User, nodes views. return addrs.Prefixes(), nil } -// evalReachability reports whether traffic from any srcPrefix to dst (in -// `host:port` form) is allowed by filter for the requested protocol. +// evalReachability reports whether traffic from every srcPrefix to dst (in +// `host:port` form) is allowed by filter for the requested protocol. With +// everyNode, as an accept asks, each node behind dst must be reached, as on +// Tailscale SaaS; otherwise any address of dst will do, so a deny passes only +// when none is reached. // // Empty proto means the default set the client applies when proto is // omitted (TCP/UDP/ICMP) — we accept a rule whose IPProto list contains // any of those, or rules with no IPProto restriction at all. -func evalReachability(srcPrefixes []netip.Prefix, dst string, proto Protocol, pol *Policy, filter []tailcfg.FilterRule, users []types.User, nodes views.Slice[types.NodeView]) (bool, error) { +func evalReachability( + srcPrefixes []netip.Prefix, + dst string, + proto Protocol, + pol *Policy, + filter []tailcfg.FilterRule, + users []types.User, + nodes views.Slice[types.NodeView], + everyNode bool, +) (bool, error) { awp, err := parseDestinationAlias(dst) if err != nil { return false, fmt.Errorf("invalid destination %q: %w", dst, err) @@ -469,19 +498,60 @@ func evalReachability(srcPrefixes []netip.Prefix, dst string, proto Protocol, po return false, fmt.Errorf("%w: %q", errTestDestinationNoIP, dst) } - dstPrefixes := dstAddrs.Prefixes() + targets := [][]netip.Prefix{dstAddrs.Prefixes()} + if everyNode { + targets = nodeTargets(dstAddrs, nodes) + } // Tailscale's tests semantics: ALL src prefixes must reach the dst for // the test to consider it allowed. A partial allow is a fail. for _, src := range srcPrefixes { - if !srcReachesDst(src, dstPrefixes, awp.Ports, proto, filter) { - return false, nil + for _, target := range targets { + if !srcReachesDst(src, target, awp.Ports, proto, filter) { + return false, nil + } } } return true, nil } +// nodeTargets splits dst into the addresses of each node it holds and, +// when any is left, the addresses no node holds. +func nodeTargets(dst ResolvedAddresses, nodes views.Slice[types.NodeView]) [][]netip.Prefix { + var ( + out [][]netip.Prefix + rest netipx.IPSetBuilder + ) + + for _, p := range dst.Prefixes() { + rest.AddPrefix(p) + } + + for _, n := range nodes.All() { + var own []netip.Prefix + + for _, ip := range n.IPs() { + if dst.Contains(ip) { + own = append(own, netip.PrefixFrom(ip, ip.BitLen())) + rest.Remove(ip) + } + } + + if len(own) > 0 { + out = append(out, own) + } + } + + // The builder only holds valid prefixes, so it cannot fail. + set, _ := rest.IPSet() + if left := set.Prefixes(); len(left) > 0 { + out = append(out, left) + } + + return out +} + // parseDestinationAlias is a thin wrapper over [AliasWithPorts.UnmarshalJSON] // so callers can hand it a bare `"host:port"` string without re-implementing // the parse logic. diff --git a/hscontrol/policy/v2/test_test.go b/hscontrol/policy/v2/test_test.go index 0a41f11d0..bcb306398 100644 --- a/hscontrol/policy/v2/test_test.go +++ b/hscontrol/policy/v2/test_test.go @@ -103,6 +103,53 @@ func TestRunTests(t *testing.T) { wantErrSub: []string{"bob@headscale.net", "tag:server:22", "expected ALLOWED"}, wantNoErrIs: errPolicyTestsFailed, }, + { + // An accept needs every node behind the dst; a deny fails + // as soon as one is reached. + name: "dst-alias-partly-reached", + policy: `{ + "groups": { "group:people": ["alice@headscale.net", "bob@headscale.net"] }, + "acls": [{ + "action": "accept", + "src": ["alice@headscale.net"], + "dst": ["alice@headscale.net:22"] + }], + "tests": [{ + "src": "alice@headscale.net", + "accept": ["group:people:22"], + "deny": ["group:people:22"] + }] + }`, + wantPass: false, + wantErrSub: []string{ + "alice@headscale.net -> group:people:22: expected ALLOWED, got DENIED", + "alice@headscale.net -> group:people:22: expected DENIED, got ALLOWED", + }, + wantNoErrIs: errPolicyTestsFailed, + }, + { + name: "src-wildcard-not-reached", + policy: `{ + "tagOwners": { "tag:server": ["alice@headscale.net"] }, + "acls": [{ + "action": "accept", + "src": ["alice@headscale.net"], + "dst": ["tag:server:22"] + }], + "tests": [ + { "src": "*", "deny": ["tag:server:80"] }, + { "src": "100.64.0.0/30", "deny": ["tag:server:80"] }, + { "src": "autogroup:tagged", "deny": ["tag:server:80"] } + ] + }`, + wantPass: false, + wantErrSub: []string{ + `unknown user or host: "*"`, + `unknown user or host: "100.64.0.0/30"`, + `unknown user or host: "autogroup:tagged"`, + }, + wantNoErrIs: errPolicyTestsFailed, + }, { name: "deny-fail-policy-allows-traffic", policy: `{ diff --git a/hscontrol/servertest/policy_test.go b/hscontrol/servertest/policy_test.go index b7d76fd9f..766d6a3f8 100644 --- a/hscontrol/servertest/policy_test.go +++ b/hscontrol/servertest/policy_test.go @@ -323,10 +323,6 @@ var rejectedPolicySkipReasons = map[string]string{ "ssh-check-tag-src/refused": "SSH_CHECK_TAG_SRC: headscale accepts a check rule with a tag source", "grant-dxp-comma-ports/refused": "GRANT_PORT_LIST: headscale accepts a comma port list in a grant ip entry", - - "policytest-src-cidr-accept/refused": "TEST_SRC_SHAPE: headscale evaluates a CIDR tests source", - "policytest-src-wildcard-deny/refused": "TEST_SRC_SHAPE: headscale evaluates \"*\" as a tests source", - "policytest-dst-multinode-partial-accept/refused": "TEST_DST_EVERY_NODE: headscale accepts when one node of the dst alias is reached", } const noTailnetDomains = "NO_TAILNET_DOMAINS: headscale does not check user:*@domain or localpart:*@domain against the tailnet's domains"