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

@@ -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
 	}

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) {