Enable Automatic Rotation of Trusted Client Certificate Chain Components

[wsugarman]

Background and Motivation

While developers can use a combination of ServerCertificateSelector and IFileProvider today to automatically reload a server's TLS certificate (as mentioned in https://github.com/dotnet/aspnetcore/issues/32351), developers cannot as easily reload the certificate(s) used to validate the client's certificate chain. Developers can of course write their own ClientCertificateValidation delegate, but I think that also means re-performing much of the logic that already exists within the Microsoft.AspNetCore.Authentication.Certificate package.

I propose adding a new API that allow developers to leverage the convenience of the Microsoft.AspNetCore.Authentication.Certificate while also enabling more dynamic scenarios, like the automatic reload of trusted components in the client certificate chain.

Proposed API

Based on some initial feedback from @Tratcher to consider the existing events, I propose adding a new event that triggers just before validation (as opposed to the existing event OnCertificateValidated that triggers just afterwards). This event's input context contains information about the HTTP request from the BaseContext<T>, as well as properties that may aid in further customizing the X509ChainPolicy.

The new event and its input are below:

namespace Microsoft.AspNetCore.Authentication.Certificate;

public class CertificateAuthenticationEvents
{
+    public Func<CertificateValidatingContext, Task> OnCertificateValidating { get; set; } = context => Task.CompletedTask;
    public Func<CertificateValidatedContext, Task> OnCertificateValidated { get; set; } = context => Task.CompletedTask;
    public Func<CertificateChallengeContext, Task> OnChallenge { get; set; } = context => Task.CompletedTask;
}

+public class CertificateValidatingContext : BaseContext<CertificateAuthenticationOptions>
+{
+    public X509ChainPolicy ChainPolicy { get; set; } = default!;
+    public X509Certificate2 ClientCertificate { get; set; } = default!;
+    public bool IsSelfSigned { get; set; }
+}

This new event is then used by the CertificateAuthenticationHandler when validating:

        var chainPolicy = BuildChainPolicy(clientCertificate, isCertificateSelfSigned);
+        var certificateValidatingContext = new CertificateValidatingContext(Context, Scheme, Options)
+        {
+            ChainPolicy = chainPolicy,
+            ClientCertificate = clientCertificate,
+            IsSelfSigned = isCertificateSelfSigned,
+        };
+
+        await Events.CertificateValidating(certificateValidatingContext);

        using var chain = new X509Chain
        {
            ChainPolicy = chainPolicy
        };
        var certificateIsValid = chain.Build(clientCertificate);

I tried to model the new event as the others exist today, such that they do not return any data and instead rely on the context as a sort of communication medium. ChainPolicy (whose property name is used by X509Chain.ChainPolicy too) can be used by developers to make changes to the validation. ClientCertificate and IsSelfSigned are used internally to generate the policy (in addition to the options), and I thought they could be helpful. I am not opposed to removing them though.

Usage Examples

Based on the docs here.

var builder = WebApplication.CreateBuilder(args);

