kiloparsec/REVIEW-237d404.md

# Review of authenticated protocol 2

Reviewed commit: `237d404bcd4d52e015b5e0d7cd4feaec4bd848ff` — Add authenticated protocol 2 and TypeScript WASM client.

Review date: 2026-10-21. Baseline: parent commit `88ceeb2`.

Follow-up design: [Concealed protocol design record](CONCEALED-PROTOCOL-DESIGN.md), including agreed decisions D1–D14, corrections C1–C8, and open questions Q1–Q14. This is separate future work; the review issues below retain their existing status.

This document records findings for separate discussion and repair. Issue numbers are stable. Implementation changes and their validation are recorded under closed issues. Proposed remedies for open issues are discussion options, not an agreed redesign.

## Assessment

The reviewed commit fixed real authentication gaps, particularly unprotected RPC response correlation, but introduced availability regressions. The findings below preserve the original analysis. I1–I5 are now fixed for release 0.8.0; I6–I7 were closed as non-defects. No review issues remain open.

The nonce itself was already present in protocol 1. Receiver-side replay tracking, the fixed window, and counter-exhaustion checks are new. These changes should be evaluated separately from authenticated RPC metadata and the server-finished acknowledgement.

## Reason for the original change

The committed assessment says the TypeScript port exposed two protocol-1 weaknesses. Inspection of the parent code supports both mechanisms:

1. Encrypted results had inner `forId = 0`; their actual request correlation was only in the plaintext outer transport ID. An intermediary could swap two outer IDs and associate authentic answers with the wrong concurrent calls when result codecs were compatible. Protocol 2 authenticates kind, correlation and priority inside encryption.
2. `L0ClientId` returned plaintext `Unit`. An intermediary could forge that completion without the server accepting the identity message. Protocol 2 returns an encrypted server-finished record echoing the exact accepted identity bytes.

Other changes include separate client/server signature domains, fresh server exchange keys per connection, repeated-handshake rejection, nonce-state preservation during identity completion, and counter-exhaustion checks. These are distinct design decisions and need not be discussed as one indivisible feature.

## What server-finished means

It is the response to the existing `L0ClientId` RPC, not an additional network round trip or an acknowledgement of every UDP packet.

Previously: client sends encrypted identity; server replies with plaintext Unit. Now: client sends encrypted identity; server replies with encrypted `ProtectedMessage(SERVER_FINISHED, 0, 0, identityBytes)`.

The client decrypts it and checks the kind, zero correlation/priority, and exact identity payload. With an honest authenticated server, this establishes that the session-key holder processed and accepted that identity message before the client announces connection completion. It does not establish application authorization, authenticate an otherwise untrusted server key, or acknowledge subsequent RPC execution.

The server completes its local established state before sending this response. Losing the response can therefore leave the server established while the client is still waiting. This is an acknowledgement, not an atomic agreement or a delivery-recovery mechanism.

## Existing nonces versus new replay checks

Protocol 1 already contained `Package(nonce: ULong, encryptedMessage)`, `private var nonce = 0UL`, and `val n = protectedOp.invoke { nonce++ }`. It derived the cipher nonce from the session token and that counter. Each sending direction has its own encryption key/counter context.

The encryption nonce selects a unique encryption instance under a key. It is not secret, is not the RPC ID, and does not require packets to arrive in order. Reusing a nonce for distinct plaintexts under the same key is unsafe. Retransmitting an already-produced ciphertext is a different question from encrypting new plaintext with a reused nonce.

Protocol 1 decrypted using the counter supplied with each packet, without remembering which counters it had already seen. Consequently, a captured valid encrypted call could be replayed and reach application execution again.

Protocol 2 adds receiver memory: highest authenticated nonce plus the set of seen nonces in a 4,096-position window. This recognizes repeats and bounds memory, at the cost of rejecting sufficiently old unseen packets. Only successfully authenticated packets advance the window; a forged large counter cannot move it forward.

The tasks are distinct: nonce uniqueness protects encryption; replay tracking prevents repeated acceptance; authenticated RPC correlation binds an answer to its request. Neither correlation nor server-finished intrinsically requires this particular replay-window size or session-closing policy.

## Repair validation (2026-10-21)

