fix content trust middleware bypass scanner pull (#11321)

Fixes #11206 1, fix middleware doesn't work for docker pull without auth 2, fix middleware doesn't bypass scanner pull Signed-off-by: wang yan <wangyan@vmware.com>
2024-06-30 16:55:07 +02:00 · 2020-03-31 11:12:21 +08:00 · 2020-03-31 11:12:21 +08:00 · 34d05dae58
commit 34d05dae58
parent 3c6f5cce54
3 changed files with 231 additions and 106 deletions
--- a/src/server/middleware/contenttrust/contenttrust.go
+++ b/src/server/middleware/contenttrust/contenttrust.go
@ -1,83 +1,66 @@
 package contenttrust

 import (
-	"net/http"
-	"net/http/httptest"
-
+	"fmt"
 	"github.com/goharbor/harbor/src/common/rbac"
 	"github.com/goharbor/harbor/src/common/security"
 	"github.com/goharbor/harbor/src/controller/project"
+	"github.com/goharbor/harbor/src/jobservice/logger"
 	"github.com/goharbor/harbor/src/lib"
 	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/pkg/signature"
-	serror "github.com/goharbor/harbor/src/server/error"
 	"github.com/goharbor/harbor/src/server/middleware"
+	"net/http"
 )

-// NotaryEndpoint ...
-var NotaryEndpoint = ""
+var (
+	// isArtifactSigned use the sign manager to check the signature, it could handle pull by tag or digtest
+	// if pull by digest, any tag of the artifact is signed, will return true.
+	isArtifactSigned = func(req *http.Request, art lib.ArtifactInfo) (bool, error) {
+		checker, err := signature.GetManager().GetCheckerByRepo(req.Context(), art.Repository)
+		if err != nil {
+			return false, err
+		}
+		return checker.IsArtifactSigned(art.Digest), nil
+	}
+)

 // Middleware handle docker pull content trust check
 func Middleware() func(http.Handler) http.Handler {
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
-			doContentTrustCheck, mf := validate(req)
-			if !doContentTrustCheck {
-				next.ServeHTTP(rw, req)
-				return
-			}
-			rec := httptest.NewRecorder()
-			next.ServeHTTP(rec, req)
-			if rec.Result().StatusCode == http.StatusOK {
-				match, err := isArtifactSigned(req, mf)
-				if err != nil {
-					serror.SendError(rw, err)
-					return
-				}
-				if !match {
-					pkgE := errors.New(nil).WithCode(errors.PROJECTPOLICYVIOLATION).WithMessage("The image is not signed in Notary.")
-					serror.SendError(rw, pkgE)
-					return
-				}
-			}
-			middleware.CopyResp(rec, rw)
-		})
-	}
-}
+	return middleware.BeforeRequest(func(r *http.Request) error {
+		ctx := r.Context()
+		none := lib.ArtifactInfo{}
+		if err := middleware.EnsureArtifactDigest(ctx); err != nil {
+			return err
+		}
+		af := lib.GetArtifactInfo(ctx)
+		if af == none {
+			return fmt.Errorf("artifactinfo middleware required before this middleware")
+		}
+		pro, err := project.Ctl.GetByName(ctx, af.ProjectName)
+		if err != nil {
+			return err
+		}
+		securityCtx, ok := security.FromContext(ctx)
+		// only authenticated robot account with scanner pull access can bypass.
+		if ok && securityCtx.IsAuthenticated() &&
+			securityCtx.Name() == "robot" &&
+			securityCtx.Can(rbac.ActionScannerPull, rbac.NewProjectNamespace(pro.ProjectID).Resource(rbac.ResourceRepository)) {
+			// the artifact is pulling by the scanner, skip the checking
+			logger.Debugf("artifact %s@%s is pulling by the scanner, skip the checking", af.Repository, af.Digest)
+			return nil
+		}

-func validate(req *http.Request) (bool, lib.ArtifactInfo) {
-	none := lib.ArtifactInfo{}
-	if err := middleware.EnsureArtifactDigest(req.Context()); err != nil {
-		return false, none
-	}
-	af := lib.GetArtifactInfo(req.Context())
-	if af == none {
-		return false, none
-	}
-	pro, err := project.Ctl.GetByName(req.Context(), af.ProjectName)
-	if err != nil {
-		return false, none
-	}
-	resource := rbac.NewProjectNamespace(pro.ProjectID).Resource(rbac.ResourceRepository)
-	securityCtx, ok := security.FromContext(req.Context())
-	if !ok {
-		return false, none
-	}
-	if !securityCtx.Can(rbac.ActionScannerPull, resource) {
-		return false, none
-	}
-	if !middleware.GetPolicyChecker().ContentTrustEnabled(af.ProjectName) {
-		return false, af
-	}
-	return true, af
-}
-
-// isArtifactSigned use the sign manager to check the signature, it could handle pull by tag or digtest
-// if pull by digest, any tag of the artifact is signed, will return true.
-func isArtifactSigned(req *http.Request, art lib.ArtifactInfo) (bool, error) {
-	checker, err := signature.GetManager().GetCheckerByRepo(req.Context(), art.Repository)
-	if err != nil {
-		return false, err
-	}
-	return checker.IsArtifactSigned(art.Digest), nil
+		if pro.ContentTrustEnabled() {
+			match, err := isArtifactSigned(r, af)
+			if err != nil {
+				return err
+			}
+			if !match {
+				pkgE := errors.New(nil).WithCode(errors.PROJECTPOLICYVIOLATION).WithMessage("The image is not signed in Notary.")
+				return pkgE
+			}
+		}
+		return nil
+	})
 }
