ARO-28729 No preflight validation that Key Vault is accessible for et… - #6487
Yury Nikitenko (YuriyNik) wants to merge 3 commits into
Conversation
|
Hi Yury Nikitenko (@YuriyNik). Thanks for your PR. I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Adds a new backend preflight validation to ensure the Azure Key Vault used for customer-managed etcd KMS encryption exists and is network-accessible, surfacing misconfigurations early during cluster reconciliation.
Changes:
- Introduces
AzureClusterKeyVaultAccessibilityValidationand wires it into backend cluster validation controllers. - Extends the FPA client builder with a Key Vault Vaults client and adds the required Azure SDK dependency.
- Adds unit tests and GoMock interfaces for the Key Vault client.
Reviewed changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
backend/pkg/utils/validationutils/azure_cluster_keyvault_accessibility_validation.go |
New cluster validation that checks Key Vault reachability and network access constraints. |
backend/pkg/utils/validationutils/azure_cluster_keyvault_accessibility_validation_test.go |
Unit tests covering public/private vault scenarios and error propagation. |
backend/pkg/azure/client/keyvault_client.go |
Defines a KeyVaultVaultsClient interface for mocking/abstraction. |
backend/pkg/azure/client/mock_keyvault_client.go |
Generated GoMock for the Key Vault client interface. |
backend/pkg/azure/client/fpa_client_builder.go |
Adds KeyVaultVaultsClient to the FPA builder and constructs the ARM Key Vault client. |
backend/pkg/azure/client/mock_fpa_client_builder.go |
Updates the FPA builder mock to include KeyVaultVaultsClient. |
backend/pkg/app/backend.go |
Wires the new validation controller into the backend controller run loop. |
backend/go.mod / backend/go.sum |
Adds armkeyvault SDK dependency to backend module. |
test-integration/go.mod / test-integration/go.sum |
Adds armkeyvault as an indirect dependency for the integration test module. |
Files not reviewed (2)
- backend/pkg/azure/client/mock_fpa_client_builder.go: Generated file
- backend/pkg/azure/client/mock_keyvault_client.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func makeVaultResponse(publicNetworkAccess string, privateEndpointCount int) armkeyvault.VaultsClientGetResponse { | ||
| var peConnections []*armkeyvault.PrivateEndpointConnectionItem | ||
| for i := range privateEndpointCount { | ||
| peConnections = append(peConnections, &armkeyvault.PrivateEndpointConnectionItem{ | ||
| ID: ptr.To("pe-" + string(rune('0'+i))), | ||
| }) | ||
| } | ||
|
|
||
| return armkeyvault.VaultsClientGetResponse{ | ||
| Vault: armkeyvault.Vault{ | ||
| Name: ptr.To(testKeyVaultName), | ||
| Properties: &armkeyvault.VaultProperties{ | ||
| PublicNetworkAccess: ptr.To(publicNetworkAccess), | ||
| PrivateEndpointConnections: peConnections, | ||
| }, | ||
| }, | ||
| } | ||
| } |
| func validateKeyVaultNetworkAccess(vault armkeyvault.Vault, kmsProfile *api.KmsEncryptionProfile) error { | ||
| if vault.Properties == nil { | ||
| return fmt.Errorf("key vault %q has no properties", *vault.Name) | ||
| } |
| return armcompute.NewUsageClient(subscriptionID, creds, b.options) | ||
| } | ||
|
|
||
| func (b *firstPartyApplicationClientBuilder) KeyVaultVaultsClient(tenantID string, subscriptionID string) (KeyVaultVaultsClient, error) { |
There was a problem hiding this comment.
This is not correct. The FPA does not have access to resources created by the end-user (this is, resources outside of the cluster's managed resource group)
There was a problem hiding this comment.
This also means that whatever identity is used, it will have to have the permissions to do what it needs against the resource. Which means that some new requirements for the end-users regarding permissions addition might be needed, which would require altering pre-existing resources / permissions too
There was a problem hiding this comment.
The implementation no longer uses the FPA.
It now authenticates as the cluster's KMS operator UAMI (ControlPlaneOperators["kms"]) through a dedicated ClusterOperatorIdentityClientBuilder.KeyVaultKeysClient, and calls the Key Vault data plane (azkeys.Client.GetKey) to verify access.
This is the same identity that etcd itself uses for encrypt/decrypt, and the customer already grants it the Key Vault Crypto User role as part of standard cluster setup — so there are no new permission requirements for end-users.
This addresses both points:
- No FPA access to customer resources — we use the cluster's own KMS identity.
- No new permission requirements — that identity already has the needed RBAC.
| return utils.TrackError(fmt.Errorf("failed to get key vault client: %w", err)) | ||
| } | ||
|
|
||
| resp, err := vaultsClient.Get(ctx, cluster.ID.ResourceGroupName, kmsProfile.ActiveKey.VaultName, nil) |
There was a problem hiding this comment.
The resourcegroup name part is not correct: As of now, there's no enforcement that requires the key vault created by the user lives in the same RG than the Cluster's RG.
What makes this even worse: They do not provide the RG name. Only the vault name. However, this requires the resourcegroup name. Starting to require that information would require a breaking change in the API, which makes this non trivial.
An alternative would be to list all the key vaults in the subcription and then filter client-side by the URL. However that is considerably costly as we don't control the number of them, as well as permissions would need to be granted to list (as well as read)
There was a problem hiding this comment.
The new implementation uses the Key Vault data plane API (azkeys.Client.GetKey) instead of the ARM API.
The data plane is addressed by vault DNS name only (https://{vaultName}.vault.azure.net/) — no resource group needed. This also aligns with how etcd itself accesses the vault at runtime.
PS: probably we need to add url as a parameter of configuration not a hardcoded one - i added separate comment about this.
c23dca4 to
d1347fa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 17 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- backend/pkg/azure/client/mock_keyvault_client.go: Generated file
- backend/pkg/azure/client/mock_smi_client_builder.go: Generated file
Suppressed comments (4)
backend/pkg/utils/validationutils/azure_cluster_keyvault_accessibility_validation.go:67
- If the customer-managed encryption type is not KMS, this Key Vault-specific validation is not applicable and should be returned as Skipped (removing the condition) rather than Passed.
if cm.EncryptionType != metadataapi.CustomerManagedEncryptionTypeKMS {
return PassedValidation(coreapi.ControllerConditionReasonAsExpected, "As expected", "Cluster does not use KMS encryption.")
}
backend/pkg/utils/validationutils/azure_cluster_keyvault_accessibility_validation.go:51
- For clusters not using customer-managed etcd KMS encryption, this validation should be reported as "Skipped" (so the ClusterValidationController removes the condition) rather than "Passed"; otherwise the KeyVaultAccessibility condition will appear as successfully validated even though it was not applicable.
This issue also appears on line 65 of the same file.
if cluster.CustomerProperties.Etcd.DataEncryption.KeyManagementMode != metadataapi.EtcdDataEncryptionKeyManagementModeTypeCustomerManaged {
return PassedValidation(coreapi.ControllerConditionReasonAsExpected, "As expected", "Cluster does not use customer-managed KMS encryption.")
}
backend/pkg/utils/validationutils/azure_cluster_keyvault_accessibility_validation.go:90
- This validation currently only attempts a data-plane GetKey call and does not implement the PR's stated behavior (ARM KeyVaultVaultsClient.Get existence/reachability check, and cross-checking the vault's PublicNetworkAccess/private endpoint configuration against the cluster's KeyVaultVisibility). As a result, it can't produce the intended actionable failures for mismatched visibility vs vault networking.
kmsProfile := cm.Kms
kmsIdentityResourceID := cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators[string(internalazure.ClusterOperatorIdentifierKMS)]
if kmsIdentityResourceID == nil {
internalAndUserMsg := fmt.Sprintf("cluster has no %q operator identity configured", internalazure.ClusterOperatorIdentifierKMS)
return FailedValidation("KmsIdentityNotConfigured", internalAndUserMsg, internalAndUserMsg)
}
clusterIdentityURL := cluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL
keysClient, err := a.smiClientBuilder.KeyVaultKeysClient(ctx, clusterIdentityURL, kmsIdentityResourceID, kmsProfile.ActiveKey.VaultName)
backend/pkg/azure/client/smi_client_builder.go:98
- KeyVaultKeysClient currently constructs an azkeys.Client with nil client options, which bypasses the repo's shared azcore client options (retry/telemetry/tracing settings). Consider wiring the existing azCoreARMClientOptions.ClientOptions into azkeys.ClientOptions so Key Vault calls behave consistently with other Azure clients.
func (b *serviceManagedIdentityClientBuilder) KeyVaultKeysClient(ctx context.Context, clusterIdentityURL string, identityResourceID *azcorearm.ResourceID, vaultName string) (KeyVaultKeysClient, error) {
creds, err := b.credentialForIdentity(ctx, clusterIdentityURL, identityResourceID)
if err != nil {
return nil, err
}
vaultURL := fmt.Sprintf("https://%s.vault.azure.net/", vaultName)
return azkeys.NewClient(vaultURL, creds, nil)
}
|
Yury Nikitenko (@YuriyNik) As a general comment can we change the PR description to describe the changes introduce rather than the issue that its trying to solve eg : "Add AzureClusterKeyVaultAccessibilityValidation controller to validate clients KeyVault". WDYT? |
|
|
||
| func (a *AzureClusterKeyVaultAccessibilityValidation) Validate(ctx context.Context, _ *coreapi.Subscription, cluster *coreapi.HCPOpenShiftCluster) ValidationResult { | ||
| if cluster.CustomerProperties.Etcd.DataEncryption.KeyManagementMode != metadataapi.EtcdDataEncryptionKeyManagementModeTypeCustomerManaged { | ||
| return PassedValidation(coreapi.ControllerConditionReasonAsExpected, "As expected", "Cluster does not use customer-managed KMS encryption.") |
There was a problem hiding this comment.
This should be Skipped and not passed
There was a problem hiding this comment.
ok, fixed - azure_cluster_keyvault_accessibility_validation.go:54 — non-customer-managed → SkippedValidation("NotApplicable", ...)
| } | ||
|
|
||
| if cm.EncryptionType != metadataapi.CustomerManagedEncryptionTypeKMS { | ||
| return PassedValidation(coreapi.ControllerConditionReasonAsExpected, "As expected", "Cluster does not use KMS encryption.") |
There was a problem hiding this comment.
This should be Skipped and not Passed
There was a problem hiding this comment.
ok, its fixed - - azure_cluster_keyvault_accessibility_validation.go:64 — non-KMS encryption type → SkippedValidation("NotApplicable", ...)
| kmsIdentityResourceID := cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators[string(internalazure.ClusterOperatorIdentifierKMS)] | ||
| if kmsIdentityResourceID == nil { | ||
| internalAndUserMsg := fmt.Sprintf("cluster has no %q operator identity configured", internalazure.ClusterOperatorIdentifierKMS) | ||
| return FailedValidation("KmsIdentityNotConfigured", internalAndUserMsg, internalAndUserMsg) |
There was a problem hiding this comment.
This seems incorrect. kmsIdentityResourceID should never be nil if the etcd dataencryption has been configured as customer managed with kms encryption
There was a problem hiding this comment.
Additionally, doublecheck if the RP Frontend is validating that. If not, that means we miss the validation there and it should be added. On CS side this validation should be there already (you can also doublecheck)
There was a problem hiding this comment.
Checked internal/admission/ — didn't find validation requiring KMS identity when CustomerManaged+KMS is selected. This check catches that gap. Should I add RP validation in this PR or as follow-up?
There was a problem hiding this comment.
Checked internal/admission/
That's not the only place where validation is performed in the frontend
There was a problem hiding this comment.
I added the missing validation to the RP frontend: validateKmsIdentityRequirement in internal/validation/validate_cluster.go (wired into ValidateCluster). When customer-managed KMS encryption is configured but the kms operator identity is absent, it now returns a field.Required error — so a create/update request can no longer reach the backend in that state. (The earlier check was only in internal/admission/, which isn't the validation path you were referring to.)
Given that guarantee, kmsIdentityResourceID should indeed never be nil here. So instead of a user-facing FailedValidation, the backend now treats a nil as an internal inconsistency and returns UnknownValidation
(InternalError) rather than blaming the user. I kept the guard (rather than dropping it) so a nil can't panic in the credential path downstream.
| } | ||
|
|
||
| return dataplane.GetCredential(b.azCoreARMClientOptions.ClientOptions, resp.ExplicitIdentities[0]) | ||
| vaultURL := fmt.Sprintf("https://%s.vault.azure.net/", vaultName) |
There was a problem hiding this comment.
The KeyVaultKeysClient currently hardcodes the .vault.azure.net suffix, which is Azure Public-only. Sovereign clouds use different suffixes (.vault.usgovcloudapi.net for Gov, .vault.azure.cn for China).
Since ARO-HCP currently only deploys to AzurePublicCloud, is it acceptable to keep this as-is for now (with a TODO/follow-up issue for sovereign cloud support), or should I add a keyVaultDNSSuffix property to the cloud
environment config in this PR?
| // cluster operator identity such as "kms") rather than as the SMI. This | ||
| // is the only credential path that has RBAC access to a customer-owned | ||
| // Key Vault; the FPA and SMI do not. | ||
| KeyVaultKeysClient(ctx context.Context, clusterIdentityURL string, identityResourceID *azcorearm.ResourceID, vaultName string) (KeyVaultKeysClient, error) |
There was a problem hiding this comment.
No this design is not correct. SMIClientBuilder is exclusively used to authenticate as the SMI identity.
Its purpose is to allow you to obtain clients that are authenticated as the SMI identity exclusively.
This is explicitly described in the type documentation:
// ServiceManagedIdentityClientBuilder offers the ability to create Azure clients
// authenticating as the Cluster's Service Managed Identity, which is
// a cluster-scoped identity.
There was a problem hiding this comment.
Agreed — KeyVaultKeysClient didn't belong on ServiceManagedIdentityClientBuilder, which is SMI-exclusive by design.
I moved it to a new dedicated interface, ClusterOperatorIdentityClientBuilder (in operator_identity_client_builder.go), whose purpose is to build clients authenticated as a cluster operator identity (here, the kms UAMI).
ServiceManagedIdentityClientBuilder no longer exposes any Key Vault method and remains SMI-only, consistent with its type documentation.
| ) | ||
| } | ||
|
|
||
| if _, err := keysClient.GetKey(ctx, kmsProfile.ActiveKey.Name, kmsProfile.ActiveKey.Version, nil); err != nil { |
There was a problem hiding this comment.
This is not really verifying whether keyvault is accessible or not. It's verifying whether it can get the key. You can have the keyvault completely accessible but that this returns an error because the key does not exist
There was a problem hiding this comment.
Thanks for flagging this. Here's how it works now:
The check uses the Key Vault data plane (GetKey) via the cluster's KMS operator identity, and the outcome is classified by the error returned:
- 401/403 → KeyVaultAccessDenied (vault not accessible/authorized for the identity)
- 404 → KmsKeyNotFound (vault is accessible, but the key doesn't exist)
- anything else → Unknown (transient/unclassified)
So the "vault reachable but key missing" case you described is reported distinctly as KmsKeyNotFound, not as an accessibility failure. GetKey is used intentionally, since it exercises the same operation etcd performs at
runtime.
There was a problem hiding this comment.
If we focus on the reported issue, they mention that it could happen that the actual visibility configuration changes. This PR and validation is not attempting to check that, which requires the ARM control plane API part. It's attempting to instead perform some kind of API request at the dataplane level to try to do some sort of connectivity verification. It's also doing that because currently the key vault resource id is not available as I mentioned in #6487 (comment), so this is trying to workaround the fact that we don't have the resourcegroup information by using key vault's data plane api and relying on the KMS identity itself (which also comes with additional calls to the MIDataplane service as a consequence) to try to do something at that level.
With that, we end up with some sort of validation that doesn't really validate what was intended originally. Even with that approach, the validation doesn't validate what it currently claims to do (according to the current code doc "Validates that the Azure Key Vault used for customer-managed etcd KMS encryption is reachable by the cluster's KMS operator managed identity -- the same identity")
With the current approach, getting a 404 key not found is treated as the validation failed, which is incorrect. That the key doesn't exist it doesn't mean there's no connectivity, it means that the key doesn't exist.
With the current approach, getting a 401/403 doesn't mean that the keyvault is not accessible at connection level, it means that the identity can't authenticate or doesn't have permissions at the data plane level, that doesn't mean there isn't connectivity.
Aside from that, even when the current approach ends with successful validation, there's no guarantee that there will be connectivity from where it's needed: The validation which runs in backend is occurring from the service cluster, but the actual interaction runs from within the management cluster, so even if there's connectivity from backend level nothing guarantees that the actual workload running in the management cluster side will have connectivity.
There was a problem hiding this comment.
What I am observing from here is that we are trying to implement something without having really done the work of understanding the issue, what is that we really want to check and whether the check is correct given that validations run in another workload than where the actual workloads interact with the keyvault which means that the network configuration, topology, source is different.
The order of events that occurred are:
- We first tried to implement the PR by checking the Key Vault's configuration using the Key Vault's control plane API
- We then noticed that we can't do 1. because for that the key vault's azure resource id is needed and we don't currently have it because it's not passed as part of the cluster's creation payload. Because of this, we tried to workaround it and we changed the original focus from configuration checking to interaction at the data plane level taking the keys endpoint and the provided key to try to do something
Before continuing any further implementation:
Understand what we want to check, and whether doing it provides any value. The original Jira seemed to be about configuration of a specific setting and not actual connectivity. N different settings can impact connectivity. If for some reason we determine we want to check connectivity instead of the config (why? and also, what is that we define as "connectivity") then understand what would be an accurate way to check that (as mentioned the actual workloads don't run in the same cluster than backend), if possible at all, and that it doesn't mix concerns (connectivity vs key existence etc...). Notice also that for example by focusing on "connectivity" you don't really detect configuration drift changes around the key vault visibility, and when focusing on configuration drift changes you don't focus on connectivity (connectivity can change not only based on the configuration of the keyvault visibility). A potential solution could even require changing the API if we determine that it's something that we want to start requiring (with its potential implications like possibly API breaking changes and so on). Don't necessarily discard something because some information not being currently there.
d1347fa to
e8f516c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: YuriyNik The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 25 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- backend/pkg/azure/client/mock_keyvault_client.go: Generated file
- backend/pkg/azure/client/mock_operator_identity_client_builder.go: Generated file
Suppressed comments (1)
backend/pkg/utils/validationutils/azure_cluster_keyvault_accessibility_validation.go:93
a.operatorIdentityClientBuildercan be nil (the constructor allows it, and the Name() test passes nil). If Validate() is ever invoked with a nil builder (miswiring, partial test setup, or future refactors), this will panic. A small guard at the start of Validate() that returnsUnknownValidation(\"InternalError\", ...)when the builder is nil would prevent a controller crash and make failures easier to diagnose.
clusterIdentityURL := cluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL
keysClient, err := a.operatorIdentityClientBuilder.KeyVaultKeysClient(ctx, clusterIdentityURL, kmsIdentityResourceID, kmsProfile.ActiveKey.VaultName)
if err != nil {
return UnknownValidation(
"InternalError",
"Unable to verify Key Vault accessibility.",
fmt.Sprintf("failed to get key vault client: %s", err),
ControllerReportingPolicyTypeError,
)
}
| func (b *clusterOperatorIdentityClientBuilder) KeyVaultKeysClient(ctx context.Context, clusterIdentityURL string, operatorIdentityResourceID *azcorearm.ResourceID, vaultName string) (KeyVaultKeysClient, error) { | ||
| creds, err := credentialForIdentity(ctx, b.fpaMIdataplaneClientBuilder, b.azCoreARMClientOptions.ClientOptions, clusterIdentityURL, operatorIdentityResourceID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| vaultURL := fmt.Sprintf("https://%s.vault.azure.net/", vaultName) | ||
| return azkeys.NewClient(vaultURL, creds, nil) | ||
| } |
| // EncryptionType == KMS. This validation runs on already-validated stored | ||
| // cluster data, so we trust those invariants (consistent with the other | ||
| // backend cluster validations). | ||
| cm := cluster.CustomerProperties.Etcd.DataEncryption.CustomerManaged | ||
| if cm.EncryptionType != metadataapi.CustomerManagedEncryptionTypeKMS { | ||
| return SkippedValidation("NotApplicable", "Cluster does not use KMS encryption.", "Cluster does not use KMS encryption.") | ||
| } | ||
|
|
||
| kmsProfile := cm.Kms |
|
|
||
| // classifyGetKeyError examines the error from GetKey and returns an appropriate | ||
| // ValidationResult based on the error type: | ||
| // - 401/403: Permission denied - the KMS identity lacks access to the Key Vault |
There was a problem hiding this comment.
// - 401/403: Permission denied - the KMS identity lacks access to the Key Vault
This claim is not correct as mentioned in https://github.com/Azure/ARO-HCP/pull/6487/changes#r3796054243.
That you get that doesn't mean that the identity doesn't have access to the key vault. It means that it can't authenticate, or doesn't have permissions to perform a very specific action (get against a specific key)
| // classifyGetKeyError examines the error from GetKey and returns an appropriate | ||
| // ValidationResult based on the error type: | ||
| // - 401/403: Permission denied - the KMS identity lacks access to the Key Vault | ||
| // - 404: Key not found - the specified key does not exist in the vault |
There was a problem hiding this comment.
// - 404: Key not found - the specified key does not exist in the vault
This statement is accurate, but this validation is not about validating whether the key exists or not. Having that as a validation outcome of failure is conceptually wrong. This is explained in detail here https://github.com/Azure/ARO-HCP/pull/6487/changes#r3796054243
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[Link to Jira: https://issues.redhat.com/browse/ARO-28729]
What
Add AzureClusterKeyVaultAccessibilityValidation backend validation controller that validates the customer's Key Vault is accessible to the cluster's KMS operator managed identity. It:
operations.
Also:
Why
There is currently no preflight check that the etcd KMS Key Vault is reachable. Misconfigurations surface late and opaquely rather than being caught early with a clear, actionable error.
[Link to Jira: https://issues.redhat.com/browse/ARO-28729]
Testing
Testing is required for feature completion and tests should be part of the pull
request along with the feature changes.
Describe the testing provided. If you did not add tests, provide a clear
justification.
Special notes for your reviewer
This PR targets the current plain-
error-based validation interface, so it'sself-contained and ready to merge independently. I also have a version
adapted to the new structured
ValidationResult/Outcometype from #6057,kept on a separate local branch — not included here since it depends on
types not yet in
main. I'll follow up with that once #6057 merges.PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.