diff --git a/agent-schema.json b/agent-schema.json index 274e2d27e5..55156192d7 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -2351,7 +2351,7 @@ }, "allow_private_ips": { "type": "boolean", - "description": "Opt in to dialling non-public IP addresses (valid for type 'fetch', 'api', 'openapi', 'a2a', and remote MCP toolsets). By default protected HTTP clients refuse connections \u2014 after DNS resolution, so DNS rebinding is also blocked \u2014 to loopback, RFC1918 private ranges, link-local (including the cloud metadata endpoint at 169.254.169.254), multicast and the unspecified address. Set this to true when an agent legitimately needs to call internal services. For fetch, 'allowed_domains' / 'blocked_domains' are evaluated independently and still apply." + "description": "Opt in to dialling non-public IP addresses (valid for type 'fetch', 'api', 'openapi', 'a2a', and remote MCP toolsets). By default, on the direct path (Docker Desktop unavailable, disabled, or target host is not in the Docker-owned allowlist), protected HTTP clients refuse connections \u2014 after DNS resolution, defeating DNS rebinding \u2014 to loopback, RFC1918 private ranges, link-local (including the cloud metadata endpoint at 169.254.169.254), multicast and the unspecified address. When Docker Desktop is running, only Docker-owned hostnames (docker.com, docker.io families) go through its PAC proxy; dial-time enforcement does not apply on that path. Set this to true when an agent legitimately needs to call internal services. For fetch, 'allowed_domains' / 'blocked_domains' are evaluated independently and still apply." }, "sudo_askpass": { "type": "boolean", diff --git a/docs/community/troubleshooting/index.md b/docs/community/troubleshooting/index.md index 1e73169e06..642e3a1453 100644 --- a/docs/community/troubleshooting/index.md +++ b/docs/community/troubleshooting/index.md @@ -159,7 +159,7 @@ Model names must match the provider's naming exactly. Common mistakes: ### Network connectivity -If the agent hangs or times out, check that you can reach the provider's API endpoint. Firewalls, VPNs, or proxy settings may block requests. +If the agent hangs or times out, check that you can reach the provider's API endpoint. Firewalls, VPNs, or proxy settings may block requests. Docker Agent does not evaluate PAC files or URLs directly. When Docker Desktop is running, eligible requests use its PAC adapter before environment proxy settings; `NO_PROXY` does not bypass that selection. Set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to restore `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing per request; see [Docker Desktop proxy](../../tools/fetch/index.md#docker-desktop-proxy) for scope and SSRF behavior. ## Tool Execution Failures diff --git a/docs/configuration/overview/index.md b/docs/configuration/overview/index.md index 705a698cf4..d395c8ffca 100644 --- a/docs/configuration/overview/index.md +++ b/docs/configuration/overview/index.md @@ -164,11 +164,12 @@ API keys and secrets are read from environment variables — never stored in con | `DOCKER_AGENT_AUTO_UPDATE` | Set to a truthy value (`1`, `true`, `yes`, `on`) to let standalone release binaries self-update before running. See [Optional Self-Updates](../../getting-started/installation/index.md#optional-self-updates). | | `DOCKER_AGENT_NO_TOKEN_EXCHANGE` | Set to `1` to stop Docker Agent from exchanging the access token stored by `docker login` for a Docker token. See [Docker authentication](../../guides/secrets/index.md#docker-authentication). | | `DOCKER_AGENT_HUB_LOGIN_URL` | Point the token exchange at a Docker staging environment. Ignored unless it is an HTTPS `docker.com` URL. | +| `DOCKER_AGENT_DISABLE_DESKTOP_PROXY` | Set to a truthy value (`1`, `true`, `yes`, `on`) to bypass Docker Desktop's PAC adapter per request and restore standard `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY`/`NO_PROXY` routing. | > [!NOTE] > **Legacy `CAGENT_*` aliases** > -> The same variables are also accepted with the legacy `CAGENT_` prefix (e.g. `CAGENT_DEFAULT_MODEL`, `CAGENT_MODELS_GATEWAY`, `CAGENT_HIDE_TELEMETRY_BANNER`) for backward compatibility. Prefer the `DOCKER_AGENT_*` form in new setups. +> The same variables are also accepted with the legacy `CAGENT_` prefix (e.g. `CAGENT_DEFAULT_MODEL`, `CAGENT_MODELS_GATEWAY`, `CAGENT_HIDE_TELEMETRY_BANNER`) for backward compatibility. `DOCKER_AGENT_DISABLE_DESKTOP_PROXY` is the exception: it has no legacy `CAGENT_*` alias. Prefer the `DOCKER_AGENT_*` form in new setups. > [!IMPORTANT] > Model references are case-sensitive: `openai/gpt-5` is not the same as `openai/GPT-5`. diff --git a/docs/features/api-server/index.md b/docs/features/api-server/index.md index 2c1bc99970..294826c24b 100644 --- a/docs/features/api-server/index.md +++ b/docs/features/api-server/index.md @@ -51,6 +51,10 @@ Each agent entry in the `GET /api/agents` response contains: | `multi` | boolean | `true` when the config defines more than one agent. | | `commands` | array of string | Sorted list of named command keys defined on the root agent. Omitted when no commands exist. | +### Remote agent sources + +For an agent loaded from a remote HTTP(S) configuration source, endpoints that need to load that source return `502 Bad Gateway` when fetching it fails. A missing configured agent returns `404 Not Found`; invalid source URLs or configuration return `500 Internal Server Error`. + ### Sessions | Method | Path | Description | diff --git a/docs/features/remote-mcp/index.md b/docs/features/remote-mcp/index.md index 564233bd57..210e0fdda8 100644 --- a/docs/features/remote-mcp/index.md +++ b/docs/features/remote-mcp/index.md @@ -62,6 +62,8 @@ For full configuration details, see the [Tool Config](../../configuration/tools/ Set `allow_private_ips: true` on a remote MCP toolset only when the MCP server or its OAuth registration/token endpoints intentionally resolve to private, loopback, or link-local addresses. The default blocks those OAuth helper requests to reduce SSRF risk. +When Docker Desktop is running, eligible OAuth discovery, registration, token exchange, refresh, and helper requests use its PAC adapter before environment proxy settings. It does not apply to remote MCP Streamable HTTP/SSE transport. Set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to restore standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing; `NO_PROXY` does not bypass Desktop PAC selection. Docker Agent does not evaluate PAC files or URLs directly—see [Docker Desktop proxy](../../tools/fetch/index.md#docker-desktop-proxy). + > [!NOTE] > **Headers forwarded during OAuth discovery** > diff --git a/docs/tools/a2a/index.md b/docs/tools/a2a/index.md index f07a9fe624..9cdc367d93 100644 --- a/docs/tools/a2a/index.md +++ b/docs/tools/a2a/index.md @@ -37,6 +37,8 @@ The `Authorization` header shown above authenticates to endpoints served with `d | `name` | string | ✗ | Tool name registered for the remote agent. Defaults to a name derived from the server's agent card. | | `headers` | map\[string\]string | ✗ | Extra HTTP headers sent with every request (useful for `Authorization`, tenant selection, tracing, \u2026). | +When Docker Desktop is running, eligible requests use its PAC adapter before environment proxy settings. Set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to restore standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing; `NO_PROXY` does not bypass Docker Desktop PAC selection. Docker Agent does not evaluate PAC files or URLs directly—see [Docker Desktop proxy](../fetch/index.md#docker-desktop-proxy). + > [!TIP] > **See also** > diff --git a/docs/tools/api/index.md b/docs/tools/api/index.md index 73e802bd5b..60b19ad9d4 100644 --- a/docs/tools/api/index.md +++ b/docs/tools/api/index.md @@ -220,7 +220,7 @@ agents: - Default 30-second timeout per request (override with the `timeout` field) - Only HTTP and HTTPS URLs are supported - No support for file uploads or multipart forms -- By default, requests to non-public IP ranges (loopback, RFC1918, link-local, the cloud-metadata endpoint, multicast, the unspecified address) are refused at dial time — even when DNS for an otherwise-public host resolves there. Set `allow_private_ips: true` to disable that check. +- On its direct path, requests to non-public IP ranges (loopback, RFC1918, link-local, the cloud-metadata endpoint, multicast, the unspecified address) are refused at dial time — even when DNS for an otherwise-public host resolves there. Set `allow_private_ips: true` to disable that check. ## Reaching internal services @@ -241,6 +241,8 @@ toolsets: > > Setting `allow_private_ips: true` re-exposes the SSRF surface for this tool. Only enable it when the configured `endpoint` is a trusted internal service — a prompt-injected agent cannot redirect the call elsewhere because the endpoint is fixed in config, but redirects from the configured host can still reach unexpected places. +When Docker Desktop is running, eligible public destinations use its PAC proxy before standard environment-proxy routing. A PAC `DIRECT` response selects Docker Desktop's direct egress. `NO_PROXY` does not bypass Desktop PAC selection; set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to bypass only the Desktop adapter per request and restore standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing. Loopback always stays direct. For guarded requests, Docker Desktop PAC routing is restricted to Docker-owned hostnames (docker.com and docker.io families); all other hosts use the direct SSRF-guarded path. Within the allowed set, local DNS preflight requires public addresses before Docker Desktop is selected; all lookup failures — including NXDOMAIN, empty results, errors, and private or mixed answers — stay on the SSRF-protected direct path. This preflight does not validate Docker Desktop-selected egress, whether PAC selects a proxy or `DIRECT`. `allow_private_ips: true` removes that direct-path address guard for trusted internal services, but Desktop PAC still takes precedence for eligible non-loopback destinations. See [Docker Desktop proxy](../fetch/index.md#docker-desktop-proxy). + > [!TIP] > **For Complex APIs** > diff --git a/docs/tools/fetch/index.md b/docs/tools/fetch/index.md index fa793a54ac..63ef8e271c 100644 --- a/docs/tools/fetch/index.md +++ b/docs/tools/fetch/index.md @@ -109,11 +109,19 @@ toolsets: > [!NOTE] > **Already blocked by default** > -> You do **not** need to add loopback, RFC1918, link-local (incl. `169.254.169.254`), multicast or the unspecified address to `blocked_domains` to be safe — the fetch tool already refuses connections to those ranges at dial time, after DNS resolution. The example above is only useful if you also want to reject those hosts _before_ any network call (and to surface a clearer error message to the agent), or if you have set `allow_private_ips: true` and want to deny a specific subset. +> You do **not** need to add loopback, RFC1918, link-local (incl. `169.254.169.254`), multicast or the unspecified address to `blocked_domains` to protect the fetch tool's SSRF-guarded direct path: it refuses those resolved addresses at dial time. When an eligible request is routed through Docker Desktop's PAC proxy, that proxy selects and enforces its own destination policy. The example above is useful if you also want to reject those hosts _before_ any network call (and to surface a clearer error message to the agent), or if you have set `allow_private_ips: true` and want to deny a specific subset. + +### Docker Desktop proxy + +When Docker Desktop is running, remote HTTP(S) agent configuration sources and built-in HTTP toolsets send eligible public destinations through its PAC proxy before normal environment-proxy routing. A PAC `DIRECT` result selects Docker Desktop's direct egress. `NO_PROXY` does not bypass Desktop PAC selection; set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to bypass only the Desktop adapter per request and restore standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing. Loopback always stays direct. + +For guarded clients, Docker Desktop PAC routing is restricted to Docker-owned hostnames (docker.com and docker.io families); all other hosts use the direct SSRF-guarded path regardless of Desktop state. Within the allowed set, local DNS preflight requires one or more public addresses before Docker Desktop is selected; all lookup failures — including NXDOMAIN, empty results, errors, and private or mixed answers — stay on the SSRF-protected direct path. This preflight does not validate Docker Desktop-selected egress, whether PAC selects a proxy or `DIRECT`. `allow_private_ips: true` removes the direct-path address guard for trusted internal services, but Desktop PAC still takes precedence for eligible non-loopback destinations. + +For Docker Desktop proxy configuration, see [Docker Desktop proxy settings](https://docs.docker.com/desktop/settings-and-maintenance/settings/#proxies). Docker's [PAC files](https://docs.docker.com/enterprise/security/hardened-desktop/air-gapped-containers/#proxy-auto-configuration-pac-files) documentation describes the `containersProxy` setting for managed container and image-pull traffic, not Docker Agent's host-proxy adapter path. ### SSRF protection and reaching localhost -By default, the fetch tool refuses connections to **non-public IP addresses** — even when DNS for an otherwise-public host resolves to one of them (so DNS rebinding is also blocked). The check happens at dial time, after DNS resolution, and rejects: +By default, the fetch tool's **direct path** refuses connections to **non-public IP addresses** — even when DNS for an otherwise-public host resolves to one of them (so DNS rebinding is also blocked). This dial-time check applies when Docker Desktop is unavailable or bypassed. Docker Agent does not evaluate PAC, so egress selected by Docker Desktop — through a proxy or with PAC `DIRECT` — is outside this local dial-time enforcement. The check rejects: - **Loopback** — `127.0.0.0/8`, `::1` (this is what blocks `http://localhost/...` and `http://127.0.0.1/...`) - **RFC1918 private ranges** — `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16` diff --git a/docs/tools/mcp/index.md b/docs/tools/mcp/index.md index b1acb0cca7..e8984c6f18 100644 --- a/docs/tools/mcp/index.md +++ b/docs/tools/mcp/index.md @@ -105,6 +105,8 @@ toolsets: For a curated list of public remote MCP endpoints (Linear, GitHub, Vercel, Notion, …) and full OAuth configuration details, see [Remote MCP Servers](../../features/remote-mcp/index.md). +When Docker Desktop is running, eligible MCP OAuth discovery, token, and helper requests use its PAC adapter before environment proxy settings, but remote MCP Streamable HTTP/SSE transport does not. Set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to restore standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing; `NO_PROXY` does not bypass Docker Desktop PAC selection. Docker Agent does not evaluate PAC files or URLs directly—see [Docker Desktop proxy](../fetch/index.md#docker-desktop-proxy). + ## MCP Prompts MCP servers can expose **prompts** — named, parameterized templates that the server provides via the `/prompts` endpoint. Docker Agent discovers these at toolset startup and registers them as **slash commands** in the TUI, so you can invoke them directly from the input box. diff --git a/docs/tools/openapi/index.md b/docs/tools/openapi/index.md index 56c0c30561..680fae65e2 100644 --- a/docs/tools/openapi/index.md +++ b/docs/tools/openapi/index.md @@ -47,7 +47,7 @@ toolsets: ### Reaching internal services -By default the OpenAPI tool refuses connections to non-public IP addresses, blocking SSRF attempts even when DNS resolves an otherwise-public host to an internal range. Opt in with `allow_private_ips` when the spec or its `servers` entries legitimately target localhost or your internal network: +By default, the OpenAPI tool's **direct path** refuses connections to non-public IP addresses, including a public hostname that resolves to an internal address. Docker Agent does not evaluate PAC, so the dial-time guard applies when Docker Desktop is unavailable or bypassed; egress selected by Docker Desktop — through a proxy or with PAC `DIRECT` — is outside local dial-time enforcement. Opt in with `allow_private_ips` when the spec or its `servers` entries legitimately target localhost or your internal network: ```yaml toolsets: @@ -56,6 +56,8 @@ toolsets: allow_private_ips: true ``` +When Docker Desktop is running, eligible public destinations use its PAC proxy before standard environment-proxy routing. A PAC `DIRECT` response selects Docker Desktop's direct egress. `NO_PROXY` does not bypass Desktop PAC selection; set `DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1` (or `true`, `yes`, or `on`) to bypass only the Desktop adapter per request and restore standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, and `NO_PROXY` routing. Loopback always stays direct. For guarded requests, Docker Desktop PAC routing is restricted to Docker-owned hostnames (docker.com and docker.io families); all other hosts use the direct SSRF-guarded path. Within the allowed set, local DNS preflight requires public addresses before Docker Desktop is selected; all lookup failures — including NXDOMAIN, empty results, errors, and private or mixed answers — stay on the SSRF-protected direct path. This preflight does not validate Docker Desktop-selected egress, whether PAC selects a proxy or `DIRECT`. `allow_private_ips: true` removes that direct-path guard for trusted internal services, but Desktop PAC still takes precedence for eligible non-loopback destinations. See [Docker Desktop proxy](../fetch/index.md#docker-desktop-proxy). + ## Properties | Property | Type | Required | Description | diff --git a/go.mod b/go.mod index c29102e041..df30358942 100644 --- a/go.mod +++ b/go.mod @@ -221,7 +221,7 @@ require ( go.opentelemetry.io/proto/otlp v1.10.0 // indirect go.yaml.in/yaml/v4 v4.0.0-rc.6 golang.org/x/crypto v0.54.0 // indirect - golang.org/x/net v0.57.0 // indirect + golang.org/x/net v0.57.0 golang.org/x/text v0.41.0 // indirect golang.org/x/time v0.15.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect diff --git a/pkg/config/latest/types.go b/pkg/config/latest/types.go index 22efa5ee46..9205e01726 100644 --- a/pkg/config/latest/types.go +++ b/pkg/config/latest/types.go @@ -1603,12 +1603,16 @@ type Toolset struct { // For the `fetch`, `api`, `openapi`, `a2a` and remote `mcp` toolsets — opt in // to dialling non-public IP addresses. // - // By default, protected HTTP clients refuse connections (after DNS - // resolution, so DNS rebinding is also blocked) to loopback (127/8, - // ::1), RFC1918 private ranges, link-local — including the cloud - // metadata endpoint at 169.254.169.254 — multicast and the unspecified - // address. Set this to true to permit those addresses, which is required - // when an agent legitimately needs to call internal services. + // By default, on the direct path (Docker Desktop unavailable, disabled, or + // target host is not in the Docker-owned allowlist), protected HTTP clients + // refuse connections — after DNS resolution, defeating DNS rebinding — to + // loopback (127/8, ::1), RFC1918 private ranges, link-local — including the + // cloud metadata endpoint at 169.254.169.254 — multicast and the unspecified + // address. When Docker Desktop is running, only Docker-owned hostnames + // (docker.com, docker.io families) go through its PAC proxy; dial-time + // enforcement does not apply on that path. Set this to true to permit + // non-public addresses, which is required when an agent legitimately needs to + // call internal services. // // For `fetch`, `allowed_domains` and `blocked_domains` are evaluated // independently of this flag: even with `allow_private_ips: true`, an diff --git a/pkg/config/sources.go b/pkg/config/sources.go index 6c6ef318c3..858d18ad66 100644 --- a/pkg/config/sources.go +++ b/pkg/config/sources.go @@ -33,6 +33,10 @@ type Source interface { type Sources map[string]Source +// ErrSourceFetchFailed reports that a remote URL source could not be fetched. +// It intentionally excludes URL validation and config parsing errors. +var ErrSourceFetchFailed = errors.New("remote source fetch failed") + // fileSource is used to load an agent configuration from a YAML file. type fileSource struct { path string @@ -336,7 +340,7 @@ func (a urlSource) Read(ctx context.Context) ([]byte, error) { } else { client = &http.Client{ Timeout: 60 * time.Second, - Transport: httpclient.NewSSRFSafeTransport(), + Transport: httpclient.NewDesktopAwareSSRFSafeTransport(), CheckRedirect: httpclient.HTTPSOnlyRedirects(10), } } @@ -349,7 +353,7 @@ func (a urlSource) Read(ctx context.Context) ([]byte, error) { slog.DebugContext(ctx, "Network error fetching URL, using cached version", "url", a.url, "error", err) return cachedData, nil } - return nil, fmt.Errorf("fetching %s: %w", a.url, err) + return nil, fmt.Errorf("%w: fetching %s: %w", ErrSourceFetchFailed, a.url, err) } defer resp.Body.Close() @@ -368,12 +372,12 @@ func (a urlSource) Read(ctx context.Context) ([]byte, error) { slog.DebugContext(ctx, "HTTP error fetching URL, using cached version", "url", a.url, "status", resp.Status) return cachedData, nil } - return nil, fmt.Errorf("fetching %s: %s", a.url, resp.Status) + return nil, fmt.Errorf("%w: fetching %s: %s", ErrSourceFetchFailed, a.url, resp.Status) } data, err := io.ReadAll(resp.Body) if err != nil { - return nil, fmt.Errorf("reading response body: %w", err) + return nil, fmt.Errorf("%w: reading response body: %w", ErrSourceFetchFailed, err) } // Cache the response @@ -477,15 +481,25 @@ func isLocalhostHTTP(rawURL string) bool { if err != nil { return false } - return u.Scheme == "http" && u.Hostname() == "localhost" + return u.Scheme == "http" && isLocalhostHTTPHost(u.Hostname()) +} + +func isLocalhostHTTPHost(host string) bool { + // Deliberately exact: this predicate decides whether an agent source may + // be fetched over plaintext HTTP with no dial-time SSRF guard (see + // urlSource.Read). *.localhost is an RFC 6761 convention, not guaranteed + // by musl, Docker's embedded DNS (127.0.0.11), or corporate resolvers. + // Use httpclient.isLoopbackHost for transport-level checks. + return strings.EqualFold(strings.TrimSuffix(host, "."), "localhost") } // validateAgentURL enforces that an agent URL uses HTTPS, with an exception // for http://localhost which is allowed for local development. SSRF protection -// (rejecting connections to loopback / private / link-local addresses) is -// done at dial time by [httpclient.NewSSRFSafeTransport] so that DNS -// rebinding cannot be used to bypass it. The SSRF transport is intentionally -// skipped for http://localhost since loopback is the whole point. +// is applied by [httpclient.NewDesktopAwareSSRFSafeTransport]: on the direct +// path (non-Docker host or Desktop unavailable) it refuses non-public IPs at +// dial time after DNS resolution; Docker-owned hosts may go through Desktop's +// PAC proxy where dial-time enforcement does not apply. The SSRF transport is +// intentionally skipped for http://localhost since loopback is the whole point. func validateAgentURL(rawURL string) error { u, err := url.Parse(rawURL) if err != nil { diff --git a/pkg/config/sources_test.go b/pkg/config/sources_test.go index 9531979264..fb510c3529 100644 --- a/pkg/config/sources_test.go +++ b/pkg/config/sources_test.go @@ -301,6 +301,7 @@ func TestURLSource_Read_HTTPError(t *testing.T) { _, err := newURLSourceForTest(server.URL, nil).Read(t.Context()) require.Error(t, err) + require.ErrorIs(t, err, ErrSourceFetchFailed) }) } } @@ -310,6 +311,7 @@ func TestURLSource_Read_ConnectionError(t *testing.T) { _, err := newURLSourceForTest("http://invalid.invalid/config.yaml", nil).Read(t.Context()) require.Error(t, err) + require.ErrorIs(t, err, ErrSourceFetchFailed) } func TestURLSource_Read_CachesContent(t *testing.T) { @@ -1095,3 +1097,29 @@ func TestResolveSources_URLReference_WithEnvProvider(t *testing.T) { require.True(t, ok) assert.NotNil(t, urlSrc.envProvider) } + +func TestIsLocalhostHTTPHost(t *testing.T) { + t.Parallel() + + tests := []struct { + host string + expected bool + }{ + {"localhost", true}, + {"LOCALHOST", true}, + {"localhost.", true}, // trailing dot + {"LOCALHOST.", true}, // trailing dot, case insensitive + {"evil.localhost", false}, // *.localhost subdomains must be rejected + {"sub.localhost", false}, + {"notlocalhost", false}, + {"localhost.evil.com", false}, + {"", false}, + } + + for _, tt := range tests { + t.Run(tt.host, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expected, isLocalhostHTTPHost(tt.host)) + }) + } +} diff --git a/pkg/desktop/transport/transport.go b/pkg/desktop/transport/transport.go index 400c2c6e9f..7476f2cf6e 100644 --- a/pkg/desktop/transport/transport.go +++ b/pkg/desktop/transport/transport.go @@ -11,80 +11,225 @@ import ( "net/http" "net/url" "strings" + "sync" "sync/atomic" "time" "github.com/docker/docker-agent/pkg/desktop" socket "github.com/docker/docker-agent/pkg/desktop/socket" - "github.com/docker/docker-agent/pkg/memoize" ) -var memoizer = memoize.New[bool](1 * time.Minute) +var ( + desktopRunning = func(ctx context.Context) (bool, error) { + return desktop.IsDockerDesktopRunning(context.WithoutCancel(ctx)), nil + } + desktopRunningOverrideMu sync.RWMutex + desktopRunningOverride func(context.Context) (bool, error) + desktopDetection desktopDetectionCache +) + +type desktopDetectionCache struct { + mu sync.Mutex + value bool + expires time.Time + hasValue bool + refreshing bool + ready *desktopDetectionWaiter + err error + // generation discards a refresh that predates a reset. A discarded refresh + // changes neither cache state nor err, and closes only its captured waiter. + generation uint64 +} + +type desktopDetectionWaiter struct { + done chan struct{} + once sync.Once +} + +func newDesktopDetectionWaiter() *desktopDetectionWaiter { + return &desktopDetectionWaiter{done: make(chan struct{})} +} + +func (w *desktopDetectionWaiter) close() { + if w != nil { + w.once.Do(func() { close(w.done) }) + } +} // New returns an HTTP transport that uses the Docker Desktop proxy // if available, and falls back to direct connections while re-probing the // proxy after a cooldown so long-lived processes recover on their own. func New(ctx context.Context) http.RoundTripper { - t, ok := http.DefaultTransport.(*http.Transport) - if !ok { - return http.DefaultTransport + return NewWithDirectTransport(ctx, nil) +} + +// NewWithDirectTransport is like New but uses direct as its direct fallback. +// A nil direct uses http.DefaultTransport. +func NewWithDirectTransport(ctx context.Context, direct http.RoundTripper) http.RoundTripper { + transport := directTransport(direct) + if running, err := DesktopRunning(ctx); err == nil && running { + return NewDesktopTransport(transport) + } + return transport +} + +// DesktopRunning reports Docker Desktop availability. It returns the most +// recent value while an expired value is refreshed in the background. +func DesktopRunning(ctx context.Context) (bool, error) { + desktopRunningOverrideMu.RLock() + override := desktopRunningOverride + desktopRunningOverrideMu.RUnlock() + if override != nil { + return override(ctx) } - transport := t.Clone() + return desktopDetection.running(ctx) +} - desktopRunning, err := memoizer.Memoize("desktopRunning", func() (bool, error) { - // Memoized once per process: detach the first caller's cancellation - // (so a cancelled caller can't poison the cached result) while keeping - // its trace context. - return desktop.IsDockerDesktopRunning(context.WithoutCancel(ctx)), nil - }) +func (c *desktopDetectionCache) running(ctx context.Context) (bool, error) { + c.mu.Lock() + if c.hasValue { + value := c.value + if time.Now().Before(c.expires) || c.refreshing { + c.mu.Unlock() + return value, nil + } + c.refreshing = true + generation := c.generation + go c.refresh(context.WithoutCancel(ctx), generation, nil) + c.mu.Unlock() + return value, nil + } + if c.refreshing { + ready := c.ready + c.mu.Unlock() + select { + case <-ready.done: + return c.running(ctx) + case <-ctx.Done(): + return false, ctx.Err() + } + } + c.refreshing = true + c.ready = newDesktopDetectionWaiter() + ready := c.ready + generation := c.generation + go c.refresh(context.WithoutCancel(ctx), generation, ready) + c.mu.Unlock() + + select { + case <-ready.done: + case <-ctx.Done(): + return false, ctx.Err() + } + c.mu.Lock() + err := c.err + c.mu.Unlock() if err != nil { - return transport - } - if desktopRunning { - // Create a proxy transport - proxyTransport := t.Clone() - proxyTransport.Proxy = http.ProxyURL(&url.URL{ - Scheme: "http", - }) - // Override the dialer to connect to the Unix socket for the proxy - proxyTransport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { - return socket.DialUnix(ctx, desktop.Paths().ProxySocket) + return false, err + } + return c.running(ctx) +} + +func (c *desktopDetectionCache) refresh(ctx context.Context, generation uint64, ready *desktopDetectionWaiter) { + value, err := desktopRunning(ctx) + + c.mu.Lock() + defer c.mu.Unlock() + if generation != c.generation { + if ready != nil { + ready.close() } + return + } + c.err = err + if err == nil { + c.value = value + c.hasValue = true + c.expires = time.Now().Add(time.Minute) + } + c.refreshing = false + if ready != nil { + ready.close() + c.ready = nil + } +} - // Return a fallback transport that tries the proxy first, then falls back to direct - return newFallbackTransport(proxyTransport, transport) +func resetDesktopDetectionForTest() { + desktopDetection.mu.Lock() + defer desktopDetection.mu.Unlock() + ready := desktopDetection.ready + desktopDetection.value = false + desktopDetection.expires = time.Time{} + desktopDetection.hasValue = false + desktopDetection.refreshing = false + desktopDetection.ready = nil + desktopDetection.err = nil + desktopDetection.generation++ + if ready != nil { + ready.close() } +} - return transport +// NewDesktopTransport returns a Docker Desktop proxy transport with direct fallback. +// A nil direct uses http.DefaultTransport. +func NewDesktopTransport(direct http.RoundTripper) http.RoundTripper { + baseTransport := directTransport(direct) + transport, ok := baseTransport.(*http.Transport) + if !ok { + return baseTransport + } + proxyTransport := transport.Clone() + proxyTransport.Proxy = http.ProxyURL(&url.URL{ + Scheme: "http", + }) + proxyTransport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + return socket.DialUnix(ctx, desktop.Paths().ProxySocket) + } + return newFallbackTransport(proxyTransport, transport) +} + +func directTransport(direct http.RoundTripper) http.RoundTripper { + if direct != nil { + return direct + } + if transport, ok := http.DefaultTransport.(*http.Transport); ok { + return transport.Clone() + } + return http.DefaultTransport +} + +// SetDesktopRunningForTest overrides Docker Desktop detection and returns a +// function that restores the previous detector. +func SetDesktopRunningForTest(detect func(context.Context) (bool, error)) func() { + desktopRunningOverrideMu.Lock() + previous := desktopRunningOverride + desktopRunningOverride = detect + desktopRunningOverrideMu.Unlock() + return func() { + desktopRunningOverrideMu.Lock() + defer desktopRunningOverrideMu.Unlock() + desktopRunningOverride = previous + } } // Bounded backoff: one probe per cooldown, not per request. const proxyRetryCooldown = 30 * time.Second -// fallbackTransport tries the proxy first, direct second. A socket error -// disables the proxy for proxyRetryCooldown, so a stale error can't latch -// the transport into direct mode for the rest of the process's lifetime. type fallbackTransport struct { proxy *http.Transport direct *http.Transport - // Zero = enabled. Non-zero = disabled until this UnixNano deadline. disabledUntilUnixNano atomic.Int64 } -// newFallbackTransport creates a transport that tries the proxy first, then falls back to direct. func newFallbackTransport(proxy, direct *http.Transport) *fallbackTransport { - return &fallbackTransport{ - proxy: proxy, - direct: direct, - } + return &fallbackTransport{proxy: proxy, direct: direct} } -// DisableCompression disables automatic gzip compression on both transports. -// This is needed for SSE streaming compatibility. func (f *fallbackTransport) DisableCompression() { f.proxy.DisableCompression = true - f.direct.DisableCompression = true + // f.direct is owned by desktopAwareTransport and set there before + // publication — mutating it here would race with in-flight requests. } func (f *fallbackTransport) proxyEnabled() bool { @@ -95,7 +240,6 @@ func (f *fallbackTransport) proxyEnabled() bool { if time.Now().UnixNano() < until { return false } - // CAS (not Store) so a concurrent disableProxy() can't be stomped. f.disabledUntilUnixNano.CompareAndSwap(until, 0) return true } @@ -118,13 +262,8 @@ func (f *fallbackTransport) RoundTrip(req *http.Request) (*http.Response, error) } slog.Warn("Docker Desktop proxy unavailable, falling back to direct connection", - "error", err.Error(), - "url", req.URL.String(), - "retry_after", proxyRetryCooldown) + "error", sanitizeForLog(err.Error()), "url", sanitizeURLForLog(req.URL), "retry_after", proxyRetryCooldown) f.disableProxy() - - // Retry direct only when the body is safe to replay; otherwise the - // proxy may have already consumed it. if req.Body != nil && req.GetBody == nil { return nil, err } @@ -139,32 +278,33 @@ func (f *fallbackTransport) RoundTrip(req *http.Request) (*http.Response, error) return f.direct.RoundTrip(retryReq) } -// isProxySocketError checks if the error indicates the proxy socket is unavailable. -// This includes: -// - "no such file or directory" - socket file was deleted -// - "connection refused" - socket exists but nothing is listening -// - "dial unix" errors - general Unix socket connection failures +func sanitizeURLForLog(u *url.URL) string { + if u == nil || u.Host == "" { + return "" + } + // Scheme+host only: paths carry credentials for common webhook targets + // (Slack /services/T/B/SECRET, Telegram /bot/...). + return sanitizeForLog(u.Scheme + "://" + u.Host) +} + +func sanitizeForLog(value string) string { + return strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f { + return -1 + } + return r + }, value) +} + func isProxySocketError(err error) bool { if err == nil { return false } - errStr := strings.ToLower(err.Error()) - - // Check for common proxy socket failure patterns - proxyErrorPatterns := []string{ - "no such file or directory", // Socket file deleted - "connect: connection refused", // Socket exists but no listener - "proxyconnect tcp", // Proxy connection failure - "dial unix", // Unix socket dial failure - "unix socket", // Generic Unix socket error - } - - for _, pattern := range proxyErrorPatterns { + for _, pattern := range []string{"no such file or directory", "connect: connection refused", "proxyconnect tcp", "dial unix", "unix socket"} { if strings.Contains(errStr, pattern) { return true } } - return false } diff --git a/pkg/desktop/transport/transport_test.go b/pkg/desktop/transport/transport_test.go index b7f06211fd..964cb136d7 100644 --- a/pkg/desktop/transport/transport_test.go +++ b/pkg/desktop/transport/transport_test.go @@ -1,57 +1,260 @@ package transport import ( + "bytes" + "context" "errors" + "log/slog" + "net" "net/http" "net/http/httptest" + "net/url" "sync/atomic" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/docker/docker-agent/pkg/desktop" ) +func TestDesktopRunningOverrideBypassesMemoizedDetection(t *testing.T) { + desktopRunningOverrideMu.Lock() + previousDesktopRunning := desktopRunning + desktopRunning = func(context.Context) (bool, error) { return false, nil } + desktopRunningOverride = nil + desktopRunningOverrideMu.Unlock() + resetDesktopDetectionForTest() + t.Cleanup(func() { + desktopRunningOverrideMu.Lock() + desktopRunning = previousDesktopRunning + desktopRunningOverride = nil + desktopRunningOverrideMu.Unlock() + resetDesktopDetectionForTest() + }) + + running, err := DesktopRunning(t.Context()) + require.NoError(t, err) + assert.False(t, running) + + calls := 0 + t.Cleanup(SetDesktopRunningForTest(func(context.Context) (bool, error) { + calls++ + return true, nil + })) + + for range 2 { + running, err = DesktopRunning(t.Context()) + require.NoError(t, err) + assert.True(t, running) + } + assert.Equal(t, 2, calls) +} + +func TestDesktopRunningRefreshesStaleValueWithoutBlocking(t *testing.T) { + refresh := make(chan struct{}) + startedRefresh := make(chan struct{}) + refreshDone := make(chan struct{}) + desktopRunningOverrideMu.Lock() + previous := desktopRunning + desktopRunning = func(context.Context) (bool, error) { + close(startedRefresh) + <-refresh + close(refreshDone) + return true, nil + } + desktopRunningOverrideMu.Unlock() + resetDesktopDetectionForTest() + desktopDetection.mu.Lock() + desktopDetection.value = false + desktopDetection.hasValue = true + desktopDetection.expires = time.Now().Add(-time.Second) + desktopDetection.mu.Unlock() + t.Cleanup(func() { + close(refresh) + <-refreshDone + desktopRunningOverrideMu.Lock() + desktopRunning = previous + desktopRunningOverrideMu.Unlock() + resetDesktopDetectionForTest() + }) + + started := make(chan struct{}) + go func() { + _, _ = DesktopRunning(t.Context()) + close(started) + }() + select { + case <-started: + case <-time.After(time.Second): + t.Fatal("stale desktop detection blocked") + } + + select { + case <-startedRefresh: + case <-time.After(time.Second): + t.Fatal("stale desktop detection did not refresh") + } + + running, err := DesktopRunning(t.Context()) + require.NoError(t, err) + assert.False(t, running) +} + +func TestResetDesktopDetectionForTestDiscardsInFlightDetection(t *testing.T) { + firstStarted := make(chan struct{}) + firstRelease := make(chan struct{}) + firstDone := make(chan struct{}) + var calls atomic.Int32 + + desktopRunningOverrideMu.Lock() + previous := desktopRunning + desktopRunning = func(context.Context) (bool, error) { + if calls.Add(1) == 1 { + close(firstStarted) + <-firstRelease + close(firstDone) + return true, errors.New("stale detection error") + } + return false, nil + } + desktopRunningOverrideMu.Unlock() + resetDesktopDetectionForTest() + t.Cleanup(func() { + if firstRelease != nil { + close(firstRelease) + } + if firstDone != nil { + <-firstDone + } + desktopRunningOverrideMu.Lock() + desktopRunning = previous + desktopRunningOverrideMu.Unlock() + resetDesktopDetectionForTest() + }) + + result := make(chan struct { + running bool + err error + }, 1) + go func() { + running, err := DesktopRunning(t.Context()) + result <- struct { + running bool + err error + }{running, err} + }() + + select { + case <-firstStarted: + case <-time.After(time.Second): + t.Fatal("desktop detection did not start") + } + + resetDesktopDetectionForTest() + + select { + case result := <-result: + require.NoError(t, result.err) + assert.False(t, result.running) + case <-time.After(time.Second): + t.Fatal("desktop detection remained blocked after reset") + } + + close(firstRelease) + <-firstDone + firstRelease = nil + firstDone = nil + + running, err := DesktopRunning(t.Context()) + require.NoError(t, err) + assert.False(t, running) + assert.Equal(t, int32(2), calls.Load()) +} + func TestNew_UsesDesktopProxyWhenAvailable(t *testing.T) { - t.Parallel() + t.Cleanup(SetDesktopRunningForTest(func(context.Context) (bool, error) { + return true, nil + })) + + rt := New(t.Context()) + require.IsType(t, &fallbackTransport{}, rt) +} + +func TestNew_PreservesWrappedDefaultTransport(t *testing.T) { + // Intentionally not parallel: mutates the http.DefaultTransport global. + previous := http.DefaultTransport + wrapped := &countingRoundTripper{} + http.DefaultTransport = wrapped + t.Cleanup(func() { http.DefaultTransport = previous }) - ctx := t.Context() + for _, test := range []struct { + name string + running bool + }{ + {name: "without Desktop", running: false}, + {name: "with Desktop", running: true}, + } { + t.Run(test.name, func(t *testing.T) { + t.Cleanup(SetDesktopRunningForTest(func(context.Context) (bool, error) { + return test.running, nil + })) + + rt := New(t.Context()) + assert.Same(t, wrapped, rt) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://example.com/", http.NoBody) + require.NoError(t, err) + resp, err := (&http.Client{Transport: rt}).Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + }) + } + + assert.Equal(t, int32(2), wrapped.calls.Load()) +} - // Create a transport - rt := New(ctx) - require.NotNil(t, rt) +func TestNewWithDirectTransportPreservesHTTPTransportAcrossDesktopAvailability(t *testing.T) { + for _, test := range []struct { + name string + running bool + }{ + {name: "without Desktop", running: false}, + {name: "with Desktop", running: true}, + } { + t.Run(test.name, func(t *testing.T) { + direct := &http.Transport{} + t.Cleanup(SetDesktopRunningForTest(func(context.Context) (bool, error) { + return test.running, nil + })) + + rt := NewWithDirectTransport(t.Context(), direct) + if !test.running { + assert.Same(t, direct, rt) + return + } - // If Docker Desktop is running, verify fallback transport is used - if desktop.IsDockerDesktopRunning(ctx) { - _, ok := rt.(*fallbackTransport) - assert.True(t, ok, "transport should be *fallbackTransport when Docker Desktop is running") - } else { - // Otherwise, it should be a plain *http.Transport - _, ok := rt.(*http.Transport) - assert.True(t, ok, "transport should be *http.Transport when Docker Desktop is not running") + fallback, ok := rt.(*fallbackTransport) + require.True(t, ok) + assert.Same(t, direct, fallback.direct) + }) } } func TestNew_WorksWithoutDesktopProxy(t *testing.T) { - t.Parallel() + t.Cleanup(SetDesktopRunningForTest(func(context.Context) (bool, error) { + return false, nil + })) - // Create a test server to simulate a registry - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) defer server.Close() - ctx := t.Context() + rt := New(t.Context()) + require.IsType(t, &http.Transport{}, rt) - // Create a transport (should work whether Desktop is running or not) - rt := New(ctx) - require.NotNil(t, rt) - - // Make a simple HTTP request to verify the transport works client := &http.Client{Transport: rt} - req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.URL, http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, http.NoBody) require.NoError(t, err) resp, err := client.Do(req) require.NoError(t, err) @@ -60,6 +263,55 @@ func TestNew_WorksWithoutDesktopProxy(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode) } +func TestSanitizeURLForLog(t *testing.T) { + t.Parallel() + + u, err := url.Parse("https://user:password@example.com/path?token=secret#fragment") + require.NoError(t, err) + + // Scheme+host only: userinfo, path, query, fragment all stripped + // to avoid leaking webhook credentials embedded in the path. + assert.Equal(t, "https://example.com", sanitizeURLForLog(u)) + assert.Empty(t, sanitizeURLForLog(&url.URL{})) +} + +func TestFallbackTransportSanitizesLogFields(t *testing.T) { + var logs bytes.Buffer + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + proxy := &http.Transport{ + Proxy: func(*http.Request) (*url.URL, error) { + return &url.URL{Scheme: "http", Host: "proxy.invalid"}, nil + }, + DialContext: func(context.Context, string, string) (net.Conn, error) { + return nil, errors.New("dial unix\nproxy socket: no such file or directory") + }, + } + fallback := newFallbackTransport(proxy, &http.Transport{}) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL+"/path?token=secret#fragment", http.NoBody) + require.NoError(t, err) + req.URL.User = url.UserPassword("user", "password") + req.Body = nil + + resp, err := fallback.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + log := logs.String() + assert.Contains(t, log, "url=http://") + assert.NotContains(t, log, "user:password") + assert.NotContains(t, log, "token=secret") + assert.NotContains(t, log, "fragment") + assert.NotContains(t, log, "\\n") +} + func TestIsProxySocketError(t *testing.T) { t.Parallel() @@ -130,12 +382,16 @@ func TestFallbackTransport_DisableCompression(t *testing.T) { assert.False(t, proxy.DisableCompression) assert.False(t, direct.DisableCompression) - // Disable compression + // Disable compression. Only the proxy transport should be mutated here; + // direct is owned by desktopAwareTransport and already configured before + // this method is reached (see desktopAwareTransport.DisableCompression). ft.DisableCompression() - // Verify compression is now disabled on both transports + // Verify compression is now disabled on the proxy transport. assert.True(t, proxy.DisableCompression) - assert.True(t, direct.DisableCompression) + // direct must NOT be mutated here — mutating it would race with + // in-flight requests on concurrent goroutines. + assert.False(t, direct.DisableCompression) } // testError is a simple error type for testing diff --git a/pkg/httpclient/client.go b/pkg/httpclient/client.go index 6381dec57f..b0f4ae720f 100644 --- a/pkg/httpclient/client.go +++ b/pkg/httpclient/client.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" - "github.com/docker/docker-agent/pkg/desktop/transport" "github.com/docker/docker-agent/pkg/userid" "github.com/docker/docker-agent/pkg/version" ) @@ -188,10 +187,9 @@ func WithQuery(query url.Values) Opt { } } -// newTransport returns an HTTP transport with automatic gzip compression disabled and using Docker Desktop proxy if available. -func newTransport(ctx context.Context) http.RoundTripper { - // Get the base transport with Desktop proxy support - rt := transport.New(ctx) +// newTransport returns an HTTP transport with automatic gzip compression disabled and Docker Desktop PAC support. +func newTransport(_ context.Context) http.RoundTripper { + rt := newAllowPrivateIPsTransport() // Disable compression for SSE streaming compatibility // Handle both direct *http.Transport and the fallback transport wrapper diff --git a/pkg/httpclient/desktop_transport.go b/pkg/httpclient/desktop_transport.go new file mode 100644 index 0000000000..c35e94fa64 --- /dev/null +++ b/pkg/httpclient/desktop_transport.go @@ -0,0 +1,235 @@ +package httpclient + +import ( + "context" + "fmt" + "log/slog" + "net" + "net/http" + "net/url" + "os" + "strings" + "sync" + + "golang.org/x/net/http/httpproxy" + + desktoptransport "github.com/docker/docker-agent/pkg/desktop/transport" +) + +const disableDesktopProxyEnv = "DOCKER_AGENT_DISABLE_DESKTOP_PROXY" + +var invalidDesktopProxySetting struct { + sync.Mutex + + value string +} + +type desktopAwareTransport struct { + direct *http.Transport + guarded bool + resolver func(context.Context, string) ([]net.IP, error) + newDesktopTransport func(context.Context, http.RoundTripper) http.RoundTripper + + mu sync.Mutex + desktopTransport http.RoundTripper + disableCompression bool + warnedCompression bool +} + +func newDesktopAwareTransport(guarded bool) http.RoundTripper { + var direct *http.Transport + if guarded { + direct = NewSSRFSafeTransport() + } else { + direct = cloneDefaultTransport(environmentProxyFunc()) + } + return &desktopAwareTransport{ + direct: direct, + guarded: guarded, + resolver: func(ctx context.Context, host string) ([]net.IP, error) { + return net.DefaultResolver.LookupIP(ctx, "ip", host) + }, + newDesktopTransport: func(_ context.Context, direct http.RoundTripper) http.RoundTripper { + return desktoptransport.NewDesktopTransport(direct) + }, + } +} + +// NewDesktopAwareSSRFSafeTransport returns a guarded transport that routes +// Docker-owned hostnames (docker.com, docker.io families) through Docker +// Desktop's PAC proxy when Desktop is available. All other hosts always use +// the direct SSRF-guarded transport (dial-time enforcement, defeats DNS +// rebinding). Docker Desktop is optional: absent, disabled, or when the target +// host is outside the allowlist, requests fall back to direct with the same +// SSRF protection as NewSSRFSafeTransport. +func NewDesktopAwareSSRFSafeTransport() http.RoundTripper { + return newDesktopAwareTransport(true) +} + +func newAllowPrivateIPsTransport() http.RoundTripper { + return newDesktopAwareTransport(false) +} + +func cloneDefaultTransport(proxy func(*http.Request) (*url.URL, error)) *http.Transport { + if base, ok := http.DefaultTransport.(*http.Transport); ok { + transport := base.Clone() + transport.Proxy = proxy + return transport + } + return &http.Transport{Proxy: proxy} +} + +func proxyFunc(config *httpproxy.Config) func(*http.Request) (*url.URL, error) { + proxyForURL := config.ProxyFunc() + return func(req *http.Request) (*url.URL, error) { + return proxyForURL(req.URL) + } +} + +func environmentProxyFunc() func(*http.Request) (*url.URL, error) { + return proxyFunc(httpproxy.FromEnvironment()) +} + +func (t *desktopAwareTransport) RoundTrip(req *http.Request) (*http.Response, error) { + host := req.URL.Hostname() + if desktopProxyDisabled() || isLoopbackHost(host) { + return t.direct.RoundTrip(req) + } + // Guarded transports restrict Desktop PAC routing to Docker-owned hosts; + // all other hosts use the direct SSRF-guarded transport regardless of Desktop state. + if t.guarded && !isDockerHost(host) { + return t.direct.RoundTrip(req) + } + + transport := t.selectedTransportFor(req.Context()) + if transport == t.direct || (t.guarded && !t.proxySafe(req.Context(), host)) { + return t.direct.RoundTrip(req) + } + return transport.RoundTrip(req) +} + +func (t *desktopAwareTransport) selectedTransportFor(ctx context.Context) http.RoundTripper { + running, err := desktoptransport.DesktopRunning(ctx) + if err != nil || !running { + return t.direct + } + t.mu.Lock() + defer t.mu.Unlock() + if t.desktopTransport == nil { + t.desktopTransport = t.newDesktopTransport(ctx, t.direct) + if t.disableCompression { + disableCompression(t.desktopTransport) + } + } + return t.desktopTransport +} + +func (t *desktopAwareTransport) proxySafe(ctx context.Context, host string) bool { + if ip := net.ParseIP(host); ip != nil { + return IsPublicIP(ip) + } + ips, err := t.resolver(ctx, host) + if err != nil { + // Fail closed: Docker-owned hostnames resolve publicly; NXDOMAIN + // suggests a broken resolver rather than a PAC-only network. + return false + } + if len(ips) == 0 { + return false + } + for _, ip := range ips { + if !IsPublicIP(ip) { + return false + } + } + return true +} + +func desktopProxyDisabled() bool { + value := strings.ToLower(strings.TrimSpace(os.Getenv(disableDesktopProxyEnv))) + switch value { + case "", "0", "false", "no", "off": + resetInvalidDesktopProxySetting() + return false + case "1", "true", "yes", "on": + resetInvalidDesktopProxySetting() + return true + default: + warnInvalidDesktopProxySetting(value) + return false + } +} + +func resetInvalidDesktopProxySetting() { + invalidDesktopProxySetting.Lock() + defer invalidDesktopProxySetting.Unlock() + invalidDesktopProxySetting.value = "" +} + +func warnInvalidDesktopProxySetting(value string) { + invalidDesktopProxySetting.Lock() + defer invalidDesktopProxySetting.Unlock() + if invalidDesktopProxySetting.value == value { + return + } + invalidDesktopProxySetting.value = value + slog.Warn("unrecognized DOCKER_AGENT_DISABLE_DESKTOP_PROXY value; treating it as disabled", "value", value) +} + +// dockerBaseDomains lists Docker's infrastructure domains that guarded +// transports may route through Desktop's PAC proxy. Any hostname that is +// exactly one of these domains, or a subdomain of one, is eligible for +// Desktop routing; all others use the direct SSRF-guarded path. +var dockerBaseDomains = [...]string{ + "docker.com", // Hub, auth, API, desktop configuration + "docker.io", // Container registry: registry-1, auth, index, cdn +} + +// isDockerHost reports whether hostname belongs to Docker's infrastructure +// (docker.com or docker.io, including all subdomains). Only guarded transports +// use this allowlist; unguarded transports (allow_private_ips) skip it. +func isDockerHost(hostname string) bool { + h := strings.ToLower(strings.TrimSuffix(hostname, ".")) + for _, base := range dockerBaseDomains { + if h == base || strings.HasSuffix(h, "."+base) { + return true + } + } + return false +} + +func isLoopbackHost(host string) bool { + host = strings.TrimSuffix(host, ".") + if strings.EqualFold(host, "localhost") || strings.HasSuffix(strings.ToLower(host), ".localhost") { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + +func (t *desktopAwareTransport) DisableCompression() { + t.mu.Lock() + defer t.mu.Unlock() + + t.disableCompression = true + if !disableCompression(t.direct) && !t.warnedCompression { + t.warnedCompression = true + slog.Warn("cannot disable compression for custom direct transport", "transport", fmt.Sprintf("%T", t.direct)) + } + disableCompression(t.desktopTransport) +} + +func disableCompression(transport any) bool { + if transport == nil { + return true + } + if disabler, ok := transport.(interface{ DisableCompression() }); ok { + disabler.DisableCompression() + return true + } + if direct, ok := transport.(*http.Transport); ok { + direct.DisableCompression = true + return true + } + return false +} diff --git a/pkg/httpclient/desktop_transport_test.go b/pkg/httpclient/desktop_transport_test.go new file mode 100644 index 0000000000..19c0f8aba0 --- /dev/null +++ b/pkg/httpclient/desktop_transport_test.go @@ -0,0 +1,628 @@ +package httpclient + +import ( + "bytes" + "context" + "errors" + "log/slog" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/net/http/httpproxy" + + desktoptransport "github.com/docker/docker-agent/pkg/desktop/transport" +) + +// legacyDisableDesktopProxyEnv is the retired name that must remain inert. +const legacyDisableDesktopProxyEnv = "CAGENT_DISABLE_DESKTOP_PROXY" + +func TestDesktopAwareTransportProxySafe(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + host string + resolver func(context.Context, string) ([]net.IP, error) + want bool + }{ + { + name: "literal private address", + host: "127.0.0.1", + want: false, + }, + { + name: "all resolved addresses private", + host: "internal.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return []net.IP{net.ParseIP("10.0.0.1"), net.ParseIP("192.168.1.1")}, nil + }, + want: false, + }, + { + name: "mixed public and private addresses stay direct", + host: "mixed.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return []net.IP{net.ParseIP("10.0.0.1"), net.ParseIP("1.1.1.1")}, nil + }, + want: false, + }, + { + name: "all resolved addresses public", + host: "public.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return []net.IP{net.ParseIP("1.1.1.1"), net.ParseIP("2606:4700:4700::1111")}, nil + }, + want: true, + }, + { + name: "empty DNS response stays direct", + host: "empty.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return nil, nil + }, + want: false, + }, + { + name: "temporary DNS error stays direct", + host: "temporary.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return nil, &net.DNSError{IsTemporary: true} + }, + want: false, + }, + { + name: "non-DNS resolver error stays direct", + host: "failed.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return nil, errors.New("resolver failed") + }, + want: false, + }, + { + name: "NXDOMAIN fails closed (Option A: broken resolver, not PAC-only network)", + host: "proxy-only.example", + resolver: func(context.Context, string) ([]net.IP, error) { + return nil, &net.DNSError{IsNotFound: true} + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + transport := newDesktopAwareTransport(true).(*desktopAwareTransport) + if tt.resolver != nil { + transport.resolver = tt.resolver + } + assert.Equal(t, tt.want, transport.proxySafe(t.Context(), tt.host)) + }) + } +} + +func TestDesktopAwareTransportLoopbackIsNeverProxied(t *testing.T) { + t.Parallel() + + assert.True(t, isLoopbackHost("localhost")) + assert.True(t, isLoopbackHost("localhost.")) + assert.True(t, isLoopbackHost("LOCALHOST")) + assert.True(t, isLoopbackHost("service.localhost")) + assert.True(t, isLoopbackHost("127.0.0.1")) + assert.True(t, isLoopbackHost("::1")) + assert.False(t, isLoopbackHost("example.com")) +} + +func TestDesktopAwareTransportProxyFunc(t *testing.T) { + proxy := proxyFunc(&httpproxy.Config{ + HTTPSProxy: "http://proxy.example:8443", + NoProxy: "bypass.example,.internal.example", + }) + + for _, test := range []struct { + host string + wantProxy bool + }{ + {host: "public.example", wantProxy: true}, + {host: "bypass.example", wantProxy: false}, + {host: "service.internal.example", wantProxy: false}, + } { + t.Run(test.host, func(t *testing.T) { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "https://"+test.host, http.NoBody) + selected, err := proxy(req) + require.NoError(t, err) + assert.Equal(t, test.wantProxy, selected != nil) + }) + } +} + +func TestDesktopAwareTransportDesktopWinsOverNoProxyUntilKillSwitch(t *testing.T) { + // Uses hub.docker.com (a Docker host in the allowlist) so the guarded + // transport routes through Desktop regardless of NO_PROXY settings. + t.Setenv(disableDesktopProxyEnv, "") + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + + transport := newDesktopAwareTransport(true).(*desktopAwareTransport) + transport.resolver = func(context.Context, string) ([]net.IP, error) { return []net.IP{net.ParseIP("52.0.0.1")}, nil } + desktop := &countingTransport{} + var proxySelections, directDials int + transport.direct.Proxy = func(req *http.Request) (*url.URL, error) { + proxySelections++ + return proxyFunc(&httpproxy.Config{HTTPProxy: "http://proxy.example:8080", NoProxy: "hub.docker.com"})(req) + } + transport.direct.DialContext = func(context.Context, string, string) (net.Conn, error) { + directDials++ + return nil, errors.New("direct dial") + } + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { return desktop } + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://hub.docker.com", http.NoBody) + require.NoError(t, err) + + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, 1, desktop.calls) + assert.Zero(t, proxySelections) + assert.Zero(t, directDials) + + t.Setenv(disableDesktopProxyEnv, "1") + resp, err = transport.RoundTrip(req) + if resp != nil { + require.NoError(t, resp.Body.Close()) + } + require.EqualError(t, err, "direct dial") + assert.Equal(t, 1, desktop.calls) + assert.Equal(t, 1, proxySelections, "the direct path must apply NO_PROXY") + assert.Equal(t, 1, directDials, "NO_PROXY must select a direct connection") +} + +func TestDesktopAwareTransportCachesDesktopTransport(t *testing.T) { + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + var factoryCalls int + branch := &countingTransport{} + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { + factoryCalls++ + return branch + } + + for range 2 { + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + } + assert.Equal(t, 1, factoryCalls) + assert.Equal(t, 2, branch.calls) +} + +func TestDesktopAwareTransportDisabledOrGuardedUsesDirect(t *testing.T) { + for _, tc := range []struct { + name string + guarded bool + host string + disabled bool + }{ + {name: "kill switch", host: "example.com", disabled: true}, + {name: "guarded loopback", guarded: true, host: "127.0.0.1"}, + {name: "unguarded loopback", host: "127.0.0.1"}, + {name: "guarded non-Docker goes direct (allowlist)", guarded: true, host: "example.com"}, + {name: "guarded non-Docker arbitrary subdomain goes direct", guarded: true, host: "evil.docker.com.attacker.com"}, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.disabled { + t.Setenv(disableDesktopProxyEnv, "1") + } + transport := newDesktopAwareTransport(tc.guarded).(*desktopAwareTransport) + transport.resolver = func(context.Context, string) ([]net.IP, error) { return []net.IP{net.ParseIP("10.0.0.1")}, nil } + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { + t.Fatal("Desktop transport must not be used") + return nil + } + transport.direct = &http.Transport{} + transport.direct.RegisterProtocol("https", roundTripperFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil + })) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://"+tc.host, http.NoBody) + require.NoError(t, err) + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + }) + } +} + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +func TestDesktopAwareTransportConsultsDesktopDetectionPerRequest(t *testing.T) { + desktopRunning := false + var detections int + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { + detections++ + return desktopRunning, nil + })) + + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + direct := &countingTransport{} + transport.direct = &http.Transport{} + transport.direct.RegisterProtocol("https", direct) + var factoryCalls int + branch := &countingTransport{} + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { + factoryCalls++ + return branch + } + + for range 2 { + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + } + assert.Equal(t, 2, detections) + assert.Equal(t, 2, direct.calls) + assert.Zero(t, factoryCalls) + + desktopRunning = true + for range 2 { + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + } + assert.Equal(t, 4, detections) + assert.Equal(t, 1, factoryCalls) + assert.Equal(t, 2, branch.calls) + + desktopRunning = false + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, 5, detections) + assert.Equal(t, 1, factoryCalls) + assert.Equal(t, 3, direct.calls) +} + +func TestDesktopAwareTransportDisableCompressionBeforeDesktopTransport(t *testing.T) { + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + transport.direct = &http.Transport{} + transport.direct.RegisterProtocol("https", roundTripperFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil + })) + transport.DisableCompression() + proxy := &compressionTransport{} + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { return proxy } + + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + assert.True(t, proxy.disabled) +} + +func TestDesktopAwareTransportDisableCompressionConcurrentDesktopCreation(t *testing.T) { + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + proxy := &compressionTransport{} + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { return proxy } + + done := make(chan struct{}) + go func() { + defer close(done) + for range 100 { + transport.DisableCompression() + } + }() + for range 100 { + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + } + <-done + assert.True(t, proxy.disabled) +} + +func TestDesktopAwareTransportDisableCompressionAcrossDirectAndDesktopFlaps(t *testing.T) { + desktopRunning := false + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { + return desktopRunning, nil + })) + + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + direct := &countingTransport{} + transport.direct = &http.Transport{} + transport.direct.RegisterProtocol("https", direct) + desktop := &compressionTransport{} + var factoryCalls int + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { + factoryCalls++ + return desktop + } + + transport.DisableCompression() + assert.True(t, transport.direct.DisableCompression) + + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, 1, direct.calls) + assert.Zero(t, desktop.calls) + + desktopRunning = true + resp = roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, 1, factoryCalls) + assert.Equal(t, 1, desktop.calls) + assert.True(t, desktop.disabled) + + desktopRunning = false + resp = roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, 2, direct.calls) + + desktopRunning = true + resp = roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, 1, factoryCalls) + assert.Equal(t, 2, desktop.calls) + assert.True(t, desktop.disabled) +} + +func TestDesktopAwareTransportRetainsDesktopBranchCooldown(t *testing.T) { + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + branch := &cooldownTransport{} + var factoryCalls int + transport.newDesktopTransport = func(context.Context, http.RoundTripper) http.RoundTripper { + factoryCalls++ + return branch + } + + for range 2 { + resp := roundTrip(t, transport) + require.NoError(t, resp.Body.Close()) + } + assert.Equal(t, 1, factoryCalls) + assert.Equal(t, 1, branch.proxyCalls) + assert.Equal(t, 2, branch.directCalls) +} + +func roundTrip(t *testing.T, transport http.RoundTripper) *http.Response { + t.Helper() + req := requestForHost(t, "example.com") + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + return resp +} + +func requestForHost(t *testing.T, host string) *http.Request { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://"+host, http.NoBody) + require.NoError(t, err) + return req +} + +type countingTransport struct { + calls int +} + +func (t *countingTransport) RoundTrip(*http.Request) (*http.Response, error) { + t.calls++ + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil +} + +type cooldownTransport struct { + proxyCalls int + directCalls int + disabled bool +} + +func (t *cooldownTransport) RoundTrip(*http.Request) (*http.Response, error) { + if !t.disabled { + t.proxyCalls++ + t.disabled = true + } + t.directCalls++ + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil +} + +type compressionTransport struct { + disabled bool + calls int +} + +func (t *compressionTransport) DisableCompression() { + t.disabled = true +} + +func (t *compressionTransport) RoundTrip(*http.Request) (*http.Response, error) { + t.calls++ + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil +} + +func TestIsDockerHost(t *testing.T) { + t.Parallel() + tests := []struct { + host string + expected bool + }{ + {"docker.com", true}, + {"docker.io", true}, + {"DOCKER.COM", true}, + {"hub.docker.com", true}, + {"registry-1.docker.io", true}, + {"auth.docker.io", true}, + {"index.docker.io", true}, + {"cdn.registry.docker.io", true}, + {"desktop.docker.com", true}, + {"api.docker.com", true}, + {"docker.com.", true}, + {"hub.docker.com.", true}, + {"example.com", false}, + {"jenkins.internal", false}, + {"evil.localhost", false}, + {"notdocker.com", false}, + {"docker.com.attacker.com", false}, + {"evil.docker.com.attacker.com", false}, + {"fakedocker.com", false}, + {"xdocker.com", false}, + {"docker.org", false}, + {"", false}, + } + for _, tt := range tests { + t.Run(tt.host, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expected, isDockerHost(tt.host)) + }) + } +} + +func TestDesktopAwareTransportUnguardedUsesDesktopForNonDockerHost(t *testing.T) { + desktopHit := false + transport := newDesktopAwareTransport(false).(*desktopAwareTransport) + transport.resolver = func(context.Context, string) ([]net.IP, error) { + return []net.IP{net.ParseIP("1.1.1.1")}, nil + } + transport.newDesktopTransport = func(_ context.Context, _ http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(_ *http.Request) (*http.Response, error) { + desktopHit = true + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil + }) + } + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.True(t, desktopHit, "unguarded transport should route non-Docker host through Desktop") +} + +func TestDesktopAwareTransportGuardedDockerHostUsesDesktop(t *testing.T) { + desktopHit := false + transport := newDesktopAwareTransport(true).(*desktopAwareTransport) + transport.resolver = func(context.Context, string) ([]net.IP, error) { + return []net.IP{net.ParseIP("52.0.0.1")}, nil + } + transport.newDesktopTransport = func(_ context.Context, _ http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(_ *http.Request) (*http.Response, error) { + desktopHit = true + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil + }) + } + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://hub.docker.com/v2/", http.NoBody) + require.NoError(t, err) + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.True(t, desktopHit, "guarded transport should route Docker host through Desktop when it resolves to a public IP") +} + +func TestDesktopAwareTransportGuardedDockerHostNXDOMAINStaysDirect(t *testing.T) { + desktopRoundTrips := 0 + directRoundTrips := 0 + transport := newDesktopAwareTransport(true).(*desktopAwareTransport) + transport.resolver = func(context.Context, string) ([]net.IP, error) { + return nil, &net.DNSError{IsNotFound: true} + } + transport.newDesktopTransport = func(_ context.Context, _ http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(_ *http.Request) (*http.Response, error) { + desktopRoundTrips++ + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil + }) + } + transport.direct = &http.Transport{} + transport.direct.RegisterProtocol("https", roundTripperFunc(func(_ *http.Request) (*http.Response, error) { + directRoundTrips++ + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil + })) + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://hub.docker.com/v2/", http.NoBody) + require.NoError(t, err) + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Zero(t, desktopRoundTrips, "Desktop RoundTrip must not be called when proxySafe returns false (NXDOMAIN)") + assert.Equal(t, 1, directRoundTrips, "direct transport must handle the request when proxySafe returns false") +} + +func TestDesktopAwareTransportGuardedDockerHostPrivateIPStaysDirect(t *testing.T) { + desktopRoundTrips := 0 + directRoundTrips := 0 + transport := newDesktopAwareTransport(true).(*desktopAwareTransport) + transport.resolver = func(context.Context, string) ([]net.IP, error) { + return []net.IP{net.ParseIP("10.0.0.1")}, nil + } + transport.newDesktopTransport = func(_ context.Context, _ http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(_ *http.Request) (*http.Response, error) { + desktopRoundTrips++ + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil + }) + } + transport.direct = &http.Transport{} + transport.direct.RegisterProtocol("https", roundTripperFunc(func(_ *http.Request) (*http.Response, error) { + directRoundTrips++ + return &http.Response{StatusCode: http.StatusNoContent, Body: http.NoBody}, nil + })) + t.Cleanup(desktoptransport.SetDesktopRunningForTest(func(context.Context) (bool, error) { return true, nil })) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://hub.docker.com/v2/", http.NoBody) + require.NoError(t, err) + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Zero(t, desktopRoundTrips, "Desktop RoundTrip must not be called when proxySafe returns false (private IP)") + assert.Equal(t, 1, directRoundTrips, "direct transport must handle the request when proxySafe returns false") +} + +func TestDesktopProxyDisabled(t *testing.T) { + for _, tc := range []struct { + value string + want bool + }{ + {"", false}, + {"0", false}, + {"false", false}, + {"anything", false}, + {"1", true}, + {"true", true}, + {"TRUE", true}, + {" yes ", true}, + {"On", true}, + } { + t.Run(tc.value, func(t *testing.T) { + t.Setenv(disableDesktopProxyEnv, tc.value) + t.Setenv(legacyDisableDesktopProxyEnv, "") + assert.Equal(t, tc.want, desktopProxyDisabled()) + }) + } + + t.Setenv(disableDesktopProxyEnv, "") + t.Setenv(legacyDisableDesktopProxyEnv, "1") + assert.False(t, desktopProxyDisabled()) +} + +func TestDesktopProxyDisabledWarnsOncePerInvalidValue(t *testing.T) { + var logs bytes.Buffer + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + resetInvalidDesktopProxySetting() + + t.Setenv(disableDesktopProxyEnv, "unexpected") + assert.False(t, desktopProxyDisabled()) + assert.False(t, desktopProxyDisabled()) + assert.Equal(t, 1, strings.Count(logs.String(), "unrecognized DOCKER_AGENT_DISABLE_DESKTOP_PROXY value")) + + t.Setenv(disableDesktopProxyEnv, "other") + assert.False(t, desktopProxyDisabled()) + assert.Equal(t, 2, strings.Count(logs.String(), "unrecognized DOCKER_AGENT_DISABLE_DESKTOP_PROXY value")) + + t.Setenv(disableDesktopProxyEnv, "") + assert.False(t, desktopProxyDisabled()) + t.Setenv(disableDesktopProxyEnv, "unexpected") + assert.False(t, desktopProxyDisabled()) + assert.Equal(t, 3, strings.Count(logs.String(), "unrecognized DOCKER_AGENT_DISABLE_DESKTOP_PROXY value")) +} diff --git a/pkg/httpclient/safeclient.go b/pkg/httpclient/safeclient.go index fc1e60f4b4..ed526121ca 100644 --- a/pkg/httpclient/safeclient.go +++ b/pkg/httpclient/safeclient.go @@ -13,12 +13,53 @@ import ( // this value uniformly affects every HTTP-based built-in tool. const DefaultToolHTTPTimeout = 30 * time.Second +var ( + safeTransport = NewDesktopAwareSSRFSafeTransport() + allowPrivateIPsTransport = newAllowPrivateIPsTransport() +) + +// TransportForAllowPrivateIPs returns the shared transport for the requested +// outbound address policy. +func TransportForAllowPrivateIPs(allowPrivateIPs bool) http.RoundTripper { + if allowPrivateIPs { + return allowPrivateIPsTransport + } + return safeTransport +} + +// ClientForAllowPrivateIPs returns a client with an independent timeout and +// shared transport for the requested outbound address policy. +func ClientForAllowPrivateIPs(timeout time.Duration, allowPrivateIPs bool) *http.Client { + return &http.Client{ + Timeout: timeout, + Transport: TransportForAllowPrivateIPs(allowPrivateIPs), + CheckRedirect: BoundedRedirects(10), + } +} + +// NewAllowPrivateIPsClient returns an HTTP client for explicit +// allow_private_ips opt-ins. It can reach private addresses directly, but uses +// Docker Desktop's PAC proxy when available; loopback is always direct. Docker +// Desktop remains optional and DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1 restores the +// default environment-proxy/direct behavior. +func NewAllowPrivateIPsClient(timeout time.Duration) *http.Client { + return ClientForAllowPrivateIPs(timeout, true) +} + // NewSafeClient returns the HTTP client used by built-in tools that issue // outbound calls to URLs the operator (or a fetched OpenAPI spec) supplies. // -// The default refuses connections to non-public IPs at dial time -// — defeating DNS rebinding to loopback / RFC1918 / link-local incl. cloud -// metadata at 169.254.169.254 — and bounds the redirect chain at 10 hops. +// On its direct path — Docker Desktop unavailable, disabled via +// DOCKER_AGENT_DISABLE_DESKTOP_PROXY, or the target host is outside the +// Docker-owned allowlist (docker.com and docker.io families) — the client +// refuses non-public IPs at dial time, defeating DNS rebinding to loopback / +// RFC1918 / link-local incl. cloud metadata at 169.254.169.254. +// +// When Docker Desktop is running, only Docker-owned hostnames (docker.com, +// docker.io and their subdomains) go through its PAC proxy. There the only +// local control is a pre-request DNS preflight (see proxySafe): Desktop +// selects and resolves the destination itself, so dial-time enforcement does +// not apply. Either way the redirect chain is bounded at 10 hops. // // When unsafe is true the client uses [http.DefaultTransport]. This branch // exists ONLY for tests, which use [httptest.NewServer] (binds to 127.0.0.1) @@ -27,9 +68,5 @@ func NewSafeClient(timeout time.Duration, unsafe bool) *http.Client { if unsafe { return &http.Client{Timeout: timeout} } - return &http.Client{ - Timeout: timeout, - Transport: NewSSRFSafeTransport(), - CheckRedirect: BoundedRedirects(10), - } + return ClientForAllowPrivateIPs(timeout, false) } diff --git a/pkg/httpclient/safeclient_test.go b/pkg/httpclient/safeclient_test.go new file mode 100644 index 0000000000..6743de05c7 --- /dev/null +++ b/pkg/httpclient/safeclient_test.go @@ -0,0 +1,52 @@ +package httpclient + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewAllowPrivateIPsClientReachesLoopback(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + request, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, http.NoBody) + require.NoError(t, err) + response, err := NewAllowPrivateIPsClient(time.Second).Do(request) + require.NoError(t, err) + defer response.Body.Close() + require.Equal(t, http.StatusNoContent, response.StatusCode) +} + +func TestClientForAllowPrivateIPsSharesPolicyTransport(t *testing.T) { + safeFirst := ClientForAllowPrivateIPs(time.Second, false) + safeSecond := ClientForAllowPrivateIPs(2*time.Second, false) + privateClient := ClientForAllowPrivateIPs(3*time.Second, true) + + require.NotSame(t, safeFirst, safeSecond) + assert.Equal(t, time.Second, safeFirst.Timeout) + assert.Equal(t, 2*time.Second, safeSecond.Timeout) + assert.Same(t, safeFirst.Transport, safeSecond.Transport) + assert.Same(t, safeFirst.Transport, TransportForAllowPrivateIPs(false)) + assert.Same(t, privateClient.Transport, TransportForAllowPrivateIPs(true)) + assert.NotSame(t, safeFirst.Transport, privateClient.Transport) +} + +func TestNewSafeClientRejectsLoopback(t *testing.T) { + server := httptest.NewServer(http.NotFoundHandler()) + defer server.Close() + + request, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, http.NoBody) + require.NoError(t, err) + response, err := NewSafeClient(time.Second, false).Do(request) + if response != nil { + defer response.Body.Close() + } + require.Error(t, err) +} diff --git a/pkg/httpclient/ssrf.go b/pkg/httpclient/ssrf.go index df043cb98f..de35a6aae0 100644 --- a/pkg/httpclient/ssrf.go +++ b/pkg/httpclient/ssrf.go @@ -107,6 +107,7 @@ func NewSSRFSafeTransport() *http.Transport { var t *http.Transport if base, ok := http.DefaultTransport.(*http.Transport); ok { t = base.Clone() + t.Proxy = environmentProxyFunc() } else { // http.DefaultTransport has been replaced by a wrapper (e.g. otelhttp). // We can't clone settings we can't see, so fall back to a minimal @@ -116,7 +117,7 @@ func NewSSRFSafeTransport() *http.Transport { "NewSSRFSafeTransport is using a minimal fallback transport — "+ "proxy env vars are honoured but other DefaultTransport settings are not inherited", "type", fmt.Sprintf("%T", http.DefaultTransport)) - t = &http.Transport{Proxy: http.ProxyFromEnvironment} + t = &http.Transport{Proxy: environmentProxyFunc()} } proxies := proxyDialAllowlist() guarded := &net.Dialer{ diff --git a/pkg/server/agent_source_errors_test.go b/pkg/server/agent_source_errors_test.go new file mode 100644 index 0000000000..d969c1097d --- /dev/null +++ b/pkg/server/agent_source_errors_test.go @@ -0,0 +1,92 @@ +package server + +import ( + "bytes" + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/config" + "github.com/docker/docker-agent/pkg/session" +) + +func TestAgentSourceErrorsAreTyped(t *testing.T) { + t.Parallel() + + fetchErr := fmt.Errorf("load source: %w", config.ErrSourceFetchFailed) + sm := NewSessionManager(t.Context(), config.Sources{ + "broken": &mockSource{name: "broken", err: fetchErr}, + "invalid": &mockSource{name: "invalid", err: errors.New("invalid configuration")}, + }, session.NewInMemorySessionStore(), 0, &config.RuntimeConfig{}) + + _, err := sm.LoadAgentConfig(t.Context(), "missing") + require.ErrorIs(t, err, ErrAgentNotFound) + _, err = sm.LoadAgentConfig(t.Context(), "broken") + require.ErrorIs(t, err, ErrAgentSourceUnavailable) + _, err = sm.LoadAgentConfig(t.Context(), "invalid") + require.Error(t, err) + require.NotErrorIs(t, err, ErrAgentSourceUnavailable) + _, err = sm.GetAgentToolCount(t.Context(), "broken", "") + require.ErrorIs(t, err, ErrAgentSourceUnavailable) + err = sm.sourceLoadError("broken", context.Canceled) + require.ErrorIs(t, err, context.Canceled) + err = sm.sourceLoadError("broken", context.DeadlineExceeded) + require.ErrorIs(t, err, context.DeadlineExceeded) +} + +func TestAgentSourceHTTPStatus(t *testing.T) { + t.Parallel() + + store := session.NewInMemorySessionStore() + sess := session.New() + require.NoError(t, store.AddSession(t.Context(), sess)) + fetchErr := fmt.Errorf("load source: %w", config.ErrSourceFetchFailed) + sm := NewSessionManager(t.Context(), config.Sources{ + "broken": &mockSource{name: "broken", err: fetchErr}, + "invalid": &mockSource{name: "invalid", err: errors.New("invalid configuration")}, + }, store, 0, &config.RuntimeConfig{}) + srv := NewWithManager(sm, "") + + for _, tc := range []struct { + name, method, path string + want int + }{ + {"config missing", http.MethodGet, "/api/agents/missing", http.StatusNotFound}, + {"config unavailable", http.MethodGet, "/api/agents/broken", http.StatusBadGateway}, + {"tool count missing", http.MethodGet, "/api/agents/missing/root/tools/count", http.StatusNotFound}, + {"tool count unavailable", http.MethodGet, "/api/agents/broken/root/tools/count", http.StatusBadGateway}, + {"run missing", http.MethodPost, "/api/sessions/" + sess.ID + "/agent/missing", http.StatusNotFound}, + {"run unavailable", http.MethodPost, "/api/sessions/" + sess.ID + "/agent/broken", http.StatusBadGateway}, + {"config invalid", http.MethodGet, "/api/agents/invalid", http.StatusInternalServerError}, + {"tool count invalid", http.MethodGet, "/api/agents/invalid/root/tools/count", http.StatusInternalServerError}, + {"run invalid", http.MethodPost, "/api/sessions/" + sess.ID + "/agent/invalid", http.StatusInternalServerError}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequestWithContext(t.Context(), tc.method, tc.path, bytes.NewBufferString(`{"messages":[]}`)) + rec := httptest.NewRecorder() + srv.e.ServeHTTP(rec, req) + assert.Equal(t, tc.want, rec.Code) + }) + } +} + +func TestGetAgentsSkipsUnavailableSources(t *testing.T) { + t.Setenv("OPENAI_API_KEY", "dummy") + + srv := NewWithManager(NewSessionManager(t.Context(), config.Sources{ + "broken": &mockSource{name: "broken", err: fmt.Errorf("load source: %w", config.ErrSourceFetchFailed)}, + "healthy": config.NewBytesSource("healthy", []byte("version: \"2\"\nagents:\n root:\n instruction: hi\n model: openai/gpt-4o\n")), + }, session.NewInMemorySessionStore(), 0, &config.RuntimeConfig{}), "") + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/agents", nil) + rec := httptest.NewRecorder() + srv.e.ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), `"name":"healthy"`) + assert.NotContains(t, rec.Body.String(), "broken") +} diff --git a/pkg/server/resolve_source_test.go b/pkg/server/resolve_source_test.go index 205f16d346..fd4943f68d 100644 --- a/pkg/server/resolve_source_test.go +++ b/pkg/server/resolve_source_test.go @@ -67,7 +67,7 @@ func TestResolveSource_AmbiguousFallbackFails(t *testing.T) { // A third tag not present exactly; two candidates share its identity. _, err := sm.resolveSource(gordonKey("v9-canary")) - require.Error(t, err) + require.ErrorIs(t, err, ErrAgentNotFound) assert.Contains(t, err.Error(), "agent not found") } @@ -83,7 +83,7 @@ func TestResolveSource_NoMatchFails(t *testing.T) { } _, err := sm.resolveSource(url.QueryEscape("http://localhost:7777/other-agent?gordonTag=v9-dev")) - require.Error(t, err) + require.ErrorIs(t, err, ErrAgentNotFound) assert.Contains(t, err.Error(), "agent not found") } diff --git a/pkg/server/server.go b/pkg/server/server.go index 6ed4008349..039ab278ab 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -184,6 +184,7 @@ func (s *Server) sessionsReady(c echo.Context) error { } func (s *Server) getAgents(c echo.Context) error { + // A failing source must not hide healthy agents from callers. agents := []api.Agent{} for k, agentSource := range s.sm.Sources { slog.Debug("API source", "source", agentSource.Name()) @@ -237,24 +238,22 @@ func agentsAPIEntry(name string, cfg *latest.Config) (api.Agent, bool) { } func (s *Server) getAgentConfig(c echo.Context) error { - agentID := c.Param("id") - - for k, agentSource := range s.sm.Sources { - if k != agentID { - continue - } - - slog.Debug("API source", "source", agentSource.Name()) - cfg, err := config.Load(c.Request().Context(), agentSource) - if err != nil { - slog.Error("Failed to load config from API source", "key", k, "error", err) - continue - } - - return c.JSON(http.StatusOK, cfg) + cfg, err := s.sm.LoadAgentConfig(c.Request().Context(), c.Param("id")) + if err != nil { + return agentSourceHTTPError("failed to load agent source", err) } + return c.JSON(http.StatusOK, cfg) +} - return echo.NewHTTPError(http.StatusNotFound) +func agentSourceHTTPError(operation string, err error) error { + switch { + case errors.Is(err, ErrAgentNotFound): + return echo.NewHTTPError(http.StatusNotFound, err.Error()) + case errors.Is(err, ErrAgentSourceUnavailable): + return echo.NewHTTPError(http.StatusBadGateway, fmt.Sprintf("%s: %v", operation, err)) + default: + return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("%s: %v", operation, err)) + } } func (s *Server) getSessions(c echo.Context) error { @@ -293,7 +292,7 @@ func (s *Server) createSession(c echo.Context) error { if errors.Is(err, ErrInvalidWorkingDir) { return echo.NewHTTPError(http.StatusBadRequest, err.Error()) } - return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("failed to create session: %v", err)) + return agentSourceHTTPError("failed to create session", err) } return c.JSON(http.StatusOK, sess) @@ -431,7 +430,7 @@ func (s *Server) updateSessionSafetyPolicy(c echo.Context) error { func (s *Server) getAgentToolCount(c echo.Context) error { count, err := s.sm.GetAgentToolCount(c.Request().Context(), c.Param("id"), c.Param("agent_name")) if err != nil { - return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("failed to get agent tool count: %v", err)) + return agentSourceHTTPError("failed to get agent tool count", err) } return c.JSON(http.StatusOK, map[string]int{"available_tools": count}) @@ -519,7 +518,7 @@ func (s *Server) runAgent(c echo.Context) error { if errors.Is(err, ErrModelSwitchingNotSupported) { return echo.NewHTTPError(http.StatusUnprocessableEntity, err.Error()) } - return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("failed to run session: %v", err)) + return agentSourceHTTPError("failed to run session", err) } c.Response().Header().Set("Content-Type", "text/event-stream") diff --git a/pkg/server/session_manager.go b/pkg/server/session_manager.go index ec9ca595c2..768689c12b 100644 --- a/pkg/server/session_manager.go +++ b/pkg/server/session_manager.go @@ -22,6 +22,7 @@ import ( "github.com/docker/docker-agent/pkg/chat" "github.com/docker/docker-agent/pkg/concurrent" "github.com/docker/docker-agent/pkg/config" + "github.com/docker/docker-agent/pkg/config/latest" "github.com/docker/docker-agent/pkg/runtime" "github.com/docker/docker-agent/pkg/session" "github.com/docker/docker-agent/pkg/sessiontitle" @@ -920,6 +921,11 @@ func (sm *SessionManager) WaitStopped(ctx context.Context, sessionID string, tim // ErrSessionBusy is returned when a session is already processing a request. var ErrSessionBusy = errors.New("session is already processing a request") +var ( + ErrAgentNotFound = errors.New("agent source not found") + ErrAgentSourceUnavailable = errors.New("agent source unavailable") +) + // RunSession runs a session with the given messages. // // When modelOverride is non-empty, it is applied to the session's current @@ -1588,13 +1594,27 @@ func (sm *SessionManager) applyAuthorSafetyDefault(ctx context.Context, sess *se } } +func (sm *SessionManager) sourceLoadError(agentFilename string, err error) error { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return err + } + if errors.Is(err, config.ErrSourceFetchFailed) { + return fmt.Errorf("%w: load %q: %w", ErrAgentSourceUnavailable, agentFilename, err) + } + return fmt.Errorf("load %q: %w", agentFilename, err) +} + func (sm *SessionManager) loadTeam(ctx context.Context, agentFilename string, runConfig *config.RuntimeConfig) (*team.Team, error) { agentSource, err := sm.resolveSource(agentFilename) if err != nil { return nil, err } - return teamloader.Load(ctx, agentSource, runConfig, loaderdefaults.Opts()...) + t, err := teamloader.Load(ctx, agentSource, runConfig, loaderdefaults.Opts()...) + if err != nil { + return nil, sm.sourceLoadError(agentFilename, err) + } + return t, nil } // loadTeamWithConfig is like loadTeam but also returns the loaded model and @@ -1606,7 +1626,26 @@ func (sm *SessionManager) loadTeamWithConfig(ctx context.Context, agentFilename } allOpts := append(loaderdefaults.Opts(), opts...) - return teamloader.LoadWithConfig(ctx, agentSource, runConfig, allOpts...) + result, err := teamloader.LoadWithConfig(ctx, agentSource, runConfig, allOpts...) + if err != nil { + return nil, sm.sourceLoadError(agentFilename, err) + } + return result, nil +} + +// LoadAgentConfig loads an agent configuration through the same source +// resolution boundary as agent execution. +func (sm *SessionManager) LoadAgentConfig(ctx context.Context, agentFilename string) (*latest.Config, error) { + agentSource, err := sm.resolveSource(agentFilename) + if err != nil { + return nil, err + } + + cfg, err := config.Load(ctx, agentSource) + if err != nil { + return nil, sm.sourceLoadError(agentFilename, err) + } + return cfg, nil } // resolveSource looks up the agent source for agentFilename. @@ -1640,7 +1679,7 @@ func (sm *SessionManager) resolveSource(agentFilename string) (config.Source, er return match, nil } - return nil, fmt.Errorf("agent not found: %s", agentFilename) + return nil, fmt.Errorf("%w: agent not found: %s", ErrAgentNotFound, agentFilename) } // applyRunModelOverride applies modelRef as the per-agent model override diff --git a/pkg/server/source_loader.go b/pkg/server/source_loader.go index 3909bcac5a..2d4bcf561b 100644 --- a/pkg/server/source_loader.go +++ b/pkg/server/source_loader.go @@ -9,6 +9,8 @@ import ( "github.com/docker/docker-agent/pkg/config" ) +var sourceRetrySchedule = []time.Duration{2 * time.Second, 15 * time.Second, 70 * time.Second} + type sourceLoader struct { inner config.Source refreshInterval time.Duration @@ -26,6 +28,10 @@ func newSourceLoader(ctx context.Context, inner config.Source, refreshInterval t sl.load(ctx) + if sl.hasError() { + go sl.retryStartup(ctx) + } + if refreshInterval > 0 { go sl.refreshLoop(ctx) } @@ -68,6 +74,29 @@ func (sl *sourceLoader) load(ctx context.Context) { } } +func (sl *sourceLoader) hasError() bool { + sl.mu.RLock() + defer sl.mu.RUnlock() + return sl.err != nil +} + +func (sl *sourceLoader) retryStartup(ctx context.Context) { + for _, delay := range sourceRetrySchedule { + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + timer.Stop() + return + case <-timer.C: + } + + sl.load(ctx) + if !sl.hasError() { + return + } + } +} + func (sl *sourceLoader) refreshLoop(ctx context.Context) { ticker := time.NewTicker(sl.refreshInterval) defer ticker.Stop() diff --git a/pkg/server/source_loader_test.go b/pkg/server/source_loader_test.go index 274974f779..bf1f970b57 100644 --- a/pkg/server/source_loader_test.go +++ b/pkg/server/source_loader_test.go @@ -211,3 +211,33 @@ func TestSourceLoader_SuccessThenError(t *testing.T) { assert.Equal(t, []byte("initial data"), data) }) } + +func TestSourceLoaderRetriesFailedStartup(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + inner := &mockSource{name: "test.yaml", err: errors.New("unavailable")} + sl := newSourceLoader(t.Context(), inner, 0) + + synctest.Wait() + time.Sleep(2 * time.Second) //nolint:forbidigo // fake time inside a synctest bubble + synctest.Wait() + assert.Equal(t, 2, inner.getReadCount()) + + inner.setErr(nil) + inner.setData([]byte("recovered")) + time.Sleep(15 * time.Second) //nolint:forbidigo // fake time inside a synctest bubble + synctest.Wait() + assert.Equal(t, 3, inner.getReadCount()) + + data, err := sl.Read(t.Context()) + require.NoError(t, err) + assert.Equal(t, []byte("recovered"), data) + }) +} + +func TestSourceRetryScheduleOutlivesDesktopDetectionCache(t *testing.T) { + var total time.Duration + for _, delay := range sourceRetrySchedule { + total += delay + } + assert.Greater(t, total, time.Minute) +} diff --git a/pkg/skills/cache.go b/pkg/skills/cache.go index d4d66680ca..74a44bb581 100644 --- a/pkg/skills/cache.go +++ b/pkg/skills/cache.go @@ -28,8 +28,10 @@ const remoteHTTPTimeout = 30 * time.Second // as instructions, so a hostile (or compromised) registry could otherwise // be used to read internal endpoints (loopback, RFC1918, link-local incl. // cloud metadata at 169.254.169.254) and exfiltrate them through prompt -// injection. The SSRF-safe client refuses such targets at dial time, after -// DNS resolution, defeating DNS rebinding. +// injection. Remote skill hosts are not in the Docker-owned allowlist, so +// they always use the direct SSRF-guarded path (no Desktop proxy): the +// client refuses such targets at dial time, after DNS resolution, defeating +// DNS rebinding. // // Tests in this package replace the var via TestMain (see main_test.go) // because httptest.NewServer binds to 127.0.0.1. diff --git a/pkg/toolinstall/registry.go b/pkg/toolinstall/registry.go index 9bfeddc53a..945ae3f818 100644 --- a/pkg/toolinstall/registry.go +++ b/pkg/toolinstall/registry.go @@ -162,14 +162,14 @@ var ( // NewRegistry creates a new Registry with default settings. func NewRegistry() *Registry { return &Registry{ - // httpclient.NewSafeClient enforces dial-time SSRF protection - // even though baseURL is hard-coded — a hostname that today - // resolves to a public IP can be DNS-rebound to 127.0.0.1 or - // 169.254.169.254 and we want the request to fail at dial, - // not after exfiltration. The 30s timeout matches the de-facto - // upper bound the request context already enforces. The - // transport is OTel-wrapped inside NewSafeClient itself, so - // outbound registry calls inject `traceparent` when enabled. + // httpclient.NewSafeClient enforces SSRF protection: the Aqua + // registry at raw.githubusercontent.com is not in the Docker-owned + // allowlist, so it always uses the direct SSRF-guarded path (no + // Desktop proxy). Dial-time enforcement refuses non-public IPs, + // defeating DNS rebinding. The 30s timeout matches the de-facto + // upper bound the request context already enforces. The transport is + // OTel-wrapped inside NewSafeClient itself, so outbound registry + // calls inject `traceparent` when enabled. httpClient: httpclient.NewSafeClient(30*time.Second, false), baseURL: registryBaseURL, cacheDir: RegistryDir(), diff --git a/pkg/tools/a2a/a2a.go b/pkg/tools/a2a/a2a.go index 6f6945d4a0..dc866a777c 100644 --- a/pkg/tools/a2a/a2a.go +++ b/pkg/tools/a2a/a2a.go @@ -185,13 +185,14 @@ func (t *Toolset) Start(ctx context.Context) error { // addresses (cloud metadata at 169.254.169.254 in particular). The // `allow_private_ips: true` opt-in disables this for legitimate // internal-service use. - resolver := agentcard.NewResolver(httpclient.NewSafeClient(t.timeout, t.allowPrivateIPs)) + client := httpclient.ClientForAllowPrivateIPs(t.timeout, t.allowPrivateIPs) + resolver := agentcard.NewResolver(client) card, err := resolver.Resolve(ctx, t.url) if err != nil { return fmt.Errorf("failed to fetch A2A agent card: %w", err) } - httpClient := httpclient.NewSafeClient(t.timeout, t.allowPrivateIPs) + httpClient := client base := httpClient.Transport if base == nil { base = http.DefaultTransport @@ -204,7 +205,7 @@ func (t *Toolset) Start(ctx context.Context) error { headers := t.expander.ExpandMap(ctx, t.headers) httpClient.Transport = upstream.NewHeaderTransportForOrigin(base, endpointOrigin, headers) - client, err := a2aclient.NewFromCard( + a2aClient, err := a2aclient.NewFromCard( ctx, card, a2aclient.WithDefaultsDisabled(), a2aclient.WithJSONRPCTransport(httpClient), @@ -214,7 +215,7 @@ func (t *Toolset) Start(ctx context.Context) error { } t.mu.Lock() - t.client = client + t.client = a2aClient t.card = card t.mu.Unlock() diff --git a/pkg/tools/builtin/api/api.go b/pkg/tools/builtin/api/api.go index cd33563360..ea467c3fcd 100644 --- a/pkg/tools/builtin/api/api.go +++ b/pkg/tools/builtin/api/api.go @@ -39,7 +39,7 @@ func (t *ToolSet) callTool(ctx context.Context, toolCall tools.ToolCall, _ tools endpoint := t.expander.Expand(ctx, t.config.Endpoint, nil) headers := t.expander.ExpandMap(ctx, t.config.Headers) - client := httpclient.NewSafeClient(t.timeout, t.allowPrivateIPs) + client := httpclient.ClientForAllowPrivateIPs(t.timeout, t.allowPrivateIPs) var reqBody io.Reader = http.NoBody switch t.config.Method { diff --git a/pkg/tools/builtin/fetch/fetch.go b/pkg/tools/builtin/fetch/fetch.go index c5cc856cad..b796e66370 100644 --- a/pkg/tools/builtin/fetch/fetch.go +++ b/pkg/tools/builtin/fetch/fetch.go @@ -106,10 +106,7 @@ func (h *fetchHandler) CallTool(ctx context.Context, params ToolArgs) (*tools.To // the unspecified address — even when DNS for an otherwise-public host // resolves there. Operators who legitimately need to call internal // services opt in via `allow_private_ips: true`. - var transport http.RoundTripper = httpclient.NewSSRFSafeTransport() - if h.allowPrivateIPs { - transport = http.DefaultTransport - } + transport := httpclient.TransportForAllowPrivateIPs(h.allowPrivateIPs) headers := h.expander.ExpandMap(ctx, h.headers) diff --git a/pkg/tools/builtin/openapi/openapi.go b/pkg/tools/builtin/openapi/openapi.go index f98b83badc..239b5c8833 100644 --- a/pkg/tools/builtin/openapi/openapi.go +++ b/pkg/tools/builtin/openapi/openapi.go @@ -123,7 +123,8 @@ func (t *ToolSet) fetchSpec(ctx context.Context) (*v3.Document, error) { req.Header.Set("Accept", "application/json") setHeaders(req, t.headers) - resp, err := httpclient.NewSafeClient(t.timeout, t.allowPrivateIPs).Do(req) + client := httpclient.ClientForAllowPrivateIPs(t.timeout, t.allowPrivateIPs) + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("request failed: %w", err) } @@ -482,7 +483,8 @@ func (h *openAPIHandler) callTool(ctx context.Context, params openAPICallArgs) ( headers := h.expander.ExpandMap(ctx, h.headers) setHeaders(req, headers) - resp, err := httpclient.NewSafeClient(h.timeout, h.allowPrivateIPs).Do(req) + client := httpclient.ClientForAllowPrivateIPs(h.timeout, h.allowPrivateIPs) + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("request failed: %w", err) } diff --git a/pkg/tools/mcp/oauthflow/oauthflow.go b/pkg/tools/mcp/oauthflow/oauthflow.go index ab59ca765a..b183e0fe9e 100644 --- a/pkg/tools/mcp/oauthflow/oauthflow.go +++ b/pkg/tools/mcp/oauthflow/oauthflow.go @@ -96,23 +96,10 @@ func SetBrowserOpenerForTesting(opener BrowserOpener) (restore func()) { // configurations that explicitly opt in to talking to a server on a private // network). func HTTPClientForAllowPrivateIPs(allowPrivateIPs bool) *http.Client { - if allowPrivateIPs { - // Clone keeps the default proxy/HTTP2/timeout behavior but gives the - // client its own connection pool: a nil Transport would share - // http.DefaultTransport's pool, which third parties may prune via - // CloseIdleConnections (httptest.Server.Close does). - transport, ok := http.DefaultTransport.(*http.Transport) - if !ok { - // Something replaced DefaultTransport (test helper, proxy shim); - // fall back to a fresh default rather than panicking. - transport = &http.Transport{} - } - return &http.Client{ - Timeout: 30 * time.Second, - Transport: transport.Clone(), - } + if !allowPrivateIPs { + return DefaultHTTPClient() } - return DefaultHTTPClient() + return httpclient.ClientForAllowPrivateIPs(30*time.Second, true) } // GenerateState generates a random state parameter for OAuth CSRF protection