From b45cbb3de5acce1930e565b245a8e8694be9244d Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:41:50 +0100 Subject: [PATCH] authprovider: refresh rejected credentials before retrying tokens Credential helpers can return tokens that expire before the auth cache TTL. Force one credential reload when an authenticated token exchange ends in 401, after the existing POST-to-GET fallback. Retry only when the effective credentials change, without altering the normal cache expiration policy. Inspect the HTTP status before formatting registry errors so structured unauthorized responses can trigger recovery. Preserve the final error format and stop if the replacement credentials are also rejected. Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> --- session/auth/authprovider/authprovider.go | 55 +++- .../auth/authprovider/authprovider_test.go | 275 ++++++++++++++++++ 2 files changed, 320 insertions(+), 10 deletions(-) diff --git a/session/auth/authprovider/authprovider.go b/session/auth/authprovider/authprovider.go index 072c10da3..7be8a8c27 100644 --- a/session/auth/authprovider/authprovider.go +++ b/session/auth/authprovider/authprovider.go @@ -98,12 +98,50 @@ func (ap *authProvider) Register(server *grpc.Server) { auth.RegisterAuthServer(server, ap) } -func (ap *authProvider) FetchToken(ctx context.Context, req *auth.FetchTokenRequest) (rr *auth.FetchTokenResponse, err error) { - ac, err := ap.getAuthConfig(ctx, req.Host, req.Scopes) +func (ap *authProvider) FetchToken(ctx context.Context, req *auth.FetchTokenRequest) (_ *auth.FetchTokenResponse, err error) { + ac, err := ap.getAuthConfig(ctx, req.Host, req.Scopes, ap.expireAc) if err != nil { return nil, err } + // Format registry errors only after deciding whether to refresh. WithDetails + // can replace the HTTP status with a structured registry error. + defer func() { + if toCredentials(*ac).Secret != "" { + err = errors.Wrap(errutil.WithDetails(err), "failed to fetch oauth token") + } else { + err = errors.Wrap(err, "failed to fetch anonymous token") + } + }() + + resp, err := ap.fetchToken(ctx, req, ac) + creds := toCredentials(*ac) + var statusErr remoteserrors.ErrUnexpectedStatus + if !errors.As(err, &statusErr) || statusErr.StatusCode != http.StatusUnauthorized || creds.Secret == "" { + return resp, err + } + + // Helpers can return credentials that expire before the cache TTL. After + // both token request methods fail, reload once without changing the normal + // expiration policy used by other requests. + refreshed, refreshErr := ap.getAuthConfig(ctx, req.Host, req.Scopes, func(time.Time, string) bool { + return true + }) + if refreshErr != nil { + return nil, refreshErr + } + newCreds := toCredentials(*refreshed) + if newCreds.Username == creds.Username && newCreds.Secret == creds.Secret && refreshed.RegistryToken == ac.RegistryToken { + return nil, err + } + ac = refreshed + + return ap.fetchToken(ctx, req, ac) +} + +// fetchToken performs a token exchange with the supplied credentials, including +// the GET fallback for registries that do not support OAuth POST requests. +func (ap *authProvider) fetchToken(ctx context.Context, req *auth.FetchTokenRequest, ac *types.AuthConfig) (rr *auth.FetchTokenResponse, err error) { // check for statically configured bearer token if ac.RegistryToken != "" { return toTokenResponse(ac.RegistryToken, time.Time{}, 0), nil @@ -129,9 +167,6 @@ func (ap *authProvider) FetchToken(ctx context.Context, req *auth.FetchTokenRequ done := func(progresswriter.SubLogger) error { return err } - defer func() { - err = errors.Wrap(errutil.WithDetails(err), "failed to fetch oauth token") - }() ap.mu.Lock() name := fmt.Sprintf("[auth] %v token for %s", strings.Join(trimScopePrefix(req.Scopes), " "), req.Host) if _, ok := ap.loggerCache[name]; !ok { @@ -167,7 +202,7 @@ func (ap *authProvider) FetchToken(ctx context.Context, req *auth.FetchTokenRequ return authutil.FetchToken(ctx, httpClient, nil, to) }) if err != nil { - return nil, errors.Wrap(err, "failed to fetch anonymous token") + return nil, err } return toTokenResponse(resp.Token, resp.IssuedAt, resp.ExpiresInSeconds), nil } @@ -226,7 +261,7 @@ func toCredentials(ac types.AuthConfig) *auth.CredentialsResponse { } func (ap *authProvider) Credentials(ctx context.Context, req *auth.CredentialsRequest) (*auth.CredentialsResponse, error) { - ac, err := ap.getAuthConfig(ctx, req.Host, nil) + ac, err := ap.getAuthConfig(ctx, req.Host, nil, ap.expireAc) if err != nil { return nil, err } @@ -266,14 +301,14 @@ func (ap *authProvider) VerifyTokenAuthority(ctx context.Context, req *auth.Veri return &auth.VerifyTokenAuthorityResponse{Signed: sign.Sign(nil, req.Payload, priv)}, nil } -func (ap *authProvider) getAuthConfig(ctx context.Context, host string, scopes []string) (*types.AuthConfig, error) { +func (ap *authProvider) getAuthConfig(ctx context.Context, host string, scopes []string, expireCheck ExpireCachedAuthCheck) (*types.AuthConfig, error) { ap.mu.Lock() defer ap.mu.Unlock() var ac types.AuthConfig if ap.provider != nil { span, _ := tracing.StartSpan(ctx, fmt.Sprintf("load credentials for %s", host)) - res, err := ap.provider(ctx, host, scopes, ap.expireAc) + res, err := ap.provider(ctx, host, scopes, expireCheck) tracing.FinishWithError(span, err) if err != nil { return nil, err @@ -289,7 +324,7 @@ func (ap *authProvider) getAuthorityKey(ctx context.Context, host string, salt [ return nil, status.Errorf(codes.Unavailable, "client side tokens disabled") } - ac, err := ap.getAuthConfig(ctx, host, nil) + ac, err := ap.getAuthConfig(ctx, host, nil, ap.expireAc) if err != nil { return nil, err } diff --git a/session/auth/authprovider/authprovider_test.go b/session/auth/authprovider/authprovider_test.go index 6b59e1b8d..7b274d0b7 100644 --- a/session/auth/authprovider/authprovider_test.go +++ b/session/auth/authprovider/authprovider_test.go @@ -2,14 +2,17 @@ package authprovider import ( "context" + "errors" "io" "net" "net/http" "net/http/httptest" + "sync" "sync/atomic" "testing" "time" + remoteserrors "github.com/containerd/containerd/v2/core/remotes/errors" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/config/types" "github.com/moby/buildkit/session/auth" @@ -17,6 +20,278 @@ import ( "github.com/stretchr/testify/require" ) +func TestFetchTokenRefreshRejectedCredentials(t *testing.T) { + const host = "registry.example" + expired := types.AuthConfig{Username: "user", Password: "expired"} + fresh := types.AuthConfig{Username: "user", Password: "fresh"} + refreshErr := errors.New("credential helper unavailable") + const registryError = `{"errors":[{"code":"UNAUTHORIZED","message":"authentication failed"}]}` + + for _, tc := range []struct { + name string + initial types.AuthConfig + replacement types.AuthConfig + postStatus int + getStatus int + acceptedSecret string + errorBody string + refreshErr error + wantMethods []string + wantRefreshes int + wantStatus int + wantError string + wantExpires int64 + }{ + { + name: "refresh before cache expiry", + initial: expired, + replacement: fresh, + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodPost}, + wantRefreshes: 1, + }, + { + name: "preserve GET fallback after refresh", + initial: expired, + replacement: fresh, + postStatus: http.StatusMethodNotAllowed, + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + }, + { + name: "successful fallback needs no refresh", + initial: expired, + replacement: fresh, + postStatus: http.StatusUnauthorized, + acceptedSecret: "expired", + wantMethods: []string{http.MethodPost, http.MethodGet}, + }, + { + name: "unchanged credentials are not retried", + initial: expired, + replacement: expired, + wantMethods: []string{http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + wantStatus: http.StatusUnauthorized, + }, + { + name: "replacement rejection stops retrying", + initial: expired, + replacement: fresh, + postStatus: http.StatusUnauthorized, + getStatus: http.StatusUnauthorized, + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + wantStatus: http.StatusUnauthorized, + }, + { + name: "refresh failure is returned", + initial: expired, + replacement: fresh, + refreshErr: refreshErr, + wantMethods: []string{http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + }, + { + name: "forbidden does not refresh credentials", + initial: expired, + replacement: fresh, + postStatus: http.StatusForbidden, + wantMethods: []string{http.MethodPost}, + wantStatus: http.StatusForbidden, + }, + { + name: "anonymous requests do not refresh credentials", + initial: types.AuthConfig{}, + replacement: fresh, + wantMethods: []string{http.MethodGet}, + wantStatus: http.StatusUnauthorized, + }, + { + name: "refresh identity token", + initial: types.AuthConfig{IdentityToken: "expired"}, + replacement: types.AuthConfig{IdentityToken: "fresh"}, + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodPost}, + wantRefreshes: 1, + }, + { + name: "refresh with structured registry error", + initial: expired, + replacement: fresh, + errorBody: registryError, + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodPost}, + wantRefreshes: 1, + }, + { + name: "preserve structured registry error after retry", + initial: expired, + replacement: fresh, + postStatus: http.StatusUnauthorized, + getStatus: http.StatusUnauthorized, + errorBody: registryError, + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + wantError: "failed to fetch oauth token: unauthorized: authentication failed", + }, + { + name: "metadata changes do not retry credentials", + initial: expired, + replacement: types.AuthConfig{Username: "user", Password: "expired", ServerAddress: host}, + wantMethods: []string{http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + wantStatus: http.StatusUnauthorized, + }, + { + name: "forbidden fallback does not refresh credentials", + initial: expired, + replacement: fresh, + getStatus: http.StatusForbidden, + wantMethods: []string{http.MethodPost, http.MethodGet}, + wantStatus: http.StatusForbidden, + }, + { + name: "replacement registry token needs no HTTP exchange", + initial: expired, + replacement: types.AuthConfig{RegistryToken: "registry-token"}, + wantMethods: []string{http.MethodPost, http.MethodGet}, + wantRefreshes: 1, + wantExpires: defaultExpiration, + }, + { + name: "empty replacement uses anonymous error context", + initial: expired, + replacement: types.AuthConfig{}, + errorBody: "authentication failed", + wantMethods: []string{http.MethodPost, http.MethodGet, http.MethodGet}, + wantRefreshes: 1, + wantStatus: http.StatusUnauthorized, + wantError: "failed to fetch anonymous token:", + }, + } { + t.Run(tc.name, func(t *testing.T) { + scopes := []string{"repository:test:pull,push"} + cfg := configfile.New("") + cfg.AuthConfigs[host] = tc.initial + load := LoadAuthConfig(cfg) + refreshes := 0 + var wantScopes []string + p := NewDockerAuthProvider(DockerAuthProviderConfig{ + AuthConfigProvider: func(ctx context.Context, gotHost string, gotScopes []string, check ExpireCachedAuthCheck) (types.AuthConfig, error) { + require.Equal(t, host, gotHost) + require.Equal(t, wantScopes, gotScopes) + if check(time.Now(), host) { + refreshes++ + if tc.refreshErr != nil { + return types.AuthConfig{}, tc.refreshErr + } + } + return load(ctx, gotHost, gotScopes, check) + }, + }).(*authProvider) + // Cache a credential whose server-side lifetime ends before the cache TTL. + _, err := p.Credentials(t.Context(), &auth.CredentialsRequest{Host: host}) + require.NoError(t, err) + cfg.AuthConfigs[host] = tc.replacement + wantScopes = scopes + + acceptedSecret := tc.acceptedSecret + if acceptedSecret == "" { + acceptedSecret = "fresh" + } + var methodsMu sync.Mutex + var methods []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + methodsMu.Lock() + methods = append(methods, r.Method) + methodsMu.Unlock() + if !assert.NoError(t, r.ParseForm()) { + w.WriteHeader(http.StatusBadRequest) + return + } + status := tc.postStatus + secret := r.Form.Get("password") + if r.Form.Get("grant_type") == "refresh_token" { + secret = r.Form.Get("refresh_token") + } + if r.Method == http.MethodGet { + status = tc.getStatus + _, secret, _ = r.BasicAuth() + } + if status == 0 && secret != acceptedSecret { + status = http.StatusUnauthorized + } + if status != 0 { + w.WriteHeader(status) + _, err := io.WriteString(w, tc.errorBody) + assert.NoError(t, err) + return + } + w.Header().Set("Content-Type", "application/json") + _, err := io.WriteString(w, `{"access_token":"registry-token","expires_in":120}`) + assert.NoError(t, err) + })) + defer srv.Close() + res, err := p.FetchToken(t.Context(), &auth.FetchTokenRequest{ + Host: host, + Realm: srv.URL, + Service: host, + Scopes: scopes, + }) + switch { + case tc.refreshErr != nil: + require.ErrorIs(t, err, tc.refreshErr) + case tc.wantStatus != 0: + var statusErr remoteserrors.ErrUnexpectedStatus + require.ErrorAs(t, err, &statusErr) + require.Equal(t, tc.wantStatus, statusErr.StatusCode) + if tc.wantError != "" { + require.ErrorContains(t, err, tc.wantError) + } + case tc.wantError != "": + require.EqualError(t, err, tc.wantError) + default: + require.NoError(t, err) + require.Equal(t, "registry-token", res.Token) + wantExpires := tc.wantExpires + if wantExpires == 0 { + wantExpires = 120 + } + require.Equal(t, wantExpires, res.ExpiresIn) + } + if err != nil { + require.Nil(t, res) + } + if tc.wantRefreshes != 0 && tc.refreshErr == nil { + // A forced refresh must not replace the normal cache policy. + wantScopes = nil + creds, err := p.Credentials(t.Context(), &auth.CredentialsRequest{Host: host}) + require.NoError(t, err) + require.Equal(t, toCredentials(tc.replacement), creds) + } + require.Equal(t, tc.wantRefreshes, refreshes) + methodsMu.Lock() + require.Equal(t, tc.wantMethods, methods) + methodsMu.Unlock() + }) + } +} + +func TestFetchTokenProviderError(t *testing.T) { + loadErr := errors.New("credential helper unavailable") + calls := 0 + p := NewDockerAuthProvider(DockerAuthProviderConfig{ + AuthConfigProvider: func(context.Context, string, []string, ExpireCachedAuthCheck) (types.AuthConfig, error) { + calls++ + return types.AuthConfig{}, loadErr + }, + }).(*authProvider) + res, err := p.FetchToken(t.Context(), &auth.FetchTokenRequest{Host: "registry.example"}) + // Loading credentials fails before an exchange, so preserve the provider's + // error without token-exchange context or a forced-refresh attempt. + require.Same(t, loadErr, err) + require.Nil(t, res) + require.Equal(t, 1, calls) +} + func TestFetchTokenRetry(t *testing.T) { var attempts atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {