feat: support enabled status for kmp keys/certs

[duffney]

Description

What this PR does / why we need it:

This PR adds an Enabled field to the KMPMapyKey struct which allows the provider to store the status of certificates and keys being pulled from the provider into Ratify's KMP. Which is needed to support multiple versions of certificates and keys being stored in the KMP. See discussions and the design doc, here, for more details.

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:

• KMP support for n-versions of certificates\keys #1751
• docs: nVersionCount support for KMP design doc #1831

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

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

• Deployed to AKS for Manual testing

> k describe keymanagementprovider akv
Name:         akv
Namespace:    
Labels:       <none>
Annotations:  <none>
API Version:  config.ratify.deislabs.io/v1beta1
Kind:         KeyManagementProvider
Metadata:
  Creation Timestamp:  2024-10-17T14:56:38Z
  Generation:          1
  Resource Version:    5614657
  UID:                 dbb53be3-1dab-4ad5-be79-f565c3269697
Spec:
  Parameters:
    Certificates:
      Name:     ratify
      Version:  5fe70628ce34425ca1396e88c509d355
      Name:     ratify
      Version:  0ff373a9259c4578a247cfd7861a8805
    Client ID:  38f733d4-05f9-41b3-b8e1-896376ba52d6
    Keys:
      Name:          issue1751
    Tenant ID:       b4c72be8-cae1-4584-be77-62b1e94ad0dc
    Vault URI:       https://example.vault.azure.net/
  Refresh Interval:  1m
  Type:              azurekeyvault
Status:
  Issuccess:        true
  Lastfetchedtime:  2024-10-17T18:56:50Z
  Properties:
    Certificates:
      Enabled:         false
      Last Refreshed:  2024-10-17T18:56:50Z
      Name:            ratify
      Version:         5fe70628ce34425ca1396e88c509d355
      Enabled:         true
      Last Refreshed:  2024-10-17T18:56:50Z
      Name:            ratify
      Version:         0ff373a9259c4578a247cfd7861a8805
    Keys:
      Enabled:         false
      Last Refreshed:  2024-10-17T18:56:50Z
      Name:            issue1751
      Version:         
Events:                <none>

> k logs deployment/ratify -n gatekeeper-system
time=2024-10-17T18:56:50.402944925Z level=info msg=reconciling cluster key management provider 'akv'
time=2024-10-17T18:56:50.403102625Z level=debug msg=vaultURI https://example.vault.azure.net/ component-type=keyManagementProvider go.version=go1.22.8
time=2024-10-17T18:56:50.495968077Z level=debug msg=fetching secret from key vault, certName ratify,  keyvault https://example.vault.azure.net/ component-type=keyManagementProvider go.version=go1.22.8
time=2024-10-17T18:56:50.525906097Z level=debug msg=fetching secret from key vault, certName ratify,  keyvault https://example.vault.azure.net/ component-type=keyManagementProvider go.version=go1.22.8
time=2024-10-17T18:56:50.578216358Z level=debug msg=azurekeyvault certprovider getCertsFromSecretBundle: 1 certificates parsed, Certificate 'ratify', version '0ff373a9259c4578a247cfd7861a8805' component-type=keyManagementProvider go.version=go1.22.8
time=2024-10-17T18:56:50.578267858Z level=debug msg=fetching key from key vault, keyName issue1751,  keyvault https://example.vault.azure.net/ component-type=keyManagementProvider go.version=go1.22.8
time=2024-10-17T18:56:50.595643612Z level=info msg=2 certificate(s) & 1 key(s) fetched for key management provider akv
time=2024-10-17T18:56:50.595670912Z level=info msg=Reconciled KeyManagementProviderintervalDuration1m0s

Unit test currently do not fully test the GetCertificate and GetKeys methods of KMP providers, but can be added if desired to the PR or a follow up PR. Also waiting to see if the azidentiy work will be done before adding unit tests because that will likely change the client that will need to be mocked to fully tests the methods.

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 91.89189% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...kg/keymanagementprovider/azurekeyvault/provider.go	89.28%	6 Missing ⚠️

Files with missing lines	Coverage Δ
...lusterresource/keymanagementprovider_controller.go	92.17% <100.00%> (-1.95%)	⬇️
...espaceresource/keymanagementprovider_controller.go	92.03% <100.00%> (-2.02%)	⬇️
pkg/controllers/utils/kmp.go	100.00% <100.00%> (ø)
pkg/keymanagementprovider/keymanagementprovider.go	93.81% <100.00%> (-0.39%)	⬇️
...kg/keymanagementprovider/azurekeyvault/provider.go	84.84% <89.28%> (+9.84%)	⬆️

... and 126 files with indirect coverage changes

[akashsinghal]

The reported metric is for the duration of the keyvault fetch operation. We should move the metric reporting to be isolated to that operation above.

[duffney]

Ah, I see that now. Now that there are two calls to AKV I can define the startTime sooner to capture the first call then reset the value for the second call.

