Enforce ProcessContainer ingress policy - #1080
Open
Branden Bonaby (bbonaby) wants to merge 1 commit into
Open
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
This was referenced Sep 1, 2026
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 1, 2026 23:58
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Native Windows policy enforcement and downlevel fallback behavior require final host-level verification.
Review tier: Balanced
Findings: None
What changed in this PR
Adds native ProcessContainer ingress-policy enforcement through the PSEC 1.1 contract while retaining PSEC 1.0 and legacy fallback behavior.
Changes:
- Exposes and negotiates PSEC ingress capabilities.
- Encodes native ingress policy and updates capability lowering.
- Integrates selection, validation, Learning Mode, and regression tests.
| File | Description |
|---|---|
src/backends/learning_mode/windows/src/secenv.rs |
Adds cached PSEC capability reporting. |
src/backends/learning_mode/windows/src/lib.rs |
Exports the support type. |
src/backends/appcontainer/common/src/test_env.rs |
Centralizes a test capability override. |
src/backends/appcontainer/common/src/base_container_runner.rs |
Integrates ingress-aware selection and validation. |
src/backends/appcontainer/common/src/base_container_helpers.rs |
Builds versioned PSEC ingress policies and compatibility fallbacks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 00:04
View session
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 00:09
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Native ingress enforcement needs backend-level tests that verify the security boundary on Windows.
Review tier: Balanced
Findings: None
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/backends/appcontainer/common/src/base_container_helpers.rs:350
- The new tests only inspect the serialized FlatBuffer, so they cannot catch an incorrect support flag/table mapping or verify that Windows actually blocks and permits inbound connections. Existing ProcessContainer network coverage only exercises egress defaults (
tests/scripts/run_base_container_network_tests.ps1:33-45) and raw-socket denial (tests/scripts/WinProcessContainer-Tests.ps1:891-926). Please add Windows backend coverage with real listener/client checks for both ingress defaults, host-loopback allow/deny, and the downlevel fallback before relying on this as a security boundary.
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 00:14
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Native ingress enforcement needs a support-gated runtime regression test beyond FlatBuffer serialization coverage.
Review tier: Balanced
Findings: None
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/backends/appcontainer/common/src/base_container_helpers.rs:350
- Add a support-gated runtime regression for native ingress. The new tests only inspect the FlatBuffer; the existing BaseContainer network script exercises egress only, so an OS-contract mismatch could leave inbound traffic unenforced while all current tests pass. Cover both
ingress.defaultandhostLoopbackallow/deny behavior with an external listener.
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 00:19
View session
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 00:23
View session
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 00:26
View session
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 03:56
View session
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 03:59
View session
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 07:22
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Probe failures are misreported as unsupported policy, and native ingress enforcement lacks adequate regression coverage.
Review tier: Balanced
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
 |
