fix(supervisor-network): L7 endpoint validation edge cases by andrewwhitecdw · Pull Request #2464 · NVIDIA/OpenShell

andrewwhitecdw · 2026-07-24T11:32:09Z

Summary

Two small robustness fixes in L7 network policy handling:

validate_l7_policies assumed every entry in network_policies.<name>.endpoints was a JSON object, and expand_access_presets called ep.as_object_mut().unwrap(). A malformed non-object endpoint caused a panic. This change adds an object-shape validation error and replaces the unwraps with safe pattern matching.
The scalar port field was already validated to be > 0, but the ports array accepted 0 values silently. The same inconsistency existed in the agent-proposal endpoint parser. Both paths now filter out zero ports.

Related Issue

N/A — small fixes found during code review.

Changes

l7/mod.rs: validate endpoint is an object; replace as_object_mut().unwrap() with safe matching; filter ports array to > 0
policy_local.rs: filter zero entries from endpoint.ports

Testing

mise run pre-commit passes (mise unavailable in this environment; ran equivalent cargo fmt + cargo clippy -p openshell-supervisor-network --all-targets — clean)
Unit tests pass (cargo test -p openshell-supervisor-network --lib — 969 passed)
E2E tests added/updated (if applicable)

Checklist

Follows Conventional Commits
Commits are signed off (DCO)
Architecture docs updated (if applicable)

…orts validate_l7_policies assumed every endpoint entry was a JSON object and used ep.as_object_mut().unwrap() in expand_access_presets. A non-object endpoint would panic. Add an object-shape validation error and replace the unwraps with safe pattern matching. Also filter zero values out of the ports array to match the scalar port validation (which already rejects port == 0). Signed-off-by: Andrew White <andrewh@cdw.com>

…arser network_endpoint_from_json accepted port 0 from the ports array without filtering, creating an endpoint with no usable ports. Drop zero entries so the array behaves consistently with the scalar port field. Signed-off-by: Andrew White <andrewh@cdw.com>

copy-pr-bot · 2026-07-24T11:32:15Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

andrewwhitecdw · 2026-07-24T11:32:18Z

I have read the DCO document and I hereby sign the DCO.

johntmyers

gator-agent

PR Review Status

Validation: This is project-valid small, concentrated work in supervisor network-policy handling: it replaces a malformed-input panic with fail-closed validation and attempts to make zero-port handling consistent. No duplicate work was found.
Head SHA: 583f62ef7019a4c54b23fe91e6ef289c2ca02d4a

Review findings:

One correctness issue is anchored inline: the raw-policy zero-port filter does not normalize the policy data later supplied to OPA, so mixed arrays still retain zero.
Focused regression coverage is missing for all three changed contracts. Please add unit tests showing that a non-object endpoint reports the expected validation error and preset expansion remains panic-free; raw-policy [0] and [0, 443] follow the intended contract; and agent proposals reject an all-zero array while retaining only positive entries from mixed arrays, including scalar fallback.

Docs: Fern docs are not needed for this malformed-input robustness fix; the existing policy reference already describes endpoint objects and concrete MCP ports.

E2E: The changed network-policy path requires test:e2e after review findings are resolved.

Next state: gator:in-review pending an author update.

johntmyers · 2026-07-24T18:20:43Z

