feat(server): several credentials, client alternate shell and RDPDR drive redirection by org-event · Pull Request #2357 · Devolutions/IronRDP

org-event · 2026-11-03T07:24:24Z

This branch has the server-side changes OpenRDP, an RDP server for macOS built on IronRDP, needs. Three of them are also open as focused PRs, which are easier to review on their own: #2356, #2358 and #2360. Tell me which you prefer: I can close this one in favour of focused PRs, or open focused PRs for the rest too.

What it contains

Several accepted credentials, also feat(server): accept several credentials #2356.
- RdpServer::set_credential_list, RdpServerHandle::set_credential_list, ServerEvent::SetCredentialList.
- Acceptor::set_credential_list.
- CredsspSequence::init_with_identities.
CredSSP checks the password against the entry whose user name matches, and CredentialsProxy::auth_data returns every entry, so SPNEGO clients match against all of them. ClientInfo credentials must match one entry. set_credentials keeps its behaviour as a list of at most one.
RdpServerInputHandler::advanced_input_opened, also feat(server): tell the input handler when the client opens ainput #2358. It is called when the client accepts FreeRDP::Advanced::Input, and forwarded through BoundInputSlot. FreeRDP-based clients open that channel and the Microsoft ones do not. A server can use it to send AVC444 only to clients that decode it, since Windows App for macOS negotiates AVC444 and then drops the connection.
No rustls-pemfile, also build(server): read PEM files with rustls-pki-types #2360. TlsIdentityCtx::init_from_paths reads PEM through rustls-pki-types (PemObject) for any file extension. This clears RUSTSEC-2025-0134.
The client's AlternateShell and WorkingDir in AcceptorResult and ConnectionInfo, kept across a reactivation. A server that runs no programs can read them as client options.
RDPDR drive redirection on the server.
- The server sends Server User Logged On after the capability exchange. Without it clients never announce their drives (MS-RDPEFS 3.1.5.1.5).
- It is sent only when the client's general capability set has RDPDR_USER_LOGGEDON_PDU (MS-RDPEFS 3.3.5.1.5).
- DeviceAnnounceHeader::device_type and the ClientDriveSetInformationResponse fields are public, so a backend can tell drives from printers and read the status of a rename or delete.

Tests

Part	Tests
1	`credssp_accepts_any_of_several_credentials` (testsuite-core); `server_handle_set_credential_list_accepts_any_listed_account` (testsuite-extra e2e)
2	`opening_ainput_tells_the_input_handler`, `the_bound_input_slot_forwards_advanced_input_opened`
3	`tls_identity_reads_pem_files_whatever_their_extension`
4	`alternate_shell_and_work_dir_are_kept_as_sent`
5	`user_logged_on_goes_only_to_a_client_that_advertises_it`; the RDPDR handshake helper now expects the PDU from a client that advertises it

cargo test -p ironrdp-testsuite-core: 2835 passed. cargo xtask check fmt: clean.

Checked against FreeRDP 3.32.2:

several NLA accounts;
AVC444 only for FreeRDP;
/drive folders mounted on macOS through the RDPDR server, with reads, writes, renames and deletes.

An `RdpServer` held one `Credentials`, so a server with more than one account could not use NLA: CredSSP checked every client against that single identity. `RdpServer::set_credential_list`, `RdpServerHandle::set_credential_list` and `ServerEvent::SetCredentialList` take a list. CredSSP checks the client's password against the entry whose user name matches, through `CredsspSequence::init_with_identities`; ClientInfo credentials must match one of the entries. `Acceptor::set_credential_list` does the same for embedders driving the acceptor themselves. `set_credentials` keeps its behaviour as a list of at most one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

FreeRDP-based clients open the advanced input channel and the Microsoft clients do not, so a server can tell them apart, for example to pick a codec only FreeRDP decodes reliably. RdpServerInputHandler::advanced_input_opened has an empty default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

rustls-pemfile is unmaintained (RUSTSEC-2025-0134) and only wraps the parser rustls-pki-types already has. The helper now reads certificates and PKCS#8 keys through `PemObject` for files without a .pem extension too, as it already did for .pem files, and drops the dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-11-03T07:26:59Z

This pull request may overlap with #2356.

