Repository navigation
Conversation
Add the Docker secrets engine client, used to read the Docker Desktop session for Docker Hub. It brings in github.com/docker/secrets-engine/x v0.8.5 and connectrpc.com/connect v1.19.1. The client module requires go1.26.10, which raises the go directive in vendor.mod from 1.26.0. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
When Docker Desktop is running and signed in, read the Docker Hub session from the Docker Desktop secrets engine, and send it as the registry credentials for Docker Hub (docker.io) and Docker Hardened Images (dhi.io) when pulling or pushing images through the daemon: docker pull, push, run, create, plugin install and push, service create and update, and stack deploy. Credentials in DOCKER_AUTH_CONFIG take precedence for the registries they cover. Without a usable Docker Desktop session (no socket, nothing listening, signed out, or no answer within 5 seconds), the CLI uses its credential store, credential helpers, and configuration file as before. docker login and stored credentials are not changed. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Cover which registries use the Docker Desktop session, keeping the production and staging sessions apart, caching the session until it expires, the retry interval after a failed lookup, and Disable. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
…ocket Run lookups against a fake secrets engine on a unix socket: signed in, signed out, no socket, a socket with nothing listening, and an engine that never answers. The benchmarks cover the same cases. BenchmarkExternalEngine runs against the engine at SECRETSENGINE_BENCH_SOCKET when it is set. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Tests that use FakeCli pull, push, and create containers through RetrieveAuthTokenFromImage. Disable the Docker Desktop lookup, so that they never reach the Docker Desktop of the machine they run on. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Check that the Docker Desktop session is used for Docker Hub and dhi.io, that other registries use the configuration file, and that DOCKER_AUTH_CONFIG takes precedence over Docker Desktop. TestRetrieveAuthTokenFromImage disables the Docker Desktop lookup. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
7c99af5 to
a4adc90
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
| s.hub = hub | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), s.timeout) |
There was a problem hiding this comment.
[low] Shared 5-second context split across two sequential RPC calls
fetch() creates a single context.WithTimeout(context.Background(), s.timeout) (5 s) and passes it to both GetDefaultSession and — when userSession.Claims.Username is empty — a subsequent GetDefaultProfile call. If the first call consumes the majority of the budget the second call gets only the remainder and may time out immediately, causing a spurious failure and a 1-minute retry backoff.
In practice the benchmarks show lookups take 2–15 ms (far below 5 s), so both calls complete well within the budget under normal conditions. The consequence of a timeout is also graceful: auth() sets retryAt and falls back to the CLI's own credentials. Consider using two separate timeouts (one per call) to make the intent explicit and to avoid a theoretical starvation when the engine is under load.
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
|
|
||
| userSession, err := s.hub.GetDefaultSession(ctx) | ||
| if err != nil { | ||
| return types.AuthConfig{}, time.Time{}, err |
There was a problem hiding this comment.
[low] Stale s.hub client is never reset after a connection failure
s.hub is set once in fetch() and never cleared when GetDefaultSession returns an error. After the 1-minute retryAt window elapses, fetch() skips the if s.hub == nil { s.connect() } branch and retries on the same client object.
For the common case of a Docker Desktop restart at the same socket path, the existing HTTP/1.1 keep-alive transport will auto-redial and the retry succeeds. However, if the socket path were to change (e.g., Docker Desktop is reinstalled at a different path), the stale transport continues dialling the old path and retries will keep failing until the process restarts.
Resetting s.hub = nil on the error path inside auth() (or inside fetch() when a *seclient.ConnectError is returned) would guarantee a clean reconnect on every retry:
| return types.AuthConfig{}, time.Time{}, err | |
| userSession, err := s.hub.GetDefaultSession(ctx) | |
| if err != nil { | |
| s.hub = nil | |
| return types.AuthConfig{}, time.Time{}, err | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
| for _, item := range resp.Msg.GetEnvelopes() { | ||
| id, err := secrets.ParseID(item.GetId()) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
[low] ParseID errors are silently swallowed in resolverClient.GetSecrets
When the server returns envelopes whose IDs cannot be parsed by secrets.ParseID, the loop continues without logging or propagating the error. If every envelope in the response has an unparseable ID, the function returns (nil, nil) — an empty slice with no error — instead of surfacing the failure.
Callers in desktopauth treat an empty/nil result as "no session found" and fall through to the next credential source, hiding a potential protocol mismatch between the client library and the secrets engine.
In normal operation the server serializes IDs that were previously validated, so this path should not trigger. However, a format change or server/client version mismatch would silently degrade rather than report an actionable error. Consider logging at debug level when an ID cannot be parsed, or returning a partial-result error.
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
3 participants
Warning
Work in progress. This is a draft to share the approach and the measurements. See Open items for what is left.
Note
This PR was created with alki, an AI coding agent, running Claude Opus 5.5: the code, the tests, the benchmarks, and this description.
Summary
When Docker Desktop is running and signed in, use its Docker Hub session as the first source of registry credentials for Docker Hub and Docker Hardened Images, as docker/compose does.
A new
internal/secretsenginepackage reads the default Docker Hub session from the Docker Desktop secrets engine, throughgithub.com/docker/secrets-engine/client.command.RetrieveAuthTokenFromImageasks for credentials in this order:DOCKER_AUTH_CONFIG, for the registries it defines.This covers
docker pull,push,run,create,plugin installandpush,service createandupdate, andstack deploy.Registries:
docker.io,index.docker.io, andregistry-1.docker.iouse the Docker Hub session.dhi.iouses the Docker Hub session, withServerAddressset todhi.io.registry-1-stage.docker.iouses the staging session.docker login,docker logout, and stored credentials are not changed. A Docker Desktop token is never written to the credential store.Falling back to the CLI's own credentials:
os.Lstat, no client is created.On macOS, Docker Desktop does not prompt Docker-signed binaries. Binaries that are not signed by Docker, such as local builds and test binaries, would prompt.
test.NewFakeCliturns the lookup off, so that unit tests never reach the Docker Desktop of the machine they run on.Commits:
vendor: github.com/docker/secrets-engine/client v0.2.2cli/command: use the Docker Desktop session for Docker Hubinternal/secretsengine: add unit testsinternal/secretsengine: test and benchmark against a secrets engine socketinternal/test: keep FakeCli from contacting Docker Desktopcli/command: test credential precedence with Docker DesktopLatency
Extra time per
docker pullof a Docker Hub ordhi.ioimage (p50, this branch minusmaster), and the cost of one lookup against the engine:DOCKER_AUTH_CONFIGset for Docker Hubdocker --version(p50, before → after)¹
ghcr.io, which never contacts the engine, differs by +0.58 to +0.79 ms in the same runs.² A lookup makes two requests to the engine. The VMs oversleep: a fake engine that sleeps 10 ms per request takes 44.7 ms per lookup on Linux and 62.0 ms on Windows, against 22.8 ms on macOS.
³ The cost is in the engine, not the CLI. On Linux the engine verifies the binary of every connecting process, and the cost grows with the size of that binary. Signed out costs the same as signed in.
Every case sent the expected credentials: the Docker Desktop session for Docker Hub and
dhi.io,DOCKER_AUTH_CONFIGover Docker Desktop, and the configuration file everywhere else.How this was measured:
masterand this branch were built withCGO_ENABLED=0.127.0.0.1recorded theX-Registry-Authof every pull.HOME, the Docker config, and the Docker Desktop socket path were isolated in a temporary directory.BenchmarkExternalEnginefrom this branch, against the same engine.internal/desktopauthtointernal/secretsengine; the code is otherwise the same.Binary size
dockerbuilt frommasterand from this branch with-tags grpcnotraceandCGO_ENABLED=0; stripped means-ldflags "-s -w". 1 MB = 1,000,000 bytes.connectrpc.com/connectv1.19.1,github.com/docker/secrets-engine/clientv0.2.2,github.com/docker/secrets-engine/xv0.8.5, andgolang.org/x/modv0.41.0. Protobuf andnet/httpwere already linked.internal/secretsengineitself is 0.006 MB. Most of the rest is the secrets engine client wiring RPCs the CLI never calls, andconnect.connect. With prototypes of that client, the increase on linux/arm64 drops to +0.20 MB unstripped and +0.13 MB stripped (−85%).-buildmode=pieon Linux and macOS, which these builds did not.Open items
docker pull).wsarecv: The I/O operation has been abortedand fell back to the CLI's credentials. This was not reproduced in 10 further runs, and nodocker pullfailed.Release notes (optional)