Substrate

[aojea]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces the Secure Outbound Gateway (sam-box) and the nano-init init system to manage, secure, and inject credentials into outbound traffic from isolated AI agent sandboxes. The review feedback highlights several critical and high-severity issues that must be addressed, including a Biscuit policy injection vulnerability in the gateway handler, an empty hub public key after enrollment in sam-box, a potential nil pointer dereference in nano-init's command cancellation, and a race condition between the zombie reaper and the main process wait loop. Additionally, improvements are suggested to prevent unbounded dial timeouts and to ensure proper command context propagation during startup.

[aojea]

/gemini review again and check comments are addressed

[gemini-code-assist]

Code Review

This pull request introduces sam-box, a Secure Outbound Gateway daemon, and nano-init, a minimal init system designed to run as PID 1 in containerized sandboxes to proxy traffic over a Unix Domain Socket. Feedback on these changes highlights a race condition in nano-init's exit code propagation, security concerns regarding hardcoded HTTP scheme downgrades and insecure file permissions on the Unix socket, and several robustness improvements, including stripping ports from target hosts during secret lookups, escaping control characters in Biscuit strings, and using the command context instead of context.Background().

[gemini-code-assist]

Use the command's context ctx instead of context.Background() when calling node.SyncHubConfig. This ensures that if the command is cancelled or timed out, the sync operation is gracefully aborted.

Suggested change

	storedPubKey, syncedAddrs, err := node.SyncHubConfig(ctx, store)
	storedPubKey, syncedAddrs, err := node.SyncHubConfig(ctx, store)

[gemini-code-assist]

Use the command's context ctx instead of context.Background() when discovering OIDC endpoints and fetching the JWT. This allows the operations to be cancelled if the process receives a shutdown signal during startup.

Suggested change

	dummyNode := &node.SamNode{}
	tokenURL, err := dummyNode.DiscoverTokenURL(ctx, oidcIssuerFlag)
	if err != nil {
	logger.Fatalf("Failed to discover OIDC endpoints: %v", err)
	}
	logger.Info("Fetching JWT via OIDC Client Credentials...")
	jwtStr, err = dummyNode.FetchJWT(ctx, tokenURL, clientIDFlag, clientSecretFlag)
	dummyNode := &node.SamNode{}
	tokenURL, err := dummyNode.DiscoverTokenURL(ctx, oidcIssuerFlag)
	if err != nil {
	logger.Fatalf("Failed to discover OIDC endpoints: %v", err)
	}
	logger.Info("Fetching JWT via OIDC Client Credentials...")
	jwtStr, err = dummyNode.FetchJWT(ctx, tokenURL, clientIDFlag, clientSecretFlag)

[gemini-code-assist]

When creating a Unix Domain Socket on the file system, it is created with default permissions based on the process's umask (often allowing read/write access to other users or groups). Since this socket is used for secure outbound gateway proxying and handles sensitive credentials, we should restrict its permissions to 0600 (or 0660 if group access is required) to prevent unauthorized local users from connecting to it.

			listener, err := net.Listen("unix", udsPathFlag)
			if err != nil {
				logger.Fatalf("Failed to listen on UDS socket: %v", err)
			}
			if err := os.Chmod(udsPathFlag, 0600); err != nil {
				logger.Fatalf("Failed to set permissions on UDS socket: %v", err)
			}

[aojea]

/gemini review again

[gemini-code-assist]

Code Review

This pull request introduces the Secure Outbound Gateway (sam-box) and a minimal init system (nano-init) to enforce access control policies and inject credentials for outbound sandbox traffic. The review feedback highlights several key issues: a potential failure in the canary template due to package manager requests being blocked by the proxy, flakiness and premature connection termination in nano-init's process and connection handling, authorization mismatches in sam-box caused by unnormalized destination ports, case-sensitivity issues in secret kind matching, and a brief security race window during Unix Domain Socket creation.

[gemini-code-assist]

The agent container attempts to run apk add --no-cache curl at startup. However, because nano-init configures HTTP_PROXY to route all traffic through sam-box, and sam-box strictly requires a valid Biscuit token in the Authorization header, the apk package manager's requests to Alpine repositories will be rejected with 401 Unauthorized. This will cause the container to fail. Consider using an image that already has curl pre-installed (e.g., alpine/curl) to avoid runtime package installation through the secure proxy.

        image: alpine/curl:8.12.1
        command:
          - "/shared-bin/nano-init"
          - "/var/run/sam/sam-box.sock"
          - "sh"
          - "-c"
          - "while true; do curl -v -H 'Authorization: Bearer dummy-token' http://example.com/test; sleep 10; done"

[gemini-code-assist]

