feat: add key management provider resource

[akashsinghal]

Description

What this PR does / why we need it:

• Adds a new Key Management Provider resource which will supersede the Certificate Store
• This new resource will encompass all key + certificate management from various configured KMS providers.
• Adds an error if Key Mangement Provider is applied while an existing Certificate Store exists on the cluster. Error is in the Ratify logs.
  • It is NOT recommended to use both CertificateStore and KeyManagementProvider resources together.
  • If using helm to upgrade Ratify, users need to delete any existing CertificateStore resources.
  • For self-managed CertificateStore resources, users should migrate to the equivalent KeyManagementProvider.
  • If migration is not possible and both resources must exist together, please make sure to use DIFFERENT names for each resource type.
  • Ratify is configured to prefer KMP resources when a matching CertificateStore with same name is found.
  • All functionality in CertificateStore is present in KMP. Users will be pointed to docs to migrate (TODO). The names of some fields are different.
• Adds e2e tests and UTs

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 #1294

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]

Codecov Report

Attention: Patch coverage is 65.31792% with 120 lines in your changes are missing coverage. Please review.

Project coverage is 64.77%. Comparing base (45c0f81) to head (5da61c3).
Report is 33 commits behind head on main.

Files	Patch %	Lines
...kg/keymanagementprovider/azurekeyvault/provider.go	68.96%	32 Missing and 13 partials ⚠️
...kg/controllers/keymanagementprovider_controller.go	50.61%	40 Missing ⚠️
pkg/keymanagementprovider/azurekeyvault/auth.go	47.36%	18 Missing and 2 partials ⚠️
pkg/manager/manager.go	0.00%	6 Missing ⚠️
pkg/keymanagementprovider/inline/provider.go	82.60%	2 Missing and 2 partials ⚠️
pkg/keymanagementprovider/factory/factory.go	88.88%	1 Missing and 1 partial ⚠️
pkg/keymanagementprovider/keymanagementprovider.go	93.10%	1 Missing and 1 partial ⚠️
pkg/controllers/certificatestore_controller.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
+ Coverage   61.60%   64.77%   +3.16%     
==========================================
  Files          98      104       +6     
  Lines        6248     5402     -846     
==========================================
- Hits         3849     3499     -350     
+ Misses       2066     1544     -522     
- Partials      333      359      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[susanshi]

looks great, left some clarification questions

[susanshi]

since this is a new CR, i m thinking if we can enforce "namespace must of provided". Since kmprovider is a namespace resource, we won't have to guess its namespace if its not specified.

[susanshi]

hmm.. the challenge is when we init the verifier, we don't know if its referencing a cerstore or a kpmprovider. Wondering if there is a way to enforce namespace

[akashsinghal]

Are we sure want to make the user always append namespace in front? I think the only scenario where a user would refere to a KMP outside the verifier's namespace is if they are referring to a clusterred KMP which would be introduced for multitenancy. I see that right now the verifier is cluster scoped. But I also see that there is previous functionality to get the cert store namespace and then append it if not present. https://github.com/deislabs/ratify/blob/8acc7f2091f8dd430d759a7e5505c54ded5b0e56/pkg/controllers/verifier_controller.go#L165

[akashsinghal]

The flow that I see already for certificate store is: 1. check if the resource has a namespace. 2. if not check if the ratify namespace env var is set. 3. when creating the notation verifier, append the namespace to the front of the certstore ref if one is not already provided.

For KMP, should we just follow the same behavior? I added a check in the notation trust store to check both the certificate store's certificate map as well as the KMP equivalent so there shouldn't be any ambiguity as to if its a KMP resource or a legacy Certificate Store.

[susanshi]

Akash and I had a offline discussion and agreed on KMP provider to mirror the CertStore behavior for this PR. We should find another time to discussion the levels of namespace enforcement as a part of Multi tenancy work.

our goal:

• intuitive and deterministic user experience.
• Straightforward application logic.

We want to review the valid combination of we should support and validate. Cluster Verifiers (Cluster certStore, Namespaced certStore ) and Namespaced Verifiers (Cluster certStore, Namespaced certStore ). cc @binbin-li

[susanshi]

Hi@akashsinghal , would you be able to add some test results/ screenshots / logs so we could review what the CRs look like for back compatibility purpose? For example, for existing customers using AKV code path, on ratify upgrade, will they see both CertStore and KMP CR?

From the code, one CR (hopefully they reconcile in a deterministic fashion ), should be in a disabled with a clear msg ( might need doc to clarify this is expected for existing customer).

[akashsinghal]

Hi@akashsinghal , would you be able to add some test results/ screenshots / logs so we could review what the CRs look like for back compatibility purpose? For example, for existing customers using AKV code path, on ratify upgrade, will they see both CertStore and KMP CR?