The credential-list work matches PR 2356 almost exactly: Acceptor::set_credential_list, CredsspSequence::init_with_identities, ServerEvent::SetCredentialList, RdpServer::set_credential_list, CredentialsProxy over multiple identities, and the credssp_accepts_any_of_several_credentials test. The diff also reproduces PR 2358's advanced_input_opened handler notification and PR 2360's rustls-pemfile removal, bundling several listed PRs' scope.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

The Client Info PDU carries `AlternateShell` and `WorkingDir`, the program a client asks the session to run and its directory. The acceptor parsed and dropped them. `AcceptorResult` and `ConnectionInfo` now carry them, kept across a reactivation like the Client Info flags, so an embedder can act on them, or read them as a client preference when it has no such programs.

A server backend tells a redirected drive from a printer or a smart card by its device type.

A server backend needs the status of a set-information request, a rename or a delete, to know whether it worked.

Clients announce their drives only after the Server User Logged On PDU ([MS-RDPEFS] 3.1.5.1.5), so FreeRDP never offered its /drive folders. The user is authenticated with the connection, so the PDU follows the Client Core Capability Response at once.

Marc-André Moreau (mamoreau-devolutions) · 2026-11-03T16:31:31Z

org-event can you generate a better PR title and description with your AI agent? There's no description, and the title doesn't say much

github-actions

PR 2357 ('Openrdp', empty description) bundles five independent changes: multi-credential CredSSP/ClientInfo support, AlternateShell/WorkingDir exposure, an RDPDR Server User Logged On behavior change, rustls-pemfile removal, and an advanced-input hook. Independent verification confirms the credential-list matching preserves prior semantics and is correctly tested for distinct account names. One real protocol defect stands: ironrdp-rdpdr now sends Server User Logged On unconditionally after the capability exchange, although MS-RDPEFS 3.3.5.1.5 gates it on the client's RDPDR_USER_LOGGEDON_PDU extendedPDU bit, which the decoded response already makes available. Test gaps (empty-list semantics, oracle markers), API-surface duplication (SetCredentials vs SetCredentialList, two-step Acceptor init), an unordered ainput callback, and minor style items are confirmed. The claimed rdpdr visibility widening was rejected: the pre-existing public RdpdrServerBackend trait requires those items to be…

[skeptical] PR bundles five unrelated changes with no description — high 🔴 — crates/ironrdp-server/src/server.rs
The diff couples: (1) multi-credential CredSSP/ClientInfo support across acceptor/credssp/server, (2) AlternateShell/WorkingDir plumbed to AcceptorResult and ConnectionInfo, (3) ironrdp-rdpdr sending Server User Logged On after capability exchange, (4) dropping rustls-pemfile for pki-types PEM iterators, and (5) a new RdpServerInputHandler::advanced_input_opened hook. None depends on another; the title ('Openrdp') and empty body give no motivation, and a maintainer already asked for a description. The rdpdr change alters behavior for every embedder of that crate and is hidden inside a credentials PR; splitting it (and the dependency removal) out is the reviewable path.
[skeptical] CredSSP multi-identity lookup resolves same-account cross-domain entries by list order — low 🟡 — crates/ironrdp-acceptor/src/credssp.rs
auth_data_by_user picks the first entry whose username matches and then overwrites data.username with the requested name (96-106). usernames_match (132-146) returns true when the stored downlevel identity has no NetBIOS domain, regardless of the requested domain. Two listed identities sharing an account name across domains therefore resolve by list order: a client's password is checked against the wrong entry (false rejection), and a client claiming DOMAIN-B\alice can authenticate with DOMAIN-A\alice's password. The new tests only cover distinct account names; the ambiguity needs a documented rule or an explicit domain tiebreaker.
[code-compressor] SetCredentials and SetCredentialList run as parallel but redundant event paths — medium 🟠 — crates/ironrdp-server/src/server.rs
ServerEvent::SetCredentialList is added alongside SetCredentials with its own enum variant, Debug arm, quit-list entry, and dispatch arms at two loop sites (3574-3579 and 4279-4284), plus a RdpServerHandle method and RdpServer setter. Single-credential semantics are a strict subset of the list form (an empty list already accepts any), so two write paths to self.creds now exist. Because ServerEvent is public and not non_exhaustive, the minimal non-breaking fix is for RdpServer::set_credentials to delegate to set_credential_list(creds.into_iter().collect()), collapsing the duplicated assignment; removing the variant entirely would break external exhaustive matches.
[code-compressor] CredentialsProxyImpl::new is a trivial single-use constructor — low 🟡 — crates/ironrdp-acceptor/src/credssp.rs
The private constructor's body is just the struct literal, and the type is crate-private and constructed exactly once (in the new_server closure). The PR already touched its signature; replacing it with a direct CredentialsProxyImpl { credentials: creds } at the call site removes the impl block and a hop with no behavioral change.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

