mirror of
https://github.com/juanfont/headscale.git
synced 2026-10-30 18:24:06 +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. Fixes ANT-2026-QMCAFJ1Q
This commit is contained in:
1 parent
b23d4dc535
commit
e3c1bfd68e
9 files changed
+185
-21
No files matched your search
@@ -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)
|
||||
|
||||
|
||||
@@ -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 <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
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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{}
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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
@@ -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(),
|
||||
|
||||
Reference in new issue
Block a user