Skip to content

fix(agent): harden package broker pipe against connection floods - #1902

Merged
Vladyslav Nikonov (vnikonov-devolutions) merged 2 commits into
masterfrom
vnikonov-devolutions-dgw-413-broker-pipe-dos-hardening
Aug 3, 2026
Merged

fix(agent): harden package broker pipe against connection floods#1902
Vladyslav Nikonov (vnikonov-devolutions) merged 2 commits into
masterfrom
vnikonov-devolutions-dgw-413-broker-pipe-dos-hardening

Conversation

@vnikonov-devolutions

Copy link
Copy Markdown
Contributor

Hardens the package broker named pipe server against cheap denial-of-service via connection floods.
Previously, every incoming connection triggered unauthenticated identity lookups (process open, executable path, account name resolution) before any signature validation, and connections were served without any concurrency limit.

Now the broker caps the number of concurrently served pipe connections, performs client identity capture off the accept loop so a slow lookup cannot stall other connections, and defers account-name resolution (which may reach a domain controller) until it is actually needed for an error message.
This bounds the work an unauthenticated flood can trigger and keeps the broker responsive for legitimate clients.

Issue: DGW-413

…path

Cap concurrent pipe connections with a semaphore, move client identity
capture off the accept loop, and defer account-name resolution (which
may hit a domain controller) to the rare mismatch error path.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@vnikonov-devolutions

Copy link
Copy Markdown
Contributor Author

Implementation notes (follow-up to #1847 (review)):

  • Connection cap: a \ okio::sync::Semaphore\ with 16 permits bounds concurrently served connections. A permit is acquired before a new pipe instance is created, so while all slots are taken no instance is listening and further clients fail fast (natural backpressure) instead of queuing unbounded work/tasks.
  • Identity capture moved off the accept loop: \PipeClient::from_connected_pipe\ now runs inside the spawned per-connection task, so a slow lookup can no longer stall accepting other connections.
  • **Removed connect-time \LookupAccountSid**: this was the most expensive unauthenticated call (may hit a domain controller for domain SIDs) and its result was only used in the \�ffective_user\ mismatch error message. Connect-time work is now limited to fast local syscalls (\GetNamedPipeClientProcessId, \OpenProcess, exe path, token user SID). The account name is resolved lazily on the rare mismatch error path, with a <unresolved>\ fallback.
  • SID-based \�ffective_user\ validation is unchanged; only the diagnostic name lookup moved.
  • Tests: existing \�roker::auth\ unit tests updated for the simplified \PipeClient\ shape (SID only, no cached domain/name); all pass, clippy clean.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the Windows package broker named-pipe server against connection-flood denial of service.

Changes:

  • Caps concurrent pipe connections at 16.
  • Moves identity capture outside the accept loop.
  • Defers client account-name resolution until needed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
devolutions-agent/src/broker/pipe.rs Adds connection limiting and asynchronous handling.
devolutions-agent/src/broker/auth.rs Stores client SID and resolves account names lazily.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread devolutions-agent/src/broker/pipe.rs Outdated
Comment thread devolutions-agent/src/broker/pipe.rs

@CBenoit Benoît Cortier (CBenoit) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete: no substantiated findings.

The protocol/API analysis found no applicable Open Specifications requirements. The skeptical concerns were not confirmed because each pipe connection serves one HTTP request and the new semaphore bounds remaining unauthenticated work to 16 concurrent connections.

Idle clients holding a connection open without sending a request could
pin all connection slots indefinitely. A 30-second per-connection
deadline frees the slot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@vnikonov-devolutions
Vladyslav Nikonov (vnikonov-devolutions) merged commit 23a9784 into master Aug 3, 2026
42 checks passed
@vnikonov-devolutions
Vladyslav Nikonov (vnikonov-devolutions) deleted the vnikonov-devolutions-dgw-413-broker-pipe-dos-hardening branch August 3, 2026 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants