From bac810b3123de3e9f099f88748bcc8bd045bd93d Mon Sep 17 00:00:00 2001 From: riccardom Date: Thu, 6 Aug 2026 15:15:43 +0200 Subject: [PATCH] Revert "Introduces a forced WG handshake on initial MLKEM bootstrap." This reverts commit b5a72eca65207cbbc0faba73bc067cecdcc30e44. --- client/internal/engine.go | 8 ------- client/internal/peer/conn.go | 29 ----------------------- client/internal/peer/endpoint.go | 33 +-------------------------- client/internal/pqkem/callbacks.go | 8 +------ client/internal/pqkem/convergence.go | 8 +++---- client/internal/pqkem/manager.go | 8 +++---- client/internal/pqkem/manager_test.go | 2 +- client/internal/pqkem_adapter.go | 18 ++------------- client/internal/pqkem_adapter_test.go | 4 ++-- 9 files changed, 15 insertions(+), 103 deletions(-) diff --git a/client/internal/engine.go b/client/internal/engine.go index 231ab7574..96e1ee827 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -670,14 +670,6 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL) conn.RequestReoffer() } }, - // On a freshly bootstrapped PSK, force the peer's WireGuard session to - // re-handshake so it adopts the PSK even if the tunnel already came up on - // a pre-PQ key (KEM-vs-endpoint-config race). - rehandshake: func(remoteKey string) { - if conn, ok := e.peerStore.PeerConn(remoteKey); ok { - conn.ForcePQRehandshake() - } - }, } e.pqkemManager = pqkem.NewManager(pqkem.LocalID(publicKey.String()), cbHandler, pqkem.NewLogger()) e.pqkemManager.Start(tr) diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index 93bacd12a..526060977 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -738,35 +738,6 @@ func (conn *Conn) RequestReoffer() { } } -// ForcePQRehandshake makes WireGuard adopt a freshly bootstrapped post-quantum PSK on -// an already-up session. The KEM exchange can complete after the WG endpoint was -// configured (a race between the KEM and the relay/ICE connection coming up), so the -// live session may be keyed with a pre-PQ key (the strict sentinel, or the ordinary -// key in permissive mode); SetPresharedKey only updates config, not the running -// session. Removing and re-adding the peer forces a fresh handshake that uses the real -// PSK. No-op if not connected yet (the upcoming endpoint config will pull the PSK) or -// if no PSK exists (a non-PQ peer stays fail-closed). -func (conn *Conn) ForcePQRehandshake() { - conn.mu.Lock() - defer conn.mu.Unlock() - - if conn.ctx.Err() != nil || conn.config.PQ == nil { - return - } - psk, ok := conn.config.PQ.PSK(conn.config.Key) - if !ok { - return - } - if conn.currentConnPriority == conntype.None { - return - } - conn.Log.Debugf("pqkem: bootstrap PSK ready, forcing WireGuard re-handshake to adopt it") - wgPsk := wgtypes.Key(psk) - if err := conn.endpointUpdater.ForceRehandshake(&wgPsk); err != nil { - conn.Log.Warnf("pqkem: force re-handshake failed: %v", err) - } -} - func (conn *Conn) onWGDisconnected(watcherCtx context.Context) { conn.mu.Lock() defer conn.mu.Unlock() diff --git a/client/internal/peer/endpoint.go b/client/internal/peer/endpoint.go index 0d741f74b..9ba1efb6e 100644 --- a/client/internal/peer/endpoint.go +++ b/client/internal/peer/endpoint.go @@ -20,13 +20,10 @@ type EndpointUpdater struct { wgConfig WgConfig initiator bool - // mu protects cancelFunc and lastEndpoint + // mu protects cancelFunc mu sync.Mutex cancelFunc func() updateWg sync.WaitGroup - // lastEndpoint is the most recent non-nil endpoint applied to the peer, used to - // re-add it on a forced re-handshake (ForceRehandshake). - lastEndpoint *net.UDPAddr } func NewEndpointUpdater(log *logrus.Entry, wgConfig WgConfig, initiator bool) *EndpointUpdater { @@ -41,10 +38,6 @@ func (e *EndpointUpdater) ConfigureWGEndpoint(addr *net.UDPAddr, presharedKey *w e.mu.Lock() defer e.mu.Unlock() - if addr != nil { - e.lastEndpoint = addr - } - if e.initiator { e.log.Debugf("configure up WireGuard as initiator") return e.configureAsInitiator(addr, presharedKey) @@ -58,36 +51,12 @@ func (e *EndpointUpdater) SwitchWGEndpoint(addr *net.UDPAddr, presharedKey *wgty e.mu.Lock() defer e.mu.Unlock() - if addr != nil { - e.lastEndpoint = addr - } - // prevent to run new update while cancel the previous update e.waitForCloseTheDelayedUpdate() return e.updateWireGuardPeer(addr, presharedKey) } -// ForceRehandshake removes and re-adds the peer so WireGuard drops the current session -// and negotiates a fresh one with the given PSK. Used when a post-quantum PSK is -// bootstrapped after the session already came up on a pre-PQ key. No-op if no endpoint -// has been applied yet (the pending config will pull the PSK itself). -func (e *EndpointUpdater) ForceRehandshake(presharedKey *wgtypes.Key) error { - e.mu.Lock() - defer e.mu.Unlock() - - if e.lastEndpoint == nil { - return nil - } - // Cancel any pending delayed responder update: it carries the stale pre-PQ key and - // would otherwise re-poison the session after we reset it. - e.waitForCloseTheDelayedUpdate() - if err := e.wgConfig.WgInterface.RemovePeer(e.wgConfig.RemoteKey); err != nil { - return err - } - return e.updateWireGuardPeer(e.lastEndpoint, presharedKey) -} - func (e *EndpointUpdater) RemoveWgPeer() error { e.mu.Lock() defer e.mu.Unlock() diff --git a/client/internal/pqkem/callbacks.go b/client/internal/pqkem/callbacks.go index 45344f413..3302e2e66 100644 --- a/client/internal/pqkem/callbacks.go +++ b/client/internal/pqkem/callbacks.go @@ -9,13 +9,7 @@ type CallbackHandler interface { // and must be programmed into the consumer's secure channel. It is invoked at // the commit point of each side: the initiator on receiving the answer, the // responder on receiving the confirm. - // - // bootstrap is true when the PSK came from a signalling exchange (initial setup or - // a wake re-negotiation) rather than a data-path rotation. On bootstrap the consumer - // must ensure the live channel actually adopts the PSK now (e.g. force a WireGuard - // re-handshake) — the channel may already be up on a pre-PQ key. A rotation - // (bootstrap=false) is adopted at the channel's next natural rekey. - OnNewPSKReady(remoteID RemoteID, psk PSK, bootstrap bool) error + OnNewPSKReady(remoteID RemoteID, psk PSK) error // OnRekeyFailed fires when an exchange fails to converge within the allotted // time. The host should tear the peer connection down so it re-establishes, and diff --git a/client/internal/pqkem/convergence.go b/client/internal/pqkem/convergence.go index 6e6ac7170..0d6085f73 100644 --- a/client/internal/pqkem/convergence.go +++ b/client/internal/pqkem/convergence.go @@ -67,7 +67,7 @@ func (m *Manager) startExchange(remoteID RemoteID, viaSignal bool, ackID Exchang // (that offer riding the data path under the freshly adopted key proves it worked), // then derives the PSK for the new offer, commits it optimistically, and returns the // framed answer. A duplicate offer returns the cached answer without re-deriving. -func (m *Manager) processOffer(remoteID RemoteID, o *OfferMsg, viaSignal bool) ([]byte, error) { +func (m *Manager) processOffer(remoteID RemoteID, o *OfferMsg) ([]byte, error) { m.trace("pqkem: offer received", "peer", remoteID, "exchange", idHex(o.ExchangeID), "acks", idHex(o.AckID)) if o.AckID != (ExchangeID{}) { @@ -114,7 +114,7 @@ func (m *Manager) processOffer(remoteID RemoteID, o *OfferMsg, viaSignal bool) ( m.trace("pqkem: new PSK derived", "peer", remoteID, "exchange", idHex(o.ExchangeID), "role", "responder", "psk_fp", pskFingerprint(psk)) // Commit optimistically so our data path can rekey to the new PSK. - if err := m.cbHandler.OnNewPSKReady(remoteID, psk, viaSignal); err != nil { + if err := m.cbHandler.OnNewPSKReady(remoteID, psk); err != nil { return nil, err } m.trace("pqkem: answer sent", "peer", remoteID, "exchange", idHex(o.ExchangeID)) @@ -125,7 +125,7 @@ func (m *Manager) processOffer(remoteID RemoteID, o *OfferMsg, viaSignal bool) ( // stateAwaitingRekey; the next offer (chained from OnDataPathRekeyed) will acknowledge // this exchange. Only valid in stateAwaitingAnswer; advancing the state under the // lock makes a concurrent/duplicate answer bail. -func (m *Manager) processAnswer(remoteID RemoteID, a *AnswerMsg, viaSignal bool) error { +func (m *Manager) processAnswer(remoteID RemoteID, a *AnswerMsg) error { m.mu.Lock() ex := m.exchanges[remoteID] if ex == nil || ex.id != a.ExchangeID || ex.state != stateAwaitingAnswer { @@ -159,7 +159,7 @@ func (m *Manager) processAnswer(remoteID RemoteID, a *AnswerMsg, viaSignal bool) m.trace("pqkem: new PSK derived", "peer", remoteID, "exchange", idHex(a.ExchangeID), "role", "initiator", "psk_fp", pskFingerprint(psk)) - return m.cbHandler.OnNewPSKReady(remoteID, psk, viaSignal) + return m.cbHandler.OnNewPSKReady(remoteID, psk) } // ackConverged (responder) records convergence of the exchange named by ackID: a diff --git a/client/internal/pqkem/manager.go b/client/internal/pqkem/manager.go index f87bbfad1..fff266f38 100644 --- a/client/internal/pqkem/manager.go +++ b/client/internal/pqkem/manager.go @@ -305,7 +305,7 @@ func (m *Manager) SignalOnOffer(remoteID RemoteID, offer []byte) ([]byte, error) if typ != MsgOffer { return nil, fmt.Errorf("expected offer from %s, got type %d", remoteID, typ) } - return m.processOffer(remoteID, msg.(*OfferMsg), true) + return m.processOffer(remoteID, msg.(*OfferMsg)) } // SignalOnAnswer processes a KEM answer the host extracted from an incoming answer. @@ -318,7 +318,7 @@ func (m *Manager) SignalOnAnswer(remoteID RemoteID, answer []byte) error { if typ != MsgAnswer { return fmt.Errorf("expected answer from %s, got type %d", remoteID, typ) } - return m.processAnswer(remoteID, msg.(*AnswerMsg), true) + return m.processAnswer(remoteID, msg.(*AnswerMsg)) } // ---- Data path ---- @@ -346,7 +346,7 @@ func (m *Manager) OnDataPathMessage(remoteID RemoteID, raw []byte) error { } switch typ { case MsgOffer: - answer, err := m.processOffer(remoteID, msg.(*OfferMsg), false) + answer, err := m.processOffer(remoteID, msg.(*OfferMsg)) if err != nil { return err } @@ -355,7 +355,7 @@ func (m *Manager) OnDataPathMessage(remoteID RemoteID, raw []byte) error { } return m.pushDataPath(remoteID, answer) case MsgAnswer: - return m.processAnswer(remoteID, msg.(*AnswerMsg), false) + return m.processAnswer(remoteID, msg.(*AnswerMsg)) default: return fmt.Errorf("unhandled data-path message type %d from %s", typ, remoteID) } diff --git a/client/internal/pqkem/manager_test.go b/client/internal/pqkem/manager_test.go index 26c32406d..eab264478 100644 --- a/client/internal/pqkem/manager_test.go +++ b/client/internal/pqkem/manager_test.go @@ -65,7 +65,7 @@ type fakeWG struct { func newFakeWG() *fakeWG { return &fakeWG{psks: map[RemoteID]PSK{}} } -func (f *fakeWG) OnNewPSKReady(remoteID RemoteID, psk PSK, bootstrap bool) error { +func (f *fakeWG) OnNewPSKReady(remoteID RemoteID, psk PSK) error { f.mu.Lock() defer f.mu.Unlock() f.psks[remoteID] = psk diff --git a/client/internal/pqkem_adapter.go b/client/internal/pqkem_adapter.go index 5d57a1979..82d2981de 100644 --- a/client/internal/pqkem_adapter.go +++ b/client/internal/pqkem_adapter.go @@ -23,30 +23,16 @@ type pqCallbackHandler struct { // reoffer re-bootstraps the KEM over Signal for a peer (a fresh signalling offer) // to recover from a persistent data-path rekey failure. Nil disables recovery. reoffer func(remoteKey string) - // rehandshake forces the peer's WireGuard session to adopt a freshly bootstrapped - // PSK (the live session may be up on a pre-PQ key). Nil disables it. - rehandshake func(remoteKey string) } // OnNewPSKReady programs the freshly derived PSK for the peer (updateOnly: a no-op // if the peer is not present, mirroring Rosenpass). -func (h pqCallbackHandler) OnNewPSKReady(remoteID pqkem.RemoteID, psk pqkem.PSK, bootstrap bool) error { +func (h pqCallbackHandler) OnNewPSKReady(remoteID pqkem.RemoteID, psk pqkem.PSK) error { // updateOnly: applies to an already-configured peer (rotation). At bootstrap the // peer is not configured yet, so this is a no-op there and the PSK is instead // pulled at peer-config time (pqHandshaker.PSK / conn.presharedKey). log.Tracef("pqkem: programming PSK for peer %s", remoteID) - if err := h.wg.SetPresharedKey(string(remoteID), wgtypes.Key(psk), true); err != nil { - return err - } - // A bootstrap PSK (initial setup or wake re-negotiation) may arrive after the - // WireGuard session already came up on a pre-PQ key (a race between the KEM - // exchange and the endpoint config). SetPresharedKey only updates config, not the - // live session, so force a re-handshake to actually adopt the PSK now. Rotations - // are left to WireGuard's next natural rekey. - if bootstrap && h.rehandshake != nil { - h.rehandshake(string(remoteID)) - } - return nil + return h.wg.SetPresharedKey(string(remoteID), wgtypes.Key(psk), true) } // OnRekeyFailed reports a failed PQ (re)key convergence and re-bootstraps the KEM over diff --git a/client/internal/pqkem_adapter_test.go b/client/internal/pqkem_adapter_test.go index 7f68cc1f9..dec3b5943 100644 --- a/client/internal/pqkem_adapter_test.go +++ b/client/internal/pqkem_adapter_test.go @@ -10,8 +10,8 @@ import ( type pqNoopHandler struct{} -func (pqNoopHandler) OnNewPSKReady(pqkem.RemoteID, pqkem.PSK, bool) error { return nil } -func (pqNoopHandler) OnRekeyFailed(pqkem.RemoteID) error { return nil } +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