diff --git a/hscontrol/app.go b/hscontrol/app.go index e3e6c9d2c..e7f456a0f 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -443,6 +443,7 @@ func (h *Headscale) scheduledTasks(ctx context.Context) { } h.cfg.SetExtraRecords(records) + h.state.WarnNodesHoldingExtraRecords() h.Change(change.ExtraRecords()) @@ -646,6 +647,9 @@ func (h *Headscale) Serve() error { defer h.extraRecordMan.Close() } + // Covers the records from the config too. + h.state.WarnNodesHoldingExtraRecords() + // Start all scheduled tasks, e.g. expiring nodes, derp updates and // records updates scheduleCtx, scheduleCancel := context.WithCancel(context.Background()) diff --git a/hscontrol/servertest/names_compat_test.go b/hscontrol/servertest/names_compat_test.go index 9762504ac..556674c4f 100644 --- a/hscontrol/servertest/names_compat_test.go +++ b/hscontrol/servertest/names_compat_test.go @@ -74,28 +74,16 @@ var nameSkipReasons = map[string]string{ "names/N12b/pdb/label": adminNameFollowsHostname, "names/N13/pdb/label": adminNameFollowsHostname, - "names/N9a/0/message": caseTwinTakenText, - "names/N9b/0/message": caseTwinTakenText, + "names/N9a/0/message": nameTakenText, + "names/N9b/0/message": nameTakenText, - "names-svc/S3/g1/label": extraRecordNotTaken, - "names-svc/S4/g1/label": extraRecordNotTaken, - "names-svc/S4/svc-other/label": extraRecordNotTaken, - "names-svc/S5/g1/label": extraRecordNotTaken, - "names-svc/S5/svc-other/label": extraRecordNotTaken, - "names-svc/S5/0/status": extraRecordNotTaken, - "names-svc/S5/0/message": recordNameTakenText, - "names-svc/S6/g1/label": extraRecordNotTaken, - "names-svc/S6/svc-other/label": extraRecordNotTaken, - "names-svc/S7/g1/label": extraRecordNotTaken, - "names-svc/S7/svc-other/label": extraRecordNotTaken, + "names-svc/S5/0/message": nameTakenText, } const ( labelSuffixOverflow = "LABEL_SUFFIX_OVERFLOW: headscale appends -N to a 63-byte label, giving a 65-byte one; SaaS cuts the label so the suffix fits in 63 bytes" adminNameFollowsHostname = "ADMIN_NAME_FOLLOWS_HOSTNAME: an admin name equal to the hostname's label counts as derived from it, so a hostname change replaces it; SaaS keeps an admin name across hostname changes" - extraRecordNotTaken = "EXTRA_RECORD_NOT_TAKEN: headscale gives a device the label an extra DNS record holds and accepts a rename to it; SaaS bumps a device past a Service's name and refuses the rename with 400" - caseTwinTakenText = "NAME_TAKEN_TEXT: headscale refuses a rename to a case twin saying the node name is not unique, where SaaS says the name is already taken" - recordNameTakenText = "EXTRA_RECORD_NOT_TAKEN+NAME_TAKEN_TEXT: headscale accepts a rename to an extra record's name; refused, its text says the node name is not unique, where SaaS says the name is already taken" + nameTakenText = "NAME_TAKEN_TEXT: headscale's v2 message for a rename refused as taken is the operation, with the reason only in data; SaaS's message says the name is already taken" v2NameBare = "V2_NAME_BARE: headscale's v2 device name is the bare label; SaaS's is the FQDN under the tailnet domain" ) diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 3bd44f6c7..0b58ac071 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -114,6 +114,9 @@ type NodeStore struct { peersFunc PeersFunc writeQueue chan work + // reservedLabel reports labels held outside the NodeStore; nil reserves + // none. Set before Start: the writer reads it without a lock. + reservedLabel func(string) bool // stopped is closed once by Stop to signal the writer goroutine to exit // and to let in-flight writes return cleanly instead of panicking with @@ -522,7 +525,7 @@ func (s *NodeStore) applyBatch(batch []work) { switch w.op { case put: n := w.node - n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName) + n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName, s.reservedLabel) nodes[w.nodeID] = n if w.nodeResult != nil { @@ -543,7 +546,9 @@ func (s *NodeStore) applyBatch(batch []work) { fn(&n) if n.GivenName != oldGivenName { - n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName) + n.GivenName = resolveGivenName( + nodes, n.ID, n.GivenName, s.reservedLabel, + ) } nodes[id] = n @@ -581,17 +586,7 @@ func (s *NodeStore) applyBatch(batch []work) { continue } - taken := false - - // DNS is case-insensitive: a legacy "Laptop" already holds "laptop". - for id, other := range nodes { - if id != w.nodeID && strings.EqualFold(other.GivenName, w.name) { - taken = true - break - } - } - - if taken { + if resolveGivenName(nodes, w.nodeID, w.name, s.reservedLabel) != w.name { setErrResults[w] = ErrGivenNameTaken nodeResultRequests[w.nodeID] = append(nodeResultRequests[w.nodeID], w) @@ -673,7 +668,12 @@ func (s *NodeStore) applyBatch(batch []work) { // unused wins. Must be called from the [NodeStore] writer goroutine // (inside [NodeStore.applyBatch]) so the nodes map reflects all earlier // ops in the batch and no other writer can interleave. -func resolveGivenName(nodes map[types.NodeID]types.Node, self types.NodeID, base string) string { +func resolveGivenName( + nodes map[types.NodeID]types.Node, + self types.NodeID, + base string, + reservedLabel func(string) bool, +) string { if base == "" { base = fallbackGivenName } @@ -690,7 +690,8 @@ func resolveGivenName(nodes map[types.NodeID]types.Node, self types.NodeID, base candidate := base for i := 1; ; i++ { - if _, busy := taken[strings.ToLower(candidate)]; !busy { + _, busy := taken[strings.ToLower(candidate)] + if !busy && (reservedLabel == nil || !reservedLabel(candidate)) { return candidate } diff --git a/hscontrol/state/node_store_hostname_test.go b/hscontrol/state/node_store_hostname_test.go index f95c546e2..dc7db02b2 100644 --- a/hscontrol/state/node_store_hostname_test.go +++ b/hscontrol/state/node_store_hostname_test.go @@ -7,8 +7,36 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" ) +func TestPutNodeBumpsPastReservedLabel(t *testing.T) { + cfg := persistTestConfig(t.TempDir() + "/headscale.db") + cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{ + {Name: "laptop." + cfg.BaseDomain}, + {Name: "laptop-1." + cfg.BaseDomain}, + }} + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + node := s.nodeStore.PutNode(createTestNode(1, 1, "alice", "laptop")) + require.Equal(t, "laptop-2", node.GivenName()) +} + +func TestSetGivenNameRejectsReservedLabel(t *testing.T) { + cfg := persistTestConfig(t.TempDir() + "/headscale.db") + cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{ + {Name: "laptop." + cfg.BaseDomain}, + }} + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + s.nodeStore.PutNode(createTestNode(1, 1, "alice", "phone")) + _, err = s.nodeStore.SetGivenName(1, "laptop") + require.ErrorIs(t, err, ErrGivenNameTaken) +} + // TestPutNodeGivenNameCollisionBumps is the reproduction for #3188. // Before the hostname-cleanroom rewrite, two nodes could be written // with the same GivenName, producing duplicates. After the rewrite, diff --git a/hscontrol/state/rename_test.go b/hscontrol/state/rename_test.go index b141147be..551a02de9 100644 --- a/hscontrol/state/rename_test.go +++ b/hscontrol/state/rename_test.go @@ -5,9 +5,85 @@ import ( "testing" "github.com/juanfont/headscale/hscontrol/db" + "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" + "tailscale.com/types/key" ) +func TestNodeNameCannotShadowExtraRecord(t *testing.T) { + cfg := persistTestConfig(t.TempDir() + "/headscale.db") + cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{ + {Name: "grafana." + cfg.BaseDomain, Type: "A", Value: "100.64.0.100"}, + }} + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + user := s.CreateUserForTest("extra-record-user") + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + register := func(hostname string) types.NodeView { + t.Helper() + + node, _, err := s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: hostname}, + }, key.NewMachine().Public()) + require.NoError(t, err) + + return node + } + static := register("grafana") + assert.Equal(t, "grafana-1", static.GivenName()) + + existing := register("existing") + + cfg.SetExtraRecords([]tailcfg.DNSRecord{ + {Name: "SVC." + cfg.BaseDomain + ".", Type: "A", Value: "100.64.0.101"}, + {Name: "existing." + cfg.BaseDomain, Type: "A", Value: "100.64.0.102"}, + {Name: "child.parent." + cfg.BaseDomain, Type: "A", Value: "100.64.0.103"}, + }) + + fresh, _ := s.GetNodeByID(existing.ID()) + assert.Equal( + t, + "existing", + fresh.GivenName(), + "a records reload keeps existing holders", + ) + assert.Equal(t, []types.NodeID{existing.ID()}, s.WarnNodesHoldingExtraRecords()) + + dynamic := register("svc") + assert.Equal(t, "svc-1", dynamic.GivenName()) + + _, _, err = s.RenameNode(existing.ID(), "Svc") + require.ErrorIs(t, err, ErrNodeNameNotUnique) + _, err = s.UpdateNodeFromMapRequest(existing.ID(), tailcfg.MapRequest{ + NodeKey: existing.NodeKey(), Hostinfo: &tailcfg.Hostinfo{Hostname: "svc"}, + }) + require.NoError(t, err) + + fresh, _ = s.GetNodeByID(existing.ID()) + assert.Equal(t, "svc-2", fresh.GivenName()) + + parent := register("parent") + assert.Equal( + t, + "parent", + parent.GivenName(), + "only exact record names are reserved", + ) + cfg.SetExtraRecords(nil) + + renamed, _, err := s.RenameNode(existing.ID(), "Svc") + require.NoError(t, err) + assert.Equal(t, "svc", renamed.GivenName()) +} + // 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 diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1c342c52d..828eee802 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -397,6 +397,9 @@ func NewState(cfg *types.Config) (*State, error) { nodeStore := NewNodeStore(nodes, policyPeersFunc(polMan), batchSize, batchTimeout) polMan.SetRouterSession(nodeStore.RouterSession) + nodeStore.reservedLabel = func(label string) bool { + return cfg.BaseDomain != "" && cfg.HasExtraRecord(label+"."+cfg.BaseDomain) + } nodeStore.Start() s := &State{ @@ -1394,6 +1397,27 @@ func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, return nodeView, c, nil } +// WarnNodesHoldingExtraRecords logs, and returns, the nodes whose name is also +// an extra DNS record. They keep it: only an operator adding the record can +// cause this, so the operator picks which one to rename. +func (s *State) WarnNodesHoldingExtraRecords() []types.NodeID { + var ids []types.NodeID + + for _, n := range s.nodeStore.ListNodes().All() { + if !s.nodeStore.reservedLabel(n.GivenName()) { + continue + } + + ids = append(ids, n.ID()) + log.Warn(). + Uint64(zf.NodeID, n.ID().Uint64()). + Str("record", n.GivenName()+"."+s.cfg.BaseDomain). + Msg("node keeps a name that is also an extra DNS record") + } + + return ids +} + // BackfillNodeIPs assigns IP addresses to nodes that don't have them. The // returned changes tell clients about the new addresses. // Like the other writes, it returns the changes alongside an error once the diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index f105d2eca..63de779b2 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -2021,10 +2021,17 @@ func (c *Config) CloneTailcfgDNSConfig() *tailcfg.DNSConfig { return c.TailcfgDNSConfig.Clone() } -// extraRecordAtOrBelow reports whether an extra DNS record is name or a -// subdomain of it. Case is ignored, as in TLS name matching; name is +// HasExtraRecord reports whether an extra DNS record is exactly name, +// ignoring case and a trailing dot. Safe for concurrent use with +// [Config.SetExtraRecords]. +func (c *Config) HasExtraRecord(name string) bool { + return c.extraRecordAt(strings.ToLower(strings.TrimSuffix(name, ".")), false) +} + +// extraRecordAt reports whether an extra DNS record is name or, with orBelow, +// a subdomain of it. Case is ignored, as in TLS name matching; name is // lowercase. -func (c *Config) extraRecordAtOrBelow(name string) bool { +func (c *Config) extraRecordAt(name string, orBelow bool) bool { tailcfgDNSMu.RLock() defer tailcfgDNSMu.RUnlock() @@ -2034,7 +2041,7 @@ func (c *Config) extraRecordAtOrBelow(name string) bool { for _, r := range c.TailcfgDNSConfig.ExtraRecords { rn := strings.ToLower(strings.TrimSuffix(r.Name, ".")) - if rn == name || strings.HasSuffix(rn, "."+name) { + if rn == name || (orBelow && strings.HasSuffix(rn, "."+name)) { return true } } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 0efcdfab8..b7f0a9af7 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -926,7 +926,7 @@ func (nv NodeView) CertDomain(cfg *Config) (string, bool) { // certificate for a name the operator gave to a service. A wildcard // order shares its challenge record with the bare name, so names // below the node's are covered too. - if cfg.extraRecordAtOrBelow(domain) { + if cfg.extraRecordAt(domain, true) { return "", false }