- 42 focused JVM tests passed, including existing security, priority and transport tests and the new replay/queue/admission regressions.
- TypeScript: 38 passed; the normally skipped interop fixture was run separately and passed against this checkout's JVM server.
- Release validation: all 64 JVM and 47 Kotlin/JS Node tests passed. Real loopback UDP exercised reordered and duplicated packets and concurrent bidirectional RPCs; real TCP exercised concurrent bidirectional RPCs and priority. The new carrier test cancels its collector before closing the listener.
- All 13 Maven publications and Dokka built successfully. A standalone consumer resolved staged 0.8.0 artifacts without source substitution and passed the TypeScript interoperability suite (authentication, duplex, pushes, errors, concurrent calls and reconnect).
- Kotlin Wasm compilation passed. Browser, Android and Apple runtime qualification was not performed. Apple KLIB publication does not qualify final Apple application linking or runtime.
- Protocol-2 wire encoding remains unchanged; older protocol-2 peers still retain their own previous replay-disconnect behavior until upgraded.
- The encrypted backlog test validates all 4098 messages, including an old low-priority message, decrypt successfully after scheduling. Concurrent real JVM preparation exercises both directions and calls, replies, pushes and handshake messages.
- A temporary preparation benchmark (source retained at `/tmp/PreparationBenchmarkTest.kt`) ran concurrent 1 MiB and 1 KiB payloads under the preparation mutex. Measured medians including lock waiting were 86.86 ms and 86.60 ms, with maxima 117.40 ms and 117.65 ms. This is a local JVM observation including serialization and allocation; large-message preparation can delay small messages before queue scheduling. The benchmark source was removed from the checkout.
- Historical probe/test counts below describe the original review, not the final repair run.

## Original review validation and limits

- 14 focused JVM tests passed: 7 existing authenticated-envelope tests, 3 security-boundary tests, and 4 temporary review probes.
- The review probes checked replay-window boundaries, adjacent reordering survival, deep reordering session closure, and local send failure session closure.
- Follow-up I5 check: 3 additional focused JVM probes passed, reproducing ignored client/server initialization rejection and confirming rejection through the application `execute` gate. Probe source saved at `/tmp/ReviewI5Test.kt` and removed from the checkout.
- TypeScript tests: 35 passed, 1 interoperability test skipped.
- No live UDP impairment, browser qualification, full cryptographic audit, or sustained-load benchmark was performed.
- Temporary probe source was saved at `/tmp/Review237d404Test.kt` and removed from the checkout. This path is ephemeral.
- Existing `.idea/misc.xml` changes and untracked `kotlin-js-store/` were left untouched.

All review issues are closed: I1–I5 are included in release 0.8.0; I6–I7 require no remediation.

## Closed issues

### Issue 1 — Duplicate or stale packets terminate the session

**Resolution (2026-10-21):** Done — duplicate/stale encrypted packets are silently discarded on Kotlin and TypeScript. Incoming calls/pushes are not executed again; response authentication/replay checks run before consuming the pending waiter. A valid later reply can still complete it. Authentication/routing failures remain fatal. No automatic retry was added.

**Original finding (before repair):**

**Priority:** High for UDP. **Status:** Confirmed regression; reproduced.

`KiloParams.decrypt` rejects a previously received nonce, or an unseen nonce at least 4,096 positions behind the highest authenticated nonce. `KiloL0Interface.executeTransportBlock` turns rejection into `SecurityException`, a subclass of `BreakConnectionException`, and the transport closes.

Observed behavior:

| Delivery | Result |
| --- | --- |
| Adjacent packets reversed | Both accepted |
| Unseen nonce 4,095 positions behind highest | Accepted |
| Unseen nonce 4,096 positions behind highest | Rejected |
| Duplicate incoming call or push | Rejected; session closes |

Duplicate responses are different: the outer transport normally ignores responses whose pending request was already removed. A stale response for a still-pending call reaches authentication and can close the session.

A controlled test held an encrypted push, delivered 4,096 newer pushes, then delivered the original. The valid original packet caused session closure. An adjacent-reordering test survived. The existing duplicate-call regression also explicitly expects connection termination.

**Impact:** ordinary datagram duplication or sufficiently deep reordering becomes a session-wide failure, affecting unrelated calls. This is an availability issue, not evidence of broken encryption.

**Discussion direction:** distinguish duplicate/stale packet disposition from fatal errors; discard without repeating application execution. If reliable delivery is desired, separately specify timeouts, retransmission and response caching. Discarding duplicates alone does not implement reliable RPC.

References: [KiloParams.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloParams.kt), [KiloL0Interface.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloL0Interface.kt), [Transport.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/Transport.kt), [AuthenticatedEnvelopeTest.kt](src/commonTest/kotlin/AuthenticatedEnvelopeTest.kt).

