Repository navigation
Fix ProcessContainer host-loopback compatibility on PSEC 1.0 - #1490
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa1ea5a5-f8e6-4a00-9b94-a8b0192e3466
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Direct PSEC 1.0 requests omit the capability needed for loopback connectivity, and the updated tests depend on host capabilities.
1 open finding
What changed in this PR
Updates ProcessContainer host-loopback compatibility for PSEC 1.0 hosts.
Changes:
- Selects PSEC 1.1 only when ingress support is advertised.
- Removes the identity-less proxy-only workaround.
- Updates PSEC version-selection and encoding tests.
| File | Description |
|---|---|
src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs |
Revises host-loopback serviceability and PSEC selection. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Verify direct and identity-less proxy loopback access, avoid shell-launch false positives, and document Windows build support. Make loopback unit tests independent of host ingress support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa1ea5a5-f8e6-4a00-9b94-a8b0192e3466
Name the direct loopback validation area explicitly while preserving its previous selector as an alias. Accept policy-blocked DNS in proxy egress checks and clarify Windows build-dependent loopback support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa1ea5a5-f8e6-4a00-9b94-a8b0192e3466
Add the loopback capability alongside the peer for direct requests when native ingress support is unavailable. Cover allow and deny encoding independently of host capabilities and document the compatibility mapping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa1ea5a5-f8e6-4a00-9b94-a8b0192e3466
Always grant networkLoopback when host loopback is allowed. Preserve ingress-query failures, align proxy discovery with compatible PSEC support, and strengthen version-selection and loopback endpoint regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bfb39885-bfc3-4c08-911e-c98dc5daa379
Keep proxy discovery and examples focused on caller requirements, and describe the validation expectations in plain language. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bfb39885-bfc3-4c08-911e-c98dc5daa379
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Reviewed the current PR changes against head ce040460c (merge-base e492072a3). The two comments below ask for coverage of newly supported network-policy combinations; neither asserts a demonstrated WFP bypass or a broken current host. The version selector now tests both ingress-support values, ingress-query errors are surfaced, and the proxy test checks a second host-reachable endpoint. No unrelated pre-existing findings are attributed to this PR.
| (true, false, false, true), | ||
| (true, true, true, true), | ||
| (true, true, false, false), | ||
| (true, true, false, true), |
There was a problem hiding this comment.
Medium (test coverage) — exercise the newly supported PSEC 1.1/no-ingress proxy host state.
Attribution: introduced_by_change — this new matrix case treats a host supporting both PSEC 1.0 and 1.1, but not advertising native network ingress, as identity-less-proxy-capable. The selector/discovery tests use synthetic support values; the reported live runs cover a PSEC 1.0 machine and a PSEC 1.1 machine with ingress support, not this combination.
Could you validate the precise combination on a host that reports PSEC 1.1 available and network ingress absent, with proxy support enabled? In particular, check that the workload reaches the configured proxy and cannot connect directly to the separate host-reachable loopback listener. If the combination cannot be staged, please note that validation gap. This is not a claim that proxy containment is currently bypassed.
There was a problem hiding this comment.
The live suite passed on a PSEC 1.1 machine with networkIngress disabled. It reached the configured proxy and blocked direct connections to the other IPv4 and IPv6 listeners.
| #[test] | ||
| fn psec_direct_host_loopback_encodes_capability_and_peer_regardless_of_ingress_support() { | ||
| let mut request = host_loopback_request(); | ||
| request.policy.network_egress = Some(crate::mxc_common::models::NetworkEgressPolicy { |
There was a problem hiding this comment.
Medium (test coverage) — exercise direct PSEC 1.0 loopback with a deny-default ingress policy.
Attribution: introduced_by_change — the new direct compatibility-path encoding test uses host_loopback_request() (ingress.default: allow) and sets egress.default: allow; the live direct-loopback check uses the same defaults. Neither establishes whether the newly accepted PSEC 1.0 request works with ingress.default: deny, hostLoopback: allow, egress.default: allow, and no proxy. That shape omits privateNetworkClientServer while retaining the explicit loopback grant.
Could you add an encoding assertion for the expected capability/peer combination and a PSEC 1.0 live check that the container reaches a medium-integrity host listener on BaseContainer, or reject/document the narrower shape if it cannot be supported? Please do not infer from this comment that egress.default: deny must also reach the listener; that is a separate policy question.
There was a problem hiding this comment.
Added this case to both the encoding test and live suite. With ingress denied by default, egress allowed, and host loopback allowed, the PSEC 1.0 run reached the host listener without a proxy.
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Requested changes — PR #1490 at ce040460c
The new direct-loopback path and the proxy-discovery update are coherent on the modeled hosts. I am requesting changes primarily for two PR-attributable issues: the new fallible ingress query can reject an otherwise viable non-loopback request, and the positive PSEC 1.0 live test does not control the host listener's integrity level on an elevated CI runner. See the inline comments for those and for the public discovery wording and newly relevant IPv6 test gap.
Coverage requests already posted
- Medium — narrower direct policy (
introduced_by_change): the new tests useingress.default: allowandegress.default: allow; they do not establish PSEC 1.0 connectivity withingress.default: deny,hostLoopback: allow, and egress allowed. This was posted in the existing inline thread; I am not filing it twice. - Medium — ingress-disabled PSEC 1.1 proxy host (
newly_exposed_by_change): the PR newly admits the PSEC 1.0 proxy mapping on a 1.1-capable host without native ingress, but synthetic support-matrix tests do not establish launch and endpoint confinement on that host state. This was posted in the existing inline thread. The new inline comment below adds the distinct missing native-creation/IPv6 test coverage.
Advisory, not a merge condition
- Low — partial-service diagnostics (
introduced_by_change):src/mxc-sdk/src/backends/process_container/common/base_container_runner.rs:508-565logs the selected PSEC version but not the PSEC 1.0 host-loopback limitation whenhostLoopback: allowis accepted. A request-aware probe warning or explicit runtime log would make failed in-container-listener/low-integrity connections easier to diagnose. This limitation is documented in the PR, so I do not treat the missing warning as a correctness defect.
Verified clean and pre-existing context
- The new
networkLoopbackcapability remains conditional on explicit host-loopback allow; deny/omitted encoding tests cover both. The proxy-mode negative test requires a real curl connection failure to a second host-reachable IPv4 endpoint. Version-selection unit tests now exercise both ingress support values. These checks do not replace live coverage of the untested host states. docs/schema.mdand the released stable schema's generic bidirectional identity-less-proxy descriptions are byte-identical to the base, and PSEC 1.0 identity-less proxies already existed before this PR. Those pre-existing statements are not attributed to this change or conditions of this review. The stale generic fallback error for other policy-incompatibility causes is likewise not charged to this PR.
Attribution was checked against the three-dot PR diff (merge-base e492072a3, recorded base ref 2366ee9b1) and head ce040460c. No new local test run or unsupported host-state validation is claimed.
|
|
||
| /// Whether BaseContainer can enforce | ||
| /// `network.ingress.hostLoopback = "allow"`. | ||
| /// Whether BaseContainer supports native bidirectional host-loopback ingress. |
There was a problem hiding this comment.
Medium (documentation drift) — carry this native-only qualification through public discovery docs.
Attribution: newly_exposed_by_change — direct hostLoopback: allow is now serviceable on PSEC 1.0 even though the IngressHostLoopbackAllow fact is false there. This revised runner comment correctly calls the fact native bidirectional support, but unchanged consumer-facing descriptions in src/mxc-sdk/src/core/mxc_engine/probe.rs:29-30, src/mxc-sdk/src/backends/process_container/common/probe.rs:74-76, and the .NET V1 PlatformDiscovery.cs enum still read as generic host-loopback allow enforcement at the tier. Clients using the flag as an all-or-nothing preflight can suppress this PR's newly supported direct request.
Fix: Define the fact consistently as full native ingress in the published Rust/.NET/Node references, and point callers to the request-aware probe for PSEC 1.0's narrower serviceable mode. I am not asking to change an immutable released schema.
There was a problem hiding this comment.
Updated the existing Rust, .NET, and Node descriptions
| -EgressDefault 'deny' -IngressDefault 'allow' -HostLoopback 'allow' ` | ||
| -NetworkProxy $proxyUrl -TimeoutMs 25000 | ||
| $loopback = Invoke-NetRun -Name 'net-proxy-direct-loopback-blocked' -ConfigPath $cfgLoopback | ||
| Record-Result -Phase 'P8e' -Name 'direct access to a second loopback endpoint is blocked' ` |
There was a problem hiding this comment.
Medium (test coverage) — also exercise native acceptance and IPv6 endpoint isolation without ingress.
Attribution: newly_exposed_by_change — the PR admits identity-less proxy requests on PSEC 1.1-capable hosts with ingress disabled, selecting a PSEC 1.0 payload. This new second-listener check covers an IPv4 endpoint; existing Rust OS-creation/launch and ::1 bypass tests in base_container_helpers/tests.rs skip when supports_ingress_host_loopback_allow() is false and construct only V1.1/ingress-enabled specs. The old PSEC 1.0-only proxy workaround predated this PR; the additional 1.1-without-ingress host state is what makes the gap newly relevant.
Fix: Run native create/launch and both IPv4 and IPv6 confinement checks with the V1.0 mapping on the newly supported host state, or add an equivalent ::1 negative assertion to the live proxy test. The separate missing-host-state validation is already in an earlier review thread.
There was a problem hiding this comment.
The native create and launch tests now use PSEC 1.0 and no longer require ingress support. The live suite also checks a separate IPv6 listener, and it passed with networkIngress disabled.
| -Pass (Test-SelectedTier $allow.Log) ` | ||
| -Detail "selectedTier=$(Get-SelectedTier $allow.Log); expected=base-container" | ||
| Record-Result -Phase 'P8d' -Name 'hostLoopback=allow without a runtime proxy reaches the host loopback anchor' ` | ||
| -Pass (($allow.Result.Stdout.Trim() -eq 'MXC-LOOPBACK-ANCHOR') -and ($allow.Result.ExitCode -eq 0) -and -not $allow.Result.TimedOut) ` |
There was a problem hiding this comment.
Medium (Windows build/CI parity) — use a controlled medium-integrity host listener.
Attribution: newly_exposed_by_change — this positive assertion now requires success on PSEC 1.0-only hosts, where the PR specifies support for medium-integrity host listeners only. Start-LoopbackListener binds the listener in the harness process/runspace (WinProcessContainer.Common.ps1:1020-1052); the CI guide says the 1ES agent runs elevated. On an elevated older-host runner the listener inherits the higher-integrity token, so this test can fail while the documented medium-integrity compatibility behavior works correctly. A host-side Invoke-WebRequest prerequisite proves reachability, not listener integrity.
Fix: Run the positive listener under a verified medium-integrity token (or lower the listener helper's token), then assert the container reaches it. Keep separate tests for other listener integrity levels with their own expected outcomes.
There was a problem hiding this comment.
Kept the listener in the test process. It is a normal host process, not another AppContainer that needs its own loopback access, so forcing it to medium integrity is not required.
| )? | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
This is wrong - we don't evaluate support here.
There was a problem hiding this comment.
Please elaborate. Do you mean 'mxc does not/should not evaluate what the underlying platform does or does not support/have enabled'? Or 'this check should be done elsewhere in mxc'?
There was a problem hiding this comment.
I no longer do the checks in this. We request loopback access and let Windows provide the behavior supported by that build.
Use the loopback behavior available on each Windows build, preserve proxy endpoint restrictions, and align discovery descriptions and regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bfb39885-bfc3-4c08-911e-c98dc5daa379
| if !BaseContainerRunner::supports_identityless_loopback_proxy() | ||
| .expect("query identity-less loopback proxy support") | ||
| { |
|
|
||
| // Temporary compatibility for explicit identity-less proxies on PSEC 1.0-only | ||
| // hosts. Remove once those hosts support PSEC 1.1; keep loopback allow opt-in. | ||
| fn psec_1_0_proxy_loopback_workaround( |
There was a problem hiding this comment.
note: with my change this proxy work around for machines with PSEC 1.0 that used unpackaged medium IL proxies is no longer needed as my change is general and will allow for this scenario, as it only needed us to pass in the "networkLoopback" capability.
updated per comments, I believe I've addressed what we discussed. Also need to unblock openAI.



📖 Description
hostLoopback: allowand the egress policy permit it, including when ingress is denied by default.🔗 References
Closes #1485.
🔍 Validation
networkIngressdisabled. The suites cover direct access with both ingress defaults and proxy endpoint isolation using separate IPv4 and IPv6 listeners.Commands run from
src/:cargo fmt --all -- --check- passed.cargo test -p mxc-sdk --lib -- psec_ loopback_and_proxy identityless_loopback_proxy runtime_proxy_uses_peer identity_scoped_proxy process_container::common::probe::tests:: mxc_engine::probe::tests::- passed: 50 tests.cargo clippy -p mxc-sdk --lib --tests -- -D warningswithCARGO_INCREMENTAL=0- passed.cargo build -p wxc -p plm -p wxc_winhttp_proxy_shim- passed.$env:RUSTDOCFLAGS='-D rustdoc::broken_intra_doc_links'; cargo doc -p mxc-sdk --no-deps --lib- failed on existing broken links in unchanged documentation; not rerun.From
sdk/node/,npm run typecheck- passed. Full .NET and Node test suites were not run; their changes are documentation-only.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 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 with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See pull request builds.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See pull request builds
for the steps.
Microsoft Reviewers: Open in CodeFlow