dotnet / dotnet/aspnetcore

Enable Automatic Rotation of Trusted Client Certificate Chain Components

Open
#49,788 15 comments 2 reactions 0 assignees View on GitHub
api-suggestion area-auth area-networking enhancement
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 6h
Merged PRs (30d)
290

Description

## 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`](https://www.nuget.org/packages/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`, as well as properties that may aid in further customizing the `X509ChainPolicy`.

The new event and its input are below:
```diff
namespace Microsoft.AspNetCore.Authentication.Certificate;

public class CertificateAuthenticationEvents
{
+ public Func OnCertificateValidating { get; set; } = context => Task.CompletedTask;
public Func OnCertificateValidated { get; set; } = context => Task.CompletedTask;
public Func OnChallenge { get; set; } = context => Task.CompletedTask;
}

+public class CertificateValidatingContext : BaseContext
+{
+ 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:
```diff
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`](https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509chain.chainpolicy?view=net-7.0) 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](https://learn.microsoft.com/en-us/aspnet/core/security/authentication/certauth?view=aspnetcore-7.0#get-started).

```csharp
var builder = WebApplication.CreateBuilder(args);

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

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.

```diff
+public class CertificateValidatingContext : BaseContext
+{
+ 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`](https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.server.kestrel.https.httpsconnectionadapteroptions.servercertificateselector?view=aspnetcore-7.0#microsoft-aspnetcore-server-kestrel-https-httpsconnectionadapteroptions-servercertificateselector) with new selector properties for the [`CertificateAuthenticationOptions`](https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.authentication.certificate.certificateauthenticationoptions?view=aspnetcore-7.0) class.

```diff
public class CertificateAuthenticationOptions : AuthenticationSchemeOptions
{
public X509Certificate2Collection AdditionalChainCertificates { get; set; }
+ public Func AdditionalChainCertificatesSelector { get; set; }
public X509Certificate2Collection CustomTrustStore { get; set; }
+ public Func 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?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.