Fix KV Functional Issues. by g2vinay · Pull Request #2671 · microsoft/mcp

g2vinay · 2026-05-19T18:34:48Z

Resolves https://github.com/microsoft/mcp-pr/issues/413, https://github.com/microsoft/mcp-pr/issues/412, https://github.com/microsoft/mcp-pr/issues/410, https://github.com/microsoft/mcp-pr/issues/409, https://github.com/microsoft/mcp-pr/issues/408

…tingsClient factory, and KeyType description - KV-01: Fix includeManagedKeys filter to be additive (include all keys when true) instead of exclusive; previously true returned only managed keys with no way to list all keys - KV-05/KV-04: Extract CreateSettingsClient factory method so GetVaultSettings uses IHttpClientFactory, applies retry policy, and wires HttpClientTransport for test recording/playback (retryPolicy parameter was previously silently ignored) - KV-10: Expand KeyType option description to list all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM) instead of only RSA and EC - KV-11: Re-throw FormatException as ArgumentException in ImportCertificate so the base handler maps it to HTTP 400 instead of HTTP 500

- Fix ArgumentException message in ImportCertificate to restore 'file path' as a tried option (message previously said 'neither raw PEM nor base64' but the code still tries File.Exists first) - Add test: CertificateImportCommand returns HTTP 400 when service throws ArgumentException for invalid certificate data (KV-11 coverage) - Add test: KeyGetCommand includes Managed=null keys when includeManagedKeys is false, documenting the != true vs == false distinction (KV-01 coverage) - Fix AdminSettingsGetCommandTests env var leak: wrap SetAzureSubscriptionId in try/finally with ClearAzureSubscriptionId to prevent AZURE_SUBSCRIPTION_ID from persisting across tests - Extract subscription-dependent RejectsInvalidArguments case into separate Fact that calls SkipIfDefaultSubscriptionConfigured (fixes pre-existing test failure when az login is active) - Add TestEnvironment.SkipIfDefaultSubscriptionConfigured() helper method for tests that validate missing --subscription but need to skip when a default subscription is available via env var or Azure CLI profile - Add changelog entry for the four KeyVault bug fixes

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes several functional bugs in the Key Vault tool: incorrect key listing filter, missing retry/transport wiring on the HSM settings client, an incomplete key-type description, and improper HTTP status mapping for invalid certificate import data. Tests are updated/added to cover the new behaviors.

Changes:

Corrected ListKeys filter so includeManagedKeys=true returns all keys and =false returns non-managed (including null Managed).
Wired retry policy and HttpClientTransport into the KeyVaultSettingsClient used by GetVaultSettings, and changed invalid certificate-data error to throw ArgumentException (→ HTTP 400) with an updated message.
Expanded the --key-type option description to enumerate all valid key types, plus test updates and a changelog entry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tools/Azure.Mcp.Tools.KeyVault/src/Services/KeyVaultService.cs	Fixed managed-key filter, switched invalid-cert exception to `ArgumentException`, added `CreateSettingsClient` with retry/transport.
tools/Azure.Mcp.Tools.KeyVault/src/Options/KeyVaultOptionDefinitions.cs	Expanded `KeyType` description to list all valid values.
tools/Azure.Mcp.Tools.KeyVault/tests/.../Key/KeyGetCommandTests.cs	Added test documenting null-`Managed` keys included when `includeManagedKeys=false`.
tools/Azure.Mcp.Tools.KeyVault/tests/.../Certificate/CertificateImportCommandTests.cs	Split subscription-missing scenario, updated invalid-data test to expect HTTP 400 / `ArgumentException`.
tools/Azure.Mcp.Tools.KeyVault/tests/.../Admin/AdminSettingsGetCommandTests.cs	Wrapped env-var setup in try/finally to clear `AZURE_SUBSCRIPTION_ID`.
servers/Azure.Mcp.Server/changelog-entries/1778609827495.yaml	New changelog entry describing the fixes.

+    private KeyVaultSettingsClient CreateSettingsClient(Uri hsmUri, Azure.Core.TokenCredential credential, RetryPolicyOptions? retry)
+    {
+        var httpClient = _httpClientFactory.CreateClient();
+        httpClient.BaseAddress = hsmUri;
+        var options = new KeyVaultAdministrationClientOptions();
+        options = ConfigureRetryPolicy(AddDefaultPolicies(options), retry);
+        options.Transport = new Azure.Core.Pipeline.HttpClientTransport(httpClient);
+        return new(hsmUri, credential, options);
+    }


