The responder wrote pendingPSK but nothing ever read it: the derived key is
consumed via m.psks (set at the same time) and the cached answer via lastSent.
Remove the field and its write to avoid a misleading store.
Found in cubic review on #7098 (client/internal/pqkem/manager.go:90).
OnDataPathRekeyed builds a chained offer, releases the lock, then sends it. A
signal re-bootstrap can supersede that exchange in the gap, after which the stale
offer still went out and the responder would reserve and commit an abandoned
exchange, splitting the keys.
Two guards, no revalidation of message contents:
- Sender side: before putting the chain offer on the wire, re-check it is still
the current exchange; drop it if a re-bootstrap already replaced it.
- Responder side: a legitimate rotation offer acknowledges the exchange we are
awaiting-ack on (ackConverged clears it on a match). Drop a chain offer whose
ack did not match our current exchange — it is a rotation a newer signal round
has superseded. A bootstrap (zero AckID) is authoritative and always wins.
A supersede landing in the infinitesimal window after the sender check still
leaks one offer, but the responder guard rejects it, so the new round stands.
Neither guard retries, so there is no loop.
Found in cubic review on #7098 (client/internal/pqkem/manager.go:402).
OnNewPSKReady could be applied out of order: two exchanges for a peer can derive
concurrently (one over signal, one over the data path), and the callbacks run
outside the manager lock, so an older exchange's apply could land after a newer
one and restore a stale WireGuard PSK, splitting the tunnel.
Give each exchange a per-peer monotonic generation, assigned under the lock at
creation so a later exchange always carries a higher one, and pass it to
OnNewPSKReady. The host adapter records the newest generation applied per peer
and drops any callback that is not newer, with the check-and-record atomic so the
slow SetPresharedKey call stays off that lock.
Found in cubic review on #7098 (client/internal/pqkem/callbacks.go:12).
Both sides recorded the exchange as converged and then called OnNewPSKReady,
returning its error but leaving the "converged" state in place. A host failure to
program the PSK therefore left the peers split between the old data-path key and
the new stored value, with no failure raised and no re-bootstrap.
Treat applying the PSK as part of the commit. On the initiator, a failed
OnNewPSKReady now drops the exchange and raises the failure so recovery
re-bootstraps. On the responder, it drops the exchange and withholds the answer,
so the initiator times out and re-bootstraps rather than converging on a key the
responder could not apply.
Found in cubic review on #7098 (client/internal/pqkem/convergence.go:228).
processAnswer advanced the exchange to stateAwaitingRekey before init.Finish
derived the PSK. A rekey firing in that window chained the next offer off an
exchange whose key was not yet committed, letting the responder commit PSK N+1
before this side committed PSK N; a lost next answer then left the two peers on
different PSKs.
Move to an intermediate stateFinishing while deriving (OnDataPathRekeyed chains
only on stateAwaitingRekey, so it will not chain mid-derive), and advance to
stateAwaitingRekey only after Finish succeeds and the exchange is still current.
Found in cubic review on #7098 (client/internal/pqkem/convergence.go:194).
Bot review (CodeRabbit, cubic) on PR #7098 surfaced several real defects:
- Strict() parsed the raw env value instead of the normalized one, so a
mixed-case NB_PQ_MLKEM_STRICT such as "tRuE" silently disabled fail-closed
mode. Parse the lower-cased value, matching Enabled().
- OnDataPathMessage labeled a data-path answer as "signal" in logs, mislabeling
every rotation answer. Use the data-path label.
- A failed sendAnswer aborted the whole connection setup, skipping the relay/ICE
listeners; a transient signalling failure now still brings the local transport
up (the peer retries the answer).
- processOffer left the reserved exchange slot in place when Respond failed, so
every retransmission of that offer was dropped forever. Clear the reservation
on a pre-commit error so a retry can derive again.
- Dropped a no-op time.Since that implied a convergence-latency metric that was
never recorded, and the now-unused startedAt field.
- The strict-mode status line asserted active blocking even for a peer that is
simply offline; reword it to state that no PSK is established yet.
- conn_test used t.Fatalf for conditions under test; use assert.
The per-exchange lifecycle logs were all at LevelTrace (below the default field
level) and did not distinguish a signalling re-bootstrap from a data-path rekey,
which made diagnosing exchange activity in the field guesswork (e.g. telling an
ICE-retry-driven re-bootstrap storm apart from normal KEM rotation).
Promote the two pivotal events — an offer going out and a PSK converging — to
LevelDebug and tag every lifecycle log with four dimensions:
- msg: offer vs answer (implicit in the message)
- role: initiator vs responder for this peer
- via: signal vs data-path (threaded into processOffer/processAnswer)
- kind: bootstrap (a new connection / reconnect) vs rotation (a rekey)
kind is derived from the exchange's AckID (zero = bootstrap) on the responder
side and from viaSignal on the initiator side. No behavioural change.
Two convergence bugs surfaced by the security review:
- Role guard (finding B): processOffer accepted an offer even when we are the
KEM initiator for the peer, and processAnswer accepted an answer when we are
the responder. The KEM is unidirectional (initiator offers, responder
answers), so a role-violating message is anomalous — a desync, a duplicate,
or an injected/spoofed data-path packet. Processing it derived and committed
a fresh PSK, overwriting a live one and silently dropping any in-flight
exchange (whose retry loop then exited without raising a failure or
re-bootstrapping). Reject offers when we are the initiator and answers when
we are not; this drops only anomalous traffic and leaves the normal flow
untouched.
- Re-bootstrap on signal re-negotiation (finding A): SignalOffer was idempotent
in stateAwaitingRekey too, replaying the frozen bootstrap offer. After the
responder restarted and lost its state it derived a different PSK from fresh
material, which our awaitingRekey side then rejected — a permanent desync with
no recovery (in strict mode the peer stays blocked). Make the idempotency
apply only while a bootstrap is still in flight (awaitingAnswer); once a PSK
is derived, a fresh signal offer starts a new exchange so both sides converge.
The controller-double-offer case the idempotency guarded is already covered by
ShouldSendBootstrapOffer. Reusing the cached offer also reused the same
ephemeral keys across exchanges, reducing forward secrecy.
Both paths have a failing-without-the-fix regression test.
To ensure two peers agree on a key, we need asymmetry. one peer is
the controller ("initiator") the other is the "responder".
Otherwise imagine two offers in parallel driving two answers at the same time
A B
| <----B-OFFER----- |
| -----A-OFFER----> |
| |
| |
---------------------------------
|****** ICE + WG Handshake ****** |
---------------------------------
| |
| <----B-ANSWER---- |
| -----A-ANSWER---> |
PSK is derived on receive of offer, so A and B derive different PSKs.
When WG handshake takes place it picks misaligned PSKs.
So we impair the two nodes and only the offer of one of the two (the controller/initiator)
carries the KEM material.
This means that if the responder OFFER/ANSWER comes first, when the controller/initiator's one
completes (and the genuine PSK is shared between A and B, we need to force a new WG handshake with
the proper keys.
- Add a trace slog level (NB_PQ_MLKEM_LOG_LEVEL=trace) and move the verbose
per-exchange lifecycle logs (offer/answer/PSK/ack/rotation) to it, so debug
stays quiet and troubleshooting is opt-in.
- Stop logging the raw preshared key; drop the temporary pqkem-dbg OnRemoteOffer/
OnRemoteAnswer probes.
- Demote the per-handshake conn log to trace.
We clock the next Offer initiation to the OnDataPathRekeyed, so we have 2 minutes
ahead of us to do our attempts and stuff before to give up.
On failure, we will know because we will not receive a new answer.. but more importantly
the wg handshake will fail :D
Define OnDataPathRekeyed event to transition from control plane path to data plane path over the WG tunnel.
Keep confirm ALWAYS on NEW established WG tunnel (posthandshake with rekeying). We keep an active method
irrelevant of the WG handshake (we might decide that the indirect wg handshake is sufficient in the future).
Optimistic commit on responder(when sending answer), while on initiator we set it on getting the answer
- Have just one manager => one lock
- Session state is needed in driver to => we have it available now.
- Isomorphically align to rosenpass components and functionality
File Role rosenpass equivalent
kem.go primitive pure X25519MLKEM768 crypto.go/handshake
message.go Offer/Answer/Confirm + Encode/Decode messages.go
manager.go Manager stateful, single lock server logic
callbacks.go WGCallbackHandler (seam output) Handler
Transport (interfaccia) seam trasporto pluggable Conn
riccardom