mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-21 12:59:08 +02:00
[client] pqkem: answer every responder offer with a fresh KEM bootstrap
Only the controller's offer carries KEM material, so when the responder starts a negotiation the controller is meant to reply with its own KEM offer. It only did so before the very first exchange: once it held a converged PSK it answered the responder's offer normally and kept that PSK, on the assumption that a stale key would heal through the WireGuard watcher reconnecting over Signal. That assumption fails when the reconnect comes from the responder. If every answer of a data-path rotation is lost, the responder commits the new PSK and the controller does not, so WireGuard cannot handshake and no further rotation runs to escalate. The watcher closes the connection after ~3.5 minutes and ICE reconnects in seconds, but the reconnect offer came from the responder, the controller kept its stale key, and the tunnel stayed down. In the lab it took 201 s and a lazy-connection idle/wake cycle to heal; with lazy connections off, the default, nothing would heal it. Treat a converged exchange as not in flight: a responder offer then always gets a fresh bootstrap, so any responder-started renegotiation re-aligns both PSKs. While the controller's own exchange is still in flight a crossing offer is answered normally as before, so offers cannot multiply. The cost is one extra Signal KEM round per responder-started reconnect.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
Reference in New Issue
Block a user