src/backends/appcontainer/common/src/base_container_runner.rs — is_ok_and collapses a failing QueryProcessSecurityEnvironmentSupport call into ordinary policy… View comment |
 |
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — Add an asymmetric serialization test (for example, default: deny with host_loopback: allow).… View resolved comment |
|
src/backends/appcontainer/common/src/base_container_runner.rs — The module documentation still says this runner prefers PSEC 1.0, but the new implementation emits… View resolved comment |
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/backends/appcontainer/common/src/base_container_runner.rs:7
- The module documentation still pins the preferred contract to PSEC 1.0, but this implementation emits PSEC 1.1 when ingress support is negotiated. Remove the fixed version so the documented behavior matches runtime selection.
src/backends/appcontainer/common/src/base_container_helpers.rs:389
- This security-boundary change is only covered by support-selection and FlatBuffer tests. The existing
tests/scripts/run_base_container_network_tests.ps1checks legacy egress behavior, not whether Windows applies PSEC ingress; add a ProcessContainer E2E that skips when the support bit is absent and verifies allowed/denied inbound and host-loopback connectivity.
if support.ingress_policy {
network.ingress = policy.network_ingress.as_ref().map(|ingress_policy| {
let mut ingress = PsecIngressPolicy::default();
ingress.default_action = psec_filter_action(ingress_policy.default);
ingress.host_loopback = psec_filter_action(ingress_policy.host_loopback);
Box::new(ingress)
});
src/backends/appcontainer/common/src/base_container_helpers.rs:559
- Both ingress inputs are
Allow, so this test still passes if the two assignments are swapped or one source field is reused for both. Make the case asymmetric and assert distinct decoded values.
request.policy.network_ingress = Some(NetworkIngressPolicy {
default: NetworkAction::Deny,
host_loopback: NetworkAction::Allow,
src/backends/appcontainer/common/src/base_container_runner.rs:413
- A failed support query is collapsed to ordinary incompatibility here. For
hostLoopback: allow, dispatch then selects AppContainer and reports an unsupported policy even though the host capability is unknown because probing failed; propagate this error through selection asBackendUnavailable, reserving rejection for a successful query with the ingress bit clear.
.is_ok_and(|decision| decision.use_process_security_environment)
}
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 07:27
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A support-query API failure is incorrectly classified as a policy rejection.
Review tier: Balanced
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_runner.rs — is_ok_and collapses a failing QueryProcessSecurityEnvironmentSupport call into ordinary policy… View comment |
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
Suppressed comments (1)
src/backends/appcontainer/common/src/base_container_runner.rs:421
- A failure from
QueryProcessSecurityEnvironmentSupportis an API/host failure, not evidence that the request is invalid. Marking itRejectedcauses telemetry to report a policy error, while a successful query with the ingress bit clear is already handled separately as a rejection. Classify this variant asBackendUnavailableas well.
BaseContainerError::ProcessSecurityEnvironmentSupport(_) => FailurePhase::Rejected,
BaseContainerError::ProcessSecurityEnvironmentApi(_)
| BaseContainerError::LearningModeApi(_) => FailurePhase::BackendUnavailable,
Branden Bonaby (bbonaby)
force-pushed
the
user/bbonaby/process-container-ingress-runtime
branch
from
September 2, 2026 16:04
3c2a9b8 to
da69f0f
Compare
Branden Bonaby (bbonaby)
changed the title
Branden Bonaby (bbonaby)
force-pushed
the
user/bbonaby/process-container-ingress-runtime
branch
from
September 2, 2026 16:05
da69f0f to
2e52d5e
Compare
Branden Bonaby (bbonaby)
marked this pull request as ready for review
September 2, 2026 16:22
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 2, 2026 16:22
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
An OS capability-query failure is incorrectly exposed as a non-retryable policy validation error.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_runner.rs — is_ok_and collapses a failing QueryProcessSecurityEnvironmentSupport call into ordinary policy… View resolved comment |
Suppressed comments (1)
src/backends/appcontainer/common/src/base_container_runner.rs:421
ProcessSecurityEnvironmentSupportrepresents a failed OS capability query, not an unsupported policy, yet this maps it toRejected. The engine converts that phase topolicy_validationand treats it as non-retryable (mxc_engine/src/dispatch.rs:96-100), so a host/API failure is reported as a caller policy error. Keep query failures asBackendUnavailable; a successful query with the ingress bit clear is already handled separately as the actual policy rejection.
BaseContainerError::ProcessSecurityEnvironmentSupport(_) => FailurePhase::Rejected,
BaseContainerError::ProcessSecurityEnvironmentApi(_)
| BaseContainerError::LearningModeApi(_) => FailurePhase::BackendUnavailable,
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 3, 2026 22:11
View session
Branden Bonaby (bbonaby)
force-pushed
the
user/bbonaby/process-container-ingress-runtime
branch
from
September 3, 2026 22:11
8b91d40 to
58cffac
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Core removal of the bidirectional private-network capability lacks an effective regression test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new removal of privateNetworkClientServer is not exercised in the case where that capability… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
Copilot started reviewing on behalf of
Branden Bonaby (bbonaby)
September 3, 2026 23:35
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Missing ingress support currently falls through to AppContainer and produces the wrong error classification.
Review tier: Balanced
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new removal of privateNetworkClientServer is not exercised in the case where that capability… View comment |
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/backends/appcontainer/common/src/base_container_runner.rs:240
- The ProcessContainer OS-support guide documents the PSEC runtime probe and the existing
PSE_SUPPORT_FS_DENYgate (docs/process-container/os-version-support.md:53-114), but it does not document this new ingress capability gate or its downlevel behavior. Update that guide to explain when PSEC 1.1 is used, when PSEC 1.0 capability lowering remains valid, and whenhostLoopback: allowis rejected.
src/backends/appcontainer/common/src/base_container_runner.rs:908
- When the support query succeeds but the ingress bit is clear,
use_process_security_environmentis false andhostLoopback: allowis also SBOX-incompatible, so this probe returns false. The dispatcher then selects an AppContainer fallback instead of reaching the newFailurePhase::Rejectedbranch inBaseContainerRunner::validate; streaming callers receivebackend_error(and may even hit an unrelated fallback/DACL error) rather than the promised unsupported-policy result. Carry this resolved unsupported state through dispatch as a policy rejection, and cover the full dispatcher path rather than only validating a directly constructed BaseContainer runner.
if decision.use_process_security_environment {
return Ok(true);
}
if !Self::is_legacy_sbox_compatible_with_request(
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The canonical ProcessContainer networking guide still documents the removed legacy host-loopback encoding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
docs/process-container/os-version-support.md — This new PSEC 1.1 behavior leaves the canonical ProcessContainer networking guide contradictory:… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new removal of privateNetworkClientServer is not exercised in the case where that capability… View resolved comment |
Comment on lines
+70
to
+71
| `PSE_SUPPORT_NETWORK_INGRESS`, MXC emits the PSEC 1.1 contract and serializes | ||
| `ingress.default` and `ingress.hostLoopback` directly. Otherwise, requests that |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Repository documentation still describes the removed PSEC 1.0 host-loopback lowering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 3
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The checked-in ProcessContainer documentation still describes the removed PSEC lowering:… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
docs/process-container/os-version-support.md — This new PSEC 1.1 behavior leaves the canonical ProcessContainer networking guide contradictory:… View comment |
|
src/backends/appcontainer/common/src/base_container_helpers.rs — The new enforcement path is covered only by support-selection and FlatBuffer serialization tests;… View comment |
| const SECURITY_ENVIRONMENT_API_SET_NAME: &str = "api-win-appmodel-processmodel~securityenvironment"; | ||
| const SECURITY_ENVIRONMENT_API_SET: &core::ffi::CStr = | ||
| c"api-win-appmodel-processmodel~securityenvironment"; | ||
| const PSE_SUPPORT_FS_DENY: u64 = 0x0000_0000_0000_0001; |
Collaborator
Author
There was a problem hiding this comment.
We'll need to revisit how we can refactor this whole file out of the learning mode module.
Member
Isn't this (or rather Why reimplement another custom thing? Refers to: src/backends/appcontainer/common/src/base_container_runner.rs:2092 in 84da496. [](commit_id = 84da496, deletion_comment = False) |
| return Ok(()); | ||
| } | ||
| let use_process_security_environment = self.uses_process_security_environment(request); | ||
| let process_security_environment_usable = self.is_psec_usable(); |
Negotiate the Process Security Environment ingress contract from the OS support query, emit PSEC 1.1 only when available, and preserve compatible PSEC 1.0 or SBOX behavior otherwise. Carry failed queries and confirmed unsupported host-loopback policy through dispatch with distinct public error classifications. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3413690e-c10e-4f33-b3c5-0a2715eb1b88
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
📖 Description
Adds OS capability detection for the Process Security Environment (PSEC) ingress contract and enforces ProcessContainer directional ingress when the host advertises support. PSEC 1.1 serializes
ingress.defaultandingress.hostLoopbackdirectly; older hosts retain the compatible PSEC 1.0 or legacy SBOX capability behavior.Capability-query failures remain retryable
backend_unavailableerrors. If the query succeeds but reports that network-ingress policy is unsupported, a request forhostLoopback: allowis rejected as unsupported policy.🔗 References
🔍 Validation
cargo fmt --manifest-path src\Cargo.toml --all -- --checkcargo test --manifest-path src\Cargo.toml -p learning_mode_windows secenv::testscargo test --manifest-path src\Cargo.toml -p appcontainer_common base_container_helpers::testscargo test --manifest-path src\Cargo.toml -p appcontainer_common base_container_runner::testscargo test --manifest-path src\Cargo.toml -p appcontainer_commoncargo test --manifest-path src\Cargo.toml -p mxc_enginecargo clippy --manifest-path src\Cargo.toml -p appcontainer_common --all-targets -- -D warnings✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type