+changes:
+  - section: "Bugs Fixed"
+    description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy and wires the HTTP transport for test recording; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"


    )
    {
-        Description = "The type of key to create (RSA, EC).",
+        Description = "The type of key to create. Valid values: RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM.",


vcolin7 · 2026-05-20T01:40:55Z

        var keys = new List<string>();

-        await foreach (var key in client.GetPropertiesOfKeysAsync(cancellationToken).Where(x => x.Managed == includeManagedKeys))
+        await foreach (var key in client.GetPropertiesOfKeysAsync(cancellationToken).Where(x => includeManagedKeys || x.Managed != true))


alzimmermsft · 2026-05-19T21:04:21Z

@@ -0,0 +1,3 @@
+changes:
+  - section: "Bugs Fixed"
+    description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy and wires the HTTP transport for test recording; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"


Suggested change

description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy and wires the HTTP transport for test recording; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"

description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"

Don't need to talk about testing details in release notes

On top of Alan's suggestion, we actually don't support creating oct and oct-HSM keys (they're exclusive to Managed HSM).

Suggested change

description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy and wires the HTTP transport for test recording; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"

description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy; KeyType option now lists all 4 valid types (RSA, RSA-HSM, EC, EC-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"

vcolin7 · 2026-05-20T01:37:38Z

@@ -0,0 +1,3 @@
+changes:
+  - section: "Bugs Fixed"
+    description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy and wires the HTTP transport for test recording; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"


On top of Alan's suggestion, we actually don't support creating oct and oct-HSM keys (they're exclusive to Managed HSM).

Suggested change

description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy and wires the HTTP transport for test recording; KeyType option now lists all 6 valid types (RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"

description: "Fixed four Key Vault bugs: ListKeys now correctly includes all keys (managed and non-managed) when --include-managed is true instead of returning only managed keys; AdminSettingsGetCommand now applies retry policy; KeyType option now lists all 4 valid types (RSA, RSA-HSM, EC, EC-HSM); ImportCertificate now returns HTTP 400 for invalid certificate data instead of HTTP 500"

vcolin7 · 2026-05-20T01:37:58Z

    )
    {
-        Description = "The type of key to create (RSA, EC).",
+        Description = "The type of key to create. Valid values: RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM.",


Currently, we only support creating keys for Azure Key Vault, not Managed HSM (so no oct and oct-HSM). We can add support for those in a future PR, but right now this description would not match reality.

That said, EC-HSM and RSA-HSM keys can only be created on premium SKU vaults. So maybe we should add a clarification on the keyvault create key tool description. What do you think?

Suggested change

Description = "The type of key to create. Valid values: RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM.",

Description = "The type of key to create. Valid values: RSA, RSA-HSM, EC, EC-HSM",

We should also make sure the tool description matches whatever we decide. It mentions all 6 types currently.

vcolin7 · 2026-05-20T01:40:55Z

        var keys = new List<string>();

-        await foreach (var key in client.GetPropertiesOfKeysAsync(cancellationToken).Where(x => x.Managed == includeManagedKeys))
+        await foreach (var key in client.GetPropertiesOfKeysAsync(cancellationToken).Where(x => includeManagedKeys || x.Managed != true))


g2vinay added 3 commits May 12, 2026 10:57

Merge remote-tracking branch 'upstream/main' into fix-kv-issues

6784fe7

github-project-automation Bot added this to Azure MCP Server May 19, 2026

github-project-automation Bot moved this to Untriaged in Azure MCP Server May 19, 2026

github-actions Bot added the tools-KeyVault label May 19, 2026

g2vinay marked this pull request as ready for review May 19, 2026 20:23

g2vinay requested review from a team, JonathanCrd and vcolin7 as code owners May 19, 2026 20:23

Copilot AI review requested due to automatic review settings May 19, 2026 20:23

g2vinay requested a review from a team as a code owner May 19, 2026 20:23

g2vinay requested review from KarishmaGhiya, anannya03, conniey, srnagar, wbreza and xirzec May 19, 2026 20:23

Copilot AI reviewed May 19, 2026

View reviewed changes

Copilot started reviewing on behalf of g2vinay May 19, 2026 20:41 View session

alzimmermsft reviewed May 19, 2026

View reviewed changes

vcolin7 reviewed May 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix KV Functional Issues.#2671

Fix KV Functional Issues.#2671
g2vinay wants to merge 3 commits into
microsoft:mainfrom
g2vinay:fix-kv-issues

g2vinay commented May 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

vcolin7 May 20, 2026

Uh oh!

alzimmermsft May 19, 2026

Uh oh!

vcolin7 May 20, 2026

Uh oh!

vcolin7 May 20, 2026

Uh oh!

vcolin7 May 20, 2026

Uh oh!

vcolin7 May 20, 2026

Uh oh!

vcolin7 May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	Description = "The type of key to create. Valid values: RSA, RSA-HSM, EC, EC-HSM, oct, oct-HSM.",
	Description = "The type of key to create. Valid values: RSA, RSA-HSM, EC, EC-HSM",

Conversation

g2vinay commented May 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

vcolin7 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

alzimmermsft May 19, 2026

Choose a reason for hiding this comment

Uh oh!

vcolin7 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

vcolin7 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

vcolin7 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

vcolin7 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

vcolin7 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants