feat(security): SOCKS5 raw-TCP egress with port-aware allowlist — closes #337 (PR 2/2)#355
feat(security): SOCKS5 raw-TCP egress with port-aware allowlist — closes #337 (PR 2/2)#355naveen-kurra wants to merge 2 commits into
Conversation
#337) Adds `egress.allowed_tcp` for raw-TCP egress (databases, message brokers) governed by the same allowlist engine that gates HTTP/HTTPS. The proxy now binds a second listener speaking SOCKS5v5 alongside the HTTP forward proxy — one policy, one audit shape, two client protocols. Skill subprocesses reach it via ALL_PROXY / SOCKS_PROXY env vars. Why the enterprise segment needs this: `libpq`/`redis-cli`/`kafka-*` etc. open raw sockets — they ignore HTTP_PROXY entirely, so today they either bypass egress (in dev) or fail closed (in locked deploys). Neither is usable. Layered on top of PR 1's `allowed_private_cidrs`, this lets an agent reach exactly `db.internal:5432` on `10.20.0.0/16` and nothing else. Key pieces: - forge-core/security/tcp_matcher.go — new port-aware matcher. Entries: `host:port`, `*.suffix:port`, `host:*`, `*.suffix:*`. Same case-insensitive wildcard-suffix rules as DomainMatcher. Invalid entries fail at Resolve() time, not at first dial. - forge-core/security/socks5.go — minimal RFC 1928 CONNECT-only server (~180 LOC, no external dep). DOMAIN/IPv4/IPv6 ATYP. BIND + UDP ASSOCIATE rejected with REP=0x07 (small explicit surface). - forge-core/security/dial_gate.go — new `ValidateAndDial` primitive shared by handleConnect and handleSOCKS5. One codepath for policy + audit + dial, no drift possible. - forge-core/security/egress_proxy.go — binds a second listener when `allowed_tcp` is non-empty (skips it otherwise — no extra port for deployments that don't need raw-TCP). `SOCKSURL()` returns the `socks5h://…` URL; the `h` forces server-side hostname resolution so the proxy can run the allowlist check against the hostname the client intended, not an IP it pre-resolved. - forge-cli/tools/exec.go — injects ALL_PROXY / SOCKS_PROXY (lowercase + uppercase) into skill subprocess env when SOCKSURL is set. Same lifecycle as HTTP_PROXY. - forge-cli/runtime/runner.go — wires the TCP matcher into the proxy and threads socksURL through registerSkillTools → SkillCommandExecutor and through RunSkillScriptTool. Backward compatibility: - HTTP audit shape unchanged (hostname-only Domain field) — the SOCKS5 path records `host:port` since port is the point. Callers key by hostname keep working; SOCKS5 event consumers get the granularity they need. - Existing tests untouched; new tests cover TCPMatcher (exact/wildcard/any- port), SOCKS5 end-to-end (echo round-trip, denied-not-in-allowlist, HTTP-allowlist-also-covers-TCP, BIND-rejected, malformed-greeting, listener-absent-without-config, empty-matcher-disables-listener, socks5h-scheme). Limitations documented in docs/security/egress-control.md: - SOCKS5v5 (no-auth) has no channel for per-request task attribution — raw-TCP audit events carry host:port + decision only, no task/correlation IDs. HTTP path retains #338 attribution via Proxy-Authorization. - `proxychains-ng` (for CLIs that don't speak SOCKS5 natively) works on Linux glibc/musl; macOS is mostly out (SIP + DYLD), static Go binaries and setuid contexts are out. Depends on: PR #348 (allowed_private_cidrs — the SafeDialer signature + `AllowedPrivateCIDRs` on EgressConfig this PR builds on). Closes: #337
Drives 100 parallel echo round-trips through one SOCKS5 proxy with distinct per-session payloads to catch cross-session byte leakage or handshake race conditions. Second test opens 50 sessions and immediately closes each to prove the relay goroutines exit cleanly instead of leaking on client-side close. Both are skipped under -short; the real signal is running them with -race. Ran green with `go test -race -count=10` on the whole security package.
faf15e7 to
004a88a
Compare
|
Rebased onto the new main after #348 landed. Only one file conflicted — Post-rebase:
Ready for review pass 2. |
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: SOCKS5 raw-TCP egress with port-aware allowlist (PR 2/2 for #337)
A new no-auth SOCKS5 listener is a serious attack surface, so I read it adversarially — bind address, dial chokepoint, localhost bypass, wildcard matching, wire parser. This is genuinely well-built. No High/Medium security issues — a few Low/doc items and one accepted limitation to confirm. Merge-ready pending the #348 rebase (the PR currently shows CONFLICTING because it carries the #348 commit).
What I verified holds (the things that could have gone badly wrong)
- Both listeners bind
127.0.0.1:0— loopback-only. The SOCKS5 proxy is not an open proxy; only in-pod processes reach it. For a no-auth SOCKS5 server this is the single most important property, and it's correct. ValidateAndDialis a real shared chokepoint. HTTP CONNECT and SOCKS5 both route through it; the SOCKS5 handler has no other dial path. Every non-localhost dial goes through SafeDialer (SSRF + private-CIDR + always-blocked-metadata + strict-IP) after the allowlist — matcher is policy, SafeDialer is IP-safety, layered.IsLocalhostis exact (== "localhost", strict-IPv4 loopback, IPv6 loopback), solocalhost.evil.com/127.0.0.1.evil.comare NOT treated as localhost — the bypass can't be tricked into a DNS-resolved external dial, and octal/hex are guarded byParseStrictIPv4. The bypass grants no new capability anyway (in-pod processes reach loopback directly).- TCP matcher wildcard keeps the leading dot (
.brokers.internal), soevilbrokers.internalcan't slip past*.brokers.internal. Entries validated atResolve()load time; nil matcher fails closed. - SOCKS5 parser is robust: bounded reads (domain ≤255, no unbounded alloc), 15s handshake deadline vs slow-loris (cleared before the long-lived relay), CONNECT-only, BIND/UDP rejected with
REP=0x07, unsupported ATYP0x08. socks5h://forces server-side DNS so the allowlist runs on the intended hostname, not a pre-resolved IP — closes the pre-resolve laundering gap.- Wiring is correct where mounted:
SetTCPMatcherbeforeStart;socksURLthreaded to skill executors; env injection gated on non-emptySOCKSURL(no leak when off), on the same minimal-env model that keeps secrets out of subprocesses. - Excellent test depth, incl.
-racestress (100 parallel sessions, goroutine-leak detection). CI fully green.
Findings — all Low / doc (see inline)
- Low (functionality + doc mismatch): IPv6-literal targets in
allowed_tcpdon't work as documented — the bracketed[::1]:5432form is stored with brackets but the SOCKS5 IPv6 ATYP yields::1(no brackets), so it never matches. Fail-closed (safe), but the doc example is non-functional. - Low (awareness/doc): the HTTP
allowed_hostsallowlist authorizes raw-TCP on any port to that host (matches pre-existing CONNECT behavior, but worth an explicit callout now that SOCKS5 makes it raw-TCP). - Nit: the localhost dial isn't ctx-aware (uses
net.DialTimeoutwhile the non-localhost path threads ctx intoSafeDialContext).
Accepted limitation worth confirming
- SOCKS5 audit has no task attribution (no-auth has no per-request credential channel) — documented and reasonable for the MVP, but since the audit story is #337's selling point, unattributed raw-TCP egress is a real observability gap. Just confirm the SOCKS5-auth-attribution follow-up is tracked; enterprises will eventually ask "which task reached the database?"
Great contribution — the shared-primitive discipline (ValidateAndDial, no two-codepath drift), loopback-only binding, and the leading-dot wildcard are exactly the details that make a SOCKS5 gate safe rather than a footgun.
| } | ||
| // SplitN on the LAST colon so IPv6 literals in brackets could be supported | ||
| // later without a rewrite. For now, IPv6 must be bracketed: `[::1]:5432`. | ||
| i := strings.LastIndex(raw, ":") |
There was a problem hiding this comment.
Low (functionality + doc mismatch). The comment above says IPv6 must be bracketed ([::1]:5432), but bracketed entries never match at runtime: LastIndex(":") on [::1]:5432 stores host [::1] (with brackets), while the SOCKS5 IPv6 ATYP path builds the host via net.IP(buf).String() → ::1 (no brackets), so IsAllowed's exact/suffix compare can't hit. (Unbracketed IPv6 like ::1:5432 happens to match by luck of the last-colon split, but a general 2001:db8::1:5432 is ambiguous.) Net: IPv6-literal targets are effectively unsupported and the documented [::1]:5432 example is non-functional. It's fail-closed, so not a security hole — but either normalize brackets on both sides (strip on parse, or net.SplitHostPort which handles [..]), or document IPv6 literals as unsupported for now so the doc doesn't promise a form that silently denies.
| return nil, fmt.Errorf("egress: %w", err) | ||
| } | ||
|
|
||
| allowed := p.matcher.IsAllowed(host) || (p.tcpMatcher != nil && p.tcpMatcher.IsAllowed(host, port)) |
There was a problem hiding this comment.
Low (awareness/doc). p.matcher.IsAllowed(host) is the port-agnostic HTTP allowlist, so a host in allowed_hosts is reachable over SOCKS5 on any port — listing api.stripe.com for HTTPS also grants arbitrary-protocol raw TCP to api.stripe.com:22, :3389, etc. This matches the pre-existing CONNECT behavior (CONNECT already checked hostname-only), so it's not newly introduced — SafeDialer still bounds it to non-internal IPs. But once raw-TCP egress is on, operators may not expect allowed_hosts to be raw-TCP-any-port. Worth an explicit line in the egress doc so allowed_hosts isn't read as HTTPS-only. (Also minor, L81-83: the localhost branch dials via net.DialTimeout, ignoring ctx — the non-localhost path threads ctx into SafeDialContext; thread it here too for cancellation symmetry.)
| // 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) |
There was a problem hiding this comment.
Accepted-limitation confirm (not blocking): because SOCKS5v5 no-auth has no per-request credential channel, this dial carries context.Background() and no identity — so raw-TCP audit events have host:port + decision but no task/correlation ID, unlike the HTTP CONNECT path (which recovers identity from Proxy-Authorization, #338). Reasonable for the MVP and documented, but since per-task attribution of egress is #337's whole value prop, an unattributed raw-TCP event ("something reached db.internal:5432") is a weaker audit signal than the HTTP path. Please make sure the SOCKS5-auth-attribution follow-up is filed as a tracked issue rather than only a docs note — enterprises will ask "which task opened that DB connection?"
2 participants
Summary
egress.allowed_tcpand binds a SOCKS5 listener alongside the existing HTTP forward proxy. Skill subprocesses reach it viaALL_PROXY/SOCKS_PROXYenv vars.ValidateAndDial— one allowlist policy, one audit shape, two client protocols. No two-codepath drift.allowed_private_cidrs): rebase this on top of that PR before merging.Merge order
fix/egress-private-cidrs)fix/egress-socks5-tcp)What lands
Config additions:
Code:
forge-core/security/tcp_matcher.gohost:port,*.suffix:port,host:*,*.suffix:*); validates at Resolve() timeforge-core/security/socks5.goforge-core/security/dial_gate.goValidateAndDialprimitive shared by handleConnect + handleSOCKS5forge-core/security/egress_proxy.goallowed_tcpnon-empty;SOCKSURL()returnssocks5h://…forge-cli/tools/exec.goALL_PROXY/SOCKS_PROXYenv vars alongsideHTTP_PROXYforge-cli/runtime/runner.godocs/security/egress-control.mdFail-closed invariants (from the ticket, verified by tests)
db.internal:5432permits only port 5432; :5433 is denied.socks5h://scheme. Forces server-side hostname resolution — clients that pre-resolve get the IP path in SafeDialer.host:portbecause port is the point.allowed_tcpempty. Zero-config deploys see no extra port + noALL_PROXY.Test plan
go test ./forge-core/...— all pass (including new TCPMatcher + SOCKS5 suites)go test ./forge-cli/...— all pass (including regression on browser tools' HTTP audit shape)go test ./forge-plugins/...— all passgolangci-lint run ./forge-core/... ./forge-cli/... ./forge-plugins/...— 0 issuesgofmt -lcleanNew tests:
socks5h://allowed_tcpvalidated at load, invalid host:port fails ResolveKnown limitations (documented)
proxychains-ngcaveats. Linux glibc/musl works. macOS mostly doesn't (SIP + DYLD). Static Go binaries and setuid contexts don't. Language-native SOCKS5 SDKs are the primary UX for CLIs.Closes: #337
Realtime end-to-end test — 19 angles across 4 dimensions
Beyond the unit test suite: booted the real EgressProxy in-process with two live listeners (HTTP + SOCKS5), real TCP + HTTP echo servers, real
curlsubprocesses, and a Go client speaking the SOCKS5 wire byte-for-byte. All 19 angles passed.-race -count=10 -run "SOCKS5|TCPMatcher|PrivateCIDR")-raceallowed_tcp— SOCKS5 listener not boundsocks5h://(server-side DNS)host:portSkillCommandExecutorenv injection: (a) SOCKSURL set →ALL_PROXY/all_proxy/SOCKS_PROXYall set; (b) SOCKSURL empty → no leak; (c)HTTP_PROXYunaffectedCross-module
-racesweep clean onforge-core/security/…,forge-cli/tools/…,forge-cli/runtime/— no data races, no goroutine leaks, no test flake across repeated runs.Not testable locally (documented):
proxychains-ngbehavior on macOS (SIP blocks DYLD_INSERT_LIBRARIES) — noted in the docs client-support matrix