From 31bf3a6637d0425d4be7e5b63c6044c46f151a97 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 08:53:35 +0000 Subject: [PATCH] Fix exit node visibility issue - filter based on autogroup:internet permission - Modified tailNode/tailNodes functions to accept exitRouteFilterFunc parameter - Added canUseExitRoutes helper to check for broad internet access permission - Added DestsContainsPrefixes method to matcher for checking prefix containment - Exit routes now only included in peer AllowedIPs when requesting node has internet access - Added comprehensive unit tests for both scenarios (with and without autogroup:internet) Fixes #2788 Co-authored-by: kradalby <98431+kradalby@users.noreply.github.com> --- hscontrol/mapper/builder.go | 62 ++++ hscontrol/mapper/exit_node_visibility_test.go | 336 ++++++++++++++++++ hscontrol/mapper/tail.go | 8 +- hscontrol/mapper/tail_test.go | 10 + hscontrol/policy/matcher/matcher.go | 7 + 5 files changed, 422 insertions(+), 1 deletion(-) create mode 100644 hscontrol/mapper/exit_node_visibility_test.go diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index b85eb908..6dde4299 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -7,12 +7,50 @@ import ( "time" "github.com/juanfont/headscale/hscontrol/policy" + "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/types" "tailscale.com/tailcfg" "tailscale.com/types/views" "tailscale.com/util/multierr" ) +// canUseExitRoutes checks if a node can access exit routes (0.0.0.0/0 and ::/0) +// based on ACL matchers. This specifically checks if the node has permission to +// access the internet broadly, which is required to use exit nodes. +// +// Exit routes should only be visible when the ACL explicitly grants broad internet +// access (e.g., via autogroup:internet), not just access to specific services. +func canUseExitRoutes(node types.NodeView, matchers []matcher.Match) bool { + src := node.IPs() + + // Sample public internet IPs to test for broad internet access. + // If the ACL grants access to these well-known public IPs, it's granting + // internet access (e.g., via autogroup:internet). + // Use popular public DNS servers as representatives of internet access. + samplePublicIPs := []netip.Addr{ + netip.MustParseAddr("1.1.1.1"), // Cloudflare DNS + netip.MustParseAddr("8.8.8.8"), // Google DNS + netip.MustParseAddr("208.67.222.222"), // OpenDNS + } + + // Check if any matcher grants access to sample public IPs + for _, matcher := range matchers { + // Check if this node is in the source + if !matcher.SrcsContainsIPs(src...) { + continue + } + + // Check if the destination includes public internet IPs. + // This will be true for autogroup:internet (which resolves to the public internet) + // but false for rules that only allow access to specific private IPs or services. + if matcher.DestsContainsIP(samplePublicIPs...) { + return true + } + } + + return false +} + // MapResponseBuilder provides a fluent interface for building tailcfg.MapResponse. type MapResponseBuilder struct { resp *tailcfg.MapResponse @@ -81,6 +119,14 @@ func (b *MapResponseBuilder) WithSelfNode() *MapResponseBuilder { func(id types.NodeID) []netip.Prefix { return policy.ReduceRoutes(nv, b.mapper.state.GetNodePrimaryRoutes(id), matchers) }, + func(id types.NodeID) []netip.Prefix { + // For self node, always include its own exit routes + peerNode, ok := b.mapper.state.GetNodeByID(id) + if !ok { + return nil + } + return peerNode.ExitRoutes() + }, b.mapper.cfg) if err != nil { b.addError(err) @@ -256,6 +302,22 @@ func (b *MapResponseBuilder) buildTailPeers(peers views.Slice[types.NodeView]) ( func(id types.NodeID) []netip.Prefix { return policy.ReduceRoutes(node, b.mapper.state.GetNodePrimaryRoutes(id), matchers) }, + func(id types.NodeID) []netip.Prefix { + // For peer nodes, only include exit routes if the requesting node can use exit nodes + peerNode, ok := b.mapper.state.GetNodeByID(id) + if !ok { + return nil + } + exitRoutes := peerNode.ExitRoutes() + if len(exitRoutes) == 0 { + return nil + } + // Check if the requesting node has permission to use exit nodes + if canUseExitRoutes(node, matchers) { + return exitRoutes + } + return nil + }, b.mapper.cfg) if err != nil { return nil, err diff --git a/hscontrol/mapper/exit_node_visibility_test.go b/hscontrol/mapper/exit_node_visibility_test.go new file mode 100644 index 00000000..be4d8ab7 --- /dev/null +++ b/hscontrol/mapper/exit_node_visibility_test.go @@ -0,0 +1,336 @@ +package mapper + +import ( + "net/netip" + "testing" + + "github.com/juanfont/headscale/hscontrol/policy" + "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/require" + "tailscale.com/net/tsaddr" + "tailscale.com/tailcfg" + "tailscale.com/types/key" +) + +// TestExitNodeVisibilityWithoutAutogroupInternet tests that exit nodes are not visible +// to nodes that don't have autogroup:internet permission in their ACL. +// This is a regression test for https://github.com/juanfont/headscale/issues/2788 +func TestExitNodeVisibilityWithoutAutogroupInternet(t *testing.T) { + mustNK := func(str string) key.NodePublic { + var k key.NodePublic + _ = k.UnmarshalText([]byte(str)) + return k + } + + mustDK := func(str string) key.DiscoPublic { + var k key.DiscoPublic + _ = k.UnmarshalText([]byte(str)) + return k + } + + mustMK := func(str string) key.MachinePublic { + var k key.MachinePublic + _ = k.UnmarshalText([]byte(str)) + return k + } + + // Create three nodes: mobile, server, exit + mobile := &types.Node{ + ID: 1, + MachineKey: mustMK( + "mkey:f08305b4ee4250b95a70f3b7504d048d75d899993c624a26d422c67af0422507", + ), + NodeKey: mustNK( + "nodekey:9b2ffa7e08cc421a3d2cca9012280f6a236fd0de0b4ce005b30a98ad930306fe", + ), + DiscoKey: mustDK( + "discokey:cf7b0fd05da556fdc3bab365787b506fd82d64a70745db70e00e86c1b1c03084", + ), + IPv4: iap("100.64.0.1"), + Hostname: "mobile", + GivenName: "mobile", + UserID: 1, + User: types.User{ + Name: "alice", + }, + } + + server := &types.Node{ + ID: 2, + MachineKey: mustMK( + "mkey:e08305b4ee4250b95a70f3b7504d048d75d899993c624a26d422c67af0422508", + ), + NodeKey: mustNK( + "nodekey:8b2ffa7e08cc421a3d2cca9012280f6a236fd0de0b4ce005b30a98ad930306ff", + ), + DiscoKey: mustDK( + "discokey:df7b0fd05da556fdc3bab365787b506fd82d64a70745db70e00e86c1b1c03085", + ), + IPv4: iap("100.64.0.2"), + Hostname: "server", + GivenName: "server", + UserID: 1, + User: types.User{ + Name: "alice", + }, + } + + exitNode := &types.Node{ + ID: 3, + MachineKey: mustMK( + "mkey:d08305b4ee4250b95a70f3b7504d048d75d899993c624a26d422c67af0422509", + ), + NodeKey: mustNK( + "nodekey:7b2ffa7e08cc421a3d2cca9012280f6a236fd0de0b4ce005b30a98ad930306fd", + ), + DiscoKey: mustDK( + "discokey:ef7b0fd05da556fdc3bab365787b506fd82d64a70745db70e00e86c1b1c03086", + ), + IPv4: iap("100.64.0.3"), + Hostname: "exit", + GivenName: "exit", + UserID: 1, + User: types.User{ + Name: "alice", + }, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + tsaddr.AllIPv4(), + tsaddr.AllIPv6(), + }, + }, + // Exit node has approved exit routes + ApprovedRoutes: []netip.Prefix{tsaddr.AllIPv4(), tsaddr.AllIPv6()}, + } + + // ACL that only allows mobile -> server:80, no autogroup:internet + pol := []byte(`{ + "hosts": { + "mobile": "100.64.0.1/32", + "server": "100.64.0.2/32", + "exit": "100.64.0.3/32" + }, + "acls": [ + { + "action": "accept", + "src": ["mobile"], + "dst": ["server:80"] + } + ] +}`) + + polMan, err := policy.NewPolicyManager(pol, []types.User{mobile.User}, types.Nodes{mobile, server, exitNode}.ViewSlice()) + require.NoError(t, err) + + matchers, err := polMan.MatchersForNode(mobile.View()) + require.NoError(t, err) + + cfg := &types.Config{ + BaseDomain: "", + RandomizeClientPort: false, + } + + // Build the exit node as a peer from mobile's perspective + exitTailNode, err := tailNode( + exitNode.View(), + 0, + polMan, + func(id types.NodeID) []netip.Prefix { + // No primary routes for this test + return nil + }, + func(id types.NodeID) []netip.Prefix { + // For peer nodes, only include exit routes if the requesting node can use exit nodes + peerNode := exitNode + if id != peerNode.ID { + return nil + } + exitRoutes := peerNode.ExitRoutes() + if len(exitRoutes) == 0 { + return nil + } + // Check if the requesting node has permission to use exit nodes + if canUseExitRoutes(mobile.View(), matchers) { + return exitRoutes + } + return nil + }, + cfg, + ) + require.NoError(t, err) + + // Verify that exit routes are NOT included in AllowedIPs + // since mobile doesn't have autogroup:internet permission + hasExitRoutes := false + for _, prefix := range exitTailNode.AllowedIPs { + if tsaddr.IsExitRoute(prefix) { + hasExitRoutes = true + break + } + } + + if hasExitRoutes { + t.Errorf("Exit node should NOT have exit routes in AllowedIPs when requesting node lacks autogroup:internet permission.\nAllowedIPs: %v", exitTailNode.AllowedIPs) + } + + // The AllowedIPs should only contain the exit node's own IP, not the exit routes + // Check the count and that no exit routes are present + if len(exitTailNode.AllowedIPs) != 1 { + t.Errorf("Expected exactly 1 IP in AllowedIPs (node's own IP), got %d: %v", len(exitTailNode.AllowedIPs), exitTailNode.AllowedIPs) + } + + // Verify the one IP is the node's own IP + expectedIP := netip.MustParsePrefix("100.64.0.3/32") + found := false + for _, ip := range exitTailNode.AllowedIPs { + if ip == expectedIP { + found = true + break + } + } + if !found { + t.Errorf("Expected to find node's own IP %s in AllowedIPs, got: %v", expectedIP, exitTailNode.AllowedIPs) + } +} + +// TestExitNodeVisibilityWithAutogroupInternet tests that exit nodes ARE visible +// to nodes that have autogroup:internet permission in their ACL. +func TestExitNodeVisibilityWithAutogroupInternet(t *testing.T) { + mustNK := func(str string) key.NodePublic { + var k key.NodePublic + _ = k.UnmarshalText([]byte(str)) + return k + } + + mustDK := func(str string) key.DiscoPublic { + var k key.DiscoPublic + _ = k.UnmarshalText([]byte(str)) + return k + } + + mustMK := func(str string) key.MachinePublic { + var k key.MachinePublic + _ = k.UnmarshalText([]byte(str)) + return k + } + + mobile := &types.Node{ + ID: 1, + MachineKey: mustMK( + "mkey:f08305b4ee4250b95a70f3b7504d048d75d899993c624a26d422c67af0422507", + ), + NodeKey: mustNK( + "nodekey:9b2ffa7e08cc421a3d2cca9012280f6a236fd0de0b4ce005b30a98ad930306fe", + ), + DiscoKey: mustDK( + "discokey:cf7b0fd05da556fdc3bab365787b506fd82d64a70745db70e00e86c1b1c03084", + ), + IPv4: iap("100.64.0.1"), + Hostname: "mobile", + GivenName: "mobile", + UserID: 1, + User: types.User{ + Name: "alice", + }, + } + + exitNode := &types.Node{ + ID: 3, + MachineKey: mustMK( + "mkey:d08305b4ee4250b95a70f3b7504d048d75d899993c624a26d422c67af0422509", + ), + NodeKey: mustNK( + "nodekey:7b2ffa7e08cc421a3d2cca9012280f6a236fd0de0b4ce005b30a98ad930306fd", + ), + DiscoKey: mustDK( + "discokey:ef7b0fd05da556fdc3bab365787b506fd82d64a70745db70e00e86c1b1c03086", + ), + IPv4: iap("100.64.0.3"), + Hostname: "exit", + GivenName: "exit", + UserID: 1, + User: types.User{ + Name: "alice", + }, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{ + tsaddr.AllIPv4(), + tsaddr.AllIPv6(), + }, + }, + ApprovedRoutes: []netip.Prefix{tsaddr.AllIPv4(), tsaddr.AllIPv6()}, + } + + // ACL that allows mobile to use autogroup:internet + pol := []byte(`{ + "hosts": { + "mobile": "100.64.0.1/32", + "exit": "100.64.0.3/32" + }, + "acls": [ + { + "action": "accept", + "src": ["mobile"], + "dst": ["autogroup:internet:*"] + } + ] +}`) + + polMan, err := policy.NewPolicyManager(pol, []types.User{mobile.User}, types.Nodes{mobile, exitNode}.ViewSlice()) + require.NoError(t, err) + + matchers, err := polMan.MatchersForNode(mobile.View()) + require.NoError(t, err) + + cfg := &types.Config{ + BaseDomain: "", + RandomizeClientPort: false, + } + + // Build the exit node as a peer from mobile's perspective + exitTailNode, err := tailNode( + exitNode.View(), + 0, + polMan, + func(id types.NodeID) []netip.Prefix { + return nil + }, + func(id types.NodeID) []netip.Prefix { + peerNode := exitNode + if id != peerNode.ID { + return nil + } + exitRoutes := peerNode.ExitRoutes() + if len(exitRoutes) == 0 { + return nil + } + // Check if the requesting node has permission to use exit nodes - mobile has autogroup:internet permission + if canUseExitRoutes(mobile.View(), matchers) { + return exitRoutes + } + return nil + }, + cfg, + ) + require.NoError(t, err) + + // Verify that exit routes ARE included in AllowedIPs + hasIPv4ExitRoute := false + hasIPv6ExitRoute := false + for _, prefix := range exitTailNode.AllowedIPs { + if prefix == tsaddr.AllIPv4() { + hasIPv4ExitRoute = true + } + if prefix == tsaddr.AllIPv6() { + hasIPv6ExitRoute = true + } + } + + if !hasIPv4ExitRoute { + t.Errorf("Exit node should have IPv4 exit route (0.0.0.0/0) in AllowedIPs when requesting node has autogroup:internet permission.\nAllowedIPs: %v", exitTailNode.AllowedIPs) + } + + if !hasIPv6ExitRoute { + t.Errorf("Exit node should have IPv6 exit route (::/0) in AllowedIPs when requesting node has autogroup:internet permission.\nAllowedIPs: %v", exitTailNode.AllowedIPs) + } +} diff --git a/hscontrol/mapper/tail.go b/hscontrol/mapper/tail.go index 3a518d94..9f3e9d21 100644 --- a/hscontrol/mapper/tail.go +++ b/hscontrol/mapper/tail.go @@ -21,6 +21,7 @@ func tailNodes( capVer tailcfg.CapabilityVersion, checker NodeCanHaveTagChecker, primaryRouteFunc routeFilterFunc, + exitRouteFunc routeFilterFunc, cfg *types.Config, ) ([]*tailcfg.Node, error) { tNodes := make([]*tailcfg.Node, 0, nodes.Len()) @@ -31,6 +32,7 @@ func tailNodes( capVer, checker, primaryRouteFunc, + exitRouteFunc, cfg, ) if err != nil { @@ -49,6 +51,7 @@ func tailNode( capVer tailcfg.CapabilityVersion, checker NodeCanHaveTagChecker, primaryRouteFunc routeFilterFunc, + exitRouteFunc routeFilterFunc, cfg *types.Config, ) (*tailcfg.Node, error) { addrs := node.Prefixes() @@ -90,7 +93,10 @@ func tailNode( routes := primaryRouteFunc(node.ID()) allowed := append(addrs, routes...) - allowed = append(allowed, node.ExitRoutes()...) + + // Only include exit routes if the exitRouteFunc allows them + exitRoutes := exitRouteFunc(node.ID()) + allowed = append(allowed, exitRoutes...) tsaddr.SortPrefixes(allowed) tNode := tailcfg.Node{ diff --git a/hscontrol/mapper/tail_test.go b/hscontrol/mapper/tail_test.go index ac96028e..8d95674f 100644 --- a/hscontrol/mapper/tail_test.go +++ b/hscontrol/mapper/tail_test.go @@ -221,6 +221,13 @@ func TestTailNode(t *testing.T) { func(id types.NodeID) []netip.Prefix { return primary.PrimaryRoutes(id) }, + func(id types.NodeID) []netip.Prefix { + // For tests, include exit routes if node has them + if id == tt.node.ID { + return tt.node.ExitRoutes() + } + return nil + }, cfg, ) @@ -281,6 +288,9 @@ func TestNodeExpiry(t *testing.T) { func(id types.NodeID) []netip.Prefix { return []netip.Prefix{} }, + func(id types.NodeID) []netip.Prefix { + return []netip.Prefix{} + }, &types.Config{}, ) if err != nil { diff --git a/hscontrol/policy/matcher/matcher.go b/hscontrol/policy/matcher/matcher.go index aac5a5f3..1e7074ac 100644 --- a/hscontrol/policy/matcher/matcher.go +++ b/hscontrol/policy/matcher/matcher.go @@ -91,3 +91,10 @@ func (m *Match) SrcsOverlapsPrefixes(prefixes ...netip.Prefix) bool { func (m *Match) DestsOverlapsPrefixes(prefixes ...netip.Prefix) bool { return slices.ContainsFunc(prefixes, m.dests.OverlapsPrefix) } + +// DestsContainsPrefixes checks if the destination IPSet contains all the given prefixes. +// This is more strict than DestsOverlapsPrefixes - it requires the entire prefix to be +// contained in the destination, not just overlapping. +func (m *Match) DestsContainsPrefixes(prefixes ...netip.Prefix) bool { + return slices.ContainsFunc(prefixes, m.dests.ContainsPrefix) +}