Skip to content

feat(server): accept several credentials - #2356

Open
org-event wants to merge 3 commits into
Devolutions:masterfrom
org-event:server-credential-list
Open

org-event wants to merge 3 commits into
Devolutions:masterfrom
org-event:server-credential-list

Conversation

@org-event

@org-event org-event commented Nov 3, 2026 •

Copy link
Copy Markdown

An RdpServer holds a single Credentials, so a server with more than one account cannot use NLA: CredSSP checks every client against that one identity.

This adds a list of accepted credentials:

  • RdpServer::set_credential_list, RdpServerHandle::set_credential_list and ServerEvent::SetCredentialList replace the accepted set for subsequent connections.
  • CredSSP checks the client's password against the entry whose user name matches, through CredsspSequence::init_with_identities. CredentialsProxy::auth_data returns every entry, so a SPNEGO client is matched against all of them.
  • ClientInfo credentials (TLS and RDP security) must match one of the entries.
  • Acceptor::set_credential_list does the same for embedders that drive the acceptor themselves.

set_credentials and CredsspSequence::init keep their behaviour: they are now a list of at most one entry. None (no credentials configured) keeps its behaviour from #1150: ClientInfo credentials are not checked. A list, even an empty one, requires a match: an empty list refuses every ClientInfo logon unless a credential validator decides them, and CredSSP fails.

Tests

testsuite-core:

  • credssp_accepts_any_of_several_credentials: the accept/refuse table for HYBRID and HYBRID_EX.
  • credssp_hands_the_matching_account_to_the_handler: with HYBRID_EX the delegated credentials are those of the matching account.
  • refused_credssp_logon_hands_nothing_to_the_handler.
  • an_empty_credential_list_fails_credssp and an_empty_credential_list_refuses_client_info_credentials.
  • no_credentials_leave_client_info_credentials_unchecked and an_empty_credential_list_leaves_client_info_credentials_to_the_validator.
  • client_info_credentials_must_match_an_accepted_account.

testsuite-extra:

  • server_handle_set_credential_list_accepts_any_listed_account: an e2e connection as the second listed account.

The table and the ClientInfo test fail when the lookup checks only the first entry.

Refs #1357.

@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/cross-cutting Spans multiple architectural boundaries size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure labels Nov 3, 2026
Ivan Zaharov and others added 2 commits November 3, 2026 22:15
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>
Splits the several-credentials CredSSP table from the checks on what
reaches the setup handler, and pins the cases the list introduces:
- the delegated credentials are those of the matching account;
- a refused logon hands nothing to the handler;
- an empty list fails CredSSP and leaves Client Info credentials
  unchecked, as no credentials at all do;
- Client Info credentials must equal one of the accounts.
@org-event
org-event force-pushed the server-credential-list branch from 882ddbb to cdf9967 Compare November 3, 2026 19:18
@github-actions github-actions Bot added triage/overlap Possible overlap with another pull request; advisory only and removed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries labels Nov 3, 2026
@glamberson

Copy link
Copy Markdown
Contributor

An empty list shouldn't accept everyone on the TLS path.

On master, RdpServerHandle::set_credentials takes a Credentials, so nothing can open a running server through the handle. set_credential_list adds that: Send it an empty list, for example after an account store reloads with no rows, and with no validator any client gets in. An empty list should refuse ClientInfo logons, and an_empty_credential_list_accepts_any_client_info_credentials should assert that.

None meaning no requirement came from my fix in #1150. It was a workaround, and CBenoit's objection there was that forgetting to configure authentication can't be told apart from skipping it on purpose. I wouldn't carry that into a new API. The ClientInfoCredentialsPolicy he sketched in that review would fit a list.

This should also reference #1357, which he keeps open as the anchor for credential changes.

An empty list from `set_credential_list` left ClientInfo credentials
unchecked, as `None` does, so a list that emptied at runtime (an account
store reloading with no rows) opened a server without a credential
validator to any client on the TLS path.

The acceptor and the server now keep `Option<Vec<Credentials>>`: `None`
still means no credentials were configured and keeps the Devolutions#1150
behaviour, while a list, empty or not, requires a match. An empty list
refuses every ClientInfo logon unless an external validator decides
them, and CredSSP still fails.

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

Copy link
Copy Markdown
Author

Thanks, agreed: an empty list shouldn't open the server.

Fixed in 8dcf6b2. The acceptor and the server now keep Option<Vec<Credentials>>:

  • None, nothing configured, keeps the fix(acceptor): skip credential check when server credentials are None #1150 behaviour, so set_credentials(None) and Acceptor::new(.., None) don't change for existing users.
  • A list from set_credential_list, empty included, requires a match. An empty list refuses every ClientInfo logon, and CredSSP still fails.
  • With set_allow_unverified_credentials, the external validator still decides ClientInfo logons.

The test is now an_empty_credential_list_refuses_client_info_credentials, next to no_credentials_leave_client_info_credentials_unchecked and an_empty_credential_list_leaves_client_info_credentials_to_the_validator.

I kept ClientInfoCredentialsPolicy out of this PR so it stays about the list. I can do it as a follow-up under #1357, or fold it in here if you and CBenoit prefer. I've added the #1357 reference to the description.

@github-actions github-actions Bot removed scope/cross-cutting Spans multiple architectural boundaries triage/overlap Possible overlap with another pull request; advisory only labels Nov 4, 2026

This branch was previously deployed

1 inactive deployment
llm-providers — 8dcf6b28 Deployed Nov 4, 2026 by org-event via Classify pull request #12943
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/protocol Affects RDP or related protocol behavior risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure

Development

Successfully merging this pull request may close these issues.

2 participants