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

Replace use of obsolete X509Certificate2 API #5688

Closed
wants to merge 4 commits into from
Closed

Replace use of obsolete X509Certificate2 API #5688

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56fcb6c
Replace use of obsolete X509Certificate2 API
drewnoakes Sep 12, 2024
3cd762a
Reuse certificate file contents
drewnoakes Sep 13, 2024
e513ee8
Add tests for reading certificates
drewnoakes Sep 13, 2024
5b6d159
Review feedback
drewnoakes Sep 13, 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
4 changes: 4 additions & 0 deletions .config/CredScanSuppressions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@
{
"file": "\\tests\\Shared\\TestCertificates\\testCert.pfx",
"_justification": "Legitimate UT certificate file with private key, from dotnet/aspnetcore"
},
{
"file": "\\tests\\Shared\\TestCertificates\\https-dsa.pem",
"_justification": "Legitimate UT certificate file with private key, from dotnet/aspnetcore"
}
]
}
1 change: 1 addition & 0 deletions Aspire.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{991DB378-6CB5-4441-BFC3-657400690FC3}"
ProjectSection(SolutionItems) = preProject
.editorconfig = .editorconfig
.config\CredScanSuppressions.json = .config\CredScanSuppressions.json
Directory.Packages.props = Directory.Packages.props
spelling.dic = spelling.dic
eng\Versions.props = eng\Versions.props
Expand Down
1 change: 1 addition & 0 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
<PackageVersion Include="Grpc.Tools" Version="2.65.0" />
<PackageVersion Include="Humanizer.Core" Version="2.14.1" />
<PackageVersion Include="KubernetesClient" Version="14.0.9" />
<PackageVersion Include="Microsoft.Bcl.Cryptography" Version="9.0.0-rc.1.24431.7" /> <!-- Can remove Microsoft.Bcl.Cryptography when the repo moves to .NET 9 or later -->
Copy link
Member

Choose a reason for hiding this comment

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

Can we can't get to the final version of the package for the .NET 9 release? Will timings work out?

If not, is it ok to ship a preview package?

Copy link
Member Author

Choose a reason for hiding this comment

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

@joperezr @davidfowl any concerns here?

Copy link
Member

Choose a reason for hiding this comment

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

There are some concerns, but perhaps not really ship-blockers. Starting from RC2, all of the packages from dotnet/runtime will only flow internally, and so far we are mostly just building externally. Our two options would be either ship depending on RC2 build (which while not ideal wouldn't have a big impact since the dashboard ships as a published app with no package references, and hence it can be stable still), or building internally so that we get the final 9.0 version that will get shipped. cc: @bartonjs

<PackageVersion Include="Microsoft.Data.SqlClient" Version="5.2.2" />
<PackageVersion Include="Microsoft.FluentUI.AspNetCore.Components" Version="4.9.3" />
<PackageVersion Include="Microsoft.FluentUI.AspNetCore.Components.Icons" Version="4.9.3" />
Expand Down
1 change: 1 addition & 0 deletions src/Aspire.Dashboard/Aspire.Dashboard.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
<PackageReference Include="Humanizer.Core" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.Certificate" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.OpenIdConnect" />
<PackageReference Include="Microsoft.Bcl.Cryptography" />
<PackageReference Include="Microsoft.FluentUI.AspNetCore.Components" />
<PackageReference Include="Microsoft.FluentUI.AspNetCore.Components.Icons" />
</ItemGroup>
Expand Down
46 changes: 8 additions & 38 deletions src/Aspire.Dashboard/ResourceService/DashboardClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,14 @@ GrpcChannel CreateChannel()
// Auth hasn't been suppressed, so configure it.
var certificates = _dashboardOptions.ResourceServiceClient.ClientCertificate.Source switch
{
DashboardClientCertificateSource.File => GetFileCertificate(),
DashboardClientCertificateSource.KeyStore => GetKeyStoreCertificate(),
DashboardClientCertificateSource.File => CertificateUtil.GetFileCertificate(
filePath: _dashboardOptions.ResourceServiceClient.ClientCertificate.FilePath!, // validated
password: _dashboardOptions.ResourceServiceClient.ClientCertificate.Password,
logger: _logger),
DashboardClientCertificateSource.KeyStore => CertificateUtil.GetKeyStoreCertificate(
subject: _dashboardOptions.ResourceServiceClient.ClientCertificate.Subject!, // validated
storeName: _dashboardOptions.ResourceServiceClient.ClientCertificate.Store,
location: _dashboardOptions.ResourceServiceClient.ClientCertificate.Location ?? StoreLocation.LocalMachine),
_ => throw new InvalidOperationException("Unable to load ResourceServiceClient client certificate.")
};

Expand Down Expand Up @@ -158,42 +164,6 @@ GrpcChannel CreateChannel()
LoggerFactory = _loggerFactory,
ThrowOperationCanceledOnCancellation = true
});

