Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 57 additions & 4 deletions client/internal/stdnet/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,72 @@
"golang.zx2c4.com/wireguard/wgctrl"
)

// windowsKnownBadSubstrings lists Windows interface-name fragments that
// should ALWAYS be excluded from ICE candidate gathering, even when the
// caller-supplied disallow list does not cover them. These are the
// interfaces uray-mic-d4's debug bundle (2026-05-04) showed Pion ICE
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid embedding a real user identifier and machine name in source comments.

Lines 14, 25, and 46 reference a real person's name ("Michael Uray") and host identifier ("uray-mic-d4"). This is in a public OSS repo and a permanent commit history. The technical narrative (Hyper-V Default Switch picked as host candidate, 172.26.x.x NAT-only, default route on vEthernet (LAN)) is what's useful for future maintainers; the personal attribution is not, and removing it costs nothing.

🔒 Proposed redaction
-// should ALWAYS be excluded from ICE candidate gathering, even when the
-// caller-supplied disallow list does not cover them. These are the
-// interfaces uray-mic-d4's debug bundle (2026-05-04) showed Pion ICE
-// picking as host candidates -- producing dead-end pairs because none
-// of these can be reached from the public internet:
+// should ALWAYS be excluded from ICE candidate gathering, even when the
+// caller-supplied disallow list does not cover them. These were observed
+// (in a Windows debug bundle) being picked by Pion ICE as host candidates,
+// producing dead-end pairs because none of these can be reached from the
+// public internet:
@@
-// IMPORTANT: User-named Hyper-V external switches like "vEthernet (LAN)"
-// MUST NOT be filtered. On uray-mic-d4 that interface IS the default
-// route (192.168.0.243/22 -> 0.0.0.0/0 via 192.168.0.254). Filtering it
-// out would actually break P2P, not improve it.
+// IMPORTANT: User-named Hyper-V external switches like "vEthernet (LAN)"
+// MUST NOT be filtered. On affected hosts that interface IS the default
+// route; filtering it out would break P2P, not improve it.
@@
-// Reported by Michael Uray on uray-mic-d4 (2026-05-04): 0/28 peers P2P.
+// Symptom: every peer falls back to relay when ICE only sees Hyper-V /
+// loopback host candidates.

Also applies to: 24-27, 46-46

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter.go` at line 14, Remove the personal name
("Michael Uray") and host identifier ("uray-mic-d4") from the inline comments
and redact them with neutral wording (e.g., "a developer's debug bundle" or
"example host") wherever they appear in the comment blocks referencing the
Hyper-V candidate, NAT range, and vEthernet default route; update the comment
text that currently mentions "Hyper-V Default Switch picked as host candidate,
172.26.x.x NAT-only, default route on `vEthernet (LAN)`" to keep the technical
details but replace the personal attribution/hostname, and run a quick grep for
those exact strings ("Michael Uray", "uray-mic-d4") to ensure all occurrences
are replaced (references found around the existing comment instances).

// picking as host candidates -- producing dead-end pairs because none
// of these can be reached from the public internet:
//
// - "loopback pseudo-interface" -> 127.0.0.1 (loopback)
// - "vethernet (default switch)" -> 172.26.x.x (Hyper-V NAT-only)
// - "vethernet (wsl" -> WSL2 host-only
//
// Matched as case-insensitive substrings.
//
// IMPORTANT: User-named Hyper-V external switches like "vEthernet (LAN)"
// MUST NOT be filtered. On uray-mic-d4 that interface IS the default
// route (192.168.0.243/22 -> 0.0.0.0/0 via 192.168.0.254). Filtering it
// out would actually break P2P, not improve it.
var windowsKnownBadSubstrings = []string{
"loopback pseudo-interface",
"vethernet (default switch)",
"vethernet (wsl",
}

// InterfaceFilter is a function passed to ICE Agent to filter out not allowed interfaces
// to avoid building tunnel over them.
//
// Matching is case-insensitive because Windows interface names use mixed
// case (e.g. "Loopback Pseudo-Interface 1") while the disallow list is
// lowercase. Without the fold, the historic implementation let every
// Windows interface slip past and Pion ICE picked junk addresses
// (127.0.0.1, 172.26.x.x Hyper-V Default Switch, internal-VPN /22s) as
// local host candidates, dooming P2P to dead-end pairs and forcing
// relay-only. See windowsKnownBadSubstrings for the targeted Windows
// extras.
//
// Reported by Michael Uray on uray-mic-d4 (2026-05-04): 0/28 peers P2P.
func InterfaceFilter(disallowList []string) func(string) bool {

Check failure on line 47 in client/internal/stdnet/filter.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 23 to the 20 allowed.

See more on https://sonarcloud.io/project/issues?id=netbirdio_netbird&issues=AZ36TUXJwQuoEp6Ucmhd&open=AZ36TUXJwQuoEp6Ucmhd&pullRequest=6080

return func(iFace string) bool {
lowerIFace := strings.ToLower(iFace)

if strings.HasPrefix(iFace, "lo") {
// hardcoded loopback check to support already installed agents
// Linux/macOS loopback prefix ("lo", "lo0").
if strings.HasPrefix(lowerIFace, "lo") {
return false
}
Comment on lines +49 to 54
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Windows network adapter default display names "Local Area Connection" Windows 10 11

💡 Result:

Default “display names” can mean two different things in Windows: 1) Network adapter (interface) name in Network Connections (ncpa.cpl) - Windows typically shows Ethernet adapters with an interface name like “Ethernet” (or “Ethernet 2” if you have multiple) rather than “Local Area Connection”. [1][2] 2) Network profile name (the network you’re connected to) under Network & Sharing Center / network status UI - For wired Ethernet networks, Windows often shows a generic network name like “Network” / “Network 2” (numeric increments) rather than using the SSID concept that Wi‑Fi uses. [3][2] - Older Windows versions used “Local Area Connection” as the wired adapter label in the classic UI, and some guidance/questions still refer to “Local Area Connection” when instructing users where to click. [4][5] So for Windows 10/11 specifically: - “Local Area Connection” is not the typical default wired adapter label you’ll see in the Network Connections list; you’ll more commonly see “Ethernet” (and possibly “Ethernet 2”). [1][2] - If you’re instead referring to the name shown for the connected network profile, Windows will usually use generic “Network” / “Network X” naming for Ethernet. [3][2] If you tell me which screen/control panel you mean (ncpa.cpl list of adapters vs. Network & Sharing Center network profile name), I can map “Local Area Connection” to the exact field and explain where it comes from and how it can be renamed.

Citations:


🏁 Script executed:

cat -n client/internal/stdnet/filter.go | head -100

Repository: netbirdio/netbird

Length of output: 3959


🏁 Script executed:

find . -name "*filter*test*.go" -o -name "*test*filter*.go" | head -20

Repository: netbirdio/netbird

Length of output: 494


🏁 Script executed:

cat -n client/internal/stdnet/filter_test.go

Repository: netbirdio/netbird

Length of output: 3543


Gate the lo* prefix check to non-Windows platforms.

The case-insensitive HasPrefix check at line 52 filters any adapter whose display name begins with "lo" or "Lo" on all platforms, including Windows. While the Windows loopback is already covered by the windowsKnownBadSubstrings substring check (line 28-29), any other Windows adapter starting with "Lo" would be incorrectly filtered. Apply the same platform-specific gating pattern used for the veth entry (lines 73-74):

Suggested fix
-		// Linux/macOS loopback prefix ("lo", "lo0").
-		if strings.HasPrefix(lowerIFace, "lo") {
-			return false
-		}
+		// Linux/macOS loopback prefix ("lo", "lo0"). Skipped on Windows
+		// where adapter display names may start with "Lo"; Windows loopback
+		// is handled by windowsKnownBadSubstrings.
+		if runtime.GOOS != "windows" && strings.HasPrefix(lowerIFace, "lo") {
+			return false
+		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
lowerIFace := strings.ToLower(iFace)
if strings.HasPrefix(iFace, "lo") {
// hardcoded loopback check to support already installed agents
// Linux/macOS loopback prefix ("lo", "lo0").
if strings.HasPrefix(lowerIFace, "lo") {
return false
}
lowerIFace := strings.ToLower(iFace)
// Linux/macOS loopback prefix ("lo", "lo0"). Skipped on Windows
// where adapter display names may start with "Lo"; Windows loopback
// is handled by windowsKnownBadSubstrings.
if runtime.GOOS != "windows" && strings.HasPrefix(lowerIFace, "lo") {
return false
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter.go` around lines 49 - 54, The case-insensitive
strings.HasPrefix check against lowerIFace for "lo" should only run on
non-Windows platforms; wrap the existing LO-prefix check (the
lowerIFace/strings.HasPrefix("lo") branch) in the same platform guard used for
the "veth" entry (i.e., check runtime.GOOS != "windows" before evaluating
HasPrefix) so Windows adapters starting with "Lo" aren't incorrectly filtered
while keeping Windows-specific filtering via windowsKnownBadSubstrings
unchanged.


