diff --git a/pkg/selfupdate/selfupdate.go b/pkg/selfupdate/selfupdate.go index 60d52b967..e30e91953 100644 --- a/pkg/selfupdate/selfupdate.go +++ b/pkg/selfupdate/selfupdate.go @@ -34,6 +34,7 @@ import ( "github.com/mattn/go-isatty" "github.com/docker/docker-agent/pkg/atomicfile" + "github.com/docker/docker-agent/pkg/httpclient" ) const ( @@ -108,14 +109,19 @@ type Updater struct { // targeting the current binary and platform. func New(currentVersion string) *Updater { return &Updater{ - CurrentVersion: currentVersion, - Owner: defaultRepoOwner, - Repo: defaultRepoName, - APIBaseURL: defaultAPIBaseURL, - DownloadBaseURL: defaultDownloadBaseURL, - GOOS: runtime.GOOS, - GOARCH: runtime.GOARCH, - HTTPClient: &http.Client{Timeout: downloadTimeout}, + CurrentVersion: currentVersion, + Owner: defaultRepoOwner, + Repo: defaultRepoName, + APIBaseURL: defaultAPIBaseURL, + DownloadBaseURL: defaultDownloadBaseURL, + GOOS: runtime.GOOS, + GOARCH: runtime.GOARCH, + HTTPClient: &http.Client{ + Timeout: downloadTimeout, + // GitHub serves release assets through a redirect to its object + // storage: follow only HTTPS hops (no downgrade) and bound the chain. + CheckRedirect: httpclient.HTTPSOnlyRedirects(10), + }, resolveExecutable: resolveExecutable, install: installExecutable, reExec: reExecProcess, @@ -237,10 +243,15 @@ type releaseInfo struct { Tag string Asset string DownloadURL string + // SHA256 is the normalized (lowercase hex) SHA-256 digest of the asset, + // extracted from the GitHub API metadata by latestRelease. + SHA256 string } // latestRelease fetches the latest GitHub release metadata and locates the -// asset matching the current platform. +// asset matching the current platform, validating its download URL and +// SHA-256 digest so a bad release is rejected before any bytes are +// downloaded. func (u *Updater) latestRelease(ctx context.Context, assetName string) (releaseInfo, error) { endpoint := fmt.Sprintf("%s/repos/%s/%s/releases/latest", u.APIBaseURL, u.Owner, u.Repo) @@ -269,6 +280,7 @@ func (u *Updater) latestRelease(ctx context.Context, assetName string) (releaseI Assets []struct { Name string `json:"name"` BrowserDownloadURL string `json:"browser_download_url"` + Digest string `json:"digest"` } `json:"assets"` } if err := json.NewDecoder(io.LimitReader(resp.Body, 4<<20)).Decode(&release); err != nil { @@ -279,31 +291,45 @@ func (u *Updater) latestRelease(ctx context.Context, assetName string) (releaseI } for _, asset := range release.Assets { - if asset.Name == assetName { - if asset.BrowserDownloadURL == "" { - return releaseInfo{}, fmt.Errorf("release asset %s has no download URL", assetName) - } - if err := u.validateDownloadURL(asset.BrowserDownloadURL); err != nil { - return releaseInfo{}, err - } - return releaseInfo{ - Tag: release.TagName, - Asset: asset.Name, - DownloadURL: asset.BrowserDownloadURL, - }, nil + if asset.Name != assetName { + continue + } + if asset.BrowserDownloadURL == "" { + return releaseInfo{}, fmt.Errorf("release asset %s has no download URL", assetName) + } + if err := u.validateDownloadURL(asset.BrowserDownloadURL, release.TagName, asset.Name); err != nil { + return releaseInfo{}, err } + // Validate the digest up front: a missing or malformed digest can + // never verify, so fail closed now instead of after transferring up + // to maxBinarySize bytes. + sha256Hex, err := parseSHA256Digest(asset.Digest) + if err != nil { + return releaseInfo{}, fmt.Errorf("release asset %s: %w", assetName, err) + } + return releaseInfo{ + Tag: release.TagName, + Asset: asset.Name, + DownloadURL: asset.BrowserDownloadURL, + SHA256: sha256Hex, + }, nil } return releaseInfo{}, fmt.Errorf("latest release %s does not contain asset %s", release.TagName, assetName) } -// validateDownloadURL rejects asset URLs that do not point at the trusted -// download host. The asset URL comes from the GitHub API response, so a -// tampered or compromised response could otherwise redirect the binary -// download to an attacker-controlled host. The trusted host is derived from -// the hardcoded DownloadBaseURL (github.com in production), and the scheme of -// that base URL is enforced too. -func (u *Updater) validateDownloadURL(rawURL string) error { +// validateDownloadURL rejects asset URLs that do not point at the canonical +// release asset location under the trusted DownloadBaseURL (github.com in +// production): ///releases/download//. The URL comes +// from the GitHub API response; pinning it to the exact release path stops a +// tampered or accidental value from steering the initial request to another +// host, repository, tag or asset. GitHub then redirects that pinned URL to +// its object storage, and the integrity of the final content rests on the +// SHA-256 digest from the same metadata API. URL and digest share that trust +// root, so neither defends against a fully compromised API; the realistic +// goal is to block redirection of the first hop and, together with the +// digest, corruption or tampering of the content in transit. +func (u *Updater) validateDownloadURL(rawURL, tag, asset string) error { parsed, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("parsing asset download URL: %w", err) @@ -315,9 +341,34 @@ func (u *Updater) validateDownloadURL(rawURL string) error { if parsed.Scheme != base.Scheme { return fmt.Errorf("asset download URL %q scheme is not %q", rawURL, base.Scheme) } + if parsed.User != nil { + return fmt.Errorf("asset download URL %q must not contain user info", rawURL) + } if !strings.EqualFold(parsed.Hostname(), base.Hostname()) { return fmt.Errorf("asset download URL host %q is not the trusted host %q", parsed.Hostname(), base.Hostname()) } + if parsed.Port() != base.Port() { + return fmt.Errorf("asset download URL %q port does not match the trusted base URL", rawURL) + } + if parsed.ForceQuery || parsed.RawQuery != "" || parsed.Fragment != "" { + return fmt.Errorf("asset download URL %q must not contain a query or fragment", rawURL) + } + + // Compare raw path segments individually and unescape each one before + // matching, so an encoded slash (%2F) cannot smuggle extra segments past a + // whole-string comparison and an oddly-encoded but equivalent URL still + // matches. + want := []string{u.Owner, u.Repo, "releases", "download", tag, asset} + segments := strings.Split(strings.TrimPrefix(parsed.EscapedPath(), "/"), "/") + if len(segments) != len(want) { + return fmt.Errorf("asset download URL %q is not the expected release asset path", rawURL) + } + for i, segment := range segments { + got, err := url.PathUnescape(segment) + if err != nil || got != want[i] { + return fmt.Errorf("asset download URL %q is not the expected release asset path", rawURL) + } + } return nil } @@ -382,10 +433,10 @@ func (u *Updater) downloadAndStage(ctx context.Context, release releaseInfo, exe return "", fmt.Errorf("setting executable permissions: %w", err) } - // Integrity check is mandatory for self-update: do not execute or install a - // downloaded binary unless GitHub provides a digest or the release publishes - // a matching SHA-256 entry in checksums.txt. - if err := u.verifyChecksum(ctx, release, hex.EncodeToString(hasher.Sum(nil))); err != nil { + // Integrity check is mandatory for self-update: do not execute or install + // a downloaded binary unless its SHA-256 matches the digest latestRelease + // extracted from the GitHub metadata. + if err := verifyChecksum(release, hex.EncodeToString(hasher.Sum(nil))); err != nil { _ = os.Remove(tmpPath) return "", err } @@ -393,64 +444,40 @@ func (u *Updater) downloadAndStage(ctx context.Context, release releaseInfo, exe return tmpPath, nil } -// verifyChecksum verifies gotHex against the SHA-256 listed for the asset in -// the release's checksums.txt, fetched from the hardcoded DownloadBaseURL -// rather than any API-supplied value. It fails closed when checksums.txt is -// missing or does not list the asset, so a tampered API response cannot -// substitute its own digest for a malicious binary. -func (u *Updater) verifyChecksum(ctx context.Context, release releaseInfo, gotHex string) error { - endpoint := fmt.Sprintf("%s/%s/%s/releases/download/%s/checksums.txt", u.DownloadBaseURL, u.Owner, u.Repo, release.Tag) - - ctx, cancel := context.WithTimeout(ctx, httpTimeout) - defer cancel() - - req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, http.NoBody) - if err != nil { - return err - } - setGitHubAuth(req) - - resp, err := u.HTTPClient.Do(req) - if err != nil { - return fmt.Errorf("fetching checksums.txt: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("fetching checksums.txt: HTTP %d", resp.StatusCode) - } - - body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) - if err != nil { - return fmt.Errorf("reading checksums.txt: %w", err) - } - - want, ok := checksumFor(string(body), release.Asset) - if !ok { - return fmt.Errorf("checksums.txt does not list %s", release.Asset) - } - - if !strings.EqualFold(want, gotHex) { - return fmt.Errorf("checksum mismatch for %s: expected %s, got %s", release.Asset, want, gotHex) +// verifyChecksum verifies gotHex against release.SHA256, the digest +// latestRelease extracted and validated from the GitHub release metadata. +// Releases of this repository do not publish a checksums.txt, so the API +// digest is the only integrity source: it protects the downloaded bytes +// against corruption or tampering in transit (including the redirect to +// GitHub's object storage), but it comes from the same API response as the +// download URL, so it is no defence against a fully compromised API. The +// comparison is case-insensitive; an empty digest fails closed. +func verifyChecksum(release releaseInfo, gotHex string) error { + if release.SHA256 == "" { + return fmt.Errorf("release asset %s has no digest to verify against", release.Asset) + } + if !strings.EqualFold(release.SHA256, gotHex) { + return fmt.Errorf("checksum mismatch for %s: expected %s, got %s", release.Asset, release.SHA256, gotHex) } return nil } -// checksumFor parses a "sha256 filename" formatted checksums file and returns -// the hex digest for the given asset. -func checksumFor(contents, asset string) (string, bool) { - for line := range strings.Lines(contents) { - fields := strings.Fields(line) - if len(fields) != 2 { - continue - } - // The filename column may carry a leading "*" (binary mode marker). - name := strings.TrimPrefix(fields[1], "*") - if name == asset { - return fields[0], true - } +// parseSHA256Digest extracts the hex digest from a GitHub "sha256:" +// asset digest string, rejecting anything that is not exactly a well-formed +// SHA-256 digest. The returned hex is normalized to lowercase. +func parseSHA256Digest(digest string) (string, error) { + if digest == "" { + return "", errors.New("GitHub API reported no digest") + } + algo, hexDigest, ok := strings.Cut(digest, ":") + if !ok || algo != "sha256" { + return "", fmt.Errorf("unsupported digest %q, expected sha256:", digest) + } + raw, err := hex.DecodeString(hexDigest) + if err != nil || len(raw) != sha256.Size { + return "", fmt.Errorf("malformed sha256 digest %q", digest) } - return "", false + return hex.EncodeToString(raw), nil } // verifyBinary sanity-checks the staged binary by executing it with the @@ -466,19 +493,37 @@ func (u *Updater) verifyBinary(ctx context.Context, path string) error { defer cancel() cmd := exec.CommandContext(ctx, path, "version") - // Mark the probe so the freshly downloaded binary does not recursively - // attempt its own self-update while we are validating it. Keep the - // environment minimal so the probe cannot read model/provider secrets. - cmd.Env = []string{envReExecMarker + "=1"} - if runtime.GOOS == "windows" { - cmd.Env = append(cmd.Env, "SYSTEMROOT="+os.Getenv("SYSTEMROOT"), "PATH="+os.Getenv("PATH")) - } + cmd.Env = verifyEnv(runtime.GOOS, os.Getenv) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("staged binary failed to run (%w): %s", err, strings.TrimSpace(string(out))) } return nil } +// verifyEnv builds the environment for the staged-binary probe. The probe +// must not read model/provider secrets (API keys, GITHUB_TOKEN, proxy +// credentials, ...), so instead of filtering the inherited environment it +// allowlists the few non-secret variables the bare "version" subcommand +// needs to start: HOME, without which telemetry initialization panics on +// Unix (pkg/desktop resolves the user home directory), and on Windows +// SYSTEMROOT and PATH for process startup plus USERPROFILE and ProgramData, +// the counterparts used by os.UserHomeDir and the Docker Desktop paths +// there. Unset or empty variables are dropped. The re-exec marker is always +// present so the probe does not recursively attempt its own self-update. +func verifyEnv(goos string, getenv func(string) string) []string { + keys := []string{"HOME"} + if goos == "windows" { + keys = append(keys, "SYSTEMROOT", "PATH", "USERPROFILE", "ProgramData") + } + env := []string{envReExecMarker + "=1"} + for _, key := range keys { + if value := getenv(key); value != "" { + env = append(env, key+"="+value) + } + } + return env +} + // resolveExecutable returns the absolute, symlink-resolved path of the running // binary. Resolving symlinks ensures we replace the real file (e.g. the // Homebrew/cli-plugins target) rather than a link to it. diff --git a/pkg/selfupdate/selfupdate_test.go b/pkg/selfupdate/selfupdate_test.go index 6758f25ce..666a5a79d 100644 --- a/pkg/selfupdate/selfupdate_test.go +++ b/pkg/selfupdate/selfupdate_test.go @@ -8,10 +8,13 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" + "runtime" "strings" "sync" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" @@ -63,56 +66,82 @@ func TestAssetName(t *testing.T) { assert.Equal(t, "docker-agent-windows-arm64.exe", u.assetName()) } -func TestChecksumFor(t *testing.T) { +func TestParseSHA256Digest(t *testing.T) { t.Parallel() - contents := "abc123 docker-agent-linux-amd64\n" + - "def456 *docker-agent-darwin-arm64\n" + - "bad999 nested/docker-agent-windows-amd64.exe\n" + sum := sha256.Sum256([]byte("payload")) + hexSum := hex.EncodeToString(sum[:]) - got, ok := checksumFor(contents, "docker-agent-linux-amd64") - assert.True(t, ok) - assert.Equal(t, "abc123", got) + got, err := parseSHA256Digest("sha256:" + hexSum) + require.NoError(t, err) + assert.Equal(t, hexSum, got) - got, ok = checksumFor(contents, "docker-agent-darwin-arm64") - assert.True(t, ok) - assert.Equal(t, "def456", got) + got, err = parseSHA256Digest("sha256:" + strings.ToUpper(hexSum)) + require.NoError(t, err) + assert.Equal(t, hexSum, got, "digest hex must be normalized to lowercase") + + for name, digest := range map[string]string{ + "empty": "", + "no algorithm": hexSum, + "wrong algorithm": "sha512:" + hexSum, + "not hex": "sha256:not-hex-at-all", + "truncated": "sha256:" + hexSum[:32], + "trailing junk": "sha256:" + hexSum + "ff", + } { + _, err := parseSHA256Digest(digest) + assert.Error(t, err, "case %s: %q", name, digest) + } +} + +func TestVerifyChecksum(t *testing.T) { + t.Parallel() - _, ok = checksumFor(contents, "docker-agent-windows-amd64.exe") - assert.False(t, ok, "path-bearing entries should not match") + sum := sha256.Sum256([]byte("payload")) + hexSum := hex.EncodeToString(sum[:]) + + require.NoError(t, verifyChecksum(releaseInfo{Asset: testAssetName, SHA256: hexSum}, hexSum)) + require.NoError(t, verifyChecksum(releaseInfo{Asset: testAssetName, SHA256: strings.ToUpper(hexSum)}, hexSum), + "digest comparison must be case-insensitive") + + require.Error(t, verifyChecksum(releaseInfo{Asset: testAssetName}, hexSum), "empty digest must fail closed") + + err := verifyChecksum(releaseInfo{Asset: testAssetName, SHA256: hexSum}, strings.Repeat("0", 64)) + require.Error(t, err) + assert.Contains(t, err.Error(), "checksum mismatch") } -// newFakeRelease returns an httptest server emulating the GitHub release API -// and download endpoints for the given tag and asset payload. const testAssetName = "docker-agent-plan9-mips" -func newFakeRelease(t *testing.T, tag string, payload []byte, withChecksums bool) *httptest.Server { +func sha256Digest(payload []byte) string { + sum := sha256.Sum256(payload) + return "sha256:" + hex.EncodeToString(sum[:]) +} + +// newFakeRelease returns an httptest server emulating the GitHub release API +// and download endpoints for the given tag and asset payload, plus a counter +// of requests hitting the asset download endpoint. The digest is reported +// verbatim in the API response; pass sha256Digest(payload) for a valid +// release. +func newFakeRelease(t *testing.T, tag string, payload []byte, digest string) (*httptest.Server, *atomic.Int64) { t.Helper() assetName := testAssetName - sum := sha256.Sum256(payload) - checksums := fmt.Sprintf("%s %s\n", hex.EncodeToString(sum[:]), assetName) + var downloads atomic.Int64 var baseURL string mux := http.NewServeMux() mux.HandleFunc("/repos/docker/docker-agent/releases/latest", func(w http.ResponseWriter, _ *http.Request) { - fmt.Fprintf(w, `{"tag_name":%q,"assets":[{"name":%q,"browser_download_url":%q}]}`, tag, assetName, baseURL+"/docker/docker-agent/releases/download/"+tag+"/"+assetName) + fmt.Fprintf(w, `{"tag_name":%q,"assets":[{"name":%q,"browser_download_url":%q,"digest":%q}]}`, tag, assetName, baseURL+"/docker/docker-agent/releases/download/"+tag+"/"+assetName, digest) }) mux.HandleFunc("/docker/docker-agent/releases/download/"+tag+"/"+assetName, func(w http.ResponseWriter, _ *http.Request) { + downloads.Add(1) _, _ = w.Write(payload) }) - mux.HandleFunc("/docker/docker-agent/releases/download/"+tag+"/checksums.txt", func(w http.ResponseWriter, _ *http.Request) { - if !withChecksums { - http.NotFound(w, nil) - return - } - _, _ = io.WriteString(w, checksums) - }) srv := httptest.NewServer(mux) baseURL = srv.URL t.Cleanup(srv.Close) - return srv + return srv, &downloads } // newTestUpdater wires an Updater against srv, targeting a non-host platform so @@ -162,7 +191,7 @@ func (c *reExecCapture) fn(path string, args, env []string) error { func TestTryUpdateSuccess(t *testing.T) { t.Parallel() payload := []byte("#!/bin/sh\necho new binary\n") - srv := newFakeRelease(t, "v2.0.0", payload, true) + srv, _ := newFakeRelease(t, "v2.0.0", payload, sha256Digest(payload)) dir := t.TempDir() exePath := filepath.Join(dir, "docker-agent") @@ -184,10 +213,31 @@ func TestTryUpdateSuccess(t *testing.T) { assert.Contains(t, capt.env, envReExecMarker+"=1") } +func TestTryUpdateUppercaseDigest(t *testing.T) { + t.Parallel() + payload := []byte("#!/bin/sh\necho new binary\n") + sum := sha256.Sum256(payload) + srv, _ := newFakeRelease(t, "v2.0.0", payload, "sha256:"+strings.ToUpper(hex.EncodeToString(sum[:]))) + + dir := t.TempDir() + exePath := filepath.Join(dir, "docker-agent") + require.NoError(t, os.WriteFile(exePath, []byte("old binary"), 0o755)) + + u, capt := newTestUpdater(t, srv, "v1.0.0", exePath) + + var stderr strings.Builder + require.NoError(t, u.tryUpdate(t.Context(), nil, &stderr), "an uppercase hex digest must verify") + + got, err := os.ReadFile(exePath) + require.NoError(t, err) + assert.Equal(t, payload, got) + assert.True(t, capt.called) +} + func TestTryUpdateDeclinedDoesNotUpdate(t *testing.T) { t.Parallel() payload := []byte("#!/bin/sh\necho new binary\n") - srv := newFakeRelease(t, "v2.0.0", payload, true) + srv, _ := newFakeRelease(t, "v2.0.0", payload, sha256Digest(payload)) dir := t.TempDir() exePath := filepath.Join(dir, "docker-agent") @@ -237,7 +287,7 @@ func TestAnswerIsYes(t *testing.T) { func TestTryUpdateAlreadyLatest(t *testing.T) { t.Parallel() - srv := newFakeRelease(t, "v1.0.0", []byte("x"), true) + srv, _ := newFakeRelease(t, "v1.0.0", []byte("x"), sha256Digest([]byte("x"))) dir := t.TempDir() exePath := filepath.Join(dir, "docker-agent") @@ -255,7 +305,7 @@ func TestTryUpdateAlreadyLatest(t *testing.T) { func TestTryUpdateDevVersionNeverUpdates(t *testing.T) { t.Parallel() - srv := newFakeRelease(t, "v1.0.0", []byte("x"), true) + srv, _ := newFakeRelease(t, "v1.0.0", []byte("x"), sha256Digest([]byte("x"))) dir := t.TempDir() exePath := filepath.Join(dir, "docker-agent") @@ -277,22 +327,10 @@ func TestTryUpdateChecksumMismatch(t *testing.T) { exePath := filepath.Join(dir, "docker-agent") require.NoError(t, os.WriteFile(exePath, []byte("old"), 0o755)) - // Server advertises a checksum that does not match the payload. - bad := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case strings.HasSuffix(r.URL.Path, "/releases/latest"): - fmt.Fprintf(w, `{"tag_name":"v2.0.0","assets":[{"name":"docker-agent-plan9-mips","browser_download_url":%q}]}`, "http://"+r.Host+"/download/docker-agent-plan9-mips") - case strings.HasSuffix(r.URL.Path, "checksums.txt"): - fmt.Fprint(w, "deadbeef docker-agent-plan9-mips\n") - default: - _, _ = w.Write(payload) - } - })) - t.Cleanup(bad.Close) + // Server advertises a well-formed digest that does not match the payload. + srv, _ := newFakeRelease(t, "v2.0.0", payload, sha256Digest([]byte("something else"))) - u, capt := newTestUpdater(t, bad, "v1.0.0", exePath) - u.APIBaseURL = bad.URL - u.DownloadBaseURL = bad.URL + u, capt := newTestUpdater(t, srv, "v1.0.0", exePath) var stderr strings.Builder err := u.tryUpdate(t.Context(), nil, &stderr) @@ -305,32 +343,47 @@ func TestTryUpdateChecksumMismatch(t *testing.T) { assert.Equal(t, "old", string(got)) } -func TestTryUpdateMissingChecksumFailsClosed(t *testing.T) { +func TestTryUpdateBadDigestFailsClosed(t *testing.T) { t.Parallel() payload := []byte("real payload") - srv := newFakeRelease(t, "v2.0.0", payload, false) - - dir := t.TempDir() - exePath := filepath.Join(dir, "docker-agent") - require.NoError(t, os.WriteFile(exePath, []byte("old"), 0o755)) - - u, capt := newTestUpdater(t, srv, "v1.0.0", exePath) - - var stderr strings.Builder - err := u.tryUpdate(t.Context(), nil, &stderr) - require.Error(t, err) - assert.Contains(t, err.Error(), "checksums.txt") - assert.False(t, capt.called) - - got, err := os.ReadFile(exePath) - require.NoError(t, err) - assert.Equal(t, "old", string(got)) + sum := sha256.Sum256(payload) + hexSum := hex.EncodeToString(sum[:]) + + for name, digest := range map[string]string{ + "missing": "", + "no algorithm": hexSum, + "wrong algorithm": "sha512:" + hexSum, + "not hex": "sha256:not-hex-at-all", + "truncated": "sha256:" + hexSum[:32], + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + srv, downloads := newFakeRelease(t, "v2.0.0", payload, digest) + + dir := t.TempDir() + exePath := filepath.Join(dir, "docker-agent") + require.NoError(t, os.WriteFile(exePath, []byte("old"), 0o755)) + + u, capt := newTestUpdater(t, srv, "v1.0.0", exePath) + + var stderr strings.Builder + err := u.tryUpdate(t.Context(), nil, &stderr) + require.Error(t, err, "digest %q must fail closed", digest) + assert.Contains(t, err.Error(), "digest") + assert.False(t, capt.called) + assert.Zero(t, downloads.Load(), "a bad digest must be rejected before the asset is requested") + + got, err := os.ReadFile(exePath) + require.NoError(t, err) + assert.Equal(t, "old", string(got), "binary must be untouched") + }) + } } func TestTryUpdateReExecFailureRestoresPreviousBinary(t *testing.T) { t.Parallel() payload := []byte("new binary") - srv := newFakeRelease(t, "v2.0.0", payload, true) + srv, _ := newFakeRelease(t, "v2.0.0", payload, sha256Digest(payload)) dir := t.TempDir() exePath := filepath.Join(dir, "docker-agent") @@ -356,7 +409,8 @@ func TestTryUpdateDownloadNotFound(t *testing.T) { // Latest resolves but the asset 404s: must fail and leave binary intact. srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.HasSuffix(r.URL.Path, "/releases/latest") { - fmt.Fprintf(w, `{"tag_name":"v2.0.0","assets":[{"name":"docker-agent-plan9-mips","browser_download_url":%q}]}`, "http://"+r.Host+"/missing/docker-agent-plan9-mips") + fmt.Fprintf(w, `{"tag_name":"v2.0.0","assets":[{"name":"docker-agent-plan9-mips","browser_download_url":%q,"digest":%q}]}`, + "http://"+r.Host+"/docker/docker-agent/releases/download/v2.0.0/docker-agent-plan9-mips", sha256Digest([]byte("x"))) return } http.NotFound(w, r) @@ -401,10 +455,13 @@ func TestRunSwallowsErrors(t *testing.T) { func TestLatestReleaseAuthHeader(t *testing.T) { t.Setenv("GITHUB_TOKEN", "secret-token") + sum := sha256.Sum256([]byte("payload")) + hexSum := hex.EncodeToString(sum[:]) var gotAuth string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { gotAuth = r.Header.Get("Authorization") - fmt.Fprintf(w, `{"tag_name":"v9.9.9","assets":[{"name":"docker-agent-plan9-mips","browser_download_url":%q}]}`, "http://"+r.Host+"/download") + fmt.Fprintf(w, `{"tag_name":"v9.9.9","assets":[{"name":"docker-agent-plan9-mips","browser_download_url":%q,"digest":%q}]}`, + "http://"+r.Host+"/docker/docker-agent/releases/download/v9.9.9/docker-agent-plan9-mips", "sha256:"+hexSum) })) t.Cleanup(srv.Close) @@ -419,9 +476,31 @@ func TestLatestReleaseAuthHeader(t *testing.T) { release, err := u.latestRelease(t.Context(), "docker-agent-plan9-mips") require.NoError(t, err) assert.Equal(t, "v9.9.9", release.Tag) + assert.Equal(t, hexSum, release.SHA256, "latestRelease must keep the normalized asset digest") assert.Equal(t, "Bearer secret-token", gotAuth) } +func TestLatestReleaseRejectsBadDigest(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, `{"tag_name":"v9.9.9","assets":[{"name":"docker-agent-plan9-mips","browser_download_url":%q,"digest":"sha512:abc"}]}`, + "http://"+r.Host+"/docker/docker-agent/releases/download/v9.9.9/docker-agent-plan9-mips") + })) + t.Cleanup(srv.Close) + + u := &Updater{ + Owner: "docker", + Repo: "docker-agent", + APIBaseURL: srv.URL, + DownloadBaseURL: srv.URL, + HTTPClient: srv.Client(), + } + + _, err := u.latestRelease(t.Context(), "docker-agent-plan9-mips") + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported digest") +} + func TestLatestReleaseRejectsUntrustedDownloadHost(t *testing.T) { t.Parallel() // The asset download URL points at an attacker-controlled host while the @@ -448,15 +527,58 @@ func TestLatestReleaseRejectsUntrustedDownloadHost(t *testing.T) { func TestValidateDownloadURL(t *testing.T) { t.Parallel() - u := &Updater{DownloadBaseURL: "https://github.com"} + u := &Updater{Owner: "docker", Repo: "docker-agent", DownloadBaseURL: "https://github.com"} + const tag, asset = "v1.119.0", "docker-agent-linux-amd64" + const good = "https://github.com/docker/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64" + + require.NoError(t, u.validateDownloadURL(good, tag, asset)) + require.NoError(t, u.validateDownloadURL("https://GitHub.com/docker/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64", tag, asset), + "host comparison must be case-insensitive") + require.NoError(t, u.validateDownloadURL("https://github.com/docker/docker-agent/releases/download/v1.119.0/docker%2Dagent-linux-amd64", tag, asset), + "escaped-but-equivalent path segments must be accepted") + + for name, raw := range map[string]string{ + "foreign host": "https://objects.githubusercontent.com/docker/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64", + "non-HTTPS": "http://github.com/docker/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64", + "userinfo": "https://user:pass@github.com/docker/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64", + "unexpected port": "https://github.com:8443/docker/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64", + "query": good + "?x=1", + "fragment": good + "#frag", + "other owner": "https://github.com/evil/docker-agent/releases/download/v1.119.0/docker-agent-linux-amd64", + "other repo": "https://github.com/docker/evil/releases/download/v1.119.0/docker-agent-linux-amd64", + "other tag": "https://github.com/docker/docker-agent/releases/download/v9.9.9/docker-agent-linux-amd64", + "other asset": "https://github.com/docker/docker-agent/releases/download/v1.119.0/evil", + "extra segment": good + "/extra", + "missing segment": "https://github.com/docker/docker-agent/releases/download/v1.119.0", + "not a release path": "https://github.com/docker/docker-agent/archive/v1.119.0/docker-agent-linux-amd64", + "encoded slash": "https://github.com/docker/docker-agent/releases/download/v1.119.0%2Fdocker-agent-linux-amd64", + "unparseable": "://bad", + } { + assert.Error(t, u.validateDownloadURL(raw, tag, asset), "case %s: %s", name, raw) + } +} + +func TestNewHTTPClientRedirectPolicy(t *testing.T) { + t.Parallel() + + check := New("v1.0.0").HTTPClient.CheckRedirect + require.NotNil(t, check, "production client must restrict redirects") + + mustParse := func(s string) *url.URL { + u, err := url.Parse(s) + require.NoError(t, err) + return u + } + + require.NoError(t, check(&http.Request{URL: mustParse("https://objects.githubusercontent.com/asset")}, make([]*http.Request, 1))) - require.NoError(t, u.validateDownloadURL("https://github.com/docker/docker-agent/releases/download/v1/asset")) - require.NoError(t, u.validateDownloadURL("https://GitHub.com/docker/docker-agent/releases/download/v1/asset")) + err := check(&http.Request{URL: mustParse("http://github.com/asset")}, make([]*http.Request, 1)) + require.Error(t, err, "HTTP downgrade must be refused") + assert.Contains(t, err.Error(), "non-https") - require.Error(t, u.validateDownloadURL("https://objects.githubusercontent.com/asset"), "foreign host must be rejected") - require.Error(t, u.validateDownloadURL("http://github.com/asset"), "non-HTTPS must be rejected") - require.Error(t, u.validateDownloadURL("https://evil.example.com/asset")) - require.Error(t, u.validateDownloadURL("://bad")) + err = check(&http.Request{URL: mustParse("https://github.com/asset")}, make([]*http.Request, 10)) + require.Error(t, err, "redirect chain must be bounded") + assert.Contains(t, err.Error(), "10 redirects") } func TestSelfUpdateEnvStripsMarkers(t *testing.T) { @@ -489,6 +611,85 @@ func countKey(env []string, key string) int { return n } +func TestVerifyEnv(t *testing.T) { + t.Parallel() + + fakeEnv := func(vars map[string]string) func(string) string { + return func(key string) string { return vars[key] } + } + + // Secrets and proxy settings must never reach the probe: the env is an + // allowlist, so asserting the exact result proves nothing else leaks. + secrets := map[string]string{ + "HOME": "/home/me", + "GITHUB_TOKEN": "gh-secret", + "GH_TOKEN": "gh-secret", + "OPENAI_API_KEY": "sk-secret", + "HTTPS_PROXY": "http://user:pass@proxy:3128", + "PATH": "/usr/bin", + } + assert.Equal(t, []string{envReExecMarker + "=1", "HOME=/home/me"}, verifyEnv("darwin", fakeEnv(secrets))) + assert.Equal(t, []string{envReExecMarker + "=1", "HOME=/home/me"}, verifyEnv("linux", fakeEnv(secrets))) + + // Unset and empty variables are dropped; the loop-guard marker always stays. + assert.Equal(t, []string{envReExecMarker + "=1"}, verifyEnv("linux", fakeEnv(nil))) + assert.Equal(t, []string{envReExecMarker + "=1"}, verifyEnv("darwin", fakeEnv(map[string]string{"HOME": ""}))) + + // Windows additionally passes through the system variables the probe + // needs to start, still excluding everything else. + got := verifyEnv("windows", fakeEnv(map[string]string{ + "HOME": `C:\home`, + "SYSTEMROOT": `C:\Windows`, + "PATH": `C:\Windows\system32`, + "USERPROFILE": `C:\Users\me`, + "ProgramData": `C:\ProgramData`, + "GITHUB_TOKEN": "gh-secret", + })) + assert.Equal(t, []string{ + envReExecMarker + "=1", + `HOME=C:\home`, + `SYSTEMROOT=C:\Windows`, + `PATH=C:\Windows\system32`, + `USERPROFILE=C:\Users\me`, + `ProgramData=C:\ProgramData`, + }, got) + + assert.Equal(t, []string{envReExecMarker + "=1", `SYSTEMROOT=C:\Windows`}, + verifyEnv("windows", fakeEnv(map[string]string{"SYSTEMROOT": `C:\Windows`})), + "unset Windows variables must be dropped, not passed empty") +} + +// TestVerifyBinaryProvidesHome reproduces the live blocker: the "version" +// subcommand resolves the user home directory during telemetry/desktop +// initialization and dies without HOME on macOS. The stand-in script fails +// exactly when one of the probe-environment guarantees is violated. +func TestVerifyBinaryProvidesHome(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("probe stand-in is a shell script") + } + + script := filepath.Join(t.TempDir(), "docker-agent") + require.NoError(t, os.WriteFile(script, fmt.Appendf(nil, `#!/bin/sh +[ "$1" = version ] || exit 13 +[ -n "$HOME" ] || exit 10 +[ -z "$GITHUB_TOKEN" ] || exit 11 +[ "$%s" = 1 ] || exit 12 +echo v9.9.9 +`, envReExecMarker), 0o755)) + + // Host platform so verifyBinary actually runs the probe. + u := &Updater{GOOS: runtime.GOOS, GOARCH: runtime.GOARCH} + + t.Setenv("HOME", t.TempDir()) + t.Setenv("GITHUB_TOKEN", "gh-secret") + require.NoError(t, u.verifyBinary(t.Context(), script)) + + t.Setenv("HOME", "") + err := u.verifyBinary(t.Context(), script) + require.Error(t, err, "probe must fail without HOME, like the version command on macOS") + assert.Contains(t, err.Error(), "staged binary failed to run") +} + func TestCleanupRemovesBackup(t *testing.T) { backup := filepath.Join(t.TempDir(), backupFilePrefix+"123") require.NoError(t, os.WriteFile(backup, []byte("old"), 0o755))