diff --git a/hscontrol/api/v1/auth.go b/hscontrol/api/v1/auth.go index ca63c3cca..dd54d8273 100644 --- a/hscontrol/api/v1/auth.go +++ b/hscontrol/api/v1/auth.go @@ -90,19 +90,12 @@ func registerAuth(api huma.API, b Backend) { nil, util.RegisterMethodCLI, ) - if err != nil { - b.Change(nodeChange) + b.Change(nodeChange) + if err != nil { return nil, mapError("registering node", err) } - routeChange, err := b.State.AutoApproveRoutes(node) - b.Change(nodeChange, routeChange) - - if err != nil { - return nil, huma.Error500InternalServerError("auto approving routes", err) - } - out := &authRegisterOutput{} out.Body.Node = nodeFromView(node) diff --git a/hscontrol/api/v1/nodes.go b/hscontrol/api/v1/nodes.go index 7b4521550..22e7f38c1 100644 --- a/hscontrol/api/v1/nodes.go +++ b/hscontrol/api/v1/nodes.go @@ -487,19 +487,11 @@ func registerNodeAdminOps(api huma.API, b Backend) { nil, util.RegisterMethodCLI, ) - if err != nil { - b.Change(nodeChange) - - return nil, mapError("registering node", err) - } - - routeChange, err := b.State.AutoApproveRoutes(node) - // Empty changes are ignored by the change sink. - b.Change(nodeChange, routeChange) + b.Change(nodeChange) if err != nil { - return nil, huma.Error500InternalServerError("auto approving routes", err) + return nil, mapError("registering node", err) } out := &nodeOutput{} diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 4a7d47904..d7b902195 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -492,27 +492,7 @@ func (h *Headscale) handleRegisterWithAuthKey( return nil, nil //nolint:nilnil // intentional: no node to return when ephemeral deleted } - // This is a bit of a back and forth, but we have a bit of a chicken and egg - // dependency here. - // Because the way the policy manager works, we need to have the node - // in the database, then add it to the policy manager and then we can - // approve the route. This means we get this dance where the node is - // first added to the database, then we add it to the policy manager via - // nodesChangedHook and then we can auto approve the routes. - // As that only approves the struct object, we need to save it again and - // ensure we send an update. - // This works, but might be another good candidate for doing some sort of - // eventbus. - // TODO(kradalby): This needs to be ran as part of the batcher maybe? - // now since we dont update the node/pol here anymore - routesChange, err := h.state.AutoApproveRoutes(node) - - // Send both changes. Empty changes are ignored by Change(). - h.Change(changed, routesChange) - - if err != nil { - return nil, fmt.Errorf("auto approving routes: %w", err) - } + h.Change(changed) resp := &tailcfg.RegisterResponse{ MachineAuthorized: true, diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index fa763b448..a1d0e3ee9 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -1059,15 +1059,15 @@ func (a *AuthProviderOIDC) handleRegistration( registrationID types.AuthID, expiry *time.Time, ) (bool, error) { - node, nodeChange, err := a.h.state.HandleNodeFromAuthPath( + _, nodeChange, err := a.h.state.HandleNodeFromAuthPath( registrationID, types.UserID(user.ID), expiry, util.RegisterMethodOIDC, ) - if err != nil { - a.h.Change(nodeChange) + a.h.Change(nodeChange) + if err != nil { if errors.Is(err, types.ErrHostinfoTooLarge) { return false, newHTTPUserError(http.StatusBadRequest, "node report too large", "This device reports too much data. Ask your administrator to reduce its reported metadata, then sign in again.", err) @@ -1076,26 +1076,6 @@ func (a *AuthProviderOIDC) handleRegistration( return false, fmt.Errorf("registering node: %w", err) } - // This is a bit of a back and forth, but we have a bit of a chicken and egg - // dependency here. - // Because the way the policy manager works, we need to have the node - // in the database, then add it to the policy manager and then we can - // approve the route. This means we get this dance where the node is - // first added to the database, then we add it to the policy manager via - // SaveNode (which automatically updates the policy manager) and then we can auto approve the routes. - // As that only approves the struct object, we need to save it again and - // ensure we send an update. - // This works, but might be another good candidate for doing some sort of - // eventbus. - routesChange, err := a.h.state.AutoApproveRoutes(node) - - // Send both changes. Empty changes are ignored by Change(). - a.h.Change(nodeChange, routesChange) - - if err != nil { - return false, fmt.Errorf("auto approving routes: %w", err) - } - return !nodeChange.IsEmpty(), nil } diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index 321b643f8..a8d430f8f 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -36,10 +36,6 @@ import ( // the step of what killed nodes leave behind. A listed check that matches // fails, so a fix must delete its entry. var lifecycleSkipReasons = map[string]string{ - "route-approval/b9/enabled": approvalOnSameKeyReauth, - "route-approval/b9/served": approvalOnSameKeyReauth, - "route-approval/b9/self-routes": approvalOnSameKeyReauth, - "ephemeral-lifecycle/E1-logout/eph-logout/outcome": logoutAnswerUnauthorized, "ephemeral-lifecycle/E1-logout/eph-logout-user/outcome": logoutAnswerUnauthorized, @@ -58,12 +54,6 @@ var lifecycleSkipReasons = map[string]string{ "ephemeral-lifecycle/collection/perm-to-eph/present-observer": ephemeralFollowsKey, } -// SaaS runs auto-approval when a router's announced routes change and on a -// re-auth that rotates its node key, over every announced route. -const ( - approvalOnSameKeyReauth = "APPROVAL_ON_SAME_KEY_REAUTH: headscale auto-approves on a re-auth that keeps the node key; SaaS does not" -) - const ( logoutAnswerUnauthorized = "LOGOUT_ANSWER_UNAUTHORIZED: headscale answers the logout that deletes an ephemeral node with MachineAuthorized false; SaaS answers true" deletedKeyListed = "DELETED_KEY_LISTED: headscale lists a deleted auth key; SaaS leaves it out of both key lists, while a read by ID still answers" diff --git a/hscontrol/state/autoapprove_test.go b/hscontrol/state/autoapprove_test.go index f4b637d5a..b0b75f1ce 100644 --- a/hscontrol/state/autoapprove_test.go +++ b/hscontrol/state/autoapprove_test.go @@ -5,9 +5,11 @@ import ( "testing" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" + "tailscale.com/types/key" ) // TestRouteApprovalWaitsForAnnouncement: approvers run when the announced @@ -113,3 +115,102 @@ func TestAutoApproveKeepsCurrentManualRoutes(t *testing.T) { }) } } + +// TestRegistrationApprovesOnlyWithNewNodeKey: a registration runs route +// auto-approval when it brings a new node key or a change to the announced +// routes, not on a login that keeps both. +func TestRegistrationApprovesOnlyWithNewNodeKey(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("approve-user") + + _, err := s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["approve-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + route := netip.MustParsePrefix("10.0.1.0/24") + machineKey := key.NewMachine().Public() + req := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{route}}, + } + + node, _, err := s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + require.Equal(t, []netip.Prefix{route}, approvedRoutes(t, s, node.ID()), "a new node") + + _, _, err = s.SetApprovedRoutes(node.ID(), nil) + require.NoError(t, err) + + _, _, err = s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + assert.Empty(t, approvedRoutes(t, s, node.ID()), "the same node key") + + added := netip.MustParsePrefix("10.0.2.0/24") + req.Hostinfo = &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{route, added}} + _, _, err = s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + assert.Equal(t, []netip.Prefix{route, added}, approvedRoutes(t, s, node.ID()), "a new announced route") + + _, _, err = s.SetApprovedRoutes(node.ID(), nil) + require.NoError(t, err) + + req.NodeKey = key.NewNode().Public() + _, _, err = s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + assert.Equal(t, []netip.Prefix{route, added}, approvedRoutes(t, s, node.ID()), "a new node key") +} + +// TestRegistrationApprovalComparesReplacedNode: registration approval compares +// with the node the writer replaced, so an announcement and an admin revoke +// that land after the lookup leave an unchanged re-registration approving +// nothing. +func TestRegistrationApprovalComparesReplacedNode(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("approve-user") + + _, err := s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["approve-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, pak) + stale, stalePAK := c.lookup(t, pak.Key) + + route := netip.MustParsePrefix("10.0.1.0/24") + hi := &tailcfg.Hostinfo{Hostname: "h0", RoutableIPs: []netip.Prefix{route}} + + _, err = s.UpdateNodeFromMapRequest(c.node.ID(), tailcfg.MapRequest{NodeKey: c.regReq.NodeKey, Hostinfo: hi}) + require.NoError(t, err) + require.Equal(t, []netip.Prefix{route}, approvedRoutes(t, s, c.node.ID()), "the announcement approves") + + _, _, err = s.SetApprovedRoutes(c.node.ID(), nil) + require.NoError(t, err) + + req := c.regReq + req.Hostinfo = hi.Clone() + node, replaced, err := s.reregisterNodeWithPAK(stale, stalePAK, req, c.machineKey, hi.Hostname, req.Hostinfo) + require.NoError(t, err) + + _, _, err = s.approveRoutesAfterRegistration(replaced, node, change.Change{}) + require.NoError(t, err) + assert.Empty(t, approvedRoutes(t, s, c.node.ID()), "the admin revoke stands") +} + +func approvedRoutes(t *testing.T, s *State, id types.NodeID) []netip.Prefix { + t.Helper() + + nv, ok := s.GetNodeByID(id) + require.True(t, ok) + + return nv.ApprovedRoutes().AsSlice() +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index b1afdcfe4..90ed3f890 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2270,10 +2270,9 @@ func sameOwner(a, b types.NodeView) bool { // applyAuthNodeUpdate applies common update logic for re-authenticating or converting // an existing node. It updates the node in [NodeStore], processes RequestTags, and -// persists changes to the database. The bool reports whether peers read the -// node it replaced and the update the same, apart from its keys; see -// [onlyKeysChanged]. -func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, bool, error) { +// persists changes to the database. It also returns the node the NodeStore +// writer replaced. +func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, types.NodeView, error) { regData := params.RegData // Log the operation type if params.IsConvertFromTag { @@ -2314,7 +2313,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView rejectedTags := s.validateRequestTagsForReauth(params.ExistingNode, authUser, requestTags) if len(rejectedTags) > 0 { - return types.NodeView{}, false, fmt.Errorf( + return types.NodeView{}, types.NodeView{}, fmt.Errorf( "%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, rejectedTags, @@ -2329,7 +2328,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView // the NodeStore NodeKey index (denying the victim service). if existing, ok := s.nodeStore.GetNodeByNodeKey(regData.NodeKey); ok && existing.MachineKey() != regData.MachineKey { - return types.NodeView{}, false, ErrNodeKeyInUse + return types.NodeView{}, types.NodeView{}, ErrNodeKeyInUse } var hostinfoErr error @@ -2411,15 +2410,13 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView }) if !ok { - return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID()) + return types.NodeView{}, types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID()) } if hostinfoErr != nil { - return types.NodeView{}, false, hostinfoErr + return types.NodeView{}, types.NodeView{}, hostinfoErr } - keysOnly := onlyKeysChanged(before, updatedNodeView, time.Now()) - // Persist to database. // Explicitly select all node columns so GORM includes nil/zero-value fields // (see nodeUpdateColumns comment). @@ -2438,7 +2435,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView _, err := s.persistNodeWith(updatedNodeView, updateColumns, nil) if err != nil { - return types.NodeView{}, false, err + return types.NodeView{}, types.NodeView{}, err } // Log completion @@ -2452,7 +2449,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView Msg("Node re-authorized") } - return updatedNodeView, keysOnly, nil + return updatedNodeView, before, nil } // createAndSaveNewNode creates a new node, allocates IPs, saves to DB, and adds to [NodeStore]. @@ -2859,21 +2856,24 @@ func (s *State) HandleNodeFromAuthPath( var ( finalNode types.NodeView + before types.NodeView keysOnly bool ) if nodeExists && !existingNode.IsTagged() { updateParams.ExistingNode = existingNode - finalNode, keysOnly, err = s.applyAuthNodeUpdate(updateParams) + finalNode, before, err = s.applyAuthNodeUpdate(updateParams) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } + + keysOnly = onlyKeysChanged(before, finalNode, time.Now()) } else if nodeExists { updateParams.ExistingNode = existingNode updateParams.IsConvertFromTag = true - finalNode, _, err = s.applyAuthNodeUpdate(updateParams) + finalNode, before, err = s.applyAuthNodeUpdate(updateParams) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } @@ -2930,7 +2930,11 @@ func (s *State) HandleNodeFromAuthPath( // keysOnly is set only for a same-user relogin; a tag->user conversion is // excluded, as it changes the peer's User — a structural change peers must // see in full, not a key-rotation patch. - return finalNode, reauthChange(finalNode, keysOnly, policyChanged), nil + return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, keysOnly, policyChanged)) +} + +func sortedPrefixes(p []netip.Prefix) []netip.Prefix { + return slices.SortedFunc(slices.Values(p), netip.Prefix.Compare) } // createNewNodeFromAuth creates a new node during auth callback. @@ -3202,15 +3206,19 @@ func (s *State) HandleNodeFromPreAuthKey( keysOnly bool ) + var before types.NodeView + // If this node exists for this user, update the node in place. For a // tags-only key (pak.User == nil) this is true when the machine already has // a tagged node (findExistingNodeForPAK matches it under UserID 0); for a // user-owned key it is true when the same user already has the node. if existsSameUser && existingNodeSameUser.Valid() { - finalNode, keysOnly, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo) + finalNode, before, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } + + keysOnly = onlyKeysChanged(before, finalNode, time.Now()) } else { // Node does not exist for this user with this machine key. // For a user-owned key, check whether the machine key is already held @@ -3304,7 +3312,34 @@ func (s *State) HandleNodeFromPreAuthKey( policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - return finalNode, reauthChange(finalNode, keysOnly, policyChanged), nil + return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, keysOnly, policyChanged)) +} + +// approveRoutesAfterRegistration runs route auto-approval after a +// registration whose NodeStore writer replaced before with node, merging its +// change into c. As on Tailscale SaaS it runs for a new node, a new node key +// or a change to the announced routes; a login that changes neither approves +// nothing. before is what the writer replaced, not the caller's lookup, so an +// announcement and an admin revoke landing in between do not count as a +// change. It waits for the registration: the policy manager must know the +// node's owner first. +func (s *State) approveRoutesAfterRegistration( + before, node types.NodeView, + c change.Change, +) (types.NodeView, change.Change, error) { + if before.Valid() && before.NodeKey() == node.NodeKey() && + slices.Equal(sortedPrefixes(before.AnnouncedRoutes()), sortedPrefixes(node.AnnouncedRoutes())) { + return node, c, nil + } + + routesChange, err := s.AutoApproveRoutes(node) + c = c.Merge(routesChange) + + if err != nil { + return node, c, fmt.Errorf("auto approving routes: %w", err) + } + + return node, c, nil } // reregisterNodeWithPAK updates an existing node in place for a pre-auth key @@ -3315,8 +3350,7 @@ func (s *State) HandleNodeFromPreAuthKey( // in between, and the key or the node can expire. So the writer decides // whether the key must be valid on the node it replaces, at its own clock, and // leaves the node untouched when it is not. existingNodeSameUser is used only -// for its ID and for logging. The bool reports whether peers read the node it -// replaced and the update the same, apart from its keys; see [onlyKeysChanged]. +// for its ID and for logging. It also returns the node the writer replaced. func (s *State) reregisterNodeWithPAK( existingNodeSameUser types.NodeView, pak *types.PreAuthKey, @@ -3324,7 +3358,7 @@ func (s *State) reregisterNodeWithPAK( machineKey key.MachinePublic, hostname string, validHostinfo *tailcfg.Hostinfo, -) (types.NodeView, bool, error) { +) (types.NodeView, types.NodeView, error) { log.Trace(). Caller(). Str(zf.NodeName, existingNodeSameUser.Hostname()). @@ -3342,7 +3376,7 @@ func (s *State) reregisterNodeWithPAK( // NodeStore NodeKey index, denying the victim service. if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok && existing.MachineKey() != machineKey { - return types.NodeView{}, false, ErrNodeKeyInUse + return types.NodeView{}, types.NodeView{}, ErrNodeKeyInUse } // prior is the node the writer replaced: the NetInfo source and the @@ -3467,7 +3501,7 @@ func (s *State) reregisterNodeWithPAK( // A nil prior means the writer never saw the node (deleted, or the store // stopped), so nothing was decided and nothing may be persisted. if !ok || prior == nil { - return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID()) + return types.NodeView{}, types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID()) } if authErr != nil { @@ -3479,7 +3513,7 @@ func (s *State) reregisterNodeWithPAK( Err(authErr). Msg("Re-registration needs a valid auth key when it applies, rejecting") - return types.NodeView{}, false, authErr + return types.NodeView{}, types.NodeView{}, authErr } // Only re-authentication writes a newly loaded key's reference. A wake-up @@ -3507,7 +3541,7 @@ func (s *State) reregisterNodeWithPAK( if err != nil { s.undoReregistration(prior, written) - return types.NodeView{}, false, fmt.Errorf("writing node to database: %w", err) + return types.NodeView{}, types.NodeView{}, fmt.Errorf("writing node to database: %w", err) } log.Trace(). @@ -3519,7 +3553,7 @@ func (s *State) reregisterNodeWithPAK( Str(zf.UserName, pak.Username()). Msg("Node re-authorized") - return updatedNodeView, onlyKeysChanged(before, updatedNodeView, time.Now()), nil + return updatedNodeView, before, nil } // undoReregistration puts back the fields of a failed re-registration that