Skip to content

feat(pam): add RDP CLI access and recording chunk batching#269

Merged
x032205 merged 20 commits into
pam-revampfrom
feat/pam-rdp-cli-access
Jun 26, 2026
Merged

feat(pam): add RDP CLI access and recording chunk batching#269
x032205 merged 20 commits into
pam-revampfrom
feat/pam-rdp-cli-access

Conversation

@bernie-g

@bernie-g bernie-g commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Context

Adds CLI support for PAM RDP sessions and caps recording chunk size with batched live uploads.
https://linear.app/infisical/issue/PAM-263/add-windows-account-rdp-web-and-cli

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Victor Hugo dos Santos and others added 8 commits June 15, 2026 22:46
…y from Cloudsmith

- Added a notice in the README about the migration to `artifacts-cli.infisical.com` and the impending shutdown of Cloudsmith.
- Implemented a one-time daily notice in the CLI for users regarding the repository migration, including a link to the migration guide.
- Introduced caching for the migration notice to prevent repeated prompts within a 24-hour period.
- Added a manual test for the migration notice display functionality.
…ransition from Cloudsmith

- Revised README to clarify the migration of the Infisical CLI Linux package repository to `artifacts-cli.infisical.com`, including the shutdown date for Cloudsmith.
- Updated the migration guide URL in the code and added a new constant for the Cloudsmith sunset date.
- Enhanced the CLI migration notice to provide clearer instructions for users on repointing their machines and the implications of the migration.
…ation notice code

- Added an integrity check in the APK upload script to ensure that the number of APKs in the staging area matches the count in S3 before proceeding with the sync, preventing incomplete uploads.
- Simplified comments in the migration notice display function to improve clarity and removed the manual test file for the migration notice.
…sage

docs: update README and CLI to reflect migration of Linux package repositoy from Cloudsmith
…ublishing packages. This step ensures sufficient storage by removing unnecessary directories, improving the overall build process efficiency.
fix(release): Enhance release workflow by adding a step to free disk space before publish
fix(release): Set AWS_DEFAULT_REGION in release workflow for S3 uploads and CloudFront invalidation
@infisical-review-police

Copy link
Copy Markdown

💬 Discussion in Slack: #pr-review-cli-269-feat-pam-add-rdp-cli-access-and-recording-chunk-batching

Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel.

carlosmonastyrski and others added 3 commits June 19, 2026 00:49
- Wire Windows/RDP case in StartPAMAccess to use RDPProxyServer
- Use gateway disconnect detection so CLI exits on server-side session termination
@bernie-g bernie-g changed the base branch from main to pam-revamp June 19, 2026 19:28
@bernie-g bernie-g marked this pull request as ready for review June 19, 2026 20:35
@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR activates RDP/Windows PAM sessions through the unified StartPAMAccess path and refactors RDPProxyServer.handleConnection to use the shared NewDisconnectChannels/WaitForDisconnect abstraction that distinguishes gateway-side drops from normal client disconnects.

  • access.go adds startRDPProxy, wiring Windows accounts into the existing switch-dispatch the same way database accounts are handled; the function is nearly identical to startDatabaseProxy but omits the auto-launch step present in StartRDPLocalProxy.
  • rdp-proxy.go replaces the old done chan struct{} (buffer 2, both goroutines shared one channel) with separate gatewayErrCh/clientErrCh channels — a meaningful semantic change, because receiving from gatewayErrCh now triggers a full proxy shutdown via HandleGatewayDisconnect, whereas the client-disconnect path just returns. A race exists between the buffered send to gatewayErrCh and the immediately-following defer connCancel(), which can cause the select to resolve on connCtx.Done() instead and silently skip shutdown.

Confidence Score: 3/5

The RDP proxy integration is straightforward, but the refactored disconnect logic in handleConnection has a real concurrency flaw that can leave the proxy alive after the gateway terminates a session.

The gateway-disconnect race in rdp-proxy.go is not theoretical: Go's asynchronous preemption means the scheduler can context-switch between gatewayErrCh <- err and defer connCancel(), making both select cases simultaneously ready. When that happens, Go randomly chooses between them, and if connCtx.Done() wins, the proxy stays running after an admin-forced termination instead of shutting down cleanly. The rest of the change is a faithful port of the database proxy pattern and looks correct.

packages/pam/local/rdp-proxy.go — specifically the select logic change in handleConnection and how WaitForDisconnect interacts with the deferred connCancel in the gateway goroutine.

Important Files Changed

Filename Overview
packages/pam/local/access.go Enables Windows/RDP accounts in StartPAMAccess via a new startRDPProxy function that closely mirrors startDatabaseProxy; auto-launch of the RDP client is absent compared to the existing StartRDPLocalProxy entry point
packages/pam/local/rdp-proxy.go Refactors handleConnection to use NewDisconnectChannels/WaitForDisconnect, introducing a race where connCtx.Done() can be selected over gatewayErrCh and silently skip HandleGatewayDisconnect on a gateway-side drop

Reviews (1): Last reviewed commit: "fix(pam): write RDP session banner to st..." | Re-trigger Greptile

Comment thread packages/pam/local/rdp-proxy.go
Comment thread packages/pam/local/access.go
Comment thread packages/pam/local/access.go
@veria-ai

veria-ai Bot commented Jun 19, 2026

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

@bernie-g bernie-g requested a review from x032205 June 19, 2026 20:46
@x032205 x032205 merged commit 3e75e41 into pam-revamp Jun 26, 2026
27 of 28 checks passed
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.

5 participants