Repository: ratify-project/ratify (go)

Issue:
Support different types of trust store
### What would you like to be added?

Notation spec supports 3 trust store types: ca, tsa and signingAuthority: https://github.com/notaryproject/specifications/blob/main/specs/trust-store-trust-policy.md#trust-policy-properties

Ratify presently doesn't differentiate between types, and we used to just set it as `ca` type as default. As we already have users use `signingAuthority` instead of `ca` type, we should provide support soon. And we also need to support `tsa` for tsa signature.

There are a few places to be updated:
1. Update `GetCertificates` implementation: https://github.com/deislabs/ratify/blob/main/pkg/verifier/notation/truststore.go#L43
2. Update `verificationCertStores` to include `trustStoreType`: https://github.com/deislabs/ratify/blob/main/pkg/verifier/notation/notation.go#L60
3. Update corresponding CR files.

And if we want to make it backward compatible, the implementation should support both old and new CRs.

### Anything else you would like to add?

_No response_

### Are you willing to submit PRs to contribute to this feature?

- [ ] Yes, I am willing to implement it.

Pull request:
feat: Support more trust store types
# Description

## What this PR does / why we need it:
<!--Please include a summary of the changes and relevant context. -->
Differentiate between trust store types: ca, tsa and signingAuthority, with full backward compatibility.
Both previous CRDs and new CRDs are supported.

## 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)*:
Closes #1174 

## Type of change
- [x] scalability for TSA
- [x] backward compatibility
- [x] code quality
  - update variable naming
  - update logging for both error message and debugging 
- [x] test cases fix & enhance

Please delete options that are not relevant.

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] 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

- [x] Default CI Test
- [x] AKS CI Test: https://github.com/ZAFT-Armored-Keeper-of-Unity/ratify/pull/16

# 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`


