diff --git a/CHANGELOG.md b/CHANGELOG.md index ff8e8039..0900c141 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,20 @@ the code base over time and make it more correct and efficient. [#2692](https://github.com/juanfont/headscale/pull/2692) - Policy: Zero or empty destination port is no longer allowed [#2606](https://github.com/juanfont/headscale/pull/2606) +- Stricter hostname validation [#2383](https://github.com/juanfont/headscale/pull/2383) + - Hostnames must be valid DNS labels (2-63 characters, alphanumeric and + hyphens only, cannot start/end with hyphen) + - **Client Registration (New Nodes)**: Invalid hostnames are automatically + renamed to `invalid-XXXXXX` format + - `my-laptop` β†’ accepted as-is + - `My-Laptop` β†’ `my-laptop` (lowercased) + - `my_laptop` β†’ `invalid-a1b2c3` (underscore not allowed) + - `test@host` β†’ `invalid-d4e5f6` (@ not allowed) + - `laptop-πŸš€` β†’ `invalid-j1k2l3` (emoji not allowed) + - **Hostinfo Updates / CLI**: Invalid hostnames are rejected with an error + - Valid names are accepted or lowercased + - Names with invalid characters, too short (<2), too long (>63), or + starting/ending with hyphen are rejected ### Changes diff --git a/CLAUDE.md b/CLAUDE.md index cf2242f8..d4034367 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -528,3 +528,4 @@ assert.EventuallyWithT(t, func(c *assert.CollectT) { - **Integration Tests**: Require Docker and can consume significant disk space - use headscale-integration-tester agent - **Performance**: NodeStore optimizations are critical for scale - be careful with changes to state management - **Quality Assurance**: Always use appropriate specialized agents for testing and validation tasks +- **NEVER create gists in the user's name**: Do not use the `create_gist` tool - present information directly in the response instead diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 22f8cd7c..e4a0d089 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -1,6 +1,7 @@ package hscontrol import ( + "cmp" "context" "errors" "fmt" @@ -283,19 +284,23 @@ func (h *Headscale) reqToNewRegisterResponse( return nil, NewHTTPError(http.StatusInternalServerError, "failed to generate registration ID", err) } - // Ensure we have valid hostinfo and hostname - validHostinfo, hostname := util.EnsureValidHostinfo( + // Ensure we have a valid hostname + hostname := util.EnsureHostname( req.Hostinfo, machineKey.String(), req.NodeKey.String(), ) + // Ensure we have valid hostinfo + hostinfo := cmp.Or(req.Hostinfo, &tailcfg.Hostinfo{}) + hostinfo.Hostname = hostname + nodeToRegister := types.NewRegisterNode( types.Node{ Hostname: hostname, MachineKey: machineKey, NodeKey: req.NodeKey, - Hostinfo: validHostinfo, + Hostinfo: hostinfo, LastSeen: ptr.To(time.Now()), }, ) @@ -396,13 +401,15 @@ func (h *Headscale) handleRegisterInteractive( return nil, fmt.Errorf("generating registration ID: %w", err) } - // Ensure we have valid hostinfo and hostname - validHostinfo, hostname := util.EnsureValidHostinfo( + // Ensure we have a valid hostname + hostname := util.EnsureHostname( req.Hostinfo, machineKey.String(), req.NodeKey.String(), ) + // Ensure we have valid hostinfo + hostinfo := cmp.Or(req.Hostinfo, &tailcfg.Hostinfo{}) if req.Hostinfo == nil { log.Warn(). Str("machine.key", machineKey.ShortString()). @@ -416,13 +423,14 @@ func (h *Headscale) handleRegisterInteractive( Str("generated.hostname", hostname). Msg("Received registration request with empty hostname, generated default") } + hostinfo.Hostname = hostname nodeToRegister := types.NewRegisterNode( types.Node{ Hostname: hostname, MachineKey: machineKey, NodeKey: req.NodeKey, - Hostinfo: validHostinfo, + Hostinfo: hostinfo, LastSeen: ptr.To(time.Now()), }, ) diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index e54011c5..5493a55c 100644 --- a/hscontrol/db/node.go +++ b/hscontrol/db/node.go @@ -5,9 +5,11 @@ import ( "errors" "fmt" "net/netip" + "regexp" "slices" "sort" "strconv" + "strings" "sync" "testing" "time" @@ -25,6 +27,10 @@ const ( NodeGivenNameTrimSize = 2 ) +var ( + invalidDNSRegex = regexp.MustCompile("[^a-z0-9-.]+") +) + var ( ErrNodeNotFound = errors.New("node not found") ErrNodeRouteIsNotAvailable = errors.New("route is not available on node") @@ -259,6 +265,10 @@ func SetLastSeen(tx *gorm.DB, nodeID types.NodeID, lastSeen time.Time) error { func RenameNode(tx *gorm.DB, nodeID types.NodeID, newName string, ) error { + if err := util.ValidateHostname(newName); err != nil { + return fmt.Errorf("renaming node: %w", err) + } + // Check if the new name is unique var count int64 if err := tx.Model(&types.Node{}).Where("given_name = ? AND id != ?", newName, nodeID).Count(&count).Error; err != nil { @@ -376,6 +386,14 @@ func RegisterNodeForTest(tx *gorm.DB, node types.Node, ipv4 *netip.Addr, ipv6 *n node.IPv4 = ipv4 node.IPv6 = ipv6 + var err error + node.Hostname, err = util.NormaliseHostname(node.Hostname) + if err != nil { + newHostname := util.InvalidString() + log.Info().Err(err).Str("invalid-hostname", node.Hostname).Str("new-hostname", newHostname).Msgf("Invalid hostname, replacing") + node.Hostname = newHostname + } + if node.GivenName == "" { givenName, err := EnsureUniqueGivenName(tx, node.Hostname) if err != nil { @@ -432,7 +450,10 @@ func NodeSave(tx *gorm.DB, node *types.Node) error { } func generateGivenName(suppliedName string, randomSuffix bool) (string, error) { - suppliedName = util.ConvertWithFQDNRules(suppliedName) + // Strip invalid DNS characters for givenName + suppliedName = strings.ToLower(suppliedName) + suppliedName = invalidDNSRegex.ReplaceAllString(suppliedName, "") + if len(suppliedName) > util.LabelHostnameLength { return "", types.ErrHostnameTooLong } diff --git a/hscontrol/db/node_test.go b/hscontrol/db/node_test.go index 84e30e0a..b51dba1c 100644 --- a/hscontrol/db/node_test.go +++ b/hscontrol/db/node_test.go @@ -640,7 +640,7 @@ func TestListEphemeralNodes(t *testing.T) { assert.Equal(t, nodeEph.Hostname, ephemeralNodes[0].Hostname) } -func TestRenameNode(t *testing.T) { +func TestNodeNaming(t *testing.T) { db, err := newSQLiteTestDB() if err != nil { t.Fatalf("creating db: %s", err) @@ -672,6 +672,26 @@ func TestRenameNode(t *testing.T) { Hostinfo: &tailcfg.Hostinfo{}, } + // Using non-ASCII characters in the hostname can + // break your network, so they should be replaced when registering + // a node. + // https://github.com/juanfont/headscale/issues/2343 + nodeInvalidHostname := types.Node{ + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "ζˆ‘ηš„η”΅θ„‘", + UserID: user2.ID, + RegisterMethod: util.RegisterMethodAuthKey, + } + + nodeShortHostname := types.Node{ + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "a", + UserID: user2.ID, + RegisterMethod: util.RegisterMethodAuthKey, + } + err = db.DB.Save(&node).Error require.NoError(t, err) @@ -684,7 +704,11 @@ func TestRenameNode(t *testing.T) { return err } _, err = RegisterNodeForTest(tx, node2, nil, nil) - + if err != nil { + return err + } + _, err = RegisterNodeForTest(tx, nodeInvalidHostname, ptr.To(mpp("100.64.0.66/32").Addr()), nil) + _, err = RegisterNodeForTest(tx, nodeShortHostname, ptr.To(mpp("100.64.0.67/32").Addr()), nil) return err }) require.NoError(t, err) @@ -692,10 +716,12 @@ func TestRenameNode(t *testing.T) { nodes, err := db.ListNodes() require.NoError(t, err) - assert.Len(t, nodes, 2) + assert.Len(t, nodes, 4) t.Logf("node1 %s %s", nodes[0].Hostname, nodes[0].GivenName) t.Logf("node2 %s %s", nodes[1].Hostname, nodes[1].GivenName) + t.Logf("node3 %s %s", nodes[2].Hostname, nodes[2].GivenName) + t.Logf("node4 %s %s", nodes[3].Hostname, nodes[3].GivenName) assert.Equal(t, nodes[0].Hostname, nodes[0].GivenName) assert.NotEqual(t, nodes[1].Hostname, nodes[1].GivenName) @@ -707,6 +733,10 @@ func TestRenameNode(t *testing.T) { assert.Len(t, nodes[1].Hostname, 4) assert.Len(t, nodes[0].GivenName, 4) assert.Len(t, nodes[1].GivenName, 13) + assert.Contains(t, nodes[2].Hostname, "invalid-") // invalid chars + assert.Contains(t, nodes[2].GivenName, "invalid-") + assert.Contains(t, nodes[3].Hostname, "invalid-") // too short + assert.Contains(t, nodes[3].GivenName, "invalid-") // Nodes can be renamed to a unique name err = db.Write(func(tx *gorm.DB) error { @@ -716,7 +746,7 @@ func TestRenameNode(t *testing.T) { nodes, err = db.ListNodes() require.NoError(t, err) - assert.Len(t, nodes, 2) + assert.Len(t, nodes, 4) assert.Equal(t, "test", nodes[0].Hostname) assert.Equal(t, "newname", nodes[0].GivenName) @@ -728,7 +758,7 @@ func TestRenameNode(t *testing.T) { nodes, err = db.ListNodes() require.NoError(t, err) - assert.Len(t, nodes, 2) + assert.Len(t, nodes, 4) assert.Equal(t, "test", nodes[0].Hostname) assert.Equal(t, "newname", nodes[0].GivenName) assert.Equal(t, "test", nodes[1].GivenName) @@ -738,6 +768,149 @@ func TestRenameNode(t *testing.T) { return RenameNode(tx, nodes[0].ID, "test") }) assert.ErrorContains(t, err, "name is not unique") + + // Rename invalid chars + err = db.Write(func(tx *gorm.DB) error { + return RenameNode(tx, nodes[2].ID, "ζˆ‘ηš„η”΅θ„‘") + }) + assert.ErrorContains(t, err, "invalid characters") + + // Rename too short + err = db.Write(func(tx *gorm.DB) error { + return RenameNode(tx, nodes[3].ID, "a") + }) + assert.ErrorContains(t, err, "at least 2 characters") + + // Rename with emoji + err = db.Write(func(tx *gorm.DB) error { + return RenameNode(tx, nodes[0].ID, "hostname-with-πŸ’©") + }) + assert.ErrorContains(t, err, "invalid characters") + + // Rename with only emoji + err = db.Write(func(tx *gorm.DB) error { + return RenameNode(tx, nodes[0].ID, "πŸš€") + }) + assert.ErrorContains(t, err, "invalid characters") +} + +func TestRenameNodeComprehensive(t *testing.T) { + db, err := newSQLiteTestDB() + if err != nil { + t.Fatalf("creating db: %s", err) + } + + user, err := db.CreateUser(types.User{Name: "test"}) + require.NoError(t, err) + + node := types.Node{ + ID: 0, + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "testnode", + UserID: user.ID, + RegisterMethod: util.RegisterMethodAuthKey, + Hostinfo: &tailcfg.Hostinfo{}, + } + + err = db.DB.Save(&node).Error + require.NoError(t, err) + + err = db.DB.Transaction(func(tx *gorm.DB) error { + _, err := RegisterNodeForTest(tx, node, nil, nil) + return err + }) + require.NoError(t, err) + + nodes, err := db.ListNodes() + require.NoError(t, err) + assert.Len(t, nodes, 1) + + tests := []struct { + name string + newName string + wantErr string + }{ + { + name: "uppercase_rejected", + newName: "User2-Host", + wantErr: "must be lowercase", + }, + { + name: "underscore_rejected", + newName: "test_node", + wantErr: "invalid characters", + }, + { + name: "at_sign_uppercase_rejected", + newName: "Test@Host", + wantErr: "must be lowercase", + }, + { + name: "at_sign_rejected", + newName: "test@host", + wantErr: "invalid characters", + }, + { + name: "chinese_chars_with_dash_rejected", + newName: "server-εŒ—δΊ¬-01", + wantErr: "invalid characters", + }, + { + name: "chinese_only_rejected", + newName: "ζˆ‘ηš„η”΅θ„‘", + wantErr: "invalid characters", + }, + { + name: "emoji_with_text_rejected", + newName: "laptop-πŸš€", + wantErr: "invalid characters", + }, + { + name: "mixed_chinese_emoji_rejected", + newName: "ζ΅‹θ―•πŸ’»ζœΊε™¨", + wantErr: "invalid characters", + }, + { + name: "only_emojis_rejected", + newName: "πŸŽ‰πŸŽŠ", + wantErr: "invalid characters", + }, + { + name: "only_at_signs_rejected", + newName: "@@@", + wantErr: "invalid characters", + }, + { + name: "starts_with_dash_rejected", + newName: "-test", + wantErr: "cannot start or end with a hyphen", + }, + { + name: "ends_with_dash_rejected", + newName: "test-", + wantErr: "cannot start or end with a hyphen", + }, + { + name: "too_long_hostname_rejected", + newName: "this-is-a-very-long-hostname-that-exceeds-sixty-three-characters-limit", + wantErr: "must not exceed 63 characters", + }, + { + name: "too_short_hostname_rejected", + newName: "a", + wantErr: "at least 2 characters", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := db.Write(func(tx *gorm.DB) error { + return RenameNode(tx, nodes[0].ID, tt.newName) + }) + assert.ErrorContains(t, err, tt.wantErr) + }) + } } func TestListPeers(t *testing.T) { diff --git a/hscontrol/db/users.go b/hscontrol/db/users.go index 26d10060..08ed048c 100644 --- a/hscontrol/db/users.go +++ b/hscontrol/db/users.go @@ -26,8 +26,7 @@ func (hsdb *HSDatabase) CreateUser(user types.User) (*types.User, error) { // CreateUser creates a new User. Returns error if could not be created // or another user already exists. func CreateUser(tx *gorm.DB, user types.User) (*types.User, error) { - err := util.ValidateUsername(user.Name) - if err != nil { + if err := util.ValidateHostname(user.Name); err != nil { return nil, err } if err := tx.Create(&user).Error; err != nil { @@ -93,8 +92,7 @@ func RenameUser(tx *gorm.DB, uid types.UserID, newName string) error { if err != nil { return err } - err = util.ValidateUsername(newName) - if err != nil { + if err = util.ValidateHostname(newName); err != nil { return err } diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1e138ea0..7585c4e3 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -662,8 +662,7 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t // RenameNode changes the display name of a node. func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, change.ChangeSet, error) { - // Validate the new name before making any changes - if err := util.CheckForFQDNRules(newName); err != nil { + if err := util.ValidateHostname(newName); err != nil { return types.NodeView{}, change.EmptySet, fmt.Errorf("renaming node: %w", err) } @@ -1112,13 +1111,17 @@ func (s *State) HandleNodeFromAuthPath( return types.NodeView{}, change.EmptySet, fmt.Errorf("failed to find user: %w", err) } - // Ensure we have valid hostinfo and hostname from the registration cache entry - validHostinfo, hostname := util.EnsureValidHostinfo( + // Ensure we have a valid hostname from the registration cache entry + hostname := util.EnsureHostname( regEntry.Node.Hostinfo, regEntry.Node.MachineKey.String(), regEntry.Node.NodeKey.String(), ) + // Ensure we have valid hostinfo + validHostinfo := cmp.Or(regEntry.Node.Hostinfo, &tailcfg.Hostinfo{}) + validHostinfo.Hostname = hostname + logHostinfoValidation( regEntry.Node.MachineKey.ShortString(), regEntry.Node.NodeKey.String(), @@ -1284,13 +1287,17 @@ func (s *State) HandleNodeFromPreAuthKey( return types.NodeView{}, change.EmptySet, err } - // Ensure we have valid hostinfo and hostname - handle nil/empty cases - validHostinfo, hostname := util.EnsureValidHostinfo( + // Ensure we have a valid hostname - handle nil/empty cases + hostname := util.EnsureHostname( regReq.Hostinfo, machineKey.String(), regReq.NodeKey.String(), ) + // Ensure we have valid hostinfo + validHostinfo := cmp.Or(regReq.Hostinfo, &tailcfg.Hostinfo{}) + validHostinfo.Hostname = hostname + logHostinfoValidation( machineKey.ShortString(), regReq.NodeKey.ShortString(), diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 6b20091b..a70861ac 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/netip" + "regexp" "slices" "sort" "strconv" @@ -27,6 +28,8 @@ var ( ErrHostnameTooLong = errors.New("hostname too long, cannot except 255 ASCII chars") ErrNodeHasNoGivenName = errors.New("node has no given name") ErrNodeUserHasNoName = errors.New("node user has no name") + + invalidDNSRegex = regexp.MustCompile("[^a-z0-9-.]+") ) type ( @@ -144,7 +147,10 @@ func (ns Nodes) ViewSlice() views.Slice[NodeView] { // GivenNameHasBeenChanged returns whether the `givenName` can be automatically changed based on the `Hostname` of the node. func (node *Node) GivenNameHasBeenChanged() bool { - return node.GivenName == util.ConvertWithFQDNRules(node.Hostname) + // Strip invalid DNS characters for givenName comparison + normalised := strings.ToLower(node.Hostname) + normalised = invalidDNSRegex.ReplaceAllString(normalised, "") + return node.GivenName == normalised } // IsExpired returns whether the node registration has expired. @@ -531,20 +537,34 @@ func (node *Node) ApplyHostnameFromHostInfo(hostInfo *tailcfg.Hostinfo) { return } - if node.Hostname != hostInfo.Hostname { + newHostname := strings.ToLower(hostInfo.Hostname) + if err := util.ValidateHostname(newHostname); err != nil { + log.Warn(). + Str("node.id", node.ID.String()). + Str("current_hostname", node.Hostname). + Str("rejected_hostname", hostInfo.Hostname). + Err(err). + Msg("Rejecting invalid hostname update from hostinfo") + return + } + + if node.Hostname != newHostname { log.Trace(). Str("node.id", node.ID.String()). Str("old_hostname", node.Hostname). - Str("new_hostname", hostInfo.Hostname). + Str("new_hostname", newHostname). Str("old_given_name", node.GivenName). Bool("given_name_changed", node.GivenNameHasBeenChanged()). Msg("Updating hostname from hostinfo") if node.GivenNameHasBeenChanged() { - node.GivenName = util.ConvertWithFQDNRules(hostInfo.Hostname) + // Strip invalid DNS characters for givenName display + givenName := strings.ToLower(newHostname) + givenName = invalidDNSRegex.ReplaceAllString(givenName, "") + node.GivenName = givenName } - node.Hostname = hostInfo.Hostname + node.Hostname = newHostname log.Trace(). Str("node.id", node.ID.String()). diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index f6d1d027..41af5d13 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -369,7 +369,7 @@ func TestApplyHostnameFromHostInfo(t *testing.T) { }, want: Node{ GivenName: "manual-test.local", - Hostname: "NewHostName.Local", + Hostname: "newhostname.local", }, }, { @@ -383,7 +383,245 @@ func TestApplyHostnameFromHostInfo(t *testing.T) { }, want: Node{ GivenName: "newhostname.local", - Hostname: "NewHostName.Local", + Hostname: "newhostname.local", + }, + }, + { + name: "invalid-hostname-with-emoji-rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "hostname-with-πŸ’©", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", // Should reject and keep old hostname + }, + }, + { + name: "invalid-hostname-with-unicode-rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "ζˆ‘ηš„η”΅θ„‘", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", // Should keep old hostname + }, + }, + { + name: "invalid-hostname-with-special-chars-rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "node-with-special!@#$%", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", // Should reject and keep old hostname + }, + }, + { + name: "invalid-hostname-too-short-rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "a", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", // Should keep old hostname + }, + }, + { + name: "invalid-hostname-uppercase-accepted-lowercased", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "ValidHostName", + }, + want: Node{ + GivenName: "validhostname", // GivenName follows hostname when it changes + Hostname: "validhostname", // Uppercase is lowercased, not rejected + }, + }, + { + name: "uppercase_to_lowercase_accepted", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "User2-Host", + }, + want: Node{ + GivenName: "user2-host", + Hostname: "user2-host", + }, + }, + { + name: "at_sign_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "Test@Host", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "chinese_chars_with_dash_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "server-εŒ—δΊ¬-01", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "chinese_only_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "ζˆ‘ηš„η”΅θ„‘", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "emoji_with_text_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "laptop-πŸš€", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "mixed_chinese_emoji_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "ζ΅‹θ―•πŸ’»ζœΊε™¨", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "only_emojis_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "πŸŽ‰πŸŽŠ", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "only_at_signs_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "@@@", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "starts_with_dash_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "-test", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "ends_with_dash_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "test-", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "too_long_hostname_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: strings.Repeat("t", 65), + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + }, + { + name: "underscore_rejected", + nodeBefore: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", + }, + change: &tailcfg.Hostinfo{ + Hostname: "test_node", + }, + want: Node{ + GivenName: "valid-hostname", + Hostname: "valid-hostname", }, }, } diff --git a/hscontrol/util/dns.go b/hscontrol/util/dns.go index 65194720..898f965d 100644 --- a/hscontrol/util/dns.go +++ b/hscontrol/util/dns.go @@ -27,7 +27,7 @@ var ( invalidCharsInUserRegex = regexp.MustCompile("[^a-z0-9-.]+") ) -var ErrInvalidUserName = errors.New("invalid user name") +var ErrInvalidHostName = errors.New("invalid hostname") // ValidateUsername checks if a username is valid. // It must be at least 2 characters long, start with a letter, and contain @@ -67,42 +67,86 @@ func ValidateUsername(username string) error { return nil } -func CheckForFQDNRules(name string) error { - // Ensure the username meets the minimum length requirement +// ValidateHostname checks if a hostname meets DNS requirements. +// This function does NOT modify the input - it only validates. +// The hostname must already be lowercase and contain only valid characters. +func ValidateHostname(name string) error { if len(name) < 2 { - return errors.New("name must be at least 2 characters long") + return fmt.Errorf( + "hostname %q is too short, must be at least 2 characters", + name, + ) } - if len(name) > LabelHostnameLength { return fmt.Errorf( - "DNS segment must not be over 63 chars. %v doesn't comply with this rule: %w", + "hostname %q is too long, must not exceed 63 characters", name, - ErrInvalidUserName, ) } if strings.ToLower(name) != name { return fmt.Errorf( - "DNS segment should be lowercase. %v doesn't comply with this rule: %w", + "hostname %q must be lowercase (try %q)", + name, + strings.ToLower(name), + ) + } + if strings.HasPrefix(name, "-") || strings.HasSuffix(name, "-") { + return fmt.Errorf( + "hostname %q cannot start or end with a hyphen", + name, + ) + } + if strings.HasPrefix(name, ".") || strings.HasSuffix(name, ".") { + return fmt.Errorf( + "hostname %q cannot start or end with a dot", name, - ErrInvalidUserName, ) } if invalidDNSRegex.MatchString(name) { return fmt.Errorf( - "DNS segment should only be composed of lowercase ASCII letters numbers, hyphen and dots. %v doesn't comply with these rules: %w", + "hostname %q contains invalid characters, only lowercase letters, numbers, hyphens and dots are allowed", name, - ErrInvalidUserName, ) } return nil } -func ConvertWithFQDNRules(name string) string { +// NormaliseHostname transforms a string into a valid DNS hostname. +// Returns error if the transformation results in an invalid hostname. +// +// Transformations applied: +// - Converts to lowercase +// - Removes invalid DNS characters +// - Truncates to 63 characters if needed +// +// After transformation, validates the result. +func NormaliseHostname(name string) (string, error) { + // Early return if already valid + if err := ValidateHostname(name); err == nil { + return name, nil + } + + // Transform to lowercase name = strings.ToLower(name) + + // Strip invalid DNS characters name = invalidDNSRegex.ReplaceAllString(name, "") - return name + // Truncate to DNS label limit + if len(name) > LabelHostnameLength { + name = name[:LabelHostnameLength] + } + + // Validate result after transformation + if err := ValidateHostname(name); err != nil { + return "", fmt.Errorf( + "hostname invalid after normalisation: %w", + err, + ) + } + + return name, nil } // generateMagicDNSRootDomains generates a list of DNS entries to be included in `Routes` in `MapResponse`. diff --git a/hscontrol/util/dns_test.go b/hscontrol/util/dns_test.go index 140b70e2..b492e4d6 100644 --- a/hscontrol/util/dns_test.go +++ b/hscontrol/util/dns_test.go @@ -2,6 +2,7 @@ package util import ( "net/netip" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -9,94 +10,173 @@ import ( "tailscale.com/util/must" ) -func TestCheckForFQDNRules(t *testing.T) { +func TestNormaliseHostname(t *testing.T) { type args struct { name string } tests := []struct { name string args args + want string wantErr bool }{ { - name: "valid: user", + name: "valid: lowercase user", args: args{name: "valid-user"}, + want: "valid-user", wantErr: false, }, { - name: "invalid: capitalized user", + name: "normalise: capitalized user", args: args{name: "Invalid-CapItaLIzed-user"}, - wantErr: true, + want: "invalid-capitalized-user", + wantErr: false, }, { - name: "invalid: email as user", + name: "normalise: email as user", args: args{name: "foo.bar@example.com"}, - wantErr: true, + want: "foo.barexample.com", + wantErr: false, }, { - name: "invalid: chars in user name", + name: "normalise: chars in user name", args: args{name: "super-user+name"}, - wantErr: true, + want: "super-username", + wantErr: false, }, { - name: "invalid: too long name for user", + name: "invalid: too long name truncated leaves trailing hyphen", args: args{ name: "super-long-useruseruser-name-that-should-be-a-little-more-than-63-chars", }, + want: "", + wantErr: true, + }, + { + name: "invalid: emoji stripped leaves trailing hyphen", + args: args{name: "hostname-with-πŸ’©"}, + want: "", + wantErr: true, + }, + { + name: "normalise: multiple emojis stripped", + args: args{name: "node-πŸŽ‰-πŸš€-test"}, + want: "node---test", + wantErr: false, + }, + { + name: "invalid: only emoji becomes empty", + args: args{name: "πŸ’©"}, + want: "", + wantErr: true, + }, + { + name: "invalid: emoji at start leaves leading hyphen", + args: args{name: "πŸš€-rocket-node"}, + want: "", + wantErr: true, + }, + { + name: "invalid: emoji at end leaves trailing hyphen", + args: args{name: "node-test-πŸŽ‰"}, + want: "", wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := CheckForFQDNRules(tt.args.name); (err != nil) != tt.wantErr { - t.Errorf("CheckForFQDNRules() error = %v, wantErr %v", err, tt.wantErr) + got, err := NormaliseHostname(tt.args.name) + if (err != nil) != tt.wantErr { + t.Errorf("NormaliseHostname() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && got != tt.want { + t.Errorf("NormaliseHostname() = %v, want %v", got, tt.want) } }) } } -func TestConvertWithFQDNRules(t *testing.T) { +func TestValidateHostname(t *testing.T) { tests := []struct { - name string - hostname string - dnsHostName string + name string + hostname string + wantErr bool + errorContains string }{ { - name: "User1.test", - hostname: "User1.Test", - dnsHostName: "user1.test", + name: "valid lowercase", + hostname: "valid-hostname", + wantErr: false, }, { - name: "User'1$2.test", - hostname: "User'1$2.Test", - dnsHostName: "user12.test", + name: "uppercase rejected", + hostname: "MyHostname", + wantErr: true, + errorContains: "must be lowercase", }, { - name: "User-^_12.local.test", - hostname: "User-^_12.local.Test", - dnsHostName: "user-12.local.test", + name: "too short", + hostname: "a", + wantErr: true, + errorContains: "too short", }, { - name: "User-MacBook-Pro", - hostname: "User-MacBook-Pro", - dnsHostName: "user-macbook-pro", + name: "too long", + hostname: "a" + strings.Repeat("b", 63), + wantErr: true, + errorContains: "too long", }, { - name: "User-Linux-Ubuntu/Fedora", - hostname: "User-Linux-Ubuntu/Fedora", - dnsHostName: "user-linux-ubuntufedora", + name: "emoji rejected", + hostname: "hostname-πŸ’©", + wantErr: true, + errorContains: "invalid characters", }, { - name: "User-[Space]123", - hostname: "User-[ ]123", - dnsHostName: "user-123", + name: "starts with hyphen", + hostname: "-hostname", + wantErr: true, + errorContains: "cannot start or end with a hyphen", + }, + { + name: "ends with hyphen", + hostname: "hostname-", + wantErr: true, + errorContains: "cannot start or end with a hyphen", + }, + { + name: "starts with dot", + hostname: ".hostname", + wantErr: true, + errorContains: "cannot start or end with a dot", + }, + { + name: "ends with dot", + hostname: "hostname.", + wantErr: true, + errorContains: "cannot start or end with a dot", + }, + { + name: "special characters", + hostname: "host!@#$name", + wantErr: true, + errorContains: "invalid characters", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - fqdnHostName := ConvertWithFQDNRules(tt.hostname) - assert.Equal(t, tt.dnsHostName, fqdnHostName) + err := ValidateHostname(tt.hostname) + if (err != nil) != tt.wantErr { + t.Errorf("ValidateHostname() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr && tt.errorContains != "" { + if err == nil || !strings.Contains(err.Error(), tt.errorContains) { + t.Errorf("ValidateHostname() error = %v, should contain %q", err, tt.errorContains) + } + } }) } } diff --git a/hscontrol/util/string.go b/hscontrol/util/string.go index 624d8bc0..d1d7ece7 100644 --- a/hscontrol/util/string.go +++ b/hscontrol/util/string.go @@ -66,6 +66,11 @@ func MustGenerateRandomStringDNSSafe(size int) string { return hash } +func InvalidString() string { + hash, _ := GenerateRandomStringDNSSafe(8) + return "invalid-" + hash +} + func TailNodesToString(nodes []*tailcfg.Node) string { temp := make([]string, len(nodes)) diff --git a/hscontrol/util/util.go b/hscontrol/util/util.go index 143998cc..a9dc748e 100644 --- a/hscontrol/util/util.go +++ b/hscontrol/util/util.go @@ -1,6 +1,7 @@ package util import ( + "cmp" "errors" "fmt" "net/netip" @@ -264,54 +265,32 @@ func IsCI() bool { // if Hostinfo is nil or Hostname is empty. This prevents nil pointer dereferences // and ensures nodes always have a valid hostname. // The hostname is truncated to 63 characters to comply with DNS label length limits (RFC 1123). -func SafeHostname(hostinfo *tailcfg.Hostinfo, machineKey, nodeKey string) string { +// EnsureHostname guarantees a valid hostname for node registration. +// This function never fails - it always returns a valid hostname. +// +// Strategy: +// 1. If hostinfo is nil/empty β†’ generate default from keys +// 2. If hostname is provided β†’ normalise it +// 3. If normalisation fails β†’ generate invalid- replacement +// +// Returns the guaranteed-valid hostname to use. +func EnsureHostname(hostinfo *tailcfg.Hostinfo, machineKey, nodeKey string) string { if hostinfo == nil || hostinfo.Hostname == "" { - // Generate a default hostname using machine key prefix - if machineKey != "" { - keyPrefix := machineKey - if len(machineKey) > 8 { - keyPrefix = machineKey[:8] - } - return fmt.Sprintf("node-%s", keyPrefix) + key := cmp.Or(machineKey, nodeKey) + if key == "" { + return "unknown-node" } - if nodeKey != "" { - keyPrefix := nodeKey - if len(nodeKey) > 8 { - keyPrefix = nodeKey[:8] - } - return fmt.Sprintf("node-%s", keyPrefix) + keyPrefix := key + if len(key) > 8 { + keyPrefix = key[:8] } - return "unknown-node" + return fmt.Sprintf("node-%s", keyPrefix) } - hostname := hostinfo.Hostname - - // Validate hostname length - DNS label limit is 63 characters (RFC 1123) - // Truncate if necessary to ensure compatibility with given name generation - if len(hostname) > 63 { - hostname = hostname[:63] + lowercased := strings.ToLower(hostinfo.Hostname) + if err := ValidateHostname(lowercased); err == nil { + return lowercased } - return hostname -} - -// EnsureValidHostinfo ensures that Hostinfo is non-nil and has a valid hostname. -// If Hostinfo is nil, it creates a minimal valid Hostinfo with a generated hostname. -// Returns the validated/created Hostinfo and the extracted hostname. -func EnsureValidHostinfo(hostinfo *tailcfg.Hostinfo, machineKey, nodeKey string) (*tailcfg.Hostinfo, string) { - if hostinfo == nil { - hostname := SafeHostname(nil, machineKey, nodeKey) - return &tailcfg.Hostinfo{ - Hostname: hostname, - }, hostname - } - - hostname := SafeHostname(hostinfo, machineKey, nodeKey) - - // Update the hostname in the hostinfo if it was empty or if it was truncated - if hostinfo.Hostname == "" || hostinfo.Hostname != hostname { - hostinfo.Hostname = hostname - } - - return hostinfo, hostname + return InvalidString() } diff --git a/hscontrol/util/util_test.go b/hscontrol/util/util_test.go index e0414071..22418e34 100644 --- a/hscontrol/util/util_test.go +++ b/hscontrol/util/util_test.go @@ -3,6 +3,7 @@ package util import ( "errors" "net/netip" + "strings" "testing" "time" @@ -795,7 +796,7 @@ over a maximum of 30 hops: } } -func TestSafeHostname(t *testing.T) { +func TestEnsureHostname(t *testing.T) { t.Parallel() tests := []struct { @@ -878,7 +879,7 @@ func TestSafeHostname(t *testing.T) { }, machineKey: "mkey12345678", nodeKey: "nkey12345678", - want: "123456789012345678901234567890123456789012345678901234567890123", + want: "invalid-", }, { name: "hostname_very_long_truncated", @@ -887,7 +888,7 @@ func TestSafeHostname(t *testing.T) { }, machineKey: "mkey12345678", nodeKey: "nkey12345678", - want: "test-node-with-very-long-hostname-that-exceeds-dns-label-limits", + want: "invalid-", }, { name: "hostname_with_special_chars", @@ -896,7 +897,7 @@ func TestSafeHostname(t *testing.T) { }, machineKey: "mkey12345678", nodeKey: "nkey12345678", - want: "node-with-special!@#$%", + want: "invalid-", }, { name: "hostname_with_unicode", @@ -905,7 +906,7 @@ func TestSafeHostname(t *testing.T) { }, machineKey: "mkey12345678", nodeKey: "nkey12345678", - want: "node-Γ±oΓ±o-ζ΅‹θ―•", + want: "invalid-", }, { name: "short_machine_key", @@ -925,20 +926,160 @@ func TestSafeHostname(t *testing.T) { nodeKey: "short", want: "node-short", }, + { + name: "hostname_with_emoji_replaced", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "hostname-with-πŸ’©", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "hostname_only_emoji_replaced", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "πŸš€", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "hostname_with_multiple_emojis_replaced", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "node-πŸŽ‰-πŸš€-test", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "uppercase_to_lowercase", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "User2-Host", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "user2-host", + }, + { + name: "underscore_removed", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "test_node", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "at_sign_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "Test@Host", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "chinese_chars_with_dash_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "server-εŒ—δΊ¬-01", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "chinese_only_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "ζˆ‘ηš„η”΅θ„‘", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "emoji_with_text_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "laptop-πŸš€", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "mixed_chinese_emoji_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "ζ΅‹θ―•πŸ’»ζœΊε™¨", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "only_emojis_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "πŸŽ‰πŸŽŠ", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "only_at_signs_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "@@@", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "starts_with_dash_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "-test", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "ends_with_dash_invalid", + hostinfo: &tailcfg.Hostinfo{ + Hostname: "test-", + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, + { + name: "very_long_hostname_truncated", + hostinfo: &tailcfg.Hostinfo{ + Hostname: strings.Repeat("t", 70), + }, + machineKey: "mkey12345678", + nodeKey: "nkey12345678", + want: "invalid-", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := SafeHostname(tt.hostinfo, tt.machineKey, tt.nodeKey) - if got != tt.want { - t.Errorf("SafeHostname() = %v, want %v", got, tt.want) + got := EnsureHostname(tt.hostinfo, tt.machineKey, tt.nodeKey) + // For invalid hostnames, we just check the prefix since the random part varies + if strings.HasPrefix(tt.want, "invalid-") { + if !strings.HasPrefix(got, "invalid-") { + t.Errorf("EnsureHostname() = %v, want prefix %v", got, tt.want) + } + } else if got != tt.want { + t.Errorf("EnsureHostname() = %v, want %v", got, tt.want) } }) } } -func TestEnsureValidHostinfo(t *testing.T) { +func TestEnsureHostnameWithHostinfo(t *testing.T) { t.Parallel() tests := []struct { @@ -976,14 +1117,6 @@ func TestEnsureValidHostinfo(t *testing.T) { machineKey: "mkey12345678", nodeKey: "nkey12345678", wantHostname: "node-mkey1234", - checkHostinfo: func(t *testing.T, hi *tailcfg.Hostinfo) { - if hi == nil { - t.Error("hostinfo should not be nil") - } - if hi.Hostname != "node-mkey1234" { - t.Errorf("hostname = %v, want node-mkey1234", hi.Hostname) - } - }, }, { name: "empty_hostname_updated", @@ -994,37 +1127,15 @@ func TestEnsureValidHostinfo(t *testing.T) { machineKey: "mkey12345678", nodeKey: "nkey12345678", wantHostname: "node-mkey1234", - checkHostinfo: func(t *testing.T, hi *tailcfg.Hostinfo) { - if hi == nil { - t.Error("hostinfo should not be nil") - } - if hi.Hostname != "node-mkey1234" { - t.Errorf("hostname = %v, want node-mkey1234", hi.Hostname) - } - if hi.OS != "darwin" { - t.Errorf("OS = %v, want darwin", hi.OS) - } - }, }, { - name: "long_hostname_truncated", + name: "long_hostname_rejected", hostinfo: &tailcfg.Hostinfo{ Hostname: "test-node-with-very-long-hostname-that-exceeds-dns-label-limits-of-63-characters", }, machineKey: "mkey12345678", nodeKey: "nkey12345678", - wantHostname: "test-node-with-very-long-hostname-that-exceeds-dns-label-limits", - checkHostinfo: func(t *testing.T, hi *tailcfg.Hostinfo) { - if hi == nil { - t.Error("hostinfo should not be nil") - } - if hi.Hostname != "test-node-with-very-long-hostname-that-exceeds-dns-label-limits" { - t.Errorf("hostname = %v, want truncated", hi.Hostname) - } - if len(hi.Hostname) != 63 { - t.Errorf("hostname length = %v, want 63", len(hi.Hostname)) - } - }, + wantHostname: "invalid-", }, { name: "nil_hostinfo_node_key_only", @@ -1128,23 +1239,20 @@ func TestEnsureValidHostinfo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - gotHostinfo, gotHostname := EnsureValidHostinfo(tt.hostinfo, tt.machineKey, tt.nodeKey) - - if gotHostname != tt.wantHostname { - t.Errorf("EnsureValidHostinfo() hostname = %v, want %v", gotHostname, tt.wantHostname) - } - if gotHostinfo == nil { - t.Error("returned hostinfo should never be nil") - } - - if tt.checkHostinfo != nil { - tt.checkHostinfo(t, gotHostinfo) + gotHostname := EnsureHostname(tt.hostinfo, tt.machineKey, tt.nodeKey) + // For invalid hostnames, we just check the prefix since the random part varies + if strings.HasPrefix(tt.wantHostname, "invalid-") { + if !strings.HasPrefix(gotHostname, "invalid-") { + t.Errorf("EnsureHostname() = %v, want prefix %v", gotHostname, tt.wantHostname) + } + } else if gotHostname != tt.wantHostname { + t.Errorf("EnsureHostname() hostname = %v, want %v", gotHostname, tt.wantHostname) } }) } } -func TestSafeHostname_DNSLabelLimit(t *testing.T) { +func TestEnsureHostname_DNSLabelLimit(t *testing.T) { t.Parallel() testCases := []string{ @@ -1157,7 +1265,7 @@ func TestSafeHostname_DNSLabelLimit(t *testing.T) { for i, hostname := range testCases { t.Run(cmp.Diff("", ""), func(t *testing.T) { hostinfo := &tailcfg.Hostinfo{Hostname: hostname} - result := SafeHostname(hostinfo, "mkey", "nkey") + result := EnsureHostname(hostinfo, "mkey", "nkey") if len(result) > 63 { t.Errorf("test case %d: hostname length = %d, want <= 63", i, len(result)) } @@ -1165,7 +1273,7 @@ func TestSafeHostname_DNSLabelLimit(t *testing.T) { } } -func TestEnsureValidHostinfo_Idempotent(t *testing.T) { +func TestEnsureHostname_Idempotent(t *testing.T) { t.Parallel() originalHostinfo := &tailcfg.Hostinfo{ @@ -1173,16 +1281,10 @@ func TestEnsureValidHostinfo_Idempotent(t *testing.T) { OS: "linux", } - hostinfo1, hostname1 := EnsureValidHostinfo(originalHostinfo, "mkey", "nkey") - hostinfo2, hostname2 := EnsureValidHostinfo(hostinfo1, "mkey", "nkey") + hostname1 := EnsureHostname(originalHostinfo, "mkey", "nkey") + hostname2 := EnsureHostname(originalHostinfo, "mkey", "nkey") if hostname1 != hostname2 { t.Errorf("hostnames not equal: %v != %v", hostname1, hostname2) } - if hostinfo1.Hostname != hostinfo2.Hostname { - t.Errorf("hostinfo hostnames not equal: %v != %v", hostinfo1.Hostname, hostinfo2.Hostname) - } - if hostinfo1.OS != hostinfo2.OS { - t.Errorf("hostinfo OS not equal: %v != %v", hostinfo1.OS, hostinfo2.OS) - } } diff --git a/integration/cli_test.go b/integration/cli_test.go index 40afd2c3..d6616d62 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -1164,7 +1164,7 @@ func TestNodeCommand(t *testing.T) { "debug", "create-node", "--name", - fmt.Sprintf("otherUser-node-%d", index+1), + fmt.Sprintf("otheruser-node-%d", index+1), "--user", "other-user", "--key", @@ -1221,8 +1221,8 @@ func TestNodeCommand(t *testing.T) { assert.Equal(t, uint64(6), listAllWithotherUser[5].GetId()) assert.Equal(t, uint64(7), listAllWithotherUser[6].GetId()) - assert.Equal(t, "otherUser-node-1", listAllWithotherUser[5].GetName()) - assert.Equal(t, "otherUser-node-2", listAllWithotherUser[6].GetName()) + assert.Equal(t, "otheruser-node-1", listAllWithotherUser[5].GetName()) + assert.Equal(t, "otheruser-node-2", listAllWithotherUser[6].GetName()) // Test list all nodes after added otherUser var listOnlyotherUserMachineUser []v1.Node @@ -1248,12 +1248,12 @@ func TestNodeCommand(t *testing.T) { assert.Equal( t, - "otherUser-node-1", + "otheruser-node-1", listOnlyotherUserMachineUser[0].GetName(), ) assert.Equal( t, - "otherUser-node-2", + "otheruser-node-2", listOnlyotherUserMachineUser[1].GetName(), ) @@ -1558,7 +1558,7 @@ func TestNodeRenameCommand(t *testing.T) { strings.Repeat("t", 64), }, ) - assert.ErrorContains(t, err, "not be over 63 chars") + assert.ErrorContains(t, err, "must not exceed 63 characters") var listAllAfterRenameAttempt []v1.Node err = executeAndUnmarshal( diff --git a/integration/general_test.go b/integration/general_test.go index ab6d4f71..83160e9b 100644 --- a/integration/general_test.go +++ b/integration/general_test.go @@ -514,7 +514,7 @@ func TestUpdateHostnameFromClient(t *testing.T) { hostnames := map[string]string{ "1": "user1-host", - "2": "User2-Host", + "2": "user2-host", "3": "user3-host", } @@ -577,7 +577,11 @@ func TestUpdateHostnameFromClient(t *testing.T) { for _, node := range nodes { hostname := hostnames[strconv.FormatUint(node.GetId(), 10)] assert.Equal(ct, hostname, node.GetName(), "Node name should match hostname") - assert.Equal(ct, util.ConvertWithFQDNRules(hostname), node.GetGivenName(), "Given name should match FQDN rules") + + // GivenName is normalized (lowercase, invalid chars stripped) + normalised, err := util.NormaliseHostname(hostname) + assert.NoError(ct, err) + assert.Equal(ct, normalised, node.GetGivenName(), "Given name should match FQDN rules") } }, 20*time.Second, 1*time.Second) @@ -675,12 +679,13 @@ func TestUpdateHostnameFromClient(t *testing.T) { for _, node := range nodes { hostname := hostnames[strconv.FormatUint(node.GetId(), 10)] givenName := fmt.Sprintf("%d-givenname", node.GetId()) - if node.GetName() != hostname+"NEW" || node.GetGivenName() != givenName { + // Hostnames are lowercased before being stored, so "NEW" becomes "new" + if node.GetName() != hostname+"new" || node.GetGivenName() != givenName { return false } } return true - }, time.Second, 50*time.Millisecond, "hostname updates should be reflected in node list with NEW suffix") + }, time.Second, 50*time.Millisecond, "hostname updates should be reflected in node list with new suffix") } func TestExpireNode(t *testing.T) {