feat(connector):Add ironrdp-vmconnector crate, integrated into existing ironrdp-client and ironrdp async #841
Conversation
|
@irvingoujAtDevolution I don’t remember very well. What is different from the previous version? |
|
@CBenoit |
| pub async fn run_until_handover( | ||
| credssp_finished: &mut CredSSPFinished, | ||
| framed: &mut Framed<impl FramedRead + FramedWrite>, | ||
| mut connector: VmClientConnector, | ||
| ) -> ConnectorResult<ClientConnector> { | ||
| let result = loop { | ||
| single_sequence_step(framed, &mut connector, &mut credssp_finished.write_buf).await?; | ||
|
|
||
| if connector.should_hand_over() { | ||
| break connector.hand_over(); | ||
| } | ||
| }; | ||
|
|
||
| info!("Handover to client connector"); | ||
| credssp_finished.write_buf.clear(); |
There was a problem hiding this comment.
question: Is handover a terminology used by VMConnect protocol?
There was a problem hiding this comment.
Not really, it's only make sense in this context
|
We were having CredSSP issues on enhanced mode, I intend to have that support in separate pull request. |
Coverage Report 🤖 ⚙️Past: New: Diff: -0.66% [this comment will be updated automatically] |
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
2 times, most recently
from
e28091e to
349b21f
Compare
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
from
082acb5 to
69264ff
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for HyperV VmConnect connections to the IronRDP client. The changes enable clients to connect to HyperV virtual machines using a VM-specific connector that handles the specialized protocol requirements.
- Introduces a new
ironrdp-vmconnectcrate to handle HyperV VM-specific connection logic with NTLM-only CredSSP - Refactors connector architecture to use trait-based approach (
ConnectorCore,SecurityConnector,CredsspSequenceFactory) for polymorphism - Updates RDCleanPath protocol to support optional x224 connection PDUs for VM connections
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web-client/iron-svelte-client/src/lib/login/login.svelte | Adds UI field for HyperV VmConnect ID input |
| web-client/iron-remote-desktop-rdp/src/main.ts | Exports new vmConnect extension function |
| crates/ironrdp-vmconnect/src/lib.rs | New module exposing VM connector functionality |
| crates/ironrdp-vmconnect/src/connector.rs | Implements VmClientConnector with VM-specific state machine |
| crates/ironrdp-vmconnect/src/credssp.rs | Implements NTLM-only CredSSP sequence for VM connections |
| crates/ironrdp-vmconnect/src/config.rs | Defines VM-specific credentials and configuration types |
| crates/ironrdp-web/src/session.rs | Integrates VM connector with web client connection flow |
| crates/ironrdp-client/src/rdp.rs | Updates TCP/WebSocket connection logic to support VM connectors |
| crates/ironrdp-client/src/config.rs | Adds CLI arguments and config for VM connections |
| crates/ironrdp-rdcleanpath/src/lib.rs | Changes x224 PDU fields to Option<T> for VM support |
| crates/ironrdp-connector/src/lib.rs | Introduces trait-based connector abstraction |
| crates/ironrdp-connector/src/credssp.rs | Refactors CredSSP into trait for extensibility |
| crates/ironrdp-connector/src/connection.rs | Implements new traits for ClientConnector |
| crates/ironrdp-async/src/connector.rs | Updates async connector to work with trait objects |
| crates/ironrdp-async/src/vmconnector.rs | Adds VM connector lifecycle management functions |
| crates/ironrdp-blocking/src/connector.rs | Updates blocking connector for trait objects |
| crates/ironrdp-testsuite-/tests/.rs | Updates tests to match new async API signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input id="pcb" type="text" bind:value={vmconnect} /> | ||
| <label for="pcb">HyperV VmConnect ID</label> |
There was a problem hiding this comment.
The input element has id=\"pcb\" but should have id=\"vmconnect\" to match the label's for attribute and avoid duplicate IDs with the Pre Connection Blob field above (line 213).
| <input id="pcb" type="text" bind:value={vmconnect} /> | |
| <label for="pcb">HyperV VmConnect ID</label> | |
| <input id="vmconnect" type="text" bind:value={vmconnect} /> | |
| <label for="vmconnect">HyperV VmConnect ID</label> |
| // let connector::ClientConnectorState::ConnectionInitiationWaitConfirm { .. } = connector.state() else { | ||
| // return Err(connector::general_err!("invalid connector state (wait confirm)")); | ||
| // }; |
There was a problem hiding this comment.
Commented-out code should be removed. If this validation is no longer needed, the commented lines should be deleted rather than left in the codebase.
| // let connector::ClientConnectorState::ConnectionInitiationWaitConfirm { .. } = connector.state() else { | |
| // return Err(connector::general_err!("invalid connector state (wait confirm)")); | |
| // }; |
|
@irvingoujAtDevolution Can you rebase on top of master and address Copilot’s suggestions? |
Hello @CBenoit , I added dynamically dispatched trait for hyperv connector and normal connectors. The main drive is to avoid copy and paste in previous version. I did it this is way is also to reuse existing code as much as possible, instead of copy each and every method and add a _vmconnect suffix. This might not be the best solution, let me know your thoughts.