Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 9, 2025

Addresses feedback on PR #1614 regarding code organization, duplication, and API surface of the credential injection implementation.

Changes

  • Reduce API surface: Changed pubpub(crate) for CredSSP functions (perform_credssp_with_server, perform_credssp_with_client, extract_tls_server_public_key, get_cached_gateway_public_key, GetPeerCert) to keep implementation details internal

  • Optimize trait bounds: Removed unnecessary 'static lifetime from handle() - only AsyncRead + AsyncWrite + Unpin + Send + Sync required

  • Eliminate duplication: Removed redundant authorization, connection mode validation, and session ID recording in handle_with_credential_injection() - now delegated to process_cleanpath()

  • Extract CredSSP MITM orchestration: Created perform_credssp_mitm() in rdp_proxy module to encapsulate stream framing, parallel CredSSP execution, and leftover byte forwarding

  • Avoid token re-extraction: Use as_deref() instead of re-parsing token from PDU

Before/After

Before:

// Duplicate authorization
let claims = authorize(...)?;
let CleanPathResult { .. } = process_cleanpath(...)?; // Also does authorization

// Inline CredSSP orchestration
let mut client_framed = TokioFramed::new(client_stream);
let mut server_framed = TokioFramed::new(server_stream);
// ... 30+ lines of CredSSP setup and cleanup

After:

// Authorization happens once in process_cleanpath
let CleanPathResult { claims, .. } = process_cleanpath(...)?;

// Clean delegation
let (client_stream, server_stream) = perform_credssp_mitm(
    client_stream, server_stream, /* ... */
)?;

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Update implementation based on feedback for credential injection Refactor RDCleanPath credential injection to reduce duplication and improve encapsulation Dec 9, 2025
Copilot AI requested a review from CBenoit December 9, 2025 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants