From eb09aed6cbb67578df0466ed8073be559c094753 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 10 Sep 2026 14:33:49 -0500 Subject: [PATCH 1/4] feat(server): add accept_finalize_with_multitransport driver Add an async driver mirroring the client side's connect_finalize_with_multitransport: it drives the acceptor sequence to completion the same way accept_finalize already does, and awaits an app-supplied handler once, synchronously, the moment the acceptor sends an Initiate Multitransport Request, so the caller can establish the sideband UDP transport (RDPEUDP2 + TLS + RDPEMT). Unlike the client-side callback, the handler reports nothing back into the sequence: the acceptor has already sent the request and moved on by the time the handler runs, so there is no response to build from an outcome. The handler should return promptly (e.g. by spawning the actual work) rather than driving the transport to completion inline, or the handshake stalls behind it. accept_finalize becomes a thin wrapper around this with a no-op handler, matching the client side's connect_finalize/connect_finalize_with_multitransport relationship. The driver seeds its "already notified" tracking from whatever request is already present rather than starting at false: a Deactivation- Reactivation Sequence rebuilds the acceptor via new_deactivation_reactivation(), which carries the original request forward without running bootstrapping again, then this function is called a second time on the rebuilt acceptor. Without the seed, that second call's first loop iteration would treat the carried-over request as newly sent and notify the handler again. Adds integration tests in ironrdp-testsuite-core driving a real Acceptor over a tokio::io::duplex pair with a hand-rolled client script: the handler fires exactly once with the sent request and does not block the handshake, a late Initiate Multitransport Response is still tolerated ahead of Confirm Active during the async-driven path, and the handler does not fire again across a reactivation round. Building the first of these surfaced a real bug: an initial take_multitransport_request() consumed the same field CapabilitiesWaitConfirm's response tolerance depends on, breaking that check the moment the driver read the request. Removed in favor of the local flag, keeping multitransport_request() a plain borrow. --- Cargo.lock | 2 + crates/ironrdp-acceptor/src/lib.rs | 56 +++ crates/ironrdp-testsuite-core/Cargo.toml | 2 + .../tests/server/mod.rs | 1 + .../tests/server/multitransport_finalize.rs | 358 ++++++++++++++++++ 5 files changed, 419 insertions(+) create mode 100644 crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs diff --git a/Cargo.lock b/Cargo.lock index 538aed934a..78076bfc73 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3384,6 +3384,7 @@ dependencies = [ "expect-test", "hex", "ironrdp-acceptor", + "ironrdp-async", "ironrdp-bulk", "ironrdp-cfg", "ironrdp-cliprdr", @@ -3415,6 +3416,7 @@ dependencies = [ "ironrdp-session", "ironrdp-str", "ironrdp-svc", + "ironrdp-tokio", "ironrdp-usb", "openh264", "paste", diff --git a/crates/ironrdp-acceptor/src/lib.rs b/crates/ironrdp-acceptor/src/lib.rs index e7f3636cff..8f186029b8 100644 --- a/crates/ironrdp-acceptor/src/lib.rs +++ b/crates/ironrdp-acceptor/src/lib.rs @@ -82,19 +82,75 @@ where } pub async fn accept_finalize( + framed: Framed, + acceptor: &mut Acceptor, +) -> ConnectorResult<(Framed, AcceptorResult)> +where + S: FramedRead + FramedWrite, +{ + accept_finalize_with_multitransport(framed, acceptor, |_, _| async {}).await +} + +/// Completes the connection sequence, notifying `multitransport_handler` each +/// time the acceptor sends an Initiate Multitransport Request, so the caller +/// can establish the sideband UDP transport (RDPEUDP2 + TLS + RDPEMT). +/// +/// Unlike [`ironrdp_async::connect_finalize_with_multitransport`] on the +/// client side, `multitransport_handler` does not report an outcome back +/// into the sequence: by the time it runs, the acceptor has already sent the +/// request and moved on to capability negotiation (see the doc comment on +/// [`AcceptorState::MultitransportBootstrapping`]), and +/// nothing here waits for the sideband transport to come up. Establishing it +/// is the caller's job, driven independently of this function's return. +/// `multitransport_handler` is awaited once per request, synchronously, +/// immediately after the sequence step that sent it: it should return +/// promptly (for example, by spawning the actual UDP-accept work on the +/// caller's own runtime) rather than driving the transport to completion +/// inline, or the RDP handshake stalls behind it. +/// +/// # Panics +/// +/// Panics if `multitransport_soft_sync_negotiated()` returns `None` right +/// after `multitransport_request()` returned `Some`, which the two methods' +/// own contract does not allow. +pub async fn accept_finalize_with_multitransport( mut framed: Framed, acceptor: &mut Acceptor, + mut multitransport_handler: H, ) -> ConnectorResult<(Framed, AcceptorResult)> where S: FramedRead + FramedWrite, + H: AsyncFnMut(ironrdp_pdu::rdp::multitransport::MultitransportRequestPdu, bool), { let mut buf = WriteBuf::new(); + // `multitransport_request()` borrows rather than consumes (the field it + // reads also gates `CapabilitiesWaitConfirm`'s tolerance for a late + // Initiate Multitransport Response), so this driver tracks locally + // whether it has already notified the caller instead. Seeded from + // whatever is already present rather than `false`: a Deactivation- + // Reactivation Sequence rebuilds the acceptor via + // `Acceptor::new_deactivation_reactivation()`, which carries the + // original request forward, then this function is called again on the + // rebuilt acceptor. Bootstrapping does not run a second time, so without + // this the first loop iteration of that fresh call would treat the + // carried-over request as newly sent and notify the handler again. + let mut notified = acceptor.multitransport_request().is_some(); loop { if let Some(result) = acceptor.get_result() { return Ok((framed, result)); } + single_sequence_step(&mut framed, acceptor, &mut buf).await?; + + if !notified && let Some(request) = acceptor.multitransport_request() { + let request = request.clone(); + let soft_sync = acceptor + .multitransport_soft_sync_negotiated() + .expect("multitransport_request() just returned Some, so a request was sent"); + multitransport_handler(request, soft_sync).await; + notified = true; + } } } diff --git a/crates/ironrdp-testsuite-core/Cargo.toml b/crates/ironrdp-testsuite-core/Cargo.toml index 2c5541b7fa..2654492a8b 100644 --- a/crates/ironrdp-testsuite-core/Cargo.toml +++ b/crates/ironrdp-testsuite-core/Cargo.toml @@ -40,6 +40,8 @@ hex = "0.4" ironrdp-cliprdr-format.path = "../ironrdp-cliprdr-format" ironrdp-cliprdr = { path = "../ironrdp-cliprdr", features = ["__test"] } ironrdp-acceptor.path = "../ironrdp-acceptor" +ironrdp-async.path = "../ironrdp-async" +ironrdp-tokio.path = "../ironrdp-tokio" ironrdp-bulk.path = "../ironrdp-bulk" ironrdp-connector.path = "../ironrdp-connector" ironrdp-displaycontrol.path = "../ironrdp-displaycontrol" diff --git a/crates/ironrdp-testsuite-core/tests/server/mod.rs b/crates/ironrdp-testsuite-core/tests/server/mod.rs index c43da782fa..c7a2e11cc0 100644 --- a/crates/ironrdp-testsuite-core/tests/server/mod.rs +++ b/crates/ironrdp-testsuite-core/tests/server/mod.rs @@ -4,6 +4,7 @@ mod connection_policy; mod credential_validator; mod fast_path; mod finalize_timeout; +mod multitransport_finalize; mod rdpdr; mod rdpei; mod remotefx_entropy_coder; diff --git a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs new file mode 100644 index 0000000000..cfed9a770c --- /dev/null +++ b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs @@ -0,0 +1,358 @@ +//! Integration coverage for `accept_finalize_with_multitransport`. +//! +//! Drives a real `Acceptor` over an in-memory duplex stream, playing the role +//! of the client by hand (raw PDU bytes, not a `ClientConnector`) so the test +//! exercises the acceptor's actual async driver rather than only the +//! synchronous `Acceptor::step()` surface already covered in `acceptor.rs`. + +use std::borrow::Cow; +use std::sync::{Arc, Mutex}; + +use ironrdp_acceptor::{Acceptor, accept_finalize_with_multitransport}; +use ironrdp_async::FramedWrite as _; +use ironrdp_connector::DesktopSize; +use ironrdp_core::{WriteBuf, decode, encode_buf, encode_vec}; +use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; +use ironrdp_pdu::mcs::{self, ConnectInitial}; +use ironrdp_pdu::nego::{self, SecurityProtocol}; +use ironrdp_pdu::rdp::multitransport::{MultitransportRequestPdu, MultitransportResponsePdu, RequestedProtocol}; +use ironrdp_pdu::x224::{X224, X224Data}; +use ironrdp_testsuite_core::gcc::CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS; +use ironrdp_testsuite_core::rdp::{ + CLIENT_DEMAND_ACTIVE_PDU_BUFFER, CLIENT_FONT_LIST_BUFFER, CLIENT_INFO_PDU_BUFFER, CLIENT_SYNCHRONIZE_BUFFER, + CONTROL_COOPERATE_BUFFER, CONTROL_REQUEST_CONTROL_BUFFER, +}; +use ironrdp_tokio::TokioFramed; +use tokio::io::DuplexStream; + +fn encode_x224_pdu<'a, T: ironrdp_pdu::x224::X224Pdu<'a>>(pdu: T) -> Vec { + let mut buf = WriteBuf::new(); + encode_buf(&X224(pdu), &mut buf).unwrap(); + buf.filled().to_vec() +} + +fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { + encode_x224_pdu(mcs::SendDataRequest { + initiator_id, + channel_id, + user_data: Cow::Borrowed(user_data), + }) +} + +/// Reads one PDU from `framed` and returns its raw bytes (TPKT/X224 header +/// stripped, matching what `decode` in the rest of this file expects). +async fn read_pdu(framed: &mut TokioFramed) -> Vec { + let (_, bytes) = framed.read_pdu().await.expect("read one PDU"); + bytes.to_vec() +} + +/// Drives the client side of a full connection sequence up to (and through) +/// finalization, over `framed`, against an acceptor configured to offer +/// reliable UDP multitransport. Sends a (soft-sync-less) Initiate +/// Multitransport Response before the Confirm Active, which the acceptor +/// must tolerate rather than error on. Returns the same stream, still open, +/// plus the user and I/O channel IDs, for a caller that wants to keep +/// driving it (e.g. through reactivation). +async fn play_client(mut framed: TokioFramed) -> (TokioFramed, u16, u16) { + // Connection Request / Confirm. Plain RDP security (empty protocol) so no + // TLS upgrade is needed and the acceptor drives straight through. + let request = nego::ConnectionRequest { + nego_data: None, + flags: nego::RequestFlags::empty(), + protocol: SecurityProtocol::empty(), + correlation_info: None, + }; + framed.write_all(&encode_x224_pdu(request)).await.unwrap(); + let _confirm = read_pdu(&mut framed).await; + + // MCS Connect Initial, advertising both a message channel and reliable + // UDP multitransport support, so the acceptor has something to offer. + let mut blocks = CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS.clone(); + blocks.network = None; + blocks.message_channel = Some(ClientMessageChannelData); + blocks.multi_transport_channel = Some(MultiTransportChannelData { + flags: MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR, + }); + let connect_initial = ConnectInitial::with_gcc_blocks(blocks).unwrap(); + let mut initial_buf = WriteBuf::new(); + ironrdp_connector::encode_x224_packet(&connect_initial, &mut initial_buf).unwrap(); + framed.write_all(initial_buf.filled()).await.unwrap(); + + let response_bytes = read_pdu(&mut framed).await; + let payload = decode::>>(&response_bytes).unwrap().0; + let response = decode::(payload.data.as_ref()).unwrap(); + let server_blocks = response.conference_create_response.gcc_blocks(); + let io_channel_id = server_blocks.network.io_channel; + let message_channel_id = server_blocks + .message_channel + .as_ref() + .expect("acceptor must negotiate a message channel") + .mcs_message_channel_id; + + // Channel connection: Erect Domain, Attach User, then join every channel + // the server expects (user, I/O, message). + framed + .write_all(&encode_x224_pdu(mcs::ErectDomainPdu { + sub_height: 0, + sub_interval: 0, + })) + .await + .unwrap(); + framed + .write_all(&encode_x224_pdu(mcs::AttachUserRequest)) + .await + .unwrap(); + + let confirm_bytes = read_pdu(&mut framed).await; + let attach_user_confirm = decode::>(&confirm_bytes).unwrap().0; + let user_channel_id = attach_user_confirm.initiator_id; + + for channel_id in [user_channel_id, io_channel_id, message_channel_id] { + framed + .write_all(&encode_x224_pdu(mcs::ChannelJoinRequest { + initiator_id: user_channel_id, + channel_id, + })) + .await + .unwrap(); + let _join_confirm = read_pdu(&mut framed).await; + } + + // Client Info PDU on the I/O channel, then the license PDU comes back. + framed + .write_all(&encode_send_data_request( + user_channel_id, + io_channel_id, + &CLIENT_INFO_PDU_BUFFER, + )) + .await + .unwrap(); + let _license = read_pdu(&mut framed).await; + + // The Initiate Multitransport Request, on the message channel. + let request_bytes = read_pdu(&mut framed).await; + let indication = mcs::decode_send_data_indication(&request_bytes).unwrap(); + assert_eq!( + indication.channel_id, message_channel_id, + "request must go out on the message channel" + ); + let request = indication.decode_user_data::().unwrap(); + assert_eq!(request.requested_protocol, RequestedProtocol::UdpFecR); + + // Demand Active, on the I/O channel. + let _demand_active = read_pdu(&mut framed).await; + + // A response arriving ahead of Confirm Active: the acceptor must + // silently absorb it and keep waiting rather than erroring. + let response = MultitransportResponsePdu::success(request.request_id); + framed + .write_all(&encode_send_data_request( + user_channel_id, + message_channel_id, + &encode_vec(&response).unwrap(), + )) + .await + .unwrap(); + + // Confirm Active, after the multitransport response: on the wire, the + // response (if any) is sent while the client resolves its own + // MultitransportPending state, strictly before it ever reads Demand + // Active, so this ordering matches a real client. CapabilitiesWaitConfirm + // must see this before the acceptor moves on to finalization. + framed + .write_all(&encode_send_data_request( + user_channel_id, + io_channel_id, + &CLIENT_DEMAND_ACTIVE_PDU_BUFFER, + )) + .await + .unwrap(); + + // Finalization: Synchronize, Control Cooperate, Control Request Control, + // Font List, each accepted without a response until Font List lands. + for pdu_bytes in [ + &CLIENT_SYNCHRONIZE_BUFFER[..], + &CONTROL_COOPERATE_BUFFER[..], + &CONTROL_REQUEST_CONTROL_BUFFER[..], + &CLIENT_FONT_LIST_BUFFER[..], + ] { + framed + .write_all(&encode_send_data_request(user_channel_id, io_channel_id, pdu_bytes)) + .await + .unwrap(); + } + + // Four finalization responses, all on the I/O channel: Synchronize + // Confirm, Control Cooperate Confirm, Control Granted Confirm, Font Map. + for _ in 0..4 { + let _ = read_pdu(&mut framed).await; + } + + (framed, user_channel_id, io_channel_id) +} + +/// Plays a Deactivation-Reactivation round: reads the fresh Demand Active +/// (`new_deactivation_reactivation` rebuilds the acceptor straight into +/// `CapabilitiesSendServer`, skipping channel join, licensing, and +/// multitransport bootstrapping entirely, per its own doc comment), then +/// redoes Confirm Active and the finalization exchange. No multitransport +/// traffic is expected this round. +async fn play_reactivation_round(mut framed: TokioFramed, user_channel_id: u16, io_channel_id: u16) { + let _demand_active = read_pdu(&mut framed).await; + + framed + .write_all(&encode_send_data_request( + user_channel_id, + io_channel_id, + &CLIENT_DEMAND_ACTIVE_PDU_BUFFER, + )) + .await + .unwrap(); + + for pdu_bytes in [ + &CLIENT_SYNCHRONIZE_BUFFER[..], + &CONTROL_COOPERATE_BUFFER[..], + &CONTROL_REQUEST_CONTROL_BUFFER[..], + &CLIENT_FONT_LIST_BUFFER[..], + ] { + framed + .write_all(&encode_send_data_request(user_channel_id, io_channel_id, pdu_bytes)) + .await + .unwrap(); + } + + for _ in 0..4 { + let _ = read_pdu(&mut framed).await; + } +} + +/// The Initiate Multitransport Request must reach the caller through +/// `accept_finalize_with_multitransport`'s handler exactly once, with the +/// request the acceptor actually sent, and without the handler blocking the +/// rest of the handshake (which the client-side script above completes +/// concurrently). +#[tokio::test] +async fn multitransport_handler_fires_once_without_blocking_finalization() { + let (client_stream, server_stream) = tokio::io::duplex(65536); + + let handler_calls: Arc>> = Arc::new(Mutex::new(Vec::new())); + let handler_calls_for_server = Arc::clone(&handler_calls); + + // `accept_finalize_with_multitransport`'s generic handler closure is + // `!Send` for the same structural reason `RdpServer`'s own future is + // (see `finalize_timeout.rs`): a `LocalSet` is required rather than a + // plain `tokio::spawn`. + let local = tokio::task::LocalSet::new(); + local + .run_until(async move { + let server = tokio::task::spawn_local(async move { + let mut acceptor = Acceptor::new( + SecurityProtocol::empty(), + DesktopSize { + width: 1920, + height: 1080, + }, + Vec::new(), + None, + ); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let framed = TokioFramed::new(server_stream); + let (_, result) = accept_finalize_with_multitransport( + framed, + &mut acceptor, + |request: MultitransportRequestPdu, soft_sync: bool| { + let handler_calls = Arc::clone(&handler_calls_for_server); + async move { + handler_calls.lock().unwrap().push((request.request_id, soft_sync)); + } + }, + ) + .await + .expect("acceptor finalize with multitransport"); + + result + }); + + let client_framed = TokioFramed::new(client_stream); + let (_client_framed, _user_channel_id, _io_channel_id) = play_client(client_framed).await; + + let result = server.await.expect("server task panicked"); + assert_eq!(result.static_channels.len(), 0); + + let calls = handler_calls.lock().unwrap(); + assert_eq!(calls.len(), 1, "handler must fire exactly once"); + assert!( + !calls[0].1, + "soft-sync was not offered, so it must not be reported negotiated" + ); + }) + .await; +} + +/// A Deactivation-Reactivation Sequence rebuilds the acceptor via +/// `Acceptor::new_deactivation_reactivation()`, which carries the original +/// Initiate Multitransport Request forward without running bootstrapping +/// again. The handler must not fire a second time when the rebuilt acceptor +/// is driven through `accept_finalize_with_multitransport` for that +/// reactivation round. +#[tokio::test] +async fn multitransport_handler_does_not_fire_again_after_reactivation() { + let (client_stream, server_stream) = tokio::io::duplex(65536); + + let handler_calls: Arc>> = Arc::new(Mutex::new(Vec::new())); + let handler_calls_for_server = Arc::clone(&handler_calls); + + let local = tokio::task::LocalSet::new(); + local + .run_until(async move { + let server = tokio::task::spawn_local(async move { + let desktop_size = DesktopSize { + width: 1920, + height: 1080, + }; + let mut acceptor = Acceptor::new(SecurityProtocol::empty(), desktop_size, Vec::new(), None); + acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); + + let handler = { + let handler_calls = Arc::clone(&handler_calls_for_server); + move |request: MultitransportRequestPdu, soft_sync: bool| { + let handler_calls = Arc::clone(&handler_calls); + async move { + handler_calls.lock().unwrap().push((request.request_id, soft_sync)); + } + } + }; + + let framed = TokioFramed::new(server_stream); + let (framed, result) = accept_finalize_with_multitransport(framed, &mut acceptor, handler.clone()) + .await + .expect("acceptor finalize with multitransport (first round)"); + + let mut acceptor = + Acceptor::new_deactivation_reactivation(acceptor, result.static_channels, desktop_size) + .expect("rebuild acceptor for reactivation"); + + let (_, result) = accept_finalize_with_multitransport(framed, &mut acceptor, handler) + .await + .expect("acceptor finalize with multitransport (reactivation round)"); + + result + }); + + let client_framed = TokioFramed::new(client_stream); + let (client_framed, user_channel_id, io_channel_id) = play_client(client_framed).await; + play_reactivation_round(client_framed, user_channel_id, io_channel_id).await; + + let result = server.await.expect("server task panicked"); + assert_eq!(result.static_channels.len(), 0); + + let calls = handler_calls.lock().unwrap(); + assert_eq!( + calls.len(), + 1, + "handler must not fire again for a reactivation that carries the same request forward" + ); + }) + .await; +} From b92ebf9cceb51d1c0b697ab171521cb738833e6b Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 18:27:22 -0500 Subject: [PATCH 2/4] review: address automated review findings on the finalize driver tests Four of the eight findings were already resolved by the rebase onto #1951's own review-response commit: the late-response tolerance now applies uniformly to every FinalizationSequence sub-state, the server MultiTransportChannelData block is filtered on the client's own block presence, and two stale doc comments were already corrected. For the remaining four, deduplicated the MCS SendDataRequest encoder and the client GCC-block builder between acceptor.rs and multitransport_finalize.rs (both made pub(super) and reused), extracted a shared play_confirm_active_and_finalization helper covering the Confirm Active plus four-PDU finalization exchange that play_client and play_reactivation_round both repeated, and extracted a recording_handler factory removing duplicated Arc::clone-into-closure plumbing across the two handler tests. play_client now also returns the request_id it decoded so the first handler test can assert the handler received the exact request the acceptor sent, rather than recording it unread. Added the missing assertion to multitransport_not_offered_by_default, whose doc comment claimed the server's GCC advertisement was checked when nothing actually was. --- .../tests/server/acceptor.rs | 10 +- .../tests/server/multitransport_finalize.rs | 111 ++++++++---------- 2 files changed, 54 insertions(+), 67 deletions(-) diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index 4d1382d3dd..ef0939cf4b 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -199,7 +199,7 @@ fn neg_failure_hybrid_required() { } } -fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { +pub(super) fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { let mut buf = WriteBuf::new(); ironrdp_core::encode_buf( &X224(mcs::SendDataRequest { @@ -315,7 +315,7 @@ fn drive_to_secure_settings_exchange( ) } -fn client_gcc_with_message_channel_and_multitransport( +pub(super) fn client_gcc_with_message_channel_and_multitransport( offer: Option, ) -> ironrdp_pdu::gcc::ClientGccBlocks { let mut blocks = CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS.clone(); @@ -641,7 +641,11 @@ fn multitransport_not_offered_by_default() { let client_blocks = client_gcc_with_message_channel_and_multitransport(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); - drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + let (.., server_multitransport) = drive_to_secure_settings_exchange(&mut acceptor, client_blocks); + assert_eq!( + server_multitransport, None, + "server must not advertise MultiTransportChannelData when multitransport is disabled" + ); acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // LicensingExchange let written = acceptor.step(&[], None, &mut WriteBuf::new()).unwrap(); // MultitransportBootstrapping diff --git a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs index cfed9a770c..89fee54831 100644 --- a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs +++ b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs @@ -5,19 +5,17 @@ //! exercises the acceptor's actual async driver rather than only the //! synchronous `Acceptor::step()` surface already covered in `acceptor.rs`. -use std::borrow::Cow; use std::sync::{Arc, Mutex}; use ironrdp_acceptor::{Acceptor, accept_finalize_with_multitransport}; use ironrdp_async::FramedWrite as _; use ironrdp_connector::DesktopSize; use ironrdp_core::{WriteBuf, decode, encode_buf, encode_vec}; -use ironrdp_pdu::gcc::{ClientMessageChannelData, MultiTransportChannelData, MultiTransportFlags}; +use ironrdp_pdu::gcc::MultiTransportFlags; use ironrdp_pdu::mcs::{self, ConnectInitial}; use ironrdp_pdu::nego::{self, SecurityProtocol}; use ironrdp_pdu::rdp::multitransport::{MultitransportRequestPdu, MultitransportResponsePdu, RequestedProtocol}; use ironrdp_pdu::x224::{X224, X224Data}; -use ironrdp_testsuite_core::gcc::CLIENT_GCC_WITHOUT_OPTIONAL_FIELDS; use ironrdp_testsuite_core::rdp::{ CLIENT_DEMAND_ACTIVE_PDU_BUFFER, CLIENT_FONT_LIST_BUFFER, CLIENT_INFO_PDU_BUFFER, CLIENT_SYNCHRONIZE_BUFFER, CONTROL_COOPERATE_BUFFER, CONTROL_REQUEST_CONTROL_BUFFER, @@ -25,20 +23,14 @@ use ironrdp_testsuite_core::rdp::{ use ironrdp_tokio::TokioFramed; use tokio::io::DuplexStream; +use super::acceptor::{client_gcc_with_message_channel_and_multitransport, encode_send_data_request}; + fn encode_x224_pdu<'a, T: ironrdp_pdu::x224::X224Pdu<'a>>(pdu: T) -> Vec { let mut buf = WriteBuf::new(); encode_buf(&X224(pdu), &mut buf).unwrap(); buf.filled().to_vec() } -fn encode_send_data_request(initiator_id: u16, channel_id: u16, user_data: &[u8]) -> Vec { - encode_x224_pdu(mcs::SendDataRequest { - initiator_id, - channel_id, - user_data: Cow::Borrowed(user_data), - }) -} - /// Reads one PDU from `framed` and returns its raw bytes (TPKT/X224 header /// stripped, matching what `decode` in the rest of this file expects). async fn read_pdu(framed: &mut TokioFramed) -> Vec { @@ -51,9 +43,11 @@ async fn read_pdu(framed: &mut TokioFramed) -> Vec { /// reliable UDP multitransport. Sends a (soft-sync-less) Initiate /// Multitransport Response before the Confirm Active, which the acceptor /// must tolerate rather than error on. Returns the same stream, still open, -/// plus the user and I/O channel IDs, for a caller that wants to keep -/// driving it (e.g. through reactivation). -async fn play_client(mut framed: TokioFramed) -> (TokioFramed, u16, u16) { +/// the user and I/O channel IDs, for a caller that wants to keep driving it +/// (e.g. through reactivation), and the request_id the acceptor's Initiate +/// Multitransport Request carried, for a caller that wants to cross-check +/// it against what a handler observed. +async fn play_client(mut framed: TokioFramed) -> (TokioFramed, u16, u16, u32) { // Connection Request / Confirm. Plain RDP security (empty protocol) so no // TLS upgrade is needed and the acceptor drives straight through. let request = nego::ConnectionRequest { @@ -67,12 +61,7 @@ async fn play_client(mut framed: TokioFramed) -> (TokioFramed) -> (TokioFramed, + user_channel_id: u16, + io_channel_id: u16, +) { framed .write_all(&encode_send_data_request( user_channel_id, @@ -185,10 +189,8 @@ async fn play_client(mut framed: TokioFramed) -> (TokioFramed) -> (TokioFramed, user_channel_id: u16, io_channel_id: u16) { let _demand_active = read_pdu(&mut framed).await; - framed - .write_all(&encode_send_data_request( - user_channel_id, - io_channel_id, - &CLIENT_DEMAND_ACTIVE_PDU_BUFFER, - )) - .await - .unwrap(); - - for pdu_bytes in [ - &CLIENT_SYNCHRONIZE_BUFFER[..], - &CONTROL_COOPERATE_BUFFER[..], - &CONTROL_REQUEST_CONTROL_BUFFER[..], - &CLIENT_FONT_LIST_BUFFER[..], - ] { - framed - .write_all(&encode_send_data_request(user_channel_id, io_channel_id, pdu_bytes)) - .await - .unwrap(); - } - - for _ in 0..4 { - let _ = read_pdu(&mut framed).await; - } + play_confirm_active_and_finalization(&mut framed, user_channel_id, io_channel_id).await; } /// The Initiate Multitransport Request must reach the caller through @@ -261,12 +240,7 @@ async fn multitransport_handler_fires_once_without_blocking_finalization() { let (_, result) = accept_finalize_with_multitransport( framed, &mut acceptor, - |request: MultitransportRequestPdu, soft_sync: bool| { - let handler_calls = Arc::clone(&handler_calls_for_server); - async move { - handler_calls.lock().unwrap().push((request.request_id, soft_sync)); - } - }, + recording_handler(handler_calls_for_server), ) .await .expect("acceptor finalize with multitransport"); @@ -275,13 +249,17 @@ async fn multitransport_handler_fires_once_without_blocking_finalization() { }); let client_framed = TokioFramed::new(client_stream); - let (_client_framed, _user_channel_id, _io_channel_id) = play_client(client_framed).await; + let (_client_framed, _user_channel_id, _io_channel_id, request_id) = play_client(client_framed).await; let result = server.await.expect("server task panicked"); assert_eq!(result.static_channels.len(), 0); let calls = handler_calls.lock().unwrap(); assert_eq!(calls.len(), 1, "handler must fire exactly once"); + assert_eq!( + calls[0].0, request_id, + "handler must receive the exact request the acceptor sent" + ); assert!( !calls[0].1, "soft-sync was not offered, so it must not be reported negotiated" @@ -290,6 +268,19 @@ async fn multitransport_handler_fires_once_without_blocking_finalization() { .await; } +/// Builds a handler for `accept_finalize_with_multitransport` that records +/// every `(request_id, soft_sync)` call into `calls`. Shared by both tests +/// below, which otherwise duplicated the identical `Arc::clone`-into-closure +/// plumbing around an inline handler. +fn recording_handler(calls: Arc>>) -> impl AsyncFnMut(MultitransportRequestPdu, bool) + Clone { + move |request: MultitransportRequestPdu, soft_sync: bool| { + let calls = Arc::clone(&calls); + async move { + calls.lock().unwrap().push((request.request_id, soft_sync)); + } + } +} + /// A Deactivation-Reactivation Sequence rebuilds the acceptor via /// `Acceptor::new_deactivation_reactivation()`, which carries the original /// Initiate Multitransport Request forward without running bootstrapping @@ -314,15 +305,7 @@ async fn multitransport_handler_does_not_fire_again_after_reactivation() { let mut acceptor = Acceptor::new(SecurityProtocol::empty(), desktop_size, Vec::new(), None); acceptor.set_multitransport_offer(Some(MultiTransportFlags::TRANSPORT_TYPE_UDP_FECR)); - let handler = { - let handler_calls = Arc::clone(&handler_calls_for_server); - move |request: MultitransportRequestPdu, soft_sync: bool| { - let handler_calls = Arc::clone(&handler_calls); - async move { - handler_calls.lock().unwrap().push((request.request_id, soft_sync)); - } - } - }; + let handler = recording_handler(handler_calls_for_server); let framed = TokioFramed::new(server_stream); let (framed, result) = accept_finalize_with_multitransport(framed, &mut acceptor, handler.clone()) @@ -341,7 +324,7 @@ async fn multitransport_handler_does_not_fire_again_after_reactivation() { }); let client_framed = TokioFramed::new(client_stream); - let (client_framed, user_channel_id, io_channel_id) = play_client(client_framed).await; + let (client_framed, user_channel_id, io_channel_id, _request_id) = play_client(client_framed).await; play_reactivation_round(client_framed, user_channel_id, io_channel_id).await; let result = server.await.expect("server task panicked"); From 95b088c67847c0be2fd857e49bb248f87720cac0 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 23:35:45 -0500 Subject: [PATCH 3/4] review: address automated review findings on the finalize driver - Document that USER_CHANNEL_ID doubles as the fixed MCS server channel ID (MS-RDPBCGR 3.3.1.5) that every server-to-client Send Data Indication in this file relies on. - Fix the finalize integration test to send an abort response instead of S_OK when SOFTSYNC_TCP_TO_UDP was not negotiated (MS-RDPBCGR 2.2.15.2), matching the existing pattern in acceptor.rs. - Rename late_multitransport_response to is_late_multitransport_response and return bool instead of an Option neither caller reads. - Simplify multitransport_acceptor to pass its Option straight through to set_multitransport_offer instead of re-wrapping it. --- crates/ironrdp-acceptor/src/connection.rs | 31 ++++++++++--------- .../tests/server/acceptor.rs | 8 ++--- .../tests/server/multitransport_finalize.rs | 5 ++- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index d3f090ca25..c8d8979958 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -24,6 +24,10 @@ use super::finalization::FinalizationSequence; use crate::util::{self, wrap_share_data}; const IO_CHANNEL_ID: u16 = 1003; +// Also the fixed MCS server channel ID (0x03EA) that MS-RDPBCGR 3.3.1.5 defines, +// which is why it doubles as the `initiator` on every server-to-client Send Data +// Indication in this file (License, Demand Active, Initiate Multitransport Request +// per 3.3.5.15.1) rather than the user's own MCS channel ID. const USER_CHANNEL_ID: u16 = 1002; pub struct Acceptor { @@ -381,13 +385,12 @@ impl Acceptor { /// handling for anything that isn't really a response, mirroring how /// `ClientConnectorState::ConnectTimeAutoDetection` demuxes the same /// channel client-side. - fn late_multitransport_response( - &self, - data: &mcs::SendDataRequest<'_>, - ) -> Option { - let sent = self.sent_multitransport_request.as_ref()?; + fn is_late_multitransport_response(&self, data: &mcs::SendDataRequest<'_>) -> bool { + let Some(sent) = self.sent_multitransport_request.as_ref() else { + return false; + }; if Some(data.channel_id) != self.message_channel_id { - return None; + return false; } // `decode` alone would accept a valid response followed by trailing // bytes. This acceptor advertises ENCRYPTION_LEVEL_NONE, so the @@ -395,9 +398,11 @@ impl Acceptor { // 2.2.15.2) and is exactly 12 bytes: anything left over means the // payload is something else. let mut cursor = ReadCursor::new(data.user_data.as_ref()); - let response = decode_cursor::(&mut cursor).ok()?; + let Ok(response) = decode_cursor::(&mut cursor) else { + return false; + }; if !cursor.is_empty() { - return None; + return false; } if response.request_id == sent.request_id { debug!( @@ -412,7 +417,7 @@ impl Acceptor { "Initiate Multitransport Response request ID does not match the sent request" ); } - Some(response) + true } pub fn new_deactivation_reactivation( @@ -1274,7 +1279,7 @@ impl Sequence for Acceptor { // response at all (Auto-Detect Response, Heartbeat), so it // falls through to the Confirm Active handling below // instead. - if self.late_multitransport_response(&data).is_some() { + if self.is_late_multitransport_response(&data) { self.state = prev_state; return Ok(Written::Nothing); } @@ -1329,7 +1334,7 @@ impl Sequence for Acceptor { client_capabilities, } => { // A late Initiate Multitransport Response can land in any - // finalization sub-state (see `late_multitransport_response`); + // finalization sub-state (see `is_late_multitransport_response`); // none of FinalizationSequence's own PDU decoders expect it, and // depending which sub-state is active it would otherwise be // silently swallowed while advancing a state, propagated as a @@ -1338,9 +1343,7 @@ impl Sequence for Acceptor { // finalization ever sees the bytes, mirroring // `CapabilitiesWaitConfirm`'s handling. let is_late_multitransport_response = match decode::>>(input) { - Ok(X224(mcs::McsMessage::SendDataRequest(data))) => { - self.late_multitransport_response(&data).is_some() - } + Ok(X224(mcs::McsMessage::SendDataRequest(data))) => self.is_late_multitransport_response(&data), _ => false, }; diff --git a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs index ef0939cf4b..e41c342fe8 100644 --- a/crates/ironrdp-testsuite-core/tests/server/acceptor.rs +++ b/crates/ironrdp-testsuite-core/tests/server/acceptor.rs @@ -327,8 +327,8 @@ pub(super) fn client_gcc_with_message_channel_and_multitransport( /// Builds an `Acceptor` for the multitransport tests below, at a common /// 1920x1080 desktop size with no static channels or credentials. `offer` is -/// passed to `set_multitransport_offer` when `Some`; pass `None` to exercise -/// the default-disabled path. +/// passed straight through to `set_multitransport_offer`; pass `None` to +/// exercise the default-disabled path. fn multitransport_acceptor(offer: Option) -> Acceptor { let mut acceptor = Acceptor::new( SecurityProtocol::SSL, @@ -339,9 +339,7 @@ fn multitransport_acceptor(offer: Option) -> Acceptor { Vec::new(), None, ); - if let Some(offer) = offer { - acceptor.set_multitransport_offer(Some(offer)); - } + acceptor.set_multitransport_offer(offer); acceptor } diff --git a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs index 89fee54831..71ad058a7e 100644 --- a/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs +++ b/crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs @@ -133,7 +133,10 @@ async fn play_client(mut framed: TokioFramed) -> (TokioFramed Date: Tue, 22 Sep 2026 17:50:43 -0500 Subject: [PATCH 4/4] review: describe is_late_multitransport_response's bool result The doc comment still described the earlier Option-returning shape ("logs it ... and returns it"); it now states what true and false mean for the caller. --- crates/ironrdp-acceptor/src/connection.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/ironrdp-acceptor/src/connection.rs b/crates/ironrdp-acceptor/src/connection.rs index c8d8979958..e11b47cdf9 100644 --- a/crates/ironrdp-acceptor/src/connection.rs +++ b/crates/ironrdp-acceptor/src/connection.rs @@ -366,14 +366,15 @@ impl Acceptor { ) } - /// If `data` (an MCS SendDataRequest already decoded from the wire) is on - /// the message channel while a multitransport request is outstanding AND - /// its payload strictly decodes as an Initiate Multitransport Response, - /// logs it against the outstanding request (matching request IDs) and - /// returns it. MS-RDPBCGR 3.2.5.15.1 gives this response no fixed - /// position relative to the rest of the handshake: it depends on when - /// the client resolves its own bootstrapping and whether the sideband - /// attempt failed, so both `CapabilitiesWaitConfirm` and + /// Returns `true` if `data` (an MCS SendDataRequest already decoded from + /// the wire) is on the message channel while a multitransport request is + /// outstanding AND its payload strictly decodes as an Initiate + /// Multitransport Response, after logging the response against the + /// outstanding request (matching request IDs); `false` otherwise, leaving + /// the caller to handle `data`. MS-RDPBCGR 3.2.5.15.1 gives this response + /// no fixed position relative to the rest of the handshake: it depends on + /// when the client resolves its own bootstrapping and whether the + /// sideband attempt failed, so both `CapabilitiesWaitConfirm` and /// `ConnectionFinalization` tolerate it landing wherever it actually /// shows up rather than only where `MultitransportBootstrapping`'s own /// comment describes as typical.