Replace use of obsolete X509Certificate2 API #5688
Conversation
In .NET 9 the `X509Certificate2` constructor we were using is being made obsolete. Instead, we must use the new `X509CertificateLoader` class. This new class was added in .NET 9, but is available for downlevel use via the `Microsoft.Bcl.Cryptography` package, which is added in this change.
| @@ -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 --> | |||
There was a problem hiding this comment.
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.
@joperezr @davidfowl any concerns here?
There was a problem hiding this comment.
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
|
Are there tests of the client loading certificates? If not, some should be added. |
We have a manual test script for these scenarios for now. |
dotnet-policy-service
bot
added
the
needs-author-action
dotnet-policy-service
bot
removed
the
needs-author-action
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| var filePath = _dashboardOptions.ResourceServiceClient.ClientCertificate.FilePath; | ||
| var password = _dashboardOptions.ResourceServiceClient.ClientCertificate.Password; | ||
|
|
||
| return [new X509Certificate2(filePath, password)]; |
There was a problem hiding this comment.
"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.
| 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
- 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 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.
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.
| [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")
|
@bartonjs thanks for the super detailed notes. Given the above, would you suggest just continuing to use the obsolete API and suppressing the warning once we move to |
Once you move to 9, X509CertificateLoader will be part of the platform. At that time, I recommend you use it rather than the obsolete methods. I'd probably take the break to only support PKCS#12/PFX at that time (as well as the alternative of loading the cert from a cert store), as none of the other formats make sense in context. |
|
This is on hold until the repo moves to .NET 9. |
That won't happen any time soon though, or where you referring once .NET 9 ships and we can add a dependency to the OOB package? If not, then we can probably just close this one based on @bartonjs feedback. |
|
Let's close and revisit as needed in future. |
Fixes #5461
In .NET 9 the
X509Certificate2constructor we were using is being made obsolete. Instead, we must use the newX509CertificateLoaderclass.This new class was added in .NET 9, but is available for downlevel use via the
Microsoft.Bcl.Cryptographypackage, which is added in this change.Checklist
<remarks />and<code />elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow