diff --git a/client/iface/bind/ice_bind_test.go b/client/iface/bind/ice_bind_test.go index 0b8db7640..8dd82d7df 100644 --- a/client/iface/bind/ice_bind_test.go +++ b/client/iface/bind/ice_bind_test.go @@ -2,17 +2,22 @@ package bind import ( "fmt" + "io" "net" "net/netip" "sync" + "sync/atomic" "testing" "time" + "github.com/pion/transport/v3" "github.com/pion/transport/v3/stdnet" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" + "golang.zx2c4.com/wireguard/device" + "golang.zx2c4.com/wireguard/tun/netstack" "github.com/netbirdio/netbird/client/iface/wgaddr" ) @@ -326,3 +331,86 @@ func listenUDP(t *testing.T, network, addr string) *net.UDPConn { require.NoError(t, err) return conn } + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface + before func() +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + if n.before != nil { + n.before() + } + return n.ifaces, nil +} + +func TestICEBind_OpenDoesNotDiscoverInterfaces(t *testing.T) { + n := newCountingNet() + iceBind := NewICEBind(n, testAddress(), 1280) + + for i := 0; i < 2; i++ { + _, port, err := iceBind.Open(0) + require.NoError(t, err) + require.NotZero(t, port, "the bind listens on a port") + assert.Equal(t, int64(0), n.calls.Load(), "opening the bind must not enumerate interfaces, attempt %d", i+1) + require.NoError(t, iceBind.Close()) + } + + _, _, err := iceBind.Open(0) + require.NoError(t, err) + t.Cleanup(func() { _ = iceBind.Close() }) + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "listen addresses come from the discovered interfaces") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered when the listen addresses are asked for") +} + +func TestICEBind_DeviceUpDoesNotWaitOnInterfaceDiscovery(t *testing.T) { + tunDev, _, err := netstack.CreateNetTUN([]netip.Addr{netip.MustParseAddr("100.64.0.1")}, nil, 1280) + require.NoError(t, err) + + var dev *device.Device + n := newCountingNet() + n.before = func() { + _ = dev.IpcGetOperation(io.Discard) + } + iceBind := NewICEBind(n, testAddress(), 1280) + dev = device.NewDevice(tunDev, iceBind, device.NewLogger(device.LogLevelSilent, "")) + + upDone := make(chan error, 1) + go func() { + upDone <- dev.Up() + }() + + select { + case err := <-upDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("device Up did not return: interface discovery ran under the device lock and waited on the device itself") + } + t.Cleanup(dev.Close) + + assert.Equal(t, int64(0), n.calls.Load(), "bringing the device up must not enumerate interfaces") + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "discovery that talks to the device succeeds once the device is up") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered outside the device lock") +} + +func newCountingNet() *countingNet { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: "eth0", Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: net.IPv4(192, 168, 1, 10), Mask: net.CIDRMask(24, 32)}) + return &countingNet{ifaces: []*transport.Interface{ifc}} +} + +func testAddress() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.64.0.1"), + Network: netip.MustParsePrefix("100.64.0.0/10"), + } +} diff --git a/client/iface/udpmux/mux.go b/client/iface/udpmux/mux.go index 3aa0b4d88..2f7287b53 100644 --- a/client/iface/udpmux/mux.go +++ b/client/iface/udpmux/mux.go @@ -47,6 +47,7 @@ type SingleSocketUDPMux struct { // for UDP connection listen at unspecified address localAddrsForUnspecified []net.Addr + unspecified bool } const maxAddrSize = 512 @@ -157,6 +158,9 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { if params.Logger == nil { params.Logger = getLogger() } + if params.Net == nil { + params.Net = stdnet.NewNet(context.Background(), nil, nil) + } mux := &SingleSocketUDPMux{ addressMap: map[string][]*udpMuxedConn{}, @@ -165,6 +169,7 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { connsIPv6: make(map[string]*udpMuxedConn), candidateConnMap: make(map[string]*udpMuxedConn), closedChan: make(chan struct{}, 1), + unspecified: isUnspecified(params.UDPConn.LocalAddr()), pool: &sync.Pool{ New: func() interface{} { // big enough buffer to fit both packet and address @@ -173,7 +178,6 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { }, } - mux.updateLocalAddresses() return mux } @@ -199,10 +203,6 @@ func (m *SingleSocketUDPMux) updateLocalAddresses() { m.params.Logger.Errorf("LocalAddr expected IPV4 or IPV6, got %T", m.params.UDPConn.LocalAddr()) } if len(networks) > 0 { - if m.params.Net == nil { - m.params.Net = stdnet.NewNet(context.Background(), nil, nil) - } - ips, err := localInterfaces(m.params.Net, m.params.InterfaceFilter, nil, networks, true) if err == nil { for _, ip := range ips { @@ -241,10 +241,7 @@ func (m *SingleSocketUDPMux) GetListenAddresses() []net.Addr { // creates the connection if an existing one can't be found func (m *SingleSocketUDPMux) GetConn(ufrag string, addr net.Addr, candidateID string) (net.PacketConn, error) { // don't check addr for mux using unspecified address - m.mu.Lock() - lenLocalAddrs := len(m.localAddrsForUnspecified) - m.mu.Unlock() - if lenLocalAddrs == 0 && m.params.UDPConn.LocalAddr().String() != addr.String() { + if !m.unspecified && m.params.UDPConn.LocalAddr().String() != addr.String() { return nil, fmt.Errorf("invalid address %s", addr.String()) } @@ -508,3 +505,8 @@ func getLogger() logging.LeveledLogger { //fac.Writer = log.StandardLogger().Writer() return fac.NewLogger("ice") } + +func isUnspecified(addr net.Addr) bool { + udpAddr, ok := addr.(*net.UDPAddr) + return ok && udpAddr.IP.IsUnspecified() +} diff --git a/client/iface/udpmux/mux_test.go b/client/iface/udpmux/mux_test.go new file mode 100644 index 000000000..2e2770282 --- /dev/null +++ b/client/iface/udpmux/mux_test.go @@ -0,0 +1,99 @@ +package udpmux + +import ( + "fmt" + "net" + "sync" + "sync/atomic" + "testing" + + "github.com/pion/transport/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + return n.ifaces, nil +} + +func TestNewSingleSocketUDPMux_DoesNotDiscoverInterfaces(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{ifaces: []*transport.Interface{testInterface("eth0", net.IPv4(192, 168, 1, 10))}} + + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + assert.Equal(t, int64(0), n.calls.Load(), "constructing the mux must not enumerate interfaces") + + addrs := mux.GetListenAddresses() + assert.Equal(t, int64(1), n.calls.Load(), "listen addresses are discovered when they are asked for") + + port := conn.LocalAddr().(*net.UDPAddr).Port + require.Len(t, addrs, 1, "one address per discovered interface IP") + assert.Equal(t, fmt.Sprintf("192.168.1.10:%d", port), addrs[0].String(), "the discovered IP is paired with the socket port") +} + +func TestSingleSocketUDPMux_GetConnWithoutListenAddresses(t *testing.T) { + t.Run("unspecified socket accepts any address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{} + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + port := conn.LocalAddr().(*net.UDPAddr).Port + pc, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: port}, "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "a connection is created for a local address of an unspecified socket") + assert.Equal(t, int64(0), n.calls.Load(), "GetConn must not depend on a prior discovery") + }) + + t.Run("specific socket rejects a foreign address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4(127, 0, 0, 1)) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + _, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: 1234}, "candidate") + assert.Error(t, err, "an address the socket is not bound to must be rejected") + + pc, err := mux.GetConn("ufrag", conn.LocalAddr(), "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "the bound address is accepted") + }) +} + +func listenUDP(t *testing.T, ip net.IP) *net.UDPConn { + t.Helper() + conn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: ip, Port: 0}) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + return conn +} + +func testInterface(name string, ip net.IP) *transport.Interface { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: name, Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: ip, Mask: net.CIDRMask(24, 32)}) + return ifc +} + +func TestSingleSocketUDPMux_ConcurrentListenAddressesWithDefaultNet(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + assert.NotEmpty(t, mux.GetListenAddresses(), "every caller gets at least the socket address") + }() + } + wg.Wait() +} diff --git a/client/internal/engine.go b/client/internal/engine.go index fdf89ae06..c4bdf5121 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -387,7 +387,7 @@ func NewEngine( mgmClient: services.MgmClient, relayManager: services.RelayManager, peerStore: peerstore.NewConnStore(), - wgDetector: stdnet.NewWGDetector(), + wgDetector: stdnet.NewWGDetector(config.WgIfaceName), syncMsgMux: &sync.Mutex{}, config: config, mobileDep: mobileDep, diff --git a/client/internal/stdnet/wgdetector.go b/client/internal/stdnet/wgdetector.go index f34e469e1..8ed290190 100644 --- a/client/internal/stdnet/wgdetector.go +++ b/client/internal/stdnet/wgdetector.go @@ -34,6 +34,8 @@ type WGDetector struct { // probe is replaced in tests; it is the call this type exists to avoid repeating. probe func(string) bool + ownIface string + mu sync.RWMutex cache map[string]wgDetectorEntry @@ -41,11 +43,12 @@ type WGDetector struct { } // NewWGDetector returns a detector with the default time to live. -func NewWGDetector() *WGDetector { +func NewWGDetector(ownIface string) *WGDetector { return &WGDetector{ - ttl: wgDetectorTTL, - probe: probeWireGuard, - cache: make(map[string]wgDetectorEntry), + ttl: wgDetectorTTL, + probe: probeWireGuard, + ownIface: ownIface, + cache: make(map[string]wgDetectorEntry), } } @@ -57,6 +60,10 @@ func (d *WGDetector) IsWireGuard(iFace string) bool { return probeWireGuard(iFace) } + if d.isOwnInterface(iFace) { + return true + } + if isWireGuard, ok := d.cached(iFace); ok { return isWireGuard } @@ -104,6 +111,10 @@ func (d *WGDetector) store(iFace string, isWireGuard bool) { d.cache[iFace] = wgDetectorEntry{isWireGuard: isWireGuard, expireAt: now.Add(d.ttl)} } +func (d *WGDetector) isOwnInterface(iFace string) bool { + return d.ownIface != "" && iFace == d.ownIface +} + func probeWireGuard(iFace string) bool { wg, err := wgctrl.New() if err != nil { diff --git a/client/internal/stdnet/wgdetector_test.go b/client/internal/stdnet/wgdetector_test.go index 68260ae59..28cf85f6a 100644 --- a/client/internal/stdnet/wgdetector_test.go +++ b/client/internal/stdnet/wgdetector_test.go @@ -150,3 +150,39 @@ func TestWGDetectorDropsExpiredEntries(t *testing.T) { assert.Len(t, d.cache, 1, "expired entries of vanished interfaces must be dropped") assert.Contains(t, d.cache, "eth0", "the fresh answer must be kept") } + +func TestWGDetectorOwnInterfaceIsWireGuardWithoutProbing(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + + assert.True(t, d.IsWireGuard("NetBird"), "the agent's own interface is a WireGuard device") + assert.Equal(t, int64(0), calls.Load(), "the own interface must never reach the probe") + + assert.False(t, d.IsWireGuard("NetBird2"), "only the exact own name is answered without probing") + assert.Equal(t, int64(1), calls.Load(), "a different interface is probed") +} + +func TestWGDetectorWithoutOwnInterfaceProbesEmptyName(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + + assert.False(t, d.IsWireGuard(""), "an unnamed interface is not mistaken for the own one") + assert.Equal(t, int64(1), calls.Load(), "with no own interface set every name is probed") +} + +func TestInterfaceFilterRejectsOwnInterfaceRegardlessOfDisallowList(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + filter := InterfaceFilter([]string{"wt"}, d) + + assert.False(t, filter("NetBird"), "the own interface is rejected even when the disallow list does not cover it") + assert.Equal(t, int64(0), calls.Load(), "the own interface is not probed") + + assert.True(t, filter("eth0"), "an ordinary interface stays available") + assert.Equal(t, int64(1), calls.Load(), "an ordinary interface is still probed") +} + +func TestNewWGDetectorKeepsTheOwnInterface(t *testing.T) { + d := NewWGDetector("NetbirdWT") + + assert.True(t, d.IsWireGuard("NetbirdWT"), "the constructor records the own interface") +}