From c497612c999597e0ed5fd59fa748f50b00a44a80 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:42 +0000 Subject: [PATCH] state: reject renames whose FQDN exceeds the hostname limit A valid label can still overflow 255 chars under a long base_domain; gate RenameNode with the new types.ValidateGivenName. Updates #3346 --- hscontrol/state/rename_test.go | 40 ++++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 5 ++++- hscontrol/types/node.go | 23 +++++++++++++++++++ hscontrol/types/node_test.go | 27 +++++++++++++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 hscontrol/state/rename_test.go diff --git a/hscontrol/state/rename_test.go b/hscontrol/state/rename_test.go new file mode 100644 index 00000000..b141147b --- /dev/null +++ b/hscontrol/state/rename_test.go @@ -0,0 +1,40 @@ +package state + +import ( + "strings" + "testing" + + "github.com/juanfont/headscale/hscontrol/db" + "github.com/stretchr/testify/require" +) + +// TestRenameNodeRejectsNameExceedingFQDNLimit proves RenameNode rejects a name +// that is a valid DNS label but whose FQDN, under the configured base_domain, +// exceeds MaxHostnameLength. Without the FQDN-length gate such a name persists +// and then breaks map generation for the node and its peers (issue #3346): +// admin-facing writes must not be able to introduce an unmappable name. +func TestRenameNodeRejectsNameExceedingFQDNLimit(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + // A long base domain so a 63-char label overflows the 255-char FQDN bound. + cfg.BaseDomain = strings.Repeat("b", 200) + ".example.com" + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("rename-user") + node := database.CreateRegisteredNodeForTest(user, "rename-node") + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // Valid 63-char DNS label, but the resulting FQDN exceeds 255 chars. + _, _, err = s.RenameNode(node.ID, strings.Repeat("a", 63)) + require.Error(t, err, "rename to a name whose FQDN exceeds the limit must be rejected") + + // A short, valid name is still accepted. + _, _, err = s.RenameNode(node.ID, "short") + require.NoError(t, err) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 7432e2bb..18cc00e8 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1032,7 +1032,10 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t // auto-sanitisation) and collisions error out rather than silently // bumping a user-facing label. See HOSTNAME.md for the CLI contract. func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, change.Change, error) { - err := dnsname.ValidLabel(newName) + // Validate the label AND that the resulting FQDN fits MaxHostnameLength: + // a valid 63-char label can still overflow under a long base_domain, and + // an unmappable name would break this node and its peers (issue #3346). + err := types.ValidateGivenName(newName, s.cfg.BaseDomain) if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %w", ErrGivenNameInvalid, err) } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index dd3dc92d..ef946737 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -19,6 +19,7 @@ import ( "tailscale.com/tailcfg" "tailscale.com/types/key" "tailscale.com/types/views" + "tailscale.com/util/dnsname" ) var ( @@ -517,6 +518,28 @@ func (node *Node) GetFQDN(baseDomain string) (string, error) { return hostname, nil } +// ValidateGivenName reports whether givenName is usable as a node's DNS label: +// a valid DNS label that, combined with baseDomain, yields an FQDN within +// MaxHostnameLength. Admin-facing write paths (e.g. node rename) reject names +// that fail this, since the mapper cannot build a map for a node — or any of +// its peers — whose GetFQDN fails. Derived paths sanitise/coerce instead. +func ValidateGivenName(givenName, baseDomain string) error { + err := dnsname.ValidLabel(givenName) + if err != nil { + return fmt.Errorf("%q is not a valid DNS label: %w", givenName, err) + } + + // Reuse GetFQDN so the length bound stays identical to what the mapper + // enforces; a valid 63-char label can still overflow under a long + // base_domain. + _, err = (&Node{GivenName: givenName}).GetFQDN(baseDomain) + if err != nil { + return err + } + + return nil +} + // AnnouncedRoutes returns the list of routes the node announces, as // reported by the client in [tailcfg.Hostinfo.RoutableIPs]. Announcement alone // does not grant visibility — see [Node.SubnetRoutes] for approval-gated diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 3c8a5ba8..6a23fc9f 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -417,6 +417,33 @@ func TestNodeFQDN(t *testing.T) { } } +func TestValidateGivenName(t *testing.T) { + tests := []struct { + name string + givenName string + baseDomain string + wantErr bool + }{ + {"valid", "test", "example.com", false}, + {"empty", "", "example.com", true}, + {"invalid label chars", "not valid", "example.com", true}, + {"label too long", strings.Repeat("a", 64), "example.com", true}, + // A valid 63-char label whose FQDN overflows only because the base + // domain is long: ValidLabel passes, the FQDN-length bound rejects it. + {"fqdn too long under long base domain", strings.Repeat("a", 63), strings.Repeat("b", 200) + ".example.com", true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := ValidateGivenName(tc.givenName, tc.baseDomain) + if (err != nil) != tc.wantErr { + t.Errorf("ValidateGivenName(%q, %q) error = %v, wantErr %v", + tc.givenName, tc.baseDomain, err, tc.wantErr) + } + }) + } +} + func TestPeerChangeFromMapRequest(t *testing.T) { nKeys := []key.NodePublic{ key.NewNode().Public(),