feat(dgw): support VMConnect through RDCleanPath - #1372
Conversation
Let maintainers know that an action is required on their side
|
b238b71 to
c244c2f
Compare
c244c2f to
dabf8e4
Compare
39f0ff1 to
3b7660f
Compare
Hyper-V over RDCleanPath needs PCB → TLS on the proxy, then CredSSP → X.224 on the client. Ordinary RDCleanPath stays X.224-first. Still VERSION_1 with the same DER fields. An explicit VMConnect request carries a Unicode PCB payload in `preconnection_blob` with no X.224; the proxy encodes the binary PCB. Generic PCB requests keep their existing X.224-first behavior. Gateway reference implementation: [Devolutions/devolutions-gateway#1372](Devolutions/devolutions-gateway#1372) Checked locally: Rust builds, formatting, Svelte typecheck, and .NET build. Real nested Hyper-V E2E through Gateway: Native rendered 18 frames, Avalonia connected and rendered its first frame, and Web rendered a non-empty 1280×720 canvas. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Marc-André Moreau <mamoreau@devolutions.net>
Detect the explicit VMConnect request shape on existing VERSION_1 fields (no X.224 + non-empty preconnection_blob), encode PCB V2 with ironrdp-pdu, TLS on the proxy, then leave CredSSP + X.224 to the client. Ordinary RDCleanPath stays PCB? → X.224 → TLS. No ironrdp-rdcleanpath version bump — stays on crates.io 0.2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7fce550 to
06dd38d
Compare
Extract PCB encode/response helpers with unit tests, reject credential injection for the pre-X.224 path with a 400 RDCleanPath error, bound PCB writes with the MS-RDPEPS 10s deadline plus flush, and fix the upstream comment for the dual ordinary/VMConnect ordering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
follow-up in d1f7e396
lab on NESTED-TGT ( all review threads resolved. |
There was a problem hiding this comment.
Pull request overview
Adds VMConnect-specific RDCleanPath sequencing while preserving ordinary X.224-first connections.
Changes:
- Detects and encodes VMConnect PCB V2 requests.
- Adds PCB write deadlines and cert-only responses.
- Rejects credential injection for VMConnect ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
devolutions-gateway/src/upstream.rs |
Documents connection sequencing. |
devolutions-gateway/src/rd_clean_path.rs |
Implements VMConnect handling and tests. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
crates.io ironrdp-pdu 0.9.0 under-counts non-BMP scalars in cchPCB. Encode PCB V2 locally with encode_utf16().count() semantics, cover it with a non-BMP unit test, and rustfmt the helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1c06391
into
master
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
The protocol branching is valid, but the implementation carries more machinery than necessary:
- Replace
is_vmconnect_requestplus laterOptionextraction with onematchover(x224_connection_pdu, preconnection_blob). This removes impossible error paths and keeps request classification beside handling. - Remove the redundant X.224 check after
connect_rdp_server; the credential-injection path already rejects requests without X.224. - Keep the 10-second timeout only on the VMConnect PCB write. Applying it to the legacy path broadens this PR's behavior and duplicates error handling.
- Remove both
flush()calls and the buffering comment. NeitherTcpStreamnor the current tunnel stream gains ordering guarantees from them. - Construct the response once, mapping
Option<Vec<u8>>into the optional X.224 field, rather than maintaining a separate VMConnect response builder and branch. - Collapse the request-shape tests into a table-driven test. Keep the wire-format regression tests because the pinned IronRDP encoder mishandles non-BMP UTF-16 lengths.
Longer term, upgrade to an IronRDP release exposing RDCleanPathMessage; that should eliminate the local classifier, response builder, and PCB encoder. Until then, retaining the corrected local encoder is safer than reverting to the known-buggy dependency implementation.
| /// master fixed this to `encode_utf16().count()` but has not published a crates.io bump yet, and | ||
| /// its MSRV is ahead of Gateway. Match the fixed wire shape here so opaque Unicode payloads stay | ||
| /// well-formed. | ||
| fn encode_vmconnect_pcb_v2(payload: String) -> anyhow::Result<Vec<u8>> { |
There was a problem hiding this comment.
This local encoder is justified as a temporary workaround: ironrdp-pdu 0.9.0 counts Unicode scalar values instead of UTF-16 code units for cchPCB, while the upstream fix is not yet available in a compatible release. Please replace it with PreconnectionBlob and ironrdp_core::encode_vec once Gateway can upgrade to the fixed IronRDP version.
Support the explicit VMConnect RDCleanPath shape from Devolutions/IronRDP#1505.
Generic PCBs keep the ordinary X.224-first path. VMConnect requests carry a Unicode payload with no X.224; Gateway encodes the binary PCB, writes it before TLS (bounded by the MS-RDPEPS 10s deadline), then leaves CredSSP and X.224 to the client. Credential injection is rejected for this ordering.
Depends-on: Devolutions/IronRDP#1505
Issue: Devolutions/IronRDP#1505