[akashsinghal]

Are we making two calls now to Keyvault? One for an initial GetCertificates and then another for a GetSecret?

[duffney]

That's correct. Instead of attempting to trap a specific error from GetSecrets I opted to use GetCertificates to get the enabled status of the certificate to determine how to add the entry to the KMP.

[akashsinghal]

Oh ok. I see. I'm concerned with the network overhead and how that will affect upstream throttling quotas for AKV. What is the value of doing the existence check first and then calling GetSecret. Is it primarily so the disabled certificate content is not returned as part of the response from KeyVault?

[duffney]

That's correct, with GetCertificate I can check the status of enabled without returning any of the secret data. It also avoids having to trap a particular error from AKV when the cert is disabled when GetSecret is called.

That said, I'm open to suggestions. :)

[binbin-li]

If we can get all required info in single request, I would vote for just calling the GetSecret.

[duffney]

I think we can get away with a single call to GetSecret if we trap the 403 response and continue on instead of erroring out. The only draw back is that means we have to assume that 403 means the certificate is disabled, which is fairly safe imo.

secretResp, err := secretClient.GetSecret(context.TODO(), certName, version, nil)
	if err != nil {
		// Check if the error is a ResponseError from Azure SDK
		if respErr, ok := err.(*azcore.ResponseError); ok {
			// Handle specific status codes
			if respErr.StatusCode == 403 {
				fmt.Printf("Certificate '%s' is disabled or inaccessible; marking as disabled and continuing.\n", certName)
				return
			}
		}
		// For other errors, log and exit
		log.Fatal(err)
	}

My only concern with just using GetSecret is if the call fails we have no information about the secret beyond what's provided in the configuration. So in the instance that a user defines a cert or key without a version we won't be able to update the status with the version of disabled certificates.

Another option is to only make a GetCertificate call when a cert is disabled to gather this info. Which switches the logic from always doing 2x calls to only sometimes doing 2x calls.

[duffney]

I've got this working in another branch, with GetCertificate only being called if GetSecret returns a 403 error indicating that the certificate is disabled. Curious to get your thoughts on the contains("403") logic and if that's sound enough. :)

[binbin-li]

I feel we also need to check if the root cause is disabled secret since 403 also indicates a permission error. Not sure if this post will help: https://stackoverflow.com/questions/71184772/handling-a-disabled-azure-key-vault-secret-using-go-azure-sdk

[duffney]

It's not pretty, but I got this working and it's a lot more specific. Great SO post btw @binbin-li that was super helpful, thank you!

		if err != nil {
			// certificate is disabled, remove it from the map
			if de, ok := err.(autorest.DetailedError); ok {

				if re, ok := de.Original.(*azure.RequestError); ok {

					if re.ServiceError.Code == "SecretDisabled" {
						certBundle, err := s.kvClient.GetCertificate(ctx, s.vaultURI, keyVaultCert.Name, keyVaultCert.Version)
						if err != nil {
							return nil, nil, fmt.Errorf("failed to get certificate objectName:%s, objectVersion:%s, error: %w", keyVaultCert.Name, keyVaultCert.Version, err)
						}

						keyVaultCert.Version = getObjectVersion(*certBundle.Kid)
						isEnabled := *certBundle.Attributes.Enabled
						lastRefreshed := startTime.Format(time.RFC3339)
						certProperty := getStatusProperty(keyVaultCert.Name, keyVaultCert.Version, strconv.FormatBool(isEnabled), lastRefreshed)
						certsStatus = append(certsStatus, certProperty)
						mapKey := keymanagementprovider.KMPMapKey{Name: keyVaultCert.Name, Version: keyVaultCert.Version, Enabled: isEnabled}
						keymanagementprovider.DeleteCertificateFromMap(s.resource, mapKey)
						continue
					}
				}
			}

			return nil, nil, fmt.Errorf("failed to get secret objectName:%s, objectVersion:%s, error: %w", keyVaultCert.Name, keyVaultCert.Version, err)
		}

[duffney]

@binbin-li how do you feel about GetCertificate being called when it's disabled to get the version info from azkv? To me, it felt like a poor user experience if the disabled certs versions are absent.

[akashsinghal]

where is the certificate value (without chain) set in certsMap?

[duffney]

When the certificate is disabled, the certificate isn't stored at all. My thinking was that this would prevent the FlattenKMPMap function from including any disabled certificates. Self-signed certs for example could be used to verify if that cert was passed to the verifier because it doesn't have any cert chain beyond itself. That said, now that there is an enabled field it's possible to update the Flatten function to look for only enabled certs. Your comment does bring up a good question, what value should be used for disabled certs?

QQ: by certificate value, you're meaning the cert in x509 format?

[akashsinghal]

I didn't realize this was for the disabled case. My bad. I should've looked at the if block condition above. Can you remind me if we plan to show if a certificate is disabled at all in status? I'm trying to understand if the operation on L165 is to clear map value but still have the key available to signify it is disabled.

