fix(server): require an executable open before acknowledging it #77

2026-08-12T12:29:17Z

Lochnair commented

2026-08-12 12:29:17 +00:00

(Migrated from github.com)

Establish the invariant that once an OPEN passes admission and negotiation, its effective parameters must be safe for the server to execute. PR #76 recorded the black-box evidence and deliberately left both policies below open; this chooses them.

Effective timestamps require a specified clock

Finding S-19 records upstream accepting an OPEN whose restricted StampAt stays non-none while the Clock tag is absent — a valid token is issued, no Clock is echoed, and the first ECHO terminates the server process. ServerCore must not reproduce that accepted-then-fatal state.

negotiated_params_are_admissible refuses it: stamp_at == StampAt::None || clock != Clock::Unspecified. The OPEN is discarded silently — no token, no session, no reply, no ServerError, consistent with the crate's "rejection is silence" rule.

No clock is synthesized and stamp_at is never rewritten to none; either would answer with a session the client did not ask for. The check runs after negotiate_params and before the no-test branch, so a later policy restricting StampAt to none will make an omitted clock safe without this rule changing, and so both open forms refuse the same incoherent request. Explicit Clock = 0 is untouched — the decoder still rejects it as out of range, which keeps the presence distinction the evidence establishes.

Clock::Unspecified alone stays valid: only the combination with a selected stamp_at is refused, so an empty parameter payload or an OPEN requesting no timestamps is unaffected, as is any conforming client, which always sends a clock when it selects timestamps.

Negative negotiated Length becomes executable rather than an encode error

PR #76 also establishes that a negative Length is accepted at OPEN and returned unchanged, so a live session can genuinely carry one. echo_packet_len rejecting it left such a session unrunnable.

Actual packet sizing is now max(header, max(length, 0)): a negative length asks for no space beyond the mandatory field block, exactly as zero does. The negotiated value is not clamped — it stays negative in the OPEN reply and verbatim in Session.params; only sizing floors it.

echo_packet_len keeps returning Result, but for a different reason than before. ProtoError::NegativePacketLength is gone — a negative length is no longer an error, and that name was nonsense for any other case. In its place, ProtoError::PacketLengthUnrepresentable covers a positive length wider than the platform's usize:

Length < 0                 -> structural floor
Length == 0                -> structural floor
0 < Length <= usize::MAX   -> max(header, Length)
Length  > usize::MAX       -> PacketLengthUnrepresentable

That is a representability check, not a size policy. A wire value that cannot even name a local buffer must not be converted into usize::MAX and handed to an allocator; on a 32-bit target that is the difference between an error and a Vec::with_capacity panic. It is unreachable on 64-bit, where every positive i64 converts, and irtt-proto is a library — OpenWrt-class 32-bit hosts are exactly the audience for a runtime-light client.

A ceiling on what a negotiated length may legitimately be — MTU, maximum packet size, allocation bounds — is deliberately not in this PR. That is server and runtime policy, and it stays with the deferred slice. Vec::with_capacity can still be handed an absurd-but-representable length on 64-bit; that is pre-existing and belongs to the same later work.

The encoders still return Result for their own reasons — payload, envelope, flag and HMAC failures are unchanged.

Scope

No ECHO core: DecodedRequestKind::Echo => Ok(None) is unchanged, and no receive state, timestamp acquisition, lifetime, DSCP, MTU or duration/interval policy is added. This slice only prepares valid session state for those.

The chosen policies are recorded in crates/irtt-server/AGENTS.md. The protocol specs, verification report and test vectors are deliberately untouched — they stay policy-neutral evidence.

Tests

proto: negative/zero/below-header/above-header sizing floors; ECHO request and ECHO reply both encode at the 16-byte field block under a negative negotiated length, and the request decodes back to its token and sequence.
server: all four non-none StampAt values with an absent Clock are refused with no session and no token consumed, for both ordinary and no-test OPENs; the empty-payload test now asserts StampAt::None + Clock::Unspecified stays valid; the negative-length test additionally asserts the stored session holds the negotiated values verbatim.