X509CertificateCollection GetFileCertificate()
{
Debug.Assert(
_dashboardOptions.ResourceServiceClient.ClientCertificate.FilePath != null,
"FilePath is validated as not null when configuration is loaded.");

var filePath = _dashboardOptions.ResourceServiceClient.ClientCertificate.FilePath;
var password = _dashboardOptions.ResourceServiceClient.ClientCertificate.Password;

return [new X509Certificate2(filePath, password)];
Copy link
Member

Choose a reason for hiding this comment

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

"99%" of the reason to use the new X509CertificateLoader over the old constructors is to not load a PKCS#12/PFX when you weren't expecting to. Since you were expecting to load a PFX, and you're loading from a local file, it's not the end of the world if you continue calling the legacy constructor. (And since you're only loading it once you don't care about "we made parallel-loading the same PFX better" feature, etc.)

So, while it is forward looking to get off of this ctor and on to the new loader model, it's not "important" right now.

}

X509CertificateCollection GetKeyStoreCertificate()
{
Debug.Assert(
_dashboardOptions.ResourceServiceClient.ClientCertificate.Subject != null,
"Subject is validated as not null when configuration is loaded.");

var subject = _dashboardOptions.ResourceServiceClient.ClientCertificate.Subject;
var storeName = _dashboardOptions.ResourceServiceClient.ClientCertificate.Store ?? "My";
var location = _dashboardOptions.ResourceServiceClient.ClientCertificate.Location ?? StoreLocation.CurrentUser;

using var store = new X509Store(storeName: storeName, storeLocation: location);

store.Open(OpenFlags.ReadOnly);

var certificates = store.Certificates.Find(X509FindType.FindBySubjectName, findValue: subject, validOnly: true);

if (certificates is [])
{
throw new InvalidOperationException($"Unable to load client certificate with subject \"{subject}\" from key store.");
}

return certificates;
}
}
}

Expand Down
48 changes: 48 additions & 0 deletions src/Aspire.Dashboard/Utils/CertificateUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Security.Cryptography.X509Certificates;

namespace Aspire.Dashboard;

internal static class CertificateUtil
{
public static X509CertificateCollection GetFileCertificate(string filePath, string? password, ILogger logger)
{
var certBytes = File.ReadAllBytes(filePath);
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to keep working in terms of the path, and not need to use ReadAllBytes.

GetCertContentType accepts path-or-bytes, but the cert load functions you need the "FromFile" variants.

The FromFile variants do things smarter than File.ReadAllBytes; but since it's a one-time/startup cost it probably doesn't matter for you.


var certContentType = X509Certificate2.GetCertContentType(certBytes);

if (certContentType is X509ContentType.Pkcs12)
{
return [X509CertificateLoader.LoadPkcs12(certBytes, password)];
}
else
{
if (password is not null)
{
logger.LogDebug("Resource service certificate {FilePath} has type {Type} which does not support passwords, yet a password was configured. The certificate password will be ignored.", filePath, certContentType);
}

return [X509CertificateLoader.LoadCertificate(certBytes)];
Copy link
Member

Choose a reason for hiding this comment

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

Technically speaking, you have a functionality regression here. The old code would load any of

  • A single certificate
    • Which is 99% useless, but SslStream has a feature where it looks in the cert stores to see if maybe you happened to have specified a certificate that you already know about and have a private key for... (which might only be on Windows?)
  • A PKCS#7 SignedCms file.
    • Where it loads the certificate used by the first signer, or fails if it can't find one (or there is no first signer).
    • And then is the same as the "plain certificate" case.
  • A Windows SerializedCert blob (Windows only)
    • This "might" have an associated private key, just by virtue of it might have written down "and my private key is named {whatever}" AND that key happens to be available on this machine/user-context.
    • Otherwise it's just a "plain certificate"
    • But basically no one uses this format.
  • An Authenticode-signed file (exe, dll, msu, cab, etc). (Windows-only)
    • It extracts the certificate of the first signer.
    • Sounds a bit like SignedCms, eh? Oh, wait, Authenticode uses SignedCms. Less coincidence, more "same code".
  • A PKCS#12/PFX file
    • Probably has a certificate in it with a private key, but, might not.

The new code will throw if it encounters any of those three middle formats, because the new loader is "one method (group), one file format". The middle ones don't really make sense in context for you, so they're not really important... but this is, technically, marginally, a breaking change.

https://github.com/dotnet/core/blob/main/release-notes/9.0/preview/preview7/libraries.md#changes-to-x509-certificate-loading shows an equivalent to what new X509Certificate2(bytesOrPath, password, flags) does.

}
}

public static X509CertificateCollection GetKeyStoreCertificate(string subject, string? storeName = null, StoreLocation location = StoreLocation.CurrentUser)
{
storeName ??= "My";

using var store = new X509Store(storeName: storeName, storeLocation: location);

store.Open(OpenFlags.ReadOnly);

var certificates = store.Certificates.Find(X509FindType.FindBySubjectName, findValue: subject, validOnly: true);

if (certificates is [])
{
throw new InvalidOperationException($"Unable to load client certificate with subject \"{subject}\" from key store.");
}

return certificates;
}
}
1 change: 1 addition & 0 deletions tests/Aspire.Dashboard.Tests/Aspire.Dashboard.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<Compile Include="$(TestsSharedDir)Logging\*.cs" LinkBase="shared/Logging" />
<Compile Include="$(TestsSharedDir)Telemetry\*.cs" LinkBase="shared/Telemetry" />
<Content Include="$(TestsSharedDir)TestCertificates\*.pfx" LinkBase="shared/TestCertificates" CopyToOutputDirectory="PreserveNewest" />
<Content Include="$(TestsSharedDir)TestCertificates\*.pem" LinkBase="shared/TestCertificates" CopyToOutputDirectory="PreserveNewest" />

