Repository navigation
fix(pdu)!: send NetworkAutoDetect over the MCS message channel - #1348
Benoît Cortier (CBenoit) merged 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR corrects Network Auto-Detect framing and routing to match MS-RDPBCGR by moving it off the I/O channel slow-path Share Data PDUs and onto the MCS message channel with the required Basic Security Header (SEC_AUTODETECT_REQ / SEC_AUTODETECT_RSP). This aligns IronRDP with mstsc/xfreerdp behavior and enables both connect-time and continuous auto-detection to actually function.
Changes:
- Add
AutoDetectReqPdu/AutoDetectRspPduwrappers (Basic Security Header framing) and remove the fabricated Share Data PDU representation for auto-detect. - Negotiate, join, and plumb the MCS message channel ID through acceptor/connector/session and use it for auto-detect traffic (client + server).
- Update and expand tests to cover message-channel negotiation and auto-detect request/response processing on that channel.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-testsuite-core/tests/session/autodetect.rs | Migrates session auto-detect tests to use message-channel framing and validates channel usage. |
| crates/ironrdp-testsuite-core/tests/server/acceptor.rs | Adds coverage ensuring the acceptor advertises Extended Client Data support and allocates a deterministic message channel ID. |
| crates/ironrdp-session/src/x224/mod.rs | Routes message-channel traffic to new auto-detect decoder/encoder path and stops handling auto-detect as Share Data. |
| crates/ironrdp-session/src/active_stage.rs | Wires negotiated message-channel ID into the session processor. |
| crates/ironrdp-server/src/server.rs | Sends auto-detect requests on message channel and decodes auto-detect responses from message channel. |
| crates/ironrdp-pdu/src/rdp/headers.rs | Removes auto-detect from ShareDataPdu / ShareDataPduType since it is not a real slow-path Share Data PDU. |
| crates/ironrdp-pdu/src/rdp/autodetect.rs | Introduces AutoDetectReqPdu / AutoDetectRspPdu with Basic Security Header framing + round-trip tests. |
| crates/ironrdp-connector/src/connection.rs | Requests message channel in GCC, joins it, enables connect-time auto-detect handling on the message channel, and propagates message_channel_id. |
| crates/ironrdp-acceptor/src/connection.rs | Negotiates and joins the message channel when requested and surfaces message_channel_id in AcceptorResult. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a7fd8d2 to
27c6446
Compare
There was a problem hiding this comment.
Thanks for chasing this down, the protocol diagnosis is right: 0x3b was never a real PDUTYPE2, and auto-detect belongs on the message channel behind a Basic Security Header. My one real concern is the connector connect-time state machine. Also I think the migrated tests cover the continuous (session) path well, but I don't see coverage for the connector connect-time branch specifically (the inlined-licensing path and the "ignore a non-autodetect message-channel PDU and keep listening" path). Those are the two riskiest branches in the PR. Could you add a test for that?
|
Added connector connect-time tests in a new tests/connector/autodetect.rs, covering the ConnectTimeAutoDetection demux you flagged: an Auto-Detect Request on the message channel is answered and the phase continues; an unrelated message-channel PDU is ignored and the phase continues; and a first licensing PDU on the I/O channel advances the connector out of auto-detection into the licensing path. That exercises the two branches you called out as riskiest plus the auto-detect path itself. The four inline points are addressed in their threads: the inlined licensing transition is now a shared helper so the terminal-state transition lives in one place; the message-channel input is decoded once; the next_pdu_hint gate carries a comment stating why it cannot stall; and the connect-time bandwidth no-op is now an explicit documented decision, with the Bandwidth Measure Results reply noted as a follow-up. I have also rebased the branch onto current master. |
27c6446 to
26b5c14
Compare
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
LGTM, thank you!
|
CI is broken because of the problem you already addressed in #1390 |
Head branch was pushed to by a user without write access
26b5c14 to
6ee4964
Compare
|
Yeah, no biggie. Rebased again, thanks. |
f5a50c0 to
86d8cc3
Compare
|
Benoît Cortier (@CBenoit) I'm just waiting on your action regarding merge order on this and #1435 to see how these sort out before looking at your refactoring work in Issue #1422 etc. further. Thanks. |
NetworkAutoDetect was modeled as a slow-path Share Data PDU (ShareDataPduType::AutoDetect, a fabricated 0x3B) carried on the I/O channel. Per [MS-RDPBCGR] 2.2.14.3 / 2.2.14.4 the auto-detect PDUs are framed by a Basic Security Header (SEC_AUTODETECT_REQ / SEC_AUTODETECT_RSP) and ride the MCS message channel, so neither mstsc nor xfreerdp answered the requests. ironrdp-pdu: add AutoDetectReqPdu / AutoDetectRspPdu, which wrap the request and response data with the security header (mirroring the multitransport PDUs). Remove the ShareDataPdu::AutoDetectReq / AutoDetectRsp variants and the ShareDataPduType::AutoDetect discriminant, which did not correspond to any real PDUTYPE2. ironrdp-server: send auto-detect requests and receive responses on the message channel surfaced by the acceptor, with the new framing. ironrdp-connector and ironrdp-session: the client now requests the message channel (Client Message Channel Data), advertises SUPPORT_NET_CHAR_AUTODETECT, joins the channel, and answers RTT requests on it. The connector handles connect-time auto-detection (previously a no-op) and the session handles continuous auto-detection. Resolves the client side of the message-channel TODO (Devolutions#140). Builds on the acceptor message-channel negotiation merged in Devolutions#1347 (efa5732).
86d8cc3 to
3c23737
Compare
|
Greg Lamberson (@glamberson) Thank you! As for #1422, I went ahead with phase 1 which was the most straightforward and easy win, but I don’t have time to go ahead with the other phases immediately. You can expect a significant cool-down period, during which I’m open to suggestions if anything looks off to you. And oops, I kind of lost track again, this PR was ready to be merged, going ahead now. Fortunately, it touches a strictly different set of API, so both changes are compatible. Just in time for the next release batch. |
8a1fd01
into
Devolutions:master
Addresses the protocol-correctness review on Devolutions#1098. Five findings, all in the client-side bootstrapping path. Soft-Sync is now negotiated rather than assumed. The server's GCC MultiTransportChannelData block was being warned about as "not supported" and discarded, so an Initiate Multitransport Response could carry S_OK to a server that never advertised SOFTSYNC_TCP_TO_UDP. Both peers' flags are checked, and the outcome is frozen into the state when it is entered. Per 2.2.15.2 the response is the Soft-Sync signalling path, so without it no response goes on the main channel at all and the outcome is reported in band on the new transport instead. One rule now covers both paths: with Soft-Sync always respond, without it never respond. Responses go on the MCS message channel, per 2.2.15.2 and 3.2.5.15.2, not the I/O channel where a Soft-Sync server would never see them. Inbound requests are validated against the same channel; the channel id was already being decoded and discarded. Same shape as Devolutions#1348 for NetworkAutoDetect. Declining now says so. Under Soft-Sync 3.2.5.15.1 requires a response whatever the outcome, and the skip path emitted nothing. Both the async and blocking drivers skip automatically, so every default client was leaving a compliant server waiting. It sends E_ABORT. Requests are surfaced on decode rather than held for a following PDU. There is no end marker for the set, and a server may send one request and then wait for the client to bring UDP up before sending Demand Active, which deadlocked against a connector waiting for a PDU that was not coming. 3.2.5.15.1 requires acting on decode. Responding returns to bootstrapping for whatever comes next. That reshape removes the buffered Demand Active and its replay, makes the API singular (complete_multitransport takes one result, multitransport_requests becomes multitransport_request), and lets the I/O channel route straight to capabilities exchange instead of being speculatively decoded as multitransport. A PDU on neither the message nor the I/O channel is now an explicit error rather than falling through to capabilities exchange. Finally, the response channel is resolved before the state is taken, so a failure there leaves the connector in MultitransportPending rather than Consumed, where the caller could neither inspect it nor decline. Not marked breaking: complete_multitransport and multitransport_request change shape, but neither has been released. They exist only inside this PR, so there is no published API for a consumer to migrate from.
Addresses the protocol-correctness review on Devolutions#1098. Five findings, all in the client-side bootstrapping path. Soft-Sync is now negotiated rather than assumed. The server's GCC MultiTransportChannelData block was being warned about as "not supported" and discarded, so an Initiate Multitransport Response could carry S_OK to a server that never advertised SOFTSYNC_TCP_TO_UDP. Both peers' flags are checked, and the outcome is frozen into the state when it is entered. Per 2.2.15.2 the response is the Soft-Sync signalling path, so without it no response goes on the main channel at all and the outcome is reported in band on the new transport instead. One rule now covers both paths: with Soft-Sync always respond, without it never respond. Responses go on the MCS message channel, per 2.2.15.2 and 3.2.5.15.2, not the I/O channel where a Soft-Sync server would never see them. Inbound requests are validated against the same channel; the channel id was already being decoded and discarded. Same shape as Devolutions#1348 for NetworkAutoDetect. Declining now says so. Under Soft-Sync 3.2.5.15.1 requires a response whatever the outcome, and the skip path emitted nothing. Both the async and blocking drivers skip automatically, so every default client was leaving a compliant server waiting. It sends E_ABORT. Requests are surfaced on decode rather than held for a following PDU. There is no end marker for the set, and a server may send one request and then wait for the client to bring UDP up before sending Demand Active, which deadlocked against a connector waiting for a PDU that was not coming. 3.2.5.15.1 requires acting on decode. Responding returns to bootstrapping for whatever comes next. That reshape removes the buffered Demand Active and its replay, makes the API singular (complete_multitransport takes one result, multitransport_requests becomes multitransport_request), and lets the I/O channel route straight to capabilities exchange instead of being speculatively decoded as multitransport. A PDU on neither the message nor the I/O channel is now an explicit error rather than falling through to capabilities exchange. Finally, the response channel is resolved before the state is taken, so a failure there leaves the connector in MultitransportPending rather than Consumed, where the caller could neither inspect it nor decline. Not marked breaking: complete_multitransport and multitransport_request change shape, but neither has been released. They exist only inside this PR, so there is no published API for a consumer to migrate from.
Addresses the protocol-correctness review on Devolutions#1098. Five findings, all in the client-side bootstrapping path. Soft-Sync is now negotiated rather than assumed. The server's GCC MultiTransportChannelData block was being warned about as "not supported" and discarded, so an Initiate Multitransport Response could carry S_OK to a server that never advertised SOFTSYNC_TCP_TO_UDP. Both peers' flags are checked, and the outcome is frozen into the state when it is entered. Per 2.2.15.2 the response is the Soft-Sync signalling path, so without it no response goes on the main channel at all and the outcome is reported in band on the new transport instead. One rule now covers both paths: with Soft-Sync always respond, without it never respond. Responses go on the MCS message channel, per 2.2.15.2 and 3.2.5.15.2, not the I/O channel where a Soft-Sync server would never see them. Inbound requests are validated against the same channel; the channel id was already being decoded and discarded. Same shape as Devolutions#1348 for NetworkAutoDetect. Declining now says so. Under Soft-Sync 3.2.5.15.1 requires a response whatever the outcome, and the skip path emitted nothing. Both the async and blocking drivers skip automatically, so every default client was leaving a compliant server waiting. It sends E_ABORT. Requests are surfaced on decode rather than held for a following PDU. There is no end marker for the set, and a server may send one request and then wait for the client to bring UDP up before sending Demand Active, which deadlocked against a connector waiting for a PDU that was not coming. 3.2.5.15.1 requires acting on decode. Responding returns to bootstrapping for whatever comes next. That reshape removes the buffered Demand Active and its replay, makes the API singular (complete_multitransport takes one result, multitransport_requests becomes multitransport_request), and lets the I/O channel route straight to capabilities exchange instead of being speculatively decoded as multitransport. A PDU on neither the message nor the I/O channel is now an explicit error rather than falling through to capabilities exchange. Finally, the response channel is resolved before the state is taken, so a failure there leaves the connector in MultitransportPending rather than Consumed, where the caller could neither inspect it nor decline. Not marked breaking: complete_multitransport and multitransport_request change shape, but neither has been released. They exist only inside this PR, so there is no published API for a consumer to migrate from.
Addresses the protocol-correctness review on Devolutions#1098. Five findings, all in the client-side bootstrapping path. Soft-Sync is now negotiated rather than assumed. The server's GCC MultiTransportChannelData block was being warned about as "not supported" and discarded, so an Initiate Multitransport Response could carry S_OK to a server that never advertised SOFTSYNC_TCP_TO_UDP. Both peers' flags are checked, and the outcome is frozen into the state when it is entered. Per 2.2.15.2 the response is the Soft-Sync signalling path, so without it no response goes on the main channel at all and the outcome is reported in band on the new transport instead. One rule now covers both paths: with Soft-Sync always respond, without it never respond. Responses go on the MCS message channel, per 2.2.15.2 and 3.2.5.15.2, not the I/O channel where a Soft-Sync server would never see them. Inbound requests are validated against the same channel; the channel id was already being decoded and discarded. Same shape as Devolutions#1348 for NetworkAutoDetect. Declining now says so. Under Soft-Sync 3.2.5.15.1 requires a response whatever the outcome, and the skip path emitted nothing. Both the async and blocking drivers skip automatically, so every default client was leaving a compliant server waiting. It sends E_ABORT. Requests are surfaced on decode rather than held for a following PDU. There is no end marker for the set, and a server may send one request and then wait for the client to bring UDP up before sending Demand Active, which deadlocked against a connector waiting for a PDU that was not coming. 3.2.5.15.1 requires acting on decode. Responding returns to bootstrapping for whatever comes next. That reshape removes the buffered Demand Active and its replay, makes the API singular (complete_multitransport takes one result, multitransport_requests becomes multitransport_request), and lets the I/O channel route straight to capabilities exchange instead of being speculatively decoded as multitransport. A PDU on neither the message nor the I/O channel is now an explicit error rather than falling through to capabilities exchange. Finally, the response channel is resolved before the state is taken, so a failure there leaves the connector in MultitransportPending rather than Consumed, where the caller could neither inspect it nor decline. Not marked breaking: complete_multitransport and multitransport_request change shape, but neither has been released. They exist only inside this PR, so there is no published API for a consumer to migrate from.
Addresses the protocol-correctness review on Devolutions#1098. Five findings, all in the client-side bootstrapping path. Soft-Sync is now negotiated rather than assumed. The server's GCC MultiTransportChannelData block was being warned about as "not supported" and discarded, so an Initiate Multitransport Response could carry S_OK to a server that never advertised SOFTSYNC_TCP_TO_UDP. Both peers' flags are checked, and the outcome is frozen into the state when it is entered. Per 2.2.15.2 the response is the Soft-Sync signalling path, so without it no response goes on the main channel at all and the outcome is reported in band on the new transport instead. One rule now covers both paths: with Soft-Sync always respond, without it never respond. Responses go on the MCS message channel, per 2.2.15.2 and 3.2.5.15.2, not the I/O channel where a Soft-Sync server would never see them. Inbound requests are validated against the same channel; the channel id was already being decoded and discarded. Same shape as Devolutions#1348 for NetworkAutoDetect. Declining now says so. Under Soft-Sync 3.2.5.15.1 requires a response whatever the outcome, and the skip path emitted nothing. Both the async and blocking drivers skip automatically, so every default client was leaving a compliant server waiting. It sends E_ABORT. Requests are surfaced on decode rather than held for a following PDU. There is no end marker for the set, and a server may send one request and then wait for the client to bring UDP up before sending Demand Active, which deadlocked against a connector waiting for a PDU that was not coming. 3.2.5.15.1 requires acting on decode. Responding returns to bootstrapping for whatever comes next. That reshape removes the buffered Demand Active and its replay, makes the API singular (complete_multitransport takes one result, multitransport_requests becomes multitransport_request), and lets the I/O channel route straight to capabilities exchange instead of being speculatively decoded as multitransport. A PDU on neither the message nor the I/O channel is now an explicit error rather than falling through to capabilities exchange. Finally, the response channel is resolved before the state is taken, so a failure there leaves the connector in MultitransportPending rather than Consumed, where the caller could neither inspect it nor decline. Not marked breaking: complete_multitransport and multitransport_request change shape, but neither has been released. They exist only inside this PR, so there is no published API for a consumer to migrate from.
Addresses the protocol-correctness review on Devolutions#1098. Five findings, all in the client-side bootstrapping path. Soft-Sync is now negotiated rather than assumed. The server's GCC MultiTransportChannelData block was being warned about as "not supported" and discarded, so an Initiate Multitransport Response could carry S_OK to a server that never advertised SOFTSYNC_TCP_TO_UDP. Both peers' flags are checked, and the outcome is frozen into the state when it is entered. Per 2.2.15.2 the response is the Soft-Sync signalling path, so without it no response goes on the main channel at all and the outcome is reported in band on the new transport instead. One rule now covers both paths: with Soft-Sync always respond, without it never respond. Responses go on the MCS message channel, per 2.2.15.2 and 3.2.5.15.2, not the I/O channel where a Soft-Sync server would never see them. Inbound requests are validated against the same channel; the channel id was already being decoded and discarded. Same shape as Devolutions#1348 for NetworkAutoDetect. Declining now says so. Under Soft-Sync 3.2.5.15.1 requires a response whatever the outcome, and the skip path emitted nothing. Both the async and blocking drivers skip automatically, so every default client was leaving a compliant server waiting. It sends E_ABORT. Requests are surfaced on decode rather than held for a following PDU. There is no end marker for the set, and a server may send one request and then wait for the client to bring UDP up before sending Demand Active, which deadlocked against a connector waiting for a PDU that was not coming. 3.2.5.15.1 requires acting on decode. Responding returns to bootstrapping for whatever comes next. That reshape removes the buffered Demand Active and its replay, makes the API singular (complete_multitransport takes one result, multitransport_requests becomes multitransport_request), and lets the I/O channel route straight to capabilities exchange instead of being speculatively decoded as multitransport. A PDU on neither the message nor the I/O channel is now an explicit error rather than falling through to capabilities exchange. Finally, the response channel is resolved before the state is taken, so a failure there leaves the connector in MultitransportPending rather than Consumed, where the caller could neither inspect it nor decline. Not marked breaking: complete_multitransport and multitransport_request change shape, but neither has been released. They exist only inside this PR, so there is no published API for a consumer to migrate from.
…1098) ## Summary Makes the `MultitransportBootstrapping` state functional instead of a no-op pass-through. After licensing the server may send 0, 1, or 2 Initiate Multitransport Request PDUs before capabilities exchange. Each one is surfaced to the application, which establishes UDP transport (RDPEUDP2 + TLS + RDPEMT) or declines, and the connector reports the outcome back to the server. ## API Mirrors the existing `should_perform_X()` pause-point pattern used by TLS upgrade and CredSSP, but uses `complete_X()` / `skip_X()` rather than `mark_X_as_done()` because completion carries result data: - `should_perform_multitransport()`: true while a request awaits an outcome - `multitransport_request()`: the request awaiting an outcome, or `None` - `complete_multitransport(result, output)`: report the outcome, resume - `skip_multitransport(output)`: decline, resume `complete_multitransport` accepts a `MultitransportResult` (a `Success` / `Failure(hresult)` enum) rather than a caller-built response PDU. The connector builds the response internally from the stored request ID. Requests are surfaced one at a time rather than as a batch. There is no end marker for the set, and MS-RDPBCGR 3.2.5.15.1 requires the client to act on a request as soon as it decodes one, so waiting to learn how many are coming is not an option the protocol offers. `should_perform_multitransport()` can therefore come round twice; the caller answers reliable and lossy separately. ## Approach **Routing.** Requests arrive on the negotiated MCS message channel (2.2.15.1) and the Demand Active on the I/O channel, so the channel decides which is which. The message channel also carries NetworkAutoDetect since #1348, so a decode still confirms what arrived there, but the I/O channel is never speculatively decoded as multitransport. A PDU on neither channel is an error. For the decode to be a sound confirmation the request decoder must reject a Demand Active, so this PR also tightens `MultitransportRequestPdu` to require the exact `SEC_TRANSPORT_REQ` security-header flag. **Yielding.** Each request is surfaced the moment it decodes. Responding returns the connector to `MultitransportBootstrapping` to read whatever comes next, which may be a second request or the Demand Active. Nothing is buffered and nothing is replayed: when the request is surfaced the Demand Active has not arrived yet. **Soft-Sync.** The Initiate Multitransport Response is the Soft-Sync signalling path (2.2.15.2), permitted only when both peers advertised `SOFTSYNC_TCP_TO_UDP` in their GCC `MultiTransportChannelData`. The server's block is retained from the GCC exchange and checked against the client's configured flags. One rule covers both paths: - Soft-Sync negotiated: always respond, `S_OK` or `E_ABORT`, including on `skip_multitransport()`, which 3.2.5.15.1 requires. Both the async and blocking drivers skip automatically, so without this every default client leaves a compliant server waiting. - Not negotiated: never respond. The outcome is reported in band on the new transport, and putting anything on the main channel would be the violation. The response goes on the message channel per 2.2.15.2 and 3.2.5.15.2. If Soft-Sync was negotiated but no message channel exists the connector errors rather than falling back to the I/O channel, and that check runs before the pending state is taken, so the caller is left with a connector it can still inspect or decline from. ## Wire behaviour On the wire TCP and UDP negotiation happen in parallel: the UDP transport is established alongside the ongoing TCP handshake, and its completion signals the dynamic-channel layer that subsequent channels may migrate to UDP. The connector's API yield point here is a Rust affordance, not a spec-mandated TCP pause. Thanks to @hardening for the correction. ## Tests Connector state-machine tests in `ironrdp-testsuite-core` drive the public API with the shared `SERVER_DEMAND_ACTIVE` fixture: - a request is surfaced on arrival, without waiting for a following PDU (regression test for the stall); - responding returns to bootstrapping so a second request is read normally; - a third request is rejected per the 2.2.15.1 cap; - a Demand Active on the I/O channel ends bootstrapping; - the response targets the message channel, decoded back off the wire; - a `Failure` result is carried through; - `skip` sends `E_ABORT` under Soft-Sync, and nothing without it; - `complete` emits nothing without Soft-Sync but still resumes; - a failed response leaves the connector in `MultitransportPending`, still able to report or decline, rather than `Consumed`; - `complete` / `skip` outside `MultitransportPending` error; - a Demand Active's user data does not decode as a `MultitransportRequestPdu` (regression test for the decoder tightening above). Fuzzing: `MultitransportRequestPdu` and `MultitransportResponsePdu` are added to the `pdu_decode` and `pdu_round_trip` oracles, so the decode surface is fuzzed against arbitrary input (auto-discovered into the fuzz CI matrix). Both targets smoke-run clean. Full end-to-end coverage against a live server still awaits server-side `MultitransportRequestPdu` emission, which `ironrdp::server::RdpServer` does not implement yet. ## Breaking changes `cargo semver-checks` reports three, so the title carries the marker: - `ClientConnector` gains the public field `server_multitransport_flags`, so struct literals need updating (`constructible_struct_adds_field`). - `ClientConnectorState::MultitransportBootstrapping` gains `message_channel_id` and `requests_seen` (`enum_struct_variant_field_added`). - Three `ClientConnectorState` discriminants shift, because the new variants are inserted mid-enum (`enum_no_repr_variant_discriminant_changed`). The third I would call a false positive worth naming rather than designing around: `ClientConnectorState` is `#[non_exhaustive]`, has no `repr`, and every variant carries fields, so there is no `as` cast a downstream crate could have depended on. The first two are real. ## Verification - `cargo xtask check fmt -v` green - `cargo xtask check lints -v` green - `cargo xtask check tests -v` green - `cargo xtask check typos -v` green - `cargo xtask check locks -v` green ## References Builds on: #1091 Related: #140, #1348 --------- Signed-off-by: Greg Lamberson <greg@lamco.io>
…1098) Makes the `MultitransportBootstrapping` state functional instead of a no-op pass-through. After licensing the server may send 0, 1, or 2 Initiate Multitransport Request PDUs before capabilities exchange. Each one is surfaced to the application, which establishes UDP transport (RDPEUDP2 + TLS + RDPEMT) or declines, and the connector reports the outcome back to the server. Mirrors the existing `should_perform_X()` pause-point pattern used by TLS upgrade and CredSSP, but uses `complete_X()` / `skip_X()` rather than `mark_X_as_done()` because completion carries result data: - `should_perform_multitransport()`: true while a request awaits an outcome - `multitransport_request()`: the request awaiting an outcome, or `None` - `complete_multitransport(result, output)`: report the outcome, resume - `skip_multitransport(output)`: decline, resume `complete_multitransport` accepts a `MultitransportResult` (a `Success` / `Failure(hresult)` enum) rather than a caller-built response PDU. The connector builds the response internally from the stored request ID. Requests are surfaced one at a time rather than as a batch. There is no end marker for the set, and MS-RDPBCGR 3.2.5.15.1 requires the client to act on a request as soon as it decodes one, so waiting to learn how many are coming is not an option the protocol offers. `should_perform_multitransport()` can therefore come round twice; the caller answers reliable and lossy separately. **Routing.** Requests arrive on the negotiated MCS message channel (2.2.15.1) and the Demand Active on the I/O channel, so the channel decides which is which. The message channel also carries NetworkAutoDetect since so a decode still confirms what arrived there, but the I/O channel is never speculatively decoded as multitransport. A PDU on neither channel is an error. For the decode to be a sound confirmation the request decoder must reject a Demand Active, so this PR also tightens `MultitransportRequestPdu` to require the exact `SEC_TRANSPORT_REQ` security-header flag. **Yielding.** Each request is surfaced the moment it decodes. Responding returns the connector to `MultitransportBootstrapping` to read whatever comes next, which may be a second request or the Demand Active. Nothing is buffered and nothing is replayed: when the request is surfaced the Demand Active has not arrived yet. **Soft-Sync.** The Initiate Multitransport Response is the Soft-Sync signalling path (2.2.15.2), permitted only when both peers advertised `SOFTSYNC_TCP_TO_UDP` in their GCC `MultiTransportChannelData`. The server's block is retained from the GCC exchange and checked against the client's configured flags. One rule covers both paths: - Soft-Sync negotiated: always respond, `S_OK` or `E_ABORT`, including on `skip_multitransport()`, which 3.2.5.15.1 requires. Both the async and blocking drivers skip automatically, so without this every default client leaves a compliant server waiting. - Not negotiated: never respond. The outcome is reported in band on the new transport, and putting anything on the main channel would be the violation. The response goes on the message channel per 2.2.15.2 and 3.2.5.15.2. If Soft-Sync was negotiated but no message channel exists the connector errors rather than falling back to the I/O channel, and that check runs before the pending state is taken, so the caller is left with a connector it can still inspect or decline from. On the wire TCP and UDP negotiation happen in parallel: the UDP transport is established alongside the ongoing TCP handshake, and its completion signals the dynamic-channel layer that subsequent channels may migrate to UDP. The connector's API yield point here is a Rust affordance, not a spec-mandated TCP pause. Thanks to @hardening for the correction. Connector state-machine tests in `ironrdp-testsuite-core` drive the public API with the shared `SERVER_DEMAND_ACTIVE` fixture: - a request is surfaced on arrival, without waiting for a following PDU (regression test for the stall); - responding returns to bootstrapping so a second request is read normally; - a third request is rejected per the 2.2.15.1 cap; - a Demand Active on the I/O channel ends bootstrapping; - the response targets the message channel, decoded back off the wire; - a `Failure` result is carried through; - `skip` sends `E_ABORT` under Soft-Sync, and nothing without it; - `complete` emits nothing without Soft-Sync but still resumes; - a failed response leaves the connector in `MultitransportPending`, still able to report or decline, rather than `Consumed`; - `complete` / `skip` outside `MultitransportPending` error; - a Demand Active's user data does not decode as a `MultitransportRequestPdu` (regression test for the decoder tightening above). Fuzzing: `MultitransportRequestPdu` and `MultitransportResponsePdu` are added to the `pdu_decode` and `pdu_round_trip` oracles, so the decode surface is fuzzed against arbitrary input (auto-discovered into the fuzz CI matrix). Both targets smoke-run clean. Full end-to-end coverage against a live server still awaits server-side `MultitransportRequestPdu` emission, which `ironrdp::server::RdpServer` does not implement yet. `cargo semver-checks` reports three, so the title carries the marker: - `ClientConnector` gains the public field `server_multitransport_flags`, so struct literals need updating (`constructible_struct_adds_field`). - `ClientConnectorState::MultitransportBootstrapping` gains `message_channel_id` and `requests_seen` (`enum_struct_variant_field_added`). - Three `ClientConnectorState` discriminants shift, because the new variants are inserted mid-enum (`enum_no_repr_variant_discriminant_changed`). The third I would call a false positive worth naming rather than designing around: `ClientConnectorState` is `#[non_exhaustive]`, has no `repr`, and every variant carries fields, so there is no `as` cast a downstream crate could have depended on. The first two are real. - `cargo xtask check fmt -v` green - `cargo xtask check lints -v` green - `cargo xtask check tests -v` green - `cargo xtask check typos -v` green - `cargo xtask check locks -v` green Builds on: #1091 Related: #140, #1348 --------- Signed-off-by: Greg Lamberson <greg@lamco.io>
) ## Summary Adds `HeartbeatPdu` (MS-RDPBCGR 2.2.16.1), decoded on the MCS message channel that #1347/#1348 wired up for Auto-Detect. The Heartbeat PDU is server-to-client only and defines no client response; the client is free to ignore `count1`/`count2`. Fixes a real gap this exposed: the message-channel demux in `ironrdp-session` unconditionally decoded incoming traffic as an Auto-Detect Request PDU. A Heartbeat PDU arriving on the same channel would fail that decode and error the session. The demux now peeks the security-header flags first (masking `SEC_RESET_SEQNO`/`SEC_IGNORE_SEQNO`, which MS-RDPBCGR 2.2.8.1.1.2.1 says MUST be ignored, the same way `MultitransportRequestPdu`/`MultitransportResponsePdu` already do) and dispatches by the masked value. Anything it doesn't recognize is logged and ignored rather than treated as session-fatal, matching the forward-safe posture the connect-time demux in `ironrdp-connector` already has. Multitransport Bootstrapping, the other optional feature the message channel carries, is already fully implemented (#1098); this PR is the Heartbeat half only. ## Validation `cargo xtask check fmt/typos/tests/locks` and `wasm check` all pass. `lints` has one pre-existing failure on master unrelated to this diff (`clippy::result_large_err` on `ErrorInfoDisconnectHandle::disconnect`, fix filed as #1812); this PR's own diff is clean under the same lints run. New tests: `HeartbeatPdu` encode/decode round-trip plus the same seqno-flag and cross-PDU-discriminator coverage `MultitransportRequestPdu` has, in `ironrdp-pdu`; a `Processor::process` integration test in `ironrdp-testsuite-core` covering the full message-channel path (Heartbeat produces no output, Auto-Detect still works alongside it, and an unrecognized PDU is ignored rather than erroring).
) The server never sends Server Heartbeat PDUs (MS-RDPBCGR 2.2.16.1), even though the client half of the feature is in place: HeartbeatPdu encode/decode landed with #1814 and clients decode and ignore them. This adds the send half, opt-in. RdpServer::enable_heartbeat(HeartbeatConfig) arms a fifth joined future in the client loop, following refresh_auto_reconnect_cookie's shape. It only runs when three gates hold: the feature is enabled, the client requested a message channel, and the client advertised RNS_UD_CS_SUPPORT_HEARTBEAT_PDU in its GCC early capability flags. That last gate needed the acceptor to expose those flags, so AcceptorResult gains client_early_capability_flags, populated the same way #1453 exposed multitransport_flags. Two spec details drove the shape. Heartbeats ride the MCS message channel framed by a Basic Security Header carrying SEC_HEARTBEAT, exactly like auto-detect after #1348, so encode_heartbeat mirrors encode_autodetect_request. And 2.2.16.1 says heartbeats SHOULD only be sent when no other PDU went out in the interval, so SharedWriter now counts successful writes and the heartbeat tick skips the send when the count moved: ordinary traffic doubles as the liveness signal. The defaults (period 5 seconds, warning after 3 missed heartbeats, reconnect after 5 more) are this library's own choice; the spec mandates no values and the client MAY ignore both counts. A zero period is bumped to one second to keep the timer well-formed. No new tests: the wire format is already covered by the HeartbeatPdu round-trip tests #1814 added in ironrdp-pdu, and ironrdp-server builds no test harness for the client-loop plumbing this touches. `cargo xtask check fmt/lints/tests/typos/locks` all pass.
…volutions#1814) ## Summary Adds `HeartbeatPdu` (MS-RDPBCGR 2.2.16.1), decoded on the MCS message channel that Devolutions#1347/Devolutions#1348 wired up for Auto-Detect. The Heartbeat PDU is server-to-client only and defines no client response; the client is free to ignore `count1`/`count2`. Fixes a real gap this exposed: the message-channel demux in `ironrdp-session` unconditionally decoded incoming traffic as an Auto-Detect Request PDU. A Heartbeat PDU arriving on the same channel would fail that decode and error the session. The demux now peeks the security-header flags first (masking `SEC_RESET_SEQNO`/`SEC_IGNORE_SEQNO`, which MS-RDPBCGR 2.2.8.1.1.2.1 says MUST be ignored, the same way `MultitransportRequestPdu`/`MultitransportResponsePdu` already do) and dispatches by the masked value. Anything it doesn't recognize is logged and ignored rather than treated as session-fatal, matching the forward-safe posture the connect-time demux in `ironrdp-connector` already has. Multitransport Bootstrapping, the other optional feature the message channel carries, is already fully implemented (Devolutions#1098); this PR is the Heartbeat half only. ## Validation `cargo xtask check fmt/typos/tests/locks` and `wasm check` all pass. `lints` has one pre-existing failure on master unrelated to this diff (`clippy::result_large_err` on `ErrorInfoDisconnectHandle::disconnect`, fix filed as Devolutions#1812); this PR's own diff is clean under the same lints run. New tests: `HeartbeatPdu` encode/decode round-trip plus the same seqno-flag and cross-PDU-discriminator coverage `MultitransportRequestPdu` has, in `ironrdp-pdu`; a `Processor::process` integration test in `ironrdp-testsuite-core` covering the full message-channel path (Heartbeat produces no output, Auto-Detect still works alongside it, and an unrecognized PDU is ignored rather than erroring). (cherry picked from commit 4275b3d)
Stacked on #1347 (acceptor message-channel negotiation). Until that merges,
the diff here also contains its commit; review the top commit
(
fix(pdu)!: send NetworkAutoDetect over the MCS message channel). I willrebase this to master-only once #1347 lands.
NetworkAutoDetect was modeled as a slow-path Share Data PDU
(ShareDataPduType::AutoDetect, a fabricated 0x3B value) carried on the I/O
channel. Per MS-RDPBCGR 2.2.14.3 / 2.2.14.4 the auto-detect PDUs are framed by
a Basic Security Header (SEC_AUTODETECT_REQ / SEC_AUTODETECT_RSP) and ride the
MCS message channel, so neither mstsc nor xfreerdp ever answered the requests.
ironrdp-pdu: add AutoDetectReqPdu / AutoDetectRspPdu, wrapping the request and
response data with the security header (mirroring the multitransport PDUs).
Remove the ShareDataPdu::AutoDetectReq / AutoDetectRsp variants and the
ShareDataPduType::AutoDetect discriminant, which did not correspond to any real
PDUTYPE2. This is the breaking change.
ironrdp-server: send auto-detect requests and receive responses on the message
channel surfaced by the acceptor, with the new framing.
ironrdp-connector and ironrdp-session: the client requests the message channel
(Client Message Channel Data), advertises SUPPORT_NET_CHAR_AUTODETECT, joins the
channel, and answers RTT requests on it. The connector handles connect-time
auto-detection (previously a no-op) and the session handles continuous
auto-detection. Resolves the client side of the message-channel TODO (#140).
Tested: the in-process client/server tests in ironrdp-testsuite-core exercise
the full negotiation and continuous auto-detect; new ironrdp-pdu round-trip
tests cover the security-header framing; the session auto-detect tests were
migrated to the message channel.