diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt index 7b5598738..71e3781fb 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -62,12 +62,19 @@ fun drainOutbound( ): ByteArray? { val parts = mutableListOf() - // Closing — emit a CONNECTION_CLOSE at the highest available level. + // Closing — emit a CONNECTION_CLOSE at the highest available level. The + // datagram-build paths below MUST satisfy two RFC 9000 constraints we + // got wrong before: + // - §10.2.3: at Initial / Handshake levels, only CONNECTION_CLOSE + // (Transport, 0x1c) is allowed; the application-level close (0x1d) is + // forbidden because app state would leak before the handshake is + // encrypted with the application key. + // - §14.1: a client datagram containing an Initial MUST be ≥ 1200 bytes + // in UDP-payload terms, even when carrying only a CONNECTION_CLOSE. if (conn.status == QuicConnection.Status.CLOSING) { - val frame = ConnectionCloseFrame(conn.closeErrorCode, null, conn.closeReason ?: "") - val packet = buildBestLevelPacket(conn, listOf(frame)) ?: return null + val datagram = buildClosingDatagram(conn, nowMillis) conn.status = QuicConnection.Status.CLOSED - return packet + return datagram } // Drain destructive frame sources into local lists, ONCE. @@ -118,21 +125,42 @@ fun drainOutbound( var natural = 0 for (p in firstPass) natural += p.size if (natural < 1200) { - val deficit = 1200 - natural - // Rewind the Initial PN — we'll reissue with the same PN and the - // same captured frames plus padding. The SentPacket entry recorded - // by the natural-size build will be overwritten by the rebuild - // below since both use the same PN. - initialState.pnSpace.rewindOutboundForRebuild() - val paddedInitial = - buildLongHeaderFromFrames( - conn = conn, - level = EncryptionLevel.INITIAL, - frames = initialContents!!.frames, // null-safe: gated by `initialNatural != null` above - tokens = initialContents.tokens, - nowMillis = nowMillis, - padBytes = deficit, - ) + // Padding rebuild: re-issue the Initial with PADDING frames inside + // the AEAD envelope so the on-wire datagram clears the §14.1 floor. + // + // Off-by-one trap: the QUIC long-header Length field is a varint + // (RFC 9000 §16). When the natural-size payload is tiny enough + // for Length to fit in 1 byte (body ≤ 63 bytes), the rebuild's + // larger body crosses the 64-byte threshold and Length grows to + // 2 bytes — adding 1 wire byte that wasn't in `natural`. Same + // shape at 16384 bytes (2 → 4) and 2^30 (4 → 8). A naive + // `padBytes = 1200 - natural` then produces a 1199-byte + // datagram for PING-only Initials. + // + // Solution: rebuild with the initial deficit, measure, and if we + // still fall short, bump by the residual and rebuild once more. + // Each iteration adds PADDING bytes 1:1 to the wire size; the + // varint grows monotonically so this terminates in ≤ 2 rounds + // for any reachable payload size. + var padBytes = 1200 - natural + var paddedInitial: ByteArray + while (true) { + initialState.pnSpace.rewindOutboundForRebuild() + paddedInitial = + buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.INITIAL, + frames = initialContents!!.frames, // null-safe: gated by `initialNatural != null` above + tokens = initialContents.tokens, + nowMillis = nowMillis, + padBytes = padBytes, + ) + var totalAfterRebuild = paddedInitial.size + if (handshakeNatural != null) totalAfterRebuild += handshakeNatural.size + if (applicationPkt != null) totalAfterRebuild += applicationPkt.size + if (totalAfterRebuild >= 1200) break + padBytes += 1200 - totalAfterRebuild + } concat(listOfNotNull(paddedInitial, handshakeNatural, applicationPkt)) } else { concat(firstPass) @@ -166,6 +194,93 @@ private fun concat(parts: List): ByteArray { return out } +/** + * Build a CONNECTION_CLOSE-only datagram at the highest encryption level we + * have keys for. Two RFC 9000 constraints make this trickier than a normal + * packet build: + * + * - §10.2.3 — at Initial / Handshake levels, only CONNECTION_CLOSE + * (Transport, 0x1c) is allowed. An application-level close is replaced + * with `APPLICATION_ERROR (0x0c)` + frameType=0 + empty reason so we don't + * leak app state pre-handshake. + * - §14.1 — a client datagram containing an Initial MUST be ≥ 1200 bytes, + * even a close-only one. We do this by building once at natural size and, + * if short, rewinding the PN and rebuilding with a PADDING-frame deficit + * inside the AEAD envelope. The rebuild loops because the long-header + * Length varint (RFC 9000 §16) can grow by 1 byte once the body crosses + * the 64-byte threshold, so a single-shot deficit can land 1 byte short + * of 1200. + */ +private fun buildClosingDatagram( + conn: QuicConnection, + nowMillis: Long, +): ByteArray? { + val app = conn.application + if (app.sendProtection != null) { + // 1-RTT level reached: app close (0x1d) is allowed and carries the + // original error code + reason. + val frame = ConnectionCloseFrame(conn.closeErrorCode, null, conn.closeReason ?: "") + return buildBestLevelPacket(conn, listOf(frame)) + } + + // Pre-1-RTT: must use transport-encoded close. RFC 9000 §20.1 + // APPLICATION_ERROR = 0x0c — "the application or application protocol + // caused the connection to be closed during the handshake". + val transportFrame = + ConnectionCloseFrame( + errorCode = APPLICATION_ERROR, + frameType = 0L, + reason = "", + ) + + val hs = conn.handshake + if (hs.sendProtection != null) { + return buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.HANDSHAKE, + frames = listOf(transportFrame), + tokens = emptyList(), + nowMillis = nowMillis, + padBytes = 0, + ) + } + + val init = conn.initial + if (init.sendProtection != null && !init.keysDiscarded) { + val natural = + buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.INITIAL, + frames = listOf(transportFrame), + tokens = emptyList(), + nowMillis = nowMillis, + padBytes = 0, + ) + if (natural.size >= 1200) return natural + var padBytes = 1200 - natural.size + var padded: ByteArray + do { + init.pnSpace.rewindOutboundForRebuild() + padded = + buildLongHeaderFromFrames( + conn = conn, + level = EncryptionLevel.INITIAL, + frames = listOf(transportFrame), + tokens = emptyList(), + nowMillis = nowMillis, + padBytes = padBytes, + ) + if (padded.size >= 1200) break + padBytes += 1200 - padded.size + } while (true) + return padded + } + return null +} + +/** RFC 9000 §20.1 — application-or-protocol caused close during handshake. */ +private const val APPLICATION_ERROR: Long = 0x0cL + private fun buildBestLevelPacket( conn: QuicConnection, frames: List, @@ -280,10 +395,37 @@ private fun collectHandshakeLevelFrames( length = cryptoChunk.data.size.toLong(), ) } + // RFC 9002 §6.2.4: PTO probe MUST be emitted at the encryption level + // that has unacknowledged data. `buildApplicationPacket` consumes + // [pendingPing] preferentially when 1-RTT keys exist; if they don't, + // the probe lives or dies here at Handshake / Initial level. Pre-fix + // the flag was set but never honored pre-handshake — the connection + // sat silent through every PTO, never retransmitting the ClientHello + // even when the first datagram was lost. This caused the + // "1-packet-on-the-wire-then-timeout" symptom against aioquic in the + // quic-interop-runner sim where the first ClientHello was dropped + // because the server hadn't finished startup yet. + if (conn.pendingPing && conn.application.sendProtection == null && level == highestPreApplicationLevel(conn)) { + frames += PingFrame + conn.pendingPing = false + } if (frames.isEmpty()) return null return HandshakeLevelContents(frames = frames, tokens = tokens) } +/** + * The highest encryption level for which we currently hold send keys, given + * that 1-RTT keys are NOT yet installed. Used by [collectHandshakeLevelFrames] + * to decide where a PTO PING probe should ride when the application level + * isn't usable yet. + */ +private fun highestPreApplicationLevel(conn: QuicConnection): EncryptionLevel = + when { + conn.handshake.sendProtection != null -> EncryptionLevel.HANDSHAKE + conn.initial.sendProtection != null && !conn.initial.keysDiscarded -> EncryptionLevel.INITIAL + else -> EncryptionLevel.INITIAL // fallback; collectHandshakeLevelFrames already early-returns on no keys + } + /** * Build a long-header packet from already-collected frames, with optional * trailing PADDING (0x00) bytes inside the encryption envelope. RFC 9000 diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt new file mode 100644 index 000000000..6f78c43c0 --- /dev/null +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/CloseDatagramRfcComplianceTest.kt @@ -0,0 +1,174 @@ +/* + * 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.QuicWriter +import com.vitorpamplona.quic.frame.ConnectionCloseFrame +import com.vitorpamplona.quic.tls.PermissiveCertificateValidator +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +/** + * Regression tests for two RFC 9000 violations found via the + * `quic-interop-runner` against aioquic on 2026-05-06: + * + * - §10.2.3 — `CONNECTION_CLOSE (Application)` (0x1d) MUST NOT appear in + * Initial / Handshake packets. Application-error closes that fire before + * 1-RTT keys exist must be encoded as `CONNECTION_CLOSE (Transport)` + * (0x1c) with `errorCode = APPLICATION_ERROR (0x0c)`. + * - §14.1 — any client datagram containing an Initial MUST be ≥ 1200 + * bytes, even when carrying only a `CONNECTION_CLOSE`. + * + * Pre-fix, the writer's CLOSING branch built a tiny ~45-byte Initial with + * frame type 0x1d. The runner's aioquic server silently dropped it (correct + * per spec) and our handshake hung until our 10s timeout. + */ +class CloseDatagramRfcComplianceTest { + @Test + fun `pre-handshake close datagram is padded to at least 1200 bytes per RFC 9000 sec 14_1`() = + runBlocking { + val conn = + QuicConnection( + serverName = "example.test", + config = QuicConnectionConfig(), + tlsCertificateValidator = PermissiveCertificateValidator(), + ) + // Initial sendProtection is wired in QuicConnection's init block; + // no handshake required to exercise the close path. + conn.status = QuicConnection.Status.CLOSING + + val datagram = drainOutbound(conn, nowMillis = 0L) + requireNotNull(datagram) { "drainOutbound must produce a close datagram when CLOSING with Initial keys" } + + assertTrue( + datagram.size >= 1200, + "client Initial datagram MUST be ≥ 1200 bytes per RFC 9000 §14.1, " + + "got ${datagram.size} (this was bug A from the aioquic interop run)", + ) + // First byte: 1100????b — long-header form + Initial type. + val firstByte = datagram[0].toInt() and 0xff + assertEquals(0xc0, firstByte and 0xf0, "must be a long-header packet") + assertEquals(0x00, firstByte and 0x30, "must be type=Initial (00)") + } + + @Test + fun `PTO probe emits a PING at Initial level pre-handshake (RFC 9002 sec 6_2_4)`() = + runBlocking { + // Reproduces the third bug found via the aioquic interop run on + // 2026-05-06: when the first ClientHello is dropped (e.g. sim + // queues it before the server is ready), the driver's PTO timer + // sets `pendingPing = true`, but the writer only honored that + // flag in the 1-RTT path. Pre-handshake the flag was silently + // discarded, so the second drain produced no Initial datagram — + // the connection sat mute until our 10s handshake timeout. + // + // Pre-fix: drainOutbound returned null and the connection slept + // on the next PTO. Post-fix: a PING-bearing Initial datagram is + // emitted, eliciting an ACK from the peer that feeds loss + // detection and unblocks CRYPTO retransmit. + val conn = + QuicConnection( + serverName = "example.test", + config = QuicConnectionConfig(), + tlsCertificateValidator = PermissiveCertificateValidator(), + ) + // Initial sendProtection is wired in QuicConnection's init {} + // block; no handshake required to exercise the PTO probe path. + // Skip conn.start() so the cryptoSend buffer is empty — this is + // exactly the post-ClientHello-sent state when PTO fires. + conn.pendingPing = true + + val datagram = drainOutbound(conn, nowMillis = 0L) + assertNotNull( + datagram, + "PTO probe MUST produce an Initial datagram pre-handshake — RFC 9002 §6.2.4", + ) + // RFC 9000 §14.1: client datagrams containing an Initial MUST + // be ≥ 1200 bytes. The writer's padding rebuild now accounts + // for Length-varint growth (1 → 2 bytes) when the natural-size + // payload is small (PING-only) so the deficit calculation + // produces a final size that strictly meets the spec floor. + assertTrue( + datagram.size >= 1200, + "PTO Initial datagram MUST be ≥ 1200 bytes per RFC 9000 §14.1, got ${datagram.size}", + ) + assertEquals(false, conn.pendingPing, "pendingPing MUST be cleared after the probe") + } + + @Test + fun `single-byte PING-only Initial pads to exactly 1200 bytes (no overshoot beyond varint growth)`() = + runBlocking { + // Boundary case for the padding-rebuild deficit calculation. + // The smallest possible Initial-level frame payload is a single + // PING frame (1 byte, encoded as 0x01 — RFC 9000 §19.2). With + // no token, the 1-byte natural payload encodes a Length varint + // of 1 byte. Once the rebuild adds ~1170 bytes of PADDING the + // Length value crosses the 63-byte varint boundary and grows + // to 2 bytes. The deficit calculation MUST account for that + // growth, otherwise the rebuilt packet falls 1 byte short of + // the §14.1 1200-byte floor. + // + // Asserts the strict floor (≥ 1200) AND that we don't overshoot + // by more than the varint-growth delta plus a small slack — the + // padded packet should land in the 1200..1203 range, never + // 1199 (pre-fix) and never 1300+ (over-correction). + val conn = + QuicConnection( + serverName = "example.test", + config = QuicConnectionConfig(), + tlsCertificateValidator = PermissiveCertificateValidator(), + ) + conn.pendingPing = true + + val datagram = drainOutbound(conn, nowMillis = 0L) + assertNotNull(datagram, "PING-only PTO probe must produce a datagram") + assertTrue( + datagram.size >= 1200, + "padded Initial MUST be ≥ 1200 bytes (RFC 9000 §14.1), got ${datagram.size}", + ) + assertTrue( + datagram.size <= 1203, + "padded Initial should not overshoot the 1200 floor by more than the " + + "Length-varint growth (≤ 3 bytes), got ${datagram.size}", + ) + } + + @Test + fun `ConnectionCloseFrame encodes type 0x1c when frameType is non-null (transport close)`() { + // Bug B fix relies on the writer passing frameType=0L (instead of + // null) when emitting a close at Initial / Handshake level. Encode + // path must produce 0x1c for that case, 0x1d only for app-level. + val transportClose = ConnectionCloseFrame(errorCode = 0x0c, frameType = 0L, reason = "") + val w1 = QuicWriter() + transportClose.encode(w1) + val transportBytes = w1.toByteArray() + assertEquals(0x1c.toByte(), transportBytes[0], "transport CONNECTION_CLOSE must serialize as 0x1c") + + val appClose = ConnectionCloseFrame(errorCode = 0, frameType = null, reason = "") + val w2 = QuicWriter() + appClose.encode(w2) + val appBytes = w2.toByteArray() + assertEquals(0x1d.toByte(), appBytes[0], "application CONNECTION_CLOSE must serialize as 0x1d") + } +}