Repository navigation
test(dgw): cover RDP credential injection end to end - #1938
irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 23 commits into
Conversation
|
Implementation notes (current as of
Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
Pull request overview
Adds process-level coverage for Gateway RDP credential injection.
Changes:
- Tests NTLM injection, reconnect attempts, expiration, and fail-closed routing.
- Adds mock KDC and CredSSP integration coverage.
- Extends test configuration and dependencies.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.lock |
Locks new test dependencies. |
testsuite/Cargo.toml |
Adds Kerberos and IronRDP test dependencies. |
testsuite/src/dgw_config.rs |
Exposes Kerberos injection test configuration. |
testsuite/tests/cli/dgw/mod.rs |
Registers new test modules. |
testsuite/tests/cli/dgw/cred_injection.rs |
Adds credential injection lifecycle tests. |
testsuite/tests/cli/dgw/cred_injection_kdc.rs |
Adds mock KDC/CredSSP integration testing. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
db4a034 to
65099a9
Compare
f3aa094 to
2865eda
Compare
bcdeb28 to
370fdce
Compare
3e5f5ba to
7a6fd9a
Compare
Keep provisioned injection mappings alive for as long as Gateway still accepts the association token, so native RDP reconnects reuse the same JWT and get injected again. If injection was provisioned but the material is gone, checkout fails closed before we dial the target. Generic RDP and RDCleanPath now share one CredSSP session path. Kerberos uses the token `dst_hst` for the client-facing TERMSRV SPN. I wrote the lifecycle contract down in [src/credential/INTENT.md](https://github.com/Devolutions/devolutions-gateway/blob/85545af64a3f911d0a7e3d3d68ecb8a105a1121f/devolutions-gateway/src/credential/INTENT.md) (following [intent-files.md](https://github.com/Devolutions/devolutions-gateway/blob/master/docs/intent-files.md)) — that's what this PR implements. ### Why this design Credentials live in two phases. Before the first connect, the preflight `time_to_live` bounds how long staged secrets wait in memory. After the first checkout, retention switches to the token acceptance deadline (`exp` + leeway). If Gateway accepts the token for a reconnect, the mapping has to still be there — the old one-shot `take_mapping` broke exactly that. `jet_reuse` starts at disconnect and the TTL starts at preflight, so neither can cap the other; the token deadline is the one clock every connection attempt shares. ### Where to look hardest - [Generation-guarded KDC publication](https://github.com/Devolutions/devolutions-gateway/blob/85545af64a3f911d0a7e3d3d68ecb8a105a1121f/devolutions-gateway/src/credential_injection.rs#L667-L698): re-provisioning the same JTI bumps a generation, and an older in-flight lease can never overwrite a newer published KDC, so the newest provisioning wins deterministically. - [Deadline-driven cleanup](https://github.com/Devolutions/devolutions-gateway/blob/85545af64a3f911d0a7e3d3d68ecb8a105a1121f/devolutions-gateway/src/credential_injection.rs#L774-L835): expired session KDC material is removed by a background task, not lazily on the next checkout, and secrets are zeroized on drop. - [Release after CredSSP](85545af): the per-connection mapping and KDC lease drop as soon as both CredSSP legs finish, so plain forwarding never holds secret material. Tests cover staging expiry, reconnect reuse, generation replacement, and physical removal without a later checkout. Manual run: 22-case DVLS matrix with live NTLM and Kerberos CredSSP through Gateway. E2E hops are stacked on #1938. Issue: DVLS-14697 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you, I really appreciate the e2e tests here.
Architecturally-wise, I think the "helpers" are getting big enough that it may warrant some of the code to be moved somewhere under the lib.rs of the testsuite crate. One example could be FakeRdpTarget. If the "TLS fixtures" are meant to be shared, then I think the library side could be the best place.
It’s already pretty close, but make things as self-contained as possible so it’s possible to use the INTENT.md / .intent.md convention for providing some human documentation about the most meaningful abstractions and important properties under test. I would appreciate such files before we merge this PR.
87daa2b to
02e3705
Compare
Lines changed
Other is The 50 removed lines are the self-signed Note LLM-assisted content (no human feedback). |
Dependency prunerScope: the Result: nothing removed. Every package was already in the testsuite's build graph through the
On running Net change: 8 new direct dependencies, 3 moved from dev to normal, 1 widened Test prunerScope: the 14 tests in Result: no deletion and no consolidation. One coverage gap is closed by Time: about 55 minutes of the one-hour budget, 11:25 to 12:17 EDT, mutation runs included. Controls
Oracle classificationEvery test cites Oracle table (14 tests: 14 independent, 0 unknown)
The only self-derived assertions are the X.224 cookie checks, already marked MutantsA fresh-context agent wrote dm-01 to dm-16 from the two intent files and the Gateway code, without reading the tests. I wrote
Unique kills (a mutant only one test catches):
Kill matrix: each failing test and the first check that failed (53 kills)
Survivors
dm-16 is not closed because the budget ran out. A gap test needs a signal from outside Gateway that it stopped at 2.e, for example the client connection closing soon after the target leg fails. I have not checked whether that signal differs under the mutant. Decisions
Not covered
Note LLM-assisted content (no human feedback). |
Intent drafts: handoff
ProvenanceLevel 1 rests on git history:
Left out on purpose
Placement
Pending experimentsNone. Note LLM-assisted content (no human feedback). |
| tokio-util = "0.7" | ||
| typed-builder = "0.21" | ||
| tokio-tungstenite = { version = "0.29", features = ["rustls-tls-native-roots"] } | ||
| x509-cert = { version = "0.3", default-features = false, features = ["std"] } |
There was a problem hiding this comment.
Any chance we could remove all the extra dependencies? I expected that using ironrdp-agent (the binary) would be enough for full e2e without implementing any client on our side, but I see the other part is to have a fake RDP server. Instead of making the testsuite crate heavier, what about creating a dedicated crate similar to agent-identity-mock?
Add process-level tests for DVLS-like preflight, first inject, jet_reuse reconnect, fail-closed missing mappings, and synthetic KDC generation reuse. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Clippy separated_literal_suffix failed CI lints on the stacked reconnect tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drive CredSSP through a TCP kdc crate and IronRDP acceptor so target-leg Kerberos injection is proven, not just log-matched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prove both CredSSP hops against mock KDC/RDP, NTLM CredSSP both legs, and Kerberos fail-closed when the password, KDC, or krb_kdc is wrong. Token-cache jet_reuse still needs signed JWTs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Decode AS-REQ/TGS-REQ on the mock KDC (cname, realm, TERMSRV/localhost), record CredSSP Finished account names and X.224 cookies, and require /jet/KdcProxy AS-REP plus TGS-REP. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Routing tests now decode X.224 Cookie instead of raw-byte search. Client-leg Kerberos asserts synthetic-KDC AS-REQ cname. Fail-closed Kerberos paths require inject-started then no Finished identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Decode X.224 cookies, KdcProxy AS/TGS principals, and attribute KDC-down TCP to Gateway via a separate refusing listener. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Checkout-before-connect on the stack below lets the fail-closed path prove accepted==0. Issue: DGW-1900 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pin the public CLI release and complete NTLM and Kerberos target CredSSP over ws://127.0.0.1/jet/rdp. Issue: DGW-1900 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Read framed X.224 cookies, bound CredSSP handshakes, and share the localhost TLS fixture. Issue: DVLS-14697 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep credential-injection tests focused on public behavior instead of serializing the removed Kerberos debug option. Issue: DVLS-14697 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Treat an expired staging mapping as absent because association tokens do not carry an injection requirement. Verify ordinary forwarding preserves the client X.224 cookie. Issue: DVLS-14697 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the direct rustls dependency added on master so the credential-injection E2E compiles under the workspace unused-qualifications lint. Issue: DVLS-14697 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move GatewayProc, provisioning, fake RDP target, mock KDC/RDP, CredSSP client leg, agent harness, and TLS fixtures from the test files into testsuite/src so the abstractions are self-contained, per review. Add empty INTENT.md placeholders to be filled by hand. Issue: DVLS-14697
Record the intended NTLM, Kerberos, reconnect, and failure scenarios. Name test tokens and identifiers by their protocol roles.
The behavioral contract already lives in devolutions-gateway/src/credential/INTENT.md; the suite-level INTENT.md covers the test scenarios, so the per-file placeholders are unnecessary. Issue: DVLS-14697
The RDCleanPath injection tests are the only coverage of that path; skipping them when the binary is missing silently drops it. Missing binary is now a failure, with IRONRDP_AGENT_SKIP=1 as explicit opt-out. CI installs a pinned ironrdp-agent (cached per version). Issue: DVLS-14697
Any Gateway test can drive a real RDP client, not just credential injection, so the harness lives at testsuite::ironrdp_agent and connect_rdcleanpath takes the credentials as parameters. Issue: DVLS-14697
Issue: DVLS-14697 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Cite the injection intent for each test so the oracle marker check passes. Split the intent's scenarios into headings so markers can cite them, and state that a domainless target user stays on NTLM. Mark the X.224 cookie asserts as mutants, since no intent decides the cookie. Drop the debug-flag test and the KDC registration count, which repeat what the other Kerberos asserts already check. Issue: DVLS-14697 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A wildcard listener makes Windows Defender Firewall ask for elevation once per test binary path. Listen on 127.0.0.1 and ::1 at the same port instead, so `localhost` still reaches the server on Windows, where it resolves to ::1 first. Issue: DVLS-14697 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The missing `krb_kdc` test checks that Gateway never dials the target, which it reaches as `localhost`. On Windows, Gateway tries ::1 first, so a dial to a target listening only on 127.0.0.1 waits for the refusal and lands after the check. A regression that fell back to ordinary forwarding passed. The target now listens on both loopback addresses. Issue: DVLS-14697 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fake RDP target, the CredSSP server, the KDCs and the CredSSP client now live in crates/credential-injection-mock, one role per process. Each role binds loopback and prints JSON event lines, and the testsuite spawns it and reads those events. This keeps the RDP and Kerberos crates out of the testsuite manifest. ironrdp-agent cannot replace the client side: its direct TCP path sends no preconnection blob, which the Gateway TCP listener requires. Issue: DVLS-14697 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0073577 to
ebd9125
Compare
No test ran an RDP client through a live Gateway with injected credentials, so the path from preflight to the RDP server had no end-to-end check. These tests start Gateway, provision credentials through
/jet/preflight, and connect over TCP and over RDCleanPath to a fake RDP server, with a mock KDC for Kerberos.They cover the scenarios in
testsuite/src/rdp_injection/INTENT.md: NTLM and Kerberos injection where the RDP server receives the target credentials while the client sends only the proxy credentials, a target username without a domain staying on NTLM, reconnects with the same association token, and ordinary forwarding when provisioning is missing or expired. A wrong target password or an unreachable KDC fails without falling back to ordinary forwarding, and missing KDC information fails before Gateway connects to the RDP server.The RDCleanPath tests run the published
ironrdp-agent0.1.0 CLI, which CI now installs and caches. The testsuite gains direct IronRDP,kdc,picky-krb,picky-asn1-derandx509-certdependencies, all already in its build graph through Gateway.Issue: DVLS-14697