feat(rdpudp): add protocol codec foundation - #1583
feat(rdpudp): add protocol codec foundation#1583Marc-André Moreau (mamoreau-devolutions) wants to merge 1 commit into
Conversation
github-actions
Bot
added
maintainer-required
8f655fe to
90a22f2
Compare
There was a problem hiding this comment.
Pull request overview
Adds foundational RDP-UDP and RDPEMT wire codecs to ironrdp-pdu.
Changes:
- Adds reliable RDP-UDP headers, handshake data, ACK vectors, and payload codecs.
- Adds RDPEMT tunnel creation/data codecs with validation.
- Exposes the new UDP module and adds unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
crates/ironrdp-pdu/src/rdp/udp.rs |
Implements RDP-UDP wire structures. |
crates/ironrdp-pdu/src/rdp/multitransport.rs |
Implements RDPEMT tunnel PDUs. |
crates/ironrdp-pdu/src/rdp/mod.rs |
Exports the UDP module. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if src.len() != payload_length { | ||
| return Err(invalid_field_err!( | ||
| "payloadLength", | ||
| "does not match the bytes following the tunnel header" | ||
| )); | ||
| } |
| fn encode(&self, payload_length: usize, dst: &mut WriteCursor<'_>) -> EncodeResult<()> { | ||
| let payload_length: u16 = cast_length!("payloadLength", payload_length)?; | ||
| let header_length: u8 = cast_length!("headerLength", self.size())?; | ||
|
|
||
| dst.write_u8(self.action.as_u8()); | ||
| dst.write_u16(payload_length); | ||
| dst.write_u8(header_length); | ||
| self.subheaders.iter().try_for_each(|subheader| subheader.encode(dst)) | ||
| } |
| /// | ||
| /// Defined in [\[MS-RDPEMT\] 2.2.1.1]. | ||
| /// | ||
| /// [\[MS-RDPEMT\] 2.2.1.1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpemt/80d0849e-91a0-4fe7-83ad-ea9eaa7d15e9 |
| impl<'de> Decode<'de> for TunnelPdu { | ||
| fn decode(src: &mut ReadCursor<'de>) -> DecodeResult<Self> { | ||
| ensure_size!(in: src, size: TunnelHeader::FIXED_PART_SIZE); |
| impl<'de> Decode<'de> for RdpUdpFecHeader { | ||
| fn decode(src: &mut ReadCursor<'de>) -> DecodeResult<Self> { | ||
| ensure_fixed_part_size!(in: src); |
| let flags = RdpUdpFlags::from_bits(src.read_u16_be()) | ||
| .ok_or_else(|| invalid_field_err!("uFlags", "contains reserved bits"))?; |
| //! | ||
| //! Defined in [\[MS-RDPEUDP\] 2.2.2.1, 2.2.2.4-2.2.2.7]. | ||
| //! | ||
| //! [\[MS-RDPEUDP\] 2.2.2.1, 2.2.2.4-2.2.2.7]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeudp/2b8a6f10-cf3a-49f6-b989-51de4d2f99e6 |
90a22f2 to
98489c3
Compare
98489c3 to
404f93b
Compare
Add RDP-UDP and RDPEMT wire codecs for the transport foundation. Reject subheaders in RDPEMT create PDUs per the protocol. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
404f93b to
3883b8b
Compare
github-actions
Bot
added
breaking-change
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wire layouts are largely correct: the tunnel-header nibble split, header/payload length rules, MTU bounds, big-endian RDPEUDP fields, and ACK-vector alignment all match the cited spec text. Four issues warrant changes. The four new tunnel Encode impls omit the ensure_size guard every neighbouring PDU uses, so encoding into an undersized caller buffer panics instead of returning NotEnoughBytes. RdpUdpFlags omits the documented 0x0080 SACK_OPTION bit, and since decode uses from_bits, a peer setting it makes the common header undecodable. TunnelHeader::decode reports truncation as an invalid field and rejects trailing bytes while exposing no framing helper. The ACK-vector decoder rejects non-zero padding the spec does not constrain. Non-blocking: two deep clones per encoded data PDU on the tunnel hot path, an orphan RdpUdpProtocolVersion, self-round-trip-only tests with no spec fixtures, RDP-UDP layered under the rdp module, and a dead length check.
Protocol analysis: partially_accepted — Three of four discrepancies survive verification: the missing RDPUDP_FLAG_SACK_OPTION 0x0080 (confirmed independently by walking the declared bit sequence, which jumps 0x0040 to 0x0100), the zero-padding rejection in RdpUdpAckVector::decode, and truncated tunnel PDUs collapsing into invalid-field. For the last I checked the supporting facts in-tree: ironrdp-core models NotEnoughBytes separately, and this crate already exposes PduHint/find_size framing helpers. read_padding ignoring content likewise supports the ACK-vector point. I downgrade the fourth: is_success comparing to zero deviates from HRESULT severity semantics but matches the existing convention in the same file and Windows behaviour, so it is low. The conforming assessments check out against the diff bytes.
| .encode(Self::PAYLOAD_SIZE, dst)?; | ||
| dst.write_u32(self.request_id); | ||
| dst.write_u32(0); | ||
| dst.write_slice(&self.security_cookie); |
There was a problem hiding this comment.
blocking / high: None of the four new `Encode::encode` impls (`TunnelCreateRequestPdu` 191, `TunnelCreateResponsePdu` 277, `TunnelDataPdu` 354, and transitively `TunnelHeader` 117 / `TunnelSubheader` 39) check the destination cursor before writing. `WriteCursor::write_array`/`write_slice` index `self.inner[self.pos..self.pos + n]` directly and panic on overflow; they do not return an error. The pre-existing PDUs in this same file guard with `ensure_fixed_part_size!(in: dst)` (lines 523, 656), and every new type in `udp.rs` does too, so this inconsistency is introduced only in the tunnel types. Failure scenario: `ironrdp_core::encode(&TunnelCreateRequestPdu::new(7, cookie), &mut buf)` with a 16-byte `buf` panics with a slice index out of range instead of returning `EncodeError::NotEnoughBytes`. `encode_vec`/`encode_buf` size the buffer from `size()` and hide this, which is why the new tests do not catch it.
| /// Congestion-window reset. | ||
| const CWR = 0x0040; | ||
| /// ACK-of-ACK vector follows the ACK vector. | ||
| const ACK_OF_ACKS = 0x0100; |
There was a problem hiding this comment.
blocking / high: `RdpUdpFlags` skips 0x0080 (`RDPUDP_FLAG_SACK_OPTION`), jumping from `CWR = 0x0040` straight to `ACK_OF_ACKS = 0x0100`. Because `RdpUdpFecHeader::decode` (line 106) uses `RdpUdpFlags::from_bits`, which returns `None` for any bit outside the declared set, a peer that sets this documented flag makes the whole common header fail to decode. This is not a tolerated-unknown-bit case: 0x0080 is a value the flag table defines, so the omission is a gap in the enumeration rather than a deliberate policy. Failure scenario: an `RDPUDP_FEC_HEADER` with `uFlags = 0x0084` (ACK | SACK_OPTION) yields `invalid uFlags: contains reserved bits`, aborting an otherwise valid RDP-UDP datagram. I verified the omission independently by walking the declared bit sequence.
| "payloadLength", | ||
| "does not match the bytes following the tunnel header" | ||
| )); | ||
| } |
There was a problem hiding this comment.
blocking / medium: `TunnelHeader::decode` requires `src.len() == payload_length` and reports any mismatch as `invalid_field`. Two consequences. A truncated buffer (fewer bytes than `PayloadLength`) is reported as a malformed PDU rather than `DecodeErrorKind::NotEnoughBytes`, so a caller framing tunnel PDUs off the TLS/DTLS stream cannot tell "await more bytes" from "drop the connection". And a buffer holding one complete PDU plus the start of the next is rejected outright, with no supported way to recover the length first: `TunnelHeader` is private and no `PduHint`/`find_size` impl is added, unlike the `X224_HINT`/`FAST_PATH_HINT` framing helpers this crate already provides for stream-carried PDUs (`lib.rs:173-220`). Failure scenario: a reader handing `decode::<TunnelPdu>` a 20-byte prefix of a 28-byte create request gets an invalid-field error and cannot learn it needed 8 more bytes.
| let encoded = src.read_slice(encoded_len).to_vec(); | ||
| if src.read_slice(padding_size).iter().any(|&byte| byte != 0) { | ||
| return Err(invalid_field_err!("padding", "must be zero")); | ||
| } |
There was a problem hiding this comment.
blocking / medium: `RdpUdpAckVector::decode` rejects the DWORD-alignment padding when any byte is non-zero. MS-RDPEUDP 2.2.2.7 describes `Padding` purely as a length adjustment so the structure ends on a DWORD boundary and states no value constraint, unlike fields the specification explicitly requires to be zero. The repository convention also points the other way: `ironrdp_core::read_padding` simply advances the cursor and ignores content. Because the encoder writes zeros, the round-trip test at line 355 passes regardless, so this only manifests against a real peer. Failure scenario: a conforming sender that leaves the one padding byte uninitialised produces an ACK datagram this decoder rejects as malformed, dropping acknowledgements and stalling the reliable transport.
| } | ||
| .size() | ||
| + self.data.len() | ||
| } |
There was a problem hiding this comment.
non_blocking / medium: `TunnelDataPdu::encode` and `TunnelDataPdu::size` each build a throwaway `TunnelHeader` with `subheaders: self.subheaders.clone()`, deep-copying a `Vec<TunnelSubheader>` (each holding its own `Vec<u8>`) purely to compute a length or write four bytes. `encode_vec`/`encode_buf` call `size()` and then `encode()`, so every emitted data PDU performs two full clones. Tunnel data is the hot path for all traffic carried by an established RDPEMT tunnel, and STYLE.md calls out avoiding needless allocations there. A borrowing header (`subheaders: &[TunnelSubheader]`) or a free function taking the slice removes both clones without changing behaviour.
| pub mod server_license; | ||
| pub mod session_info; | ||
| pub mod suppress_output; | ||
| pub mod udp; |
There was a problem hiding this comment.
non_blocking / medium: Placement conflates protocol layers. The `rdp` module holds MS-RDPBCGR RDP-layer PDUs that ride inside the share/security layer; RDP-UDP is a transport below X.224, alongside `tpkt`, `tpdu`, and `x224`, all of which are top-level modules in this crate. `rdp::udp` therefore reads as "an RDP-layer PDU named udp", which it is not. Relatedly, `multitransport.rs` still carries a module doc scoped to MS-RDPBCGR 2.2.15.2 (lines 1-7) while now hosting MS-RDPEMT 2.2.1-2.2.2 tunnel PDUs, with no doc update. Both are cheap to fix now and awkward later: moving a public module is a breaking change once released.
| let length: u8 = cast_length!("subHeaderLength", self.size())?; | ||
| if usize::from(length) < Self::FIXED_PART_SIZE { | ||
| return Err(invalid_field_err!("subHeaderLength", "must be at least two bytes")); | ||
| } |
There was a problem hiding this comment.
non_blocking / low: Dead branch. `length` is cast from `self.size()`, which is `FIXED_PART_SIZE + self.data.len()` and therefore always at least `FIXED_PART_SIZE`; the subsequent `usize::from(length) < Self::FIXED_PART_SIZE` check can never be true. The equivalent check in `decode` (line 56) is the one that matters, since there the value comes from the wire. Removing the encode-side check makes it clear which of the two is load-bearing.
| //! structures used by reliable RDP-UDP versions 1 and 2. | ||
| //! | ||
| //! Defined in [\[MS-RDPEUDP\] 2.2.2.1, 2.2.2.4-2.2.2.7]. | ||
| //! |
There was a problem hiding this comment.
non_blocking / low: The module doc claims coverage of "[MS-RDPEUDP] 2.2.2.1, 2.2.2.4-2.2.2.7", but 2.2.2.6 (ACK of ACKs) is not implemented even though `RdpUdpFlags::ACK_OF_ACKS` advertises it, while 2.2.2.9 is partially covered by `RdpUdpProtocolVersion` and is not mentioned. Listing the implemented sections explicitly avoids a reader assuming an ACK-of-acks codec exists.
| self.hr_response == Self::S_OK | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
non_blocking / low: `is_success` tests `hr_response == 0`, but `HrResponse` is typed as an HRESULT, where success is indicated by a cleared severity bit rather than a zero value, so a success code such as S_FALSE (0x00000001) would be reported as a rejection. I rate this low rather than blocking: the pre-existing `MultitransportResponsePdu` in this same file already uses the equality convention, and Windows servers are documented to send only S_OK, so changing it in isolation would introduce an inconsistency for no observed gain. If it is left as is, the doc comment on `S_OK` should state that the check is deliberately exact-match.
| //! structures used by reliable RDP-UDP versions 1 and 2. | ||
| //! | ||
| //! Defined in [\[MS-RDPEUDP\] 2.2.2.1, 2.2.2.4-2.2.2.7]. | ||
| //! |
There was a problem hiding this comment.
question / low: The module states that the transport state machine lives outside this crate, and nothing in the workspace consumes these types yet. Which crate is intended to own the RDP-UDP state machine and the RDPEMT tunnel session, and is it planned for a follow-up here? The answer determines whether these structures belong in `ironrdp-pdu` at all or in a dedicated crate next to their state machine, and it also settles where the framing helper noted above should live.
github-actions
Bot
added
the
maintainer-required
Add RDP-UDP and RDPEMT wire codecs for the transport foundation.
Reject subheaders in RDPEMT create PDUs per the protocol.
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com