feat(rdpeudp): add the RDP-UDP crate and the v1 handshake PDUs - #1627
feat(rdpeudp): add the RDP-UDP crate and the v1 handshake PDUs#1627Greg Lamberson (glamberson) wants to merge 1 commit into
Conversation
Starts `ironrdp-rdpeudp` with the structures [MS-RDPEUDP] section 2.2 defines for the three-way handshake that opens an RDP-UDP connection: the FEC header and its flags, the SYN data and extended SYN data payloads that negotiate sequence numbers, MTU and protocol version, the ACK vector with its run-length encoding, the AckOfAcks header and the correlation ID payload, plus the composite datagram that assembles them in the order 2.2.2 requires. Everything here is big-endian. Section 2.2 says "all of the messages written to the network or read from the network MUST be in network byte order", and the field diagrams number bits most significant first. The MS-RDPEUDP2 data transfer that a version 3 handshake leads to takes the opposite convention on both counts, which is worth keeping in mind when reading the two side by side. Two places where the spec is easy to read the wrong way, both decided against the captures in section 4 rather than against the prose: The ACK flag does not always announce an ACK vector. Section 2.2.2.1 defines it that way, but 3.1.5.1.3 builds the SYN+ACK as a plain SYN with the flag set and snSourceAck filled in, and the capture in 4.1.2 shows the SYNDATA payload following the header directly. So on a SYN the flag says only that snSourceAck is meaningful, and encode refuses a SYN that carries a vector rather than writing bytes a peer will misread. The ACK vector is padded to a DWORD boundary and its elements pack the state into the top two bits, both confirmed by the ACK packet capture in 4.2.3. No connection state machine yet; that follows, as does the v2 data transfer format.
github-actions
Bot
added
maintainer-required
|
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 |
# chore(xtask): allow the rdpeudp and rdpemt title scopes Two entries in `CANONICAL_SCOPES`, following #1615. The RDP-UDP transport @mamoreau-devolutions asked for on #140 arrives as two crates, `ironrdp-rdpeudp` and `ironrdp-rdpemt`. Neither name is in the list, so `cargo xtask pr check-message` rejects the titles of the PRs that add them: #1626 and #1627 are both red on `Check message` for exactly this, with everything else green. Placed next to `rdpeusb`, which keeps the protocol extension crates together. ## No scope for the tokio adapter The transport also comes with `ironrdp-rdpeudp-tokio`, and I have deliberately not added a scope for it. The list names subsystems rather than crates, which is why there is no `tokio`, `async`, `blocking` or `-native` entry either, so changes to that crate belong under `rdpeudp`. Say the word if you would rather have one per crate and I will add it. ## Test plan `cargo xtask check fmt/lints/tests/typos/locks` Also checked directly against the titles this unblocks, by feeding each to `cargo xtask pr check-message` with a synthetic event file: the two filed PRs and the three still to come all validate, as does this PR's own title.
Marc-André Moreau (mamoreau-devolutions)
left a comment
Marc-André Moreau (mamoreau-devolutions)
left a comment
There was a problem hiding this comment.
Please address the inline protocol, build, test, and documentation defects before merging.
Note
LLM-assisted content (no human feedback).
| #[test] | ||
| fn encode_ack_vector_with_elements() { | ||
| let header = V1AckVectorHeader { | ||
| elements: vec![ |
There was a problem hiding this comment.
cargo test -p ironrdp-rdpeudp cannot compile this test module because vec! is not in scope in this no_std crate. Import alloc::vec alongside Vec so the crate's unit tests can run.
Note
LLM-assisted content (no human feedback).
| // Per spec: MUST be present in client→server SYN with v3, MUST NOT be present otherwise. | ||
| // We detect its presence by checking for remaining bytes because the caller may not | ||
| // know the role (client vs server) at the PDU layer. | ||
| let cookie_hash = if udp_ver == UdpVersion::V3 && src.len() >= Self::COOKIE_HASH_SIZE { |
There was a problem hiding this comment.
This cannot use remaining length to decide whether cookieHash is present. SYN and SYN+ACK datagrams are padded to the negotiated MTU, so a v3 server SYN+ACK will decode 32 padding bytes as a hash even though MS-RDPEUDP forbids cookieHash in that direction. Thread endpoint direction into decoding, or parse the hash only in a caller that knows it is handling a client SYN.
Note
LLM-assisted content (no human feedback).
| fn encode(&self, dst: &mut WriteCursor<'_>) -> EncodeResult<()> { | ||
| ironrdp_core::ensure_fixed_part_size!(in: dst); | ||
|
|
||
| dst.write_u32_be(self.initial_sequence_number); |
There was a problem hiding this comment.
Encoding writes arbitrary public MTU values, while decoding rejects values outside 1132..=1232. Validate both MTUs before writing so callers cannot emit a non-conformant SYN/SYN+ACK that this crate itself would reject on decode.
Note
LLM-assisted content (no human feedback).
|
|
||
| // Manually set DATA flag in the wire bytes | ||
| // Flags are at offset 6..8 in FecHeader (after snSourceAck(4) + windowSize(2)) | ||
| let flags_raw = u16::from_le_bytes([encoded[6], encoded[7]]); |
There was a problem hiding this comment.
FecHeader flags are big-endian. This little-endian conversion writes wire bytes for CORRELATION_ID (0x0800), not DATA (0x0008), so the test passes on an unrelated truncated-correlation error rather than the DATA rejection. Use big-endian conversion and assert the expected error path.
Note
LLM-assisted content (no human feedback).
| //! RDPUDP_FEC_HEADER: the mandatory header for every v1 datagram. | ||
| //! | ||
| //! MS-RDPEUDP Section 2.2.2.1. | ||
| //! Wire layout: `snSourceAck(4)` + `uReceiveWindowSize(2)` + `uFlags(2)` = 8 bytes, little-endian. |
There was a problem hiding this comment.
The header codec uses read_u*_be/write_u*_be, and MS-RDPEUDP requires network byte order. Change this to big-endian (or network byte order) to avoid contradicting the implementation and protocol.
Note
LLM-assisted content (no human feedback).
| //! both wire-level decode and encode failures from `ironrdp-core` and | ||
| //! protocol-level failures from the state machine itself. | ||
| //! | ||
| //! [`RdpeudpConnection`]: crate::RdpeudpConnection |
There was a problem hiding this comment.
RdpeudpConnection is not defined or re-exported by this crate, so this link and the poll_transmit link below render as unresolved rustdoc warnings. Remove the links or defer this connection-state-machine API documentation until that API exists.
Note
LLM-assisted content (no human feedback).
| the one this crate implements. Note that the documents take opposite byte | ||
| orders, and number the bits in their diagrams in opposite directions. | ||
|
|
||
| Sans-I/O: the state machine is driven by datagrams and by a caller-supplied |
There was a problem hiding this comment.
This crate currently exposes only PDU codecs; it has no state machine, caller-supplied instant, or transmit API. Reword this as planned design or scope the README to the implemented handshake-PDU layer so users are not promised unavailable functionality.
Note
LLM-assisted content (no human feedback).
2 participants
feat(rdpeudp): add the RDP-UDP crate and the v1 handshake PDUs
Second of six, and the first of the RDP-UDP transport Marc-André Moreau (@mamoreau-devolutions)
asked for on #140. Independent of the RDPEMT PR; the two can land in either
order.
Adds
ironrdp-rdpeudpwith the structures [MS-RDPEUDP] section 2.2 defines forthe three-way handshake: the FEC header and flags, the SYN and extended SYN
payloads that negotiate initial sequence numbers, MTU and protocol version, the
ACK vector with its run-length encoding, the AckOfAcks header, the correlation
ID payload, and the composite datagram that assembles them in the order 2.2.2
requires.
The convention that matters when reading this
Everything here is big-endian, and the diagrams number bits most
significant first. Section 2.2: "all of the messages written to the network or
read from the network MUST be in network byte order."
MS-RDPEUDP2, which the data transfer switches to, is little-endian and numbers
diagram bits least significant first. The two documents are opposite on both
counts. That is why the two wire formats are split across two PRs rather than
reviewed together.
Two readings I would particularly like checked
The ACK flag does not always announce an ACK vector. 2.2.2.1 defines it that
way, but 3.1.5.1.3 builds the SYN+ACK as a plain SYN with the flag set and
snSourceAckfilled in, and the capture in 4.1.2 confirms it:uFlagsis0x0005and the SYNDATA payload follows the 8-byte header directly, with novector between them. So on a SYN the flag says only that
snSourceAckismeaningful.
encoderefuses a SYN carrying a vector rather than writing bytes apeer would read as the start of SYNDATA.
The ACK vector's element layout and padding were settled against the ACK
packet capture in 4.2.3 (
00 01 04 00): a big-endianuAckVectorSize, thestate in the top two bits of each element, and padding out to a DWORD boundary.
Both captures are decoded byte by byte in
pdu_v1_datagram.rsrather than onlyround tripped against our own encoder. A mistake made symmetrically in encode
and decode is invisible to a round trip, so the captures are the only thing that
can catch it.
What is not here
No connection state machine, no v2 data transfer format. Those are the next two
PRs in the stack.
Test plan
cargo xtask check fmt/lints/tests/typos/locks43 tests in
ironrdp-testsuite-core, including the section 4.1.1, 4.1.2 and4.2.3 captures decoded whole.