feat(auth): mint Docker tokens from the stored access token - #3935
feat(auth): mint Docker tokens from the stored access token#3935dgageot wants to merge 1 commit into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
🟢 No issues found — LGTM! View logs.
aheritier
left a comment
There was a problem hiding this comment.
Review of #3935 — head 0968511
CI on 0968511 is green (build-and-test, windows-tests, lint, license-check, build-image ×2; the three push-only jobs skipped). mergeable_state: blocked is REVIEW_REQUIRED, not a conflict.
This is a well-built package: singleflighted exchange, redirects refused, endpoint pinned to docker.com, passwords never exchanged, a 0600 token file in a 0700 directory, clock-skew learning, and genuinely thorough tests (go test -race ./pkg/hubauth/... ./pkg/desktop/... ./pkg/httpclient/... ./pkg/model/provider/base/... ./pkg/modelsgateway/... ./cmd/root/... passes locally). Two behavioural holes in pkg/desktop's new cache need fixing first.
[blocking] A refused token is served again — and then pinned in the cache for its whole life
GetTokenWithSource protects the direct fetch with wasRejected (pkg/desktop/login.go:78) but not the forced-refresh result: runTokenRefresh accepts any non-expiring token Desktop hands back (login.go:304) and login.go:85 passes it straight to remember. cached() (login.go:118-126) doesn't consult cache.rejected either, so the refused token is then served from memory without even an attempt to mint.
Here is the failing case — the token Docker refused comes back one log line later, same fingerprint:
WARN Docker Desktop served a token Docker refused fingerprint=081e6ec5
WARN Forcing a Docker Desktop token refresh
INFO Recovered a fresh token from Docker Desktop fingerprint=081e6ec5
served token == refused? true (source "docker desktop")
second call served refused token? true, mint attempted 0 time(s)
// pkg/desktop
func TestRejectedTokenIsNeverServedAgain(t *testing.T) {
refused := makeToken(t, time.Now().Add(time.Hour))
backend := &fakeBackend{token: refused, loggedIn: true}
installFakeBackend(t, backend) // minting unavailable, as on a password-only login
require.Equal(t, refused, GetToken(t.Context()))
InvalidateToken(refused) // the gateway answered 401
token, _ := GetTokenWithSource(t.Context())
assert.NotEqual(t, refused, token, "a refused token must never be served again")
}This is reached whenever minting is unavailable at that moment: DOCKER_AGENT_NO_TOKEN_EXCHANGE=1 (deterministic), a password-only docker login, an unreachable Hub, or the 5-minute rejectedCooldown after Hub itself refused the PAT. It also makes recovery worse than on main: once remembered, cached() short-circuits, so Desktop is never asked again for the remainder of that token's life (~15 min) and a fresh token Desktop does eventually produce is ignored — where today every call re-fetched.
Smallest fix: refuse to remember a rejected token (if wasRejected(token) { return "", SourceNone } at the top of remember) and skip rejected tokens in runTokenRefresh's accept condition.
[should-fix] docker logout / an account switch goes unnoticed while a minted token is cached
hubauth re-checks credentials every 30s on purpose (pkg/hubauth/token.go:35-39, token.go:88 — "so a docker logout or an account switch is picked up quickly"), but pkg/desktop's cache sits in front of it and its only exit condition is expiry (login.go:122). While it holds a minted token, hubauth.Token is never called, so the credential store is never consulted.
Here is the failing case:
minted := makeToken(t, time.Now().Add(4*time.Hour))
installFakeBackend(t, &fakeBackend{}) // Desktop signed out / absent
mintToken = func(context.Context) (string, error) { return minted, nil }
require.Equal(t, minted, GetToken(t.Context()))
// docker logout
mintToken = func(context.Context) (string, error) { return "", errors.New("no access token") }
assert.Empty(t, GetToken(t.Context()), "a logout must stop the previous account's token from being served")Error: Should be empty, but was <the pre-logout token>
still serving the pre-logout token? true; credential store consulted 0 time(s)
After an account switch, requests keep going out as the previous account for the remainder of the minted token's lifetime. Either give the cached minted token its own re-check window, or don't cache minted tokens here at all and let hubauth.Token — already in-memory and singleflighted — be the cache, since it does the credential check itself.
[should-fix] New user-facing env vars and changed behaviour aren't documented
The description presents DOCKER_AGENT_NO_TOKEN_EXCHANGE as the way to opt out, but nothing under docs/ mentions it (nor DOCKER_AGENT_HUB_LOGIN_URL); the runtime-override table at docs/configuration/overview/index.md:159-164 is where readers look for it. Two more places now contradict the code:
docs/features/cli/index.md:676—debug authis documented as printing "Docker Desktop authentication info"; it now also reports minted tokens and prints a newSourcefield.docs/guides/secrets/index.md:185— presents Docker Desktop as how signed-in users are authenticated, with no mention that the stored access token is exchanged with Hub or that the minted bearer token is cached on disk.
Since this reads a credential from the user's store and sends it to Hub, the opt-out in particular deserves a documented home.
[optional] Shared cache and clock skew
pkg/hubauth/cache.go:74writes the bearer token to disk in cleartext, while MCP OAuth tokens go through the keyring first and fall back to a 0600 file only when it is unavailable (pkg/tools/mcp/keyringstore/tokenstore.go:154). The 0700-dir reasoning is sound; reusing the keyring path would just be stronger. Relatedly,load(cache.go:51-56) trusts the fingerprint and expiry alone and skips the issuer/audiencevalidate()a fresh exchange gets — free defence in depth.learnClockSkew(resp.Header)runs before the status check (pkg/hubauth/exchange.go:164), so an error page from a TLS-terminating corporate proxy can shiftnow()process-wide — which also movesExpiringdecisions for Desktop's tokens. Learning it only from a 200 would keep the source narrow.
Questions
- Minted tokens carry
aud: https://hub.docker.com, andDOCKER_TOKENis consumed outside the models gateway too: injected into sandboxes (cmd/root/sandbox.go:232) and eval subprocesses (pkg/evaluation/eval.go:420), and forwarded when pulling agents from*.docker.com(pkg/config/sources.go:454). Have those paths been exercised with a minted token? - Minted Hub tokens outlive Desktop's 15-minute one. Is that longer TTL acceptable for the copy handed into a sandbox container?
|
Thanks — both behavioural findings reproduced exactly as written, log lines and all. All four are addressed in [blocking] A refused token is served againFixed, and wider than the two spots you pointed at. Two things I changed beyond the suggestion:
[should-fix]
|
Docker Desktop hands out a 15-minute token it never renews, so a stuck refresher leaves every caller with a dead one. The access token `docker login` stores is long-lived, and Docker Hub exchanges it for a fresh one with no user interaction. pkg/hubauth owns that exchange: a token renewed ahead of its expiry, credential re-checks so a logout is noticed, a cache shared with sibling processes, retries, issuer and audience validation, and clock-skew correction learned from Hub. Desktop's token still comes first while it is usable; one the gateway refuses is never served again and the request is replayed once with a fresh one. `docker agent debug auth` reports which token is in use and where it came from, and DOCKER_AGENT_NO_TOKEN_EXCHANGE opts out entirely. Signed-off-by: David Gageot <david.gageot@docker.com>
aheritier
left a comment
There was a problem hiding this comment.
LGTM. All four findings are addressed — I checked each against the code, and two of the fixes are better than what I suggested: folding the tombstone check into remember() under one lock closes the check-then-store window, and making cache.rejected a set closes a hole I missed (Desktop regressing to an earlier refused token). Your learnClockSkew placement is also right: after the status check but still before validate(), which needs the skew.
CI is green; go test -race -count=2 on pkg/hubauth, pkg/desktop and pkg/httpclient is clean here too.
On the sandbox TTL question: it's moot — LoginKit declares DOCKER_TOKEN as a proxyManaged sentinel, so the real token never enters the sandbox. Only pkg/evaluation/eval.go:420 copies it into a container env, which is fine for a local eval run.
One non-blocking wording nit inline.
| @@ -40,6 +42,22 @@ func GatewayAuthToken(ctx context.Context, env environment.Provider, gateway str | |||
| return token, nil | |||
There was a problem hiding this comment.
Three messages still point at Docker Desktop as the only remedy, on the path where docker login is now the fix:
base.go:11—NoDesktopTokenErrorMessage, returned on line 40 just abovegateway.go:26—"sorry, you first need to sign in Docker Desktop to use the Docker AI Gateway"config.go:177— same string
They're reachable with a valid docker login in the store (DOCKER_AGENT_NO_TOKEN_EXCHANGE=1, or a password-only credential), so they send the user to the wrong remedy. You already fixed the equivalent wording in doctor.go and debug auth. This PR or a follow-up, either is fine.
Docker Desktop's backend API only hands out its own access token — valid
for 15 minutes — and never the refresh token behind it, so an expired JWT
cannot be renewed: a stuck refresher on Desktop's side leaves every
caller with the same dead token. The access token
docker login(andDesktop's own sign-in) leaves in the credential store is long-lived, and
Docker Hub exchanges it for a fresh token with no user interaction.
This package owns that exchange: an in-memory token renewed ahead of its
expiry, credential re-checks so a logout or an account switch is noticed,
a shared cache so sibling processes don't each mint their own, retries
for transient failures (honouring Retry-After), a long back-off when the
token is refused, issuer and audience validation, and clock-skew
correction learned from Hub's Date header so expiry decisions follow the
issuer's clock rather than a drifting local one.
The access token itself never leaves the process except in the exchange
request: the endpoint is pinned to a Docker host, redirects are not
followed, and account passwords are never sent. Set
DOCKER_AGENT_NO_TOKEN_EXCHANGE to opt out entirely.