Fix bugs when pushing image(with index) and CNAB

1. As "List" method of artifact DAO doesn't return the artifacts that referenced by other and without tag, so we introduce a new method "GetByDigest" to check the existence of artifact
2. The "Www-Authenticate" header is needed to be returned when the request is unauthorized. This is required in the OCI distribution spec and is needed when pushing CNAB

Signed-off-by: Wenkai Yin <yinw@vmware.com>

src/api/artifact/abstractor/resolver/image/index.go

@@ -17,14 +17,12 @@ package image
 import (
 	"context"
 	"encoding/json"
-	"fmt"
 	"github.com/docker/distribution/manifest/manifestlist"
 	"github.com/goharbor/harbor/src/api/artifact/abstractor/resolver"
 	"github.com/goharbor/harbor/src/api/artifact/descriptor"
 	"github.com/goharbor/harbor/src/common/utils/log"
 	ierror "github.com/goharbor/harbor/src/internal/error"
 	"github.com/goharbor/harbor/src/pkg/artifact"
-	"github.com/goharbor/harbor/src/pkg/q"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
@@ -59,22 +57,13 @@ func (i *indexResolver) ResolveMetadata(ctx context.Context, manifest []byte, ar
 	// populate the referenced artifacts
 	for _, mani := range index.Manifests {
 		digest := mani.Digest.String()
-		_, arts, err := i.artMgr.List(ctx, &q.Query{
+		// make sure the child artifact exist
-			Keywords: map[string]interface{}{
+		ar, err := i.artMgr.GetByDigest(ctx, art.RepositoryID, digest)
-				"RepositoryID": art.RepositoryID,
-				"Digest":       digest,
-			},
-		})
 		if err != nil {
 			return err
 		}
-		// make sure the child artifact exist
-		if len(arts) == 0 {
-			return fmt.Errorf("the referenced artifact with digest %s not found under repository %d",
-				digest, art.RepositoryID)
-		}
 		art.References = append(art.References, &artifact.Reference{
-			ChildID:  arts[0].ID,
+			ChildID:  ar.ID,
 			Platform: mani.Platform,
 		})
 	}

src/api/artifact/abstractor/resolver/image/index_test.go

@@ -120,15 +120,13 @@ func (i *indexResolverTestSuite) TestResolveMetadata() {
   "schemaVersion": 2
 }`
 	art := &artifact.Artifact{}
-	i.artMgr.On("List").Return(1, []*artifact.Artifact{
+	i.artMgr.On("GetByDigest").Return(&artifact.Artifact{
-		{
+		ID: 1,
-			ID: 1,
-		},
 	}, nil)
 	err := i.resolver.ResolveMetadata(nil, []byte(manifest), art)
 	i.Require().Nil(err)
 	i.artMgr.AssertExpectations(i.T())
-	i.Assert().Len(art.References, 8)
+	i.Require().Len(art.References, 8)
 	i.Assert().Equal(int64(1), art.References[0].ChildID)
 	i.Assert().Equal("amd64", art.References[0].Platform.Architecture)
 }

src/api/artifact/controller.go

@@ -119,19 +119,15 @@ func (c *controller) Ensure(ctx context.Context, repositoryID int64, digest stri
 
 // ensure the artifact exists under the repository, create it if doesn't exist.
 func (c *controller) ensureArtifact(ctx context.Context, repositoryID int64, digest string) (bool, int64, error) {
-	query := &q.Query{
+	art, err := c.artMgr.GetByDigest(ctx, repositoryID, digest)
-		Keywords: map[string]interface{}{
-			"repository_id": repositoryID,
-			"digest":        digest,
-		},
-	}
-	_, artifacts, err := c.artMgr.List(ctx, query)
-	if err != nil {
-		return false, 0, err
-	}
 	// the artifact already exists under the repository, return directly
-	if len(artifacts) > 0 {
+	if err == nil {
-		return false, artifacts[0].ID, nil
+		return false, art.ID, nil
+	}
+
+	// got other error when get the artifact, return the error
+	if !ierror.IsErr(err, ierror.NotFoundCode) {
+		return false, 0, err
 	}
 
 	// the artifact doesn't exist under the repository, create it first
@@ -162,13 +158,11 @@ func (c *controller) ensureArtifact(ctx context.Context, repositoryID int64, dig
 	if err != nil {
 		// if got conflict error, try to get the artifact again
 		if ierror.IsConflictErr(err) {
-			_, artifacts, err = c.artMgr.List(ctx, query)
+			art, err = c.artMgr.GetByDigest(ctx, repositoryID, digest)
-			if err != nil {
+			if err == nil {
-				return false, 0, err
+				return false, art.ID, nil
-			}
-			if len(artifacts) > 0 {
-				return false, artifacts[0].ID, nil
 			}
+			return false, 0, err
 		}
 		return false, 0, err
 	}
@@ -246,20 +240,11 @@ func (c *controller) getByDigest(ctx context.Context, repository, digest string,
 	if err != nil {
 		return nil, err
 	}
-	_, artifacts, err := c.List(ctx, &q.Query{
+	art, err := c.artMgr.GetByDigest(ctx, repo.RepositoryID, digest)
-		Keywords: map[string]interface{}{
-			"RepositoryID": repo.RepositoryID,
-			"Digest":       digest,
-		},
-	}, option)
 	if err != nil {
 		return nil, err
 	}
-	if len(artifacts) == 0 {
+	return c.assembleArtifact(ctx, art, option), nil
-		return nil, ierror.New(nil).WithCode(ierror.NotFoundCode).
-			WithMessage("artifact %s@%s not found", repository, digest)
-	}
-	return artifacts[0], nil
 }
 
 func (c *controller) getByTag(ctx context.Context, repository, tag string, option *Option) (*Artifact, error) {

src/api/artifact/controller_test.go

@@ -158,10 +158,8 @@ func (c *controllerTestSuite) TestEnsureArtifact() {
 	digest := "sha256:418fb88ec412e340cdbef913b8ca1bbe8f9e8dc705f9617414c1f2c8db980180"
 
 	// the artifact already exists
-	c.artMgr.On("List").Return(1, []*artifact.Artifact{
+	c.artMgr.On("GetByDigest").Return(&artifact.Artifact{
-		{
+		ID: 1,
-			ID: 1,
-		},
 	}, nil)
 	created, id, err := c.ctl.ensureArtifact(nil, 1, digest)
 	c.Require().Nil(err)
@@ -175,7 +173,7 @@ func (c *controllerTestSuite) TestEnsureArtifact() {
 	c.repoMgr.On("Get").Return(&models.RepoRecord{
 		ProjectID: 1,
 	}, nil)
-	c.artMgr.On("List").Return(1, []*artifact.Artifact{}, nil)
+	c.artMgr.On("GetByDigest").Return(nil, ierror.NotFoundError(nil))
 	c.artMgr.On("Create").Return(1, nil)
 	c.abstractor.On("AbstractMetadata").Return(nil)
 	created, id, err = c.ctl.ensureArtifact(nil, 1, digest)
@@ -233,7 +231,7 @@ func (c *controllerTestSuite) TestEnsure() {
 	c.repoMgr.On("Get").Return(&models.RepoRecord{
 		ProjectID: 1,
 	}, nil)
-	c.artMgr.On("List").Return(1, []*artifact.Artifact{}, nil)
+	c.artMgr.On("GetByDigest").Return(nil, ierror.NotFoundError(nil))
 	c.artMgr.On("Create").Return(1, nil)
 	c.tagMgr.On("List").Return(1, []*tag.Tag{}, nil)
 	c.tagMgr.On("Create").Return(1, nil)
@@ -298,7 +296,7 @@ func (c *controllerTestSuite) TestGetByDigest() {
 	c.repoMgr.On("GetByName").Return(&models.RepoRecord{
 		RepositoryID: 1,
 	}, nil)
-	c.artMgr.On("List").Return(0, nil, nil)
+	c.artMgr.On("GetByDigest").Return(nil, ierror.NotFoundError(nil))
 	c.abstractor.On("ListSupportedAdditions").Return([]string{"BUILD_HISTORY"})
 	art, err := c.ctl.getByDigest(nil, "library/hello-world",
 		"sha256:418fb88ec412e340cdbef913b8ca1bbe8f9e8dc705f9617414c1f2c8db980180", nil)
@@ -312,11 +310,9 @@ func (c *controllerTestSuite) TestGetByDigest() {
 	c.repoMgr.On("GetByName").Return(&models.RepoRecord{
 		RepositoryID: 1,
 	}, nil)
-	c.artMgr.On("List").Return(1, []*artifact.Artifact{
+	c.artMgr.On("GetByDigest").Return(&artifact.Artifact{
-		{
+		ID:           1,
-			ID:           1,
+		RepositoryID: 1,
-			RepositoryID: 1,
-		},
 	}, nil)
 	c.abstractor.On("ListSupportedAdditions").Return([]string{"BUILD_HISTORY"})
 	art, err = c.ctl.getByDigest(nil, "library/hello-world",
@@ -367,11 +363,9 @@ func (c *controllerTestSuite) TestGetByReference() {
 	c.repoMgr.On("GetByName").Return(&models.RepoRecord{
 		RepositoryID: 1,
 	}, nil)
-	c.artMgr.On("List").Return(1, []*artifact.Artifact{
+	c.artMgr.On("GetByDigest").Return(&artifact.Artifact{
-		{
+		ID:           1,
-			ID:           1,
+		RepositoryID: 1,
-			RepositoryID: 1,
-		},
 	}, nil)
 	c.abstractor.On("ListSupportedAdditions").Return([]string{"BUILD_HISTORY"})
 	art, err := c.ctl.GetByReference(nil, "library/hello-world",

src/go.mod

@@ -35,8 +35,12 @@ require (
 	github.com/garyburd/redigo v1.6.0
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-openapi/errors v0.19.2
+	github.com/go-openapi/loads v0.19.3
 	github.com/go-openapi/runtime v0.19.5
+	github.com/go-openapi/spec v0.19.3
 	github.com/go-openapi/strfmt v0.19.3
+	github.com/go-openapi/swag v0.19.5
+	github.com/go-openapi/validate v0.19.3
 	github.com/go-sql-driver/mysql v1.4.1
 	github.com/gobwas/glob v0.2.3 // indirect
 	github.com/gocraft/work v0.5.1

src/pkg/artifact/dao/dao.go

@@ -26,10 +26,13 @@ import (
 type DAO interface {
 	// Count returns the total count of artifacts according to the query
 	Count(ctx context.Context, query *q.Query) (total int64, err error)
-	// List artifacts according to the query
+	// List artifacts according to the query. The artifacts that referenced by others and
+	// without tags are not returned
 	List(ctx context.Context, query *q.Query) (artifacts []*Artifact, err error)
 	// Get the artifact specified by ID
 	Get(ctx context.Context, id int64) (*Artifact, error)
+	// GetByDigest returns the artifact specified by repository ID and digest
+	GetByDigest(ctx context.Context, repositoryID int64, digest string) (artifact *Artifact, err error)
 	// Create the artifact
 	Create(ctx context.Context, artifact *Artifact) (id int64, err error)
 	// Delete the artifact specified by ID
@@ -112,6 +115,28 @@ func (d *dao) Get(ctx context.Context, id int64) (*Artifact, error) {
 	}
 	return artifact, nil
 }
+
+func (d *dao) GetByDigest(ctx context.Context, repositoryID int64, digest string) (*Artifact, error) {
+	qs, err := orm.QuerySetter(ctx, &Artifact{}, &q.Query{
+		Keywords: map[string]interface{}{
+			"RepositoryID": repositoryID,
+			"Digest":       digest,
+		},
+	})
+	if err != nil {
+		return nil, err
+	}
+	artifacts := []*Artifact{}
+	if _, err = qs.All(&artifacts); err != nil {
+		return nil, err
+	}
+	if len(artifacts) == 0 {
+		return nil, ierror.New(nil).WithCode(ierror.NotFoundCode).
+			WithMessage("artifact %s under the repository %d not found", digest, repositoryID)
+	}
+	return artifacts[0], nil
+}
+
 func (d *dao) Create(ctx context.Context, artifact *Artifact) (int64, error) {
 	ormer, err := orm.FromContext(ctx)
 	if err != nil {

src/pkg/artifact/dao/dao_test.go

@@ -322,6 +322,19 @@ func (d *daoTestSuite) TestGet() {
 	d.Equal(d.parentArtID, artifact.ID)
 }
 
+func (d *daoTestSuite) TestGetByDigest() {
+	// get the non-exist artifact
+	_, err := d.dao.GetByDigest(d.ctx, 1, "non_existing_digest")
+	d.Require().NotNil(err)
+	d.True(ierror.IsErr(err, ierror.NotFoundCode))
+
+	// get the exist artifact
+	artifact, err := d.dao.GetByDigest(d.ctx, 1, "child_digest_02")
+	d.Require().Nil(err)
+	d.Require().NotNil(artifact)
+	d.Equal(d.childArt02ID, artifact.ID)
+}
+
 func (d *daoTestSuite) TestCreate() {
 	// the happy pass case is covered in Setup
 

src/pkg/artifact/manager.go

@@ -28,10 +28,13 @@ var (
 
 // Manager is the only interface of artifact module to provide the management functions for artifacts
 type Manager interface {
-	// List artifacts according to the query, returns all artifacts if query is nil
+	// List artifacts according to the query. The artifacts that referenced by others and
+	// without tags are not returned
 	List(ctx context.Context, query *q.Query) (total int64, artifacts []*Artifact, err error)
 	// Get the artifact specified by the ID
 	Get(ctx context.Context, id int64) (artifact *Artifact, err error)
+	// GetByDigest returns the artifact specified by repository ID and digest
+	GetByDigest(ctx context.Context, repositoryID int64, digest string) (artifact *Artifact, err error)
 	// Create the artifact. If the artifact is an index, make sure all the artifacts it references
 	// already exist
 	Create(ctx context.Context, artifact *Artifact) (id int64, err error)
@@ -82,6 +85,15 @@ func (m *manager) Get(ctx context.Context, id int64) (*Artifact, error) {
 	}
 	return m.assemble(ctx, art)
 }
+
+func (m *manager) GetByDigest(ctx context.Context, repositoryID int64, digest string) (*Artifact, error) {
+	art, err := m.dao.GetByDigest(ctx, repositoryID, digest)
+	if err != nil {
+		return nil, err
+	}
+	return m.assemble(ctx, art)
+}
+
 func (m *manager) Create(ctx context.Context, artifact *Artifact) (int64, error) {
 	id, err := m.dao.Create(ctx, artifact.To())
 	if err != nil {

src/pkg/artifact/manager_test.go

@@ -40,6 +40,10 @@ func (f *fakeDao) Get(ctx context.Context, id int64) (*dao.Artifact, error) {
 	args := f.Called()
 	return args.Get(0).(*dao.Artifact), args.Error(1)
 }
+func (f *fakeDao) GetByDigest(ctx context.Context, repositoryID int64, digest string) (*dao.Artifact, error) {
+	args := f.Called()
+	return args.Get(0).(*dao.Artifact), args.Error(1)
+}
 func (f *fakeDao) Create(ctx context.Context, artifact *dao.Artifact) (int64, error) {
 	args := f.Called()
 	return int64(args.Int(0)), args.Error(1)
@@ -166,6 +170,29 @@ func (m *managerTestSuite) TestGet() {
 	m.Equal(art.ID, artifact.ID)
 }
 
+func (m *managerTestSuite) TestGetByDigest() {
+	art := &dao.Artifact{
+		ID:                1,
+		Type:              "IMAGE",
+		MediaType:         "application/vnd.oci.image.config.v1+json",
+		ManifestMediaType: "application/vnd.oci.image.manifest.v1+json",
+		ProjectID:         1,
+		RepositoryID:      1,
+		Digest:            "sha256:418fb88ec412e340cdbef913b8ca1bbe8f9e8dc705f9617414c1f2c8db980180",
+		Size:              1024,
+		PushTime:          time.Now(),
+		PullTime:          time.Now(),
+		ExtraAttrs:        `{"attr1":"value1"}`,
+		Annotations:       `{"anno1":"value1"}`,
+	}
+	m.dao.On("GetByDigest", mock.Anything).Return(art, nil)
+	m.dao.On("ListReferences").Return([]*dao.ArtifactReference{}, nil)
+	artifact, err := m.mgr.GetByDigest(nil, 1, "sha256:418fb88ec412e340cdbef913b8ca1bbe8f9e8dc705f9617414c1f2c8db980180")
+	m.Require().Nil(err)
+	m.Require().NotNil(artifact)
+	m.Equal(art.ID, artifact.ID)
+}
+
 func (m *managerTestSuite) TestCreate() {
 	m.dao.On("Create", mock.Anything).Return(1, nil)
 	m.dao.On("CreateReference").Return(1, nil)

src/server/error/error.go

@@ -58,6 +58,9 @@ func SendError(w http.ResponseWriter, err error) {
 		// only log the error whose status code < 500 when debugging to avoid log flooding
 		log.Debug(errPayload)
 	}
+	if statusCode == http.StatusUnauthorized {
+		w.Header().Set("Www-Authenticate", `Basic realm="harbor"`)
+	}
 	w.WriteHeader(statusCode)
 	fmt.Fprintln(w, errPayload)
 }

src/server/error/error_test.go

@@ -25,9 +25,17 @@ import (
 )
 
 func TestSendError(t *testing.T) {
-	// internal server error
+	// unauthorized error
 	rw := httptest.NewRecorder()
-	err := ierror.New(nil).WithCode(ierror.GeneralCode).WithMessage("unknown")
+	err := ierror.New(nil).WithCode(ierror.UnAuthorizedCode).WithMessage("unauthorized")
+	SendError(rw, err)
+	assert.Equal(t, http.StatusUnauthorized, rw.Code)
+	assert.Equal(t, `{"errors":[{"code":"UNAUTHORIZED","message":"unauthorized"}]}`+"\n", rw.Body.String())
+	assert.Equal(t, `Basic realm="harbor"`, rw.Header().Get("Www-Authenticate"))
+
+	// internal server error
+	rw = httptest.NewRecorder()
+	err = ierror.New(nil).WithCode(ierror.GeneralCode).WithMessage("unknown")
 	SendError(rw, err)
 	assert.Equal(t, http.StatusInternalServerError, rw.Code)
 	assert.Equal(t, `{"errors":[{"code":"UNKNOWN","message":"internal server error"}]}`+"\n", rw.Body.String())

src/server/middleware/util.go

@@ -34,8 +34,6 @@ const (
 	manifestInfoKey = contextKey("ManifestInfo")
 	// ScannerPullCtxKey the context key for robot account to bypass the pull policy check.
 	ScannerPullCtxKey = contextKey("ScannerPullCheck")
-	// SkipInjectRegistryCredKey is the context key telling registry proxy to skip adding credentials
-	SkipInjectRegistryCredKey = contextKey("SkipInjectRegistryCredential")
 )
 
 var (
@@ -96,12 +94,6 @@ func ArtifactInfoFromContext(ctx context.Context) (*ArtifactInfo, bool) {
 	return info, ok
 }
 
-// SkipInjectRegistryCred reflects whether the inject credentials should be skipped
-func SkipInjectRegistryCred(ctx context.Context) bool {
-	res, ok := ctx.Value(SkipInjectRegistryCredKey).(bool)
-	return ok && res
-}
-
 // NewManifestInfoContext returns context with manifest info
 func NewManifestInfoContext(ctx context.Context, info *ManifestInfo) context.Context {
 	return context.WithValue(ctx, manifestInfoKey, info)

src/server/middleware/v2auth/auth.go

@@ -15,7 +15,7 @@
 package v2auth
 
 import (
-	"context"
+	"errors"
 	"fmt"
 	serror "github.com/goharbor/harbor/src/server/error"
 	"net/http"
@@ -70,8 +70,7 @@ func (rc *reqChecker) check(req *http.Request) error {
 	} else if len(middleware.V2CatalogURLRe.FindStringSubmatch(req.URL.Path)) == 1 && !securityCtx.IsSysAdmin() {
 		return fmt.Errorf("unauthorized to list catalog")
 	} else if req.URL.Path == "/v2/" && !securityCtx.IsAuthenticated() {
-		ctx := context.WithValue(req.Context(), middleware.SkipInjectRegistryCredKey, true)
+		return errors.New("unauthorized")
-		*req = *(req.WithContext(ctx))
 	}
 	return nil
 }

src/server/registry/proxy.go

@@ -17,7 +17,6 @@ package registry
 import (
 	"fmt"
 	"github.com/goharbor/harbor/src/core/config"
-	"github.com/goharbor/harbor/src/server/middleware"
 	"net/http"
 	"net/http/httputil"
 	"net/url"
@@ -39,7 +38,7 @@ func newProxy() http.Handler {
 func basicAuthDirector(d func(*http.Request)) func(*http.Request) {
 	return func(r *http.Request) {
 		d(r)
-		if r != nil && !middleware.SkipInjectRegistryCred(r.Context()) {
+		if r != nil {
 			u, p := config.RegistryCredential()
 			r.SetBasicAuth(u, p)
 		}

src/testing/pkg/artifact/manager.go

@@ -47,6 +47,16 @@ func (f *FakeManager) Get(ctx context.Context, id int64) (*artifact.Artifact, er
 	return art, args.Error(1)
 }
 
+// GetByDigest ...
+func (f *FakeManager) GetByDigest(ctx context.Context, repositoryID int64, digest string) (*artifact.Artifact, error) {
+	args := f.Called()
+	var art *artifact.Artifact
+	if args.Get(0) != nil {
+		art = args.Get(0).(*artifact.Artifact)
+	}
+	return art, args.Error(1)
+}
+
 // Create ...
 func (f *FakeManager) Create(ctx context.Context, artifact *artifact.Artifact) (int64, error) {
 	args := f.Called()