mirror of
https://github.com/juanfont/headscale.git
synced 2026-11-04 04:34:07 +09:00
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.
This commit is contained in:
1 parent
ce5189dc55
commit
f192bf05b2
8 files changed
+184
-21
No files matched your search
@@ -2,6 +2,8 @@ package mapper
|
||||
|
||||
import (
|
||||
"cmp"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/netip"
|
||||
"slices"
|
||||
"time"
|
||||
@@ -286,12 +288,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 <name>", 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 <name>`", peer.ID(), peer.ID())
|
||||
Msgf("dropping peer %d from map response: invalid node data; "+
|
||||
"fix with `%s`", peer.ID(), fix)
|
||||
|
||||
continue
|
||||
}
|
||||
@@ -324,9 +332,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
|
||||
}
|
||||
|
||||
|
||||
@@ -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{}
|
||||
|
||||
@@ -687,8 +687,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
|
||||
@@ -700,13 +700,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()
|
||||
@@ -738,14 +757,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)
|
||||
@@ -760,16 +779,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")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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{
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
+26
-1
@@ -32,6 +32,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
|
||||
@@ -592,6 +595,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
|
||||
@@ -1322,6 +1340,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
|
||||
@@ -1394,7 +1419,7 @@ func (nv NodeView) TailNode(
|
||||
User: nv.TailscaleUserID(),
|
||||
|
||||
Key: nv.NodeKey(),
|
||||
KeyExpiry: keyExpiry.UTC(),
|
||||
KeyExpiry: keyExpiry,
|
||||
|
||||
Machine: nv.MachineKey(),
|
||||
DiscoKey: nv.DiscoKey(),
|
||||
|
||||
Reference in new issue
Block a user