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

[clintcan]

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:

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.

[maryny4]

Another downstream data point for the ConnectionHandler-hook direction.

hypr-rdp (a Wayland RDP server on ironrdp-server) just declined a session-hooks feature (run a user command when a session starts/ends — monitor scale switching, DPMS wake) precisely because the current seams can't express it safely: on_accept fires pre-TLS/pre-CredSSP, so a connect-and-close probe is indistinguishable from a session, and on_disconnected(error) conflates auth failures with benign drops — MuNeNiCK/hypr-rdp#27 has the full analysis.

From that use case, requirements an observation hook would ideally satisfy:

1. Fired strictly post-auth, as proposed — on_authenticated(true, None) is exactly a safe "session started" boundary, and pairing it with the already-guaranteed on_disconnected gives an ordered do/undo lifecycle.
2. TLS-only servers (no NLA configured) never run CredSSP — worth defining what the hook means there (fire on connection activation? never?), so passwordless setups don't silently lose the feature.
3. The feat(server)!: expose per-connection keyboard metadata via ConnectionHandler #1691 observation applies here too: embedders driving run_connection directly never see ConnectionHandler — worth keeping the new hook reachable for them.

Happy to test the patch against hypr-rdp's session-hooks use case once it's up.

[clintcan]

Chiming in as the downstream that opened this — macrdp ships exactly this hook today (a vendored ConnectionHandler::on_authenticated, feeding an event="auth" SIEM/audit stream), so a couple of field answers to Marynych Oleksandr (@maryny4)'s open questions, plus a firing-point convergence with #1691:

On (2), TLS-only / passwordless semantics — I think this resolves by letting the two hooks partition cleanly rather than one hook trying to cover both. The reject-vs-never-completed distinction only exists where CredSSP runs (Hybrid) — that's the whole audit value of on_authenticated(success, reason), and it's inherently CredSSP-specific: a TLS-only server has no credential exchange to succeed or fail, so there's no auth verdict to observe, only a transport that came up. So the activation boundary a passwordless server (and Marynych Oleksandr (@maryny4)'s do/undo lifecycle) actually wants is #1691's on_connection_info — fired from client_accepted for every security mode. on_authenticated is then the narrower "…and here's the CredSSP verdict" edge that only fires under Hybrid. Passwordless setups don't lose the lifecycle (on_connection_info + on_disconnected); they simply have no verdict to receive, which is correct — there wasn't one.

On (3), embedder reachability — one concrete wrinkle worth surfacing: #1691's on_connection_info fires from client_accepted after validation succeeds, so it captures the success boundary but not the failure case — and the failure-with-reason is the specific audit value here. So the success edge can ride on_connection_info, but the failure-capturing variant needs a firing point that also sees accept_credssp's Err. (macrdp's shipped hook fires around accept_credssp inside run's accept loop, which — as Greg Lamberson (@glamberson) found for keyboard metadata — an embedder calling run_connection directly never sees; so making the failure edge both reachable and embedder-visible is a genuine open point.)

Happy to rebase macrdp's patch onto whatever firing point you land on and test it against both the SIEM/audit case and Marynych Oleksandr (@maryny4)'s session-hooks lifecycle. Two downstream servers now want this edge; the open call is just where it fires and how the TLS-only case is named.

[maryny4]

Following up with test results instead of promises: the partition works, verified live.

I ported #1691's on_connection_info onto the IronRDP revision hypr-rdp pins (the branch doesn't apply there directly — that fork carries its own keyboard-data plumbing), and rebuilt hypr-rdp's declined session-hooks feature on it: the connect hook fires from on_connection_info, the disconnect hook from on_disconnected, and the disconnect command waits for the connect command to finish first (ordered do/undo).

Live results with a real client (Remmina, Hybrid security):

• TCP probes and non-RDP payloads never fire the hooks — the exact failure mode that got the feature declined while it lived on on_accept (feat: run session hooks on client connect and disconnect MuNeNiCK/hypr-rdp#27).
• Across repeated sessions, the connect hook fired exactly once per session, at session establishment (post-CredSSP), and the disconnect hook exactly once, strictly after the connect command completed.

So for the session-lifecycle half, on_connection_info + on_disconnected is sufficient as-is, exactly as clintcan laid out — my question (2) is fully resolved by the partition. Two implementation notes from the port (reactivation re-firing, downstream testability of ConnectionInfo) are on #1691 where they belong.

[glamberson]

For anyone tracking this thread specifically: the design question here (auth-outcome hook vs. routing through CredentialValidator) came up concretely on #1691's review, where maryny4 and clintcan both ported and live-tested the on_connection_info hook against their own ConnectionHandler implementations. clintcan's read (#1691 (comment)) is that the partition should be on_connection_info for the success/activation edge, with a separate hook here on #1484 for the auth-outcome verdict, since that generalizes better than folding everything through one hook. That matches where this issue's discussion was already heading.