The destination fact is created using targetHost, which may contain a port (e.g., example.com:443 or example.com:80). If a Biscuit token is minted with a host-only right (e.g., example.com), the authorization will fail because the destination fact (example.com:443) won't match the right's destination (example.com). Consider normalizing targetHost by stripping the port before creating the destination fact, similar to how it is handled for the secretStore lookup.

		destHost, _, err := net.SplitHostPort(targetHost)
		if err != nil {
			destHost = targetHost
		}
		destinationFact, err := parser.FromStringFact(fmt.Sprintf("destination(\"%s\")", escapeBiscuitString(destHost)))

[gemini-code-assist]

In handleConnection, when one of the io.Copy goroutines finishes (for example, if the client closes its write side), <-errc unblocks and the function exits, immediately closing both client and uds connections. This can lead to premature connection termination and data loss if the other direction still had pending data to copy (e.g., a response from the UDS server). Consider propagating the half-close (e.g., using CloseWrite() if the connection supports it) and waiting for both directions to complete before fully closing the sockets.

[gemini-code-assist]

The secret.Kind matching is case-sensitive (e.g., it expects exactly "Bearer", "BasicAuth", or "CustomHeader"). If an operator configures kind: bearer (lowercase) in secrets.yaml, it will silently fail to inject the secret. Consider normalizing the kind to lowercase or performing case-insensitive matching to prevent silent configuration errors.

Suggested change

	switch secret.Kind {
	case SecretKindBearer:
	r.Header.Set("Authorization", "Bearer "+secret.Value)
	case SecretKindBasicAuth:
	r.Header.Set("Authorization", "Basic "+secret.Value)
	case SecretKindCustomHeader:
	r.Header.Set(secret.HeaderName, secret.Value)
	}
	switch strings.ToLower(string(secret.Kind)) {
	case "bearer":
	r.Header.Set("Authorization", "Bearer "+secret.Value)
	case "basicauth":
	r.Header.Set("Authorization", "Basic "+secret.Value)
	case "customheader":
	r.Header.Set(secret.HeaderName, secret.Value)
	}

[gemini-code-assist]

When net.Listen("unix", ...) is called, the socket file is created using the default system umask (often allowing group or world access) before os.Chmod restricts it to 0600. This creates a brief race window where other local users could connect to the socket. To prevent this, consider setting a restrictive umask (e.g., syscall.Umask(0077)) before listening, or ensuring the parent directory is restricted to 0700.

[aojea]

/Gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Secure Outbound Gateway (sam-box) and a minimal init system (nano-init) to enforce access control policies and inject credentials for outbound traffic from isolated AI agent sandboxes. Feedback on the implementation highlights a potential race condition in nano-init when waiting for background zombie-reaping, which can be resolved by blocking on the channel read. Additionally, using logger.Fatalf in sam-box's Cobra command bypasses deferred resource cleanups, so refactoring to RunE is recommended. Finally, the gateway's secret injection should support case-insensitive matching, and its loopback scheme-matching logic should be extended to include the IPv6 loopback address (::1).

[gemini-code-assist]

Using logger.Fatalf (which internally calls os.Exit(1)) inside the Cobra Run function after resources have been allocated prevents deferred functions from executing.

For example, if node.LoadNodeConfig fails on line 124, logger.Fatalf is called, and the deferred store.Close() on line 127 is bypassed. This can leave the bbolt database file locked or corrupted. Similarly, any failures during server startup will bypass the cleanup of the Unix Domain Socket file (os.Remove(udsPathFlag)).

Recommendation:
Refactor the Cobra command to use RunE instead of Run, allowing you to return errors and handle them gracefully in main, or avoid using logger.Fatal once resources like the database store or network listeners have been initialized.

[gemini-code-assist]

The gateway currently checks if the host is "localhost" or "127.0.0.1" to decide whether to use http instead of https. However, it does not check for the IPv6 loopback address "::1".

If an agent attempts to connect to an IPv6 loopback address, the gateway will incorrectly upgrade the scheme to https, which will fail if the local service is only listening on plain HTTP.

Suggested change

	if host == "localhost" || host == "127.0.0.1" {
	req.URL.Scheme = "http"
	} else {
	req.URL.Scheme = "https"
	}
	if host == "localhost" || host == "127.0.0.1" || host == "::1" {
	req.URL.Scheme = "http"
	} else {
	req.URL.Scheme = "https"
	}

[aojea]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the Secure Outbound Gateway (sam-box) and the nano-init init system to secure outbound traffic from AI agent sandboxes, along with configurable DHT options and paginated service discovery. Key feedback highlights a critical path traversal vulnerability in the interceptor bootstrap endpoint, compilation failures on Windows due to syscall.Umask, and potential integer overflow panics in pagination. Additionally, improvements are needed to handle temporary socket accept and UDP read errors gracefully, prevent data loss in the TLS upgrade path, fix a race condition in child process reaping, and correct the IPv6 loopback mapping in the DNS spoofer.

[aojea]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the Secure Outbound Gateway (sam-box) and the nano-init init system to enforce zero-trust access control and transparent credential injection for containerized agent sandboxes. It also adds DHT performance tuning flags to sam-node and sam-router, and implements pagination for remote service discovery. The review feedback highlights critical issues that need to be addressed, including a potential silent hang and hardcoded credentials in the gateway's server loop, a path traversal vulnerability when loading interceptor binaries, a thread-safety race condition in the C socket interceptor, and a minor optimization opportunity in the DNS spoofer's IPv6 address handling.

[gemini-code-assist]

This implementation has two major issues:

1. Silent Failure / Goroutine Leak: If server.Serve(tlsListener) fails to start or crashes, the error is sent to a local channel but never handled. The Serve function continues to block on listener.Accept(), leading to a silent hang where connections are accepted but never processed.
2. Hardcoded Credentials: The director function always injects a hardcoded "Bearer mock-token" instead of looking up and injecting the configured credentials from g.SecretStore.

This combined suggestion resolves both issues by propagating the server error to unblock listener.Accept() and implementing the secret lookup logic.

func (g *Gateway) Serve(listener net.Listener) error {
	tlsListener := &channelListener{
		conns:  make(chan net.Conn, 100),
		closed: make(chan struct{}),
	}
	defer func() { _ = tlsListener.Close() }()

	director := func(req *http.Request) {
		req.URL.Scheme = "https"
		req.URL.Host = req.Host
		if config, ok := g.SecretStore[req.Host]; ok {
			switch config.Kind {
			case SecretKindBearer:
				req.Header.Set("Authorization", "Bearer "+config.Value)
			case SecretKindBasicAuth:
				req.Header.Set("Authorization", "Basic "+config.Value)
			case SecretKindCustomHeader:
				if config.HeaderName != "" {
					req.Header.Set(config.HeaderName, config.Value)
				}
			}
		} else {
			req.Header.Set("Authorization", "Bearer mock-token")
		}
	}

	proxy := &httputil.ReverseProxy{
		Director:  director,
		Transport: g.Transport,
	}

	server := &http.Server{
		Handler: proxy,
	}

	var serverErr error
	var errMu sync.Mutex

	go func() {
		if err := server.Serve(tlsListener); err != nil && !errors.Is(err, http.ErrServerClosed) {
			errMu.Lock()
			serverErr = err
			errMu.Unlock()
			_ = listener.Close()
		}
	}()

	for {
		rawConn, err := listener.Accept()
		if err != nil {
			errMu.Lock()
			sErr := serverErr
			errMu.Unlock()
			if sErr != nil {
				return sErr
			}

			select {
			case <-tlsListener.closed:
				return nil
			default:
				if errors.Is(err, net.ErrClosed) {
					return nil
				}
				log.Printf("Accept error: %v; retrying in 50ms", err)
				time.Sleep(50 * time.Millisecond)
				continue
			}
		}

		go g.handleConnection(rawConn, tlsListener)
	}
}

[aojea]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the Secure Outbound Gateway (sam-box) and the nano-init init system to enforce zero-trust outbound security and transparent credential injection for isolated agent sandboxes. It also adds pagination to remote service discovery and exposes several new DHT configuration options in sam-node and sam-router. The feedback identifies critical security and stability issues in the gateway and init system, including multiple slowloris and CPU-exhaustion Denial of Service (DoS) vulnerabilities, potential connection and goroutine leaks in the proxy forwarding logic, a port-matching bug in secret injection, and a thread-safety issue in the gateway's internal listener.

[gemini-code-assist]

Calling tlsConn.Handshake() without setting a deadline on the underlying connection makes the gateway vulnerable to slowloris-style Denial of Service (DoS) attacks. A client can open a connection, send a single byte of the TLS ClientHello, and block the handshake goroutine indefinitely. Setting a reasonable handshake deadline (e.g., 10 seconds) and clearing it afterward prevents resource exhaustion.

func (g *Gateway) handleConnection(rawConn net.Conn, tlsListener *channelListener) {
	br := bufio.NewReader(rawConn)
	peekBytes, err := br.Peek(5)
	if err != nil {
		_ = rawConn.Close()
		return
	}

	conn := &bufferedConn{
		Conn: rawConn,
		r:    br,
	}

	if len(peekBytes) > 0 && peekBytes[0] == 0x16 {
		tlsConfig := &tls.Config{
			GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) {
				sni := info.ServerName
				if sni == "" {
					return nil, fmt.Errorf("missing SNI")
				}
				return g.CertCache.GetCertificate(sni, g.CA)
			},
		}
		_ = rawConn.SetDeadline(time.Now().Add(10 * time.Second))
		tlsConn := tls.Server(conn, tlsConfig)
		if err := tlsConn.Handshake(); err != nil {
			_ = tlsConn.Close()
			return
		}
		_ = rawConn.SetDeadline(time.Time{})

		select {
		case tlsListener.conns <- tlsConn:
		case <-tlsListener.closed:
			_ = tlsConn.Close()
		}
	} else {
		g.handleHTTPConnection(conn, tlsListener)
	}
}