Add RDP E2E tests#230
Draft
bernie-g wants to merge 26 commits into
Draft
Conversation
6 subtests: connection, bad credentials, unreachable target, reconnect, concurrent connections, and session duration. Uses an xrdp container as the target and FreeRDP under xvfb for headless verification. CI pam-test job updated to build the Rust bridge and CLI with -tags rdp.
|
💬 Discussion in Slack: #pr-review-cli-230-add-rdp-e2e-tests Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
The backend requires a pam_project_recording_configs row to exist before allowing Windows resource creation. Insert a dummy row directly into the database, bypassing FK checks since we don't need a real app connection for the E2E tests.
Switch from full graphical xfreerdp sessions to /auth-only mode which verifies NLA credential injection without needing a display server. Fix unreachable-target by creating the resource while the container is alive then terminating it before connecting. Drop reconnect and session-duration tests to match the depth of other PAM resource tests.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33062794 | Triggered | Generic CLI Secret | b720c1a | packages/cmd/login_status_test.go | View secret |
| 9387833 | Triggered | Generic Password | b720c1a | packages/pam/handlers/rdp/native/src/rdcleanpath.rs | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…p display Regenerate oapi-codegen client to include createWindowsPamResource and createWindowsPamAccount operations. Switch RDP tests from raw HTTP calls to the generated client, matching the pattern used by SSH/Redis/Postgres tests. Wrap xfreerdp with xvfb-run when DISPLAY is unset so /auth-only works in headless CI. Fix GatewayId pointer type across all PAM tests and add CrlDistributionPointUrls field to agent CA config to match the updated API spec.
freerdp2 treats EAGAIN on the first BIO_read as fatal rather than retrying, so if the gateway is still fetching credentials when xfreerdp connects the transport fails immediately. Retry up to 3 times on ERRCONNECT_CONNECT_TRANSPORT_FAILED to give the bridge time to start.
freerdp2 treats EAGAIN on the first BIO_read as fatal, which always fails through the multi-hop proxy tunnel. freerdp3 handles this properly. The test already prefers xfreerdp3 over xfreerdp, so just install freerdp3-x11 in CI instead of freerdp2-x11.
Start Xvfb and export DISPLAY in CI so xfreerdp3 runs directly without the xvfb-run shell wrapper. Use container.Host(ctx) for the resource host instead of getOutboundIP, matching the pattern used by SSH, Postgres and Redis tests. Increase xfreerdp timeout to 60s.
Pre-connect to the RDP proxy and verify the Rust bridge is ready with an X.224 handshake probe before letting xfreerdp connect. This avoids the race where freerdp's non-blocking BIO_read gets EAGAIN because the multi-hop tunnel hasn't finished setting up the bridge yet.
Adds a direct-xrdp-connection subtest that runs xfreerdp /auth-only directly against the xrdp container with no proxy chain. This will show whether the CI failures are caused by xrdp/NLA configuration in Docker or by the multi-hop proxy chain.
The /auth-only flag fails against xrdp because xrdp sends a license error blob that freerdp3 treats as fatal, even though NLA succeeded. Switch to full graphical sessions that are held for a few seconds then killed. If the process stays alive past the hold time, the connection is considered successful. Also removes warmBridgeProxy since the transport failure was caused by the /auth-only license issue, not a bridge startup race.
…ency In production sdl-freerdp's GUI initialization gives the proxy chain time to establish the tunnel before the first packet. In tests xfreerdp connects instantly and hits EAGAIN. Retry up to 3 times with a 2s delay on ERRCONNECT_CONNECT_TRANSPORT_FAILED to handle this.
Adds a proxy-tcp-debug subtest that connects a raw TCP socket to the proxy, sends X.224 CR, and measures timing of the round trip. This will show exactly how long the proxy chain takes to respond and whether the issue is timing or something else.
The proxy returns EOF after 39ms instead of X.224 CC. Something in the chain is failing and closing the connection. Dump the proxy command's stderr to see the actual error.
The proxy logs no error, just "connection closed". The failure is on the gateway side where the bridge/handler runs. Dump both proxy and gateway output to see the actual error.
The setupRecordingConfig function inserted a recording config with a random connectionId that doesn't map to any real app connection. When the gateway tried to fetch session credentials, the backend followed the bogus connection ID and returned 404. This was the actual cause of all the proxy chain failures, not timing or bridge startup.
…recording config The backend requires a recording config for Windows resources. The previous implementation used storageBackend 'aws-s3' with a fake connectionId, which broke the credentials API (404 on app connection lookup). Now creates a dummy app connection row to satisfy the FK constraint and uses storageBackend 'postgres' which never resolves the connection at runtime.
Replace direct DB insertion with LocalStack-based setup. The upstream backend now rejects postgres storage for Windows resources and requires a decryptable app connection. LocalStack handles both STS GetCallerIdentity (app connection validation) and S3 HeadBucket/PutObject (recording config validation).
The backend S3 client uses virtual-hosted-style addressing which fails in Docker (bucket.localstack doesn't resolve). Create the app connection via API (STS works fine) then insert the recording config row directly into Postgres, skipping the HeadBucket/PutObject validation.
…tion Replace direct DB insertion with proper API calls. Add bucket name as a Docker network alias on the LocalStack container so the backend's S3 client can resolve virtual-hosted-style hostnames (bucket.localstack). Set LOCALSTACK_HOST=localstack so LocalStack parses the bucket name from the Host header.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-tags rdp, FreeRDP + xvfb installopenapi-cfg.yamlupdated withcreateWindowsPamResourceandcreateWindowsPamAccountfor future client regenerationTest plan