### Issue 2 — Priority scheduling can exceed the replay window locally

**Resolution (2026-10-21):** Done — `OutQueue` enforces the 2730-position allowance; one preparation mutex covers nonce allocation, sealing, encoding and admission. The transport owns the nonce-aware writer and submits serially to the existing adapter, preserving its selected order. Encryption stays in secure-layer callbacks. Full queues reject immediately rather than wait under the mutex.

**Original finding (before repair):**

**Priority:** High under sustained priority traffic. **Status:** Code-established interaction; actual scheduler saturation not reproduced.

Encryption allocates a nonce before `PriorityBlockWriter` selects transmission order. A low-priority encrypted packet can remain queued while newer high-priority packets overtake it. There is no fairness or maximum-overtaking guarantee. The 128-item queue bound limits simultaneous occupancy, not the total number of packets that can overtake one waiting packet.

If overtaking spans 4,096 nonce positions, eventual delivery triggers issue 1. This can occur even with ordered TCP/WebSocket delivery: the reordering happens before the carrier sends the bytes. Calls, replies and pushes share one nonce sequence per sending direction.

**Agreed design (2026-10-21):** introduce a specific `OutQueue` with `push(message, nonce, priority)` and `pop()`. Preserve encryption in its existing layer. Pass nonce metadata explicitly to the queue rather than decoding encrypted-package structures in the scheduler.

For receiver window `W = 4096`, use a local overtaking allowance `B = floor(2 * W / 3) = 2730`:

```text
pop():
    oldest = minimum queued nonce
    eligible = messages with nonce - oldest <= B
    select highest priority among eligible
    break priority ties by smallest nonce
```

Use subtraction after establishing the minimum to avoid addition overflow. The allowance is based on nonce distance, not the number of successful sends. This accommodates gaps from failed or cancelled messages and prevents starvation by indefinitely newer high-priority traffic.

One mutex per sending direction covers nonce allocation, encryption/encoding, and queue insertion as one ordered preparation/admission operation. Queue insertion and selection must themselves be synchronized. This prevents an older allocated nonce from remaining outside the queue while newer packets are transmitted. All encrypted message paths sharing that sending key must participate, including calls, replies, pushes, and handshake ciphertexts.

Wait for transmission completion outside the mutex. Queue-capacity waiting must not prevent the writer from draining the queue; coordinate capacity reservation/admission accordingly. Failed preparation may consume a nonce, which must never be reused. Cancellation and close must release reservations and completion waiters.

The queue and writer must preserve the selected transmission order. This bounds scheduler-induced reordering while leaving roughly one third of the receiver window for subsequent network/receive-processing reordering. It does not guarantee acceptance after arbitrarily deep external reordering; issue 1 remains a separate fix.

**Validation targets:** priority selection inside the permitted range; oldest-message progress under sustained higher-priority traffic; exact window boundary; nonce gaps; concurrent preparation/admission; cancellation, capacity and close behavior; and an integration test showing a delayed low-priority encrypted message arrives without becoming stale. Measure preparation-lock contention with large messages rather than assuming encryption cost is negligible.

**Implementation status:** implemented and verified. A 4098-message encrypted backlog, concurrent duplex admission, queue capacity/cancellation/close and existing saturated-link responsiveness tests pass. Network reordering beyond the receiver window remains discardable, without closing the session.

References: [KiloParams.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloParams.kt), [PriorityBlockWriter.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/adapter/PriorityBlockWriter.kt).

### Issue 3 — Local send failures are misclassified as authentication failures

**Resolution (2026-10-21):** Done — plaintext remote errors are explicitly distinguished at the receive boundary. Local preparation/admission/write failures retain their original errors and do not abort the healthy session. A local reply-send failure is logged without fabricating a plaintext remote error; the caller retains its timeout/cancellation policy.

**Original finding (before repair):**

**Priority:** Medium. **Status:** Confirmed regression; reproduced.

`KiloRemoteInterface.call` catches almost every exception from `transport.callWithId`, closes the transport, and throws `SecurityException("unauthenticated outer error")`. That operation includes local encryption and sending, not only receipt of a plaintext remote error.

The priority writer throws `RemoteInterface.Exception("Outgoing block queue is full")` on admission failure. Injecting that same local send exception reproduced an authentication error and termination of both peers.

