From 07ba23a71ceead9a382e1fa1f862b49bd1b2e72d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 19:57:55 +0000 Subject: [PATCH] fix(quic): second-pass audit fixes for path validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness bugs surfaced by post-fix re-audit, plus minor cleanups. - Bug A (validation hang): checkPathValidationTimeoutLocked was only called from handlePtoFired. PATH_CHALLENGE is ack-eliciting so the peer ACKs it; that ACK resets consecutivePtoCount, which means the PTO timer that used to host the budget check stops firing. Validation could hang indefinitely on a peer that ACKs but doesn't reply with PATH_RESPONSE. Fix: drive the budget check from drainOutbound (every send-loop wake). - Bug B (stale retire): under abrupt-migration semantics the prior CID is abandoned the moment we rotate. Queuing the retire only inside applyPathResponse meant two consecutive failed validations would leave the original seq=0 unretired forever. Fix: queue priorSeq in tryStartValidation; advance activeCidSequence at trigger time so it tracks the on-wire DCID. - Bug C (spec MUST violation): RFC 9000 §5.1.2 requires server- forced retirement of the active CID when the peer's retire_prior_to advances past it. Previously the parser silently accepted the offer and we kept stamping a now-retired CID. Fix: new PathValidator.forceRotateToHigherSequence; called from applyPeerNewConnectionIdLocked after a successful Stored result. No PATH_CHALLENGE needed (same path, just different CID). Closes connection with CONNECTION_ID_LIMIT_ERROR if the pool is empty when forced rotation is needed. Concurrency: - Add @Volatile to consecutivePtoCount. The driver kdoc claimed it already was; it wasn't. The send-loop reads it lockless for backoff calculation while three writers mutate it (driver PTO fire, parser ACK reset, applyPeerPathResponseLocked reset). Cleanup: - Drop redundant destinationConnectionId re-stamp in applyPeerPathResponseLocked (already rotated at challenge time). - Fix PathMigrationResult kdoc to acknowledge that NotConnected is produced only by the connection-level wrapper. - Update applyPeerNewConnectionIdLocked kdoc with the §5.1.2 forced-rotation contract. Tests: - PathValidatorTest: + triggerRetiresPriorSequenceImmediately (Bug B) + twoConsecutiveFailedValidationsRetireAllAbandonedSequences (Bug B regression — would have caught the original miss) + forceRotateRunsWhenWatermarkPassesActiveCid (Bug C) + forceRotateNoOpWhenWatermarkBelowActive (Bug C edge) + forceRotateRotatesAgainWhenNewerOfferAdvancesWatermark (Bug C cascading) - ClientPathMigrationTest: + newConnectionIdWithRetirePriorToPastActiveForcesRotationOnSamePath (Bug C wire-level) + Updated fullMigrationRoundTrip to assert RETIRE rides in the same packet as PATH_CHALLENGE under abrupt-migration semantics. + Updated pathResponseWithMismatchingPayloadKeepsValidatingAndDcid to reflect activeCidSequence advances at trigger time. All :quic:jvmTest (39 tests in path-validation suite) and :nestsClient:jvmTest pass. https://claude.ai/code/session_01PVVhSQXvw4K4oQ46FzpgaT --- .../quic/connection/PathValidator.kt | 140 +++++++++++++++--- .../quic/connection/QuicConnection.kt | 91 ++++++++---- .../quic/connection/QuicConnectionWriter.kt | 11 ++ .../connection/ClientPathMigrationTest.kt | 96 +++++++++--- .../quic/connection/PathValidatorTest.kt | 100 +++++++++++++ 5 files changed, 369 insertions(+), 69 deletions(-) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt index 0e79d1255..8ed2a7289 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt @@ -116,8 +116,12 @@ sealed class PathValidationState { } /** - * Outcome of [PathValidator.tryStartValidation], used by the caller - * (driver / test) to know whether a probe was actually issued. + * Outcome of attempting to start a client-initiated path + * migration. Produced by [PathValidator.tryStartValidation] (which + * never returns [NotConnected]) and the connection-level wrapper + * [QuicConnection.triggerPathMigrationLocked] (which adds the + * pre-handshake gate). Used by the driver / application code that + * fires the trigger to decide whether to retry later. */ enum class PathMigrationResult { /** A `PATH_CHALLENGE` was queued; state is [PathValidationState.Validating]. */ @@ -402,11 +406,16 @@ class PathValidator( * sequence unused CID, generates a random 8-byte challenge, * queues the challenge in [pendingChallenges], and transitions * [state] to [PathValidationState.Validating]. The selected - * CID is removed from the unused pool; on validation success - * [applyPathResponse] commits it (callers swap the writer's - * DCID), and on timeout [checkValidationTimeout] queues it - * for RETIRE_CONNECTION_ID so the peer can drop the routing - * entry. + * CID is removed from the unused pool. The prior CID's + * sequence is queued for RETIRE_CONNECTION_ID immediately — + * we're abandoning that path and the peer can drop the + * routing entry now (RFC 9000 §5.1.2). [activeCidSequence] + * is bumped to the new sequence so it tracks what the writer + * is putting on the wire. + * + * On success the writer just promotes [PathValidationState] to + * [PathValidationState.Succeeded]; on timeout the failed CID + * is also queued for retire (see [checkValidationTimeout]). * * Caller is responsible for two follow-ups when this returns * [PathMigrationResult.Started]: @@ -428,12 +437,25 @@ class PathValidator( require(it.size == 8) { "challenge payload supplier must return 8 bytes" } } pendingChallenges.addLast(payload) + val priorSeq = activeCidSequence + // Bug-B fix: retire the prior sequence at trigger time. + // Abrupt migration means we won't use the old DCID again, + // so the peer's routing entry for it is dead state. Without + // this, two consecutive failed validations leave the + // initial seq=0 unretired indefinitely. + if (priorSeq != seq) queueRetireSequence(priorSeq) + // [activeCidSequence] tracks "the seq the writer is currently + // stamping on the wire", so it advances at trigger time + // alongside `destinationConnectionId`. The + // [PathValidationState.Validating.priorCidSequence] field + // preserves the value for diagnostic / qlog purposes. + activeCidSequence = seq state = PathValidationState.Validating( challengeData = payload, newCidSequence = seq, newConnectionId = entry.connectionId, - priorCidSequence = activeCidSequence, + priorCidSequence = priorSeq, startedAtMillis = nowMillis, priorPtoMillis = currentPtoMillis, ) @@ -441,30 +463,33 @@ class PathValidator( } /** - * Process an inbound `PATH_RESPONSE` payload. Returns true when - * it matched the outstanding challenge and the migration just - * completed; false if there's no outstanding challenge or the - * payload doesn't match (an attacker echoing random bytes). + * Process an inbound `PATH_RESPONSE` payload. Confirms the + * outstanding challenge: state transitions to + * [PathValidationState.Succeeded] and a [ValidationOutcome.Validated] + * is returned with the new sequence + connection-id bytes for + * the connection-level wrapper to surface in qlog. * - * Side effects on success: - * - [state] transitions to [PathValidationState.Succeeded]. - * - [activeCidSequence] is bumped to the validated sequence. - * - The prior sequence is queued for RETIRE_CONNECTION_ID. - * - Returns the new entry so the connection can swap its - * `destinationConnectionId` field. + * Note that [activeCidSequence] and the retire-queue entry for + * the prior sequence were already mutated at challenge-issue + * time (see [tryStartValidation]) — under the abrupt-migration + * model the prior CID is abandoned the moment we rotate, not + * when the response arrives. So this method has no further + * cleanup work beyond marking the state as Succeeded. + * + * Returns [ValidationOutcome.NotValidating] if no challenge + * is outstanding, or [ValidationOutcome.PayloadMismatch] if + * the bytes don't match. Both are silently dropped at the + * caller per RFC 9000 §8.2.2. */ fun applyPathResponse(payload: ByteArray): ValidationOutcome { val current = state as? PathValidationState.Validating ?: return ValidationOutcome.NotValidating if (!payload.contentEquals(current.challengeData)) return ValidationOutcome.PayloadMismatch - val priorSeq = activeCidSequence - activeCidSequence = current.newCidSequence - if (priorSeq != current.newCidSequence) queueRetireSequence(priorSeq) state = PathValidationState.Succeeded successfulValidations += 1 return ValidationOutcome.Validated( newSequence = current.newCidSequence, connectionId = current.newConnectionId, - retiredSequence = priorSeq, + retiredSequence = current.priorCidSequence, ) } @@ -534,6 +559,77 @@ class PathValidator( } } + /** + * RFC 9000 §5.1.2: "If the active connection ID's sequence + * number is less than the Retire Prior To value, the endpoint + * MUST retire the active connection ID and adopt one with a + * higher sequence number." + * + * Server-forced retirement does NOT require path validation — + * it's the same 4-tuple, just a different CID. Pick the + * lowest-sequence entry from the pool, swap it in, queue the + * old active sequence for RETIRE_CONNECTION_ID. If no spare + * is available, returns null and the caller should close the + * connection (we have no CID to use that satisfies the peer's + * watermark). + * + * If a path validation happens to be in progress when this + * fires, the in-flight challenge's CID may itself fall under + * the watermark — abandon the validation and queue the failed + * sequence for retire alongside the swap. + */ + fun forceRotateToHigherSequence(): ForcedRotationResult? { + if (activeCidSequence >= retirePriorToWatermark) return null + val (seq, entry) = unusedCids.entries.firstOrNull() ?: return ForcedRotationResult.NoSpareCid + unusedCids.remove(seq) + val priorSeq = activeCidSequence + queueRetireSequence(priorSeq) + activeCidSequence = seq + + // If we were mid-validation, the challenge we issued is + // for a CID that may now be retired (priorSeq could be + // the seq we were trying to validate). Abandon it. + val s = state + if (s is PathValidationState.Validating) { + queueRetireSequence(s.newCidSequence) + pendingChallenges.removeAll { it.contentEquals(s.challengeData) } + state = PathValidationState.Failed + failedValidations += 1 + } + return ForcedRotationResult.Rotated(newSequence = seq, connectionId = entry.connectionId, retiredSequence = priorSeq) + } + + sealed class ForcedRotationResult { + /** + * Watermark moved past active CID but the pool was empty — + * the caller cannot satisfy the spec MUST. RFC 9000 §5.1.2 + * leaves the recovery to the caller; closing the connection + * with CONNECTION_ID_LIMIT_ERROR is the safe default. + */ + object NoSpareCid : ForcedRotationResult() + + data class Rotated( + val newSequence: Long, + val connectionId: ByteArray, + val retiredSequence: Long, + ) : ForcedRotationResult() { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is Rotated) return false + return newSequence == other.newSequence && + connectionId.contentEquals(other.connectionId) && + retiredSequence == other.retiredSequence + } + + override fun hashCode(): Int { + var h = newSequence.hashCode() + h = 31 * h + connectionId.contentHashCode() + h = 31 * h + retiredSequence.hashCode() + return h + } + } + } + companion object { /** * Default cap on the unused-CID pool. RFC 9000 §18.2 default 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 bf198fed1..c951b3f24 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -560,7 +560,17 @@ class QuicConnection( * PTO expiration without an intervening ACK; reset to 0 when an * inbound ACK acknowledges any ack-eliciting packet. The driver * doubles its sleep between probes by `1 shl consecutivePtoCount`. + * + * `@Volatile` because the driver's send-loop reads this without + * holding `streamsLock` (in the backoff calculation in + * [QuicConnectionDriver.sendLoopBody]), while three writers + * mutate it: the driver's PTO firing path, the parser's ACK + * handler resetting on inbound ACK, and the + * [applyPeerPathResponseLocked] reset on successful migration. + * Without volatility the JIT can cache a stale value + * indefinitely and the backoff multiplier grows unbounded. */ + @Volatile internal var consecutivePtoCount: Int = 0 /** @@ -1911,13 +1921,15 @@ class QuicConnection( * RFC 9000 §19.15: store a peer-issued connection ID into the * [pathValidator] pool, enforcing `retire_prior_to` semantics * and capping pool size. On a peer protocol violation - * ([PathValidator.RecordResult.RetirePriorToExceedsSequence] / - * [PathValidator.RecordResult.RetirePriorToRegressed] / - * [PathValidator.RecordResult.DuplicateSequenceMismatch] / - * [PathValidator.RecordResult.InvalidCidLength] / - * [PathValidator.RecordResult.InvalidStatelessResetToken]) the - * connection is closed with FRAME_ENCODING_ERROR — these are - * MUSTs in the spec. + * the connection is closed with FRAME_ENCODING_ERROR / + * PROTOCOL_VIOLATION — these are MUSTs in the spec. + * + * After a successful store, if the peer's `retire_prior_to` + * has advanced past our currently-active CID sequence, + * RFC 9000 §5.1.2 obligates us to "retire the active + * connection ID and adopt one with a higher sequence number." + * That's a server-forced rotation on the same path — no + * PATH_CHALLENGE needed (Bug-C fix). * * Caller must hold [streamsLock]. */ @@ -1927,23 +1939,18 @@ class QuicConnection( connectionId: ByteArray, statelessResetToken: ByteArray, ) { - when ( + val recordResult = pathValidator.recordPeerNewConnectionId( sequenceNumber = sequenceNumber, retirePriorTo = retirePriorTo, connectionId = connectionId, statelessResetToken = statelessResetToken, ) - ) { - PathValidator.RecordResult.Stored -> { - Unit - } - - PathValidator.RecordResult.Duplicate -> { - Unit - } - - PathValidator.RecordResult.AlreadyRetired -> { + when (recordResult) { + PathValidator.RecordResult.Stored, + PathValidator.RecordResult.Duplicate, + PathValidator.RecordResult.AlreadyRetired, + -> { Unit } @@ -1953,31 +1960,65 @@ class QuicConnection( // we MAY treat this as CONNECTION_ID_LIMIT_ERROR. We // only drop silently here — pinning memory is the // real concern, and the cap defense already handled - // that. + // that. Skip the watermark check below; this offer + // wasn't accepted. + return } PathValidator.RecordResult.RetirePriorToExceedsSequence -> { markClosedExternally( "FRAME_ENCODING_ERROR: NEW_CONNECTION_ID retire_prior_to ($retirePriorTo) > sequence_number ($sequenceNumber)", ) + return } PathValidator.RecordResult.DuplicateSequenceMismatch -> { markClosedExternally( "PROTOCOL_VIOLATION: NEW_CONNECTION_ID seq=$sequenceNumber re-issued with different bytes", ) + return } PathValidator.RecordResult.InvalidCidLength -> { markClosedExternally( "FRAME_ENCODING_ERROR: NEW_CONNECTION_ID has invalid cid length", ) + return } PathValidator.RecordResult.InvalidStatelessResetToken -> { markClosedExternally( "FRAME_ENCODING_ERROR: NEW_CONNECTION_ID has invalid stateless reset token length", ) + return + } + } + + // Bug-C fix: §5.1.2 server-forced rotation. The watermark + // may have advanced past our active CID as a side effect of + // [recordPeerNewConnectionId]. If so we MUST swap on the + // same path before the next outbound packet (which would + // otherwise stamp a now-retired CID). + when (val rotation = pathValidator.forceRotateToHigherSequence()) { + null -> { + Unit + } + + // active CID is still valid; nothing to do. + PathValidator.ForcedRotationResult.NoSpareCid -> { + // Watermark forced retirement of the active CID but + // the pool is empty — we have nothing valid to use. + // RFC 9000 §5.1.2 leaves recovery to the endpoint; + // CONNECTION_ID_LIMIT_ERROR is the canonical close. + markClosedExternally( + "CONNECTION_ID_LIMIT_ERROR: peer retired our active CID with no spare available", + ) + } + + is PathValidator.ForcedRotationResult.Rotated -> { + destinationConnectionId = ConnectionId(rotation.connectionId) + qlogObserver.onConnectionIdActivated("peer", rotation.newSequence, rotation.connectionId) + qlogObserver.onConnectionIdRetired("peer", rotation.retiredSequence) } } } @@ -2014,14 +2055,12 @@ class QuicConnection( qlogObserver.onConnectionIdActivated("peer", outcome.newSequence, outcome.connectionId) qlogObserver.onConnectionIdRetired("peer", outcome.retiredSequence) pathValidator.acknowledgeTerminal() - // Note: the writer's `destinationConnectionId` was - // already swapped to [outcome.connectionId] inside + // The writer's `destinationConnectionId` was already + // swapped at challenge time inside // [triggerPathMigrationLocked] (abrupt-migration - // model — the challenge itself MUST go out on the - // new path per RFC 9000 §9.3). On success there's - // nothing more to swap; we just confirm the - // promotion is durable by re-stamping (idempotent). - destinationConnectionId = ConnectionId(outcome.connectionId) + // model — RFC 9000 §9.3 requires the challenge on + // the new path). On success there is no further + // mutation to make. } } } 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 c5ebaf9fd..bc27225ab 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -91,6 +91,17 @@ fun drainOutbound( return datagram } + // Bug-A fix: drive the §8.2.4 3*PTO validation budget on every + // drain, not just on PTO expiration. The peer ACKs PATH_CHALLENGE + // (it's ack-eliciting), and ACK arrival resets consecutivePtoCount — + // so the PTO timer that previously hosted [checkPathValidationTimeoutLocked] + // can stop firing before the budget elapses, leaving validation + // hung indefinitely. Drain happens at least every PTO_BASE + // (~30-100ms) even when the application is idle, which covers the + // budget-elapsed condition reliably. Cheap (one type-cast + + // time comparison) when the validator is in [PathValidationState.Idle]. + conn.checkPathValidationTimeoutLocked(nowMillis) + // Drain destructive frame sources into local lists, ONCE. val initialContents = collectHandshakeLevelFrames(conn, EncryptionLevel.INITIAL, nowMillis) val handshakeContents = collectHandshakeLevelFrames(conn, EncryptionLevel.HANDSHAKE, nowMillis) diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt index d52dc6ac8..0b7c2fc05 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt @@ -163,14 +163,24 @@ class ClientPathMigrationTest { "DCID must rotate at challenge time so PATH_CHALLENGE goes out on the new path", ) - // Step 3 — the next outbound application packet must carry a - // PATH_CHALLENGE stamped with the NEW DCID. + // Step 3 — the next outbound application packet must carry + // BOTH the PATH_CHALLENGE (stamped with the NEW DCID) AND + // a RETIRE_CONNECTION_ID for the old sequence (Bug-B fix: + // abrupt migration retires the prior CID at trigger time, + // not at response time, so the writer drains both in the + // same packet). val challengeOut = drainOutbound(client, nowMillis = 100L) assertTrue(challengeOut != null, "client must emit a packet carrying PATH_CHALLENGE") val outboundFrames = pipe.decryptClientApplicationFrames(challengeOut) assertTrue(outboundFrames != null, "client outbound must decrypt with server keys") val challenge = outboundFrames.firstOrNull { it is PathChallengeFrame } as? PathChallengeFrame assertTrue(challenge != null, "outbound must contain PATH_CHALLENGE — got ${outboundFrames.map { it::class.simpleName }}") + val retire = outboundFrames.firstOrNull { it is RetireConnectionIdFrame } as? RetireConnectionIdFrame + assertTrue( + retire != null, + "outbound must contain RETIRE_CONNECTION_ID for the prior CID alongside PATH_CHALLENGE — got ${outboundFrames.map { it::class.simpleName }}", + ) + assertEquals(0L, retire.sequenceNumber) // Step 4 — server echoes the payload in PATH_RESPONSE. val response = @@ -179,26 +189,13 @@ class ClientPathMigrationTest { )!! feedDatagram(client, response, nowMillis = 200L) - // Step 5 — validation succeeded; DCID is still the new bytes - // (already rotated at step 2), the prior sequence (0) is - // queued for RETIRE_CONNECTION_ID, and activeCidSequence now - // reflects the new sequence. + // Step 5 — validation succeeded; state transitions to + // Idle (after acknowledgeTerminal), activeCidSequence is + // still the new sequence (already bumped at trigger), + // DCID still the new bytes. assertContentEquals(newCidBytes, client.destinationConnectionId.bytes) assertEquals(1L, client.pathValidator.activeCidSequence) - assertTrue( - client.pathValidator.pendingRetireSequences.contains(0L), - "old sequence number must be queued for retire", - ) - - // Step 6 — the next outbound packet should carry the - // RETIRE_CONNECTION_ID for the old sequence. - val retireOut = drainOutbound(client, nowMillis = 200L) - assertTrue(retireOut != null, "client must emit a packet after PATH_RESPONSE") - val frames = pipe.decryptClientApplicationFrames(retireOut) - assertTrue(frames != null, "outbound after rotation must decrypt") - val retire = frames.firstOrNull { it is RetireConnectionIdFrame } as? RetireConnectionIdFrame - assertTrue(retire != null, "outbound must contain RETIRE_CONNECTION_ID — got ${frames.map { it::class.simpleName }}") - assertEquals(0L, retire.sequenceNumber) + assertEquals(1L, client.pathValidator.successfulValidations) // Sanity: connection still alive after the rotation. assertEquals(QuicConnection.Status.CONNECTED, client.status) @@ -243,7 +240,11 @@ class ClientPathMigrationTest { assertContentEquals(newCid, client.destinationConnectionId.bytes, "mismatched response must not undo the rotation") assertTrue(client.pathValidator.state is PathValidationState.Validating, "still validating") - assertEquals(0L, client.pathValidator.activeCidSequence, "active sequence still old until validation succeeds") + assertEquals( + 1L, + client.pathValidator.activeCidSequence, + "active sequence tracks what the writer is putting on the wire — bumped at challenge time per Bug-B fix", + ) } @Test @@ -330,6 +331,59 @@ class ClientPathMigrationTest { assertEquals(0, client.consecutivePtoCount, "successful path validation must reset consecutivePtoCount") } + @Test + fun newConnectionIdWithRetirePriorToPastActiveForcesRotationOnSamePath() = + runBlocking { + // Bug-C regression: RFC 9000 §5.1.2 — a peer that + // advances retire_prior_to past our active CID forces + // us to rotate on the SAME path (no PATH_CHALLENGE). + // Pre-fix the parser silently accepted the offer and + // we'd keep stamping the now-retired seq=0 on outbound + // packets. + val (client, pipe) = newConnectedClient() + val originalDcid = client.destinationConnectionId + val replacementCid = ByteArray(8) { (0xB0 or it).toByte() } + val token = ByteArray(16) { 0x33 } + + // First offer: seq=1, no retirement. Pool fills. + feedDatagram( + client, + pipe.buildServerApplicationDatagram( + listOf(NewConnectionIdFrame(1L, 0L, replacementCid, token)), + )!!, + nowMillis = 0L, + ) + assertEquals(originalDcid, client.destinationConnectionId, "first offer doesn't force rotation") + + // Second offer: seq=2 with retire_prior_to=1. Watermark + // advances past our active seq=0. MUST rotate on the + // same path to seq=1 (the lowest spare). + val secondToken = ByteArray(16) { 0x44 } + feedDatagram( + client, + pipe.buildServerApplicationDatagram( + listOf(NewConnectionIdFrame(2L, 1L, ByteArray(8) { 0xCC.toByte() }, secondToken)), + )!!, + nowMillis = 0L, + ) + + assertContentEquals( + replacementCid, + client.destinationConnectionId.bytes, + "DCID must rotate to the lowest spare (seq=1) when watermark advances past seq=0", + ) + assertEquals(1L, client.pathValidator.activeCidSequence) + assertTrue( + client.pathValidator.pendingRetireSequences.contains(0L), + "old active seq=0 must be queued for RETIRE_CONNECTION_ID", + ) + assertEquals( + QuicConnection.Status.CONNECTED, + client.status, + "server-forced rotation must NOT close the connection", + ) + } + @Test fun triggerPathMigrationBeforeHandshakeReturnsNotConnected() = runBlocking { diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt index 0a5f81433..545fcf39e 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt @@ -173,6 +173,106 @@ class PathValidatorTest { assertEquals(1L, v.successfulValidations) } + @Test + fun triggerRetiresPriorSequenceImmediately() { + // Bug-B regression: under abrupt-migration semantics the + // prior CID is abandoned the moment we rotate (RFC 9000 + // §5.1.2). Queuing the retire only at PATH_RESPONSE success + // means a failed first attempt followed by a never-validated + // second attempt would leave seq=0 unretired forever. + val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x11 } }) + v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 }) + v.tryStartValidation(0L, 100L) + assertTrue( + v.pendingRetireSequences.contains(0L), + "prior sequence must be queued for retire at trigger time, not at response time", + ) + assertEquals(1L, v.activeCidSequence, "activeCidSequence advances at trigger to track the on-wire DCID") + } + + @Test + fun twoConsecutiveFailedValidationsRetireAllAbandonedSequences() { + // Bug-B regression: prior to the fix, two failed validations + // would leave the original seq=0 unretired (the priorSeq + // queue entry was scheduled inside applyPathResponse, which + // never runs for failed validations). + val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x22 } }) + v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 }) + v.recordPeerNewConnectionId(2L, 0L, ByteArray(8) { 0x02 }, ByteArray(16) { 0x20 }) + val pto = 1_000L + // First attempt fails by timeout. + v.tryStartValidation(0L, pto) + v.checkValidationTimeout(nowMillis = pto * 4L) + v.acknowledgeTerminal() + // Second attempt also fails. + v.tryStartValidation(nowMillis = 10_000L, currentPtoMillis = pto) + v.checkValidationTimeout(nowMillis = 10_000L + pto * 4L) + + // All three sequences abandoned along the way (the original + // seq=0 plus seq=1 plus seq=2) MUST be queued for retire. + for (seq in listOf(0L, 1L, 2L)) { + assertTrue( + v.pendingRetireSequences.contains(seq), + "seq=$seq abandoned but not queued for retire — pending=${v.pendingRetireSequences}", + ) + } + } + + @Test + fun forceRotateRunsWhenWatermarkPassesActiveCid() { + // Bug-C regression: RFC 9000 §5.1.2 — "If the active + // connection ID's sequence number is less than the Retire + // Prior To value, the endpoint MUST retire the active + // connection ID and adopt one with a higher sequence number." + val v = PathValidator() + v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 }) + v.recordPeerNewConnectionId(2L, 0L, ByteArray(8) { 0x02 }, ByteArray(16) { 0x20 }) + // Now the peer advances retire_prior_to past our active CID + // (seq 0), forcing same-path rotation. + v.recordPeerNewConnectionId(3L, retirePriorTo = 1L, ByteArray(8) { 0x03 }, ByteArray(16) { 0x30 }) + + val rotation = v.forceRotateToHigherSequence() + assertTrue(rotation is PathValidator.ForcedRotationResult.Rotated, "got $rotation") + assertEquals(1L, rotation.newSequence, "must pick the lowest spare sequence") + assertEquals(0L, rotation.retiredSequence) + assertContentEquals(ByteArray(8) { 0x01 }, rotation.connectionId) + assertEquals(1L, v.activeCidSequence) + assertTrue(v.pendingRetireSequences.contains(0L)) + } + + @Test + fun forceRotateNoOpWhenWatermarkBelowActive() { + val v = PathValidator() + v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 }) + // Watermark is still 0; active is also 0 (initial). Nothing + // to rotate. + assertEquals(null, v.forceRotateToHigherSequence()) + assertEquals(0L, v.activeCidSequence) + } + + @Test + fun forceRotateRotatesAgainWhenNewerOfferAdvancesWatermark() { + // Cascading server-forced rotation: peer issues seq=1 then + // (later) seq=2 with retire_prior_to=2, retiring seq=1 + // immediately after we'd just adopted it. + val v = PathValidator() + v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 }) + // Tester adopts seq=1 via initial-state simulation: trigger + // a validation just to bump activeCidSequence past 0. + v.tryStartValidation(0L, 100L) + v.applyPathResponse(ByteArray(8) { 0x00 }) // mismatched, validation stays Validating + v.acknowledgeTerminal() // benign no-op (state isn't terminal) + assertEquals(1L, v.activeCidSequence) + + // Peer now advances watermark to 2 with a fresh offer. + v.recordPeerNewConnectionId(2L, retirePriorTo = 2L, ByteArray(8) { 0x02 }, ByteArray(16) { 0x20 }) + val rotation = v.forceRotateToHigherSequence() + assertTrue(rotation is PathValidator.ForcedRotationResult.Rotated, "got $rotation") + assertEquals(2L, rotation.newSequence) + assertEquals(1L, rotation.retiredSequence) + assertEquals(2L, v.activeCidSequence) + } + @Test fun pathResponseWithMismatchedPayloadIsIgnored() { val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x77 } })