Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add certificate allow list configuration #5172

Merged
merged 4 commits into from
Aug 7, 2024
Merged

Add certificate allow list configuration #5172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f812661
Add certificate allow list configuration
JamesNK Aug 5, 2024
1173b08
Clean up
JamesNK Aug 5, 2024
a08579e
Add validation and test
JamesNK Aug 6, 2024
34df34a
Fix test
JamesNK Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Aspire.Dashboard/Configuration/DashboardOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ public sealed class ResourceServiceClientCertificateOptions
public StoreLocation? Location { get; set; }
}

public sealed class AllowedCertificateRule
{
public string? Thumbprint { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@JamesNK JamesNK Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Don't set values after validating/parsing options.
public sealed class OtlpOptions
{
Expand All @@ -76,6 +81,8 @@ public sealed class OtlpOptions

public string? HttpEndpointUrl { get; set; }

public List<AllowedCertificateRule> AllowedCertificates { get; set; } = new();

public Uri? GetGrpcEndpointUri()
{
return _parsedGrpcEndpointUrl;
Expand Down Expand Up @@ -169,7 +176,7 @@ internal bool TryParseOptions([NotNullWhen(false)] out string? errorMessage)
{
if (string.IsNullOrEmpty(EndpointUrls))
{
errorMessage = "One or more frontend endpoint URLs are not configured. Specify a Dashboard:Frontend:EndpointUrls value.";
errorMessage = $"One or more frontend endpoint URLs are not configured. Specify an {DashboardConfigNames.DashboardFrontendUrlName.ConfigKey} value.";
return false;
}
else
Expand Down
10 changes: 9 additions & 1 deletion src/Aspire.Dashboard/Configuration/ValidateDashboardOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,18 @@ public ValidateOptionsResult Validate(string? name, DashboardOptions options)
case OtlpAuthMode.ApiKey:
if (string.IsNullOrEmpty(options.Otlp.PrimaryApiKey))
{
errorMessages.Add("PrimaryApiKey is required when OTLP authentication mode is API key. Specify a Dashboard:Otlp:PrimaryApiKey value.");
errorMessages.Add($"PrimaryApiKey is required when OTLP authentication mode is API key. Specify a {DashboardConfigNames.DashboardOtlpPrimaryApiKeyName.ConfigKey} value.");
}
break;
case OtlpAuthMode.ClientCertificate:
for (var i = 0; i < options.Otlp.AllowedCertificates.Count; i++)
{
var allowedCertRule = options.Otlp.AllowedCertificates[i];
if (string.IsNullOrEmpty(allowedCertRule.Thumbprint))
{
errorMessages.Add($"Thumbprint on allow certificate rule is not configured. Specify a {DashboardConfigNames.DashboardOtlpAllowedCertificatesName.ConfigKey}:{i}:Thumbprint value.");
}
}
break;
case null:
errorMessages.Add($"OTLP endpoint authentication is not configured. Either specify {DashboardConfigNames.DashboardUnsecuredAllowAnonymousName.ConfigKey}=true, or specify {DashboardConfigNames.DashboardOtlpAuthModeName.ConfigKey}. Possible values: {string.Join(", ", typeof(OtlpAuthMode).GetEnumNames())}");
Expand Down
21 changes: 21 additions & 0 deletions src/Aspire.Dashboard/DashboardWebApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my knowledge, why do we need our own custom option? Doesn't Kestrel have an existing setting for which certs are valid?

Copy link
Member Author

@JamesNK JamesNK Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var allowed = false;
foreach (var rule in allowList)
{
// Thumbprint is hexadecimal and is case-insensitive.
if (string.Equals(rule.Thumbprint, context.ClientCertificate.Thumbprint, StringComparison.OrdinalIgnoreCase))
{
allowed = true;
break;
}
}

if (!allowed)
{
context.Fail("Certificate doesn't match allow list.");
return Task.CompletedTask;
}
}

var claims = new[]
{
new Claim(ClaimTypes.NameIdentifier,
Expand Down
1 change: 1 addition & 0 deletions src/Shared/DashboardConfigNames.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ internal static class DashboardConfigNames
public static readonly ConfigName DashboardOtlpSecondaryApiKeyName = new("Dashboard:Otlp:SecondaryApiKey", "DASHBOARD__OTLP__SECONDARYAPIKEY");
public static readonly ConfigName DashboardOtlpCorsAllowedOriginsKeyName = new("Dashboard:Otlp:Cors:AllowedOrigins", "DASHBOARD__OTLP__CORS__ALLOWEDORIGINS");
public static readonly ConfigName DashboardOtlpCorsAllowedHeadersKeyName = new("Dashboard:Otlp:Cors:AllowedHeaders", "DASHBOARD__OTLP__CORS__ALLOWEDHEADERS");
public static readonly ConfigName DashboardOtlpAllowedCertificatesName = new("Dashboard:Otlp:AllowedCertificates", "DASHBOARD__OTLP__ALLOWEDCERTIFICATES");
public static readonly ConfigName DashboardFrontendAuthModeName = new("Dashboard:Frontend:AuthMode", "DASHBOARD__FRONTEND__AUTHMODE");
public static readonly ConfigName DashboardFrontendBrowserTokenName = new("Dashboard:Frontend:BrowserToken", "DASHBOARD__FRONTEND__BROWSERTOKEN");
public static readonly ConfigName DashboardFrontendMaxConsoleLogCountName = new("Dashboard:Frontend:MaxConsoleLogCount", "DASHBOARD__FRONTEND__MAXCONSOLELOGCOUNT");
Expand Down
2 changes: 1 addition & 1 deletion tests/Aspire.Dashboard.Tests/DashboardOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public void FrontendOptions_EmptyEndpointUrl()
var result = new ValidateDashboardOptions().Validate(null, options);

Assert.False(result.Succeeded);
Assert.Equal("One or more frontend endpoint URLs are not configured. Specify a Dashboard:Frontend:EndpointUrls value.", result.FailureMessage);
Assert.Equal("One or more frontend endpoint URLs are not configured. Specify an ASPNETCORE_URLS value.", result.FailureMessage);
}

[Fact]
Expand Down
50 changes: 48 additions & 2 deletions tests/Aspire.Dashboard.Tests/Integration/OtlpGrpcServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,10 @@ public async Task CallService_OtlpEndpoint_RequiredClientCertificateMissing_Fail
Assert.True(ex.StatusCode is StatusCode.Unavailable or StatusCode.Internal, "gRPC call fails without cert.");
}

[Fact]
public async Task CallService_OtlpEndpoint_RequiredClientCertificateValid_Success()
[Theory]
[InlineData(null)]
[InlineData("91113E785A7100C246D4664420621157498BCC66")]
public async Task CallService_OtlpEndpoint_RequiredClientCertificateValid_Success(string? allowedThumbprint)
{
// Arrange
X509Certificate2? clientCallbackCert = null;
Expand All @@ -299,6 +301,11 @@ public async Task CallService_OtlpEndpoint_RequiredClientCertificateValid_Succes

config[DashboardConfigNames.DashboardOtlpAuthModeName.ConfigKey] = OtlpAuthMode.ClientCertificate.ToString();

if (allowedThumbprint != null)
{
config[$"{DashboardConfigNames.DashboardOtlpAllowedCertificatesName.ConfigKey}:0:Thumbprint"] = allowedThumbprint;
}

config["Dashboard:Otlp:CertificateAuthOptions:AllowedCertificateTypes"] = "SelfSigned";
config["Dashboard:Otlp:CertificateAuthOptions:ValidateValidityPeriod"] = "false";
});
Expand All @@ -322,4 +329,43 @@ public async Task CallService_OtlpEndpoint_RequiredClientCertificateValid_Succes
// Assert
Assert.Equal(0, response.PartialSuccess.RejectedLogRecords);
}

[Fact]
public async Task CallService_OtlpEndpoint_RequiredClientCertificateValid_NotInAllowedList_Failure()
{
// Arrange
X509Certificate2? clientCallbackCert = null;

await using var app = IntegrationTestHelpers.CreateDashboardWebApplication(_testOutputHelper, config =>
{
// Change dashboard to HTTPS so the caller can negotiate a HTTP/2 connection.
config[DashboardConfigNames.DashboardOtlpGrpcUrlName.ConfigKey] = "https://127.0.0.1:0";

config[DashboardConfigNames.DashboardOtlpAuthModeName.ConfigKey] = OtlpAuthMode.ClientCertificate.ToString();

config[$"{DashboardConfigNames.DashboardOtlpAllowedCertificatesName.ConfigKey}:0:Thumbprint"] = "123";

config["Authentication:Schemes:Certificate:AllowedCertificateTypes"] = "SelfSigned";
config["Authentication:Schemes:Certificate:ValidateValidityPeriod"] = "false";
});
await app.StartAsync();

var clientCertificates = new X509CertificateCollection(new[] { TestCertificateLoader.GetTestCertificate("eku.client.pfx") });
using var channel = IntegrationTestHelpers.CreateGrpcChannel(
$"https://{app.OtlpServiceGrpcEndPointAccessor().EndPoint}",
_testOutputHelper,
validationCallback: cert =>
{
clientCallbackCert = cert;
},
clientCertificates: clientCertificates);

var client = new LogsService.LogsServiceClient(channel);

// Act
var ex = await Assert.ThrowsAsync<RpcException>(() => client.ExportAsync(new ExportLogsServiceRequest()).ResponseAsync);

// Assert
Assert.Equal(StatusCode.Unauthenticated, ex.StatusCode);
}
}
20 changes: 18 additions & 2 deletions tests/Aspire.Dashboard.Tests/Integration/StartupTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,24 @@ public async Task Configuration_NoExtraConfig_Error()

// Assert
Assert.Collection(app.ValidationFailures,
s => s.Contains("Dashboard:Frontend:EndpointUrls"),
s => s.Contains("Dashboard:Otlp:EndpointUrl"));
s => Assert.Contains("ASPNETCORE_URLS", s),
s => Assert.Contains("DOTNET_DASHBOARD_OTLP_ENDPOINT_URL", s));
Comment on lines +46 to +47
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but I noticed this test is wrong when adding the allow certs test.

}

[Fact]
public async Task Configuration_EmptyAllowedCertificateRule_Error()
{
// Arrange & Act
await using var app = IntegrationTestHelpers.CreateDashboardWebApplication(testOutputHelper,
additionalConfiguration: data =>
{
data["Dashboard:Otlp:AuthMode"] = nameof(OtlpAuthMode.ClientCertificate);
data["Dashboard:Otlp:AllowedCertificates:0"] = string.Empty;
});

// Assert
Assert.Collection(app.ValidationFailures,
s => Assert.Contains("Dashboard:Otlp:AllowedCertificates:0:Thumbprint", s));
}

[Fact]
Expand Down