feat(agent): audit package policy changes - #1980
feat(agent): audit package policy changes#1980Benoît Cortier (CBenoit) wants to merge 5 commits into
Conversation
Record bounded write attempts and operation-specific outcomes without exposing policy content or blocking request admission on Event Log I/O. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Compile localized message resources for release and production builds using trusted installed Windows SDK tools. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Verify every shared event code and policy insertion string across both localized Windows message catalogs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Implementation notes:
Note Human-tuned, LLM-assisted content. |
There was a problem hiding this comment.
🟡 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-codesplus catalog parity tests. - Adds and embeds Devolutions Agent/Gateway Windows Event Log message catalogs via
mc.exeduringrelease/productionbuilds. - Threads per-request write-audit context through the package broker server and policy store to record deny/fail/success and external-change observations.
File summaries
| File | Description |
|---|---|
| devolutions-gateway/devolutions-gateway.mc | Adds policy management audit message templates (8000–8011). |
| devolutions-gateway/build.rs | Embeds the Gateway .mc into release/production binaries; adds mc.exe discovery logic. |
| devolutions-agent/devolutions-agent.mc | New Agent Windows Event Log message catalog including policy audit IDs. |
| devolutions-agent/build.rs | Embeds the Agent .mc into release/production binaries. |
| crates/sysevent-codes/src/lib.rs | Defines policy audit event codes and structured field builders (+ unit tests for field order). |
| crates/sysevent-codes/tests/message_catalog_parity.rs | Validates both .mc catalogs define all event codes and correct insertion counts. |
| crates/now-package-broker/src/lib.rs | Adds the Windows-only audit module. |
| crates/now-package-broker/src/audit.rs | New audit implementation: bounded fields, async-safe recording, and Windows Event Log worker (prod). |
| crates/now-package-broker/src/auth.rs | Exposes authenticated client executable path for auditing. |
| crates/now-package-broker/src/server/mod.rs | Creates/scopes per-request policy write audit context for PUT /v1/policy. |
| crates/now-package-broker/src/policy_store/mod.rs | Adds audited replace path and emits external-change audit events on publication. |
| crates/now-package-broker/Cargo.toml | Adds sysevent dependencies needed for auditing. |
| Cargo.lock | Locks new workspace dependency edges for auditing. |
| .github/workflows/ci.yml | Finds mc.exe via installed Windows SDK roots and exports WindowsSdkVerBinPath for builds. |
Review details
Suppressed comments (1)
crates/now-package-broker/src/policy_store/mod.rs:1360
- 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;
- Files reviewed: 13/14 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Restrict message compiler discovery to trusted SDK paths, keep thread-local audit assertions on one runtime thread, and avoid an unnecessary path allocation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 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.
crates/now-package-broker/src/policy_store/mod.rs:405
audit_path = observation.canonical_path.clone()is done even whenauditis None, which adds an avoidable allocation on the hot error paths. Sincepublish_external_observationreturns a snapshot containing the same path, you can defer path materialization to theif let Some(audit)branch and borrow frommanagement.configured_path(and apply the same pattern to the similar audit_path clones in the other error branches below).
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Borrow published management paths only when an audit terminal event is recorded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 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.
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
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 and embeds them in production Agent and Gateway binaries. Windows CI resolves
mc.exeonly from trusted installed SDK locations. Targeted policy audit tests and production Agent and Gateway builds cover the event contract and embedded resources.Stacked on #1979. Extracted from #1963.
Issue: #1963