Files
amethyst/nestsClient
Claude e250b76272 fix(quic): six critical correctness + security bugs from review
Synthesizes findings from four parallel layer reviews. Each fix here would
have broken or weakened live interop:

C1 — TlsClient stored cipher suite (was hardcoded)
  TlsClient.currentCipherSuite() always returned AES-128-GCM-SHA256, even
  when the server picked TLS_CHACHA20_POLY1305_SHA256. The QUIC layer would
  then install AES-GCM AEAD + AES-ECB header protection over a ChaCha20-
  derived secret → silent 1-RTT decrypt failure. Now stores the negotiated
  cipher from ServerHello and returns it.

C2 — AckTracker records the actual packet PN, not the largest received
  dispatchFrames() in QuicConnectionParser was passing
  state.pnSpace.largestReceived to the ACK tracker. With two coalesced
  packets in one datagram, only the larger PN was ever tracked → server
  retransmits the smaller forever. Plumb the parsed packet's PN through
  dispatchFrames and feed it to receivedPacket(). Also always record (even
  for non-ack-eliciting packets) so the peer's loss recovery sees a
  contiguous picture.

C5 — bounds-check every readVarint().toInt() length in frame decode
  CRYPTO, STREAM (LEN), CONNECTION_CLOSE reason, DATAGRAM_LEN, and ACK
  range count all read a 62-bit varint, truncate to Int, and pass straight
  to readBytes / repeat. A hostile peer could send length=2^62-1 → crash or
  multi-GB allocation. Added boundedLength() + boundedRangeCount() helpers
  that reject if value < 0 or > remaining.

C6 — frame type dispatch uses readVarint, not readByte
  RFC 9000 §12.4 specifies frame types as varints. We were reading a single
  byte, so any extension frame type ≥ 0x40 (e.g. ACK_FREQUENCY 0xAF) would
  be mis-dispatched. All current types are < 64 so the 1-byte form matches
  the 1-byte varint, but the change is forward-compatible.

C7 — CertificateValidator required (no silent skip)
  Both QuicConnection and TlsClient previously had `validator: ... = null`
  defaults. A misconfigured caller would silently accept any server's
  certificate. Removed the defaults; null is now an explicit opt-in for
  in-process loopback tests. Added JdkCertificateValidator backed by the
  platform / JDK system trust store with proper SAN-based hostname check
  and signature verification for ECDSA / RSA-PSS / RSA-PKCS1 / Ed25519.
  QuicWebTransportFactory uses it by default.

C8 — thread-safety on connection state
  QuicConnection.streams, pendingDatagrams, nextLocalBidiIndex/UniIndex
  were mutated from the driver loops and from app coroutines without
  synchronization → ConcurrentModificationException waiting to happen.
  Moved the mutex onto QuicConnection itself; the driver wraps feed/drain
  with `connection.lock.withLock { ... }`, public mutators became suspend
  and acquire the same lock. Internal helpers used by feed/drain are
  marked `Locked` to make the precondition explicit.

  Also replaced the `delay(2)` send-loop polling with a CONFLATED
  `Channel<Unit>` wakeup — app writes (queueDatagram, openBidiStream,
  stream write via the WT adapter) call `driver.wakeup()`. Idle CPU
  drops to zero between packets.

  awaitHandshake() replaces the busy-poll over `conn.status` in
  QuicWebTransportFactory.connect — backed by a CompletableDeferred that
  the TLS listener completes on onHandshakeComplete() or fails on a torn
  down read loop.

Tests: full :quic:jvmTest and :nestsClient:jvmTest suites pass — every
RFC 9001 Appendix A vector still verifies bit-for-bit.

Remaining critical work (in progress, separate commits):
  C3+C4 — HTTP/3 frame reader + WebTransport response :status check
  C9    — flow-control enforcement + MAX_STREAM_DATA crediting

https://claude.ai/code/session_01EC1tfXfap8k8GyKvrxkxZx
2026-04-25 21:58:45 +00:00
..