From 915266ffffe4ef05e5e12ece82fb172e1092dd36 Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 4 Aug 2026 18:04:10 +0200 Subject: [PATCH] Allow non strict mode --- client/internal/pqkem/capability_test.go | 64 ++++++++++++++++++++++++ client/internal/pqkem/convergence.go | 2 + client/internal/pqkem/manager.go | 36 ++++++++++++- client/internal/pqkem_adapter.go | 16 ++++++ client/internal/pqkem_adapter_test.go | 37 ++++++++++++++ 5 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 client/internal/pqkem/capability_test.go create mode 100644 client/internal/pqkem_adapter_test.go diff --git a/client/internal/pqkem/capability_test.go b/client/internal/pqkem/capability_test.go new file mode 100644 index 000000000..b42acc79d --- /dev/null +++ b/client/internal/pqkem/capability_test.go @@ -0,0 +1,64 @@ +package pqkem + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestManager_NonCapablePeerNotOffered: a peer known not to run the KEM (it advertised +// no PQ port over signalling) is never offered an exchange, and no failure is raised — +// this is what stops the reoffer storm against non-PQ peers (e.g. Rosenpass peers). +func TestManager_NonCapablePeerNotOffered(t *testing.T) { + wg := newFakeWG() + d := NewManager("bbbb", wg, nil) // initiator vs "aaaa" + d.Start(&loopback{ep: epB, sw: newSwitch()}) + defer d.Stop() + + d.MarkNonCapable("aaaa") + + offer, err := d.SignalOffer("aaaa") + require.NoError(t, err) + require.Nil(t, offer, "a non-capable peer must not be offered a KEM exchange") + require.Empty(t, wg.failed, "a non-capable peer must not raise a rekey failure") +} + +// TestManager_MarkNonCapableCancelsInFlight: if we start an exchange with a peer whose +// capability is not yet known and then learn it does not run the KEM, the in-flight +// exchange is cancelled and no further offer is produced (no timeout -> no failure). +func TestManager_MarkNonCapableCancelsInFlight(t *testing.T) { + wg := newFakeWG() + d := NewManager("bbbb", wg, nil) + d.Start(&loopback{ep: epB, sw: newSwitch()}) + defer d.Stop() + + // Capability unknown -> the bootstrap offer goes out optimistically. + offer, err := d.SignalOffer("aaaa") + require.NoError(t, err) + require.NotNil(t, offer) + + // Now we learn the peer is non-PQ: the exchange must be dropped. + d.MarkNonCapable("aaaa") + + next, err := d.SignalOffer("aaaa") + require.NoError(t, err) + require.Nil(t, next, "after learning non-capability the peer is no longer offered") + require.Empty(t, wg.failed, "cancelling an in-flight exchange must not raise a failure") +} + +// TestManager_EstablishedPeerNotDowngraded: a stray zero-port observation must not tear +// down a peer we already have a working PQ session with. +func TestManager_EstablishedPeerNotDowngraded(t *testing.T) { + dA, dB, _, wgB, _ := pair(t) + defer dA.Stop() + defer dB.Stop() + + bootstrap(t, dA, dB) + require.NotEqual(t, PSK{}, wgB.psk("aaaa"), "established a PSK") + + dB.MarkNonCapable("aaaa") // stray zero after establishment + + offer, err := dB.SignalOffer("aaaa") + require.NoError(t, err) + require.NotNil(t, offer, "an established peer must keep running the KEM despite a stray zero") +} diff --git a/client/internal/pqkem/convergence.go b/client/internal/pqkem/convergence.go index 8094f0e92..0d6085f73 100644 --- a/client/internal/pqkem/convergence.go +++ b/client/internal/pqkem/convergence.go @@ -108,6 +108,7 @@ func (m *Manager) processOffer(remoteID RemoteID, o *OfferMsg) ([]byte, error) { ex.lastSent = raw ex.pendingPSK = psk m.psks[remoteID] = psk + m.capable[remoteID] = true // a real KEM offer proves the peer runs the exchange m.mu.Unlock() m.trace("pqkem: new PSK derived", "peer", remoteID, "exchange", idHex(o.ExchangeID), "role", "responder", "psk_fp", pskFingerprint(psk)) @@ -153,6 +154,7 @@ func (m *Manager) processAnswer(remoteID RemoteID, a *AnswerMsg) error { m.established[remoteID] = true m.failures[remoteID] = 0 m.psks[remoteID] = psk + m.capable[remoteID] = true // a real KEM answer proves the peer runs the exchange m.mu.Unlock() m.trace("pqkem: new PSK derived", "peer", remoteID, "exchange", idHex(a.ExchangeID), "role", "initiator", "psk_fp", pskFingerprint(psk)) diff --git a/client/internal/pqkem/manager.go b/client/internal/pqkem/manager.go index 8306433c7..205194ebd 100644 --- a/client/internal/pqkem/manager.go +++ b/client/internal/pqkem/manager.go @@ -107,6 +107,7 @@ type Manager struct { established map[RemoteID]bool // peer has completed at least one exchange failures map[RemoteID]int // consecutive rekey failures per peer psks map[RemoteID]PSK // latest derived PSK per peer (pulled at WG peer-config time) + capable map[RemoteID]bool // peer runs the KEM (advertised a PQ port); false = known non-capable peerAddrs map[RemoteID]netip.AddrPort // remoteID -> data-path endpoint (send routing) peersByAddr map[netip.AddrPort]RemoteID // reverse: source endpoint -> remoteID (inbound) wait sync.WaitGroup @@ -133,6 +134,7 @@ func NewManager(localID LocalID, h CallbackHandler, logger *slog.Logger) *Manage established: make(map[RemoteID]bool), failures: make(map[RemoteID]int), psks: make(map[RemoteID]PSK), + capable: make(map[RemoteID]bool), peerAddrs: make(map[RemoteID]netip.AddrPort), peersByAddr: make(map[netip.AddrPort]RemoteID), } @@ -184,7 +186,8 @@ func (m *Manager) trace(msg string, args ...any) { } // AddPeer registers where a peer's data-path messages are sent and received: its -// overlay endpoint (IP:port). +// overlay endpoint (IP:port). A peer that advertises a PQ endpoint is, by that fact, +// running the KEM, so it is marked capable. func (m *Manager) AddPeer(remoteID RemoteID, endpoint netip.AddrPort) { if !endpoint.IsValid() { return @@ -195,9 +198,35 @@ func (m *Manager) AddPeer(remoteID RemoteID, endpoint netip.AddrPort) { } m.peerAddrs[remoteID] = endpoint m.peersByAddr[endpoint] = remoteID + m.capable[remoteID] = true m.mu.Unlock() } +// MarkNonCapable records that a peer does not run the KEM: it answered our offer with +// no KEM material over signalling (the capability signal is the peer's payload, not its +// optional data-path port). Any in-flight exchange is cancelled and further offers are +// suppressed (see SignalOffer), so a non-PQ peer never drives the rekey-recovery storm. +// An already-established peer is left untouched — a stray empty answer must not tear +// down a working PQ session. +func (m *Manager) MarkNonCapable(remoteID RemoteID) { + m.mu.Lock() + defer m.mu.Unlock() + if m.established[remoteID] { + return + } + if prev, ok := m.capable[remoteID]; ok && !prev { + return // already known non-capable, nothing to do + } + m.capable[remoteID] = false + if ex := m.exchanges[remoteID]; ex != nil { + if ex.cancel != nil { + ex.cancel() + } + delete(m.exchanges, remoteID) + } + m.trace("pqkem: peer advertises no PQ service — treating as non-capable, no KEM attempted", "peer", remoteID) +} + // RemovePeer stops any in-flight exchange for a peer and drops its state and routing. func (m *Manager) RemovePeer(remoteID RemoteID) { m.mu.Lock() @@ -210,6 +239,7 @@ func (m *Manager) RemovePeer(remoteID RemoteID) { delete(m.established, remoteID) delete(m.failures, remoteID) delete(m.psks, remoteID) + delete(m.capable, remoteID) if ep, ok := m.peerAddrs[remoteID]; ok { delete(m.peersByAddr, ep) delete(m.peerAddrs, remoteID) @@ -246,6 +276,10 @@ func (m *Manager) SignalOffer(remoteID RemoteID) ([]byte, error) { return nil, nil } m.mu.Lock() + if capable, ok := m.capable[remoteID]; ok && !capable { + m.mu.Unlock() + return nil, nil // peer does not run the KEM; do not offer (avoids a failure/reoffer loop) + } if ex := m.exchanges[remoteID]; ex != nil && ex.viaSignal && ex.state == stateAwaitingAnswer { last := ex.lastSent m.mu.Unlock() diff --git a/client/internal/pqkem_adapter.go b/client/internal/pqkem_adapter.go index d99eb1b5d..332dc4da4 100644 --- a/client/internal/pqkem_adapter.go +++ b/client/internal/pqkem_adapter.go @@ -64,6 +64,14 @@ func (p pqHandshaker) OfferPayload(remoteKey string) ([]byte, int) { func (p pqHandshaker) AnswerPayload(remoteKey string, recvOffer []byte) ([]byte, int) { if len(recvOffer) == 0 { + // Capability signal (responder side): the KEM offer flows initiator->responder, + // so if we are the responder for this peer (it is the KEM initiator by role) an + // empty offer means it does not run the KEM. If we are the initiator, an empty + // offer is normal — the peer is the responder and puts its material in the + // answer — so we must not flag it. + if !p.mgr.IsInitiator(pqkem.RemoteID(remoteKey)) { + p.mgr.MarkNonCapable(pqkem.RemoteID(remoteKey)) + } return nil, p.mgr.LocalPort() } payload, err := p.mgr.SignalOnOffer(pqkem.RemoteID(remoteKey), recvOffer) @@ -75,6 +83,14 @@ func (p pqHandshaker) AnswerPayload(remoteKey string, recvOffer []byte) ([]byte, func (p pqHandshaker) OnAnswer(remoteKey string, recvAnswer []byte) { if len(recvAnswer) == 0 { + // Capability signal (initiator side): the KEM answer flows responder->initiator, + // so an empty answer to our offer means the peer does not run the KEM — mark it + // non-capable to stop offering (no failure/reoffer storm). Only meaningful when + // we are the initiator: as the responder we also receive an (empty) answer to + // our own non-KEM offer from a perfectly capable peer, which must not be flagged. + if p.mgr.IsInitiator(pqkem.RemoteID(remoteKey)) { + p.mgr.MarkNonCapable(pqkem.RemoteID(remoteKey)) + } return } if err := p.mgr.SignalOnAnswer(pqkem.RemoteID(remoteKey), recvAnswer); err != nil { diff --git a/client/internal/pqkem_adapter_test.go b/client/internal/pqkem_adapter_test.go new file mode 100644 index 000000000..dec3b5943 --- /dev/null +++ b/client/internal/pqkem_adapter_test.go @@ -0,0 +1,37 @@ +package internal + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/pqkem" +) + +type pqNoopHandler struct{} + +func (pqNoopHandler) OnNewPSKReady(pqkem.RemoteID, pqkem.PSK) error { return nil } +func (pqNoopHandler) OnRekeyFailed(pqkem.RemoteID) error { return nil } + +// TestPQAdapter_CapabilityRoleAware locks the role-aware capability signal: the KEM +// payload only flows initiator-offer -> responder-answer, so an empty message in the +// other direction comes from a perfectly capable peer and must NOT flag it. Only the +// message that should carry material (the answer we receive as initiator) marks a peer +// non-capable when empty. +func TestPQAdapter_CapabilityRoleAware(t *testing.T) { + // localID "zzzz" > "aaaa" => this manager is the KEM initiator for peer "aaaa". + mgr := pqkem.NewManager("zzzz", pqNoopHandler{}, nil) + defer mgr.Stop() + h := pqHandshaker{mgr: mgr} + + // An empty OFFER from our peer is normal here: as the initiator's responder it puts + // its material in the answer, not the offer. It must not disable our offering. + h.AnswerPayload("aaaa", nil) + payload, _ := h.OfferPayload("aaaa") + require.NotNil(t, payload, "an empty offer from a responder-role peer must not mark it non-capable") + + // An empty ANSWER to our offer means the peer does not run the KEM -> stop offering. + h.OnAnswer("aaaa", nil) + payload2, _ := h.OfferPayload("aaaa") + require.Nil(t, payload2, "an empty answer to our offer marks the peer non-capable, so we stop offering") +}