fix(toolinstall): route the registry client through httpclient.NewSafeClient#2726
Open
dgageot wants to merge 1 commit intodocker:mainfrom
Open
fix(toolinstall): route the registry client through httpclient.NewSafeClient#2726dgageot wants to merge 1 commit intodocker:mainfrom
dgageot wants to merge 1 commit intodocker:mainfrom
Conversation
…eClient The tool-install Registry talks to a hard-coded URL (registryBaseURL), but it does so via http.DefaultClient — meaning no dial-time SSRF protection, no redirect filter, and a no-timeout default that relies on the request context for liveness. A hard-coded hostname is not by itself a defence: today's resolution to a public IP can become tomorrow's resolution to 127.0.0.1 or 169.254.169.254 (DNS rebinding), at which point the registry client silently follows along. httpclient.NewSafeClient enforces an IP allow-list at dial time, bounds the redirect chain at 10 hops, and in this call site we pin a 30s timeout to match the de-facto bound the request context already enforces. No behavioural change for the happy path: the public registry URL resolves to a public IP, dial proceeds, the same JSON comes back. The change closes a defence-in-depth gap that becomes load-bearing the day the registry is moved to a hostname an attacker can influence (an internal mirror, a CDN with a wider TTL, an operator-overridable env).
docker-agent
reviewed
May 9, 2026
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The change is correct and well-motivated. Replacing http.DefaultClient with httpclient.NewSafeClient(30*time.Second, false) cleanly closes the DNS-rebinding SSRF gap without touching any call sites or altering the happy-path behaviour. No bugs, resource leaks, or logic errors found in the added lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The tool-install
Registrytalks to a hard-coded URL(
registryBaseURL) but does so viahttp.DefaultClient— so nodial-time SSRF protection, no redirect filter, no timeout (relying
on the request context for liveness).
A hard-coded hostname is not by itself a defence: today's resolution
to a public IP can become tomorrow's resolution to
127.0.0.1or169.254.169.254under DNS rebinding (or under a future operatoroverride of the registry URL), at which point the registry client
silently follows along.
httpclient.NewSafeClientenforces an IP allow-list at dial time,bounds the redirect chain at 10 hops, and we pin a 30s timeout here
to match the de-facto bound the request context already enforces.
Behaviour
No change for the happy path — the public registry URL resolves to
a public IP, dial proceeds, the same JSON comes back. This is a
defence-in-depth fix that closes a gap before someone moves the
registry to a hostname an attacker can influence (internal mirror,
CDN with a wider TTL, env-overridable URL, etc.).