diff --git a/quic/interop/src/main/kotlin/com/vitorpamplona/quic/interop/runner/InteropClient.kt b/quic/interop/src/main/kotlin/com/vitorpamplona/quic/interop/runner/InteropClient.kt index a8a1ac3a7..d942d5c4f 100644 --- a/quic/interop/src/main/kotlin/com/vitorpamplona/quic/interop/runner/InteropClient.kt +++ b/quic/interop/src/main/kotlin/com/vitorpamplona/quic/interop/runner/InteropClient.kt @@ -522,11 +522,25 @@ private fun runTransferTest( // (1-RTT keys derived) which is one ack ahead of HANDSHAKE_DONE // arriving. if (initiateKeyUpdate) { - withTimeoutOrNull(2_000L) { - while (conn.status != QuicConnection.Status.CONNECTED) delay(10) + // RFC 9001 §6.1 + §4.1.2: client MUST wait for + // HANDSHAKE_DONE before rolling KEY_PHASE. quinn / + // quic-go / picoquic close us with PROTOCOL_VIOLATION + // ("illegal packet: key update error") if we update + // earlier. status == CONNECTED is too lenient — it + // flips when TLS Finished is derived (well before + // HANDSHAKE_DONE arrives), so we wait on the + // handshake-confirmed signal instead. + val confirmed = + withTimeoutOrNull(2_000L) { + conn.awaitHandshakeConfirmed() + true + } ?: false + if (!confirmed) { + System.err.println("[boot] keyupdate: HANDSHAKE_DONE not received within 2s — skipping rotation") + } else { + conn.initiateKeyUpdate() + System.err.println("[boot] keyupdate: client initiated rotation to phase 1") } - conn.initiateKeyUpdate() - System.err.println("[boot] keyupdate: client initiated rotation to phase 1") } val outcome = diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt index af9b080de..c10a662e6 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -290,6 +290,31 @@ class QuicConnection( var handshakeComplete: Boolean = false private set + /** + * RFC 9001 §4.1.2 — at the CLIENT, the QUIC handshake is considered + * confirmed only when a `HANDSHAKE_DONE` frame is received from the + * server. NOT the same as [handshakeComplete] (which is the TLS-side + * "Finished derived" state, set inside [onHandshakeComplete]). + * + * Once confirmed, the client MAY: + * - initiate a 1-RTT key update (RFC 9001 §6.1), + * - initiate connection migration (RFC 9000 §9.1). + * + * Quinn (and other strict servers like quic-go / picoquic) close the + * connection with PROTOCOL_VIOLATION ("illegal packet: key update + * error") when a client rolls KEY_PHASE before HANDSHAKE_DONE has + * been delivered. The interop runner's `keyupdate` testcase against + * quinn flushed this — see + * `quic/plans/2026-05-08-keyupdate-vs-quinn.md`. + * + * Awaitable via [awaitHandshakeConfirmed]. + */ + @Volatile + var handshakeConfirmed: Boolean = false + private set + + private val handshakeConfirmedSignal = CompletableDeferred() + /** * Lock-split refactor (2026-05-08): @Volatile because the writer/parser * read this without acquiring [lifecycleLock] (the field is written @@ -943,14 +968,53 @@ class QuicConnection( /** * Suspend until the handshake completes or fails. Throws if the connection * was closed before reaching CONNECTED. + * + * "Handshake completes" here is the TLS-side notion (Finished + * derived locally). For the QUIC-spec definition of "handshake + * confirmed" (HANDSHAKE_DONE received from the server, RFC 9001 + * §4.1.2), use [awaitHandshakeConfirmed] — required before + * initiating a 1-RTT key update or connection migration. */ suspend fun awaitHandshake() { handshakeDoneSignal.await() } + /** + * Suspend until the QUIC handshake is **confirmed** (RFC 9001 + * §4.1.2: HANDSHAKE_DONE has been received), or until the + * connection terminates. Stricter than [awaitHandshake]: a peer + * that finishes TLS but never sends HANDSHAKE_DONE will leave + * `handshakeComplete = true` AND this still suspended. Required + * gate before: + * - [initiateKeyUpdate] (RFC 9001 §6.1) + * - [triggerPathMigration] (RFC 9000 §9.1) + * + * Cooperatively releases on connection close so callers don't + * hang forever when the peer drops mid-handshake. + */ + suspend fun awaitHandshakeConfirmed() { + handshakeConfirmedSignal.await() + } + + /** + * Called by the parser when a `HANDSHAKE_DONE` frame is processed + * (RFC 9000 §19.20). Idempotent — duplicate frames are silently + * tolerated per §19.20. + */ + internal fun markHandshakeConfirmed() { + if (!handshakeConfirmed) { + handshakeConfirmed = true + handshakeConfirmedSignal.complete(Unit) + } + } + /** Mark the handshake as failed (called when read loop dies, peer closes, or local close runs). */ internal fun signalHandshakeFailed(cause: Throwable) { if (!handshakeDoneSignal.isCompleted) handshakeDoneSignal.completeExceptionally(cause) + // Don't leave [awaitHandshakeConfirmed] suspended forever when + // the connection drops before HANDSHAKE_DONE arrives — same + // semantics as [handshakeDoneSignal] on a failed handshake. + if (!handshakeConfirmedSignal.isCompleted) handshakeConfirmedSignal.completeExceptionally(cause) } val tls: TlsClient = @@ -2099,11 +2163,16 @@ class QuicConnection( * and server". */ fun initiateKeyUpdate(): Boolean { - // RFC 9001 §6.5: handshake MUST be confirmed before initiating - // a key update. We use [handshakeComplete] as the proxy — - // application keys are installed and the peer's HANDSHAKE_DONE - // has been processed. - if (!handshakeComplete) return false + // RFC 9001 §6.1 + §4.1.2: a client MUST NOT initiate a key + // update before the handshake is confirmed — i.e. before + // HANDSHAKE_DONE has been received from the server. Strict + // servers (quinn, quic-go, picoquic) close us with + // PROTOCOL_VIOLATION ("illegal packet: key update error") + // otherwise. [handshakeComplete] is the TLS-side flag (set + // when our TLS Finished is derived) which fires earlier + // than confirmation; [handshakeConfirmed] only flips once + // HANDSHAKE_DONE is processed by the parser. + if (!handshakeConfirmed) return false // RFC 9001 §6.4: MUST NOT initiate a subsequent rotation until // the previous one is confirmed. The parser clears this flag // when it observes an inbound packet that AEAD-decrypts under @@ -2490,7 +2559,13 @@ class QuicConnection( nowMillis: Long, currentPtoMillis: Long, ): PathMigrationResult { - if (!handshakeComplete || status != Status.CONNECTED) { + // RFC 9000 §9.1: an endpoint MUST NOT initiate connection + // migration before the handshake is confirmed (RFC 9001 + // §4.1.2 — HANDSHAKE_DONE received). [handshakeComplete] is + // the TLS-side flag (Finished derived) which fires earlier + // than confirmation; gating on [handshakeConfirmed] matches + // the spec. + if (!handshakeConfirmed || status != Status.CONNECTED) { return PathMigrationResult.NotConnected } val result = pathValidator.tryStartValidation(nowMillis, currentPtoMillis) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt index 9bbbfbbd8..1b960159d 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -1132,6 +1132,13 @@ private fun dispatchFrames( // residual handshake CRYPTO bookkeeping that's no longer // needed for the lifetime of the connection. conn.handshake.discardKeys() + // RFC 9001 §4.1.2 — only NOW is the QUIC handshake + // considered confirmed at the client, gating + // 1-RTT key update + connection migration. The TLS-side + // `onHandshakeComplete` callback fires earlier (when + // Finished is derived), but the spec requires waiting + // for HANDSHAKE_DONE before either operation. + conn.markHandshakeConfirmed() } is PingFrame -> { diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ConnectedClientFixture.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ConnectedClientFixture.kt index c009322bf..50768643e 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ConnectedClientFixture.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ConnectedClientFixture.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.quic.connection +import com.vitorpamplona.quic.frame.HandshakeDoneFrame import com.vitorpamplona.quic.tls.InProcessTlsServer import com.vitorpamplona.quic.tls.PermissiveCertificateValidator import kotlinx.coroutines.runBlocking @@ -49,6 +50,17 @@ fun newConnectedClient( maxData: Long = 1L * 1024 * 1024, maxStreamData: Long = 64L * 1024, handshakeRounds: Int = 16, + /** + * Deliver a `HANDSHAKE_DONE` frame at the end of the handshake + * so the returned client matches the production "handshake + * confirmed" state (RFC 9001 §4.1.2). Tests that need to + * exercise the pre-confirmation window (gate around + * [QuicConnection.initiateKeyUpdate] and + * [QuicConnection.triggerPathMigration]) pass `false` to skip + * the delivery and assert against + * [QuicConnection.handshakeConfirmed] = false. + */ + deliverHandshakeDone: Boolean = true, ): Pair = runBlocking { val client = @@ -90,5 +102,18 @@ fun newConnectedClient( client.start() pipe.drive(maxRounds = handshakeRounds) assertEquals(QuicConnection.Status.CONNECTED, client.status) + if (deliverHandshakeDone) { + // Pre-2026-05-08 the fixture stopped at TLS-Finished and + // most tests didn't notice — the path-migration and + // key-update gates both fired off `handshakeComplete`, + // which flips at TLS-Finished. After the + // `handshakeConfirmed` split (driven by the keyupdate- + // vs-quinn bug, see `quic/plans/2026-05-08-keyupdate-vs-quinn.md`), + // both gates require HANDSHAKE_DONE — so the default + // fixture now matches the production "handshake + // confirmed" state. + val packet = pipe.buildServerApplicationDatagram(listOf(HandshakeDoneFrame()))!! + feedDatagram(client, packet, nowMillis = 0L) + } client to pipe } diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyUpdateClientInitiatedTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyUpdateClientInitiatedTest.kt new file mode 100644 index 000000000..f9981311a --- /dev/null +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/KeyUpdateClientInitiatedTest.kt @@ -0,0 +1,159 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quic.connection + +import com.vitorpamplona.quic.frame.HandshakeDoneFrame +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +/** + * Pins the client-initiated 1-RTT key update gate (RFC 9001 §6.1 + + * §4.1.2): the client MUST NOT roll `KEY_PHASE` before + * `HANDSHAKE_DONE` has been received from the server. Strict servers + * — quinn, quic-go, picoquic — close the connection with + * PROTOCOL_VIOLATION ("illegal packet: key update error") otherwise. + * + * Pre-2026-05-08 the gate inside [QuicConnection.initiateKeyUpdate] + * was just "1-RTT keys installed", which fires when TLS Finished is + * derived — well before HANDSHAKE_DONE arrives. Confirmed against + * quinn's reference server: at ~33% of runs the interop runner's + * `keyupdate` testcase initiated the update inside the gap and + * tripped the violation. See `quic/plans/2026-05-08-keyupdate-vs-quinn.md`. + * + * The fix introduces a separate [QuicConnection.handshakeConfirmed] + * field flipped only by the parser on `HANDSHAKE_DONE` receipt; + * [QuicConnection.initiateKeyUpdate] now requires it. + */ +class KeyUpdateClientInitiatedTest { + @Test + fun initiateKeyUpdateBeforeHandshakeDoneIsRejectedAndDoesNotRotateKeys() = + runBlocking { + val (client, _) = newConnectedClient(deliverHandshakeDone = false) + + // newConnectedClient drives the handshake to TLS-Finished + // (so 1-RTT keys are installed and `handshakeComplete` is + // true), but DOES NOT deliver HANDSHAKE_DONE — exactly + // the window where strict servers reject a client-side + // key update. + assertTrue(client.handshakeComplete, "TLS-side handshake completed") + assertFalse(client.handshakeConfirmed, "HANDSHAKE_DONE not yet received") + + val originalReceiveProtection = client.application.receiveProtection + val originalSendProtection = client.application.sendProtection + + assertFalse( + client.initiateKeyUpdate(), + "initiateKeyUpdate must reject before HANDSHAKE_DONE — RFC 9001 §6.1 + §4.1.2", + ) + + // Phase bits stay at 0; protections stay at the post-handshake + // values; no demotion to previousReceiveProtection. + assertEquals(false, client.currentSendKeyPhase) + assertEquals(false, client.currentReceiveKeyPhase) + assertEquals(originalReceiveProtection, client.application.receiveProtection) + assertEquals(originalSendProtection, client.application.sendProtection) + assertEquals(null, client.previousReceiveProtection) + } + + @Test + fun initiateKeyUpdateAfterHandshakeDoneRotatesBothDirections() = + runBlocking { + val (client, pipe) = newConnectedClient(deliverHandshakeDone = false) + + // Deliver HANDSHAKE_DONE — the parser flips + // [QuicConnection.handshakeConfirmed] and unblocks the + // gate inside `initiateKeyUpdate`. + val handshakeDonePacket = pipe.buildServerApplicationDatagram(listOf(HandshakeDoneFrame()))!! + feedDatagram(client, handshakeDonePacket, nowMillis = 0L) + assertTrue(client.handshakeConfirmed, "HANDSHAKE_DONE must flip handshakeConfirmed") + + val originalReceiveProtection = client.application.receiveProtection + val originalSendProtection = client.application.sendProtection + assertNotNull(originalReceiveProtection) + assertNotNull(originalSendProtection) + + assertTrue(client.initiateKeyUpdate(), "must accept after HANDSHAKE_DONE") + + // Both directions rotated; phase bits flipped; old receive + // keys retained for the §6.1 reorder window. + assertEquals(true, client.currentSendKeyPhase) + assertEquals(true, client.currentReceiveKeyPhase) + assertEquals( + originalReceiveProtection, + client.previousReceiveProtection, + "pre-rotation receive keys must move into previousReceiveProtection", + ) + assertTrue( + originalReceiveProtection != client.application.receiveProtection, + "fresh receive protection installed", + ) + assertTrue( + originalSendProtection != client.application.sendProtection, + "fresh send protection installed", + ) + } + + @Test + fun awaitHandshakeConfirmedSuspendsUntilHandshakeDone() = + runBlocking { + val (client, pipe) = newConnectedClient(deliverHandshakeDone = false) + assertFalse(client.handshakeConfirmed, "not confirmed before HANDSHAKE_DONE") + // Deliver HANDSHAKE_DONE and then await — the awaiter must + // observe the post-receipt state without suspending forever. + val packet = pipe.buildServerApplicationDatagram(listOf(HandshakeDoneFrame()))!! + feedDatagram(client, packet, nowMillis = 0L) + client.awaitHandshakeConfirmed() + assertTrue(client.handshakeConfirmed) + } + + @Test + fun triggerPathMigrationBeforeHandshakeDoneReturnsNotConnected() = + runBlocking { + // Symmetric gate: RFC 9000 §9.1 also requires handshake + // confirmation before initiating connection migration. + // Pre-fix this gated on `handshakeComplete && status == + // CONNECTED` — both flip on TLS Finished, so migration + // could fire before HANDSHAKE_DONE. + val (client, pipe) = newConnectedClient(deliverHandshakeDone = false) + // Pool a CID so NoSpareCid wouldn't shadow the gate. + feedDatagram( + client, + pipe.buildServerApplicationDatagram( + listOf( + com.vitorpamplona.quic.frame.NewConnectionIdFrame( + sequenceNumber = 1L, + retirePriorTo = 0L, + connectionId = ByteArray(8) { 0x42 }, + statelessResetToken = ByteArray(16) { 0x77 }, + ), + ), + )!!, + nowMillis = 0L, + ) + assertFalse(client.handshakeConfirmed) + val result = client.triggerPathMigration(nowMillis = 0L, currentPtoMillis = 100L) + assertEquals(PathMigrationResult.NotConnected, result) + } +}