github-actions · 2026-11-03T17:21:12Z

+        // [MS-RDPEFS] 3.1.5.1.5: clients announce their drives only after the
+        // Server User Logged On PDU. The user was authenticated when the
+        // connection was, so it follows the capability exchange at once.
+        Ok(vec![SvcMessage::from(RdpdrPdu::UserLoggedon)])


[protocol] Server User Logged On sent without checking the client's RDPDR_USER_LOGGEDON_PDU capability bit — medium 🟠 — MS-RDPEFS 3.3.5.1.5 requires sending the Server User Logged On PDU only when the client's general capability set advertises RDPDR_USER_LOGGEDON_PDU (0x00000004) in extendedPDU (else 3.3.5.1.7 re-sends Client ID Confirm on logon). The new code returns RdpdrPdu::UserLoggedon unconditionally after every ClientCoreCapabilityResponse, although response.capabilities — including the already-modeled ExtendedPdu::RDPDR_USER_LOGGEDON_PDU flag (efs.rs 987) — is in scope for the guard. Non-advertising clients receive an unsolicited PDU, and the comment cites 3.1.5.1.5 (client side) rather than the governing server-side 3.3.5.1.5.

github-actions · 2026-11-03T17:21:12Z

+        let handler = Arc::clone(&self.handler);
+        task::spawn_blocking(move || {
+            handler.blocking_lock().advanced_input_opened();
+        });


[skeptical] advanced_input_opened notification is not ordered before advanced-input mouse events — low 🟡 — AInputHandler::start dispatches advanced_input_opened via task::spawn_blocking, and ClientPdu::Mouse dispatches mouse() the same way (1015-1018). Both contend for the same tokio Mutex from independent blocking-pool tasks, so the pool may execute a mouse task first; nothing guarantees the callback precedes the first advanced-input mouse event. An embedder using the hook to initialize state for advanced input can observe events before notification. Either invoke it synchronously where possible or document that no ordering holds.

github-actions · 2026-11-03T17:21:12Z

+        let mut acceptor = Acceptor::new(security.flag(), desktop_size, capabilities, None);
+        acceptor.set_credential_list(creds);


[skeptical + code-compressor] Acceptor credentials have two parallel construction paths (Option<Credentials> constructor plus list setter) — low 🟡 — Acceptor::new still takes Option<Credentials> while the internal storage became Vec<Credentials>, so callers wanting a list construct with None and then call set_credential_list — ironrdp-server's PendingConnection::new does exactly this (1919-1920), and the constructor keeps an obsolete Option->Vec conversion. A single Vec<Credentials> (or impl IntoIterator<Item = Credentials>) parameter accepts Some(cred)-style callers and lists alike, eliminating the setter-for-construction use and the transient no-credentials state. This is a breaking change to ironrdp-acceptor's public constructor; the PR already extends AcceptorResult and reworks credentials, so the extra break is marginal.

github-actions · 2026-11-03T17:21:12Z

+    // With HYBRID_EX the delegated credentials reach the handler during CredSSP.
+    if protocol == SecurityProtocol::HYBRID_EX && accepted {
+        assert_eq!(handler.received.len(), 1);
+        assert_eq!(handler.received[0].credentials.username, username);
+        assert_eq!(handler.received[0].credentials.password, password);
+    } else if !accepted {
+        assert!(handler.received.is_empty());
+    }


[test] Parametrized CredSSP test body branches on its parameters, hiding several tests behind one name — low 🟡 — credssp_accepts_any_of_several_credentials parametrizes over username/password/accepted and protocol, then branches: the HYBRID_EX && accepted arm asserts delegated credentials on the handler, the !accepted arm asserts the handler got nothing, and HYBRID + accepted asserts nothing. A conditional expected value per combination is several tests behind one name, so a failure reports a case row rather than the behavior that broke. Keep the rstest as the acceptance table asserting result.is_ok() == accepted only, and move the HYBRID_EX delegation and rejection-leaves-handler-empty checks into separate named tests sharing the setup helpers.

github-actions · 2026-11-03T17:21:12Z

