From 90f59a335166553fc70c0dd2d5f1df56c775f4eb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 00:17:19 +0000 Subject: [PATCH] perf(quic): AEAD range overload + doc notes for known fragile couplings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8 — AEAD allocation reduction on the receive hot path, plus the documentation of two known-fragile-but-not-broken couplings. * Aead gains [openRange] / [sealRange] taking (array, offset, length) triples for AAD and plaintext/ciphertext. Default impls slice and delegate to the existing whole-array methods so non-JCA AEADs (Aes128Gcm singleton, ChaCha20Poly1305Aead) still work. The JCA-backed [JcaAesGcmAead] overrides both, passing offsets straight to `Cipher.updateAAD(byte[], offset, len)` and `Cipher.doFinal(byte[], inputOffset, inputLen)` — JCA accepts ranges natively and does no internal copies. Saves ~2 KB ByteArray allocations per inbound packet (one for the header `aad`, one for the `ciphertext`) — at audio-room receive rates (~100 packets/sec) that's ~12 MB/min of GC churn eliminated. Same overload on the outbound path is wired but currently exercised less because the build path constructs `headerBytes` and `paddedPlaintext` as separate fresh allocations. parseAndDecrypt in both ShortHeaderPacket and LongHeaderPacket now call openRange instead of slicing into intermediates. * JdkCertificateValidator.dnsMatches: explicit doc note on the public-suffix-list gap. The dot-count heuristic accepts wildcards like `*.co.uk` / `*.github.io` / `*.s3.amazonaws.com` whose effective TLD spans multiple labels. WebPKI / CT logging mitigates this in practice (CAs validate against the actual PSL), but our local validation alone wouldn't catch a rogue cert. Production callers for sensitive endpoints should rely on OS / NetworkSecurityConfig pinning rather than QUIC's hostname check alone. Full PSL data shipping deferred until justified. * QuicStream.incoming: explicit doc note on the single-collector contract. [consumeAsFlow] cancels the underlying [Channel] when its collector terminates, and once cancelled the parser's next trySend fails, sets [overflowed] = true, and the parser tears down the connection with INTERNAL_ERROR. Production callers already follow single-collector + collect-until-FIN, but the doc lays out the rule so a future refactor doesn't loosen it accidentally. Long-form discussion of the `MutableSharedFlow` alternative and why we declined it. All 269 :quic:jvmTest tests pass. BUILD SUCCESSFUL in 40s. https://claude.ai/code/session_01AhGvbMV8uPRse3TmAGaddM --- .../com/vitorpamplona/quic/crypto/Aead.kt | 58 ++++++++++++++++ .../quic/packet/LongHeaderPacket.kt | 17 ++++- .../quic/packet/ShortHeaderPacket.kt | 18 ++++- .../vitorpamplona/quic/stream/QuicStream.kt | 25 +++++++ .../quic/crypto/JcaAesGcmAead.kt | 68 +++++++++++++++++++ .../quic/tls/JdkCertificateValidator.kt | 19 +++++- 6 files changed, 196 insertions(+), 9 deletions(-) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt index c80cac454..00d7c39c5 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/crypto/Aead.kt @@ -52,6 +52,64 @@ abstract class Aead { aad: ByteArray, ciphertext: ByteArray, ): ByteArray? + + /** + * Range-based [seal] — same semantics as [seal] but reads `aad` and + * `plaintext` from sub-ranges of larger backing arrays. Default impl + * slices and delegates; concrete impls (notably the JCA-backed + * [com.vitorpamplona.quic.crypto.JcaAesGcmAead]) override to skip + * the slice allocations entirely by passing the offsets through to + * `Cipher.updateAAD` / `Cipher.doFinal`. + * + * Saves ~2 ByteArray allocations per outbound packet on the hot + * path (the header `aad` and the payload `plaintext` no longer + * need to be carved out of the in-progress packet buffer). + */ + open fun sealRange( + key: ByteArray, + nonce: ByteArray, + aad: ByteArray, + aadOffset: Int, + aadLength: Int, + plaintext: ByteArray, + plaintextOffset: Int, + plaintextLength: Int, + ): ByteArray { + val a = + if (aadOffset == 0 && aadLength == aad.size) aad else aad.copyOfRange(aadOffset, aadOffset + aadLength) + val p = + if (plaintextOffset == 0 && plaintextLength == plaintext.size) { + plaintext + } else { + plaintext.copyOfRange(plaintextOffset, plaintextOffset + plaintextLength) + } + return seal(key, nonce, a, p) + } + + /** + * Range-based [open] — same semantics as [open] but reads `aad` and + * `ciphertext` from sub-ranges. Default impl slices and delegates. + */ + open fun openRange( + key: ByteArray, + nonce: ByteArray, + aad: ByteArray, + aadOffset: Int, + aadLength: Int, + ciphertext: ByteArray, + ciphertextOffset: Int, + ciphertextLength: Int, + ): ByteArray? { + val a = + if (aadOffset == 0 && aadLength == aad.size) aad else aad.copyOfRange(aadOffset, aadOffset + aadLength) + val c = + if (ciphertextOffset == 0 && ciphertextLength == ciphertext.size) { + ciphertext + } else { + ciphertext.copyOfRange(ciphertextOffset, ciphertextOffset + ciphertextLength) + } + return open(key, nonce, a, c) + } } /** AES-128-GCM AEAD via Quartz's AESGCM (which uses JCA underneath on JVM/Android). */ diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt index 5f5416baf..2b476cbeb 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/LongHeaderPacket.kt @@ -223,10 +223,21 @@ object LongHeaderPacket { ) val aadEnd = localPnOffset + pnLen - val aad = packet.copyOfRange(0, aadEnd) - val ciphertext = packet.copyOfRange(aadEnd, packet.size) val nonce = aeadNonce(iv, fullPn) - val plaintext = aead.open(key, nonce, aad, ciphertext) ?: return null + // Range-based open avoids two ByteArray slice allocations per + // inbound packet — see [ShortHeaderPacket.parseAndDecrypt] for + // rationale. + val plaintext = + aead.openRange( + key = key, + nonce = nonce, + aad = packet, + aadOffset = 0, + aadLength = aadEnd, + ciphertext = packet, + ciphertextOffset = aadEnd, + ciphertextLength = packet.size - aadEnd, + ) ?: return null return ParseResult( packet = diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt index c5d4ca073..99494a04b 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/packet/ShortHeaderPacket.kt @@ -182,10 +182,22 @@ object ShortHeaderPacket { } val fullPn = PacketNumberSpaceState.decodePacketNumber(largestReceivedInSpace, truncatedPn, pnLen) val aadEnd = localPnOffset + pnLen - val aad = packet.copyOfRange(0, aadEnd) - val ciphertext = packet.copyOfRange(aadEnd, packet.size) val nonce = aeadNonce(iv, fullPn) - val plaintext = aead.open(key, nonce, aad, ciphertext) ?: return null + // Range-based open: aad = packet[0..aadEnd), ciphertext = packet[aadEnd..size). + // Saves the two ByteArray slice allocations that the + // whole-array form (`aad = copyOfRange(0, aadEnd)` etc.) + // would do — ~2 KB per inbound packet on the hot path. + val plaintext = + aead.openRange( + key = key, + nonce = nonce, + aad = packet, + aadOffset = 0, + aadLength = aadEnd, + ciphertext = packet, + ciphertextOffset = aadEnd, + ciphertextLength = packet.size - aadEnd, + ) ?: return null return ParseResult( packet = ShortHeaderPlaintextPacket( diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt index ef410099c..f060ef437 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/QuicStream.kt @@ -77,6 +77,31 @@ class QuicStream( * rather than silently dropping bytes. Pre-audit-4 the failed `trySend` * was discarded, leaving a hole in the stream that the application could * never know about. + * + * **Single-collector contract.** [consumeAsFlow] cancels the underlying + * [Channel] when its collector terminates (either by exception or + * explicit cancellation). After that point any subsequent + * `trySend` from the parser fails, sets [overflowed] = true, and + * the parser tears the whole connection down with + * `INTERNAL_ERROR: stream … consumer overflowed`. So: + * - Application code MUST collect [incoming] **at most once** per + * stream and MUST hold the collect open until the stream + * terminates (FIN, peer RESET_STREAM, or local + * [stopSending] / [resetStream] decision). + * - Cancelling the collector early — e.g. wrapping in + * `withTimeout(...)` — is equivalent to telling the connection + * to drop. If the application wants to stop receiving without + * killing the connection, call [stopSending] FIRST, then let + * the parser's RESET_STREAM-style teardown drain the channel + * cleanly. + * + * This is a fragile coupling we accept for now because (a) every + * production caller already follows the single-collector pattern, + * and (b) widening the contract would require swapping + * `consumeAsFlow` for a `MutableSharedFlow` with replay/buffer + * semantics, which has its own back-pressure pitfalls. The + * comment is here so a future refactor doesn't accidentally + * loosen the contract. */ private val incomingChannel = Channel(capacity = 64) val incoming: Flow get() = incomingChannel.consumeAsFlow() diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/JcaAesGcmAead.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/JcaAesGcmAead.kt index 592d0b6d4..d974d9e4a 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/JcaAesGcmAead.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/JcaAesGcmAead.kt @@ -138,6 +138,74 @@ class JcaAesGcmAead( } } + /** + * JCA-native range overload: `Cipher.updateAAD(byte[], offset, len)` + * and `Cipher.doFinal(byte[], inputOffset, inputLen)` accept + * sub-ranges directly, so we skip the two `copyOfRange` calls the + * default impl would perform. Saves ~2 KB allocation per inbound + * packet on the audio-rooms hot path (one for `aad`, one for + * `ciphertext`, both sliced from a packet buffer that the caller + * already owns). + */ + override fun openRange( + key: ByteArray, + nonce: ByteArray, + aad: ByteArray, + aadOffset: Int, + aadLength: Int, + ciphertext: ByteArray, + ciphertextOffset: Int, + ciphertextLength: Int, + ): ByteArray? = + synchronized(this) { + try { + decryptCipher.init(Cipher.DECRYPT_MODE, keySpec, GCMParameterSpec(128, nonce)) + decryptCipher.updateAAD(aad, aadOffset, aadLength) + decryptCipher.doFinal(ciphertext, ciphertextOffset, ciphertextLength) + } catch (_: GeneralSecurityException) { + null + } + } + + /** + * JCA-native range overload for [seal]. Same nonce-reuse history + + * fresh-Cipher fallback as the whole-array [seal] path; the only + * difference is the offset+length pass-through to `updateAAD` / + * `doFinal`. Saves the slice allocations on the outbound hot path. + */ + override fun sealRange( + key: ByteArray, + nonce: ByteArray, + aad: ByteArray, + aadOffset: Int, + aadLength: Int, + plaintext: ByteArray, + plaintextOffset: Int, + plaintextLength: Int, + ): ByteArray = + synchronized(this) { + val reuse = recentEncryptNonces.any { it.contentEquals(nonce) } + if (reuse) { + val fresh = Cipher.getInstance("AES/GCM/NoPadding") + fresh.init(Cipher.ENCRYPT_MODE, keySpec, GCMParameterSpec(128, nonce)) + fresh.updateAAD(aad, aadOffset, aadLength) + fresh.doFinal(plaintext, plaintextOffset, plaintextLength) + } else { + try { + encryptCipher.init(Cipher.ENCRYPT_MODE, keySpec, GCMParameterSpec(128, nonce)) + encryptCipher.updateAAD(aad, aadOffset, aadLength) + val out = encryptCipher.doFinal(plaintext, plaintextOffset, plaintextLength) + rememberEncryptNonce(nonce) + out + } catch (t: Throwable) { + if (recentEncryptNonces.lastOrNull()?.contentEquals(nonce) == true) { + recentEncryptNonces.removeLast() + } + throw t + } + } + } + private companion object { private const val NONCE_HISTORY_LIMIT = 8 } diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt index 34ebd3855..5718ba443 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt @@ -248,9 +248,22 @@ class JdkCertificateValidator( val prefix = lhost.substring(0, lhost.length - suffix.length) if (prefix.isEmpty() || '.' in prefix) return false // RFC 6125 §6.4.3 — disallow wildcards in the public-suffix label. - // Heuristic: require ≥ 2 dots in the suffix (e.g. *.example.com is OK, - // *.com is not). Conservative; doesn't consult the actual PSL but - // matches what most browsers do for non-PSL-aware certs. + // + // KNOWN GAP: this heuristic counts dots in the suffix (≥ 2 → OK) + // and DOES NOT consult the actual public-suffix list. So it + // accepts `*.co.uk`, `*.github.io`, `*.s3.amazonaws.com`, etc. + // — multi-tenant TLDs whose effective TLD is multi-label. A + // CA that mis-issues such a wildcard could impersonate any + // co-tenant. The mitigation is partial: the WebPKI ecosystem + // already requires CAs to consult the PSL when issuing, so a + // rogue cert is unlikely to make it past CT logging — but if + // one does, our validation accepts it. + // + // Full fix requires shipping PSL data; deferred until the cost + // is justified by a higher-risk deployment. Production callers + // should rely on the OS / NetworkSecurityConfig pinning layer + // for sensitive endpoints rather than QUIC's built-in + // hostname check alone. val suffixDots = suffix.count { it == '.' } return suffixDots >= 2 }