From e3c1bfd68e40cb037cfaaa81eeeb91401b164945 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 28 Oct 2026 13:30:46 +0000 Subject: [PATCH] mapper: skip peers whose key expiry cannot be encoded json.Marshal refuses a year outside [0,9999], so one stored expiry failed every map with that node. TailNode and key patches send UTC, the builder drops what fails and the boot scan names the node. Fixes ANT-2026-QMCAFJ1Q --- CHANGELOG.md | 1 + hscontrol/mapper/builder.go | 26 +++++++++++++-- hscontrol/mapper/builder_test.go | 21 ++++++++++++ hscontrol/mapper/mapper_test.go | 47 ++++++++++++++++++++------- hscontrol/state/node_health.go | 30 ++++++++++++++--- hscontrol/state/node_health_test.go | 31 ++++++++++++++++++ hscontrol/types/change/change.go | 6 ++-- hscontrol/types/change/change_test.go | 17 ++++++++++ hscontrol/types/node.go | 27 ++++++++++++++- 9 files changed, 185 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d5873850..de3e52204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -156,6 +156,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - A route that is a single address inside the node ranges, in either spelling, is approved and listed but never served; wider routes and `::ffff:` prefixes are still served - Require a valid pre-auth key to re-register an expired tagged node - Another user's key or login on a tagged machine now creates a new device for that user instead of re-authenticating the tagged node +- Fix one node with a stored key expiry outside the years 0 to 9999 in UTC failing the map of every node that can see it; it is now left out of its peers' maps and the startup check names it with the fix command ## 0.29.7 (202x-xx-xx) diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index 5ea2f5a45..795aab706 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -2,6 +2,8 @@ package mapper import ( "cmp" + "errors" + "fmt" "net/netip" "slices" "time" @@ -283,12 +285,18 @@ func (b *MapResponseBuilder) buildTailPeers(peers views.Slice[types.NodeView]) ( // 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. + fix := fmt.Sprintf("headscale nodes rename %d ", peer.ID()) + if errors.Is(err, types.ErrExpiryOutOfRange) { + fix = fmt.Sprintf("headscale nodes expire --identifier %d", peer.ID()) + } + 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()) + Msgf("dropping peer %d from map response: invalid node data; "+ + "fix with `%s`", peer.ID(), fix) continue } @@ -321,9 +329,21 @@ func (b *MapResponseBuilder) WithPingRequest(pr *tailcfg.PingRequest) *MapRespon return b } -// WithPeerChangedPatch adds peer change patches. +// WithPeerChangedPatch adds peer change patches. A patch whose key expiry +// cannot be encoded is left out, as [MapResponseBuilder.buildTailPeers] leaves +// out its peer, so it cannot fail the whole response. func (b *MapResponseBuilder) WithPeerChangedPatch(changes []*tailcfg.PeerChange) *MapResponseBuilder { - b.resp.PeersChangedPatch = changes + for _, pc := range changes { + if pc.KeyExpiry != nil { + _, err := types.ValidateExpiry(*pc.KeyExpiry) + if err != nil { + continue + } + } + + b.resp.PeersChangedPatch = append(b.resp.PeersChangedPatch, pc) + } + return b } diff --git a/hscontrol/mapper/builder_test.go b/hscontrol/mapper/builder_test.go index 8581d835b..d282f542f 100644 --- a/hscontrol/mapper/builder_test.go +++ b/hscontrol/mapper/builder_test.go @@ -1,6 +1,7 @@ package mapper import ( + "encoding/json" "testing" "time" @@ -168,6 +169,26 @@ func TestMapResponseBuilder_WithPeerChangedPatch(t *testing.T) { assert.False(t, builder.hasErrors()) } +// TestMapResponseBuilder_WithPeerChangedPatchDropsUnencodableExpiry proves a +// patch whose key expiry json.Marshal refuses is left out, as buildTailPeers +// leaves out such a peer, instead of failing the viewer's whole response. +func TestMapResponseBuilder_WithPeerChangedPatchDropsUnencodableExpiry(t *testing.T) { + m := &mapper{cfg: &types.Config{}, state: &state.State{}} + + latest := time.Date(9999, time.December, 31, 23, 59, 59, 0, time.UTC) + beyond := latest.Add(time.Second) + changes := []*tailcfg.PeerChange{ + {NodeID: 123, KeyExpiry: &beyond}, + {NodeID: 456, KeyExpiry: &latest}, + } + + builder := m.NewMapResponseBuilder(types.NodeID(1)).WithPeerChangedPatch(changes) + assert.Equal(t, changes[1:], builder.resp.PeersChangedPatch) + + _, err := json.Marshal(builder.resp) + require.NoError(t, err) +} + func TestMapResponseBuilder_WithPeersRemoved(t *testing.T) { cfg := &types.Config{} mockState := &state.State{} diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 12f6fd140..49ddbeff1 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -552,8 +552,8 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { } } -// TestFullMapResponseSurvivesPeerWithInvalidName proves a single node with an -// FQDN-invalid GivenName must not break map generation for its peers. +// TestFullMapResponseSurvivesPeerWithInvalidData proves a single node with +// stored data that cannot be sent 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 @@ -565,13 +565,32 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { // 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) { +// +// A key expiry outside the years [0,9999] in UTC is the same class, caught +// later: TailNode sends it in UTC and json.Marshal refuses it. +func TestFullMapResponseSurvivesPeerWithInvalidData(t *testing.T) { + // Each expiry is in range in its own zone and out of range in UTC. + west := time.FixedZone("", -(23*60+59)*60) + east := time.FixedZone("", (23*60+59)*60) + for _, tt := range []struct { - name string - badName string + name string + column string + value any }{ - {"empty given name", ""}, - {"over-long fqdn", strings.Repeat("a", types.MaxHostnameLength+1)}, + {"empty given name", "given_name", ""}, + { + "over-long fqdn", "given_name", + strings.Repeat("a", types.MaxHostnameLength+1), + }, + { + "expiry after year 9999", "expiry", + time.Date(9999, time.December, 31, 23, 59, 59, 0, west), + }, + { + "expiry before year 0", "expiry", + time.Date(0, time.January, 1, 0, 0, 0, 0, east), + }, } { t.Run(tt.name, func(t *testing.T) { tmp := t.TempDir() @@ -603,14 +622,14 @@ func TestFullMapResponseSurvivesPeerWithInvalidName(t *testing.T) { user := database.CreateUserForTest("u1") n1 := database.CreateRegisteredNodeForTest(user, "n1") // viewer, valid - bad := database.CreateRegisteredNodeForTest(user, "bad") // peer, name corrupted below + bad := database.CreateRegisteredNodeForTest(user, "bad") // peer, bad data 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) + Update(tt.column, tt.value).Error) require.NoError(t, database.Close()) s, err := state.NewState(cfg) @@ -625,16 +644,22 @@ func TestFullMapResponseSurvivesPeerWithInvalidName(t *testing.T) { 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.NoError(t, err, + "n1's map must build despite a peer with invalid data") require.NotNil(t, resp) require.NotNil(t, resp.Node, "n1 must receive its own self node") + _, err = json.Marshal(resp) + require.NoError(t, err, + "n1's map must encode despite a peer with invalid data") + 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.False(t, peers[bad.ID.NodeID()], + "the peer with invalid data must be dropped") assert.True(t, peers[good.ID.NodeID()], "valid peers must remain in the map") }) } diff --git a/hscontrol/state/node_health.go b/hscontrol/state/node_health.go index 4edaf3986..c4347e7c5 100644 --- a/hscontrol/state/node_health.go +++ b/hscontrol/state/node_health.go @@ -19,9 +19,9 @@ type nodeHealthCheck struct { check func(nv types.NodeView, cfg *types.Config) (problem, fixHint string, ok bool) } -// nodeHealthChecks is the registry of node-data health checks. Today it carries -// the one issue #3346 needs; append to it rather than reshaping callers. -var nodeHealthChecks = []nodeHealthCheck{givenNameMapsToValidFQDN} +// nodeHealthChecks is the registry of node-data health checks; append to it +// rather than reshaping callers. +var nodeHealthChecks = []nodeHealthCheck{givenNameMapsToValidFQDN, expiryIsEncodable} // givenNameMapsToValidFQDN flags a node whose stored GivenName cannot produce a // valid FQDN (empty, or longer than MaxHostnameLength once base_domain is @@ -39,6 +39,27 @@ var givenNameMapsToValidFQDN = nodeHealthCheck{ }, } +// expiryIsEncodable flags a node whose stored key expiry cannot be encoded for +// clients. Such a node cannot be rendered into a netmap — neither its own nor +// any peer's — so its expiry must be reset to recover. +var expiryIsEncodable = nodeHealthCheck{ + name: "expiry-is-encodable", + check: func(nv types.NodeView, _ *types.Config) (string, string, bool) { + // No expiry is sent as the zero time, which is in range. + expiry, _ := nv.Expiry().GetOk() + + _, err := types.ValidateExpiry(expiry) + if err != nil { + return err.Error(), fmt.Sprintf( + "headscale nodes expire --identifier %d", + nv.ID(), + ), false + } + + return "", "", true + }, +} + // nodeHealthFinding is a single failed check for a single node. type nodeHealthFinding struct { nodeID types.NodeID @@ -87,6 +108,7 @@ func (s *State) logNodeHealth() { Str("check", f.check). Str("problem", f.problem). Str("fix", f.fixHint). - Msg("node has invalid data that breaks map generation; rename it to restore connectivity") + Msg("node has invalid data that breaks map generation; " + + "run the fix to restore connectivity") } } diff --git a/hscontrol/state/node_health_test.go b/hscontrol/state/node_health_test.go index c7b27623b..720a6cb72 100644 --- a/hscontrol/state/node_health_test.go +++ b/hscontrol/state/node_health_test.go @@ -2,6 +2,7 @@ package state import ( "testing" + "time" "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/types" @@ -20,6 +21,36 @@ func TestGivenNameMapsToValidFQDNCheck(t *testing.T) { require.Contains(t, fixHint, "rename 7", "fix hint must name the offending node") } +func TestExpiryIsEncodableCheck(t *testing.T) { + cfg := &types.Config{} + + _, _, ok := expiryIsEncodable.check((&types.Node{ID: 1}).View(), cfg) + require.True(t, ok, "a node without expiry must pass the check") + + earliest := time.Date(0, time.January, 1, 0, 0, 0, 0, time.UTC) + latest := time.Date(9999, time.December, 31, 23, 59, 59, 0, time.UTC) + + for _, tt := range []struct { + expiry time.Time + ok bool + }{ + {earliest, true}, + {earliest.Add(-time.Second), false}, + {latest, true}, + {latest.Add(time.Second), false}, + } { + node := &types.Node{ID: 7, Expiry: &tt.expiry} + problem, fixHint, ok := expiryIsEncodable.check(node.View(), cfg) + require.Equal(t, tt.ok, ok, "expiry %s", tt.expiry) + + if !ok { + require.NotEmpty(t, problem) + require.Equal(t, "headscale nodes expire --identifier 7", fixHint, + "the fix must name the node and keep an expired one expired") + } + } +} + // TestScanNodeHealthReportsInvalidNameWithoutMutating proves the boot scan // reports a node whose stored name would break map generation (issue #3346) // with an actionable fix, and that it never rewrites the stored name — the diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index b9678edb4..14091bd13 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -604,10 +604,12 @@ func NodeKeyRotated(node types.NodeView) Change { // KeyExpiry is always set: the zero value clears any prior expiry // timestamp on the peer, and a non-zero value carries the new expiry. It - // does not clear [tailcfg.Node.Expired]. + // does not clear [tailcfg.Node.Expired]. It is sent in UTC, as + // [types.NodeView.TailNode] sends it: the stored zone depends on the + // database. var expiry time.Time if e, ok := node.Expiry().GetOk(); ok { - expiry = e + expiry = e.UTC() } c := PeerPatched("node key rotated (relogin)", &tailcfg.PeerChange{ diff --git a/hscontrol/types/change/change_test.go b/hscontrol/types/change/change_test.go index 589efb277..0ab10d066 100644 --- a/hscontrol/types/change/change_test.go +++ b/hscontrol/types/change/change_test.go @@ -1,6 +1,7 @@ package change import ( + "encoding/json" "net/netip" "reflect" "testing" @@ -987,3 +988,19 @@ func TestNodeKeyRotatedEmitsPatchNotWholeNode(t *testing.T) { assert.Equal(t, expiry, *patch.KeyExpiry) assert.Equal(t, []netip.AddrPort(node.Endpoints), patch.Endpoints, "patch must carry endpoints") } + +// TestNodeKeyRotatedSendsExpiryInUTC proves the patch carries the expiry in +// UTC, as [types.NodeView.TailNode] does. PostgreSQL loads it in the server's +// zone, where an expiry late in 9999 is already in the year 10000, which +// json.Marshal refuses. +func TestNodeKeyRotatedSendsExpiryInUTC(t *testing.T) { + latest := time.Date(9999, time.December, 31, 23, 59, 59, 0, time.UTC) + loaded := latest.In(time.FixedZone("", 60*60)) + + c := NodeKeyRotated((&types.Node{ID: 7, Expiry: &loaded}).View()) + require.Len(t, c.PeerPatches, 1) + + _, err := json.Marshal(c.PeerPatches[0]) + require.NoError(t, err) + assert.True(t, latest.Equal(*c.PeerPatches[0].KeyExpiry)) +} diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 50c029302..4c949e601 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -31,6 +31,9 @@ var ( ErrNodeUserHasNoName = errors.New("node user has no name") ErrCannotRemoveAllTags = errors.New("cannot remove all tags from node") ErrInvalidNodeView = errors.New("cannot convert invalid NodeView to tailcfg.Node") + ErrExpiryOutOfRange = errors.New( + "key expiry is outside the years 0 to 9999 in UTC", + ) ) // RouteFunc is a function that takes a node ID and returns a list of @@ -553,6 +556,21 @@ func ValidateGivenName(givenName, baseDomain string) error { return nil } +// ValidateExpiry returns expiry in UTC, as [tailcfg.Node.KeyExpiry] carries it, +// or an error when it cannot be sent to clients. [time.Time.MarshalJSON] refuses +// a year outside [0,9999], failing every map response that carries it. A time +// that is in range in its own zone can still leave the range in UTC. +func ValidateExpiry(expiry time.Time) (time.Time, error) { + utc := expiry.UTC() + if year := utc.Year(); year < 0 || year > 9999 { + return time.Time{}, fmt.Errorf( + "%w: %s", ErrExpiryOutOfRange, expiry.Format(time.RFC3339), + ) + } + + return utc, 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 @@ -1269,6 +1287,13 @@ func (nv NodeView) TailNode( keyExpiry = nv.Expiry().Get() } + // Fail here, where the mapper drops just this peer, rather than in + // json.Marshal, which fails the whole response. + keyExpiry, err = ValidateExpiry(keyExpiry) + if err != nil { + return nil, err + } + // disable-ipv4 (https://tailscale.com/docs/reference/troubleshooting/network-configuration/cgnat-conflicts) // drops the node's own IPv4 CGNAT prefix from Addresses and from // the AllowedIPs slot the node's own /32 occupies. Advertised @@ -1341,7 +1366,7 @@ func (nv NodeView) TailNode( User: nv.TailscaleUserID(), Key: nv.NodeKey(), - KeyExpiry: keyExpiry.UTC(), + KeyExpiry: keyExpiry, Machine: nv.MachineKey(), DiscoKey: nv.DiscoKey(),