mirror of
https://github.com/juanfont/headscale.git
synced 2026-02-07 20:04:00 +01:00
state: fix expiry handling during node tag conversion
Previously, expiry handling ran BEFORE processReauthTags(), using the old tagged status to determine whether to set/clear expiry. This caused: - Personal → Tagged: Expiry remained set (should be cleared to nil) - Tagged → Personal: Expiry remained nil (should be set from client) Move expiry handling after tag processing and handle all four transition cases based on the new tagged status: - Tagged → Personal: Set expiry from client request - Personal → Tagged: Clear expiry (tagged nodes don't expire) - Personal → Personal: Update expiry from client - Tagged → Tagged: Keep existing nil expiry Fixes #3048
This commit is contained in:
Kristoffer Dalby
parent
0694caf4d2
commit
7b6990f63e
@ -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).
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user