Repository navigation
feat(rdpeudp): add the async driver for the RDP-UDP transport - #1687
Conversation
|
Benoît Cortier (@CBenoit) this one is size/XXL too, so automated review got skipped. Would you mind adding ai-review/allow-oversized so it goes through the normal pipeline? |
There was a problem hiding this comment.
Pull request overview
Adds a Tokio-based RDP-UDP transport integrating RDPEUDP2, TLS, and RDPEMT.
Changes:
- Adds the UDP driver, stream/framing adapters, TLS, and tunnel orchestration.
- Adds multitransport bootstrapping and transport lifecycle management.
- Adds unit and full-stack loopback tests.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.lock |
Locks new crate and test dependencies. |
crates/ironrdp-rdpeudp-tokio/Cargo.toml |
Defines the new transport crate. |
crates/ironrdp-rdpeudp-tokio/README.md |
Documents architecture and usage. |
crates/ironrdp-rdpeudp-tokio/src/clock.rs |
Bridges monotonic protocol time to system time. |
crates/ironrdp-rdpeudp-tokio/src/driver.rs |
Implements the socket/state-machine event loop. |
crates/ironrdp-rdpeudp-tokio/src/error.rs |
Defines driver and transport errors. |
crates/ironrdp-rdpeudp-tokio/src/framed.rs |
Implements session framing traits. |
crates/ironrdp-rdpeudp-tokio/src/lib.rs |
Exposes the crate’s public API. |
crates/ironrdp-rdpeudp-tokio/src/multitransport.rs |
Orchestrates multitransport negotiation. |
crates/ironrdp-rdpeudp-tokio/src/stream.rs |
Adapts RDPEUDP2 to asynchronous stream I/O. |
crates/ironrdp-rdpeudp-tokio/src/tls.rs |
Performs client and server TLS handshakes. |
crates/ironrdp-rdpeudp-tokio/src/transport.rs |
Implements connection, acceptance, and task lifecycle. |
crates/ironrdp-rdpeudp-tokio/src/tunnel.rs |
Drives RDPEMT framing and data processing. |
crates/ironrdp-testsuite-extra/Cargo.toml |
Adds integration-test dependencies. |
crates/ironrdp-testsuite-extra/tests/main.rs |
Registers the new test module. |
crates/ironrdp-testsuite-extra/tests/rdpeudp_tokio.rs |
Adds full-stack loopback tests. |
Marc-André Moreau (mamoreau-devolutions)
left a comment
There was a problem hiding this comment.
Request changes: the new client-side API has two interoperability regressions.
Note
LLM-assisted content (no human feedback).
Wires the sans-I/O connection and the RDPEMT tunnel to a real socket. A single task owns the `UdpSocket` and the `RdpeudpConnection` and runs a select loop over three sources: datagrams arriving, plaintext the TLS layer wants to send, and timer expiry. It is the only place in the stack that reads a clock. `RdpeudpStream` gives tokio-rustls something to wrap, bridging the driver to TLS through a pair of buffers, so the TLS layer never learns it is running over UDP. Above that, `connect_udp` and `accept_udp` run the whole sequence: RDP-UDP handshake, TLS, RDPEMT tunnel, then a data pump. `UdpTransport` implements `FramedRead` and `FramedWrite` so the session layer reaches it through the same traits as the TCP path. Four things this layer has to get right that are easy to miss: A task that is spawned and then dropped keeps running. All three tasks (driver, read pump, write pump) are held in a guard that aborts on drop and can be taken back when the task is to be awaited, so the failure paths between spawning the driver and returning the transport do not leave any of them holding the socket. `shutdown()` waits for all three in turn: the read pump gets EOF once the shared I/O bridge is marked closed, the write pump exits on its own once the send channel is dropped, and the driver is aborted last since it may still be sitting in its select loop. The read buffer is bounded. Ceasing to read from the socket is the backpressure the protocol expects: acknowledgements stop, and the receive window closes on the sender. A reader waking the driver once it has taken bytes out is what lets it resume. An empty tunnel PDU is not end of stream. `Framed` reads a zero-length result as EOF, and [MS-RDPEMT] 2.2.2.3 puts no minimum on HigherLayerData; [MS-RDPBCGR] 1.3.9 sends the auto-detect messages in subheaders with nothing behind them. The cookie hash the version 3 SYN carries is derived here from the tunnel config, so the sans-I/O crate takes the finished 32 bytes and needs no cryptographic dependency of its own. The Initiate Multitransport Request/Response PDUs moved during Devolutions#1626's review round: they now live as `MultitransportRequestPdu`/ `MultitransportResponsePdu` in `ironrdp-pdu::rdp::multitransport` instead of `ironrdp-rdpemt`, and decode/encode their own `BasicSecurityHeader` rather than expecting a caller to have stripped it. `MultitransportBootstrap` is updated to the real shape: it builds `TunnelConfig` directly from the two fields it needs instead of calling a `to_tunnel_config()` method that moved with the type and no longer exists. `TunnelEvent::Data` also changed, from a tuple variant to `Data { sub_headers, data }`, and `TunnelEvent` is now `#[non_exhaustive]`; the data loop matches on `data` and ignores `sub_headers` (auto-detect bandwidth measurement, out of scope here) and any future variant, rather than discarding one payload silently or panicking on either.
62357ac to
35f13ac
Compare
|
OK I've addressed everything I think. Thanks. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
crates/ironrdp-rdpeudp-tokio/src/transport.rs:550
- The server has the same early-driver-exit problem: if sending or receiving during the RDPEUDP handshake fails, this notification never arrives and
accept_udpreports a timeout only after the full accept budget. Select between connection notification and driver completion so the real driver error is returned immediately.
// Wait for the RDPEUDP2 handshake to complete (ACK received)
connected_notify.notified().await;
if let Some(driver) = driver_handle.take_if_finished() {
crates/ironrdp-rdpeudp-tokio/src/transport.rs:371
- This timeout waits only for
connected_notify; if the driver exits first with a socket or protocol error, no notification is sent, soconnect_udpwaits the full timeout and reportsHandshakeTimeoutinstead of the actual failure. Race the notification against driver task completion and preserve the task result.
This issue also appears on line 547 of the same file.
// Wait for the RDPEUDP2 handshake to complete (Event::Connected)
let handshake_result = tokio::time::timeout(config.handshake_timeout, connected_notify.notified()).await;
if handshake_result.is_err() {
return Err(UdpTransportError::handshake_timeout("connect udp"));
| if !data.is_empty() { | ||
| self.conn | ||
| .send(data) | ||
| .map_err(|error| DriverError::rdpeudp("queue outbound data", error))?; |
| let mut config = UdpTransportConfig::new(server_addr, server_name, tunnel_config); | ||
| config.connection_config = connection_config; |
9aa8a4b
into
Devolutions:master
|
Greg Lamberson (@glamberson) I was trying to get the checks unstuck to merge this PR so I re-launched GitHub Copilot, which found a few things you may want to look at in a follow-up PR, if necessary |
|
Marc-André Moreau (@mamoreau-devolutions) OK Thanks. I'll take care of it and submit the next one as well. Thanks. |
## Summary - connect_udp and accept_udp_inner both waited on connected_notify without racing it against the driver task's own completion - a driver that exits early with a real error (socket failure, fatal protocol error) went unnoticed until the configured handshake timeout elapsed, or until the caller's outer accept_timeout canceled the whole accept sequence for accept_udp_inner, which had no timeout of its own on this wait at all - either way the caller got a generic HandshakeTimeout instead of the driver's actual error - both call sites now race connected_notify against the driver's JoinHandle directly, via a shared driver_exit_during_handshake helper that turns a join result into the right UdpTransportError - added AbortOnDrop::handle_mut to borrow the wrapped JoinHandle for a select! branch without taking ownership of it ## Validation `cargo xtask check fmt/lints/tests/typos/locks` all pass. Four new tests cover the shared helper's three outcomes (real driver error, clean exit treated as failure, panic) and the select! race itself (a driver that dies immediately is detected well inside a 5-second bound instead of falling through to a 60-second stand-in timeout). ## Notes Surfaced by Copilot's second review pass on #1687, posted 11 minutes before that PR merged; not addressed there.
…strap::connect (#1706) ## Summary - connect_udp already accepted a server_cert_verifier to opt into real TLS certificate validation, added in #1687's own first review round - MultitransportBootstrap builds its UdpTransportConfig internally and never set that field, so a caller going through the high-level bootstrap API had no way to reach the verification connect_udp itself already supported - connect() now takes server_cert_verifier as a fourth parameter and forwards it the same way connection_config already is - breaking change to this crate's public API (adds a required parameter); the crate has no external consumers yet ## Validation `cargo xtask check fmt/lints/tests/typos/locks` all pass. Added a full-stack test proving the verifier is actually consulted through the bootstrap path, reusing the AlwaysRejectVerifier double from the sibling connect_udp test; mutation-verified by temporarily reverting the fix and confirming the new test fails. ## Notes Surfaced by Copilot's second review pass on #1687, posted 11 minutes before that PR merged; not addressed there. Third of three follow-up PRs (see #1704 and #1705 for the other two).
…error (#1705) ## Summary - RdpeudpConnection::send's SendBufferFull is documented as transient, but the async driver treated it as fatal, tearing down an otherwise healthy connection under sustained write load - send() took the payload by value with no way to hand it back on error, so the rejected bytes were also gone by the time the error propagated - send()'s signature now returns the rejected data alongside the error on every rejection, via a new SendError type; breaking change to this crate's public API, but it has no external consumers yet - the driver holds a backpressured write in a new pending_write field instead of dropping it, and retries once poll_transmit has moved entries out of the send buffer (an incoming ACK or a retransmit both qualify) - the write-data branch of the driver's select loop stops pulling new data out of the shared write buffer while a write is already pending, so a retry never gets interleaved out of order ## Validation `cargo xtask check fmt/lints/tests/typos/locks` all pass. Four new driver-level tests build a real, in-memory-established connection and mutation-verify the fix: filling the send buffer to its bound no longer errors, the rejected payload comes back intact, a retry delivers once drain_transmits frees room, and a retry into an already-closed connection does not turn a clean shutdown into an error. ## Notes Surfaced by Copilot's second review pass on #1687, posted 11 minutes before that PR merged; not addressed there. Second of three follow-up PRs (see #1704 for the first).
Fifth of six. Stacked on the connection state machine PR, and needs the
RDPEMT PR too.
Wires the sans-I/O connection and the RDPEMT tunnel to a real socket. A single
task owns the
UdpSocketand theRdpeudpConnectionand runs a select loop overthree sources: datagrams arriving, plaintext the TLS layer wants to send, and
timer expiry. It is the only place in the stack that reads a clock.
RdpeudpStreamgives tokio-rustls something to wrap, bridging the driver to TLSthrough a pair of buffers so the TLS layer never learns it is running over UDP.
connect_udpandaccept_udprun the whole sequence: RDP-UDP handshake, TLS,RDPEMT tunnel, then a data pump.
UdpTransportimplementsFramedReadandFramedWrite, so the session layer reaches it through the same traits as the TCPpath.
Three things this layer has to get right
A spawned task that is dropped keeps running. All three handles (driver,
read pump, write pump) live in a guard that aborts on drop and can be taken
back when the task is meant to be awaited. That matters because the TLS
upgrade and the tunnel handshake both sit between spawning the driver and
returning the transport, and both propagate with
?. Making the guardstructural means each early return cleans up without every path having to
remember.
shutdown()follows the same three-task shape in reverse: the readpump gets EOF once the shared I/O bridge is marked closed, the write pump
exits on its own once the send channel is dropped, and the driver is aborted
last since it may still be sitting in its select loop.
The read buffer is bounded. Ceasing to read from the socket is the
backpressure the protocol expects rather than a drop policy: unread datagrams
stay in the socket buffer, acknowledgements stop, and the receive window closes
on the sender, which is what [MS-RDPEUDP2] 3.1.1.2.2 has it for. The half that is
easy to leave out is resuming, so
poll_readwakes the driver once it has takenbytes out.
An empty tunnel PDU is not end of stream.
Framedreads a zero-length resultas EOF, and [MS-RDPEMT] 2.2.2.3 puts no minimum on
HigherLayerData.[MS-RDPBCGR] 1.3.9 sends the four Continuous Auto-Detection messages
"encapsulated in the RDP_TUNNEL_SUBHEADER structure ... over the sideband
channels that are in active use", and those carriers have nothing behind the
subheaders.
On the cookie hash
The SHA-256 the version 3 SYN carries is derived here from the tunnel config,
which already holds the security cookie. That keeps
ironrdp-rdpeudpfree of acryptographic dependency and means a caller cannot supply a hash over a different
cookie than the tunnel will present.
Test plan
cargo xtask check fmt/lints/tests/typos/lockspluscargo xtask wasm checkUnit tests for the stream adapter, the framing adapter, task lifetime,
backpressure, and clean-vs-truncated tunnel PDU framing, plus nine full-stack
loopback tests in
ironrdp-testsuite-extrathat run a client and serverthrough handshake, TLS, tunnel and bidirectional data over localhost,
including IPv4 and IPv6, an oversized-payload rejection, a caller-supplied
certificate verifier, and a failed-reconnect state check.
One caveat on those loopback tests, since it caught me out: localhost carries an
oversized datagram perfectly well, so they cannot detect an MTU violation. The
datagram sizes are asserted in the connection tests instead.