No request/spec size limits #24

Activity

1. pimlock

   added this to the march-mvp milestone on Feb 27, 2026
2. pimlock

   added

   topic:securitySecurity issuesSecurity issues

   on Feb 27, 2026
3. johntmyers commented on Mar 5, 2026

   

   johntmyers

   on Mar 5, 2026

   Collaborator

   More actions

   🔒 security-review-agent

   Security Review

   Determination: Legitimate concern

   Summary

   The gRPC endpoints for CreateSandbox, CreateInferenceRoute, and UpdateInferenceRoute accept unbounded string fields and nested message payloads without enforcing application-level size or length limits. While tonic imposes a default 4 MB max decoding message size and the server requires mTLS authentication (reducing the attack surface to authenticated clients only), there are no per-field length checks on spec fields such as providers, policy, routing_hint, base_url, api_key, model_id, environment maps, or template structures like pod_template / resources / volume_claim_templates.

   Severity Assessment

   • Impact: Low-to-medium. An authenticated attacker could send maximally-sized payloads (up to 4 MB per request) to exhaust server memory, database storage, or serialization overhead over time. The SandboxSpec includes arbitrarily nested google.protobuf.Struct fields (resources, pod_template, volume_claim_templates) and unbounded map<string,string> fields that could amplify storage and processing cost.
   • Exploitability: Low. The server enforces mTLS (tls.rs:24-25), meaning only clients with valid certificates signed by the cluster CA can connect. This limits the attack surface to trusted, authenticated callers. Additionally, tonic's built-in 4 MB message size limit (max_decoding_message_size defaults in the generated proto code at navigator.v1.rs:685) provides a baseline cap, though this is not explicitly configured and relies on the library default.
   • Affected components:
     • crates/navigator-server/src/grpc.rs:72-170 — create_sandbox accepts spec with unbounded providers list, policy blob, environment maps, and template structs
     • crates/navigator-server/src/grpc.rs:487-500 — create_provider has no field length validation
     • crates/navigator-server/src/inference.rs:60-101 — create_inference_route validates non-empty fields but no max lengths
     • crates/navigator-server/src/inference.rs:103-142 — update_inference_route same issue
     • crates/navigator-server/src/multiplex.rs:47-48 — NavigatorServer::new() and InferenceServer::new() are created without explicit max_decoding_message_size() configuration

   Attack Scenario

   1. An authenticated client (with valid mTLS cert) sends repeated CreateSandbox requests with maximally-padded spec fields — e.g., a 4 MB policy blob, thousands of provider names, or deeply nested pod_template Struct values
   2. Each request is persisted to the SQLite store via put_message without size checks, accumulating storage
   3. Over sustained requests, this could exhaust database storage, increase memory pressure during serialization/deserialization, and degrade server responsiveness for legitimate clients
   4. Similarly, CreateInferenceRoute could be used to store many routes with near-max-size api_key or base_url strings

   Remediation Plan

   1. Explicitly configure tonic message size limits — call .max_decoding_message_size(N) on NavigatorServer and InferenceServer in multiplex.rs:47-48 with a conservative limit (e.g., 1 MB) rather than relying on the implicit 4 MB default
   2. Add application-level field validation in create_sandbox and the inference route handlers:
      • Cap spec.providers list length (e.g., max 32 entries)
      • Cap individual string field lengths (name, routing_hint, base_url, api_key, model_id) to reasonable bounds (e.g., 1024 bytes)
      • Cap environment map size (entries and key/value lengths)
      • Validate or cap the serialized size of SandboxTemplate fields (resources, pod_template, volume_claim_templates)
      • Cap policy blob serialized size
   3. Add rate limiting at the connection or request level for mutating RPCs as a defense-in-depth measure

   Additional Notes

   • The mTLS requirement is a strong mitigating factor — this is not exploitable by unauthenticated external attackers. The risk is limited to compromised or malicious authenticated clients within the cluster trust boundary.
   • Tonic's default 4 MB limit already provides a baseline defense, but it is not documented or intentional in this codebase, so it should be made explicit.
   • The push_sandbox_logs streaming endpoint already has a per-batch cap of 100 lines (grpc.rs:1153), showing the pattern is established elsewhere in the codebase.
4. johntmyers

   added

   state:review-readyReady for human reviewReady for human review

   on Mar 5, 2026
