From ff2b99d711d3b77a8a2dc2b23418b7245eea6b67 Mon Sep 17 00:00:00 2001 From: Wang Yan Date: Fri, 14 Jul 2023 13:35:56 +0800 Subject: [PATCH] enable notary v2 policy checker (#18927) add notary v2 pull policy, when it enables, the artifact cannot be pull without the notation signature. Signed-off-by: wang yan --- .../{cosign.go => contentrust.go} | 85 ++++++++------ .../{cosign_test.go => contentrust_test.go} | 106 ++++++++++++++---- src/server/middleware/util/util.go | 6 +- src/server/registry/route.go | 4 +- ...test_project_level_policy_content_trust.py | 6 +- tests/robot-cases/Group1-Nightly/Common.robot | 2 +- 6 files changed, 142 insertions(+), 67 deletions(-) rename src/server/middleware/contenttrust/{cosign.go => contentrust.go} (52%) rename src/server/middleware/contenttrust/{cosign_test.go => contentrust_test.go} (68%) diff --git a/src/server/middleware/contenttrust/cosign.go b/src/server/middleware/contenttrust/contentrust.go similarity index 52% rename from src/server/middleware/contenttrust/cosign.go rename to src/server/middleware/contenttrust/contentrust.go index 6fba6eaa0..28003dbc9 100644 --- a/src/server/middleware/contenttrust/cosign.go +++ b/src/server/middleware/contenttrust/contentrust.go @@ -15,6 +15,7 @@ package contenttrust import ( + "context" "net/http" "github.com/goharbor/harbor/src/controller/artifact" @@ -27,13 +28,11 @@ import ( "github.com/goharbor/harbor/src/server/middleware/util" ) -// Cosign handle docker pull content trust check -func Cosign() func(http.Handler) http.Handler { +// ContentTrust handle docker pull content trust check +func ContentTrust() func(http.Handler) http.Handler { return middleware.BeforeRequest(func(r *http.Request) error { ctx := r.Context() - logger := log.G(ctx) - none := lib.ArtifactInfo{} af := lib.GetArtifactInfo(ctx) if af == none { @@ -44,42 +43,56 @@ func Cosign() func(http.Handler) http.Handler { return err } - // If cosign policy enabled, it has to at least have one cosign signature. + // If signature policy enabled, it has to at least have one signature. if pro.ContentTrustCosignEnabled() { - art, err := artifact.Ctl.GetByReference(ctx, af.Repository, af.Reference, &artifact.Option{ - WithAccessory: true, - }) - if err != nil { + if err := signatureChecking(ctx, r, af, pro.ProjectID, model.TypeCosignSignature); err != nil { + return err + } + } + if pro.ContentTrustEnabled() { + if err := signatureChecking(ctx, r, af, pro.ProjectID, model.TypeNotationSignature); err != nil { return err } - - ok, err := util.SkipPolicyChecking(r, pro.ProjectID, art.ID) - if err != nil { - return err - } - if ok { - logger.Debugf("artifact %s@%s is pulling by the scanner/cosign, skip the checking", af.Repository, af.Digest) - return nil - } - - if len(art.Accessories) == 0 { - pkgE := errors.New(nil).WithCode(errors.PROJECTPOLICYVIOLATION).WithMessage("The image is not signed in Cosign.") - return pkgE - } - - var hasCosignSignature bool - for _, acc := range art.Accessories { - if acc.GetData().Type == model.TypeCosignSignature { - hasCosignSignature = true - break - } - } - if !hasCosignSignature { - pkgE := errors.New(nil).WithCode(errors.PROJECTPOLICYVIOLATION).WithMessage("The image is not signed in Cosign.") - return pkgE - } } - return nil }) } + +func signatureChecking(ctx context.Context, r *http.Request, af lib.ArtifactInfo, projectID int64, signatureType string) error { + logger := log.G(ctx) + + art, err := artifact.Ctl.GetByReference(ctx, af.Repository, af.Reference, &artifact.Option{ + WithAccessory: true, + }) + if err != nil { + return err + } + + ok, err := util.SkipPolicyChecking(r, projectID, art.ID) + if err != nil { + return err + } + if ok { + logger.Debugf("skip the checking of pulling artifact %s@%s", af.Repository, af.Digest) + return nil + } + + if len(art.Accessories) == 0 { + pkgE := errors.New(nil).WithCode(errors.PROJECTPOLICYVIOLATION).WithMessage("The image is not signed.") + return pkgE + } + + var hasSignature bool + for _, acc := range art.Accessories { + if acc.GetData().Type == signatureType { + hasSignature = true + break + } + } + if !hasSignature { + pkgE := errors.New(nil).WithCode(errors.PROJECTPOLICYVIOLATION).WithMessage("The image is not signed.") + return pkgE + } + + return nil +} diff --git a/src/server/middleware/contenttrust/cosign_test.go b/src/server/middleware/contenttrust/contentrust_test.go similarity index 68% rename from src/server/middleware/contenttrust/cosign_test.go rename to src/server/middleware/contenttrust/contentrust_test.go index 0935b9503..6db4174a5 100644 --- a/src/server/middleware/contenttrust/cosign_test.go +++ b/src/server/middleware/contenttrust/contentrust_test.go @@ -38,7 +38,7 @@ import ( accessorytesting "github.com/goharbor/harbor/src/testing/pkg/accessory" ) -type CosignMiddlewareTestSuite struct { +type ContentTrustMiddlewareTestSuite struct { suite.Suite originalArtifactController artifact.Controller @@ -56,7 +56,7 @@ type CosignMiddlewareTestSuite struct { next http.Handler } -func (suite *CosignMiddlewareTestSuite) SetupTest() { +func (suite *ContentTrustMiddlewareTestSuite) SetupTest() { suite.originalArtifactController = artifact.Ctl suite.artifactController = &artifacttesting.Controller{} artifact.Ctl = suite.artifactController @@ -89,13 +89,13 @@ func (suite *CosignMiddlewareTestSuite) SetupTest() { } -func (suite *CosignMiddlewareTestSuite) TearDownTest() { +func (suite *ContentTrustMiddlewareTestSuite) TearDownTest() { artifact.Ctl = suite.originalArtifactController project.Ctl = suite.originalProjectController accessory.Mgr = suite.originalAccessMgr } -func (suite *CosignMiddlewareTestSuite) makeRequest(setHeader ...bool) *http.Request { +func (suite *ContentTrustMiddlewareTestSuite) makeRequest(setHeader ...bool) *http.Request { req := httptest.NewRequest("GET", "/v1/library/photon/manifests/2.0", nil) info := lib.ArtifactInfo{ Repository: "library/photon", @@ -111,29 +111,29 @@ func (suite *CosignMiddlewareTestSuite) makeRequest(setHeader ...bool) *http.Req return req.WithContext(lib.WithArtifactInfo(req.Context(), info)) } -func (suite *CosignMiddlewareTestSuite) TestGetArtifactFailed() { +func (suite *ContentTrustMiddlewareTestSuite) TestGetArtifactFailed() { mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) mock.OnAnything(suite.artifactController, "GetByReference").Return(nil, fmt.Errorf("error")) req := suite.makeRequest() rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusInternalServerError) } -func (suite *CosignMiddlewareTestSuite) TestGetProjectFailed() { +func (suite *ContentTrustMiddlewareTestSuite) 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() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusInternalServerError) } -func (suite *CosignMiddlewareTestSuite) TestContentTrustDisabled() { +func (suite *ContentTrustMiddlewareTestSuite) TestContentTrustDisabled() { mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) suite.project.Metadata[proModels.ProMetaEnableContentTrustCosign] = "false" mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) @@ -141,19 +141,19 @@ func (suite *CosignMiddlewareTestSuite) TestContentTrustDisabled() { req := suite.makeRequest() rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusOK) } -func (suite *CosignMiddlewareTestSuite) TestNoneArtifact() { +func (suite *ContentTrustMiddlewareTestSuite) TestNoneArtifact() { req := httptest.NewRequest("GET", "/v1/library/photon/manifests/nonexist", nil) rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusNotFound) } -func (suite *CosignMiddlewareTestSuite) TestAuthenticatedUserPulling() { +func (suite *ContentTrustMiddlewareTestSuite) TestAuthenticatedUserPulling() { mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) mock.OnAnything(suite.accessMgr, "List").Return([]accessorymodel.Accessory{}, nil) @@ -166,11 +166,11 @@ func (suite *CosignMiddlewareTestSuite) TestAuthenticatedUserPulling() { req = req.WithContext(security.NewContext(req.Context(), securityCtx)) rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusPreconditionFailed) } -func (suite *CosignMiddlewareTestSuite) TestScannerPulling() { +func (suite *ContentTrustMiddlewareTestSuite) TestScannerPulling() { mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) mock.OnAnything(suite.accessMgr, "List").Return([]accessorymodel.Accessory{}, nil) @@ -183,11 +183,11 @@ func (suite *CosignMiddlewareTestSuite) TestScannerPulling() { req = req.WithContext(security.NewContext(req.Context(), securityCtx)) rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusOK) } -func (suite *CosignMiddlewareTestSuite) TestCosignPulling() { +func (suite *ContentTrustMiddlewareTestSuite) TestCosignPulling() { mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) mock.OnAnything(suite.accessMgr, "List").Return([]accessorymodel.Accessory{}, nil) @@ -200,12 +200,12 @@ func (suite *CosignMiddlewareTestSuite) TestCosignPulling() { req = req.WithContext(security.NewContext(req.Context(), securityCtx)) rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(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 *CosignMiddlewareTestSuite) TestUnAuthenticatedUserPulling() { +func (suite *ContentTrustMiddlewareTestSuite) TestUnAuthenticatedUserPulling() { mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) mock.OnAnything(suite.accessMgr, "List").Return([]accessorymodel.Accessory{}, nil) @@ -217,12 +217,12 @@ func (suite *CosignMiddlewareTestSuite) TestUnAuthenticatedUserPulling() { req := suite.makeRequest() rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusPreconditionFailed) } // pull cosign signature when policy checker is enabled. -func (suite *CosignMiddlewareTestSuite) TestSignaturePulling() { +func (suite *ContentTrustMiddlewareTestSuite) TestCosignSignaturePulling() { mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) acc := &basemodel.Default{ @@ -240,10 +240,70 @@ func (suite *CosignMiddlewareTestSuite) TestSignaturePulling() { req := suite.makeRequest() rr := httptest.NewRecorder() - Cosign()(suite.next).ServeHTTP(rr, req) + ContentTrust()(suite.next).ServeHTTP(rr, req) + suite.Equal(rr.Code, http.StatusOK) +} + +// notation signature checking when policy checker is enabled. +func (suite *ContentTrustMiddlewareTestSuite) TestNotationSignaturePulling() { + mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) + mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) + acc := &basemodel.Default{ + Data: accessorymodel.AccessoryData{ + ID: 1, + ArtifactID: 2, + SubArtifactDigest: suite.artifact.Digest, + Type: accessorymodel.TypeNotationSignature, + }, + } + suite.project.Metadata[proModels.ProMetaEnableContentTrust] = "true" + suite.project.Metadata[proModels.ProMetaEnableContentTrustCosign] = "false" + suite.artifact.Accessories = []accessorymodel.Accessory{acc} + mock.OnAnything(suite.accessMgr, "List").Return([]accessorymodel.Accessory{ + acc, + }, nil) + + req := suite.makeRequest() + rr := httptest.NewRecorder() + + ContentTrust()(suite.next).ServeHTTP(rr, req) + suite.Equal(rr.Code, http.StatusOK) +} + +// notation & cosign signature when both policy checker are enabled. +func (suite *ContentTrustMiddlewareTestSuite) TestBothSignaturePulling() { + mock.OnAnything(suite.artifactController, "GetByReference").Return(suite.artifact, nil) + mock.OnAnything(suite.projectController, "GetByName").Return(suite.project, nil) + acc1 := &basemodel.Default{ + Data: accessorymodel.AccessoryData{ + ID: 1, + ArtifactID: 2, + SubArtifactDigest: suite.artifact.Digest, + Type: accessorymodel.TypeNotationSignature, + }, + } + acc2 := &basemodel.Default{ + Data: accessorymodel.AccessoryData{ + ID: 1, + ArtifactID: 3, + SubArtifactDigest: suite.artifact.Digest, + Type: accessorymodel.TypeCosignSignature, + }, + } + suite.project.Metadata[proModels.ProMetaEnableContentTrust] = "true" + suite.project.Metadata[proModels.ProMetaEnableContentTrustCosign] = "true" + suite.artifact.Accessories = []accessorymodel.Accessory{acc1, acc2} + mock.OnAnything(suite.accessMgr, "List").Return([]accessorymodel.Accessory{ + acc1, acc2, + }, nil) + + req := suite.makeRequest() + rr := httptest.NewRecorder() + + ContentTrust()(suite.next).ServeHTTP(rr, req) suite.Equal(rr.Code, http.StatusOK) } func TestCosignMiddlewareTestSuite(t *testing.T) { - suite.Run(t, &CosignMiddlewareTestSuite{}) + suite.Run(t, &ContentTrustMiddlewareTestSuite{}) } diff --git a/src/server/middleware/util/util.go b/src/server/middleware/util/util.go index ee27f3630..9e04e7b4b 100644 --- a/src/server/middleware/util/util.go +++ b/src/server/middleware/util/util.go @@ -63,12 +63,14 @@ func SkipPolicyChecking(r *http.Request, projectID, artID int64) (bool, error) { secCtx, ok := security.FromContext(r.Context()) // 1, scanner pull access can bypass. - // 2, cosign pull can bypass, it needs to pull the manifest before pushing the signature. + // 2, cosign/notation pull can bypass, it needs to pull the manifest before pushing the signature. // 3, pull cosign signature can bypass. if ok && secCtx.Name() == "v2token" { if secCtx.Can(r.Context(), rbac.ActionScannerPull, project.NewNamespace(projectID).Resource(rbac.ResourceRepository)) || (secCtx.Can(r.Context(), rbac.ActionPush, project.NewNamespace(projectID).Resource(rbac.ResourceRepository)) && - strings.Contains(r.UserAgent(), "cosign")) { + strings.Contains(r.UserAgent(), "cosign")) || + (secCtx.Can(r.Context(), rbac.ActionPush, project.NewNamespace(projectID).Resource(rbac.ResourceRepository)) && + strings.Contains(r.UserAgent(), "notation")) { return true, nil } } diff --git a/src/server/registry/route.go b/src/server/registry/route.go index 7c783e948..833f8c78d 100644 --- a/src/server/registry/route.go +++ b/src/server/registry/route.go @@ -54,7 +54,7 @@ func RegisterRoutes() { Path("/*/manifests/:reference"). Middleware(metric.InjectOpIDMiddleware(metric.ManifestOperationID)). Middleware(repoproxy.ManifestMiddleware()). - Middleware(contenttrust.Cosign()). + Middleware(contenttrust.ContentTrust()). Middleware(vulnerable.Middleware()). HandlerFunc(getManifest) root.NewRoute(). @@ -62,7 +62,7 @@ func RegisterRoutes() { Path("/*/manifests/:reference"). Middleware(metric.InjectOpIDMiddleware(metric.ManifestOperationID)). Middleware(repoproxy.ManifestMiddleware()). - Middleware(contenttrust.Cosign()). + Middleware(contenttrust.ContentTrust()). Middleware(vulnerable.Middleware()). HandlerFunc(getManifest) root.NewRoute(). diff --git a/tests/apitests/python/test_project_level_policy_content_trust.py b/tests/apitests/python/test_project_level_policy_content_trust.py index 2ebe8a2bb..002741e04 100644 --- a/tests/apitests/python/test_project_level_policy_content_trust.py +++ b/tests/apitests/python/test_project_level_policy_content_trust.py @@ -45,7 +45,7 @@ class TestProjects(unittest.TestCase): 4. Image(IA) should exist; 5. Pull image(IA) successfully; 6. Enable content trust in project(PA) configuration; - 7. Pull image(IA) failed and the reason is "The image is not signed in Cosign". + 7. Pull image(IA) failed and the reason is "The image is not signed". Tear down: 1. Delete repository(RA) by user(UA); 2. Delete project(PA); @@ -79,12 +79,12 @@ class TestProjects(unittest.TestCase): self.project.update_project(TestProjects.project_content_trust_id, metadata = {"enable_content_trust_cosign": "true"}, **TestProjects.USER_CONTENT_TRUST_CLIENT) self.project.get_project(TestProjects.project_content_trust_id) - #7. Pull image(IA) failed and the reason is "The image is not signed in Cosign". + #7. Pull image(IA) failed and the reason is "The image is not signed". docker_image_clean_all() restart_process("containerd") restart_process("dockerd") time.sleep(30) - pull_harbor_image(harbor_server, ADMIN_CLIENT["username"], ADMIN_CLIENT["password"], TestProjects.repo_name, tag, expected_error_message = "The image is not signed in Cosign") + pull_harbor_image(harbor_server, ADMIN_CLIENT["username"], ADMIN_CLIENT["password"], TestProjects.repo_name, tag, expected_error_message = "The image is not signed") if __name__ == '__main__': unittest.main() diff --git a/tests/robot-cases/Group1-Nightly/Common.robot b/tests/robot-cases/Group1-Nightly/Common.robot index 7346860cc..50de09214 100644 --- a/tests/robot-cases/Group1-Nightly/Common.robot +++ b/tests/robot-cases/Group1-Nightly/Common.robot @@ -773,7 +773,7 @@ Test Case - Cosign And Cosign Deployment Security Policy Go Into Project project${d} Go Into Repo project${d} ${image} Should Not Be Signed By Cosign ${tag} - Cannot Pull Image ${ip} ${user} ${pwd} project${d} ${image}:${tag} err_msg=The image is not signed in Cosign. + Cannot Pull Image ${ip} ${user} ${pwd} project${d} ${image}:${tag} err_msg=The image is not signed. Cosign Generate Key Pair Cosign Verify ${ip}/project${d}/${image}:${tag} ${false}