diff --git a/design/multiport.md b/design/multiport.md index 8c49e7fe..dbeaaf3f 100644 --- a/design/multiport.md +++ b/design/multiport.md @@ -415,10 +415,13 @@ all. ### Per-tunnel: back to a single session -`newLaneSet` returns `nil`, and the tunnel is an ordinary one, when: +The tunnel gets no lane set and is an ordinary one when: -- the peer advertised no port count (vanilla peer, or multiport off there), or -- `sessions < 2`, i.e. neither side offers a lane. +- the peer advertised no port count (vanilla peer, or multiport off there), +- the peer's advertised range is invalid -- a zero base port, or a range running + past 65535. `maybeAllocLanes` logs `Ignoring multiport advert with an invalid + port range` rather than aiming lanes at ports that wrapped, or +- `sessions < 2`, i.e. neither side offers a lane (`newLaneSet` returns `nil`). `txLanes` can also land at 1 -- we bound ports but the peer bound only one -- in which case the set exists for RX but we never send on a lane. @@ -520,7 +523,9 @@ up never demotes them. Logs worth grepping: `multiport enabled` and `multiport routines` at startup, the `lanes` attr on handshake completion (`tx`, `sessions`, `peerBasePort`, `peerPorts`, `portOffset`), and `Multiport lane up` / `Multiport lane demoted`, -both of which name the `udpAddr` involved. +both of which name the `udpAddr` involved. A lane that has never come up logs +`Multiport lane not coming up` at Warn, once, after 3 unanswered probes in a +row -- usually a sign that only the base port is open. ## Known gaps diff --git a/handshake/machine.go b/handshake/machine.go index ab82fee9..f4db7262 100644 --- a/handshake/machine.go +++ b/handshake/machine.go @@ -345,8 +345,8 @@ func (m *Machine) processPayload(msg []byte, flags msgFlags) error { // Multiport advert from the peer's side of the exchange. Out-of-range // values mean a peer we can't pair lanes with; ignore the advert // rather than failing the handshake — the tunnel itself is fine, it - // just won't get lanes. Semantic policing (port-count caps, lane - // clamping) belongs to the handshake manager. + // just won't get lanes. Semantic policing (the port range, lane + // clamping) belongs to the handshake manager and newLaneSet. var peerLanes *LaneDetails if m.result.Initiator { peerLanes = payload.ResponderLanes diff --git a/handshake_manager.go b/handshake_manager.go index 8e4763fc..65da1d61 100644 --- a/handshake_manager.go +++ b/handshake_manager.go @@ -732,6 +732,13 @@ func (hm *HandshakeManager) maybeAllocLanes(hostinfo *HostInfo, result *handshak if len(hm.f.myVpnAddrs) == 0 || len(hostinfo.vpnAddrs) == 0 { return } + if !validLaneAdvert(result.PeerBasePort, result.PeerPortCount) { + // Lane targets are computed in uint16, so a range like this would wrap + // onto low ports. The tunnel itself is fine; it just gets no lanes. + hostinfo.logger(hm.l).Warn("Ignoring multiport advert with an invalid port range", + "peerBasePort", result.PeerBasePort, "peerPorts", result.PeerPortCount) + return + } hostinfo.lanes = newLaneSet(result, hm.config.laneCount, hm.f.myVpnAddrs[0], hostinfo.vpnAddrs[0]) } diff --git a/lanes.go b/lanes.go index 19cb56bd..b031fa7b 100644 --- a/lanes.go +++ b/lanes.go @@ -64,6 +64,11 @@ const ( // laneMaxFails caps the failure counter; the backoff saturates well before. laneMaxFails = 8 + // laneWarnFails is how many unanswered probes a lane that has never come up + // takes before it is worth a warning. One or two can be loss; this many in a + // row is usually a firewall that only opens the base port. + laneWarnFails = 3 + // laneProbeTimeout is how long a probe may go unacked before it counts as a // failure. It is shorter than the connection manager's check interval on // purpose: an outstanding probe is judged on the next tick either way, and a @@ -170,6 +175,12 @@ type laneProbeState struct { retryAt time.Time } +// validLaneAdvert reports whether a peer's advertised port range is one we can +// aim lanes at: a real base port, and every port in the range a valid one. +func validLaneAdvert(basePort, portCount uint32) bool { + return basePort != 0 && portCount != 0 && basePort+portCount-1 <= 0xffff +} + // newLaneSet sets up the lanes for a freshly completed base handshake. It // returns nil when the pair has no lane beyond the base tunnel, which is the // normal answer for a peer running without multiport. No session is derived @@ -592,6 +603,11 @@ func (f *Interface) probeLanes(hostinfo *HostInfo, now time.Time, nb, out []byte if up { ls.txAddr[s].Store(nil) hostinfo.logger(f.l).Info("Multiport lane demoted, probe unanswered", "lane", s, "udpAddr", p.target) + } else if p.lastAck.IsZero() && p.fails == laneWarnFails { + // Said once per lane: fails only passes through this value again + // after an ack or a reset clears it. + hostinfo.logger(f.l).Warn("Multiport lane not coming up, probes unanswered; check that the peer's lane ports are reachable", + "lane", s, "udpAddr", p.target, "fails", p.fails) } continue } diff --git a/lanes_test.go b/lanes_test.go index cdf39418..99b11ea7 100644 --- a/lanes_test.go +++ b/lanes_test.go @@ -1,8 +1,10 @@ package nebula import ( + "bytes" "log/slog" "net/netip" + "strings" "testing" "time" @@ -1107,3 +1109,67 @@ func TestHandshakeManagerVpnIpsIdentityDelete(t *testing.T) { hm.DeleteHostInfo(pendingBase) assert.Nil(t, hm.QueryVpnAddr(vpnIp)) } + +func TestValidLaneAdvert(t *testing.T) { + assert.True(t, validLaneAdvert(4242, 4)) + assert.True(t, validLaneAdvert(65535, 1)) + assert.True(t, validLaneAdvert(65532, 4), "a range ending on 65535 is valid") + assert.False(t, validLaneAdvert(0, 4), "no base port") + assert.False(t, validLaneAdvert(4242, 0), "no ports") + assert.False(t, validLaneAdvert(65533, 4), "the range runs past 65535") +} + +// A peer advertising a port range that would wrap gets a plain tunnel, and a +// warning saying why, rather than lanes aimed at low ports. +func TestMaybeAllocLanesRejectsInvalidRange(t *testing.T) { + hostMap := newHostMap(test.NewLogger()) + ifce := newLaneTestInterface(hostMap) + ifce.myVpnAddrs = []netip.Addr{testMyAddr} + hm := ifce.handshakeManager + hm.config.laneCount = 4 + var buf bytes.Buffer + hm.l = test.NewLoggerWithOutputAndLevel(&buf, slog.LevelWarn) + + initR, _ := runTestHandshake(t) + + hi := &HostInfo{vpnAddrs: []netip.Addr{testPeerAddr}} + initR.PeerBasePort, initR.PeerPortCount, initR.PeerTxLanes = 65530, 16, 4 + hm.maybeAllocLanes(hi, initR) + assert.Nil(t, hi.lanes) + assert.Contains(t, buf.String(), "invalid port range") + + hi = &HostInfo{vpnAddrs: []netip.Addr{testPeerAddr}} + initR.PeerBasePort = 4242 + hm.maybeAllocLanes(hi, initR) + assert.NotNil(t, hi.lanes) +} + +// A lane that has never come up warns once after laneWarnFails unanswered +// probes, and not again while it keeps failing. +func TestLaneNotComingUpWarnsOnce(t *testing.T) { + hostMap := newHostMap(test.NewLogger()) + ifce := newLaneTestInterface(hostMap) + var buf bytes.Buffer + ifce.l = test.NewLoggerWithOutputAndLevel(&buf, slog.LevelWarn) + + initR, _ := runTestHandshake(t) + ls := newTestLaneSet(t, initR, 2, 2, 4242, 2) + hi := newTestLaneHostInfo(t, initR, ls) + + nb := make([]byte, 12) + out := make([]byte, mtu) + now := time.Now() + const msg = "Multiport lane not coming up" + for i := 1; i <= laneWarnFails+3; i++ { + ls.demand[1].Store(true) + ifce.probeLanes(hi, now, nb, out) + now = now.Add(laneProbeTimeout) + ifce.probeLanes(hi, now, nb, out) + now = now.Add(laneRetryMax) + + if i < laneWarnFails { + require.NotContains(t, buf.String(), msg, "warned after %d failures", i) + } + } + assert.Equal(t, 1, strings.Count(buf.String(), msg)) +}