policy/v2: match default proto set for tests with no proto
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
This commit is contained in:
parent
e4e209f919
commit
d5b2837231
@ -36,9 +36,7 @@ import (
|
|||||||
// disagrees with Tailscale SaaS on whether the policy should be accepted.
|
// 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
|
// 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.
|
// 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
|
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)",
|
|
||||||
}
|
|
||||||
|
|
||||||
// policyTesterCompatUsers / policyTesterCompatNodes mirror the small
|
// policyTesterCompatUsers / policyTesterCompatNodes mirror the small
|
||||||
// shared topology used to record the captures. When more captures land
|
// shared topology used to record the captures. When more captures land
|
||||||
|
|||||||
@ -314,8 +314,17 @@ func parseDestinationAlias(dst string) (*AliasWithPorts, error) {
|
|||||||
// srcReachesDst walks the compiled filter rules and reports whether
|
// srcReachesDst walks the compiled filter rules and reports whether
|
||||||
// traffic from src to any prefix in dstPrefixes on at least one of ports
|
// 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.
|
// (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 {
|
func srcReachesDst(src netip.Prefix, dstPrefixes []netip.Prefix, ports []tailcfg.PortRange, proto Protocol, filter []tailcfg.FilterRule) bool {
|
||||||
requestedProtos := proto.toIANAProtocolNumbers()
|
requestedProtos := proto.toIANAProtocolNumbers()
|
||||||
|
if len(requestedProtos) == 0 {
|
||||||
|
requestedProtos = []int{ProtocolTCP, ProtocolUDP, ProtocolICMP, ProtocolIPv6ICMP}
|
||||||
|
}
|
||||||
|
|
||||||
for _, rule := range filter {
|
for _, rule := range filter {
|
||||||
if !ruleMatchesSource(rule, src) {
|
if !ruleMatchesSource(rule, src) {
|
||||||
@ -354,21 +363,15 @@ func ruleMatchesSource(rule tailcfg.FilterRule, src netip.Prefix) bool {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
// ruleMatchesProto reports whether the rule permits the requested
|
// ruleMatchesProto reports whether the rule permits any of requestedProtos.
|
||||||
// protocols. An unset rule.IPProto means "any protocol" and matches
|
// An unset rule.IPProto means "any protocol" and matches everything.
|
||||||
// everything; an empty requestedProtos (proto == "") means the default
|
// requestedProtos is the per-test protocol set: a single proto for an
|
||||||
// set, which matches any rule including unset ones.
|
// explicit test.Proto, or the default set when test.Proto is empty.
|
||||||
func ruleMatchesProto(rule tailcfg.FilterRule, requestedProtos []int) bool {
|
func ruleMatchesProto(rule tailcfg.FilterRule, requestedProtos []int) bool {
|
||||||
if len(rule.IPProto) == 0 {
|
if len(rule.IPProto) == 0 {
|
||||||
return true
|
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 {
|
for _, ruleProto := range rule.IPProto {
|
||||||
if slices.Contains(requestedProtos, ruleProto) {
|
if slices.Contains(requestedProtos, ruleProto) {
|
||||||
return true
|
return true
|
||||||
|
|||||||
@ -338,6 +338,61 @@ func TestNewPolicyManagerSkipsTests(t *testing.T) {
|
|||||||
require.ErrorIs(t, err, errPolicyTestsFailed)
|
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
|
// TestPolicyTestResultsErrorsRendering checks the multi-line render layout
|
||||||
// since the body becomes the user-facing error.
|
// since the body becomes the user-facing error.
|
||||||
func TestPolicyTestResultsErrorsRendering(t *testing.T) {
|
func TestPolicyTestResultsErrorsRendering(t *testing.T) {
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user