Merge pull request containerd#9236 from dmcgowan/backport-1.6-fix-bas…

…ic-auth-error

remotes/docker/authorizer.go

@@ -194,15 +194,15 @@ func (a *dockerAuthorizer) AddResponses(ctx context.Context, responses []*http.R
 				return err
 			}
 
-			if username != "" && secret != "" {
-				common := auth.TokenOptions{
-					Username: username,
-					Secret:   secret,
-				}
-
-				a.handlers[host] = newAuthHandler(a.client, a.header, c.Scheme, common)
-				return nil
+			if username == "" || secret == "" {
+				return fmt.Errorf("%w: no basic auth credentials", ErrInvalidAuthorization)
 			}
+
+			a.handlers[host] = newAuthHandler(a.client, a.header, c.Scheme, auth.TokenOptions{
+				Username: username,
+				Secret:   secret,
+			})
+			return nil
 		}
 	}
 	return fmt.Errorf("failed to find supported auth scheme: %w", errdefs.ErrNotImplemented)

remotes/docker/resolver.go

@@ -34,6 +34,7 @@ import (
 	"github.com/containerd/containerd/reference"
 	"github.com/containerd/containerd/remotes"
 	"github.com/containerd/containerd/remotes/docker/schema1"
+	remoteerrors "github.com/containerd/containerd/remotes/errors"
 	"github.com/containerd/containerd/version"
 	digest "github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -308,11 +309,11 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 				if resp.StatusCode > 399 {
 					// Set firstErr when encountering the first non-404 status code.
 					if firstErr == nil {
-						firstErr = fmt.Errorf("pulling from host %s failed with status code %v: %v", host.Host, u, resp.Status)
+						firstErr = remoteerrors.NewUnexpectedStatusErr(resp)
 					}
 					continue // try another host
 				}
-				return "", ocispec.Descriptor{}, fmt.Errorf("pulling from host %s failed with unexpected status code %v: %v", host.Host, u, resp.Status)
+				return "", ocispec.Descriptor{}, remoteerrors.NewUnexpectedStatusErr(resp)
 			}
 			size := resp.ContentLength
 			contentType := getManifestMediaType(resp)

remotes/docker/resolver_test.go

