Add certificate allow list configuration #5172
Conversation
| @@ -61,6 +61,11 @@ public sealed class ResourceServiceClientCertificateOptions | |||
| public StoreLocation? Location { get; set; } | |||
| } | |||
|
|
|||
| public sealed class AllowedCertificateRule | |||
| { | |||
| public string? Thumbprint { get; set; } | |||
There was a problem hiding this comment.
Wondering why this is nullable. Why would we want an instance of this with a null thumbprint?
If the system was configured with a single rule having a null thumbprint, I don't think any connection would ever be accepted.
X509Certificate2.Thumbprint doens't seem to be nullable either.
There was a problem hiding this comment.
The property is set by configuration so it could be left out and be null. In that case it has no impact because it will never match a cert.
But I will add some validation logic for letting the person launching the dashboard that they're made a mistake.
In case you're wondering why there is AllowedCertificateRule rather than a string list of thumbprints, it leaves open the option for extra options when allowing certificates, e.g. allow certs based on issuer.
| @@ -560,6 +560,27 @@ private static void ConfigureAuthentication(WebApplicationBuilder builder, Dashb | |||
| { | |||
| OnCertificateValidated = context => | |||
| { | |||
| var options = context.HttpContext.RequestServices.GetRequiredService<IOptions<DashboardOptions>>().Value; | |||
| if (options.Otlp.AllowedCertificates is { Count: > 0 } allowList) | |||
There was a problem hiding this comment.
For my knowledge, why do we need our own custom option? Doesn't Kestrel have an existing setting for which certs are valid?
There was a problem hiding this comment.
Kestrel and ASP.NET Core cert auth provides settings for allowing or rejecting invalid certs. However, just because a cert is valid doesn't necessarily mean that it should be allowed to be authenticated.
https://learn.microsoft.com/en-us/aspnet/core/security/authentication/certauth?view=aspnetcore-8.0 - see OnCertificateValidated
Called after the certificate has been validated, passed validation and a default principal has been created. This event allows you to perform your own validation and augment or replace the principal.
This PR adds configuration and code for that check.
| s => Assert.Contains("ASPNETCORE_URLS", s), | ||
| s => Assert.Contains("DOTNET_DASHBOARD_OTLP_ENDPOINT_URL", s)); |
There was a problem hiding this comment.
Unrelated, but I noticed this test is wrong when adding the allow certs test.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
JamesNK
force-pushed
the
jamesnk/certificate-allow-list
branch
from
a48228f
to
34df34a
Compare
Fixes #2995
Adds an optional allow list to certification authentication.
The allow list is optional for two reasons:
Microsoft Reviewers: Open in CodeFlow