public/headscale
1
0
Fork 0
mirror of https://github.com/juanfont/headscale.git synced 2026-05-23 10:42:30 +09:00
Code Issues Packages Projects Releases Wiki Activity

policy/v2: validate tests block at parse boundary

Browse Source 
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
This commit is contained in:
Kristoffer Dalby
2026-04-29 14:34:10 +00:00
parent c0774a739b
commit f172dba0e3
4 changed files with 257 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
hscontrol/policy/v2/policytester_compat_test.go
View File

@@ -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())
}
})
}

21
hscontrol/policy/v2/test_test.go
View File

@@ -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: `{

91
hscontrol/policy/v2/types.go
View File

@@ -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
}

136
hscontrol/policy/v2/types_test.go
View File

@@ -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,