feat(server): validate auto-reconnect cookies

[Marc-André Moreau (mamoreau-devolutions)]

Summary

• parse and carry ARC_CS_PRIVATE_PACKET data through the acceptor
• validate returning Enhanced RDP Security cookies with HMAC-MD5 before reconnecting
• rotate reconnect randoms per connection and hourly, with runtime cookie updates
• restrict cookie authentication to TLS/Hybrid and document the behavior

Testing

• cargo test -p ironrdp-pdu -p ironrdp-acceptor -p ironrdp-server
• cargo clippy -p ironrdp-pdu -p ironrdp-acceptor -p ironrdp-server --all-targets -- -D warnings

Fixes: #1508

[Copilot]

🟡 Not ready to approve

Cookie secrets can leak through debug logging, runtime update failures invalidate usable cookies, and the arbitrary feature does not compile.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds server-side validation and rotation for RDP auto-reconnect cookies.

Changes:

• Parses and propagates client reconnect packets.
• Validates HMAC-MD5 verifiers and bypasses credential revalidation for valid reconnects.
• Adds per-connection/hourly rotation and runtime updates.

File summaries

File	Description
crates/ironrdp-server/src/server.rs	Implements validation, rotation, and runtime updates.
crates/ironrdp-server/src/lib.rs	Exports the update handle.
crates/ironrdp-server/src/builder.rs	Documents lifecycle behavior.
crates/ironrdp-server/Cargo.toml	Adds cryptographic and timing dependencies.
crates/ironrdp-pdu/src/rdp/client_info.rs	Decodes client reconnect packets.
crates/ironrdp-acceptor/src/connection.rs	Propagates reconnect data through acceptance.
Cargo.lock	Locks new dependencies.

Review details

• Files reviewed: 6/7 changed files
• Comments generated: 5
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Marc-André Moreau (mamoreau-devolutions)]

Fixed in 546f061. ServerEvent now implements a redacted Debug representation, so cookie and credential payloads cannot appear in event trace logs or SendError debug output.

[Marc-André Moreau (mamoreau-devolutions)]

Fixed in 546f061. Runtime replacement now writes the Save Session Info PDU before committing the new cookie state, preserving the prior state if delivery fails.

[Marc-André Moreau (mamoreau-devolutions)]

Fixed in 546f061. AutoReconnectCookieHandle::set now explicitly documents that the update is queued and when a None update takes effect.

[Marc-André Moreau (mamoreau-devolutions)]

Fixed in 546f061. The builder now parses and stores the matching typed representation alongside the raw cookie, with regression coverage for valid builder input.

[Greg Lamberson (glamberson)]

Marc-André Moreau (@mamoreau-devolutions) heads up on an overlap, plus one thing in the patch that is independent of it.

I have two open on this surface: #1496 (reissue the cookie mid-session) and #1501 (client half, session resume). #1501 adds ClientAutoReconnect to crates/ironrdp-pdu/src/rdp/client_info.rs, same name and same two fields as the one here, and #1496 adds the same ServerEvent::SetAutoReconnectCookie variant. The verifiers agree, so the halves interoperate: #1501 derives HMAC-MD5 keyed with random_bits over the 32 zero bytes, which is exactly what auto_reconnect_verifier_matches checks.

Tell me the merge order you want and I will rebase to fit, including closing either of mine if you would rather this PR carry the whole thing. The acceptor plumbing and the validation path here are yours and I am not looking to duplicate them.

Separately, one thing worth a look regardless of the ordering. In ExtendedClientOptionalInfo::decode:

optional_data.auto_reconnect = Some(ClientAutoReconnect::decode(&mut ReadCursor::new(&reconnect_cookie))?);

That ? propagates, so a client sending a 28-byte cookie with an unexpected cbLen or version now fails the entire Client Info PDU and loses the connection, where before those bytes were stored and ignored. That points the other way from #1458, #1489 and #1506. Treating a failed parse as "no auto-reconnect offered" instead of a decode error would keep the tolerance.

[Marc-André Moreau (mamoreau-devolutions)]