<Content Include="..\..\src\Aspire.Dashboard\wwwroot\**\*.*" LinkBase="wwwroot" CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>
Expand Down
132 changes: 132 additions & 0 deletions tests/Aspire.Dashboard.Tests/Utils/CertificateUtilTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using Microsoft.AspNetCore.InternalTesting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Xunit;

namespace Aspire.Dashboard;

public sealed class CertificateUtilTests
{
[Theory]
[InlineData("testCert.pfx", X509ContentType.Pkcs12)]
[InlineData("https-dsa.pem", X509ContentType.Cert)]
public void TestCertificateTypes(string fileName, X509ContentType contentType)
{
var filePath = TestCertificateLoader.GetCertPath(fileName);

Assert.Equal(contentType, X509Certificate2.GetCertContentType(filePath));
}

[Fact]
public void GetFileCertificate_Pkcs12_CorrectPassword()
{
var certificates = CertificateUtil.GetFileCertificate(
filePath: TestCertificateLoader.GetCertPath("testCert.pfx"),
password: "testPassword",
logger: NullLogger.Instance);

Assert.NotNull(certificates);

var cert = Assert.Single(certificates.Cast<X509Certificate>());

Assert.NotNull(cert);
Assert.Equal("CN=localhost", cert.Subject);
}

[Fact]
public void GetFileCertificate_Pkcs12_IncorrectPassword()
{
Assert.Throws<CryptographicException>(() => CertificateUtil.GetFileCertificate(
filePath: TestCertificateLoader.GetCertPath("testCert.pfx"),
password: "wrongPassword",
logger: NullLogger.Instance));
}

[Fact]
public void GetFileCertificate_Cert_WithRedundantPassword()
{
var filePath = TestCertificateLoader.GetCertPath("https-dsa.pem");
Copy link
Member

Choose a reason for hiding this comment

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

It's a little weird to me that you're testing with DSA, since that algorithm itself is now entirely obsolete. (FIPS 186-5 withdrew it, so it's not just "the least popular", it's "RIP")


using MockLogger logger = new()
{
$"Resource service certificate {filePath} has type {X509ContentType.Cert} which does not support passwords, yet a password was configured. The certificate password will be ignored."
};

var certificates = CertificateUtil.GetFileCertificate(
filePath: filePath,
password: "testPassword",
logger: logger);

Assert.NotNull(certificates);

var cert = Assert.Single(certificates.Cast<X509Certificate>());

Assert.NotNull(cert);
Assert.Equal("OU=Development, O=Contoso, L=Alexandria, S=Virginia, C=US", cert.Subject);
}

[Fact]
public void GetFileCertificate_Cert()
{
var filePath = TestCertificateLoader.GetCertPath("https-dsa.pem");

var certificates = CertificateUtil.GetFileCertificate(
filePath: filePath,
password: null, // No password required for this file
logger: null!); // Should not be called, and a sneaky null validates this

Assert.NotNull(certificates);

var cert = Assert.Single(certificates.Cast<X509Certificate>());

Assert.NotNull(cert);
Assert.Equal("OU=Development, O=Contoso, L=Alexandria, S=Virginia, C=US", cert.Subject);
}

private sealed class MockLogger : ILogger, IDisposable, IEnumerable
{
private readonly List<string> _expected = [];

IDisposable? ILogger.BeginScope<TState>(TState state) => null;

bool ILogger.IsEnabled(LogLevel logLevel) => true;

void ILogger.Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter)
{
if (_expected.Count == 0)
{
Assert.Fail("Unexpected log invocation.");
}

var actual = formatter(state, exception);
var expected = _expected[0];
_expected.RemoveAt(0);

Assert.Equal(expected, actual);
}

public void Add(string expected)
{
_expected.Add(expected);
}

IEnumerator IEnumerable.GetEnumerator()
{
throw new NotSupportedException("Only IEnumerable to support collection initializers.");
}

void IDisposable.Dispose()
{
if (_expected.Count is not 0)
{
Assert.Fail($"Not all expected log messages were observed. {_expected.Count} remain.");
}
}
}
}
20 changes: 20 additions & 0 deletions tests/Shared/TestCertificates/https-dsa.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDWTCCAxWgAwIBAgIUFRQGA90GHC74cNK/hNzQDi7XJFYwCwYJYIZIAWUDBAMC
MF0xCzAJBgNVBAYTAlVTMREwDwYDVQQIDAhWaXJnaW5pYTETMBEGA1UEBwwKQWxl
eGFuZHJpYTEQMA4GA1UECgwHQ29udG9zbzEUMBIGA1UECwwLRGV2ZWxvcG1lbnQw
HhcNMjAwNjE5MTkyODIwWhcNMjAwNzE5MTkyODIwWjBdMQswCQYDVQQGEwJVUzER
MA8GA1UECAwIVmlyZ2luaWExEzARBgNVBAcMCkFsZXhhbmRyaWExEDAOBgNVBAoM
B0NvbnRvc28xFDASBgNVBAsMC0RldmVsb3BtZW50MIIBtjCCASsGByqGSM44BAEw
ggEeAoGBAJyiyioeXx1O98gRCMEjlPKMpr79KrcDkoroghtuXO1U6Cx34pBRjOQm
QLDPqSOriEo5VuG6SJc/ppfZx9TrSrzqB26hKTUmiaOKmwpfIfzpi72wgsZeMOtU
7JQ+FThfGyS8VxGh6G0h7xw26B/9ALxRw25zO1cy9ZJs0EY3hsHzAhUA/4dpclsc
k8K+SkWBTcPfU+x7wTUCgYB4LP6UvrvIiiFPxhk7AEGMMr0MhcJ3hhsgKWukUqIY
sJKBM5MpKCnej5BHvnLXdKodIxygcKR4dJX7BRv69L+2RJk+UrYL1qBco5HpUslu
mA0e3gNdwRLoOoGD14dn1LD1LdESsyMgwfHHJ0RRkYwacgCVXsvHv/eAkA8qq136
dwOBhAACgYAHltgzkK3zD8yGdcGY0YgvN5l3lna1voLmcK+XtmehjMVy7OSSFICN
KybLBOvO8paydhCb1J0klkLPAoAjgP2cEd+KueeRyJpx+jD1MsjIEXIn5jtjXdUH
d0JJmHWAyHdNzmhXrXC7JLnj4ri7xMAV3GZGDpAnYvvL0LiXzFyomqNTMFEwHQYD
VR0OBBYEFF1l4ZrF3ND05CjGd//ev0dJLCB7MB8GA1UdIwQYMBaAFF1l4ZrF3ND0
5CjGd//ev0dJLCB7MA8GA1UdEwEB/wQFMAMBAf8wCwYJYIZIAWUDBAMCAzEAMC4C
FQD6plYf60MDCvMjf1yQ8SBaFX3YYwIVAKqRQklh2b0Qhv+US222hb8xySJV
-----END CERTIFICATE-----
Loading