You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds structured Windows Event Log auditing for package policy write attempts and outcomes (IDs 8000–8005), plus externally observed policy changes (8010–8011). Events publish after authoritative state changes, use bounded privacy-safe fields, and flow through a bounded asynchronous Event Log queue.
Adds EN/FR/DE message catalogs embedded in production Agent and Gateway binaries, along with an MSI-managed Agent Event Log source registration whose EventMessageFile value is removed on uninstall through the ordinary MSI component lifecycle (RegistryKeyAction.create), leaving any other value under the source key untouched. Windows CI resolves mc.exe only from trusted installed SDK locations, and pins the declared source registration for both architecture variants of the MSI component. Installing and removing the MSI is not exercised in CI; the install/uninstall behavior itself is Windows Installer's own registry lifecycle, not something this repository tests.
Preserves the shared audit contract: 8000/8001 record attempts and denials; 8002/8003 are Create failure/success; 8004/8005 are Update, Repair, or ReplaceIdentity failure/success; 8010/8011 record externally applied/rejected changes.
Publishes externally observed state before emitting 8010/8011 and records exactly one terminal API outcome through an atomic lifecycle guard.
Restricts Event Log fields to bounded actor, path, operation, outcome, reason, and policy identity metadata. Policy bodies, drafts, receipts, and store tokens are excluded.
Sends release/production Event Log writes through one bounded 256-entry nonblocking queue. Overflow drops only the Event Log copy and emits rate-limited tracing telemetry.
Adds EN/FR/DE catalogs, catalog parity checks, production resource embedding, and trusted installed-SDK mc.exe discovery without downloading executable PowerShell modules.
Targeted validation passed: sysevent-codes 3/3, now-package-broker 430 passed with 2 ignored, rustfmt, focused Clippy with warnings denied, and diff checks. The same audit/build/catalog content passed production Agent and Gateway resource builds with Windows SDK 10.0.28000.0 before the final installer-only restacks.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The release/production mc.exe discovery still consults PATH (weakening the “trusted SDK locations only” goal) and the new async tests depend on thread-local audit capture that can be flaky under Tokio’s default multi-thread runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Windows Event Log auditing for package-policy write attempts/outcomes and external policy changes, wiring the broker’s policy-management path to emit structured sysevent entries backed by embedded .mc message catalogs.
Changes:
Introduces policy write/external-change audit events (IDs 8000–8005, 8010–8011) in sysevent-codes plus catalog parity tests.
Adds and embeds Devolutions Agent/Gateway Windows Event Log message catalogs via mc.exe during release/production builds.
Threads per-request write-audit context through the package broker server and policy store to record deny/fail/success and external-change observations.
This test relies on crate::audit::take_test_events() (thread-local in tests). With the default multi-thread Tokio runtime, the task can migrate threads across .await points and make the thread-local assertions flaky. Consider switching this test to a current-thread runtime.
std::fs::create_dir_all(managed.parent().expect("managed path has a parent"))
.expect("create managed directory");
std::fs::write(&managed, b"managed").expect("write managed marker");
crate::audit::take_test_events();
store.reload_from_disk(ReloadCause::ExternalChange).await;
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It introduces new Windows Event Log auditing behavior across request handling, persistence, and build/CI plumbing, and should receive final human review despite only minor issues found.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
audit_path = observation.canonical_path.clone() is done even when audit is None, which adds an avoidable allocation on the hot error paths. Since publish_external_observation returns a snapshot containing the same path, you can defer path materialization to the if let Some(audit) branch and borrow from management.configured_path (and apply the same pattern to the similar audit_path clones in the other error branches below).
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It introduces a cross-cutting Windows auditing pipeline and build/CI changes that should be validated by a human reviewer on Windows runners and release packaging flows.
Addressed the Medium finding from the latest review: the accepted connections are now drained on every accept loop exit path, not just the normal cancellation.
What was wrong.run_pipe_server drained its JoinSet only at the tail of the loop body it reached after break. The one call that could fail mid-iteration was create_pipe_instance(&pipe_name, first_instance)?, so that ? returned while the set was dropped without being awaited. A connection accepted before it, still serving a request, could then run its audit recorder destructor after run_pipe_server returned, i.e. after the caller's audit::drain(), and that terminal event was lost.
What I changed — structurally, not the literal suggestion. Rather than add a drain to the error arm, the accept loop moved into its own accept_connections(&state, &shutdown, &mut connections) and run_pipe_server is now:
letmut connections = tokio::task::JoinSet::new();let result = accept_connections(&state,&shutdown,&mut connections).await;drain_after_accept_loop(&mut connections,CONNECTION_SHUTDOWN_GRACE, result).await
drain_after_accept_loop waits for the connections (aborting on grace expiry) and then returns what the loop returned. A ? inside the loop can no longer bypass the drain, and a future exit path added to the loop inherits that guarantee instead of having to remember it. Verified by reading every return/? in the new loop body: the only early exit is break, and every branch after it falls through to the single drain.
Regression test.an_accept_loop_failure_waits_for_the_connections_it_accepted spawns a connection task that never finishes, calls the drain with an Err result and a 200 ms grace, and asserts the result is still reported as an error, that the grace actually elapsed, that the connection task was aborted, and that the set is empty. It is the same DropFlag pattern as the existing shutdown_aborts_the_connections_that_outlive_the_grace, so it fails on the previous shape of the code (which returned before waiting at all).
Validation:cargo test -p now-package-broker --locked → 422 passed, 0 failed, 2 ignored (421 before; the new test); cargo clippy --workspace --tests --locked -- -D warnings and cargo clippy -p now-package-broker --release --locked -- -D warnings → exit 0; cargo +nightly fmt --all → no reformat; cargo test -p testsuite --test integration_tests --locked sysevent → 7 passed.
Aborting a connection is cooperative. A connection inside synchronous
work, such as authenticating a client or reading the policy storage,
never reaches a cancellation point, so `abort_all()` alone does not stop
it.
The join after the abort was unbounded, so such a connection could hold
the shutdown past the runtime's own budget. The agent then stops the
runtime before the audit queue is drained, which loses every queued
event rather than the events of that one connection.
The settle after the abort is now bounded by the same grace as the
drain, and reports the residual window instead of silently waiting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Addressed the High finding from review 5358710307 ("Unbounded shutdown join can lose queued audit events") in 690dad92.
The problem was real.connections.abort_all() is cooperative, so a connection parked in synchronous work never observes it. Two paths can do that from a connection task: auth::validate_connection (synchronous WinVerifyTrust, called from the async handlers in server/mod.rs) and the synchronous storage.create/replace in policy_store/mod.rs. The while connections.join_next().await.is_some() {} that followed the abort therefore had no upper bound. The agent gives its tasks 3 × 10 s and then calls runtime.shutdown_timeout(1 s) in service.rs::stop, so a single stuck connection could push the whole shutdown past that budget — the runtime stops before task.rs reaches audit::drain(), and the entire queue is lost rather than the events of that one connection.
The fix bounds the settle. The same CONNECTION_SHUTDOWN_GRACE now also bounds the wait for the aborted connections to actually stop:
let settled = tokio::time::timeout(grace,async{while connections.join_next().await.is_some(){}}).await;if settled.is_err(){error!("Named pipe connections are still running blocking work; their policy audit events may be lost");}
Worst case is now 2 × 5 s inside the agent's ~31 s budget, audit::drain() always runs, and the residual window is logged at error! instead of being silent. The doc comments on CONNECTION_SHUTDOWN_GRACE, wait_for_connections and the crate's shutdown contract state the residual window honestly.
Why this option rather than moving the blocking work behind a shutdown-aware boundary. The reviewer offered both. Bounding strictly dominates for this invariant: we now always reach the drain, risking at most one connection's event instead of all queued events. Moving the request-path validators to spawn_blocking is a broader change to the request path and to auth.rs/policy_store — it would touch this PR's layering for no additional audit guarantee, since some synchronous work would still have to be bounded somewhere. If you would rather have the deeper fix, it is a separate change and I am happy to follow up on it.
Regression test.shutdown_gives_up_on_a_connection_stuck_in_blocking_work spawns a connection that blocks its worker inside block_in_place (the only faithful simulation of work that no abort can interrupt) and asserts the wait returns with that connection still in the set. Returning with the set non-empty is exactly the property under test — an unbounded settle only returns once the set is empty — so the assertion is timing-free. I verified it fails on the previous code and passes now.
Validation (at 690dad92):
cargo +nightly fmt --all — clean, no reformat
cargo test -p now-package-broker --locked — 423 passed, 0 failed, 2 ignored (422 before, +1 new test)
New test proven to fail on the previous code (assert!(!connections.is_empty()))
The earlier cli::jetsocat::jmux_proxy_write_hello_world failure on tests [linux x64] is in a test this PR does not touch; I am re-running that job to confirm it is a flake.
… commit
Serving a named pipe connection can commit a policy and record its terminal
audit event, and that write is synchronous, so shutdown can give up on a
connection that is still inside it. The recorder was then drained right away,
closing the queue, so the event of the policy that connection went on to commit
was rejected instead of being emitted.
Each connection now holds an audit lease for as long as it can record. The
recorder closes the queue only when no lease is alive; otherwise it flushes the
accepted entries and leaves the queue accepting, so a late terminal event is
emitted rather than rejected. Closing the queue is what loses the event, and
waiting for the connection to finish is the unbounded join the shutdown must
not perform.
The flush is bounded, and giving up on a stalled Windows Event Log sink is
logged with the number of entries still pending.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Addressed the remaining finding on pipe.rs in 268554c5.
The finding. After the bounded settle, connections can still hold a task doing synchronous policy persistence, run_pipe_server returns, and BrokerTask calls audit::drain() right away — which closed the queue. A connection that then finished committing its policy had its terminal event rejected by the closed queue.
Why not just wait. Waiting for that connection is the unbounded join the previous finding asked to remove: the write is synchronous and uncancellable, so "don't drain until the work finishes" and "the shutdown is bounded" cannot both hold by waiting. The problem is not that drain() ran early — it is that drain()closes the queue.
The fix. A connection holds an audit lease while it can record (AuditLease, first statement of the spawned task in accept_connections). The recorder closes the queue only when no lease is alive:
pub(crate)fndrain(){ifletSome(recorder) = RECORDER.get(){shutdown(recorder.as_ref(),AUDIT_PRODUCERS.load(Ordering::Acquire));}}fnshutdown(recorder:&dynAuditRecorder,producers:usize){if producers == 0{
recorder.drain();}else{
tracing::warn!(producers,"... flushing the policy audit queue without closing it");
recorder.flush();}}
With a connection still alive, the queue is flushed — a bounded wait for the accepted entries to reach the sink — and left accepting, so the terminal event of the policy write that connection is still finishing is emitted instead of rejected. The normal path (no connection left, every lease dropped) keeps today's guaranteed close-and-join.
The lease is exact: a JoinSet task's future is dropped when it completes or is aborted and reaped by join_next(), so an empty set means a count of zero, while a task stuck in synchronous work cannot be dropped mid-poll and therefore keeps its lease alive exactly as long as it can still record. The external-change producer is unaffected — the watcher is awaited before the drain.
Bounds and telemetry. Flushing is capped by EVENT_LOG_FLUSH_GRACE (5 s) and gives up with a warn! carrying the number of entries still pending, so a stalled Windows Event Log sink cannot extend the shutdown. Worst case is now 10 s of connection settle + 5 s of flush, inside the agent's ~31 s shutdown budget. The error! on the give-up path in wait_for_connections now says those events "may not reach the Windows Event Log before the agent stops the process" — the remaining exposure is the process exiting before the worker's last emit, which no amount of waiting inside a bounded shutdown can remove.
a_lease_holds_the_recorder_open_until_the_last_one_is_dropped — nested leases, counted on an explicit counter so it stays deterministic next to the pipe tests.
a_live_producer_makes_the_shutdown_flush_instead_of_closing_the_queue — the decision itself, with explicit producer counts.
a_flushed_queue_still_accepts_the_terminal_event_of_a_live_producer — the reported bug: the queue is open after a flush, the terminal event is accepted, and both events reach the sink.
flushing_gives_up_on_a_sink_that_stopped_making_progress — the bound holds and the entry stays queued.
Validation:cargo +nightly fmt --all (no reformat), cargo test -p now-package-broker --locked 427 passed, cargo clippy --workspace --tests --locked -- -D warnings clean, cargo clippy -p now-package-broker --release --locked -- -D warnings clean (this is what type-checks the release-only SystemRecorder::flush), cargo test -p testsuite --test integration_tests --locked sysevent 7 passed, cargo doc -p now-package-broker --no-deps clean under RUSTDOCFLAGS=-D warnings.
This test uses the thread-local audit recorder before and after await, but this crate enables Tokio’s multi-thread runtime. The task may resume on another worker, so the event can remain on the original thread and make this assertion flaky. Run this test with flavor = "current_thread", as the other take_events() tests in policy_store/mod.rs do.
Closing the Event Log queue ended the worker's iteration, but the normal
shutdown then joined it without a bound, so a sink that stopped making
progress held the shutdown until the agent stopped the process, despite
the documented five-second limit.
The worker is now joined only while it finishes within the same grace the
flush uses, and is detached afterwards: it holds nothing the process needs
to release, and a panic is still reported. Both waits share one polling
helper, so the flush and the worker exit cannot drift apart.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Addressed both open findings from the last review (5359092721) in 3ec08860, plus the one it flagged as previously missed.
High — the shutdown could still block (audit.rs). Closing the queue ends the worker's iteration only once it has drained what is left, and the join that followed had no bound, so the documented five-second grace covered the flush but not the worker exit: a sink that stopped making progress held the shutdown until the agent stopped the process. drain() is now drain_with_grace(EVENT_LOG_SHUTDOWN_GRACE): drop the sender, wait at most the grace for JoinHandle::is_finished(), join and report a panic if it finished, otherwise log and detach. Detaching is safe because the worker owns nothing the process must release, and the give-up is logged rather than silent. The flush and the worker exit now share one wait_until(settled, grace) helper, and the constant is renamed since it bounds both. New test: closing_the_queue_gives_up_on_a_sink_that_stopped_making_progress.
Medium — "drain only after all policy tasks finish" (pipe.rs:188). Already fixed in 268554c5; answered in the thread. The queue is closed only when no connection can still commit (lease-counted), and is flushed-but-left-open otherwise, so a late terminal event is no longer refused.
Previously missed.readiness_reloads_each_disk_state_after_provisional_load now runs on #[tokio::test(flavor = "current_thread")]. It was the last take_events() test on the multi-thread runtime, where an await can move the task to another worker and the thread-local recording is then missed; I checked every other take_events() test and they were already pinned.
The Updater and PEDM families were each given their own hundred, but the
user session header still described a ten-wide range. Name it 6000-6099 in
both the event table and the Agent message catalog, so the three families
read the same way.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the two now-policy crates consistent: both require the 0.7 series
rather than a single patch release.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Adds structured Windows Event Log auditing for package policy write attempts and outcomes (IDs 8000–8005), plus externally observed policy changes (8010–8011). Events publish after authoritative state changes, use bounded privacy-safe fields, and flow through a bounded asynchronous Event Log queue.
Adds EN/FR/DE message catalogs embedded in production Agent and Gateway binaries, along with an MSI-managed Agent Event Log source registration whose
EventMessageFilevalue is removed on uninstall through the ordinary MSI component lifecycle (RegistryKeyAction.create), leaving any other value under the source key untouched. Windows CI resolvesmc.exeonly from trusted installed SDK locations, and pins the declared source registration for both architecture variants of the MSI component. Installing and removing the MSI is not exercised in CI; the install/uninstall behavior itself is Windows Installer's own registry lifecycle, not something this repository tests.