fix(quic): gate client-initiated key update + path migration on HANDSHAKE_DONE

Pre-fix, [QuicConnection.initiateKeyUpdate] only checked that 1-RTT keys were installed — which fires when the TLS handshake derives Finished, well before HANDSHAKE_DONE arrives. RFC 9001 §6.1 + §4.1.2 require the CLIENT to consider the handshake confirmed (HANDSHAKE_DONE received) before rolling KEY_PHASE; quinn / quic-go / picoquic correctly close the connection with PROTOCOL_VIOLATION ("illegal packet: key update error") when an early update arrives. The interop runner's keyupdate testcase against quinn flushed this: the client polled `status == CONNECTED` (which flips on TLS Finished via `onHandshakeComplete`) and called `initiateKeyUpdate()` before HANDSHAKE_DONE landed. ~33% repro rate; the rest of the runs HANDSHAKE_DONE happened to arrive in the gap between the status check and the update. Fix: - Add `QuicConnection.handshakeConfirmed: Boolean` flipped only by the parser when a HANDSHAKE_DONE frame is processed. - Add `awaitHandshakeConfirmed()` for callers that need the spec-confirmed state. - Gate `initiateKeyUpdate()` on `handshakeConfirmed` (returns false otherwise — same shape as the existing app-keys-not-yet branch). - Tighten `triggerPathMigrationLocked` to also gate on `handshakeConfirmed` (RFC 9000 §9.1: same confirmation requirement). Pre-fix it gated on `handshakeComplete`, which flips at TLS-Finished too. - InteropClient's keyupdate flow now waits on `awaitHandshakeConfirmed` (with a 2s upper bound) rather than polling `status == CONNECTED`. The test fixture in ConnectedClientFixture.newConnectedClient now delivers HANDSHAKE_DONE by default — most tests want the production "fully ready" state. New `deliverHandshakeDone = false` parameter for tests that need to exercise the pre-confirmation window (KeyUpdateClientInitiatedTest). Tests: - KeyUpdateClientInitiatedTest: initiateKeyUpdateBeforeHandshakeDoneIsRejectedAndDoesNotRotateKeys initiateKeyUpdateAfterHandshakeDoneRotatesBothDirections awaitHandshakeConfirmedSuspendsUntilHandshakeDone triggerPathMigrationBeforeHandshakeDoneReturnsNotConnected Verified against the live interop runner — 5/5 against quinn, 3/3 against quic-go, 3/3 against picoquic (pre-fix: ~33% pass rate against quinn). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-08 20:22:03 -04:00
parent 707fcee5b6
commit e8991fab69
5 changed files with 290 additions and 10 deletions
@@ -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 =
@@ -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<Unit>()
+
    /**
     * 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)
@@ -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 -> {
@@ -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<QuicConnection, InMemoryQuicPipe> =
    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
    }
@@ -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)
+        }
+}