fix(permissions): match network rules against the resource host - #1958
fix(permissions): match network rules against the resource host#1958Scorpion197 wants to merge 1 commit into
Conversation
Network rule patterns written in the documented host form (`api.example.com`, `api.example.com:443`, `*.example.com`, `*`) never matched, because the kernel checks the URI form of the resource (`tcp://host:port`, `dns://host`) and a single `*` cannot cross the `//`. A documented allowlist denied every host and a documented blocklist permitted every host. Scheme-less `network` patterns are now matched against the host subject of the resource: the bare host, and `host:port` when a port is present. Patterns that carry a scheme keep matching the full URI. Resources that do not parse as `scheme://subject` are only matched by full-URI patterns, so unexpected shapes fail closed. Other pattern scopes are untouched. Existing tests that fed bare `host:port` resources into the network evaluator now use the `tcp://` form the kernel actually produces.
defiufo
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is correct, and it's worth stating that this is a fail-open, not only a fail-closed: with default: "allow" plus deny rules, no host was ever blocked, because a single * can't cross the // in tcp://… (permission_glob_matches treats * as not crossing /). An operator who wrote a blocklist got silent full egress. That framing probably belongs in the PR body.
Also a good catch that the existing service.rs tests were feeding the evaluator bare host:port resources the kernel never actually emits — those assertions were describing behavior that didn't exist.
The approach looks right to me: scheme-carrying patterns keep matching the full URI, scheme-less patterns match the parsed host subject, unparseable resources fail closed, and the domain parameter is threaded through both evaluators so the post-DNS-resolution path in service.rs doesn't keep the old semantics. The child_process_patterns_are_not_subject_parsed test pinning the other scopes is a nice touch.
Three things before this lands:
1. Fixes #1884 should be narrowed to Partially addresses #1884.
Issue #1884 has two halves. The second one is untouched here:
// packages/core/src/agent-os.ts:3276
const hostPermissions = options?.permissions ?? { ...allowAll, binding: "allow" };Any explicit permissions object replaces the default wholesale, which contradicts docs/content/docs/permissions.mdx:32 ("Your policy is merged over this baseline. Omitted scopes keep their default; they are not denied") and drops the documented binding auto-grant. Merging this as-is would auto-close a live bug.
There's some irony here: that merge bug is exactly what makes this fix hard to verify end-to-end, since a repro has to spell out all six scopes or { network: {...} } alone silently denies fs/process too. I've filed the merge half separately so this PR can close cleanly.
2. Case sensitivity now splits by scheme.
canonical_dns_subject lowercases (crates/kernel/src/dns.rs:464); format_tcp_resource does not (crates/kernel/src/network_policy.rs:30). So patterns: ["API.example.com"] matches the dns:// check and misses the tcp:// check for the same connection — a rule that half-applies, which is arguably worse than one that cleanly fails. Could you lowercase the host subject (and the pattern) before globbing, with a test asserting Api.Example.COM matches tcp://api.example.com:443?
3. unix: and wildcard-port resources fall through network_resource_subject.
It returns None when there's no :// or when the remainder contains /, so under default: "deny":
| Resource | Producer | Subject | patterns: ["*"] |
|---|---|---|---|
tcp://api.example.com:443 |
format_tcp_resource |
api.example.com |
matches |
unix:/tmp/app.sock |
format_unix_socket_resource (javascript/rpc.rs:4963) |
None |
denied |
unix://tmp/app.sock |
socket_query_resource (execution/process.rs:2141) |
None |
denied |
tcp://127.0.0.1:* |
socket_query_resource, port omitted |
host is 127.0.0.1:* |
"127.0.0.1" denied |
unix:abstract:{hex} |
JS abstract sockets | None |
denied |
None of these are regressions — they didn't match before either. But the docs line this PR adds says * "matches every host", and these are now the only places where it silently doesn't. Either extend the parser (strip a bare scheme: prefix and match the remainder; treat a * port component as literal), or document the carve-out and pin it with tests for unix: and the :* inspection form, so it's intentional rather than incidental.
Minor: network_pattern_has_scheme's starts_with("unix:") special case is fragile — it's correct today, but it means the scheme test isn't a single rule. Normalizing unix resources to unix:// at the producers would let one rule cover everything.
CI: mergeable_state is currently unstable — could you re-run and post the failure if it isn't a flake?
Test coverage here is genuinely above the median for this repo (17 tests, both directions, both evaluators, cross-scheme isolation, IPv6, last-rule-wins, fail-closed). With the Fixes line narrowed and case handling sorted, this looks good to me.
(Drive-by review — I don't have merge rights on this repo.)
Fixes #1884.
api.example.com,api.example.com:443,*.example.com,*) never matched, because the kernel checks the URI form of the resource (tcp://host:port,dns://host) and a single*cannot cross the//. A documented allowlist therefore denied every host, and a documented blocklist permitted every host.networkpatterns are now matched against the host subject of the resource: the bare host, andhost:portwhen a port is present. Patterns that carry a scheme (tcp://...,dns://...) keep matching the full URI, so existing policies written in that form are unchanged.scheme://subjectare only matched by full-URI patterns, so unexpected resource shapes fail closed. Other pattern scopes are untouched.crates/native-sidecar-core/tests/network_permissions.rscovering the matrix from the report in both directions, the URI form, IPv6 literals, last-rule-wins, the post-resolution evaluator, and the fail-closed cases. Existing tests that fed barehost:portresources into the network evaluator now use thetcp://form the kernel actually produces.permissions.mdx. The shippedallow-one-hostexample is unchanged and now works.This is the matcher alternative offered in #1910, which corrects the docs to the URI form instead. The TypeScript merge-over-default behavior described in the second half of #1884 is a separate layer and is not addressed here.