feat(ironrdp): server-side Kerberos support #839
Conversation
…ent passing the network client;
…ment it for `ReqwestNetworkClient`;
| @@ -24,13 +24,13 @@ ironrdp-core = { path = "../ironrdp-core", version = "0.1" } # public | |||
| ironrdp-error = { path = "../ironrdp-error", version = "0.1" } # public | |||
| ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.5", features = ["std"] } # public | |||
| arbitrary = { version = "1", features = ["derive"], optional = true } # public | |||
| sspi = "0.15" # public | |||
| sspi = { path = "../../../sspi-rs" } # public | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Note that we can’t publish until the dev branch is merged onto master.
There was a problem hiding this comment.
Yes, I remember. I am working on one more fix for sspi-rs (I found a problem when connecting using mstsc)
| #[cfg(feature = "reqwest")] | ||
| pub trait AsyncSendableNetworkClient: Send + Sync { | ||
| fn send<'a>( | ||
| &'a mut self, | ||
| network_request: &'a sspi::NetworkRequest, | ||
| ) -> Pin<Box<dyn core::future::Future<Output = ironrdp_connector::ConnectorResult<Vec<u8>>> + Send + 'a>>; | ||
| } |
There was a problem hiding this comment.
What?
It may look weird, but let me explain myself.
The existing AsyncNetworkClient trait is !Send and returns !Send future. This is a problem for the Devolutions-Gateway because it requires the network client to be Send, and, even more, the returned future should also be Send.
I cannot just change the existing trait, because the implemented network client in ironrdp-web returns !Send future, and there is no way of making it Send. Moreover, ironrdp-async and all other crates work pretty well with the !Send network client, and I do not want to restrict it.
Thus, I added a new trait which is almost identical but is Send but returns a Sendable future. It will be used (it is already used, but only locally on my laptop) by Devolutions-Gateway (and other future RDP clients which require Sendable futures).
This approach is also great because it doesn't force me to refactor other ironrdp-* crates.
Why do not define this trait in the DG?
Because we need the ReqwestNetworkClient in the DG, an alternative approach is to make all ReqwestNetworkClient methods public (like send_tcp, send_http, etc.) and implement everything in the DG. But I do not like this idea and do not want to expose these methods.
There was a problem hiding this comment.
I've thought this through, and I don't believe this is a good direction for a few reasons:
- Unused trait: This introduces a new trait that isn't used elsewhere in IronRDP. That adds cognitive overhead without clear benefit.
- Unnecessary API surface: The trait wraps functionality that could be exposed directly on
ReqwestNetworkClient. It doesn't meaningfully improve abstraction, and adds public API surface we’ll need to maintain. - Cross-cutting concerns: This mixes reqwest-specific logic (behind a feature flag) with core code (framed trait implementation). The
reqwestfeature inironrdp-tokiois already something I’m not really happy about. I’d rather keep its logic self-contained. Granted this can be fixed by moving the trait definition in the appropriate module.
Alternatives
Here are two options I believe are clearer. Feel free to pick the one that fits best:
Option 1: Add a send method directly to ReqwestNetworkClient
Add an async send() method on impl ReqwestNetworkClient, instead of exposing send_tcp etc. directly. This keeps the abstraction clear: it's a helper specific to ReqwestNetworkClient, not meant for generic use.
Given that the sister PR doesn’t require trait-level genericity, this helper approach would be sufficient and avoids trait proliferation.
We also save an unnecessary heap-allocation, but it’s not really critical.
Option 2: Make AsyncNetworkClient non-object-safe
Define the trait as:
pub trait AsyncNetworkClient {
async fn send<'a>(&'a mut self, network_request: &'a NetworkRequest) -> ConnectorResult<Vec<u8>>;
}And monomorphize the code in ironrdp-async::connector:
#[instrument(skip_all)]
pub async fn connect_finalize<S, R>(
_: Upgraded,
framed: &mut Framed<S>,
mut connector: ClientConnector,
server_name: ServerName,
server_public_key: Vec<u8>,
network_client: Option<&mut R>,
kerberos_config: Option<KerberosConfig>,
) -> ConnectorResult<ConnectionResult>
where
S: FramedRead + FramedWrite,
R: AsyncNetworkClient,
{This is fine because the function is already monomorphized for similar reasons for FramedRead and FramedWrite traits.
| Ok(ServerState::ReplyNeeded(ts_request)) => (Some(ts_request), CredsspState::Ongoing), | ||
| Ok(ServerState::ReplyNeeded(ts_request)) => (Some(Box::new(ts_request)), CredsspState::Ongoing), | ||
| Ok(ServerState::Finished(_id)) => (None, CredsspState::Finished), | ||
| Err(err) => (Some(err.ts_request), CredsspState::ServerError(err.error)), | ||
| Err(err) => (err.ts_request, CredsspState::ServerError(err.error)), |
There was a problem hiding this comment.
suggestion: Instead of boxing the TS request in the success path, what about unboxing the TS request in the error path? The Rust compiler has a specialization when dereferencing Box types for that: *err.ts_request.
| let response = network_client | ||
| .send(&request) | ||
| .await | ||
| .inspect_err(|err| error!(?err, "Failed to send a Kerberos message")) |
There was a problem hiding this comment.
question: Do we really need inspect_err here? I think we are propagating the error using map_err.
There was a problem hiding this comment.
Looking mostly good to me. However, I would like to have changes in ironrdp-tokio (#839 (comment)) in a separate PR. We can merge this now, and this should be a dedicated changelog item.
|
Thank you for the review. I will address these comments when I finish my fix for |
Hi,
This PR contains small refactoring and improvements for the server-side Kerberos support.