From eb1ecefd9e5253185de02af6dcdc5e38ee85ab12 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 1 May 2025 08:05:42 +0300 Subject: [PATCH] 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 * auth: ensure that routes are autoapproved when the node is stored Signed-off-by: Kristoffer Dalby --------- Signed-off-by: Kristoffer Dalby --- hscontrol/auth.go | 24 +++++++++++++++++------- hscontrol/grpcv1.go | 19 ++++++++++++++++++- hscontrol/oidc.go | 18 +++++++++++++++++- integration/route_test.go | 10 +++++----- 4 files changed, 57 insertions(+), 14 deletions(-) diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 08de1235..941b51b2 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -213,9 +213,6 @@ func (h *Headscale) handleRegisterWithAuthKey( nodeToRegister.Expiry = ®Req.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 = ®Req.Expiry } - // Ensure any auto approved routes are handled before saving. - policy.AutoApproveRoutes(h.polMan, &nodeToRegister.Node) - h.registrationCache.Set( registrationId, nodeToRegister, diff --git a/hscontrol/grpcv1.go b/hscontrol/grpcv1.go index 0e36cf63..8b516c3e 100644 --- a/hscontrol/grpcv1.go +++ b/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)) } diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index e566d64a..ad2b0fba 100644 --- a/hscontrol/oidc.go +++ b/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, diff --git a/integration/route_test.go b/integration/route_test.go index 8801767d..e4b6239b 100644 --- a/integration/route_test.go +++ b/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]