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>
This commit is contained in:
copilot-swe-agent[bot]
2025-11-01 08:59:28 +00:00
parent 31bf3a6637
commit 4aa9292b91
3 changed files with 14 additions and 15 deletions

View File

@@ -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...) {

View File

@@ -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)
}