From eceb9b2345dfd3a71fb6de1bb052b7a6382c07d3 Mon Sep 17 00:00:00 2001 From: Wenkai Yin Date: Wed, 12 Feb 2020 20:46:04 +0800 Subject: [PATCH] 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 --- .../abstractor/resolver/image/index.go | 17 ++------ .../abstractor/resolver/image/index_test.go | 8 ++-- src/api/artifact/controller.go | 43 ++++++------------- src/api/artifact/controller_test.go | 28 +++++------- src/go.mod | 4 ++ src/pkg/artifact/dao/dao.go | 27 +++++++++++- src/pkg/artifact/dao/dao_test.go | 13 ++++++ src/pkg/artifact/manager.go | 14 +++++- src/pkg/artifact/manager_test.go | 27 ++++++++++++ src/server/error/error.go | 3 ++ src/server/error/error_test.go | 12 +++++- src/server/middleware/util.go | 8 ---- src/server/middleware/v2auth/auth.go | 5 +-- src/server/registry/proxy.go | 3 +- src/testing/pkg/artifact/manager.go | 10 +++++ 15 files changed, 140 insertions(+), 82 deletions(-) diff --git a/src/api/artifact/abstractor/resolver/image/index.go b/src/api/artifact/abstractor/resolver/image/index.go index de10d2293..5b646361c 100644 --- a/src/api/artifact/abstractor/resolver/image/index.go +++ b/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{ - Keywords: map[string]interface{}{ - "RepositoryID": art.RepositoryID, - "Digest": digest, - }, - }) + // make sure the child artifact exist + ar, err := i.artMgr.GetByDigest(ctx, art.RepositoryID, 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, }) } diff --git a/src/api/artifact/abstractor/resolver/image/index_test.go b/src/api/artifact/abstractor/resolver/image/index_test.go index 54e88c00c..4d3b7c1f1 100644 --- a/src/api/artifact/abstractor/resolver/image/index_test.go +++ b/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{ - { - ID: 1, - }, + i.artMgr.On("GetByDigest").Return(&artifact.Artifact{ + 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) } diff --git a/src/api/artifact/controller.go b/src/api/artifact/controller.go index ad9621df0..035b777ef 100644 --- a/src/api/artifact/controller.go +++ b/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{ - Keywords: map[string]interface{}{ - "repository_id": repositoryID, - "digest": digest, - }, - } - _, artifacts, err := c.artMgr.List(ctx, query) - if err != nil { - return false, 0, err - } + art, err := c.artMgr.GetByDigest(ctx, repositoryID, digest) // the artifact already exists under the repository, return directly - if len(artifacts) > 0 { - return false, artifacts[0].ID, nil + if err == 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) - if err != nil { - return false, 0, err - } - if len(artifacts) > 0 { - return false, artifacts[0].ID, nil + art, err = c.artMgr.GetByDigest(ctx, repositoryID, digest) + if err == nil { + return false, art.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{ - Keywords: map[string]interface{}{ - "RepositoryID": repo.RepositoryID, - "Digest": digest, - }, - }, option) + art, err := c.artMgr.GetByDigest(ctx, repo.RepositoryID, digest) if err != nil { return nil, err } - if len(artifacts) == 0 { - return nil, ierror.New(nil).WithCode(ierror.NotFoundCode). - WithMessage("artifact %s@%s not found", repository, digest) - } - return artifacts[0], nil + return c.assembleArtifact(ctx, art, option), nil } func (c *controller) getByTag(ctx context.Context, repository, tag string, option *Option) (*Artifact, error) { diff --git a/src/api/artifact/controller_test.go b/src/api/artifact/controller_test.go index 1d88fc2cd..2df1569d4 100644 --- a/src/api/artifact/controller_test.go +++ b/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{ - { - ID: 1, - }, + c.artMgr.On("GetByDigest").Return(&artifact.Artifact{ + 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{ - { - ID: 1, - RepositoryID: 1, - }, + c.artMgr.On("GetByDigest").Return(&artifact.Artifact{ + ID: 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{ - { - ID: 1, - RepositoryID: 1, - }, + c.artMgr.On("GetByDigest").Return(&artifact.Artifact{ + ID: 1, + RepositoryID: 1, }, nil) c.abstractor.On("ListSupportedAdditions").Return([]string{"BUILD_HISTORY"}) art, err := c.ctl.GetByReference(nil, "library/hello-world", diff --git a/src/go.mod b/src/go.mod index d22d0a7ef..a9c1c47d4 100644 --- a/src/go.mod +++ b/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 diff --git a/src/pkg/artifact/dao/dao.go b/src/pkg/artifact/dao/dao.go index a8268d56b..f34806266 100644 --- a/src/pkg/artifact/dao/dao.go +++ b/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 { diff --git a/src/pkg/artifact/dao/dao_test.go b/src/pkg/artifact/dao/dao_test.go index ad37d2f39..f516ec753 100644 --- a/src/pkg/artifact/dao/dao_test.go +++ b/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 diff --git a/src/pkg/artifact/manager.go b/src/pkg/artifact/manager.go index 005d566ef..f522afbf1 100644 --- a/src/pkg/artifact/manager.go +++ b/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 { diff --git a/src/pkg/artifact/manager_test.go b/src/pkg/artifact/manager_test.go index 960acebe9..a48ecb542 100644 --- a/src/pkg/artifact/manager_test.go +++ b/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) diff --git a/src/server/error/error.go b/src/server/error/error.go index 99c77756e..e823359dd 100644 --- a/src/server/error/error.go +++ b/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) } diff --git a/src/server/error/error_test.go b/src/server/error/error_test.go index 905c2a0d7..7aceac6e6 100644 --- a/src/server/error/error_test.go +++ b/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()) diff --git a/src/server/middleware/util.go b/src/server/middleware/util.go index 30930b876..39b33e1fe 100644 --- a/src/server/middleware/util.go +++ b/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) diff --git a/src/server/middleware/v2auth/auth.go b/src/server/middleware/v2auth/auth.go index 742533b39..764b99840 100644 --- a/src/server/middleware/v2auth/auth.go +++ b/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) - *req = *(req.WithContext(ctx)) + return errors.New("unauthorized") } return nil } diff --git a/src/server/registry/proxy.go b/src/server/registry/proxy.go index 8e68c330c..d891f8386 100644 --- a/src/server/registry/proxy.go +++ b/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) } diff --git a/src/testing/pkg/artifact/manager.go b/src/testing/pkg/artifact/manager.go index b168651a9..fa117202d 100644 --- a/src/testing/pkg/artifact/manager.go +++ b/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()