diff --git a/.github/workflows/no-new-replace.yml b/.github/workflows/no-new-replace.yml new file mode 100644 index 000000000..b906ce450 --- /dev/null +++ b/.github/workflows/no-new-replace.yml @@ -0,0 +1,78 @@ +name: No New Replace Directives + +on: + pull_request: + paths: + - "go.mod" + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }}-${{ github.head_ref || github.actor_id }} + cancel-in-progress: true + +jobs: + check-replace-directives: + name: check-replace-directives + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + fetch-depth: 0 + + - name: Install Go + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: go.mod + + - name: Compare replace directives against the base branch + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + + # A replace directive only applies when this module is the main + # module. Anything importing netbird as a library, the embedded + # clients among them, resolves the replaced path upstream instead and + # fails to build against whatever the replacement provides. Requiring + # a fork under its own module path avoids that; a replace does not. + # + # go.mod is parsed rather than diffed so that reordering, comments and + # single-line versus block syntax do not register as changes. + # + # Versions are part of the key because a replace can be scoped to one + # version of a module. Keyed on paths alone, retargeting such a + # directive at a different version would read as unchanged. + list_replaces() { + go mod edit -json "$1" \ + | jq -r ' + def ref: .Path + (if (.Version // "") == "" then "" else " " + .Version end); + (.Replace // [])[] | "\(.Old | ref) => \(.New | ref)" + ' \ + | sort + } + + git show "${BASE_SHA}:go.mod" > /tmp/base-go.mod + list_replaces /tmp/base-go.mod > /tmp/base-replaces + list_replaces go.mod > /tmp/head-replaces + + added=$(comm -13 /tmp/base-replaces /tmp/head-replaces) + if [ -n "$added" ]; then + echo "::error::This PR adds a replace directive to go.mod:" + echo "$added" | sed 's/^/ /' + echo "" + echo "A replace directive applies only to the main module, so it does not" + echo "reach anything that imports netbird as a library. Require the module" + echo "under a path you control instead, as done for github.com/netbirdio/go-nat." + exit 1 + fi + + removed=$(comm -23 /tmp/base-replaces /tmp/head-replaces) + if [ -n "$removed" ]; then + echo "This PR removes replace directives:" + echo "$removed" | sed 's/^/ /' + fi + echo "No new replace directives." diff --git a/client/internal/peer/worker_ice.go b/client/internal/peer/worker_ice.go index 67f76f2e6..83cac13f5 100644 --- a/client/internal/peer/worker_ice.go +++ b/client/internal/peer/worker_ice.go @@ -389,6 +389,17 @@ func (w *WorkerICE) injectPortForwardedCandidate(srflxCandidate ice.Candidate) { return } + // A forwarded candidate only makes sense for an IPv4 mapping, which + // translates a port on the gateway's address. An IPv6 pinhole translates + // nothing: it unblocks the address ICE already gathers as a host candidate, + // so there is no second address to advertise. Injecting one here would also + // paste an IPv6 address onto whichever server-reflexive candidate arrived + // first, which is usually IPv4. + if mapping.ExternalIP != nil && mapping.ExternalIP.To4() == nil { + w.log.Debugf("skipping port-forwarded candidate: %s mapping is IPv6-only", mapping.NATType) + return + } + w.muxAgent.Lock() if w.portForwardAttempted { w.muxAgent.Unlock() diff --git a/client/internal/portforward/manager.go b/client/internal/portforward/manager.go index b0680160c..7d5a4cb9e 100644 --- a/client/internal/portforward/manager.go +++ b/client/internal/portforward/manager.go @@ -10,10 +10,8 @@ import ( "sync" "time" - "github.com/libp2p/go-nat" + "github.com/netbirdio/go-nat" log "github.com/sirupsen/logrus" - - "github.com/netbirdio/netbird/client/internal/portforward/pcp" ) const ( @@ -168,6 +166,11 @@ func (m *Manager) setup(ctx context.Context) (nat.NAT, *Mapping, error) { if err != nil { return nil, nil, fmt.Errorf("create port mapping: %w", err) } + + // Only meaningful once a mapping has been attempted: that is what opens the + // pinhole and records its outcome. + logIPv6Pinhole(gateway) + return gateway, mapping, nil } @@ -265,7 +268,9 @@ func (m *Manager) checkHealthAndRecreate(ctx context.Context, gateway nat.NAT) b return false } - pcpNAT, ok := gateway.(*pcp.NAT) + // Assert on the interface, not on a concrete type: a dual-stack gateway is + // a wrapper around the IPv4 NAT, so a type assertion misses it. + checker, ok := gateway.(nat.HealthChecker) if !ok { return false } @@ -273,7 +278,7 @@ func (m *Manager) checkHealthAndRecreate(ctx context.Context, gateway nat.NAT) b ctx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() - epoch, serverRestarted, err := pcpNAT.CheckServerHealth(ctx) + epoch, serverRestarted, err := checker.CheckServerHealth(ctx) if err != nil { log.Debugf("PCP health check failed: %v", err) return false @@ -340,3 +345,18 @@ func (m *Manager) startTearDown(ctx context.Context) { func isPermanentLeaseRequired(err error) bool { return err != nil && upnpErrPermanentLeaseOnly.MatchString(err.Error()) } + +// logIPv6Pinhole reports the outcome of the IPv6 pinhole. Pinholes are best +// effort and never fail a mapping on their own, so this is the only way to see +// whether one was actually opened. +func logIPv6Pinhole(gateway nat.NAT) { + reporter, ok := gateway.(nat.IPv6PinholeReporter) + if !ok { + return + } + if err := reporter.IPv6PinholeError(); err != nil { + log.Warnf("IPv6 pinhole: %v", err) + return + } + log.Infof("IPv6 pinhole open") +} diff --git a/client/internal/portforward/pcp/client.go b/client/internal/portforward/pcp/client.go deleted file mode 100644 index f6d243ef9..000000000 --- a/client/internal/portforward/pcp/client.go +++ /dev/null @@ -1,408 +0,0 @@ -package pcp - -import ( - "context" - "crypto/rand" - "errors" - "fmt" - "net" - "net/netip" - "sync" - "time" - - log "github.com/sirupsen/logrus" -) - -const ( - defaultTimeout = 3 * time.Second - responseBufferSize = 128 - - // RFC 6887 Section 8.1.1 retry timing - initialRetryDelay = 3 * time.Second - maxRetryDelay = 1024 * time.Second - maxRetries = 4 // 3s + 6s + 12s + 24s = 45s total worst case -) - -// Client is a PCP protocol client. -// All methods are safe for concurrent use. -type Client struct { - gateway netip.Addr - timeout time.Duration - - mu sync.Mutex - // localIP caches the resolved local IP address. - localIP netip.Addr - // lastEpoch is the last observed server epoch value. - lastEpoch uint32 - // epochTime tracks when lastEpoch was received for state loss detection. - epochTime time.Time - // externalIP caches the external IP from the last successful MAP response. - externalIP netip.Addr - // epochStateLost is set when epoch indicates server restart. - epochStateLost bool -} - -// NewClient creates a new PCP client for the gateway at the given IP. -func NewClient(gateway net.IP) *Client { - addr, ok := netip.AddrFromSlice(gateway) - if !ok { - log.Debugf("invalid gateway IP: %v", gateway) - } - return &Client{ - gateway: addr.Unmap(), - timeout: defaultTimeout, - } -} - -// NewClientWithTimeout creates a new PCP client with a custom timeout. -func NewClientWithTimeout(gateway net.IP, timeout time.Duration) *Client { - addr, ok := netip.AddrFromSlice(gateway) - if !ok { - log.Debugf("invalid gateway IP: %v", gateway) - } - return &Client{ - gateway: addr.Unmap(), - timeout: timeout, - } -} - -// SetLocalIP sets the local IP address to use in PCP requests. -func (c *Client) SetLocalIP(ip net.IP) { - addr, ok := netip.AddrFromSlice(ip) - if !ok { - log.Debugf("invalid local IP: %v", ip) - } - c.mu.Lock() - c.localIP = addr.Unmap() - c.mu.Unlock() -} - -// Gateway returns the gateway IP address. -func (c *Client) Gateway() net.IP { - return c.gateway.AsSlice() -} - -// Announce sends a PCP ANNOUNCE request to discover PCP support. -// Returns the server's epoch time on success. -func (c *Client) Announce(ctx context.Context) (epoch uint32, err error) { - localIP, err := c.getLocalIP() - if err != nil { - return 0, fmt.Errorf("get local IP: %w", err) - } - - req := buildAnnounceRequest(localIP) - resp, err := c.sendRequest(ctx, req) - if err != nil { - return 0, fmt.Errorf("send announce: %w", err) - } - - parsed, err := parseResponse(resp) - if err != nil { - return 0, fmt.Errorf("parse announce response: %w", err) - } - - if parsed.ResultCode != ResultSuccess { - return 0, fmt.Errorf("PCP ANNOUNCE failed: %s", ResultCodeString(parsed.ResultCode)) - } - - c.mu.Lock() - if c.updateEpochLocked(parsed.Epoch) { - log.Warnf("PCP server epoch indicates state loss - mappings may need refresh") - } - c.mu.Unlock() - return parsed.Epoch, nil -} - -// AddPortMapping requests a port mapping from the PCP server. -func (c *Client) AddPortMapping(ctx context.Context, protocol string, internalPort int, lifetime time.Duration) (*MapResponse, error) { - return c.addPortMappingWithHint(ctx, protocol, internalPort, internalPort, netip.Addr{}, lifetime) -} - -// AddPortMappingWithHint requests a port mapping with suggested external port and IP. -// Use lifetime <= 0 to delete a mapping. -func (c *Client) AddPortMappingWithHint(ctx context.Context, protocol string, internalPort, suggestedExtPort int, suggestedExtIP net.IP, lifetime time.Duration) (*MapResponse, error) { - var extIP netip.Addr - if suggestedExtIP != nil { - var ok bool - extIP, ok = netip.AddrFromSlice(suggestedExtIP) - if !ok { - log.Debugf("invalid suggested external IP: %v", suggestedExtIP) - } - extIP = extIP.Unmap() - } - return c.addPortMappingWithHint(ctx, protocol, internalPort, suggestedExtPort, extIP, lifetime) -} - -func (c *Client) addPortMappingWithHint(ctx context.Context, protocol string, internalPort, suggestedExtPort int, suggestedExtIP netip.Addr, lifetime time.Duration) (*MapResponse, error) { - localIP, err := c.getLocalIP() - if err != nil { - return nil, fmt.Errorf("get local IP: %w", err) - } - - proto, err := protocolNumber(protocol) - if err != nil { - return nil, fmt.Errorf("parse protocol: %w", err) - } - - var nonce [12]byte - if _, err := rand.Read(nonce[:]); err != nil { - return nil, fmt.Errorf("generate nonce: %w", err) - } - - // Convert lifetime to seconds. Lifetime 0 means delete, so only apply - // default for positive durations that round to 0 seconds. - var lifetimeSec uint32 - if lifetime > 0 { - lifetimeSec = uint32(lifetime.Seconds()) - if lifetimeSec == 0 { - lifetimeSec = DefaultLifetime - } - } - - req := buildMapRequest(localIP, nonce, proto, uint16(internalPort), uint16(suggestedExtPort), suggestedExtIP, lifetimeSec) - - resp, err := c.sendRequest(ctx, req) - if err != nil { - return nil, fmt.Errorf("send map request: %w", err) - } - - mapResp, err := parseMapResponse(resp) - if err != nil { - return nil, fmt.Errorf("parse map response: %w", err) - } - - if mapResp.Nonce != nonce { - return nil, fmt.Errorf("nonce mismatch in response") - } - - if mapResp.Protocol != proto { - return nil, fmt.Errorf("protocol mismatch: requested %d, got %d", proto, mapResp.Protocol) - } - if mapResp.InternalPort != uint16(internalPort) { - return nil, fmt.Errorf("internal port mismatch: requested %d, got %d", internalPort, mapResp.InternalPort) - } - - if mapResp.ResultCode != ResultSuccess { - return nil, &Error{ - Code: mapResp.ResultCode, - Message: ResultCodeString(mapResp.ResultCode), - } - } - - c.mu.Lock() - if c.updateEpochLocked(mapResp.Epoch) { - log.Warnf("PCP server epoch indicates state loss - mappings may need refresh") - } - c.cacheExternalIPLocked(mapResp.ExternalIP) - c.mu.Unlock() - return mapResp, nil -} - -// DeletePortMapping removes a port mapping by requesting zero lifetime. -func (c *Client) DeletePortMapping(ctx context.Context, protocol string, internalPort int) error { - if _, err := c.addPortMappingWithHint(ctx, protocol, internalPort, 0, netip.Addr{}, 0); err != nil { - var pcpErr *Error - if errors.As(err, &pcpErr) && pcpErr.Code == ResultNotAuthorized { - return nil - } - return fmt.Errorf("delete mapping: %w", err) - } - return nil -} - -// GetExternalAddress returns the external IP address. -// First checks for a cached value from previous MAP responses. -// If not cached, creates a short-lived mapping to discover the external IP. -func (c *Client) GetExternalAddress(ctx context.Context) (net.IP, error) { - c.mu.Lock() - if c.externalIP.IsValid() { - ip := c.externalIP.AsSlice() - c.mu.Unlock() - return ip, nil - } - c.mu.Unlock() - - // Use an ephemeral port in the dynamic range (49152-65535). - // Port 0 is not valid with UDP/TCP protocols per RFC 6887. - ephemeralPort := 49152 + int(uint16(time.Now().UnixNano()))%(65535-49152) - - // Use minimal lifetime (1 second) for discovery. - resp, err := c.AddPortMapping(ctx, "udp", ephemeralPort, time.Second) - if err != nil { - return nil, fmt.Errorf("create temporary mapping: %w", err) - } - - if err := c.DeletePortMapping(ctx, "udp", ephemeralPort); err != nil { - log.Debugf("cleanup temporary PCP mapping: %v", err) - } - - return resp.ExternalIP.AsSlice(), nil -} - -// LastEpoch returns the last observed server epoch value. -// A decrease in epoch indicates the server may have restarted and mappings may be lost. -func (c *Client) LastEpoch() uint32 { - c.mu.Lock() - defer c.mu.Unlock() - return c.lastEpoch -} - -// EpochStateLost returns true if epoch state loss was detected and clears the flag. -func (c *Client) EpochStateLost() bool { - c.mu.Lock() - defer c.mu.Unlock() - lost := c.epochStateLost - c.epochStateLost = false - return lost -} - -// updateEpoch updates the epoch tracking and detects potential state loss. -// Returns true if state loss was detected (server likely restarted). -// Caller must hold c.mu. -func (c *Client) updateEpochLocked(newEpoch uint32) bool { - now := time.Now() - stateLost := false - - // RFC 6887 Section 8.5: Detect invalid epoch indicating server state loss. - // client_delta = time since last response - // server_delta = epoch change since last response - // Invalid if: client_delta+2 < server_delta - server_delta/16 - // OR: server_delta+2 < client_delta - client_delta/16 - // The +2 handles quantization, /16 (6.25%) handles clock drift. - if !c.epochTime.IsZero() && c.lastEpoch > 0 { - clientDelta := uint32(now.Sub(c.epochTime).Seconds()) - serverDelta := newEpoch - c.lastEpoch - - // Check for epoch going backwards or jumping unexpectedly. - // Subtraction is safe: serverDelta/16 is always <= serverDelta. - if clientDelta+2 < serverDelta-(serverDelta/16) || - serverDelta+2 < clientDelta-(clientDelta/16) { - stateLost = true - c.epochStateLost = true - } - } - - c.lastEpoch = newEpoch - c.epochTime = now - return stateLost -} - -// cacheExternalIP stores the external IP from a successful MAP response. -// Caller must hold c.mu. -func (c *Client) cacheExternalIPLocked(ip netip.Addr) { - if ip.IsValid() && !ip.IsUnspecified() { - c.externalIP = ip - } -} - -// sendRequest sends a PCP request with retries per RFC 6887 Section 8.1.1. -func (c *Client) sendRequest(ctx context.Context, req []byte) ([]byte, error) { - addr := &net.UDPAddr{IP: c.gateway.AsSlice(), Port: Port} - - var lastErr error - delay := initialRetryDelay - - for range maxRetries { - resp, err := c.sendOnce(ctx, addr, req) - if err == nil { - return resp, nil - } - lastErr = err - - if ctx.Err() != nil { - return nil, ctx.Err() - } - - // RFC 6887 Section 8.1.1: RT = (1 + RAND) * MIN(2 * RTprev, MRT) - // RAND is random between -0.1 and +0.1 - select { - case <-ctx.Done(): - return nil, ctx.Err() - case <-time.After(retryDelayWithJitter(delay)): - } - delay = min(delay*2, maxRetryDelay) - } - - return nil, fmt.Errorf("PCP request failed after %d retries: %w", maxRetries, lastErr) -} - -// retryDelayWithJitter applies RFC 6887 jitter: multiply by (1 + RAND) where RAND is [-0.1, +0.1]. -func retryDelayWithJitter(d time.Duration) time.Duration { - var b [1]byte - _, _ = rand.Read(b[:]) - // Convert byte to range [-0.1, +0.1]: (b/255 * 0.2) - 0.1 - jitter := (float64(b[0])/255.0)*0.2 - 0.1 - return time.Duration(float64(d) * (1 + jitter)) -} - -func (c *Client) sendOnce(ctx context.Context, addr *net.UDPAddr, req []byte) ([]byte, error) { - // Use ListenUDP instead of DialUDP to validate response source address per RFC 6887 §8.3. - conn, err := net.ListenUDP("udp", nil) - if err != nil { - return nil, fmt.Errorf("listen: %w", err) - } - defer func() { - if err := conn.Close(); err != nil { - log.Debugf("close UDP connection: %v", err) - } - }() - - timeout := c.timeout - if deadline, ok := ctx.Deadline(); ok { - if remaining := time.Until(deadline); remaining < timeout { - timeout = remaining - } - } - - if err := conn.SetDeadline(time.Now().Add(timeout)); err != nil { - return nil, fmt.Errorf("set deadline: %w", err) - } - - if _, err := conn.WriteToUDP(req, addr); err != nil { - return nil, fmt.Errorf("write: %w", err) - } - - resp := make([]byte, responseBufferSize) - n, from, err := conn.ReadFromUDP(resp) - if err != nil { - return nil, fmt.Errorf("read: %w", err) - } - - // RFC 6887 §8.3: Validate response came from expected PCP server. - if !from.IP.Equal(addr.IP) { - return nil, fmt.Errorf("response from unexpected source %s (expected %s)", from.IP, addr.IP) - } - - return resp[:n], nil -} - -func (c *Client) getLocalIP() (netip.Addr, error) { - c.mu.Lock() - defer c.mu.Unlock() - - if !c.localIP.IsValid() { - return netip.Addr{}, fmt.Errorf("local IP not set for gateway %s", c.gateway) - } - return c.localIP, nil -} - -func protocolNumber(protocol string) (uint8, error) { - switch protocol { - case "udp", "UDP": - return ProtoUDP, nil - case "tcp", "TCP": - return ProtoTCP, nil - default: - return 0, fmt.Errorf("unsupported protocol: %s", protocol) - } -} - -// Error represents a PCP error response. -type Error struct { - Code uint8 - Message string -} - -func (e *Error) Error() string { - return fmt.Sprintf("PCP error: %s (%d)", e.Message, e.Code) -} diff --git a/client/internal/portforward/pcp/client_test.go b/client/internal/portforward/pcp/client_test.go deleted file mode 100644 index 79f44a426..000000000 --- a/client/internal/portforward/pcp/client_test.go +++ /dev/null @@ -1,187 +0,0 @@ -package pcp - -import ( - "context" - "net" - "net/netip" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestAddrConversion(t *testing.T) { - tests := []struct { - name string - addr netip.Addr - }{ - {"IPv4", netip.MustParseAddr("192.168.1.100")}, - {"IPv4 loopback", netip.MustParseAddr("127.0.0.1")}, - {"IPv6", netip.MustParseAddr("2001:db8::1")}, - {"IPv6 loopback", netip.MustParseAddr("::1")}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - b16 := addrTo16(tt.addr) - - recovered := addrFrom16(b16) - assert.Equal(t, tt.addr, recovered, "address should round-trip") - }) - } -} - -func TestBuildAnnounceRequest(t *testing.T) { - clientIP := netip.MustParseAddr("192.168.1.100") - req := buildAnnounceRequest(clientIP) - - require.Len(t, req, headerSize) - assert.Equal(t, byte(Version), req[0], "version") - assert.Equal(t, byte(OpAnnounce), req[1], "opcode") - - // Check client IP is properly encoded as IPv4-mapped IPv6 - assert.Equal(t, byte(0xff), req[18], "IPv4-mapped prefix byte 10") - assert.Equal(t, byte(0xff), req[19], "IPv4-mapped prefix byte 11") - assert.Equal(t, byte(192), req[20], "IP octet 1") - assert.Equal(t, byte(168), req[21], "IP octet 2") - assert.Equal(t, byte(1), req[22], "IP octet 3") - assert.Equal(t, byte(100), req[23], "IP octet 4") -} - -func TestBuildMapRequest(t *testing.T) { - clientIP := netip.MustParseAddr("192.168.1.100") - nonce := [12]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12} - req := buildMapRequest(clientIP, nonce, ProtoUDP, 51820, 51820, netip.Addr{}, 3600) - - require.Len(t, req, mapRequestSize) - assert.Equal(t, byte(Version), req[0], "version") - assert.Equal(t, byte(OpMap), req[1], "opcode") - - // Lifetime at bytes 4-7 - assert.Equal(t, uint32(3600), (uint32(req[4])<<24)|(uint32(req[5])<<16)|(uint32(req[6])<<8)|uint32(req[7]), "lifetime") - - // Nonce at bytes 24-35 - assert.Equal(t, nonce[:], req[24:36], "nonce") - - // Protocol at byte 36 - assert.Equal(t, byte(ProtoUDP), req[36], "protocol") - - // Internal port at bytes 40-41 - assert.Equal(t, uint16(51820), (uint16(req[40])<<8)|uint16(req[41]), "internal port") - - // External port at bytes 42-43 - assert.Equal(t, uint16(51820), (uint16(req[42])<<8)|uint16(req[43]), "external port") -} - -func TestParseResponse(t *testing.T) { - // Construct a valid ANNOUNCE response - resp := make([]byte, headerSize) - resp[0] = Version - resp[1] = OpAnnounce | OpReply - // Result code = 0 (success) - // Lifetime = 0 - // Epoch = 12345 - resp[8] = 0 - resp[9] = 0 - resp[10] = 0x30 - resp[11] = 0x39 - - parsed, err := parseResponse(resp) - require.NoError(t, err) - assert.Equal(t, uint8(Version), parsed.Version) - assert.Equal(t, uint8(OpAnnounce|OpReply), parsed.Opcode) - assert.Equal(t, uint8(ResultSuccess), parsed.ResultCode) - assert.Equal(t, uint32(12345), parsed.Epoch) -} - -func TestParseResponseErrors(t *testing.T) { - t.Run("too short", func(t *testing.T) { - _, err := parseResponse([]byte{1, 2, 3}) - assert.Error(t, err) - }) - - t.Run("wrong version", func(t *testing.T) { - resp := make([]byte, headerSize) - resp[0] = 1 // Wrong version - resp[1] = OpReply - _, err := parseResponse(resp) - assert.Error(t, err) - }) - - t.Run("missing reply bit", func(t *testing.T) { - resp := make([]byte, headerSize) - resp[0] = Version - resp[1] = OpAnnounce // Missing OpReply bit - _, err := parseResponse(resp) - assert.Error(t, err) - }) -} - -func TestResultCodeString(t *testing.T) { - assert.Equal(t, "SUCCESS", ResultCodeString(ResultSuccess)) - assert.Equal(t, "NOT_AUTHORIZED", ResultCodeString(ResultNotAuthorized)) - assert.Equal(t, "ADDRESS_MISMATCH", ResultCodeString(ResultAddressMismatch)) - assert.Contains(t, ResultCodeString(255), "UNKNOWN") -} - -func TestProtocolNumber(t *testing.T) { - proto, err := protocolNumber("udp") - require.NoError(t, err) - assert.Equal(t, uint8(ProtoUDP), proto) - - proto, err = protocolNumber("tcp") - require.NoError(t, err) - assert.Equal(t, uint8(ProtoTCP), proto) - - proto, err = protocolNumber("UDP") - require.NoError(t, err) - assert.Equal(t, uint8(ProtoUDP), proto) - - _, err = protocolNumber("icmp") - assert.Error(t, err) -} - -func TestClientCreation(t *testing.T) { - gateway := netip.MustParseAddr("192.168.1.1").AsSlice() - - client := NewClient(gateway) - assert.Equal(t, net.IP(gateway), client.Gateway()) - assert.Equal(t, defaultTimeout, client.timeout) - - clientWithTimeout := NewClientWithTimeout(gateway, 5*time.Second) - assert.Equal(t, 5*time.Second, clientWithTimeout.timeout) -} - -func TestNATType(t *testing.T) { - n := NewNAT(netip.MustParseAddr("192.168.1.1").AsSlice(), netip.MustParseAddr("192.168.1.100").AsSlice()) - assert.Equal(t, "PCP", n.Type()) -} - -// Integration test - skipped unless PCP_TEST_GATEWAY env is set -func TestClientIntegration(t *testing.T) { - t.Skip("Integration test - run manually with PCP_TEST_GATEWAY=") - - gateway := netip.MustParseAddr("10.0.1.1").AsSlice() // Change to your test gateway - localIP := netip.MustParseAddr("10.0.1.100").AsSlice() // Change to your local IP - - client := NewClient(gateway) - client.SetLocalIP(localIP) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - // Test ANNOUNCE - epoch, err := client.Announce(ctx) - require.NoError(t, err) - t.Logf("Server epoch: %d", epoch) - - // Test MAP - resp, err := client.AddPortMapping(ctx, "udp", 51820, 1*time.Hour) - require.NoError(t, err) - t.Logf("Mapping: internal=%d external=%d externalIP=%s", - resp.InternalPort, resp.ExternalPort, resp.ExternalIP) - - // Cleanup - err = client.DeletePortMapping(ctx, "udp", 51820) - require.NoError(t, err) -} diff --git a/client/internal/portforward/pcp/nat.go b/client/internal/portforward/pcp/nat.go deleted file mode 100644 index 0e635b6c8..000000000 --- a/client/internal/portforward/pcp/nat.go +++ /dev/null @@ -1,222 +0,0 @@ -package pcp - -import ( - "context" - "fmt" - "net" - "net/netip" - "runtime" - "sync" - "time" - - log "github.com/sirupsen/logrus" - - "github.com/libp2p/go-nat" - "github.com/libp2p/go-netroute" -) - -var _ nat.NAT = (*NAT)(nil) - -// NAT implements the go-nat NAT interface using PCP. -// Supports dual-stack (IPv4 and IPv6) when available. -// All methods are safe for concurrent use. -// -// TODO: IPv6 pinholes use the local IPv6 address. If the address changes -// (e.g., due to SLAAC rotation or network change), the pinhole becomes stale -// and needs to be recreated with the new address. -type NAT struct { - client *Client - - mu sync.RWMutex - // client6 is the IPv6 PCP client, nil if IPv6 is unavailable. - client6 *Client - // localIP6 caches the local IPv6 address used for PCP requests. - localIP6 netip.Addr -} - -// NewNAT creates a new NAT instance backed by PCP. -func NewNAT(gateway, localIP net.IP) *NAT { - client := NewClient(gateway) - client.SetLocalIP(localIP) - return &NAT{ - client: client, - } -} - -// Type returns "PCP" as the NAT type. -func (n *NAT) Type() string { - return "PCP" -} - -// GetDeviceAddress returns the gateway IP address. -func (n *NAT) GetDeviceAddress() (net.IP, error) { - return n.client.Gateway(), nil -} - -// GetExternalAddress returns the external IP address. -func (n *NAT) GetExternalAddress() (net.IP, error) { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - return n.client.GetExternalAddress(ctx) -} - -// GetInternalAddress returns the local IP address used to communicate with the gateway. -func (n *NAT) GetInternalAddress() (net.IP, error) { - addr, err := n.client.getLocalIP() - if err != nil { - return nil, err - } - return addr.AsSlice(), nil -} - -// AddPortMapping creates a port mapping on both IPv4 and IPv6 (if available). -func (n *NAT) AddPortMapping(ctx context.Context, protocol string, internalPort int, _ string, timeout time.Duration) (int, error) { - resp, err := n.client.AddPortMapping(ctx, protocol, internalPort, timeout) - if err != nil { - return 0, fmt.Errorf("add mapping: %w", err) - } - - n.mu.RLock() - client6 := n.client6 - localIP6 := n.localIP6 - n.mu.RUnlock() - - if client6 == nil { - return int(resp.ExternalPort), nil - } - - if _, err := client6.AddPortMapping(ctx, protocol, internalPort, timeout); err != nil { - log.Warnf("IPv6 PCP mapping failed (continuing with IPv4): %v", err) - return int(resp.ExternalPort), nil - } - - log.Infof("created IPv6 PCP pinhole: %s:%d", localIP6, internalPort) - return int(resp.ExternalPort), nil -} - -// DeletePortMapping removes a port mapping from both IPv4 and IPv6. -func (n *NAT) DeletePortMapping(ctx context.Context, protocol string, internalPort int) error { - err := n.client.DeletePortMapping(ctx, protocol, internalPort) - - n.mu.RLock() - client6 := n.client6 - n.mu.RUnlock() - - if client6 != nil { - if err6 := client6.DeletePortMapping(ctx, protocol, internalPort); err6 != nil { - log.Warnf("IPv6 PCP delete mapping failed: %v", err6) - } - } - - if err != nil { - return fmt.Errorf("delete mapping: %w", err) - } - return nil -} - -// CheckServerHealth sends an ANNOUNCE to verify the server is still responsive. -// Returns the current epoch and whether the server may have restarted (epoch state loss detected). -func (n *NAT) CheckServerHealth(ctx context.Context) (epoch uint32, serverRestarted bool, err error) { - epoch, err = n.client.Announce(ctx) - if err != nil { - return 0, false, fmt.Errorf("announce: %w", err) - } - return epoch, n.client.EpochStateLost(), nil -} - -// DiscoverPCP attempts to discover a PCP-capable gateway. -// Returns a NAT interface if PCP is supported, or an error otherwise. -// Discovers both IPv4 and IPv6 gateways when available. -func DiscoverPCP(ctx context.Context) (nat.NAT, error) { - gateway, localIP, err := getDefaultGateway() - if err != nil { - return nil, fmt.Errorf("get default gateway: %w", err) - } - - client := NewClient(gateway) - client.SetLocalIP(localIP) - if _, err := client.Announce(ctx); err != nil { - return nil, fmt.Errorf("PCP announce: %w", err) - } - - result := &NAT{client: client} - discoverIPv6(ctx, result) - - return result, nil -} - -func discoverIPv6(ctx context.Context, result *NAT) { - gateway6, localIP6, err := getDefaultGateway6() - if err != nil { - log.Debugf("IPv6 gateway discovery failed: %v", err) - return - } - - client6 := NewClient(gateway6) - client6.SetLocalIP(localIP6) - if _, err := client6.Announce(ctx); err != nil { - log.Debugf("PCP IPv6 announce failed: %v", err) - return - } - - addr, ok := netip.AddrFromSlice(localIP6) - if !ok { - log.Debugf("invalid IPv6 local IP: %v", localIP6) - return - } - result.mu.Lock() - result.client6 = client6 - result.localIP6 = addr - result.mu.Unlock() - log.Debugf("PCP IPv6 gateway discovered: %s (local: %s)", gateway6, localIP6) -} - -// getDefaultGateway returns the default IPv4 gateway and local IP using the system routing table. -func getDefaultGateway() (gateway net.IP, localIP net.IP, err error) { - router, err := netroute.New() - if err != nil { - return nil, nil, err - } - - dst := net.IPv4zero - if runtime.GOOS == "linux" || runtime.GOOS == "android" { - // go-netroute v0.4.0 rejects unspecified destinations client-side on Linux/Android. - // TODO: on android/ios, use platform APIs (ConnectivityManager.getLinkProperties / - // NWPathMonitor) when netlink-based lookup is restricted or unavailable. - dst = net.IPv4(0, 0, 0, 1) - } - _, gateway, localIP, err = router.Route(dst) - if err != nil { - return nil, nil, err - } - - if gateway == nil { - return nil, nil, nat.ErrNoNATFound - } - - return gateway, localIP, nil -} - -// getDefaultGateway6 returns the default IPv6 gateway IP address using the system routing table. -func getDefaultGateway6() (gateway net.IP, localIP net.IP, err error) { - router, err := netroute.New() - if err != nil { - return nil, nil, err - } - - dst := net.IPv6zero - if runtime.GOOS == "linux" || runtime.GOOS == "android" { - // ::2 - dst = net.IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2} - } - _, gateway, localIP, err = router.Route(dst) - if err != nil { - return nil, nil, err - } - - if gateway == nil { - return nil, nil, nat.ErrNoNATFound - } - - return gateway, localIP, nil -} diff --git a/client/internal/portforward/pcp/protocol.go b/client/internal/portforward/pcp/protocol.go deleted file mode 100644 index d81c50c8c..000000000 --- a/client/internal/portforward/pcp/protocol.go +++ /dev/null @@ -1,225 +0,0 @@ -// Package pcp implements the Port Control Protocol (RFC 6887). -// -// # Implemented Features -// -// - ANNOUNCE opcode: Discovers PCP server support -// - MAP opcode: Creates/deletes port mappings (IPv4 NAT) and firewall pinholes (IPv6) -// - Dual-stack: Simultaneous IPv4 and IPv6 support via separate clients -// - Nonce validation: Prevents response spoofing -// - Epoch tracking: Detects server restarts per Section 8.5 -// - RFC-compliant retry timing: 3s initial, exponential backoff to 1024s max (Section 8.1.1) -// -// # Not Implemented -// -// - PEER opcode: For outbound peer connections (not needed for inbound NAT traversal) -// - THIRD_PARTY option: For managing mappings on behalf of other devices -// - PREFER_FAILURE option: Requires exact external port or fail (IPv4 NAT only, not needed for IPv6 pinholing) -// - FILTER option: To restrict remote peer addresses -// -// These optional features are omitted because the primary use case is simple -// port forwarding for WireGuard, which only requires MAP with default behavior. -package pcp - -import ( - "encoding/binary" - "fmt" - "net/netip" -) - -const ( - // Version is the PCP protocol version (RFC 6887). - Version = 2 - - // Port is the standard PCP server port. - Port = 5351 - - // DefaultLifetime is the default requested mapping lifetime in seconds. - DefaultLifetime = 7200 // 2 hours - - // Header sizes - headerSize = 24 - mapPayloadSize = 36 - mapRequestSize = headerSize + mapPayloadSize // 60 bytes -) - -// Opcodes -const ( - OpAnnounce = 0 - OpMap = 1 - OpPeer = 2 - OpReply = 0x80 // OR'd with opcode in responses -) - -// Protocol numbers for MAP requests -const ( - ProtoUDP = 17 - ProtoTCP = 6 -) - -// Result codes (RFC 6887 Section 7.4) -const ( - ResultSuccess = 0 - ResultUnsuppVersion = 1 - ResultNotAuthorized = 2 - ResultMalformedRequest = 3 - ResultUnsuppOpcode = 4 - ResultUnsuppOption = 5 - ResultMalformedOption = 6 - ResultNetworkFailure = 7 - ResultNoResources = 8 - ResultUnsuppProtocol = 9 - ResultUserExQuota = 10 - ResultCannotProvideExt = 11 - ResultAddressMismatch = 12 - ResultExcessiveRemotePeers = 13 -) - -// ResultCodeString returns a human-readable string for a result code. -func ResultCodeString(code uint8) string { - switch code { - case ResultSuccess: - return "SUCCESS" - case ResultUnsuppVersion: - return "UNSUPP_VERSION" - case ResultNotAuthorized: - return "NOT_AUTHORIZED" - case ResultMalformedRequest: - return "MALFORMED_REQUEST" - case ResultUnsuppOpcode: - return "UNSUPP_OPCODE" - case ResultUnsuppOption: - return "UNSUPP_OPTION" - case ResultMalformedOption: - return "MALFORMED_OPTION" - case ResultNetworkFailure: - return "NETWORK_FAILURE" - case ResultNoResources: - return "NO_RESOURCES" - case ResultUnsuppProtocol: - return "UNSUPP_PROTOCOL" - case ResultUserExQuota: - return "USER_EX_QUOTA" - case ResultCannotProvideExt: - return "CANNOT_PROVIDE_EXTERNAL" - case ResultAddressMismatch: - return "ADDRESS_MISMATCH" - case ResultExcessiveRemotePeers: - return "EXCESSIVE_REMOTE_PEERS" - default: - return fmt.Sprintf("UNKNOWN(%d)", code) - } -} - -// Response represents a parsed PCP response header. -type Response struct { - Version uint8 - Opcode uint8 - ResultCode uint8 - Lifetime uint32 - Epoch uint32 -} - -// MapResponse contains the full response to a MAP request. -type MapResponse struct { - Response - Nonce [12]byte - Protocol uint8 - InternalPort uint16 - ExternalPort uint16 - ExternalIP netip.Addr -} - -// addrTo16 converts an address to its 16-byte IPv4-mapped IPv6 representation. -func addrTo16(addr netip.Addr) [16]byte { - if addr.Is4() { - return netip.AddrFrom4(addr.As4()).As16() - } - return addr.As16() -} - -// addrFrom16 extracts an address from a 16-byte representation, unmapping IPv4. -func addrFrom16(b [16]byte) netip.Addr { - return netip.AddrFrom16(b).Unmap() -} - -// buildAnnounceRequest creates a PCP ANNOUNCE request packet. -func buildAnnounceRequest(clientIP netip.Addr) []byte { - req := make([]byte, headerSize) - req[0] = Version - req[1] = OpAnnounce - mapped := addrTo16(clientIP) - copy(req[8:24], mapped[:]) - return req -} - -// buildMapRequest creates a PCP MAP request packet. -func buildMapRequest(clientIP netip.Addr, nonce [12]byte, protocol uint8, internalPort, suggestedExtPort uint16, suggestedExtIP netip.Addr, lifetime uint32) []byte { - req := make([]byte, mapRequestSize) - - // Header - req[0] = Version - req[1] = OpMap - binary.BigEndian.PutUint32(req[4:8], lifetime) - mapped := addrTo16(clientIP) - copy(req[8:24], mapped[:]) - - // MAP payload - copy(req[24:36], nonce[:]) - req[36] = protocol - binary.BigEndian.PutUint16(req[40:42], internalPort) - binary.BigEndian.PutUint16(req[42:44], suggestedExtPort) - if suggestedExtIP.IsValid() { - extMapped := addrTo16(suggestedExtIP) - copy(req[44:60], extMapped[:]) - } - - return req -} - -// parseResponse parses the common PCP response header. -func parseResponse(data []byte) (*Response, error) { - if len(data) < headerSize { - return nil, fmt.Errorf("response too short: %d bytes", len(data)) - } - - resp := &Response{ - Version: data[0], - Opcode: data[1], - ResultCode: data[3], // Byte 2 is reserved, byte 3 is result code (RFC 6887 §7.2) - Lifetime: binary.BigEndian.Uint32(data[4:8]), - Epoch: binary.BigEndian.Uint32(data[8:12]), - } - - if resp.Version != Version { - return nil, fmt.Errorf("unsupported PCP version: %d", resp.Version) - } - - if resp.Opcode&OpReply == 0 { - return nil, fmt.Errorf("response missing reply bit: opcode=0x%02x", resp.Opcode) - } - - return resp, nil -} - -// parseMapResponse parses a complete MAP response. -func parseMapResponse(data []byte) (*MapResponse, error) { - if len(data) < mapRequestSize { - return nil, fmt.Errorf("MAP response too short: %d bytes", len(data)) - } - - resp, err := parseResponse(data) - if err != nil { - return nil, fmt.Errorf("parse header: %w", err) - } - - mapResp := &MapResponse{ - Response: *resp, - Protocol: data[36], - InternalPort: binary.BigEndian.Uint16(data[40:42]), - ExternalPort: binary.BigEndian.Uint16(data[42:44]), - ExternalIP: addrFrom16([16]byte(data[44:60])), - } - copy(mapResp.Nonce[:], data[24:36]) - - return mapResp, nil -} diff --git a/client/internal/portforward/pinhole_test.go b/client/internal/portforward/pinhole_test.go new file mode 100644 index 000000000..46b07a9e7 --- /dev/null +++ b/client/internal/portforward/pinhole_test.go @@ -0,0 +1,116 @@ +//go:build !js + +package portforward + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/netbirdio/go-nat" + log "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockPinholeNAT is a gateway that also reports an IPv6 pinhole outcome, the +// shape a dual-stack gateway has. +type mockPinholeNAT struct { + *mockNAT + pinholeErr error +} + +func (m *mockPinholeNAT) IPv6PinholeError() error { + return m.pinholeErr +} + +func TestSetupLogsPinholeOutcome(t *testing.T) { + pinholeErr := errors.New("pcp ipv6: NOT_AUTHORIZED") + + tests := []struct { + name string + pinholeErr error + mappingErr error + wantLevel log.Level + wantText string + }{ + { + name: "an open pinhole is reported", + wantLevel: log.InfoLevel, + wantText: "IPv6 pinhole open", + }, + { + name: "a failed pinhole is reported without failing the mapping", + // The IPv4 mapping is what the caller asked for, so the pinhole + // failure surfaces only in the log. + pinholeErr: pinholeErr, + wantLevel: log.WarnLevel, + wantText: pinholeErr.Error(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gateway := &mockPinholeNAT{mockNAT: newMockNAT(), pinholeErr: tt.pinholeErr} + hook := stubGatewayDiscovery(t, gateway) + + m := NewManager() + m.wgPort = 51820 + + _, mapping, err := m.setup(context.Background()) + + require.NoError(t, err) + require.NotNil(t, mapping) + + entry := findEntry(hook, tt.wantText) + require.NotNil(t, entry, "no log entry mentioning %q", tt.wantText) + assert.Equal(t, tt.wantLevel, entry.Level) + }) + } + + t.Run("a failed mapping reports no pinhole outcome", func(t *testing.T) { + // Nothing opened the pinhole, so whatever it currently reports says + // nothing about this attempt. + gateway := &mockPinholeNAT{mockNAT: newMockNAT()} + gateway.addMappingErr = errors.New("gateway refused") + hook := stubGatewayDiscovery(t, gateway) + + m := NewManager() + m.wgPort = 51820 + + _, _, err := m.setup(context.Background()) + + require.Error(t, err) + assert.Nil(t, findEntry(hook, "IPv6 pinhole")) + }) +} + +// stubGatewayDiscovery makes discovery return gateway and captures log output. +func stubGatewayDiscovery(t *testing.T, gateway nat.NAT) *test.Hook { + t.Helper() + + orig := discoverGateway + discoverGateway = func(context.Context) (nat.NAT, error) { return gateway, nil } + t.Cleanup(func() { discoverGateway = orig }) + + hook := test.NewGlobal() + origLevel := log.GetLevel() + log.SetLevel(log.DebugLevel) + t.Cleanup(func() { + hook.Reset() + log.SetLevel(origLevel) + }) + + return hook +} + +func findEntry(hook *test.Hook, substr string) *log.Entry { + for _, entry := range hook.AllEntries() { + if strings.Contains(entry.Message, substr) { + return entry + } + } + return nil +} diff --git a/client/internal/portforward/state.go b/client/internal/portforward/state.go index b1315cdc0..a21368e58 100644 --- a/client/internal/portforward/state.go +++ b/client/internal/portforward/state.go @@ -4,27 +4,94 @@ package portforward import ( "context" + "errors" "fmt" + "time" - "github.com/libp2p/go-nat" + "github.com/netbirdio/go-nat" + "github.com/netbirdio/go-nat/pcp" log "github.com/sirupsen/logrus" - - "github.com/netbirdio/netbird/client/internal/portforward/pcp" ) // discoverGateway is the function used for NAT gateway discovery. // It can be replaced in tests to avoid real network operations. -// Tries PCP first, then falls back to NAT-PMP/UPnP. var discoverGateway = defaultDiscoverGateway -func defaultDiscoverGateway(ctx context.Context) (nat.NAT, error) { - pcpGateway, err := pcp.DiscoverPCP(ctx) - if err == nil { - return pcpGateway, nil - } - log.Debugf("PCP discovery failed: %v, trying NAT-PMP/UPnP", err) +// pinholeDiscoveryTimeout is the slice of the discovery budget held back for +// the IPv6 pinhole probe. +// +// Sizing it is coarser than it looks: PCP retransmits on a 3s socket timeout +// and a 3s first backoff, so a second attempt needs about 9s. Anything from +// roughly 1s to 8s therefore buys exactly one attempt, and this only sets how +// long that attempt waits. A PCP server sits on the local link and answers in +// milliseconds, so 3s is margin rather than need, and the rest is left to +// gateway discovery, whose multicast SSDP search alone takes 5s. A probe lost +// to a dropped packet is retried by the next discovery round. +// +// It is a variable so tests can shorten it. +var pinholeDiscoveryTimeout = 3 * time.Second - return nat.DiscoverGateway(ctx) +// Discovery entry points, as variables so tests can drive the fallback without +// touching the network. +var ( + discoverNATGateway = nat.DiscoverGateway + + discoverPCPPinhole = func(ctx context.Context) (nat.NAT, error) { + pinhole, err := pcp.DiscoverPCP(ctx) + if err != nil { + return nil, err + } + return pinhole, nil + } +) + +// defaultDiscoverGateway finds a gateway that can make the WireGuard port +// reachable. DiscoverGateway prefers PCP for IPv4, races UPnP and NAT-PMP +// behind it, and attaches an IPv6 pinhole independently of which IPv4 protocol +// wins. +// +// It reports no gateway on a network offering only IPv6, having no IPv4 mapping +// to attach a pinhole to. Such a network still needs one: there is no +// translation to traverse, but the router drops inbound IPv6 until something +// opens it. Fall back to PCP alone, which yields a gateway holding just the +// pinhole. +func defaultDiscoverGateway(ctx context.Context) (nat.NAT, error) { + gatewayCtx, cancel := reserveForPinhole(ctx) + defer cancel() + + gateway, err := discoverNATGateway(gatewayCtx) + if err == nil { + return gateway, nil + } + if !errors.Is(err, nat.ErrNoNATFound) { + return nil, err + } + + pinhole, pinholeErr := discoverPCPPinhole(ctx) + if pinholeErr != nil { + log.Debugf("no IPv6 pinhole after %v: %v", err, pinholeErr) + return nil, err + } + + log.Infof("no IPv4 gateway, continuing with an IPv6 pinhole only") + return pinhole, nil +} + +// reserveForPinhole shortens ctx so that a pinhole probe still has time to run +// afterwards. Finding nothing takes gateway discovery everything it is given, +// so on the unshortened context the probe would start already expired. A budget +// too small to divide is left to gateway discovery, which is the likelier win. +func reserveForPinhole(ctx context.Context) (context.Context, context.CancelFunc) { + deadline, ok := ctx.Deadline() + if !ok { + return context.WithCancel(ctx) + } + + remaining := time.Until(deadline) + if remaining <= pinholeDiscoveryTimeout { + return context.WithCancel(ctx) + } + return context.WithTimeout(ctx, remaining-pinholeDiscoveryTimeout) } // State is persisted only for crash recovery cleanup diff --git a/client/internal/portforward/state_test.go b/client/internal/portforward/state_test.go new file mode 100644 index 000000000..8a584eecb --- /dev/null +++ b/client/internal/portforward/state_test.go @@ -0,0 +1,140 @@ +//go:build !js + +package portforward + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/netbirdio/go-nat" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// stubDiscovery replaces both discovery entry points for the duration of a +// test. gatewayDelay simulates gateway discovery spending everything it is +// given before reporting that it found nothing. +func stubDiscovery(t *testing.T, gateway nat.NAT, gatewayErr error, gatewayDelay time.Duration, pinhole nat.NAT, pinholeErr error) { + t.Helper() + + origGateway, origPinhole := discoverNATGateway, discoverPCPPinhole + discoverNATGateway = func(ctx context.Context) (nat.NAT, error) { + if gatewayDelay > 0 { + select { + case <-time.After(gatewayDelay): + case <-ctx.Done(): + } + } + return gateway, gatewayErr + } + discoverPCPPinhole = func(ctx context.Context) (nat.NAT, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + return pinhole, pinholeErr + } + + t.Cleanup(func() { discoverNATGateway, discoverPCPPinhole = origGateway, origPinhole }) +} + +func TestDefaultDiscoverGateway(t *testing.T) { + ipv4Gateway := &mockNAT{natType: "PCP+PCPv6"} + ipv6Pinhole := &mockNAT{natType: "PCP"} + otherErr := errors.New("routing table unavailable") + + t.Run("an IPv4 gateway is used as is", func(t *testing.T) { + stubDiscovery(t, ipv4Gateway, nil, 0, ipv6Pinhole, nil) + + got, err := defaultDiscoverGateway(context.Background()) + + require.NoError(t, err) + assert.Same(t, ipv4Gateway, got) + }) + + t.Run("no IPv4 gateway still opens an IPv6 pinhole", func(t *testing.T) { + stubDiscovery(t, nil, nat.ErrNoNATFound, 0, ipv6Pinhole, nil) + + got, err := defaultDiscoverGateway(context.Background()) + + require.NoError(t, err) + assert.Same(t, ipv6Pinhole, got) + }) + + t.Run("no gateway and no pinhole reports the original failure", func(t *testing.T) { + stubDiscovery(t, nil, nat.ErrNoNATFound, 0, nil, errors.New("no IPv6 route")) + + got, err := defaultDiscoverGateway(context.Background()) + + assert.Nil(t, got) + assert.ErrorIs(t, err, nat.ErrNoNATFound, "the pinhole failure must not mask why no gateway was found") + }) + + t.Run("a failure other than no-gateway is reported as is", func(t *testing.T) { + stubDiscovery(t, nil, otherErr, 0, ipv6Pinhole, nil) + + got, err := defaultDiscoverGateway(context.Background()) + + assert.Nil(t, got) + assert.ErrorIs(t, err, otherErr) + }) + + t.Run("the pinhole survives gateway discovery using its whole budget", func(t *testing.T) { + // On one shared context the probe would start already expired, which is + // how this failed against a real gateway. + reserve := 50 * time.Millisecond + origReserve := pinholeDiscoveryTimeout + pinholeDiscoveryTimeout = reserve + t.Cleanup(func() { pinholeDiscoveryTimeout = origReserve }) + + budget := 4 * reserve + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + stubDiscovery(t, nil, nat.ErrNoNATFound, budget, ipv6Pinhole, nil) + + got, err := defaultDiscoverGateway(ctx) + + require.NoError(t, err) + assert.Same(t, ipv6Pinhole, got) + }) +} + +func TestReserveForPinhole(t *testing.T) { + origReserve := pinholeDiscoveryTimeout + pinholeDiscoveryTimeout = time.Second + t.Cleanup(func() { pinholeDiscoveryTimeout = origReserve }) + + t.Run("a budget is divided", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + gatewayCtx, cancelGateway := reserveForPinhole(ctx) + defer cancelGateway() + + deadline, ok := gatewayCtx.Deadline() + require.True(t, ok) + assert.InDelta(t, 9*time.Second, time.Until(deadline), float64(500*time.Millisecond)) + }) + + t.Run("a budget too small to divide is left whole", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancel() + + gatewayCtx, cancelGateway := reserveForPinhole(ctx) + defer cancelGateway() + + deadline, ok := gatewayCtx.Deadline() + require.True(t, ok) + assert.InDelta(t, 500*time.Millisecond, time.Until(deadline), float64(100*time.Millisecond)) + }) + + t.Run("no deadline stays unbounded", func(t *testing.T) { + gatewayCtx, cancelGateway := reserveForPinhole(context.Background()) + defer cancelGateway() + + _, ok := gatewayCtx.Deadline() + assert.False(t, ok) + }) +} diff --git a/go.mod b/go.mod index e8d65e568..265cd962f 100644 --- a/go.mod +++ b/go.mod @@ -73,7 +73,6 @@ require ( github.com/hashicorp/go-version v1.7.0 github.com/jackc/pgx/v5 v5.5.5 github.com/libdns/route53 v1.5.0 - github.com/libp2p/go-nat v0.2.0 github.com/libp2p/go-netroute v0.4.0 github.com/lrh3321/ipset-go v0.0.0-20250619021614-54a0a98ace81 github.com/mdlayher/socket v0.5.1 @@ -81,6 +80,7 @@ require ( github.com/miekg/dns v1.1.72 github.com/mitchellh/hashstructure/v2 v2.0.2 github.com/moby/moby/api v1.54.1 + github.com/netbirdio/go-nat v0.0.0-20260821095157-6b2c8c5c74e8 github.com/netbirdio/management-integrations/integrations v0.0.0-20260416123949-2355d972be42 github.com/netbirdio/signal-dispatcher/dispatcher v0.0.0-20250805121659-6b4ac470ca45 github.com/oapi-codegen/runtime v1.1.2 diff --git a/go.sum b/go.sum index 99adaa2cb..d9d880ede 100644 --- a/go.sum +++ b/go.sum @@ -407,8 +407,6 @@ github.com/libdns/libdns v0.2.2 h1:O6ws7bAfRPaBsgAYt8MDe2HcNBGC29hkZ9MX2eUSX3s= github.com/libdns/libdns v0.2.2/go.mod h1:4Bj9+5CQiNMVGf87wjX4CY3HQJypUHRuLvlsfsZqLWQ= github.com/libdns/route53 v1.5.0 h1:2SKdpPFl/qgWsXQvsLNJJAoX7rSxlk7zgoL4jnWdXVA= github.com/libdns/route53 v1.5.0/go.mod h1:joT4hKmaTNKHEwb7GmZ65eoDz1whTu7KKYPS8ZqIh6Q= -github.com/libp2p/go-nat v0.2.0 h1:Tyz+bUFAYqGyJ/ppPPymMGbIgNRH+WqC5QrT5fKrrGk= -github.com/libp2p/go-nat v0.2.0/go.mod h1:3MJr+GRpRkyT65EpVPBstXLvOlAPzUVlG6Pwg9ohLJk= github.com/libp2p/go-netroute v0.4.0 h1:sZZx9hyANYUx9PZyqcgE/E1GUG3iEtTZHUEvdtXT7/Q= github.com/libp2p/go-netroute v0.4.0/go.mod h1:Nkd5ShYgSMS5MUKy/MU2T57xFoOKvvLR92Lic48LEyA= github.com/lrh3321/ipset-go v0.0.0-20250619021614-54a0a98ace81 h1:J56rFEfUTFT9j9CiRXhi1r8lUJ4W5idG3CiaBZGojNU= @@ -480,6 +478,8 @@ github.com/netbirdio/dex/api/v2 v2.0.0-20260512110716-8d70ad8647c1 h1:neE7z+FPUk github.com/netbirdio/dex/api/v2 v2.0.0-20260512110716-8d70ad8647c1/go.mod h1:awuTyT29CYALpEyET0S307EgNlPWrc7fFKRAyhsO45M= github.com/netbirdio/easyjson v0.9.0 h1:6Nw2lghSVuy8RSkAYDhDv1thBVEmfVbKZnV7T7Z6Aus= github.com/netbirdio/easyjson v0.9.0/go.mod h1:1+xMtQp2MRNVL/V1bOzuP3aP8VNwRW55fQUto+XFtTU= +github.com/netbirdio/go-nat v0.0.0-20260821095157-6b2c8c5c74e8 h1:pBxXEsxcsO3qVUND//5j1kelYlO57x5IrRviNF0+0iA= +github.com/netbirdio/go-nat v0.0.0-20260821095157-6b2c8c5c74e8/go.mod h1:mFViabv4PpnoDw9w7W21a7xux6APA4q7KQZRsv4BCl8= github.com/netbirdio/ice/v4 v4.0.0-20250908184934-6202be846b51 h1:Ov4qdafATOgGMB1wbSuh+0aAHcwz9hdvB6VZjh1mVMI= github.com/netbirdio/ice/v4 v4.0.0-20250908184934-6202be846b51/go.mod h1:ZSIbPdBn5hePO8CpF1PekH2SfpTxg1PDhEwtbqZS7R8= github.com/netbirdio/management-integrations/integrations v0.0.0-20260416123949-2355d972be42 h1:F3zS5fT9xzD1OFLfcdAE+3FfyiwjGukF1hvj0jErgs8=