Merge pull request #7339 from crazy-max/auth-refresh-rejected-credentials

authprovider: refresh rejected credentials before retrying tokens

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