**Impact:** local congestion closes healthy sessions and unrelated pending calls; diagnostics incorrectly suggest a remote security failure.

**Discussion direction:** explicitly distinguish plaintext remote errors at the receive boundary from local encoding, admission and send failures.

References: [KiloRemoteInterface.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloRemoteInterface.kt), [PriorityBlockWriter.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/adapter/PriorityBlockWriter.kt).

### Issue 4 — Replay tracking adds a full-window scan per advancing packet

**Resolution (2026-10-21):** Done — Kotlin and TypeScript use 4096-bit circular replay windows. Normal in-order advancement clears one bit, large jumps clear a fixed bitmap, and only successfully authenticated packets update history.

**Original finding (before repair):**

**Priority:** Medium performance concern. **Status:** Confirmed algorithmic cost; throughput impact not benchmarked.

Kotlin uses `received.removeAll` when the high watermark advances; TypeScript scans its received set. In steady in-order traffic with a populated window, this is approximately 4,096 age checks per packet. Kotlin performs this under the lock also used for nonce allocation and decryption.

**Impact:** bounded memory, but avoidable CPU work and contention for a high-throughput transport.

**Discussion direction:** if retaining windowed replay protection, use a circular bitmap or another bounded representation with inexpensive advancement.

References: [KiloParams.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloParams.kt), [client.ts](typescript/src/client.ts). Algorithm background: [RFC 6479](https://www.rfc-editor.org/rfc/rfc6479.html).

### Issue 5 — Connection-init policy rejection is swallowed

**Status:** Done on 2026-10-21 — initialization rejection now propagates on both ends.

**Design decision:** preserve Kotlin's code-based remote-party authorization through the local interface and initialization handlers. Both peers receive verified identity and session data through `KiloScope`. No trusted-key-list constructor parameter is required; the earlier proposal to add one is withdrawn.

**Original defect:** the shared `invokeAll` helper discarded every handler exception using `runCatching`, including deliberate security/connection rejection. This defect predates commit `237d404`. Follow-up probes reproduced successful connection and application execution despite rejection on either peer.

**Fix:** replace `runCatching` with explicit `try/catch`. Rethrow `LocalInterface.BreakConnectionException` and all subclasses, including `RemoteInterface.SecurityException`; continue ignoring other handler failures as before. Rejection stops subsequent handlers. The shared helper also propagates this exception family when invoked for disconnect handlers.

**Validation:** `ConnectionInitializationTest` covers client and server initialization rejection using a custom `SecurityException` subclass and a custom `BreakConnectionException` subclass. Each case verifies that the policy sees the verified remote identity, both connection runs finish, later init handlers do not run, connected state is not announced, and a subsequent client RPC fails as closed. A fifth test verifies that ordinary handler failures still allow initialization and a successful RPC. The four rejection cases failed before the fix and passed after it. All 15 focused JVM tests passed, including `AuthenticatedEnvelopeTest` and `SecurityBoundaryTest`.

**Scope:** this completes the requested exception-propagation repair. Parameter publication order and initialization-time RPC semantics were not changed; these tests do not establish a general concurrent-dispatch barrier before initialization completes.

References: [KiloClientConnection.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloClientConnection.kt), [ConnectionInitializationTest.kt](src/commonTest/kotlin/ConnectionInitializationTest.kt).

### Issue 6 — Protocol 2 requires coordinated peer upgrades

**Status:** Closed — intentional compatibility decision, not a defect; no remediation required.

Both peers require protocol 2, with no automatic v1 fallback. The new envelope and identity acknowledgement deliberately break wire compatibility with v1. Coordinated peer upgrades were an agreed design decision, not an outstanding issue.

The original issue number is retained for reference.

References: [KiloProtocol.kt](src/commonMain/kotlin/net/sergeych/kiloparsec/KiloProtocol.kt), [SECURITY.md](typescript/docs/SECURITY.md).

### Issue 7 — Replay protection is not business-operation deduplication

**Status:** Closed on 2026-10-21 — not a protocol defect; excluded from remediation.

Kiloparsec protects against replayed encrypted packets and forged or altered protocol messages. A new valid RPC issued by application code, on the same connection or after reconnecting, is a new request. The protocol cannot determine whether repeating that operation is intentional or an application mistake.

Business-operation deduplication requires application-defined semantics, such as a durable operation identifier and an explicit idempotency contract. It belongs to business logic and should not have been classified as a Kiloparsec issue.

**Resolution:** no protocol change required. The original issue number is retained for reference.