Replace use of obsolete X509Certificate2 API #5688
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. 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. 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; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. 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."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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----- |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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