Repository navigation
feat: decode WFP Learning Mode network events - #1418
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
0cb155e to
94648ef
Compare
ff0d00b to
d66c3b4
Compare
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Verified review notes
The public Rust DeniedResource construction break is the principal finding. The other inline comments cover network-detail aggregation, event-rate work, regression tests and the unused direction value. This is a comment-only review, as requested.
Verified clean: The diff matches GitHub's patch. The new decoder has 12 focused tests (none in this file at the base), including app-isolation capability mapping, a network-details assertion, explicit-deny/proxy exclusions, invalid remote addresses, source-identity rejection and a raw-property TDH fixture. The final stack deliberately excludes brokered network events from guarded WPR and documents pid: 0; neither is being charged to this PR. I did not verify the event schema against an options-capable live Windows host.
Verified pre-existing — not attributed to this PR
No independent pre-existing defect is being charged to the author. The (resource, accessType) dedup key and the verbose byte-budget implementation predate this PR; the comments below specifically address their interaction with the new network metadata and high-cardinality network events. Claims about a missing complete details assertion, a broken guarded-WPR network capture path, and a security bypass from synthetic inbound events were not substantiated and are not posted as findings.
94648ef to
08def3b
Compare
b3f6785 to
31a0195
Compare
|
Follow-up review pass found and fixed two additional issues in 05c16d0: the new public verbose provider/reason vocabulary is now explicitly non-exhaustive and the serialized document version is bumped to 3; high-cardinality actionable overflow now caches saturation and avoids repeated 4,096-group scans and serialized-size work. The full stacked tip passes 289 focused Learning Mode tests and mxc-sdk Clippy with warnings denied. |
05c16d0 to
183a905
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The decoder does not enforce the stated 24-property schema, and public PID/timestamp documentation conflicts with the new behavior.
2 open findings
What changed in this PR
Adds decoding and reporting for WFP Learning Mode network-denial events.
Changes:
- Decodes App Isolation and Tessera network decisions.
- Produces actionable network/capability denials and verbose diagnostics.
- Adds TDH, extraction, routing, redaction, and saturation tests.
| File | Description |
|---|---|
verbose_telemetry.rs |
Maps the network provider GUID. |
tdh_decode.rs |
Adds a synthetic network TDH fixture. |
network_extractors.rs |
Implements network event extraction. |
mod.rs |
Registers the network extractor module. |
extractors.rs |
Routes, classifies, and sanitizes network events. |
etl_decode.rs |
Integrates events and bounded verbose aggregation. |
verbose_logging.rs |
Adds network diagnostic categories. |
model.rs |
Documents network resources and deduplication. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Follow-up review
The six findings in my earlier comment review are resolved or moot in the updated decoder. One distinct public Rust API compatibility break remains: this PR changes two previously exhaustive, publicly re-exported enums while mxc-sdk is still version 1.0.0. That is the requested change in the inline comment.
Verified clean: The current GitHub patch matches the captured diff at 183a905 against base 7cd00d1. Removing DeniedResource.details restores the old public struct-literal shape; the first-observation contract is documented and tested, overflow avoids repeated serialized-size calculation after actionable-only saturation, ApplicationId redaction has direct and end-to-end tests, the six schema/decision guards have independent negative cases, and the unproduced Forward type is removed. The current tree matches the tree already verified locally with cargo test --quiet -p mxc-sdk --lib learning_mode_windows (241 passed) and cargo test --quiet -p mxc-sdk --lib learning_mode_core (49 passed); those results were reused rather than rerunning identical content. I did not validate against an options-capable live Windows host.
Verified pre-existing — not attributed to this PR
None. Both diagnostic enums were publicly re-exported and exhaustive at the PR base, but adding variants and #[non_exhaustive] to them is introduced by this PR. The old source contract, not its existence, is the compatibility baseline.
There was a problem hiding this comment.
🟡 Changes recommended
An oversized actionable signature can unnecessarily evict all retained diagnostic groups before overflowing.
1 open finding
5 resolved since last review
Normalize port zero and ICMP ports in structured egress endpoints Preserve observed event identity for unsupported and decode-error outcomes Validate each artifact version against its closed schema Require complete v1 schema before issuing denials Document network exceptions for DeniedResource fields
🧠 Review effort: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Preserve the existing DeniedResource source contract, keep network metadata in redacted verbose signatures, fail closed on every schema gate, and avoid serialized-size work once verbose retention is saturated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Make the public provider and outcome enums extensible, bump the closed verbose JSON vocabulary to version 3, and cache actionable-only saturation so high-cardinality overflow avoids repeated scans and serialization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Keep the published verbose enums exhaustive, route complete WFP diagnostics through an internal version-5 model, and retain reason-specific configuration guidance without changing DeniedResource. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
2492bb0 to
aa925c7
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e408b60b-e267-416c-806d-0a7e119fe53f
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Re-verified #1418 at 1212563 against base e157fa4 and am approving on the published v1.0.0 Rust API compatibility baseline. The PR no longer adds variants or #[non_exhaustive] to the public verbose enums; WFP-only diagnostics use crate-internal versioned types. The external-crate test exhaustively matches the surviving public variants. The main-only provider/reason and event_name additions from #1396 landed after the v1.0.0 release; I accept their removal here to restore the published contract, rather than treating those unreleased additions as a blocker for this PR. This does not attribute separate post-release enum additions from #1395 to #1418.
All six findings in my first comment review are now addressed or moot. On this exact tree (0062c923), cargo test --quiet -p mxc-sdk --test learning_mode_public_api_compat passed (1), cargo test --quiet -p mxc-sdk --lib learning_mode_core passed (57), cargo test --quiet -p mxc-sdk --lib learning_mode_windows passed (268), and cargo test --quiet -p mxc-sdk --lib verbose_telemetry passed (10). This is follow-up verification of the filed findings, not a new full-source audit. I did not validate a live options-capable Windows host or non-Windows targets.


📖 Description
Adds decoding for WFP Learning Mode network decision events and maps actionable direct-default-deny decisions into the caller-facing denial model.
This is PR 2 of 4 in the WFP Learning Mode stack. Its base is the API-layer branch.
🔗 References
Related to #1286.
Depends on the preceding API PR in this stack.
🔍 Validation
learning_mode_core— 48 tests passed plus doctest.learning_mode_windows— 230 tests passed.mxc_engine— 97 tests passed plus focused integration target.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (not applicable)📋 Issue Type
🧱 Stack
Review and merge in this order.
Microsoft Reviewers: Open in CodeFlow