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