cargo fmt --check, check, test, and clippy -D warnings all pass across the workspace with --all-targets --all-features --locked, as does RUSTDOCFLAGS="-D warnings" cargo doc for both crates.

Establish the invariant that once an OPEN passes admission and negotiation, its effective parameters must be safe for the server to execute. PR #76 recorded the black-box evidence and deliberately left both policies below open; this chooses them. ## Effective timestamps require a specified clock Finding S-19 records upstream accepting an OPEN whose restricted `StampAt` stays non-none while the Clock tag is absent — a valid token is issued, no Clock is echoed, and the first ECHO terminates the server process. `ServerCore` must not reproduce that accepted-then-fatal state. `negotiated_params_are_admissible` refuses it: `stamp_at == StampAt::None || clock != Clock::Unspecified`. The OPEN is discarded silently — no token, no session, no reply, no `ServerError`, consistent with the crate's "rejection is silence" rule. No clock is synthesized and `stamp_at` is never rewritten to none; either would answer with a session the client did not ask for. The check runs **after** `negotiate_params` and **before** the no-test branch, so a later policy restricting `StampAt` to none will make an omitted clock safe without this rule changing, and so both open forms refuse the same incoherent request. Explicit `Clock = 0` is untouched — the decoder still rejects it as out of range, which keeps the presence distinction the evidence establishes. `Clock::Unspecified` alone stays valid: only the combination with a selected `stamp_at` is refused, so an empty parameter payload or an OPEN requesting no timestamps is unaffected, as is any conforming client, which always sends a clock when it selects timestamps. ## Negative negotiated Length becomes executable rather than an encode error PR #76 also establishes that a negative Length is accepted at OPEN and returned unchanged, so a live session can genuinely carry one. `echo_packet_len` rejecting it left such a session unrunnable. Actual packet sizing is now `max(header, max(length, 0))`: a negative length asks for no space beyond the mandatory field block, exactly as zero does. The negotiated *value* is not clamped — it stays negative in the OPEN reply and verbatim in `Session.params`; only sizing floors it. `echo_packet_len` keeps returning `Result`, but for a different reason than before. `ProtoError::NegativePacketLength` is gone — a negative length is no longer an error, and that name was nonsense for any other case. In its place, `ProtoError::PacketLengthUnrepresentable` covers a *positive* length wider than the platform's `usize`: ``` Length < 0 -> structural floor Length == 0 -> structural floor 0 < Length <= usize::MAX -> max(header, Length) Length > usize::MAX -> PacketLengthUnrepresentable ``` That is a representability check, not a size policy. A wire value that cannot even name a local buffer must not be converted into `usize::MAX` and handed to an allocator; on a 32-bit target that is the difference between an error and a `Vec::with_capacity` panic. It is unreachable on 64-bit, where every positive `i64` converts, and `irtt-proto` is a library — OpenWrt-class 32-bit hosts are exactly the audience for a runtime-light client. A ceiling on what a negotiated length may legitimately *be* — MTU, maximum packet size, allocation bounds — is deliberately **not** in this PR. That is server and runtime policy, and it stays with the deferred slice. `Vec::with_capacity` can still be handed an absurd-but-representable length on 64-bit; that is pre-existing and belongs to the same later work. The encoders still return `Result` for their own reasons — payload, envelope, flag and HMAC failures are unchanged. ## Scope No ECHO core: `DecodedRequestKind::Echo => Ok(None)` is unchanged, and no receive state, timestamp acquisition, lifetime, DSCP, MTU or duration/interval policy is added. This slice only prepares valid session state for those. The chosen policies are recorded in `crates/irtt-server/AGENTS.md`. The protocol specs, verification report and test vectors are deliberately untouched — they stay policy-neutral evidence. ## Tests - proto: negative/zero/below-header/above-header sizing floors; ECHO request and ECHO reply both encode at the 16-byte field block under a negative negotiated length, and the request decodes back to its token and sequence. - server: all four non-none `StampAt` values with an absent Clock are refused with no session and no token consumed, for both ordinary and no-test OPENs; the empty-payload test now asserts `StampAt::None` + `Clock::Unspecified` stays valid; the negative-length test additionally asserts the stored session holds the negotiated values verbatim. `cargo fmt --check`, `check`, `test`, and `clippy -D warnings` all pass across the workspace with `--all-targets --all-features --locked`, as does `RUSTDOCFLAGS="-D warnings" cargo doc` for both crates.