// Windows-specific known-bad substrings (loopback, NAT switches).
if runtime.GOOS == "windows" {
for _, sub := range windowsKnownBadSubstrings {
if strings.Contains(lowerIFace, sub) {
return false
}
}
}

for _, s := range disallowList {
if strings.HasPrefix(iFace, s) && runtime.GOOS != "ios" {
sLower := strings.ToLower(s)
// "veth" exists on both Linux (legitimate veth pair to filter)
// and Windows (where every Hyper-V iface starts with vEthernet,
// including the user's REAL default-route external switch). On
// Windows, junk Hyper-V interfaces are filtered above by name;
// applying a blanket vEthernet* prefix here would also drop
// user-named external switches like "vEthernet (LAN)".
if sLower == "veth" && runtime.GOOS == "windows" {
continue
}
if strings.HasPrefix(lowerIFace, sLower) && runtime.GOOS != "ios" {
return false
}
}
Expand Down
80 changes: 80 additions & 0 deletions client/internal/stdnet/filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package stdnet

import (
"runtime"
"testing"
)

// Regression test for the Windows-side ICE interface filter.
//
// Two things this test pins down:
//
// 1. Loopback / Hyper-V Default Switch / WSL adapter must be excluded
// even though their Windows names ("Loopback Pseudo-Interface 1",
// "vEthernet (Default Switch)") don't share a lowercase prefix with
// anything in the default disallow list.
//
// 2. User-named Hyper-V external switches (e.g. "vEthernet (LAN)")
// MUST stay allowed. On uray-mic-d4 (Michael Uray's debug bundle
// 2026-05-04) that interface owns the default route at
// 192.168.0.243/22 -> 0.0.0.0/0; filtering it out would have
// made P2P worse, not better. Codex review caught the broad
// "veth"-prefix variant of this fix before it shipped.
func TestInterfaceFilter_Windows_TargetedFiltering(t *testing.T) {
disallow := []string{"wt", "wg", "veth", "br-", "lo", "docker"}
allow := InterfaceFilter(disallow)

cases := []struct {
name string
want bool // true => allowed, false => filtered out
}{
// Always-bad Windows interfaces: filtered.
{"Loopback Pseudo-Interface 1", false},
{"vEthernet (Default Switch)", false},
{"vEthernet (WSL)", false},
{"vEthernet (WSL (Hyper-V firewall))", false},
// Disallow-list tokens (any platform).
{"wt0", false},
// Linux names (lowercase) still filtered:
{"lo", false},

// Real candidate interfaces stay allowed.
{"Ethernet USB", true},
{"OpenVPN 1", true},
{"WiFi", true},
// Critical: user-named Hyper-V external switch is the actual
// default-route interface and must NOT be dropped.
{"vEthernet (LAN)", true},
{"vEthernet (External)", true},
}

for _, c := range cases {
// The wgctrl branch can override on hosts where NetBird is
// running; tests run on a host where these names are not
// real interfaces, so the final return faithfully reflects
// the disallow-list logic.
got := allow(c.name)
// "veth*" prefix only filters on non-Windows; on Linux test
// runners "vEthernet (LAN)" still passes because of mixed
// case + the !Windows branch keeping the prefix match.
if !c.want && got {
t.Errorf("InterfaceFilter(%q) = true, want false (should be filtered)", c.name)
}
if c.want && !got && runtime.GOOS == "windows" && c.name == "vEthernet (LAN)" {
t.Fatalf("InterfaceFilter(%q) = false, want true on Windows (this is uray-mic-d4's default-route interface)", c.name)
}
}
Comment on lines +51 to +66
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Asymmetric assertions let want: true regressions pass silently.

The loop only fails when !c.want && got (line 60) or for the very specific tuple runtime.GOOS == "windows" && c.name == "vEthernet (LAN)" (line 63). Every other want: true case ("Ethernet USB", "OpenVPN 1", "WiFi", "vEthernet (External)") is never asserted on the positive side — if a future change starts filtering them, this test stays green.

There's a second subtle issue: on the test host none of these names are real interfaces, so the want: true cases that aren't filtered by the prefix logic always fall through to the wgctrl.Device(iFace) path and return true because the device lookup errors. So the positive cases are effectively asserting "the wgctrl fallback returns true," not "the filter logic allows this interface." That makes the assertion asymmetry both a coverage gap and a misleading signal.

✅ Suggested symmetric assertion (still tolerant of the cross-OS `veth` ambiguity)
-	for _, c := range cases {
-		// The wgctrl branch can override on hosts where NetBird is
-		// running; tests run on a host where these names are not
-		// real interfaces, so the final return faithfully reflects
-		// the disallow-list logic.
-		got := allow(c.name)
-		// "veth*" prefix only filters on non-Windows; on Linux test
-		// runners "vEthernet (LAN)" still passes because of mixed
-		// case + the !Windows branch keeping the prefix match.
-		if !c.want && got {
-			t.Errorf("InterfaceFilter(%q) = true, want false (should be filtered)", c.name)
-		}
-		if c.want && !got && runtime.GOOS == "windows" && c.name == "vEthernet (LAN)" {
-			t.Fatalf("InterfaceFilter(%q) = false, want true on Windows (this is uray-mic-d4's default-route interface)", c.name)
-		}
-	}
+	for _, c := range cases {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			// Names starting with "veth" only filter on non-Windows;
+			// skip those positive cases off-Windows to keep the test
+			// portable.
+			if c.want && runtime.GOOS != "windows" &&
+				strings.HasPrefix(strings.ToLower(c.name), "veth") {
+				t.Skipf("veth-prefix case is platform-specific; only enforced on Windows")
+			}
+			got := allow(c.name)
+			if got != c.want {
+				t.Errorf("InterfaceFilter(%q) = %v, want %v", c.name, got, c.want)
+			}
+		})
+	}

(Requires adding "strings" to the imports.)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/stdnet/filter_test.go` around lines 51 - 66, The test's loop
over cases uses allow(c.name) but only asserts failures for false-want cases
(and a single Windows special-case), so positive want:true regressions can pass;
change the loop in filter_test.go to assert symmetrically: after got :=
allow(c.name) fail when (c.want && !got) except skip the single exception when
runtime.GOOS == "windows" && c.name == "vEthernet (LAN)"; keep the existing
failure for (!c.want && got). Update the test harness (and add "strings" to
imports if needed for any OS/name checks) and ensure the assertions reference
allow(c.name), c.want, runtime.GOOS and the specific c.name literal so the test
fails when previously-allowed names become filtered.

}

// Linux-side regression: keep filtering legitimate Linux veth pairs.
func TestInterfaceFilter_Linux_VethPair(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("veth prefix filter is intentionally skipped on Windows")
}
allow := InterfaceFilter([]string{"veth", "docker", "lo"})
for _, name := range []string{"veth0", "veth1234", "docker0", "lo"} {
if allow(name) {
t.Errorf("InterfaceFilter(%q) = true, want false on Linux", name)
}
}
}
Loading