hscontrol: enforce that tagged nodes never have user_id

Tagged nodes are owned by their tags, not a user. Enforce this
invariant at every write path:

- createAndSaveNewNode: do not set UserID for tagged PreAuthKey
  registration; clear UserID when advertise-tags are applied
  during OIDC/CLI registration
- SetNodeTags: clear UserID/User when tags are assigned
- processReauthTags: clear UserID/User when tags are applied
  during re-authentication
- validateNodeOwnership: reject tagged nodes with non-nil UserID
- NodeStore: skip nodesByUser indexing for tagged nodes since
  they have no owning user
- HandleNodeFromPreAuthKey: add fallback lookup for tagged PAK
  re-registration (tagged nodes indexed under UserID(0)); guard
  against nil User deref for tagged nodes in different-user check

Since tagged nodes now have user_id = NULL, ListNodesByUser
will not return them and DestroyUser naturally allows deleting
users whose nodes have all been tagged. The ON DELETE CASCADE
FK cannot reach tagged nodes through a NULL foreign key.

Also tone down shouty comments throughout state.go.

Fixes #3077
This commit is contained in:
Kristoffer Dalby
2026-02-20 09:27:11 +00:00
parent 52d454d0c8
commit 75e56df9e4
5 changed files with 73 additions and 66 deletions

View File

@@ -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)")
}