auth: ensure that routes are autoapproved when the node is stored (#2550)

* integration: ensure route is set before node joins, reproduce

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* auth: ensure that routes are autoapproved when the node is stored

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

hscontrol/auth.go

@@ -213,9 +213,6 @@ func (h *Headscale) handleRegisterWithAuthKey(
 		nodeToRegister.Expiry = &regReq.Expiry
 	}
 
-	// Ensure any auto approved routes are handled before saving.
-	policy.AutoApproveRoutes(h.polMan, &nodeToRegister)
-
 	ipv4, ipv6, err := h.ipAlloc.Next()
 	if err != nil {
 		return nil, fmt.Errorf("allocating IPs: %w", err)
@@ -248,7 +245,23 @@ func (h *Headscale) handleRegisterWithAuthKey(
 		return nil, fmt.Errorf("nodes changed hook: %w", err)
 	}
 
-	if !updateSent {
+	// 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.
+	routesChanged := policy.AutoApproveRoutes(h.polMan, node)
+	if err := h.db.DB.Save(node).Error; err != nil {
+		return nil, fmt.Errorf("saving auto approved routes to node: %w", err)
+	}
+
+	if !updateSent || routesChanged {
 		ctx := types.NotifyCtx(context.Background(), "node updated", node.Hostname)
 		h.nodeNotifier.NotifyAll(ctx, types.UpdatePeerChanged(node.ID))
 	}
@@ -285,9 +298,6 @@ func (h *Headscale) handleRegisterInteractive(
 		nodeToRegister.Node.Expiry = &regReq.Expiry
 	}
 
-	// Ensure any auto approved routes are handled before saving.
-	policy.AutoApproveRoutes(h.polMan, &nodeToRegister.Node)
-
 	h.registrationCache.Set(
 		registrationId,
 		nodeToRegister,

hscontrol/grpcv1.go

@@ -268,7 +268,24 @@ func (api headscaleV1APIServer) RegisterNode(
 	if err != nil {
 		return nil, fmt.Errorf("updating resources using node: %w", err)
 	}
-	if !updateSent {
+
+	// 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.
+	routesChanged := policy.AutoApproveRoutes(api.h.polMan, node)
+	if err := api.h.db.DB.Save(node).Error; err != nil {
+		return nil, fmt.Errorf("saving auto approved routes to node: %w", err)
+	}
+
+	if !updateSent || routesChanged {
 		ctx = types.NotifyCtx(context.Background(), "web-node-login", node.Hostname)
 		api.h.nodeNotifier.NotifyAll(ctx, types.UpdatePeerChanged(node.ID))
 	}

hscontrol/oidc.go

@@ -528,7 +528,23 @@ func (a *AuthProviderOIDC) handleRegistration(
 		return false, fmt.Errorf("updating resources using node: %w", err)
 	}
 
-	if !updateSent {
+	// 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.
+	routesChanged := policy.AutoApproveRoutes(a.polMan, node)
+	if err := a.db.DB.Save(node).Error; err != nil {
+		return false, fmt.Errorf("saving auto approved routes to node: %w", err)
+	}
+
+	if !updateSent || routesChanged {
 		ctx := types.NotifyCtx(context.Background(), "oidc-expiry-self", node.Hostname)
 		a.notifier.NotifyByNodeID(
 			ctx,

integration/route_test.go

@@ -1654,6 +1654,11 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 					assertNoErrGetHeadscale(t, err)
 					assert.NotNil(t, headscale)
 
+					// Set the route of usernet1 to be autoapproved
+					tt.pol.AutoApprovers.Routes[route.String()] = []string{tt.approver}
+					err = headscale.SetPolicy(tt.pol)
+					require.NoError(t, err)
+
 					if advertiseDuringUp {
 						tsOpts = append(tsOpts,
 							tsic.WithExtraLoginArgs([]string{"--advertise-routes=" + route.String()}),
@@ -1691,11 +1696,6 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 					}
 					// extra creation end.
 
-					// Set the route of usernet1 to be autoapproved
-					tt.pol.AutoApprovers.Routes[route.String()] = []string{tt.approver}
-					err = headscale.SetPolicy(tt.pol)
-					require.NoError(t, err)
-
 					routerUsernet1ID := routerUsernet1.MustID()
 
 					web := services[0]