starred/headscale
1
0
Fork 0
mirror of https://github.com/juanfont/headscale.git synced 2026-01-11 11:50:30 +01:00
Code Issues 98 Packages Projects Releases 10 Wiki Activity

[PR #2852] [CLOSED] Fix exit node visibility - enforce autogroup:internet ACL requirement #2896

New Issue
Closed
opened 2025-12-29 04:19:33 +01:00 by adam · 0 comments
adam commented 2025-12-29 04:19:33 +01:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/juanfont/headscale/pull/2852
Author: @Copilot
Created: 11/1/2025
Status: ❌ Closed

Base: main ← Head: copilot/investigate-visibility-issue-2788

📝 Commits (3)

  • 309437f Initial plan
  • 31bf3a6 Fix exit node visibility issue - filter based on autogroup:internet permission
  • 4aa9292 Address code review feedback - clarify comments and logic

📊 Changes

6 files changed (+427 additions, -7 deletions)

View changed files

📝 go.sum (+0 -6)
📝 hscontrol/mapper/builder.go (+67 -0)
➕ hscontrol/mapper/exit_node_visibility_test.go (+336 -0)
📝 hscontrol/mapper/tail.go (+7 -1)
📝 hscontrol/mapper/tail_test.go (+10 -0)
📝 hscontrol/policy/matcher/matcher.go (+7 -0)

📄 Description

Exit nodes were visible to all nodes regardless of ACL permissions. A node with ACL mobile -> server:80 could see exit nodes in tailscale exit-node list, even without autogroup:internet permission.

Root cause: tailNode() unconditionally appended ExitRoutes() to peer AllowedIPs, making exit routes visible independent of ACL evaluation.

Changes

Modified exit route inclusion to respect ACL permissions:

  • Added exitRouteFilterFunc parameter to tailNode() and tailNodes() (parallel to existing primaryRouteFunc)
  • Self node: Always includes own advertised exit routes
  • Peer nodes: Only include exit routes if requesting node has internet access permission

Internet access detection:

  • Added canUseExitRoutes() helper that checks if ACL grants access to public IPs (1.1.1.1, 8.8.8.8, 208.67.222.222)
  • This distinguishes autogroup:internet (resolves to public internet) from specific service ACLs (e.g., 100.64.0.2:80)
  • Cannot use CanAccessRoute() directly - it checks prefix overlap, making any destination "overlap" with 0.0.0.0/0

Supporting changes:

  • Added DestsContainsPrefixes() method to matcher.Match for strict prefix containment checking (unused in final implementation but available for future use)

Example

Before (incorrect):

// mobile has ACL: mobile -> server:80 (NO autogroup:internet)
// mobile's peer list includes exit node with AllowedIPs: [0.0.0.0/0, 100.64.0.3/32]

After (correct):

// mobile's peer list includes exit node with AllowedIPs: [100.64.0.3/32]
// Exit routes only included if ACL contains autogroup:internet

Fixes #2788

Original prompt

This is a complicated visibility issue, investigate https://github.com/juanfont/headscale/issues/2788, it is vital that you look at all the user added content

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/juanfont/headscale/pull/2852 **Author:** [@Copilot](https://github.com/apps/copilot-swe-agent) **Created:** 11/1/2025 **Status:** ❌ Closed **Base:** `main` ← **Head:** `copilot/investigate-visibility-issue-2788` --- ### 📝 Commits (3) - [`309437f`](https://github.com/juanfont/headscale/commit/309437fa14646c4f32f7f77d81360eedbf21cf83) Initial plan - [`31bf3a6`](https://github.com/juanfont/headscale/commit/31bf3a6637d0425d4be7e5b63c6044c46f151a97) Fix exit node visibility issue - filter based on autogroup:internet permission - [`4aa9292`](https://github.com/juanfont/headscale/commit/4aa9292b917d0a20627818e9fbff4de8413e0aff) Address code review feedback - clarify comments and logic ### 📊 Changes **6 files changed** (+427 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `go.sum` (+0 -6) 📝 `hscontrol/mapper/builder.go` (+67 -0) ➕ `hscontrol/mapper/exit_node_visibility_test.go` (+336 -0) 📝 `hscontrol/mapper/tail.go` (+7 -1) 📝 `hscontrol/mapper/tail_test.go` (+10 -0) 📝 `hscontrol/policy/matcher/matcher.go` (+7 -0) </details> ### 📄 Description Exit nodes were visible to all nodes regardless of ACL permissions. A node with ACL `mobile -> server:80` could see exit nodes in `tailscale exit-node list`, even without `autogroup:internet` permission. **Root cause:** `tailNode()` unconditionally appended `ExitRoutes()` to peer `AllowedIPs`, making exit routes visible independent of ACL evaluation. ## Changes **Modified exit route inclusion to respect ACL permissions:** - Added `exitRouteFilterFunc` parameter to `tailNode()` and `tailNodes()` (parallel to existing `primaryRouteFunc`) - Self node: Always includes own advertised exit routes - Peer nodes: Only include exit routes if requesting node has internet access permission **Internet access detection:** - Added `canUseExitRoutes()` helper that checks if ACL grants access to public IPs (1.1.1.1, 8.8.8.8, 208.67.222.222) - This distinguishes `autogroup:internet` (resolves to public internet) from specific service ACLs (e.g., `100.64.0.2:80`) - Cannot use `CanAccessRoute()` directly - it checks prefix overlap, making any destination "overlap" with `0.0.0.0/0` **Supporting changes:** - Added `DestsContainsPrefixes()` method to `matcher.Match` for strict prefix containment checking (unused in final implementation but available for future use) ## Example Before (incorrect): ```go // mobile has ACL: mobile -> server:80 (NO autogroup:internet) // mobile's peer list includes exit node with AllowedIPs: [0.0.0.0/0, 100.64.0.3/32] ``` After (correct): ```go // mobile's peer list includes exit node with AllowedIPs: [100.64.0.3/32] // Exit routes only included if ACL contains autogroup:internet ``` Fixes #2788 <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > This is a complicated visibility issue, investigate https://github.com/juanfont/headscale/issues/2788, it is vital that you look at all the user added content </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
adam added the pull-request label 2025-12-29 04:19:33 +01:00
adam closed this issue 2025-12-29 04:19:33 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
CLI
DERP
DNS
Nix
OIDC
SSH
bug
database
documentation
duplicate
enhancement
faq
good first issue
grants
help wanted
might-come
needs design doc
needs investigation
no-stale-bot
out of scope
performance
policy 📝
pull-request
Mirrored from GitHub Pull Request
 question
regression
routes
stale
tags
tailscale-feature-gap
well described ❤️
wontfix
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/headscale#2896
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?