feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337)#348
Conversation
Adds `egress.allowed_private_cidrs` to forge.yaml as a scoped alternative to `allow_private_ips: true`. When the boolean is false, IPs falling inside any listed CIDR bypass the private-IP block; everything else private stays blocked. Always-blocked ranges (cloud metadata, loopback, `0.0.0.0/8`) remain blocked unconditionally — no allowlist entry can punch a hole in them. Why: `allow_private_ips: true` is a nuclear switch that opens all of RFC 1918. Enterprise deployments (issue #337 target) want to reach one internal database subnet without exposing the rest of the private-IP space. This lands the narrower policy on its own so the follow-up SOCKS5 raw-TCP work has an unblocked private-destination path to build against. Wire changes: - `IsBlockedIP` and `NewSafeDialer` take an `[]*net.IPNet` for the allowed ranges; the value plumbs through `NewSafeTransport`, `NewEgressProxy`, and `NewEgressEnforcer` (all in-tree callers updated). - `Resolve()` validates the CIDR strings via `ParsePrivateCIDRs` and fails closed on the first invalid entry — bad config trips at load, not at first dial. - Runner parses the resolved CIDR list once and shares the slice between the in-process enforcer and the subprocess proxy so both enforcement paths see the same policy. Tests cover: CIDR-listed IP permitted, CIDR-outside IP blocked, always-blocked ranges cannot be reopened via the list, `allow_private_ips: true` supersedes the list, invalid CIDR at config-load fails, bare IPs (missing /mask) rejected. Related: issue #337 (raw-TCP egress — this is the prereq).
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: allowed_private_cidrs — narrow the private-IP block
Well-scoped, security-conscious change. I traced every claim to the branch source rather than the PR description. Correct, the SSRF invariants hold, and enforcement is wired consistently across both runtime paths. Merge-ready with two nits below.
What I verified (the parts that matter for an egress control)
IsBlockedIPordering is correct. Always-blocked ranges (metadata169.254.169.254/32, loopback,0.0.0.0/8) are checked before the allowlist and return blocked unconditionally — noallowed_private_cidrsentry can punch a hole in them, even if an operator lists169.254.0.0/16. The allowlist is consulted only on theallowPrivate == falsepath and only to narrow the private block, never to open always-blocked space.nilIP fails closed.- The allowlist is effective end-to-end, not just in isolation (the failure mode I most wanted to rule out).
runner.goparses the CIDR list once and hands the same slice to both the in-process enforcer (NewEgressEnforcer) and the subprocess proxy (NewEgressProxy). Inside the proxy, both dial paths carry the CIDRs: HTTP viasafeTransport, HTTPS CONNECT viasafeDialer.SafeDialContext. So the policy applies to plaintext and tunneled egress. The IPv6-transition recursion (NAT64/6to4/Teredo) threads the CIDRs through too — no bypass via an embedded-IPv4 destination. - The
nilinegress_stage.gois harmless — I chased it specifically. It looked like a drop-the-policy gap next toforgecore.Compilepassing the real list. It isn't: the build artifacts that consume the resolved config —GenerateAllowlistJSON(domains only) andGenerateK8sNetworkPolicy(Mode + domain annotation only;egress: to:[]on 443/80, no CIDR IPBlocks) — never readAllowedPrivateCIDRs. Runtime IP enforcement re-resolves from raw config inrunner.go, so deployed-mode enforcement isn't weakened. - Fail-closed config handling.
Resolve()validates CIDR strings at load;runner.go's re-parse, on the should-never-happen error, sets the list tonil→ blocks all private (safe direction). - Tests match the claims and hit the right cases — unit table for
IsBlockedIP(inside/outside CIDR, multi-CIDR,allow_private_ipssupersedes, metadata & loopback stay blocked even when listed, nil default) andSafeDialerintegration (DNS resolving into a listed CIDR passes, outside blocked, metadata never reachable via the list). The always-blocked-wins invariant is pinned at both layers. CI fully green (all 6 builds incl. Windows, Lint, Test, Integration).
Findings — both non-blocking nits (see inline)
- Consistency: build-time CIDR validation is skipped because
egress_stage.gopassesnil. - Behavior/doc mismatch: non-canonical CIDRs (host bits set) are silently widened to the network, despite the doc implying stricter validation.
Optional aside (not a finding): the list isn't restricted to RFC 1918 — 0.0.0.0/0 would open all private ranges (metadata still blocked). Operator's choice, no worse than allow_private_ips: true; a one-line doc caution could prevent a footgun.
Nice contribution — the always-blocked-wins invariant being tested at both the unit and dialer-integration level is exactly the right instinct for this kind of control.
| allowed = append(allowed, security.LLMProviderDomains(bc.Config)...) | ||
|
|
||
| resolved, err := security.Resolve(cfg.Profile, cfg.Mode, allowed, toolNames, cfg.Capabilities) | ||
| resolved, err := security.Resolve(cfg.Profile, cfg.Mode, allowed, toolNames, cfg.Capabilities, nil) |
There was a problem hiding this comment.
Nit (consistency): this passes nil for allowedPrivateCIDRs while forgecore.Compile passes req.Config.Egress.AllowedPrivateCIDRs into the same Resolve(). It's harmless for enforcement — I verified the build artifacts (GenerateAllowlistJSON, GenerateK8sNetworkPolicy) never read the CIDRs, and runtime re-resolves from raw config in runner.go. But the side effect is that forge build won't validate allowed_private_cidrs strings: a typo'd CIDR in forge.yaml sails through the build pipeline and only trips at runtime. Passing cfg.AllowedPrivateCIDRs here instead of nil would make a bad CIDR fail the build, matching Compile and the commit's stated "fail at load, not at first dial" intent. Non-blocking.
|
|
||
| // ParsePrivateCIDRs parses a list of CIDR strings into net.IPNet values. | ||
| // Returns an error naming the first invalid entry. Entries must be canonical | ||
| // CIDR notation (e.g. "10.0.0.0/8"); bare IPs are rejected — the intent is |
There was a problem hiding this comment.
Nit (behavior/doc mismatch): the comment says entries "must be canonical CIDR notation" and "bare IPs are rejected," but net.ParseCIDR("10.20.0.5/16") returns 10.20.0.0/16 with no error — host bits are silently masked. So an operator who writes 10.20.0.5/16 intending a single host gets a whole /16 allowed, which is the security-relevant "wider than intended" direction. Either reject host-bits-set entries (compare the returned n.IP against the parsed address and error if they differ) or reword the doc to say host bits are masked to the network. Low severity, operator-controlled.
…, dead flag) - [Low] egress-resolve failure now fails CLOSED: a deny-all allowlist enforcer instead of an unenforced http.DefaultClient, so a future config that makes Resolve error can't silently drop egress enforcement. - [Nit] drop the dead --yes flag (parsed, never read). - [Low/nit] correct the paste-key comment: the key is session-only and never written to disk, even under --keep; note the consequence (a kept agent has no credential on disk). - [Nit/UX] Ctrl-C at the 'you ›' prompt now exits: stdin reads on a goroutine and the REPL selects on ctx.Done() (scanner.Scan isn't ctx-aware). - [Medium/#348 coupling] add an explicit NOTE that buildTryEgress is on the pre-#348 security signatures and must thread allowedPrivateCIDRs when #348 lands (tracked; can't rebase onto an unmerged branch). Build/vet/lint clean; cmd + runtime + tryview suites green.
…canonical CIDRs Two nits from @initializ-mk's review of #348, both non-blocking but genuine: **Nit 1 — build stage skipped CIDR validation.** `forge-cli/build/egress_stage.go` passed `nil` for `allowedPrivateCIDRs` to `security.Resolve`, so a typo in `allowed_private_cidrs` sailed through `forge build` and only tripped at `forge run`. Enforcement wasn't affected (build artifacts don't consume the CIDRs; runtime re-resolves from raw config), but the fail-loud posture should be identical between build and run. Now passes the actual field. **Nit 2 — silent widening on non-canonical CIDRs.** `net.ParseCIDR("10.20.0.5/16")` returns `10.20.0.0/16` without an error — an operator who wrote a single-host IP with a /16 mask (typo or misconception) would silently get a whole /16 allowed instead of one host. That's the security-wrong direction of "wider than intended". `ParsePrivateCIDRs` now compares the parsed IP against the returned network and errors on mismatch. The error message names both fixes (`10.20.0.0/16` for the range, `10.20.0.5/32` for the single host) so the log line is actionable without a second look at the code. Tests: table entries for host-bits-set rejection (IPv4 + IPv6), canonical network address accepted, single-host /32 accepted, error message names both fix suggestions. All existing tests still pass (they were already canonical). Doc: updated `docs/security/egress-control.md` to spell out the stricter validation rule and note that `forge build` now validates too. Review context: #348 (review)
|
Thanks for the sharp review — both nits addressed in e3c0c40 (just pushed). Nit 1 — build-time validation. Nit 2 — silent widening. The error message deliberately spells out both the range form and the /32 single-host form so the log line is actionable without a second look at the code. Tests cover: IPv4 host-bits-set rejection, IPv6 host-bits-set rejection ( Optional aside — the |
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review: fix commits e3c0c406 + 7c780d5d — both nits resolved
Verified against the diffs. CI fully green (all 6 builds incl. Windows, Lint, Test, Integration, Doc-link).
Finding 1 (build-time CIDR validation) — ✅ resolved
egress_stage.go now passes cfg.AllowedPrivateCIDRs instead of nil, so forge build validates the strings via Resolve→ParsePrivateCIDRs — a typo trips the build, matching forge run and Compile. The comment correctly preserves the nuance: build artifacts still don't consume the CIDRs; this is purely fail-loud symmetry.
Finding 2 (non-canonical silent widening) — ✅ resolved, correctly
The rejection logic is exactly right: ip, n, _ := net.ParseCIDR(s); if !ip.Equal(n.IP) { reject }.
10.20.0.5/16→ip=.5,n.IP=.0→ not equal → rejected (the silent-widening case).10.20.0.0/16(canonical) and10.20.0.5/32(single host) → equal → accepted. No false positives.- Using
net.IP.Equal(notbytes.Equal) is the correct choice — robust to the 4-byte vs 16-byte representationParseCIDRcan return. - The error names both fixes (
/16range,/32host), andsingleHostMaskcorrectly picks/32vs/128by family — the log line is self-explanatory.
Tests are thorough: IPv4 non-canonical rejected (asserts all three message substrings), IPv6 non-canonical rejected (asserts the /128 suggestion), canonical v4+v6 accepted, /32 single-host accepted — pinning both the behavior and the actionable message.
Bonus — 0.0.0.0/0 doc aside (7c780d5d) — ✅ added
Documents that 0.0.0.0/0 isn't restricted, that always-blocked still wins, and nudges toward allow_private_ips: true for clearer intent. Accurate.
Verdict: both nits resolved with the stronger option, docs updated to match, tests lock the behavior and the error message, CI fully green. Nothing dangling — merge-ready.
One cross-PR note for whoever merges: once this lands, #353's buildTryEgress carries a NOTE that its egress constructors are on the pre-#348 signatures and must thread allowedPrivateCIDRs through — worth doing in the #353 rebase so forge try honors allowed_private_cidrs too.
…canonical CIDRs Two nits from @initializ-mk's review of #348, both non-blocking but genuine: **Nit 1 — build stage skipped CIDR validation.** `forge-cli/build/egress_stage.go` passed `nil` for `allowedPrivateCIDRs` to `security.Resolve`, so a typo in `allowed_private_cidrs` sailed through `forge build` and only tripped at `forge run`. Enforcement wasn't affected (build artifacts don't consume the CIDRs; runtime re-resolves from raw config), but the fail-loud posture should be identical between build and run. Now passes the actual field. **Nit 2 — silent widening on non-canonical CIDRs.** `net.ParseCIDR("10.20.0.5/16")` returns `10.20.0.0/16` without an error — an operator who wrote a single-host IP with a /16 mask (typo or misconception) would silently get a whole /16 allowed instead of one host. That's the security-wrong direction of "wider than intended". `ParsePrivateCIDRs` now compares the parsed IP against the returned network and errors on mismatch. The error message names both fixes (`10.20.0.0/16` for the range, `10.20.0.5/32` for the single host) so the log line is actionable without a second look at the code. Tests: table entries for host-bits-set rejection (IPv4 + IPv6), canonical network address accepted, single-host /32 accepted, error message names both fix suggestions. All existing tests still pass (they were already canonical). Doc: updated `docs/security/egress-control.md` to spell out the stricter validation rule and note that `forge build` now validates too. Review context: #348 (review)
#337) Adds `egress.allowed_tcp` for raw-TCP egress (databases, message brokers) governed by the same allowlist engine that gates HTTP/HTTPS. The proxy now binds a second listener speaking SOCKS5v5 alongside the HTTP forward proxy — one policy, one audit shape, two client protocols. Skill subprocesses reach it via ALL_PROXY / SOCKS_PROXY env vars. Why the enterprise segment needs this: `libpq`/`redis-cli`/`kafka-*` etc. open raw sockets — they ignore HTTP_PROXY entirely, so today they either bypass egress (in dev) or fail closed (in locked deploys). Neither is usable. Layered on top of PR 1's `allowed_private_cidrs`, this lets an agent reach exactly `db.internal:5432` on `10.20.0.0/16` and nothing else. Key pieces: - forge-core/security/tcp_matcher.go — new port-aware matcher. Entries: `host:port`, `*.suffix:port`, `host:*`, `*.suffix:*`. Same case-insensitive wildcard-suffix rules as DomainMatcher. Invalid entries fail at Resolve() time, not at first dial. - forge-core/security/socks5.go — minimal RFC 1928 CONNECT-only server (~180 LOC, no external dep). DOMAIN/IPv4/IPv6 ATYP. BIND + UDP ASSOCIATE rejected with REP=0x07 (small explicit surface). - forge-core/security/dial_gate.go — new `ValidateAndDial` primitive shared by handleConnect and handleSOCKS5. One codepath for policy + audit + dial, no drift possible. - forge-core/security/egress_proxy.go — binds a second listener when `allowed_tcp` is non-empty (skips it otherwise — no extra port for deployments that don't need raw-TCP). `SOCKSURL()` returns the `socks5h://…` URL; the `h` forces server-side hostname resolution so the proxy can run the allowlist check against the hostname the client intended, not an IP it pre-resolved. - forge-cli/tools/exec.go — injects ALL_PROXY / SOCKS_PROXY (lowercase + uppercase) into skill subprocess env when SOCKSURL is set. Same lifecycle as HTTP_PROXY. - forge-cli/runtime/runner.go — wires the TCP matcher into the proxy and threads socksURL through registerSkillTools → SkillCommandExecutor and through RunSkillScriptTool. Backward compatibility: - HTTP audit shape unchanged (hostname-only Domain field) — the SOCKS5 path records `host:port` since port is the point. Callers key by hostname keep working; SOCKS5 event consumers get the granularity they need. - Existing tests untouched; new tests cover TCPMatcher (exact/wildcard/any- port), SOCKS5 end-to-end (echo round-trip, denied-not-in-allowlist, HTTP-allowlist-also-covers-TCP, BIND-rejected, malformed-greeting, listener-absent-without-config, empty-matcher-disables-listener, socks5h-scheme). Limitations documented in docs/security/egress-control.md: - SOCKS5v5 (no-auth) has no channel for per-request task attribution — raw-TCP audit events carry host:port + decision only, no task/correlation IDs. HTTP path retains #338 attribution via Proxy-Authorization. - `proxychains-ng` (for CLIs that don't speak SOCKS5 natively) works on Linux glibc/musl; macOS is mostly out (SIP + DYLD), static Go binaries and setuid contexts are out. Depends on: PR #348 (allowed_private_cidrs — the SafeDialer signature + `AllowedPrivateCIDRs` on EgressConfig this PR builds on). Closes: #337
2 participants
Summary
egress.allowed_private_cidrstoforge.yaml. Whenallow_private_ipsis false (or unset), IPs falling inside a listed CIDR bypass the private-IP block; everything else private stays blocked.0.0.0.0/8) remain blocked regardless — no allowlist entry punches a hole in them.Merge order
fix/egress-private-cidrs)fix/egress-socks5-tcp(coming next)AllowedPrivateCIDRsonEgressConfigWhat changed
IsBlockedIP/NewSafeDialer/NewSafeTransport/NewEgressProxy/NewEgressEnforcerall take an[]*net.IPNetfor the allowed private ranges (all in-tree callers updated).Resolve()validates CIDR strings viaParsePrivateCIDRsand fails closed on the first invalid entry — bad config trips at load, not at first dial.docs/security/egress-control.mdgains a "Narrow Private-CIDR Allowlist" subsection + updated yaml example.Semantics (checked by tests)
allow_private_ipsallowed_private_cidrs[10.20.0.0/16][169.254.0.0/16]169.254.169.254) still blocked — always-blocked winsTest plan
go test ./forge-core/...— all packages passgo test ./forge-cli/runtime/... ./forge-cli/tools/...— all packages passgolangci-lint run ./forge-core/... ./forge-cli/... ./forge-plugins/...— 0 issuesallow_private_ips: truesupersedes the list; invalid CIDR at config-load fails; bare IPs rejected; IP-literal path works.Closes-partial: #337