Skip to content

Add RDP E2E tests#230

Open
bernie-g wants to merge 30 commits into
mainfrom
feat/pam-rdp-tests
Open

Add RDP E2E tests#230
bernie-g wants to merge 30 commits into
mainfrom
feat/pam-rdp-tests

Conversation

@bernie-g
Copy link
Copy Markdown
Contributor

@bernie-g bernie-g commented May 11, 2026

Summary

  • Add RDP E2E tests covering connection, bad credentials, unreachable target, and concurrent connections
  • Tests exercise the full proxy chain: xfreerdp -> local proxy -> relay -> gateway -> Rust bridge -> xrdp
  • LocalStack added for mocking AWS STS/S3 in the recording config setup
  • CI updated to build the Rust RDP bridge and run tests headlessly

Test plan

  • All RDP subtests pass in CI
  • Existing PAM tests unaffected

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.
@infisical-review-police
Copy link
Copy Markdown

💬 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.

bernie-g added 4 commits May 11, 2026 15:56
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
Copy link
Copy Markdown

gitguardian Bot commented May 26, 2026

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
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
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. 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


🦉 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.

@bernie-g bernie-g changed the base branch from feat/pam-rdp-recording to main May 26, 2026 13:41
bernie-g added 22 commits May 26, 2026 10:10
…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.
Network alias approach didn't resolve virtual-hosted-style S3 hostnames.
Direct DB insert bypasses S3 validation entirely and is simpler. App
connection still created via API for proper KMS encryption.
bernie-g added 2 commits May 27, 2026 09:41
…iPost

Add createAwsAppConnection to openapi-cfg.yaml and regenerate client.
Use CreateAwsAppConnectionWithBodyWithResponse instead of raw HTTP.
@bernie-g bernie-g marked this pull request as ready for review May 27, 2026 13:56
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 27, 2026

Greptile Summary

Adds RDP E2E testing infrastructure: an xrdp Docker container as the test target, FreeRDP/Xvfb for headless CI verification, raw HTTP helpers for Windows PAM resource and account creation, and CI updates to build the Rust bridge and install RDP dependencies.

  • 4 of the 6 advertised RDP subtests are implemented (connection, bad-credentials, unreachable-target, concurrent-connections); reconnect and session-duration are absent despite being listed in the PR description and test plan.
  • buildFreeRDPArgs can call t.Skip() from a spawned goroutine in the concurrent-connections test, which is unsafe per Go testing conventions.
  • Minor: double container termination in unreachable-target and a missing Xvfb readiness check in the workflow.

Confidence Score: 3/5

The test infrastructure is solid, but the implemented subtests fall short of what the PR description and test plan commit to, and an unsafe goroutine/skip pattern in the concurrent test could silently pass when it should skip.

The test plan explicitly states "All 6 RDP subtests pass in CI," yet only 4 are written — meaning the CI gate described in this PR cannot be fully green as documented. Additionally, calling t.Skip() from a non-test goroutine in the concurrent-connections subtest violates Go testing rules and can cause the test to silently omit its skip signal, potentially producing false-positive passes in environments without a display server.

e2e/pam/rdp_test.go needs the two missing subtests and a fix to the goroutine/skip pattern; .github/workflows/run-cli-e2e-tests.yml should add a Xvfb readiness check.

Important Files Changed

Filename Overview
e2e/pam/rdp_test.go Core RDP E2E test file; 4 of the 6 described subtests are present (reconnect and session-duration are missing), and t.Skip() is called from goroutines in the concurrent-connections test.
e2e/pam/pam_helpers.go New shared PAM test infra; adds WithLocalStack option and SetupPAMInfra/LoginUser helpers used by the RDP tests. No issues found.
e2e/pam/testdata/rdp-server/entrypoint.sh xrdp container entrypoint; creates test user, sets up D-Bus and xrdp-sesman, then starts xrdp. Credentials match test constants (testuser/testpass).
.github/workflows/run-cli-e2e-tests.yml Adds Rust bridge build, CGO/rdp tag to CLI build, and installs FreeRDP+Xvfb; Xvfb is started in background with no readiness check before DISPLAY is exported.
e2e/packages/client/client.gen.go Generated OpenAPI client; adds Windows PAM resource/account and AWS app connection types and request builders. No logic changes.

Reviews (1): Last reviewed commit: "docs: explain why recording config bypas..." | Re-trigger Greptile

Comment thread e2e/pam/rdp_test.go
Comment thread e2e/pam/rdp_test.go
Comment thread e2e/pam/rdp_test.go
Comment thread .github/workflows/run-cli-e2e-tests.yml
Pre-build xfreerdp args (which may call t.Skip) on the test goroutine,
then pass them into worker goroutines that only call tryConnectFreeRDP.
@bernie-g bernie-g requested review from saifsmailbox98 and x032205 and removed request for x032205 May 27, 2026 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant