allow users to be defined with @ in v1 (#2495)

* allow users to be defined with @ in v1

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* remove integration test rewrite hack

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* remove test rewrite hack

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* add @ to integration tests

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* a bit to agressive removeals

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* fix last test

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

hscontrol/db/node_test.go

@@ -440,16 +440,11 @@ func TestAutoApproveRoutes(t *testing.T) {
 				adb, err := newSQLiteTestDB()
 				require.NoError(t, err)
 
-				suffix := ""
+				user, err := adb.CreateUser(types.User{Name: "test"})
-				if version == 1 {
-					suffix = "@"
-				}
-
-				user, err := adb.CreateUser(types.User{Name: "test" + suffix})
 				require.NoError(t, err)
-				_, err = adb.CreateUser(types.User{Name: "test2" + suffix})
+				_, err = adb.CreateUser(types.User{Name: "test2"})
 				require.NoError(t, err)
-				taggedUser, err := adb.CreateUser(types.User{Name: "tagged" + suffix})
+				taggedUser, err := adb.CreateUser(types.User{Name: "tagged"})
 				require.NoError(t, err)
 
 				node := types.Node{
@@ -572,7 +567,7 @@ func TestEphemeralGarbageCollectorLoads(t *testing.T) {
 	})
 	go e.Start()
 
-	for i := 0; i < want; i++ {
+	for i := range want {
 		go e.Schedule(types.NodeID(i), 1*time.Second)
 	}
 

hscontrol/policy/policy_test.go

@@ -97,19 +97,6 @@ func TestTheInternet(t *testing.T) {
 	}
 }
 
-// addAtForFilterV1 returns a copy of the given userslice
-// and adds "@" character to the Name field.
-// This is a "compatibility" move to allow the old tests
-// to run against the "new" format which requires "@".
-func addAtForFilterV1(users types.Users) types.Users {
-	ret := make(types.Users, len(users))
-	for idx := range users {
-		ret[idx] = users[idx]
-		ret[idx].Name = ret[idx].Name + "@"
-	}
-	return ret
-}
-
 func TestReduceFilterRules(t *testing.T) {
 	users := types.Users{
 		types.User{Model: gorm.Model{ID: 1}, Name: "mickael"},
@@ -780,11 +767,7 @@ func TestReduceFilterRules(t *testing.T) {
 			t.Run(fmt.Sprintf("%s-v%d", tt.name, version), func(t *testing.T) {
 				var pm PolicyManager
 				var err error
-				if version == 1 {
+				pm, err = pmf(users, append(tt.peers, tt.node))
-					pm, err = pmf(addAtForFilterV1(users), append(tt.peers, tt.node))
-				} else {
-					pm, err = pmf(users, append(tt.peers, tt.node))
-				}
 				require.NoError(t, err)
 				got := pm.Filter()
 				got = ReduceFilterRules(tt.node, got)

hscontrol/policy/v1/acls.go

@@ -969,6 +969,10 @@ var (
 func findUserFromToken(users []types.User, token string) (types.User, error) {
 	var potentialUsers []types.User
 
+	// This adds the v2 support to looking up users with the new required
+	// policyv2 format where usernames have @ at the end if they are not emails.
+	token = strings.TrimSuffix(token, "@")
+
 	for _, user := range users {
 		if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == token {
 			// Prioritize ProviderIdentifier match and exit early

hscontrol/policy/v1/acls_test.go

@@ -2964,6 +2964,16 @@ func TestFindUserByToken(t *testing.T) {
 			want:    types.User{},
 			wantErr: true,
 		},
+		{
+			name: "test-v2-format-working",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user1", Email: "another1@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user2", Email: "another2@example.com"},
+			},
+			token:   "user2",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user2", Email: "another2@example.com"},
+			wantErr: false,
+		},
 	}
 
 	for _, tt := range tests {

hscontrol/util/dns.go

@@ -196,7 +196,7 @@ func GenerateIPv6DNSRootDomain(ipPrefix netip.Prefix) []dnsname.FQDN {
 	// and from what I can see, the generateMagicDNSRootDomains
 	// function is called only once over the lifetime of a server process.
 	prefixConstantParts := []string{}
-	for i := 0; i < maskBits/nibbleLen; i++ {
+	for i := range maskBits / nibbleLen {
 		prefixConstantParts = append(
 			[]string{string(nibbleStr[i])},
 			prefixConstantParts...)
@@ -215,7 +215,7 @@ func GenerateIPv6DNSRootDomain(ipPrefix netip.Prefix) []dnsname.FQDN {
 	} else {
 		domCount := 1 << (maskBits % nibbleLen)
 		fqdns = make([]dnsname.FQDN, 0, domCount)
-		for i := 0; i < domCount; i++ {
+		for i := range domCount {
 			varNibble := fmt.Sprintf("%x", i)
 			dom, err := makeDomain(varNibble)
 			if err != nil {

hscontrol/util/string_test.go

@@ -8,7 +8,7 @@ import (
 )
 
 func TestGenerateRandomStringDNSSafe(t *testing.T) {
-	for i := 0; i < 100000; i++ {
+	for range 100000 {
 		str, err := GenerateRandomStringDNSSafe(8)
 		require.NoError(t, err)
 		assert.Len(t, str, 8)

integration/acl_test.go

@@ -137,13 +137,13 @@ func TestACLHostsInNetMapTable(t *testing.T) {
 				ACLs: []policyv1.ACL{
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: []string{"user1:*"},
+						Destinations: []string{"user1@:*"},
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user2"},
+						Sources:      []string{"user2@"},
-						Destinations: []string{"user2:*"},
+						Destinations: []string{"user2@:*"},
 					},
 				},
 			}, want: map[string]int{
@@ -160,23 +160,23 @@ func TestACLHostsInNetMapTable(t *testing.T) {
 				ACLs: []policyv1.ACL{
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: []string{"user1:22"},
+						Destinations: []string{"user1@:22"},
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user2"},
+						Sources:      []string{"user2@"},
-						Destinations: []string{"user2:22"},
+						Destinations: []string{"user2@:22"},
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: []string{"user2:22"},
+						Destinations: []string{"user2@:22"},
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user2"},
+						Sources:      []string{"user2@"},
-						Destinations: []string{"user1:22"},
+						Destinations: []string{"user1@:22"},
 					},
 				},
 			}, want: map[string]int{
@@ -194,18 +194,18 @@ func TestACLHostsInNetMapTable(t *testing.T) {
 				ACLs: []policyv1.ACL{
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: []string{"user1:*"},
+						Destinations: []string{"user1@:*"},
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user2"},
+						Sources:      []string{"user2@"},
-						Destinations: []string{"user2:*"},
+						Destinations: []string{"user2@:*"},
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: []string{"user2:*"},
+						Destinations: []string{"user2@:*"},
 					},
 				},
 			}, want: map[string]int{
@@ -219,18 +219,18 @@ func TestACLHostsInNetMapTable(t *testing.T) {
 				ACLs: []policyv1.ACL{
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: append([]string{"user1:*"}, veryLargeDestination...),
+						Destinations: append([]string{"user1@:*"}, veryLargeDestination...),
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user2"},
+						Sources:      []string{"user2@"},
-						Destinations: append([]string{"user2:*"}, veryLargeDestination...),
+						Destinations: append([]string{"user2@:*"}, veryLargeDestination...),
 					},
 					{
 						Action:       "accept",
-						Sources:      []string{"user1"},
+						Sources:      []string{"user1@"},
-						Destinations: append([]string{"user2:*"}, veryLargeDestination...),
+						Destinations: append([]string{"user2@:*"}, veryLargeDestination...),
 					},
 				},
 			}, want: map[string]int{
@@ -299,8 +299,8 @@ func TestACLAllowUser80Dst(t *testing.T) {
 			ACLs: []policyv1.ACL{
 				{
 					Action:       "accept",
-					Sources:      []string{"user1"},
+					Sources:      []string{"user1@"},
-					Destinations: []string{"user2:80"},
+					Destinations: []string{"user2@:80"},
 				},
 			},
 		},
@@ -351,7 +351,7 @@ func TestACLDenyAllPort80(t *testing.T) {
 	scenario := aclScenario(t,
 		&policyv1.ACLPolicy{
 			Groups: map[string][]string{
-				"group:integration-acl-test": {"user1", "user2"},
+				"group:integration-acl-test": {"user1@", "user2@"},
 			},
 			ACLs: []policyv1.ACL{
 				{
@@ -400,8 +400,8 @@ func TestACLAllowUserDst(t *testing.T) {
 			ACLs: []policyv1.ACL{
 				{
 					Action:       "accept",
-					Sources:      []string{"user1"},
+					Sources:      []string{"user1@"},
-					Destinations: []string{"user2:*"},
+					Destinations: []string{"user2@:*"},
 				},
 			},
 		},
@@ -456,7 +456,7 @@ func TestACLAllowStarDst(t *testing.T) {
 			ACLs: []policyv1.ACL{
 				{
 					Action:       "accept",
-					Sources:      []string{"user1"},
+					Sources:      []string{"user1@"},
 					Destinations: []string{"*:*"},
 				},
 			},
@@ -912,8 +912,8 @@ func TestACLDevice1CanAccessDevice2(t *testing.T) {
 		"group": {
 			policy: policyv1.ACLPolicy{
 				Groups: map[string][]string{
-					"group:one": {"user1"},
+					"group:one": {"user1@"},
-					"group:two": {"user2"},
+					"group:two": {"user2@"},
 				},
 				ACLs: []policyv1.ACL{
 					{
@@ -1079,15 +1079,12 @@ func TestPolicyUpdateWhileRunningWithCLIInDatabase(t *testing.T) {
 		ACLs: []policyv1.ACL{
 			{
 				Action:       "accept",
-				Sources:      []string{"user1"},
+				Sources:      []string{"user1@"},
-				Destinations: []string{"user2:*"},
+				Destinations: []string{"user2@:*"},
 			},
 		},
 		Hosts: policyv1.Hosts{},
 	}
-	if usePolicyV2ForTest {
-		hsic.RewritePolicyToV2(&p)
-	}
 
 	pBytes, _ := json.Marshal(p)
 

integration/cli_test.go

@@ -263,7 +263,7 @@ func TestPreAuthKeyCommand(t *testing.T) {
 	keys := make([]*v1.PreAuthKey, count)
 	assertNoErr(t, err)
 
-	for index := 0; index < count; index++ {
+	for index := range count {
 		var preAuthKey v1.PreAuthKey
 		err := executeAndUnmarshal(
 			headscale,
@@ -639,7 +639,7 @@ func TestApiKeyCommand(t *testing.T) {
 
 	keys := make([]string, count)
 
-	for idx := 0; idx < count; idx++ {
+	for idx := range count {
 		apiResult, err := headscale.Execute(
 			[]string{
 				"headscale",
@@ -716,7 +716,7 @@ func TestApiKeyCommand(t *testing.T) {
 	expiredPrefixes := make(map[string]bool)
 
 	// Expire three keys
-	for idx := 0; idx < 3; idx++ {
+	for idx := range 3 {
 		_, err := headscale.Execute(
 			[]string{
 				"headscale",
@@ -951,7 +951,7 @@ func TestNodeAdvertiseTagCommand(t *testing.T) {
 					},
 				},
 				TagOwners: map[string][]string{
-					"tag:test": {"user1"},
+					"tag:test": {"user1@"},
 				},
 			},
 			wantTag: true,
@@ -960,7 +960,7 @@ func TestNodeAdvertiseTagCommand(t *testing.T) {
 			name: "with-policy-groups",
 			policy: &policyv1.ACLPolicy{
 				Groups: policyv1.Groups{
-					"group:admins": []string{"user1"},
+					"group:admins": []string{"user1@"},
 				},
 				ACLs: []policyv1.ACL{
 					{
@@ -1357,7 +1357,7 @@ func TestNodeExpireCommand(t *testing.T) {
 	assert.True(t, listAll[3].GetExpiry().AsTime().IsZero())
 	assert.True(t, listAll[4].GetExpiry().AsTime().IsZero())
 
-	for idx := 0; idx < 3; idx++ {
+	for idx := range 3 {
 		_, err := headscale.Execute(
 			[]string{
 				"headscale",
@@ -1484,7 +1484,7 @@ func TestNodeRenameCommand(t *testing.T) {
 	assert.Contains(t, listAll[3].GetGivenName(), "node-4")
 	assert.Contains(t, listAll[4].GetGivenName(), "node-5")
 
-	for idx := 0; idx < 3; idx++ {
+	for idx := range 3 {
 		res, err := headscale.Execute(
 			[]string{
 				"headscale",
@@ -1751,12 +1751,9 @@ func TestPolicyCommand(t *testing.T) {
 			},
 		},
 		TagOwners: map[string][]string{
-			"tag:exists": {"user1"},
+			"tag:exists": {"user1@"},
 		},
 	}
-	if usePolicyV2ForTest {
-		hsic.RewritePolicyToV2(&p)
-	}
 
 	pBytes, _ := json.Marshal(p)
 
@@ -1797,11 +1794,6 @@ func TestPolicyCommand(t *testing.T) {
 
 	assert.Len(t, output.TagOwners, 1)
 	assert.Len(t, output.ACLs, 1)
-	if usePolicyV2ForTest {
-		assert.Equal(t, output.TagOwners["tag:exists"], []string{"user1@"})
-	} else {
-		assert.Equal(t, output.TagOwners["tag:exists"], []string{"user1"})
-	}
 }
 
 func TestPolicyBrokenConfigCommand(t *testing.T) {
@@ -1840,12 +1832,9 @@ func TestPolicyBrokenConfigCommand(t *testing.T) {
 			},
 		},
 		TagOwners: map[string][]string{
-			"tag:exists": {"user1"},
+			"tag:exists": {"user1@"},
 		},
 	}
-	if usePolicyV2ForTest {
-		hsic.RewritePolicyToV2(&p)
-	}
 
 	pBytes, _ := json.Marshal(p)
 

integration/general_test.go

@@ -345,7 +345,7 @@ func TestTaildrop(t *testing.T) {
 
 	retry := func(times int, sleepInterval time.Duration, doWork func() error) error {
 		var err error
-		for attempts := 0; attempts < times; attempts++ {
+		for range times {
 			err = doWork()
 			if err == nil {
 				return nil

integration/hsic/hsic.go

@@ -12,7 +12,6 @@ import (
 	"net/netip"
 	"os"
 	"path"
-	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -413,10 +412,6 @@ func New(
 	}
 
 	if hsic.aclPolicy != nil {
-		// Rewrite all user entries in the policy to have an @ at the end.
-		if hsic.policyV2 {
-			RewritePolicyToV2(hsic.aclPolicy)
-		}
 		data, err := json.Marshal(hsic.aclPolicy)
 		if err != nil {
 			return nil, fmt.Errorf("failed to marshal ACL Policy to JSON: %w", err)
@@ -878,50 +873,3 @@ func (t *HeadscaleInContainer) SendInterrupt() error {
 
 	return nil
 }
-
-// TODO(kradalby): Remove this function when v1 is deprecated
-func rewriteUsersToV2(strs []string) []string {
-	var result []string
-	userPattern := regexp.MustCompile(`^user\d+$`)
-
-	for _, username := range strs {
-		parts := strings.Split(username, ":")
-		if len(parts) == 0 {
-			result = append(result, username)
-			continue
-		}
-		firstPart := parts[0]
-		if userPattern.MatchString(firstPart) {
-			modifiedFirst := firstPart + "@"
-			if len(parts) > 1 {
-				rest := strings.Join(parts[1:], ":")
-				username = modifiedFirst + ":" + rest
-			} else {
-				username = modifiedFirst
-			}
-		}
-		result = append(result, username)
-	}
-
-	return result
-}
-
-// rewritePolicyToV2 rewrites the policy to v2 format.
-// This mostly means adding the @ prefix to user names.
-// replaces are done inplace
-func RewritePolicyToV2(pol *policyv1.ACLPolicy) {
-	for idx := range pol.ACLs {
-		pol.ACLs[idx].Sources = rewriteUsersToV2(pol.ACLs[idx].Sources)
-		pol.ACLs[idx].Destinations = rewriteUsersToV2(pol.ACLs[idx].Destinations)
-	}
-	for idx := range pol.Groups {
-		pol.Groups[idx] = rewriteUsersToV2(pol.Groups[idx])
-	}
-	for idx := range pol.TagOwners {
-		pol.TagOwners[idx] = rewriteUsersToV2(pol.TagOwners[idx])
-	}
-	for idx := range pol.SSHs {
-		pol.SSHs[idx].Sources = rewriteUsersToV2(pol.SSHs[idx].Sources)
-		pol.SSHs[idx].Destinations = rewriteUsersToV2(pol.SSHs[idx].Destinations)
-	}
-}

integration/route_test.go

@@ -796,7 +796,7 @@ func TestEnableDisableAutoApprovedRoute(t *testing.T) {
 				},
 			},
 			TagOwners: map[string][]string{
-				"tag:approve": {"user1"},
+				"tag:approve": {"user1@"},
 			},
 			AutoApprovers: policyv1.AutoApprovers{
 				Routes: map[string][]string{
@@ -901,7 +901,7 @@ func TestAutoApprovedSubRoute2068(t *testing.T) {
 					},
 				},
 				TagOwners: map[string][]string{
-					"tag:approve": {user},
+					"tag:approve": {user + "@"},
 				},
 				AutoApprovers: policyv1.AutoApprovers{
 					Routes: map[string][]string{
@@ -964,7 +964,7 @@ func TestSubnetRouteACL(t *testing.T) {
 	}, hsic.WithTestName("clienableroute"), hsic.WithACLPolicy(
 		&policyv1.ACLPolicy{
 			Groups: policyv1.Groups{
-				"group:admins": {user},
+				"group:admins": {user + "@"},
 			},
 			ACLs: []policyv1.ACL{
 				{

integration/ssh_test.go

@@ -26,7 +26,7 @@ var retry = func(times int, sleepInterval time.Duration,
 	var stderr string
 	var err error
 
-	for attempts := 0; attempts < times; attempts++ {
+	for range times {
 		tempResult, tempStderr, err := doWork()
 
 		result += tempResult
@@ -94,7 +94,7 @@ func TestSSHOneUserToAll(t *testing.T) {
 	scenario := sshScenario(t,
 		&policyv1.ACLPolicy{
 			Groups: map[string][]string{
-				"group:integration-test": {"user1"},
+				"group:integration-test": {"user1@"},
 			},
 			ACLs: []policyv1.ACL{
 				{
@@ -159,7 +159,7 @@ func TestSSHMultipleUsersAllToAll(t *testing.T) {
 	scenario := sshScenario(t,
 		&policyv1.ACLPolicy{
 			Groups: map[string][]string{
-				"group:integration-test": {"user1", "user2"},
+				"group:integration-test": {"user1@", "user2@"},
 			},
 			ACLs: []policyv1.ACL{
 				{
@@ -212,7 +212,7 @@ func TestSSHNoSSHConfigured(t *testing.T) {
 	scenario := sshScenario(t,
 		&policyv1.ACLPolicy{
 			Groups: map[string][]string{
-				"group:integration-test": {"user1"},
+				"group:integration-test": {"user1@"},
 			},
 			ACLs: []policyv1.ACL{
 				{
@@ -254,7 +254,7 @@ func TestSSHIsBlockedInACL(t *testing.T) {
 	scenario := sshScenario(t,
 		&policyv1.ACLPolicy{
 			Groups: map[string][]string{
-				"group:integration-test": {"user1"},
+				"group:integration-test": {"user1@"},
 			},
 			ACLs: []policyv1.ACL{
 				{
@@ -303,8 +303,8 @@ func TestSSHUserOnlyIsolation(t *testing.T) {
 	scenario := sshScenario(t,
 		&policyv1.ACLPolicy{
 			Groups: map[string][]string{
-				"group:ssh1": {"user1"},
+				"group:ssh1": {"user1@"},
-				"group:ssh2": {"user2"},
+				"group:ssh2": {"user2@"},
 			},
 			ACLs: []policyv1.ACL{
 				{