From d5b2837231d4b79f16d4081efcd434c44106d45e Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 29 Apr 2026 12:26:20 +0000 Subject: [PATCH] policy/v2: match default proto set for tests with no proto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The policy `tests` block lets entries omit `proto`. Tailscale's client maps that to the default protocol set {TCP, UDP, ICMP, ICMPv6} — the captured packet_filter_matches show all four IANA numbers explicitly when no proto is set — and a rule restricted to any one of them satisfies an empty-proto reachability test. srcReachesDst was passing the empty Protocol through unchanged, which landed an empty []int in ruleMatchesProto. The matcher then short- circuited to "no match" for every rule with a non-empty IPProto restriction, including TCP-only grants compiled from `ip: ["tcp:80"]`. The bug surfaced in the captured allpass-acls-and-grants-mixed scenario: the grant `tag:client → webserver:80` was reachable in the compiled filter but the empty-proto test could not see it. Expand the empty Protocol to the default set at the call site so ruleMatchesProto's intersection check sees the right requested protocols. Drop the now-dead empty-requestedProtos branch from the matcher. The last divergence drops out of knownPolicyTesterDivergences as a result. Updates #1803 --- .../policy/v2/policytester_compat_test.go | 4 +- hscontrol/policy/v2/test.go | 23 ++++---- hscontrol/policy/v2/test_test.go | 55 +++++++++++++++++++ 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/hscontrol/policy/v2/policytester_compat_test.go b/hscontrol/policy/v2/policytester_compat_test.go index 9feb1537..47f6e955 100644 --- a/hscontrol/policy/v2/policytester_compat_test.go +++ b/hscontrol/policy/v2/policytester_compat_test.go @@ -36,9 +36,7 @@ import ( // disagrees with Tailscale SaaS on whether the policy should be accepted. // 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)", -} +var knownPolicyTesterDivergences = map[string]string{} //nolint:gosec // strings here are human-readable notes, not credentials // policyTesterCompatUsers / policyTesterCompatNodes mirror the small // shared topology used to record the captures. When more captures land diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 4f1d5e27..a93b7c6b 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -314,8 +314,17 @@ func parseDestinationAlias(dst string) (*AliasWithPorts, error) { // srcReachesDst walks the compiled filter rules and reports whether // traffic from src to any prefix in dstPrefixes on at least one of ports // (or any port when ports is empty) is allowed under proto. +// +// An empty test proto means the Tailscale client default set +// {TCP, UDP, ICMP, ICMPv6} — the protocols the client tries when proto +// is omitted. The captured Tailscale matches show these four IANA +// numbers explicitly when no proto is set, so a rule restricted to any +// of them satisfies an empty-proto test. func srcReachesDst(src netip.Prefix, dstPrefixes []netip.Prefix, ports []tailcfg.PortRange, proto Protocol, filter []tailcfg.FilterRule) bool { requestedProtos := proto.toIANAProtocolNumbers() + if len(requestedProtos) == 0 { + requestedProtos = []int{ProtocolTCP, ProtocolUDP, ProtocolICMP, ProtocolIPv6ICMP} + } for _, rule := range filter { if !ruleMatchesSource(rule, src) { @@ -354,21 +363,15 @@ func ruleMatchesSource(rule tailcfg.FilterRule, src netip.Prefix) bool { return false } -// ruleMatchesProto reports whether the rule permits the requested -// protocols. An unset rule.IPProto means "any protocol" and matches -// everything; an empty requestedProtos (proto == "") means the default -// set, which matches any rule including unset ones. +// ruleMatchesProto reports whether the rule permits any of requestedProtos. +// An unset rule.IPProto means "any protocol" and matches everything. +// requestedProtos is the per-test protocol set: a single proto for an +// explicit test.Proto, or the default set when test.Proto is empty. func ruleMatchesProto(rule tailcfg.FilterRule, requestedProtos []int) bool { if len(rule.IPProto) == 0 { return true } - if len(requestedProtos) == 0 { - // Default set: a rule restricted to a non-default protocol does - // not match the default request. - return false - } - for _, ruleProto := range rule.IPProto { if slices.Contains(requestedProtos, ruleProto) { return true diff --git a/hscontrol/policy/v2/test_test.go b/hscontrol/policy/v2/test_test.go index e6849da7..fc29f483 100644 --- a/hscontrol/policy/v2/test_test.go +++ b/hscontrol/policy/v2/test_test.go @@ -338,6 +338,61 @@ func TestNewPolicyManagerSkipsTests(t *testing.T) { require.ErrorIs(t, err, errPolicyTestsFailed) } +// TestRunTestsEmptyProtoMatchesDefaultProtocols captures the bug where a +// test entry with no `proto` field fails to match a filter rule whose +// IPProto is restricted to a default protocol (TCP, UDP, ICMP, ICMPv6). +// Tailscale's client default set is {6, 17, 1, 58} when proto is omitted, +// so a TCP-only rule must satisfy an empty-proto test. +// +// The capture +// testdata/policytest_results/policytest-allpass-acls-and-grants-mixed.hujson +// is the captured signal for this same bug (api_response_code 200, two +// passing tests including `tag:client → webserver:80` with no proto over +// a `ip: tcp:80` grant). +func TestRunTestsEmptyProtoMatchesDefaultProtocols(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "odin", Email: "odin@example.com"}, + } + nodes := types.Nodes{ + { + ID: 1, + Hostname: "client", + IPv4: ap("100.64.0.10"), + IPv6: ap("fd7a:115c:a1e0::a"), + Tags: []string{"tag:client"}, + }, + { + ID: 2, + Hostname: "webserver", + IPv4: ap("100.64.0.16"), + IPv6: ap("fd7a:115c:a1e0::10"), + Tags: []string{"tag:server"}, + }, + } + + policy := `{ + "tagOwners": { + "tag:client": ["odin@example.com"], + "tag:server": ["odin@example.com"] + }, + "hosts": { + "webserver": "100.64.0.16" + }, + "grants": [ + {"src": ["tag:client"], "dst": ["webserver"], "ip": ["tcp:80"]} + ], + "tests": [ + {"src": "tag:client", "accept": ["webserver:80"]} + ] + }` + + pm, err := NewPolicyManager([]byte(policy), users, nodes.ViewSlice()) + require.NoError(t, err, "policy must parse and compile") + + require.NoError(t, pm.RunTests(), + "empty-proto test must match a tcp-only grant rule (TCP is in the client default set)") +} + // TestPolicyTestResultsErrorsRendering checks the multi-line render layout // since the body becomes the user-facing error. func TestPolicyTestResultsErrorsRendering(t *testing.T) {