feat: refactor certStore to support multi-tenancy

charts/ratify/templates/akv-key-management-provider.yaml

@@ -1,6 +1,6 @@
 {{- if or .Values.azurekeyvault.enabled .Values.akvCertConfig.enabled }}
 apiVersion: config.ratify.deislabs.io/v1beta1
-kind: NamespacedKeyManagementProvider
+kind: KeyManagementProvider
 metadata:
   name: kmprovider-akv
   annotations:

charts/ratify/templates/inline-key-management-provider.yaml

@@ -2,7 +2,7 @@
 ---
 {{- if .Values.notationCert }} 
 apiVersion: config.ratify.deislabs.io/v1beta1
-kind: NamespacedKeyManagementProvider
+kind: KeyManagementProvider
 metadata:
   name: {{$fullname}}-notation-inline-cert
   annotations:
@@ -17,7 +17,7 @@ spec:
 ---
 {{-  range $i, $cert := .Values.notationCerts }}
 apiVersion: config.ratify.deislabs.io/v1beta1
-kind: NamespacedKeyManagementProvider
+kind: KeyManagementProvider
 metadata:
   name: {{$fullname}}-notation-inline-cert-{{$i}}
   annotations:
@@ -32,7 +32,7 @@ spec:
 ---
 {{-  range $i, $key := .Values.cosignKeys }}
 apiVersion: config.ratify.deislabs.io/v1beta1
-kind: NamespacedKeyManagementProvider
+kind: KeyManagementProvider
 metadata:
   name: {{$fullname}}-cosign-inline-key-{{$i}}
   annotations:

pkg/controllers/certificatestore_controller.go → pkg/controllers/namespaceresource/certificatestore_controller.go