+#[rstest::rstest]
+#[case::first_user("alice", "secret", true)]
+#[case::second_user("bob", "hunter2", true)]
+#[case::other_users_password("bob", "secret", false)]
+#[case::unknown_user("carol", "secret", false)]
+#[tokio::test]


[test] credssp_accepts_any_of_several_credentials has no oracle marker — low 🟡 — Oracle markers are the established convention in this suite, and the sibling new test alternate_shell_and_work_dir_are_kept_as_sent is the only new test in this PR marked. The expected accepted/rejected results here are independently derivable from the credentials the test seeds via set_credential_list and the client's username/password, but no marker names that source. Add an oracle line above the test so the next review can classify it cheaply.

github-actions · 2026-11-03T17:21:12Z

+    // [MS-RDPEFS] 3.1.5.1.5: the Server User Logged On PDU lets the client announce its drives.
+    let logged_on = server.process(&capability_response).unwrap();
+    assert_eq!(logged_on.len(), 1);
+    assert_eq!(
+        logged_on[0].encode_unframed_pdu().unwrap(),
+        encode_vec(&RdpdrPdu::UserLoggedon).unwrap()
+    );


[test] handshake_to_active helper asserts new User Logged On timing without an oracle marker — low 🟡 ❓ — The shared helper — used by ~19 rdpdr tests — now asserts the server answers ClientCoreCapabilityResponse with exactly one User Logged On PDU. The cited MS-RDPEFS 3.1.5.1.5 explains why clients need the PDU before announcing drives but does not pin send-at-once-after-capability-exchange; that sequencing is this PR's design decision in rdpdr/src/server.rs. Add an oracle line and confirm with the author which source decides the timing, so the marker cites text that actually decides the assertion. Note the underlying send itself has a separate protocol defect (missing capability-bit guard) that may change this assertion.

github-actions · 2026-11-03T17:21:12Z

+                        && !self.creds.is_empty()
+                        && !self.creds.iter().any(|expected| credentials_match(expected, &creds))


