Unify audit with capture denial routing - #847
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Unifies --audit with the existing captureDenials pipeline and adds guarded-WPR ETL retention.
Changes:
- Routes audit capture through
mxc_engine. - Adds guarded analysis-plus-ETL transfer.
- Reuses canonical denials for audit artifact generation.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/host/plm/src/stop.rs |
Extracts canonical-denials post-processing. |
src/host/plm/src/elevated.rs |
Adds analyzed trace transfer protocol. |
src/host/plm/src/analysis.rs |
Returns the written denials document. |
src/core/wxc/src/main.rs |
Routes audit through engine capture. |
src/core/wxc/src/audit.rs |
Prepares and finalizes audit artifacts. |
src/core/wxc/Cargo.toml |
Adds learning-mode dependency. |
src/core/mxc_engine/src/run.rs |
Exposes audit runner resolution. |
src/core/mxc_engine/src/lib.rs |
Re-exports audit resolver. |
src/core/mxc_engine/src/guarded_capture.rs |
Enables PLM trace transfer. |
src/Cargo.lock |
Records dependency update. |
src/backends/appcontainer/common/src/guarded_capture.rs |
Extends guarded-capture interfaces. |
src/backends/appcontainer/common/src/base_container_runner.rs |
Retains guarded ETL in BaseContainer. |
src/backends/appcontainer/common/src/appcontainer_runner.rs |
Retains guarded ETL in AppContainer. |
README.md |
Documents unified audit behavior. |
docs/learning-mode/capabilities.md |
Updates capture and retention guidance. |
.github/copilot-instructions.md |
Updates architecture documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/core/wxc/src/audit.rs:104
- If the ETL move succeeds but the following denials move fails (for example, due to an I/O error after the preflight),
finalizereturns whilecapture.etlPathstill points at the removed source. The retained trace is then present attrace.etlbut undiscoverable through the emitted metadata. Update each metadata path immediately after its corresponding move so partial relocation failures still identify every preserved artifact.
move_new_file(&source_etl, &final_etl)?;
move_new_file(&source_denials, &final_denials)?;
capture.output_path = final_denials.to_string_lossy().into_owned();
capture.etl_path = Some(final_etl.to_string_lossy().into_owned());
src/backends/appcontainer/common/src/base_container_runner.rs:2708
Droppassesallow_retention = false, but the guarded branch still invokesstop_analyzed()and writes a denials document. Unlike the native branch above, an abandoned guarded capture therefore performs analysis and leaves an orphan unique JSON output with no observable metadata. Route the drop path throughsession.discard()and only analyze/write outputs after a terminalwait().
.filter(|_| allow_retention);
let exit_code = self.last_exit_code.unwrap_or(-1);
let stop_result = match etl_path.as_deref() {
Some(etl_path) => session.stop_analyzed_with_trace(etl_path),
None => session.stop_analyzed(),
};
src/backends/appcontainer/common/src/appcontainer_runner.rs:1688
Dropcalls this method withallow_trace_transfer = false, but this branch then callsstop_analyzed()and proceeds to write the canonical JSON. Dropping an unwaited guarded sandbox therefore creates an orphan denials file even though no caller can observe its unique path, rather than discarding the capture as the new abandonment contract requires. UseGuardedCaptureSession::discard()on the drop path and skip JSON generation; reserve stop/analyze (and optional ETL transfer) for a terminalwait().
let capture_result = match etl_path.as_deref() {
Some(etl_path) => session.stop_analyzed_with_trace(etl_path),
None => session.stop_analyzed(),
};
src/host/plm/src/elevated.rs:1295
- The trace has already been persisted by
read_analysis_and_trace_response, but a subsequent guardian exit/wait failure makes this method returnErr. Both callers infer transfer completion from the overall result beingOk, so this case leaves a retained ETL withoutcaptureDenialsError.etlPath. Preserve transfer completion independently (or reliably remove the persisted destination before returning the wait error) so every remaining ETL stays discoverable.
let analysis = result?;
wait_result?;
Ok(analysis)
Route wxc-exec --audit through captureDenials so native PSEC/V2 remains preferred and guarded WPR provides compatible analysis and ETL retention. Reuse canonical denials for adjusted policy generation and support retainEtl consistently across both capture providers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
Update repository guidance for captureDenials-backed audit routing and guarded retainEtl parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
a300213 to
5e1090c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/host/plm/src/elevated.rs:1590
- The ETL has already been persisted when
resultisOk, but a subsequent child wait failure makes this method returnErr. Both runners inferetl_was_transferredfromstop_result.is_ok(), so this path leaves a retained ETL on disk without publishingcaptureDenialsError.etlPath. Preserve transfer completion independently (or reliably remove the destination) when reporting the child-exit error.
let wait_result = wait_for_child_exit(
self.process.0,
deadline.saturating_duration_since(Instant::now()),
);
let analysis = result?;
wait_result?;
Ok(analysis)
src/core/wxc/src/audit.rs:104
- If the ETL move succeeds but the following denials move fails, metadata still points to the now-missing source ETL because both fields are updated only after both moves. Update each metadata path immediately after its corresponding successful move so every partial-failure pointer remains valid.
move_new_file(&source_etl, &final_etl)?;
move_new_file(&source_denials, &final_denials)?;
capture.output_path = final_denials.to_string_lossy().into_owned();
capture.etl_path = Some(final_etl.to_string_lossy().into_owned());
docs/learning-mode/capabilities.md:250
- This new guarded-retention behavior leaves the public documentation contradictory:
docs/schema.md:94-95still says guarded WPR rejects retention, anddocs/process-container/os-version-support.md:95-96says raw ETL never crosses into the SDK result. Update those behavior-defining references in this PR so users do not receive incompatible contracts.
for diagnostics after a terminal wait. Both native PSEC/V2 capture and the
guarded-WPR fallback honor retention; guarded WPR transfers the sealed trace
back to the unelevated caller after process-scoped analysis. Native retention
begins under `%LOCALAPPDATA%\Microsoft\MXC\capture-denials\working` and moves
to a protected per-run directory under `capture-denials\retained` only after
sealing succeeds. Guarded WPR writes the retained ETL beside its unique
denials JSON output, using the same file stem with an `.etl` extension.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
src/core/wxc/src/audit.rs:104
- These moves are not transactional: if the ETL move succeeds but the denials move fails (for example, during the copy fallback),
finalizereturns while metadata still points to the now-missing ETL source. Update each metadata path immediately after its corresponding successful move so the emitted pointer remains usable on a partial failure.
move_new_file(&source_etl, &final_etl)?;
move_new_file(&source_denials, &final_denials)?;
capture.output_path = final_denials.to_string_lossy().into_owned();
capture.etl_path = Some(final_etl.to_string_lossy().into_owned());
src/host/plm/src/elevated.rs:1591
- The ETL has already been fully persisted when
resultisOk, but a subsequent guardian wait timeout/nonzero exit makes this method returnErr. Both runner callers currently infer transfer completion fromstop_result.is_ok(), so this leaves a valid retained ETL on disk withoutcaptureDenialsError.etlPath. Return transfer completion separately from the guardian-exit result (or otherwise ensure callers advertise or remove a successfully persisted trace).
let analysis = result?;
wait_result?;
Ok(analysis)
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/host/plm/src/elevated.rs:1590
- The trace has already been persisted by
read_analysis_and_trace_responsebefore this child-exit check. Ifwait_for_child_exitfails or times out, this method returnsErrwhile leaving the ETL attrace_destination; both runner call sites deriveetl_was_transferredfromstop_result.is_ok(), so they suppresscaptureDenialsError.etlPathand leave an undiscoverable retained trace. Return transfer completion independently (or reliably remove the destination) for post-transfer wait failures.
let analysis = result?;
wait_result?;
src/core/wxc/src/audit.rs:104
- These two moves are not transactional, but metadata is updated only after both succeed. If the ETL move succeeds and the denials move then fails,
capture.etl_pathstill points to the now-missing source while the retained ETL is stranded attrace.etl; the CLI subsequently emits that stale capture pointer. Update each metadata field immediately after its corresponding successful move so partial failure still advertises the preserved artifacts.
move_new_file(&source_etl, &final_etl)?;
move_new_file(&source_denials, &final_denials)?;
capture.output_path = final_denials.to_string_lossy().into_owned();
capture.etl_path = Some(final_etl.to_string_lossy().into_owned());
src/backends/learning_mode/windows/src/etl_decode.rs:620
- This converts the canonical analyzer's event-budget truncation into a hard filtering failure. On the
(MAX_PROCESSED_EVENTS + 1)th in-scope event, the existing analyzer would setdenied_resources_truncatedand emit partial JSON, but this pass aborts before creating the filtered ETL, contrary to the PR's stated behavior that truncated analysis preserves JSON and ETL. Keep at most one extra in-scope event as a truncation sentinel while continuing to count source events; the filtered trace will then cause the canonical analyzer to report truncation normally.
if acc.relog_selected_event_indices.len() >= MAX_PROCESSED_EVENTS {
acc.decode_error = Some(format!(
"trace exceeded the {MAX_PROCESSED_EVENTS}-event process-scoped relogging limit"
));
acc.stop_requested = true;
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eff3ee2-323c-494d-99b9-f7b54e495216
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/core/wxc/src/audit.rs:104
- The two moves are not transactional. If moving the ETL succeeds but moving the JSON fails, the metadata still points to the old ETL path, so audit failure output leaves the relocated trace undiscoverable. Update each metadata path immediately after its corresponding successful move.
move_new_file(&source_etl, &final_etl)?;
move_new_file(&source_denials, &final_denials)?;
capture.output_path = final_denials.to_string_lossy().into_owned();
capture.etl_path = Some(final_etl.to_string_lossy().into_owned());
src/backends/appcontainer/common/src/capture_output.rs:180
- This only recognizes the literal
.etlextension, butoutputPathaccepts Win32 aliases. For example,denials.etl::$DATAproduces JSONdenials.<run>.etl::$DATAand retained ETLdenials.<run>.etl, which are the same default data stream;.etlhas the same trailing-space collision. The trace transfer then occupies the JSON target and JSON creation fails. Normalize Win32 trailing-dot/space and::$DATAaliases when deriving the pair, or reject such configured paths.
let retained_name = match extension {
Some(ext) if ext.eq_ignore_ascii_case("etl") => {
format!("{stem}.{run_id}.trace.etl")
}
Some(_) => format!("{stem}.{run_id}.etl"),
None => format!("{file_name}.{run_id}.etl"),
src/host/plm/src/elevated.rs:1590
read_analysis_and_trace_responsehas already persistedtrace_destinationbefore this wait. If the guardian then times out or exits nonzero,wait_result?returns an error; both runner callers currently deriveetl_was_transferredfromResult::is_ok(), so they omitcaptureDenialsError.etlPatheven though the retained ETL remains on disk. Preserve transfer completion independently in the return contract, or remove/report the persisted destination on this post-transfer failure path.
}
}
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/core/wxc/src/audit.rs:104
- These two moves are not atomic. If the ETL move succeeds but the denials move fails (for example due to an I/O or AV error),
finalizereturns before either metadata field is rewritten, leavingetlPathpointing to the now-missing source while the trace exists attrace.etl. Update each metadata path immediately after its corresponding successful move, or roll the first move back when the second fails.
move_new_file(&source_etl, &final_etl)?;
move_new_file(&source_denials, &final_denials)?;
capture.output_path = final_denials.to_string_lossy().into_owned();
capture.etl_path = Some(final_etl.to_string_lossy().into_owned());
src/host/plm/src/elevated.rs:1585
- If the ETL is received and persisted successfully but
wait_for_child_exitthen fails, this method returnsErreven thoughtrace_destinationstill exists. Both guarded runners infer transfer completion withstop_result.is_ok(), so they omitcaptureDenialsError.etlPathand leave a retained trace that callers cannot discover or clean up. Preserve the transfer-completed state independently of the guardian wait result (or reliably remove the persisted destination before returning the error).
let analysis = result?;
wait_result?;
Ok(analysis)
src/backends/appcontainer/common/src/capture_output.rs:180
- The paired names are still not guaranteed to be distinct for accepted Windows aliases. For example,
outputPath: "C:\\logs\\denials.etl::$DATA"produces JSON atdenials.<run-id>.etl::$DATAand the retained trace atdenials.<run-id>.etl;::$DATAdenotes that same unnamed stream, so transfer creates the JSON target and the latercreate_newwrite always fails. Trailing-space/dot variants of.etlhave the same problem. Reject/normalize these aliases or derive an ETL name that remains distinct under Windows target semantics, not just string extension comparison.
let retained_name = match extension {
Some(ext) if ext.eq_ignore_ascii_case("etl") => {
format!("{stem}.{run_id}.trace.etl")
}
Some(_) => format!("{stem}.{run_id}.etl"),
None => format!("{file_name}.{run_id}.etl"),
| } else { | ||
| mxc_engine::resolve_runner(&request, &mut logger) | ||
| }; | ||
| #[cfg(not(target_os = "windows"))] |
There was a problem hiding this comment.
issue (blocking): Non-Windows builds reference Windows-only DACL state.
wxc remains a workspace member on Linux and macOS, but the newly added non-Windows resolution path flows into the ungated DACL cleanup slot, guard, resolved.dacl_manager access, and cleanup calls. ResolvedRunner::dacl_manager and wxc_common::filesystem_dacl only exist on Windows, so these references break non-Windows compilation.
Could we gate the complete DACL lifecycle with #[cfg(target_os = "windows")], including field extraction and cleanup calls? A Linux/macOS cargo check -p wxc CI check would prevent this from regressing.
| } | ||
| } | ||
|
|
||
| fn read_analysis_and_trace_response( |
There was a problem hiding this comment.
issue (blocking): A failed optional ETL transfer discards successful denial analysis.
read_analysis_and_trace_response receives a valid AnalysisResult before reading the retained ETL response. If that later response is an error or its payload transfer fails, the function returns Err and drops the analysis, so the caller cannot write the primary denials.json.
Could we return the analysis and trace-transfer status separately? The caller should always write JSON after successful analysis and report retention failure through capture_denials_error. Please add a regression test where analysis succeeds and the following trace response fails.
| } | ||
| let event_index = self.event_cursor.fetch_add(1, Ordering::Relaxed); | ||
| let selected_index = self.selected_event_cursor.load(Ordering::Relaxed); | ||
| if self.selected_event_indices.get(selected_index) == Some(&event_index) { |
There was a problem hiding this comment.
issue (blocking, security): Relogging does not revalidate the selected event's PID.
The first ETW consumer selects events using PID and lifetime but records only their ordinals. The second consumer injects by ordinal without checking EventHeader.ProcessId. If equal-timestamp events from different processes are ordered differently by the consumers, a foreign-process event can be injected while the existing count checks still pass.
Could we pass the attested PID set into ProcessScopedTraceFilter and require the current event PID to be attested before calling Inject? A regression test should cover an ordinal match paired with a foreign PID.
| let final_etl = context.log_dir.join("trace.etl"); | ||
| ensure_destination_available(&source_etl, &final_etl)?; | ||
| ensure_destination_available(&source_denials, &final_denials)?; | ||
| move_new_file(&source_etl, &final_etl)?; |
There was a problem hiding this comment.
issue (non-blocking): Partial relocation can strand the ETL and leave stale metadata.
The ETL is moved before the denials JSON, while both metadata paths are updated only after both moves succeed. If the JSON move fails, trace.etl remains at its destination but metadata still points to the deleted source ETL.
Could we move the primary JSON first or update each metadata path immediately after its move succeeds? A fault-injection test for "first move succeeds, second move fails" would lock in the expected behavior.
| .filter(|_| allow_trace_transfer); | ||
| let exit_code = self.last_exit_code.unwrap_or(-1); | ||
| let capture_result = match session.stop_analyzed() { | ||
| let capture_result = match etl_path.as_deref() { |
There was a problem hiding this comment.
suggestion (non-blocking): Share guarded-capture finalization between both runners.
This teardown duplicates the BaseContainer flow for selecting analysis versus analysis-plus-trace, tracking transfer success, writing JSON, patching metadata, and constructing failure metadata. The two implementations already use different control-flow shapes, which makes correctness fixes easy to apply to only one.
Could we extract a shared guarded-capture finalizer in guarded_capture.rs or capture_output.rs and call it from both runners?
| .as_ref() | ||
| .is_some_and(|config| config.retain_etl) | ||
| && !self | ||
| .guarded_capture_factory |
There was a problem hiding this comment.
suggestion (non-blocking): Centralize the retainEtl provider-capability check.
The same invariant is implemented inline here and through a BaseContainer-only helper, with different test coverage.
Could we move the predicate beside RETAIN_ETL_UNSUPPORTED_MSG in shared guarded-capture code and have both runners call it?
| let is_retained_capture_dir = directory | ||
| .parent() | ||
| .and_then(Path::file_name) | ||
| .is_some_and(|name| name.eq_ignore_ascii_case("retained")); |
There was a problem hiding this comment.
suggestion (non-blocking): Avoid coupling cleanup to the private "retained" directory literal.
The CLI recognizes a backend-managed directory by a string independently owned by base_container_runner.rs. Renaming the backend directory would silently disable cleanup.
Could we expose a shared constant or backend-owned path/predicate helper instead of duplicating this convention?
|
|
||
| /// Resolve a runner for the `wxc-exec --audit` compatibility workflow. | ||
| #[cfg(target_os = "windows")] | ||
| pub fn resolve_runner_for_audit( |
There was a problem hiding this comment.
suggestion (non-blocking): Remove the behaviorally redundant audit resolver.
On Windows, resolve_runner_for_audit and resolve_runner call the same inner function with identical arguments and error conversion. The new public API and call-site branch imply audit-specific resolution that does not currently exist.
Could we delete resolve_runner_for_audit, its re-export, and the cli.audit branch, then call resolve_runner unconditionally?
| // SAFETY: COM is initialized on this thread, the in-proc class ID is | ||
| // fixed, and the returned interface remains apartment-local. | ||
| let relogger: ITraceRelogger = unsafe { | ||
| CoCreateInstance(&CLSID_TraceRelogger, None, CLSCTX_INPROC_SERVER) |
There was a problem hiding this comment.
suggestion (non-blocking, tests): Add a deterministic seam around Trace Relogger creation.
Directly constructing CLSID_TraceRelogger forces tests for the security-sensitive injection path to use real COM and ETL resources.
Could we isolate relogger construction and event injection behind a small factory/interface so tests can use a fake for PID validation, count reconciliation, and failure handling?
| } | ||
| } | ||
|
|
||
| fn read_analysis_and_trace_response( |
There was a problem hiding this comment.
suggestion (non-blocking, tests): Separate protocol parsing from OS liveness polling.
Taking a concrete std::fs::File and Windows HANDLE prevents in-memory tests for partial reads, malformed frames, and the analysis-success/trace-failure sequence.
Could we parse framing and payloads over impl Read and inject process-liveness polling through a callback or small trait?
| its public certificate: | ||
|
|
||
| ```powershell | ||
| $cert = New-SelfSignedCertificate ` |
There was a problem hiding this comment.
suggestion (non-blocking): Move the developer code-signing walkthrough out of this PR.
The self-signed certificate and signtool walkthrough documents an existing trust gate rather than the audit/capture-denials unification. Keeping it here expands an already large, security-sensitive review without contributing to the stated change.
Could we move this walkthrough to a separate documentation PR or commit?
📖 Description
Unifies
wxc-exec --auditwith theprocessContainer.captureDenialsexecution pipeline.
--auditsynthesizes allow-mode capture with ETL retention andpermissiveLearningMode, then delegates backend selection and execution tomxc_engine. Native PSEC/V2 capture remains preferred when available andpolicy-compatible; legacy SBOX and AppContainer tiers use guarded WPR with
exact handle-attested process scope.
WPR records a host-wide source ETL inside protected elevated scratch. After the
sandbox process tree terminates, the guardian uses normalized
FILETIMEselection and Windows Trace Relogger to create a second ETL containing only
supported Learning Mode events from the attested process generations. Analysis
and explicit retention both consume that filtered ETL. The host-wide source
never crosses the privilege boundary, and relogging failure transfers no trace.
Both native and guarded providers honor explicit
retainEtlafter a terminalwait. Abandoned process handles discard the trace rather than exposing
incomplete output.
Audit post-processing consumes the canonical
DenialsDocumentreturned by theselected provider, relocates outputs to
denials.jsonandtrace.etl,snapshots file-based source policies, and generates
Adjusted_*.jsonwithoutdecoding the ETL again. Truncated analysis preserves JSON, ETL, and the source
snapshot but does not generate an adjusted policy.
🔗 References
🔍 Validation
cargo test -p learning_mode_windows -p plm --libcargo clippy --workspace --all-targets -- -D warningswxc-exec.exeandplm.exe✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity withthe GitHub Actions build; it runs on merge to
main, and Microsoft reviewerswith write access can trigger it on a PR with
/azp run. Seedocs/pull-requests.md.
If the
dependency-feed-checkcheck fails on a new dependency, the crate mustbe added to the feed before the PR can pass. See
docs/pull-requests.md
for the steps.
Microsoft Reviewers: Open in CodeFlow