Devolutions / Devolutions/IronRDP
Untangle `ironrdp-session` from `ironrdp-connector` and make sspi/NLA an injectable, optional dependency
- Dominant language
- Rust
- Stars
- 3.2k
- Forks
- 275
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 189
Description
Umbrella tracking issue for a staged refactoring with two goals:
1. **Let decode-only consumers use `ironrdp-session` without pulling the connector/sspi stack.** Today `ironrdp-session` still depends on `ironrdp-connector` (only for `ConnectionResult` and `ConnectionActivationSequence`), which drags in `sspi` (with `scard`) and its ~150-crate tree, even for a consumer that only decodes fast-path/bitmap/RFX updates.
2. **Make NLA/CredSSP a dependency-injected, optional backend** so that *any* consumer that doesn't perform NLA (not just decode-only) sheds sspi, and embedders can plug in their own auth backend (platform SSPI, GSSAPI, …).
Builds on #1419, which already relocated the pure-PDU `legacy` module from `ironrdp-connector` into `ironrdp-pdu`.
### Key findings that shape the design
- `ironrdp-session` never *drives* the connection-activation sequence — it stores it, `reset_clone()`s it on Deactivate-All, and hands it back out. The consumer already owns the reactivation drive loop (see `ironrdp-client/src/rdp.rs`). So session's involvement is pure ceremony, and the only remaining connector types it touches are `ConnectionResult` and `ConnectionActivationSequence`.
- The `Sequence` state machine (nego, channel-connection, license, activation, finalization) produces **no** sspi errors. `sspi::Error` enters in exactly one place (`connector/src/credssp.rs`), and NLA is already driven separately from the `Sequence` loop (`perform_credssp_step` in `ironrdp-async`/`ironrdp-blocking`). So the `Sequence` trait and its error can be sspi-free, and CredSSP can sit behind a trait.
- `Config`/`Credentials`/`KerberosConfig`/`SmartCardIdentity` are already sspi-agnostic raw types; all sspi/picky translation is confined to `credssp.rs`.
### Target architecture
New/changed crates and dependency edges:
```
ironrdp-sequence (Sequence, State, Written, SequenceError, ServerName, DesktopSize) [sspi-free]
ironrdp-credssp (CredsspProvider/Backend traits, CredsspError, Credentials, noop) -> ironrdp-sequence
ironrdp-credssp-impl (vetted backend over the `sspi` crate + picky) -> ironrdp-credssp
ironrdp-connector (ClientConnector, Config, nego, activation/finalization, ...) -> (ironrdp-sequence, ironrdp-credssp) [no sspi]
ironrdp-session (decode/render + input encode) -> (pdu, core, svc, dvc, displaycontrol, graphics, bulk) [no connector, no sspi]
ironrdp-acceptor -> (ironrdp-sequence, ironrdp-credssp) [no connector]
final consumer -> (ironrdp-connector, ironrdp-credssp-impl, ironrdp-session)
```
### Error model (end state)
The global `ConnectorError`/`ConnectorResult` is retired in favor of three focused types:
- `SequenceError` (in `ironrdp-sequence`) — `Encode`/`Decode`/`Reason`/`General`/`Custom`/`Negotiation`. Returned by the whole `Sequence` chain and by `single_sequence_step` (framed-read I/O errors fold into `Custom`, as today). No sspi.
- `CredsspError` (trait, in `ironrdp-credssp`) — `kind()` covers `WrongPassword`/`LogonFailure`/`AccessDenied`/`Network`/`Protocol`/`Other`. `AccessDenied` moves here (it's an `EarlyUserAuthResult` outcome, currently `credssp.rs:199`). sspi lives only in `ironrdp-credssp-impl`.
- `ConnectError` (in `ironrdp-connector`) — the load-bearing connect-flow union `{ Sequence(SequenceError), Credssp(Box) }`, returned by `connect_begin`/`connect_finalize`. This is the *only* place the two failure domains are unified; granular code returns `SequenceError` or `CredsspError` directly.
### Plan (phased roadmap)
Sub-issues are linked natively (below) and their ordering is enforced through issue dependencies ("blocked by"/"blocks"). Each item is one PR.
- **Phase 1 — Decouple `ironrdp-session`:** #1423, #1424
- **Phase 2 — Extract `ironrdp-sequence`:** #1425, #1426
- **Phase 3 — CredSSP dependency injection (additive-deprecation window):** #1427, #1428, #1429, #1430, #1431
- **Phase 4 — Decouple `ironrdp-acceptor`:** #1432
- **Phase 5 — Converge (remove the deprecated surface):** #1433
- **Cross-cutting (independent):** #1434
### PR atomicity (important for whoever splits/implements)
CI gates merges: a `cargo build --workspace` check must stay green, so a PR that leaves the workspace un-compilable cannot be merged. Any breaking change to an in-workspace crate's public API **must** fix every in-workspace consumer in the same PR. Notable points:
- **#1425** changes `Sequence::step`'s return type → the same PR must update every `impl Sequence` in `ironrdp-connector` **and** `ironrdp-acceptor`, `single_sequence_step` in `ironrdp-async`/`ironrdp-blocking`, and the reactivation drive loops in `ironrdp-client`/`ironrdp-web` that call it directly.
- **#1426** stays non-breaking to downstream **only** because `ironrdp-connector` re-exports the moved types (`pub use ironrdp_sequence::…`). Keep those re-exports or the footprint explodes.
- **#1429** (`ironrdp-credssp-impl`) is additive — `ironrdp-connector/src/credssp.rs` stays in place until #1433.
- **The CredSSP transition (#1427–#1433) is deliberately an additive-deprecation window, not a single breaking cutover.** #1430 adds the trait-based API and the focused `ConnectError` type *alongside* the old ones (the global `ConnectorError` and the old `connect_*` become `#[deprecated]`); consumers migrate in #1431 and #1432; the old sspi-based surface, the global `ConnectorError`, `pub use sspi`, and the `sspi`/`picky` deps are removed only in #1433. The precursor #1427 removes the `ironrdp_connector::sspi` re-export as a breaking axis ahead of time, so #1433 is a clean deletion.
### Compatibility & deprecation policy
Converging to the target architecture is the priority — compatibility is a courtesy, never a reason to preserve the old shape. But when consumers can be kept compiling with minimal churn *without leaving untracked debt*, do so. Two distinct cases:
- **Debt-free facade (no `#[deprecated]`).** Re-export a type from its new home when the crate legitimately wants it in its public surface. This is permanent and carries no deprecation. Example: `ironrdp-connector` re-exporting `Sequence`/`Written`/`SequenceError`/`ServerName`/`DesktopSize` from `ironrdp-sequence` (#1426) — those types belong in the connector's API, so the re-export stays.
- **Tracked shim (`#[deprecated]` required).** When a kept item is meant to be removed, it must carry `#[deprecated(since = "", note = "; see Devolutions/IronRDP#")]` plus an inline doc comment explaining the migration and linking the tracker issue that owns its removal. (Leave `""` verbatim; the actual version number will be computed in the release-plz PR.) Example: the old `connect_*` and the global `ConnectorError`/`ConnectorResult` kept during the CredSSP window are deprecated in #1430 and removed in #1433. Every deprecated shim is owned by exactly one tracked issue, and removing it is that issue's acceptance criterion.
A required xtask/CI guard (#1434) fails on any `#[deprecated]` whose `note` lacks a `Devolutions/IronRDP#` reference, keeping this policy enforced.
Contributor guide
No contributing guide indexed for this repository
Research direction
This is an umbrella refactoring issue rather than a single implementation task. Read the phased roadmap and start with the linked sub-issues in Phase 1; inspect the named crates and consumers, and use `cargo build --workspace` as the compatibility check. Done means the staged dependency and authentication changes are completed without leaving the workspace uncompilable.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100