Repository navigation
NVX Phase 2: Policy Support and Image Selection - #1538
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
5474b31 to
4c5d844
Compare
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for finding 1: the new image-reference schema regex has quadratic rejection cost on unbounded invalid input, blocking synchronous JavaScript schema validation. There are also four Medium findings concerning CI guards, filesystem/network enforcement coverage, and the development policy reference. The coverage findings are missing regression protection, not demonstrated native containment failures.
All five findings were rechecked against base 1cc2dbd05e56cc18e882af87567047676299db4b and head 4c5d844ee6bee3d0991142e67a1a5fd1fc3c7f79. The local and GitHub diffs match apart from abbreviated index hashes. Four findings are inline; finding 5 is below because its fix belongs in an unchanged file.
Verification and limits
The exact emitted regex was exercised directly in Node v26.13.0: rejecting "a.".repeat(n / 2) took approximately 287 ms at 32 KB, 1,155 ms at 64 KB, and 4,679 ms at 128 KB. This is measured regex behavior, not an inference from syntax alone.
Checks that passed on the reviewed revision, from src/:
cargo test --offline --locked -p mxc-sdk --no-default-features --test nvx_e2e: 6 passed, covering feature-disabled/fail-closed behavior; no VM execution.cargo test --offline --locked -p mxc-sdk --no-default-features --lib microvm: 6 passed in the name-filtered shared-library selection.
Live Windows x64 NVX execution against the signed bundle was not run. Bundle-integrity tests had 10 passes and two access-denied junction failures in the review sandbox; the Ajv image-reference test did not execute because ajv was unavailable. The High finding affects JavaScript schema consumers, not the Rust image validator.
Findings outside the diff
5. Medium (documentation drift) - Document the newly required image in the development policy reference.
docs/containment-policy/dev/policy.md:138-146
Attribution: newly_exposed_by_change - this file is byte-identical between base and head, but this PR adds the mandatory microvm.image requirement in docs/schema.md:11-15 and the development contract. The linked self-contained development policy reference lists microvm as a backend but has no microvm row in its supported-fields table. A caller following that reference can omit the section and receive microvm.image is required without the reference explaining the new requirement.
Fix: Add a microvm row describing the required registry-qualified image and the optional provision.memoryMib field, including its current backend rejection, and link to the NVX support matrix. This documentation gap is non-blocking by itself.
Verified pre-existing - not attributed to this PR
No unrelated pre-existing defects are included as findings or merge conditions. The unchanged development policy reference was byte-compared and is retained only for the newly introduced requirement described above.
Combine Phase 2 policy and image selection with the existing versioned Rust JSON SDK integration. Address image schema backtracking and strengthen nightly filesystem and network enforcement coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add matching TCP and any-protocol controls plus a UDP-only rule that must block the same reachable TCP services. Verify the scenario fails when protocol mapping is deliberately ignored. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reuse runtime capabilities and native request validation without probing or provisioning a VM. Reject invalid requests and missing bundles while preserving credential redaction and cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4c5d844 to
be42796
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed PR #1538 at 619cbb66aefd31ca80c79181b2a22ac616166ebb. The five findings below are Medium: two enforcement-test gaps, two error-classification defects, and one IR layering violation. Please address them before approval. The test gaps do not establish native enforcement failures.
Attribution was rechecked against merge-base 0bf74a4258ab46e26f9221dcaa2a0ddb8e0b4f48 with current base b10d2dd4e9d03d93d042ac63dd6248284dbfc1cc. The branch is behind main. The local PR-range diff matches gh pr diff after normalizing line endings and abbreviated blob IDs; all five inline anchors are added lines.
Verified clean / validation evidence
The five findings from review 5700717565 were verified addressed in the earlier follow-up; they are not repeated here. In the full rerun, node --test tests\microvm-image-reference.test.js passed both tests from scripts\versioning, and node scripts\ci\resolve-validation-test-matrix.mjs --plan nightly passed. The exact-head Windows x64 Build job 124216283138 and Contracts job 124216283176 passed. The overall Build run 41092720841 failed in Linux SDK integration on an LXC image configuration error; that failure was not attributed to this PR.
Local Rust execution remains blocked by sandbox access to the Rust toolchain settings. No live NVX VM execution was verified locally. Error-path findings are source-traced, not signed-runtime reproductions.
Findings outside the diff
None. Each retained finding is attached to an added line. Finding 2 explicitly distinguishes the unchanged normal-run fallback from the new native provision-validation path that exposes it.
Verified pre-existing - not attributed to this PR
The normal-run fallback that recognizes only PolicyValidation predates this PR. It is not charged as newly introduced code; finding 2 concerns its newly exposed handling of native provision validation. The rerun also excluded the pre-existing PTY authorization concern and floating-image acquisition behavior.
The IR layering item corrects the rerun report's earlier omission: absence of a demonstrated runtime failure does not resolve the explicit separation of normalization DTOs and runtime models.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The enforcement and staging changes require design approval, maintainer confirmation of containment safeguards, and Windows x64/WHP validation with the pinned signed NVX agent, none of which is established by the supplied evidence.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
| const MAX_CWD_BYTES: usize = 4095; | ||
| const PROXY_ENVIRONMENT_KEYS: [&str; 4] = ["HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", "NO_PROXY"]; | ||
|
|
||
| pub(super) fn provision_request( |
There was a problem hiding this comment.
..or rather in this?
fn deserialize_v0_9_request(
json: &str,
phase: Option<crate::mxc_contract::published::v0_9_0_alpha::Phase>,
) -> Result<crate::mxc_contract::published::v0_9_0_alpha::Request, ParseError> {
use crate::mxc_contract::published::v0_9_0_alpha::{
self as contract, Containment, Phase, ProvisionRequest, Request,
};
match phase {
None => {
let request: contract::OneShotRequest =
deserialize_development_root(json, "one-shot", "0.9", false)?;
contract::validate_one_shot_request(&request)
.map_err(|error| ParseError::OneShot(WxcError::ConfigParse(error.to_string())))?;
Ok(Request::OneShot(Box::new(request)))
}
Some(Phase::Provision) => {
let request = match contract::probe_containment(json).map_err(v0_9_containment_error)? {
Containment::IsolationSession => {
deserialize_development_root(json, "IsolationSession provision", "0.9", true)
.map(ProvisionRequest::IsolationSession)
}
Containment::Wslc => {
deserialize_development_root(json, "WSLC provision", "0.9", true)
.map(ProvisionRequest::Wslc)
}
}?;
Ok(Request::Provision(request))
}
Some(Phase::Start) => {
deserialize_development_root(json, "start", "0.9", true).map(Request::Start)
}
Some(Phase::Exec) => {
deserialize_development_root(json, "exec", "0.9", true).map(Request::Exec)
}
Some(Phase::Stop) => {
deserialize_development_root(json, "stop", "0.9", true).map(Request::Stop)
}
Some(Phase::Deprovision) => {
deserialize_development_root(json, "deprovision", "0.9", true).map(Request::Deprovision)
}
}
}
There was a problem hiding this comment.
This ProvisionRequest is the signed NVX agent’s request, not the MXC lifecycle request. The backend builds it to translate MXC settings into the agent’s JSON format.
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
All five findings from review 5717107792 are addressed at 122b5aa, including the High/blocking IR layering issue. Approval follows focused source and regression-coverage verification. At verification time Windows CI was still running; local Rust tests remained sandbox-blocked, and live NVX VM scenarios were not executed locally.
|
NVX currently returns completed, captured output rather than a live process handle. That is why this path invokes it separately. Phase 3 (#1539) adds a live process handle and handles this concern. |


📖 Description
Add NVX image selection and supported filesystem, network, and proxy policy to the MicroVM backend, executor, development JSON contract, and functional validation.
🔗 References
✅ Checklist
feature-acceptedCargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type
Microsoft Reviewers: Open in CodeFlow