fix(dgw): target Gateway hostname when using the credential-injection fake KDC proxy#1856
fix(dgw): target Gateway hostname when using the credential-injection fake KDC proxy#1856Pavlo Myroniuk (TheBestTvarynka) wants to merge 1 commit into
Conversation
…must be proxy hostname;
Let maintainers know that an action is required on their side
|
|
Benoît Cortier (@CBenoit) kind reminder 🙃 |
There was a problem hiding this comment.
Pull request overview
Updates the credential-injection fake KDC to issue Kerberos tickets for the Gateway hostname rather than the downstream RDP host.
Changes:
- Stores the Gateway hostname in
CredentialService. - Uses it when building per-session KDC configuration.
- Updates constructors and existing unit tests for the required hostname.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
devolutions-gateway/src/service.rs |
Passes the configured hostname to the credential service. |
devolutions-gateway/src/lib.rs |
Updates mock state initialization. |
devolutions-gateway/src/credential_injection_kdc.rs |
Uses the Gateway hostname as the Kerberos target. |
Comments suppressed due to low confidence (1)
devolutions-gateway/src/credential_injection_kdc.rs:529
- The central regression is no longer covered: the previous test asserted the token's
dst_hst, while the updated tests only pass a constructor argument and never verify it is used. Add a test with different Gateway and token destination hostnames and assert thatkdc_forselects the Gateway hostname; otherwise this behavior can silently regress again.
CredentialInjectionKdc::from_parts(jti, credential_entry, self.hostname.clone(), session)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// through this service, so callers see one handle instead of coordinating a store and a registry. | ||
| #[derive(Debug, Clone)] | ||
| pub struct CredentialService { | ||
| hostname: String, |
There was a problem hiding this comment.
This is something I would prefer we address before merging
|
|
||
| impl CredentialService { | ||
| pub fn new() -> Self { | ||
| pub fn new(hostname: String) -> Self { |
There was a problem hiding this comment.
issue: I know it’s probably not a real issue today because we don’t change the hostname dynamically typically; that being said ConfHandle supports hot-reloading, and here we retrieve a snapshot of the hostname based on what was configured at startup time. The better style would be to take the ConfHandle and to retrieve the hostname each time it is necessary, so we have an up to date value (get_conf() for each request). This is relevant because CredentialService is not short-lived.
|
|
||
| impl CredentialService { | ||
| pub fn new() -> Self { | ||
| pub fn new(hostname: String) -> Self { |
There was a problem hiding this comment.
question: Also a question I got while reviewing. This is a fine, minimal fix, but it’s worth questioning if it’s not better to use the hostname of the external URL of the listener being used for this request. This makes the fix a bit more involved, and may be a follow up, but it would be "more correct". If a client reaches the gateway through a listener whose external hostname differs from conf.hostname, the SPN mismatch fixed here could resurface.
Hi,
I fixed the target hostname for the fake KDC inside the KDC proxy. I tried to write the minimal possible solution.
The problem: during the credentials injection, the RDP client does the Kerberos authentication via a KDC proxy with a fake KDC. In that case, the target hostname (Kerberos service name for the TGS) must be a proxy hostname. But prior to this PR, the KDC proxy always configured the internal KDC to the real target machine hostname. This problem was introduced in #1768.
For example, my local configuration:
dgateway.tbt.com.WIN-956CQOSSJTF.tbt.com.When I connect to the target machine using credentials injection, my FreeRDP command looks like this:
During the FreeRDP's NLA stage, it should perform Kerberos auth against
dgateway.tbt.com(instead ofWIN-956CQOSSJTF.tbt.com).I hope I explained the problem well 😄.