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.
When the controller receives the responder's (KEM-less) offer it replies with
its own KEM offer instead of answering, so the only transaction that brings the
tunnel up is the one that also carries the PSK. Guard that reply with
ShouldSendBootstrapOffer so it fires only when no exchange is in flight: without
it, every responder offer triggered another offer (an offer-per-offer runaway).
The whole behaviour is isolated to the KEM path (config.PQ != nil); non-PQ
connections answer as before.
The controller sends its KEM offer both on its own guard event and in reply
to the responder's offer. SignalOffer was idempotent only while awaiting the
answer; once the answer arrived (awaitingRekey) a repeat call started a fresh
exchange with a different PSK, desyncing the two peers (one on the old PSK,
one on the new) so WireGuard derived misaligned transport keys and dropped all
data. Treat awaitingRekey as in-flight too and return the same offer.
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