From 0e10ca4e9a27d0eb379207c2939e76dc789484ce Mon Sep 17 00:00:00 2001 From: Akhilesh Arora Date: Thu, 16 Apr 2026 19:46:22 +0200 Subject: [PATCH] state: preserve nil expiry on user owned registration when no default is configured When a user owned node registers or re registers with a PreAuthKey and the client sends zero client expiry while node.expiry is set to 0, the expiry column ends up stored as 0001-01-01 00:00:00 instead of NULL. Two sites in HandleNodeFromPreAuthKey build a non nil pointer to regReq.Expiry even when the value is zero time, and the needsDefaultExpiry guard only replaces it when s.cfg.Node.Expiry > 0, so the pointer to zero time survives to the database. Convert an unset regReq.Expiry to nil before handing it off so the needsDefaultExpiry path is the only place that assigns a non nil pointer. This is a narrower sibling of #3170 on the user owned PreAuthKey path. The regression was introduced alongside the fix for #3111 in 6337a3db. --- hscontrol/auth_tags_test.go | 64 +++++++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 17 ++++++++-- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/hscontrol/auth_tags_test.go b/hscontrol/auth_tags_test.go index b5fd448c..10d1eeb4 100644 --- a/hscontrol/auth_tags_test.go +++ b/hscontrol/auth_tags_test.go @@ -1119,3 +1119,67 @@ func TestReregistrationAppliesDefaultExpiry(t *testing.T) { assert.True(t, node2.Expiry().Get().After(firstExpiry), "re-registration expiry should be later than initial registration expiry") } + +// TestReregistrationZeroExpiryStaysNil tests that when a user-owned node +// re-registers with zero client expiry and node.expiry is disabled (0), +// the node's expiry stays nil rather than being set to a pointer to zero +// time. Regression test for the else branch introduced in commit 6337a3db +// which assigned `®Req.Expiry` (pointer to time.Time{}) instead of nil, +// causing the database row to hold `0001-01-01 00:00:00` instead of NULL. +func TestReregistrationZeroExpiryStaysNil(t *testing.T) { + t.Parallel() + + // node.expiry = 0 means "no default expiry" + app := createTestAppWithNodeExpiry(t, 0) + + user := app.state.CreateUserForTest("node-owner") + + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + + // Initial registration with zero client expiry + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: pak.Key, + }, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "reregister-zero-expiry", + }, + Expiry: time.Time{}, + } + + resp, err := app.handleRegisterWithAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.True(t, resp.MachineAuthorized) + + node, found := app.state.GetNodeByNodeKey(nodeKey.Public()) + require.True(t, found) + assert.False(t, node.Expiry().Valid(), + "initial registration with zero expiry and no default should leave expiry nil") + + // Re-register with a new node key but same machine key + user + nodeKey2 := key.NewNode() + regReq2 := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: pak.Key, + }, + NodeKey: nodeKey2.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "reregister-zero-expiry", + }, + Expiry: time.Time{}, // still zero + } + + resp2, err := app.handleRegisterWithAuthKey(regReq2, machineKey.Public()) + require.NoError(t, err) + require.True(t, resp2.MachineAuthorized) + + node2, found := app.state.GetNodeByNodeKey(nodeKey2.Public()) + require.True(t, found) + assert.False(t, node2.Expiry().Valid(), + "re-registration with zero client expiry and no default should leave expiry nil, not pointer to zero time") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index b7bc7432..6fceac05 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2176,7 +2176,9 @@ func (s *State) HandleNodeFromPreAuthKey( // Tagged nodes keep their existing expiry (disabled). // User-owned nodes update expiry from the client request, // falling back to the configured default if the client - // did not request a specific expiry. + // did not request a specific expiry. If neither is set, + // clear the expiry so the database holds NULL instead of + // a pointer to zero time. if !node.IsTagged() { if !regReq.Expiry.IsZero() { node.Expiry = ®Req.Expiry @@ -2184,7 +2186,7 @@ func (s *State) HandleNodeFromPreAuthKey( exp := time.Now().Add(s.cfg.Node.Expiry) node.Expiry = &exp } else { - node.Expiry = ®Req.Expiry + node.Expiry = nil } } }) @@ -2271,6 +2273,15 @@ func (s *State) HandleNodeFromPreAuthKey( pakUser = *pak.User } + // Only pass the client-requested expiry when it is actually set. + // A pointer to a zero time.Time gets persisted as "0001-01-01 00:00:00" + // rather than NULL, which breaks downstream consumers that distinguish + // "no expiry" from "expires at year 1". + var reqExpiry *time.Time + if !regReq.Expiry.IsZero() { + reqExpiry = ®Req.Expiry + } + var err error finalNode, err = s.createAndSaveNewNode(newNodeParams{ @@ -2281,7 +2292,7 @@ func (s *State) HandleNodeFromPreAuthKey( Hostname: hostname, Hostinfo: validHostinfo, Endpoints: nil, // Endpoints not available in RegisterRequest - Expiry: ®Req.Expiry, + Expiry: reqExpiry, RegisterMethod: util.RegisterMethodAuthKey, PreAuthKey: pak, ExistingNodeForNetinfo: cmp.Or(existingNodeAnyUser, types.NodeView{}),