feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337)#348
feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337)#348naveen-kurra wants to merge 1 commit into
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.
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