feat(server): add accept_finalize_with_multitransport driver by glamberson · Pull Request #1953 · Devolutions/IronRDP

Greg Lamberson (glamberson) · 2026-09-10T21:26:24Z

Depends on #1951

This PR is stacked on #1951 (feat/acceptor-multitransport-bootstrapping). GitHub does not support cross-fork stacked PRs, so this diff is filed against master and is therefore cumulative with #1951's; the incremental diff is at lamco-admin/IronRDP/compare/feat/acceptor-multitransport-bootstrapping...feat/acceptor-multitransport-finalize.

Summary

Add accept_finalize_with_multitransport, an async driver mirroring the client side's connect_finalize_with_multitransport.
It drives the acceptor sequence to completion exactly like 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 it runs.
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.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass.

Added two integration tests to ironrdp-testsuite-core/tests/server/multitransport_finalize.rs, 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 without blocking the handshake, and it does not fire again across a Deactivation-Reactivation round. Writing the first test caught a real bug in the initial implementation (a take_multitransport_request() that cleared the same field CapabilitiesWaitConfirm's response tolerance depends on); fixed before this PR's only commit.

Notes

The driver seeds its "already notified" tracking from whatever request is already present rather than false, since new_deactivation_reactivation carries the original request forward without re-running bootstrapping.

One PR is stacked on this

#1954 (ironrdp-server wiring, consuming accept_finalize_with_multitransport) is stacked on this branch. Its diff is filed against master and is cumulative with this one and #1951; see its own body for the incremental compare.

Add a MultitransportBootstrapping state to the acceptor sequence, entered right after licensing. It advertises UDP multitransport support in the GCC Server MultiTransportChannelData block when configured (set_multitransport_offer), and, once the client reciprocates the reliable-UDP flag, sends the Initiate Multitransport Request (MS-RDPBCGR 2.2.15.1) on the MCS message channel before moving straight on to capability negotiation. The acceptor does not wait for the client's Initiate Multitransport Response before continuing: MS-RDPBCGR 3.2.5.15.1 only obliges the client to send one when Soft-Sync is negotiated or the sideband attempt failed, so blocking on it would stall the handshake on the common successful path. multitransport_request() surfaces the sent request so the caller can establish the sideband UDP transport in parallel. A response that does arrive lands before the mandatory Confirm Active (the client sends it, if at all, before it ever reads Demand Active), so CapabilitiesWaitConfirm recognizes and drops it by channel rather than erroring on the unexpected payload. Adds ironrdp-testsuite-core coverage for the offer/no-offer/no-client- support paths and for the response-before-Confirm-Active ordering.

github-actions · 2026-09-10T21:36:00Z

Potential duplicate detected: #1951.

PR #1951 describes the identical feature: a MultitransportBootstrapping acceptor state after licensing, set_multitransport_offer config, GCC Server MultiTransportChannelData advertisement, Initiate Multitransport Request on the MCS message channel, and tests in tests/server/acceptor.rs. This diff matches point-for-point under a different head SHA.

Maintainer review is required.

Depends on Devolutions#1953 (stacked branch, feat/acceptor-multitransport-finalize). Add RdpServerBuilder::with_udp_transport(udp_bind_addr), opt-in and None by default. When set (and the security mode is Tls or Hybrid, matching the reference client's Enhanced-Security-only gate), the acceptor offers reliable UDP multitransport, and accept_finalize uses accept_finalize_with_multitransport with a callback that binds a fresh UDP socket per connection, reuses the connection's own TLS certificate (TlsAcceptor::config()) for the sideband transport, and calls accept_udp(). Any failure at any stage falls back to TCP-only, never fails the connection. Once established, the transport is used to migrate EGFX graphics traffic off TCP: request_reliable_udp is called opportunistically the first time EGFX has data to send (its dynamic channel id is only known once the client opens it, well after multitransport bootstrapping), and outgoing EGFX SvcMessages route onto the tunnel via encode_unframed_pdu() once the client has acknowledged the Soft-Sync request. A new client_loop select arm feeds incoming tunnel payloads into DrdynvcServer::process_tunnel(), whose responses go back over TCP. A closed transport degrades the arm to pending forever rather than ending the session, matching the non-fatal posture throughout. Adds a regression test verifying that configuring UDP transport on the server does not disturb a client that never advertises support for it (the common case for any client that predates this feature).

github-actions

Adds opt-in server-side UDP multitransport bootstrapping: a post-licensing MultitransportBootstrapping state that sends an Initiate Multitransport Request on the MCS message channel when configured and reciprocated, GCC advertisement of Server MultiTransportChannelData, tolerance for a late response in CapabilitiesWaitConfirm, and an accept_finalize_with_multitransport driver mirroring the client side, plus sync and duplex-stream tests. The design is coherent, defaults preserve existing behavior, and the renotification seeding for reactivation is correct. Published findings: an unverified response-ordering assumption on the mandatory soft-sync path (raised as a question), advertisement not gated on the client's block per MS-RDPBCGR 2.2.1.4.6, a stale doc claim about reactivation, and test-layer duplication/coverage gaps.

The Server MultiTransportChannelData block was advertised whenever the server's own offer was configured, regardless of whether the client actually populated its own Client MultiTransportChannelData block. MS-RDPBCGR 2.2.1.4 requires the server block to be omitted when the client did not send one. Added a client_offered_multitransport field tracking presence separately from multitransport_flags, which alone can't distinguish "absent" from "present but empty", and gated the offer on it. The Initiate Multitransport Response has no fixed position relative to the rest of the handshake (3.2.5.15.1): a conforming client can send it after Confirm Active, during ConnectionFinalization, not only before it. None of FinalizationSequence's own PDU decoders expect it, so depending which sub-state was active a late response was silently swallowed while advancing a state, propagated as a connection-ending decode error, or surfaced to the embedding application as a raw input event. Added the same tolerance CapabilitiesWaitConfirm already had to ConnectionFinalization. The late-response guard itself only checked channel and outstanding- request, not whether the payload actually decoded as a response. Since the message channel also carries Auto-Detect Response and Heartbeat PDUs (2.2.1.4.5, 2.2.8.1.1.2.1), that traffic was misclassified and dropped instead of falling through to its own handling. The guard now requires a successful strict decode, mirroring how ClientConnectorState::ConnectTimeAutoDetection demuxes the same channel client-side. Also: corrected the multitransport_request() doc, which claimed it returns None on reactivation when the carried-forward request in fact keeps it Some (intentional, needed for the late-response guard to keep working across reactivation); simplified an Option<u32> round-trip in the response-logging path down to a direct comparison, since the calling guard already guarantees a request is outstanding; and fixed two test cases constructing an S_OK response without the server advertising Soft-Sync, which 2.2.15.2 disallows. Regression tests added for the finalization tolerance and the non-response message-channel traffic case; both verified to fail against the prior behavior and pass with the fix.

…ping Documents the interim limitation of set_multitransport_offer: this acceptor only sends the request, it does not establish the sideband UDP transport itself. Marks AcceptorState non_exhaustive, matching ClientConnectorState's convention. Changes multitransport_soft_sync_negotiated to return Option<bool>, None before a request was actually sent, rather than deriving from GCC flags alone which could report true with nothing sent. Replaces the client_offered_multitransport bool with a single multitransport_flags: Option<MultiTransportFlags> field, removing the duplicated absent-vs-empty distinction. Merges log_multitransport_response into late_multitransport_response, removing the panic-prone two-step coupling, and folds the CapabilitiesWaitConfirm pre-check into the main match arm. Adds a multitransport_acceptor(offer) test factory, removing repeated setup across four tests.

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.

Four of the eight findings were already resolved by the rebase onto Devolutions#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.

Depends on Devolutions#1953 (stacked branch, feat/acceptor-multitransport-finalize). Add RdpServerBuilder::with_udp_transport(udp_bind_addr), opt-in and None by default. When set (and the security mode is Tls or Hybrid, matching the reference client's Enhanced-Security-only gate), the acceptor offers reliable UDP multitransport, and accept_finalize uses accept_finalize_with_multitransport with a callback that binds a fresh UDP socket per connection, reuses the connection's own TLS certificate (TlsAcceptor::config()) for the sideband transport, and calls accept_udp(). Any failure at any stage falls back to TCP-only, never fails the connection. Once established, the transport is used to migrate EGFX graphics traffic off TCP: request_reliable_udp is called opportunistically the first time EGFX has data to send (its dynamic channel id is only known once the client opens it, well after multitransport bootstrapping), and outgoing EGFX SvcMessages route onto the tunnel via encode_unframed_pdu() once the client has acknowledged the Soft-Sync request. A new client_loop select arm feeds incoming tunnel payloads into DrdynvcServer::process_tunnel(), whose responses go back over TCP. A closed transport degrades the arm to pending forever rather than ending the session, matching the non-fatal posture throughout. Adds a regression test verifying that configuring UDP transport on the server does not disturb a client that never advertises support for it (the common case for any client that predates this feature).

github-actions · 2026-09-11T23:39:13Z

This pull request may overlap with #1951.

Both PRs cover the same acceptor-side scope: a MultitransportBootstrapping state after licensing, set_multitransport_offer() advertising the Server MultiTransportChannelData GCC block only when the client sent one (MS-RDPBCGR 2.2.1.4), an Initiate Multitransport Request on the MCS message channel (2.2.15.1), a multitransport_request() accessor, and acceptor tests in ironrdp-testsuite-core; this diff also adds accept_finalize_with_multitransport.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

Fixes three high-severity issues: Soft-Sync now requires both a negotiated SOFT_SYNC_TCP_TO_UDP flag and a successful Initiate Multitransport Response before migrating any channel, the shared UDP transport handle exposes a lock-free sender independent of its receive-side mutex, and the finalize handler no longer blocks the RDP handshake on the UDP accept, spawning it instead and picking it up opportunistically from client_loop's own select loop once it resolves. Fixes a medium-severity bug in ironrdp-dvc's Soft-Sync response handling: a declined channel stayed routed for outgoing data because the outgoing tunnel map was never filtered by the response, only the incoming one. Addresses four low-severity findings: corrects a false single-connection premise in the UDP accept doc comment, documents the AddrInUse tradeoff under session preemption, combines a duplicated drdynvc guard into one failure path, and confirms two findings already resolved by rebasing onto PR Devolutions#1953's own review-response commit.

github-actions

Adds acceptor-side UDP multitransport bootstrapping: a Server MultiTransportChannelData block gated on the client's own block plus message channel, a MultitransportBootstrapping state sending the Initiate Multitransport Request after licensing, late-response tolerance in CapabilitiesWaitConfirm and ConnectionFinalization, and an accept_finalize_with_multitransport driver notifying a handler once per request, with sync and async integration tests. The implementation is sound: the guard's strict TRANSPORT_RSP decode rules out misclassifying auto-detect/heartbeat traffic, the handler-notified seeding is correct across reactivation, and the GCC block gating matches MS-RDPBCGR 2.2.1.4. Five low-severity findings published: a corrected initiator-channel maintainability nit, a spec-forbidden S_OK in the new async test, and three compression cleanups. No correctness defects.

[code-compressor] Finalization arm re-decodes input on every step before finalization decodes it again — low 🟡 — crates/ironrdp-acceptor/src/connection.rs
Every ConnectionFinalization step decodes the full X224<mcs::McsMessage> solely to test for a late multitransport response, then finalization.step re-decodes the same bytes internally. For connections where no multitransport request was ever sent (the default configuration) the pre-check is guaranteed false, yet the outstanding-request early-return in late_multitransport_response only runs after the outer decode succeeds, so every client PDU still pays a full TPKT/X.224/MCS parse. Minimal shape with no API churn: gate the pre-check on self.sent_multitransport_request.is_some() before decoding. Full decode deduplication would require widening the exported FinalizationSequence::step byte-slice contract and is not worth the negligible CPU.

- 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<PDU> neither caller reads. - Simplify multitransport_acceptor to pass its Option straight through to set_multitransport_offer instead of re-wrapping it.

Depends on Devolutions#1953 (stacked branch, feat/acceptor-multitransport-finalize). Add RdpServerBuilder::with_udp_transport(udp_bind_addr), opt-in and None by default. When set (and the security mode is Tls or Hybrid, matching the reference client's Enhanced-Security-only gate), the acceptor offers reliable UDP multitransport, and accept_finalize uses accept_finalize_with_multitransport with a callback that binds a fresh UDP socket per connection, reuses the connection's own TLS certificate (TlsAcceptor::config()) for the sideband transport, and calls accept_udp(). Any failure at any stage falls back to TCP-only, never fails the connection. Once established, the transport is used to migrate EGFX graphics traffic off TCP: request_reliable_udp is called opportunistically the first time EGFX has data to send (its dynamic channel id is only known once the client opens it, well after multitransport bootstrapping), and outgoing EGFX SvcMessages route onto the tunnel via encode_unframed_pdu() once the client has acknowledged the Soft-Sync request. A new client_loop select arm feeds incoming tunnel payloads into DrdynvcServer::process_tunnel(), whose responses go back over TCP. A closed transport degrades the arm to pending forever rather than ending the session, matching the non-fatal posture throughout. Adds a regression test verifying that configuring UDP transport on the server does not disturb a client that never advertises support for it (the common case for any client that predates this feature).

Fixes three high-severity issues: Soft-Sync now requires both a negotiated SOFT_SYNC_TCP_TO_UDP flag and a successful Initiate Multitransport Response before migrating any channel, the shared UDP transport handle exposes a lock-free sender independent of its receive-side mutex, and the finalize handler no longer blocks the RDP handshake on the UDP accept, spawning it instead and picking it up opportunistically from client_loop's own select loop once it resolves. Fixes a medium-severity bug in ironrdp-dvc's Soft-Sync response handling: a declined channel stayed routed for outgoing data because the outgoing tunnel map was never filtered by the response, only the incoming one. Addresses four low-severity findings: corrects a false single-connection premise in the UDP accept doc comment, documents the AddrInUse tradeoff under session preemption, combines a duplicated drdynvc guard into one failure path, and confirms two findings already resolved by rebasing onto PR Devolutions#1953's own review-response commit.

Fixes a high-severity bug: after the sideband UDP tunnel closes, the shared transport handle now gets cleared so dispatch_egfx_messages actually falls back to TCP instead of silently dropping every subsequent EGFX batch onto a dead connection. Documents an accepted timing limitation: a late Initiate Multitransport Response arriving after finalization completes cannot retroactively enable Soft-Sync migration, since nothing on the message channel recognizes it post-handoff. This degrades to TCP-only for the session rather than causing any correctness issue. Inherits the S_OK/SOFTSYNC test fix from PR Devolutions#1953 by rebasing onto its review-response commit, reconciling the resulting connection.rs conflict between that PR's bool-returning rename and this branch's own earlier &mut self change for response tracking. Addresses three low-severity findings: removes an unused accessor, substitutes an equivalent enum match with the existing tls_acceptor() helper, and reuses get_svc_processor() instead of inlining its body.

Greg Lamberson (glamberson) mentioned this pull request Sep 10, 2026

feat(server): add server-side UDP multitransport bootstrapping #1951

Open

Greg Lamberson (glamberson) deployed to llm-providers September 10, 2026 21:26 — with GitHub Actions Active

Greg Lamberson (glamberson) mentioned this pull request Sep 10, 2026

feat(server): wire UDP multitransport into ironrdp-server #1954

Open

github-actions Bot reviewed Sep 11, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed and removed maintainer-required Maintainer review or intervention is required labels Sep 11, 2026

Greg Lamberson (glamberson) added 3 commits September 11, 2026 14:38

Greg Lamberson (glamberson) force-pushed the feat/acceptor-multitransport-finalize branch from 7e7919d to e2925a7 Compare September 11, 2026 23:08

Greg Lamberson (glamberson) had a problem deploying to llm-providers September 11, 2026 23:09 — with GitHub Actions Error

Greg Lamberson (glamberson) deployed to llm-providers September 11, 2026 23:28 — with GitHub Actions Active

github-actions Bot reviewed Sep 12, 2026

View reviewed changes

Comment thread crates/ironrdp-acceptor/src/connection.rs

Comment thread crates/ironrdp-testsuite-core/tests/server/multitransport_finalize.rs Outdated

Comment thread crates/ironrdp-acceptor/src/connection.rs Outdated

Comment thread crates/ironrdp-testsuite-core/tests/server/acceptor.rs Outdated

github-actions Bot added ai-reviewed/2 Final automated review completed and removed ai-reviewed/1 One automated review completed labels Sep 12, 2026

Greg Lamberson (glamberson) deployed to llm-providers September 12, 2026 04:38 — with GitHub Actions Active

github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny and removed risk/medium Behavioral change that does not substantially alter a core public API labels Sep 12, 2026

Navigation Menu

feat(server): add accept_finalize_with_multitransport driver - #1953

feat(server): add accept_finalize_with_multitransport driver#1953
Greg Lamberson (glamberson) wants to merge 6 commits into
Devolutions:masterfrom
lamco-admin:feat/acceptor-multitransport-finalize

Greg Lamberson (glamberson) commented Sep 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 10, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Conversation

Greg Lamberson (glamberson) commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Depends on #1951

Summary

Validation

Notes

One PR is stacked on this

Uh oh!

github-actions Bot commented Sep 10, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Greg Lamberson (glamberson) commented Sep 10, 2026 •

edited

Loading