--- a/src/server/middleware/contenttrust/contenttrust_test.go
+++ b/src/server/middleware/contenttrust/contenttrust_test.go
@ -1 +1,183 @@
+// Copyright Project Harbor Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package contenttrust
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/goharbor/harbor/src/common/models"
+	"github.com/goharbor/harbor/src/common/security"
+	"github.com/goharbor/harbor/src/controller/artifact"
+	"github.com/goharbor/harbor/src/controller/project"
+	"github.com/goharbor/harbor/src/lib"
+	securitytesting "github.com/goharbor/harbor/src/testing/common/security"
+	artifacttesting "github.com/goharbor/harbor/src/testing/controller/artifact"
+	projecttesting "github.com/goharbor/harbor/src/testing/controller/project"
+	"github.com/goharbor/harbor/src/testing/mock"
+	"github.com/stretchr/testify/suite"
+)
+
+type MiddlewareTestSuite struct {
+	suite.Suite
+
+	originalArtifactController artifact.Controller
+	artifactController         *artifacttesting.Controller
+
+	originalProjectController project.Controller
+	projectController         *projecttesting.Controller
+
+	artifact *artifact.Artifact
+	project  *models.Project
+
+	isArtifactSigned func(req *http.Request, art lib.ArtifactInfo) (bool, error)
+	next             http.Handler
+}
+
+func (suite *MiddlewareTestSuite) SetupTest() {
+	suite.originalArtifactController = artifact.Ctl
+	suite.artifactController = &artifacttesting.Controller{}
+	artifact.Ctl = suite.artifactController
+
+	suite.originalProjectController = project.Ctl
+	suite.projectController = &projecttesting.Controller{}
+	project.Ctl = suite.projectController
+
+	suite.isArtifactSigned = isArtifactSigned
+	suite.artifact = &artifact.Artifact{}
+	suite.artifact.Type = artifact.ImageType
+	suite.artifact.ProjectID = 1
+	suite.artifact.RepositoryName = "library/photon"
+	suite.artifact.Digest = "digest"
+
+	suite.project = &models.Project{
+		ProjectID: suite.artifact.ProjectID,
+		Name:      "library",
+		Metadata: map[string]string{
+			models.ProMetaEnableContentTrust: "true",
+		},
+	}
+
+	suite.next = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+
+	isArtifactSigned = func(req *http.Request, art lib.ArtifactInfo) (bool, error) {
+		return false, nil
+	}
+}
+
+func (suite *MiddlewareTestSuite) TearDownTest() {
+	artifact.Ctl = suite.originalArtifactController
+	project.Ctl = suite.originalProjectController
+}
+
+func (suite *MiddlewareTestSuite) makeRequest() *http.Request {
+	req := httptest.NewRequest("GET", "/v1/library/photon/manifests/2.0", nil)
+	info := lib.ArtifactInfo{
+		Repository: "library/photon",
+		Reference:  "2.0",
+		Tag:        "2.0",
+		Digest:     "",
+	}
+	return req.WithContext(lib.WithArtifactInfo(req.Context(), info))
+}
+
+func (suite *MiddlewareTestSuite) TestGetArtifactFailed() {
+	mock.OnAnything(suite.artifactController, "GetByReference").Return(nil, fmt.Errorf("error"))
+
+	req := suite.makeRequest()
+	rr := httptest.NewRecorder()
+
+	Middleware()(suite.next).ServeHTTP(rr, req)
+	suite.Equal(rr.Code, http.StatusInternalServerError)
+}
+
+func (suite *MiddlewareTestSuite) TestGetProjectFailed() {
+	mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil)
+	mock.OnAnything(suite.projectController, "GetByName").Return(nil, fmt.Errorf("err"))
+
+	req := suite.makeRequest()
+	rr := httptest.NewRecorder()
+
+	Middleware()(suite.next).ServeHTTP(rr, req)
+	suite.Equal(rr.Code, http.StatusInternalServerError)
+}
+
+func (suite *MiddlewareTestSuite) TestContentTrustDisabled() {
+	mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil)
+	suite.project.Metadata[models.ProMetaEnableContentTrust] = "false"
+	mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil)
+
+	req := suite.makeRequest()
+	rr := httptest.NewRecorder()
+
+	Middleware()(suite.next).ServeHTTP(rr, req)
+	suite.Equal(rr.Code, http.StatusOK)
+}
+
+func (suite *MiddlewareTestSuite) TestAuthenticatedUserPulling() {
+	mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil)
+	mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil)
+	securityCtx := &securitytesting.Context{}
+	mock.OnAnything(securityCtx, "Name").Return("local")
+	mock.OnAnything(securityCtx, "Can").Return(true, nil)
+	mock.OnAnything(securityCtx, "IsAuthenticated").Return(true)
+
+	req := suite.makeRequest()
+	req = req.WithContext(security.NewContext(req.Context(), securityCtx))
+	rr := httptest.NewRecorder()
+
+	Middleware()(suite.next).ServeHTTP(rr, req)
+	suite.Equal(rr.Code, http.StatusPreconditionFailed)
+}
+
+func (suite *MiddlewareTestSuite) TestScannerPulling() {
+	mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil)
+	mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil)
+	securityCtx := &securitytesting.Context{}
+	mock.OnAnything(securityCtx, "Name").Return("robot")
+	mock.OnAnything(securityCtx, "Can").Return(true, nil)
+	mock.OnAnything(securityCtx, "IsAuthenticated").Return(true)
+
+	req := suite.makeRequest()
+	req = req.WithContext(security.NewContext(req.Context(), securityCtx))
+	rr := httptest.NewRecorder()
+
+	Middleware()(suite.next).ServeHTTP(rr, req)
+	suite.Equal(rr.Code, http.StatusOK)
+}
+
+// pull a public project a un-signed image when policy checker is enabled.
+func (suite *MiddlewareTestSuite) TestUnAuthenticatedUserPulling() {
+	mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil)
+	mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil)
+	securityCtx := &securitytesting.Context{}
+	mock.OnAnything(securityCtx, "Name").Return("local")
+	mock.OnAnything(securityCtx, "Can").Return(true, nil)
+	mock.OnAnything(securityCtx, "IsAuthenticated").Return(false)
+
+	req := suite.makeRequest()
+	rr := httptest.NewRecorder()
+
+	Middleware()(suite.next).ServeHTTP(rr, req)
+	suite.Equal(rr.Code, http.StatusPreconditionFailed)
+}
+
+func TestMiddlewareTestSuite(t *testing.T) {
+	suite.Run(t, &MiddlewareTestSuite{})
+}
--- a/src/server/middleware/util.go
+++ b/src/server/middleware/util.go
@ -8,10 +8,7 @@ import (
 	"regexp"

 	"github.com/docker/distribution/reference"
-	"github.com/goharbor/harbor/src/common/utils/log"
 	"github.com/goharbor/harbor/src/controller/artifact"
-	"github.com/goharbor/harbor/src/core/config"
-	"github.com/goharbor/harbor/src/core/promgr"
 	"github.com/goharbor/harbor/src/lib"
 	"github.com/opencontainers/go-digest"
 )
