diff --git a/cmd/headscale/cli/policy.go b/cmd/headscale/cli/policy.go index b8a9a2ad..f99d5390 100644 --- a/cmd/headscale/cli/policy.go +++ b/cmd/headscale/cli/policy.go @@ -127,12 +127,6 @@ var setPolicy = &cobra.Command{ ErrorOutput(err, fmt.Sprintf("Error reading the policy file: %s", err), output) } - _, err = policy.NewPolicyManager(policyBytes, nil, views.Slice[types.NodeView]{}) - if err != nil { - ErrorOutput(err, fmt.Sprintf("Error parsing the policy file: %s", err), output) - return - } - if bypass, _ := cmd.Flags().GetBool(bypassFlag); bypass { confirm := false force, _ := cmd.Flags().GetBool("force") @@ -159,6 +153,17 @@ var setPolicy = &cobra.Command{ ErrorOutput(err, fmt.Sprintf("Failed to open database: %s", err), output) } + users, err := d.ListUsers() + if err != nil { + ErrorOutput(err, fmt.Sprintf("Failed to load users for policy validation: %s", err), output) + } + + _, err = policy.NewPolicyManager(policyBytes, users, views.Slice[types.NodeView]{}) + if err != nil { + ErrorOutput(err, fmt.Sprintf("Error parsing the policy file: %s", err), output) + return + } + _, err = d.SetPolicy(string(policyBytes)) if err != nil { ErrorOutput(err, fmt.Sprintf("Failed to set ACL Policy: %s", err), output) diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index 4e4533be..70d3afaf 100644 --- a/hscontrol/db/node.go +++ b/hscontrol/db/node.go @@ -18,6 +18,7 @@ import ( "github.com/juanfont/headscale/hscontrol/util" "github.com/rs/zerolog/log" "gorm.io/gorm" + "tailscale.com/net/tsaddr" "tailscale.com/types/key" "tailscale.com/types/ptr" ) @@ -232,6 +233,17 @@ func SetApprovedRoutes( return nil } + // When approving exit routes, ensure both IPv4 and IPv6 are included + // If either 0.0.0.0/0 or ::/0 is being approved, both should be approved + hasIPv4Exit := slices.Contains(routes, tsaddr.AllIPv4()) + hasIPv6Exit := slices.Contains(routes, tsaddr.AllIPv6()) + + if hasIPv4Exit && !hasIPv6Exit { + routes = append(routes, tsaddr.AllIPv6()) + } else if hasIPv6Exit && !hasIPv4Exit { + routes = append(routes, tsaddr.AllIPv4()) + } + b, err := json.Marshal(routes) if err != nil { return err diff --git a/hscontrol/db/node_test.go b/hscontrol/db/node_test.go index b51dba1c..0efd0e8b 100644 --- a/hscontrol/db/node_test.go +++ b/hscontrol/db/node_test.go @@ -476,7 +476,7 @@ func TestAutoApproveRoutes(t *testing.T) { require.NoError(t, err) } - newRoutes2, changed2 := policy.ApproveRoutesWithPolicy(pm, nodeTagged.View(), node.ApprovedRoutes, tt.routes) + newRoutes2, changed2 := policy.ApproveRoutesWithPolicy(pm, nodeTagged.View(), nodeTagged.ApprovedRoutes, tt.routes) if changed2 { err = SetApprovedRoutes(adb.DB, nodeTagged.ID, newRoutes2) require.NoError(t, err) @@ -490,7 +490,7 @@ func TestAutoApproveRoutes(t *testing.T) { if len(expectedRoutes1) == 0 { expectedRoutes1 = nil } - if diff := cmp.Diff(expectedRoutes1, node1ByID.SubnetRoutes(), util.Comparers...); diff != "" { + if diff := cmp.Diff(expectedRoutes1, node1ByID.AllApprovedRoutes(), util.Comparers...); diff != "" { t.Errorf("unexpected enabled routes (-want +got):\n%s", diff) } @@ -501,7 +501,7 @@ func TestAutoApproveRoutes(t *testing.T) { if len(expectedRoutes2) == 0 { expectedRoutes2 = nil } - if diff := cmp.Diff(expectedRoutes2, node2ByID.SubnetRoutes(), util.Comparers...); diff != "" { + if diff := cmp.Diff(expectedRoutes2, node2ByID.AllApprovedRoutes(), util.Comparers...); diff != "" { t.Errorf("unexpected enabled routes (-want +got):\n%s", diff) } }) diff --git a/hscontrol/derp/derp.go b/hscontrol/derp/derp.go index 6c8244f5..42d74abe 100644 --- a/hscontrol/derp/derp.go +++ b/hscontrol/derp/derp.go @@ -12,6 +12,7 @@ import ( "net/url" "os" "reflect" + "slices" "sync" "time" @@ -126,7 +127,17 @@ func shuffleDERPMap(dm *tailcfg.DERPMap) { return } - for id, region := range dm.Regions { + // Collect region IDs and sort them to ensure deterministic iteration order. + // Map iteration order is non-deterministic in Go, which would cause the + // shuffle to be non-deterministic even with a fixed seed. + ids := make([]int, 0, len(dm.Regions)) + for id := range dm.Regions { + ids = append(ids, id) + } + slices.Sort(ids) + + for _, id := range ids { + region := dm.Regions[id] if len(region.Nodes) == 0 { continue } diff --git a/hscontrol/derp/derp_test.go b/hscontrol/derp/derp_test.go index 9334de05..91d605a6 100644 --- a/hscontrol/derp/derp_test.go +++ b/hscontrol/derp/derp_test.go @@ -83,9 +83,9 @@ func TestShuffleDERPMapDeterministic(t *testing.T) { RegionCode: "sea", RegionName: "Seattle", Nodes: []*tailcfg.DERPNode{ - {Name: "10b", RegionID: 10, HostName: "derp10b.tailscale.com"}, - {Name: "10c", RegionID: 10, HostName: "derp10c.tailscale.com"}, {Name: "10d", RegionID: 10, HostName: "derp10d.tailscale.com"}, + {Name: "10c", RegionID: 10, HostName: "derp10c.tailscale.com"}, + {Name: "10b", RegionID: 10, HostName: "derp10b.tailscale.com"}, }, }, 2: { @@ -93,9 +93,9 @@ func TestShuffleDERPMapDeterministic(t *testing.T) { RegionCode: "sfo", RegionName: "San Francisco", Nodes: []*tailcfg.DERPNode{ - {Name: "2f", RegionID: 2, HostName: "derp2f.tailscale.com"}, - {Name: "2e", RegionID: 2, HostName: "derp2e.tailscale.com"}, {Name: "2d", RegionID: 2, HostName: "derp2d.tailscale.com"}, + {Name: "2e", RegionID: 2, HostName: "derp2e.tailscale.com"}, + {Name: "2f", RegionID: 2, HostName: "derp2f.tailscale.com"}, }, }, }, @@ -169,6 +169,74 @@ func TestShuffleDERPMapDeterministic(t *testing.T) { }, }, }, + { + name: "same dataset with another base domain", + baseDomain: "another.example.com", + derpMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 4: { + RegionID: 4, + RegionCode: "fra", + RegionName: "Frankfurt", + Nodes: []*tailcfg.DERPNode{ + {Name: "4f", RegionID: 4, HostName: "derp4f.tailscale.com"}, + {Name: "4g", RegionID: 4, HostName: "derp4g.tailscale.com"}, + {Name: "4h", RegionID: 4, HostName: "derp4h.tailscale.com"}, + {Name: "4i", RegionID: 4, HostName: "derp4i.tailscale.com"}, + }, + }, + }, + }, + expected: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 4: { + RegionID: 4, + RegionCode: "fra", + RegionName: "Frankfurt", + Nodes: []*tailcfg.DERPNode{ + {Name: "4h", RegionID: 4, HostName: "derp4h.tailscale.com"}, + {Name: "4f", RegionID: 4, HostName: "derp4f.tailscale.com"}, + {Name: "4g", RegionID: 4, HostName: "derp4g.tailscale.com"}, + {Name: "4i", RegionID: 4, HostName: "derp4i.tailscale.com"}, + }, + }, + }, + }, + }, + { + name: "same dataset with yet another base domain", + baseDomain: "yetanother.example.com", + derpMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 4: { + RegionID: 4, + RegionCode: "fra", + RegionName: "Frankfurt", + Nodes: []*tailcfg.DERPNode{ + {Name: "4f", RegionID: 4, HostName: "derp4f.tailscale.com"}, + {Name: "4g", RegionID: 4, HostName: "derp4g.tailscale.com"}, + {Name: "4h", RegionID: 4, HostName: "derp4h.tailscale.com"}, + {Name: "4i", RegionID: 4, HostName: "derp4i.tailscale.com"}, + }, + }, + }, + }, + expected: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 4: { + RegionID: 4, + RegionCode: "fra", + RegionName: "Frankfurt", + Nodes: []*tailcfg.DERPNode{ + {Name: "4i", RegionID: 4, HostName: "derp4i.tailscale.com"}, + {Name: "4h", RegionID: 4, HostName: "derp4h.tailscale.com"}, + {Name: "4f", RegionID: 4, HostName: "derp4f.tailscale.com"}, + {Name: "4g", RegionID: 4, HostName: "derp4g.tailscale.com"}, + }, + }, + }, + }, + }, } for _, tt := range tests { diff --git a/hscontrol/mapper/tail_test.go b/hscontrol/mapper/tail_test.go index ac96028e..3a3b39d1 100644 --- a/hscontrol/mapper/tail_test.go +++ b/hscontrol/mapper/tail_test.go @@ -108,11 +108,12 @@ func TestTailNode(t *testing.T) { Hostinfo: &tailcfg.Hostinfo{ RoutableIPs: []netip.Prefix{ tsaddr.AllIPv4(), + tsaddr.AllIPv6(), netip.MustParsePrefix("192.168.0.0/24"), netip.MustParsePrefix("172.0.0.0/10"), }, }, - ApprovedRoutes: []netip.Prefix{tsaddr.AllIPv4(), netip.MustParsePrefix("192.168.0.0/24")}, + ApprovedRoutes: []netip.Prefix{tsaddr.AllIPv4(), tsaddr.AllIPv6(), netip.MustParsePrefix("192.168.0.0/24")}, CreatedAt: created, }, dnsConfig: &tailcfg.DNSConfig{}, @@ -150,6 +151,7 @@ func TestTailNode(t *testing.T) { Hostinfo: hiview(tailcfg.Hostinfo{ RoutableIPs: []netip.Prefix{ tsaddr.AllIPv4(), + tsaddr.AllIPv6(), netip.MustParsePrefix("192.168.0.0/24"), netip.MustParsePrefix("172.0.0.0/10"), }, diff --git a/hscontrol/policy/matcher/matcher.go b/hscontrol/policy/matcher/matcher.go index aac5a5f3..afc3cf68 100644 --- a/hscontrol/policy/matcher/matcher.go +++ b/hscontrol/policy/matcher/matcher.go @@ -7,6 +7,7 @@ import ( "github.com/juanfont/headscale/hscontrol/util" "go4.org/netipx" + "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" ) @@ -91,3 +92,12 @@ func (m *Match) SrcsOverlapsPrefixes(prefixes ...netip.Prefix) bool { func (m *Match) DestsOverlapsPrefixes(prefixes ...netip.Prefix) bool { return slices.ContainsFunc(prefixes, m.dests.OverlapsPrefix) } + +// DestsIsTheInternet reports if the destination is equal to "the internet" +// which is a IPSet that represents "autogroup:internet" and is special +// cased for exit nodes. +func (m Match) DestsIsTheInternet() bool { + return m.dests.Equal(util.TheInternet()) || + m.dests.ContainsPrefix(tsaddr.AllIPv4()) || + m.dests.ContainsPrefix(tsaddr.AllIPv6()) +} diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index b849d470..c016fa58 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -10,6 +10,7 @@ import ( "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gorm.io/gorm" "tailscale.com/tailcfg" @@ -782,12 +783,287 @@ func TestReduceNodes(t *testing.T) { got = append(got, v.AsStruct()) } if diff := cmp.Diff(tt.want, got, util.Comparers...); diff != "" { - t.Errorf("FilterNodesByACL() unexpected result (-want +got):\n%s", diff) + t.Errorf("ReduceNodes() unexpected result (-want +got):\n%s", diff) + t.Log("Matchers: ") + for _, m := range matchers { + t.Log("\t+", m.DebugString()) + } } }) } } +func TestReduceNodesFromPolicy(t *testing.T) { + n := func(id types.NodeID, ip, hostname, username string, routess ...string) *types.Node { + var routes []netip.Prefix + for _, route := range routess { + routes = append(routes, netip.MustParsePrefix(route)) + } + + return &types.Node{ + ID: id, + IPv4: ap(ip), + Hostname: hostname, + User: types.User{Name: username}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: routes, + }, + ApprovedRoutes: routes, + } + } + + type args struct { + } + tests := []struct { + name string + nodes types.Nodes + policy string + node *types.Node + want types.Nodes + wantMatchers int + }{ + { + name: "2788-exit-node-too-visible", + nodes: types.Nodes{ + n(1, "100.64.0.1", "mobile", "mobile"), + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + policy: ` +{ + "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" + ] + } + ] +}`, + node: n(1, "100.64.0.1", "mobile", "mobile"), + want: types.Nodes{ + n(2, "100.64.0.2", "server", "server"), + }, + wantMatchers: 1, + }, + { + name: "2788-exit-node-autogroup:internet", + nodes: types.Nodes{ + n(1, "100.64.0.1", "mobile", "mobile"), + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + policy: ` +{ + "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" + ] + }, + { + "action": "accept", + "src": [ + "mobile" + ], + "dst": [ + "autogroup:internet:*" + ] + } + ] +}`, + node: n(1, "100.64.0.1", "mobile", "mobile"), + want: types.Nodes{ + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + wantMatchers: 2, + }, + { + name: "2788-exit-node-0000-route", + nodes: types.Nodes{ + n(1, "100.64.0.1", "mobile", "mobile"), + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + policy: ` +{ + "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" + ] + }, + { + "action": "accept", + "src": [ + "mobile" + ], + "dst": [ + "0.0.0.0/0:*" + ] + } + ] +}`, + node: n(1, "100.64.0.1", "mobile", "mobile"), + want: types.Nodes{ + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + wantMatchers: 2, + }, + { + name: "2788-exit-node-::0-route", + nodes: types.Nodes{ + n(1, "100.64.0.1", "mobile", "mobile"), + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + policy: ` +{ + "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" + ] + }, + { + "action": "accept", + "src": [ + "mobile" + ], + "dst": [ + "::0/0:*" + ] + } + ] +}`, + node: n(1, "100.64.0.1", "mobile", "mobile"), + want: types.Nodes{ + n(2, "100.64.0.2", "server", "server"), + n(3, "100.64.0.3", "exit", "server", "0.0.0.0/0", "::/0"), + }, + wantMatchers: 2, + }, + { + name: "2784-split-exit-node-access", + nodes: types.Nodes{ + n(1, "100.64.0.1", "user", "user"), + n(2, "100.64.0.2", "exit1", "exit", "0.0.0.0/0", "::/0"), + n(3, "100.64.0.3", "exit2", "exit", "0.0.0.0/0", "::/0"), + n(4, "100.64.0.4", "otheruser", "otheruser"), + }, + policy: ` +{ + "hosts": { + "user": "100.64.0.1/32", + "exit1": "100.64.0.2/32", + "exit2": "100.64.0.3/32", + "otheruser": "100.64.0.4/32", + }, + + "acls": [ + { + "action": "accept", + "src": [ + "user" + ], + "dst": [ + "exit1:*" + ] + }, + { + "action": "accept", + "src": [ + "otheruser" + ], + "dst": [ + "exit2:*" + ] + } + ] +}`, + node: n(1, "100.64.0.1", "user", "user"), + want: types.Nodes{ + n(2, "100.64.0.2", "exit1", "exit", "0.0.0.0/0", "::/0"), + }, + wantMatchers: 2, + }, + } + + for _, tt := range tests { + for idx, pmf := range PolicyManagerFuncsForTest([]byte(tt.policy)) { + t.Run(fmt.Sprintf("%s-index%d", tt.name, idx), func(t *testing.T) { + var pm PolicyManager + var err error + pm, err = pmf(nil, tt.nodes.ViewSlice()) + require.NoError(t, err) + + matchers, err := pm.MatchersForNode(tt.node.View()) + require.NoError(t, err) + assert.Len(t, matchers, tt.wantMatchers) + + gotViews := ReduceNodes( + tt.node.View(), + tt.nodes.ViewSlice(), + matchers, + ) + // Convert views back to nodes for comparison in tests + var got types.Nodes + for _, v := range gotViews.All() { + got = append(got, v.AsStruct()) + } + if diff := cmp.Diff(tt.want, got, util.Comparers...); diff != "" { + t.Errorf("TestReduceNodesFromPolicy() unexpected result (-want +got):\n%s", diff) + t.Log("Matchers: ") + for _, m := range matchers { + t.Log("\t+", m.DebugString()) + } + } + }) + } + } +} + func TestSSHPolicyRules(t *testing.T) { users := []types.User{ {Name: "user1", Model: gorm.Model{ID: 1}}, diff --git a/hscontrol/policy/v2/filter.go b/hscontrol/policy/v2/filter.go index abdd4ffb..bb7d089a 100644 --- a/hscontrol/policy/v2/filter.go +++ b/hscontrol/policy/v2/filter.go @@ -99,14 +99,16 @@ func (pol *Policy) compileFilterRulesForNode( return nil, ErrInvalidAction } - rule, err := pol.compileACLWithAutogroupSelf(acl, users, node, nodes) + aclRules, err := pol.compileACLWithAutogroupSelf(acl, users, node, nodes) if err != nil { log.Trace().Err(err).Msgf("compiling ACL") continue } - if rule != nil { - rules = append(rules, *rule) + for _, rule := range aclRules { + if rule != nil { + rules = append(rules, *rule) + } } } @@ -115,27 +117,32 @@ func (pol *Policy) compileFilterRulesForNode( // compileACLWithAutogroupSelf compiles a single ACL rule, handling // autogroup:self per-node while supporting all other alias types normally. +// It returns a slice of filter rules because when an ACL has both autogroup:self +// and other destinations, they need to be split into separate rules with different +// source filtering logic. func (pol *Policy) compileACLWithAutogroupSelf( acl ACL, users types.Users, node types.NodeView, nodes views.Slice[types.NodeView], -) (*tailcfg.FilterRule, error) { - // Check if any destination uses autogroup:self - hasAutogroupSelfInDst := false +) ([]*tailcfg.FilterRule, error) { + var autogroupSelfDests []AliasWithPorts + var otherDests []AliasWithPorts for _, dest := range acl.Destinations { if ag, ok := dest.Alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { - hasAutogroupSelfInDst = true - break + autogroupSelfDests = append(autogroupSelfDests, dest) + } else { + otherDests = append(otherDests, dest) } } - var srcIPs netipx.IPSetBuilder + protocols, _ := acl.Protocol.parseProtocol() + var rules []*tailcfg.FilterRule + + var resolvedSrcIPs []*netipx.IPSet - // Resolve sources to only include devices from the same user as the target node. for _, src := range acl.Sources { - // autogroup:self is not allowed in sources if ag, ok := src.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { return nil, fmt.Errorf("autogroup:self cannot be used in sources") } @@ -147,92 +154,121 @@ func (pol *Policy) compileACLWithAutogroupSelf( } if ips != nil { - if hasAutogroupSelfInDst { - // Instead of iterating all addresses (which could be millions), - // check each node's IPs against the source set - for _, n := range nodes.All() { - if n.User().ID == node.User().ID && !n.IsTagged() { - // Check if any of this node's IPs are in the source set - for _, nodeIP := range n.IPs() { - if ips.Contains(nodeIP) { - n.AppendToIPSet(&srcIPs) - break // Found this node, move to next - } - } - } - } - } else { - // No autogroup:self in destination, use all resolved sources - srcIPs.AddSet(ips) - } + resolvedSrcIPs = append(resolvedSrcIPs, ips) } } - srcSet, err := srcIPs.IPSet() - if err != nil { - return nil, err + if len(resolvedSrcIPs) == 0 { + return rules, nil } - if srcSet == nil || len(srcSet.Prefixes()) == 0 { - // No sources resolved, skip this rule - return nil, nil //nolint:nilnil - } + // Handle autogroup:self destinations (if any) + if len(autogroupSelfDests) > 0 { + // Pre-filter to same-user untagged devices once - reuse for both sources and destinations + sameUserNodes := make([]types.NodeView, 0) + for _, n := range nodes.All() { + if n.User().ID == node.User().ID && !n.IsTagged() { + sameUserNodes = append(sameUserNodes, n) + } + } - protocols, _ := acl.Protocol.parseProtocol() - - var destPorts []tailcfg.NetPortRange - - for _, dest := range acl.Destinations { - if ag, ok := dest.Alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { - for _, n := range nodes.All() { - if n.User().ID == node.User().ID && !n.IsTagged() { - for _, port := range dest.Ports { - for _, ip := range n.IPs() { - pr := tailcfg.NetPortRange{ - IP: ip.String(), - Ports: port, - } - destPorts = append(destPorts, pr) + if len(sameUserNodes) > 0 { + // Filter sources to only same-user untagged devices + var srcIPs netipx.IPSetBuilder + for _, ips := range resolvedSrcIPs { + for _, n := range sameUserNodes { + // Check if any of this node's IPs are in the source set + for _, nodeIP := range n.IPs() { + if ips.Contains(nodeIP) { + n.AppendToIPSet(&srcIPs) + break } } } } - } else { - ips, err := dest.Resolve(pol, users, nodes) + + srcSet, err := srcIPs.IPSet() if err != nil { - log.Trace().Err(err).Msgf("resolving destination ips") - continue + return nil, err } - if ips == nil { - log.Debug().Msgf("destination resolved to nil ips: %v", dest) - continue - } - - prefixes := ips.Prefixes() - - for _, pref := range prefixes { - for _, port := range dest.Ports { - pr := tailcfg.NetPortRange{ - IP: pref.String(), - Ports: port, + if srcSet != nil && len(srcSet.Prefixes()) > 0 { + var destPorts []tailcfg.NetPortRange + for _, dest := range autogroupSelfDests { + for _, n := range sameUserNodes { + for _, port := range dest.Ports { + for _, ip := range n.IPs() { + destPorts = append(destPorts, tailcfg.NetPortRange{ + IP: ip.String(), + Ports: port, + }) + } + } } - destPorts = append(destPorts, pr) + } + + if len(destPorts) > 0 { + rules = append(rules, &tailcfg.FilterRule{ + SrcIPs: ipSetToPrefixStringList(srcSet), + DstPorts: destPorts, + IPProto: protocols, + }) } } } } - if len(destPorts) == 0 { - // No destinations resolved, skip this rule - return nil, nil //nolint:nilnil + if len(otherDests) > 0 { + var srcIPs netipx.IPSetBuilder + + for _, ips := range resolvedSrcIPs { + srcIPs.AddSet(ips) + } + + srcSet, err := srcIPs.IPSet() + if err != nil { + return nil, err + } + + if srcSet != nil && len(srcSet.Prefixes()) > 0 { + var destPorts []tailcfg.NetPortRange + + for _, dest := range otherDests { + ips, err := dest.Resolve(pol, users, nodes) + if err != nil { + log.Trace().Err(err).Msgf("resolving destination ips") + continue + } + + if ips == nil { + log.Debug().Msgf("destination resolved to nil ips: %v", dest) + continue + } + + prefixes := ips.Prefixes() + + for _, pref := range prefixes { + for _, port := range dest.Ports { + pr := tailcfg.NetPortRange{ + IP: pref.String(), + Ports: port, + } + destPorts = append(destPorts, pr) + } + } + } + + if len(destPorts) > 0 { + rules = append(rules, &tailcfg.FilterRule{ + SrcIPs: ipSetToPrefixStringList(srcSet), + DstPorts: destPorts, + IPProto: protocols, + }) + } + } } - return &tailcfg.FilterRule{ - SrcIPs: ipSetToPrefixStringList(srcSet), - DstPorts: destPorts, - IPProto: protocols, - }, nil + return rules, nil } func sshAction(accept bool, duration time.Duration) tailcfg.SSHAction { @@ -260,46 +296,30 @@ func (pol *Policy) compileSSHPolicy( var rules []*tailcfg.SSHRule for index, rule := range pol.SSHs { - // Check if any destination uses autogroup:self - hasAutogroupSelfInDst := false + // Separate destinations into autogroup:self and others + // This is needed because autogroup:self requires filtering sources to same-user only, + // while other destinations should use all resolved sources + var autogroupSelfDests []Alias + var otherDests []Alias + for _, dst := range rule.Destinations { if ag, ok := dst.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { - hasAutogroupSelfInDst = true - break - } - } - - // If autogroup:self is used, skip tagged nodes - if hasAutogroupSelfInDst && node.IsTagged() { - continue - } - - var dest netipx.IPSetBuilder - for _, src := range rule.Destinations { - // Handle autogroup:self specially - if ag, ok := src.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { - // For autogroup:self, only include the target user's untagged devices - for _, n := range nodes.All() { - if n.User().ID == node.User().ID && !n.IsTagged() { - n.AppendToIPSet(&dest) - } - } + autogroupSelfDests = append(autogroupSelfDests, dst) } else { - ips, err := src.Resolve(pol, users, nodes) - if err != nil { - log.Trace().Caller().Err(err).Msgf("resolving destination ips") - continue - } - dest.AddSet(ips) + otherDests = append(otherDests, dst) } } - destSet, err := dest.IPSet() + // Note: Tagged nodes can't match autogroup:self destinations, but can still match other destinations + + // Resolve sources once - we'll use them differently for each destination type + srcIPs, err := rule.Sources.Resolve(pol, users, nodes) if err != nil { - return nil, err + log.Trace().Caller().Err(err).Msgf("SSH policy compilation failed resolving source ips for rule %+v", rule) + continue // Skip this rule if we can't resolve sources } - if !node.InIPSet(destSet) { + if srcIPs == nil || len(srcIPs.Prefixes()) == 0 { continue } @@ -313,50 +333,9 @@ func (pol *Policy) compileSSHPolicy( return nil, fmt.Errorf("parsing SSH policy, unknown action %q, index: %d: %w", rule.Action, index, err) } - var principals []*tailcfg.SSHPrincipal - srcIPs, err := rule.Sources.Resolve(pol, users, nodes) - if err != nil { - log.Trace().Caller().Err(err).Msgf("SSH policy compilation failed resolving source ips for rule %+v", rule) - continue // Skip this rule if we can't resolve sources - } - - // If autogroup:self is in destinations, filter sources to same user only - if hasAutogroupSelfInDst { - var filteredSrcIPs netipx.IPSetBuilder - // Instead of iterating all addresses, check each node's IPs - for _, n := range nodes.All() { - if n.User().ID == node.User().ID && !n.IsTagged() { - // Check if any of this node's IPs are in the source set - for _, nodeIP := range n.IPs() { - if srcIPs.Contains(nodeIP) { - n.AppendToIPSet(&filteredSrcIPs) - break // Found this node, move to next - } - } - } - } - - srcIPs, err = filteredSrcIPs.IPSet() - if err != nil { - return nil, err - } - - if srcIPs == nil || len(srcIPs.Prefixes()) == 0 { - // No valid sources after filtering, skip this rule - continue - } - } - - for addr := range util.IPSetAddrIter(srcIPs) { - principals = append(principals, &tailcfg.SSHPrincipal{ - NodeIP: addr.String(), - }) - } - userMap := make(map[string]string, len(rule.Users)) if rule.Users.ContainsNonRoot() { userMap["*"] = "=" - // by default, we do not allow root unless explicitly stated userMap["root"] = "" } @@ -366,11 +345,108 @@ func (pol *Policy) compileSSHPolicy( for _, u := range rule.Users.NormalUsers() { userMap[u.String()] = u.String() } - rules = append(rules, &tailcfg.SSHRule{ - Principals: principals, - SSHUsers: userMap, - Action: &action, - }) + + // Handle autogroup:self destinations (if any) + // Note: Tagged nodes can't match autogroup:self, so skip this block for tagged nodes + if len(autogroupSelfDests) > 0 && !node.IsTagged() { + // Build destination set for autogroup:self (same-user untagged devices only) + var dest netipx.IPSetBuilder + for _, n := range nodes.All() { + if n.User().ID == node.User().ID && !n.IsTagged() { + n.AppendToIPSet(&dest) + } + } + + destSet, err := dest.IPSet() + if err != nil { + return nil, err + } + + // Only create rule if this node is in the destination set + if node.InIPSet(destSet) { + // Filter sources to only same-user untagged devices + // Pre-filter to same-user untagged devices for efficiency + sameUserNodes := make([]types.NodeView, 0) + for _, n := range nodes.All() { + if n.User().ID == node.User().ID && !n.IsTagged() { + sameUserNodes = append(sameUserNodes, n) + } + } + + var filteredSrcIPs netipx.IPSetBuilder + for _, n := range sameUserNodes { + // Check if any of this node's IPs are in the source set + for _, nodeIP := range n.IPs() { + if srcIPs.Contains(nodeIP) { + n.AppendToIPSet(&filteredSrcIPs) + break // Found this node, move to next + } + } + } + + filteredSrcSet, err := filteredSrcIPs.IPSet() + if err != nil { + return nil, err + } + + if filteredSrcSet != nil && len(filteredSrcSet.Prefixes()) > 0 { + var principals []*tailcfg.SSHPrincipal + for addr := range util.IPSetAddrIter(filteredSrcSet) { + principals = append(principals, &tailcfg.SSHPrincipal{ + NodeIP: addr.String(), + }) + } + + if len(principals) > 0 { + rules = append(rules, &tailcfg.SSHRule{ + Principals: principals, + SSHUsers: userMap, + Action: &action, + }) + } + } + } + } + + // Handle other destinations (if any) + if len(otherDests) > 0 { + // Build destination set for other destinations + var dest netipx.IPSetBuilder + for _, dst := range otherDests { + ips, err := dst.Resolve(pol, users, nodes) + if err != nil { + log.Trace().Caller().Err(err).Msgf("resolving destination ips") + continue + } + if ips != nil { + dest.AddSet(ips) + } + } + + destSet, err := dest.IPSet() + if err != nil { + return nil, err + } + + // Only create rule if this node is in the destination set + if node.InIPSet(destSet) { + // For non-autogroup:self destinations, use all resolved sources (no filtering) + var principals []*tailcfg.SSHPrincipal + for addr := range util.IPSetAddrIter(srcIPs) { + principals = append(principals, &tailcfg.SSHPrincipal{ + NodeIP: addr.String(), + }) + } + + if len(principals) > 0 { + rules = append(rules, &tailcfg.SSHRule{ + Principals: principals, + SSHUsers: userMap, + Action: &action, + }) + } + } + } } return &tailcfg.SSHPolicy{ diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index 9f2845ac..37ff8730 100644 --- a/hscontrol/policy/v2/filter_test.go +++ b/hscontrol/policy/v2/filter_test.go @@ -1339,3 +1339,70 @@ func TestSSHWithAutogroupSelfExcludesTaggedDevices(t *testing.T) { assert.Empty(t, sshPolicy2.Rules, "tagged node should get no SSH rules with autogroup:self") } } + +// TestSSHWithAutogroupSelfAndMixedDestinations tests that SSH rules can have both +// autogroup:self and other destinations (like tag:router) in the same rule, and that +// autogroup:self filtering only applies to autogroup:self destinations, not others. +func TestSSHWithAutogroupSelfAndMixedDestinations(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "user1"}, + {Model: gorm.Model{ID: 2}, Name: "user2"}, + } + + nodes := types.Nodes{ + {User: users[0], IPv4: ap("100.64.0.1"), Hostname: "user1-device"}, + {User: users[0], IPv4: ap("100.64.0.2"), Hostname: "user1-device2"}, + {User: users[1], IPv4: ap("100.64.0.3"), Hostname: "user2-device"}, + {User: users[1], IPv4: ap("100.64.0.4"), Hostname: "user2-router", ForcedTags: []string{"tag:router"}}, + } + + policy := &Policy{ + TagOwners: TagOwners{ + Tag("tag:router"): Owners{up("user2@")}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{agp("autogroup:member")}, + Destinations: SSHDstAliases{agp("autogroup:self"), tp("tag:router")}, + Users: []SSHUser{"admin"}, + }, + }, + } + + err := policy.validate() + require.NoError(t, err) + + // Test 1: Compile for user1's device (should only match autogroup:self destination) + node1 := nodes[0].View() + sshPolicy1, err := policy.compileSSHPolicy(users, node1, nodes.ViewSlice()) + require.NoError(t, err) + require.NotNil(t, sshPolicy1) + require.Len(t, sshPolicy1.Rules, 1, "user1's device should have 1 SSH rule (autogroup:self)") + + // Verify autogroup:self rule has filtered sources (only same-user devices) + selfRule := sshPolicy1.Rules[0] + require.Len(t, selfRule.Principals, 2, "autogroup:self rule should only have user1's devices") + selfPrincipals := make([]string, len(selfRule.Principals)) + for i, p := range selfRule.Principals { + selfPrincipals[i] = p.NodeIP + } + require.ElementsMatch(t, []string{"100.64.0.1", "100.64.0.2"}, selfPrincipals, + "autogroup:self rule should only include same-user untagged devices") + + // Test 2: Compile for router (should only match tag:router destination) + routerNode := nodes[3].View() // user2-router + sshPolicyRouter, err := policy.compileSSHPolicy(users, routerNode, nodes.ViewSlice()) + require.NoError(t, err) + require.NotNil(t, sshPolicyRouter) + require.Len(t, sshPolicyRouter.Rules, 1, "router should have 1 SSH rule (tag:router)") + + routerRule := sshPolicyRouter.Rules[0] + routerPrincipals := make([]string, len(routerRule.Principals)) + for i, p := range routerRule.Principals { + routerPrincipals[i] = p.NodeIP + } + require.Contains(t, routerPrincipals, "100.64.0.1", "router rule should include user1's device (unfiltered sources)") + require.Contains(t, routerPrincipals, "100.64.0.2", "router rule should include user1's other device (unfiltered sources)") + require.Contains(t, routerPrincipals, "100.64.0.3", "router rule should include user2's device (unfiltered sources)") +} diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 5191368a..bbde136e 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -2,6 +2,7 @@ package v2 import ( "net/netip" + "slices" "testing" "github.com/google/go-cmp/cmp" @@ -439,3 +440,82 @@ func TestAutogroupSelfReducedVsUnreducedRules(t *testing.T) { require.Empty(t, peerMap[node1.ID], "node1 should have no peers (can only reach itself)") require.Empty(t, peerMap[node2.ID], "node2 should have no peers") } + +// When separate ACL rules exist (one with autogroup:self, one with tag:router), +// the autogroup:self rule should not prevent the tag:router rule from working. +// This ensures that autogroup:self doesn't interfere with other ACL rules. +func TestAutogroupSelfWithOtherRules(t *testing.T) { + users := types.Users{ + {Model: gorm.Model{ID: 1}, Name: "test-1", Email: "test-1@example.com"}, + {Model: gorm.Model{ID: 2}, Name: "test-2", Email: "test-2@example.com"}, + } + + // test-1 has a regular device + test1Node := &types.Node{ + ID: 1, + Hostname: "test-1-device", + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + User: users[0], + UserID: users[0].ID, + Hostinfo: &tailcfg.Hostinfo{}, + } + + // test-2 has a router device with tag:node-router + test2RouterNode := &types.Node{ + ID: 2, + Hostname: "test-2-router", + IPv4: ap("100.64.0.2"), + IPv6: ap("fd7a:115c:a1e0::2"), + User: users[1], + UserID: users[1].ID, + ForcedTags: []string{"tag:node-router"}, + Hostinfo: &tailcfg.Hostinfo{}, + } + + nodes := types.Nodes{test1Node, test2RouterNode} + + // This matches the exact policy from issue #2838: + // - First rule: autogroup:member -> autogroup:self (allows users to see their own devices) + // - Second rule: group:home -> tag:node-router (should allow group members to see router) + policy := `{ + "groups": { + "group:home": ["test-1@example.com", "test-2@example.com"] + }, + "tagOwners": { + "tag:node-router": ["group:home"] + }, + "acls": [ + { + "action": "accept", + "src": ["autogroup:member"], + "dst": ["autogroup:self:*"] + }, + { + "action": "accept", + "src": ["group:home"], + "dst": ["tag:node-router:*"] + } + ] + }` + + pm, err := NewPolicyManager([]byte(policy), users, nodes.ViewSlice()) + require.NoError(t, err) + + peerMap := pm.BuildPeerMap(nodes.ViewSlice()) + + // test-1 (in group:home) should see: + // 1. Their own node (from autogroup:self rule) + // 2. The router node (from group:home -> tag:node-router rule) + test1Peers := peerMap[test1Node.ID] + + // Verify test-1 can see the router (group:home -> tag:node-router rule) + require.True(t, slices.ContainsFunc(test1Peers, func(n types.NodeView) bool { + return n.ID() == test2RouterNode.ID + }), "test-1 should see test-2's router via group:home -> tag:node-router rule, even when autogroup:self rule exists (issue #2838)") + + // Verify that test-1 has filter rules (including autogroup:self and tag:node-router access) + rules, err := pm.FilterForNode(test1Node.View()) + require.NoError(t, err) + require.NotEmpty(t, rules, "test-1 should have filter rules from both ACL rules") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1d450cb6..c340adc2 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -456,9 +456,9 @@ func (s *State) Connect(id types.NodeID) []change.ChangeSet { log.Info().Uint64("node.id", id.Uint64()).Str("node.name", node.Hostname()).Msg("Node connected") // Use the node's current routes for primary route update - // SubnetRoutes() returns only the intersection of announced AND approved routes - // We MUST use SubnetRoutes() to maintain the security model - routeChange := s.primaryRoutes.SetRoutes(id, node.SubnetRoutes()...) + // AllApprovedRoutes() returns only the intersection of announced AND approved routes + // We MUST use AllApprovedRoutes() to maintain the security model + routeChange := s.primaryRoutes.SetRoutes(id, node.AllApprovedRoutes()...) if routeChange { c = append(c, change.NodeAdded(id)) @@ -656,7 +656,7 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t // Update primary routes table based on SubnetRoutes (intersection of announced and approved). // The primary routes table is what the mapper uses to generate network maps, so updating it // here ensures that route changes are distributed to peers. - routeChange := s.primaryRoutes.SetRoutes(nodeID, nodeView.SubnetRoutes()...) + routeChange := s.primaryRoutes.SetRoutes(nodeID, nodeView.AllApprovedRoutes()...) // If routes changed or the changeset isn't already a full update, trigger a policy change // to ensure all nodes get updated network maps @@ -1711,7 +1711,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest } if needsRouteUpdate { - // SetNodeRoutes sets the active/distributed routes, so we must use SubnetRoutes() + // SetNodeRoutes sets the active/distributed routes, so we must use AllApprovedRoutes() // which returns only the intersection of announced AND approved routes. // Using AnnouncedRoutes() would bypass the security model and auto-approve everything. log.Debug(). @@ -1719,9 +1719,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest Uint64("node.id", id.Uint64()). Strs("announcedRoutes", util.PrefixesToString(updatedNode.AnnouncedRoutes())). Strs("approvedRoutes", util.PrefixesToString(updatedNode.ApprovedRoutes().AsSlice())). - Strs("subnetRoutes", util.PrefixesToString(updatedNode.SubnetRoutes())). + Strs("allApprovedRoutes", util.PrefixesToString(updatedNode.AllApprovedRoutes())). Msg("updating node routes for distribution") - nodeRouteChange = s.SetNodeRoutes(id, updatedNode.SubnetRoutes()...) + nodeRouteChange = s.SetNodeRoutes(id, updatedNode.AllApprovedRoutes()...) } _, policyChange, err := s.persistNodeToDB(updatedNode) diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 8cf40ced..c6429669 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -269,11 +269,19 @@ func (node *Node) Prefixes() []netip.Prefix { // node has any exit routes enabled. // If none are enabled, it will return nil. func (node *Node) ExitRoutes() []netip.Prefix { - if slices.ContainsFunc(node.SubnetRoutes(), tsaddr.IsExitRoute) { - return tsaddr.ExitRoutes() + var routes []netip.Prefix + + for _, route := range node.AnnouncedRoutes() { + if tsaddr.IsExitRoute(route) && slices.Contains(node.ApprovedRoutes, route) { + routes = append(routes, route) + } } - return nil + return routes +} + +func (node *Node) IsExitNode() bool { + return len(node.ExitRoutes()) > 0 } func (node *Node) IPsAsString() []string { @@ -311,9 +319,16 @@ func (node *Node) CanAccess(matchers []matcher.Match, node2 *Node) bool { return true } + // Check if the node has access to routes that might be part of a + // smaller subnet that is served from node2 as a subnet router. if matcher.DestsOverlapsPrefixes(node2.SubnetRoutes()...) { return true } + + // If the dst is "the internet" and node2 is an exit node, allow access. + if matcher.DestsIsTheInternet() && node2.IsExitNode() { + return true + } } return false @@ -440,16 +455,22 @@ func (node *Node) AnnouncedRoutes() []netip.Prefix { return node.Hostinfo.RoutableIPs } -// SubnetRoutes returns the list of routes that the node announces and are approved. +// SubnetRoutes returns the list of routes (excluding exit routes) that the node +// announces and are approved. // -// IMPORTANT: This method is used for internal data structures and should NOT be used -// for the gRPC Proto conversion. For Proto, SubnetRoutes must be populated manually -// with PrimaryRoutes to ensure it includes only routes actively served by the node. -// See the comment in Proto() method and the implementation in grpcv1.go/nodesToProto. +// IMPORTANT: This method is used for internal data structures and should NOT be +// used for the gRPC Proto conversion. For Proto, SubnetRoutes must be populated +// manually with PrimaryRoutes to ensure it includes only routes actively served +// by the node. See the comment in Proto() method and the implementation in +// grpcv1.go/nodesToProto. func (node *Node) SubnetRoutes() []netip.Prefix { var routes []netip.Prefix for _, route := range node.AnnouncedRoutes() { + if tsaddr.IsExitRoute(route) { + continue + } + if slices.Contains(node.ApprovedRoutes, route) { routes = append(routes, route) } @@ -463,6 +484,11 @@ func (node *Node) IsSubnetRouter() bool { return len(node.SubnetRoutes()) > 0 } +// AllApprovedRoutes returns the combination of SubnetRoutes and ExitRoutes +func (node *Node) AllApprovedRoutes() []netip.Prefix { + return append(node.SubnetRoutes(), node.ExitRoutes()...) +} + func (node *Node) String() string { return node.Hostname } @@ -653,6 +679,7 @@ func (node Node) DebugString() string { fmt.Fprintf(&sb, "\tApprovedRoutes: %v\n", node.ApprovedRoutes) fmt.Fprintf(&sb, "\tAnnouncedRoutes: %v\n", node.AnnouncedRoutes()) fmt.Fprintf(&sb, "\tSubnetRoutes: %v\n", node.SubnetRoutes()) + fmt.Fprintf(&sb, "\tExitRoutes: %v\n", node.ExitRoutes()) sb.WriteString("\n") return sb.String() @@ -678,27 +705,11 @@ func (v NodeView) InIPSet(set *netipx.IPSet) bool { } func (v NodeView) CanAccess(matchers []matcher.Match, node2 NodeView) bool { - if !v.Valid() || !node2.Valid() { + if !v.Valid() { return false } - src := v.IPs() - allowedIPs := node2.IPs() - for _, matcher := range matchers { - if !matcher.SrcsContainsIPs(src...) { - continue - } - - if matcher.DestsContainsIP(allowedIPs...) { - return true - } - - if matcher.DestsOverlapsPrefixes(node2.SubnetRoutes()...) { - return true - } - } - - return false + return v.ж.CanAccess(matchers, node2.AsStruct()) } func (v NodeView) CanAccessRoute(matchers []matcher.Match, route netip.Prefix) bool { @@ -730,6 +741,13 @@ func (v NodeView) IsSubnetRouter() bool { return v.ж.IsSubnetRouter() } +func (v NodeView) AllApprovedRoutes() []netip.Prefix { + if !v.Valid() { + return nil + } + return v.ж.AllApprovedRoutes() +} + func (v NodeView) AppendToIPSet(build *netipx.IPSetBuilder) { if !v.Valid() { return @@ -808,6 +826,13 @@ func (v NodeView) ExitRoutes() []netip.Prefix { return v.ж.ExitRoutes() } +func (v NodeView) IsExitNode() bool { + if !v.Valid() { + return false + } + return v.ж.IsExitNode() +} + // RequestTags returns the ACL tags that the node is requesting. func (v NodeView) RequestTags() []string { if !v.Valid() || !v.Hostinfo().Valid() { diff --git a/integration/acl_test.go b/integration/acl_test.go index 122eeea7..50924891 100644 --- a/integration/acl_test.go +++ b/integration/acl_test.go @@ -1611,37 +1611,170 @@ func TestACLAutogroupTagged(t *testing.T) { } // Test that only devices owned by the same user can access each other and cannot access devices of other users +// Test structure: +// - user1: 2 regular nodes (tests autogroup:self for same-user access) +// - user2: 2 regular nodes (tests autogroup:self for same-user access and cross-user isolation) +// - user-router: 1 node with tag:router-node (tests that autogroup:self doesn't interfere with other rules) func TestACLAutogroupSelf(t *testing.T) { IntegrationSkip(t) - scenario := aclScenario(t, - &policyv2.Policy{ - ACLs: []policyv2.ACL{ - { - Action: "accept", - Sources: []policyv2.Alias{ptr.To(policyv2.AutoGroupMember)}, - Destinations: []policyv2.AliasWithPorts{ - aliasWithPorts(ptr.To(policyv2.AutoGroupSelf), tailcfg.PortRangeAny), - }, + // Policy with TWO separate ACL rules: + // 1. autogroup:member -> autogroup:self (same-user access) + // 2. group:home -> tag:router-node (router access) + // This tests that autogroup:self doesn't prevent other rules from working + policy := &policyv2.Policy{ + Groups: policyv2.Groups{ + policyv2.Group("group:home"): []policyv2.Username{ + policyv2.Username("user1@"), + policyv2.Username("user2@"), + }, + }, + TagOwners: policyv2.TagOwners{ + policyv2.Tag("tag:router-node"): policyv2.Owners{ + usernameOwner("user-router@"), + }, + }, + ACLs: []policyv2.ACL{ + { + Action: "accept", + Sources: []policyv2.Alias{ptr.To(policyv2.AutoGroupMember)}, + Destinations: []policyv2.AliasWithPorts{ + aliasWithPorts(ptr.To(policyv2.AutoGroupSelf), tailcfg.PortRangeAny), + }, + }, + { + Action: "accept", + Sources: []policyv2.Alias{groupp("group:home")}, + Destinations: []policyv2.AliasWithPorts{ + aliasWithPorts(tagp("tag:router-node"), tailcfg.PortRangeAny), + }, + }, + { + Action: "accept", + Sources: []policyv2.Alias{tagp("tag:router-node")}, + Destinations: []policyv2.AliasWithPorts{ + aliasWithPorts(groupp("group:home"), tailcfg.PortRangeAny), }, }, }, - 2, - ) + } + + // Create custom scenario: user1 and user2 with regular nodes, plus user-router with tagged node + spec := ScenarioSpec{ + NodesPerUser: 2, + Users: []string{"user1", "user2"}, + } + + scenario, err := NewScenario(spec) + require.NoError(t, err) defer scenario.ShutdownAssertNoPanics(t) - err := scenario.WaitForTailscaleSyncWithPeerCount(1, integrationutil.PeerSyncTimeout(), integrationutil.PeerSyncRetryInterval()) + err = scenario.CreateHeadscaleEnv( + []tsic.Option{ + tsic.WithNetfilter("off"), + tsic.WithDockerEntrypoint([]string{ + "/bin/sh", + "-c", + "/bin/sleep 3 ; apk add python3 curl ; update-ca-certificates ; python3 -m http.server --bind :: 80 & tailscaled --tun=tsdev", + }), + tsic.WithDockerWorkdir("/"), + }, + hsic.WithACLPolicy(policy), + hsic.WithTestName("acl-autogroup-self"), + hsic.WithEmbeddedDERPServerOnly(), + hsic.WithTLS(), + ) require.NoError(t, err) + // Add router node for user-router (single shared router node) + networks := scenario.Networks() + var network *dockertest.Network + if len(networks) > 0 { + network = networks[0] + } + + headscale, err := scenario.Headscale() + require.NoError(t, err) + + routerUser, err := scenario.CreateUser("user-router") + require.NoError(t, err) + + authKey, err := scenario.CreatePreAuthKey(routerUser.GetId(), true, false) + require.NoError(t, err) + + // Create router node (tagged with tag:router-node) + routerClient, err := tsic.New( + scenario.Pool(), + "unstable", + tsic.WithCACert(headscale.GetCert()), + tsic.WithHeadscaleName(headscale.GetHostname()), + tsic.WithNetwork(network), + tsic.WithTags([]string{"tag:router-node"}), + tsic.WithNetfilter("off"), + tsic.WithDockerEntrypoint([]string{ + "/bin/sh", + "-c", + "/bin/sleep 3 ; apk add python3 curl ; update-ca-certificates ; python3 -m http.server --bind :: 80 & tailscaled --tun=tsdev", + }), + tsic.WithDockerWorkdir("/"), + ) + require.NoError(t, err) + + err = routerClient.WaitForNeedsLogin(integrationutil.PeerSyncTimeout()) + require.NoError(t, err) + + err = routerClient.Login(headscale.GetEndpoint(), authKey.GetKey()) + require.NoError(t, err) + + err = routerClient.WaitForRunning(integrationutil.PeerSyncTimeout()) + require.NoError(t, err) + + userRouterObj := scenario.GetOrCreateUser("user-router") + userRouterObj.Clients[routerClient.Hostname()] = routerClient + user1Clients, err := scenario.GetClients("user1") require.NoError(t, err) - user2Clients, err := scenario.GetClients("user2") require.NoError(t, err) - // Test that user1's devices can access each other + var user1Regular, user2Regular []TailscaleClient for _, client := range user1Clients { - for _, peer := range user1Clients { + status, err := client.Status() + require.NoError(t, err) + if status.Self != nil && (status.Self.Tags == nil || status.Self.Tags.Len() == 0) { + user1Regular = append(user1Regular, client) + } + } + for _, client := range user2Clients { + status, err := client.Status() + require.NoError(t, err) + if status.Self != nil && (status.Self.Tags == nil || status.Self.Tags.Len() == 0) { + user2Regular = append(user2Regular, client) + } + } + + require.NotEmpty(t, user1Regular, "user1 should have regular (untagged) devices") + require.NotEmpty(t, user2Regular, "user2 should have regular (untagged) devices") + require.NotNil(t, routerClient, "router node should exist") + + // Wait for all nodes to sync with their expected peer counts + // With our ACL policy: + // - Regular nodes (user1/user2): 1 same-user regular peer + 1 router-node = 2 peers + // - Router node: 2 user1 regular + 2 user2 regular = 4 peers + for _, client := range user1Regular { + err := client.WaitForPeers(2, integrationutil.PeerSyncTimeout(), integrationutil.PeerSyncRetryInterval()) + require.NoError(t, err, "user1 regular device %s should see 2 peers (1 same-user peer + 1 router)", client.Hostname()) + } + for _, client := range user2Regular { + err := client.WaitForPeers(2, integrationutil.PeerSyncTimeout(), integrationutil.PeerSyncRetryInterval()) + require.NoError(t, err, "user2 regular device %s should see 2 peers (1 same-user peer + 1 router)", client.Hostname()) + } + err = routerClient.WaitForPeers(4, integrationutil.PeerSyncTimeout(), integrationutil.PeerSyncRetryInterval()) + require.NoError(t, err, "router should see 4 peers (all group:home regular nodes)") + + // Test that user1's regular devices can access each other + for _, client := range user1Regular { + for _, peer := range user1Regular { if client.Hostname() == peer.Hostname() { continue } @@ -1656,13 +1789,13 @@ func TestACLAutogroupSelf(t *testing.T) { result, err := client.Curl(url) assert.NoError(c, err) assert.Len(c, result, 13) - }, 10*time.Second, 200*time.Millisecond, "user1 device should reach other user1 device") + }, 10*time.Second, 200*time.Millisecond, "user1 device should reach other user1 device via autogroup:self") } } - // Test that user2's devices can access each other - for _, client := range user2Clients { - for _, peer := range user2Clients { + // Test that user2's regular devices can access each other + for _, client := range user2Regular { + for _, peer := range user2Regular { if client.Hostname() == peer.Hostname() { continue } @@ -1677,36 +1810,64 @@ func TestACLAutogroupSelf(t *testing.T) { result, err := client.Curl(url) assert.NoError(c, err) assert.Len(c, result, 13) - }, 10*time.Second, 200*time.Millisecond, "user2 device should reach other user2 device") + }, 10*time.Second, 200*time.Millisecond, "user2 device should reach other user2 device via autogroup:self") } } - // Test that devices from different users cannot access each other - for _, client := range user1Clients { - for _, peer := range user2Clients { + // Test that user1's regular devices can access router-node + for _, client := range user1Regular { + fqdn, err := routerClient.FQDN() + require.NoError(t, err) + url := fmt.Sprintf("http://%s/etc/hostname", fqdn) + t.Logf("url from %s (user1) to %s (router-node) - should SUCCEED", client.Hostname(), fqdn) + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + result, err := client.Curl(url) + assert.NoError(c, err) + assert.NotEmpty(c, result, "user1 should be able to access router-node via group:home -> tag:router-node rule") + }, 10*time.Second, 200*time.Millisecond, "user1 device should reach router-node (proves autogroup:self doesn't interfere)") + } + + // Test that user2's regular devices can access router-node + for _, client := range user2Regular { + fqdn, err := routerClient.FQDN() + require.NoError(t, err) + url := fmt.Sprintf("http://%s/etc/hostname", fqdn) + t.Logf("url from %s (user2) to %s (router-node) - should SUCCEED", client.Hostname(), fqdn) + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + result, err := client.Curl(url) + assert.NoError(c, err) + assert.NotEmpty(c, result, "user2 should be able to access router-node via group:home -> tag:router-node rule") + }, 10*time.Second, 200*time.Millisecond, "user2 device should reach router-node (proves autogroup:self doesn't interfere)") + } + + // Test that devices from different users cannot access each other's regular devices + for _, client := range user1Regular { + for _, peer := range user2Regular { fqdn, err := peer.FQDN() require.NoError(t, err) url := fmt.Sprintf("http://%s/etc/hostname", fqdn) - t.Logf("url from %s (user1) to %s (user2) - should FAIL", client.Hostname(), fqdn) + t.Logf("url from %s (user1) to %s (user2 regular) - should FAIL", client.Hostname(), fqdn) result, err := client.Curl(url) - assert.Empty(t, result, "user1 should not be able to access user2's devices with autogroup:self") - assert.Error(t, err, "connection from user1 to user2 should fail") + assert.Empty(t, result, "user1 should not be able to access user2's regular devices (autogroup:self isolation)") + assert.Error(t, err, "connection from user1 to user2 regular device should fail") } } - for _, client := range user2Clients { - for _, peer := range user1Clients { + for _, client := range user2Regular { + for _, peer := range user1Regular { fqdn, err := peer.FQDN() require.NoError(t, err) url := fmt.Sprintf("http://%s/etc/hostname", fqdn) - t.Logf("url from %s (user2) to %s (user1) - should FAIL", client.Hostname(), fqdn) + t.Logf("url from %s (user2) to %s (user1 regular) - should FAIL", client.Hostname(), fqdn) result, err := client.Curl(url) - assert.Empty(t, result, "user2 should not be able to access user1's devices with autogroup:self") - assert.Error(t, err, "connection from user2 to user1 should fail") + assert.Empty(t, result, "user2 should not be able to access user1's regular devices (autogroup:self isolation)") + assert.Error(t, err, "connection from user2 to user1 regular device should fail") } } }