Repository: ratify-project/ratify
Pull request #2028 (2025-01-13 16:15:14)

chore: upgrade CRL revocation factory interface
# Description

## What this PR does / why we need it:

Removed `NewValidator` from interface of CRL revocation factory,
Replaced with calling `revocation.NewWithOptions` from notation library directly.
Update unit tests accordingly.

## Which issue(s) this PR fixes *(optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged)*:


## Type of change

Please delete options that are not relevant.

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Helm Chart Change (any edit/addition/update that is necessary for changes merged to the `main` branch)
- [ ] This change requires a documentation update

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

- [ ] CI Test


# Checklist:

- [ ] Does the affected code have corresponding tests?
- [ ] Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
- [ ] Does this introduce breaking changes that would require an announcement or bumping the major version?
- [ ] Do all new files have appropriate license header?

# Post Merge Requirements
- [ ] MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates `Helm Chart Change`


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/ratify-project/ratify/pull/2028?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ratify-project) Report
All modified and coverable lines are covered by tests :white_check_mark:

| [Files with missing lines](https://app.codecov.io/gh/ratify-project/ratify/pull/2028?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ratify-project) | Coverage Δ | |
|---|---|---|
| [pkg/verifier/notation/notation.go](https://app.codecov.io/gh/ratify-project/ratify/pull/2028?src=pr&el=tree&filepath=pkg%2Fverifier%2Fnotation%2Fnotation.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ratify-project#diff-cGtnL3ZlcmlmaWVyL25vdGF0aW9uL25vdGF0aW9uLmdv) | `92.90% <100.00%> (-5.15%)` | :arrow_down: |
| [pkg/verifier/notation/notationrevocationfactory.go](https://app.codecov.io/gh/ratify-project/ratify/pull/2028?src=pr&el=tree&filepath=pkg%2Fverifier%2Fnotation%2Fnotationrevocationfactory.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ratify-project#diff-cGtnL3ZlcmlmaWVyL25vdGF0aW9uL25vdGF0aW9ucmV2b2NhdGlvbmZhY3RvcnkuZ28=) | `78.57% <ø> (+4.65%)` | :arrow_up: |
| [pkg/verifier/notation/revocationfactory.go](https://app.codecov.io/gh/ratify-project/ratify/pull/2028?src=pr&el=tree&filepath=pkg%2Fverifier%2Fnotation%2Frevocationfactory.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ratify-project#diff-cGtnL3ZlcmlmaWVyL25vdGF0aW9uL3Jldm9jYXRpb25mYWN0b3J5Lmdv) | `86.36% <ø> (ø)` | |

... and [31 files with indirect coverage changes](https://app.codecov.io/gh/ratify-project/ratify/pull/2028/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ratify-project)


Patch:
diff --git a/pkg/verifier/notation/notation.go b/pkg/verifier/notation/notation.go
index 2453719..625a539 100644
--- a/pkg/verifier/notation/notation.go
+++ b/pkg/verifier/notation/notation.go
@@ -193,14 +193,14 @@ func getVerifierService(ctx context.Context, conf *NotationPluginVerifierConfig,
 	if err != nil {
 		logger.GetLogger(ctx, logOpt).Warnf("Unable to create CRL fetcher for notation verifier %s with error: %s", conf.Name, err)
 	}
-	revocationCodeSigningValidator, err := revocationFactory.NewValidator(revocation.Options{
+	revocationCodeSigningValidator, err := revocation.NewWithOptions(revocation.Options{
 		CRLFetcher:       crlFetcher,
 		CertChainPurpose: purpose.CodeSigning,
 	})
 	if err != nil {
 		return nil, err
 	}
-	revocationTimestampingValidator, err := revocationFactory.NewValidator(revocation.Options{
+	revocationTimestampingValidator, err := revocation.NewWithOptions(revocation.Options{
 		CRLFetcher:       crlFetcher,
 		CertChainPurpose: purpose.Timestamping,
 	})
diff --git a/pkg/verifier/notation/notationrevocationfactory.go b/pkg/verifier/notation/notationrevocationfactory.go
index 1a9683a..b1a0898 100644
--- a/pkg/verifier/notation/notationrevocationfactory.go
+++ b/pkg/verifier/notation/notationrevocationfactory.go
@@ -17,7 +17,6 @@ import (
 	"net/http"
 	"sync"
 
-	"github.com/notaryproject/notation-core-go/revocation"
 	corecrl "github.com/notaryproject/notation-core-go/revocation/crl"
 	"github.com/notaryproject/notation-go/dir"
 	"github.com/ratify-project/ratify/config"
@@ -62,8 +61,3 @@ func (h *CRLHandler) NewFetcher() (corecrl.Fetcher, error) {
 	}
 	return globalFetcher, nil
 }
-
-// NewValidator returns a new validator instance
-func (h *CRLHandler) NewValidator(opts revocation.Options) (revocation.Validator, error) {
-	return revocation.NewWithOptions(opts)
-}
diff --git a/pkg/verifier/notation/revocationfactory.go b/pkg/verifier/notation/revocationfactory.go
index 73486bb..0155985 100644
--- a/pkg/verifier/notation/revocationfactory.go
+++ b/pkg/verifier/notation/revocationfactory.go
@@ -19,7 +19,6 @@ import (
 	"net/http"
 	"sync"
 
-	"github.com/notaryproject/notation-core-go/revocation"
 	corecrl "github.com/notaryproject/notation-core-go/revocation/crl"
 	"github.com/notaryproject/notation-go/dir"
 	"github.com/notaryproject/notation-go/verifier/crl"
@@ -33,9 +32,6 @@ import (
 type RevocationFactory interface {
 	// NewFetcher returns a new fetcher instance
 	NewFetcher() (corecrl.Fetcher, error)
-
-	// NewValidator returns a new validator instance
-	NewValidator(revocation.Options) (revocation.Validator, error)
 }
 
 // CreateCRLFetcher returns a new fetcher instance


Test patch:
diff --git a/pkg/keymanagementprovider/refresh/kubeRefresh_test.go b/pkg/keymanagementprovider/refresh/kubeRefresh_test.go
index 859180e..3c1bda1 100644
--- a/pkg/keymanagementprovider/refresh/kubeRefresh_test.go
+++ b/pkg/keymanagementprovider/refresh/kubeRefresh_test.go
@@ -26,7 +26,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/notaryproject/notation-core-go/revocation"
 	corecrl "github.com/notaryproject/notation-core-go/revocation/crl"
 	re "github.com/ratify-project/ratify/errors"
 	"github.com/ratify-project/ratify/pkg/keymanagementprovider"
@@ -180,10 +179,6 @@ func (h *MockCRLHandler) NewFetcher() (corecrl.Fetcher, error) {
 	return nil, re.ErrorCodeConfigInvalid.WithDetail("failed to create CRL fetcher")
 }
 
-func (h *MockCRLHandler) NewValidator(_ revocation.Options) (revocation.Validator, error) {
-	return nil, nil
-}
-
 func TestKubeRefresher_GetResult(t *testing.T) {
 	kr := &KubeRefresher{
 		Result:     ctrl.Result{RequeueAfter: time.Minute},
diff --git a/pkg/verifier/notation/notation_test.go b/pkg/verifier/notation/notation_test.go
index c0ae0d2..61a0c2e 100644
--- a/pkg/verifier/notation/notation_test.go
+++ b/pkg/verifier/notation/notation_test.go
@@ -23,11 +23,10 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/notaryproject/notation-core-go/revocation"
 	corecrl "github.com/notaryproject/notation-core-go/revocation/crl"
-	"github.com/notaryproject/notation-core-go/revocation/purpose"
 	sig "github.com/notaryproject/notation-core-go/signature"
 	"github.com/notaryproject/notation-go"
+	"github.com/notaryproject/notation-go/verifier/trustpolicy"
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	ratifyconfig "github.com/ratify-project/ratify/config"
@@ -582,43 +581,27 @@ func TestGetVerifierService(t *testing.T) {
 			errContent:        nil,
 		},
 		{
-			name: "failed to create file cache",
-			conf: &NotationPluginVerifierConfig{
-				VerificationCerts: []string{defaultCertDir},
-			},
-			pluginDir:         "",
-			RevocationFactory: mockRevocationFactory{httpClient: &http.Client{}, failFileCache: true},
-			expectErr:         true,
-			errContent:        nil,
-		},
-		{
-			name: "failed to create code signing validator",
-			conf: &NotationPluginVerifierConfig{
-				VerificationCerts: []string{defaultCertDir},
-			},
-			pluginDir:         "",
-			RevocationFactory: mockRevocationFactory{httpClient: &http.Client{}, failCodeSigningValidator: true},
-			expectErr:         true,
-			errContent:        fmt.Errorf("failed to create code signing validator"),
-		},
-		{
-			name: "failed to create timestamping validator",
-			conf: &NotationPluginVerifierConfig{
-				VerificationCerts: []string{defaultCertDir},
-			},
-			pluginDir:         "",
-			RevocationFactory: mockRevocationFactory{httpClient: &http.Client{}, failTimestampingValidator: true},
-			expectErr:         true,
-			errContent:        fmt.Errorf("failed to create timestamping validator"),
-		},
-		{
-			name: "failed to create verifier",
+			name: "successfully create verifier service",
 			conf: &NotationPluginVerifierConfig{
 				VerificationCerts: []string{defaultCertDir},
+				TrustPolicyDoc: trustpolicy.Document{
+					Version: "1.0",
+					TrustPolicies: []trustpolicy.TrustPolicy{
+						{
+							Name:           "default",
+							RegistryScopes: []string{"*"},
+							SignatureVerification: trustpolicy.SignatureVerification{
+								VerificationLevel: "strict",
+							},
+							TrustStores:       []string{"ca:certs"},
+							TrustedIdentities: []string{"*"},
+						},
+					},
+				},
 			},
 			pluginDir:         "",
-			RevocationFactory: mockRevocationFactory{httpClient: &http.Client{}, failVerifier: true},
-			expectErr:         true,
+			RevocationFactory: mockRevocationFactory{httpClient: &http.Client{}},
+			expectErr:         false,
 			errContent:        nil,
 		},
 	}
@@ -637,12 +620,8 @@ func TestGetVerifierService(t *testing.T) {
 }
 
 type mockRevocationFactory struct {
-	failFetcher               bool
-	failFileCache             bool
-	failCodeSigningValidator  bool
-	failTimestampingValidator bool
-	failVerifier              bool
-	httpClient                *http.Client
+	failFetcher bool
+	httpClient  *http.Client
 }
 
 func (m mockRevocationFactory) NewFetcher() (corecrl.Fetcher, error) {
@@ -651,13 +630,3 @@ func (m mockRevocationFactory) NewFetcher() (corecrl.Fetcher, error) {
 	}
 	return corecrl.NewHTTPFetcher(m.httpClient)
 }
-
-func (m mockRevocationFactory) NewValidator(opts revocation.Options) (revocation.Validator, error) {
-	if m.failCodeSigningValidator && opts.CertChainPurpose == purpose.CodeSigning {
-		return nil, fmt.Errorf("failed to create code signing validator")
-	}
-	if m.failTimestampingValidator && opts.CertChainPurpose == purpose.Timestamping {
-		return nil, fmt.Errorf("failed to create timestamping validator")
-	}
-	return revocation.NewWithOptions(opts)
-}
diff --git a/pkg/verifier/notation/notationrevocationfactory_test.go b/pkg/verifier/notation/notationrevocationfactory_test.go
index 1cbd43e..4021313 100644
--- a/pkg/verifier/notation/notationrevocationfactory_test.go
+++ b/pkg/verifier/notation/notationrevocationfactory_test.go
@@ -18,7 +18,6 @@ import (
 	"runtime"
 	"testing"
 
-	"github.com/notaryproject/notation-core-go/revocation"
 	"github.com/notaryproject/notation-go/dir"
 	re "github.com/ratify-project/ratify/errors"
 	"github.com/stretchr/testify/assert"
@@ -72,14 +71,6 @@ func TestNewFetcher(t *testing.T) {
 	globalFetcher, _ = CreateCRLFetcher(&http.Client{}, dir.PathCRLCache)
 }
 
-func TestNewValidator(t *testing.T) {
-	factory := CreateCRLHandlerFromConfig()
-	opts := revocation.Options{}
-
-	validator, err := factory.NewValidator(opts)
-	assert.NoError(t, err)
-	assert.NotNil(t, validator)
-}
 func TestNewFileCache(t *testing.T) {
 	tests := []struct {
 		name      string