Devolutions / Devolutions/IronRDP

ConnectionHandler: observation-only auth-outcome hook, or route through CredentialValidator?

Open
#1,484 5 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
3.2k
Forks
275
Avg merge
1d 11h
Merged PRs (30d)
189

Description

## Question

`ConnectionHandler` reports connection accept (`on_accept`) and teardown (`on_disconnected`), but there's no way to **observe the client authentication outcome** in between. With the `CredentialValidator` seam now in place, I wanted to ask about the intended pattern before proposing anything: should observing "did this client authenticate?" live as an observation hook on `ConnectionHandler`, or do you consider it something to route through `CredentialValidator`?

## Why the existing seams don't quite cover it

- **`on_disconnected(error)`** conflates an auth failure with any other disconnect — a bad password and a benign early network drop both surface as "ended with an error," so you can't cleanly tell them apart.
- **`CredentialValidator`** is a *decision* seam, not an *observation* one:
- It only runs when a validator is **installed** — a server that authenticates against **static credentials** (`RdpServerOptions`/`self.creds`, no custom validator) never invokes `validate()`, so there's nothing to observe there.
- Using it purely to *watch* outcomes forces a consumer to take over validation (return `CredentialDecision`), coupling observation to the decision logic.
- It fires on credential *presentation*; it wouldn't capture a client that aborts CredSSP before presenting anything, or a transport error mid-exchange — cases an audit trail cares about ("auth did not complete" vs "credentials rejected").

## Motivation

A downstream single-session server (uses static credentials, no custom validator) wants an **audit/metrics** record of the real login verdict — distinguishing *credentials rejected* from *auth never completed* — for things like a SIEM stream or fail2ban-style lockout. Today that has to be inferred from connection duration / disconnect error, which is ambiguous.

## Proposed shape (if a hook is the right home)

A default-no-op observation method on `ConnectionHandler`, fired once around `accept_credssp` under `RdpServerSecurity::Hybrid` — after the TLS upgrade (so a pre-TLS negotiation blip can't be mistaken for an auth failure) and before `on_disconnected`:

```rust
fn on_authenticated(&mut self, success: bool, reason: Option<&str>) {
let _ = (success, reason);
}
```

`Ok` → `on_authenticated(true, None)`; `Err(e)` → `on_authenticated(false, Some(&e.to_string()))`, then the error is propagated unchanged. Additive and non-breaking (default no-op; the auth path is byte-identical when no handler is installed).

I have a small ready patch (+30/-2, one file) implementing exactly this if the hook direction is what you'd want — but I'd rather confirm the fit first than push a seam that overlaps `CredentialValidator` if you'd prefer to consolidate auth extensibility there. Happy to go whichever way you prefer.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at ConnectionHandler and the accept_credssp path under RdpServerSecurity::Hybrid, then compare it with the CredentialValidator seam and static RdpServerOptions credentials. Confirm which seam should own observation and define completion as distinguishing successful authentication from rejected or incomplete authentication while preserving existing error propagation.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
authentication, networking
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.