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 #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?

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 😅

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.

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.