-
Notifications
You must be signed in to change notification settings - Fork 23
RDP proxy server-side Kerberos support #1396
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: dev/rdp-kerberos-credentials-injection
Are you sure you want to change the base?
RDP proxy server-side Kerberos support #1396
Conversation
| UsernamePassword { username: String, password: Password }, | ||
| UsernamePassword { | ||
| username: String, | ||
| domain: Option<String>, |
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.
issue: The absence of the domain field is intentional, we decided to focus on supporting only the FQDN/UPN format, e.g.: username@domain via the username field.
This is handled cleanly by sspi::Username::parse.
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.
Done
devolutions-gateway/src/rdp_proxy.rs
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Verify this inspect_err is really necessary, since we are already propagating the original error.
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.
removed inspect_err
406f0f8 to
60b5d33
Compare
|
The newer IronRDP crates are published! |
60b5d33 to
c4adbb1
Compare
…Kerberos server config;
d60b303 to
ef23b69
Compare
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 updates the ironrdp dependencies from version 0.5 to 0.6 and adds support for Kerberos server configuration during RDP proxying with DNS resolution capabilities.
- Updates ironrdp-tokio, ironrdp-connector, and ironrdp-acceptor packages to version 0.6
- Adds Kerberos server configuration structures and support for User-to-User authentication
- Integrates ReqwestNetworkClient for DNS resolution during CredSSP authentication
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| devolutions-gateway/Cargo.toml | Updates ironrdp dependencies to 0.6 and adds reqwest feature to ironrdp-tokio |
| Cargo.lock | Reflects dependency updates including tokio 1.48.0, sspi 0.16.1, and new DNS resolution libraries |
| devolutions-gateway/src/config.rs | Adds DomainUser and KerberosServer configuration structures for Kerberos authentication |
| devolutions-gateway/src/rdp_proxy.rs | Integrates Kerberos server config, network client for DNS resolution, and new generator resolver functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CBenoit, This PR is now ready for review again. I will update the PR description soon, but you can review it in the meantime. In short: I implemented Kerberos support for the RDP proxy. We need the KDC implementation within the KDC proxy to support Kerberos credential injection fully. I will provide it in a separate PR. |
Kerberos support requires the network client to send requests to the KDC, so I enabled the Why don't we want I see that KDC proxy extracts KDC URL from the JWT token and expects it to have "tcp" or "udp" scheme. Can I assume the same applies to the RDP proxy and add the KDC URL to the configuration file? |
Maybe there is a problem in how we handle the dependencies in
We don’t want to have both aws-lc-rs AND ring in the same build. We only need one crypto backend. Embedding more just make the executable bigger for no compelling reason so far. I know it’s not very fun to deal with that, but it’s a problem with how dependency injection is implemented for rustls (not that I would have done better).
I guess we need to talk to the real KDC too? Is there a reason why we would prefer not having the tcp or udp scheme? I think there is both a TCP and a UDP mode (at least as far as I can see in the KDC proxy module). |
devolutions-gateway/src/config.rs
Outdated
| pub users: Vec<DomainUser>, | ||
| /// The maximum allowed time difference between client and proxy clocks | ||
| /// | ||
| /// The value must be in seconds. |
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.
| /// The value must be in seconds. | |
| /// The value is expressed in seconds. |
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.
done
devolutions-gateway/src/rdp_proxy.rs
Outdated
| let krb_server_config = if conf.debug.enable_unstable { | ||
| if let Some(KerberosServer { |
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: You can probably collapse the nested if by using if-let-chain
let krb_server_config = if conf.debug.enable_unstable
&& if let Some(KerberosServer { … }) = config.debug.kerberos_server.as_ref()
{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.
done
devolutions-gateway/src/rdp_proxy.rs
Outdated
| username, // The username is in the FQDN format. Thus, the domain field can be empty. | ||
| "", password, |
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: Maybe move the comment just above the line CredentialsBuffers::AuthIdentity(AuthIdentityBuffers::from_utf8(
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.
done
devolutions-gateway/src/rdp_proxy.rs
Outdated
|
|
||
| Some(KerberosServerConfig { | ||
| kerberos_config: SspiKerberosConfig { | ||
| // The sspi will automatically try to resolve the KDC host via DNS and/or environment variable. |
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.
| // The sspi will automatically try to resolve the KDC host via DNS and/or environment variable. | |
| // The sspi library will automatically try to resolve the KDC host via DNS and/or environment variable. |
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.
done
devolutions-gateway/src/rdp_proxy.rs
Outdated
| security_protocol, | ||
| ironrdp_connector::ServerName::new(server_name), | ||
| server_public_key, | ||
| // We do not need to specify the Kerberos config here: the sspi-rs can automatically resolve the KDC host via DNS and/or env variable. |
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.
| // We do not need to specify the Kerberos config here: the sspi-rs can automatically resolve the KDC host via DNS and/or env variable. | |
| // We do not need to specify the Kerberos config here: sspi-rs can automatically resolve the KDC host via DNS and/or env variable. |
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.
done
devolutions-gateway/src/rdp_proxy.rs
Outdated
| security_protocol, | ||
| ironrdp_connector::ServerName::new(server_name), | ||
| server_public_key, | ||
| // We do not need to specify the Kerberos config here: the sspi-rs can automatically resolve the KDC host via DNS and/or env variable. |
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’m pretty sure we’re not going to use the env variables. Do you think the DNS approach is enough?
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.
Do you think the DNS approach is enough?
I improved the KerberosServr structure: I added an ability to configure the KDC address
CBenoit
left a comment
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.
This is looking good to me overall. We need to address the dependency issue and we can merge.
…oxy and add ability to configure KDC addr;
Yes, you are right.
I think no.
I refactored this part. I reused the KDC proxy code and implemented a simple network client based on its code. Can you review it again? |
|
@CBenoit The CI has failed because the hardcoded certificate (end-certificate in the certificate chain) had expired on November 5th: devolutions-gateway/testsuite/tests/cli/jetsocat.rs Lines 346 to 445 in 4304148
I do not think I can regenerate them, because the CA cert and the middle certs are trusted.
|

Hi,
I added Kerberos support for the RDP proxy.
This is the first PR in a series of PRs for the Kerberos credentials injection feature. I tried to make as few changes as possible.
The Kerberos server feature is unstable, so it will only work if you enable the
enable_unstableoption in the config file and provide the Kerberos server configuration. Here is an example of my config:If
enable_unstableis not enabled, then NTLM will be used (even if you provide the Kerberos configuration).We need the KDC implementation within the KDC proxy to support Kerberos credential injection fully. I will provide it in a separate PR.