diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index ca341cc11..e0e303d8a 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -86,8 +86,8 @@ type PQHandshaker interface { // peer is not the KEM initiator) and the local PQ data-path port to announce. OfferPayload(remoteKey string) (payload []byte, port uint16) // ShouldSendBootstrapOffer reports whether, as the controller, we should reply to a - // received responder offer with our own KEM offer (true only when no exchange is - // already in flight — so we kick the KEM once and ignore further offers). + // 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 // 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. diff --git a/client/internal/peer/handshaker.go b/client/internal/peer/handshaker.go index e81fc4cf5..c7789847b 100644 --- a/client/internal/peer/handshaker.go +++ b/client/internal/peer/handshaker.go @@ -208,11 +208,10 @@ func (h *Handshaker) handleRemoteAnswer(remoteOfferAnswer OfferAnswer) { // // 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 an exchange is already in flight (e.g. every PQ peer past its bootstrap, -// which is the steady state under lazy connections) — it returns false so the caller keeps -// handling the offer normally (sendAnswer + notifyListeners). Returning true unconditionally -// would drop the ICE credentials and relay info the responder's offer carries, stalling a -// responder-initiated reconnect. +// 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. func (h *Handshaker) pqControllerReoffer() bool { if h.config.PQ == nil || !isController(h.config) { return false diff --git a/client/internal/pqkem/manager.go b/client/internal/pqkem/manager.go index f2ba97031..caa80bfab 100644 --- a/client/internal/pqkem/manager.go +++ b/client/internal/pqkem/manager.go @@ -374,7 +374,7 @@ func (m *Manager) SignalOffer(remoteID RemoteID) ([]byte, error) { // still-awaitingRekey side would reject — a permanent desync. Starting a fresh exchange // re-bootstraps and both sides converge (the responder always adopts the latest). The // controller-double-offer case the idempotency once guarded is already covered by - // ShouldSendBootstrapOffer (false while any exchange exists). + // ShouldSendBootstrapOffer (false while an exchange is in flight). if ex := m.exchanges[remoteID]; ex != nil && ex.viaSignal && ex.state == stateAwaitingAnswer { last := ex.lastSent m.mu.Unlock() @@ -395,11 +395,14 @@ func (m *Manager) SignalOffer(remoteID RemoteID) ([]byte, error) { } // ShouldSendBootstrapOffer reports whether we should emit a fresh KEM offer to kick a -// bootstrap for this peer. True only if we are the initiator, the peer is not known -// non-capable, and no exchange is already in flight. The host uses this when it (as the -// controller) receives the responder's offer: it replies with a KEM offer exactly once -// to start the exchange, and ignores further responder offers while one is in flight, -// avoiding an offer-per-offer runaway. +// bootstrap for this peer. True if we are the initiator, the peer is not known +// non-capable, and no exchange of ours is in flight. The host uses this when it (as the +// controller) receives the responder's offer, which never carries KEM material: it replies +// with a KEM offer, and ignores further responder offers while that one is in flight, +// avoiding an offer-per-offer runaway. A converged exchange does not count as in flight: a +// responder offer then means the responder is renegotiating (a reconnect or a wake), and +// our current PSK may be the reason, e.g. split by a rotation whose answers were lost. A +// fresh bootstrap re-aligns both sides, so that reconnect cannot come back on a stale key. func (m *Manager) ShouldSendBootstrapOffer(remoteID RemoteID) bool { if !m.IsInitiator(remoteID) { return false @@ -409,7 +412,8 @@ func (m *Manager) ShouldSendBootstrapOffer(remoteID RemoteID) bool { if capable, ok := m.capable[remoteID]; ok && !capable { return false } - return m.exchanges[remoteID] == nil + ex := m.exchanges[remoteID] + return ex == nil || ex.state == stateAwaitingRekey } // SignalOnOffer processes a KEM offer the host extracted from an incoming offer and diff --git a/client/internal/pqkem/split_test.go b/client/internal/pqkem/split_test.go new file mode 100644 index 000000000..5edd9764e --- /dev/null +++ b/client/internal/pqkem/split_test.go @@ -0,0 +1,69 @@ +package pqkem + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestManager_ResponderOfferRebootstrapsSplitKeys reproduces a split PSK and checks that a +// reconnect started by the responder heals it. When every answer of a rotation is lost, +// the responder has committed the new PSK and the initiator has not, so WireGuard can no +// longer handshake. The connection layer then reconnects, and the reconnect offer may come +// from the responder, carrying no KEM. The controller used to answer it normally whenever +// it held a converged exchange, keeping its stale key, so the tunnel stayed down. A +// responder offer must instead be answered with a fresh KEM bootstrap. +func TestManager_ResponderOfferRebootstrapsSplitKeys(t *testing.T) { + sw := newSwitch() + wgA, wgB := newFakeWG(), newFakeWG() + dA := NewManager("aaaa", wgA, nil) // responder + dB := NewManager("bbbb", wgB, nil) // initiator (controller) + dB.retryInterval = 10 * time.Millisecond + dB.maxRetries = 2 + lbA := &loopback{ep: epA, sw: sw} + dA.Start(lbA) + dB.Start(&loopback{ep: epB, sw: sw}) + dA.AddPeer("bbbb", epB) + dB.AddPeer("aaaa", epA) + defer dA.Stop() + defer dB.Stop() + + bootstrap(t, dA, dB) + + // Lose every answer of a rotation: the responder commits, the initiator times out. + lbA.drop.Store(true) + dB.OnDataPathRekeyed("aaaa", 0) + require.Eventually(t, func() bool { + dB.mu.Lock() + defer dB.mu.Unlock() + ex := dB.exchanges["aaaa"] + return ex == nil || ex.state != stateAwaitingAnswer + }, time.Second, 5*time.Millisecond, "the rotation must time out on the initiator") + lbA.drop.Store(false) + require.NotEqual(t, wgA.psk("bbbb"), wgB.psk("aaaa"), "precondition: the two PSKs are split") + + // The responder's reconnect offer carries no KEM: the controller must answer it with + // a fresh bootstrap, which brings both peers back to one PSK. + require.True(t, dB.ShouldSendBootstrapOffer("aaaa"), + "a controller holding a converged exchange must still re-bootstrap on a responder offer") + bootstrap(t, dA, dB) + assert.Equal(t, wgA.psk("bbbb"), wgB.psk("aaaa"), "the re-bootstrap must heal the split PSKs") +} + +// TestManager_NoRebootstrapWhileExchangeInFlight keeps the glare protection: while the +// controller's own exchange is waiting for its answer, a crossing responder offer must not +// start a second exchange, which would supersede the first and churn the keys. +func TestManager_NoRebootstrapWhileExchangeInFlight(t *testing.T) { + dA, dB, _, _, _ := pair(t) + defer dA.Stop() + defer dB.Stop() + + offer, err := dB.SignalOffer("aaaa") + require.NoError(t, err) + require.NotNil(t, offer, "the controller's bootstrap must be in flight") + + assert.False(t, dB.ShouldSendBootstrapOffer("aaaa"), + "no second exchange while the controller's own offer awaits its answer") +}