diff --git a/hscontrol/auth_tags_test.go b/hscontrol/auth_tags_test.go index bbaa834b..e7b74b75 100644 --- a/hscontrol/auth_tags_test.go +++ b/hscontrol/auth_tags_test.go @@ -625,6 +625,152 @@ func TestTaggedNodeReauthPreservesDisabledExpiry(t *testing.T) { "Tagged node should have expiry PRESERVED as disabled after re-auth") } +// TestExpiryDuringPersonalToTaggedConversion tests that when a personal node +// is converted to tagged via reauth with RequestTags, the expiry is cleared to nil. +// BUG #3048: Previously expiry was NOT cleared because expiry handling ran +// BEFORE processReauthTags. +func TestExpiryDuringPersonalToTaggedConversion(t *testing.T) { + app := createTestApp(t) + user := app.state.CreateUserForTest("expiry-test-user") + + // Update policy to allow user to own tags + err := app.state.UpdatePolicyManagerUsersForTest() + require.NoError(t, err) + + policy := `{ + "tagOwners": { + "tag:server": ["expiry-test-user@"] + }, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }` + _, err = app.state.SetPolicy([]byte(policy)) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey1 := key.NewNode() + + // Step 1: Create user-owned node WITH expiry set + clientExpiry := time.Now().Add(24 * time.Hour) + registrationID1 := types.MustRegistrationID() + regEntry1 := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), + NodeKey: nodeKey1.Public(), + Hostname: "personal-to-tagged", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "personal-to-tagged", + RequestTags: []string{}, // No tags - user-owned + }, + Expiry: &clientExpiry, + }) + app.state.SetRegistrationCacheEntry(registrationID1, regEntry1) + + node, _, err := app.state.HandleNodeFromAuthPath( + registrationID1, types.UserID(user.ID), nil, "webauth", + ) + require.NoError(t, err) + require.False(t, node.IsTagged(), "Node should be user-owned initially") + require.True(t, node.Expiry().Valid(), "User-owned node should have expiry set") + + // Step 2: Re-auth with tags (Personal → Tagged conversion) + nodeKey2 := key.NewNode() + registrationID2 := types.MustRegistrationID() + regEntry2 := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), + NodeKey: nodeKey2.Public(), + Hostname: "personal-to-tagged", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "personal-to-tagged", + RequestTags: []string{"tag:server"}, // Adding tags + }, + Expiry: &clientExpiry, // Client still sends expiry + }) + app.state.SetRegistrationCacheEntry(registrationID2, regEntry2) + + nodeAfter, _, err := app.state.HandleNodeFromAuthPath( + registrationID2, types.UserID(user.ID), nil, "webauth", + ) + require.NoError(t, err) + require.True(t, nodeAfter.IsTagged(), "Node should be tagged after conversion") + + // CRITICAL ASSERTION: Tagged nodes should NOT have expiry + assert.False(t, nodeAfter.Expiry().Valid(), + "Tagged node should have expiry cleared to nil") +} + +// TestExpiryDuringTaggedToPersonalConversion tests that when a tagged node +// is converted to personal via reauth with empty RequestTags, expiry is set +// from the client request. +// BUG #3048: Previously expiry was NOT set because expiry handling ran +// BEFORE processReauthTags (node was still tagged at check time). +func TestExpiryDuringTaggedToPersonalConversion(t *testing.T) { + app := createTestApp(t) + user := app.state.CreateUserForTest("expiry-test-user2") + + // Update policy to allow user to own tags + err := app.state.UpdatePolicyManagerUsersForTest() + require.NoError(t, err) + + policy := `{ + "tagOwners": { + "tag:server": ["expiry-test-user2@"] + }, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }` + _, err = app.state.SetPolicy([]byte(policy)) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey1 := key.NewNode() + + // Step 1: Create tagged node (expiry should be nil) + registrationID1 := types.MustRegistrationID() + regEntry1 := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), + NodeKey: nodeKey1.Public(), + Hostname: "tagged-to-personal", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-to-personal", + RequestTags: []string{"tag:server"}, // Tagged node + }, + }) + app.state.SetRegistrationCacheEntry(registrationID1, regEntry1) + + node, _, err := app.state.HandleNodeFromAuthPath( + registrationID1, types.UserID(user.ID), nil, "webauth", + ) + require.NoError(t, err) + require.True(t, node.IsTagged(), "Node should be tagged initially") + require.False(t, node.Expiry().Valid(), "Tagged node should have nil expiry") + + // Step 2: Re-auth with empty tags (Tagged → Personal conversion) + nodeKey2 := key.NewNode() + clientExpiry := time.Now().Add(48 * time.Hour) + registrationID2 := types.MustRegistrationID() + regEntry2 := types.NewRegisterNode(types.Node{ + MachineKey: machineKey.Public(), + NodeKey: nodeKey2.Public(), + Hostname: "tagged-to-personal", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-to-personal", + RequestTags: []string{}, // Empty tags - convert to user-owned + }, + Expiry: &clientExpiry, // Client requests expiry + }) + app.state.SetRegistrationCacheEntry(registrationID2, regEntry2) + + nodeAfter, _, err := app.state.HandleNodeFromAuthPath( + registrationID2, types.UserID(user.ID), nil, "webauth", + ) + require.NoError(t, err) + require.False(t, nodeAfter.IsTagged(), "Node should be user-owned after conversion") + + // CRITICAL ASSERTION: User-owned nodes should have expiry from client + assert.True(t, nodeAfter.Expiry().Valid(), + "User-owned node should have expiry set") + assert.WithinDuration(t, clientExpiry, nodeAfter.Expiry().Get(), 5*time.Second, + "Expiry should match client request") +} + // TestReAuthWithDifferentMachineKey tests the edge case where a node attempts // to re-authenticate with the same NodeKey but a DIFFERENT MachineKey. // This scenario should be handled gracefully (currently creates a new node). diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 634467a4..a9d82e62 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -136,6 +136,7 @@ func NewState(cfg *types.Config) (*State, error) { for _, node := range nodes { node.IsOnline = ptr.To(false) } + users, err := db.ListUsers() if err != nil { return nil, fmt.Errorf("loading users: %w", err) @@ -157,6 +158,7 @@ func NewState(cfg *types.Config) (*State, error) { if batchSize == 0 { batchSize = defaultNodeStoreBatchSize } + batchTimeout := cfg.Tuning.NodeStoreBatchTimeout if batchTimeout == 0 { batchTimeout = defaultNodeStoreBatchTimeout @@ -190,7 +192,8 @@ func NewState(cfg *types.Config) (*State, error) { func (s *State) Close() error { s.nodeStore.Stop() - if err := s.db.Close(); err != nil { + err := s.db.Close() + if err != nil { return fmt.Errorf("closing database: %w", err) } @@ -573,12 +576,14 @@ func (s *State) ListNodes(nodeIDs ...types.NodeID) views.Slice[types.NodeView] { // Filter nodes by the requested IDs allNodes := s.nodeStore.ListNodes() + nodeIDSet := make(map[types.NodeID]struct{}, len(nodeIDs)) for _, id := range nodeIDs { nodeIDSet[id] = struct{}{} } var filteredNodes []types.NodeView + for _, node := range allNodes.All() { if _, exists := nodeIDSet[node.ID()]; exists { filteredNodes = append(filteredNodes, node) @@ -601,12 +606,14 @@ func (s *State) ListPeers(nodeID types.NodeID, peerIDs ...types.NodeID) views.Sl // For specific peerIDs, filter from all nodes allNodes := s.nodeStore.ListNodes() + nodeIDSet := make(map[types.NodeID]struct{}, len(peerIDs)) for _, id := range peerIDs { nodeIDSet[id] = struct{}{} } var filteredNodes []types.NodeView + for _, node := range allNodes.All() { if _, exists := nodeIDSet[node.ID()]; exists { filteredNodes = append(filteredNodes, node) @@ -619,6 +626,7 @@ func (s *State) ListPeers(nodeID types.NodeID, peerIDs ...types.NodeID) views.Sl // ListEphemeralNodes retrieves all ephemeral (temporary) nodes in the system. func (s *State) ListEphemeralNodes() views.Slice[types.NodeView] { allNodes := s.nodeStore.ListNodes() + var ephemeralNodes []types.NodeView for _, node := range allNodes.All() { @@ -750,7 +758,8 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t // RenameNode changes the display name of a node. func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, change.Change, error) { - if err := util.ValidateHostname(newName); err != nil { + err := util.ValidateHostname(newName) + if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("renaming node: %w", err) } @@ -1080,6 +1089,7 @@ func preserveNetInfo(existingNode types.NodeView, nodeID types.NodeID, validHost if existingNode.Valid() { existingHostinfo = existingNode.Hostinfo().AsStruct() } + return netInfoFromMapRequest(nodeID, existingHostinfo, validHostinfo) } @@ -1164,19 +1174,43 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView node.RegisterMethod = params.RegEntry.Node.RegisterMethod } - // Expiry handling differs based on node type: - // - Tagged nodes keep their existing expiry (disabled) - // - User-owned nodes update expiry from the provided value or registration entry - // - Converting from tagged to user-owned: always set expiry - if params.IsConvertFromTag || !node.IsTagged() { + // Track tagged status BEFORE processing tags + wasTagged := node.IsTagged() + + // Process tags - may change node.Tags and node.UserID + rejectedTags = s.processReauthTags(node, requestTags, params.User, oldTags) + + // Handle expiry AFTER tag processing, based on transition + // This ensures expiry is correctly set/cleared based on the NEW tagged status + isTagged := node.IsTagged() + + switch { + case wasTagged && !isTagged: + // Tagged → Personal: set expiry from client request + if params.Expiry != nil { + node.Expiry = params.Expiry + } else { + node.Expiry = params.RegEntry.Node.Expiry + } + case !wasTagged && isTagged: + // Personal → Tagged: clear expiry (tagged nodes don't expire) + node.Expiry = nil + case params.IsConvertFromTag: + // Explicit conversion path (convertTaggedNodeToUser) + if params.Expiry != nil { + node.Expiry = params.Expiry + } else { + node.Expiry = params.RegEntry.Node.Expiry + } + case !isTagged: + // Personal → Personal: update expiry from client if params.Expiry != nil { node.Expiry = params.Expiry } else { node.Expiry = params.RegEntry.Node.Expiry } } - - rejectedTags = s.processReauthTags(node, requestTags, params.User, oldTags) + // Tagged → Tagged: keep existing expiry (nil) - no action needed }) if !ok { @@ -1304,6 +1338,7 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro nodeToRegister.User = params.PreAuthKey.User nodeToRegister.Tags = nil } + nodeToRegister.AuthKey = params.PreAuthKey nodeToRegister.AuthKeyID = ¶ms.PreAuthKey.ID } else { @@ -1373,12 +1408,14 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro if err != nil { return types.NodeView{}, fmt.Errorf("failed to ensure unique given name: %w", err) } + nodeToRegister.GivenName = givenName } // New node - database first to get ID, then NodeStore savedNode, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { - if err := tx.Save(&nodeToRegister).Error; err != nil { + err := tx.Save(&nodeToRegister).Error + if err != nil { return nil, fmt.Errorf("failed to save node: %w", err) } @@ -1874,6 +1911,7 @@ func (s *State) HandleNodeFromPreAuthKey( } var err error + finalNode, err = s.createAndSaveNewNode(newNodeParams{ User: pakUser, MachineKey: machineKey, @@ -2011,7 +2049,9 @@ func (s *State) autoApproveNodes() ([]change.Change, error) { } mu.Lock() + cs = append(cs, c) + mu.Unlock() } @@ -2081,6 +2121,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest if hi := req.Hostinfo; hi != nil { hasNewRoutes = len(hi.RoutableIPs) > 0 } + needsRouteApproval = hostinfoChanged && (routesChanged(currentNode.View(), req.Hostinfo) || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0)) if needsRouteApproval { // Extract announced routes from request @@ -2233,6 +2274,7 @@ func hostinfoEqual(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool { if !oldNode.Valid() || newHI == nil { return false } + old := oldNode.AsStruct().Hostinfo return old.Equal(newHI)