Add CertificateClient and KeyClient support to Aspire.Azure.Security.KeyVault

[james-gould]

Description

Adds the new builder.AddExtendedAzureKeyVaultClient method to provide KeyClient and CertificateClient support. While these are significantly less used than Secrets they still are useful and often required. Users are having to mix the Aspire and Azure.Security libraries together to achieve the functionality currently.

I've extended several large internal applications at work to make use of .NET Aspire and these two clients missing from the new package was a bit frustrating.

An example of what these extensions provide:

builder.AddExtendedAzureKeyVaultClient("keyVault")
       .AddKeyClient()
       .AddCertificateClient();

With included support for Keyed clients too:

builder.AddExtendedKeyedAzureKeyVaultClient("my-secret-client")
       .AddKeyedKeyClient("my-key-client")
       .AddKeyedCertificateClient("my-certificate-client");

The implementation was loosely based off the existing Aspire.OpenAI package which also makes use of a builder pattern. I've also made sure to migrate the namespaces of the extension method classes to Microsoft.Extensions.Hosting as well to avoid needing to introduce more using statements.

I'm aware that secrets are the main use-case for KV which is why these two clients weren't implemented previously, I'm hoping this implementation is both useful to customers/users (myself included) but also conforms to the principle of secrets first.

Fixes issues:

#8099

#3930

Additional dependencies

Adds in the Certificates.CertificateClient and Keys.KeyClient packages from Azure.Security, alongside the existing Secrets.SecretClient dep.

Checklist

• Is this feature complete?
  • Yes. Ready to ship.
• Are you including unit tests for the changes and scenario tests if relevant?
  • Yes
• Did you add public API?
  • No
• Does the change make any security assumptions or guarantees?
  • No
• Does the change require an update in our Aspire docs?
  • No

[james-gould]

Conscious the build analysis is failing. There is a bypass which I won't use personally but the changes are backwards compatible with existing code, the previous tests are all green as well as the new ones. I'm unable to see the full failure list without being in the MS tenancy so I could do with some assistance fixing any issues if they are valid though 👍

[davidfowl]

Those pipelines should be visible to you. Also you made a breaking API change that’s why it’s complaining

[james-gould]

Those pipelines should be visible to you.

I can see the first couple of issues but it's truncated, when I log into Devops to see the others I'm met with:

Selected user account does not exist in tenant 'Microsoft' and cannot access the application '499b84ac-1321-427f-aa17-267ca6975798' in that tenant. The account needs to be added as an external user in the tenant first. Please use a different account.

This is a personal MS account though.

Also you made a breaking API change that’s why it’s complaining

Makes sense, thanks. Do I need to address anything in that case?

[james-gould]

@eerhardt Apologies for the nudge, is there anything outstanding from my end? Appreciate the builds are shouting a bit, nothing actually breaks from the old API but the return type has changed from void so it's not happy. Cheers!

[davidfowl]

Minimally, we won't break the API, we need a different proposal that is non breaking.

[james-gould]

@davidfowl Understood, however this doesn't break the previous behaviour. Updating from the current version to one with this change in would compile and work the same way, the signatures and configuration are the same.

The previous return type was void, so this change wouldn't require a code change, but is being picked up as breaking.

The registration of the SecretClient in the current version still occurs without using the extensions, the namespaces are identical to the current version too. For example this is the redefined entry point, which is functionally identical to the current version except the return type has been changed (the breaking change), but not using the builder is perfectly acceptable:

    public static AzureKeyVaultClientBuilder AddAzureKeyVaultClient(
        this IHostApplicationBuilder builder,
        string connectionName,
        Action<AzureSecurityKeyVaultSettings>? configureSettings = null,
        Action<IAzureClientBuilder<SecretClient, SecretClientOptions>>? configureClientBuilder = null)
    {
        ArgumentNullException.ThrowIfNull(builder);
        ArgumentException.ThrowIfNullOrEmpty(connectionName);

        return new AzureKeyVaultClientBuilder(builder, connectionName, configureSettings)
                   .AddSecretClient(configureClientBuilder);
    }

Underneath it still immediately calls AzureComponent<AzureSecurityKeyVaultSettings, TClient, TClientOptions>().AddClient to register it into the DI container, For example the existing test case below still passes with this change in place, with no changes my end to the test:

// tests/Components/Aspire.Azure.Security.KeyVault.Tests/AspireKeyVaultExtensionsTests.cs

    public void VaultUriCanBeSetInCode(bool useKeyed)
    {
        var vaultUri = new Uri(ConformanceConstants.VaultUri);

        var builder = Host.CreateEmptyApplicationBuilder(null);
        builder.Configuration.AddInMemoryCollection([
            new KeyValuePair<string, string?>("ConnectionStrings:secrets", "https://unused.vault.azure.net/")
        ]);

        if (useKeyed)
        {
            builder.AddKeyedAzureKeyVaultClient("secrets", settings => settings.VaultUri = vaultUri);
        }
        else
        {
            builder.AddAzureKeyVaultClient("secrets", settings => settings.VaultUri = vaultUri);
        }

        using var host = builder.Build();
        var client = useKeyed ?
            host.Services.GetRequiredKeyedService<SecretClient>("secrets") :
            host.Services.GetRequiredService<SecretClient>();

        Assert.Equal(vaultUri, client.VaultUri);
    }

The ConformanceTests for the SecretClient are passing too, so a connection to the Secrets area in the Aspire key vault to fetch the secret IsAlive is functioning without changing the entrypoint signature.

Would you still consider this breaking with this context? Maybe I'm misunderstanding what you mean by a breaking change but I've made absolute sure that the existing implementation isn't impacted at all.

[eerhardt]

@james-gould - check out https://learn.microsoft.com/dotnet/core/compatibility for how we thinking breaking changes/compatibility.

The change you are proposing is what we call a "Binary breaking change". If a library is already built calling AddAzureKeyVaultClient, it will fail to load with the new version because the signature changed.

[james-gould]

@sebastienros are you okay with leaving IL2026 present in #6fe9df6? I'll test in a moment but I'm sure it broke at run time as Roslyn couldn't figure out the configuration source.

[sebastienros]

@james-gould if the change is breaking then sure it's fine to disable the warning locally with a comment. I "personally" don't feel like this code suggestion is useful.

[james-gould]

It did have an issue with the configuration source, I've added the comment from @eerhardt to the 3 components.

[eerhardt]

lol - lots of "key"s in this method name.

[eerhardt]

Is this going to throw if settings.DisableHealthChecks == false?

[sebastienros]

@james-gould I think you need to add this is the two components that don't provide the health-check

protected override bool GetHealthCheckEnabled(AzureSecurityKeyVaultSettings settings)
{
    return false;
}

I will add it myself and approve the PR, unless you think it's wrong and we can then change it.