[duffney]

Not a problem. :) And yes, the status is now shown in the KMP status. Here's an example:

 Certificates:
      Enabled:         true
      Last Refreshed:  2024-10-21T20:27:23Z
      Name:            ratify
      Version:         0ff373a9259c4578a247cfd7861a8804

L165 is meant to clear the value so the disabled certificate data so it's not stored in the KMP but the key remains. The key values are then used to populate the status.

Technically, speaking I don't think there is any reason the key need to remain in the map either, other than to match the status that's being reflected to the user.

[binbin-li]

After looking into the way we use certsMap, seems we can just remove L165 since the disabled cert will not be used anywhere. Correct me if I'm wrong.

[duffney]

@akashsinghal what do you think about not storing disabled cert entries in the KMP cache? Then if a cert is found to be disabled it's reported in the status but removed from the cache.

[akashsinghal]

Sure that should work too because the status will always get updated alongside an upstream KV request fetch

[duffney]

@akashsinghal @binbin-li I got this working in a separate branch by adding a few functions to the KMP to delete entries from a resource in cache. This takes care of the edge case where there is only one certificate or key stored in the cache that was once enabled and is now disabled. I can either merge it into this branch or submit it as PR once this one makes it through. Lmk which is prefered. :)

[binbin-li]

I would prefer merging into this PR as we already have context on it.

[duffney]

Changes merged into this PR. :)

[binbin-li]

If we can get all required info in single request, I would vote for just calling the GetSecret.

[binbin-li]

nit: I would recommend to define a bool var for enabled.

[duffney]

That can certainly be done, I'll get that refactored.

[binbin-li]

After looking into the way we use certsMap, seems we can just remove L165 since the disabled cert will not be used anywhere. Correct me if I'm wrong.

[duffney]

@akashsinghal @binbin-li @junczhu did you notice the changes I pushed that adds an interface to the azkv provider to improve test coverage? Lmk if you've got any suggestion on those changes.

[binbin-li]

@akashsinghal @binbin-li @junczhu did you notice the changes I pushed that adds an interface to the azkv provider to improve test coverage? Lmk if you've got any suggestion on those changes.

I like the refactoring, it definitely makes the unit test easier.

[binbin-li]

lgtm once all nitpick comments resolved.

[duffney]

lgtm once all nitpick comments resolved.

@binbin-li, I think I got all the nits implemented. Please lmk if I missed any.

[shahramk64]

Both kvClient and kv.BaseClient rely on autorest (see https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/services/keyvault/v7.1/keyvault#BaseClient) which is what we need to avoid as it's deprecated. I have refactored the code to replace whatever depends on autorest with azidentity (see #1904 ).
In a nutshell, instead of "github.com/Azure/azure-sdk-for-go/services/keyvault/v7.1/keyvault", we can use azidentity with azcertificates, azsecrets, and azkeys. They abstract away the process of getting AAD access token and authenticating with AKV. Currently in my PR I have implemented the workload identity scenario (which is the scenario for AKS), but it can be extended to other auth types like managed identity and CLI using changedIdentityCredential.
@susanshi @duffney We can refactor this PR to use azidentity. Let me know if you need this to be done on this PR and I am happy to help. Otherwise, we can merge this PR first, and then resolve the conflicts in my PR.

[susanshi]

thanks @shahramk64 , given this PR has been open for a while, I would recommend complete this first, we want to avoid further expansion more in scope :)

[shahramk64]

I agree. Let's merge this PR once it's ready, I'll resolve the conflicts with my PR after that.

[akashsinghal]

Is this value exposed to the user for configuration? What happens when they set a value to resource field? I see that this field is set in the controller. But I'm wondering why we need to JSON serialize this field if it's not exposed as a configurable user field?

[duffney]

Good point. The value doesn't need to be exposed. And there's no reason to JSON serialize it.

If the resource isn't passed into the create method via the kmProviderConfig, where would you suggesting passing it in? Perhaps, passing it into factory.CreateKeyManagementProviderFromConfig would work.

[akashsinghal]

Ok I see the issue. The config struct is currently the only place to pass the resource to the AKV KMP in the Create method. And we need to JSON serialize since the whole config is Marshalled and then Unmarshalled into the type.

[akashsinghal]

Perhaps, passing it into factory.CreateKeyManagementProviderFromConfig would work.

This would in turn require updating the KMP factory API 'Create' to incorporate another parameter for resource. IN the future if we need to pass even more non config info to the each KMP, adding parameters to the API is not extensible. I think this is a larger discussion not just specific to this PR. For now, let's just proceed as is.

[binbin-li]

lgtm once @akashsinghal 's comment is resolved.

[akashsinghal]

LGTM. Thanks for all the work here!

[shahramk64]

Should this be Enabled instead?

[duffney]

Good catch @shahramk64! Yes, it should be.