5. johntmyers commented on Mar 5, 2026

   

   johntmyers

   on Mar 5, 2026

   Last edited by johntmyers

   Collaborator

   More actions

   🏗️ build-plan

   Implementation Plan

   Issue type: fix
   Complexity: Low
   Confidence: High

   Summary

   Add application-level field validation (max lengths, list sizes, map sizes) to create_sandbox and create_provider handlers, and explicitly configure tonic's max_decoding_message_size on both gRPC servers in the multiplex layer. This hardens the server against resource-exhaustion attacks from authenticated clients sending oversized payloads. Inference route validation is excluded — issue #133 is removing the entire inference route CRUD system.

   Scope

   • crates/navigator-server/src/multiplex.rs: Set explicit max_decoding_message_size(1MB) on both NavigatorServer and InferenceServer instead of relying on the implicit 4 MB tonic default.
   • crates/navigator-server/src/grpc.rs: Add a validate_sandbox_spec function that enforces max lengths on name, log_level, environment map (entry count + key/value lengths), providers list (max 32), and caps on serialized size of SandboxTemplate Struct fields. Call it in create_sandbox before persisting. Add field length checks to create_provider_record for name, type, credentials map, and config map.

   Implementation Steps

   1. Configure explicit tonic message size limits — In multiplex.rs, chain .max_decoding_message_size(1_048_576) (1 MB) on both NavigatorServer::new(...) and InferenceServer::new(...).
   2. Add validate_sandbox_spec in grpc.rs — Create a validation function that checks:
      • request.name length ≤ 253 chars (Kubernetes name limit)
      • spec.providers length ≤ 32
      • spec.log_level length ≤ 32
      • spec.environment map: ≤ 128 entries, keys ≤ 256 bytes, values ≤ 8192 bytes
      • spec.template string fields (image, runtime_class_name, agent_socket) ≤ 1024 bytes
      • spec.template map fields (labels, annotations, environment) ≤ 128 entries each
      • spec.template Struct fields (resources, pod_template, volume_claim_templates) serialized size ≤ 65536 bytes each
      • Policy serialized size ≤ 262144 bytes (256 KB)
   3. Wire validate_sandbox_spec into create_sandbox — Call the new validation function right after extracting the spec, before the provider-existence loop.
   4. Add field length checks to create_provider_record — Validate name ≤ 253, type ≤ 64, credentials map ≤ 32 entries with keys ≤ 256 and values ≤ 8192, config map ≤ 64 entries with same key/value limits.
   5. Add unit tests for all new validation — Test boundary conditions (at limit, over limit) for each validated field in grpc.rs::tests.
   6. Verify existing integration tests still pass — Run cargo test -p navigator-server to confirm no regressions.

   Test Plan

   • Unit tests: Add tests in crates/navigator-server/src/grpc.rs (in the existing mod tests block) for validate_sandbox_spec and the new provider field limits.
   • Integration tests: Existing tests in crates/navigator-server/tests/ should continue to pass unchanged (they use small payloads).
   • E2E tests: N/A — unit tests provide sufficient coverage for input validation.

   Risks & Open Questions

   • Limit values are best-guess — The specific limits are reasonable defaults but may need tuning based on real usage patterns. They should be documented as constants for easy adjustment.
   • Struct size validation — Checking serialized size of google.protobuf.Struct fields requires encoding them, which adds a small CPU cost per request. Acceptable since sandbox creation is not a hot path.
   • No rate limiting in scope — Rate limiting is recommended as defense-in-depth but excluded to keep scope focused. Can be a follow-up issue.
   • Backward compatibility — The 1 MB message limit is 4x smaller than the current implicit 4 MB default. Existing clients sending legitimately large payloads (unlikely) could be rejected.
   • Inference routes excluded — Issue Simplify inference routing: introduce inference.local and remove implicit catch-all #133 is rearchitecting the entire inference system (removing route CRUD, introducing inference.local). Adding field validation to APIs that are about to be deleted is wasted effort.

   Documentation Impact

   • None expected — internal server hardening. Limits documented as code-level constants.

   ---

   Revision 2 — removed inference route validation (excluded per #133 rearchitecture)
   Revision 1 — initial plan
6. drew

   assigned

   johntmyers

   on Mar 5, 2026
7. johntmyers

   added

   state:agent-readyApproved for agent implementationApproved for agent implementation

   state:in-progressWork is currently in progressWork is currently in progress

   on Mar 6, 2026
8. johntmyers

   added a commit that references this issue on Mar 6, 2026

   fix(server): add field-level size limits to sandbox and provider crea…

   ...

   This issue will close once commit 7062ee1 is merged into the 'main' branch.

   7062ee1
9. johntmyers commented on Mar 6, 2026

   

   johntmyers

   on Mar 6, 2026

   Collaborator

   More actions

   🏗️ build-from-issue-agent

   Implementation Complete

   PR: #145

   What was built

   Added application-level field validation to create_sandbox and create_provider gRPC handlers with 14 named constants, and configured explicit 1 MB tonic max_decoding_message_size on both gRPC servers. Inference routes excluded per #133 rearchitecture.

   Tests

   • Unit: 21 tests added covering boundary conditions for all validated fields

   Docs updated

   • None needed — internal server hardening

   The issue will auto-close when the PR is merged.
10. johntmyers

    added

    state:pr-openedPR has been opened for this issuePR has been opened for this issue

    and removed

    state:in-progressWork is currently in progressWork is currently in progress

    state:review-readyReady for human reviewReady for human review

    on Mar 6, 2026
11. johntmyers

    closed this as completedin #145on Mar 6, 2026
12. kirit93

    added a commit that references this issue on Mar 9, 2026

    fix(server): add field-level size limits to sandbox and provider crea…

    ...

    This issue will close once commit 39f8a8d is merged into the 'main' branch.

    39f8a8d
13. drew

    added 3 commits that reference this issue on Mar 16, 2026

    fix(sandbox): enforce network namespace and proxy policy in SSH sessi…

    ...

    This issue will close once commit aab1e34 is merged into the 'main' branch.

    aab1e34

    fix(server): add field-level size limits to sandbox and provider crea…

    ...

    This issue will close once commit 890dfcc is merged into the 'main' branch.

    890dfcc

    fix(sandbox): enforce network namespace and proxy policy in SSH sessi…

    ...

    This issue will close once commit 11fd7ef is merged into the 'main' branch.

    11fd7ef
14. factory-octavian

    added 2 commits that reference this issue on Mar 31, 2026

    chore: align sandbox tooling and policies with upstream OpenShell (NV…

    ...

    d430717

    fix: restore cluster_pods network policy for nemoclaw and openclaw (N…

    ...

    7335566
15. maxamillion

    added a commit that references this issue on Apr 8, 2026

    Merge pull request NVIDIA#24 from LobsterTrap/fix/rpm-modules-and-pod…

    ...

    4b67305
16. morphis

    added a commit that references this issue on Apr 27, 2026

    Merge pull request NVIDIA#24 from markatcanonical/fix/gateway-listing…

    ...

    6317a86