From b8307b98d9d105331385c32404b4388c05c86147 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 22 Oct 2026 17:02:55 +0200 Subject: [PATCH] fix(desktop): preserve secrets-engine auth fallback behavior SecretsEngineError no longer blocks on an in-flight lookup, so callers get an immediate "slow" error instead of stalling; the stalled engine still keeps serving the legacy account info in the meantime. Expiry checks for live and cached sessions now go through the shared skew-aware ExpiringAt helper instead of duplicating the leeway math. Staging host detection for DNS hosts is now case-insensitive. Adds coverage against a real secrets-engine SDK client for missing socket, 404, and 501 responses to confirm the legacy fallback kicks in. Assisted-By: docker-agent --- pkg/desktop/login.go | 2 +- pkg/desktop/secrets_engine.go | 10 ++- pkg/desktop/secrets_engine_compat_test.go | 89 +++++++++++++++++++ pkg/desktop/secrets_engine_test.go | 31 ++++++- pkg/hubauth/exchange.go | 3 +- pkg/hubauth/exchange_test.go | 5 ++ pkg/hubauth/expiry.go | 8 +- pkg/hubauth/expiry_test.go | 103 ++++++++++++++++++++++ 8 files changed, 243 insertions(+), 8 deletions(-) create mode 100644 pkg/desktop/secrets_engine_compat_test.go create mode 100644 pkg/hubauth/expiry_test.go diff --git a/pkg/desktop/login.go b/pkg/desktop/login.go index 3d018cbc77..4e051d63fd 100644 --- a/pkg/desktop/login.go +++ b/pkg/desktop/login.go @@ -166,7 +166,7 @@ func cachedExpiring() bool { if cache.expiresAt.IsZero() { return hubauth.Expiring(cache.token) } - return !time.Now().Add(hubauth.ExpiryLeeway).Before(cache.expiresAt) + return hubauth.ExpiringAt(cache.expiresAt) } // usable reports whether a token is worth handing to callers: one that dies diff --git a/pkg/desktop/secrets_engine.go b/pkg/desktop/secrets_engine.go index d290b659d7..1cbc786b56 100644 --- a/pkg/desktop/secrets_engine.go +++ b/pkg/desktop/secrets_engine.go @@ -83,7 +83,7 @@ func secretsEngineToken(ctx context.Context) (string, DockerHubInfo, bool) { slog.WarnContext(ctx, "The secrets engine served a session without an account", "fingerprint", tokenFingerprint(token)) return "", DockerHubInfo{}, false - case session.ExpiresWithin(hubauth.ExpiryLeeway): + case claims.ExpiresAt == nil || hubauth.ExpiringAt(claims.ExpiresAt.Time): slog.DebugContext(ctx, "The secrets engine served a session that expired, is about to, or has no expiry", "fingerprint", tokenFingerprint(token), "expires_at", sessionExpiry(session)) @@ -108,7 +108,7 @@ func sessionExpiry(session dockerhub.UserSession) string { return "none" } -// SecretsEngineError returns why the last secrets engine lookup failed, or nil, once any running lookup ends. +// SecretsEngineError returns the last lookup error, or a timeout error while a lookup is still running. func SecretsEngineError() error { return defaultSecretsEngine().lastError() } @@ -191,11 +191,13 @@ func (e *secretsEngine) wait() { } } -// lastError returns why the last lookup failed, or nil, once any running lookup ends. +// lastError reports failures without waiting for a lookup the caller already stopped waiting for. func (e *secretsEngine) lastError() error { - e.wait() e.mu.Lock() defer e.mu.Unlock() + if e.inflight != nil { + return errSecretsEngineSlow + } return e.lastErr } diff --git a/pkg/desktop/secrets_engine_compat_test.go b/pkg/desktop/secrets_engine_compat_test.go new file mode 100644 index 0000000000..f40b11ccf0 --- /dev/null +++ b/pkg/desktop/secrets_engine_compat_test.go @@ -0,0 +1,89 @@ +//go:build darwin || linux || windows + +package desktop + +import ( + "context" + "net" + "net/http" + "os" + "path/filepath" + "testing" + "time" + + secretsengine "github.com/docker/secrets-engine/client" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSecretsEngineLegacyFallback(t *testing.T) { + dir, err := os.MkdirTemp("", "se") //nolint:forbidigo,usetesting // t.TempDir embeds the test name and can exceed the Unix socket path limit. + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + + for _, tt := range []struct { + name string + status int + }{ + {name: "missing"}, + {name: "404", status: http.StatusNotFound}, + {name: "501", status: http.StatusNotImplemented}, + } { + t.Run(tt.name, func(t *testing.T) { + for _, source := range []Source{SourceDesktop, SourceMinted} { + t.Run(string(source), func(t *testing.T) { + fromLegacy := makeToken(t, time.Now().Add(time.Hour)) + account := DockerHubInfo{Username: "legacy-user", Email: "legacy@example.com"} + backend := &fakeBackend{info: &account} + if source == SourceDesktop { + backend.token = fromLegacy + } + installFakeBackend(t, backend) + if source == SourceMinted { + mintToken = func(context.Context) (string, error) { return fromLegacy, nil } + } + + opts := []secretsengine.Option{secretsengine.WithSocketPath(filepath.Join(dir, "engine.sock"))} + if tt.status != 0 { + ln := newMemListener() + server := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "unsupported API", tt.status) + })} + go func() { _ = server.Serve(ln) }() + t.Cleanup(func() { + _ = server.Close() + _ = ln.Close() + }) + opts = []secretsengine.Option{secretsengine.WithDialContext(func(ctx context.Context, _, _ string) (net.Conn, error) { + return ln.dial(ctx) + })} + } + client, err := secretsengine.New(opts...) + require.NoError(t, err) + engine := &secretsEngine{client: client.HubAuth()} + old := defaultSecretsEngine + defaultSecretsEngine = func() *secretsEngine { return engine } + t.Cleanup(func() { + engine.wait() + defaultSecretsEngine = old + }) + + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + assert.Equal(t, account, GetUserInfo(ctx)) + token, gotSource := GetTokenWithSource(ctx) + assert.Equal(t, fromLegacy, token) + assert.Equal(t, source, gotSource) + require.NoError(t, ctx.Err()) + require.Error(t, SecretsEngineError()) + if tt.status == 0 { + require.ErrorIs(t, SecretsEngineError(), secretsengine.ErrSecretsEngineNotRunning) + } + _, err = engine.fetch(ctx) + require.ErrorIs(t, err, errSecretsEngineCoolingDown) + assert.Zero(t, backend.refreshes()) + }) + } + }) + } +} diff --git a/pkg/desktop/secrets_engine_test.go b/pkg/desktop/secrets_engine_test.go index 730791b962..0bcb49a05b 100644 --- a/pkg/desktop/secrets_engine_test.go +++ b/pkg/desktop/secrets_engine_test.go @@ -451,7 +451,7 @@ func TestSecretsEngineLastError(t *testing.T) { }) }) - t.Run("a slow lookup reports the error it ends with", func(t *testing.T) { + t.Run("a slow lookup is reported without waiting for it", func(t *testing.T) { t.Parallel() synctest.Test(t, func(t *testing.T) { engine := newTestEngine(t, &fakeEngine{block: true}) @@ -461,11 +461,40 @@ func TestSecretsEngineLastError(t *testing.T) { _, err := engine.fetch(ctx) require.ErrorIs(t, err, errSecretsEngineSlow) + start := time.Now() + require.ErrorIs(t, engine.lastError(), errSecretsEngineSlow) + assert.Zero(t, time.Since(start)) + require.NoError(t, ctx.Err()) + + engine.wait() require.ErrorIs(t, engine.lastError(), context.DeadlineExceeded) }) }) } +func TestSecretsEngineDiagnosticsPreserveLegacyAccount(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + account := DockerHubInfo{Username: "legacy-user", Email: "legacy@example.com"} + fromDesktop := makeToken(t, time.Now().Add(time.Hour)) + installFakeBackend(t, &fakeBackend{token: fromDesktop, info: &account}) + installFakeEngine(t, &fakeEngine{block: true}) + + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + start := time.Now() + + token, source := GetTokenWithSource(ctx) + require.Equal(t, fromDesktop, token) + require.Equal(t, SourceDesktop, source) + require.Equal(t, time.Second, time.Since(start)) + + require.ErrorIs(t, SecretsEngineError(), errSecretsEngineSlow) + require.NoError(t, ctx.Err()) + assert.Equal(t, account, GetUserInfo(ctx)) + assert.Equal(t, time.Second, time.Since(start)) + }) +} + func newTestEngine(t *testing.T, fake *fakeEngine) *secretsEngine { t.Helper() diff --git a/pkg/hubauth/exchange.go b/pkg/hubauth/exchange.go index f9a60b5225..67dc11f044 100644 --- a/pkg/hubauth/exchange.go +++ b/pkg/hubauth/exchange.go @@ -85,6 +85,7 @@ func resolveLoginEndpoint() string { } func isDockerHost(host string) bool { + host = strings.ToLower(host) return host == "docker.com" || strings.HasSuffix(host, ".docker.com") } @@ -99,7 +100,7 @@ func isStagingEndpoint(endpoint string) bool { if err != nil || !isDockerHost(u.Hostname()) { return false } - label, _, _ := strings.Cut(u.Hostname(), ".") + label, _, _ := strings.Cut(strings.ToLower(u.Hostname()), ".") return strings.HasSuffix(label, "-stage") } diff --git a/pkg/hubauth/exchange_test.go b/pkg/hubauth/exchange_test.go index 4f2624f6f8..042b99fbe4 100644 --- a/pkg/hubauth/exchange_test.go +++ b/pkg/hubauth/exchange_test.go @@ -176,6 +176,7 @@ func TestLoginEndpointOverride(t *testing.T) { want string }{ {name: "unset", url: "", want: defaultLoginEndpoint}, + {name: "mixed-case docker host", url: "https://HUB-STAGE.DOCKER.COM/v2/users/login", want: "https://HUB-STAGE.DOCKER.COM/v2/users/login"}, {name: "docker host", url: "https://hub-stage.docker.com/v2/users/login", want: "https://hub-stage.docker.com/v2/users/login"}, {name: "other host", url: "https://evil.example.com/login", want: defaultLoginEndpoint}, {name: "plain HTTP", url: "http://hub.docker.com/v2/users/login", want: defaultLoginEndpoint}, @@ -197,6 +198,10 @@ func TestIsStagingEndpoint(t *testing.T) { {endpoint: defaultLoginEndpoint, want: false}, {endpoint: "https://hub-stage.docker.com/v2/users/login", want: true}, {endpoint: "https://login-stage.docker.com/v2/users/login", want: true}, + {endpoint: "https://HUB-STAGE.docker.com/v2/users/login", want: true}, + {endpoint: "https://HuB-StAgE.DoCkEr.CoM/v2/users/login", want: true}, + {endpoint: "https://HUB.DOCKER.COM/v2/users/login", want: false}, + {endpoint: "https://HUB-STAGE.DOCKER.COM.evil.example/login", want: false}, {endpoint: "https://hub.docker.com/hub-stage/v2/users/login", want: false}, {endpoint: "https://hub-stage.example.com/v2/users/login", want: false}, {endpoint: "https://hub-stage.docker.com.example.com/login", want: false}, diff --git a/pkg/hubauth/expiry.go b/pkg/hubauth/expiry.go index 55d00d5583..e900bd3884 100644 --- a/pkg/hubauth/expiry.go +++ b/pkg/hubauth/expiry.go @@ -33,7 +33,13 @@ func Expiring(token string) bool { if !ok { return false } - return exp.Before(now().Add(ExpiryLeeway)) + return ExpiringAt(exp) +} + +// ExpiringAt reports whether expiresAt is zero, has passed, or is less than +// [ExpiryLeeway] away, using the issuer's clock. +func ExpiringAt(expiresAt time.Time) bool { + return expiresAt.IsZero() || expiresAt.Before(now().Add(ExpiryLeeway)) } // renewAt returns the time from which token must be replaced. diff --git a/pkg/hubauth/expiry_test.go b/pkg/hubauth/expiry_test.go new file mode 100644 index 0000000000..c281be5956 --- /dev/null +++ b/pkg/hubauth/expiry_test.go @@ -0,0 +1,103 @@ +package hubauth + +import ( + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/assert" +) + +func TestExpiringAt(t *testing.T) { + previousSkew := clockSkew.Swap(0) + t.Cleanup(func() { clockSkew.Store(previousSkew) }) + + for _, tc := range []struct { + name string + expiresAt time.Time + want bool + }{ + {name: "zero", want: true}, + {name: "expired", expiresAt: time.Now().Add(-time.Minute), want: true}, + {name: "near expiry", expiresAt: time.Now().Add(ExpiryLeeway / 2), want: true}, + {name: "beyond leeway", expiresAt: time.Now().Add(2 * ExpiryLeeway)}, + {name: "future", expiresAt: time.Now().Add(time.Hour)}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, ExpiringAt(tc.expiresAt)) + assert.Equal(t, tc.want, Expiring(makeToken(t, tc.expiresAt))) + }) + } +} + +func TestExpiringAtClockSkew(t *testing.T) { + previousSkew := clockSkew.Load() + t.Cleanup(func() { clockSkew.Store(previousSkew) }) + + for _, tc := range []struct { + name string + expiresIn time.Duration + initialSkew time.Duration + correctedSkew time.Duration + wantInitial bool + wantCorrected bool + }{ + { + name: "local clock ahead", + expiresIn: -50 * time.Minute, + correctedSkew: -time.Hour, + wantInitial: true, + }, + { + name: "local clock behind", + expiresIn: 10 * time.Minute, + correctedSkew: time.Hour, + wantCorrected: true, + }, + { + name: "previously accepted expiry after correction changes", + expiresIn: -50 * time.Minute, + initialSkew: -time.Hour, + wantCorrected: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + expiresAt := time.Now().Add(tc.expiresIn) + token := makeToken(t, expiresAt) + + clockSkew.Store(int64(tc.initialSkew)) + assert.Equal(t, tc.wantInitial, ExpiringAt(expiresAt)) + assert.Equal(t, tc.wantInitial, Expiring(token)) + + clockSkew.Store(int64(tc.correctedSkew)) + assert.Equal(t, tc.wantCorrected, ExpiringAt(expiresAt)) + assert.Equal(t, tc.wantCorrected, Expiring(token)) + }) + } +} + +func TestExpiringUnknownExpiry(t *testing.T) { + for _, tc := range []struct { + name string + token string + }{ + {name: "empty"}, + {name: "unparseable", token: "not-a-jwt"}, + { + name: "missing exp", + token: makeToken(t, time.Now(), func(claims jwt.MapClaims) { + delete(claims, "exp") + }), + }, + { + name: "invalid exp", + token: makeToken(t, time.Now(), func(claims jwt.MapClaims) { + claims["exp"] = "invalid" + }), + }, + } { + t.Run(tc.name, func(t *testing.T) { + assert.False(t, Expiring(tc.token)) + }) + } +}