From f172dba0e33b7a4140f805fbd9444eb67baf139b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 29 Apr 2026 14:34:10 +0000 Subject: [PATCH] policy/v2: validate tests block at parse boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `tests` entry describes one connection attempt to one specific host on one specific port over a connection-oriented protocol, and asserts whether it is allowed or denied. Five shape rules follow — single-port dst, proto in {tcp, udp, sctp, ""}, no autogroup:internet dst, no CIDR-typed dst (raw `/N` or hosts:-alias to a multi-host prefix), at least one of accept/deny — and every one was previously silently accepted by headscale even though Tailscale SaaS rejects them as "test(s) failed". Enforce them in one pass over `pol.Tests` from `Policy.validate()`, reusing the existing parse-time multierr aggregation. The same shapes remain valid inside ACL or Grant destinations where the rule does not apply; the validator only walks the tests array. The compat runner now treats parse-time errors equivalently to SetPolicy errors so the captured Tailscale body still matches via substring regardless of which step surfaces the rejection. Nine divergences resolved by this validation pass drop out of knownPolicyTesterDivergences. Updates #1803 --- .../policy/v2/policytester_compat_test.go | 48 ++++--- hscontrol/policy/v2/test_test.go | 21 +-- hscontrol/policy/v2/types.go | 91 ++++++++++++ hscontrol/policy/v2/types_test.go | 136 ++++++++++++++++++ 4 files changed, 257 insertions(+), 39 deletions(-) diff --git a/hscontrol/policy/v2/policytester_compat_test.go b/hscontrol/policy/v2/policytester_compat_test.go index 0285e51c..ba0a2b1f 100644 --- a/hscontrol/policy/v2/policytester_compat_test.go +++ b/hscontrol/policy/v2/policytester_compat_test.go @@ -37,18 +37,9 @@ import ( // Each entry is a real bug to fix in a follow-up; documenting them here // keeps the compat suite green and the divergence list visible. var knownPolicyTesterDivergences = map[string]string{ //nolint:gosec // strings here are human-readable notes, not credentials - "policytest-allpass-acls-and-grants-mixed": "evaluator denies tag:client → webserver:80 in mixed acls+grants policy; SaaS accepts (Updates #1803)", - "policytest-proto-numeric": "validateProtocolPortCompatibility rejects numeric proto \"6\" with specific ports; SaaS accepts (Updates #1803)", - "policytest-accept-fail-proto-numeric-mismatch": "validateProtocolPortCompatibility rejects numeric proto \"6\" with specific ports; SaaS accepts (Updates #1803)", - "policytest-port-shape-range": "SaaS rejects port-range syntax in test dst; evaluator accepts (Updates #1803)", - "policytest-port-shape-list": "SaaS rejects port-list syntax in test dst; evaluator accepts (Updates #1803)", - "policytest-port-shape-wildcard": "SaaS rejects port-wildcard in test dst when paired with non-wildcard rule; evaluator accepts (Updates #1803)", - "policytest-proto-icmp": "SaaS rejects proto=icmp tests; evaluator accepts (Updates #1803)", - "policytest-deny-fail-autogroup-internet-dst": "SaaS rejects deny-fail with autogroup:internet dst; evaluator passes (Updates #1803)", - "policytest-malformed-test-missing-accept-and-deny": "SaaS rejects test entry with neither accept nor deny; evaluator passes (Updates #1803)", - "policytest-allpass-grants-only-tag-src-cidr-dst": "SaaS rejects test dst that resolves to a multi-host CIDR; evaluator accepts based on filter coverage (Updates #1803)", - "policytest-allpass-grants-only-tag-src-prefix32-dst": "SaaS rejects /32 CIDR dst (distinct from bare IP literal); evaluator accepts (Updates #1803)", - "policytest-allpass-grants-only-tag-src-host-alias-cidr-dst": "SaaS rejects hosts:-alias to CIDR as test dst; evaluator accepts (Updates #1803)", + "policytest-allpass-acls-and-grants-mixed": "evaluator denies tag:client → webserver:80 in mixed acls+grants policy; SaaS accepts (Updates #1803)", + "policytest-proto-numeric": "validateProtocolPortCompatibility rejects numeric proto \"6\" with specific ports; SaaS accepts (Updates #1803)", + "policytest-accept-fail-proto-numeric-mismatch": "validateProtocolPortCompatibility rejects numeric proto \"6\" with specific ports; SaaS accepts (Updates #1803)", } // policyTesterCompatUsers / policyTesterCompatNodes mirror the small @@ -141,29 +132,42 @@ func TestPolicyTesterCompat(t *testing.T) { policyJSON := []byte(c.Input.FullPolicy) - pm, err := NewPolicyManager(policyJSON, users, nodes.ViewSlice()) - require.NoError(t, err, "policy must parse during boot path") - - _, setErr := pm.SetPolicy(policyJSON) + pm, parseErr := NewPolicyManager(policyJSON, users, nodes.ViewSlice()) + // Tailscale validates and runs tests as one POST step: + // either failure mode produces the same 400. Headscale + // splits structural validation (parse) from test + // evaluation (SetPolicy). For the compat assertion, the + // two are equivalent — whichever surfaces first carries + // the captured body. if c.Input.APIResponseCode == 200 { + require.NoError(t, parseErr, "tailscale accepted this policy; headscale must parse it") + + _, setErr := pm.SetPolicy(policyJSON) require.NoError(t, setErr, "tailscale accepted this policy; headscale tests should pass") return } - // Failure case: tailscale rejected the policy with a body - // captured in APIResponseBody.Message. Our error must - // contain that body as a substring. - require.Error(t, setErr, "tailscale rejected; headscale tests should also fail") + var got error + + switch { + case parseErr != nil: + got = parseErr + default: + _, setErr := pm.SetPolicy(policyJSON) + got = setErr + } + + require.Error(t, got, "tailscale rejected; headscale must reject too") if c.Input.APIResponseBody == nil || c.Input.APIResponseBody.Message == "" { return } want := c.Input.APIResponseBody.Message - if !strings.Contains(setErr.Error(), want) { - t.Errorf("error body mismatch\n tailscale wants: %q\n headscale got: %q", want, setErr.Error()) + if !strings.Contains(got.Error(), want) { + t.Errorf("error body mismatch\n tailscale wants: %q\n headscale got: %q", want, got.Error()) } }) } diff --git a/hscontrol/policy/v2/test_test.go b/hscontrol/policy/v2/test_test.go index b87a1856..e6849da7 100644 --- a/hscontrol/policy/v2/test_test.go +++ b/hscontrol/policy/v2/test_test.go @@ -138,23 +138,10 @@ func TestRunTests(t *testing.T) { wantErrSub: []string{"ghost@headscale.net", "failed to resolve source"}, wantNoErrIs: errPolicyTestsFailed, }, - { - name: "malformed-dst-missing-port", - policy: `{ - "acls": [{ - "action": "accept", - "src": ["*"], - "dst": ["*:*"] - }], - "tests": [{ - "src": "alice@headscale.net", - "accept": ["alice-laptop"] - }] - }`, - wantPass: false, - wantErrSub: []string{"alice-laptop", "error testing"}, - wantNoErrIs: errPolicyTestsFailed, - }, + // "malformed-dst-missing-port" used to live here; structural + // shape errors are now caught at parse by validateTests, so + // RunTests no longer sees them. The parse-side behaviour is + // covered by TestUnmarshalPolicy/tests-* in types_test.go. { name: "wildcard-src-passes", policy: `{ diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 13fffa94..435616ba 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -125,6 +125,11 @@ var ( ErrUnknownSSHSrcAlias = errors.New("unknown SSH source alias type") ErrUnknownField = errors.New("unknown field") ErrProtocolNoSpecificPorts = errors.New("protocol does not support specific ports") + ErrTestEmptyAssertions = errors.New("test entry must have at least one of \"accept\" or \"deny\"") + ErrTestProtocolNotAllowed = errors.New("test protocol must be tcp, udp, sctp, or empty") + ErrTestDestinationMultiPort = errors.New("test destination port must be a single port") + ErrTestDestinationCIDR = errors.New("test destination must be a single host, not a CIDR range") + ErrAutogroupInternetTestDst = errors.New("autogroup:internet not valid as a test destination") ) type resolved struct { @@ -2676,6 +2681,10 @@ func (p *Policy) validate() error { } } + if err := validateTests(p, p.Tests); err != nil { //nolint:noinlineerr + errs = append(errs, err) + } + if len(errs) > 0 { return multierr.New(errs...) } @@ -3052,3 +3061,85 @@ func validateProtocolPortCompatibility(protocol Protocol, destinations []AliasWi return nil } + +// validateTests enforces the four shape rules a tests-block entry must +// follow: a tests entry describes one connection attempt to one specific +// destination port over a connection-oriented protocol and asserts +// whether that attempt is allowed or denied. The same shapes remain +// valid inside ACL or Grant destinations where the rule does not apply. +func validateTests(pol *Policy, tests []PolicyTest) error { + var errs []error + + for i, t := range tests { + if len(t.Accept) == 0 && len(t.Deny) == 0 { + errs = append(errs, fmt.Errorf("test %d: %w", i, ErrTestEmptyAssertions)) + } + + if t.Proto != "" && + t.Proto != ProtocolNameTCP && + t.Proto != ProtocolNameUDP && + t.Proto != ProtocolNameSCTP { + errs = append(errs, fmt.Errorf("test %d: %w: %q", i, ErrTestProtocolNotAllowed, t.Proto)) + } + + for _, dst := range t.Accept { + err := validateTestDestination(pol, dst) + if err != nil { + errs = append(errs, fmt.Errorf("test %d, accept %q: %w", i, dst, err)) + } + } + + for _, dst := range t.Deny { + err := validateTestDestination(pol, dst) + if err != nil { + errs = append(errs, fmt.Errorf("test %d, deny %q: %w", i, dst, err)) + } + } + } + + if len(errs) > 0 { + return fmt.Errorf("%w:\n%w", errPolicyTestsFailed, multierr.New(errs...)) + } + + return nil +} + +// validateTestDestination enforces that a tests-block dst describes one +// connection attempt to one specific host on one specific port. SaaS +// rejects three shapes that violate the rule: autogroup:internet (routed +// by exit-node AllowedIPs, not the packet filter); multi-port +// (range/list/wildcard, no single allow/deny answer); and CIDR ranges +// — both raw `/N` syntax and `hosts:`-table aliases whose RHS is a +// multi-host prefix. Bare IP literals reach this function as *Prefix +// /32 or /128 just like explicit `/32` / `/128` does, so the CIDR +// check inspects the raw input string for `/` rather than the parsed +// alias type. +func validateTestDestination(pol *Policy, dst string) error { + awp, err := parseDestinationAlias(dst) + if err != nil { + return err + } + + if ag, ok := awp.Alias.(*AutoGroup); ok && *ag == AutoGroupInternet { + return ErrAutogroupInternetTestDst + } + + if len(awp.Ports) != 1 || awp.Ports[0].First != awp.Ports[0].Last { + return ErrTestDestinationMultiPort + } + + if _, isPrefix := awp.Alias.(*Prefix); isPrefix && strings.Contains(dst, "/") { + return ErrTestDestinationCIDR + } + + if h, isHost := awp.Alias.(*Host); isHost && pol != nil { + if pref, ok := pol.Hosts[*h]; ok { + p := netip.Prefix(pref) + if p.Bits() < p.Addr().BitLen() { + return ErrTestDestinationCIDR + } + } + } + + return nil +} diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index ec2ad751..e9e024b3 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -2051,6 +2051,142 @@ func TestUnmarshalPolicy(t *testing.T) { `, wantErr: "invalid localpart format", }, + // A test entry with neither accept nor deny asserts nothing + // and is silently accepted today. Tailscale rejects the policy. + { + name: "tests-empty-assertions", + input: ` +{ + "acls": [ + {"action": "accept", "src": ["*"], "dst": ["*:22"]} + ], + "tests": [ + {"src": "*"} + ] +} +`, + wantErr: `test entry must have at least one of "accept" or "deny"`, + }, + // Tests can only describe connection-oriented reachability, so + // proto must be tcp, udp, sctp, or empty. Tailscale rejects + // proto=icmp on a test entry even though icmp is valid in rules. + { + name: "tests-proto-icmp-not-allowed", + input: ` +{ + "acls": [ + {"action": "accept", "proto": "icmp", "src": ["*"], "dst": ["*:*"]} + ], + "tests": [ + {"src": "*", "proto": "icmp", "accept": ["*:*"]} + ] +} +`, + wantErr: `test protocol must be tcp, udp, sctp, or empty`, + }, + // A test asserts one connection attempt to one specific port. + // Port ranges, lists, and wildcards conflate multiple attempts + // and have no single answer; Tailscale rejects them in tests. + { + name: "tests-dst-port-range-not-allowed", + input: ` +{ + "acls": [ + {"action": "accept", "src": ["*"], "dst": ["*:8000-8100"]} + ], + "tests": [ + {"src": "*", "accept": ["*:8000-8100"]} + ] +} +`, + wantErr: `test destination port must be a single port`, + }, + // autogroup:internet is delivered via exit-node AllowedIPs, not + // packet-filter rules, so reachability to it has no meaning in + // the filter model used by tests. Tailscale rejects it in + // test destinations even though it is valid in rule destinations. + { + name: "tests-dst-autogroup-internet-not-allowed", + input: ` +{ + "tagOwners": { + "tag:client": ["admin@example.com"] + }, + "acls": [ + {"action": "accept", "src": ["tag:client"], "dst": ["autogroup:internet:*"]} + ], + "tests": [ + {"src": "tag:client", "deny": ["autogroup:internet:*"]} + ] +} +`, + wantErr: `autogroup:internet not valid as a test destination`, + }, + // A test asserts one connection attempt to one specific host. + // Tailscale rejects raw `/N` notation in test dsts even when + // the prefix is `/32` — the alias parser distinguishes a bare + // IP literal from an explicit single-host CIDR even though + // they cover the same address. + { + name: "tests-dst-cidr-prefix32-not-allowed", + input: ` +{ + "tagOwners": { + "tag:client": ["admin@example.com"] + }, + "acls": [ + {"action": "accept", "src": ["tag:client"], "dst": ["100.64.0.16/32:22"]} + ], + "tests": [ + {"src": "tag:client", "accept": ["100.64.0.16/32:22"]} + ] +} +`, + wantErr: `test destination must be a single host, not a CIDR range`, + }, + // Multi-host prefixes in test dsts are rejected for the same + // reason port ranges are: the assertion has no single answer + // once the prefix covers more than one host. + { + name: "tests-dst-cidr-multi-host-not-allowed", + input: ` +{ + "tagOwners": { + "tag:client": ["admin@example.com"] + }, + "acls": [ + {"action": "accept", "src": ["tag:client"], "dst": ["10.0.0.0/8:22"]} + ], + "tests": [ + {"src": "tag:client", "accept": ["10.0.0.0/8:22"]} + ] +} +`, + wantErr: `test destination must be a single host, not a CIDR range`, + }, + // hosts: aliases with a multi-host RHS are rejected after + // resolution. The alias text has no slash but the alias still + // resolves to a range, which is the rule SaaS enforces. + { + name: "tests-dst-host-alias-cidr-not-allowed", + input: ` +{ + "tagOwners": { + "tag:client": ["admin@example.com"] + }, + "hosts": { + "internal": "10.0.0.0/8" + }, + "acls": [ + {"action": "accept", "src": ["tag:client"], "dst": ["internal:22"]} + ], + "tests": [ + {"src": "tag:client", "accept": ["internal:22"]} + ] +} +`, + wantErr: `test destination must be a single host, not a CIDR range`, + }, } cmps := append(util.Comparers,