@@ -34,6 +34,7 @@ import (
 
 	"github.com/containerd/containerd/remotes"
 	"github.com/containerd/containerd/remotes/docker/auth"
+	remoteerrors "github.com/containerd/containerd/remotes/errors"
 	digest "github.com/opencontainers/go-digest"
 	specs "github.com/opencontainers/image-spec/specs-go"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -210,6 +211,14 @@ func TestPostBasicAuthTokenResolver(t *testing.T) {
 	runBasicTest(t, "testname", withTokenServer(th, creds))
 }
 
+func TestBasicAuthResolver(t *testing.T) {
+	creds := func(string) (string, string, error) {
+		return "totallyvaliduser", "totallyvalidpassword", nil
+	}
+
+	runBasicTest(t, "testname", withBasicAuthServer(creds))
+}
+
 func TestBadTokenResolver(t *testing.T) {
 	th := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
 		if r.Method != http.MethodPost {
@@ -231,7 +240,7 @@ func TestBadTokenResolver(t *testing.T) {
 	defer close()
 
 	resolver := NewResolver(ro)
-	image := fmt.Sprintf("%s/doesntmatter:sometatg", base)
+	image := fmt.Sprintf("%s/doesntmatter:sometag", base)
 
 	_, _, err := resolver.Resolve(ctx, image)
 	if err == nil {
@@ -242,6 +251,59 @@ func TestBadTokenResolver(t *testing.T) {
 	}
 }
 
+func TestMissingBasicAuthResolver(t *testing.T) {
+	creds := func(string) (string, string, error) {
+		return "", "", nil
+	}
+
+	ctx := context.Background()
+	h := newContent(ocispec.MediaTypeImageManifest, []byte("not anything parse-able"))
+
+	base, ro, close := withBasicAuthServer(creds)(logHandler{t, h})
+	defer close()
+
+	resolver := NewResolver(ro)
+	image := fmt.Sprintf("%s/doesntmatter:sometag", base)
+
+	_, _, err := resolver.Resolve(ctx, image)
+	if err == nil {
+		t.Fatal("Expected error getting token with inssufficient scope")
+	}
+	if !errors.Is(err, ErrInvalidAuthorization) {
+		t.Fatal(err)
+	}
+	if !strings.Contains(err.Error(), "no basic auth credentials") {
+		t.Fatalf("expected \"no basic auth credentials\" message, got %s", err.Error())
+	}
+}
+
+func TestWrongBasicAuthResolver(t *testing.T) {
+	creds := func(string) (string, string, error) {
+		return "totallyvaliduser", "definitelythewrongpassword", nil
+	}
+
+	ctx := context.Background()
+	h := newContent(ocispec.MediaTypeImageManifest, []byte("not anything parse-able"))
+
+	base, ro, close := withBasicAuthServer(creds)(logHandler{t, h})
+	defer close()
+
+	resolver := NewResolver(ro)
+	image := fmt.Sprintf("%s/doesntmatter:sometag", base)
+
+	_, _, err := resolver.Resolve(ctx, image)
+	if err == nil {
+		t.Fatal("Expected error getting token with inssufficient scope")
+	}
+	var rerr remoteerrors.ErrUnexpectedStatus
+	if !errors.As(err, &rerr) {
+		t.Fatal(err)
+	}
+	if rerr.StatusCode != 403 {
+		t.Fatalf("expected 403 status code, got %d", rerr.StatusCode)
+	}
+}
+
 func TestHostFailureFallbackResolver(t *testing.T) {
 	sf := func(h http.Handler) (string, ResolverOptions, func()) {
 		s := httptest.NewServer(h)
@@ -543,6 +605,37 @@ func withTokenServer(th http.Handler, creds func(string) (string, string, error)
 	}
 }
 
+func withBasicAuthServer(creds func(string) (string, string, error)) func(h http.Handler) (string, ResolverOptions, func()) {
+	return func(h http.Handler) (string, ResolverOptions, func()) {
+		// Wrap with basic auth
+		wrapped := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+			user, password, ok := r.BasicAuth()
+			if ok {
+				if user != "totallyvaliduser" || password != "totallyvalidpassword" {
+					rw.WriteHeader(http.StatusForbidden)
+					rw.Write([]byte(`{"errors":[{"code":"DENIED"}]}`))
+					return
+				}
+			} else {
+				authHeader := "Basic realm=\"testserver\""
+				rw.Header().Set("WWW-Authenticate", authHeader)
+				rw.WriteHeader(http.StatusUnauthorized)
+				return
+			}
+			h.ServeHTTP(rw, r)
+		})
+
+		base, options, close := tlsServer(wrapped)
+		options.Hosts = ConfigureDefaultRegistries(
+			WithClient(options.Client),
+			WithAuthorizer(NewDockerAuthorizer(
+				WithAuthCreds(creds),
+			)),
+		)
+		return base, options, close
+	}
+}
+
 func tlsServer(h http.Handler) (string, ResolverOptions, func()) {
 	s := httptest.NewUnstartedServer(h)
 	s.StartTLS()
@@ -558,6 +651,7 @@ func tlsServer(h http.Handler) (string, ResolverOptions, func()) {
 			},
 		},
 	}
+
 	options := ResolverOptions{
 		Hosts: ConfigureDefaultRegistries(WithClient(client)),
 		// Set deprecated field for tests to use for configuration

remotes/errors/errors.go

@@ -33,7 +33,7 @@ type ErrUnexpectedStatus struct {
 }
 
 func (e ErrUnexpectedStatus) Error() string {
-	return fmt.Sprintf("unexpected status: %s", e.Status)
+	return fmt.Sprintf("unexpected status from %s request to %s: %s", e.RequestMethod, e.RequestURL, e.Status)
 }
 
 // NewUnexpectedStatusErr creates an ErrUnexpectedStatus from HTTP response