From 4aa9292b917d0a20627818e9fbff4de8413e0aff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Nov 2025 08:59:28 +0000 Subject: [PATCH] Address code review feedback - clarify comments and logic - Fixed DestsContainsPrefixes comment to accurately describe behavior (checks if ANY prefix is contained) - Enhanced canUseExitRoutes documentation to explain why checking ANY sample public IP is sufficient - Clarified that DestsContainsIP variadic behavior is intentional and correct for internet access detection No code logic changes, only documentation improvements. Co-authored-by: kradalby <98431+kradalby@users.noreply.github.com> --- go.sum | 6 ------ hscontrol/mapper/builder.go | 17 +++++++++++------ hscontrol/policy/matcher/matcher.go | 6 +++--- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/go.sum b/go.sum index e78e9aff..39858cdc 100644 --- a/go.sum +++ b/go.sum @@ -124,8 +124,6 @@ github.com/creachadair/command v0.2.0 h1:qTA9cMMhZePAxFoNdnk6F6nn94s1qPndIg9hJbq github.com/creachadair/command v0.2.0/go.mod h1:j+Ar+uYnFsHpkMeV9kGj6lJ45y9u2xqtg8FYy6cm+0o= github.com/creachadair/flax v0.0.5 h1:zt+CRuXQASxwQ68e9GHAOnEgAU29nF0zYMHOCrL5wzE= github.com/creachadair/flax v0.0.5/go.mod h1:F1PML0JZLXSNDMNiRGK2yjm5f+L9QCHchyHBldFymj8= -github.com/creachadair/mds v0.25.2 h1:xc0S0AfDq5GX9KUR5sLvi5XjA61/P6S5e0xFs1vA18Q= -github.com/creachadair/mds v0.25.2/go.mod h1:+s4CFteFRj4eq2KcGHW8Wei3u9NyzSPzNV32EvjyK/Q= github.com/creachadair/mds v0.25.10 h1:9k9JB35D1xhOCFl0liBhagBBp8fWWkKZrA7UXsfoHtA= github.com/creachadair/mds v0.25.10/go.mod h1:4hatI3hRM+qhzuAmqPRFvaBM8mONkS7nsLxkcuTYUIs= github.com/creachadair/taskgroup v0.13.2 h1:3KyqakBuFsm3KkXi/9XIb0QcA8tEzLHLgaoidf0MdVc= @@ -278,8 +276,6 @@ github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfC github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= github.com/jsimonetti/rtnetlink v1.4.1 h1:JfD4jthWBqZMEffc5RjgmlzpYttAVw1sdnmiNaPO3hE= github.com/jsimonetti/rtnetlink v1.4.1/go.mod h1:xJjT7t59UIZ62GLZbv6PLLo8VFrostJMPBAheR6OM8w= -github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo= -github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ= github.com/klauspost/compress v1.18.1 h1:bcSGx7UbpBqMChDtsF28Lw6v/G94LPrrbMbdC3JH2co= github.com/klauspost/compress v1.18.1/go.mod h1:ZQFFVG+MdnR0P+l6wpXgIL4NTtwiKIdBnrBd8Nrxr+0= github.com/klauspost/cpuid/v2 v2.0.9/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg= @@ -463,8 +459,6 @@ github.com/tailscale/peercred v0.0.0-20250107143737-35a0c7bd7edc h1:24heQPtnFR+y github.com/tailscale/peercred v0.0.0-20250107143737-35a0c7bd7edc/go.mod h1:f93CXfllFsO9ZQVq+Zocb1Gp4G5Fz0b0rXHLOzt/Djc= github.com/tailscale/setec v0.0.0-20250305161714-445cadbbca3d h1:mnqtPWYyvNiPU9l9tzO2YbHXU/xV664XthZYA26lOiE= github.com/tailscale/setec v0.0.0-20250305161714-445cadbbca3d/go.mod h1:9BzmlFc3OLqLzLTF/5AY+BMs+clxMqyhSGzgXIm8mNI= -github.com/tailscale/squibble v0.0.0-20250108170732-a4ca58afa694 h1:95eIP97c88cqAFU/8nURjgI9xxPbD+Ci6mY/a79BI/w= -github.com/tailscale/squibble v0.0.0-20250108170732-a4ca58afa694/go.mod h1:veguaG8tVg1H/JG5RfpoUW41I+O8ClPElo/fTYr8mMk= github.com/tailscale/squibble v0.0.0-20251030164342-4d5df9caa993 h1:FyiiAvDAxpB0DrW2GW3KOVfi3YFOtsQUEeFWbf55JJU= github.com/tailscale/squibble v0.0.0-20251030164342-4d5df9caa993/go.mod h1:xJkMmR3t+thnUQhA3Q4m2VSlS5pcOq+CIjmU/xfKKx4= github.com/tailscale/tailsql v0.0.0-20250421235516-02f85f087b97 h1:JJkDnrAhHvOCttk8z9xeZzcDlzzkRA7+Duxj9cwOyxk= diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index 6dde4299..109c9ac3 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -20,16 +20,20 @@ import ( // // 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. +// +// The function tests if the ACL grants access to well-known public DNS servers. +// If any of these are accessible, it indicates the ACL grants broad internet access +// (as opposed to just specific private services), which is sufficient for exit node usage. 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. + // If the ACL grants access to any of these well-known public IPs, it indicates + // broad internet access (e.g., via autogroup:internet) rather than just access + // to specific private services. samplePublicIPs := []netip.Addr{ - netip.MustParseAddr("1.1.1.1"), // Cloudflare DNS - netip.MustParseAddr("8.8.8.8"), // Google DNS + netip.MustParseAddr("1.1.1.1"), // Cloudflare DNS + netip.MustParseAddr("8.8.8.8"), // Google DNS netip.MustParseAddr("208.67.222.222"), // OpenDNS } @@ -40,7 +44,8 @@ func canUseExitRoutes(node types.NodeView, matchers []matcher.Match) bool { continue } - // Check if the destination includes public internet IPs. + // Check if the destination includes any public internet IPs. + // DestsContainsIP returns true if ANY of the provided IPs is in the destination set. // 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...) { diff --git a/hscontrol/policy/matcher/matcher.go b/hscontrol/policy/matcher/matcher.go index 1e7074ac..751d5971 100644 --- a/hscontrol/policy/matcher/matcher.go +++ b/hscontrol/policy/matcher/matcher.go @@ -92,9 +92,9 @@ 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. +// DestsContainsPrefixes checks if the destination IPSet contains any of the given prefixes. +// Returns true if at least one prefix is fully contained in the destination IPSet. +// This is more strict than DestsOverlapsPrefixes which only requires overlap. func (m *Match) DestsContainsPrefixes(prefixes ...netip.Prefix) bool { return slices.ContainsFunc(prefixes, m.dests.ContainsPrefix) }