sandbox: flip the boundary to named HTTP tunnels via tun2connect - #346
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the sandbox boundary protocol from SOCKS5 to named HTTP tunnels (CONNECT for TCP and connect-udp for UDP), replacing tun2socks with tun2connect in nano-init and updating sam-box to serve HTTP CONNECT. It also transitions the synthetic DNS pools from link-local to CGNAT and discard prefixes to prevent SSRF blocks. Feedback on the changes highlights a potential HTTP response splitting vulnerability in the error response headers, a parsing bug with escaped slashes in MASQUE UDP paths, fragile Proxy-Authorization header parsing, and a script termination risk in the installation script due to the addition of pipefail.
| @@ -1,5 +1,5 @@ | |||
| #!/usr/bin/env bash | |||
| set -e | |||
| set -euo pipefail | |||
There was a problem hiding this comment.
Adding pipefail combined with set -e will cause the script to exit immediately if any command in a pipeline fails. In VERSION=$(curl -s $LATEST_RELEASE_URL | grep '"tag_name":' | sed ...) (line 28), if grep finds no match (e.g., due to rate limits or API errors), it exits with status 1. With pipefail and set -e, this will terminate the script silently, preventing the error handling block on line 30 from executing. Consider appending || true to the pipeline on line 28, or handling pipeline failures gracefully.
| case req.Method == http.MethodConnect: | ||
| dst, err = connectDestination(req.Host) | ||
| case req.Method == http.MethodGet && strings.EqualFold(req.Header.Get("Upgrade"), "connect-udp"): | ||
| dst, err = masqueDestination(req.URL.Path) |
There was a problem hiding this comment.
Using req.URL.Path here is problematic because Go's net/http parser automatically unescapes the path (including %2F to /) before populating URL.Path. If the target host contains an escaped slash, strings.Split in masqueDestination will split on the unescaped slash, changing the segment count and failing the validation. Use req.URL.EscapedPath() instead to preserve the escaped characters during splitting, which are then correctly unescaped by url.PathUnescape inside masqueDestination.
| dst, err = masqueDestination(req.URL.Path) | |
| dst, err = masqueDestination(req.URL.EscapedPath()) |
| func parseProxyBasicAuth(header string) *Credentials { | ||
| const prefix = "Basic " | ||
| if len(header) < len(prefix) || !strings.EqualFold(header[:len(prefix)], prefix) { | ||
| return nil | ||
| } | ||
| decoded, err := base64.StdEncoding.DecodeString(header[len(prefix):]) | ||
| if err != nil { |
There was a problem hiding this comment.
Parsing the Proxy-Authorization header by slicing with a fixed prefix length is fragile if there are multiple spaces or leading/trailing whitespace (e.g., Basic <base64>). A more robust approach is to split the header into fields using strings.Fields and validate that the first field is case-insensitively equal to Basic.
| func parseProxyBasicAuth(header string) *Credentials { | |
| const prefix = "Basic " | |
| if len(header) < len(prefix) || !strings.EqualFold(header[:len(prefix)], prefix) { | |
| return nil | |
| } | |
| decoded, err := base64.StdEncoding.DecodeString(header[len(prefix):]) | |
| if err != nil { | |
| func parseProxyBasicAuth(header string) *Credentials { | |
| fields := strings.Fields(header) | |
| if len(fields) != 2 || !strings.EqualFold(fields[0], "Basic") { | |
| return nil | |
| } | |
| decoded, err := base64.StdEncoding.DecodeString(fields[1]) |
| func writeRefusal(conn net.Conn, status int, reason string, extraHeaders ...string) { | ||
| msg := fmt.Sprintf("HTTP/1.1 %d %s\r\nBoundary-Reason: %s\r\n", | ||
| status, http.StatusText(status), reason) |
There was a problem hiding this comment.
Writing reason directly into the HTTP response headers without sanitization can lead to HTTP Response Splitting (CWE-113) if reason contains carriage return (\r) or newline (\n) characters. Since reason can contain err.Error() (which may include untrusted input from the request path or host), sanitize reason by removing or replacing newline characters before formatting the header.
func writeRefusal(conn net.Conn, status int, reason string, extraHeaders ...string) {
reason = strings.NewReplacer("\r", "", "\n", "").Replace(reason)
msg := fmt.Sprintf("HTTP/1.1 %d %s\r\nBoundary-Reason: %s\r\n",
status, http.StatusText(status), reason)
The guest side of the sandbox boundary is now the tun2connect library (github.com/aojea/agents.net/tun2connect v0.0.1): gVisor terminates the sandbox's TCP/IP in userspace and every flow leaves as an authority-form CONNECT (RFC 9110) or connect-udp (RFC 9298) tunnel carrying the destination NAME. sam-box serves the matching boundary: CONNECT for TCP, connect-udp with capsules (RFC 9297) for UDP, policy on names, deny by default. One protocol now runs in both directions, since the reverse channel already spoke CONNECT. - internal/sambox: SOCKS5Server -> ConnectServer, standalone capsule codec (the library would drag a netstack into the root module) - cmd/nano-init: tun2socks/SOCKS5 client and placeholder resolver replaced by the library; synthetic pools move to 100.64.0.0/10 and 100::/64 (SSRF guards commonly block 169.254/16), resolver at 100.127.255.253; tun0 stays the default route so a hardcoded resolver is still answered by the engine; UDP enabled - cmd/sam-box, internal/bench, cmd/sam-bench: CONNECT client and help - tests: integration suite and e2e CUJs flipped to the CONNECT wire; canary resolv.conf and its cross-check follow the new resolver - docs: agent-architecture Decision 1 rewritten as an explicit reversal (kept: names, protocol-agnostic tunnels, refusal vocabulary; gained: one protocol both directions, headers as extension point, UDP that fits one socket, curl-testability); running-agents, secure-gateway, nano-init README and AGENTS.md follow - .goreleaser.yaml: nano-init is linux-only; release sam-console, which install.sh and the helm chart already expected; sam-bench stays unreleased on purpose - site/static/install.sh: resync with the canonical root script
1 participant
The guest side of the sandbox boundary is now the tun2connect library (github.com/aojea/agents.net/tun2connect v0.0.1): gVisor terminates the sandbox's TCP/IP in userspace and every flow leaves as an authority-form CONNECT (RFC 9110) or connect-udp (RFC 9298) tunnel carrying the destination NAME. sam-box serves the matching boundary: CONNECT for TCP, connect-udp with capsules (RFC 9297) for UDP, policy on names, deny by default. One protocol now runs in both directions, since the reverse channel already spoke CONNECT.