Skip to content

fix(sandbox): validate always-blocked IPs at load time, enrich denial logs, and filter un-fixable proposals#815

Open
johntmyers wants to merge 1 commit intomainfrom
fix/814-always-blocked-validation
Open

fix(sandbox): validate always-blocked IPs at load time, enrich denial logs, and filter un-fixable proposals#815
johntmyers wants to merge 1 commit intomainfrom
fix/814-always-blocked-validation

Conversation

@johntmyers
Copy link
Copy Markdown
Collaborator

What changes for users

  1. Policies with always-blocked allowed_ips now fail to load. If your policy YAML contains allowed_ips entries targeting loopback (127.0.0.0/8), link-local (169.254.0.0/16), unspecified (0.0.0.0), or 0.0.0.0/0, the proxy rejects them with a clear error naming the offending entry and citing SSRF hardening. Previously these were silently accepted but never actually worked.

  2. Literal IP hosts that are always-blocked are also caught. A policy with host: '127.0.0.1' no longer silently synthesizes an allowed_ips entry that would be blocked at runtime.

  3. Denial logs now explain why traffic was blocked. The shorthand log format for DENIED events includes a [reason:...] suffix:

    NET:OPEN [MED] DENIED curl(1618) -> 169.254.169.254:80 [policy:- engine:ssrf] [reason:resolves to always-blocked address]
    

    Previously "allowlist miss" and "structurally un-allowable" denials looked identical in openshell logs.

  4. HTTP shorthand now shows [engine:] tag. Consistent with NET shorthand — was previously omitted.

  5. The TUI stops showing un-fixable proposals. The policy advisor no longer generates proposals for always-blocked destinations, ending the infinite 10-second notification loop.

  6. Server rejects always-blocked entries on approval. Defense-in-depth: if a draft chunk with always-blocked allowed_ips somehow reaches the gateway, ApproveDraftChunk returns INVALID_ARGUMENT instead of silently merging an un-enforceable rule.

Related Issue

Closes #814

Changes

Shared helpers (openshell-core)

  • New crates/openshell-core/src/net.rs with is_always_blocked_ip, is_always_blocked_net, is_internal_ip — eliminates duplication between proxy, mapper, and server

Sandbox proxy (openshell-sandbox)

  • parse_allowed_ips rejects entries overlapping always-blocked ranges (hard error, not warning)
  • implicit_allowed_ips_for_ip_host returns empty for always-blocked literal IPs
  • All 3 HTTP forward-proxy denial paths now set .status_detail(&reason) on HttpActivityBuilder

OCSF logging (openshell-ocsf)

  • HttpActivityBuilder gains status_detail field and builder method
  • Shorthand formatter appends [reason:{status_detail}] for DENIED events (truncated at 80 chars)
  • HTTP shorthand gains [engine:] tag in [policy:X engine:Y] (was [policy:X] only)

Mechanistic mapper (openshell-sandbox)

  • generate_proposals skips always-blocked destinations via is_always_blocked_destination
  • resolve_allowed_ips_if_private filters always-blocked IPs from resolved results

Gateway server (openshell-server)

  • validate_rule_not_always_blocked called in merge_chunk_into_policy — rejects literal always-blocked hosts, localhost, and always-blocked allowed_ips entries with INVALID_ARGUMENT

Documentation

  • architecture/security-policy.md — SSRF validation flow, load-time rejection, server-side defense
  • architecture/policy-advisor.md — mapper filtering, gateway validation, known behavior
  • architecture/sandbox.md — shared helper mention
  • docs/security/best-practices.mdx — user-facing SSRF section
  • docs/reference/policy-schema.mdx — added missing allowed_ips field to endpoint table
  • docs/observability/logging.mdx — shorthand examples with [reason:] and [engine:]
  • docs/observability/ocsf-json-export.mdxstatus_detail in JSON example

E2E tests

  • Updated SSRF-3 test to assert [reason:] tag in denied event shorthand
  • Updated SSRF-7 docstring to reflect new code path (parse-time rejection)

Testing

  • cargo fmt --check passes
  • Unit tests added/updated (64+ new tests across 5 crates)
  • All affected crate tests pass: core (87), ocsf (115), sandbox (433+5), server (212+11), policy (43)
  • E2E test assertions updated
  • mise run pre-commit — blocked by pre-existing z3.h environment issue (unrelated)
  • mise run e2e — requires running cluster

Checklist

  • Follows Conventional Commits
  • Architecture docs updated
  • Published docs updated
  • No secrets or credentials committed

… logs, and filter un-fixable proposals (#814)

Policies with allowed_ips entries targeting loopback, link-local, or
unspecified ranges now fail at connection time instead of being silently
blocked at runtime. The shorthand log format for DENIED events includes
a [reason:...] suffix so operators can distinguish 'allowlist miss' from
'structurally un-allowable'. The mechanistic mapper skips proposals for
always-blocked destinations, preventing the infinite TUI notification
loop. The gateway validates proposed rules on approval as defense-in-depth.

- Extract shared IP helpers (is_always_blocked_ip, is_always_blocked_net,
  is_internal_ip) to openshell_core::net
- Reject always-blocked entries in parse_allowed_ips with hard error
- Skip implicit allowed_ips synthesis for always-blocked literal IP hosts
- Add status_detail to HttpActivityBuilder for denial reason propagation
- Enrich NET and HTTP shorthand with [reason:...] for DENIED events
- Add engine: tag to HTTP shorthand (consistency with NET shorthand)
- Filter always-blocked proposals in mechanistic mapper generate_proposals
- Add validate_rule_not_always_blocked server-side defense-in-depth
- Update architecture docs, published docs, and E2E test assertions
@johntmyers johntmyers self-assigned this Apr 12, 2026
@johntmyers johntmyers requested a review from a team as a code owner April 12, 2026 00:20
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Apr 12, 2026

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.

@github-actions
Copy link
Copy Markdown

@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: link-local / loopback entries in allowed_ips are silently accepted but never enforced

1 participant