@@ -11,7 +11,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package controllers
+package namespaceresource
 
 import (
 	"context"
@@ -23,6 +23,7 @@ import (
 	"github.com/deislabs/ratify/pkg/certificateprovider"
 	_ "github.com/deislabs/ratify/pkg/certificateprovider/azurekeyvault" // register azure keyvault certificate provider
 	_ "github.com/deislabs/ratify/pkg/certificateprovider/inline"        // register inline certificate provider
+	"github.com/deislabs/ratify/pkg/controllers"
 	"github.com/deislabs/ratify/pkg/utils"
 
 	"github.com/sirupsen/logrus"
@@ -63,8 +64,7 @@ func (r *CertificateStoreReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	if err := r.Get(ctx, req.NamespacedName, &certStore); err != nil {
 		if apierrors.IsNotFound(err) {
 			logger.Infof("deletion detected, removing certificate store %v", resource)
-			// TODO: pass the actual namespace once multi-tenancy is supported.
-			NamespacedCertStores.DeleteStore(constants.EmptyNamespace, resource)
+			controllers.NamespacedCertStores.DeleteStore(resource)
 		} else {
 			logger.Error(err, "unable to fetch certificate store")
 		}
@@ -95,8 +95,7 @@ func (r *CertificateStoreReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		return ctrl.Result{}, fmt.Errorf("Error fetching certificates in store %v with %v provider, error: %w", resource, certStore.Spec.Provider, err)
 	}
 
-	// TODO: pass the actual namespace once multi-tenancy is supported.
-	NamespacedCertStores.AddStore(constants.EmptyNamespace, resource, certificates)
+	controllers.NamespacedCertStores.AddStore(resource, certificates)
 	isFetchSuccessful = true
 	emptyErrorString := ""
 	writeCertStoreStatus(ctx, r, certStore, logger, isFetchSuccessful, emptyErrorString, lastFetchedTime, certAttributes)
@@ -148,8 +147,8 @@ func writeCertStoreStatus(ctx context.Context, r *CertificateStoreReconciler, ce
 func updateErrorStatus(certStore *configv1beta1.CertificateStore, errorString string, operationTime *metav1.Time) {
 	// truncate brief error string to maxBriefErrLength
 	briefErr := errorString
-	if len(errorString) > maxBriefErrLength {
-		briefErr = fmt.Sprintf("%s...", errorString[:maxBriefErrLength])
+	if len(errorString) > constants.MaxBriefErrLength {
+		briefErr = fmt.Sprintf("%s...", errorString[:constants.MaxBriefErrLength])
 	}
 	certStore.Status.IsSuccess = false
 	certStore.Status.Error = errorString

pkg/controllers/certificatestore_controller_test.go → pkg/controllers/namespaceresource/certificatestore_controller_test.go

@@ -13,7 +13,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package controllers
+package namespaceresource
 
 import (
 	"fmt"

pkg/controllers/verifier_controller.go

@@ -19,13 +19,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"os"
 
 	configv1beta1 "github.com/deislabs/ratify/api/v1beta1"
 	"github.com/deislabs/ratify/config"
-	re "github.com/deislabs/ratify/errors"
 	"github.com/deislabs/ratify/internal/constants"
-	"github.com/deislabs/ratify/pkg/utils"
 	vc "github.com/deislabs/ratify/pkg/verifier/config"
 	vf "github.com/deislabs/ratify/pkg/verifier/factory"
 	"github.com/deislabs/ratify/pkg/verifier/types"
@@ -76,13 +73,7 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	namespace, err := getCertStoreNamespace(req.Namespace)
-	if err != nil {
-		verifierLogger.Error(err, "unable to get default namespace for certstore specified in verifier crd")
-		return ctrl.Result{}, err
-	}
-
-	if err = verifierAddOrReplace(verifier.Spec, resource, namespace); err != nil {
+	if err := verifierAddOrReplace(verifier.Spec, resource, constants.EmptyNamespace); err != nil {
 		verifierLogger.Error(err, "unable to create verifier from verifier crd")
 		writeVerifierStatus(ctx, r, &verifier, verifierLogger, false, err.Error())
 		return ctrl.Result{}, err
@@ -152,23 +143,6 @@ func (r *VerifierReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-// Historically certStore defined in trust policy only contains name which means the CertStore cannot be uniquely identified
-// If verifierNamespace is not empty, this method returns the default cert store namespace else returns the ratify deployed namespace
-func getCertStoreNamespace(verifierNamespace string) (string, error) {
-	// first, check if we can use the verifier namespace as the cert store namespace
-	if verifierNamespace != "" {
-		return verifierNamespace, nil
-	}
-
-	// next, return the ratify deployed namespace
-	ns, found := os.LookupEnv(utils.RatifyNamespaceEnvVar)
-	if !found {
-		return "", re.ErrorCodeEnvNotSet.WithComponentType(re.Verifier).WithDetail(fmt.Sprintf("environment variable %s not set", utils.RatifyNamespaceEnvVar))
-	}
-
-	return ns, nil
-}
-
 func writeVerifierStatus(ctx context.Context, r client.StatusClient, verifier *configv1beta1.Verifier, logger *logrus.Entry, isSuccess bool, errorString string) {
 	if isSuccess {
 		verifier.Status.IsSuccess = true

pkg/controllers/verifier_controller_test.go

@@ -212,31 +212,6 @@ func TestWriteVerifierStatus(t *testing.T) {
 	}
 }
 
-func TestGetCertStoreNamespace(t *testing.T) {
-	// error scenario, everything is empty, expect error
-	_, err := getCertStoreNamespace("")
-	if err.Error() == "environment variable" {
-		t.Fatalf("env not set should trigger an error")
-	}
-
-	ratifyDeployedNamespace := "sample"
-	os.Setenv(utils.RatifyNamespaceEnvVar, ratifyDeployedNamespace)
-	defer os.Unsetenv(utils.RatifyNamespaceEnvVar)
-
-	// scenario1, when default namespace is provided, then we should expect default
-	verifierNamespace := "verifierNamespace"
-	ns, _ := getCertStoreNamespace(verifierNamespace)
-	if ns != verifierNamespace {
-		t.Fatalf("default namespace expected")
-	}
-
-	// scenario2, default is empty, should return ratify installed namespace
-	ns, _ = getCertStoreNamespace("")
-	if ns != ratifyDeployedNamespace {
-		t.Fatalf("default namespace expected")
-	}
-}
-
 func resetVerifierMap() {
 	NamespacedVerifiers = verifiers.NewActiveVerifiers()
 }

pkg/customresources/certificatestores/api.go

@@ -13,19 +13,19 @@ limitations under the License.
 
 package certificatestores
 
-import "crypto/x509"
+import (
+	"context"
+	"crypto/x509"
+)
 
 // CertStoreManager is an interface that defines the methods for managing certificate stores across different scopes.
 type CertStoreManager interface {
-	// GetCertStores returns certificates for the given scope.
-	GetCertStores(scope string) map[string][]*x509.Certificate
+	// GetCertsFromStore returns certificates from the given certificate store.
+	GetCertsFromStore(ctx context.Context, storeName string) []*x509.Certificate
 
-	// AddStore adds the given certificate under the given scope.
-	AddStore(scope, storeName string, cert []*x509.Certificate)
+	// AddStore adds the given certificate.
+	AddStore(storeName string, cert []*x509.Certificate)
 
 	// DeleteStore deletes the certificate from the given scope.
-	DeleteStore(scope, storeName string)
-
-	// IsEmpty returns true if there are no certificates.
-	IsEmpty() bool
+	DeleteStore(storeName string)
 }

pkg/customresources/certificatestores/certificatestores.go

@@ -14,72 +14,82 @@ limitations under the License.
 package certificatestores
 
 import (
+	"context"
 	"crypto/x509"
+	"os"
+	"strings"
+	"sync"
 
 	"github.com/deislabs/ratify/internal/constants"
+	ctxUtils "github.com/deislabs/ratify/internal/context"
+	"github.com/deislabs/ratify/pkg/utils"
+	vu "github.com/deislabs/ratify/pkg/verifier/utils"
 )
 
+const defaultNamespace = "default"
+
 // ActiveCertStores implements the CertStoreManager interface
 type ActiveCertStores struct {
-	// TODO: Implement concurrent safety using sync.Map
-	// The structure of the map is as follows:
-	// The first level maps from scope to certificate stores.
-	// The second level maps from certificate store name to certificates.
+	// scopedCertStores is mapping from cert store name to certificate list.
 	// The certificate store name is prefixed with the namespace.
 	// Example:
 	// {
-	//   "namespace1": {
-	//     "namespace1/store1": []*x509.Certificate,
-	//     "namespace1/store2": []*x509.Certificate
-	//   },
-	//   "namespace2": {
-	//     "namespace2/store1": []*x509.Certificate,
-	//     "namespace2/store2": []*x509.Certificate
-	//   }
+	//   "namespace1/store1": []*x509.Certificate,
+	//   "namespace2/store2": []*x509.Certificate
 	// }
-	// Note: Scope is utilized for organizing and isolating cert stores. In a Kubernetes (K8s) environment, the scope can be either a namespace or an empty string ("") for cluster-wide cert stores.
-	ScopedCertStores map[string]map[string][]*x509.Certificate
+	// Note: The namespace "default" is reserved for cluster-wide scenario.
+	scopedCertStores sync.Map
 }
 
 func NewActiveCertStores() CertStoreManager {
-	return &ActiveCertStores{
-		ScopedCertStores: make(map[string]map[string][]*x509.Certificate),
-	}
+	return &ActiveCertStores{}
 }
 
 // GetCertStores fulfills the CertStoreManager interface.
-// It returns a list of cert stores for the given scope. If no cert stores are found for the given scope, it returns cluster-wide cert stores.
-// TODO: Current implementation always fetches cluster-wide cert stores. Will support actual namespaced certStores in future.
-func (c *ActiveCertStores) GetCertStores(_ string) map[string][]*x509.Certificate {
-	return c.ScopedCertStores[constants.EmptyNamespace]
+// It returns a list of certificates in the given store.
+func (c *ActiveCertStores) GetCertsFromStore(ctx context.Context, storeName string) []*x509.Certificate {
+	storeName = prependNamespaceToStoreName(storeName)
+	if !isCompatibleNamespace(ctx, storeName) {
+		return []*x509.Certificate{}
+	}
+	if certs, ok := c.scopedCertStores.Load(storeName); ok {
+		return certs.([]*x509.Certificate)
+	}
+	return []*x509.Certificate{}
 }
 
 // AddStore fulfills the CertStoreManager interface.
-// It adds the given certificate under the given scope.
-// TODO: Current implementation always adds the given certificate to cluster-wide cert store. Will support actual namespaced certStores in future.
-func (c *ActiveCertStores) AddStore(_, storeName string, certs []*x509.Certificate) {
-	scope := constants.EmptyNamespace
-	if c.ScopedCertStores[scope] == nil {
-		c.ScopedCertStores[scope] = make(map[string][]*x509.Certificate)
-	}
-	c.ScopedCertStores[scope][storeName] = certs
+// It adds the given certificate under cert store.
+func (c *ActiveCertStores) AddStore(storeName string, cert []*x509.Certificate) {
+	c.scopedCertStores.Store(storeName, cert)
 }
 
 // DeleteStore fulfills the CertStoreManager interface.
-// It deletes the certificate from the given scope.
-// TODO: Current implementation always deletes the cluster-wide cert store. Will support actual namespaced certStores in future.
-func (c *ActiveCertStores) DeleteStore(_, storeName string) {
-	if store, ok := c.ScopedCertStores[constants.EmptyNamespace]; ok {
-		delete(store, storeName)
+// It deletes the given cert store.
+func (c *ActiveCertStores) DeleteStore(storeName string) {
+	c.scopedCertStores.Delete(storeName)
+}
+
+// Namespaced verifiers could access certStores in the same namespace or "default" namespace.
+// Cluster-wide verifier could access all certStores.
+// Note: the cluster-wide behavior is different from KMP as we need to keep the behavior backward compatible.
+func isCompatibleNamespace(ctx context.Context, storeName string) bool {
+	namespace := ctxUtils.GetNamespace(ctx)
+	if namespace == constants.EmptyNamespace {
+		return true
 	}
+	return strings.HasPrefix(storeName, namespace+constants.NamespaceSeperator) || strings.HasPrefix(storeName, defaultNamespace+constants.NamespaceSeperator)
 }
 
-// IsEmpty fulfills the CertStoreManager interface.
-// It returns true if there are no certificates.
-func (c *ActiveCertStores) IsEmpty() bool {
-	count := 0
-	for _, certStores := range c.ScopedCertStores {
-		count += len(certStores)
+// prependNamespaceToStoreName prepends namespace to store name if not already present.
+// If the namespace where Ratify deployed is not set, `default` namespace will be prepended. However, this case should never happen.
+func prependNamespaceToStoreName(storeName string) string {
+	if vu.IsNamespacedNamed(storeName) {
+		return storeName
+	}
+	defaultNS := defaultNamespace
+	if ns, found := os.LookupEnv(utils.RatifyNamespaceEnvVar); found {
+		defaultNS = ns
 	}
-	return count == 0
+	return defaultNS + constants.NamespaceSeperator + storeName
 }