[client] pqkem: leave a responder offer unanswered during an offer glare

When the controller's KEM offer and the responder's offer cross, the
controller used to answer the responder's offer without KEM. If the
responder read that answer before the KEM offer, it could bring WireGuard
up on no PSK and open a classical session. The responder's details still
reach the controller in its answer to the KEM offer.

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)

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 }

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

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")
 }

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"

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")
+}

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.