feat(policy): harden the package broker pipe transport

[Benoît Cortier (CBenoit)]

Summary

Hardens the default NamedPipeBrokerTransport of Devolutions.Now.Policy.Client and makes it configurable, so consumers no longer need their own pipe transports.

Default behavior (Windows)

• Least-privilege access: opens the package broker pipe with PipeAccessRights.Read | Synchronize | WriteData (FILE_GENERIC_READ | FILE_WRITE_DATA) instead of GENERIC_READ | GENERIC_WRITE. This is a subset of what current and upcoming brokers grant, so the change is backward compatible. Write-attributes access is not requested because the read mode is never changed.
• Limited impersonation: the server gets identification-level impersonation by default (SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION).
• Server verification before any request byte is written: GetNamedPipeServerProcessId must match the running process of the DevolutionsAgent (MSI) or devolutions-agent (manual registration) service, reported by the SCM (QueryServiceStatusEx), and that service must be configured to run as LocalSystem (QueryServiceConfig). This works for standard users. When the caller can open the server process (elevated callers), its token user must also be LocalSystem, and the process handle is held until the exchange completes. Failures throw BrokerClientException with the new BrokerClientErrorKind.ServerVerificationFailed. On other platforms, verification fails closed unless disabled.
• Busy retry: 503 replies carrying Retry-After are retried over a new connection, at most 3 times by default, with the delay capped at 5 s. 503 replies without Retry-After are returned as before.
• Closed-connection retry: when the broker closes a connection before sending any response byte, the request is retried over a new connection. Retries share the same budget and use an exponential backoff that starts at 100 ms, capped by the same delay. A request is resent only if the write failed, meaning the broker never received the complete request, or if the request has no side effects: GET, HEAD, evaluation, status and policy validation. Other requests, such as executions and policy replacements, fail with BrokerUnavailable. Partial responses are never retried. Busy and temporarily missing pipe instances were already retried while connecting, within ConnectTimeout.
• The request header and body are sent in a single write right after connecting.
• Strict HTTP/1.1 framing:
  • the status line must be HTTP/1.1 with a status code from 100 to 599;
  • header field names must be HTTP tokens;
  • a single valid Content-Length is required and Transfer-Encoding is rejected;
  • excess data is detected with an EOF probe;
  • header and body size budgets are checked before the body buffer is allocated.

Configuration (additive API)

• NamedPipeBrokerTransportOptions (record), accepted by a new NamedPipeBrokerTransport(NamedPipeBrokerTransportOptions) constructor and by BrokerClientOptions.NamedPipeTransport:
  • PipeName
  • ConnectTimeout, ResponseTimeout
  • MaxResponseHeaderBytes, MaxResponseBodyBytes
  • ImpersonationLevel
  • VerifyServer, ServerServiceNames
  • ServerAuthenticator, ServerAuthenticatorTimeout
  • MaxBusyRetries, MaxBusyRetryDelay
  • IncompleteResponseErrorKind
• BrokerPipeServerAuthenticator hook with BrokerPipeServerContext. The context exposes the connected pipe, the pipe name, the endpoint, the server process id, and the held server process handle when available. The hook runs after built-in verification and before any write, and is bounded by its own deadline. Its returned IDisposable lives until the response is read, and late results are disposed.
• NamedPipeBrokerTransport.Trace. BrokerClient forwards it to BrokerClient.Trace.
• Existing constructors and options are unchanged. ClientExecutablePath still defaults to Environment.ProcessPath, the real process image path.

Note: new NamedPipeBrokerTransport(null) with a literal null is now ambiguous at compile time. new NamedPipeBrokerTransport() and passing a string? variable still compile. Binary compatibility is unaffected.

Validation

• New unit tests cover:
  • access-mask selection
  • option defaults and validation
  • request encoding
  • response framing, including size budgets, incomplete responses, and malformed status lines and headers
  • Retry-After handling, resend safety, and disconnect backoff
  • server verification logic, using an inspector abstraction
• New real named-pipe tests (Windows, Linux, macOS) cover:
  • the request and response round trip
  • bounded busy retries
  • connections dropped right after accept, or closed after reading the request, for safe and unsafe requests
  • a shared retry budget for busy replies and closed connections
  • partial responses, which are not retried
  • verification failure, with no bytes sent
  • authenticator success, rejection, async and synchronous timeouts, and late-result disposal
  • configurable timeouts and body budget
  • BrokerClient defaults
• Windows-only tests:
  • a pipe with a broker-like DACL rejects a GENERIC_WRITE client but accepts the transport
  • the server observes the configured impersonation level
  • the server process id and handle are exposed
• Checked manually as a non-elevated user against live LocalSystem services: the SCM status and configuration queries succeed, OpenProcess is denied as expected, and the verifier accepts the matching service.
• dotnet format --verify-no-changes passes, and dotnet test passes on net10.0 locally (387 tests). net9.0 is covered by CI.

