fix(sspi): don't split UPNs into username/domain#475
fix(sspi): don't split UPNs into username/domain#475Alex Yusiuk (RRRadicalEdward) wants to merge 1 commit into
Conversation
|
Hi Benoît Cortier (@CBenoit). Could you have a look? |
|
HI Benoît Cortier (@CBenoit)! Did you miss this one, or is there a reason you don't merge it? |
Hi! Thanks for the reminder. I didn’t merge yet because I think the API change may need a slightly different approach. I’d like to explore that direction before merging this as-is. |
PR Devolutions#475's visibility downgrade (pub -> pub(crate)) on account_name(), domain_name(), inner(), and format() breaks ironrdp-acceptor 0.8.0, which calls these methods from outside the crate. Keep the behavior fix from PR Devolutions#475 (UPN account_name returns full UPN, domain_name returns None for UPN format) but revert the visibility change so the existing public API surface is preserved.
|
Hi Benoît Cortier (@CBenoit) / Alex Yusiuk (@RRRadicalEdward) — adding a real-world data point that might help shake this loose, plus what I think might be the different API approach that you referenced Context: I'm running an IronRDP-based implementation that authenticates users against Active Directory forests where alternate UPN suffixes are registered (i.e. an AD where a user's userPrincipalName attribute is set to user@example.com while the actual NetBIOS domain is EXAMPLE). My targets are typically reached by raw IP, so CredSSP falls back to NTLM (no SPN to build for Kerberos against an IP). Without this PR, sspi-rs splits the UPN at @ and emits NTLM AUTHENTICATE with UserName=user, DomainName=example.com. The target Windows VM treats example.com as a NetBIOS domain name, fails to find a DC for it (the actual NetBIOS is EXAMPLE), and returns STATUS_LOGON_FAILURE [0xc000006d]. With this PR's behavior fix applied, NTLM goes out as UserName=user@example.com, DomainName="", the target's LSA does GC-based UPN resolution against the registered alternate suffix, and authentication succeeds — matching what FreeRDP-based stacks do for the same scenario. I've confirmed this against multiple separate AD forests with no trust relationships between them; in each case the workspace authenticates as long as the suffix is registered in the target forest. On the API design concern you flagged: I think I hit it directly when I tried to compile against this PR. The visibility downgrade on account_name(), domain_name(), inner(), and format() (pub → pub(crate)) breaks ironrdp-acceptor 0.8.0 — its credssp module calls username.account_name() from outside the crate. Compile error: error[E0624]: method If the goal of the visibility downgrade was to reduce the API surface so callers wouldn't accidentally rely on the split-UPN semantics, I'd argue the behavior fix already accomplishes that — once account_name() returns the full UPN and domain_name() returns None for UPN-format usernames, callers reading those values get the new semantics regardless of visibility. Keeping them pub preserves the existing API contract for consumers like ironrdp-acceptor (which uses them for principal-name comparison, not for splitting). My fork carries the behavior change from this PR with the visibility kept pub, and ironrdp-acceptor compiles + works correctly through both server-side and WASM-client CredSSP paths. Originally I'd considered an additive approach — a new Username::new_raw_upn(value: &str) constructor that produces the raw-UPN data shape (format=UPN, sep_idx=None, value=full_upn), paired with a new Credentials::UsernamePasswordRawUpn variant in ironrdp-connector so callers opt in explicitly. That preserves existing behavior for any consumer who happens to depend on the current split semantics. The downside is two parallel APIs — and arguably a UPN-format Username should return the full UPN from account_name(), which is what this PR ultimately does. Happy to help land whichever direction you prefer: Behavior fix only (this PR's change minus the visibility downgrade) — minimal, doesn't break downstream consumers, and is what I'm running today. Either way, this fix unblocks a real-world IronRDP deployment, so anything I can do to help land it would be appreciated. |
No description provided.