@ -65,40 +62,3 @@ func CopyResp(rec *httptest.ResponseRecorder, rw http.ResponseWriter) {
 	rw.WriteHeader(rec.Result().StatusCode)
 	rw.Write(rec.Body.Bytes())
 }
-
-// PolicyChecker checks the policy of a project by project name, to determine if it's needed to check the image's status under this project.
-type PolicyChecker interface {
-	// contentTrustEnabled returns whether a project has enabled content trust.
-	ContentTrustEnabled(name string) bool
-}
-
-// PmsPolicyChecker ...
-type PmsPolicyChecker struct {
-	pm promgr.ProjectManager
-}
-
-// ContentTrustEnabled ...
-func (pc PmsPolicyChecker) ContentTrustEnabled(name string) bool {
-	project, err := pc.pm.Get(name)
-	if err != nil {
-		log.Errorf("Unexpected error when getting the project, error: %v", err)
-		return true
-	}
-	if project == nil {
-		log.Debugf("project %s not found", name)
-		return false
-	}
-	return project.ContentTrustEnabled()
-}
-
-// NewPMSPolicyChecker returns an instance of an pmsPolicyChecker
-func NewPMSPolicyChecker(pm promgr.ProjectManager) PolicyChecker {
-	return &PmsPolicyChecker{
-		pm: pm,
-	}
-}
-
-// GetPolicyChecker ...
-func GetPolicyChecker() PolicyChecker {
-	return NewPMSPolicyChecker(config.GlobalProjectMgr)
-}