builder.Services
    .AddAuthentication(CertificateAuthenticationDefaults.AuthenticationScheme)
    .AddCertificate(options =>
    {
        options.Events = new CertificateAuthenticationEvents
        {
            OnCertificateValidating = context =>
            {
                var caCertProvider = context.HttpContext.RequestServices.GetRequiredService<ICaCertProvider>();

                context.ChainPolicty.CustomTrustStore.Clear();
                context.ChainPolicty.Add(caCertProvider.Certificate);

                return Task.CompletedTask;
            },
        },
    };

var app = builder.Build();

app.UseAuthentication();

app.MapGet("/", () => "Hello World!");

app.Run();

Alternative Designs

Event Context Properties

Instead of access to the entire X509ChainPolicy, developers could be simply given the CustomTrustStore and AdditionalChainCertificates collections thereby limiting the scope of changes that could be made.

+public class CertificateValidatingContext : BaseContext<CertificateAuthenticationOptions>
+{
+    public X509CertificateCollection CustomTrustStore { get; set } = default!;
+    public X509CertificateCollection AdditionalChainCertificates { get; set; } = default!;
+}

Selector Properties

At first, I was first imaging this API to be a parallel of HttpsConnectionAdapterOptions.ServerCertificateSelector with new selector properties for the CertificateAuthenticationOptions class.

public class CertificateAuthenticationOptions : AuthenticationSchemeOptions
{
    public X509Certificate2Collection AdditionalChainCertificates { get; set; }
+    public Func<X509Certificate2Collection> AdditionalChainCertificatesSelector { get; set; }
    public X509Certificate2Collection CustomTrustStore { get; set; }
+    public Func<X509Certificate2Collection> CustomTrustStoreSelector { get; set; }
}

There should be some input to the delegates, and the names could be a lot more creative, but I think this adds too much bloat to the class. It may also be confusing. Although I do appreciate the symmetry for client CA certificates.

Risks

The biggest problem I see with the proposal is that it may expose too much in the new event; it could be overwhelming and/or simply unnecessary for developers. In fact, developers could completely overwrite the entire 509ChainPolicy! Is exposing the X509Certificate2 object before validation also risky business?

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[wsugarman]

I am going to update this issue and PR based on the comments soon. I've just gotten distracted by other commitments.

[wsugarman]

I have updated this proposal to be... better 😅

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[halter73]

API Review Notes:

• Why does the event expose a X509ChainPolicy rather than the X509CertificateCollection?

  • The X509ChainPolicy is built per request and depends on whether the incoming certificate is self-signed. It should be more flexible than just specifying an X509CertificateCollection for CustomTrustStore.

• Should we expose IsSelfSigned and ClientCertificate?

  • It makes sense to be able to read it, but why does it need to be settable?

• Do the properties need to be settable?

  • The current PR does not read the context after firing the event, so anything set to a new instance will be missed.
  • We do not currently see a use case for setting the ClientCertificate or IsSelfSigned properties.
  • Everything on X509ChainPolicy seems to be mutable, and it's not reused, so we think that can stay non-settable as well.

• Should we use ResultContext rather than BaseContext?

  • Yes. This matches the validated event and makes the event a little more powerful since it can short circuit with a helpful failure exception.

• We need to add a constructor. We'll make all the non-ctor properties required init properties.

• Is the event bad for performance after you rotate certs?

  • After cert rotation, the handler would first populate the X509ChainPolicy with outdated certs from CertificateAuthenticationOptions.CustomTrustStore, then the new OnCertificateValidating would be forced to clear the outdated certs and add the newly rotated ones during each validation.

If we decide that we're not concerned about the performance of clearing and reconfiguring X509ChainPolicy.CustomTrustStore, and should stick to the event, this is how we think the API should look:

namespace Microsoft.AspNetCore.Authentication.Certificate;

public class CertificateAuthenticationEvents
{
+    public Func<CertificateValidatingContext, Task> OnCertificateValidating { get; set; } = context => Task.CompletedTask;
    public Func<CertificateValidatedContext, Task> OnCertificateValidated { get; set; } = context => Task.CompletedTask;
    public Func<CertificateChallengeContext, Task> OnChallenge { get; set; } = context => Task.CompletedTask;
}

+public class CertificateValidatingContext : ResultContext<CertificateAuthenticationOptions>
+{
+       public CertificateValidatingContext(HttpContext context, AuthenticationScheme scheme, CertificateAuthenticationOptions options);
+    public required X509ChainPolicy ChainPolicy { get; init; }
+    public required X509Certificate2 ClientCertificate { get; init; }
+    public required bool IsSelfSigned { get; init; }
+}

[Tratcher]

• Is the event bad for performance after you rotate certs?

  • After cert rotation, the handler would first populate the X509ChainPolicy with outdated certs from CertificateAuthenticationOptions.CustomTrustStore, then the new OnCertificateValidating would be forced to clear the outdated certs and add the newly rotated ones during each validation.

If you're using the event to provide certs then I don't think you'd be setting CertificateAuthenticationOptions.CustomTrustStore at all, so you wouldn't have to add and then clear them.

The perf doesn't change after a rotation, the certs are always provided dynamically.

[wsugarman]

Actually, in the middle of writing this response, I noticed that there may be an even easier way to solve this problem...! I'll keep my thoughts/responses to the earlier proposal after this new idea.

@halter73, @Tratcher - I see that the underlying AuthenticationHandler<TOptions> exposes an OptionsMonitor property. We could "snapshot" it via OptionsMonitor.Get(Scheme.Name) at the start of HandleAuthenticateAsync and pass it through the other helper methods. That would enable users to use the options pattern to trigger a reload of the settings when the issuer certificate changes.

What do you both think?

Extra notes

@halter73 - Thanks for taking a look! When it comes to the aforementioned review, are these questions and subsequent answers/thoughts from that review? I'll comment on them assuming that to be the case.

• Why does the event expose a X509ChainPolicy rather than the X509CertificateCollection?

  • The X509ChainPolicy is built per request and depends on whether the incoming certificate is self-signed. It should be more flexible than just specifying an X509CertificateCollection for CustomTrustStore.

I went back-and-forth on this when formulating the proposal. I agree that ultimately exposing the X509ChainPolicy provides more flexibility instead of exposing portions of the policy, via the context, piecemeal as requested by users.

• Should we expose IsSelfSigned and ClientCertificate?

  • It makes sense to be able to read it, but why does it need to be settable?

I agree. These fields should not be settable.

• Do the properties need to be settable?

  • The current PR does not read the context after firing the event, so anything set to a new instance will be missed.
  • We do not currently see a use case for setting the ClientCertificate or IsSelfSigned properties.
  • Everything on X509ChainPolicy seems to be mutable, and it's not reused, so we think that can stay non-settable as well.

I agree that these properties should not be mutable. The X509ChainPolicy itself its mutable (e.g. adding certificates to the CustomTrustStore), so that should be sufficient. No need to support replacing it wholesale.

• Should we use ResultContext rather than BaseContext?

  • Yes. This matches the validated event and makes the event a little more powerful since it can short circuit with a helpful failure exception.

What would be the purpose of the Result property on the "validating" event? Could it be set by users to skip validation? E.g. a user could pre-emptively fail validation based on some criteria?

• We need to add a constructor. We'll make all the non-ctor properties required init properties.

Makes sense!

• Is the event bad for performance after you rotate certs?

  • After cert rotation, the handler would first populate the X509ChainPolicy with outdated certs from CertificateAuthenticationOptions.CustomTrustStore, then the new OnCertificateValidating would be forced to clear the outdated certs and add the newly rotated ones during each validation.

Yeah, I've also been thinking about this too. The server certificate already has established a pattern with HttpsConnectionAdapterOptions.ServerCertificateSelector, but it may not be relevant given:

1. The server certificate API is located in a different project/assembly
2. While there is only one server certificate, there may be a variable number of issuer certificates

So, I see a few approaches for the client certificates trusted components:

1. Double-down on this sort of "selector" approach and similarly allow users to provide an X509CertificateCollection for the issuer(s) via some delegate (e.g. Func<X509CertificateCollection>)
   • Honestly, this approach seems pretty ugly 😅

2. Expand the existing events to logically fire before validation, like in the current proposal where components, including the CustomTrustStore, may be augmented.

   • Unfortunately, while this API fits nicely into the existing space, the handler for the above scenario would look like the following where the policy's X509CertificateCollection would be presumably be reset each time. On the other hand, we may be making mountains out of molehills; if the user does not supply CertificateAuthenticationOptions.CustomTrustStore, then invoking Clear() is unnecessary, and the adding the certificate to the store via the event is no different than via the options.

     options.Events = new CertificateAuthenticationEvents
     {
     OnCertificateValidating = context =>
     {
     var caCertProvider = context.HttpContext.RequestServices.GetRequiredService<ICaCertProvider>();
     
     context.ChainPolicty.CustomTrustStore.Clear(); // This may be superfluous
     context.ChainPolicty.Add(caCertProvider.Certificate);
     
     return Task.CompletedTask;
     },
     },

[Tratcher]

Options are already snapshotted at the start of a request here: https://github.com/dotnet/aspnetcore/blob/f8f03ea3764826d009f7ceb9bb91e482ce4a3fa9/src/Security/Authentication/Core/src/AuthenticationHandler.cs#L155

Note a handler's lifetime is per-request.

That leaves the question of how to trigger an options reload. This would happen automatically if we were binding to IConfiguration, but we're not in this case. However we should be able to implement a IOptionsChangeTokenSource that tracks file changes and triggers the options reload. Then the original config lambda would run again and get the latest content. That could be a lot nicer than the developer managing the caching. https://github.com/dotnet/runtime/blob/d2c7d8ea3bbd59215ac4689734d440bdeaf570e1/src/libraries/Microsoft.Extensions.Options.ConfigurationExtensions/src/ConfigurationChangeTokenSource.cs#L15C61-L15C86

I think the base mechanic would be something like: services.Configure<ClientCertificateOptions>(authScheme, configure).MonitorFile(filePath);

[wsugarman]

Oh! I'm sorry -- I missed that this service is transient.

https://github.com/dotnet/aspnetcore/blob/76b5785b4ca4d6107baad792e2fe4d7b6f938b9b/src/Security/Authentication/Core/src/AuthenticationBuilder.cs#L51

I agree that there should be some easy way for users to easily monitor the file like you suggested.

[halter73]

I agree that there should be some easy way for users to easily monitor the file like you suggested.

I also agree, but this would need more design. @adityamandaleeka @amcasey Can one of you look into this and mark it api-ready-for-review again if/when it gets to a point where we should look at it in API review again?

If you're happy going with just the updated event proposal in https://github.com/dotnet/aspnetcore/issues/49788#issuecomment-1804855894 for now, just say so and reapply the label, but I'm thinking we can probably make it both easier and faster.

[wsugarman]

@halter73 - I am unsure if I really like my own proposal given the potential of a perhaps more idiomatic approach with IOptionsMonitor<T> 😅

I'll try to noodle on a design. I think whatever is built for the client CA certificate also should work for configuring the server certificate as well. I was recently writing TLS support for a gRPC service and had to come up with some sort of abstraction for automatically reloading both certificates, and I would have loved some built-in support: https://github.com/wsugarman/durabletask-azurestorage-scaler/blob/Chart_2.0.0/src/Keda.Scaler.DurableTask.AzureStorage/Security/TlsConfigure.cs

[amcasey]

Sorry, I haven't had a chance to look into client certs yet - too many other cert problems. :wink: Are we working towards a deadline other than 9.0?

[wsugarman]

I finally found some time to play around with a design, and I looked at the FileConfigurationProvider as an inspiration.

I would propose some new extension methods on IOptionsBuilder<CertificateAuthenticationOptions>, like so:

public static class OptionsBuilderExtensions
{
    public static OptionsBuilder<CertificateAuthenticationOptions> WithAdditionalChainCertificate(this OptionsBuilder<CertificateAuthenticationOptions> builder, Action<CertificateLoadOptions> configureLoad)

    public static OptionsBuilder<CertificateAuthenticationOptions> WithCustomTrustedAuthority(this OptionsBuilder<CertificateAuthenticationOptions> builder, Action<CertificateLoadOptions> configureLoad);
}

This would register a IConfigureNamedOptions<TOptions> in the service collection that would load the certificate into memory based on the given options. Much of the API is based on Microsoft.Extensions.Configuration.FileExtensions:

public class CertificateLoadOptions
{
    public IFileProvider? FileProvider { get; set; }

    public CertificateFileFormat Format { get; set; }

    public string? KeyPath { get; set; }

    public Action<FileLoadExceptionContext>? OnLoadException { get; set; }

    [DisallowNull]
    public string? Path { get; set; }

    [Range(0, int.MaxValue)]
    public int ReloadDelay { get; set; } = 250;

    public bool ReloadOnChange { get; set; }
}

public enum CertificateFileFormat
{
    PFX,
    PEM,
}

public class FileLoadExceptionContext
{
    public Exception Exception { get; set; } = null!;

    public string Path { get; set; } = null!
}

With this API, users can simply use the existing options pattern to further configure their authentication:

services
    .AddOptions<CertificateAuthenticationOptions>()
    .WithCustomTrustedAuthority(o =>
    {
        o.Path = "/mnt/secrets/cert.pem",
        o.Key = "/mnt/secrets/key.pem",
        o.Format = CertificateFileFormat.PEM,
        o.ReloadOnChange = true,
    });

The trickiest part I noticed when writing an initial implementation is that it may be tricky to cache the certificate as it's changing. Maintaining a reference to the same X509Certificate2 object used by the middleware could be tricky, as we would presumably want to Dispose it upon detecting a change in the certificate file; however, there may be concurrent requests using that certificate. Alternatively, we probably do not want to keep creating the X509Certificate2 objects from a cached byte[] because we'd just keep creating new objects without disposing. Ideally, we'd probably have some sort of idiomatic reader/writer lock in the certificate auth middleware in which we can use to safely dispose of an obsolete certificate.

[wsugarman]

One more aspect to consider would be reusing this API for configuring the server TLS certificate used by Kestrel.