feat(pam-rdp): thread AD domain through RDP bridge#203
feat(pam-rdp): thread AD domain through RDP bridge#203
Conversation
Plumbs the domain string (empty for local accounts, AD domain for domain-joined NTLM CredSSP) end-to-end: API session credentials → CLI session struct → RDPProxyConfig.InjectDomain → cgo wrapper → Rust FFI → IronRDP connector config.
|
💬 Discussion in Slack: #pr-review-cli-203-feat-pam-rdp-thread-ad-domain-through-bridge-for-ntlm-credssp Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
bernie-g
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mechanical plumbing of an optional AD domain field through the existing RDP bridge layers — pattern is consistent and the cgo/FFI null handling looks correct, but flagging for human eyes since this touches NTLM CredSSP credential injection.
Extended reasoning...
Overview
This PR threads a new optional domain string end-to-end through the RDP bridge: PAM session credentials API → CLI session struct → RDPProxyConfig.InjectDomain → cgo wrapper → Rust FFI → IronRDP connector_config. It also gates the Acceptor/connector flow so empty/missing domain preserves the existing local-account behavior. Eleven files touched, all changes are mechanical signature/field additions following the established InjectUsername/InjectPassword pattern.
Security risks
The change itself is additive plumbing with no logic branching beyond empty → NULL/None. Worth a careful human read because: (1) it modifies the NTLM CredSSP credential flow at the connector Config.domain field, which changes whether the target's LSA authenticates against AD or local SAM; (2) it crosses a Go↔Rust FFI boundary where memory ownership and NUL-termination matter. The cgo side correctly conditions C.CString/C.free on non-empty domain, and the Rust side uses c_str_to_owned with .filter(|s| !s.is_empty()) symmetrically, so empty and NULL are handled identically on both sides.
Level of scrutiny
Higher than a typical config addition because of the auth/FFI surface, but lower than a full crypto/auth redesign — the auth machinery itself (CredSSP/NTLM, IronRDP connector) is unchanged; only one previously-hardcoded None becomes a caller-supplied value. End-to-end testing was reported against a real AD-joined Windows host.
Other factors
No bugs were surfaced by the automated review pass. The header documentation and Rust safety doc were updated to describe the new parameter's NULL/empty semantics, which is a nice touch. Stacked on feat/pam-rdp-mvp with a companion backend PR, so the chain may want to be reviewed together.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 32113716 | Triggered | Generic Password | 05b05ad | dev/pam/resources/ssh-server/entrypoint.sh | View secret |
| 32113717 | Triggered | Generic Password | 05b05ad | dev/pam/resources/mongodb/init/seed.js | View secret |
| 32113716 | Triggered | Generic Password | 05b05ad | dev/pam/resources/mssql/entrypoint.sh | View secret |
| 32113718 | Triggered | Username Password | 05b05ad | dev/pam/setup.sh | View secret |
| 32113718 | Triggered | Username Password | 05b05ad | dev/pam/docker-compose.yml | 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.
# Conflicts: # packages/pam/handlers/rdp/bridge_cgo_unix.go # packages/pam/handlers/rdp/bridge_cgo_windows.go
2 participants
Description 📣
Adds AD-domain support to the RDP bridge so PAM sessions targeting domain-joined Windows hosts can authenticate via NTLM CredSSP. Plumbs an optional
domainstring end-to-end: PAM session credentials API → CLI session struct →RDPProxyConfig.InjectDomain→ cgo wrapper → Rust FFI → IronRDP connector config. Empty/missing domain preserves the existing local-account behaviour, so non-AD RDP sessions are unchanged. Stacked onfeat/pam-rdp-mvp; companion infisical PR adds the backend/UI side.Type ✨
Tests 🛠️
Built locally with
-tags rdpon darwin/arm64 (gateway) and cross-built linux/amd64 via cargo-zigbuild for the EC2 gateway. End-to-end RDP access against an AD-joined Windows resource using a discovered domain account succeeds once the account has a non-empty password.cargo build --release --manifest-path packages/pam/handlers/rdp/native/Cargo.toml go build -tags rdp -o infisical .