Improve docker agent sbx

cmd/root/sandbox.go

@@ -18,7 +18,6 @@ import (
 	"github.com/spf13/pflag"
 
 	"github.com/docker/docker-agent/pkg/config"
-	"github.com/docker/docker-agent/pkg/desktop"
 	"github.com/docker/docker-agent/pkg/environment"
 	"github.com/docker/docker-agent/pkg/paths"
 	"github.com/docker/docker-agent/pkg/sandbox"
@@ -99,51 +98,38 @@ func runInSandbox(ctx context.Context, cmd *cobra.Command, args []string, runCon
 	// ...) but blocks every *.docker.com host as well as every
 	// package-registry / source-host the auto-installer reaches for.
 	// Open the minimum: the configured Docker AI gateway when set, and
-	// the package-host set only when the kit-build determined the
-	// agent has at least one MCP / LSP toolset that may auto-install.
-	needsToolInstall := kitResult != nil && kitResult.NeedsToolInstall
-	allowSandboxHosts(ctx, backend, name, runConfig.ModelsGateway, needsToolInstall)
+	// the per-toolset package hosts the kit-build resolved against the
+	// aqua registry. The kit narrows by package type (Go module proxy
+	// for go_install, GitHub releases for github_release) so we don't
+	// open holes for hosts the agent doesn't actually need.
+	var toolHosts []string
+	if kitResult != nil {
+		toolHosts = kitResult.ToolInstallHosts
+	}
+	allowSandboxHosts(ctx, backend, name, runConfig.ModelsGateway, toolHosts)
 
 	// Resolve env vars the agent needs and forward them into the sandbox.
 	// Docker Desktop proxies well-known API keys automatically; this handles
 	// any additional vars (e.g. MCP tool secrets).
 	envFlags, envVars := sandbox.EnvForAgent(ctx, agentRef, envProvider)
 
-	// Forward the gateway via the docker-agent process env (not as an
-	// inline `-e KEY=VALUE` argument) so a gateway URL that happens to
-	// carry credentials never leaks into the slog'd `docker sandbox
-	// exec` argv.
+	// Forward the gateway by name so a URL with credentials never
+	// shows up in the slog'd `docker sandbox exec` argv. We do not
+	// forward DOCKER_TOKEN: inside the sandbox it must come only from
+	// sandbox-tokens.json (kept fresh by StartTokenWriterIfNeeded).
 	if gateway := runConfig.ModelsGateway; gateway != "" {
 		envFlags = append(envFlags, "-e", envModelsGateway)
 		envVars = append(envVars, envModelsGateway+"="+gateway)
-
-		// Forward a *fresh* Docker Desktop token. We deliberately bypass
-		// envProvider here: that chain consults the OS environment first,
-		// where any pre-existing DOCKER_TOKEN value is by definition stale
-		// (the gateway issues short-lived JWTs that expire roughly
-		// hourly). Going straight to the Docker Desktop backend gives us
-		// the same fresh token that [sandbox.StartTokenWriterIfNeeded]
-		// will keep refreshing in the background; seeding it as an env
-		// var lets the inner agent's startup check
-		// ([config.CheckRequiredEnvVars]) succeed even on existing sandbox
-		// images that read sandbox-tokens.json from the wrong path because
-		// of the persistent-pre-run bug fixed in pkg/cli/flags.go.
-		//
-		// Like the gateway above, the token is forwarded by name only —
-		// it would otherwise show up in the slog'd argv as plaintext.
-		if token := desktop.GetToken(ctx); token != "" {
-			envFlags = append(envFlags, "-e", environment.DockerDesktopTokenEnv)
-			envVars = append(envVars, environment.DockerDesktopTokenEnv+"="+token)
-		}
 	}
 
-	// Point the in-sandbox resolvers at the staged kit. We use the
-	// `-e KEY=VALUE` form so the value is set directly inside the
-	// container; we deliberately do not append it to envVars (which
-	// would set it on the host docker CLI process too — a path that
-	// only makes sense inside the sandbox).
+	// Point the in-sandbox resolvers at the staged kit. The sandbox CLI
+	// exposes extra workspaces at the same path as on the host, so we
+	// forward HostDir verbatim. We use the `-e KEY=VALUE` form so the
+	// value is set directly inside the container; we deliberately do not
+	// append it to envVars (which would set it on the host docker CLI
+	// process too — a path that only makes sense inside the sandbox).
 	if kitResult != nil {
-		envFlags = append(envFlags, "-e", skills.KitDirEnv+"="+kit.MountPath)
+		envFlags = append(envFlags, "-e", skills.KitDirEnv+"="+kitResult.HostDir)
 	}
 
 	dockerCmd := backend.BuildExecCmd(ctx, name, wd, dockerAgentArgs, envFlags, envVars)
@@ -194,55 +180,31 @@ func dockerAgentArgs(cmd *cobra.Command, args []string, configDir string) []stri
 	return dockerAgentArgs
 }
 
-// autoInstallHosts is the set of hostnames the toolinstall package
-// reaches for when fetching tools at runtime: the aqua registry data
-// (raw.githubusercontent.com), the GitHub API (latest release
-// resolution), GitHub releases themselves, the redirected release
-// asset host (objects.githubusercontent.com), and the Go module
-// proxy + checksum DB used by `go install`. We allowlist this whole
-// set whenever a sandbox is launched with the kit pipeline so that
-// auto-install — which is on by default for every lsp / mcp toolset
-// — can actually fetch what it needs. Without this, missing tools
-// (gopls, golangci-lint, ...) report a misleading "403 blocked by
-// network policy" from go install / curl instead of installing.
-var autoInstallHosts = []string{
-	"github.com",
-	"api.github.com",
-	"raw.githubusercontent.com",
-	"objects.githubusercontent.com",
-	"codeload.github.com",
-	"proxy.golang.org",
-	"sum.golang.org",
-	// `go install` downloads the Go toolchain from Google's blob
-	// storage when a module's go.mod pins a newer Go than the one
-	// already in the sandbox image.
-	"storage.googleapis.com",
-}
-
 // allowSandboxHosts adds per-sandbox allow-network rules for every
 // host the in-sandbox runtime is known to need: the configured
 // models gateway (when set) and the package hosts the auto-installer
-// reaches for (when needsToolInstall is true). The default sandbox
-// proxy denies all of them; without this, the inner agent's first
-// request returns a misleading "403 Blocked by network policy".
+// reaches for (when the kit build identified at least one
+// auto-installable toolset). The default sandbox proxy denies all of
+// them; without this, the inner agent's first request returns a
+// misleading "403 Blocked by network policy".
 //
 // Holes are punched only when the corresponding feature is in play:
 //   - the gateway host is added only when gatewayURL is non-empty;
-//   - the autoInstallHosts set is added only when needsToolInstall
-//     is true (i.e. the kit build saw at least one MCP / LSP
-//     toolset that might auto-install). Sandboxes that don't run
-//     auto-install keep the strict default-deny.
+//   - the per-agent install hosts come from the kit build, which
+//     looks each toolset up against the aqua registry and contributes
+//     only the hosts that toolset's install path actually uses (Go
+//     module proxy + toolchain bootstrap for go_install packages,
+//     GitHub release hosts for github_release packages). When a
+//     lookup failed, the kit folds in [toolinstall.FallbackHosts]
+//     so the run can still succeed.
 //
 // Best-effort: a malformed gateway URL or a backend that doesn't
 // support per-sandbox policies is logged at debug level and the run
 // proceeds. The user will then see a network-policy 403 from the
 // inner and we surface that diagnostic verbatim.
-func allowSandboxHosts(ctx context.Context, backend *sandbox.Backend, name, gatewayURL string, needsToolInstall bool) {
+func allowSandboxHosts(ctx context.Context, backend *sandbox.Backend, name, gatewayURL string, toolInstallHosts []string) {
 	var hosts []string
-
-	if needsToolInstall {
-		hosts = append(hosts, autoInstallHosts...)
-	}
+	hosts = append(hosts, toolInstallHosts...)
 
 	if gatewayURL != "" {
 		if h := gatewayHostPort(gatewayURL); h != "" {
@@ -420,15 +382,27 @@ func printModelsGateway(w io.Writer, gateway string) {
 	fmt.Fprintf(w, "Models gateway: %s (allowlisting %s in the sandbox proxy)\n", display, host)
 }
 
-// printToolInstallAllowance prints a single line announcing whether
-// the package-host allowlist was opened for this sandbox, and why.
-// We surface this so the user sees what holes were punched in the
-// default-deny network policy. Silent when the kit isn't built or
-// when no auto-installable toolset was detected.
+// printToolInstallAllowance prints a multi-line description of the
+// package-host allowlist opened for this sandbox: a one-liner
+// summary followed by every host on its own indented line so the
+// user can see exactly what holes the run punched in the default-
+// deny network policy. Silent when the kit isn't built or when no
+// auto-installable toolset was detected.
+//
+// When per-toolset registry resolution failed for at least one
+// toolset, a best-effort fallback union was used instead and a
+// warning line names each unresolved toolset so the user can spot
+// why the allowlist is wider than expected.
 func printToolInstallAllowance(w io.Writer, kitResult *kit.Result) {
 	if kitResult == nil || !kitResult.NeedsToolInstall {
 		return
 	}
-	fmt.Fprintf(w, "Tool install: agent has at least one MCP/LSP toolset, allowlisting %d package hosts in the sandbox proxy\n",
-		len(autoInstallHosts))
+	fmt.Fprintf(w, "Tool install: agent has at least one MCP/LSP toolset, allowlisting %d package host(s) in the sandbox proxy:\n",
+		len(kitResult.ToolInstallHosts))
+	for _, h := range kitResult.ToolInstallHosts {
+		fmt.Fprintf(w, "  - %s\n", h)
+	}
+	for _, e := range kitResult.ToolInstallHostsResolutionErr {
+		fmt.Fprintf(w, "  ! %s (using fallback host set)\n", e.Error())
+	}
 }

cmd/root/sandbox_test.go

@@ -1,13 +1,16 @@
 package root
 
 import (
+	"errors"
 	"slices"
 	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/docker/docker-agent/pkg/sandbox/kit"
 )
 
 // TestDockerAgentArgs_NoDuplicateArgs is a regression test for a bug where the
@@ -189,33 +192,62 @@ func TestPrintModelsGateway(t *testing.T) {
 	}
 }
 
-func TestAutoInstallHosts(t *testing.T) {
+func TestPrintToolInstallAllowance(t *testing.T) {
 	t.Parallel()
 
-	// Spot-check the static set: the package hosts the auto-installer
-	// reaches at runtime (Go module proxy, GitHub releases, the toolchain
-	// blob storage backing `go install`) must all be in the allowlist
-	// or the inner agent will see "403 Blocked by network policy" with
-	// no other diagnostic.
-	required := []string{
-		"github.com",
-		"api.github.com",
-		"raw.githubusercontent.com",
-		"objects.githubusercontent.com",
-		"proxy.golang.org",
-		"sum.golang.org",
-		"storage.googleapis.com",
-	}
-	for _, host := range required {
-		assert.Contains(t, autoInstallHosts, host,
-			"%s must be in autoInstallHosts so auto-install can reach it inside the sandbox", host)
+	tests := []struct {
+		name    string
+		result  *kit.Result
+		want    string
+		wantNot []string
+	}{
+		{
+			name:   "nil result is silent",
+			result: nil,
+			want:   "",
+		},
+		{
+			name:   "no auto-install is silent",
+			result: &kit.Result{NeedsToolInstall: false},
+			want:   "",
+		},
+		{
+			name: "lists every host on its own line",
+			result: &kit.Result{
+				NeedsToolInstall: true,
+				ToolInstallHosts: []string{
+					"api.github.com", "proxy.golang.org", "sum.golang.org",
+				},
+			},
+			want: "Tool install: agent has at least one MCP/LSP toolset, allowlisting 3 package host(s) in the sandbox proxy:\n" +
+				"  - api.github.com\n" +
+				"  - proxy.golang.org\n" +
+				"  - sum.golang.org\n",
+		},
+		{
+			name: "resolution errors are surfaced after the host list",
+			result: &kit.Result{
+				NeedsToolInstall: true,
+				ToolInstallHosts: []string{"api.github.com"},
+				ToolInstallHostsResolutionErr: []kit.ToolHostError{
+					{Command: "gopls", Version: "golang/tools@v0.21.0", Err: errors.New("boom")},
+				},
+			},
+			want: "Tool install: agent has at least one MCP/LSP toolset, allowlisting 1 package host(s) in the sandbox proxy:\n" +
+				"  - api.github.com\n" +
+				"  ! resolving install hosts for \"gopls\"@\"golang/tools@v0.21.0\": boom (using fallback host set)\n",
+			wantNot: []string{},
+		},
 	}
 
-	// And the list itself must be commaless / spaceless: AllowHosts
-	// rejects entries that look like they could smuggle several rules
-	// past the policy engine.
-	for _, host := range autoInstallHosts {
-		assert.NotContains(t, host, ",", "%q must not contain a comma", host)
-		assert.NotContains(t, host, " ", "%q must not contain whitespace", host)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var buf strings.Builder
+			printToolInstallAllowance(&buf, tt.result)
+			assert.Equal(t, tt.want, buf.String())
+			for _, ne := range tt.wantNot {
+				assert.NotContains(t, buf.String(), ne)
+			}
+		})
 	}
 }

pkg/environment/sandbox_tokens.go

@@ -152,13 +152,19 @@ func (w *SandboxTokenWriter) writeOnce(ctx context.Context) {
 		return
 	}
 
-	// Ensure the parent directory exists.
+	// Ensure the parent directory exists. Keep it 0o700: the directory's
+	// mode is the actual confidentiality boundary on the host (no other
+	// host user can traverse into it).
 	if err := os.MkdirAll(filepath.Dir(w.path), 0o700); err != nil {
 		slog.DebugContext(ctx, "Failed to create sandbox tokens directory", "path", w.path, "error", err)
 		return
 	}
 
-	if err := atomicfile.Write(w.path, bytes.NewReader(data), 0o600); err != nil {
+	// The file is bind-mounted into a sandbox whose user is not the host
+	// user that wrote it; 0o600 would make it unreadable inside the
+	// sandbox and break DOCKER_TOKEN forwarding. The 0o700 parent dir
+	// already prevents other host users from reaching this file.
+	if err := atomicfile.Write(w.path, bytes.NewReader(data), 0o644); err != nil {
 		slog.DebugContext(ctx, "Failed to write sandbox tokens file", "path", w.path, "error", err)
 		return
 	}

pkg/environment/sandbox_tokens_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 	"time"
 
@@ -108,6 +109,30 @@ func TestSandboxTokenWriter_WritesFileOnStart(t *testing.T) {
 	assert.Equal(t, "fresh-token", tokens.DockerToken)
 }
 
+// The token file is bind-mounted into a sandbox whose UID does not
+// match the host user that wrote it. The mode must therefore include
+// other-read; the 0o700 parent dir is what keeps other host users out.
+func TestSandboxTokenWriter_FileIsReadableByOther(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX file modes are not enforced on Windows")
+	}
+	t.Parallel()
+
+	dir := t.TempDir()
+	path := filepath.Join(dir, SandboxTokensFileName)
+
+	provider := NewEnvListProvider([]string{"DOCKER_TOKEN=fresh-token"})
+	w := NewSandboxTokenWriter(path, provider, time.Hour)
+	w.Start(t.Context())
+	defer w.Stop()
+
+	info, err := os.Stat(path)
+	require.NoError(t, err)
+	assert.NotZero(t, info.Mode().Perm()&0o004,
+		"sandbox-tokens.json must be readable by other so the sandbox UID can read it; got mode %#o",
+		info.Mode().Perm())
+}
+
 func TestSandboxTokenWriter_RefreshesToken(t *testing.T) {
 	t.Parallel()