diff --git a/hscontrol/policy/v2/tailscale_grants_compat_test.go b/hscontrol/policy/v2/tailscale_grants_compat_test.go index a8799ef81..760a43585 100644 --- a/hscontrol/policy/v2/tailscale_grants_compat_test.go +++ b/hscontrol/policy/v2/tailscale_grants_compat_test.go @@ -171,11 +171,8 @@ var grantSkipReasons = map[string]string{ "via-grant-v62": "FILTER_ADDRESS_FORM: same addresses, different wire form", // Checks keyed //filter: a listed check that matches - // fails, so a fix must delete its entry. - "grant-dxp-comma-ports/refused": grantPortList, - - // Contiguous pinned addresses; TestViaGrantHACompat compares these - // filters by address. + // fails, so a fix must delete its entry. Contiguous pinned addresses; + // TestViaGrantHACompat compares these filters by address. "via-grant-v64/client-a/filter": srcAddressForm, "via-grant-v64/client-b/filter": srcAddressForm, "via-grant-v64/plain/filter": srcAddressForm, @@ -190,8 +187,6 @@ var grantSkipReasons = map[string]string{ "via-grant-v65/via-r/filter": srcAddressForm, } -const grantPortList = "GRANT_PORT_LIST: SaaS refuses a comma port list in a grant ip entry (only one port range allowed); headscale accepts it" - // TestGrantsCompat is a data-driven test that loads all GRANT-*.json // test files captured from Tailscale SaaS and compares headscale's grants // engine output against the real Tailscale behavior. diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 9e40e4b47..495bc1e8c 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -74,9 +74,23 @@ var ( ErrACLAutogroupSelfInvalidSource = errors.New("autogroup:self can only be used with users, groups, or supported autogroups") ) +// ErrTooManyDstPorts is returned for a policy over [maxDstPorts]. +var ErrTooManyDstPorts = errors.New( + "too many combinations of destination and port range", +) + +// maxDstPorts bounds the [tailcfg.NetPortRange] entries the ACLs and grants +// of one policy compile to. A rule compiles to one entry per destination +// prefix and port range, so a grant, which lists the two separately, grows +// the filter with the product of its lists and not with the size of the +// policy. The limit is about what 1 MiB of policy holds when every +// combination is written out as an ACL destination. +const maxDstPorts = 100_000 + // Grant validation errors. var ( ErrGrantMissingIPOrApp = errors.New("ip and app can not both be empty") + ErrGrantIPPortList = errors.New("only one port range allowed") ErrGrantViaNotATag = errors.New("via can only be a tag") ErrProtocolPortInvalidFormat = errors.New("expected only one colon in Internet protocol and port type") ErrCapNameInvalidForm = errors.New("capability name must have the form {domain}/{path}") @@ -965,38 +979,34 @@ func (ve *ProtocolPort) UnmarshalJSON(b []byte) error { return nil } - // Only contains a port, no protocol - if !strings.Contains(vs, ":") { - ports, err := parsePortRange(vs) - if err != nil { - return fmt.Errorf("port range %q: %w", vs, err) + protocol, portsPart := ProtocolNameWildcard, vs + + // "proto:ports", or ports alone for any protocol. + if strings.Contains(vs, ":") { + parts := strings.Split(vs, ":") + if len(parts) != 2 { + return fmt.Errorf("%w, got: %v(%d)", ErrProtocolPortInvalidFormat, parts, len(parts)) } - ve.Protocol = ProtocolNameWildcard - ve.Ports = ports + protocol, portsPart = Protocol(parts[0]), parts[1] - return nil + err := protocol.validate() + if err != nil { + return err + } } - parts := strings.Split(vs, ":") - if len(parts) != 2 { - return fmt.Errorf("%w, got: %v(%d)", ErrProtocolPortInvalidFormat, parts, len(parts)) - } - - protocol := Protocol(parts[0]) - - err := protocol.validate() - if err != nil { - return err - } - - portsPart := parts[1] - ports, err := parsePortRange(portsPart) if err != nil { return fmt.Errorf("port range %q: %w", portsPart, err) } + // Tailscale SaaS takes one range per entry; a list goes in + // entries of its own. + if len(ports) > 1 { + return fmt.Errorf("%w: %q", ErrGrantIPPortList, vs) + } + ve.Protocol = protocol ve.Ports = ports @@ -2408,6 +2418,17 @@ func validateGrantSrcDstCombination(sources Aliases, destinations Aliases) error return nil } +// minDstPrefixes returns how many prefixes dst resolves to as far as the +// policy alone tells. Users, groups and tags count as one, their devices are +// not known before the policy is compiled. +func minDstPrefixes(dst Alias) int64 { + if ag, ok := dst.(*AutoGroup); ok && ag.Is(AutoGroupInternet) { + return int64(len(util.TheInternet().Prefixes())) + } + + return 1 +} + // validate reports if there are any errors in a policy after // the unmarshaling process. // It runs through all rules and checks if there are any inconsistencies @@ -2423,6 +2444,10 @@ func (p *Policy) validate() error { // when adding more validation, please add to the list of errors. var errs []error + // Summed over the policy, many rules just below a limit of their own + // would add up again. int64: an int can wrap on 32-bit builds. + var dstPorts int64 + for _, acl := range p.ACLs { for _, src := range acl.Sources { switch src := src.(type) { @@ -2463,6 +2488,8 @@ func (p *Policy) validate() error { } for _, dst := range acl.Destinations { + dstPorts += minDstPrefixes(dst.Alias) * int64(len(dst.Ports)) + switch h := dst.Alias.(type) { case *Host: if !p.Hosts.exist(*h) { @@ -2701,8 +2728,12 @@ func (p *Policy) validate() error { } } + var dstPrefixes int64 + // Validate destinations (empty arrays are allowed — they produce no rules) for _, dst := range grant.Destinations { + dstPrefixes += minDstPrefixes(dst) + switch h := dst.(type) { case *Host: if !p.Hosts.exist(*h) { @@ -2733,6 +2764,10 @@ func (p *Policy) validate() error { } } + for _, ipp := range grant.InternetProtocols { + dstPorts += dstPrefixes * int64(len(ipp.Ports)) + } + // Validate via tags. Wording matches Tailscale SaaS // ("tag %q not found"), which differs from the ACL-src // wording ("src=tag not found: %q"). @@ -2754,6 +2789,12 @@ func (p *Policy) validate() error { } } + if dstPorts > maxDstPorts { + errs = append(errs, fmt.Errorf( + "%w: %d, limit %d", ErrTooManyDstPorts, dstPorts, maxDstPorts, + )) + } + for _, na := range p.NodeAttrs { // SaaS accepts entries with neither attr nor ipPool (they // compile to a no-op); headscale follows suit so policies diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index f0f19a911..44d02fca2 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -5260,6 +5260,36 @@ func TestUnmarshalGrants(t *testing.T) { `, wantErr: "ip and app can not both be empty", }, + { + name: "invalid-grant-ip-port-list", + input: ` +{ + "grants": [ + { + "src": ["*"], + "dst": ["*"], + "ip": ["tcp:80,443"] + } + ] +} +`, + wantErr: `only one port range allowed: "tcp:80,443"`, + }, + { + name: "invalid-grant-ip-bare-port-list", + input: ` +{ + "grants": [ + { + "src": ["*"], + "dst": ["*"], + "ip": ["80-90,443"] + } + ] +} +`, + wantErr: `only one port range allowed: "80-90,443"`, + }, { name: "valid-grant-empty-sources", input: ` @@ -6334,3 +6364,94 @@ func TestPolicyMetadataFields(t *testing.T) { }) } } + +// TestUnmarshalPolicyMaxDstPorts checks that the size limit counts what the +// filter holds an entry for, the combinations of destination prefix and port +// range, and not the length of either list. +func TestUnmarshalPolicyMaxDstPorts(t *testing.T) { + list := func(item string, n int) string { + return strings.TrimSuffix(strings.Repeat(item+",", n), ",") + } + grant := func(extra, dst string, dsts int, ip string) string { + return fmt.Sprintf( + `{"src": ["*"], %s "dst": [%s], "ip": [%s]}`, + extra, list(`"`+dst+`"`, dsts), ip, + ) + } + // One range per ip entry; repeats still count. + ports := func(n int) string { + return list(`"443"`, n) + } + + tests := []struct { + name string + policy string + wantErr bool + }{ + { + name: "grant with many ports", + policy: `{"grants": [` + grant("", "*", 1, ports(50_000)) + `]}`, + }, + { + name: "grant with many destinations", + policy: `{"grants": [` + grant("", "*", 50_000, ports(1)) + `]}`, + }, + { + name: "many destinations and many ports in different grants", + policy: `{"grants": [` + grant("", "*", 50_000, ports(1)) + `,` + + grant("", "*", 1, ports(50_000)) + `]}`, + }, + { + name: "grant at the limit", + policy: `{"grants": [` + grant("", "*", 100, ports(1000)) + `]}`, + }, + { + name: "grant with many destinations and many ports", + policy: `{"grants": [` + grant("", "*", 1000, ports(1000)) + `]}`, + wantErr: true, + }, + { + name: "via grant", + policy: `{"tagOwners": {"tag:router": []}, "grants": [` + + grant(`"via": ["tag:router"],`, "10.0.0.0/24", 1000, ports(1000)) + + `]}`, + wantErr: true, + }, + { + name: "grants that add up", + policy: `{"grants": [` + list(grant("", "*", 100, ports(100)), 11) + `]}`, + wantErr: true, + }, + { + name: "autogroup:internet counts as its prefixes", + policy: `{"grants": [` + + grant("", "autogroup:internet", 100, ports(100)) + `]}`, + wantErr: true, + }, + { + name: "acl with many ports", + policy: `{"acls": [{"action": "accept", "src": ["*"], "dst": ["*:` + + list("443", 50_000) + `"]}]}`, + }, + { + name: "acl with many ports on autogroup:internet", + policy: `{"acls": [{"action": "accept", "src": ["*"], ` + + `"dst": ["autogroup:internet:` + list("443", 50_000) + `"]}]}`, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := unmarshalPolicy([]byte(tt.policy)) + + if tt.wantErr { + require.ErrorIs(t, err, ErrTooManyDstPorts) + + return + } + + require.NoError(t, err) + }) + } +} diff --git a/hscontrol/servertest/policy_test.go b/hscontrol/servertest/policy_test.go index 766d6a3f8..7279aee72 100644 --- a/hscontrol/servertest/policy_test.go +++ b/hscontrol/servertest/policy_test.go @@ -321,8 +321,6 @@ var rejectedPolicySkipReasons = map[string]string{ "acl-err11/refused": "ROUTE_HOST_BITS: headscale accepts an autoApprovers route with host bits", "ssh-check-tag-src/refused": "SSH_CHECK_TAG_SRC: headscale accepts a check rule with a tag source", - - "grant-dxp-comma-ports/refused": "GRANT_PORT_LIST: headscale accepts a comma port list in a grant ip entry", } const noTailnetDomains = "NO_TAILNET_DOMAINS: headscale does not check user:*@domain or localpart:*@domain against the tailnet's domains"