Given that this particular PR is not tested, just implemented based on the related issue, consider that your other PRs have precedence over mine. I will rebase and resolve conflicts accordingly

[Greg Lamberson (glamberson)]

Marc-André Moreau (@mamoreau-devolutions) two more I'd found, with the fix instead of the question. Against 20736d56. Apply, adapt or ignore.

Rotation retires the old cookie too early. rotate_auto_reconnect_cookie commits the replacement the moment write_all returns, and from then on only the new one verifies. But write_all returning means the bytes hit the socket, not that the client read them, so a client that drops around the rotation is left holding a cookie the server no longer knows and can't reconnect at all. That's the ungraceful disconnect the feature exists for. The fix keeps the previous cookie valid until the next rotation, and treats an explicit set_auto_reconnect_cookie as a revocation so None still means none.

The validator bypass isn't documented. server.rs:1568 skips CredentialValidator when a cookie verifies. That's defensible and probably what you want, but it quietly changes what with_credential_validator promises, and Rui Carmo (@rcarmo)'s #1413 builds multi-user PAM handoff on that same validator. The fix documents it on both entry points rather than adding a knob.

--- a/crates/ironrdp-server/src/server.rs
+++ b/crates/ironrdp-server/src/server.rs
@@ struct RdpServer
     auto_reconnect_cookie: Option<rdp::session_info::ServerAutoReconnect>,
+    /// The cookie the current one replaced, still accepted on reconnect.
+    ///
+    /// Sending a replacement only proves the bytes reached the socket, not that
+    /// the client read them. Retiring the old cookie at that moment leaves a
+    /// client that dropped around the rotation holding one the server no longer
+    /// knows, unable to reconnect: precisely the ungraceful disconnect this
+    /// feature exists for. Honouring the previous cookie until the next rotation
+    /// closes that window, at the cost of two live cookies per session.
+    previous_auto_reconnect_cookie: Option<rdp::session_info::ServerAutoReconnect>,
     auto_reconnect_sent: bool,

@@ RdpServer::new
             auto_reconnect_cookie: None,
+            previous_auto_reconnect_cookie: None,
             auto_reconnect_sent: false,

@@ set_auto_reconnect_cookie
         self.auto_reconnect_cookie = cookie;
+        // An explicit set is a revocation, unlike a rotation: drop the previous
+        // cookie rather than carrying it over, so `None` really does invalidate
+        // what the server currently honours.
+        self.previous_auto_reconnect_cookie = None;
         self.auto_reconnect_sent = false;

@@ verify_auto_reconnect_cookie
-        self.auto_reconnect_cookie
-            .as_ref()
-            .is_some_and(|cookie| auto_reconnect_cookie_matches(cookie, reconnect))
+        // Either the current cookie or the one it replaced: see
+        // `previous_auto_reconnect_cookie` for why the old one stays valid.
+        [&self.auto_reconnect_cookie, &self.previous_auto_reconnect_cookie]
+            .into_iter()
+            .flatten()
+            .any(|cookie| auto_reconnect_cookie_matches(cookie, reconnect))

@@ send_next_auto_reconnect_cookie AND rotate_auto_reconnect_cookie (both sites)
-        self.auto_reconnect_cookie = Some(cookie);
+        self.previous_auto_reconnect_cookie = self.auto_reconnect_cookie.replace(cookie);
         self.auto_reconnect_sent = true;

Plus the same six-line doc note on with_credential_validator (builder.rs) and set_credential_validator (server.rs), stating that the validator isn't consulted on a valid auto-reconnect and that a deployment wanting it to gate every connection must leave the cookie unset.

cargo xtask check fmt/lints/tests green. One gap worth naming: there's no test for the two-cookie window. The server-side auto-reconnect path has none to extend, and standing up an RdpServer harness looked like scope you didn't ask for.

[clintcan]

Tested this end-to-end against real mstsc — it works. This is in response to your answer to issue #1508

