Devolutions / Devolutions/sspi-rs

Remove `Username::domain_name()` in favor of `Username::parts()`

Open
#708 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
83
Forks
50
Avg merge
3d 12h
Merged PRs (30d)
9

Description

### Background

`Username::domain_name()` returns `Option<&str>` but the `&str` means two different things depending on the internal format:

- **UPN** (`user@corp.example.com`) → returns the UPN **suffix** (`corp.example.com`)
- **Down-level logon name** (`CORP\user`) → returns the **NetBIOS domain** (`CORP`)

A UPN suffix and a NetBIOS domain are not interchangeable, but the accessor hands them back through the same type with no signal of which one you got. A caller that wants "the NetBIOS domain for a down-level logon" and calls `domain_name()` on a UPN silently receives a DNS-style suffix instead, the wrong value, shaped plausibly enough to flow downstream undetected.

#709 adds a format-tagged replacement, `Username::parts() -> UsernameParts<'_>`, whose UPN arm exposes only `suffix` and whose down-level arm exposes only `netbios_domain: Option<&str>`. Because the enum is matched exhaustively, treating a UPN suffix as a NetBIOS domain no longer compiles. `domain_name()` is deprecated in that PR; this issue tracks its removal, which is a breaking change and must ship in its own `fix!`/`feat!` release.

### Why this is a real footgun, not a theoretical one

`impl From for AuthIdentityBuffers` writes `domain_name().unwrap_or_default()` straight into the `domain` field of `AuthIdentityBuffers`, a slot consumed as a NetBIOS domain by the NTLM/Kerberos paths. For a UPN identity this places the DNS suffix where a NetBIOS domain is expected. This is the exact confusion the type-level split is meant to make impossible.

### External impact

`Username` is part of the published `sspi` crate's public API and is used by downstream consumers (IronRDP, Devolutions Gateway, and third parties). Removal is a breaking change:

### Checklist

- [ ] Land the deprecation (companion PR) in a minor release first, giving downstreams a compiler-visible migration window.
- [ ] Remove in a later dedicated breaking release, commit marked `fix!`/`feat!` with a `BREAKING CHANGE:` footer.

### Migration example for downstreams

```rust
// Before
let domain = username.domain_name().unwrap_or_default();

// After
let domain = match username.parts() {
UsernameParts::DownLevelLogonName { netbios_domain, .. } => netbios_domain.unwrap_or_default(),
UsernameParts::UserPrincipalName { .. } => "", // a UPN has no NetBIOS domain
};
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by searching the repository for Username::domain_name(), especially the AuthIdentity to AuthIdentityBuffers conversion, and read the Username::parts() API introduced by #709. Check all in-repository callers and relevant tests for migration to the format-specific match. Done means the deprecated accessor is removed, callers are migrated, and the breaking release commit includes the required fix!/feat! marker and BREAKING CHANGE: footer.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
authentication
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.