From 3e353004b842c30e9823860036d85be8cf3adf80 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 13:40:15 +0200 Subject: [PATCH 01/11] Migrate ACLs syntax to new Tailscale format Implements #617. Tailscale has changed the format of their ACLs to use a more firewall-y terms ("users" & "ports" -> "src" & "dst"). They have also started using all-lowercase tags. This PR applies these changes. --- acls.go | 22 +++---- acls_test.go | 28 ++++----- acls_types.go | 23 ++++---- machine_test.go | 4 +- tests/acls/acl_policy_1.hujson | 57 ++++++++++--------- tests/acls/acl_policy_basic_1.hujson | 10 ++-- tests/acls/acl_policy_basic_groups.hujson | 12 ++-- .../acl_policy_basic_namespace_as_user.hujson | 10 ++-- tests/acls/acl_policy_basic_range.hujson | 10 ++-- tests/acls/acl_policy_basic_wildcards.hujson | 8 +-- tests/acls/acl_policy_basic_wildcards.yaml | 10 ++-- tests/acls/acl_policy_invalid.hujson | 56 +++++++++--------- 12 files changed, 126 insertions(+), 124 deletions(-) diff --git a/acls.go b/acls.go index 2ca65393..c7376ee0 100644 --- a/acls.go +++ b/acls.go @@ -123,11 +123,11 @@ func (h *Headscale) generateACLRules() ([]tailcfg.FilterRule, error) { } srcIPs := []string{} - for innerIndex, user := range acl.Users { - srcs, err := h.generateACLPolicySrcIP(machines, *h.aclPolicy, user) + for innerIndex, src := range acl.Sources { + srcs, err := h.generateACLPolicySrcIP(machines, *h.aclPolicy, src) if err != nil { log.Error(). - Msgf("Error parsing ACL %d, User %d", index, innerIndex) + Msgf("Error parsing ACL %d, Source %d", index, innerIndex) return nil, err } @@ -135,11 +135,11 @@ func (h *Headscale) generateACLRules() ([]tailcfg.FilterRule, error) { } destPorts := []tailcfg.NetPortRange{} - for innerIndex, ports := range acl.Ports { - dests, err := h.generateACLPolicyDestPorts(machines, *h.aclPolicy, ports) + for innerIndex, dest := range acl.Destinations { + dests, err := h.generateACLPolicyDest(machines, *h.aclPolicy, dest) if err != nil { log.Error(). - Msgf("Error parsing ACL %d, Port %d", index, innerIndex) + Msgf("Error parsing ACL %d, Destination %d", index, innerIndex) return nil, err } @@ -158,17 +158,17 @@ func (h *Headscale) generateACLRules() ([]tailcfg.FilterRule, error) { func (h *Headscale) generateACLPolicySrcIP( machines []Machine, aclPolicy ACLPolicy, - u string, + src string, ) ([]string, error) { - return expandAlias(machines, aclPolicy, u, h.cfg.OIDC.StripEmaildomain) + return expandAlias(machines, aclPolicy, src, h.cfg.OIDC.StripEmaildomain) } -func (h *Headscale) generateACLPolicyDestPorts( +func (h *Headscale) generateACLPolicyDest( machines []Machine, aclPolicy ACLPolicy, - d string, + dest string, ) ([]tailcfg.NetPortRange, error) { - tokens := strings.Split(d, ":") + tokens := strings.Split(dest, ":") if len(tokens) < expectedTokenItems || len(tokens) > 3 { return nil, errInvalidPortFormat } diff --git a/acls_test.go b/acls_test.go index e91e95d9..9a7d8a69 100644 --- a/acls_test.go +++ b/acls_test.go @@ -62,7 +62,7 @@ func (s *Suite) TestBasicRule(c *check.C) { func (s *Suite) TestInvalidAction(c *check.C) { app.aclPolicy = &ACLPolicy{ ACLs: []ACL{ - {Action: "invalidAction", Users: []string{"*"}, Ports: []string{"*:*"}}, + {Action: "invalidAction", Sources: []string{"*"}, Destinations: []string{"*:*"}}, }, } err := app.UpdateACLRules() @@ -70,14 +70,14 @@ func (s *Suite) TestInvalidAction(c *check.C) { } func (s *Suite) TestInvalidGroupInGroup(c *check.C) { - // this ACL is wrong because the group in users sections doesn't exist + // this ACL is wrong because the group in Sources sections doesn't exist app.aclPolicy = &ACLPolicy{ Groups: Groups{ "group:test": []string{"foo"}, "group:error": []string{"foo", "group:test"}, }, ACLs: []ACL{ - {Action: "accept", Users: []string{"group:error"}, Ports: []string{"*:*"}}, + {Action: "accept", Sources: []string{"group:error"}, Destinations: []string{"*:*"}}, }, } err := app.UpdateACLRules() @@ -88,7 +88,7 @@ func (s *Suite) TestInvalidTagOwners(c *check.C) { // this ACL is wrong because no tagOwners own the requested tag for the server app.aclPolicy = &ACLPolicy{ ACLs: []ACL{ - {Action: "accept", Users: []string{"tag:foo"}, Ports: []string{"*:*"}}, + {Action: "accept", Sources: []string{"tag:foo"}, Destinations: []string{"*:*"}}, }, } err := app.UpdateACLRules() @@ -97,8 +97,8 @@ func (s *Suite) TestInvalidTagOwners(c *check.C) { // this test should validate that we can expand a group in a TagOWner section and // match properly the IP's of the related hosts. The owner is valid and the tag is also valid. -// the tag is matched in the Users section. -func (s *Suite) TestValidExpandTagOwnersInUsers(c *check.C) { +// the tag is matched in the Sources section. +func (s *Suite) TestValidExpandTagOwnersInSources(c *check.C) { namespace, err := app.CreateNamespace("user1") c.Assert(err, check.IsNil) @@ -131,7 +131,7 @@ func (s *Suite) TestValidExpandTagOwnersInUsers(c *check.C) { Groups: Groups{"group:test": []string{"user1", "user2"}}, TagOwners: TagOwners{"tag:test": []string{"user3", "group:test"}}, ACLs: []ACL{ - {Action: "accept", Users: []string{"tag:test"}, Ports: []string{"*:*"}}, + {Action: "accept", Sources: []string{"tag:test"}, Destinations: []string{"*:*"}}, }, } err = app.UpdateACLRules() @@ -143,7 +143,7 @@ func (s *Suite) TestValidExpandTagOwnersInUsers(c *check.C) { // this test should validate that we can expand a group in a TagOWner section and // match properly the IP's of the related hosts. The owner is valid and the tag is also valid. -// the tag is matched in the Ports section. +// the tag is matched in the Destinations section. func (s *Suite) TestValidExpandTagOwnersInPorts(c *check.C) { namespace, err := app.CreateNamespace("user1") c.Assert(err, check.IsNil) @@ -177,7 +177,7 @@ func (s *Suite) TestValidExpandTagOwnersInPorts(c *check.C) { Groups: Groups{"group:test": []string{"user1", "user2"}}, TagOwners: TagOwners{"tag:test": []string{"user3", "group:test"}}, ACLs: []ACL{ - {Action: "accept", Users: []string{"*"}, Ports: []string{"tag:test:*"}}, + {Action: "accept", Sources: []string{"*"}, Destinations: []string{"tag:test:*"}}, }, } err = app.UpdateACLRules() @@ -222,7 +222,7 @@ func (s *Suite) TestInvalidTagValidNamespace(c *check.C) { app.aclPolicy = &ACLPolicy{ TagOwners: TagOwners{"tag:test": []string{"user1"}}, ACLs: []ACL{ - {Action: "accept", Users: []string{"user1"}, Ports: []string{"*:*"}}, + {Action: "accept", Sources: []string{"user1"}, Destinations: []string{"*:*"}}, }, } err = app.UpdateACLRules() @@ -287,9 +287,9 @@ func (s *Suite) TestValidTagInvalidNamespace(c *check.C) { TagOwners: TagOwners{"tag:webapp": []string{"user1"}}, ACLs: []ACL{ { - Action: "accept", - Users: []string{"user1"}, - Ports: []string{"tag:webapp:80,443"}, + Action: "accept", + Sources: []string{"user1"}, + Destinations: []string{"tag:webapp:80,443"}, }, }, } @@ -645,7 +645,7 @@ func Test_expandPorts(t *testing.T) { wantErr: false, }, { - name: "two ports", + name: "two Destinations", args: args{portsStr: "80,443"}, want: &[]tailcfg.PortRange{ {First: 80, Last: 80}, diff --git a/acls_types.go b/acls_types.go index fb869821..6434509c 100644 --- a/acls_types.go +++ b/acls_types.go @@ -11,18 +11,19 @@ import ( // ACLPolicy represents a Tailscale ACL Policy. type ACLPolicy struct { - Groups Groups `json:"Groups" yaml:"Groups"` - Hosts Hosts `json:"Hosts" yaml:"Hosts"` - TagOwners TagOwners `json:"TagOwners" yaml:"TagOwners"` - ACLs []ACL `json:"ACLs" yaml:"ACLs"` - Tests []ACLTest `json:"Tests" yaml:"Tests"` + Groups Groups `json:"groups" yaml:"groups"` + Hosts Hosts `json:"hosts" yaml:"hosts"` + TagOwners TagOwners `json:"tagOwners" yaml:"tagOwners"` + ACLs []ACL `json:"acls" yaml:"acls"` + Tests []ACLTest `json:"tests" yaml:"tests"` } // ACL is a basic rule for the ACL Policy. type ACL struct { - Action string `json:"Action" yaml:"Action"` - Users []string `json:"Users" yaml:"Users"` - Ports []string `json:"Ports" yaml:"Ports"` + Action string `json:"action" yaml:"action"` + Protocol string `json:"protocol" yaml:"protocol"` + Sources []string `json:"src" yaml:"src"` + Destinations []string `json:"dst" yaml:"dst"` } // Groups references a series of alias in the ACL rules. @@ -36,9 +37,9 @@ type TagOwners map[string][]string // ACLTest is not implemented, but should be use to check if a certain rule is allowed. type ACLTest struct { - User string `json:"User" yaml:"User"` - Allow []string `json:"Allow" yaml:"Allow"` - Deny []string `json:"Deny,omitempty" yaml:"Deny,omitempty"` + Source string `json:"src" yaml:"src"` + Accept []string `json:"accept" yaml:"accept"` + Deny []string `json:"deny,omitempty" yaml:"deny,omitempty"` } // UnmarshalJSON allows to parse the Hosts directly into netaddr objects. diff --git a/machine_test.go b/machine_test.go index bde96057..48ccb153 100644 --- a/machine_test.go +++ b/machine_test.go @@ -188,8 +188,8 @@ func (s *Suite) TestGetACLFilteredPeers(c *check.C) { Hosts: map[string]netaddr.IPPrefix{}, TagOwners: map[string][]string{}, ACLs: []ACL{ - {Action: "accept", Users: []string{"admin"}, Ports: []string{"*:*"}}, - {Action: "accept", Users: []string{"test"}, Ports: []string{"test:*"}}, + {Action: "accept", Sources: []string{"admin"}, Destinations: []string{"*:*"}}, + {Action: "accept", Sources: []string{"test"}, Destinations: []string{"test:*"}}, }, Tests: []ACLTest{}, } diff --git a/tests/acls/acl_policy_1.hujson b/tests/acls/acl_policy_1.hujson index 8f70148b..3ef1a477 100644 --- a/tests/acls/acl_policy_1.hujson +++ b/tests/acls/acl_policy_1.hujson @@ -1,6 +1,6 @@ { // Declare static groups of users beyond those in the identity service. - "Groups": { + "groups": { "group:example": [ "user1@example.com", "user2@example.com", @@ -11,12 +11,12 @@ ], }, // Declare hostname aliases to use in place of IP addresses or subnets. - "Hosts": { + "hosts": { "example-host-1": "100.100.100.100", "example-host-2": "100.100.101.100/24", }, // Define who is allowed to use which tags. - "TagOwners": { + "tagOwners": { // Everyone in the montreal-admins or global-admins group are // allowed to tag servers as montreal-webserver. "tag:montreal-webserver": [ @@ -29,17 +29,18 @@ ], }, // Access control lists. - "ACLs": [ + "acls": [ // Engineering users, plus the president, can access port 22 (ssh) // and port 3389 (remote desktop protocol) on all servers, and all // ports on git-server or ci-server. { - "Action": "accept", - "Users": [ + "action": "accept", + "protocol": "tcp", + "src": [ "group:example2", "192.168.1.0/24" ], - "Ports": [ + "dst": [ "*:22,3389", "git-server:*", "ci-server:*" @@ -48,22 +49,22 @@ // Allow engineer users to access any port on a device tagged with // tag:production. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "group:example" ], - "Ports": [ + "dst": [ "tag:production:*" ], }, // Allow servers in the my-subnet host and 192.168.1.0/24 to access hosts // on both networks. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "example-host-2", ], - "Ports": [ + "dst": [ "example-host-1:*", "192.168.1.0/24:*" ], @@ -72,22 +73,22 @@ // Comment out this section if you want to define specific ACL // restrictions above. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "*" ], - "Ports": [ + "dst": [ "*:*" ], }, // All users in Montreal are allowed to access the Montreal web // servers. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "example-host-1" ], - "Ports": [ + "dst": [ "tag:montreal-webserver:80,443" ], }, @@ -96,30 +97,30 @@ // In contrast, this doesn't grant API servers the right to initiate // any connections. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "tag:montreal-webserver" ], - "Ports": [ + "dst": [ "tag:api-server:443" ], }, ], // Declare tests to check functionality of ACL rules - "Tests": [ + "tests": [ { - "User": "user1@example.com", - "Allow": [ + "src": "user1@example.com", + "accept": [ "example-host-1:22", "example-host-2:80" ], - "Deny": [ + "deny": [ "exapmle-host-2:100" ], }, { - "User": "user2@example.com", - "Allow": [ + "src": "user2@example.com", + "accept": [ "100.60.3.4:22" ], }, diff --git a/tests/acls/acl_policy_basic_1.hujson b/tests/acls/acl_policy_basic_1.hujson index 4f86af3d..db78ea9c 100644 --- a/tests/acls/acl_policy_basic_1.hujson +++ b/tests/acls/acl_policy_basic_1.hujson @@ -3,19 +3,19 @@ { - "Hosts": { + "hosts": { "host-1": "100.100.100.100", "subnet-1": "100.100.101.100/24", }, - "ACLs": [ + "acls": [ { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "subnet-1", "192.168.1.0/24" ], - "Ports": [ + "dst": [ "*:22,3389", "host-1:*", ], diff --git a/tests/acls/acl_policy_basic_groups.hujson b/tests/acls/acl_policy_basic_groups.hujson index ed11a0d0..ecfcbfd1 100644 --- a/tests/acls/acl_policy_basic_groups.hujson +++ b/tests/acls/acl_policy_basic_groups.hujson @@ -1,24 +1,24 @@ // This ACL is used to test group expansion { - "Groups": { + "groups": { "group:example": [ "testnamespace", ], }, - "Hosts": { + "hosts": { "host-1": "100.100.100.100", "subnet-1": "100.100.101.100/24", }, - "ACLs": [ + "acls": [ { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "group:example", ], - "Ports": [ + "dst": [ "host-1:*", ], }, diff --git a/tests/acls/acl_policy_basic_namespace_as_user.hujson b/tests/acls/acl_policy_basic_namespace_as_user.hujson index 881349fc..9a553b08 100644 --- a/tests/acls/acl_policy_basic_namespace_as_user.hujson +++ b/tests/acls/acl_policy_basic_namespace_as_user.hujson @@ -1,18 +1,18 @@ // This ACL is used to test namespace expansion { - "Hosts": { + "hosts": { "host-1": "100.100.100.100", "subnet-1": "100.100.101.100/24", }, - "ACLs": [ + "acls": [ { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "testnamespace", ], - "Ports": [ + "dst": [ "host-1:*", ], }, diff --git a/tests/acls/acl_policy_basic_range.hujson b/tests/acls/acl_policy_basic_range.hujson index 8bcbc798..2a4208fb 100644 --- a/tests/acls/acl_policy_basic_range.hujson +++ b/tests/acls/acl_policy_basic_range.hujson @@ -1,18 +1,18 @@ // This ACL is used to test the port range expansion { - "Hosts": { + "hosts": { "host-1": "100.100.100.100", "subnet-1": "100.100.101.100/24", }, - "ACLs": [ + "acls": [ { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "subnet-1", ], - "Ports": [ + "dst": [ "host-1:5400-5500", ], }, diff --git a/tests/acls/acl_policy_basic_wildcards.hujson b/tests/acls/acl_policy_basic_wildcards.hujson index ec5ce468..e1a1f714 100644 --- a/tests/acls/acl_policy_basic_wildcards.hujson +++ b/tests/acls/acl_policy_basic_wildcards.hujson @@ -1,18 +1,18 @@ // This ACL is used to test wildcards { - "Hosts": { + "hosts": { "host-1": "100.100.100.100", "subnet-1": "100.100.101.100/24", }, - "ACLs": [ + "acls": [ { "Action": "accept", - "Users": [ + "src": [ "*", ], - "Ports": [ + "dst": [ "host-1:*", ], }, diff --git a/tests/acls/acl_policy_basic_wildcards.yaml b/tests/acls/acl_policy_basic_wildcards.yaml index 8e7c817f..4318fcd2 100644 --- a/tests/acls/acl_policy_basic_wildcards.yaml +++ b/tests/acls/acl_policy_basic_wildcards.yaml @@ -1,10 +1,10 @@ --- -Hosts: +hosts: host-1: 100.100.100.100/32 subnet-1: 100.100.101.100/24 -ACLs: - - Action: accept - Users: +acls: + - action: accept + src: - "*" - Ports: + dst: - host-1:* diff --git a/tests/acls/acl_policy_invalid.hujson b/tests/acls/acl_policy_invalid.hujson index ad640dfe..3684b1f1 100644 --- a/tests/acls/acl_policy_invalid.hujson +++ b/tests/acls/acl_policy_invalid.hujson @@ -1,18 +1,18 @@ { // Declare static groups of users beyond those in the identity service. - "Groups": { + "groups": { "group:example": [ "user1@example.com", "user2@example.com", ], }, // Declare hostname aliases to use in place of IP addresses or subnets. - "Hosts": { + "hosts": { "example-host-1": "100.100.100.100", "example-host-2": "100.100.101.100/24", }, // Define who is allowed to use which tags. - "TagOwners": { + "tagOwners": { // Everyone in the montreal-admins or global-admins group are // allowed to tag servers as montreal-webserver. "tag:montreal-webserver": [ @@ -26,17 +26,17 @@ ], }, // Access control lists. - "ACLs": [ + "acls": [ // Engineering users, plus the president, can access port 22 (ssh) // and port 3389 (remote desktop protocol) on all servers, and all // ports on git-server or ci-server. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "group:engineering", "president@example.com" ], - "Ports": [ + "dst": [ "*:22,3389", "git-server:*", "ci-server:*" @@ -45,23 +45,23 @@ // Allow engineer users to access any port on a device tagged with // tag:production. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "group:engineers" ], - "Ports": [ + "dst": [ "tag:production:*" ], }, // Allow servers in the my-subnet host and 192.168.1.0/24 to access hosts // on both networks. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "my-subnet", "192.168.1.0/24" ], - "Ports": [ + "dst": [ "my-subnet:*", "192.168.1.0/24:*" ], @@ -70,22 +70,22 @@ // Comment out this section if you want to define specific ACL // restrictions above. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "*" ], - "Ports": [ + "dst": [ "*:*" ], }, // All users in Montreal are allowed to access the Montreal web // servers. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "group:montreal-users" ], - "Ports": [ + "dst": [ "tag:montreal-webserver:80,443" ], }, @@ -94,30 +94,30 @@ // In contrast, this doesn't grant API servers the right to initiate // any connections. { - "Action": "accept", - "Users": [ + "action": "accept", + "src": [ "tag:montreal-webserver" ], - "Ports": [ + "dst": [ "tag:api-server:443" ], }, ], // Declare tests to check functionality of ACL rules - "Tests": [ + "tests": [ { - "User": "user1@example.com", - "Allow": [ + "src": "user1@example.com", + "accept": [ "example-host-1:22", "example-host-2:80" ], - "Deny": [ + "deny": [ "exapmle-host-2:100" ], }, { - "User": "user2@example.com", - "Allow": [ + "src": "user2@example.com", + "accept": [ "100.60.3.4:22" ], }, From ab1aac9f3e5e507d7579e8c0f3d1b7438e7012b1 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 17:43:59 +0200 Subject: [PATCH 02/11] Improve ACLs by adding protocol parsing support --- acls.go | 70 ++++++++++++++++++++++++++++++++++++++++++++++++--- acls_test.go | 35 ++++++++++++++++++-------- acls_types.go | 2 +- 3 files changed, 93 insertions(+), 14 deletions(-) diff --git a/acls.go b/acls.go index c7376ee0..799db955 100644 --- a/acls.go +++ b/acls.go @@ -23,6 +23,7 @@ const ( errInvalidGroup = Error("invalid group") errInvalidTag = Error("invalid tag") errInvalidPortFormat = Error("invalid port format") + errWildcardIsNeeded = Error("wildcard as port is required for the procotol") ) const ( @@ -134,9 +135,17 @@ func (h *Headscale) generateACLRules() ([]tailcfg.FilterRule, error) { srcIPs = append(srcIPs, srcs...) } + protocols, needsWildcard, err := parseProtocol(acl.Protocol) + if err != nil { + log.Error(). + Msgf("Error parsing ACL %d. protocol unknown %s", index, acl.Protocol) + + return nil, err + } + destPorts := []tailcfg.NetPortRange{} for innerIndex, dest := range acl.Destinations { - dests, err := h.generateACLPolicyDest(machines, *h.aclPolicy, dest) + dests, err := h.generateACLPolicyDest(machines, *h.aclPolicy, dest, needsWildcard) if err != nil { log.Error(). Msgf("Error parsing ACL %d, Destination %d", index, innerIndex) @@ -149,6 +158,7 @@ func (h *Headscale) generateACLRules() ([]tailcfg.FilterRule, error) { rules = append(rules, tailcfg.FilterRule{ SrcIPs: srcIPs, DstPorts: destPorts, + IPProto: protocols, }) } @@ -167,6 +177,7 @@ func (h *Headscale) generateACLPolicyDest( machines []Machine, aclPolicy ACLPolicy, dest string, + needsWildcard bool, ) ([]tailcfg.NetPortRange, error) { tokens := strings.Split(dest, ":") if len(tokens) < expectedTokenItems || len(tokens) > 3 { @@ -195,7 +206,7 @@ func (h *Headscale) generateACLPolicyDest( if err != nil { return nil, err } - ports, err := expandPorts(tokens[len(tokens)-1]) + ports, err := expandPorts(tokens[len(tokens)-1], needsWildcard) if err != nil { return nil, err } @@ -214,6 +225,54 @@ func (h *Headscale) generateACLPolicyDest( return dests, nil } +// parseProtocol reads the proto field of the ACL and generates a list of +// protocols that will be allowed, following the IANA IP protocol number +// https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml +// +// If the ACL proto field is empty, it allows ICMPv4, ICMPv6, TCP, and UDP, +// as per Tailscale behaviour (see tailcfg.FilterRule). +// +// Also returns a boolean indicating if the protocol +// requires all the destinations to use wildcard as port number (only TCP, +// UDP and SCTP support specifying ports). +func parseProtocol(protocol string) ([]int, bool, error) { + switch protocol { + case "": + return []int{1, 58, 6, 17}, false, nil + case "igmp": + return []int{2}, true, nil + case "ipv4", "ip-in-ip": + return []int{4}, true, nil + case "tcp": + return []int{6}, false, nil + case "egp": + return []int{8}, true, nil + case "igp": + return []int{9}, true, nil + case "udp": + return []int{17}, false, nil + case "gre": + return []int{47}, true, nil + case "esp": + return []int{50}, true, nil + case "ah": + return []int{51}, true, nil + case "sctp": + return []int{132}, false, nil + case "icmp": + return []int{1, 58}, true, nil + + default: + protocolNumber, err := strconv.Atoi(protocol) + if err != nil { + return nil, false, err + } + needsWildcard := protocolNumber != 6 && protocolNumber != 17 && protocolNumber != 132 + + return []int{protocolNumber}, needsWildcard, nil + } +} + // expandalias has an input of either // - a namespace // - a group @@ -268,6 +327,7 @@ func expandAlias( alias, ) } + return ips, nil } else { return ips, err @@ -359,13 +419,17 @@ func excludeCorrectlyTaggedNodes( return out } -func expandPorts(portsStr string) (*[]tailcfg.PortRange, error) { +func expandPorts(portsStr string, needsWildcard bool) (*[]tailcfg.PortRange, error) { if portsStr == "*" { return &[]tailcfg.PortRange{ {First: portRangeBegin, Last: portRangeEnd}, }, nil } + if needsWildcard { + return nil, errWildcardIsNeeded + } + ports := []tailcfg.PortRange{} for _, portStr := range strings.Split(portsStr, ",") { rang := strings.Split(portStr, "-") diff --git a/acls_test.go b/acls_test.go index 9a7d8a69..eaf578bf 100644 --- a/acls_test.go +++ b/acls_test.go @@ -628,7 +628,8 @@ func Test_expandTagOwners(t *testing.T) { func Test_expandPorts(t *testing.T) { type args struct { - portsStr string + portsStr string + needsWildcard bool } tests := []struct { name string @@ -638,15 +639,29 @@ func Test_expandPorts(t *testing.T) { }{ { name: "wildcard", - args: args{portsStr: "*"}, + args: args{portsStr: "*", needsWildcard: true}, want: &[]tailcfg.PortRange{ {First: portRangeBegin, Last: portRangeEnd}, }, wantErr: false, }, + { + name: "needs wildcard but does not require it", + args: args{portsStr: "*", needsWildcard: false}, + want: &[]tailcfg.PortRange{ + {First: portRangeBegin, Last: portRangeEnd}, + }, + wantErr: false, + }, + { + name: "needs wildcard but gets port", + args: args{portsStr: "80,443", needsWildcard: true}, + want: nil, + wantErr: true, + }, { name: "two Destinations", - args: args{portsStr: "80,443"}, + args: args{portsStr: "80,443", needsWildcard: false}, want: &[]tailcfg.PortRange{ {First: 80, Last: 80}, {First: 443, Last: 443}, @@ -655,7 +670,7 @@ func Test_expandPorts(t *testing.T) { }, { name: "a range and a port", - args: args{portsStr: "80-1024,443"}, + args: args{portsStr: "80-1024,443", needsWildcard: false}, want: &[]tailcfg.PortRange{ {First: 80, Last: 1024}, {First: 443, Last: 443}, @@ -664,38 +679,38 @@ func Test_expandPorts(t *testing.T) { }, { name: "out of bounds", - args: args{portsStr: "854038"}, + args: args{portsStr: "854038", needsWildcard: false}, want: nil, wantErr: true, }, { name: "wrong port", - args: args{portsStr: "85a38"}, + args: args{portsStr: "85a38", needsWildcard: false}, want: nil, wantErr: true, }, { name: "wrong port in first", - args: args{portsStr: "a-80"}, + args: args{portsStr: "a-80", needsWildcard: false}, want: nil, wantErr: true, }, { name: "wrong port in last", - args: args{portsStr: "80-85a38"}, + args: args{portsStr: "80-85a38", needsWildcard: false}, want: nil, wantErr: true, }, { name: "wrong port format", - args: args{portsStr: "80-85a38-3"}, + args: args{portsStr: "80-85a38-3", needsWildcard: false}, want: nil, wantErr: true, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got, err := expandPorts(test.args.portsStr) + got, err := expandPorts(test.args.portsStr, test.args.needsWildcard) if (err != nil) != test.wantErr { t.Errorf("expandPorts() error = %v, wantErr %v", err, test.wantErr) diff --git a/acls_types.go b/acls_types.go index 6434509c..1c952e22 100644 --- a/acls_types.go +++ b/acls_types.go @@ -21,7 +21,7 @@ type ACLPolicy struct { // ACL is a basic rule for the ACL Policy. type ACL struct { Action string `json:"action" yaml:"action"` - Protocol string `json:"protocol" yaml:"protocol"` + Protocol string `json:"proto" yaml:"proto"` Sources []string `json:"src" yaml:"src"` Destinations []string `json:"dst" yaml:"dst"` } From 8287ba24b9c43998161394b87773e8ac7def5a3c Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 17:55:32 +0200 Subject: [PATCH 03/11] Do not lint the protocol magic numbers I happily use https://pkg.go.dev/golang.org/x/net/internal/iana, but it is internal --- acls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acls.go b/acls.go index 799db955..f3ff11fb 100644 --- a/acls.go +++ b/acls.go @@ -23,7 +23,7 @@ const ( errInvalidGroup = Error("invalid group") errInvalidTag = Error("invalid tag") errInvalidPortFormat = Error("invalid port format") - errWildcardIsNeeded = Error("wildcard as port is required for the procotol") + errWildcardIsNeeded = Error("wildcard as port is required for the protocol") ) const ( @@ -267,7 +267,7 @@ func parseProtocol(protocol string) ([]int, bool, error) { if err != nil { return nil, false, err } - needsWildcard := protocolNumber != 6 && protocolNumber != 17 && protocolNumber != 132 + needsWildcard := protocolNumber != 6 && protocolNumber != 17 && protocolNumber != 132 // nolint return []int{protocolNumber}, needsWildcard, nil } From 39f03b86c808972dd42a3cce9785d0fe7a0cae33 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 18:06:25 +0200 Subject: [PATCH 04/11] Added ACL test file --- acls_test.go | 14 ++++++++++++++ tests/acls/acl_policy_1.hujson | 1 - 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/acls_test.go b/acls_test.go index eaf578bf..0d62cbc3 100644 --- a/acls_test.go +++ b/acls_test.go @@ -321,6 +321,20 @@ func (s *Suite) TestPortRange(c *check.C) { c.Assert(rules[0].DstPorts[0].Ports.Last, check.Equals, uint16(5500)) } +func (s *Suite) TestProtocolParsing(c *check.C) { + err := app.LoadACLPolicy("./tests/acls/acl_policy_basic_protocols.hujson") + c.Assert(err, check.IsNil) + + rules, err := app.generateACLRules() + c.Assert(err, check.IsNil) + c.Assert(rules, check.NotNil) + + c.Assert(rules, check.HasLen, 3) + c.Assert(rules[0].IPProto[0], check.Equals, 6) // tcp + c.Assert(rules[1].IPProto[0], check.Equals, 17) // udp + c.Assert(rules[2].IPProto[1], check.Equals, 58) // icmp v4 +} + func (s *Suite) TestPortWildcard(c *check.C) { err := app.LoadACLPolicy("./tests/acls/acl_policy_basic_wildcards.hujson") c.Assert(err, check.IsNil) diff --git a/tests/acls/acl_policy_1.hujson b/tests/acls/acl_policy_1.hujson index 3ef1a477..dba403f1 100644 --- a/tests/acls/acl_policy_1.hujson +++ b/tests/acls/acl_policy_1.hujson @@ -35,7 +35,6 @@ // ports on git-server or ci-server. { "action": "accept", - "protocol": "tcp", "src": [ "group:example2", "192.168.1.0/24" From c47354bdc3d4d6b82a69cdc41fec135a50e3b117 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 18:12:47 +0200 Subject: [PATCH 05/11] Update internal docs to the new syntax --- docs/acls.md | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/docs/acls.md b/docs/acls.md index 910e3262..5ff5e433 100644 --- a/docs/acls.md +++ b/docs/acls.md @@ -33,7 +33,7 @@ Note: Namespaces will be created automatically when users authenticate with the Headscale server. ACLs could be written either on [huJSON](https://github.com/tailscale/hujson) -or Yaml. Check the [test ACLs](../tests/acls) for further information. +or YAML. Check the [test ACLs](../tests/acls) for further information. When registering the servers we will need to add the flag `--advertised-tags=tag:,tag:`, and the user (namespace) that is @@ -83,8 +83,8 @@ Here are the ACL's to implement the same permissions as above: // boss have access to all servers { "action": "accept", - "users": ["group:boss"], - "ports": [ + "src": ["group:boss"], + "dst": [ "tag:prod-databases:*", "tag:prod-app-servers:*", "tag:internal:*", @@ -96,8 +96,8 @@ Here are the ACL's to implement the same permissions as above: // admin have only access to administrative ports of the servers { "action": "accept", - "users": ["group:admin"], - "ports": [ + "src": ["group:admin"], + "dst": [ "tag:prod-databases:22", "tag:prod-app-servers:22", "tag:internal:22", @@ -110,8 +110,8 @@ Here are the ACL's to implement the same permissions as above: // they can only view the applications servers in prod and have no access to databases servers in production { "action": "accept", - "users": ["group:dev"], - "ports": [ + "src": ["group:dev"], + "dst": [ "tag:dev-databases:*", "tag:dev-app-servers:*", "tag:prod-app-servers:80,443" @@ -124,37 +124,37 @@ Here are the ACL's to implement the same permissions as above: // https://github.com/juanfont/headscale/issues/502 { "action": "accept", - "users": ["group:dev"], - "ports": ["10.20.0.0/16:443,5432", "router.internal:0"] + "src": ["group:dev"], + "dst": ["10.20.0.0/16:443,5432", "router.internal:0"] }, // servers should be able to talk to database. Database should not be able to initiate connections to // applications servers { "action": "accept", - "users": ["tag:dev-app-servers"], - "ports": ["tag:dev-databases:5432"] + "src": ["tag:dev-app-servers"], + "dst": ["tag:dev-databases:5432"] }, { "action": "accept", - "users": ["tag:prod-app-servers"], - "ports": ["tag:prod-databases:5432"] + "src": ["tag:prod-app-servers"], + "dst": ["tag:prod-databases:5432"] }, // interns have access to dev-app-servers only in reading mode { "action": "accept", - "users": ["group:intern"], - "ports": ["tag:dev-app-servers:80,443"] + "src": ["group:intern"], + "dst": ["tag:dev-app-servers:80,443"] }, // We still have to allow internal namespaces communications since nothing guarantees that each user have // their own namespaces. - { "action": "accept", "users": ["boss"], "ports": ["boss:*"] }, - { "action": "accept", "users": ["dev1"], "ports": ["dev1:*"] }, - { "action": "accept", "users": ["dev2"], "ports": ["dev2:*"] }, - { "action": "accept", "users": ["admin1"], "ports": ["admin1:*"] }, - { "action": "accept", "users": ["intern1"], "ports": ["intern1:*"] } + { "action": "accept", "src": ["boss"], "dst": ["boss:*"] }, + { "action": "accept", "src": ["dev1"], "dst": ["dev1:*"] }, + { "action": "accept", "src": ["dev2"], "dst": ["dev2:*"] }, + { "action": "accept", "src": ["admin1"], "dst": ["admin1:*"] }, + { "action": "accept", "src": ["intern1"], "dst": ["intern1:*"] } ] } ``` From 818d26b5f95f647a29a5517e5266efd680dc21d9 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 18:12:56 +0200 Subject: [PATCH 06/11] Updated changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e991f139..0c11f9f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## 0.16.0 (2022-xx-xx) +### BREAKING + +- Old ACL syntax is no longer supported ("users" & "ports" -> "src" & "dst"). Please check [the new syntax](https://tailscale.com/kb/1018/acls/). + ### Changes - **Drop** armhf (32-bit ARM) support. [#609](https://github.com/juanfont/headscale/pull/609) @@ -22,6 +26,7 @@ - This change disables the logs by default - Use [Prometheus]'s duration parser, supporting days (`d`), weeks (`w`) and years (`y`) [#598](https://github.com/juanfont/headscale/pull/598) - Add support for reloading ACLs with SIGHUP [#601](https://github.com/juanfont/headscale/pull/601) +- Use new ACL syntax [#618](https://github.com/juanfont/headscale/pull/618) ## 0.15.0 (2022-03-20) From 5bc11891f514f70ee64e971b64b2ce33f8bccf95 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 18:15:38 +0200 Subject: [PATCH 07/11] Update internal docs with protocol usage --- docs/acls.md | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/docs/acls.md b/docs/acls.md index 5ff5e433..d69ed8fb 100644 --- a/docs/acls.md +++ b/docs/acls.md @@ -93,10 +93,11 @@ Here are the ACL's to implement the same permissions as above: ] }, - // admin have only access to administrative ports of the servers + // admin have only access to administrative ports of the servers, in tcp/22 { "action": "accept", "src": ["group:admin"], + "proto": "tcp", "dst": [ "tag:prod-databases:22", "tag:prod-app-servers:22", @@ -106,6 +107,20 @@ Here are the ACL's to implement the same permissions as above: ] }, + // we also allow admin to ping the servers + { + "action": "accept", + "src": ["group:admin"], + "proto": "icmp", + "dst": [ + "tag:prod-databases:*", + "tag:prod-app-servers:*", + "tag:internal:*", + "tag:dev-databases:*", + "tag:dev-app-servers:*" + ] + }, + // developers have access to databases servers and application servers on all ports // they can only view the applications servers in prod and have no access to databases servers in production { @@ -128,11 +143,12 @@ Here are the ACL's to implement the same permissions as above: "dst": ["10.20.0.0/16:443,5432", "router.internal:0"] }, - // servers should be able to talk to database. Database should not be able to initiate connections to + // servers should be able to talk to database in tcp/5432. Database should not be able to initiate connections to // applications servers { "action": "accept", "src": ["tag:dev-app-servers"], + "proto": "tcp", "dst": ["tag:dev-databases:5432"] }, { From 19b968849fadea2a4d88f9e76893fe7e714e58d6 Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Wed, 8 Jun 2022 18:21:35 +0200 Subject: [PATCH 08/11] Added missing file --- tests/acls/acl_policy_basic_protocols.hujson | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 tests/acls/acl_policy_basic_protocols.hujson diff --git a/tests/acls/acl_policy_basic_protocols.hujson b/tests/acls/acl_policy_basic_protocols.hujson new file mode 100644 index 00000000..6772c564 --- /dev/null +++ b/tests/acls/acl_policy_basic_protocols.hujson @@ -0,0 +1,41 @@ +// This ACL is used to test wildcards + +{ + "hosts": { + "host-1": "100.100.100.100", + "subnet-1": "100.100.101.100/24", + }, + + "acls": [ + { + "Action": "accept", + "src": [ + "*", + ], + "proto": "tcp", + "dst": [ + "host-1:*", + ], + }, + { + "Action": "accept", + "src": [ + "*", + ], + "proto": "udp", + "dst": [ + "host-1:53", + ], + }, + { + "Action": "accept", + "src": [ + "*", + ], + "proto": "icmp", + "dst": [ + "host-1:*", + ], + }, + ], +} \ No newline at end of file From 735a6aaa399cefe40381c99283c77550211a20fa Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Sat, 11 Jun 2022 14:09:08 +0200 Subject: [PATCH 09/11] Use const for IANA protcol numbers --- acls.go | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/acls.go b/acls.go index f3ff11fb..c7a84afc 100644 --- a/acls.go +++ b/acls.go @@ -37,6 +37,23 @@ const ( expectedTokenItems = 2 ) +// For some reason golang.org/x/net/internal/iana is an internal package +const ( + protocolICMP = 1 // Internet Control Message + protocolIGMP = 2 // Internet Group Management + protocolIPv4 = 4 // IPv4 encapsulation + protocolTCP = 6 // Transmission Control + protocolEGP = 8 // Exterior Gateway Protocol + protocolIGP = 9 // any private interior gateway (used by Cisco for their IGRP) + protocolUDP = 17 // User Datagram + protocolGRE = 47 // Generic Routing Encapsulation + protocolESP = 50 // Encap Security Payload + protocolAH = 51 // Authentication Header + protocolIPv6ICMP = 58 // ICMP for IPv6 + protocolSCTP = 132 // Stream Control Transmission Protocol + ProtocolFC = 133 // Fibre Channel +) + // LoadACLPolicy loads the ACL policy from the specify path, and generates the ACL rules. func (h *Headscale) LoadACLPolicy(path string) error { log.Debug(). @@ -238,36 +255,36 @@ func (h *Headscale) generateACLPolicyDest( func parseProtocol(protocol string) ([]int, bool, error) { switch protocol { case "": - return []int{1, 58, 6, 17}, false, nil + return []int{protocolICMP, protocolIPv6ICMP, protocolTCP, protocolUDP}, false, nil case "igmp": - return []int{2}, true, nil + return []int{protocolIGMP}, true, nil case "ipv4", "ip-in-ip": - return []int{4}, true, nil + return []int{protocolIPv4}, true, nil case "tcp": - return []int{6}, false, nil + return []int{protocolTCP}, false, nil case "egp": - return []int{8}, true, nil + return []int{protocolEGP}, true, nil case "igp": - return []int{9}, true, nil + return []int{protocolIGP}, true, nil case "udp": - return []int{17}, false, nil + return []int{protocolUDP}, false, nil case "gre": - return []int{47}, true, nil + return []int{protocolGRE}, true, nil case "esp": - return []int{50}, true, nil + return []int{protocolESP}, true, nil case "ah": - return []int{51}, true, nil + return []int{protocolAH}, true, nil case "sctp": - return []int{132}, false, nil + return []int{protocolSCTP}, false, nil case "icmp": - return []int{1, 58}, true, nil + return []int{protocolICMP, protocolIPv6ICMP}, true, nil default: protocolNumber, err := strconv.Atoi(protocol) if err != nil { return nil, false, err } - needsWildcard := protocolNumber != 6 && protocolNumber != 17 && protocolNumber != 132 // nolint + needsWildcard := protocolNumber != protocolTCP && protocolNumber != protocolUDP && protocolNumber != protocolSCTP return []int{protocolNumber}, needsWildcard, nil } From 3d7be5b287dfa9c06dc3b531dc08568cbdfb115d Mon Sep 17 00:00:00 2001 From: Juan Font Alonso Date: Sat, 11 Jun 2022 14:12:53 +0200 Subject: [PATCH 10/11] Minor rename --- acls_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acls_test.go b/acls_test.go index 0d62cbc3..abe523b5 100644 --- a/acls_test.go +++ b/acls_test.go @@ -144,7 +144,7 @@ func (s *Suite) TestValidExpandTagOwnersInSources(c *check.C) { // this test should validate that we can expand a group in a TagOWner section and // match properly the IP's of the related hosts. The owner is valid and the tag is also valid. // the tag is matched in the Destinations section. -func (s *Suite) TestValidExpandTagOwnersInPorts(c *check.C) { +func (s *Suite) TestValidExpandTagOwnersInDestinations(c *check.C) { namespace, err := app.CreateNamespace("user1") c.Assert(err, check.IsNil) From 569f3caab99a83cb98b458fdfca56a4e34f5f024 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 11 Jun 2022 13:17:07 +0000 Subject: [PATCH 11/11] Use constants in tests --- acls_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/acls_test.go b/acls_test.go index abe523b5..9e24f99f 100644 --- a/acls_test.go +++ b/acls_test.go @@ -330,9 +330,9 @@ func (s *Suite) TestProtocolParsing(c *check.C) { c.Assert(rules, check.NotNil) c.Assert(rules, check.HasLen, 3) - c.Assert(rules[0].IPProto[0], check.Equals, 6) // tcp - c.Assert(rules[1].IPProto[0], check.Equals, 17) // udp - c.Assert(rules[2].IPProto[1], check.Equals, 58) // icmp v4 + c.Assert(rules[0].IPProto[0], check.Equals, protocolTCP) + c.Assert(rules[1].IPProto[0], check.Equals, protocolUDP) + c.Assert(rules[2].IPProto[1], check.Equals, protocolIPv6ICMP) } func (s *Suite) TestPortWildcard(c *check.C) {