[Copilot]

Copilot review overview

🟡 Changes recommended

Cancellation classification and strict HTTP/1.1 validation have unresolved correctness issues.

Review effort: Balanced
Findings: 2

Open (2)

• Enforce strict HTTP/1.1 status-line format · New
• Avoid misclassifying authenticator cancellation as timeout · New

What changed in this PR

Hardens and configures the default named-pipe broker transport, adding server verification, strict response handling, bounded retries, and authentication hooks.

Changes:

• Adds configurable transport security, timeouts, retries, and server authentication.
• Implements Windows service/process verification and stricter HTTP framing.
• Adds comprehensive unit and cross-platform pipe tests.

File	Description
WindowsBrokerServerInspector.cs	Adds Windows process, token, and service inspection.
README.md	Documents hardened transport behavior and configuration.
NamedPipeBrokerTransportOptions.cs	Defines and validates transport options.
NamedPipeBrokerTransport.cs	Implements verification, authentication, retries, and timeouts.
Devolutions.Now.Policy.Client.csproj	Enables unsafe interop and test access.
BrokerServerVerifier.cs	Verifies service process and LocalSystem identity.
BrokerPipeServerAuthenticator.cs	Adds the custom authentication API and context.
BrokerHttp.cs	Encodes requests and strictly parses framed responses.
BrokerClientOptions.cs	Exposes named-pipe transport configuration.
BrokerClientErrorKind.cs	Adds the server-verification failure kind.
BrokerClient.cs	Creates and traces the configured default transport.
BrokerBusyRetry.cs	Parses and caps Retry-After delays.
NamedPipeBrokerTransportTests.cs	Tests real pipe behavior and hardened defaults.
BrokerTransportUnitTests.cs	Tests framing, options, retries, and verification.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Request framing permits conflicting transfer encoding, and oversized timeout values bypass validation.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Avoid misclassifying authenticator cancellation as timeout
• Enforce strict HTTP/1.1 status-line format

Previously missed (2)

In code that hasn't changed since last review

Strip Transfer-Encoding when emitting fixed-length bodies

policies/​dotnet/​Devolutions.Now.Policy.Client/​BrokerHttp.cs:25

Transfer-Encoding is still forwarded even though this encoder always adds Content-Length. A caller that supplies Transfer-Encoding: chunked therefore emits conflicting framing, so the broker may parse or reject the body as chunked despite the transport encoding it as a fixed-length body. Treat this as another transport-owned framing header and omit it (or reject the request).

Validate timeout values against supported timer limits

policies/​dotnet/​Devolutions.Now.Policy.Client/​NamedPipeBrokerTransportOptions.cs:147

This validation accepts arbitrarily large positive timeouts, but each value is later passed to CancellationTokenSource.CancelAfter, whose TimeSpan overload rejects delays above its supported timer range. For example, TimeSpan.MaxValue passes construction and then causes Send to throw ArgumentOutOfRangeException instead of applying the configured timeout. Validate the upper bound here (or implement long-delay handling); apply the same constraint to MaxBusyRetryDelay, which is passed to Task.Delay.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The strict HTTP parser still accepts malformed field names and out-of-range status codes.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Reject whitespace around HTTP header field names

policies/​dotnet/​Devolutions.Now.Policy.Client/​BrokerHttp.cs:105

Trimming the field name turns malformed lines such as Content-Length : 2 or Content-Length: 2 into a valid framing header. That violates the strict parser contract and can make an invalid response pass framing validation; validate the raw field name as an HTTP token and reject whitespace around it instead of normalizing it.

Reject HTTP status codes outside the 100–599 range

policies/​dotnet/​Devolutions.Now.Policy.Client/​BrokerHttp.cs:178

The three-digit check still accepts status codes 600–999, although valid HTTP status codes are limited to 100–599. With strict HTTP/1.1 framing, these responses should produce InvalidResponse rather than flow into broker response handling.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Security-sensitive native Windows verification and protocol-framing changes warrant final human review despite comprehensive tests.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🟡 Changes recommended

Partial-response I/O failures are misclassified, and long configured retry backoffs stop growing below their documented cap.

Review effort: Balanced
Findings: 2

Open (2)

• Backoff exponent cap prevents reaching configured maximum delay · New
• Translate post-response I/O failures as incomplete responses · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Token access denial can bypass the required LocalSystem process-token verification after the process handle has been opened.

Review effort: Balanced
Findings: 1

Open (1)

• Fail closed when process token access is denied · New

Resolved since last review (2)

• Translate post-response I/O failures as incomplete responses
• Backoff exponent cap prevents reaching configured maximum delay

[Copilot]

Copilot review overview

🔵 Needs a closer look

The security-sensitive native Windows verification and retry semantics warrant final human validation despite comprehensive tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Fail closed when process token access is denied