Devolutions / Devolutions/IronRDP

Untangle `ironrdp-session` from `ironrdp-connector` and make sspi/NLA an injectable, optional dependency

Open
#1,422 0 comments 0 reactions 0 assignees View on GitHub
kind/technical-debt scope/core
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.