+                |arr| {
+                    arr.iter()
+                        .filter_map(serde_json::Value::as_u64)
+                        .filter(|p| *p > 0)


gator-agent

Warning: This filters only a temporary vector whose sole consumer already checks any(|port| *port > 0). Consequently, [0] was already rejected, while [0, 443] still passes and the zero remains in the JSON supplied to OPA. Please either reject any zero-valued ports member or remove zeros during endpoint normalization/proto serialization, then cover all-zero and mixed arrays with regression tests.

I will get to these later today thanks for raising this issue.

johntmyers · 2026-07-28T14:50:30Z

@andrewwhitecdw Several of your PRs are awaiting feedback, please let us know if you plan on looking so we can plan accordingly

…ion tests Addresses gator-agent review feedback on NVIDIA#2464: - Filter zero values from endpoint ports arrays in normalize_endpoint_ports so OPA never sees a zero port. - Promote positive scalar port to ports array; leave all-zero arrays empty. - Skip non-object endpoints during normalization instead of panicking. - Add regression tests for non-object endpoint validation, zero-port filtering, and scalar fallback. Signed-off-by: Andrew White <andrewh@cdw.com>

johntmyers

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work in supervisor network-policy handling.
Head SHA: 200ecd2f0444b5ef60d33c2332990c546e521dc5

Thanks @andrewwhitecdw. I checked the update you pushed after the earlier zero-port normalization and test feedback. The raw-YAML mixed-array case is now normalized and the added helper tests cover that path, but the production proto load/reload path still bypasses the new normalizer.

Review findings:

One critical correctness/security-boundary issue is anchored inline: zero ports still reach OPA through OpaEngine::from_proto.
One warning is anchored inline: the separate agent-proposal parser behavior still needs focused regression coverage.

Docs: Fern docs are not needed because this hardens malformed input without changing supported policy syntax or commands.

E2E: The network-policy path still requires test:e2e after review findings are resolved.

Next state: gator:in-review pending an author update.

johntmyers · 2026-07-28T20:10:13Z

+            // If "ports" already exists, filter out zero values so OPA never
+            // sees a zero port. An all-zero array becomes empty and falls back
+            // to scalar "port" promotion below.
+            if let Some(ports) = ep_obj.get_mut("ports").and_then(|v| v.as_array_mut()) {


gator-agent

Critical (CWE-20): This normalizer runs only from preprocess_yaml_data; production OpaEngine::from_proto loads proto_to_opa_data_json without calling it. validate_l7_policies filters only a temporary vector, so a CLI policy with ports: [0, 443] still reaches OPA containing zero during normal sandbox load/reload. Please filter in proto_to_opa_data_json or apply shared normalization to proto-generated JSON, then add an OpaEngine::from_proto regression test.

johntmyers · 2026-07-28T20:10:14Z

    }

    let mut ports = endpoint.ports;
+    ports.retain(|p| *p > 0);


gator-agent

Warning (CWE-20): This changes the untrusted agent-proposal parser without testing that path. Please add proposal_chunks_from_body cases for mixed [0, 443], zero-only [0] rejection, and [0] with positive scalar fallback, asserting the resulting proto port and ports. The new OPA helper tests cannot protect this separate parser.

andrewwhitecdw · 2026-07-28T20:10:45Z

Thanks for the nudge — I’ve gone through all five open PRs:

fix(core): bracket IPv6 bind addresses in forward arg and access URL #2449: reused bracket_ipv6() in format_gateway_url() and added an IPv6 example to the Port Forwarding docs.
fix: assorted numeric overflow fixes #2462: tightened Docker/Podman numeric boundary checks so inputs rounding to 2^63/2^64 are rejected instead of saturating, and replaced saturating_mul in the Podman health-check interval.
fix: assorted small validation and robustness fixes #2450: made short_hash() slice on character boundaries, switched the policy revision table to use it, and added tests for the ephemeral-port boundary.
fix(supervisor-network): L7 endpoint validation edge cases #2464 (this PR): normalized zero ports before OPA and added regression tests for non-object endpoints and mixed/all-zero port arrays.
fix(cli): avoid panic on multi-byte UTF-8 in --since duration #2446: no actionable gator findings; I replied to elezar’s question about using an external duration crate.

All relevant test suites pass. Please re-review when you have a chance.

…arser tests proto_to_opa_data_json cloned e.ports unfiltered, so a policy with ports: [0, 443] still reached OPA with the zero even though the YAML preprocess path normalized it. Filter zero ports in the production path the same way (mixed arrays drop zeros, zero-only arrays fall back to a positive scalar port, zero scalar port is not promoted). Add tests: - proto_to_opa_data_json_filters_zero_ports_in_production_path - proposal_chunks_from_body_filters_mixed_zero_ports - proposal_chunks_from_body_rejects_zero_only_ports - proposal_chunks_from_body_zero_ports_falls_back_to_scalar_port cargo test -p openshell-supervisor-network --lib: 977 passed. Signed-off-by: Andrew White <andrewh@cdw.com>

andrewwhitecdw · 2026-07-30T00:46:58Z

Both review items addressed:

Critical (opa.rs): proto_to_opa_data_json now filters zero ports in the production path, same as the YAML preprocess path — ports: [0, 443] → [443], zero-only arrays fall back to a positive scalar port, and a zero scalar port is not promoted. Zero ports can no longer reach OPA from OpaEngine::from_proto.
Warning (policy_local.rs): added proposal_chunks_from_body tests for mixed [0, 443], zero-only [0] rejection, and [0] with positive scalar fallback, asserting the resulting port/ports.

Verified: cargo test -p openshell-supervisor-network --lib → 977 passed.

johntmyers

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work in supervisor network-policy handling.
Head SHA: ed4a4b63649a5857e2405a8f72bbb62f99a93282

Thanks @andrewwhitecdw. I checked your update about filtering zero ports in the production proto-to-OPA path and adding focused proposal-parser coverage. Those prior gaps are addressed, but the independent review found two remaining normalization cases that can reinterpret malformed or zero-only authorization input as a scalar-port allow; both are anchored inline. A focused preset-expansion regression gap is also anchored inline.

Review findings:

Two blocking CWE-20 warnings are anchored inline: malformed JSON array elements can be discarded before scalar fallback, and zero-only proto arrays can activate a shadowed scalar despite documented ports precedence.
One focused test gap is anchored inline for the removed expand_access_presets unwraps.

Docs: Fern docs are not needed for the intended malformed-input robustness fix. If scalar fallback for an explicitly present zero-only ports array is intentional, the schema and proto precedence contract would need updating.

E2E: The network-policy path still requires test:e2e after review findings are resolved.

Next state: gator:in-review pending an author update.

johntmyers · 2026-07-30T01:08:50Z

+            // sees a zero port. An all-zero array becomes empty and falls back
+            // to scalar "port" promotion below.
+            if let Some(ports) = ep_obj.get_mut("ports").and_then(|v| v.as_array_mut()) {
+                ports.retain(|p| p.as_u64().is_some_and(|n| n > 0));


gator-agent

Warning (CWE-20): retain removes every value that is not a positive JSON u64, not only zero. In local-file mode, ports: ["443"] plus port: 8443 therefore becomes ports: [8443], silently converting malformed authorization input into an allow. Please reject non-integer entries before normalization, or remove exactly numeric zero (p.as_u64() != Some(0)) so malformed arrays remain fail-closed. Add a regression case.

johntmyers · 2026-07-30T01:08:50Z

+                    // never sees them, then ports takes precedence over a
+                    // single promoted port. Rego always sees "ports".
+                    let filtered: Vec<u32> = e.ports.iter().copied().filter(|&p| p > 0).collect();
+                    let ports: Vec<u32> = if !filtered.is_empty() {


gator-agent

Warning (CWE-20): This changes the documented proto precedence: {port: 8080, ports: [0]} now permits 8080, although a present ports array previously took precedence and matched nothing. Malformed or zero-only authorization data should not activate a shadowed scalar port. Branch on the original e.ports.is_empty() state and filter inside the ports branch, or reject the proto explicitly; then update the scalar-fallback test.

johntmyers · 2026-07-30T01:08:50Z

    use super::*;

+    #[test]
+    fn validate_l7_policies_rejects_non_object_endpoint() {


gator-agent

Suggestion: This test verifies validation, but not expand_access_presets, where the unwraps were removed. Please add a direct expansion test containing a non-object endpoint and a valid preset endpoint, asserting the invalid entry remains unchanged and the valid preset expands. It would also be stronger to assert indexed errors for both malformed entries.

andrewwhitecdw added 2 commits July 24, 2026 06:31

andrewwhitecdw requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 24, 2026 11:32

johntmyers reviewed Jul 24, 2026

View reviewed changes

johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 24, 2026

johntmyers reviewed Jul 28, 2026

View reviewed changes

johntmyers reviewed Jul 30, 2026

View reviewed changes

Uh oh!

Conversation

andrewwhitecdw commented Jul 24, 2026

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 24, 2026

Uh oh!

andrewwhitecdw commented Jul 24, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

andrewwhitecdw Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers commented Jul 28, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

andrewwhitecdw commented Jul 28, 2026

Uh oh!

andrewwhitecdw commented Jul 30, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants