aspire icon indicating copy to clipboard operation
aspire copied to clipboard

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

Open james-gould opened this issue 9 months ago • 14 comments

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?
    • [x] Yes. Ready to ship.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • [x] Yes
  • Did you add public API?
    • [x] No
  • Does the change make any security assumptions or guarantees?
    • [x] No
  • Does the change require an update in our Aspire docs?
    • [x] No

james-gould avatar Mar 29 '25 20:03 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 👍

2025-03-29 20_29_09-Window

james-gould avatar Mar 29 '25 20:03 james-gould

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

davidfowl avatar Mar 29 '25 20:03 davidfowl

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 avatar Mar 29 '25 21:03 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!

james-gould avatar Apr 04 '25 15:04 james-gould

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

davidfowl avatar Apr 04 '25 16:04 davidfowl

@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.

james-gould avatar Apr 04 '25 16:04 james-gould

@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.

eerhardt avatar Apr 04 '25 16:04 eerhardt

@eerhardt Appreciate the added context, along with the binary breaking change, new(ish) area for me in .NET so please forgive the misunderstanding. Build is fixed and happy, no breaking changes :)

james-gould avatar Apr 04 '25 18:04 james-gould

@eerhardt @sebastienros can you review?

davidfowl avatar Apr 15 '25 13:04 davidfowl

Trying to keep the PR up to date with main, please let me know if that's causing any issues/pushing it back in a queue for review etc.

james-gould avatar Apr 22 '25 14:04 james-gould

@davidfowl sorry for the nudge - any chance for a review soon please?

james-gould avatar Apr 29 '25 13:04 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.

james-gould avatar Apr 29 '25 16:04 james-gould

@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.

sebastienros avatar Apr 29 '25 16:04 sebastienros

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

james-gould avatar Apr 29 '25 17:04 james-gould