-
Notifications
You must be signed in to change notification settings - Fork 649
Replace use of obsolete X509Certificate2 API #5688
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 |
||
} | ||
} | ||
|
||
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; | ||
} | ||
} |
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); | ||
drewnoakes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
drewnoakes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
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."); | ||
} | ||
} | ||
} | ||
} |
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----- |
Uh oh!
There was an error while loading. Please reload this page.