fix(network): normalize Windows binary paths (NVBug 6782969) - #3482
prekshivyas wants to merge 2 commits into
Conversation
Match Windows executable identities using a stable case-insensitive, separator-normalized representation across policy data, L4 input, and L7 relay evaluation. Preserve exact matching on other platforms and keep the original path for hashing and filesystem access. NVBug 6782969 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Verification update for NVBug 6782969 (Windows ARM64, real signed MXC):
The live harness removed L7 protocol fields solely to isolate CONNECT/L4 binary identity and bypassed the known test-only proxy preflight issue; those harness edits are not in this PR. Original executable paths remain in use for hashing/filesystem access, and non-Windows path matching remains exact. |
|
CI follow-up:
The PR remains mergeable and has no review requests outstanding. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
shailendra-nv
left a comment
shailendra-nv
left a comment
There was a problem hiding this comment.
Thanks for addressing the Windows path-spelling mismatch reported by NVBug. I am requesting changes because blanket ASCII lowercasing crosses the binary-identity authorization boundary on Windows configurations that opt into case-sensitive lookup. The inline comments also call out the missing L7/native-Windows validation and the public documentation contract.
|
|
||
| #[cfg(any(target_os = "windows", test))] | ||
| fn windows_network_binary_match_path(path: &str) -> String { | ||
| path.replace('\\', "/").to_ascii_lowercase() |
There was a problem hiding this comment.
This comparison is unsafe for Windows directories with case sensitivity enabled. NTFS can treat Trusted.exe and trusted.exe as distinct files there, but to_ascii_lowercase() grants them the same network/provider permissions. The runtime identity ultimately comes from the MXC command[0], so a case-distinct executable can match a policy intended for another file. Please make equality reflect actual Windows file identity (or detect these paths and fail closed), add a negative test using a case-sensitive directory, and handle \\?\ verbatim paths explicitly rather than rewriting them. References: Windows case sensitivity and verbatim path behavior.
| .binary | ||
| .as_ref() | ||
| .map(|path| path.to_string_lossy().into_owned()) | ||
| .map(|path| crate::opa::network_binary_match_path(path)) |
There was a problem hiding this comment.
Please validate this L7 path directly. The new tests exercise OpaEngine::evaluate_network (the L4 decision) only; none sends an HTTP request through this changed relay with mixed casing/separators. The PR also lacks the test:windows label, so its native Windows x64 and ARM64 PR jobs are currently skipped. Add a mixed-case/separator L7 regression test and run both native Windows jobs before merge.
| /// Normalizing both policy data and runtime input prevents equivalent spellings from | ||
| /// being denied while leaving the original path intact for filesystem access and | ||
| /// executable hashing. Other platforms retain exact path matching. | ||
| pub(crate) fn network_binary_match_path(path: &Path) -> String { |
There was a problem hiding this comment.
This introduces an externally observable policy-matching contract, but the schema and security guidance remain unchanged. Please document the Windows normalization behavior—including ASCII-only case folding, separator handling, and the chosen behavior for case-sensitive/verbatim paths—in docs/reference/policy-schema.mdx and docs/security/best-practices.mdx. Users and policy-authoring agents rely on the published schema as the source of truth.
2 participants
Summary
Fixes NVBug 6782969 on Windows by giving configured network-policy binary paths and runtime executable identities one stable comparison representation before Rego evaluates them. Windows ASCII casing and
\//separators are normalized consistently across protobuf/YAML policy loading, L4 authorization, ancestor matching, and the generation-pinned L7 evaluator; the original executable path remains unchanged for filesystem access and SHA-256 identity checks.Base:
NVIDIA/OpenShell@fb2980e077288b61ef03a2e6187e162d158526aa(windows).Related Issue
Root cause
MXC passes its raw
command[0]path to the host proxy. Rego comparesb.path == exec.pathliterally, so Windows-equivalent spellings such asC:/WINDOWS/SYSTEM32/CMD.EXEandC:\WINDOWS\System32\cmd.exedid not match. The exact byte-for-byte control matched, proving that endpoint authorization worked and isolating the defect to binary-path representation.Changes
/separators.Before / after reproduction
Environment: Windows ARM64 (Yukon), Rust 1.95.0
aarch64-pc-windows-msvc, signed ARM64wxc-exec.exeSHA-256DDE1C592270E9A659B01DCCAD70362DA7B99FEC114885FA4D625507AA775A503.The live repro used the real
processcontainerbackend withegress_proxy = true, allowedexample.com:443, and isolated L4 binary identity by omitting L7 protocol fields. The policy usedC:/WINDOWS/SYSTEM32/CMD.EXE;command[0]used the equivalent native Windows spelling. A known test-only proxy preflight issue was bypassed; none of those harness edits are committed.Command:
fb2980e077..., unmodified path-matching code): FAIL, exit 101; workload exit 7;CONNECT tunnel failed, response 403; scenario 0.50 s (command 16.58 s).d7926c8bf..., identical scenario): PASS, exit 0;AgentCompleted; scenario 0.85 s (command 17.07 s).from_proto_matches_windows_equivalent_binary_pathfailed because the equivalent binary was denied.Testing
cargo fmt --all -- --checkgit diff --checkcargo test -p openshell-supervisor-network— 1,233 passed, 0 failed, 2 ignored; integration tests 2 passed, 5 environment-dependent tests ignoredcargo test -p openshell-driver-mxc --lib— 93 passedcargo clippy -p openshell-supervisor-network --all-targets— passes; one unchanged dependency warning inopenshell-extension-coremise run pre-commit— blocked on this ARM64 host by unrelated tip/toolchain issues: Biome exits 5 without diagnostics; Python 3.14 ARM64 cannot buildgrpcio-toolsbecause MSVC receives incompatible/std:c++17and/std:c11; workspace Rust lint separately reaches unchangedopenshell-driver-mxc/src/driver.rs:819and fails-D warningsonunused_self.Security impact and scope
This remains fail-closed: it broadens equality only for ASCII case and separator differences that refer to the same ordinary Windows path. A genuinely different executable is still denied, command-line paths remain excluded from grants, hashes still use the original file, and non-Windows semantics are unchanged. The static per-sandbox identity model, per-process attribution, and unrelated policy-enforcement behavior are out of scope. Non-ASCII Windows case equivalence is not broadened by this change.
Checklist