fix(pdu): keep auto-reconnect credential material out of Debug output - #1496
Merged
Benoît Cortier (CBenoit) merged 1 commit intoAug 3, 2026
Conversation
github-actions
Bot
added
A-community
Greg Lamberson (glamberson)
changed the title
Greg Lamberson (glamberson)
force-pushed
the
fix/auto-reconnect-cookie-reissue
branch
from
July 31, 2026 15:41
78824b1 to
d18b7de
Compare
github-actions
Bot
added
size/S
Greg Lamberson (glamberson)
force-pushed
the
fix/auto-reconnect-cookie-reissue
branch
from
July 31, 2026 16:55
d18b7de to
bbe5c3d
Compare
github-actions
Bot
added
size/S
Contributor
There was a problem hiding this comment.
Pull request overview
Adds mid-session Server Auto-Reconnect Cookie reissuance to ironrdp-server.
Changes:
- Adds
ServerEvent::SetAutoReconnectCookie. - Reuses a shared Save Session Info PDU encoder.
- Adds wire-format coverage for cookie delivery.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greg Lamberson (glamberson)
force-pushed
the
fix/auto-reconnect-cookie-reissue
branch
from
July 31, 2026 18:25
bbe5c3d to
bf80b83
Compare
github-actions
Bot
added
A-core
Greg Lamberson (glamberson)
force-pushed
the
fix/auto-reconnect-cookie-reissue
branch
from
August 2, 2026 23:39
bf80b83 to
b961d5c
Compare
github-actions
Bot
added
size/S
Greg Lamberson (glamberson)
changed the title
Contributor
Author
|
Marc-André Moreau (@mamoreau-devolutions) ready here I think. |
Marc-André Moreau (mamoreau-devolutions)
pushed a commit
that referenced
this pull request
Aug 3, 2026
…uto-reconnect cookie (#1501) > **Rebased onto post-#1522 master.** #1509 landed the server half of #1508 while this was open, including the `ClientAutoReconnect` structure. This PR no longer declares it; it extends it, and picks up the parts #1509 did not build. ## What The client half of automatic reconnection. The session layer surfaces the Server Auto-Reconnect Cookie, `ironrdp-pdu` derives and verifies the client's response to it, and the connector sends that response when resuming a session. ## Why A client whose connection drops ungracefully can reattach to its session instead of making the user log on again, provided it returns the cookie the server issued during logon ([MS-RDPBCGR] 1.3.1.5). #1509 built the server side of that: it validates a returning `ARC_CS_PRIVATE_PACKET` and rotates the random. Nothing answers it. `ironrdp-session` decodes the cookie and drops it, `ironrdp-connector` has no way to send one back, and `TODO(#271)` still sits in `ironrdp-client`. So `ironrdp-client` cannot resume a session against `ironrdp-server`, and the validation #1509 added has no in-tree counterpart to exercise it. The wire encoding was already there. `ExtendedClientOptionalInfo` carries, encodes and decodes a 28-byte `autoReconnectCookie` and its builder already had a `reconnect_cookie` step; `ServerAutoReconnect` already decoded; #1509 added `ClientAutoReconnect` and its decode. Nothing connected them. ## The three parts **Receive.** `SaveSessionInfo` now also surfaces the cookie, as `ProcessorOutput::AutoReconnectCookie` and `ActiveStageOutput::AutoReconnectCookie`. #1522 added a `SaveSessionInfo { logon_complete }` output on that same handler; the two coexist rather than compete, since both are read off one PDU and neither supersedes the other. The handler emits the logon notification unconditionally and appends the cookie when one is present, and a test pins that surfacing the cookie does not suppress the notification. #1509's server replaces the cookie whenever a client connects and again hourly ([MS-RDPBCGR] 3.3.6.2), so this can arrive more than once in a session and the consumer keeps the most recent. **Derive.** `ClientAutoReconnect::from_server_cookie` implements [MS-RDPBCGR] 5.5: > The auto-reconnect random is used to key the HMAC function ([RFC2104]), which uses MD5 as the iterative hash function. The security verifier is derived by applying the HMAC to the client random received in Step 3. > > `SecurityVerifier = HMAC(AutoReconnectRandom, ClientRandom)` > > When Enhanced RDP Security is in effect the client random value is not generated (section 5.3.2). In this case, for the purpose of generating the security verifier, the client random is assumed to be an array of 32 zero bytes. IronRDP implements no Standard RDP Security path (there is no Security Exchange PDU), so the zero-client-random case is the only one that arises. As 5.5 notes, that makes the verifier constant for a given cookie, so it proves possession of the cookie and nothing more; session security comes from the outer TLS/CredSSP handshake. @clintcan independently confirmed this construction against real **mstsc** while validating #1509 ([comment](#1509 (comment))): a Windows client's `ARC_CS_PRIVATE_PACKET` verifies against `HMAC-MD5(random_bits, [0u8; 32])`. That is the same derivation implemented here, so the two halves interoperate with Microsoft's client and not only with each other. **Send.** `ClientConnector::with_auto_reconnect_cookie` takes the cookie last received and makes the connector put the derived Client Auto-Reconnect Packet ([MS-RDPBCGR] 2.2.4.3) in the Client Info PDU. Absent, that PDU is byte-for-byte what it was. Unlike the server packet, this structure has no enclosing logon-info field header, so it encodes to exactly the 28 bytes the cookie field expects. `to_bytes` writes that layout directly rather than going through `Encode`, so filling a fixed-size field has no error path a caller must handle; a test pins the two to agree. ## One derivation, not two Putting `from_server_cookie` in `ironrdp-pdu` would leave the workspace with two implementations of 5.5, since #1509 added a private HMAC to `ironrdp-server`. So `ClientAutoReconnect` also gains `verify`, and the server routes through it. `verify` keeps the constant-time comparison the server had. The verifier is the whole credential, so a comparison returning early on the first differing byte would let a peer recover it a byte at a time from the timing; the session identifier is not secret and is compared normally. `ironrdp-server` keeps the policy around the check, which cookies are live and whether the security protocol permits auto-reconnect, and drops its `hmac` and `md-5` dependencies. `hmac` moves to `ironrdp-pdu` as `default-features = false`; the crate's full feature powerset still checks clean, including `--no-default-features`. I would rather not have reached into `ironrdp-server` in a `pdu,session,connector` change, but the alternative was shipping the duplicate and filing a follow-up to remove it, which is a worse trade for reviewer time. ## Tests that were not running That move also rehomes the known-answer tests @clintcan contributed on #1509. They went in as an inline `#[cfg(test)]` module in `crates/ironrdp-server/src/server.rs`, and that crate sets `[lib] test = false`, so they have never executed in CI. They now live in `ironrdp-testsuite-core` against the public API, where CI runs them: his HMAC-MD5 reference vector is kept as a second vector alongside a differently-keyed one, plus the cases for a tampered verifier and a mismatched logon ID. Worth flagging separately: `ironrdp-server` is not alone. `ironrdp-agent`, `ironrdp-session` and `ironrdp-web` also set `[lib] test = false` and between them carry 16 files of inline `#[cfg(test)]` modules that CI never runs. That is out of scope here, but I am happy to open an issue if it would be useful. ## Breaking changes `ActiveStageOutput` and `x224::ProcessorOutput` gain a variant, and `ClientConnector` gains a public field, so exhaustive matches and struct literals need updating. Confirmed with `cargo-semver-checks` against the merge-base: those three are the only findings this branch introduces. The others it reports on `master` today (`ShareDataPdu::Compressed` and the `ShareDataCtx` fields from #1518, `ProcessorBuilder.bulk_decompressor` from #1518, `ServerEvent::SetAutoReconnectCookie` from #1509) are present on `master` unchanged. The `ironrdp-pdu` additions are additive. ## Scope This is the library half. `ironrdp-client`, `ironrdp-web` and the FFI bindings gain an arm for the new output but none of them reconnect automatically yet; that is the remaining part of #271, and the existing `TODO(#271)` in `ironrdp-client` marks where it goes. I kept receive, derive and send together deliberately. Split up, none of them is usable on its own: without the receive half there is no way to obtain a cookie, and without the send half there is nothing to do with one. ## Tests Thirteen, all in `ironrdp-testsuite-core`. On the packet and the derivation: the `SecurityVerifier` matches two independently computed HMAC-MD5 vectors of 32 zero bytes under different keys, so the tests pin the derivation rather than restating the code; the logon ID carries over from the server cookie; the encoding matches the 2.2.4.3 field layout byte for byte with `cbLen` fixed at `0x1C`; `to_bytes` agrees with `Encode`; it round-trips; and it rejects both a wrong packet length and an unknown version. On verification: a derived answer is accepted, a single flipped byte in the verifier is rejected, a correct verifier under a different logon ID is rejected, and an answer derived from a different random is rejected. On the surfacing path: a Save Session Info PDU framed the way a server sends it, through the real x224 processor, yields an `AutoReconnectCookie` carrying the right logon ID and random bits, alongside #1522's logon notification rather than in place of it; and one without a cookie surfaces no cookie. ## Verification `cargo xtask check fmt/lints/tests/typos/locks` all pass on 1.94.1, including a `fuzz/` build before the lock check. ## Note #1496 also touches the `ClientAutoReconnect` declaration. Whichever of the two lands second needs a one-line rebase on the derive attribute; happy to take that in either order. [MS-RDPBCGR]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/e729948a-3f4e-4568-9aef-d355e30b5389 [RFC2104]: https://www.rfc-editor.org/rfc/rfc2104
The auto-reconnect cookie is credential material in both directions, and
both directions are logged today at debug level.
ServerAutoReconnect::random_bits keys the HMAC that proves, on reconnect,
that a client was the one last attached to the session (MS-RDPBCGR 5.5).
ClientAutoReconnect::security_verifier is that HMAC. Under Enhanced RDP
Security there is no client random, so 5.5 computes the verifier over 32
zero bytes: it is constant for a given cookie, and replaying it is enough
to resume the session. ExtendedClientOptionalInfo::reconnect_cookie is
the wire form of the same verifier.
All three derived Debug, and a client running at debug level writes both
halves to its log:
ironrdp-session/src/x224/mod.rs:212 logs the whole SaveSessionInfoPdu
it receives, so the random lands
in the log
ironrdp-connector/src/connection.rs:873 logs the whole ClientInfoPdu
it sends, so the verifier lands
in the log
That second line is why Credentials in this file already hand-writes
Debug to hide the password. The verifier travels in the same PDU and
deserves the same treatment.
This is not theoretical. A contributor validating Devolutions#1509 against mstsc
posted a server log reading
auto-reconnect cookie provisioned ... random_bits=[44, 11, 95, ...]
which is a live reconnect credential in a public comment, produced by the
derived impl.
Hand-write Debug on the three types following the Credentials pattern.
logon_id is not secret and stays visible, so the output remains useful
for diagnosis. Redacting the parsed field alone would not be enough,
since reconnect_cookie carries the same bytes unparsed, and redacting the
leaf alone would not be enough either, so the tests assert the elision
survives nesting inside the Debug-derived parents.
Greg Lamberson (glamberson)
force-pushed
the
fix/auto-reconnect-cookie-reissue
branch
from
August 3, 2026 00:52
b961d5c to
e9e3c42
Compare
github-actions
Bot
added
size/S
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
Member
There was a problem hiding this comment.
Reviewed the proposed head: the manual Debug implementations narrowly redact auto-reconnect credential material while retaining useful non-secret context, and the added direct and nested formatting tests cover the intended leak paths. The protocol handoff confirms that encoding, decoding, field layout, sequencing, and capability behavior are unchanged. No actionable findings.
Greg Lamberson (glamberson)
deleted the
fix/auto-reconnect-cookie-reissue
branch
August 3, 2026 13:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ServerAutoReconnect,ClientAutoReconnectandExtendedClientOptionalInfoderiveDebugover auto-reconnect credential material. Hand-write those three impls so the secret is elided andlogon_idis not.Why
The auto-reconnect cookie is credential material in both directions:
ServerAutoReconnect::random_bitskeys the HMAC that proves, on reconnect, that a client was the one last attached to the session (MS-RDPBCGR 5.5). 5.5 has the client hold the cookie in memory and never allow programmatic access to it.ClientAutoReconnect::security_verifieris that HMAC. Under Enhanced RDP Security there is no client random, so 5.5 computes the verifier over 32 zero bytes: it is constant for a given cookie, and replaying it is enough to resume the session.ExtendedClientOptionalInfo::reconnect_cookieis the wire form of the same verifier.A client running at debug level writes both halves to its log today:
ironrdp-session/src/x224/mod.rs:212SaveSessionInfoPduit receives, so the random lands in the logironrdp-connector/src/connection.rs:873ClientInfoPduit sends, so the verifier lands in the logThat second line is why
Credentialsinclient_info.rsalready hand-writesDebugto hide the password. The verifier travels in the same PDU and deserves the same treatment, so this PR follows that impl exactly.Not theoretical: while validating #1509 against mstsc, clintcan posted a server log reading
which is a live reconnect credential in a public comment, produced by the derived impl.
Why all three
Redacting the parsed field alone would not help, because
reconnect_cookiecarries the same bytes unparsed. Redacting the leaf alone would not help either if aDebug-derived parent reproduced them, so the tests assert the elision survives nesting insideLogonInfoExtended,SaveSessionInfoPduandExtendedClientOptionalInfo.logon_idis not secret and stays visible, so the output remains useful for diagnosis.Not breaking
No public item is added, removed or changed.
cargo semver-checks -p ironrdp-pdureports the same two findings on this branch as onmaster(ShareDataPdu::Compressedand theShareDataCtxfields, both from #1518), and none from this diff.Tests
Two, both in the existing inline modules beside the code they cover.
ExtendedClientOptionalInfo's fields are private, so the client-side test cannot be written fromironrdp-testsuite-core.Verification
cargo xtask check fmt/lints/tests/typos/locksall pass on 1.94.1.Rebased onto #1501
#1501 merged first and touched the same
ClientAutoReconnectdeclaration, so this is rebased onto it. The resolution is the one-line change both PRs anticipated: #1501's fuller doc comment is kept andDebugcomes off the derive list. Nothing else changed.Redaction now covers what #1501 added as well, since the client half it introduced carries the verifier this PR elides.