Merge pull request #707 from restanrm/fix-bug-in-excludecorrectlytaggednodes

Fix bug in excludeCorrectlyTaggedNodes

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## 0.17.0 (2022-xx-xx)
 
 - Updated dependencies (including the library that lacked armhf support) [#722](https://github.com/juanfont/headscale/pull/722)
+- Fix missing group expansion in function `excludeCorretlyTaggedNodes` [#563](https://github.com/juanfont/headscale/issues/563)
 
 ## 0.16.0 (2022-07-25)
 

acls.go

@@ -162,7 +162,12 @@ func (h *Headscale) generateACLRules() ([]tailcfg.FilterRule, error) {
 
 		destPorts := []tailcfg.NetPortRange{}
 		for innerIndex, dest := range acl.Destinations {
-			dests, err := h.generateACLPolicyDest(machines, *h.aclPolicy, dest, needsWildcard)
+			dests, err := h.generateACLPolicyDest(
+				machines,
+				*h.aclPolicy,
+				dest,
+				needsWildcard,
+			)
 			if err != nil {
 				log.Error().
 					Msgf("Error parsing ACL %d, Destination %d", index, innerIndex)
@@ -255,7 +260,12 @@ func (h *Headscale) generateACLPolicyDest(
 func parseProtocol(protocol string) ([]int, bool, error) {
 	switch protocol {
 	case "":
-		return []int{protocolICMP, protocolIPv6ICMP, protocolTCP, protocolUDP}, false, nil
+		return []int{
+			protocolICMP,
+			protocolIPv6ICMP,
+			protocolTCP,
+			protocolUDP,
+		}, false, nil
 	case "igmp":
 		return []int{protocolIGMP}, true, nil
 	case "ipv4", "ip-in-ip":
@@ -284,7 +294,9 @@ func parseProtocol(protocol string) ([]int, bool, error) {
 		if err != nil {
 			return nil, false, err
 		}
-		needsWildcard := protocolNumber != protocolTCP && protocolNumber != protocolUDP && protocolNumber != protocolSCTP
+		needsWildcard := protocolNumber != protocolTCP &&
+			protocolNumber != protocolUDP &&
+			protocolNumber != protocolSCTP
 
 		return []int{protocolNumber}, needsWildcard, nil
 	}
@@ -367,7 +379,7 @@ func expandAlias(
 
 	// if alias is a namespace
 	nodes := filterMachinesByNamespace(machines, alias)
-	nodes = excludeCorrectlyTaggedNodes(aclPolicy, nodes, alias)
+	nodes = excludeCorrectlyTaggedNodes(aclPolicy, nodes, alias, stripEmailDomain)
 
 	for _, n := range nodes {
 		ips = append(ips, n.IPAddresses.ToStringSlice()...)
@@ -405,10 +417,13 @@ func excludeCorrectlyTaggedNodes(
 	aclPolicy ACLPolicy,
 	nodes []Machine,
 	namespace string,
+	stripEmailDomain bool,
 ) []Machine {
 	out := []Machine{}
 	tags := []string{}
 	for tag, ns := range aclPolicy.TagOwners {
+		owners, _ := expandTagOwners(aclPolicy, namespace, stripEmailDomain)
+		ns = append(owners, namespace)
 		if contains(ns, namespace) {
 			tags = append(tags, tag)
 		}

acls_test.go

@@ -62,7 +62,11 @@ func (s *Suite) TestBasicRule(c *check.C) {
 func (s *Suite) TestInvalidAction(c *check.C) {
 	app.aclPolicy = &ACLPolicy{
 		ACLs: []ACL{
-			{Action: "invalidAction", Sources: []string{"*"}, Destinations: []string{"*:*"}},
+			{
+				Action:       "invalidAction",
+				Sources:      []string{"*"},
+				Destinations: []string{"*:*"},
+			},
 		},
 	}
 	err := app.UpdateACLRules()
@@ -77,7 +81,11 @@ func (s *Suite) TestInvalidGroupInGroup(c *check.C) {
 			"group:error": []string{"foo", "group:test"},
 		},
 		ACLs: []ACL{
-			{Action: "accept", Sources: []string{"group:error"}, Destinations: []string{"*:*"}},
+			{
+				Action:       "accept",
+				Sources:      []string{"group:error"},
+				Destinations: []string{"*:*"},
+			},
 		},
 	}
 	err := app.UpdateACLRules()
@@ -88,7 +96,11 @@ 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", Sources: []string{"tag:foo"}, Destinations: []string{"*:*"}},
+			{
+				Action:       "accept",
+				Sources:      []string{"tag:foo"},
+				Destinations: []string{"*:*"},
+			},
 		},
 	}
 	err := app.UpdateACLRules()
@@ -131,7 +143,11 @@ func (s *Suite) TestValidExpandTagOwnersInSources(c *check.C) {
 		Groups:    Groups{"group:test": []string{"user1", "user2"}},
 		TagOwners: TagOwners{"tag:test": []string{"user3", "group:test"}},
 		ACLs: []ACL{
-			{Action: "accept", Sources: []string{"tag:test"}, Destinations: []string{"*:*"}},
+			{
+				Action:       "accept",
+				Sources:      []string{"tag:test"},
+				Destinations: []string{"*:*"},
+			},
 		},
 	}
 	err = app.UpdateACLRules()
@@ -177,7 +193,11 @@ func (s *Suite) TestValidExpandTagOwnersInDestinations(c *check.C) {
 		Groups:    Groups{"group:test": []string{"user1", "user2"}},
 		TagOwners: TagOwners{"tag:test": []string{"user3", "group:test"}},
 		ACLs: []ACL{
-			{Action: "accept", Sources: []string{"*"}, Destinations: []string{"tag:test:*"}},
+			{
+				Action:       "accept",
+				Sources:      []string{"*"},
+				Destinations: []string{"tag:test:*"},
+			},
 		},
 	}
 	err = app.UpdateACLRules()
@@ -222,7 +242,11 @@ func (s *Suite) TestInvalidTagValidNamespace(c *check.C) {
 	app.aclPolicy = &ACLPolicy{
 		TagOwners: TagOwners{"tag:test": []string{"user1"}},
 		ACLs: []ACL{
-			{Action: "accept", Sources: []string{"user1"}, Destinations: []string{"*:*"}},
+			{
+				Action:       "accept",
+				Sources:      []string{"user1"},
+				Destinations: []string{"*:*"},
+			},
 		},
 	}
 	err = app.UpdateACLRules()
@@ -1201,9 +1225,10 @@ func Test_expandAlias(t *testing.T) {
 
 func Test_excludeCorrectlyTaggedNodes(t *testing.T) {
 	type args struct {
-		aclPolicy ACLPolicy
+		aclPolicy        ACLPolicy
-		nodes     []Machine
+		nodes            []Machine
-		namespace string
+		namespace        string
+		stripEmailDomain bool
 	}
 	tests := []struct {
 		name    string
@@ -1247,7 +1272,59 @@ func Test_excludeCorrectlyTaggedNodes(t *testing.T) {
 						Namespace: Namespace{Name: "joe"},
 					},
 				},
-				namespace: "joe",
+				namespace:        "joe",
+				stripEmailDomain: true,
+			},
+			want: []Machine{
+				{
+					IPAddresses: MachineAddresses{netaddr.MustParseIP("100.64.0.4")},
+					Namespace:   Namespace{Name: "joe"},
+				},
+			},
+		},
+		{
+			name: "exclude nodes with valid tags, and owner is in a group",
+			args: args{
+				aclPolicy: ACLPolicy{
+					Groups: Groups{
+						"group:accountant": []string{"joe", "bar"},
+					},
+					TagOwners: TagOwners{
+						"tag:accountant-webserver": []string{"group:accountant"},
+					},
+				},
+				nodes: []Machine{
+					{
+						IPAddresses: MachineAddresses{
+							netaddr.MustParseIP("100.64.0.1"),
+						},
+						Namespace: Namespace{Name: "joe"},
+						HostInfo: HostInfo{
+							OS:          "centos",
+							Hostname:    "foo",
+							RequestTags: []string{"tag:accountant-webserver"},
+						},
+					},
+					{
+						IPAddresses: MachineAddresses{
+							netaddr.MustParseIP("100.64.0.2"),
+						},
+						Namespace: Namespace{Name: "joe"},
+						HostInfo: HostInfo{
+							OS:          "centos",
+							Hostname:    "foo",
+							RequestTags: []string{"tag:accountant-webserver"},
+						},
+					},
+					{
+						IPAddresses: MachineAddresses{
+							netaddr.MustParseIP("100.64.0.4"),
+						},
+						Namespace: Namespace{Name: "joe"},
+					},
+				},
+				namespace:        "joe",
+				stripEmailDomain: true,
 			},
 			want: []Machine{
 				{
@@ -1288,7 +1365,8 @@ func Test_excludeCorrectlyTaggedNodes(t *testing.T) {
 						Namespace: Namespace{Name: "joe"},
 					},
 				},
-				namespace: "joe",
+				namespace:        "joe",
+				stripEmailDomain: true,
 			},
 			want: []Machine{
 				{
@@ -1333,7 +1411,8 @@ func Test_excludeCorrectlyTaggedNodes(t *testing.T) {
 						Namespace: Namespace{Name: "joe"},
 					},
 				},
-				namespace: "joe",
+				namespace:        "joe",
+				stripEmailDomain: true,
 			},
 			want: []Machine{
 				{
@@ -1373,6 +1452,7 @@ func Test_excludeCorrectlyTaggedNodes(t *testing.T) {
 				test.args.aclPolicy,
 				test.args.nodes,
 				test.args.namespace,
+				test.args.stripEmailDomain,
 			)
 			if !reflect.DeepEqual(got, test.want) {
 				t.Errorf("excludeCorrectlyTaggedNodes() = %v, want %v", got, test.want)

acls_types.go

@@ -21,9 +21,9 @@ type ACLPolicy struct {
 // ACL is a basic rule for the ACL Policy.
 type ACL struct {
 	Action       string   `json:"action" yaml:"action"`
-	Protocol     string   `json:"proto" yaml:"proto"`
+	Protocol     string   `json:"proto"  yaml:"proto"`
-	Sources      []string `json:"src"  yaml:"src"`
+	Sources      []string `json:"src"    yaml:"src"`
-	Destinations []string `json:"dst"  yaml:"dst"`
+	Destinations []string `json:"dst"    yaml:"dst"`
 }
 
 // Groups references a series of alias in the ACL rules.
@@ -37,8 +37,8 @@ type TagOwners map[string][]string
 
 // ACLTest is not implemented, but should be use to check if a certain rule is allowed.
 type ACLTest struct {
-	Source string   `json:"src"           yaml:"src"`
+	Source string   `json:"src"            yaml:"src"`
-	Accept []string `json:"accept"          yaml:"accept"`
+	Accept []string `json:"accept"         yaml:"accept"`
 	Deny   []string `json:"deny,omitempty" yaml:"deny,omitempty"`
 }
 

api.go

@@ -271,7 +271,8 @@ func (h *Headscale) RegistrationHandler(
 		if machine.NodeKey == NodePublicKeyStripPrefix(registerRequest.NodeKey) {
 			// The client sends an Expiry in the past if the client is requesting to expire the key (aka logout)
 			//   https://github.com/tailscale/tailscale/blob/main/tailcfg/tailcfg.go#L648
-			if !registerRequest.Expiry.IsZero() && registerRequest.Expiry.UTC().Before(now) {
+			if !registerRequest.Expiry.IsZero() &&
+				registerRequest.Expiry.UTC().Before(now) {
 				h.handleMachineLogOut(writer, req, machineKey, *machine)
 
 				return
@@ -289,7 +290,13 @@ func (h *Headscale) RegistrationHandler(
 		// The NodeKey we have matches OldNodeKey, which means this is a refresh after a key expiration
 		if machine.NodeKey == NodePublicKeyStripPrefix(registerRequest.OldNodeKey) &&
 			!machine.isExpired() {
-			h.handleMachineRefreshKey(writer, req, machineKey, registerRequest, *machine)
+			h.handleMachineRefreshKey(
+				writer,
+				req,
+				machineKey,
+				registerRequest,
+				*machine,
+			)
 
 			return
 		}

app.go

@@ -418,16 +418,20 @@ func (h *Headscale) createRouter(grpcMux *runtime.ServeMux) *mux.Router {
 	router.HandleFunc("/health", h.HealthHandler).Methods(http.MethodGet)
 	router.HandleFunc("/key", h.KeyHandler).Methods(http.MethodGet)
 	router.HandleFunc("/register", h.RegisterWebAPI).Methods(http.MethodGet)
-	router.HandleFunc("/machine/{mkey}/map", h.PollNetMapHandler).Methods(http.MethodPost)
+	router.HandleFunc("/machine/{mkey}/map", h.PollNetMapHandler).
+		Methods(http.MethodPost)
 	router.HandleFunc("/machine/{mkey}", h.RegistrationHandler).Methods(http.MethodPost)
 	router.HandleFunc("/oidc/register/{mkey}", h.RegisterOIDC).Methods(http.MethodGet)
 	router.HandleFunc("/oidc/callback", h.OIDCCallback).Methods(http.MethodGet)
 	router.HandleFunc("/apple", h.AppleConfigMessage).Methods(http.MethodGet)
-	router.HandleFunc("/apple/{platform}", h.ApplePlatformConfig).Methods(http.MethodGet)
+	router.HandleFunc("/apple/{platform}", h.ApplePlatformConfig).
+		Methods(http.MethodGet)
 	router.HandleFunc("/windows", h.WindowsConfigMessage).Methods(http.MethodGet)
-	router.HandleFunc("/windows/tailscale.reg", h.WindowsRegConfig).Methods(http.MethodGet)
+	router.HandleFunc("/windows/tailscale.reg", h.WindowsRegConfig).
+		Methods(http.MethodGet)
 	router.HandleFunc("/swagger", SwaggerUI).Methods(http.MethodGet)
-	router.HandleFunc("/swagger/v1/openapiv2.json", SwaggerAPIv1).Methods(http.MethodGet)
+	router.HandleFunc("/swagger/v1/openapiv2.json", SwaggerAPIv1).
+		Methods(http.MethodGet)
 
 	if h.cfg.DERP.ServerEnabled {
 		router.HandleFunc("/derp", h.DERPHandler)
@@ -692,7 +696,10 @@ func (h *Headscale) Serve() error {
 				h.pollNetMapStreamWG.Wait()
 
 				// Gracefully shut down servers
-				ctx, cancel := context.WithTimeout(context.Background(), HTTPShutdownTimeout)
+				ctx, cancel := context.WithTimeout(
+					context.Background(),
+					HTTPShutdownTimeout,
+				)
 				if err := promHTTPServer.Shutdown(ctx); err != nil {
 					log.Error().Err(err).Msg("Failed to shutdown prometheus http")
 				}
@@ -819,7 +826,10 @@ func (h *Headscale) setLastStateChangeToNow(namespaces ...string) {
 	if len(namespaces) == 0 {
 		namespaces, err = h.ListNamespacesStr()
 		if err != nil {
-			log.Error().Caller().Err(err).Msg("failed to fetch all namespaces, failing to update last changed state.")
+			log.Error().
+				Caller().
+				Err(err).
+				Msg("failed to fetch all namespaces, failing to update last changed state.")
 		}
 	}
 

cmd/headscale/cli/api_key.go

@@ -134,7 +134,9 @@ If you loose a key, create a new one and revoke (expire) the old one.`,
 
 		expiration := time.Now().UTC().Add(time.Duration(duration))
 
-		log.Trace().Dur("expiration", time.Duration(duration)).Msg("expiration has been set")
+		log.Trace().
+			Dur("expiration", time.Duration(duration)).
+			Msg("expiration has been set")
 
 		request.Expiration = timestamppb.New(expiration)
 

cmd/headscale/cli/preauthkeys.go

@@ -164,7 +164,9 @@ var createPreAuthKeyCmd = &cobra.Command{
 
 		expiration := time.Now().UTC().Add(time.Duration(duration))
 
-		log.Trace().Dur("expiration", time.Duration(duration)).Msg("expiration has been set")
+		log.Trace().
+			Dur("expiration", time.Duration(duration)).
+			Msg("expiration has been set")
 
 		request.Expiration = timestamppb.New(expiration)
 

cmd/headscale/cli/utils.go

@@ -24,7 +24,10 @@ const (
 func getHeadscaleApp() (*headscale.Headscale, error) {
 	cfg, err := headscale.GetHeadscaleConfig()
 	if err != nil {
-		return nil, fmt.Errorf("failed to load configuration while creating headscale instance: %w", err)
+		return nil, fmt.Errorf(
+			"failed to load configuration while creating headscale instance: %w",
+			err,
+		)
 	}
 
 	app, err := headscale.NewHeadscale(cfg)

grpcv1.go

@@ -199,7 +199,7 @@ func (api headscaleV1APIServer) SetTags(
 		err := validateTag(tag)
 		if err != nil {
 			return &v1.SetTagsResponse{
-					Machine: nil,
+				Machine: nil,
 			}, status.Error(codes.InvalidArgument, err.Error())
 		}
 	}

integration_cli_test.go

@@ -69,7 +69,13 @@ func (s *IntegrationCLITestSuite) SetupTest() {
 
 	err = s.pool.RemoveContainerByName(headscaleHostname)
 	if err != nil {
-		s.FailNow(fmt.Sprintf("Could not remove existing container before building test: %s", err), "")
+		s.FailNow(
+			fmt.Sprintf(
+				"Could not remove existing container before building test: %s",
+				err,
+			),
+			"",
+		)
 	}
 
 	fmt.Println("Creating headscale container for CLI tests")

integration_common_test.go

@@ -227,7 +227,6 @@ func getIPs(
 func getDNSNames(
 	headscale *dockertest.Resource,
 ) ([]string, error) {
-
 	listAllResult, err := ExecuteCommand(
 		headscale,
 		[]string{
@@ -261,7 +260,6 @@ func getDNSNames(
 func getMagicFQDN(
 	headscale *dockertest.Resource,
 ) ([]string, error) {
-
 	listAllResult, err := ExecuteCommand(
 		headscale,
 		[]string{
@@ -286,7 +284,11 @@ func getMagicFQDN(
 	hostnames := make([]string, len(listAll))
 
 	for index := range listAll {
-		hostnames[index] = fmt.Sprintf("%s.%s.headscale.net", listAll[index].GetGivenName(), listAll[index].GetNamespace().GetName())
+		hostnames[index] = fmt.Sprintf(
+			"%s.%s.headscale.net",
+			listAll[index].GetGivenName(),
+			listAll[index].GetNamespace().GetName(),
+		)
 	}
 
 	return hostnames, nil

integration_embedded_derp_test.go

@@ -129,7 +129,13 @@ func (s *IntegrationDERPTestSuite) SetupSuite() {
 
 	err = s.pool.RemoveContainerByName(headscaleHostname)
 	if err != nil {
-		s.FailNow(fmt.Sprintf("Could not remove existing container before building test: %s", err), "")
+		s.FailNow(
+			fmt.Sprintf(
+				"Could not remove existing container before building test: %s",
+				err,
+			),
+			"",
+		)
 	}
 
 	log.Println("Creating headscale container for DERP integration tests")

integration_general_test.go

@@ -246,7 +246,13 @@ func (s *IntegrationTestSuite) SetupSuite() {
 
 	err = s.pool.RemoveContainerByName(headscaleHostname)
 	if err != nil {
-		s.FailNow(fmt.Sprintf("Could not remove existing container before building test: %s", err), "")
+		s.FailNow(
+			fmt.Sprintf(
+				"Could not remove existing container before building test: %s",
+				err,
+			),
+			"",
+		)
 	}
 
 	log.Println("Creating headscale container for core integration tests")

machine_test.go

@@ -188,8 +188,16 @@ func (s *Suite) TestGetACLFilteredPeers(c *check.C) {
 		Hosts:     map[string]netaddr.IPPrefix{},
 		TagOwners: map[string][]string{},
 		ACLs: []ACL{
-			{Action: "accept", Sources: []string{"admin"}, Destinations: []string{"*:*"}},
+			{
-			{Action: "accept", Sources: []string{"test"}, Destinations: []string{"test:*"}},
+				Action:       "accept",
+				Sources:      []string{"admin"},
+				Destinations: []string{"*:*"},
+			},
+			{
+				Action:       "accept",
+				Sources:      []string{"test"},
+				Destinations: []string{"test:*"},
+			},
 		},
 		Tests: []ACLTest{},
 	}

oidc.go

@@ -474,7 +474,11 @@ func (h *Headscale) validateMachineForOIDCCallback(
 				Caller().
 				Err(err).
 				Msg("Failed to refresh machine")
-			http.Error(writer, "Failed to refresh machine", http.StatusInternalServerError)
+			http.Error(
+				writer,
+				"Failed to refresh machine",
+				http.StatusInternalServerError,
+			)
 
 			return nil, true, err
 		}

platform_config.go

@@ -325,7 +325,9 @@ func (h *Headscale) ApplePlatformConfig(
 	default:
 		writer.Header().Set("Content-Type", "text/plain; charset=utf-8")
 		writer.WriteHeader(http.StatusBadRequest)
-		_, err := writer.Write([]byte("Invalid platform, only ios and macos is supported"))
+		_, err := writer.Write(
+			[]byte("Invalid platform, only ios and macos is supported"),
+		)
 		if err != nil {
 			log.Error().
 				Caller().
@@ -362,7 +364,8 @@ func (h *Headscale) ApplePlatformConfig(
 		return
 	}
 
-	writer.Header().Set("Content-Type", "application/x-apple-aspen-config; charset=utf-8")
+	writer.Header().
+		Set("Content-Type", "application/x-apple-aspen-config; charset=utf-8")
 	writer.WriteHeader(http.StatusOK)
 	_, err = writer.Write(content.Bytes())
 	if err != nil {