-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[DO NOT MERGE][Keyvault] Filter out certificates from Get-AzKeyVaultSecret and Get-AzKeyVaultKey
#29015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[DO NOT MERGE][Keyvault] Filter out certificates from Get-AzKeyVaultSecret and Get-AzKeyVaultKey
#29015
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
|
Get-AzKeyVaultSecret and Get-AzKeyVaultKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds filtering to the Get-AzKeyVaultKey and Get-AzKeyVaultSecret cmdlets to exclude certificate-managed keys and secrets from their results. This aligns PowerShell behavior with Azure CLI, which already filters out these items. When a certificate is imported into Azure Key Vault, it automatically creates both a key and a secret with the same name and marks them as managed by the certificate. This change ensures that these automatically-created items don't appear in key and secret listings.
Key changes:
- Added filtering logic based on the
Managedproperty in both Track2 and legacy client implementations - Filters are applied to both list operations (GetKeys/GetSecrets) and version listing operations (GetKeyVersions/GetSecretVersions)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/KeyVault/KeyVault/Track2Models/Track2VaultClient.cs | Added filtering to exclude certificate-managed keys in GetKeys and GetKeyVersions methods using continue statements when Managed property is true |
| src/KeyVault/KeyVault/Models/Client/KeyVaultDataServiceClient.cs | Added LINQ Where clause to filter certificate-managed secrets in GetSecrets and GetSecretVersions methods |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
This reverts commit 74a1c36.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/KeyVault/KeyVault/Track2Models/Track2VaultClient.cs:152
- The filtering logic for Managed HSM keys is missing. The Track2HsmClient.GetKeys() and Track2HsmClient.GetKeyAllVersions() methods in Track2HsmClient.cs should also filter out managed (certificate-backed) keys, similar to how regular vault keys are filtered here. Certificate-backed keys exist in Managed HSMs as well, and for consistency with Azure CLI and the regular vault behavior, they should be excluded from these list operations.
private IEnumerable<PSKeyVaultKeyIdentityItem> GetKeys(KeyClient client)
{
var results = new List<PSKeyVaultKeyIdentityItem>();
var allKeys = client.GetPropertiesOfKeys();
foreach (var keyProperties in allKeys)
{
if (keyProperties.Managed == true)
{
continue;
}
results.Add(new PSKeyVaultKeyIdentityItem(keyProperties, _vaultUriHelper, false));
}
return results;
}
| result.Where((secretItem) => secretItem.Managed != true) | ||
| .Select((secretItem) => new PSKeyVaultSecretIdentityItem(secretItem, this.vaultUriHelper)); |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider filtering deleted secrets as well. The GetDeletedSecrets() method should also filter out certificate-backed secrets (where Managed == true) for consistency. When a certificate is deleted, its corresponding secret is also deleted, and it should remain hidden from the deleted secrets list. Apply the same filtering pattern used here for GetSecrets() and GetSecretVersions().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: need to discuss if we should actually implement this on deleted secrets, azure-cli does not filter on this property for deleted keys/secrets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| $vaultName = 'danielKV7103' | ||
| . "$PSScriptRoot\..\Scripts\Common.ps1" |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded vault name 'danielKV7103' should not be committed to the repository. This appears to be a personal test vault. Following the pattern in other test files like Certificate.Tests.ps1 and Key.Tests.ps1, you should either use a placeholder comment or use a vault name that's clearly for testing purposes. Consider using a BeforeAll block to set this value as well, consistent with Certificate.Tests.ps1.
| $vaultName = 'danielKV7103' | |
| . "$PSScriptRoot\..\Scripts\Common.ps1" | |
| . "$PSScriptRoot\..\Scripts\Common.ps1" | |
| $vaultName = Get-KeyVaultName |
| $debugModulePath = "$PSScriptRoot\..\..\..\..\artifacts\Debug\Az.KeyVault\Az.KeyVault.psd1" | ||
| Import-Module $debugModulePath -Force | ||
|
|
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded debug module path should not be included in committed tests. Other test files in the PesterTests directory do not import modules explicitly at the file level. This import should either be removed or handled through the test infrastructure. Additionally, the path uses a relative reference that assumes a specific build artifact location which may not be portable across different test environments.
| $debugModulePath = "$PSScriptRoot\..\..\..\..\artifacts\Debug\Az.KeyVault\Az.KeyVault.psd1" | |
| Import-Module $debugModulePath -Force |
|
|
||
| Start-Sleep -Seconds 30 | ||
| $cert = Get-AzKeyVaultCertificate -VaultName $vaultName -Name $certName | ||
| $cert | Should Not BeNullOrEmpty |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test uses outdated Pester syntax without the dash separator. Following the established convention in this codebase (see ManagedHsmDataPlaneTests.Tests.ps1 and MhsmKey.Tests.ps1), this should use modern Pester syntax with 'Should -Not -BeNullOrEmpty'. The same issue applies to lines 22, 27, 30, 47, 52, and 55.
| $policy = New-AzKeyVaultCertificatePolicy -SecretContentType "application/x-pkcs12" -SubjectName "CN=test.contoso.com" -IssuerName Self -ValidityInMonths 6 | ||
| $certOp = Add-AzKeyVaultCertificate -VaultName $vaultName -Name $certName -CertificatePolicy $policy | ||
|
|
||
| Start-Sleep -Seconds 30 |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 30-second sleep duration may not be sufficient for certificate creation to complete in all environments. Azure Key Vault certificate creation is asynchronous and can take longer depending on load. Consider either increasing the sleep duration to 60 seconds, or implementing a polling mechanism that waits for the certificate operation to complete (checking $certOp.Status until it's "completed"). The same issue exists on line 43.
Description
Fixes #26217
This PR filters out secrets or keys by the
Managedproperty, to prevent certificates showing on theGetcmdlets for Keys and Secrets.This matches the filtering that
azure-clidoes.Note on included tests: There was some trouble creating scenario tests for this functionality, this is apparently a known limitation of scenario tests in the certificates domain. I have included pester tests to describe/manually test desired functionality.
Note: Need to discuss if we would classify this as a breaking change, it doesn't seem to fit cleanly into any breaking change definition in the guide. But it will do additional filtering to results that wasn't there before, and may confuse users who started relying on incumbent behaviour of affected cmdlets.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.