-
Notifications
You must be signed in to change notification settings - Fork 139
feat(ffi): integrate Devolutions.IronRDP with RDCleanPath #1014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Add RDCleanPath FFI bindings - Implement WebSocket stream for gateway connections - Add GatewayConnection API with CredSSP hostname extraction - Support both direct and gateway connection modes
Add KerberosConfig FFI bindings and HTTP/HTTPS network protocol handling for KDC proxy authentication via Devolutions Gateway. - Expose KerberosConfig::new() in FFI layer - Add GenerateKdcToken() method to TokenGenerator - Update GatewayConnection to accept KDC proxy parameters - Implement HTTP/HTTPS request handling in ResolveGenerator - Auto-generate client hostname for Kerberos authentication 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Coverage Report 🤖 ⚙️Past: New: Diff: -0.02% [this comment will be updated automatically] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR integrates Devolutions.IronRDP with Gateway by adding RDCleanPath protocol support and KDC proxy capabilities for .NET. The implementation enables RDP connections through Devolutions Gateway using WebSocket transport with optional Kerberos authentication via KDC proxy.
- Adds RDCleanPath protocol FFI bindings for gateway communication
- Implements KDC proxy support for Kerberos authentication through the gateway
- Provides WebSocket transport layer and gateway connection methods for .NET
Reviewed Changes
Copilot reviewed 14 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ffi/src/rdcleanpath.rs | Adds FFI bindings for RDCleanPath protocol operations |
ffi/src/lib.rs | Registers new rdcleanpath module |
ffi/src/error.rs | Adds error handling for RDCleanPath and DER operations |
ffi/src/credssp/mod.rs | Extends KerberosConfig with KDC proxy support |
ffi/dotnet/Devolutions.IronRdp/src/WebsocketStream.cs | Implements WebSocket stream adapter for network transport |
ffi/dotnet/Devolutions.IronRdp/src/GatewayConnection.cs | Provides gateway connection methods using RDCleanPath |
ffi/dotnet/Devolutions.IronRdp/src/Framed.cs | Adds custom PDU hint interface for flexible frame detection |
ffi/dotnet/Devolutions.IronRdp/src/Connection.cs | Updates connection logic to support HTTP/HTTPS requests for KDC proxy |
ffi/dotnet/Devolutions.IronRdp.ConnectExample/Program.cs | Updates example to remove unused parameter |
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/TokenGenerator.cs | Adds JWT token generation client for gateway authentication |
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/MainWindow.axaml.cs | Integrates gateway connection support in example application |
ffi/dotnet/Devolutions.IronRdp.ConnectExample/Devolutions.IronRdp.ConnectExample.csproj | Updates ImageSharp package version |
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/Devolutions.IronRdp.AvaloniaExample.csproj | Updates Avalonia package versions |
ffi/Cargo.toml | Adds ironrdp-rdcleanpath dependency |
Comments suppressed due to low confidence (1)
ffi/dotnet/Devolutions.IronRdp/src/Connection.cs:129
- [nitpick] Use conventional null comparison style 'decoded == null' instead of Yoda condition 'null == decoded' for better readability and consistency with C# conventions.
if (null == decoded)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/MainWindow.axaml.cs
Outdated
Show resolved
Hide resolved
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/MainWindow.axaml.cs
Outdated
Show resolved
Hide resolved
Please, open a separate PR for the KDC proxy part. |
ffi/dotnet/Devolutions.IronRdp.AvaloniaExample/MainWindow.axaml.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 17 out of 39 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
…o gateway-with-ffi
if let Err(err) = self.sender.send(message) { | ||
error!("Failed to send clipboard message: {:?}", err); | ||
if let Err(error) = self.sender.send(message) { | ||
error!(?error, "Failed to send clipboard message"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixing styles along the way, let me know if you mind
} | ||
|
||
pub fn with_dynamic_channel_display_control(&mut self) -> Result<(), Box<IronRdpError>> { | ||
self.with_dvc(DisplayControlClient::new(|c| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixing styles along the way, let me know if you mind
Ready for review @CBenoit |
ActiveStage? _activeStage; | ||
DecodedImage? _decodedImage; | ||
Framed<SslStream>? _framed; | ||
Framed<Stream>? _framed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Run dotnet format
var username = Environment.GetEnvironmentVariable("IRONRDP_USERNAME"); | ||
var password = Environment.GetEnvironmentVariable("IRONRDP_PASSWORD"); | ||
var domain = Environment.GetEnvironmentVariable("IRONRDP_DOMAIN"); | ||
var domain = Environment.GetEnvironmentVariable("IRONRDP_DOMAIN"); // Optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Inconsistent. IRONRDP_DOMAIN is not the only optional env variable. IRONRDP_PORT is too. Either document what is required and what is not consistently, or remove the outdated comment for now.
var domain = Environment.GetEnvironmentVariable("IRONRDP_DOMAIN"); // Optional | |
var domain = Environment.GetEnvironmentVariable("IRONRDP_DOMAIN"); |
/// <param name="pcb">Optional preconnection blob for Hyper-V VM connections</param> | ||
/// <param name="factory">Optional clipboard backend factory</param> | ||
/// <returns>A tuple containing the connection result and framed WebSocket stream</returns> | ||
public static async Task<(ConnectionResult, Framed<WebSocketStream>)> ConnectViaGateway( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I suggest a more generic naming here: ConnectRDCleanPath
. Try to see if there are other similar places.
// Step 2: Get client local address (dummy for WebSocket) | ||
string clientAddr = "127.0.0.1:33899"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I guess this is fine? But that’s not what we do in the Rust native client. Look for the connect_ws
function.
pub struct GenericError(pub anyhow::Error); | ||
|
||
impl Display for GenericError { | ||
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
write!(f, "{:#}", self.0) | ||
} | ||
} | ||
|
||
impl From<GenericError> for IronRdpErrorKind { | ||
fn from(_val: GenericError) -> Self { | ||
IronRdpErrorKind::Generic | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This can be made less verbose to write, and the IronRdpErrorKind type can be removed. Will be a follow up from me.
} | ||
|
||
/// Converts the PDU into a typed enum for pattern matching | ||
pub fn into_enum(&self) -> Result<Box<RDCleanPathResult>, Box<IronRdpError>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: In the FFI, we do not return an enum. Actually, I don’t think you need a whole different type, it’s not useful here since we can’t manipulate field by field, and there is no destructuring pattern matching.
/// Returns 0 if not present or not a GeneralError variant | ||
pub fn get_http_status_code(&self) -> u16 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Instead of the magic number, can this be Option? (Nullable u16)
Add RDCleanPath support for Devolutions.IronRDP .NET package