Rather than wire it into our downstream server (pinned well behind master, which would test the backport rather than this PR), I ran it on this branch via the repo's own crates/ironrdp/examples/server.rs, instrumented with .with_auto_reconnect_cookie(...) and an Enter-to-drop trigger (server.event_sender() → ServerEvent::Quit), --sec hybrid + a self-signed cert, and drove a real Windows mstsc client.

The full auto-reconnect cycle (log):

auto-reconnect cookie provisioned … random_bits=[44, 11, 95, …]
Received connection peer=192.168.0.149:…        # mstsc connects
Sent Server Auto-Reconnect Cookie (Save Session Info PDU)
drop trigger — sending ServerEvent::Quit         # forced ungraceful drop
End of client loop: Ok(Disconnect)
Received connection peer=…                        # mstsc auto-reconnects
Client accepted
Auto-reconnect cookie validation accepted         # HMAC verified

Session resumed cleanly. So a real mstsc's ARC_CS_PRIVATE_PACKET verifier matches your HMAC-MD5(random_bits, [0;32]) construction, and the rotation timing is right (validated against the last-sent random, not a prematurely-rotated one).

Bonus: the reconnect happened to arrive from a different source IP than the initial connection (a ZeroTier address vs the LAN one) — so validation is correctly IP-independent and survived a network change (roaming client).

I also added a known-answer test that pins the verifier construction against an independent Python hmac/hashlib reference, so a future refactor can't silently break the HMAC. Feel free to fold it in:

#[cfg(test)]
mod auto_reconnect_tests {
    use ironrdp_pdu::rdp::client_info::ClientAutoReconnect;
    use ironrdp_pdu::rdp::session_info::ServerAutoReconnect;

    use super::{auto_reconnect_cookie_matches, auto_reconnect_verifier_matches};

    // Independently computed (Python hmac/hashlib):
    //   HMAC-MD5(key = 01..10, msg = 32 zero bytes) = 894025a9…261c
    const RANDOM_BITS: [u8; 16] = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];
    const EXPECTED_VERIFIER: [u8; 16] = [
        0x89, 0x40, 0x25, 0xa9, 0x9d, 0x64, 0xab, 0x96, 0x64, 0x19, 0xec, 0x1e, 0xf1, 0x3c, 0x26, 0x1c,
    ];

    #[test]
    fn verifier_matches_reference_hmac_md5() {
        assert!(auto_reconnect_verifier_matches(&RANDOM_BITS, &EXPECTED_VERIFIER));
    }

    #[test]
    fn verifier_rejects_a_wrong_value() {
        let mut wrong = EXPECTED_VERIFIER;
        wrong[0] ^= 0xff;
        assert!(!auto_reconnect_verifier_matches(&RANDOM_BITS, &wrong));
    }

    #[test]
    fn cookie_match_requires_both_logon_id_and_verifier() {
        let cookie = ServerAutoReconnect { logon_id: 0x1234_5678, random_bits: RANDOM_BITS };
        let good = ClientAutoReconnect { logon_id: 0x1234_5678, security_verifier: EXPECTED_VERIFIER };
        assert!(auto_reconnect_cookie_matches(&cookie, &good));
        let wrong_id = ClientAutoReconnect { logon_id: 1, security_verifier: EXPECTED_VERIFIER };
        assert!(!auto_reconnect_cookie_matches(&cookie, &wrong_id));
    }
}

One minor API nit while I was here: with_auto_reconnect_cookie takes a ServerAutoReconnect, but ironrdp-server doesn't re-export it — a direct consumer of the crate has to reach ironrdp_pdu::rdp::session_info::ServerAutoReconnect. A pub use from the server crate would be friendlier.

Overall: LGTM and suitable — validated against a real client. Thanks for picking this up so fast!

[Marc-André Moreau (mamoreau-devolutions)]

Greg Lamberson (@glamberson): addressed the tolerant client-cookie decoding, two-cookie rotation grace window, explicit revocation behavior, and validator documentation in 546f061. I am keeping #1496 and #1501 as the precedence path per the existing merge-order discussion.nn@clintcan: thank you for the mstsc validation. I also incorporated the independent HMAC-MD5 known-answer tests and re-exported ServerAutoReconnect in 546f061.