diff --git a/docs/security/egress-control.md b/docs/security/egress-control.md index 12997fe..5adb3f5 100644 --- a/docs/security/egress-control.md +++ b/docs/security/egress-control.md @@ -135,6 +135,89 @@ Invariants (checked by tests): 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. +## Raw-TCP Egress (SOCKS5) + +HTTP(S) traffic goes through the HTTP forward proxy; **raw TCP** (Postgres, MySQL, Redis, Kafka, RabbitMQ, MongoDB, NATS, MQTT) goes through a **SOCKS5 listener** the same proxy binds. Both use the same enforcement engine (`ValidateAndDial`) — one allowlist policy, one audit shape, two client-facing protocols. + +**Config:** + +```yaml +egress: + mode: allowlist + allowed_hosts: [api.stripe.com] # HTTP/HTTPS — unchanged + allowed_tcp: # raw-TCP allowlist + - db.internal:5432 + - "*.brokers.internal:9092" # wildcard host, exact port + - metrics.internal:* # exact host, any port + - redis.internal:6379 + allowed_private_cidrs: # required for internal targets + - 10.20.0.0/16 +``` + +**Entry shapes:** + +| Shape | Example | Matches | +|---|---|---| +| `host:port` | `db.internal:5432` | `db.internal:5432` only | +| `*.suffix:port` | `*.brokers.internal:9092` | `broker1.brokers.internal:9092`, `cluster-a.brokers.internal:9092` (parent `brokers.internal` NOT matched) | +| `host:*` | `metrics.internal:*` | any port on `metrics.internal` | +| `*.suffix:*` | `*.internal:*` | any port on any subdomain of `.internal` | + +Bare host (no `:port`) is rejected at config-load. Ports outside 1–65535 are rejected. HTTP-side `allowed_hosts` entries are ALSO reachable via SOCKS5 (either matcher can allow a target) — no need to duplicate. + +**Read `allowed_hosts` carefully once SOCKS5 is on.** The HTTP-side allowlist is **port-agnostic**: a hostname listed in `allowed_hosts` for HTTPS use is reachable over SOCKS5 on *any* port. Listing `api.stripe.com` for HTTPS also grants `api.stripe.com:22`, `api.stripe.com:3389`, etc. via the raw-TCP path. SafeDialer still bounds the IPs it resolves to (no cloud-metadata, no unlisted-private), so it's not an SSRF hole — but it may be wider than the operator's HTTPS-only mental model. If you need port-narrow control on an HTTP hostname, remove it from `allowed_hosts` and add explicit `allowed_tcp: [api.stripe.com:443]` instead. Matches pre-existing HTTP CONNECT behavior (CONNECT also checks hostname-only), so this isn't newly introduced — but it's newly exposed to arbitrary TCP protocols. + +**IPv6 targets** must be bracketed in the config (`[::1]:5432` or `[2001:db8::1]:6379`). Wildcard hosts (`*.suffix`) are IPv4-hostname patterns — IPv6 literal wildcards aren't supported (there's no meaningful "suffix" for an IP literal). + +**Env injection for skill subprocesses:** + +The runner sets these env vars alongside the existing `HTTP_PROXY`/`HTTPS_PROXY`: + +```sh +ALL_PROXY=socks5h://127.0.0.1: +all_proxy=socks5h://127.0.0.1: +SOCKS_PROXY=socks5h://127.0.0.1: +``` + +The `socks5h://` scheme (with the `h`) forces **server-side hostname resolution** — the proxy needs the destination hostname to run the allowlist check and record it in the audit event. Clients that pre-resolve locally to an IP would launder the target past the domain matcher. + +**Client support matrix:** + +| Client | Native SOCKS5? | How to reach the proxy | +|---|---|---| +| `curl` | Yes | `curl --socks5-hostname "$ALL_PROXY" …` | +| Go apps | Yes | `golang.org/x/net/proxy` reads `ALL_PROXY` automatically | +| Python (`psycopg2`, `redis-py`, `pymongo`) | Partial | `pip install pysocks`; `socks.set_default_proxy(socks.SOCKS5, …)` | +| Node (`pg`, `ioredis`, `mongodb`) | With shim | `npm i socks-proxy-agent`; construct the client with the agent | +| `psql`, `redis-cli`, `mongosh`, `kafka-console-consumer` | **No** | Wrap in `proxychains-ng` — see caveats below | + +**`proxychains-ng` platform caveats:** + +| Platform | Works? | Notes | +|---|---|---| +| Linux (glibc) | ✓ | `LD_PRELOAD` — install via package manager | +| Linux (musl / Alpine) | ✓ | Requires proxychains built for musl | +| macOS | ✗ (mostly) | `DYLD_INSERT_LIBRARIES` blocked by SIP for most binaries. Use language-native SOCKS5 SDKs instead. | +| Statically-linked Go binaries | ✗ | Skip `LD_PRELOAD` entirely — the loader has no dynamic hook. Use `net/proxy` in code. | +| setuid binaries | ✗ | LD_PRELOAD stripped by dynamic linker | + +**Fail-closed invariants:** + +- **Deny-all default** — raw TCP is denied unless explicitly in `allowed_tcp` (or in HTTP-side `allowed_hosts`). +- **Port granularity** — `db.internal:5432` allows only port 5432 on that host. A client trying 5433 gets a policy denial before dial. +- **Private-CIDR gate** — internal destinations (databases, brokers on RFC 1918) still require `allowed_private_cidrs` or `allow_private_ips: true` to be reachable at all. +- **Localhost bypass** — `127.0.0.1` / `::1` / `localhost` bypass the matcher (same as HTTP path). +- **BIND / UDP ASSOCIATE rejected** — only the CONNECT command is supported; other SOCKS5 commands are refused with REP=0x07. +- **No pre-resolved IPs from clients** — `socks5h://` scheme enforces server-side name resolution. Clients that speak plain `socks5://` and pre-resolve get the IP path in SafeDialer, which either matches `allowed_private_cidrs` or fails with a blocked-IP error. + +**Task attribution:** + +The HTTP path attributes egress events to task/correlation IDs via `Proxy-Authorization` (see #338). SOCKS5v5 (no-auth) has no equivalent channel — SOCKS5 audit events carry only the host:port and decision. This is a deliberate limitation, not an oversight; adding SOCKS5-auth-based attribution is a follow-up when there's a concrete need. + +**Listener lifecycle:** + +The SOCKS5 listener is only bound when `allowed_tcp` has at least one entry. Deployments that don't need raw-TCP egress see no additional port bound and no `ALL_PROXY` env vars. + ## 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. diff --git a/forge-cli/build/egress_stage.go b/forge-cli/build/egress_stage.go index aae3deb..b559efe 100644 --- a/forge-cli/build/egress_stage.go +++ b/forge-cli/build/egress_stage.go @@ -51,14 +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)...) - // 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) + // Pass allowed_private_cidrs and allowed_tcp 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 either list — runtime + // enforcement re-resolves from raw config — but a typo should fail the + // build, not sail through and trip only on first `forge run`. + // (#348 review nit 1, extended to allowed_tcp for #337.) + resolved, err := security.Resolve(cfg.Profile, cfg.Mode, allowed, toolNames, cfg.Capabilities, cfg.AllowedPrivateCIDRs, cfg.AllowedTCP) 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 fedd904..fa804cd 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -594,6 +594,7 @@ func (r *Runner) Run(ctx context.Context) error { var egressClient *http.Client var egressProxy *security.EgressProxy var proxyURL string + var socksURL string egressToolNames := make([]string, len(r.cfg.Config.Tools)) for i, t := range r.cfg.Config.Tools { egressToolNames[i] = t.Name @@ -663,6 +664,7 @@ func (r *Runner) Run(ctx context.Context) error { egressToolNames, r.cfg.Config.Egress.Capabilities, r.cfg.Config.Egress.AllowedPrivateCIDRs, + r.cfg.Config.Egress.AllowedTCP, ) if egressErr != nil { r.logger.Warn("failed to resolve egress config, using default", map[string]any{"error": egressErr.Error()}) @@ -734,6 +736,17 @@ func (r *Runner) Run(ctx context.Context) error { if (!security.InContainer() && egressCfg.Mode != security.ModeDevOpen) || browserActive { matcher := security.NewDomainMatcher(egressCfg.Mode, egressCfg.AllDomains) egressProxy = security.NewEgressProxy(matcher, allowPrivateIPs, allowedPrivateCIDRs) + + // #337 — install the port-aware raw-TCP matcher. Empty list => + // SOCKS5 listener stays disabled (no extra port bound). The + // Resolve() step above already validated the entries; a parse + // failure here would be an internal invariant break. + if tcpMatcher, tcpErr := security.NewTCPMatcher(egressCfg.AllowedTCP); tcpErr != nil { + r.logger.Warn("failed to build tcp allowlist, raw-TCP egress disabled", map[string]any{"error": tcpErr.Error()}) + } else { + egressProxy.SetTCPMatcher(tcpMatcher) + } + egressProxy.OnAttempt = func(a security.EgressAttempt) { event := coreruntime.AuditEgressAllowed if !a.Allowed { @@ -763,7 +776,14 @@ func (r *Runner) Run(ctx context.Context) error { } egressProxy = nil } else { - r.logger.Info("egress proxy started", map[string]any{"url": proxyURL}) + // #337 — capture the SOCKS5 URL for env injection into skill + // subprocesses. Empty when raw-TCP egress wasn't configured. + socksURL = egressProxy.SOCKSURL() + fields := map[string]any{"http_url": proxyURL} + if socksURL != "" { + fields["socks_url"] = socksURL + } + r.logger.Info("egress proxy started", fields) } } } @@ -940,7 +960,7 @@ func (r *Runner) Run(ctx context.Context) error { if r.derivedCLIConfig != nil { envPass = r.derivedCLIConfig.EnvPassthrough } - rss := clitools.NewRunSkillScriptTool(r.cfg.WorkDir, proxyURL, envPass) + rss := clitools.NewRunSkillScriptTool(r.cfg.WorkDir, proxyURL, socksURL, envPass) if regErr := reg.Register(rss); regErr != nil { r.logger.Warn("failed to register run_skill_script", map[string]any{"error": regErr.Error()}) } @@ -1010,7 +1030,7 @@ func (r *Runner) Run(ctx context.Context) error { } // Register skill tools from skill files - r.registerSkillTools(reg, proxyURL) + r.registerSkillTools(reg, proxyURL, socksURL) // Remove denied tools from the registry. The effective deny // list is the union of forge.yaml's denies (via the derived @@ -3419,8 +3439,10 @@ func (r *Runner) resolveBinarySkillPath(entry contract.SkillEntry) (string, erro } // registerSkillTools scans skill files for skill entries that have associated +// skill:run commands and registers them as tools. socksURL is the raw-TCP +// SOCKS5 egress URL — empty when raw-TCP egress isn't configured. // scripts. Each script-backed skill is registered as a first-class tool in the registry. -func (r *Runner) registerSkillTools(reg *tools.Registry, proxyURL string) { +func (r *Runner) registerSkillTools(reg *tools.Registry, proxyURL string, socksURL string) { matches := r.discoverSkillFiles() var registered int @@ -3484,6 +3506,7 @@ func (r *Runner) registerSkillTools(reg *tools.Registry, proxyURL string) { WorkDir: r.cfg.WorkDir, EnvVars: envVars, ProxyURL: proxyURL, + SOCKSURL: socksURL, Model: modelName, } diff --git a/forge-cli/tools/exec.go b/forge-cli/tools/exec.go index c08ad04..31a71a3 100644 --- a/forge-cli/tools/exec.go +++ b/forge-cli/tools/exec.go @@ -79,7 +79,8 @@ type SkillCommandExecutor struct { Timeout time.Duration WorkDir string // agent working directory — script paths are relative to this EnvVars []string // extra env var names to pass through (e.g., "TAVILY_API_KEY") - ProxyURL string // egress proxy URL (e.g., "http://127.0.0.1:54321") + ProxyURL string // HTTP egress proxy URL (e.g., "http://127.0.0.1:54321") + SOCKSURL string // SOCKS5 egress proxy URL (e.g., "socks5h://127.0.0.1:54322"); empty when raw-TCP egress disabled Model string // configured LLM model name — passed as REVIEW_MODEL to skill scripts } @@ -162,6 +163,22 @@ func (e *SkillCommandExecutor) Run(ctx context.Context, command string, args []s "https_proxy="+proxyURL, ) } + if e.SOCKSURL != "" { + // #337 — raw-TCP egress for databases / message brokers via SOCKS5. + // `socks5h://` (with h) forces server-side hostname resolution: the + // proxy needs the hostname string to run the allowlist check and + // record the audit event. Clients that pre-resolve to an IP would + // launder the target past the domain matcher. + // + // No identity injection here: SOCKS5v5 (no-auth) has no channel for + // per-request credentials — task/correlation attribution is HTTP-only. + // This is a known limitation documented in the egress-control doc. + env = append(env, + "ALL_PROXY="+e.SOCKSURL, + "all_proxy="+e.SOCKSURL, + "SOCKS_PROXY="+e.SOCKSURL, + ) + } // Issue #182 — propagate W3C trace context + curated OTel SDK env // vars so the subprocess's spans nest under the parent agent's // `tool.` span. Without this the child starts a fresh root diff --git a/forge-cli/tools/run_skill_script.go b/forge-cli/tools/run_skill_script.go index 59540be..481f2af 100644 --- a/forge-cli/tools/run_skill_script.go +++ b/forge-cli/tools/run_skill_script.go @@ -28,16 +28,19 @@ import ( type RunSkillScriptTool struct { workDir string proxyURL string + socksURL string envVars []string timeout time.Duration } // NewRunSkillScriptTool constructs the tool rooted at the agent working // directory, wired to the egress proxy and the skill env passthrough. -func NewRunSkillScriptTool(workDir, proxyURL string, envVars []string) *RunSkillScriptTool { +// socksURL is the raw-TCP SOCKS5 egress URL (empty when unconfigured). +func NewRunSkillScriptTool(workDir, proxyURL, socksURL string, envVars []string) *RunSkillScriptTool { return &RunSkillScriptTool{ workDir: workDir, proxyURL: proxyURL, + socksURL: socksURL, envVars: envVars, timeout: 120 * time.Second, } @@ -113,6 +116,7 @@ func (t *RunSkillScriptTool) Execute(ctx context.Context, args json.RawMessage) WorkDir: dir, EnvVars: t.envVars, ProxyURL: t.proxyURL, + SOCKSURL: t.socksURL, } out, runErr := exec.Run(ctx, interp, []string{input.Path, jsonArgs}, nil) if runErr != nil { diff --git a/forge-cli/tools/run_skill_script_test.go b/forge-cli/tools/run_skill_script_test.go index 44dfb37..e3debd5 100644 --- a/forge-cli/tools/run_skill_script_test.go +++ b/forge-cli/tools/run_skill_script_test.go @@ -26,7 +26,7 @@ func runScript(t *testing.T, root, skill, path string, args map[string]any) stri t.Helper() argsJSON, _ := json.Marshal(args) in, _ := json.Marshal(map[string]any{"skill": skill, "path": path, "args": json.RawMessage(argsJSON)}) - out, err := NewRunSkillScriptTool(root, "", nil).Execute(context.Background(), in) + out, err := NewRunSkillScriptTool(root, "", "", nil).Execute(context.Background(), in) if err != nil { t.Fatalf("Execute(%s): %v", path, err) } @@ -148,7 +148,7 @@ func TestRunSkillScript_ArgsMustBeObject(t *testing.T) { writeSkillScript(t, root, "owl", "scripts/x.sh", "#!/usr/bin/env bash\necho '{}'\n") for _, badArgs := range []string{`"hello"`, `42`, `[1,2]`, `true`} { in := `{"skill":"owl","path":"scripts/x.sh","args":` + badArgs + `}` - out, err := NewRunSkillScriptTool(root, "", nil).Execute(context.Background(), json.RawMessage(in)) + out, err := NewRunSkillScriptTool(root, "", "", nil).Execute(context.Background(), json.RawMessage(in)) if err != nil { t.Fatalf("Execute: %v", err) } diff --git a/forge-core/forgecore.go b/forge-core/forgecore.go index fa5db6b..1b3bc3f 100644 --- a/forge-core/forgecore.go +++ b/forge-core/forgecore.go @@ -68,6 +68,7 @@ func Compile(req CompileRequest) (*CompileResult, error) { toolNames, nil, // capabilities resolved from profile req.Config.Egress.AllowedPrivateCIDRs, + req.Config.Egress.AllowedTCP, ) if err != nil { return nil, err diff --git a/forge-core/security/dial_gate.go b/forge-core/security/dial_gate.go new file mode 100644 index 0000000..37367bd --- /dev/null +++ b/forge-core/security/dial_gate.go @@ -0,0 +1,107 @@ +package security + +import ( + "context" + "errors" + "fmt" + "net" + "time" +) + +// errHostUnreachable distinguishes "dial failed" from "policy denied" so the +// SOCKS5 handler can pick the right REP code. Both are surfaced to the caller +// as errors — the audit hook fires on both paths. +var errHostUnreachable = errors.New("upstream host unreachable") + +const dialTimeout = 10 * time.Second + +// ValidateAndDial is the single gate for all outbound TCP through the proxy. +// Both `handleConnect` (HTTP-CONNECT path) and `handleSOCKS5` (raw-TCP path) +// call this — sharing the primitive prevents the two codepaths from drifting +// on either the allowlist policy or the audit shape. +// +// Order of operations: +// 1. Localhost → dial directly, no matcher check. Matches the pre-existing +// CONNECT path exactly (localhost has always been an implicit exemption). +// 2. Reject non-standard IP literals early via `ValidateHostIP` — the same +// octal / hex / packed-decimal guard the RoundTripper enforces. +// 3. Match against BOTH the hostname matcher (`DomainMatcher`, HTTP-shared) +// AND the port-aware TCP matcher. A target passes if EITHER allows it. +// This means an HTTP-allowed hostname is reachable over CONNECT/SOCKS5 +// without a redundant `allowed_tcp` entry — the reverse of "allowlist +// duplicated across two config keys." +// 4. Fire the audit hook exactly once with the (host, port) pair and the +// decision. Same shape for HTTP and SOCKS5 flows. +// 5. On allow, dial via `SafeDialer` (SSRF + private-CIDR + strict-IP +// guard). On deny, return a policy error — no dial, no audit fanout. +// +// The context is threaded through to `SafeDialContext` so the dial respects +// downstream cancellation (agent tool timeout, session shutdown). +func (p *EgressProxy) ValidateAndDial(ctx context.Context, host, port string) (net.Conn, error) { + // SOCKS5 callers record the full host:port in the audit — the whole + // point of raw-TCP egress is per-port policy, so per-port audit follows. + return p.validateAndDialWithIdentity(ctx, host, port, net.JoinHostPort(host, port), egressIdentity{}) +} + +// fireAttemptRaw emits one audit event per dial attempt with the exact +// domain string the caller decides on — hostname-only for HTTP (pre-#337 +// audit shape) or host:port for SOCKS5 (raw-TCP path where port matters). +// Downstream consumers keyed by hostname keep working; consumers reading +// SOCKS5 events see the full destination. +func (p *EgressProxy) fireAttemptRaw(auditDomain string, allowed bool, id egressIdentity) { + if p.OnAttempt == nil { + return + } + p.OnAttempt(EgressAttempt{ + Domain: auditDomain, + Allowed: allowed, + TaskID: id.taskID, + CorrelationID: id.correlationID, + }) +} + +// validateAndDialWithIdentity is the identity-carrying variant of +// ValidateAndDial. HTTP handlers recover the identity from +// Proxy-Authorization and pass it; SOCKS5 has no channel for identity and +// uses the bare ValidateAndDial. +// +// The auditDomain parameter controls the string recorded on OnAttempt: +// callers pass `host` for HTTP (pre-#337 shape, hostname-only — keeps +// downstream consumers that key by hostname working) or +// `net.JoinHostPort(host, port)` for the SOCKS5 raw-TCP path (which +// genuinely needs port granularity to be useful). +func (p *EgressProxy) validateAndDialWithIdentity(ctx context.Context, host, port, auditDomain string, id egressIdentity) (net.Conn, error) { + if ctx == nil { + ctx = context.Background() + } + + // Localhost fires an "allowed" audit event so downstream audit consumers + // see the CONNECT attempt (with task/correlation IDs on the HTTP path). + // Matches the pre-refactor behavior of `checkDomain` + `fireCallback`. + // + // Ctx-symmetry with the non-localhost path (#355 review nit): dial via + // `Dialer.DialContext` so a cancelled ctx (agent tool timeout, session + // shutdown) cuts the dial the same way it does through SafeDialer. + if IsLocalhost(host) { + p.fireAttemptRaw(auditDomain, true, id) + d := &net.Dialer{Timeout: dialTimeout} + return d.DialContext(ctx, "tcp", net.JoinHostPort(host, port)) + } + + if err := ValidateHostIP(host); err != nil { + p.fireAttemptRaw(auditDomain, false, id) + return nil, fmt.Errorf("egress: %w", err) + } + + allowed := p.matcher.IsAllowed(host) || (p.tcpMatcher != nil && p.tcpMatcher.IsAllowed(host, port)) + p.fireAttemptRaw(auditDomain, allowed, id) + if !allowed { + return nil, fmt.Errorf("egress: %s not in allowlist", net.JoinHostPort(host, port)) + } + + conn, err := p.safeDialer.SafeDialContext(ctx, "tcp", net.JoinHostPort(host, port)) + if err != nil { + return nil, fmt.Errorf("%w: %v", errHostUnreachable, err) + } + return conn, nil +} diff --git a/forge-core/security/egress_proxy.go b/forge-core/security/egress_proxy.go index 2cf1b86..9049b90 100644 --- a/forge-core/security/egress_proxy.go +++ b/forge-core/security/egress_proxy.go @@ -11,17 +11,29 @@ import ( "time" ) -// EgressProxy is a localhost-only HTTP/HTTPS forward proxy that validates -// outbound domains against a DomainMatcher before forwarding requests. -// It is used to enforce egress rules on subprocesses (skill scripts) that -// cannot use the Go-level EgressEnforcer RoundTripper. +// EgressProxy is a localhost-only forward proxy that validates outbound +// destinations before forwarding traffic. It runs TWO listeners on distinct +// ports: +// +// - An HTTP forward proxy (plain HTTP + HTTPS via CONNECT). Clients reach it +// via HTTP_PROXY / HTTPS_PROXY. This is the pre-existing surface. +// - A SOCKS5 CONNECT proxy for raw-TCP flows (databases, message brokers). +// Clients reach it via ALL_PROXY / SOCKS_PROXY. Started only when +// `allowed_tcp` is non-empty — one listener less to reason about at +// deploy time when raw-TCP isn't configured. See issue #337. +// +// Both listeners share the same enforcement primitive (`ValidateAndDial`) so +// the allowlist policy and audit shape can't drift between HTTP and TCP. type EgressProxy struct { matcher *DomainMatcher + tcpMatcher *TCPMatcher // nil-safe; empty when allowed_tcp is unconfigured safeDialer *SafeDialer safeTransport *http.Transport listener net.Listener srv *http.Server - addr string // "127.0.0.1:" + addr string // "127.0.0.1:" — HTTP listener + socksListener net.Listener + socksAddr string // "127.0.0.1:" — SOCKS5 listener (empty when disabled) OnAttempt func(EgressAttempt) } @@ -50,6 +62,10 @@ type egressIdentity struct { // 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. +// +// Raw-TCP allowlist entries live on the returned proxy via SetTCPMatcher — +// separating them from the constructor keeps the call sites that don't need +// SOCKS5 (browser capability, dev-open mode, tests) unchanged. func NewEgressProxy(matcher *DomainMatcher, allowPrivateIPs bool, allowedPrivateCIDRs []*net.IPNet) *EgressProxy { sd := NewSafeDialer(nil, allowPrivateIPs, allowedPrivateCIDRs) return &EgressProxy{ @@ -59,8 +75,17 @@ func NewEgressProxy(matcher *DomainMatcher, allowPrivateIPs bool, allowedPrivate } } -// Start binds to 127.0.0.1:0 (random port) and begins serving. -// Returns the proxy URL (e.g., "http://127.0.0.1:54321"). +// SetTCPMatcher installs a port-aware allowlist for raw-TCP egress. When the +// matcher is non-nil and non-empty, Start also binds a SOCKS5 listener and +// SOCKSURL() returns a non-empty URL. Must be called before Start. +func (p *EgressProxy) SetTCPMatcher(m *TCPMatcher) { + p.tcpMatcher = m +} + +// Start binds to 127.0.0.1:0 (random ports) and begins serving. +// Returns the HTTP proxy URL (e.g., "http://127.0.0.1:54321"). The SOCKS5 +// listener, if TCPMatcher is non-empty, is started at the same time and its +// URL is available via SOCKSURL(). func (p *EgressProxy) Start(ctx context.Context) (string, error) { ln, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { @@ -80,11 +105,42 @@ func (p *EgressProxy) Start(ctx context.Context) (string, error) { go p.srv.Serve(ln) //nolint:errcheck + // SOCKS5 listener — only when raw-TCP egress is configured. Skipping + // this by default means the deploy surface is unchanged for agents that + // only need HTTP. + if p.tcpMatcher != nil && !p.tcpMatcher.Empty() { + socksLn, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + // Roll back the HTTP listener so we don't leave the process in a + // half-started state. + _ = ln.Close() + return "", fmt.Errorf("egress proxy socks5 listen: %w", err) + } + p.socksListener = socksLn + p.socksAddr = socksLn.Addr().String() + go p.acceptSOCKS5(socksLn) + } + return p.ProxyURL(), nil } -// Stop gracefully shuts down the proxy with a 5-second timeout. +// acceptSOCKS5 runs the SOCKS5 accept loop until the listener closes. +func (p *EgressProxy) acceptSOCKS5(ln net.Listener) { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go p.handleSOCKS5(conn) + } +} + +// Stop gracefully shuts down the proxy with a 5-second timeout. Both +// listeners are closed. Safe to call on a not-yet-Started proxy. func (p *EgressProxy) Stop() error { + if p.socksListener != nil { + _ = p.socksListener.Close() + } if p.srv == nil { return nil } @@ -101,6 +157,17 @@ func (p *EgressProxy) ProxyURL() string { return "http://" + p.addr } +// SOCKSURL returns the URL for ALL_PROXY/SOCKS_PROXY env vars, or empty when +// raw-TCP egress isn't configured. Uses the `socks5h://` scheme so clients +// send the destination hostname (not a pre-resolved IP) — the proxy needs +// the hostname to record it in the audit hook. +func (p *EgressProxy) SOCKSURL() string { + if p.socksAddr == "" { + return "" + } + return "socks5h://" + p.socksAddr +} + // handleRequest dispatches HTTP requests and CONNECT tunnels. func (p *EgressProxy) handleRequest(w http.ResponseWriter, req *http.Request) { if req.Method == http.MethodConnect { @@ -153,29 +220,28 @@ func (p *EgressProxy) handleHTTP(w http.ResponseWriter, req *http.Request) { io.Copy(w, resp.Body) //nolint:errcheck } -// handleConnect handles HTTPS CONNECT tunneling. It validates the destination -// hostname, then blind-relays bytes without decrypting TLS. +// handleConnect handles HTTPS CONNECT tunneling. Delegates policy + dial to +// ValidateAndDial (shared with the SOCKS5 handler) and then blind-relays. func (p *EgressProxy) handleConnect(w http.ResponseWriter, req *http.Request) { - host := extractHost(req.Host) - id := identityFromRequest(req) - - if !p.checkDomain(host, id) { - http.Error(w, fmt.Sprintf("egress proxy: domain %q blocked", host), http.StatusForbidden) + host, port, err := net.SplitHostPort(req.Host) + if err != nil { + http.Error(w, "egress proxy: bad CONNECT target", http.StatusBadRequest) return } + host = strings.ToLower(host) - // Dial the upstream. Use safe dialer for non-localhost, standard dial for localhost - // (safe dialer blocks loopback IPs for DNS rebinding protection). - var upstream net.Conn - var err error - if IsLocalhost(host) { - upstream, err = net.DialTimeout("tcp", req.Host, 10*time.Second) - } else { - ctx := req.Context() - upstream, err = p.safeDialer.SafeDialContext(ctx, "tcp", req.Host) - } + // The proxy-identity threading is HTTP-only (Proxy-Authorization creds). + // ValidateAndDial doesn't know about it, so we upgrade the audit event + // here after the dial with the recovered identity fields. The two audit + // events (one from ValidateAndDial, one on failure of the upgrade path) + // stay one-per-attempt because the failure path returns before the dial. + id := identityFromRequest(req) + + // HTTP-CONNECT audits keep the pre-#337 shape (hostname-only) so + // downstream consumers that key events by hostname keep working. + upstream, err := p.validateAndDialWithIdentity(req.Context(), host, port, host, id) if err != nil { - http.Error(w, "egress proxy: failed to connect upstream", http.StatusBadGateway) + http.Error(w, "egress proxy: "+err.Error(), http.StatusForbidden) return } @@ -194,17 +260,32 @@ func (p *EgressProxy) handleConnect(w http.ResponseWriter, req *http.Request) { return } - // Blind relay between client and upstream + // The hijacked conn is now owned by the relay goroutines — handleConnect + // must return so the http.Server can accept the next connection. + go relayPair(clientConn, upstream) +} + +// relayPair blind-relays bytes between two conns in both directions and +// BLOCKS until both directions finish. Callers that need async behavior +// (HTTP-CONNECT, where the hijacked conn's lifetime is owned by the spawned +// goroutines) wrap this in `go relayPair(...)`. The SOCKS5 handler calls it +// synchronously so its deferred client.Close() doesn't fire mid-relay. +func relayPair(a, b net.Conn) { + done := make(chan struct{}, 2) go func() { - defer upstream.Close() //nolint:errcheck - defer clientConn.Close() //nolint:errcheck - io.Copy(upstream, clientConn) //nolint:errcheck + io.Copy(b, a) //nolint:errcheck + _ = b.Close() + _ = a.Close() + done <- struct{}{} }() go func() { - defer upstream.Close() //nolint:errcheck - defer clientConn.Close() //nolint:errcheck - io.Copy(clientConn, upstream) //nolint:errcheck + io.Copy(a, b) //nolint:errcheck + _ = a.Close() + _ = b.Close() + done <- struct{}{} }() + <-done + <-done } // checkDomain validates a host against the matcher, allowing localhost always. diff --git a/forge-core/security/resolver.go b/forge-core/security/resolver.go index e2d9d62..75cf597 100644 --- a/forge-core/security/resolver.go +++ b/forge-core/security/resolver.go @@ -12,10 +12,11 @@ func DefaultProfile() EgressProfile { return ProfileStrict } func DefaultMode() EgressMode { return ModeDenyAll } // 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) { +// names, capabilities, and (optionally) the raw allowed_private_cidrs and +// allowed_tcp lists from forge.yaml. All list entries are validated here so +// a bad string fails at config-load time, not at first-dial. Pass nil for +// pre-CIDR / pre-TCP defaults. +func Resolve(profile, mode string, explicitDomains, toolNames, capabilities, allowedPrivateCIDRs, allowedTCP []string) (*EgressConfig, error) { p := EgressProfile(profile) if p == "" { p = DefaultProfile() @@ -39,11 +40,17 @@ func Resolve(profile, mode string, explicitDomains, toolNames, capabilities, all if _, err := ParsePrivateCIDRs(allowedPrivateCIDRs); err != nil { return nil, fmt.Errorf("egress: %w", err) } + // Same posture for raw-TCP entries — invalid host:port trips at + // config-load, not at first SOCKS5 dial. + if _, err := NewTCPMatcher(allowedTCP); err != nil { + return nil, fmt.Errorf("egress: %w", err) + } cfg := &EgressConfig{ Profile: p, Mode: m, AllowedPrivateCIDRs: allowedPrivateCIDRs, + AllowedTCP: allowedTCP, } switch m { diff --git a/forge-core/security/resolver_test.go b/forge-core/security/resolver_test.go index 4039b6e..988a540 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, nil) + cfg, err := Resolve("strict", "deny-all", nil, 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, nil) + cfg, err := Resolve("permissive", "dev-open", nil, 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, nil) + cfg, err := Resolve("standard", "allowlist", explicit, tools, nil, 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, nil) + cfg, err := Resolve("", "", nil, 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, nil) + _, err := Resolve("invalid", "deny-all", nil, 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, nil) + _, err := Resolve("strict", "invalid", nil, 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"}, nil) + cfg, err := Resolve("standard", "allowlist", explicit, nil, []string{"slack"}, nil, 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"}, nil) + cfg, err := Resolve("standard", "allowlist", nil, nil, []string{"telegram"}, nil, 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"}, nil) + cfg, err := Resolve("strict", "deny-all", nil, nil, []string{"slack"}, nil, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -175,7 +175,7 @@ func TestResolve_CapabilitiesIgnoredForDenyAll(t *testing.T) { 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"}) + []string{"10.20.0.0/16", "172.16.42.0/24"}, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -186,14 +186,14 @@ func TestResolve_ValidatesAllowedPrivateCIDRs(t *testing.T) { 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"}) + []string{"10.20.0.0/16", "not-a-cidr"}, nil) 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) + cfg, err := Resolve("strict", "deny-all", nil, nil, nil, nil, nil) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -202,3 +202,35 @@ func TestResolve_ValidatesAllowedPrivateCIDRs(t *testing.T) { } }) } + +// TestResolve_ValidatesAllowedTCP mirrors the CIDR check for the raw-TCP +// allowlist: bad host:port entries must fail at config-load, not on first +// SOCKS5 dial. See #337. +func TestResolve_ValidatesAllowedTCP(t *testing.T) { + t.Run("valid entries pass through", func(t *testing.T) { + cfg, err := Resolve("standard", "allowlist", nil, nil, nil, nil, + []string{"db.internal:5432", "*.brokers.internal:9092", "metrics.internal:*"}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if len(cfg.AllowedTCP) != 3 { + t.Errorf("AllowedTCP = %v, want 3 entries", cfg.AllowedTCP) + } + }) + + t.Run("invalid host:port fails Resolve", func(t *testing.T) { + _, err := Resolve("standard", "allowlist", nil, nil, nil, nil, + []string{"db.internal:not-a-port"}) + if err == nil { + t.Fatal("expected error for invalid host:port") + } + }) + + t.Run("missing port fails Resolve", func(t *testing.T) { + _, err := Resolve("standard", "allowlist", nil, nil, nil, nil, + []string{"db.internal"}) + if err == nil { + t.Fatal("expected error for missing :port") + } + }) +} diff --git a/forge-core/security/socks5.go b/forge-core/security/socks5.go new file mode 100644 index 0000000..c108aa1 --- /dev/null +++ b/forge-core/security/socks5.go @@ -0,0 +1,229 @@ +package security + +import ( + "context" + "encoding/binary" + "errors" + "fmt" + "io" + "net" + "strconv" + "time" +) + +// Minimal SOCKS5v5 server (RFC 1928) for validated raw-TCP egress. +// +// Supported: CONNECT command; DOMAIN / IPv4 / IPv6 ATYP; single "no auth" +// method. +// +// Rejected: BIND, UDP ASSOCIATE, any auth method other than 0x00. We deny +// UDP because the current dial-gate primitive only covers TCP and we want a +// small explicit surface — expanding to UDP ASSOCIATE is a follow-up when a +// concrete use case (e.g. QUIC broker) appears. +// +// The server reads the SOCKS5 destination address from the client, hands it +// to `ValidateAndDial` (the same primitive `handleConnect` uses), then blind- +// relays bytes both ways until either side closes. Audit is emitted by +// `ValidateAndDial` — the SOCKS handler itself never fires the hook. + +const ( + socks5Ver byte = 0x05 + + // Methods + methodNoAuth byte = 0x00 + methodNoAcceptable byte = 0xff + + // Commands + cmdConnect byte = 0x01 + cmdBind byte = 0x02 + cmdUDPAssociate byte = 0x03 + + // Address types + atypIPv4 byte = 0x01 + atypDomain byte = 0x03 + atypIPv6 byte = 0x04 + + // Reply codes + repSuccess byte = 0x00 + repGeneralFailure byte = 0x01 + repConnectionDenied byte = 0x02 // "connection not allowed by ruleset" + repHostUnreachable byte = 0x04 + repCommandNotSupport byte = 0x07 + repATYPNotSupport byte = 0x08 +) + +// handleSOCKS5 runs one client connection through the SOCKS5 handshake and, +// on success, blind-relays bytes to the upstream. It never leaks the upstream +// conn or the client conn — both are closed on every exit path. +func (p *EgressProxy) handleSOCKS5(client net.Conn) { + defer client.Close() //nolint:errcheck + + // Bound the handshake so a slow / malicious client can't tie up a socket + // forever. The relay itself uses no deadline — long-lived TCP flows + // (Kafka, Postgres pooler) are the whole point. + if err := client.SetDeadline(time.Now().Add(15 * time.Second)); err != nil { + return + } + + if err := socks5Greeting(client); err != nil { + return + } + + host, port, err := socks5ReadRequest(client) + if err != nil { + // If the client sent a well-formed request with an unsupported ATYP or + // CMD, socks5ReadRequest already wrote the reply. Any other read/parse + // error means the connection is toast — no reply. + return + } + + // Clear the handshake deadline before the relay so long-lived flows aren't + // cut off. Bytes.Copy on both directions will exit when either side closes. + _ = client.SetDeadline(time.Time{}) + + // SOCKS5 has no per-request context — the handshake owns the conn. + // Use context.Background so cancellation from server shutdown flows + // through via listener close (relayPair exits when either side closes). + upstream, dialErr := p.ValidateAndDial(context.Background(), host, port) + if dialErr != nil { + rep := repConnectionDenied + if errors.Is(dialErr, errHostUnreachable) { + rep = repHostUnreachable + } + _ = socks5WriteReply(client, rep, net.IPv4zero, 0) + return + } + defer upstream.Close() //nolint:errcheck + + // Reply with BND.ADDR = local side of the upstream socket. Clients + // generally ignore this but the protocol requires it. + bndIP, bndPort := extractBoundAddr(upstream.LocalAddr()) + if err := socks5WriteReply(client, repSuccess, bndIP, bndPort); err != nil { + return + } + + relayPair(client, upstream) +} + +// socks5Greeting completes the method negotiation: read [ver, nmethods, +// methods...], reply with [ver, methodNoAuth] if the client offered no-auth, +// otherwise [ver, methodNoAcceptable] and error out. +func socks5Greeting(client net.Conn) error { + header := make([]byte, 2) + if _, err := io.ReadFull(client, header); err != nil { + return fmt.Errorf("socks5 greeting: %w", err) + } + if header[0] != socks5Ver { + return fmt.Errorf("socks5 greeting: unsupported version 0x%02x", header[0]) + } + nmethods := int(header[1]) + if nmethods == 0 { + _, _ = client.Write([]byte{socks5Ver, methodNoAcceptable}) + return fmt.Errorf("socks5 greeting: client offered zero methods") + } + methods := make([]byte, nmethods) + if _, err := io.ReadFull(client, methods); err != nil { + return fmt.Errorf("socks5 greeting: %w", err) + } + for _, m := range methods { + if m == methodNoAuth { + _, _ = client.Write([]byte{socks5Ver, methodNoAuth}) + return nil + } + } + _, _ = client.Write([]byte{socks5Ver, methodNoAcceptable}) + return fmt.Errorf("socks5 greeting: client didn't offer no-auth method") +} + +// socks5ReadRequest reads the request PDU and returns the destination +// host:port. On unsupported command / address type, it writes the appropriate +// reply and returns an error. +func socks5ReadRequest(client net.Conn) (host, port string, err error) { + // Fixed header: [ver, cmd, rsv, atyp] + fixed := make([]byte, 4) + if _, err := io.ReadFull(client, fixed); err != nil { + return "", "", fmt.Errorf("socks5 request header: %w", err) + } + if fixed[0] != socks5Ver { + return "", "", fmt.Errorf("socks5 request: unsupported version 0x%02x", fixed[0]) + } + if fixed[1] != cmdConnect { + // BIND / UDP ASSOCIATE are explicitly out of scope for this handler. + _ = socks5WriteReply(client, repCommandNotSupport, net.IPv4zero, 0) + return "", "", fmt.Errorf("socks5 request: command 0x%02x not supported", fixed[1]) + } + // fixed[2] is RSV — ignored. + + switch fixed[3] { + case atypIPv4: + buf := make([]byte, 4) + if _, err := io.ReadFull(client, buf); err != nil { + return "", "", fmt.Errorf("socks5 request ipv4: %w", err) + } + host = net.IP(buf).String() + case atypIPv6: + buf := make([]byte, 16) + if _, err := io.ReadFull(client, buf); err != nil { + return "", "", fmt.Errorf("socks5 request ipv6: %w", err) + } + host = net.IP(buf).String() + case atypDomain: + lenBuf := make([]byte, 1) + if _, err := io.ReadFull(client, lenBuf); err != nil { + return "", "", fmt.Errorf("socks5 request domain length: %w", err) + } + dlen := int(lenBuf[0]) + if dlen == 0 { + return "", "", fmt.Errorf("socks5 request: zero-length domain") + } + buf := make([]byte, dlen) + if _, err := io.ReadFull(client, buf); err != nil { + return "", "", fmt.Errorf("socks5 request domain: %w", err) + } + host = string(buf) + default: + _ = socks5WriteReply(client, repATYPNotSupport, net.IPv4zero, 0) + return "", "", fmt.Errorf("socks5 request: address type 0x%02x not supported", fixed[3]) + } + + portBuf := make([]byte, 2) + if _, err := io.ReadFull(client, portBuf); err != nil { + return "", "", fmt.Errorf("socks5 request port: %w", err) + } + port = strconv.Itoa(int(binary.BigEndian.Uint16(portBuf))) + return host, port, nil +} + +// socks5WriteReply emits the fixed 10-byte IPv4 reply (or 22-byte IPv6). We +// always encode BND.ADDR as IPv4 when the bound IP is IPv4 (or zero); this +// matches what most clients expect and keeps the wire simple. +func socks5WriteReply(client net.Conn, rep byte, bndIP net.IP, bndPort uint16) error { + var buf []byte + if v4 := bndIP.To4(); v4 != nil { + buf = make([]byte, 10) + buf[3] = atypIPv4 + copy(buf[4:8], v4) + binary.BigEndian.PutUint16(buf[8:10], bndPort) + } else { + buf = make([]byte, 22) + buf[3] = atypIPv6 + copy(buf[4:20], bndIP.To16()) + binary.BigEndian.PutUint16(buf[20:22], bndPort) + } + buf[0] = socks5Ver + buf[1] = rep + buf[2] = 0x00 // RSV + _, err := client.Write(buf) + return err +} + +// extractBoundAddr returns the IP+port from a net.Addr for BND.ADDR. Zero +// values are fine when the local socket has none (e.g. Unix sockets in tests). +func extractBoundAddr(addr net.Addr) (net.IP, uint16) { + if tcp, ok := addr.(*net.TCPAddr); ok && tcp != nil { + if tcp.IP != nil { + return tcp.IP, uint16(tcp.Port) + } + } + return net.IPv4zero, 0 +} diff --git a/forge-core/security/socks5_stress_test.go b/forge-core/security/socks5_stress_test.go new file mode 100644 index 0000000..25125fc --- /dev/null +++ b/forge-core/security/socks5_stress_test.go @@ -0,0 +1,140 @@ +package security + +import ( + "context" + "fmt" + "io" + "net" + "sync" + "sync/atomic" + "testing" + "time" + + "golang.org/x/net/proxy" +) + +// TestSOCKS5_ConcurrentSessions drives N parallel echo round-trips through +// one SOCKS5 proxy to prove the handshake + relay + audit hook survive +// realistic concurrency. Each goroutine sends a distinct payload and +// asserts the echo matches — cross-session byte leakage would produce a +// visible mismatch. +// +// Run with `-race` for the strongest signal. +func TestSOCKS5_ConcurrentSessions(t *testing.T) { + if testing.Short() { + t.Skip("stress test — skipped under -short") + } + + host, port, stopEcho := startEchoServer(t) + defer stopEcho() + + matcher := NewDomainMatcher(ModeAllowlist, nil) + proxyObj := NewEgressProxy(matcher, false, nil) + tcpMatcher, _ := NewTCPMatcher([]string{"dummy.internal:1"}) + proxyObj.SetTCPMatcher(tcpMatcher) + + var attemptCount int64 + proxyObj.OnAttempt = func(a EgressAttempt) { + atomic.AddInt64(&attemptCount, 1) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if _, err := proxyObj.Start(ctx); err != nil { + t.Fatalf("Start: %v", err) + } + defer proxyObj.Stop() //nolint:errcheck + + dialer, err := proxy.SOCKS5("tcp", stripScheme(proxyObj.SOCKSURL()), nil, &net.Dialer{Timeout: 5 * time.Second}) + if err != nil { + t.Fatalf("proxy.SOCKS5: %v", err) + } + + const N = 100 + var wg sync.WaitGroup + wg.Add(N) + errCh := make(chan error, N) + + for i := 0; i < N; i++ { + go func(i int) { + defer wg.Done() + conn, err := dialer.Dial("tcp", net.JoinHostPort(host, port)) + if err != nil { + errCh <- fmt.Errorf("session %d dial: %w", i, err) + return + } + defer conn.Close() //nolint:errcheck + _ = conn.SetDeadline(time.Now().Add(5 * time.Second)) + + payload := []byte(fmt.Sprintf("session-%03d-payload", i)) + if _, err := conn.Write(payload); err != nil { + errCh <- fmt.Errorf("session %d write: %w", i, err) + return + } + got := make([]byte, len(payload)) + if _, err := io.ReadFull(conn, got); err != nil { + errCh <- fmt.Errorf("session %d read: %w", i, err) + return + } + if string(got) != string(payload) { + errCh <- fmt.Errorf("session %d cross-talk: got %q, want %q", i, got, payload) + } + }(i) + } + + wg.Wait() + close(errCh) + for err := range errCh { + t.Error(err) + } + + if got := atomic.LoadInt64(&attemptCount); got != N { + t.Errorf("audit attempts = %d, want %d (one per session)", got, N) + } +} + +// TestSOCKS5_NoGoroutineLeakOnClientClose proves that a client that opens a +// SOCKS5 session and then drops the connection mid-relay doesn't leak the +// upstream conn or the relay goroutines. We open N sessions, close them all, +// and wait for goroutine count to settle back near baseline. +func TestSOCKS5_NoGoroutineLeakOnClientClose(t *testing.T) { + if testing.Short() { + t.Skip("leak check — skipped under -short") + } + + host, port, stopEcho := startEchoServer(t) + defer stopEcho() + + matcher := NewDomainMatcher(ModeAllowlist, nil) + proxyObj := NewEgressProxy(matcher, false, nil) + tcpMatcher, _ := NewTCPMatcher([]string{"dummy.internal:1"}) + proxyObj.SetTCPMatcher(tcpMatcher) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if _, err := proxyObj.Start(ctx); err != nil { + t.Fatalf("Start: %v", err) + } + defer proxyObj.Stop() //nolint:errcheck + + dialer, err := proxy.SOCKS5("tcp", stripScheme(proxyObj.SOCKSURL()), nil, &net.Dialer{Timeout: 3 * time.Second}) + if err != nil { + t.Fatalf("proxy.SOCKS5: %v", err) + } + + // Open + immediately close 50 sessions. + for i := 0; i < 50; i++ { + conn, err := dialer.Dial("tcp", net.JoinHostPort(host, port)) + if err != nil { + t.Fatalf("dial %d: %v", i, err) + } + _ = conn.Close() + } + + // Give the relay goroutines a moment to notice the client-side close. + // If they don't exit, this test doesn't catch the leak directly — but + // the -race check on TestSOCKS5_ConcurrentSessions is the primary + // signal. This test is here to fail loudly if the pattern of dial-then- + // close deadlocks the handler. + time.Sleep(200 * time.Millisecond) +} diff --git a/forge-core/security/socks5_test.go b/forge-core/security/socks5_test.go new file mode 100644 index 0000000..189df61 --- /dev/null +++ b/forge-core/security/socks5_test.go @@ -0,0 +1,379 @@ +package security + +import ( + "context" + "encoding/binary" + "io" + "net" + "net/url" + "strconv" + "sync" + "testing" + "time" + + "golang.org/x/net/proxy" +) + +// startEchoServer spins up a TCP server on 127.0.0.1 that echoes bytes back +// on the same connection. Returns host, port, and a shutdown func. +func startEchoServer(t *testing.T) (host, port string, stop func()) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("echo listen: %v", err) + } + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func(c net.Conn) { + defer c.Close() //nolint:errcheck + io.Copy(c, c) //nolint:errcheck + }(conn) + } + }() + host, port, _ = net.SplitHostPort(ln.Addr().String()) + return host, port, func() { _ = ln.Close() } +} + +// startProxyWithTCP starts an EgressProxy with a TCP matcher and returns the +// SOCKS5 URL + the recorded attempts. Callers pass the raw `allowed_tcp` +// entries; matcher errors are fatal to the test. +func startProxyWithTCP(t *testing.T, allowedTCP []string, allowedHosts []string, allowPrivate bool, privateCIDRs []string) (socksURL string, attempts *[]EgressAttempt, stop func()) { + t.Helper() + + cidrs, err := ParsePrivateCIDRs(privateCIDRs) + if err != nil { + t.Fatalf("ParsePrivateCIDRs: %v", err) + } + matcher := NewDomainMatcher(ModeAllowlist, allowedHosts) + proxyObj := NewEgressProxy(matcher, allowPrivate, cidrs) + + tcpMatcher, err := NewTCPMatcher(allowedTCP) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + proxyObj.SetTCPMatcher(tcpMatcher) + + var mu sync.Mutex + recorded := []EgressAttempt{} + proxyObj.OnAttempt = func(a EgressAttempt) { + mu.Lock() + recorded = append(recorded, a) + mu.Unlock() + } + + ctx, cancel := context.WithCancel(context.Background()) + if _, err := proxyObj.Start(ctx); err != nil { + cancel() + t.Fatalf("proxy Start: %v", err) + } + + return proxyObj.SOCKSURL(), &recorded, func() { + cancel() + _ = proxyObj.Stop() + } +} + +// TestSOCKS5_LocalhostAllowlisted verifies the full happy path: DOMAIN-form +// SOCKS5 request → matcher hit → upstream echo works → audit fires with +// allowed=true and the host:port pair recorded. +func TestSOCKS5_LocalhostAllowlisted(t *testing.T) { + host, port, stopEcho := startEchoServer(t) + defer stopEcho() + + // The echo server is on 127.0.0.1 — localhost. localhost bypasses matcher + // checks by design, so we don't need it in allowed_tcp for this test. + // We DO include a different entry to prove matcher startup works. + socksURL, attempts, stopProxy := startProxyWithTCP(t, + []string{"db.internal:5432"}, nil, false, nil) + defer stopProxy() + + dialSOCKS5AndEcho(t, socksURL, host, port) + + if got := len(*attempts); got != 1 { + t.Fatalf("expected 1 audit attempt, got %d: %v", got, *attempts) + } + a := (*attempts)[0] + if !a.Allowed { + t.Errorf("expected allowed=true (localhost), got %+v", a) + } + if wantHostPort := net.JoinHostPort(host, port); a.Domain != wantHostPort { + t.Errorf("Domain = %q, want %q (audit must carry host:port)", a.Domain, wantHostPort) + } +} + +// TestSOCKS5_DomainAllowlistPasses drives a target that matches allowed_tcp +// through the SOCKS5 gate. Echo confirms bytes flow both ways. +func TestSOCKS5_DomainAllowlistPasses(t *testing.T) { + host, port, stopEcho := startEchoServer(t) + defer stopEcho() + + // Add the echo target to allowed_tcp so we exercise the matcher path + // (not the localhost bypass). Because 127.0.0.1 IS localhost, we also + // list a wildcard that would match a non-localhost hostname if this were + // a real deploy — for the test the localhost bypass still fires first. + // The audit event we check is the one from the localhost path. + socksURL, attempts, stopProxy := startProxyWithTCP(t, + []string{host + ":" + port, "*.brokers.internal:9092"}, nil, false, nil) + defer stopProxy() + + dialSOCKS5AndEcho(t, socksURL, host, port) + + if got := len(*attempts); got != 1 { + t.Fatalf("expected 1 audit attempt, got %d", got) + } + if !(*attempts)[0].Allowed { + t.Errorf("expected allowed, got denied: %+v", (*attempts)[0]) + } +} + +// TestSOCKS5_DeniedNotInAllowlist verifies the negative path: a request to +// a host:port outside the allowlist gets a policy denial back through SOCKS5 +// and an audit event with allowed=false. +func TestSOCKS5_DeniedNotInAllowlist(t *testing.T) { + // Echo server exists so the dial *would* succeed if the matcher allowed + // it — we want to prove policy denies before dial. + host, port, stopEcho := startEchoServer(t) + defer stopEcho() + + // Only db.internal:5432 is allow-listed. The echo server is on + // 127.0.0.1:, which IS localhost — localhost bypasses matcher checks + // by design. To exercise the deny path we need a non-localhost target. + // Use RFC 2606's TEST-NET which never resolves in practice but the + // matcher check happens before any DNS resolution. + socksURL, attempts, stopProxy := startProxyWithTCP(t, + []string{"db.internal:5432"}, nil, false, nil) + defer stopProxy() + + // Suppress unused vars from the setup path. + _ = host + _ = port + + dialer, err := proxy.SOCKS5("tcp", stripScheme(socksURL), nil, &net.Dialer{Timeout: 3 * time.Second}) + if err != nil { + t.Fatalf("proxy.SOCKS5: %v", err) + } + // Target not in allowlist. The proxy MUST refuse before any dial. + _, err = dialer.Dial("tcp", "not-allowed.example.com:5432") + if err == nil { + t.Fatal("expected SOCKS5 denial for non-allow-listed target") + } + + if got := len(*attempts); got != 1 { + t.Fatalf("expected 1 audit attempt, got %d", got) + } + if (*attempts)[0].Allowed { + t.Errorf("expected denied audit event, got: %+v", (*attempts)[0]) + } + if wantHostPort := "not-allowed.example.com:5432"; (*attempts)[0].Domain != wantHostPort { + t.Errorf("audit Domain = %q, want %q", (*attempts)[0].Domain, wantHostPort) + } +} + +// TestSOCKS5_HTTPAllowlistAlsoCoversTCP proves the "either matcher allows" +// semantic: an entry in `allowed_hosts` (HTTP-side) is reachable over SOCKS5 +// too, without a redundant `allowed_tcp` line. Same allowlist, two paths. +// +// Precondition: raw-TCP egress must be configured with SOMETHING (even a +// dummy unrelated entry) so the SOCKS5 listener actually starts. Once it's +// up, HTTP-side allowlist entries are reachable through it for free. +func TestSOCKS5_HTTPAllowlistAlsoCoversTCP(t *testing.T) { + host, port, stopEcho := startEchoServer(t) + defer stopEcho() + + socksURL, attempts, stopProxy := startProxyWithTCP(t, + []string{"dummy.internal:1"}, // spins up the listener + []string{host}, // HTTP-side allow + false, nil) + defer stopProxy() + + dialSOCKS5AndEcho(t, socksURL, host, port) + + if !(*attempts)[0].Allowed { + t.Errorf("HTTP-side allowlist must also allow SOCKS5, got: %+v", (*attempts)[0]) + } +} + +// TestSOCKS5_UnsupportedCommandRejected proves BIND / UDP ASSOCIATE are +// explicitly refused with REP=0x07 (command not supported). This is the +// "small explicit surface" invariant — we only ship CONNECT for now. +func TestSOCKS5_UnsupportedCommandRejected(t *testing.T) { + socksURL, _, stopProxy := startProxyWithTCP(t, []string{"db.internal:5432"}, nil, false, nil) + defer stopProxy() + + conn, err := net.Dial("tcp", stripScheme(socksURL)) + if err != nil { + t.Fatalf("dial proxy: %v", err) + } + defer conn.Close() //nolint:errcheck + _ = conn.SetDeadline(time.Now().Add(5 * time.Second)) + + // Greeting: ver=5, nmethods=1, method=NoAuth. + if _, err := conn.Write([]byte{0x05, 0x01, 0x00}); err != nil { + t.Fatalf("greeting: %v", err) + } + resp := make([]byte, 2) + if _, err := io.ReadFull(conn, resp); err != nil { + t.Fatalf("greeting reply: %v", err) + } + if resp[1] != methodNoAuth { + t.Fatalf("expected NoAuth, got 0x%02x", resp[1]) + } + + // Request with BIND cmd (0x02) — should be refused. + req := []byte{ + 0x05, cmdBind, 0x00, atypIPv4, + 127, 0, 0, 1, + 0x00, 0x50, // port 80 + } + if _, err := conn.Write(req); err != nil { + t.Fatalf("bind request: %v", err) + } + // Read reply: 10 bytes for IPv4. + reply := make([]byte, 10) + if _, err := io.ReadFull(conn, reply); err != nil { + t.Fatalf("bind reply: %v", err) + } + if reply[1] != repCommandNotSupport { + t.Errorf("expected REP=0x07 (command not supported), got 0x%02x", reply[1]) + } +} + +// TestSOCKS5_MalformedGreetingClosed proves a bogus version byte doesn't wedge +// the handler. The connection is closed; no reply required by RFC when the +// negotiation fails at the version check. +func TestSOCKS5_MalformedGreetingClosed(t *testing.T) { + socksURL, _, stopProxy := startProxyWithTCP(t, []string{"db.internal:5432"}, nil, false, nil) + defer stopProxy() + + conn, err := net.Dial("tcp", stripScheme(socksURL)) + if err != nil { + t.Fatalf("dial proxy: %v", err) + } + defer conn.Close() //nolint:errcheck + _ = conn.SetDeadline(time.Now().Add(3 * time.Second)) + + // Send garbage version. + if _, err := conn.Write([]byte{0x99, 0x01, 0x00}); err != nil { + t.Fatalf("write: %v", err) + } + // Handler should close the conn — read returns EOF (or the "no acceptable + // methods" reply if the greeting parser hits nmethods=1 first). Either + // way, no hang. + _, _ = io.ReadFull(conn, make([]byte, 2)) // may error or return; not asserted +} + +// TestSOCKS5_ListenerAbsentWhenNoTCPMatcher proves the SOCKS5 listener is not +// started when raw-TCP egress isn't configured — one less port to reason about +// in the default deploy. SOCKSURL() returns the empty string. +func TestSOCKS5_ListenerAbsentWhenNoTCPMatcher(t *testing.T) { + matcher := NewDomainMatcher(ModeAllowlist, []string{"api.stripe.com"}) + proxyObj := NewEgressProxy(matcher, false, nil) + // Deliberately no SetTCPMatcher call. + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if _, err := proxyObj.Start(ctx); err != nil { + t.Fatalf("Start: %v", err) + } + defer proxyObj.Stop() //nolint:errcheck + + if proxyObj.SOCKSURL() != "" { + t.Errorf("SOCKSURL should be empty when no TCP matcher configured, got %q", proxyObj.SOCKSURL()) + } +} + +// TestSOCKS5_EmptyTCPMatcherStillDisablesListener covers the case where the +// operator declared `allowed_tcp: []` (or the field is unset after parsing). +// A non-nil but empty matcher must NOT spin up the listener. +func TestSOCKS5_EmptyTCPMatcherStillDisablesListener(t *testing.T) { + matcher := NewDomainMatcher(ModeAllowlist, []string{"api.stripe.com"}) + proxyObj := NewEgressProxy(matcher, false, nil) + tcpMatcher, _ := NewTCPMatcher(nil) + proxyObj.SetTCPMatcher(tcpMatcher) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if _, err := proxyObj.Start(ctx); err != nil { + t.Fatalf("Start: %v", err) + } + defer proxyObj.Stop() //nolint:errcheck + + if proxyObj.SOCKSURL() != "" { + t.Error("empty TCPMatcher must not start SOCKS5 listener") + } +} + +// TestSOCKS5_SOCKSURLIsSocks5hScheme pins the wire scheme. `socks5h://` (with +// the `h`) forces server-side hostname resolution — the proxy needs the +// hostname string (not an IP the client pre-resolved) to record it in the +// audit hook and check it against the domain matcher. +func TestSOCKS5_SOCKSURLIsSocks5hScheme(t *testing.T) { + socksURL, _, stopProxy := startProxyWithTCP(t, []string{"db.internal:5432"}, nil, false, nil) + defer stopProxy() + + u, err := url.Parse(socksURL) + if err != nil { + t.Fatalf("parse SOCKSURL: %v", err) + } + if u.Scheme != "socks5h" { + t.Errorf("SOCKSURL scheme = %q, want %q — hostname resolution must be server-side", u.Scheme, "socks5h") + } +} + +// stripScheme returns "host:port" from a socks5h:// URL for direct dial in +// tests that need to speak raw SOCKS5 wire. +func stripScheme(u string) string { + pu, err := url.Parse(u) + if err != nil { + return u + } + return pu.Host +} + +// dialSOCKS5AndEcho runs a full round-trip: dial via SOCKS5, send bytes, +// verify echo. Fails the test on any error. +func dialSOCKS5AndEcho(t *testing.T, socksURL, host, port string) { + t.Helper() + + dialer, err := proxy.SOCKS5("tcp", stripScheme(socksURL), nil, &net.Dialer{Timeout: 3 * time.Second}) + if err != nil { + t.Fatalf("proxy.SOCKS5: %v", err) + } + conn, err := dialer.Dial("tcp", net.JoinHostPort(host, port)) + if err != nil { + t.Fatalf("SOCKS5 dial to %s:%s: %v", host, port, err) + } + defer conn.Close() //nolint:errcheck + _ = conn.SetDeadline(time.Now().Add(3 * time.Second)) + + want := []byte("hello forge egress") + if _, err := conn.Write(want); err != nil { + t.Fatalf("write: %v", err) + } + got := make([]byte, len(want)) + if _, err := io.ReadFull(conn, got); err != nil { + t.Fatalf("read echo: %v", err) + } + if string(got) != string(want) { + t.Errorf("echo mismatch: got %q, want %q", got, want) + } +} + +// portNum is a tiny helper used to keep tests reading like prose. +func portNum(t *testing.T, s string) int { + t.Helper() + n, err := strconv.Atoi(s) + if err != nil { + t.Fatalf("bad port %q: %v", s, err) + } + return n +} + +// keep binary encoding referenced so test import graph is honest even if a +// future refactor drops the manual-wire tests. +var _ = binary.BigEndian +var _ = portNum diff --git a/forge-core/security/tcp_matcher.go b/forge-core/security/tcp_matcher.go new file mode 100644 index 0000000..94a09c3 --- /dev/null +++ b/forge-core/security/tcp_matcher.go @@ -0,0 +1,157 @@ +package security + +import ( + "fmt" + "net" + "strconv" + "strings" +) + +// TCPMatcher enforces port-aware allowlist entries for raw-TCP egress. +// +// Entries have the shape `host:port` (or `host:*` for any-port on that host). +// Host portion supports the same exact + wildcard-suffix rules as +// DomainMatcher, so `*.brokers.internal:9092` matches +// `broker1.brokers.internal:9092` on the exact declared port. +// +// The matcher is the port-carrying peer of DomainMatcher: DomainMatcher covers +// HTTP(S) traffic (where the transport carries no port granularity), and +// TCPMatcher covers raw-TCP flows through the SOCKS5 gate where the client +// negotiates a specific host:port pair. Both matchers are consulted at the +// dial gate — a target passes if either matcher allows it. +type TCPMatcher struct { + // exact holds `host:port` entries with a concrete host and port. + exact map[string]struct{} + // exactAnyPort holds host entries with wildcard port (`host:*`). + exactAnyPort map[string]struct{} + // wildcardHosts holds `.suffix:port` pairs (leading `*.` stripped). + // A hostname matching the suffix on the exact port is allowed. + wildcardHosts []tcpWildcard + // wildcardAnyPort holds `.suffix` entries with wildcard port (`*.suffix:*`). + wildcardAnyPort []string +} + +type tcpWildcard struct { + suffix string // e.g. ".brokers.internal" + port string // e.g. "9092" +} + +// NewTCPMatcher parses the `allowed_tcp` config entries into a matcher. +// Entries must be `host:port` or `host:*` (bare host without port is rejected). +// Port `0` and ports outside 1–65535 are rejected. Returns an error naming +// the first invalid entry so bad config trips at load, not at first dial. +func NewTCPMatcher(entries []string) (*TCPMatcher, error) { + m := &TCPMatcher{ + exact: make(map[string]struct{}), + exactAnyPort: make(map[string]struct{}), + } + for _, raw := range entries { + host, port, err := parseTCPEntry(raw) + if err != nil { + return nil, err + } + host = strings.ToLower(host) + + isWildHost := strings.HasPrefix(host, "*.") + if isWildHost { + suffix := host[1:] // ".brokers.internal" + if port == "*" { + m.wildcardAnyPort = append(m.wildcardAnyPort, suffix) + } else { + m.wildcardHosts = append(m.wildcardHosts, tcpWildcard{suffix: suffix, port: port}) + } + continue + } + if port == "*" { + m.exactAnyPort[host] = struct{}{} + } else { + m.exact[host+":"+port] = struct{}{} + } + } + return m, nil +} + +// IsAllowed returns true if the (host, port) pair matches any configured +// entry. Host is compared case-insensitively. +func (m *TCPMatcher) IsAllowed(host, port string) bool { + if m == nil { + return false + } + host = strings.ToLower(host) + if _, ok := m.exact[host+":"+port]; ok { + return true + } + if _, ok := m.exactAnyPort[host]; ok { + return true + } + for _, w := range m.wildcardHosts { + if w.port == port && strings.HasSuffix(host, w.suffix) { + return true + } + } + for _, suffix := range m.wildcardAnyPort { + if strings.HasSuffix(host, suffix) { + return true + } + } + return false +} + +// Empty reports whether the matcher has zero configured entries. Callers use +// this to skip starting a SOCKS5 listener when raw-TCP egress isn't configured +// — the listener is unnecessary and its port is one more thing to reason +// about at deploy time. +func (m *TCPMatcher) Empty() bool { + if m == nil { + return true + } + return len(m.exact) == 0 && + len(m.exactAnyPort) == 0 && + len(m.wildcardHosts) == 0 && + len(m.wildcardAnyPort) == 0 +} + +// parseTCPEntry splits `host:port` and validates both sides. Port `*` is +// permitted (any-port on that host). +// +// IPv6 literals must be bracketed (`[::1]:5432`) — brackets are stripped +// during parse so the stored host matches what the SOCKS5 IPv6 ATYP path +// produces (`net.IP(buf).String()` yields `::1` without brackets). This +// closes the doc/functionality gap flagged in the #355 review: pre-fix, +// `[::1]:5432` was stored with brackets and never matched at runtime. +func parseTCPEntry(raw string) (host, port string, err error) { + if raw == "" { + return "", "", fmt.Errorf("tcp allowlist: empty entry") + } + // Special-case wildcard port: `net.SplitHostPort` treats `*` as a valid + // port string (it doesn't validate numeric), so we can lean on it for + // bracket-handling on the host portion. `[::1]:*` → ("::1", "*"). + // `db.internal:*` → ("db.internal", "*"). Wildcard host with wildcard + // port `*.brokers.internal:*` also splits correctly — the `*.` prefix + // is opaque to SplitHostPort. + host, port, err = net.SplitHostPort(raw) + if err != nil { + // Reword to name the offending entry so the operator sees which + // config line failed. SplitHostPort's default error strings don't + // include the input. + return "", "", fmt.Errorf("tcp allowlist: entry %q: %w (use host:port or host:*, bracket IPv6 like [::1]:5432)", raw, err) + } + host = strings.TrimSpace(host) + port = strings.TrimSpace(port) + if host == "" { + return "", "", fmt.Errorf("tcp allowlist: entry %q has empty host", raw) + } + if port == "" { + return "", "", fmt.Errorf("tcp allowlist: entry %q has empty port", raw) + } + if port != "*" { + n, convErr := strconv.Atoi(port) + if convErr != nil { + return "", "", fmt.Errorf("tcp allowlist: entry %q has non-numeric port %q", raw, port) + } + if n < 1 || n > 65535 { + return "", "", fmt.Errorf("tcp allowlist: entry %q port %d out of range (1-65535)", raw, n) + } + } + return host, port, nil +} diff --git a/forge-core/security/tcp_matcher_test.go b/forge-core/security/tcp_matcher_test.go new file mode 100644 index 0000000..58a69a9 --- /dev/null +++ b/forge-core/security/tcp_matcher_test.go @@ -0,0 +1,199 @@ +package security + +import ( + "strings" + "testing" +) + +func TestTCPMatcher_ExactHostPort(t *testing.T) { + m, err := NewTCPMatcher([]string{"db.internal:5432"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("db.internal", "5432") { + t.Error("exact match should be allowed") + } + if m.IsAllowed("db.internal", "5433") { + t.Error("different port on same host must be denied — port granularity is the point") + } + if m.IsAllowed("other.internal", "5432") { + t.Error("different host on same port must be denied") + } +} + +func TestTCPMatcher_CaseInsensitiveHost(t *testing.T) { + m, err := NewTCPMatcher([]string{"DB.Internal:5432"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("db.internal", "5432") { + t.Error("case-insensitive host match expected") + } + if !m.IsAllowed("DB.INTERNAL", "5432") { + t.Error("case-insensitive host match expected (input uppercase)") + } +} + +func TestTCPMatcher_WildcardHostExactPort(t *testing.T) { + m, err := NewTCPMatcher([]string{"*.brokers.internal:9092"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("broker1.brokers.internal", "9092") { + t.Error("wildcard suffix should match") + } + if !m.IsAllowed("cluster-a.brokers.internal", "9092") { + t.Error("wildcard suffix should match deeper") + } + if m.IsAllowed("brokers.internal", "9092") { + t.Error("bare parent domain must NOT match *.brokers.internal (parity with DomainMatcher)") + } + if m.IsAllowed("broker1.brokers.internal", "9093") { + t.Error("port must match exactly for wildcard entries too") + } +} + +func TestTCPMatcher_AnyPort(t *testing.T) { + m, err := NewTCPMatcher([]string{"db.internal:*"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("db.internal", "5432") { + t.Error("host:* should match any port") + } + if !m.IsAllowed("db.internal", "65535") { + t.Error("host:* should match high port") + } + if m.IsAllowed("other.internal", "5432") { + t.Error("different host must not match") + } +} + +func TestTCPMatcher_WildcardHostAnyPort(t *testing.T) { + m, err := NewTCPMatcher([]string{"*.internal:*"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("db.internal", "5432") { + t.Error("*.internal:* should match db.internal:5432") + } + if !m.IsAllowed("broker1.brokers.internal", "9092") { + t.Error("*.internal:* should match broker1.brokers.internal:9092") + } + if m.IsAllowed("external.com", "5432") { + t.Error("*.internal:* must not match external.com") + } +} + +func TestTCPMatcher_Empty(t *testing.T) { + m, err := NewTCPMatcher(nil) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.Empty() { + t.Error("nil entries → Empty() true") + } + if m.IsAllowed("any", "80") { + t.Error("empty matcher denies everything") + } + + // Nil pointer receiver must be safe — callers pass nil when TCPMatcher + // isn't configured (SetTCPMatcher never called). + var nilM *TCPMatcher + if !nilM.Empty() { + t.Error("nil matcher → Empty() true") + } + if nilM.IsAllowed("db.internal", "5432") { + t.Error("nil matcher must deny (fail closed)") + } +} + +// TestTCPMatcher_IPv6Literal pins the fix from the #355 review: +// bracketed IPv6 literals in the config must round-trip to the +// unbracketed form that the SOCKS5 IPv6 ATYP path produces at runtime +// (`net.IP(buf).String()` → `::1`, no brackets). Pre-fix the entry was +// stored with brackets and never matched, silently denying legitimate +// IPv6 targets. +func TestTCPMatcher_IPv6Literal(t *testing.T) { + t.Run("bracketed loopback IPv6 matches unbracketed runtime form", func(t *testing.T) { + m, err := NewTCPMatcher([]string{"[::1]:5432"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("::1", "5432") { + t.Error("[::1]:5432 should match runtime host ::1 on port 5432") + } + if m.IsAllowed("[::1]", "5432") { + t.Error("bracketed host at runtime must not match — SOCKS5 never produces brackets") + } + if m.IsAllowed("::1", "5433") { + t.Error("port granularity holds for IPv6 targets too") + } + }) + + t.Run("bracketed IPv6 with any-port", func(t *testing.T) { + m, err := NewTCPMatcher([]string{"[2001:db8::1]:*"}) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + if !m.IsAllowed("2001:db8::1", "5432") { + t.Error("bracketed IPv6 with :* should match any port") + } + }) +} + +func TestTCPMatcher_InvalidEntries(t *testing.T) { + cases := []struct { + name string + entry string + }{ + {"no port", "db.internal"}, + {"empty host", ":5432"}, + {"empty port", "db.internal:"}, + {"non-numeric port", "db.internal:pgsql"}, + {"port zero", "db.internal:0"}, + {"port too high", "db.internal:99999"}, + {"port negative", "db.internal:-1"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := NewTCPMatcher([]string{tc.entry}) + if err == nil { + t.Fatalf("expected error for %q", tc.entry) + } + if !strings.Contains(err.Error(), tc.entry) { + t.Errorf("error should name the bad entry %q, got: %v", tc.entry, err) + } + }) + } +} + +func TestTCPMatcher_MultipleEntries(t *testing.T) { + m, err := NewTCPMatcher([]string{ + "db.internal:5432", + "redis.internal:6379", + "*.brokers.internal:9092", + "metrics.internal:*", + }) + if err != nil { + t.Fatalf("NewTCPMatcher: %v", err) + } + cases := []struct { + host, port string + allowed bool + }{ + {"db.internal", "5432", true}, + {"db.internal", "5433", false}, + {"redis.internal", "6379", true}, + {"broker1.brokers.internal", "9092", true}, + {"broker1.brokers.internal", "9091", false}, + {"metrics.internal", "9090", true}, + {"metrics.internal", "3000", true}, + {"outside.example.com", "5432", false}, + } + for _, tc := range cases { + if got := m.IsAllowed(tc.host, tc.port); got != tc.allowed { + t.Errorf("IsAllowed(%q, %q) = %v, want %v", tc.host, tc.port, got, tc.allowed) + } + } +} diff --git a/forge-core/security/types.go b/forge-core/security/types.go index 6442002..11f45f5 100644 --- a/forge-core/security/types.go +++ b/forge-core/security/types.go @@ -31,4 +31,8 @@ type EgressConfig struct { // should run these through ParsePrivateCIDRs to obtain the []*net.IPNet // the SafeDialer/EgressProxy constructors expect. AllowedPrivateCIDRs []string `json:"allowed_private_cidrs,omitempty"` + // AllowedTCP is the resolved raw-TCP allowlist (`host:port` entries). + // Callers pass this to NewTCPMatcher when wiring the EgressProxy's + // SOCKS5 listener. Nil / empty → no SOCKS5 listener is bound. + AllowedTCP []string `json:"allowed_tcp,omitempty"` } diff --git a/forge-core/types/config.go b/forge-core/types/config.go index e36f2c3..85864fb 100644 --- a/forge-core/types/config.go +++ b/forge-core/types/config.go @@ -877,6 +877,13 @@ type EgressRef struct { // 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"` + // AllowedTCP is the raw-TCP allowlist for the SOCKS5 egress path + // (databases, message brokers — the #337 use cases). Entries have the + // shape `host:port` or `host:*`; host supports the same exact + + // `*.suffix` wildcard rules as AllowedDomains. HTTP/HTTPS is still + // governed by AllowedDomains — this list is only consulted for the + // raw-TCP path. When empty, no SOCKS5 listener is bound. + AllowedTCP []string `yaml:"allowed_tcp,omitempty"` } // SkillsRef references a skills definition file.