Repository: deislabs/ratify
Pull request #1395 (2024-04-15 08:38:27)

feat: add compatibility check in KMP while fetching certs/keys [multi-tenancy PR 6]
# Description

## What this PR does / why we need it:
This is the 6th PR for multi-tenancy support. Please review https://github.com/deislabs/ratify/pull/1382 first. Check diff between PR 5 and PR 6 at: https://github.com/binbin-li/ratify/pull/121

1. Add context.Context to GetCerts and GetKeys methods so that it can fetch the namespace and check the compatibility of verifier against trust store.
2. Add missing DeleteKeysFromMap step in controller.


## 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)*:
Fixes #

## 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

- [ ] Test A
- [ ] Test B

# 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/deislabs/ratify/pull/1395?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) Report
Attention: Patch coverage is `54.46429%` with `51 lines` in your changes are missing coverage. Please review.
> Project coverage is 64.97%. Comparing base [(`73ef709`)](https://app.codecov.io/gh/deislabs/ratify/commit/73ef7091e919c18ad37c7ec0a6e00b5471db3748?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) to head [(`6596383`)](https://app.codecov.io/gh/deislabs/ratify/pull/1395?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs).
> Report is 15 commits behind head on staging.

| [Files](https://app.codecov.io/gh/deislabs/ratify/pull/1395?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) | Patch % | Lines |
|---|---|---|
| [pkg/utils/test\_utils.go](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&filepath=pkg%2Futils%2Ftest_utils.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs#diff-cGtnL3V0aWxzL3Rlc3RfdXRpbHMuZ28=) | 0.00% | [41 Missing :warning: ](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) |
| [...kg/controllers/keymanagementprovider\_controller.go](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&filepath=pkg%2Fcontrollers%2Fkeymanagementprovider_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs#diff-cGtnL2NvbnRyb2xsZXJzL2tleW1hbmFnZW1lbnRwcm92aWRlcl9jb250cm9sbGVyLmdv) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) |
| [pkg/verifier/notation/truststore.go](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&filepath=pkg%2Fverifier%2Fnotation%2Ftruststore.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs#diff-cGtnL3ZlcmlmaWVyL25vdGF0aW9uL3RydXN0c3RvcmUuZ28=) | 42.85% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) |
| [pkg/controllers/certificatestore\_controller.go](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&filepath=pkg%2Fcontrollers%2Fcertificatestore_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs#diff-cGtnL2NvbnRyb2xsZXJzL2NlcnRpZmljYXRlc3RvcmVfY29udHJvbGxlci5nbw==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/deislabs/ratify/pull/1395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) |

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           staging    #1395      +/-   ##
===========================================
- Coverage    65.31%   64.97%   -0.35%     
===========================================
  Files          105       95      -10     
  Lines         5458     4939     -519     
===========================================
- Hits          3565     3209     -356     
+ Misses        1535     1443      -92     
+ Partials       358      287      -71     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/deislabs/ratify/pull/1395?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs).

binbin-li: Rebased to resolve conflicts.
binbin-li: Added context to `GetKeys` so that `namespace` could be passed to trustPolicy.

Patch:
diff --git a/pkg/controllers/keymanagementprovider_controller.go b/pkg/controllers/keymanagementprovider_controller.go
index eaf9af2..13826e0 100644
--- a/pkg/controllers/keymanagementprovider_controller.go
+++ b/pkg/controllers/keymanagementprovider_controller.go
@@ -33,7 +33,7 @@ import (
 
 	configv1beta1 "github.com/deislabs/ratify/api/v1beta1"
 	c "github.com/deislabs/ratify/config"
-	"github.com/deislabs/ratify/pkg/keymanagementprovider"
+	kmp "github.com/deislabs/ratify/pkg/keymanagementprovider"
 	"github.com/deislabs/ratify/pkg/keymanagementprovider/config"
 	"github.com/deislabs/ratify/pkg/keymanagementprovider/factory"
 	"github.com/deislabs/ratify/pkg/keymanagementprovider/types"
@@ -60,7 +60,8 @@ func (r *KeyManagementProviderReconciler) Reconcile(ctx context.Context, req ctr
 	if err := r.Get(ctx, req.NamespacedName, &keyManagementProvider); err != nil {
 		if apierrors.IsNotFound(err) {
 			logger.Infof("deletion detected, removing key management provider %v", resource)
-			keymanagementprovider.DeleteCertificatesFromMap(resource)
+			kmp.DeleteCertificatesFromMap(resource)
+			kmp.DeleteKeysFromMap(resource)
 		} else {
 			logger.Error(err, "unable to fetch key management provider")
 		}
@@ -103,8 +104,8 @@ func (r *KeyManagementProviderReconciler) Reconcile(ctx context.Context, req ctr
 		writeKMProviderStatus(ctx, r, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil)
 		return ctrl.Result{}, fmt.Errorf("Error fetching keys in KMProvider %v with %v provider, error: %w", resource, keyManagementProvider.Spec.Type, err)
 	}
-	keymanagementprovider.SetCertificatesInMap(resource, certificates)
-	keymanagementprovider.SetKeysInMap(resource, keyManagementProvider.Spec.Type, keys)
+	kmp.SetCertificatesInMap(resource, certificates)
+	kmp.SetKeysInMap(resource, keyManagementProvider.Spec.Type, keys)
 	// merge certificates and keys status into one
 	maps.Copy(keyAttributes, certAttributes)
 	isFetchSuccessful = true
@@ -130,7 +131,7 @@ func (r *KeyManagementProviderReconciler) SetupWithManager(mgr ctrl.Manager) err
 }
 
 // specToKeyManagementProvider creates KeyManagementProviderProvider from  KeyManagementProviderSpec config
-func specToKeyManagementProvider(spec configv1beta1.KeyManagementProviderSpec) (keymanagementprovider.KeyManagementProvider, error) {
+func specToKeyManagementProvider(spec configv1beta1.KeyManagementProviderSpec) (kmp.KeyManagementProvider, error) {
 	kmProviderConfig, err := rawToKeyManagementProviderConfig(spec.Parameters.Raw, spec.Type)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse key management provider config: %w", err)
@@ -162,7 +163,7 @@ func rawToKeyManagementProviderConfig(raw []byte, keyManagamentSystemName string
 }
 
 // writeKMProviderStatus updates the status of the key management provider resource
-func writeKMProviderStatus(ctx context.Context, r client.StatusClient, keyManagementProvider *configv1beta1.KeyManagementProvider, logger *logrus.Entry, isSuccess bool, errorString string, operationTime metav1.Time, kmProviderStatus keymanagementprovider.KeyManagementProviderStatus) {
+func writeKMProviderStatus(ctx context.Context, r client.StatusClient, keyManagementProvider *configv1beta1.KeyManagementProvider, logger *logrus.Entry, isSuccess bool, errorString string, operationTime metav1.Time, kmProviderStatus kmp.KeyManagementProviderStatus) {
 	if isSuccess {
 		updateKMProviderSuccessStatus(keyManagementProvider, &operationTime, kmProviderStatus)
 	} else {
@@ -188,7 +189,7 @@ func updateKMProviderErrorStatus(keyManagementProvider *configv1beta1.KeyManagem
 
 // updateKMProviderSuccessStatus updates the key management provider status if status argument is non nil
 // Success status includes last fetched time and other provider-specific properties
-func updateKMProviderSuccessStatus(keyManagementProvider *configv1beta1.KeyManagementProvider, lastOperationTime *metav1.Time, kmProviderStatus keymanagementprovider.KeyManagementProviderStatus) {
+func updateKMProviderSuccessStatus(keyManagementProvider *configv1beta1.KeyManagementProvider, lastOperationTime *metav1.Time, kmProviderStatus kmp.KeyManagementProviderStatus) {
 	keyManagementProvider.Status.IsSuccess = true
 	keyManagementProvider.Status.Error = ""
 	keyManagementProvider.Status.BriefError = ""
diff --git a/pkg/keymanagementprovider/keymanagementprovider.go b/pkg/keymanagementprovider/keymanagementprovider.go
index d1267ca..b8e7ffe 100644
--- a/pkg/keymanagementprovider/keymanagementprovider.go
+++ b/pkg/keymanagementprovider/keymanagementprovider.go
@@ -110,7 +110,10 @@ func SetCertificatesInMap(resource string, certs map[KMPMapKey][]*x509.Certifica
 }
 
 // GetCertificatesFromMap gets the certificates from the map and returns an empty map of certificate arrays if not found
-func GetCertificatesFromMap(resource string) map[KMPMapKey][]*x509.Certificate {
+func GetCertificatesFromMap(ctx context.Context, resource string) map[KMPMapKey][]*x509.Certificate {
+	if !isCompatibleNamespace(ctx, resource) {
+		return map[KMPMapKey][]*x509.Certificate{}
+	}
 	certs, ok := certificatesMap.Load(resource)
 	if !ok {
 		return map[KMPMapKey][]*x509.Certificate{}
@@ -143,7 +146,10 @@ func SetKeysInMap(resource string, providerType string, keys map[KMPMapKey]crypt
 }
 
 // GetKeysFromMap gets the keys from the map and returns an empty map with false boolean if not found
-func GetKeysFromMap(resource string) (map[KMPMapKey]PublicKey, bool) {
+func GetKeysFromMap(ctx context.Context, resource string) (map[KMPMapKey]PublicKey, bool) {
+	if !isCompatibleNamespace(ctx, resource) {
+		return map[KMPMapKey]PublicKey{}, false
+	}
 	keys, ok := keyMap.Load(resource)
 	if !ok {
 		return map[KMPMapKey]PublicKey{}, false
@@ -155,3 +161,10 @@ func GetKeysFromMap(resource string) (map[KMPMapKey]PublicKey, bool) {
 func DeleteKeysFromMap(resource string) {
 	keyMap.Delete(resource)
 }
+
+// Namespaced verifiers could access both cluster-scoped and namespaced certStores.
+// But cluster-wide verifiers could only access cluster-scoped certStores.
+// TODO: current implementation always returns true. Check the namespace once we support multi-tenancy.
+func isCompatibleNamespace(_ context.Context, _ string) bool {
+	return true
+}
diff --git a/pkg/verifier/cosign/cosign.go b/pkg/verifier/cosign/cosign.go
index 819dd05..d85a2cc 100644
--- a/pkg/verifier/cosign/cosign.go
+++ b/pkg/verifier/cosign/cosign.go
@@ -529,7 +529,7 @@ func getKeysMapsDefault(ctx context.Context, trustPolicies *TrustPolicies, refer
 	logger.GetLogger(ctx, logOpt).Debugf("selected trust policy %s for reference %s", trustPolicy.GetName(), reference)
 
 	// get the map of keys for that reference
-	keysMap, err := trustPolicy.GetKeys(namespace)
+	keysMap, err := trustPolicy.GetKeys(ctx, namespace)
 	if err != nil {
 		return nil, cosign.CheckOpts{}, err
 	}
diff --git a/pkg/verifier/cosign/trustpolicy.go b/pkg/verifier/cosign/trustpolicy.go
index 9975af1..371a215 100644
--- a/pkg/verifier/cosign/trustpolicy.go
+++ b/pkg/verifier/cosign/trustpolicy.go
@@ -70,7 +70,7 @@ type trustPolicy struct {
 
 type TrustPolicy interface {
 	GetName() string
-	GetKeys(string) (map[PKKey]keymanagementprovider.PublicKey, error)
+	GetKeys(ctx context.Context, namespace string) (map[PKKey]keymanagementprovider.PublicKey, error)
 	GetScopes() []string
 	GetCosignOpts(context.Context) (cosign.CheckOpts, error)
 }
@@ -138,7 +138,7 @@ func (tp *trustPolicy) GetName() string {
 }
 
 // GetKeys returns the public keys defined in the trust policy
-func (tp *trustPolicy) GetKeys(namespace string) (map[PKKey]keymanagementprovider.PublicKey, error) {
+func (tp *trustPolicy) GetKeys(ctx context.Context, namespace string) (map[PKKey]keymanagementprovider.PublicKey, error) {
 	keyMap := make(map[PKKey]keymanagementprovider.PublicKey)
 	// preload the local keys into the map of keys to be returned
 	for key, pubKey := range tp.localKeys {
@@ -153,7 +153,7 @@ func (tp *trustPolicy) GetKeys(namespace string) (map[PKKey]keymanagementprovide
 		// must prepend namespace to key management provider name if not provided since namespace is prepended during key management provider intialization
 		namespacedKMP := prependNamespaceToKMPName(keyConfig.Provider, namespace)
 		// get the key management provider resource which contains a map of keys
-		kmpResource, ok := keymanagementprovider.GetKeysFromMap(namespacedKMP)
+		kmpResource, ok := keymanagementprovider.GetKeysFromMap(ctx, namespacedKMP)
 		if !ok {
 			return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(tp.verifierName).WithDetail(fmt.Sprintf("trust policy %s failed: key management provider %s not found", tp.config.Name, namespacedKMP))
 		}
diff --git a/pkg/verifier/notation/truststore.go b/pkg/verifier/notation/truststore.go
index 89a7fa9..bdf3ad6 100644
--- a/pkg/verifier/notation/truststore.go
+++ b/pkg/verifier/notation/truststore.go
@@ -56,7 +56,7 @@ func (s trustStore) getCertificatesInternal(ctx context.Context, namedStore stri
 	if certGroup := s.certStores[namedStore]; len(certGroup) > 0 {
 		for _, certStore := range certGroup {
 			logger.GetLogger(ctx, logOpt).Debugf("truststore getting certStore %v", certStore)
-			result := keymanagementprovider.FlattenKMPMap(keymanagementprovider.GetCertificatesFromMap(certStore))
+			result := keymanagementprovider.FlattenKMPMap(keymanagementprovider.GetCertificatesFromMap(ctx, certStore))
 			// notation verifier does not consider specific named/versioned certificates within a key management provider resource
 			if len(result) == 0 {
 				logger.GetLogger(ctx, logOpt).Warnf("no certificate fetched for Key Management Provider %+v", certStore)


Test patch:
diff --git a/pkg/keymanagementprovider/keymanagementprovider_test.go b/pkg/keymanagementprovider/keymanagementprovider_test.go
index 266d790..1b74418 100644
--- a/pkg/keymanagementprovider/keymanagementprovider_test.go
+++ b/pkg/keymanagementprovider/keymanagementprovider_test.go
@@ -16,6 +16,7 @@ limitations under the License.
 package keymanagementprovider
 
 import (
+	"context"
 	"crypto"
 	"crypto/rsa"
 	"crypto/x509"
@@ -142,7 +143,7 @@ func TestSetCertificatesInMap(t *testing.T) {
 func TestGetCertificatesFromMap(t *testing.T) {
 	certificatesMap.Delete("test")
 	SetCertificatesInMap("test", map[KMPMapKey][]*x509.Certificate{{}: {{Raw: []byte("testcert")}}})
-	certs := GetCertificatesFromMap("test")
+	certs := GetCertificatesFromMap(context.Background(), "test")
 	if len(certs) != 1 {
 		t.Fatalf("certificates should have been fetched from the map")
 	}
@@ -151,7 +152,7 @@ func TestGetCertificatesFromMap(t *testing.T) {
 // TestGetCertificatesFromMap_FailedToFetch checks if certificates are fetched from the map
 func TestGetCertificatesFromMap_FailedToFetch(t *testing.T) {
 	certificatesMap.Delete("test")
-	certs := GetCertificatesFromMap("test")
+	certs := GetCertificatesFromMap(context.Background(), "test")
 	if len(certs) != 0 {
 		t.Fatalf("certificates should not have been fetched from the map")
 	}
@@ -188,7 +189,7 @@ func TestSetKeysInMap(t *testing.T) {
 func TestGetKeysFromMap(t *testing.T) {
 	keyMap.Delete("test")
 	SetKeysInMap("test", "", map[KMPMapKey]crypto.PublicKey{{}: &rsa.PublicKey{}})
-	keys, _ := GetKeysFromMap("test")
+	keys, _ := GetKeysFromMap(context.Background(), "test")
 	if len(keys) != 1 {
 		t.Fatalf("keys should have been fetched from the map")
 	}
@@ -197,7 +198,7 @@ func TestGetKeysFromMap(t *testing.T) {
 // TestGetKeysFromMap_FailedToFetch checks if keys fail to fetch from map
 func TestGetKeysFromMap_FailedToFetch(t *testing.T) {
 	keyMap.Delete("test")
-	keys, _ := GetKeysFromMap("test")
+	keys, _ := GetKeysFromMap(context.Background(), "test")
 	if len(keys) != 0 {
 		t.Fatalf("keys should not have been fetched from the map")
 	}
diff --git a/pkg/verifier/cosign/trustpolicy_test.go b/pkg/verifier/cosign/trustpolicy_test.go
index 84155da..948c3d2 100644
--- a/pkg/verifier/cosign/trustpolicy_test.go
+++ b/pkg/verifier/cosign/trustpolicy_test.go
@@ -16,6 +16,7 @@ limitations under the License.
 package cosign
 
 import (
+	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"testing"
@@ -188,7 +189,7 @@ func TestGetKeys(t *testing.T) {
 			if err != nil {
 				t.Fatalf("expected no error, got %v", err)
 			}
-			keys, err := trustPolicy.GetKeys("ns")
+			keys, err := trustPolicy.GetKeys(context.Background(), "ns")
 			if (err != nil) != tt.wantErr {
 				t.Fatalf("expected %v, got %v", tt.wantErr, err)
 			}