Patch:
diff --git a/pkg/verifier/notation/certStores.go b/pkg/verifier/notation/certStores.go
new file mode 100644
index 0000000..495e26b
--- /dev/null
+++ b/pkg/verifier/notation/certStores.go
@@ -0,0 +1,28 @@
+/*
+Copyright The Ratify 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 notation
+
+import (
+	"context"
+
+	"github.com/notaryproject/notation-go/verifier/truststore"
+)
+
+// certStores is an interface that defines the methods for managing certificate stores.
+type certStores interface {
+	// GetCertGroup returns certain type of cert group from namedStore
+	GetCertGroup(ctx context.Context, storeType truststore.Type, namedStore string) (certGroup []string)
+}
diff --git a/pkg/verifier/notation/certstoresbytype.go b/pkg/verifier/notation/certstoresbytype.go
new file mode 100644
index 0000000..db9d37d
--- /dev/null
+++ b/pkg/verifier/notation/certstoresbytype.go
@@ -0,0 +1,105 @@
+/*
+Copyright The Ratify 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 notation
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/notaryproject/notation-go/verifier/truststore"
+	"github.com/ratify-project/ratify/internal/logger"
+)
+
+type certStoreType string
+
+const (
+	CA               certStoreType = "CA"
+	SigningAuthority certStoreType = "signingAuthority"
+)
+
+func (certstoretype certStoreType) String() string {
+	return string(certstoretype)
+}
+
+// verificationCertStores describes the configuration of verification certStores
+// type verificationCertStores supports new format map[string]map[string][]string
+//
+//	{
+//	  "ca": {
+//	    "certs": {"kv1", "kv2"},
+//	  },
+//	  "signingauthority": {
+//	    "certs": {"kv3"}
+//	  },
+//	}
+//
+// type verificationCertStores supports legacy format map[string][]string as well.
+//
+//	{
+//	  "certs": {"kv1", "kv2"},
+//	},
+type verificationCertStores map[string]interface{}
+
+// certStoresByType implements certStores interface and place certs under the trustStoreType
+//
+//	{
+//	  "ca": {
+//	    "certs": {"kv1", "kv2"},
+//	  },
+//	  "signingauthority": {
+//	    "certs": {"kv3"}
+//	  },
+//	}
+type certStoresByType map[certStoreType]map[string][]string
+
+// newCertStoreByType performs type assertion and converts certificate stores configuration into certStoresByType
+func newCertStoreByType(confInNewFormat verificationCertStores) (certStores, error) {
+	s := make(certStoresByType)
+	for certstoretype, storeData := range confInNewFormat {
+		s[certStoreType(certstoretype)] = make(map[string][]string)
+		parsedStoreData, ok := storeData.(verificationCertStores)
+		if !ok {
+			return nil, fmt.Errorf("certStores: %s assertion to type verificationCertStores failed", storeData)
+		}
+		for storeName, certProviderList := range parsedStoreData {
+			var certProviderNames []string
+			parsedCertProviders, ok := certProviderList.([]interface{})
+			if !ok {
+				return nil, fmt.Errorf("certProviderList: %s assertion to type []interface{} failed", certProviderList)
+			}
+			for _, certProvider := range parsedCertProviders {
+				certProviderName, ok := certProvider.(string)
+				if !ok {
+					return nil, fmt.Errorf("certProvider: %s assertion to type string failed", certProvider)
+				}
+				certProviderNames = append(certProviderNames, certProviderName)
+			}
+			s[certStoreType(certstoretype)][storeName] = certProviderNames
+		}
+	}
+	return s, nil
+}
+
+// GetCertGroup returns certain type of certs from namedStore
+func (s certStoresByType) GetCertGroup(ctx context.Context, storeType truststore.Type, namedStore string) (certGroup []string) {
+	if certStores, ok := s[certStoreType(storeType)]; ok {
+		if certGroup, ok = certStores[namedStore]; ok {
+			return
+		}
+	}
+	logger.GetLogger(ctx, logOpt).Warnf("unable to fetch certGroup from namedStore: %+v in type: %v", namedStore, storeType)
+	return
+}
diff --git a/pkg/verifier/notation/notation.go b/pkg/verifier/notation/notation.go
index 6841dc9..30648e4 100644
--- a/pkg/verifier/notation/notation.go
+++ b/pkg/verifier/notation/notation.go
@@ -41,12 +41,15 @@ import (
 	"github.com/notaryproject/notation-go"
 	notationVerifier "github.com/notaryproject/notation-go/verifier"
 	"github.com/notaryproject/notation-go/verifier/trustpolicy"
+	"github.com/notaryproject/notation-go/verifier/truststore"
 	oci "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 const (
-	verifierType    = "notation"
-	defaultCertPath = "ratify-certs/notation/truststore"
+	verifierType                      = "notation"
+	defaultCertPath                   = "ratify-certs/notation/truststore"
+	trustStoreTypeCA                  = string(truststore.TypeCA)
+	trustStoreTypeypeSigningAuthority = string(truststore.TypeSigningAuthority)
 )
 
 // NotationPluginVerifierConfig describes the configuration of notation verifier
@@ -56,8 +59,21 @@ type NotationPluginVerifierConfig struct { //nolint:revive // ignore linter to h
 
 	// VerificationCerts is array of directories containing certificates.
 	VerificationCerts []string `json:"verificationCerts"`
-	// VerificationCerts is map defining which keyvault certificates belong to which trust store
-	VerificationCertStores map[string][]string `json:"verificationCertStores"`
+	// VerificationCertStores defines a collection of Notary Project Trust Stores.
+	// VerificationCertStores accepts new format map[string]map[string][]string
+	// {
+	//   "ca": {
+	//     "certs": {"kv1", "kv2"},
+	//   },
+	//   "signingauthority": {
+	//     "certs": {"kv3"}
+	//   },
+	// }
+	// VerificationCertStores accepts legacy format map[string][]string as well.
+	// {
+	//   "certs": {"kv1", "kv2"},
+	// },
+	VerificationCertStores verificationCertStores `json:"verificationCertStores"`
 	// TrustPolicyDoc represents a trustpolicy.json document. Reference: https://pkg.go.dev/github.com/notaryproject/notation-go@v0.12.0-beta.1.0.20221125022016-ab113ebd2a6c/verifier/trustpolicy#Document
 	TrustPolicyDoc trustpolicy.Document `json:"trustPolicyDoc"`
 }
@@ -168,11 +184,10 @@ func (v *notationPluginVerifier) Verify(ctx context.Context,
 }
 
 func getVerifierService(conf *NotationPluginVerifierConfig, pluginDirectory string) (notation.Verifier, error) {
-	store := &trustStore{
-		certPaths:  conf.VerificationCerts,
-		certStores: conf.VerificationCertStores,
+	store, err := newTrustStore(conf.VerificationCerts, conf.VerificationCertStores)
+	if err != nil {
+		return nil, err
 	}
-
 	return notationVerifier.New(&conf.TrustPolicyDoc, store, NewRatifyPluginManager(pluginDirectory))
 }
 
@@ -201,6 +216,11 @@ func parseVerifierConfig(verifierConfig config.VerifierConfig, _ string) (*Notat
 
 	defaultCertsDir := paths.Join(homedir.Get(), ratifyconfig.ConfigFileDir, defaultCertPath)
 	conf.VerificationCerts = append(conf.VerificationCerts, defaultCertsDir)
+	if len(conf.VerificationCertStores) > 0 {
+		if err := normalizeVerificationCertsStores(conf); err != nil {
+			return nil, err
+		}
+	}
 	return conf, nil
 }
 
@@ -208,3 +228,25 @@ func parseVerifierConfig(verifierConfig config.VerifierConfig, _ string) (*Notat
 func (v *notationPluginVerifier) GetNestedReferences() []string {
 	return []string{}
 }
+
+// normalizeVerificationCertsStores normalize the structure does not match the latest spec
+func normalizeVerificationCertsStores(conf *NotationPluginVerifierConfig) error {
+	isCertStoresByType, isLegacyCertStore := false, false
+	for key := range conf.VerificationCertStores {
+		if key != trustStoreTypeCA && key != trustStoreTypeypeSigningAuthority {
+			isLegacyCertStore = true
+			logger.GetLogger(context.Background(), logOpt).Debugf("Get VerificationCertStores in legacy format")
+		} else {
+			isCertStoresByType = true
+		}
+	}
+	if isCertStoresByType && isLegacyCertStore {
+		return re.ErrorCodeConfigInvalid.NewError(re.Verifier, conf.Name, re.EmptyLink, nil, "both old VerificationCertStores and new VerificationCertStores are provided, please provide only one", re.HideStackTrace)
+	} else if !isCertStoresByType && isLegacyCertStore {
+		// normalize <store>:<certs> to ca:<store><certs> if no store type is provided
+		conf.VerificationCertStores = verificationCertStores{
+			trustStoreTypeCA: conf.VerificationCertStores,
+		}
+	}
+	return nil
+}
diff --git a/pkg/verifier/notation/truststore.go b/pkg/verifier/notation/truststore.go
index 6a7314e..cb71ac6 100644
--- a/pkg/verifier/notation/truststore.go
+++ b/pkg/verifier/notation/truststore.go
@@ -34,26 +34,39 @@ var logOpt = logger.Option{
 
 type trustStore struct {
 	certPaths  []string
-	certStores map[string][]string
+	certStores certStores
+}
+
+func newTrustStore(certPaths []string, verificationCertStores verificationCertStores) (*trustStore, error) {
+	certStores, err := newCertStoreByType(verificationCertStores)
+	if err != nil {
+		return nil, err
+	}
+	store := &trustStore{
+		certPaths:  certPaths,
+		certStores: certStores,
+	}
+	return store, nil
 }
 
 // trustStore implements GetCertificates API of X509TrustStore interface: [https://pkg.go.dev/github.com/notaryproject/notation-go@v1.0.0-rc.3/verifier/truststore#X509TrustStore]
 // Note: this api gets invoked when Ratify calls verify API, so the certificates
 // will be loaded for each signature verification.
 // And this API must follow the Notation Trust Store spec: https://github.com/notaryproject/notaryproject/blob/main/specs/trust-store-trust-policy.md#trust-store
-func (s trustStore) GetCertificates(ctx context.Context, _ truststore.Type, namedStore string) ([]*x509.Certificate, error) {
-	certs, err := s.getCertificatesInternal(ctx, namedStore)
+func (s *trustStore) GetCertificates(ctx context.Context, trustStoreType truststore.Type, namedStore string) ([]*x509.Certificate, error) {
+	certs, err := s.getCertificatesInternal(ctx, trustStoreType, namedStore)
 	if err != nil {
 		return nil, err
 	}
 	return s.filterValidCerts(certs)
 }
 
-func (s trustStore) getCertificatesInternal(ctx context.Context, namedStore string) ([]*x509.Certificate, error) {
+func (s *trustStore) getCertificatesInternal(ctx context.Context, storeType truststore.Type, namedStore string) ([]*x509.Certificate, error) {
 	certs := make([]*x509.Certificate, 0)
 
+	certGroup := s.certStores.GetCertGroup(ctx, storeType, namedStore)
 	// certs configured for this namedStore overrides cert path
-	if certGroup := s.certStores[namedStore]; len(certGroup) > 0 {
+	if len(certGroup) > 0 {
 		for _, certStore := range certGroup {
 			logger.GetLogger(ctx, logOpt).Debugf("truststore getting certStore %v", certStore)
 			certMap, err := keymanagementprovider.GetCertificatesFromMap(ctx, certStore)
@@ -93,7 +106,7 @@ func (s trustStore) getCertificatesInternal(ctx context.Context, namedStore stri
 }
 
 // filterValidCerts keeps CA certificates and self-signed certs.
-func (s trustStore) filterValidCerts(certs []*x509.Certificate) ([]*x509.Certificate, error) {
+func (s *trustStore) filterValidCerts(certs []*x509.Certificate) ([]*x509.Certificate, error) {
 	filteredCerts := make([]*x509.Certificate, 0)
 	for _, cert := range certs {
 		if !cert.IsCA {


Test patch:
diff --git a/pkg/verifier/notation/certstoresbytype_test.go b/pkg/verifier/notation/certstoresbytype_test.go
new file mode 100644
index 0000000..dc3d0da
--- /dev/null
+++ b/pkg/verifier/notation/certstoresbytype_test.go
@@ -0,0 +1,61 @@
+/*
+Copyright The Ratify 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 notation
+
+import "testing"
+
+func TestNewCertStoreByTypeInvalidInput(t *testing.T) {
+	tests := []struct {
+		name      string
+		conf      verificationCertStores
+		expectErr bool
+	}{
+		{
+			name: "invalid certStores type",
+			conf: verificationCertStores{
+				trustStoreTypeCA: []string{},
+			},
+			expectErr: true,
+		},
+		{
+			name: "invalid certProviderList type",
+			conf: verificationCertStores{
+				trustStoreTypeCA: verificationCertStores{
+					"certstore1": "akv1",
+					"certstore2": []interface{}{"akv3", "akv4"},
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name: "invalid certProvider type",
+			conf: verificationCertStores{
+				trustStoreTypeCA: verificationCertStores{
+					"certstore1": []interface{}{"akv1", []string{}},
+				},
+			},
+			expectErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := newCertStoreByType(tt.conf)
+			if (err != nil) != tt.expectErr {
+				t.Errorf("error = %v, expectErr = %v", err, tt.expectErr)
+			}
+		})
+	}
+}
diff --git a/pkg/verifier/notation/notation_test.go b/pkg/verifier/notation/notation_test.go
index 10a9c63..c320a82 100644
--- a/pkg/verifier/notation/notation_test.go
+++ b/pkg/verifier/notation/notation_test.go
@@ -248,7 +248,7 @@ func TestParseVerifierConfig(t *testing.T) {
 				"name":              test,
 				"verificationCerts": []string{testPath},
 				"verificationCertStores": map[string][]string{
-					"certstore1": {"defaultns/akv1", "akv2"},
+					"certstore1": {"akv1", "akv2"},
 					"certstore2": {"akv3", "akv4"},
 				},
 			},
@@ -256,9 +256,11 @@ func TestParseVerifierConfig(t *testing.T) {
 			expect: &NotationPluginVerifierConfig{
 				Name:              test,
 				VerificationCerts: []string{testPath, defaultCertDir},
-				VerificationCertStores: map[string][]string{
-					"certstore1": {"defaultns/akv1", "akv2"},
-					"certstore2": {"akv3", "akv4"},
+				VerificationCertStores: verificationCertStores{
+					trustStoreTypeCA: verificationCertStores{
+						"certstore1": []interface{}{"akv1", "akv2"},
+						"certstore2": []interface{}{"akv3", "akv4"},
+					},
 				},
 			},
 		},
@@ -408,3 +410,50 @@ func TestGetNestedReferences(t *testing.T) {
 		t.Fatalf("notation signature should not have nested references")
 	}
 }
+
+func TestNormalizeVerificationCertsStores(t *testing.T) {
+	tests := []struct {
+		name      string
+		conf      *NotationPluginVerifierConfig
+		expectErr bool
+	}{
+		{
+			name: "successfully normalizaVerificationCertsStores",
+			conf: &NotationPluginVerifierConfig{
+				Name:              test,
+				VerificationCerts: []string{testPath, defaultCertDir},
+				VerificationCertStores: verificationCertStores{
+					trustStoreTypeCA: verificationCertStores{
+						"certstore1": []interface{}{"akv1", "akv2"},
+						"certstore2": []interface{}{"akv3", "akv4"},
+					},
+				},
+			},
+			expectErr: false,
+		},
+		{
+
+			name: "failed normalizaVerificationCertsStores with both old VerificationCertStores and new VerificationCertStores are provided",
+			conf: &NotationPluginVerifierConfig{
+				Name:              test,
+				VerificationCerts: []string{testPath, defaultCertDir},
+				VerificationCertStores: verificationCertStores{
+					trustStoreTypeCA: verificationCertStores{
+						"certstore1": []interface{}{"akv1", "akv2"},
+					},
+					"certstore2": []interface{}{"akv3", "akv4"},
+				},
+			},
+			expectErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := normalizeVerificationCertsStores(tt.conf)
+			if (err != nil) != tt.expectErr {
+				t.Errorf("error = %v, expectErr = %v", err, tt.expectErr)
+			}
+		})
+	}
+}
diff --git a/pkg/verifier/notation/truststore_test.go b/pkg/verifier/notation/truststore_test.go
index 12c331f..15bbea4 100644
--- a/pkg/verifier/notation/truststore_test.go
+++ b/pkg/verifier/notation/truststore_test.go
@@ -21,6 +21,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/notaryproject/notation-go/verifier/truststore"
 	"github.com/ratify-project/ratify/pkg/controllers"
 )
 
@@ -48,26 +49,32 @@ func (m *mockCertStores) DeleteStore(_ string) {}
 
 func TestGetCertificates_EmptyCertMap(t *testing.T) {
 	resetCertStore()
-	certStore := map[string][]string{}
-	certStore["store1"] = []string{"kv1"}
-	certStore["store2"] = []string{"kv2"}
-	store := &trustStore{
-		certStores: certStore,
+	certStore := verificationCertStores{
+		trustStoreTypeCA: verificationCertStores{
+			"certstore1": []interface{}{"akv1", "akv2"},
+			"certstore2": []interface{}{"akv3", "akv4"},
+		},
 	}
-
-	if _, err := store.getCertificatesInternal(context.Background(), "store1"); err == nil {
+	store, err := newTrustStore([]string{}, certStore)
+	if err != nil {
+		panic("failed to parse verificationCertStores: " + err.Error())
+	}
+	if _, err := store.getCertificatesInternal(context.Background(), truststore.TypeCA, "certstore1"); err == nil {
 		t.Fatalf("error expected if cert map is empty")
 	}
 }
 
 func TestGetCertificates_NamedStore(t *testing.T) {
 	resetCertStore()
-	certStore := map[string][]string{}
-	certStore["store1"] = []string{"default/kv1"}
-	certStore["store2"] = []string{"projecta/kv2"}
-
-	store := &trustStore{
-		certStores: certStore,
+	certStore := verificationCertStores{
+		trustStoreTypeCA: verificationCertStores{
+			"certstore1": []interface{}{"default/kv1"},
+			"certstore2": []interface{}{"projecta/kv2"},
+		},
+	}
+	store, err := newTrustStore(nil, certStore)
+	if err != nil {
+		panic("failed to parse verificationCertStores: " + err.Error())
 	}
 
 	kv1Cert := getCert(certStr)
@@ -81,7 +88,7 @@ func TestGetCertificates_NamedStore(t *testing.T) {
 	}
 
 	// only the certificate in the specified namedStore should be returned
-	result, _ := store.getCertificatesInternal(context.Background(), "store1")
+	result, _ := store.getCertificatesInternal(context.Background(), truststore.TypeCA, "certstore1")
 	expectedLen := 1
 
 	if len(result) != expectedLen {
@@ -105,9 +112,10 @@ func TestGetCertificates_certPath(t *testing.T) {
 	}
 
 	trustStore := &trustStore{
-		certPaths: []string{tmpFile.Name()},
+		certPaths:  []string{tmpFile.Name()},
+		certStores: certStoresByType{},
 	}
-	certs, err := trustStore.getCertificatesInternal(context.Background(), "")
+	certs, err := trustStore.getCertificatesInternal(context.Background(), truststore.TypeCA, "")
 	if err != nil {
 		t.Fatalf("failed to get certs: %v", err)
 	}