Repository navigation
fix: route captured network decisions through Tessera - #1419
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
ff0d00b to
d66c3b4
Compare
33bf935 to
731c328
Compare
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Verified review notes
The substantive issue is treating the normalized network_egress Option as evidence that a caller explicitly requested egress; the second comment asks for a negative control on the separate no-capture guard. This is a comment-only review, not a change request.
Verified clean: GitHub's patch matches the local delta. The new tests preserve the proxy posture and prove that the default-deny egress table is still serialized. ensure_capability avoids duplicate capability strings. The runner gains only two new tests (48 to 50); neither covers a parsed request with absent or ingress-only network policy. I did not reproduce a brokered-DNS/Tessera bypass on a PSEC host and am not asserting one.
Verified pre-existing — not attributed to this PR
src/mxc-sdk/src/core/mxc_common/network_parser.rs:159-175 is byte-identical at base and head. Its normalization of absent egress to Some(default) is not itself charged to this PR; the new gate's reliance on presence is the introduced defect. The existing egress-rule serializer and guarded AppContainer capability helper are also unchanged and are not being treated as regressions.
d66c3b4 to
b3f6785
Compare
731c328 to
cd9e80a
Compare
b3f6785 to
31a0195
Compare
cdbd9d6 to
82dce5d
Compare
05c16d0 to
183a905
Compare
72c85b5 to
67e25e8
Compare
|
Gudge (@MGudgin) The remaining capability-presence finding is fixed in a01d125. network.egress absence now remains distinct through parsing, and the parse-to-PSEC matrix covers absent network, ingress-only, explicit deny-default egress, plus the no-capture negative control. The complete mxc-sdk library run passed 2,775 tests; one unrelated host-dependent loopback proxy test timed out consistently on this machine. Please re-review the updated tip. |
a01d125 to
7f147b9
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive network enforcement changes depend on preceding stacked ABI and decoder changes, warranting final human validation.
0 open findings
What changed in this PR
Routes ProcessContainer denial-capture network decisions through Tessera while preserving authoritative PSEC enforcement and proxy isolation.
Changes:
- Synthesizes direct-egress capability only for eligible network-aware captures.
- Emits and validates version 5 capture diagnostics.
- Adds regression coverage for direct, proxy, and legacy postures.
| File | Description |
|---|---|
src/tools/wxc/src/audit.rs |
Validates and publishes versioned verbose diagnostics. |
src/mxc-sdk/src/core/mxc_common/network_parser.rs |
Preserves explicit egress-section presence. |
src/mxc-sdk/src/core/mxc_common/mod.rs |
Exposes the parser within the crate. |
src/mxc-sdk/src/backends/process_container/common/capture_output.rs |
Writes version 5 native-capture diagnostics. |
src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs |
Selects network-aware capture routing and decoding. |
src/mxc-sdk/src/backends/process_container/common/base_container_helpers/tests.rs |
Updates helper invocations for capture routing. |
src/mxc-sdk/src/backends/process_container/common/base_container_helpers.rs |
Adds conditional internetClient synthesis. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2492bb0 to
aa925c7
Compare
6150aa6 to
7bc39c7
Compare
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
7bc39c7 to
4ce4047
Compare
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Fresh review of the seven-file PR
Requested change: Preserve implicit deny-default egress for proxy validation while retaining authored-egress presence for the native-capture capability gate. The shared parser currently rejects an ingress-only ProcessContainer proxy configuration that passed at the PR base. The other comments are non-blocking performance, coverage, and maintainability observations; the two findings in my earlier review are addressed and are not repeated.
Verified clean: Current head 4ce4047 and base c6f301d have the expected merge-base, and the captured patch matches GitHub's PR diff. I inserted the same temporary ingress-only-plus-loopback-proxy regression test into detached base/head worktrees: it passed at the base and failed at the head with ConfigParse("runtimeConfig.networkProxy requires an egress policy"); both temporary edits were removed. The direct/proxy capability tests and v5 writer/CLI acceptance checks pass. The new capability requires both explicit egress and a network-aware trace, preserves PSEC deny-default egress, and excludes runtime proxy mode. I found no demonstrated sandbox bypass. I could not exercise a live options-capable PSEC/Tessera host.
Verified pre-existing — not attributed to this PR
docs/schema.md and the pre-existing proxy validator are byte-identical at base and head; they are relevant only because the new parser presence semantics make the validator reject an accepted configuration. The network event extractor and verbose aggregator are also byte-identical; the performance note concerns their newly reachable high-cardinality path after this PR's capability grant, not an assertion that this PR introduced their implementation. #1420 documents the v5 artifact and capture capability exception, so those interim documentation gaps are not charged here. I did not repeat the optional v5 collision-test suggestion from the report: both writers use the existing tested transactional paired-output helper.
|
|
||
| let egress = convert_egress(network.egress)?; | ||
| policy.network_egress = Some(egress); | ||
| policy.network_egress = network.egress.map(convert_egress).transpose()?; |
There was a problem hiding this comment.
High (correctness; also reliability and cross-platform parity) — Preserve implicit deny egress for ingress-only proxy requests.
Attribution: introduced_by_change — this new assignment leaves network_egress as None when network.ingress is present without network.egress; previously the parser supplied Some(default-deny). The unchanged validate_directional_proxy_policy rejects None when a runtime proxy and directional ingress are present.
I reproduced this with network.ingress = {default: allow, hostLoopback: allow}, runtimeConfig.networkProxy = http://127.0.0.1:8080, and omitted network.egress: the same temporary unit test passes at base c6f301d but fails at this head with runtimeConfig.networkProxy requires an egress policy. docs/schema.md documents omitted egress as deny. The request is rejected before sandbox creation; this is a compatibility regression, not an egress bypass.
Fix: Keep authored-egress presence for the capture gate, but have proxy validation evaluate an absent egress section as effective ruleless deny. Add a permanent parser regression covering ingress-only plus proxy.
There was a problem hiding this comment.
Fixed in 8a693f0. Proxy validation now treats absent egress as effective ruleless deny without populating network_egress, so the capture capability gate still distinguishes authored egress. Added an ingress-only + runtime proxy parser regression for ProcessContainer, LXC, and Seatbelt, and documented the omitted-egress proxy behavior in docs/schema.md. Explicit allow-default and direct-rule proxy restrictions remain unchanged.
Validation from src: cargo fmt --all -- --check passed; cargo test -q -p mxc-sdk --lib -- network_parser base_container_runner capture_output capture_diagnostics etl_decode config_parser passed (279 tests, one opt-in benchmark ignored); cargo clippy -q -p mxc-sdk --lib --tests -- -D warnings passed.
| // Let outbound attempts pass the capability gate so Tessera's egress | ||
| // policy makes and records the decision. Proxy mode must remain direct- | ||
| // egress denied and therefore deliberately omits internetClient. | ||
| ensure_capability(&mut capabilities, INTERNET_CLIENT_CAPABILITY); |
There was a problem hiding this comment.
Medium (performance) — Measure the newly reachable high-cardinality capture path.
Attribution: newly_exposed_by_change — this capture-only internetClient grant lets direct default-deny attempts reach Tessera; the native path now persists their v5 WFP diagnostic signatures. The unchanged extractor includes volatile fields such as local ports and filter IDs in signature properties, and the bounded aggregator sizes and inserts each distinct key into a sorted vector before saturation.
Network-heavy workloads may produce thousands of distinct groups for repeated connections to the same destination. This is an unmeasured CPU/teardown risk, not an unbounded-output claim or a merge condition. Fix: Benchmark a representative high-cardinality capture; if material, aggregate successfully decoded events by stable reason/endpoint fields or use a keyed structure while retaining detailed malformed-event diagnostics.
There was a problem hiding this comment.
Measured in 8a693f0 with the new opt-in benchmark, documented in docs/development/build-and-test/pull-requests.md. Command from src: cargo test -q --release -p mxc-sdk --lib -- benchmark_high_cardinality_network_capture --ignored --nocapture passed.
The benchmark calls the production post-TDH extractor/accumulator, using valid direct-default-deny events to the same TCP destination, and compares identical signatures against changing filter IDs and ephemeral local ports. Descending filter IDs exercise front-of-vector insertions. Results on this Windows x64 host with the pinned Rust 1.93 toolchain:
| Events | Repeated-signature processing | High-cardinality processing |
|---|---|---|
| 4,096 | 68.79 ms | 148.83 ms |
| 100,000 | 1.702 s | 1.835 s |
| 1,000,000 | 16.769 s | 18.344 s |
High-cardinality retention saturated at 4,096 groups, with one actionable destination denial and exact overflow accounting; the verbose JSON was 9,539,872 bytes at one million events. Document merge + pretty JSON writing took about 65 ms. The high-cardinality timings include fixture-property mutation, so the difference is not purely aggregator cost.
I have kept the existing bounded diagnostic representation rather than adding an unproven keyed structure or dropping successfully decoded properties: the measured incremental cost is about 80 ms at the group bound and about 9% at one million events. This is a synthetic decoded-event measurement, not an end-to-end native capture benchmark: TDH decoding, ETL I/O, and native stop/close are explicitly excluded, so it does not establish OS teardown latency.
| "captureDenials: failed to inspect learning-mode trace sources: {error}" | ||
| )) | ||
| })? | ||
| .includes_network(), |
There was a problem hiding this comment.
Medium (testability/test coverage) — Exercise the actual source-mode-to-PSEC decision.
Attribution: introduced_by_change — production now derives the new capability gate from capture_factory.trace_sources().includes_network(), but the test-only spec builder assumes capture_denials.is_some() means a network-aware trace. The builder-only test passes a literal Boolean; existing fake-factory tests never return AccessOnly through this production connection.
Inverting or bypassing includes_network() here would leave those tests green. Fix: Give the orchestration a deterministic entry point shared with production, using the injectable factory or resolved trace sources, and assert the generated PSEC has no internetClient for AccessOnly and does have it for AccessAndNetwork, retaining deny-default egress in both cases.
There was a problem hiding this comment.
Fixed in 8a693f0. spawn_base now calls build_capture_aware_security_environment_spec, which resolves trace sources through the injected CaptureSessionFactory and builds the PSEC. The source-mode regression calls that same production entry point with factories returning AccessOnly and AccessAndNetwork, asserting no internetClient for AccessOnly, internetClient for AccessAndNetwork, and ruleless deny-default egress in both. It also checks both source modes with capture disabled. The other PSEC tests now delegate through this entry point with a deterministic fake factory instead of deriving network-awareness from capture_denials presence.
| ); | ||
| let document = DenialsDocument::new(analysis.denials, summary); | ||
|
|
||
| write_paired_output_files( |
There was a problem hiding this comment.
Low (maintainability) — Share the paired-output orchestration between native and legacy writers.
Attribution: introduced_by_change — write_capture_denials_document repeats sibling-path derivation, summary/document construction, the transactional paired-file invocation, and result-pointer construction from write_denials_document above; only the verbose-document writer differs.
The two paths could drift on collision policy or returned metadata when maintained separately. Fix: Extract a shared helper accepting the verbose writer closure while retaining separate v4/v5 document types and the current failure behavior.
There was a problem hiding this comment.
Fixed in 8a693f0. Both writers now delegate to write_denials_document_pair, sharing sibling derivation, actionable document/summary construction, transactional CreateNew writes, and result metadata. The verbose writer remains a closure over its separate legacy/v5 document type. Native metadata assertions and a native verbose-sibling collision regression cover the shared behavior; the existing paired-write failure and legacy collision tests still pass.
| source_verbose_logging.display() | ||
| ) | ||
| })?; | ||
| mxc_sdk::learning_mode_core::verbose_logging::validate_verbose_logging_document( |
There was a problem hiding this comment.
Low (performance) — The new version validator scans the verbose file twice.
Attribution: introduced_by_change — validate_verbose_logging_document first deserializes the version envelope from the full JSON byte slice, then deserializes the same bytes into the selected v3/v4/v5 document type. The prior audit path decoded its document once.
Output size is bounded and this is not a blocker. Fix: If large audit artifacts make the extra scan material, use a single-pass version-dispatching deserializer without weakening per-version structural validation.
There was a problem hiding this comment.
Measured alongside the high-cardinality benchmark in 8a693f0. For the 9,539,872-byte v5 artifact, ten-call averages were 46.40 ms for version-dispatching validation versus 37.63 ms for direct v5 deserialization (about 8.77 ms extra). The 100,000-event artifact gave 47.19 ms versus 37.95 ms. For the single-group artifact, the extra scan was about 10 microseconds.
Given the bounded output and small absolute overhead in this measurement, I left the exact per-version deserializers unchanged rather than introducing a new dispatching deserializer. The opt-in benchmark persists both measurements for future changes; the existing cross-version/unknown-field vocabulary rejection tests pass.
Preserve implicit deny egress for ingress-only proxy requests, exercise the production trace-source capability gate, and share paired denial output orchestration. Add a reproducible high-cardinality diagnostic and validation benchmark. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cca08650-4309-40fc-a5dc-ba7ad14384fa
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Re-verified #1419 at 8a693f0 against base c6f301d. The ingress-only runtime-proxy regression is fixed: omitted egress is treated as effective ruleless deny for proxy validation, with ProcessContainer/LXC/Seatbelt regression coverage. Production now calls the same trace-source-aware PSEC construction path exercised by the AccessOnly/AccessAndNetwork tests. Both native and legacy paired-output writers use the shared helper, with a native collision test.
The two remaining performance observations are non-blocking: the author measured bounded high-cardinality post-TDH processing and an approximately 8.8 ms extra version-validation pass on a 9.5 MB artifact. I did not independently rerun the opt-in release benchmark or verify live PSEC/ETL teardown, so this approval does not claim an end-to-end performance bound.
On this exact tree, cargo test -q -p mxc-sdk --lib -- network_parser base_container_runner capture_output capture_diagnostics etl_decode passed (170 tests, one ignored benchmark); cargo fmt --all -- --check and cargo clippy -q -p mxc-sdk --lib --tests -- -D warnings passed. The current PR checks were green at verification. My earlier requested change is resolved; no remaining filed finding is a merge condition.
Preserve the integration fixes merged in #1419 and retain only documentation changes relative to main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 16300264-31dd-4214-a056-f01395e3179e
📖 Description
Routes direct ProcessContainer denial-capture traffic through Tessera so WFP Learning Mode can observe the policy decision without weakening enforcement.
internetClientonly for direct directionalcaptureDenialsrequests.This is PR 3 of 4 in the WFP Learning Mode stack. Its base is the decoder-layer branch.
🔗 References
Related to #1286.
Depends on the preceding decoder PR in this stack.
🔍 Validation
cargo test -p mxc-sdk --lib 'capture_denials_'— 25 passed.cargo test -p mxc-sdk --lib 'network_policy_helpers::tests::'— 5 passed.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (not applicable)📋 Issue Type
🧱 Stack
Review and merge in this order.
Microsoft Reviewers: Open in CodeFlow