👍 1

chatgpt-codex-connector[bot] (Migrated from github.com) reviewed

2026-08-12 12:34:41 +00:00

chatgpt-codex-connector[bot] (Migrated from github.com) left a comment

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca2c3dcbd8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

### 💡 Codex Review Here are some automated review suggestions for this pull request. **Reviewed commit:** `ca2c3dcbd8` <details> <summary>ℹ️ About Codex in GitHub</summary> [Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). Reviews are triggered when you - Open a pull request for review - Mark a draft as ready - Comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". </details>

crates/irtt-proto/src/layout.rs

						
				@@ -111,12 +111,41 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {

				    PacketLayout::echo(hmac, params).header_len()

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-08-12 12:34:42 +00:00

Reject packet lengths that cannot fit in usize

On 32-bit targets, a positive wire length above u32::MAX now becomes usize::MAX; encode_request and encode_echo_reply pass that value to Vec::with_capacity, which panics with a capacity overflow instead of returning a protocol error as the previous checked conversion did. This can terminate a process encoding untrusted or otherwise oversized Params, so only negative values should be floored while positive values that do not fit the platform should remain fallible.

AGENTS.md reference: AGENTS.md:L76-L78

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Reject packet lengths that cannot fit in usize** On 32-bit targets, a positive wire length above `u32::MAX` now becomes `usize::MAX`; `encode_request` and `encode_echo_reply` pass that value to `Vec::with_capacity`, which panics with a capacity overflow instead of returning a protocol error as the previous checked conversion did. This can terminate a process encoding untrusted or otherwise oversized `Params`, so only negative values should be floored while positive values that do not fit the platform should remain fallible. AGENTS.md reference: [AGENTS.md:L76-L78](https://github.com/Lochnair/irtt-rs/blob/ca2c3dcbd80694663299c9b6c84f0b161aa5b657/AGENTS.md#L76-L78) Useful? React with 👍 / 👎.

Lochnair (Migrated from github.com) reviewed

2026-08-12 12:41:27 +00:00

crates/irtt-proto/src/layout.rs

						
				@@ -111,12 +111,41 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {

				    PacketLayout::echo(hmac, params).header_len()

Lochnair (Migrated from github.com) commented

2026-08-12 12:41:26 +00:00

Corrected the misleading comment in c833b06, but kept the saturating behavior — the premise that the old conversion guarded oversized lengths only holds on 32-bit. On 64-bit (every target in dist-workspace.toml) usize::try_from succeeds for all positive i64, so main already reached Vec::with_capacity with an absurd length and panicked identically; the old error was incidental to the negative check and even reported "must be non-negative" for a positive value. It is also not attacker-reachable: the client rejects a negotiated length that is negative or increased above its own request (negotiation.rs:178), ClientConfig::length is u32, and the server does not encode ECHO replies yet. Reintroducing Result to guard that would undo this PR's API cleanup for a case no shipped target and no untrusted input can reach — a real cap belongs with the deferred maximum-packet-length policy. What the comment claimed was false, though: there are no "encoder's own limits", so it now says the allocation is where it fails.

🤖 Addressed by Claude Code

Corrected the misleading comment in c833b06, but kept the saturating behavior — the premise that the old conversion guarded oversized lengths only holds on 32-bit. On 64-bit (every target in `dist-workspace.toml`) `usize::try_from` succeeds for all positive `i64`, so `main` already reached `Vec::with_capacity` with an absurd length and panicked identically; the old error was incidental to the negative check and even reported "must be non-negative" for a positive value. It is also not attacker-reachable: the client rejects a negotiated length that is negative or increased above its own request (`negotiation.rs:178`), `ClientConfig::length` is `u32`, and the server does not encode ECHO replies yet. Reintroducing `Result` to guard that would undo this PR's API cleanup for a case no shipped target and no untrusted input can reach — a real cap belongs with the deferred maximum-packet-length policy. What the comment claimed was false, though: there are no "encoder's own limits", so it now says the allocation is where it fails. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_

Lochnair (Migrated from github.com) reviewed

2026-08-12 13:05:47 +00:00

crates/irtt-proto/src/layout.rs

						
				@@ -111,12 +111,41 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {

				    PacketLayout::echo(hmac, params).header_len()

Lochnair (Migrated from github.com) commented

2026-08-12 13:05:47 +00:00

Reopened and fixed properly in 3845142 — my earlier reply argued the narrow facts correctly but drew the wrong conclusion, so this now does what you asked for.

echo_packet_len is fallible again, but negatives stay floored and the error is a new PacketLengthUnrepresentable { length: i64 } rather than a resurrected NegativePacketLength, whose name would have been nonsense here:

Length < 0                 -> structural floor
Length == 0                -> structural floor
0 < Length <= usize::MAX   -> max(header, Length)
Length  > usize::MAX       -> PacketLengthUnrepresentable

Representability only — no MTU, maximum packet size, or allocation bound; that stays with the deferred policy slice, as does the pre-existing 64-bit Vec::with_capacity hazard for absurd-but-representable lengths. recv_buffer_size and the client call sites go back to Result with it; propagating an error that negotiation makes unreachable beats baking a 64-bit assumption into shared protocol code.

Verified on an actual 32-bit target rather than by argument: cargo check -p irtt-proto --target i686-unknown-linux-gnu --all-targets passes, and a temporary deliberate type error inside the new #[cfg(target_pointer_width = "32")] test failed the 32-bit check while the 64-bit check stayed green — so the test really is compiled where the branch exists. Workspace fmt/check/test/clippy -D warnings and RUSTDOCFLAGS="-D warnings" cargo doc are all green. PR description updated, since it still claimed the API was infallible.

🤖 Addressed by Claude Code

Reopened and fixed properly in 3845142 — my earlier reply argued the narrow facts correctly but drew the wrong conclusion, so this now does what you asked for. `echo_packet_len` is fallible again, but negatives stay floored and the error is a new `PacketLengthUnrepresentable { length: i64 }` rather than a resurrected `NegativePacketLength`, whose name would have been nonsense here: ``` Length < 0 -> structural floor Length == 0 -> structural floor 0 < Length <= usize::MAX -> max(header, Length) Length > usize::MAX -> PacketLengthUnrepresentable ``` Representability only — no MTU, maximum packet size, or allocation bound; that stays with the deferred policy slice, as does the pre-existing 64-bit `Vec::with_capacity` hazard for absurd-but-representable lengths. `recv_buffer_size` and the client call sites go back to `Result` with it; propagating an error that negotiation makes unreachable beats baking a 64-bit assumption into shared protocol code. Verified on an actual 32-bit target rather than by argument: `cargo check -p irtt-proto --target i686-unknown-linux-gnu --all-targets` passes, and a temporary deliberate type error inside the new `#[cfg(target_pointer_width = "32")]` test failed the 32-bit check while the 64-bit check stayed green — so the test really is compiled where the branch exists. Workspace `fmt`/`check`/`test`/`clippy -D warnings` and `RUSTDOCFLAGS="-D warnings" cargo doc` are all green. PR description updated, since it still claimed the API was infallible. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Lochnair-Labs/irtt-rs#77

				`@@ -111,12 +111,41 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {`
				`PacketLayout::echo(hmac, params).header_len()`