From 267beeb9bbcdf8e5bfb3f9bca6f6072aa1dd2671 Mon Sep 17 00:00:00 2001 From: naveen-kurra Date: Mon, 20 Jul 2026 16:27:57 -0400 Subject: [PATCH 1/3] =?UTF-8?q?feat(security):=20allowed=5Fprivate=5Fcidrs?= =?UTF-8?q?=20=E2=80=94=20narrow=20the=20private-IP=20block?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- docs/security/egress-control.md | 28 ++++ forge-cli/build/egress_stage.go | 2 +- forge-cli/runtime/runner.go | 14 +- forge-cli/tools/browser/manager_test.go | 4 +- forge-cli/tools/browser/manual_demo_test.go | 2 +- forge-core/forgecore.go | 1 + forge-core/security/egress_enforcer.go | 22 +-- forge-core/security/egress_enforcer_test.go | 16 +- .../security/egress_integration_test.go | 6 +- forge-core/security/egress_proxy.go | 10 +- forge-core/security/egress_proxy_test.go | 22 +-- forge-core/security/ip_validator.go | 69 +++++++-- forge-core/security/ip_validator_test.go | 138 +++++++++++++++++- forge-core/security/resolver.go | 20 ++- forge-core/security/resolver_test.go | 51 +++++-- forge-core/security/safe_dialer.go | 33 +++-- forge-core/security/safe_dialer_test.go | 91 ++++++++++-- forge-core/security/types.go | 4 + forge-core/types/config.go | 6 + go.work.sum | 4 +- 20 files changed, 446 insertions(+), 97 deletions(-) diff --git a/docs/security/egress-control.md b/docs/security/egress-control.md index 94cc70b5..a1202014 100644 --- a/docs/security/egress-control.md +++ b/docs/security/egress-control.md @@ -109,6 +109,30 @@ The `allowPrivateIPs` setting is resolved with this precedence: Cloud metadata (`169.254.169.254`) is **always** blocked regardless of the `allowPrivateIPs` setting. +### Narrow Private-CIDR Allowlist (issue #337 preparation) + +`allow_private_ips: true` opens **all** RFC 1918 / link-local / CGNAT / ULA ranges at once — the right choice for a Kubernetes pod that talks to any service in the mesh, but too wide for an agent that only needs to reach one internal database. + +`allowed_private_cidrs` narrows that: when `allow_private_ips` is false (or unset), IPs falling inside any listed CIDR bypass the private-block, and everything else private stays blocked. + +```yaml +egress: + mode: allowlist + allow_private_ips: false # keep RFC 1918 blocked wholesale + allowed_private_cidrs: # ...but open these two slices + - 10.20.0.0/16 # internal databases + - 172.16.42.0/24 # message brokers +``` + +Invariants (checked by tests): + +- **Always-blocked ranges (cloud metadata `169.254.169.254`, loopback, `0.0.0.0/8`) stay blocked** even if an operator lists a CIDR that would otherwise contain them. No allowlist punches a hole in these. +- **`allow_private_ips: true` supersedes** the CIDR list — the boolean means "all private allowed" and the list is redundant. A warning in that case is a good future addition, but the effective policy is safe (superset). +- **Invalid CIDR strings fail at config-load time**, not at first dial. `Resolve()` validates each entry via `net.ParseCIDR`; bare IPs (missing `/mask`) are rejected because the config intent is range-level exemption. +- CIDRs are consulted by both the in-process `EgressEnforcer` (Go clients) and the subprocess `EgressProxy` (skill scripts) — one policy, two enforcement paths. + +Companion feature for #337: raw-TCP egress (databases, message brokers) requires this narrow-private mechanism to be reachable in the first place. Landing the CIDR allowlist first (this PR) unblocks the SOCKS5-based TCP path (follow-up PR) without opening RFC 1918 wholesale. + ## Runtime Egress Enforcer The `EgressEnforcer` (`forge-core/security/egress_enforcer.go`) is an `http.RoundTripper` that wraps a `SafeTransport`. Every outbound HTTP request from in-process Go code (builtins like `http_request`, `web_search`, LLM API calls) passes through it. @@ -307,10 +331,14 @@ egress: - slack - telegram allow_private_ips: false # default: auto-detect from container env + allowed_private_cidrs: # narrow private-IP allowlist (see above) + - 10.20.0.0/16 ``` The `allow_private_ips` field controls whether RFC 1918 addresses are allowed through the SafeDialer. When omitted, it defaults to `true` inside containers (detected via `KUBERNETES_SERVICE_HOST` or `/.dockerenv`) and `false` otherwise. Cloud metadata (`169.254.169.254`) is always blocked. +`allowed_private_cidrs` (added in issue #337) is a narrower alternative: it lets you reach a specific slice of the private-IP space (e.g. only the internal-database subnet) without opening RFC 1918 wholesale. See "Narrow Private-CIDR Allowlist" above. + ## Production vs Development | Setting | Production | Development | diff --git a/forge-cli/build/egress_stage.go b/forge-cli/build/egress_stage.go index f6507cea..b7e2e080 100644 --- a/forge-cli/build/egress_stage.go +++ b/forge-cli/build/egress_stage.go @@ -51,7 +51,7 @@ func (s *EgressStage) Execute(ctx context.Context, bc *pipeline.BuildContext) er // time out depending on which side notices first. 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) if err != nil { return fmt.Errorf("resolving egress: %w", err) } diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index c0ecc0f4..fedd9046 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -662,6 +662,7 @@ func (r *Runner) Run(ctx context.Context) error { egressDomains, egressToolNames, r.cfg.Config.Egress.Capabilities, + r.cfg.Config.Egress.AllowedPrivateCIDRs, ) if egressErr != nil { r.logger.Warn("failed to resolve egress config, using default", map[string]any{"error": egressErr.Error()}) @@ -675,7 +676,16 @@ func (r *Runner) Run(ctx context.Context) error { allowPrivateIPs = true } - enforcer := security.NewEgressEnforcer(nil, egressCfg.Mode, egressCfg.AllDomains, allowPrivateIPs) + // Parse the CIDR allowlist here so the enforcer and proxy share one + // resolved slice. Resolve() already validated the strings, so this + // only fails on an internal programming error. + allowedPrivateCIDRs, cidrErr := security.ParsePrivateCIDRs(egressCfg.AllowedPrivateCIDRs) + if cidrErr != nil { + r.logger.Warn("failed to parse allowed_private_cidrs, ignoring", map[string]any{"error": cidrErr.Error()}) + allowedPrivateCIDRs = nil + } + + enforcer := security.NewEgressEnforcer(nil, egressCfg.Mode, egressCfg.AllDomains, allowPrivateIPs, allowedPrivateCIDRs) enforcer.OnAttempt = func(ctx context.Context, domain string, allowed bool) { event := coreruntime.AuditEgressAllowed if !allowed { @@ -723,7 +733,7 @@ func (r *Runner) Run(ctx context.Context) error { browserActive := r.derivedBrowserConfig != nil if (!security.InContainer() && egressCfg.Mode != security.ModeDevOpen) || browserActive { matcher := security.NewDomainMatcher(egressCfg.Mode, egressCfg.AllDomains) - egressProxy = security.NewEgressProxy(matcher, allowPrivateIPs) + egressProxy = security.NewEgressProxy(matcher, allowPrivateIPs, allowedPrivateCIDRs) egressProxy.OnAttempt = func(a security.EgressAttempt) { event := coreruntime.AuditEgressAllowed if !a.Allowed { diff --git a/forge-cli/tools/browser/manager_test.go b/forge-cli/tools/browser/manager_test.go index b5bddd41..b54b346d 100644 --- a/forge-cli/tools/browser/manager_test.go +++ b/forge-cli/tools/browser/manager_test.go @@ -44,7 +44,7 @@ func requireChromium(t *testing.T) string { // trivial expression. Returns nil only if the browser genuinely started. func probeBrowserLaunch(bin string) error { matcher := security.NewDomainMatcher(security.ModeAllowlist, nil) - proxy := security.NewEgressProxy(matcher, false) + proxy := security.NewEgressProxy(matcher, false, nil) proxyURL, err := proxy.Start(context.Background()) if err != nil { return err @@ -71,7 +71,7 @@ func probeBrowserLaunch(bin string) error { // OnAttempt calls. Returns the proxy URL and the attempts map (guarded by mu). func startProxy(t *testing.T, matcher *security.DomainMatcher) (string, *sync.Mutex, map[string]bool) { t.Helper() - proxy := security.NewEgressProxy(matcher, false) + proxy := security.NewEgressProxy(matcher, false, nil) mu := &sync.Mutex{} attempts := map[string]bool{} proxy.OnAttempt = func(a security.EgressAttempt) { diff --git a/forge-cli/tools/browser/manual_demo_test.go b/forge-cli/tools/browser/manual_demo_test.go index cfd41bd1..d88167c4 100644 --- a/forge-cli/tools/browser/manual_demo_test.go +++ b/forge-cli/tools/browser/manual_demo_test.go @@ -49,7 +49,7 @@ func TestManualBrowseDemo(t *testing.T) { // Allowlist exactly the target host (plus its www/apex sibling), so this // demonstrates real egress enforcement, not dev-open. matcher := security.NewDomainMatcher(security.ModeAllowlist, []string{u.Hostname()}) - proxy := security.NewEgressProxy(matcher, false) + proxy := security.NewEgressProxy(matcher, false, nil) proxy.OnAttempt = func(a security.EgressAttempt) { verdict := "ALLOW" if !a.Allowed { diff --git a/forge-core/forgecore.go b/forge-core/forgecore.go index 317a826a..fa5db6ba 100644 --- a/forge-core/forgecore.go +++ b/forge-core/forgecore.go @@ -67,6 +67,7 @@ func Compile(req CompileRequest) (*CompileResult, error) { req.Config.Egress.AllowedDomains, toolNames, nil, // capabilities resolved from profile + req.Config.Egress.AllowedPrivateCIDRs, ) if err != nil { return nil, err diff --git a/forge-core/security/egress_enforcer.go b/forge-core/security/egress_enforcer.go index 114b1ac7..5b742315 100644 --- a/forge-core/security/egress_enforcer.go +++ b/forge-core/security/egress_enforcer.go @@ -3,6 +3,7 @@ package security import ( "context" "fmt" + "net" "net/http" "strings" ) @@ -13,24 +14,27 @@ type egressClientKey struct{} // EgressEnforcer is an http.RoundTripper that validates outbound requests // against a domain allowlist before forwarding them to the base transport. type EgressEnforcer struct { - base http.RoundTripper - matcher *DomainMatcher - AllowPrivateIPs bool - OnAttempt func(ctx context.Context, domain string, allowed bool) + base http.RoundTripper + matcher *DomainMatcher + AllowPrivateIPs bool + AllowedPrivateCIDRs []*net.IPNet + OnAttempt func(ctx context.Context, domain string, allowed bool) } // NewEgressEnforcer creates a new EgressEnforcer wrapping the given base transport. // If base is nil, a SafeTransport is used instead of http.DefaultTransport. // Domains may include wildcard prefixes (e.g. "*.github.com") which match any subdomain. -func NewEgressEnforcer(base http.RoundTripper, mode EgressMode, domains []string, allowPrivateIPs bool) *EgressEnforcer { +// allowedPrivateCIDRs narrows the SafeDialer's private-IP block; see NewSafeDialer. +func NewEgressEnforcer(base http.RoundTripper, mode EgressMode, domains []string, allowPrivateIPs bool, allowedPrivateCIDRs []*net.IPNet) *EgressEnforcer { if base == nil { - base = NewSafeTransport(nil, allowPrivateIPs) + base = NewSafeTransport(nil, allowPrivateIPs, allowedPrivateCIDRs) } return &EgressEnforcer{ - base: base, - matcher: NewDomainMatcher(mode, domains), - AllowPrivateIPs: allowPrivateIPs, + base: base, + matcher: NewDomainMatcher(mode, domains), + AllowPrivateIPs: allowPrivateIPs, + AllowedPrivateCIDRs: allowedPrivateCIDRs, } } diff --git a/forge-core/security/egress_enforcer_test.go b/forge-core/security/egress_enforcer_test.go index bf1c5fef..71d84ba0 100644 --- a/forge-core/security/egress_enforcer_test.go +++ b/forge-core/security/egress_enforcer_test.go @@ -92,7 +92,7 @@ func TestEgressEnforcerAllowlist(t *testing.T) { })) defer ts.Close() - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, tt.domains, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, tt.domains, false, nil) req, err := http.NewRequest("GET", tt.url, nil) if err != nil { @@ -119,7 +119,7 @@ func TestEgressEnforcerAllowlist(t *testing.T) { } func TestEgressEnforcerDenyAll(t *testing.T) { - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDenyAll, nil, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDenyAll, nil, false, nil) req, _ := http.NewRequest("GET", "https://api.openai.com/v1/chat", nil) _, err := enforcer.RoundTrip(req) @@ -134,7 +134,7 @@ func TestEgressEnforcerDenyAllAllowsLocalhost(t *testing.T) { })) defer ts.Close() - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDenyAll, nil, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDenyAll, nil, false, nil) req, _ := http.NewRequest("GET", ts.URL+"/test", nil) resp, err := enforcer.RoundTrip(req) @@ -150,7 +150,7 @@ func TestEgressEnforcerDevOpen(t *testing.T) { })) defer ts.Close() - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDevOpen, nil, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDevOpen, nil, false, nil) req, _ := http.NewRequest("GET", ts.URL+"/test", nil) resp, err := enforcer.RoundTrip(req) @@ -167,7 +167,7 @@ func TestEgressEnforcerOnAttemptCallback(t *testing.T) { allowed bool } - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"api.openai.com"}, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"api.openai.com"}, false, nil) enforcer.OnAttempt = func(_ context.Context, domain string, allowed bool) { mu.Lock() calls = append(calls, struct { @@ -201,7 +201,7 @@ func TestEgressEnforcerOnAttemptCallback(t *testing.T) { func TestEgressEnforcerDevOpenCallback(t *testing.T) { var called bool - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDevOpen, nil, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDevOpen, nil, false, nil) enforcer.OnAttempt = func(_ context.Context, domain string, allowed bool) { called = true if !allowed { @@ -256,7 +256,7 @@ func TestEgressContextMissing(t *testing.T) { } func TestEgressEnforcerNilBase(t *testing.T) { - enforcer := NewEgressEnforcer(nil, ModeAllowlist, []string{"example.com"}, false) + enforcer := NewEgressEnforcer(nil, ModeAllowlist, []string{"example.com"}, false, nil) if enforcer.base == nil { t.Error("nil base should be replaced with SafeTransport") } @@ -285,7 +285,7 @@ func TestIsLocalhost(t *testing.T) { func TestEgressEnforcerSSRFBypass(t *testing.T) { // Non-standard IP formats should be blocked by ValidateHostIP - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"example.com"}, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"example.com"}, false, nil) tests := []struct { name string diff --git a/forge-core/security/egress_integration_test.go b/forge-core/security/egress_integration_test.go index f0d263c9..141e3977 100644 --- a/forge-core/security/egress_integration_test.go +++ b/forge-core/security/egress_integration_test.go @@ -20,7 +20,7 @@ func TestEgressEnforcerIntegration(t *testing.T) { defer ts.Close() // Build an enforcer that allows only localhost (test server is localhost) - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"api.openai.com"}, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"api.openai.com"}, false, nil) var attemptLog []struct { domain string @@ -78,7 +78,7 @@ func TestEgressEnforcerDenyAllIntegration(t *testing.T) { })) defer ts.Close() - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDenyAll, nil, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeDenyAll, nil, false, nil) client := &http.Client{Transport: enforcer} // Localhost should still work even in deny-all @@ -90,7 +90,7 @@ func TestEgressEnforcerDenyAllIntegration(t *testing.T) { } func TestEgressEnforcerWildcardIntegration(t *testing.T) { - enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"*.example.com"}, false) + enforcer := NewEgressEnforcer(http.DefaultTransport, ModeAllowlist, []string{"*.example.com"}, false, nil) var attempts []struct { domain string diff --git a/forge-core/security/egress_proxy.go b/forge-core/security/egress_proxy.go index 3a7f05af..2cf1b861 100644 --- a/forge-core/security/egress_proxy.go +++ b/forge-core/security/egress_proxy.go @@ -47,13 +47,15 @@ type egressIdentity struct { } // NewEgressProxy creates a new EgressProxy that validates domains using the -// given DomainMatcher. Call Start to bind and begin serving. -func NewEgressProxy(matcher *DomainMatcher, allowPrivateIPs bool) *EgressProxy { - sd := NewSafeDialer(nil, allowPrivateIPs) +// given DomainMatcher. Call Start to bind and begin serving. allowedPrivateCIDRs +// narrows the private-IP block: when allowPrivateIPs is false, IPs inside any +// of the listed CIDRs bypass the private block. Pass nil for pre-CIDR defaults. +func NewEgressProxy(matcher *DomainMatcher, allowPrivateIPs bool, allowedPrivateCIDRs []*net.IPNet) *EgressProxy { + sd := NewSafeDialer(nil, allowPrivateIPs, allowedPrivateCIDRs) return &EgressProxy{ matcher: matcher, safeDialer: sd, - safeTransport: NewSafeTransport(nil, allowPrivateIPs), + safeTransport: NewSafeTransport(nil, allowPrivateIPs, allowedPrivateCIDRs), } } diff --git a/forge-core/security/egress_proxy_test.go b/forge-core/security/egress_proxy_test.go index 5ad1e8ff..6d99d2b5 100644 --- a/forge-core/security/egress_proxy_test.go +++ b/forge-core/security/egress_proxy_test.go @@ -25,7 +25,7 @@ func TestEgressProxyAllowedHTTP(t *testing.T) { upstreamURL, _ := url.Parse(upstream.URL) matcher := NewDomainMatcher(ModeAllowlist, []string{upstreamURL.Hostname()}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -58,7 +58,7 @@ func TestEgressProxyAllowedHTTP(t *testing.T) { func TestEgressProxyBlockedHTTP(t *testing.T) { matcher := NewDomainMatcher(ModeAllowlist, []string{"allowed.com"}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -91,7 +91,7 @@ func TestEgressProxyBlockedHTTP(t *testing.T) { func TestEgressProxyLocalhostAlwaysAllowed(t *testing.T) { // Even with deny-all, localhost should pass matcher := NewDomainMatcher(ModeDenyAll, nil) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) // Start a local test server upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -129,7 +129,7 @@ func TestEgressProxyLocalhostAlwaysAllowed(t *testing.T) { func TestEgressProxyCONNECTBlocked(t *testing.T) { matcher := NewDomainMatcher(ModeAllowlist, []string{"allowed.com"}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -168,7 +168,7 @@ func TestEgressProxyCONNECTAllowed(t *testing.T) { host, port, _ := net.SplitHostPort(upstreamURL.Host) matcher := NewDomainMatcher(ModeAllowlist, []string{host}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -210,7 +210,7 @@ func TestEgressProxyDevOpenPassthrough(t *testing.T) { // dev-open should allow everything matcher := NewDomainMatcher(ModeDevOpen, nil) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -249,7 +249,7 @@ func TestEgressProxyOnAttemptCallback(t *testing.T) { upstreamURL, _ := url.Parse(upstream.URL) matcher := NewDomainMatcher(ModeAllowlist, []string{upstreamURL.Hostname()}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) var mu sync.Mutex var calls []EgressAttempt @@ -299,7 +299,7 @@ func TestEgressProxyOnAttemptCallback(t *testing.T) { func TestEgressProxyStop(t *testing.T) { matcher := NewDomainMatcher(ModeDevOpen, nil) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -321,7 +321,7 @@ func TestEgressProxyStop(t *testing.T) { } func TestEgressProxyURL(t *testing.T) { - proxy := NewEgressProxy(NewDomainMatcher(ModeDevOpen, nil), false) + proxy := NewEgressProxy(NewDomainMatcher(ModeDevOpen, nil), false, nil) if proxy.ProxyURL() != "" { t.Error("ProxyURL should be empty before Start") } @@ -395,7 +395,7 @@ func TestEgressProxyAttributesIdentity(t *testing.T) { upstreamURL, _ := url.Parse(upstream.URL) matcher := NewDomainMatcher(ModeAllowlist, []string{upstreamURL.Hostname()}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) var mu sync.Mutex var got EgressAttempt @@ -451,7 +451,7 @@ func TestEgressProxyAttributesIdentityCONNECT(t *testing.T) { host, port, _ := net.SplitHostPort(upstreamURL.Host) matcher := NewDomainMatcher(ModeAllowlist, []string{host}) - proxy := NewEgressProxy(matcher, false) + proxy := NewEgressProxy(matcher, false, nil) var mu sync.Mutex var got EgressAttempt diff --git a/forge-core/security/ip_validator.go b/forge-core/security/ip_validator.go index cdc51ba5..f7fca236 100644 --- a/forge-core/security/ip_validator.go +++ b/forge-core/security/ip_validator.go @@ -55,30 +55,50 @@ func ParseStrictIPv4(s string) net.IP { } // IsBlockedIP checks whether an IP is in a blocked CIDR range. -// When allowPrivate is true, RFC 1918 and link-local ranges are permitted -// (for container/K8s environments), but cloud metadata and loopback are -// always blocked. Returns true (blocked) for nil IPs (fail closed). -func IsBlockedIP(ip net.IP, allowPrivate bool) bool { +// +// Semantics: +// - Always-blocked ranges (cloud metadata, loopback, "this" network) win +// unconditionally — no allowlist punches a hole in them. +// - If allowPrivate is true, RFC 1918 / link-local / CGNAT / IPv6 ULA are +// all permitted (container/K8s posture). +// - Otherwise, private ranges are blocked EXCEPT for IPs that fall inside +// one of allowedPrivateCIDRs. That lets an operator open a narrow slice +// of the private space (e.g. only 10.20.0.0/16) without opening RFC 1918 +// wholesale. +// +// Returns true (blocked) for nil IPs (fail closed). +func IsBlockedIP(ip net.IP, allowPrivate bool, allowedPrivateCIDRs []*net.IPNet) bool { if ip == nil { return true // fail closed } // Check IPv6 transition addresses that embed blocked IPv4 - if isBlockedIPv6Transition(ip, allowPrivate) { + if isBlockedIPv6Transition(ip, allowPrivate, allowedPrivateCIDRs) { return true } + // Always-blocked wins: cloud metadata + loopback + "this" network are + // never reachable, even if an operator adds them to allowedPrivateCIDRs. for _, n := range alwaysBlockedCIDRs { if n.Contains(ip) { return true } } - if !allowPrivate { - for _, n := range privateBlockedCIDRs { - if n.Contains(ip) { - return true - } + if allowPrivate { + return false + } + + // Private-block path: honor the CIDR allowlist first, then fall through. + for _, n := range allowedPrivateCIDRs { + if n.Contains(ip) { + return false + } + } + + for _, n := range privateBlockedCIDRs { + if n.Contains(ip) { + return true } } @@ -87,7 +107,7 @@ func IsBlockedIP(ip net.IP, allowPrivate bool) bool { // isBlockedIPv6Transition detects IPv6 transition addresses (NAT64, 6to4, Teredo) // that embed blocked IPv4 addresses. -func isBlockedIPv6Transition(ip net.IP, allowPrivate bool) bool { +func isBlockedIPv6Transition(ip net.IP, allowPrivate bool, allowedPrivateCIDRs []*net.IPNet) bool { // Ensure we're working with a 16-byte representation ip16 := ip.To16() if ip16 == nil { @@ -102,31 +122,50 @@ func isBlockedIPv6Transition(ip net.IP, allowPrivate bool) bool { nat64Prefix := []byte{0, 0x64, 0xff, 0x9b, 0, 0, 0, 0, 0, 0, 0, 0} if bytesEqual(ip16[:12], nat64Prefix) { embedded := net.IP(ip16[12:16]) - return IsBlockedIP(embedded.To4(), allowPrivate) + return IsBlockedIP(embedded.To4(), allowPrivate, allowedPrivateCIDRs) } // NAT64 extended: 64:ff9b:1::/48 — embedded IPv4 in last 4 bytes nat64ExtPrefix := []byte{0, 0x64, 0xff, 0x9b, 0, 0x01} if bytesEqual(ip16[:6], nat64ExtPrefix) { embedded := net.IP(ip16[12:16]) - return IsBlockedIP(embedded.To4(), allowPrivate) + return IsBlockedIP(embedded.To4(), allowPrivate, allowedPrivateCIDRs) } // 6to4: 2002::/16 — embedded IPv4 in bytes 2-5 if ip16[0] == 0x20 && ip16[1] == 0x02 { embedded := net.IPv4(ip16[2], ip16[3], ip16[4], ip16[5]) - return IsBlockedIP(embedded.To4(), allowPrivate) + return IsBlockedIP(embedded.To4(), allowPrivate, allowedPrivateCIDRs) } // Teredo: 2001:0000::/32 — XOR'd IPv4 in last 4 bytes if ip16[0] == 0x20 && ip16[1] == 0x01 && ip16[2] == 0x00 && ip16[3] == 0x00 { embedded := net.IPv4(ip16[12]^0xff, ip16[13]^0xff, ip16[14]^0xff, ip16[15]^0xff) - return IsBlockedIP(embedded.To4(), allowPrivate) + return IsBlockedIP(embedded.To4(), allowPrivate, allowedPrivateCIDRs) } return false } +// 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 +// range-level exemption, not per-host holes. +func ParsePrivateCIDRs(cidrs []string) ([]*net.IPNet, error) { + if len(cidrs) == 0 { + return nil, nil + } + out := make([]*net.IPNet, 0, len(cidrs)) + for _, s := range cidrs { + _, n, err := net.ParseCIDR(s) + if err != nil { + return nil, fmt.Errorf("invalid CIDR %q: %w", s, err) + } + out = append(out, n) + } + return out, nil +} + func bytesEqual(a, b []byte) bool { if len(a) != len(b) { return false diff --git a/forge-core/security/ip_validator_test.go b/forge-core/security/ip_validator_test.go index ba703eeb..b192f1e4 100644 --- a/forge-core/security/ip_validator_test.go +++ b/forge-core/security/ip_validator_test.go @@ -2,6 +2,7 @@ package security import ( "net" + "strings" "testing" ) @@ -87,9 +88,9 @@ func TestIsBlockedIP(t *testing.T) { if tt.ip != "" { ip = net.ParseIP(tt.ip) } - got := IsBlockedIP(ip, tt.allowPrivate) + got := IsBlockedIP(ip, tt.allowPrivate, nil) if got != tt.blocked { - t.Errorf("IsBlockedIP(%v, %v) = %v, want %v", ip, tt.allowPrivate, got, tt.blocked) + t.Errorf("IsBlockedIP(%v, %v, nil) = %v, want %v", ip, tt.allowPrivate, got, tt.blocked) } }) } @@ -139,14 +140,143 @@ func TestIsBlockedIPv6Transition(t *testing.T) { if ip == nil { t.Fatalf("failed to parse IP %q", tt.ip) } - got := IsBlockedIP(ip, tt.allowPrivate) + got := IsBlockedIP(ip, tt.allowPrivate, nil) if got != tt.blocked { - t.Errorf("IsBlockedIP(%v, %v) = %v, want %v", ip, tt.allowPrivate, got, tt.blocked) + t.Errorf("IsBlockedIP(%v, %v, nil) = %v, want %v", ip, tt.allowPrivate, got, tt.blocked) } }) } } +// mustParseCIDRs is a test helper that panics on invalid input — makes each +// row of a table test one-liner readable without repeating error handling. +func mustParseCIDRs(t *testing.T, cidrs ...string) []*net.IPNet { + t.Helper() + parsed, err := ParsePrivateCIDRs(cidrs) + if err != nil { + t.Fatalf("ParsePrivateCIDRs(%v): %v", cidrs, err) + } + return parsed +} + +// TestIsBlockedIP_PrivateCIDRAllowlist covers the new narrow-private path: +// an operator lists specific private CIDRs to reach (e.g. only 10.20.0.0/16) +// without opening RFC 1918 wholesale. +func TestIsBlockedIP_PrivateCIDRAllowlist(t *testing.T) { + tests := []struct { + name string + ip string + allowPrivate bool + cidrs []*net.IPNet + blocked bool + }{ + { + name: "private IP inside listed CIDR is permitted", + ip: "10.20.0.5", + cidrs: mustParseCIDRs(t, "10.20.0.0/16"), + blocked: false, + }, + { + name: "private IP outside listed CIDR stays blocked", + ip: "10.99.0.5", + cidrs: mustParseCIDRs(t, "10.20.0.0/16"), + blocked: true, + }, + { + name: "unrelated RFC 1918 range not in list stays blocked", + ip: "192.168.1.1", + cidrs: mustParseCIDRs(t, "10.20.0.0/16"), + blocked: true, + }, + { + name: "multiple CIDRs — hit on second one", + ip: "172.16.42.5", + cidrs: mustParseCIDRs(t, "10.0.0.0/8", "172.16.0.0/12"), + blocked: false, + }, + { + name: "allowPrivate=true supersedes CIDR list (all private allowed)", + ip: "192.168.1.1", + allowPrivate: true, + cidrs: nil, + blocked: false, + }, + // Always-blocked ranges MUST NOT be opened via the CIDR list. + { + name: "metadata IP stays blocked even if CIDR list opens it", + ip: "169.254.169.254", + cidrs: mustParseCIDRs(t, "169.254.0.0/16"), + blocked: true, + }, + { + name: "loopback stays blocked even if CIDR list opens it", + ip: "127.0.0.1", + cidrs: mustParseCIDRs(t, "127.0.0.0/8"), + blocked: true, + }, + // Empty CIDR list = pre-CIDR default behavior. + { + name: "nil CIDR list = default block-all-private", + ip: "10.0.0.1", + cidrs: nil, + blocked: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ip := net.ParseIP(tt.ip) + if ip == nil { + t.Fatalf("failed to parse IP %q", tt.ip) + } + got := IsBlockedIP(ip, tt.allowPrivate, tt.cidrs) + if got != tt.blocked { + t.Errorf("IsBlockedIP(%v, %v, %v) = %v, want %v", + ip, tt.allowPrivate, tt.cidrs, got, tt.blocked) + } + }) + } +} + +func TestParsePrivateCIDRs(t *testing.T) { + t.Run("nil input returns nil", func(t *testing.T) { + got, err := ParsePrivateCIDRs(nil) + if err != nil { + t.Fatalf("ParsePrivateCIDRs(nil): %v", err) + } + if got != nil { + t.Errorf("expected nil, got %v", got) + } + }) + + t.Run("valid CIDRs parse", func(t *testing.T) { + got, err := ParsePrivateCIDRs([]string{"10.0.0.0/8", "172.16.0.0/12"}) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if len(got) != 2 { + t.Fatalf("expected 2 CIDRs, got %d", len(got)) + } + }) + + t.Run("invalid CIDR errors with the offending value", func(t *testing.T) { + _, err := ParsePrivateCIDRs([]string{"10.0.0.0/8", "not-a-cidr"}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "not-a-cidr") { + t.Errorf("error should name the bad CIDR, got: %v", err) + } + }) + + t.Run("bare IP without /mask is rejected", func(t *testing.T) { + _, err := ParsePrivateCIDRs([]string{"10.0.0.1"}) + if err == nil { + t.Fatal("bare IPs must be rejected — the config takes CIDR ranges") + } + }) +} + func TestValidateHostIP(t *testing.T) { tests := []struct { name string diff --git a/forge-core/security/resolver.go b/forge-core/security/resolver.go index 03ae2873..e2d9d627 100644 --- a/forge-core/security/resolver.go +++ b/forge-core/security/resolver.go @@ -11,8 +11,11 @@ func DefaultProfile() EgressProfile { return ProfileStrict } // DefaultMode returns the default egress mode. func DefaultMode() EgressMode { return ModeDenyAll } -// Resolve builds an EgressConfig from profile, mode, explicit domains, tool names, and capabilities. -func Resolve(profile, mode string, explicitDomains, toolNames, capabilities []string) (*EgressConfig, error) { +// Resolve builds an EgressConfig from profile, mode, explicit domains, tool +// names, capabilities, and (optionally) the raw allowed_private_cidrs list +// from forge.yaml. CIDR entries are validated here so a bad string fails at +// config-load time, not at first-dial. Pass nil for pre-CIDR defaults. +func Resolve(profile, mode string, explicitDomains, toolNames, capabilities, allowedPrivateCIDRs []string) (*EgressConfig, error) { p := EgressProfile(profile) if p == "" { p = DefaultProfile() @@ -29,9 +32,18 @@ func Resolve(profile, mode string, explicitDomains, toolNames, capabilities []st return nil, err } + // Validate CIDRs early. We throw away the parsed value here; callers + // re-parse via ParsePrivateCIDRs when they need []*net.IPNet. This keeps + // EgressConfig JSON-serializable (net.IPNet is not) while still failing + // closed on invalid config. + if _, err := ParsePrivateCIDRs(allowedPrivateCIDRs); err != nil { + return nil, fmt.Errorf("egress: %w", err) + } + cfg := &EgressConfig{ - Profile: p, - Mode: m, + Profile: p, + Mode: m, + AllowedPrivateCIDRs: allowedPrivateCIDRs, } switch m { diff --git a/forge-core/security/resolver_test.go b/forge-core/security/resolver_test.go index 72dbc9cf..4039b6e6 100644 --- a/forge-core/security/resolver_test.go +++ b/forge-core/security/resolver_test.go @@ -5,7 +5,7 @@ import ( ) func TestResolve_DenyAll(t *testing.T) { - cfg, err := Resolve("strict", "deny-all", nil, nil, nil) + cfg, err := Resolve("strict", "deny-all", nil, nil, nil, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -21,7 +21,7 @@ func TestResolve_DenyAll(t *testing.T) { } func TestResolve_DevOpen(t *testing.T) { - cfg, err := Resolve("permissive", "dev-open", nil, nil, nil) + cfg, err := Resolve("permissive", "dev-open", nil, nil, nil, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -34,7 +34,7 @@ func TestResolve_Allowlist(t *testing.T) { explicit := []string{"api.example.com", "data.example.com"} tools := []string{"web_search", "github_api"} - cfg, err := Resolve("standard", "allowlist", explicit, tools, nil) + cfg, err := Resolve("standard", "allowlist", explicit, tools, nil, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -62,7 +62,7 @@ func TestResolve_Allowlist(t *testing.T) { } func TestResolve_Defaults(t *testing.T) { - cfg, err := Resolve("", "", nil, nil, nil) + cfg, err := Resolve("", "", nil, nil, nil, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -75,14 +75,14 @@ func TestResolve_Defaults(t *testing.T) { } func TestResolve_InvalidProfile(t *testing.T) { - _, err := Resolve("invalid", "deny-all", nil, nil, nil) + _, err := Resolve("invalid", "deny-all", nil, nil, nil, nil) if err == nil { t.Fatal("expected error for invalid profile") } } func TestResolve_InvalidMode(t *testing.T) { - _, err := Resolve("strict", "invalid", nil, nil, nil) + _, err := Resolve("strict", "invalid", nil, nil, nil, nil) if err == nil { t.Fatal("expected error for invalid mode") } @@ -115,7 +115,7 @@ func TestInferToolDomains_Unknown(t *testing.T) { func TestResolve_AllowlistWithCapabilities(t *testing.T) { explicit := []string{"api.example.com"} - cfg, err := Resolve("standard", "allowlist", explicit, nil, []string{"slack"}) + cfg, err := Resolve("standard", "allowlist", explicit, nil, []string{"slack"}, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -143,7 +143,7 @@ func TestResolve_AllowlistWithCapabilities(t *testing.T) { } func TestResolve_AllowlistTelegramCapability(t *testing.T) { - cfg, err := Resolve("standard", "allowlist", nil, nil, []string{"telegram"}) + cfg, err := Resolve("standard", "allowlist", nil, nil, []string{"telegram"}, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -161,7 +161,7 @@ func TestResolve_AllowlistTelegramCapability(t *testing.T) { } func TestResolve_CapabilitiesIgnoredForDenyAll(t *testing.T) { - cfg, err := Resolve("strict", "deny-all", nil, nil, []string{"slack"}) + cfg, err := Resolve("strict", "deny-all", nil, nil, []string{"slack"}, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -169,3 +169,36 @@ func TestResolve_CapabilitiesIgnoredForDenyAll(t *testing.T) { t.Errorf("AllDomains should be empty for deny-all, got %v", cfg.AllDomains) } } + +// TestResolve_ValidatesAllowedPrivateCIDRs pins the fail-closed behavior: +// a bad CIDR must trip config load, not defer failure to the first dial. +func TestResolve_ValidatesAllowedPrivateCIDRs(t *testing.T) { + t.Run("valid CIDRs pass through", func(t *testing.T) { + cfg, err := Resolve("standard", "allowlist", nil, nil, nil, + []string{"10.20.0.0/16", "172.16.42.0/24"}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if len(cfg.AllowedPrivateCIDRs) != 2 { + t.Errorf("AllowedPrivateCIDRs = %v, want 2 entries", cfg.AllowedPrivateCIDRs) + } + }) + + t.Run("invalid CIDR fails Resolve", func(t *testing.T) { + _, err := Resolve("standard", "allowlist", nil, nil, nil, + []string{"10.20.0.0/16", "not-a-cidr"}) + if err == nil { + t.Fatal("expected error for invalid CIDR") + } + }) + + t.Run("nil CIDR list is fine", func(t *testing.T) { + cfg, err := Resolve("strict", "deny-all", nil, nil, nil, nil) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if cfg.AllowedPrivateCIDRs != nil { + t.Errorf("expected nil AllowedPrivateCIDRs, got %v", cfg.AllowedPrivateCIDRs) + } + }) +} diff --git a/forge-core/security/safe_dialer.go b/forge-core/security/safe_dialer.go index ae03d7af..7b222bcb 100644 --- a/forge-core/security/safe_dialer.go +++ b/forge-core/security/safe_dialer.go @@ -16,22 +16,28 @@ type Resolver interface { // SafeDialer validates resolved IPs before establishing connections, // preventing DNS rebinding and SSRF via post-resolution checks. type SafeDialer struct { - resolver Resolver - dialer net.Dialer - allowPrivate bool + resolver Resolver + dialer net.Dialer + allowPrivate bool + allowedPrivateCIDRs []*net.IPNet } // NewSafeDialer creates a SafeDialer. If resolver is nil, net.DefaultResolver // is used. Set allowPrivateIPs to true in container environments where RFC 1918 -// addresses are used for inter-service communication. -func NewSafeDialer(resolver Resolver, allowPrivateIPs bool) *SafeDialer { +// addresses are used for inter-service communication. allowedPrivateCIDRs is +// a narrower alternative: when allowPrivateIPs is false, IPs falling inside +// any of these CIDRs bypass the private-block (but always-blocked ranges — +// cloud metadata, loopback — still win unconditionally). Pass nil for the +// pre-CIDR default behavior. +func NewSafeDialer(resolver Resolver, allowPrivateIPs bool, allowedPrivateCIDRs []*net.IPNet) *SafeDialer { if resolver == nil { resolver = net.DefaultResolver } return &SafeDialer{ - resolver: resolver, - dialer: net.Dialer{Timeout: 10 * time.Second}, - allowPrivate: allowPrivateIPs, + resolver: resolver, + dialer: net.Dialer{Timeout: 10 * time.Second}, + allowPrivate: allowPrivateIPs, + allowedPrivateCIDRs: allowedPrivateCIDRs, } } @@ -51,7 +57,7 @@ func (s *SafeDialer) SafeDialContext(ctx context.Context, network, addr string) // If it's an IP literal, validate and dial directly if ip := net.ParseIP(host); ip != nil { - if IsBlockedIP(ip, s.allowPrivate) { + if IsBlockedIP(ip, s.allowPrivate, s.allowedPrivateCIDRs) { return nil, fmt.Errorf("safe dialer: blocked IP %s", ip) } return s.dialer.DialContext(ctx, network, addr) @@ -68,7 +74,7 @@ func (s *SafeDialer) SafeDialContext(ctx context.Context, network, addr string) // Validate ALL resolved IPs — reject if ANY is blocked for _, a := range addrs { - if IsBlockedIP(a.IP, s.allowPrivate) { + if IsBlockedIP(a.IP, s.allowPrivate, s.allowedPrivateCIDRs) { return nil, fmt.Errorf("safe dialer: domain %q resolved to blocked IP %s", host, a.IP) } } @@ -79,9 +85,10 @@ func (s *SafeDialer) SafeDialContext(ctx context.Context, network, addr string) } // NewSafeTransport creates an http.Transport that uses SafeDialer for all -// connections. If resolver is nil, net.DefaultResolver is used. -func NewSafeTransport(resolver Resolver, allowPrivateIPs bool) *http.Transport { - sd := NewSafeDialer(resolver, allowPrivateIPs) +// connections. If resolver is nil, net.DefaultResolver is used. See +// NewSafeDialer for the semantics of allowedPrivateCIDRs. +func NewSafeTransport(resolver Resolver, allowPrivateIPs bool, allowedPrivateCIDRs []*net.IPNet) *http.Transport { + sd := NewSafeDialer(resolver, allowPrivateIPs, allowedPrivateCIDRs) return &http.Transport{ DialContext: sd.SafeDialContext, MaxIdleConns: 100, diff --git a/forge-core/security/safe_dialer_test.go b/forge-core/security/safe_dialer_test.go index b1df1c22..6d5c233c 100644 --- a/forge-core/security/safe_dialer_test.go +++ b/forge-core/security/safe_dialer_test.go @@ -30,7 +30,7 @@ func TestSafeDialerBlocksPrivateResolution(t *testing.T) { }, } - sd := NewSafeDialer(resolver, false) + sd := NewSafeDialer(resolver, false, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "internal.example.com:80") if err == nil { t.Fatal("expected error for domain resolving to private IP") @@ -47,7 +47,7 @@ func TestSafeDialerAllowsPrivateWhenConfigured(t *testing.T) { }, } - sd := NewSafeDialer(resolver, true) + sd := NewSafeDialer(resolver, true, nil) // This will fail at the dial stage (no actual service), but should // pass IP validation _, err := sd.SafeDialContext(context.Background(), "tcp", "service.cluster.local:80") @@ -67,7 +67,7 @@ func TestSafeDialerBlocksMetadataAlways(t *testing.T) { } // Even with allowPrivate=true, metadata must be blocked - sd := NewSafeDialer(resolver, true) + sd := NewSafeDialer(resolver, true, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "metadata.internal:80") if err == nil { t.Fatal("expected error for metadata IP") @@ -84,7 +84,7 @@ func TestSafeDialerBlocksLoopbackAlways(t *testing.T) { }, } - sd := NewSafeDialer(resolver, true) + sd := NewSafeDialer(resolver, true, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "loopback.example.com:80") if err == nil { t.Fatal("expected error for loopback IP") @@ -104,7 +104,7 @@ func TestSafeDialerBlocksMixedIPs(t *testing.T) { }, } - sd := NewSafeDialer(resolver, false) + sd := NewSafeDialer(resolver, false, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "mixed.example.com:80") if err == nil { t.Fatal("expected error when any resolved IP is blocked") @@ -119,7 +119,7 @@ func TestSafeDialerDNSFailure(t *testing.T) { addrs: map[string][]net.IPAddr{}, } - sd := NewSafeDialer(resolver, false) + sd := NewSafeDialer(resolver, false, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "nonexistent.example.com:80") if err == nil { t.Fatal("expected error for DNS failure") @@ -127,7 +127,7 @@ func TestSafeDialerDNSFailure(t *testing.T) { } func TestSafeDialerIPLiteral(t *testing.T) { - sd := NewSafeDialer(nil, false) + sd := NewSafeDialer(nil, false, nil) // Blocked IP literal _, err := sd.SafeDialContext(context.Background(), "tcp", "169.254.169.254:80") @@ -140,7 +140,7 @@ func TestSafeDialerIPLiteral(t *testing.T) { } func TestSafeDialerRejectsNonStandardIP(t *testing.T) { - sd := NewSafeDialer(nil, false) + sd := NewSafeDialer(nil, false, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "0x7f000001:80") if err == nil { @@ -152,10 +152,83 @@ func TestSafeDialerRejectsNonStandardIP(t *testing.T) { } func TestSafeDialerInvalidAddress(t *testing.T) { - sd := NewSafeDialer(nil, false) + sd := NewSafeDialer(nil, false, nil) _, err := sd.SafeDialContext(context.Background(), "tcp", "no-port") if err == nil { t.Fatal("expected error for invalid address") } } + +// TestSafeDialer_PrivateCIDRAllowlist covers the vend-time integration: +// a hostname resolving into a private CIDR listed under allowedPrivateCIDRs +// should pass the IP check (dial still fails on unreachable target, which +// is fine — we only assert we get past the block, not that the connection +// succeeds). +func TestSafeDialer_PrivateCIDRAllowlist(t *testing.T) { + cidrs, err := ParsePrivateCIDRs([]string{"10.20.0.0/16"}) + if err != nil { + t.Fatalf("ParsePrivateCIDRs: %v", err) + } + + t.Run("resolved IP inside listed CIDR passes IP check", func(t *testing.T) { + resolver := &mockResolver{ + addrs: map[string][]net.IPAddr{ + "db.internal": {{IP: net.ParseIP("10.20.0.5")}}, + }, + } + sd := NewSafeDialer(resolver, false, cidrs) + _, err := sd.SafeDialContext(context.Background(), "tcp", "db.internal:5432") + // Dial will fail (nothing listening at 10.20.0.5), but the failure + // must not be from the IP block — that's the invariant we care about. + if err != nil && strings.Contains(err.Error(), "blocked IP") { + t.Errorf("CIDR-listed private IP should pass IP check, got: %v", err) + } + }) + + t.Run("resolved IP outside listed CIDR stays blocked", func(t *testing.T) { + resolver := &mockResolver{ + addrs: map[string][]net.IPAddr{ + "other.internal": {{IP: net.ParseIP("10.99.0.5")}}, + }, + } + sd := NewSafeDialer(resolver, false, cidrs) + _, err := sd.SafeDialContext(context.Background(), "tcp", "other.internal:5432") + if err == nil { + t.Fatal("expected block for private IP outside CIDR list") + } + if !strings.Contains(err.Error(), "blocked IP") { + t.Errorf("expected 'blocked IP' error, got: %v", err) + } + }) + + t.Run("cloud metadata cannot be opened via CIDR list", func(t *testing.T) { + // Even if an operator lists 169.254.0.0/16, metadata IP stays blocked. + metadataCIDRs, err := ParsePrivateCIDRs([]string{"169.254.0.0/16"}) + if err != nil { + t.Fatalf("ParsePrivateCIDRs: %v", err) + } + resolver := &mockResolver{ + addrs: map[string][]net.IPAddr{ + "metadata": {{IP: net.ParseIP("169.254.169.254")}}, + }, + } + sd := NewSafeDialer(resolver, false, metadataCIDRs) + _, err = sd.SafeDialContext(context.Background(), "tcp", "metadata:80") + if err == nil { + t.Fatal("metadata IP must NEVER be reachable via CIDR list") + } + if !strings.Contains(err.Error(), "blocked IP") { + t.Errorf("expected 'blocked IP' error, got: %v", err) + } + }) + + t.Run("IP literal inside listed CIDR passes IP check", func(t *testing.T) { + // This exercises the ParseIP path (no DNS lookup). + sd := NewSafeDialer(nil, false, cidrs) + _, err := sd.SafeDialContext(context.Background(), "tcp", "10.20.0.5:5432") + if err != nil && strings.Contains(err.Error(), "blocked IP") { + t.Errorf("CIDR-listed private IP literal should pass, got: %v", err) + } + }) +} diff --git a/forge-core/security/types.go b/forge-core/security/types.go index b2526135..64420021 100644 --- a/forge-core/security/types.go +++ b/forge-core/security/types.go @@ -27,4 +27,8 @@ type EgressConfig struct { ToolDomains []string `json:"tool_domains,omitempty"` // inferred from tools AllDomains []string `json:"all_domains,omitempty"` // deduplicated union AllowPrivateIPs bool `json:"allow_private_ips,omitempty"` + // AllowedPrivateCIDRs is the resolved list of CIDR strings. Callers + // should run these through ParsePrivateCIDRs to obtain the []*net.IPNet + // the SafeDialer/EgressProxy constructors expect. + AllowedPrivateCIDRs []string `json:"allowed_private_cidrs,omitempty"` } diff --git a/forge-core/types/config.go b/forge-core/types/config.go index 76e554da..e36f2c36 100644 --- a/forge-core/types/config.go +++ b/forge-core/types/config.go @@ -871,6 +871,12 @@ type EgressRef struct { AllowedDomains []string `yaml:"allowed_domains,omitempty"` Capabilities []string `yaml:"capabilities,omitempty"` // capability bundles (e.g., "slack", "telegram") AllowPrivateIPs *bool `yaml:"allow_private_ips,omitempty"` + // AllowedPrivateCIDRs opens a narrow slice of the private-IP space + // (e.g. "10.20.0.0/16") without opening RFC 1918 wholesale via + // AllowPrivateIPs. Ignored when AllowPrivateIPs is true (that already + // permits all private ranges). Always-blocked ranges (cloud metadata, + // loopback) remain blocked regardless of what's listed here. + AllowedPrivateCIDRs []string `yaml:"allowed_private_cidrs,omitempty"` } // SkillsRef references a skills definition file. diff --git a/go.work.sum b/go.work.sum index a9e6653f..f4542c48 100644 --- a/go.work.sum +++ b/go.work.sum @@ -33,17 +33,17 @@ github.com/peterbourgon/diskv v2.0.1+incompatible/go.mod h1:uqqh8zWWbv1HBMNONnaR github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/sahilm/fuzzy v0.1.1/go.mod h1:VFvziUEIMCrT6A6tw2RFIXPXXmzXbOsSHF0DOI8ZK9Y= -github.com/spf13/pflag v1.0.10 h1:4EBh2KAYBwaONj6b2Ye1GiHfwjqyROoF4RwYO+vPwFk= -github.com/spf13/pflag v1.0.10/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spiffe/go-spiffe/v2 v2.6.0/go.mod h1:gm2SeUoMZEtpnzPNs2Csc0D/gX33k1xIx7lEzqblHEs= go.opentelemetry.io/contrib/detectors/gcp v1.42.0/go.mod h1:W9zQ439utxymRrXsUOzZbFX4JhLxXU4+ZnCt8GG7yA8= go.yaml.in/yaml/v2 v2.4.3 h1:6gvOSjQoTB3vt1l+CU+tSyi/HOjfOjRLJ4YwYZGwRO0= go.yaml.in/yaml/v2 v2.4.3/go.mod h1:zSxWcmIDjOzPXpjlTTbAsKokqkDNAVtZO0WOMiT90s8= +golang.org/x/crypto v0.51.0/go.mod h1:8AdwkbraGNABw2kOX6YFPs3WM22XqI4EXEd8g+x7Oc8= golang.org/x/mod v0.32.0/go.mod h1:SgipZ/3h2Ci89DlEtEXWUk/HteuRin+HHhN+WbNhguU= golang.org/x/mod v0.35.0/go.mod h1:+GwiRhIInF8wPm+4AoT6L0FA1QWAad3OMdTRx4tFYlU= golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= golang.org/x/net v0.49.0/go.mod h1:/ysNB2EvaqvesRkuLAyjI1ycPZlQHM3q01F02UY/MV8= golang.org/x/net v0.53.0/go.mod h1:JvMuJH7rrdiCfbeHoo3fCQU24Lf5JJwT9W3sJFulfgs= +golang.org/x/net v0.54.0/go.mod h1:Sj4oj8jK6XmHpBZU/zWHw3BV3abl4Kvi+Ut7cQcY+cQ= golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/telemetry v0.0.0-20260109210033-bd525da824e2/go.mod h1:b7fPSJ0pKZ3ccUh8gnTONJxhn3c/PS6tyzQvyqw4iA8= golang.org/x/telemetry v0.0.0-20260409153401-be6f6cb8b1fa/go.mod h1:kHjTxDEnAu6/Nl9lDkzjWpR+bmKfxeiRuSDlsMb70gE= From e3c0c4061da5460d2564ce24109ea4ffde8fe325 Mon Sep 17 00:00:00 2001 From: naveen-kurra Date: Tue, 21 Jul 2026 14:20:31 -0400 Subject: [PATCH 2/3] =?UTF-8?q?fix(security):=20#348=20review=20nits=20?= =?UTF-8?q?=E2=80=94=20build-time=20validation=20+=20reject=20non-canonica?= =?UTF-8?q?l=20CIDRs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/initializ/forge/pull/348#pullrequestreview-461725977 --- docs/security/egress-control.md | 3 +- forge-cli/build/egress_stage.go | 9 +++- forge-core/security/ip_validator.go | 24 ++++++++++- forge-core/security/ip_validator_test.go | 54 ++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 3 deletions(-) diff --git a/docs/security/egress-control.md b/docs/security/egress-control.md index a1202014..4a82d56a 100644 --- a/docs/security/egress-control.md +++ b/docs/security/egress-control.md @@ -128,7 +128,8 @@ Invariants (checked by tests): - **Always-blocked ranges (cloud metadata `169.254.169.254`, loopback, `0.0.0.0/8`) stay blocked** even if an operator lists a CIDR that would otherwise contain them. No allowlist punches a hole in these. - **`allow_private_ips: true` supersedes** the CIDR list — the boolean means "all private allowed" and the list is redundant. A warning in that case is a good future addition, but the effective policy is safe (superset). -- **Invalid CIDR strings fail at config-load time**, not at first dial. `Resolve()` validates each entry via `net.ParseCIDR`; bare IPs (missing `/mask`) are rejected because the config intent is range-level exemption. +- **Invalid CIDR strings fail at config-load time**, not at first dial. `Resolve()` validates each entry via `net.ParseCIDR`; bare IPs (missing `/mask`) are rejected because the config intent is range-level exemption. **Non-canonical entries with host bits set** (e.g. `10.20.0.5/16`) are also rejected — Go's `net.ParseCIDR` silently masks those to the network (widening from a single host to a /16), which is the security-wrong direction. Write `10.20.0.5/32` (single host) or `10.20.0.0/16` (range) — never the ambiguous mix. +- **Both `forge build` and `forge run` validate the CIDR strings** — a typo trips the build, not just the first launch. - CIDRs are consulted by both the in-process `EgressEnforcer` (Go clients) and the subprocess `EgressProxy` (skill scripts) — one policy, two enforcement paths. Companion feature for #337: raw-TCP egress (databases, message brokers) requires this narrow-private mechanism to be reachable in the first place. Landing the CIDR allowlist first (this PR) unblocks the SOCKS5-based TCP path (follow-up PR) without opening RFC 1918 wholesale. diff --git a/forge-cli/build/egress_stage.go b/forge-cli/build/egress_stage.go index b7e2e080..aae3deb8 100644 --- a/forge-cli/build/egress_stage.go +++ b/forge-cli/build/egress_stage.go @@ -51,7 +51,14 @@ func (s *EgressStage) Execute(ctx context.Context, bc *pipeline.BuildContext) er // time out depending on which side notices first. allowed = append(allowed, security.LLMProviderDomains(bc.Config)...) - resolved, err := security.Resolve(cfg.Profile, cfg.Mode, allowed, toolNames, cfg.Capabilities, nil) + // Pass allowed_private_cidrs through so `forge build` validates the + // strings at build time (same fail-loud posture as `forge run`). The + // build artifacts themselves (`GenerateAllowlistJSON`, + // `GenerateK8sNetworkPolicy`) don't consume the CIDRs — runtime IP + // enforcement re-resolves from raw config — but a typo in + // `allowed_private_cidrs` should fail the build, not sail through and + // trip only on first `forge run`. (#348 review nit 1.) + resolved, err := security.Resolve(cfg.Profile, cfg.Mode, allowed, toolNames, cfg.Capabilities, cfg.AllowedPrivateCIDRs) if err != nil { return fmt.Errorf("resolving egress: %w", err) } diff --git a/forge-core/security/ip_validator.go b/forge-core/security/ip_validator.go index f7fca236..43d75f2e 100644 --- a/forge-core/security/ip_validator.go +++ b/forge-core/security/ip_validator.go @@ -151,21 +151,43 @@ func isBlockedIPv6Transition(ip net.IP, allowPrivate bool, allowedPrivateCIDRs [ // 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 // range-level exemption, not per-host holes. +// +// Non-canonical entries with host bits set (e.g. "10.20.0.5/16") are +// rejected too. `net.ParseCIDR` silently masks those to the network +// (10.20.0.0/16), which is the "wider than intended" direction — an operator +// who wrote "10.20.0.5/16" expecting a single host would instead get the +// whole /16 allowed. Failing loud here forces the operator to either write +// "10.20.0.5/32" (single host — explicit) or "10.20.0.0/16" (the range they +// really meant). #348 review nit 2. func ParsePrivateCIDRs(cidrs []string) ([]*net.IPNet, error) { if len(cidrs) == 0 { return nil, nil } out := make([]*net.IPNet, 0, len(cidrs)) for _, s := range cidrs { - _, n, err := net.ParseCIDR(s) + ip, n, err := net.ParseCIDR(s) if err != nil { return nil, fmt.Errorf("invalid CIDR %q: %w", s, err) } + if !ip.Equal(n.IP) { + return nil, fmt.Errorf("non-canonical CIDR %q: host bits are set — use %q for the range or %q for a single host", + s, n.String(), ip.String()+singleHostMask(ip)) + } out = append(out, n) } return out, nil } +// singleHostMask returns the /32 (IPv4) or /128 (IPv6) suffix that turns a +// bare IP into a single-host CIDR. Used only in error messages so the fix +// the operator needs is obvious in the log line. +func singleHostMask(ip net.IP) string { + if ip.To4() != nil { + return "/32" + } + return "/128" +} + func bytesEqual(a, b []byte) bool { if len(a) != len(b) { return false diff --git a/forge-core/security/ip_validator_test.go b/forge-core/security/ip_validator_test.go index b192f1e4..097d3c78 100644 --- a/forge-core/security/ip_validator_test.go +++ b/forge-core/security/ip_validator_test.go @@ -275,6 +275,60 @@ func TestParsePrivateCIDRs(t *testing.T) { t.Fatal("bare IPs must be rejected — the config takes CIDR ranges") } }) + + // #348 review nit 2 — silent widening was the concrete risk: + // net.ParseCIDR("10.20.0.5/16") returns 10.20.0.0/16 without error, + // which turns an operator's "single host" intent into a whole /16. + // We reject those explicitly and the error message names both fixes + // (the range and the /32 single-host form) so the log line is + // actionable without a second look at the code. + t.Run("non-canonical CIDR with host bits set is rejected (IPv4)", func(t *testing.T) { + _, err := ParsePrivateCIDRs([]string{"10.20.0.5/16"}) + if err == nil { + t.Fatal("host-bits-set CIDR must be rejected (silent widening prevention)") + } + // Error should name both the widened range and the single-host form. + msg := err.Error() + if !strings.Contains(msg, "10.20.0.5/16") { + t.Errorf("error should quote the bad input, got: %v", err) + } + if !strings.Contains(msg, "10.20.0.0/16") { + t.Errorf("error should suggest the range fix, got: %v", err) + } + if !strings.Contains(msg, "10.20.0.5/32") { + t.Errorf("error should suggest the single-host fix, got: %v", err) + } + }) + + t.Run("non-canonical CIDR with host bits set is rejected (IPv6)", func(t *testing.T) { + _, err := ParsePrivateCIDRs([]string{"2001:db8::1/32"}) + if err == nil { + t.Fatal("host-bits-set IPv6 CIDR must be rejected") + } + if !strings.Contains(err.Error(), "/128") { + t.Errorf("IPv6 error should suggest /128 single-host form, got: %v", err) + } + }) + + t.Run("canonical CIDR (network address) is accepted", func(t *testing.T) { + got, err := ParsePrivateCIDRs([]string{"10.20.0.0/16", "2001:db8::/32"}) + if err != nil { + t.Fatalf("canonical entries should pass: %v", err) + } + if len(got) != 2 { + t.Fatalf("expected 2 CIDRs, got %d", len(got)) + } + }) + + t.Run("single-host /32 is accepted (canonical)", func(t *testing.T) { + got, err := ParsePrivateCIDRs([]string{"10.20.0.5/32"}) + if err != nil { + t.Fatalf("single-host /32 should pass: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 CIDR, got %d", len(got)) + } + }) } func TestValidateHostIP(t *testing.T) { From 7c780d5dbdeb08791d273a04c76ab999f6286c2b Mon Sep 17 00:00:00 2001 From: naveen-kurra Date: Tue, 21 Jul 2026 14:21:12 -0400 Subject: [PATCH 3/3] docs(security): note the 0.0.0.0/0 aside in allowed_private_cidrs (#348) --- docs/security/egress-control.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/security/egress-control.md b/docs/security/egress-control.md index 4a82d56a..12997fe9 100644 --- a/docs/security/egress-control.md +++ b/docs/security/egress-control.md @@ -130,6 +130,7 @@ Invariants (checked by tests): - **`allow_private_ips: true` supersedes** the CIDR list — the boolean means "all private allowed" and the list is redundant. A warning in that case is a good future addition, but the effective policy is safe (superset). - **Invalid CIDR strings fail at config-load time**, not at first dial. `Resolve()` validates each entry via `net.ParseCIDR`; bare IPs (missing `/mask`) are rejected because the config intent is range-level exemption. **Non-canonical entries with host bits set** (e.g. `10.20.0.5/16`) are also rejected — Go's `net.ParseCIDR` silently masks those to the network (widening from a single host to a /16), which is the security-wrong direction. Write `10.20.0.5/32` (single host) or `10.20.0.0/16` (range) — never the ambiguous mix. - **Both `forge build` and `forge run` validate the CIDR strings** — a typo trips the build, not just the first launch. +- **`0.0.0.0/0` is not restricted.** Listing the whole IPv4 space here is equivalent to `allow_private_ips: true` in effect — cloud metadata / loopback / `0.0.0.0/8` still can't be reached because always-blocked wins. It's an explicit operator choice, not a hole, but if you find yourself writing it you probably want `allow_private_ips: true` for the same effect + clearer intent. - CIDRs are consulted by both the in-process `EgressEnforcer` (Go clients) and the subprocess `EgressProxy` (skill scripts) — one policy, two enforcement paths. Companion feature for #337: raw-TCP egress (databases, message brokers) requires this narrow-private mechanism to be reachable in the first place. Landing the CIDR allowlist first (this PR) unblocks the SOCKS5-based TCP path (follow-up PR) without opening RFC 1918 wholesale.