diff --git a/hscontrol/db/users.go b/hscontrol/db/users.go index 36ea50e5..b9e6bb3f 100644 --- a/hscontrol/db/users.go +++ b/hscontrol/db/users.go @@ -48,7 +48,8 @@ func (hsdb *HSDatabase) DestroyUser(uid types.UserID) error { } // DestroyUser destroys a User. Returns error if the User does -// not exist or if there are nodes associated with it. +// not exist or if there are user-owned nodes associated with it. +// Tagged nodes have user_id = NULL so they do not block deletion. func DestroyUser(tx *gorm.DB, uid types.UserID) error { user, err := GetUserByID(tx, uid) if err != nil { diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 1c921d6d..460808c1 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -425,7 +425,12 @@ func snapshotFromNodes(nodes map[types.NodeID]types.Node, peersFunc PeersFunc) S nodeView := n.View() userID := n.TypedUserID() - newSnap.nodesByUser[userID] = append(newSnap.nodesByUser[userID], nodeView) + // Tagged nodes are owned by their tags, not a user, + // so they are not indexed by user. + if !n.IsTagged() { + newSnap.nodesByUser[userID] = append(newSnap.nodesByUser[userID], nodeView) + } + newSnap.nodesByNodeKey[n.NodeKey] = nodeView // Build machine key index @@ -531,23 +536,12 @@ func (s *NodeStore) DebugString() string { for userID, nodes := range snapshot.nodesByUser { if len(nodes) > 0 { userName := "unknown" - taggedCount := 0 - if len(nodes) > 0 && nodes[0].Valid() { + if nodes[0].Valid() && nodes[0].User().Valid() { userName = nodes[0].User().Name() - // Count tagged nodes (which have UserID set but are owned by "tagged-devices") - for _, n := range nodes { - if n.IsTagged() { - taggedCount++ - } - } } - if taggedCount > 0 { - sb.WriteString(fmt.Sprintf(" - User %d (%s): %d nodes (%d tagged)\n", userID, userName, len(nodes), taggedCount)) - } else { - sb.WriteString(fmt.Sprintf(" - User %d (%s): %d nodes\n", userID, userName, len(nodes))) - } + sb.WriteString(fmt.Sprintf(" - User %d (%s): %d nodes\n", userID, userName, len(nodes))) } } diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 35544aa3..44d68526 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -472,10 +472,9 @@ func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { // Connect marks a node as connected and updates its primary routes in the state. func (s *State) Connect(id types.NodeID) []change.Change { - // CRITICAL FIX: Update the online status in NodeStore BEFORE creating change notification - // This ensures that when the NodeCameOnline change is distributed and processed by other nodes, - // the NodeStore already reflects the correct online status for full map generation. - // now := time.Now() + // Update online status in NodeStore before creating change notification + // so the NodeStore already reflects the correct state when other nodes + // process the NodeCameOnline change for full map generation. node, ok := s.nodeStore.UpdateNode(id, func(n *types.Node) { n.IsOnline = new(true) // n.LastSeen = ptr.To(now) @@ -488,9 +487,8 @@ func (s *State) Connect(id types.NodeID) []change.Change { log.Info().EmbedObject(node).Msg("node connected") - // Use the node's current routes for primary route update - // AllApprovedRoutes() returns only the intersection of announced AND approved routes - // We MUST use AllApprovedRoutes() to maintain the security model + // Use the node's current routes for primary route update. + // AllApprovedRoutes() returns only the intersection of announced and approved routes. routeChange := s.primaryRoutes.SetRoutes(id, node.AllApprovedRoutes()...) if routeChange { @@ -674,9 +672,8 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.Nod // SetNodeTags assigns tags to a node, making it a "tagged node". // Once a node is tagged, it cannot be un-tagged (only tags can be changed). -// The UserID is preserved as "created by" information. +// Setting tags clears UserID since tagged nodes are owned by their tags. func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, change.Change, error) { - // CANNOT REMOVE ALL TAGS if len(tags) == 0 { return types.NodeView{}, change.Change{}, types.ErrCannotRemoveAllTags } @@ -716,7 +713,9 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, // make the exact same change. n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) { node.Tags = validatedTags - // UserID is preserved as "created by" - do NOT set to nil + // Tagged nodes are owned by their tags, not a user. + node.UserID = nil + node.User = nil }) if !ok { @@ -1307,17 +1306,10 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro // Assign ownership based on PreAuthKey if params.PreAuthKey != nil { if params.PreAuthKey.IsTagged() { - // TAGGED NODE - // Tags from PreAuthKey are assigned ONLY during initial authentication + // Tagged nodes are owned by their tags, not a user. + // UserID is intentionally left nil. nodeToRegister.Tags = params.PreAuthKey.Proto().GetAclTags() - // Set UserID to track "created by" (who created the PreAuthKey) - if params.PreAuthKey.UserID != nil { - nodeToRegister.UserID = params.PreAuthKey.UserID - nodeToRegister.User = params.PreAuthKey.User - } - // If PreAuthKey.UserID is nil, the node is "orphaned" (system-created) - // Tagged nodes have key expiry disabled. nodeToRegister.Expiry = nil } else { @@ -1358,6 +1350,11 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro slices.Sort(nodeToRegister.Tags) nodeToRegister.Tags = slices.Compact(nodeToRegister.Tags) + // Node is now tagged, so clear user ownership. + // Tagged nodes are owned by their tags, not a user. + nodeToRegister.UserID = nil + nodeToRegister.User = nil + // Tagged nodes have key expiry disabled. nodeToRegister.Expiry = nil @@ -1504,7 +1501,10 @@ func (s *State) processReauthTags( wasTagged := node.IsTagged() node.Tags = approvedTags - // Note: UserID is preserved as "created by" tracking, consistent with SetNodeTags + // Tagged nodes are owned by their tags, not a user. + node.UserID = nil + node.User = nil + if !wasTagged { log.Info(). Uint64(zf.NodeID, uint64(node.ID)). @@ -1729,9 +1729,15 @@ func (s *State) HandleNodeFromPreAuthKey( existingNodeSameUser, existsSameUser = s.nodeStore.GetNodeByMachineKey(machineKey, types.UserID(pak.User.ID)) } + // Tagged nodes have nil UserID, so they are indexed under UserID(0) + // in nodesByMachineKey. Check there too for tagged PAK re-registration. + if !existsSameUser && pak.IsTagged() { + existingNodeSameUser, existsSameUser = s.nodeStore.GetNodeByMachineKey(machineKey, 0) + } + // For existing nodes, skip validation if: // 1. MachineKey matches (cryptographic proof of machine identity) - // 2. User matches (from the PAK being used) + // 2. User/tag ownership matches (from the PAK being used) // 3. Not a NodeKey rotation (rotation requires fresh validation) // // Security: MachineKey is the cryptographic identity. If someone has the MachineKey, @@ -1739,9 +1745,7 @@ func (s *State) HandleNodeFromPreAuthKey( // We don't check which specific PAK was used originally because: // - Container restarts may use different PAKs (e.g., env var changed) // - Original PAK may be deleted - // - MachineKey + User is sufficient to prove this is the same node - // - // Note: For tags-only keys, existsSameUser is always false, so we always validate. + // - MachineKey + ownership is sufficient to prove this is the same node isExistingNodeReregistering := existsSameUser && existingNodeSameUser.Valid() // Check if this is a NodeKey rotation (different NodeKey) @@ -1828,11 +1832,9 @@ func (s *State) HandleNodeFromPreAuthKey( node.RegisterMethod = util.RegisterMethodAuthKey - // CRITICAL: Tags from PreAuthKey are ONLY applied during initial authentication - // On re-registration, we MUST NOT change tags or node ownership - // The node keeps whatever tags/user ownership it already has - // - // Only update AuthKey reference + // Tags from PreAuthKey are only applied during initial registration. + // On re-registration the node keeps its existing tags and ownership. + // Only update AuthKey reference. node.AuthKey = pak node.AuthKeyID = &pak.ID node.IsOnline = new(false) @@ -1885,19 +1887,27 @@ func (s *State) HandleNodeFromPreAuthKey( // Check if node exists with this machine key for a different user existingNodeAnyUser, existsAnyUser := s.nodeStore.GetNodeByMachineKeyAnyUser(machineKey) - // For user-owned keys, check if node exists for a different user - // For tags-only keys (pak.User == nil), this check is skipped - if pak.User != nil && existsAnyUser && existingNodeAnyUser.Valid() && existingNodeAnyUser.UserID().Get() != pak.User.ID { - // Node exists but belongs to a different user - // Create a NEW node for the new user (do not transfer) - // This allows the same machine to have separate node identities per user - oldUser := existingNodeAnyUser.User() + // For user-owned keys, check if node exists for a different user. + // Tags-only keys (pak.User == nil) skip this check. + // Tagged nodes are also skipped since they have no owning user. + existingIsUserOwned := existsAnyUser && + existingNodeAnyUser.Valid() && + !existingNodeAnyUser.IsTagged() + belongsToDifferentUser := pak.User != nil && + existingIsUserOwned && + existingNodeAnyUser.UserID().Get() != pak.User.ID + + if belongsToDifferentUser { + // Node exists but belongs to a different user. + // Create a new node for the new user (do not transfer). + oldUserName := existingNodeAnyUser.User().Name() + log.Info(). Caller(). Str(zf.ExistingNodeName, existingNodeAnyUser.Hostname()). Uint64(zf.ExistingNodeID, existingNodeAnyUser.ID().Uint64()). Str(zf.MachineKey, machineKey.ShortString()). - Str(zf.OldUser, oldUser.Name()). + Str(zf.OldUser, oldUserName). Str(zf.NewUser, pakUsername()). Msg("Creating new node for different user (same machine key exists for another user)") } diff --git a/hscontrol/state/tags.go b/hscontrol/state/tags.go index 56dff16e..fbc05d9b 100644 --- a/hscontrol/state/tags.go +++ b/hscontrol/state/tags.go @@ -20,22 +20,26 @@ var ( ErrRequestedTagsInvalidOrNotPermitted = errors.New("requested tags") ) -// validateNodeOwnership ensures proper node ownership model. -// A node must be EITHER user-owned OR tagged (mutually exclusive by behavior). -// Tagged nodes CAN have a UserID for "created by" tracking, but the tag is the owner. -func validateNodeOwnership(node *types.Node) error { - isTagged := node.IsTagged() +// ErrTaggedNodeHasUser is returned when a tagged node has a UserID set. +var ErrTaggedNodeHasUser = errors.New("tagged node must not have user_id set") - // Tagged nodes: Must have tags, UserID is optional (just "created by") - if isTagged { +// validateNodeOwnership ensures proper node ownership model. +// A node must be either user-owned or tagged, and these are mutually exclusive: +// tagged nodes must not have a UserID, and user-owned nodes must not have tags. +func validateNodeOwnership(node *types.Node) error { + if node.IsTagged() { if len(node.Tags) == 0 { return fmt.Errorf("%w: %q", ErrNodeMarkedTaggedButHasNoTags, node.Hostname) } - // UserID can be set (created by) or nil (orphaned), both valid for tagged nodes + + if node.UserID != nil { + return fmt.Errorf("%w: %q", ErrTaggedNodeHasUser, node.Hostname) + } + return nil } - // User-owned nodes: Must have UserID, must NOT have tags + // User-owned nodes must have a UserID. if node.UserID == nil { return fmt.Errorf("%w: %q", ErrNodeHasNeitherUserNorTags, node.Hostname) } @@ -59,7 +63,7 @@ func logTagOperation(existingNode types.NodeView, newTags []string) { log.Info(). EmbedObject(existingNode). - Uint("created.by.user", userID). + Uint("previous.user", userID). Strs("new.tags", newTags). Msg("Converting user-owned node to tagged node") } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index e705a33a..05b27c19 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -105,10 +105,8 @@ type Node struct { // parts of headscale. GivenName string `gorm:"type:varchar(63);unique_index"` - // UserID is set for ALL nodes (tagged and user-owned) to track "created by". - // For tagged nodes, this is informational only - the tag is the owner. - // For user-owned nodes, this identifies the owner. - // Only nil for orphaned nodes (should not happen in normal operation). + // UserID identifies the owning user for user-owned nodes. + // Nil for tagged nodes, which are owned by their tags. UserID *uint User *User `gorm:"constraint:OnDelete:CASCADE;"`