[test] Empty-credential-list semantics (accept any ClientInfo, fail CredSSP) are documented but untested — medium 🟠 — The PR makes an empty creds list skip ClientInfo validation entirely (1565-1566, accepting arbitrary client-supplied credentials on non-CredSSP paths) while lib.rs fails CredSSP on an empty list, and the doc comments state both halves as intended (mirroring the old None). No added or existing test exercises set_credential_list(vec![]): neither that arbitrary Client Info credentials are accepted, nor that CredSSP fails. This is security-relevant acceptance behavior introduced by this diff; each half needs a named test pinning it (the author's documented intent is the apparent source).

github-actions · 2026-11-03T17:21:13Z

+    fn advanced_input_opened(&mut self) {
+        let mut bound = self.bound.lock().expect("bound input lock poisoned");
+        if let Some(handler) = bound.as_mut() {
+            handler.advanced_input_opened();
+        } else {
+            drop(bound);
+            self.default.advanced_input_opened();
+        }
+    }


[code-compressor] advanced_input_opened is a third verbatim copy of the bound-input dispatch block — low 🟡 — advanced_input_opened (645-653) repeats line-for-line the lock/bound-check/fallback pattern of BoundInputSlot::keyboard (625-633) and mouse (635-643). A private helper such as fn dispatch(&mut self, f: impl FnOnce(&mut dyn RdpServerInputHandler)) that locks once and invokes the closure on the bound handler or the default makes all three trait methods one-liners and the next event additive. Behavior-preserving: same lock, same poison message, same drop-before-fallback ordering.

github-actions · 2026-11-03T17:21:13Z

+    /// The program the client asked the session to run instead of the shell,
+    /// from its Client Info PDU ([MS-RDPBCGR] 2.2.1.11.1.1, `AlternateShell`).
+    /// Empty when it asked for none. A server without such programs may read
+    /// it as a client preference.
+    pub alternate_shell: String,
+    /// The working directory for [`Self::alternate_shell`] (`WorkingDir`); empty when none.
+    pub work_dir: String,


[rust-style] alternate_shell/work_dir docs cite MS-RDPBCGR 2.2.1.11.1.1 without a reference-style link — low 🟡 — The added doc comments cite the section inline as ([MS-RDPBCGR] 2.2.1.11.1.1, AlternateShell) and (WorkingDir) with no link target, while the neighboring pre-existing client_info_flags doc (453-462) already defines the reference-style [2.2.1.11.1.1] link for the same Info Packet section. Reuse that link in the new field docs per the doc-comment convention.

MS-RDPEFS 3.3.5.1.5 has the server send Server User Logged On to a client whose general capability set has RDPDR_USER_LOGGEDON_PDU in extendedPDU. It went to every client after the capability exchange. `CapabilityMessage::user_logged_on_supported` reads the bit, and the server checks it in the Client Core Capability Response.

org-event · 2026-11-03T19:42:10Z

Thanks for the review, Marc-André Moreau (@mamoreau-devolutions), and for the automated one. I've split this branch into focused pull requests, one change each, so they can be reviewed and merged on their own:

feat(server): accept several credentials #2356 feat(server): accept several credentials
feat(server): tell the input handler when the client opens ainput #2358 feat(server): tell the input handler when the client opens ainput
build(server): read PEM files with rustls-pki-types #2360 build(server): read PEM files with rustls-pki-types
feat(server): pass the client's alternate shell and working directory #2361 feat(server): pass the client's alternate shell and working directory
fix(rdpdr): send Server User Logged On after the capability exchange #2362 fix(rdpdr): send Server User Logged On after the capability exchange (now gated on RDPDR_USER_LOGGEDON_PDU, as the automated review pointed out)
feat(rdpdr): let server backends read device types and set-information status #2363 feat(rdpdr): let server backends read device types and set-information status

The review points that apply to them are addressed there: separate named tests, empty-list coverage, oracle markers, one dispatch helper in BoundInputSlot, and the ordering note on advanced_input_opened. Closing this one in favour of those.

Ivan Zaharov and others added 4 commits November 3, 2026 09:57

fix(server): forward advanced_input_opened through the bound input slot

03deb38

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

org-event temporarily deployed to llm-providers November 3, 2026 07:25 — with GitHub Actions Inactive

org-event temporarily deployed to llm-providers November 3, 2026 11:55 — with GitHub Actions Inactive

feat(rdpdr): make DeviceAnnounceHeader::device_type public

373b807

A server backend tells a redirected drive from a printer or a smart card by its device type.

org-event temporarily deployed to llm-providers November 3, 2026 12:19 — with GitHub Actions Inactive

feat(rdpdr): make ClientDriveSetInformationResponse fields public

0873ecd

A server backend needs the status of a set-information request, a rename or a delete, to know whether it worked.

github-actions Bot added automation-failed Exact-head automated classification or review failed or was unavailable risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny scope/core Touches the core architectural tier labels Nov 3, 2026

org-event temporarily deployed to llm-providers November 3, 2026 12:23 — with GitHub Actions Inactive

org-event temporarily deployed to llm-providers November 3, 2026 12:26 — with GitHub Actions Inactive

github-actions Bot removed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries automation-failed Exact-head automated classification or review failed or was unavailable labels Nov 3, 2026

github-actions Bot reviewed Nov 3, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor labels Nov 3, 2026

org-event changed the title ~~Openrdp~~ feat(server): several credentials, client alternate shell and RDPDR drive redirection Nov 3, 2026

org-event temporarily deployed to llm-providers November 3, 2026 18:04 — with GitHub Actions Inactive

org-event mentioned this pull request Nov 3, 2026

Send Server User Logged On only to clients that take it org-event/OpenRDP#24

Merged

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Nov 3, 2026

org-event mentioned this pull request Nov 3, 2026

Openrdp multi credentials #2359

Closed

org-event temporarily deployed to llm-providers November 3, 2026 18:09 — with GitHub Actions Inactive

This was referenced Nov 3, 2026

feat(server): accept several credentials #2356

Open

feat(server): tell the input handler when the client opens ainput #2358

Open

org-event closed this Nov 3, 2026

		let mut acceptor = Acceptor::new(security.flag(), desktop_size, capabilities, None);
		acceptor.set_credential_list(creds);

		&& !self.creds.is_empty()
		&& !self.creds.iter().any(\|expected\| credentials_match(expected, &creds))

Conversation

org-event commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it contains

Tests

Uh oh!

github-actions Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marc-André Moreau (mamoreau-devolutions) commented Nov 3, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

org-event commented Nov 3, 2026

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

org-event commented Nov 3, 2026 •

edited

Loading

github-actions Bot commented Nov 3, 2026 •

edited

Loading