fix(quic): preserve Initial PN namespace across Retry (RFC 9001 §5.7)
aioquic's retry test result, surfaced via qlog: Check of downloaded files succeeded. Client reset the packet number. Check failed for PN 0 Our applyRetry called LevelState.resetForVersionNegotiation, which creates a fresh PacketNumberSpaceState() — resetting PN to 0. The qlog confirmed: PN=0 sent at t=388 (pre-Retry ClientHello), then PN=0 again at t=1468 (post-Retry retried ClientHello). Same PN reused across the boundary. RFC 9001 §5.7 + RFC 9000 §17.2.5: the Initial PN namespace CONTINUES across the Retry boundary. The new Initial keys are derived from the new DCID, but PN doesn't reset. Reusing a PN under different keys makes the runner's pcap-decryption check fail (it's also a security concern in the general case, hence the strict spec rule). Fix: new LevelState.resetForRetry that's identical to resetForVersionNegotiation EXCEPT it preserves pnSpace. applyRetry calls resetForRetry. Two regression tests updated to assert the post-Retry Initial uses PN=1 (continues from PN=0 of the pre-Retry attempt) rather than PN=0 (the buggy reset behavior). For Version Negotiation the original semantics still apply (RFC 9000 §6.2: client treats VN as if the original Initial was never sent; PN reset to 0 is correct). This should bring the retry testcase from ✕(S) to ✓(S) against servers that exercise the Retry path. The handshake / transfer already succeeded over the Retry per the qlog (the server's check "Check of downloaded files succeeded." passed); only the PN-reuse flag was failing the test. https://claude.ai/code/session_01HcvfQq1ttPV9PkRoJb4nyT
This commit is contained in:
@@ -153,4 +153,45 @@ class LevelState {
|
||||
this.sendProtection = sendProtection
|
||||
this.receiveProtection = receiveProtection
|
||||
}
|
||||
|
||||
/**
|
||||
* Reset the Initial-level state for a successful Retry.
|
||||
*
|
||||
* Differs from [resetForVersionNegotiation] in that the packet number
|
||||
* namespace is **preserved**: per RFC 9001 §5.7 + RFC 9000 §17.2.5,
|
||||
* the Initial PN space spans the original AND the post-Retry Initials.
|
||||
* The client MUST NOT reuse a packet number across the boundary —
|
||||
* the next Initial sent after Retry uses PN = (last-used) + 1.
|
||||
*
|
||||
* The runner's retry test specifically checks for this: a client that
|
||||
* resets the PN gets "Client reset the packet number. Check failed
|
||||
* for PN 0".
|
||||
*
|
||||
* Everything else resets to mirror VN semantics:
|
||||
* - cryptoSend cleared so the caller can re-enqueue the cached
|
||||
* ClientHello on top of fresh empty buffer state.
|
||||
* - sentPackets cleared because the pre-Retry Initial's loss-recovery
|
||||
* state references PNs the server will never ACK (the server
|
||||
* discarded that packet when it sent the Retry).
|
||||
* - keys re-derived from the new DCID and reinstalled.
|
||||
* - keysDiscarded latch reset (the pre-Retry keys are gone, but the
|
||||
* level itself is still alive with the new keys).
|
||||
*/
|
||||
internal fun resetForRetry(
|
||||
sendProtection: PacketProtection,
|
||||
receiveProtection: PacketProtection,
|
||||
) {
|
||||
// pnSpace is INTENTIONALLY preserved — see kdoc above.
|
||||
ackTracker =
|
||||
com.vitorpamplona.quic.recovery
|
||||
.AckTracker()
|
||||
cryptoSend = SendBuffer()
|
||||
cryptoReceive = ReceiveBuffer()
|
||||
sentPackets.clear()
|
||||
largestAckedPn = null
|
||||
largestAckedSentTimeMs = null
|
||||
keysDiscarded = false
|
||||
this.sendProtection = sendProtection
|
||||
this.receiveProtection = receiveProtection
|
||||
}
|
||||
}
|
||||
|
||||
@@ -579,7 +579,11 @@ class QuicConnection(
|
||||
destinationConnectionId = retryPacket.scid
|
||||
val proto = InitialSecrets.derive(destinationConnectionId.bytes)
|
||||
val hp = AesEcbHeaderProtection(PlatformAesOneBlock)
|
||||
initial.resetForVersionNegotiation(
|
||||
// Use resetForRetry, NOT resetForVersionNegotiation: RFC 9001 §5.7
|
||||
// requires the Initial PN namespace to continue across the Retry
|
||||
// boundary. Resetting PN to 0 caused the runner to flag
|
||||
// "Client reset the packet number. Check failed for PN 0".
|
||||
initial.resetForRetry(
|
||||
sendProtection =
|
||||
PacketProtection(bestAes128GcmAead(proto.clientKey), proto.clientKey, proto.clientIv, hp, proto.clientHp),
|
||||
receiveProtection =
|
||||
|
||||
@@ -142,8 +142,14 @@ class RetryHandlingTest {
|
||||
assertContentEquals(retryToken, client.retryToken)
|
||||
assertTrue(client.retryConsumed)
|
||||
|
||||
// Initial PN space reset — next allocation is 0 again.
|
||||
assertEquals(0L, client.initial.pnSpace.nextPacketNumber)
|
||||
// RFC 9001 §5.7 + RFC 9000 §17.2.5: the Initial PN namespace
|
||||
// CONTINUES across the Retry boundary. The first ClientHello at
|
||||
// PN=0 already consumed PN=0 (it was sent on the wire even though
|
||||
// the server discarded it in favor of replying with Retry). The
|
||||
// post-Retry Initial uses PN=1, not PN=0. The runner's retry
|
||||
// testcase explicitly checks this — a client that resets PN to 0
|
||||
// gets "Client reset the packet number. Check failed for PN 0".
|
||||
assertEquals(1L, client.initial.pnSpace.nextPacketNumber)
|
||||
assertEquals(-1L, client.initial.pnSpace.largestReceived)
|
||||
|
||||
// Next drain produces the retried Initial: token in header, ClientHello
|
||||
@@ -179,7 +185,10 @@ class RetryHandlingTest {
|
||||
largestReceivedInSpace = -1L,
|
||||
)
|
||||
assertNotNull(parsed, "retried Initial must decrypt under keys derived from new DCID")
|
||||
assertEquals(0L, parsed.packet.packetNumber, "retried Initial PN must be 0 (RFC 9000 §17.2.5.2)")
|
||||
// RFC 9001 §5.7: Initial PN namespace continues across Retry. The
|
||||
// pre-Retry Initial in this test already consumed PN=0, so the
|
||||
// retried Initial uses PN=1.
|
||||
assertEquals(1L, parsed.packet.packetNumber, "retried Initial PN continues from pre-Retry counter (RFC 9001 §5.7)")
|
||||
// Decoded payload starts with at least one CRYPTO frame (frame type 0x06).
|
||||
val frames =
|
||||
com.vitorpamplona.quic.frame
|
||||
|
||||
Reference in New Issue
Block a user