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,