diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index 9de4c8000..c74583e32 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -89,6 +89,9 @@ type PQHandshaker interface { // received responder offer with our own KEM offer (true unless an exchange of ours is // already in flight, so we kick the KEM once and ignore further offers meanwhile). ShouldSendBootstrapOffer(remoteKey string) bool + // SignalOfferInFlight reports whether, as the controller, our KEM offer over + // signalling is still waiting for its answer, so a crossing responder offer is a glare. + SignalOfferInFlight(remoteKey string) bool // AnswerPayload processes a received KEM offer (nil if absent) and returns the KEM // answer to embed in the outgoing answer (nil if none) and the local PQ port. AnswerPayload(remoteKey string, recvOffer []byte) (payload []byte, port uint16) diff --git a/client/internal/peer/conn_pq_test.go b/client/internal/peer/conn_pq_test.go index 6c14fe6cd..85e7d714c 100644 --- a/client/internal/peer/conn_pq_test.go +++ b/client/internal/peer/conn_pq_test.go @@ -25,6 +25,7 @@ type fakePQ struct { func (f fakePQ) OfferPayload(string) ([]byte, uint16) { return nil, 0 } func (f fakePQ) ShouldSendBootstrapOffer(string) bool { return false } +func (f fakePQ) SignalOfferInFlight(string) bool { return false } func (f fakePQ) AnswerPayload(string, []byte) ([]byte, uint16) { return nil, 0 } func (f fakePQ) OnAnswer(string, []byte) {} func (f fakePQ) PSK(string) (wgtypes.Key, bool) { return f.psk, f.ok } diff --git a/client/internal/peer/handshaker.go b/client/internal/peer/handshaker.go index c7789847b..c168a46eb 100644 --- a/client/internal/peer/handshaker.go +++ b/client/internal/peer/handshaker.go @@ -206,16 +206,20 @@ func (h *Handshaker) handleRemoteAnswer(remoteOfferAnswer OfferAnswer) { // one is in flight are ignored (re-sending on every responder offer would be a runaway). // The re-offer reuses our stable ICE session id, so the peer dedups repeats. // -// Returns true ONLY when it actually sent a re-offer and thus took ownership of the peer's -// offer (the caller must not answer it). When there is nothing to re-offer — the peer is -// non-capable, or our own exchange is already in flight (a crossing offer) — it returns -// false so the caller keeps handling the offer normally (sendAnswer + notifyListeners). -// Returning true without sending would drop the ICE credentials and relay info the -// responder's offer carries, stalling a responder-initiated reconnect. +// Returns true when the caller must not answer the peer's offer: either a re-offer went +// out, or our own KEM offer is already in flight (an offer glare). In the glare the +// peer's credentials reach us in its answer to our offer, and a KEM-less answer from us +// could let the peer bring WireGuard up before it reads our offer, on no PSK at all. +// Otherwise, as for a non-capable peer, it returns false and the caller answers normally: +// dropping the offer then would lose the only copy of the peer's ICE and relay details. func (h *Handshaker) pqControllerReoffer() bool { if h.config.PQ == nil || !isController(h.config) { return false } + if h.config.PQ.SignalOfferInFlight(h.config.Key) { + h.log.Debugf("pqkem: controller received a responder offer while its own KEM offer is in flight, leaving it unanswered") + return true + } if !h.config.PQ.ShouldSendBootstrapOffer(h.config.Key) { h.log.Debugf("pqkem: controller received a responder offer with no fresh KEM offer to send, handling it normally") return false diff --git a/client/internal/peer/handshaker_pq_test.go b/client/internal/peer/handshaker_pq_test.go index ec28954aa..22da26599 100644 --- a/client/internal/peer/handshaker_pq_test.go +++ b/client/internal/peer/handshaker_pq_test.go @@ -14,13 +14,15 @@ import ( sProto "github.com/netbirdio/netbird/shared/signal/proto" ) -// bootstrapPQ is a fakePQ whose controller decision is set by the test. +// bootstrapPQ is a fakePQ whose controller decisions are set by the test. type bootstrapPQ struct { fakePQ bootstrap bool + inFlight bool } func (b bootstrapPQ) ShouldSendBootstrapOffer(string) bool { return b.bootstrap } +func (b bootstrapPQ) SignalOfferInFlight(string) bool { return b.inFlight } // newControllerHandshaker builds a handshaker for the controller side of a peer pair and // records the type of every signalling message it sends. @@ -62,13 +64,28 @@ func TestHandshaker_ControllerReoffersOnResponderOffer(t *testing.T) { "the controller must reply with its own offer, not an answer") } -// TestHandshaker_ControllerAnswersWhileExchangeInFlight keeps the glare protection: with -// the controller's own exchange in flight, the responder's offer gets a normal answer. -func TestHandshaker_ControllerAnswersWhileExchangeInFlight(t *testing.T) { - h, sent := newControllerHandshaker(t, bootstrapPQ{bootstrap: false}) +// TestHandshaker_ControllerIgnoresResponderOfferInGlare covers an offer glare: the +// controller's KEM offer and the responder's KEM-less offer cross. The controller must leave +// the responder's offer unanswered. A KEM-less answer could reach the responder before the +// KEM offer and let it bring WireGuard up with no PSK. +func TestHandshaker_ControllerIgnoresResponderOfferInGlare(t *testing.T) { + h, sent := newControllerHandshaker(t, bootstrapPQ{inFlight: true}) + notified := false + h.AddICEListener(func(*OfferAnswer) { notified = true }) // called synchronously + + h.handleRemoteOffer(OfferAnswer{IceCredentials: IceCredentials{UFrag: "ufrag", Pwd: "pwd"}}) + + assert.Empty(t, sent(), "in a glare the controller must send neither an answer nor a second offer") + assert.False(t, notified, "in a glare the controller must not bring the connection up from the responder's offer") +} + +// TestHandshaker_ControllerAnswersNonCapablePeer keeps the normal answer when there is no +// KEM to run with the peer: its offer is then the only copy of its connection details. +func TestHandshaker_ControllerAnswersNonCapablePeer(t *testing.T) { + h, sent := newControllerHandshaker(t, bootstrapPQ{}) h.handleRemoteOffer(OfferAnswer{}) assert.Equal(t, []sProto.Body_Type{sProto.Body_ANSWER}, sent(), - "with an exchange in flight the controller must answer normally") + "without a KEM to run the controller must answer normally") } diff --git a/client/internal/pqkem/manager.go b/client/internal/pqkem/manager.go index d05b45b3f..6d24731a0 100644 --- a/client/internal/pqkem/manager.go +++ b/client/internal/pqkem/manager.go @@ -417,6 +417,19 @@ func (m *Manager) ShouldSendBootstrapOffer(remoteID RemoteID) bool { return ex == nil || ex.state == stateAwaitingRekey } +// SignalOfferInFlight reports whether we are the initiator and our bootstrap offer over +// signalling still awaits its answer. A data-path rotation in flight does not count: no +// offer of ours is crossing the peer's on the signalling channel then. +func (m *Manager) SignalOfferInFlight(remoteID RemoteID) bool { + if !m.IsInitiator(remoteID) { + return false + } + m.mu.Lock() + defer m.mu.Unlock() + ex := m.exchanges[remoteID] + return ex != nil && ex.viaSignal && ex.state == stateAwaitingAnswer +} + // SignalOnOffer processes a KEM offer the host extracted from an incoming offer and // returns the KEM answer for the host to embed in its outgoing answer. Every failure // still returns an error marker: an empty answer means "this peer does not run the KEM" diff --git a/client/internal/pqkem/split_test.go b/client/internal/pqkem/split_test.go index 5edd9764e..bfc0e1b41 100644 --- a/client/internal/pqkem/split_test.go +++ b/client/internal/pqkem/split_test.go @@ -67,3 +67,29 @@ func TestManager_NoRebootstrapWhileExchangeInFlight(t *testing.T) { assert.False(t, dB.ShouldSendBootstrapOffer("aaaa"), "no second exchange while the controller's own offer awaits its answer") } + +// TestManager_SignalOfferInFlight: only our own bootstrap offer over signalling, still +// awaiting its answer, makes a crossing responder offer a glare. A converged exchange, a +// data-path rotation, or the responder role must not. +func TestManager_SignalOfferInFlight(t *testing.T) { + dA, dB, _, _, _ := pair(t) // dB ("bbbb") initiator + defer dA.Stop() + defer dB.Stop() + + assert.False(t, dB.SignalOfferInFlight("aaaa"), "no exchange yet: nothing in flight") + + offer, err := dB.SignalOffer("aaaa") + require.NoError(t, err) + require.NotNil(t, offer) + assert.True(t, dB.SignalOfferInFlight("aaaa"), "our bootstrap offer awaits its answer") + assert.False(t, dA.SignalOfferInFlight("bbbb"), "the responder never has a signalling offer in flight") + + answer, err := dA.SignalOnOffer("bbbb", offer) + require.NoError(t, err) + require.NoError(t, dB.SignalOnAnswer("aaaa", answer)) + assert.False(t, dB.SignalOfferInFlight("aaaa"), "a converged exchange is not in flight") + + _, err = dB.startExchangeTest("aaaa", false, ExchangeID{}) + require.NoError(t, err) + assert.False(t, dB.SignalOfferInFlight("aaaa"), "a data-path rotation is not a signalling offer") +} diff --git a/client/internal/pqkem_adapter.go b/client/internal/pqkem_adapter.go index 8d7a66950..3f8e3992b 100644 --- a/client/internal/pqkem_adapter.go +++ b/client/internal/pqkem_adapter.go @@ -187,6 +187,12 @@ func (p pqHandshaker) ShouldSendBootstrapOffer(remoteKey string) bool { return p.mgr.ShouldSendBootstrapOffer(pqkem.RemoteID(remoteKey)) } +// SignalOfferInFlight reports whether our KEM offer over signalling to the peer still +// awaits its answer. +func (p pqHandshaker) SignalOfferInFlight(remoteKey string) bool { + return p.mgr.SignalOfferInFlight(pqkem.RemoteID(remoteKey)) +} + // AnswerPayload processes a received KEM offer (nil when absent) and returns the KEM // answer to attach to the outgoing signalling answer, plus the data-path port to announce // (0 when on DefaultPort). An empty offer is treated as a capability signal.