From the code, one CR (hopefully they reconcile in a deterministic fashion ), should be in a disabled with a clear msg ( might need doc to clarify this is expected for existing customer).

@susanshi, @binbin-li, and I had a discussion offline about this. The goal is to transition users to use the new KeyManagementProvider (KMP) resource without breaking them. New users ideally should be installing KMP instead of CertificateStore. For upgrade scenarios, users would already have an existing CertificateStore resource on a cluster and we cannot force the user to update while causing potential Ratify downtime. Introducing KMP should not be a breaking change initially until v2.0.0. As such, here is the proposed approach:

• All existing CertificateStore support will remain. Implementation wise, CertificateStore will take precedence over KMP if both exist with SAME unique metadata.name.
• The helm chart will be updated to install the KMP resource instead of CertificateStore. If the KMP controller detects the existence of a CertificateStore resource already, an error will be logged showing the conflict however BOTH resources will be operational. Deference will be given to the CertificateStore resource.
• In Ratify v2.0, the CertificateStore will be entirely deprecated and it's implementation/support removed. Once done, the KMP will not perform any check for existence of CertificateStore.

Documentation and TODOs will be added with above behavior

Here's a sample of logs when a user updates to newer version of Ratify from current version:

time=2024-03-20T18:33:02.798193985Z level=error msg=Error: key management provider and certificate store cannot be configured together, Code: KEY_MANAGEMENT_CONFLICT, Plugin Name: gatekeeper-system/kmprovider-akv, Component Type: keyManagementProvider, Detail: certificate store already exists

[akashsinghal]

Hi@akashsinghal , would you be able to add some test results/ screenshots / logs so we could review what the CRs look like for back compatibility purpose? For example, for existing customers using AKV code path, on ratify upgrade, will they see both CertStore and KMP CR?
From the code, one CR (hopefully they reconcile in a deterministic fashion ), should be in a disabled with a clear msg ( might need doc to clarify this is expected for existing customer).

@susanshi, @binbin-li, and I had a discussion offline about this. The goal is to transition users to use the new KeyManagementProvider (KMP) resource without breaking them. New users ideally should be installing KMP instead of CertificateStore. For upgrade scenarios, users would already have an existing CertificateStore resource on a cluster and we cannot force the user to update while causing potential Ratify downtime. Introducing KMP should not be a breaking change initially until v2.0.0. As such, here is the proposed approach:

• All existing CertificateStore support will remain. Implementation wise, CertificateStore will take precedence over KMP if both exist with SAME unique metadata.name.
• The helm chart will be updated to install the KMP resource instead of CertificateStore. If the KMP controller detects the existence of a CertificateStore resource already, an error will be logged showing the conflict however BOTH resources will be operational. Deference will be given to the CertificateStore resource.
• In Ratify v2.0, the CertificateStore will be entirely deprecated and it's implementation/support removed. Once done, the KMP will not perform any check for existence of CertificateStore.

Documentation and TODOs will be added with above behavior

Here's a sample of logs when a user updates to newer version of Ratify from current version:

time=2024-03-20T18:33:02.798193985Z level=error msg=Error: key management provider and certificate store cannot be configured together, Code: KEY_MANAGEMENT_CONFLICT, Plugin Name: gatekeeper-system/kmprovider-akv, Component Type: keyManagementProvider, Detail: certificate store already exists

Community Meeting 3/20/24 discussion:

• We need to prioritize the existing Certificate Store. KMP should be an added feature that existing users can decide to switch to
• It makes sense to have v1.2 helm chart switch to KMP so that new users start using KMP from start
• We need to add documentation, improve error message in the log, and add BREAKING CHANGE guidance in the release notes.
• The plan outlined above originally, will be largely adhered to
• The main point of contention is for users who do not use helm chart for Ratify install/upgrade. If they have an existing CertStore and then attempt to apply a new KMP with the SAME name as the CertStore, then the new KMP is essentially disregarded since notation will give preference to CertStore. This scenario we need to highlight and tell users to preferably delete the old CertStore or use a different named KMP. This name conflict will not affect helm chart upgrades since the new KMP resource will have a different name then the old charts with the CertStore. The notation verifier will accordingly be updated to reference the new KMP name instead so things should just work out of the box.

[susanshi]

I m happy for this PR to merge as this to unblock the series of cosgin work. As long as we don't release, we have time discuss and update the compat story.

[akashsinghal]

@susanshi appreciate another round of review. From your last review: I updated an error message to be more descriptive. I switched the behavior so that now a KMP resource is favored over a CertificateStore resource when both have the same metadata.name. This behavior change is in response to our discussions with @fseldow .