feat: add opt-in self-update#2993
Conversation
Prompt users on interactive terminals before installing updates, while auto-confirming on non-interactive sessions (CI, pipes) so automation continues to work seamlessly.
Cleanup() now checks that the DOCKER_AGENT_SELF_UPDATE_BACKUP path basename matches our expected .docker-agent-backup- prefix before removing it. This prevents an attacker-controlled environment variable from turning startup into arbitrary file deletion.
isManagementInvocation now scans all args for --help/-h, not just args[0]. This prevents per-subcommand help requests (e.g. 'run --help') from triggering a self-update check.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR adds a well-structured opt-in self-update mechanism with checksum verification, backup/restore, loop guard, and platform-specific exec. A few issues were found in the added code worth addressing.
| return releaseInfo{ | ||
| Tag: release.TagName, | ||
| Asset: asset.Name, | ||
| DownloadURL: asset.BrowserDownloadURL, |
There was a problem hiding this comment.
[MEDIUM] Binary download URL from GitHub API is followed without hostname validation
latestRelease stores asset.BrowserDownloadURL directly from the GitHub API response, and downloadAndStage issues an HTTP request to that URL without validating its origin. In a scenario where the GitHub API response is tampered with (e.g., via a compromised API token or a server-side compromise), the URL could redirect the binary download to an attacker-controlled host.
Importantly, the checksum verification path is partially protected: verifyChecksum fetches checksums.txt from the hardcoded u.DownloadBaseURL (not from the API-supplied URL), so the checksum is verified from a trusted source. However, if the asset also supplies a Digest field in the API response (line 289), that digest is API-supplied — so a compromised API response can supply a matching digest for a malicious binary, bypassing checksum verification entirely.
Consider validating the BrowserDownloadURL hostname is github.com or objects.githubusercontent.com before following it, and/or ignoring the Digest field from the API response in favour of always using the checksums.txt fallback from the hardcoded base URL.
There was a problem hiding this comment.
Fixed in 001a22a. The asset browser_download_url is now validated against the hardcoded DownloadBaseURL host and scheme (validateDownloadURL) before it is followed, so a tampered API response can no longer redirect the download to an attacker host. The API-supplied digest field has been dropped entirely: verifyChecksum now always fetches checksums.txt from the trusted DownloadBaseURL, so a compromised API response cannot supply a matching digest for a malicious binary. Added TestLatestReleaseRejectsUntrustedDownloadHost and TestValidateDownloadURL.
There was a problem hiding this comment.
Perfect, thank you for the thorough fix! I've verified the implementation:
✅ validateDownloadURL now enforces that the asset URL matches the hardcoded DownloadBaseURL scheme and hostname before following it, blocking tampered API responses from redirecting downloads.
✅ API-supplied digest dropped: The Digest field is no longer extracted from the API response. verifyChecksum now unconditionally fetches checksums.txt from the trusted DownloadBaseURL, eliminating the path where a compromised API could supply a matching digest for a malicious binary.
✅ Test coverage: The new tests properly exercise the validation (including hostname and scheme checks) and the untrusted-host rejection.
The security issue is fully resolved.
|
|
||
| // Re-execute the freshly installed binary with the original arguments, | ||
| // marking the child so it will not attempt to update again. | ||
| env := append(os.Environ(), envReExecMarker+"=1", envBackupMarker+"="+backup) |
There was a problem hiding this comment.
[LOW] append(os.Environ(), ...) may leave duplicate env vars; child reads stale backup path
If DOCKER_AGENT_SELF_UPDATE_BACKUP is already present in the parent environment (from a prior update cycle that didn't clean up), append(os.Environ(), ..., envBackupMarker+"="+backup) produces two entries for the same key. On Linux, os.Getenv (and the underlying getenv(3)) returns the first match — so the re-executed child's Cleanup() reads the old, stale backup path rather than the newly created backup, and the new backup temp file is never removed.
While the re-exec loop guard (DOCKER_AGENT_SELF_UPDATE_REEXEC) prevents a second update attempt, the stale env var is still passed down. The fix is to deduplicate/filter existing DOCKER_AGENT_SELF_UPDATE_* entries from os.Environ() before appending the new values.
There was a problem hiding this comment.
Fixed in 001a22a. Before re-exec, selfUpdateEnv strips any inherited DOCKER_AGENT_SELF_UPDATE_* entries from os.Environ() so the fresh markers are the only ones present. This prevents a stale backup path from a prior cycle shadowing the new one (getenv returns the first match) and leaking the temp backup. Added TestSelfUpdateEnvStripsMarkers.
There was a problem hiding this comment.
Perfect! The selfUpdateEnv filter cleanly solves the duplicate env var issue. Stripping inherited DOCKER_AGENT_SELF_UPDATE_* entries before appending fresh markers ensures getenv always reads the correct backup path, and the test coverage confirms both the filtering logic and the single-entry guarantee.
Thanks for the thorough fix and test — this closes the temp file leak scenario cleanly.
| } | ||
|
|
||
| return answerIsYes(line) | ||
| } |
There was a problem hiding this comment.
[LOW] isInteractive type-asserts to *os.File; any wrapped stdin silently auto-confirms
isInteractive returns false for any stdin that is not a *os.File, which causes confirmUpdate to auto-confirm the update without user interaction. In the current call site Execute(ctx, stdin, ...) passes stdin as-is, so if a caller wraps os.Stdin (e.g., in a test harness, integration runner, or a future refactor that buffers stdin), the update is silently auto-confirmed.
The comment documents this behaviour for *os.File vs non-*os.File, but a wrapped os.Stdin from a real terminal is a realistic scenario. Consider also accepting an *os.File unwrapped via io.Reader interface probing, or document the contract explicitly so callers know they must pass os.Stdin directly rather than a wrapper.
There was a problem hiding this comment.
Fixed in 001a22a. isInteractive now probes for an interface{ Fd() uintptr } accessor instead of asserting the concrete *os.File type, so a wrapped real terminal (test harness, buffered stdin, future refactor) is still detected and prompted rather than silently auto-confirmed. The doc comment was updated accordingly.
There was a problem hiding this comment.
Perfect! The interface probe for Fd() method is exactly the right approach — it handles wrapped terminals while still correctly returning false for test buffers or non-terminal stdin. The updated doc comment makes the contract clear for future callers.
Thanks for the quick fix!
Addresses the remaining PR review findings: - Validate the asset download URL against the hardcoded trusted host and scheme before following it, so a tampered GitHub API response cannot redirect the binary download to an attacker-controlled host. - Drop the API-supplied asset digest entirely; checksum verification now always uses checksums.txt fetched from the hardcoded DownloadBaseURL, so a compromised API response cannot supply a matching digest for a malicious binary. - Strip inherited DOCKER_AGENT_SELF_UPDATE_* markers before re-exec so a stale backup path cannot shadow the new one and leak the temp backup. - isInteractive now probes for an Fd() accessor instead of asserting a concrete *os.File, so a wrapped real terminal is still detected.
docker-agent can now update itself from GitHub releases via the opt-in
DOCKER_AGENT_AUTO_UPDATEenvironment variable. The implementation performs digest and checksum verification to ensure authenticity, and includes fail-safe fallback and restore mechanisms on re-exec failure. Metadata, version, help, and shell completion invocations are skipped to avoid unnecessary updates.When a newer release is available, the user is now asked to confirm the upgrade:
The feature is fully tested and documented, with implementation details covering Unix and Windows platforms separately. It integrates seamlessly with the existing agent lifecycle and respects configuration preferences.
All changes are backward compatible. Existing users will not be affected unless they explicitly enable the auto-update feature.