diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index 20b9e9186..1e214a11b 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -679,6 +679,47 @@ func TestNodeExpiryRouteFailover(t *testing.T) { } } +// TestReloginWithoutExpiryClearsPeerExpiry: a relogin can drop a node's expiry on +// the default config, and peers must not keep the old one and cut the node off. +func TestReloginWithoutExpiryClearsPeerExpiry(t *testing.T) { + t.Parallel() + + h := servertest.NewHarness(t, 2) + client, observer := h.Client(0), h.Client(1) + id := findNodeID(t, h.Server, client.Name) + + future := time.Now().Add(time.Hour).UTC() + _, c, err := h.Server.State().SetNodeExpiry(id, &future) + require.NoError(t, err) + h.Server.App.Change(c) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + p, ok := observer.PeerByName(client.Name) + if assert.True(c, ok) { + assert.True(c, p.KeyExpiry().Equal(future)) + } + }, 10*time.Second, 50*time.Millisecond, "observer holds the expiry") + + nv, ok := h.Server.State().GetNodeByID(id) + require.True(t, ok) + + key := h.Server.CreatePreAuthKey(t, nv.TypedUserID()) + hostinfo := &tailcfg.Hostinfo{BackendLogID: "servertest-" + client.Name, Hostname: client.Name} + require.NoError(t, client.Relogin(t.Context(), key, true, hostinfo)) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + cur, ok := h.Server.State().GetNodeByID(id) + if assert.True(c, ok) { + assert.False(c, cur.Expiry().Valid(), "the server holds no expiry") + } + + p, ok := observer.PeerByName(client.Name) + if assert.True(c, ok) { + assert.True(c, p.KeyExpiry().IsZero(), "observer holds %v", p.KeyExpiry()) + } + }, 10*time.Second, 50*time.Millisecond, "observer drops the expiry") +} + // TestReloginFollowsNewHostname: a node's name follows a new hostname at a login // as at a map request, and later hostname changes still rename it. An admin-set // name stays. diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index dd10878c2..21850e778 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -3957,7 +3957,9 @@ func reauthChange(node types.NodeView, r relogin, policyChanged bool) change.Cha // reloginChanged compares what peers read before and after a relogin. // [tailcfg.PeerChange] carries neither [tailcfg.Node.Expired] nor Hostinfo: // peers left holding an expired node drop its WireGuard handshakes, and the -// relogin stored the Hostinfo, so no later MapRequest shows the change. +// relogin stored the Hostinfo, so no later MapRequest shows the change. It +// cannot remove a key expiry either, as it omits a zero one, and peers would +// cut the node off when the old one passes. func reloginChanged(before, after types.NodeView, now time.Time) relogin { expiry := func(n types.NodeView) time.Time { e, _ := n.Expiry().GetOk() @@ -3965,13 +3967,20 @@ func reloginChanged(before, after types.NodeView, now time.Time) relogin { return e } + // What a peer patch cannot carry. + expiredChanged := before.IsExpiredAt(now) != after.IsExpiredAt(now) + hostinfoChanged := !peerHostinfoEqual(before.Hostinfo(), after.Hostinfo()) + expiryRemoved := before.Expiry().Valid() && !after.Expiry().Valid() + + // What a peer patch carries. + keysSame := before.NodeKey() == after.NodeKey() && before.DiscoKey() == after.DiscoKey() + expirySame := expiry(before).Equal(expiry(after)) + endpointsSame := views.SliceEqual(before.Endpoints(), after.Endpoints()) + switch { - case before.IsExpiredAt(now) != after.IsExpiredAt(now) || - !peerHostinfoEqual(before.Hostinfo(), after.Hostinfo()): + case expiredChanged || hostinfoChanged || expiryRemoved: return reloginWhole - case before.NodeKey() == after.NodeKey() && before.DiscoKey() == after.DiscoKey() && - expiry(before).Equal(expiry(after)) && - views.SliceEqual(before.Endpoints(), after.Endpoints()): + case keysSame && expirySame && endpointsSame: return reloginNone default: return reloginKeys diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 11c22d580..b9dd8224a 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -625,9 +625,9 @@ func NodeKeyRotated(node types.NodeView) Change { nk := node.NodeKey() dk := node.DiscoKey() - // 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]. It is sent in UTC, as + // KeyExpiry carries the new expiry. The zero value does not reach the wire + // ([tailcfg.PeerChange] omits it), so a removed expiry needs the whole node. + // It 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