From 08956d51a45c52434b1dbc7aa3d33fd373fb0192 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:19 +0000 Subject: [PATCH] mapper: skip peers with invalid names instead of failing the map A peer whose GivenName fails GetFQDN aborted the whole map for every node that could see it. Drop and log it; SSH policy errors degrade too. Fixes #3346 --- hscontrol/mapper/builder.go | 26 +++++++++- hscontrol/mapper/mapper_test.go | 89 +++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index 4a8e25ae..eaf4dde4 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -9,6 +9,8 @@ import ( "github.com/juanfont/headscale/hscontrol/policy" policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" + "github.com/rs/zerolog/log" "tailscale.com/tailcfg" "tailscale.com/types/views" "tailscale.com/util/multierr" @@ -152,7 +154,15 @@ func (b *MapResponseBuilder) WithSSHPolicy() *MapResponseBuilder { sshPolicy, err := b.mapper.state.SSHPolicy(node) if err != nil { - b.addError(err) + // SSH policy is optional for a node to function. Rather than fail the + // whole map (leaving the node unable to connect), log and continue + // without it; the node still receives a usable netmap. + log.Warn().Caller(). + Err(err). + Uint64(zf.NodeID, node.ID().Uint64()). + Str(zf.NodeHostname, node.Hostname()). + Msg("building map response: skipping SSH policy for node; node will receive a map without SSH rules") + return b } @@ -284,7 +294,19 @@ func (b *MapResponseBuilder) buildTailPeers(peers views.Slice[types.NodeView]) ( return b.mapper.state.RoutesForPeer(node, peer, matchers) }, b.mapper.cfg, allCapMaps[peer.ID()]) if err != nil { - return nil, err + // One peer with invalid data (e.g. an empty or over-long + // GivenName that fails GetFQDN) must not blank out the map for + // every node that can see it. Drop the offending peer, log it + // with the identity an operator needs to fix it, and keep + // building from the remaining valid peers. + log.Warn().Caller(). + Err(err). + Uint64(zf.NodeID, peer.ID().Uint64()). + Str(zf.NodeHostname, peer.Hostname()). + Uint64("map.viewer.node.id", b.nodeID.Uint64()). + Msgf("dropping peer %d from map response: invalid node data; fix with `headscale nodes rename %d `", peer.ID(), peer.ID()) + + continue } // [tailcfg.Node.CapMap] on a peer carries the small set of diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 94880c78..9b19c949 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -3,6 +3,7 @@ package mapper import ( "fmt" "net/netip" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -536,6 +537,94 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { } } +// TestFullMapResponseSurvivesPeerWithInvalidName proves a single node with an +// FQDN-invalid GivenName must not break map generation for its peers. +// +// A node whose stored GivenName is empty (ErrNodeHasNoGivenName) or yields an +// FQDN longer than MaxHostnameLength (ErrHostnameTooLong) makes GetFQDN, and +// therefore TailNode, return an error. buildTailPeers used to abort the entire +// peer list on the first such error, so MapResponseBuilder.Build() failed for +// every node that could see the bad peer; on the initial-connection path that +// surfaced as "PollNetMap: ... unexpected EOF" and the "Unable to connect to +// the Tailscale coordination server" health warning. A legacy DB row loads +// verbatim (NewNodeStore reads db.ListNodes() without re-sanitising names), so +// the bad peer persists across restart. The build for an unaffected viewer +// must succeed: the bad peer is dropped, valid peers and self survive. +func TestFullMapResponseSurvivesPeerWithInvalidName(t *testing.T) { + for _, tt := range []struct { + name string + badName string + }{ + {"empty given name", ""}, + {"over-long fqdn", strings.Repeat("a", types.MaxHostnameLength+1)}, + } { + t.Run(tt.name, func(t *testing.T) { + tmp := t.TempDir() + p4 := netip.MustParsePrefix("100.64.0.0/10") + p6 := netip.MustParsePrefix("fd7a:115c:a1e0::/48") + cfg := &types.Config{ + Database: types.DatabaseConfig{ + Type: types.DatabaseSqlite, + Sqlite: types.SqliteConfig{Path: tmp + "/h.db"}, + }, + PrefixV4: &p4, + PrefixV6: &p6, + IPAllocation: types.IPAllocationStrategySequential, + BaseDomain: "headscale.test", + Policy: types.PolicyConfig{Mode: types.PolicyModeDB}, + DERP: types.DERPConfig{ + DERPMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{999: {RegionID: 999}}, + }, + }, + Tuning: types.Tuning{ + NodeStoreBatchSize: state.TestBatchSize, + NodeStoreBatchTimeout: state.TestBatchTimeout, + }, + } + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("u1") + n1 := database.CreateRegisteredNodeForTest(user, "n1") // viewer, valid + bad := database.CreateRegisteredNodeForTest(user, "bad") // peer, name corrupted below + good := database.CreateRegisteredNodeForTest(user, "good") // peer, valid control + + // Simulate a legacy/corrupt row that v29 loads verbatim. + require.NoError(t, database.DB. + Model(&types.Node{}). + Where("id = ?", bad.ID). + Update("given_name", tt.badName).Error) + require.NoError(t, database.Close()) + + s, err := state.NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // Allow-all so n1 sees both peers; the bad one must still be dropped. + _, err = s.SetPolicy([]byte(`{"acls":[{"action":"accept","src":["*"],"dst":["*:*"]}]}`)) + require.NoError(t, err) + + m := &mapper{state: s, cfg: cfg} + capVer := tailcfg.CurrentCapabilityVersion + + resp, err := m.fullMapResponse(n1.ID, capVer) + require.NoError(t, err, "n1's map must build despite a peer with an invalid name") + require.NotNil(t, resp) + require.NotNil(t, resp.Node, "n1 must receive its own self node") + + peers := map[tailcfg.NodeID]bool{} + for _, p := range resp.Peers { + peers[p.ID] = true + } + + assert.False(t, peers[bad.ID.NodeID()], "the peer with an invalid name must be dropped") + assert.True(t, peers[good.ID.NodeID()], "valid peers must remain in the map") + }) + } +} + // TestGenerateDNSConfigNilHostinfoNoPanic proves generateDNSConfig does not // panic when a node's Hostinfo is nil (e.g. a legacy DB row with a NULL // host_info column). addNextDNSMetadata dereferenced node.Hostinfo().OS()