Repository navigation
feat(rdpeudp): add the RDP-UDP2 data transfer PDUs and packet framing - #1679
Conversation
github-actions
Bot
added
needs-review
|
This pull request is Please split it into focused pull requests that can each be reviewed on their own. When the parts build on each other, stacked pull requests let you open each one on top of the last without waiting for the one below to merge. Stacks require every branch to live in this repository, so from a fork, please open separate pull requests instead. Automated review resumes once the change is below the |
|
Marc-André Moreau (@mamoreau-devolutions) Could you kickoff an initial review of this please? Thanks! |
There was a problem hiding this comment.
Pull request overview
Adds MS-RDPEUDP2 data-transfer PDUs and packet framing to the RDPEUDP crate.
Changes:
- Implements v2 headers, flags, ACK, control, and data payloads.
- Adds composite packet encoding/decoding and prefix-byte framing.
- Adds extensive protocol and round-trip tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
crates/ironrdp-rdpeudp/src/pdu/mod.rs |
Exposes v2 types and implements composite packets. |
crates/ironrdp-rdpeudp/src/pdu/prefix.rs |
Implements prefix framing and byte swapping. |
crates/ironrdp-rdpeudp/src/pdu/v2_ack.rs |
Implements ACK and acknowledgment vectors. |
crates/ironrdp-rdpeudp/src/pdu/v2_control.rs |
Implements control payloads. |
crates/ironrdp-rdpeudp/src/pdu/v2_data.rs |
Implements data header and body payloads. |
crates/ironrdp-rdpeudp/src/pdu/v2_flags.rs |
Defines v2 packet flags. |
crates/ironrdp-rdpeudp/src/pdu/v2_header.rs |
Implements the packed v2 header. |
crates/ironrdp-testsuite-core/tests/rdpeudp/mod.rs |
Registers the new test modules. |
crates/ironrdp-testsuite-core/tests/rdpeudp/pdu_prefix.rs |
Tests prefix framing. |
crates/ironrdp-testsuite-core/tests/rdpeudp/pdu_v2_ack.rs |
Tests acknowledgment payloads. |
crates/ironrdp-testsuite-core/tests/rdpeudp/pdu_v2_control.rs |
Tests control payloads. |
crates/ironrdp-testsuite-core/tests/rdpeudp/pdu_v2_data.rs |
Tests data payloads. |
crates/ironrdp-testsuite-core/tests/rdpeudp/pdu_v2_packet.rs |
Tests composite v2 packets. |
Suppressed comments (1)
crates/ironrdp-rdpeudp/src/pdu/v2_ack.rs:234
- This constant's documentation repeats the timestamp block in the wrong order; encoding and decoding use
TimeStamp(3)followed bySendAckTimeGapInMs(1).
/// Optional timestamp block: SendAckTimeGapInMs(1) + TimeStamp(3) = 4 bytes.
Adds the structures [MS-RDPEUDP2] section 2.2.1 defines for data transfer, which is where a connection goes once the handshake settles on protocol version 3: the packet header and its flags, the ACK and acknowledgment vector payloads, the OverheadSize, DelayAckInfo and AckOfAcks control payloads, the DataHeader and DataBody, the composite packet, and the PacketPrefixByte framing from 2.2.1.3. This document takes the opposite conventions to MS-RDPEUDP in the previous commit. It is little-endian, and 2.2 numbers the bits in its packet diagrams least significant first. Reading a v2 diagram with v1 habits gives a plausible and wrong layout for several fields, so the following were settled against worked examples rather than diagrams: The packet prefix byte's Packet_Type_Index occupies bits 1 to 4 and Short_Packet_Length bits 5 to 7. The example in 3.1.1.1.5.1 has PacketPrefixByte 0x10 for a 10-byte packet, which is Packet_Type_Index 8 only under least-significant-first numbering. In the ACK payload, numDelayedAcks is the low nibble and delayAckTimeScale the high one, per the bit ranges in 2.2.1.2.1. In the acknowledgment vector, TimeStamp precedes SendAckTimeGapInMs. In DelayAckInfo, MaxDelayedAcks is a whole byte rather than a nibble. The header carries exactly the six flags 2.2.1.1 lists, all of them announcing a payload. There is deliberately no CN, CWR or DUMMY flag here: the first two belong to MS-RDPEUDP's own header, and a dummy packet is marked by Packet_Type_Index 8 in the prefix byte, one layer down.
da49109 to
2fec3ce
Compare
|
Addressed all 9 review findings. Three had real fixes: V2Header::encode now rejects an out-of-range log_window_size and an ACK/ACKVEC combination instead of writing them silently, AckVectorPayload::encode now rejects out-of-range StateMap/RunLength values instead of masking them, and it no longer silently drops a timestamp/send_ack_time_gap_ms pair when only one is set. Five were documentation corrections (nibble direction, timestamp/gap byte order in two spots, MaxDelayedAcks field width, the PacketPrefixByte bit layout, and a self-contradictory line about which header fields survive encode). One strengthened an existing test with an exact-byte assertion. Replied individually on each thread. |
github-actions
Bot
added
ai-reviewed/1
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, well-tested addition of MS-RDPEUDP2 v2 data-transfer PDUs and PacketPrefixByte framing, building on the already-merged v1 crates. The review thread already caught and fixed nine bit-layout/masking bugs using worked examples. But re-deriving the cited 3.1.1.1.5.1 worked example against the actual public API shows compute_short_length still diverges from that same example for dummy packets >=7 bytes: the example's Short_Packet_Length is 0 for a 10-byte dummy packet, but dummy() computes 7 (same as normal packets), producing prefix byte 0xF0 instead of 0x10. No test exercises the real production functions against this example, so the gap is silent. A secondary, minor finding notes two defensive length checks in the same file that look unreachable given the surrounding invariants.
0eeed99
into
Devolutions:master
|
This merged before my second round of comments (on the dummy-packet Short_Packet_Length question and the two dead-code checks) made it into a push, so those two code comments aren't actually in what landed, only the thread replies explaining the reasoning are. The reasoning itself stands either way: dummy() is spec-correct as written, and the two bounds checks are intentionally defensive. Not planning a follow-up PR for comment-only additions this small. |
Fourth of six. Filed against master directly; #1626, #1627 and #1679 have all merged. Adds the sans-I/O `RdpeudpConnection` and the machinery it drives: send and receive windows, loss detection, NewReno congestion control, an RFC 6298 RTT estimator, the timer table, the reliability controller that matches retransmissions to the packets they replace, and sequence-number reconstruction from 16-bit wire values. This is the largest PR in the stack. I looked at splitting the state machine from the reliability primitives, but `connection.rs` drives all of them and the tests span both, so the seam would have been artificial. ## Sans-I/O No clock, no socket. Time arrives as a `MonotonicInstant` argument and outgoing packets leave as `Transmit` values. That keeps it testable without a network, and avoids `std::time::Instant`, whose `now` panics on `wasm32-unknown-unknown`. **`MonotonicInstant` is defined here, and #1530 adds one to `ironrdp-connector` for the same reason.** Two copies of one abstraction is a wart. `ironrdp-core` looks like the right home to me, and I raised it on #140; happy to move it wherever you prefer, in this PR or a follow-up. ## Behaviour that is required rather than chosen **Version 3 in the SYN.** That is the version [MS-RDPEUDP] 1.3.2.2 and the 2.2.2.9 table tie to the MS-RDPEUDP2 data transfer. Version 2 selects the MS-RDPEUDP one, which this crate does not implement. Version 3 requires the SHA-256 of the `securityCookie` in the client's SYN (2.2.2.9), so `ConnectionConfig` carries it, `connect` refuses without it, and the server performs the check 3.1.5.1.1 asks for. **The handshake retransmits.** 1.3.1 delivers the SYN, SYN+ACK and ACK by persistent retransmits whatever mode the transport runs in; 3.1.5.4.1 gives up after between three and five unanswered tries. A repeated datagram from the peer draws a repeat of ours rather than an error, including a SYN+ACK arriving after we have moved on to v2, which is how a client learns its final ACK was lost. **The delayed-ACK timer tracks the RTT instead of a fixed duration.** MS-RDPEUDP2 3.1.5.2 gives half the round trip time as the receiver's default. The handshake round trip seeds the existing RFC 6298 estimator (skipped when the handshake datagram was retransmitted, per Karn's algorithm), and the computed timeout is clamped to the 50-200ms band [MS-RDPEUDP] 3.1.6.3 gives a version-2 connection, since MS-RDPEUDP2 itself states no floor or cap of its own. **`UdpVersion` widens from a closed enum to a newtype carrying the raw wire value.** This is a breaking change to the public API #1627 shipped. MS-RDPEUDP 1.7 and 3.1.5.1.3 require a responder to negotiate down to a version it supports when the peer advertises one it does not recognize; hard-failing decode on an unrecognized value made that MUST clause unsatisfiable. Every call site already used the named constants, so this is the only consequential part of the change. **`error.rs` and its `Cargo.toml`/README wiring are back.** #1627 correctly dropped them at its own narrower, PDU-only scope; this PR's state machine is what actually needs them. **The receive window advances on both events in 3.1.1.2.2**, not only on AckOfAcks. The first one is what fires on a connection that is losing nothing; without it the window fills one window in and stops accepting. **AckOfAcks carries our own lowest unacknowledged sequence number** (2.2.1.2.4, 3.1.5.3). It is the only thing that can move a receiver past a packet the sender gave up on, since the retransmission carries a fresh `DataSeqNum` and the original is never filled. **Writes are split to the MTU.** MS-RDPEUDP2 does not segment: 3.1.1.2.4.2 forwards each packet's payload straight up and nothing marks a first or last fragment. `ChannelSeqNum` looks like it would serve but 3.1.5.5 gives it a different job, matching a retransmission to the packet it replaces. So anything longer than one packet is split before it becomes packets. **Dummy packets** are accounted for by the transport and their contents dropped, per 3.1.1.1.5. ## Test plan `cargo xtask check fmt/lints/tests/typos/locks` 179 rdpeudp tests in `ironrdp-testsuite-core` with this PR applied, plus 179 inline unit tests across the crate's components. The connection tests cover a clean transfer longer than the window, a loss in the middle of one, handshake retransmission to the give-up limit, the version and cookie negotiation paths, decoding an unrecognized `uUdpVer`, the ack-delay timeout's RTT-tracking and clamping behaviour, and the review round's findings: an out-of-range ACK no longer discards outstanding data, the ACK vector respects its 127-entry wire limit, `log_window_size` is validated, the ACK builders encode real timestamps and gaps, the handshake's Karn's-algorithm check is ordered correctly, the retransmit timer restarts on real progress, `accept` completes the negotiate-down behaviour, and `send` enforces a buffer bound.
feat(rdpeudp): add the RDP-UDP2 data transfer PDUs and packet framing
Third of six. Builds on the RDPEMT tunnel crate (#1626) and the v1 handshake
PDUs (#1627), both merged, adding files to the crate the second one creates.
Adds the structures [MS-RDPEUDP2] section 2.2.1 defines for data transfer, which
is where a connection goes once the handshake settles on protocol version 3: the
packet header and flags, the ACK and acknowledgment vector payloads, the
OverheadSize, DelayAckInfo and AckOfAcks control payloads, DataHeader and
DataBody, the composite packet, and the PacketPrefixByte framing from 2.2.1.3.
Why this is a separate PR from the v1 PDUs
The two documents take opposite conventions. MS-RDPEUDP is big-endian and
numbers diagram bits most significant first; MS-RDPEUDP2 is little-endian and
numbers them least significant first. Reading a v2 diagram with v1 habits
produces a plausible and wrong layout for several fields, so I would rather each
be reviewed against one document at a time.
Four fields where that difference bites, all settled against worked examples
rather than against the diagrams:
The prefix byte.
Packet_Type_Indexoccupies bits 1 to 4 andShort_Packet_Lengthbits 5 to 7. The worked example in 3.1.1.1.5.1 givesPacketPrefixByte = 0x10for a 10-byte packet, which isPacket_Type_Index = 8(dummy) only under least-significant-first numbering. Read the other way round
the same byte says something else entirely.
The ACK payload nibbles. 2.2.1.2.1 puts
numDelayedAcksat bits 48-51 anddelayAckTimeScaleat 52-55, so the count is the low nibble.The acknowledgment vector field order. 2.2.1.2.6 places
TimeStamp(bits24-47) before
SendAckTimeGapInMs(48-55).DelayAckInfo.
MaxDelayedAcksis a whole byte, not a nibble.On the flag set
The header carries exactly the six flags 2.2.1.1 lists, every one of them
announcing a payload. There is deliberately no
CN,CWRorDUMMYhere: thefirst two belong to MS-RDPEUDP's own
RDPUDP_FEC_HEADER, and a dummy packet ismarked by
Packet_Type_Index8 in the prefix byte, one layer down. With nostandalone flags the field is derived entirely from which payloads are present,
so a caller cannot set a bit that describes nothing.
What is not here
No connection state machine. That is the next PR.
Test plan
cargo xtask check fmt/lints/tests/typos/locks122 rdpeudp tests in
ironrdp-testsuite-corewith this PR applied, plus 44inline
#[cfg(test)]tests in the crate itself covering packet-prefix framingand header/flags internals, including the 3.1.1.1.5.1 worked example.