-
Notifications
You must be signed in to change notification settings - Fork 686
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
Don't use obsolete X509Certificate2 constructor on net9.0 #5911
Conversation
#if NET9_0_OR_GREATER | ||
case CRYPT_E_BAD_DECODE: | ||
#endif |
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.
@bartonjs @vcsjones FYI this was uncovered by the ExecuteCommandAsync_WithEmptyPkcs7File_RaisesErrorsOnceAsync test, X509CertificateLoader returns a different hresult in this case on !Windows because we're now returning this from managed code.
Might be something we should call out in the breaking change doc.
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.
HResult values from exceptions aren't guaranteed (especially when they can change because of an OS change); so I don't anticipate putting it in the breaking change documentation.
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.
I'd agree with you in an ideal world, but this real world code makes me think we should at least give a heads up in the doc :)
#if NET9_0_OR_GREATER | ||
cert = X509CertificateLoader.LoadPkcs12FromFile(options.CertificatePath, options.CertificatePassword); | ||
#else | ||
cert = new X509Certificate2(options.CertificatePath, options.CertificatePassword); |
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.
The preferred fix is to add a package reference to Microsoft.Bcl.Cryptography and always call X509CertificateLoader; but we don't have the power to mark things Obsolete in older TFMs.
If adding the package is unacceptable, then this works. The main concern is "don't get surprised by a PFX", but this expects a PFX. There are also some subtle differences in how PFX loading works on Windows between the two, mainly fixing issues with loading very old files or loading the same file in parallel (making Microsoft.Bcl.Cryptography/X509CertificateLoader the better choice)
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.
Is X509CertificateLoader
brand new, not even in .NET 9 Preview 6? I had a quick look at the preview 6 package on nuget.org, but it doesn't appear to have the API.
For this reason, plus the fact that NuGet doesn't use the Arcade SDK, means I don't think NuGet can use this package for .NET 9.0.100 SDK. Maybe after .NET 9 goes GA. For now, I think we need to take this conditional compilation change.
bump |
#if NET9_0_OR_GREATER | ||
cert = X509CertificateLoader.LoadPkcs12FromFile(options.CertificatePath, options.CertificatePassword); | ||
#else | ||
cert = new X509Certificate2(options.CertificatePath, options.CertificatePassword); |
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.
Is X509CertificateLoader
brand new, not even in .NET 9 Preview 6? I had a quick look at the preview 6 package on nuget.org, but it doesn't appear to have the API.
For this reason, plus the fact that NuGet doesn't use the Arcade SDK, means I don't think NuGet can use this package for .NET 9.0.100 SDK. Maybe after .NET 9 goes GA. For now, I think we need to take this conditional compilation change.
.NET 9 Preview 7, also available via the Microsoft.Bcl.Cryptography NuGet package (which won't work for .NET 9 Preview 1-6, since the package expects the type to be inbox for .NET 9) |
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
Bug
Fixes: NuGet/Home#13612
Description
When building nuget.client in the VMR for source-build we're targeting net9.0. This means we get the new obsoletion of X509Certificate2 ctor: dotnet/docs#41662.
This PR fixes all instances to use the recommended X509CertificateLoader replacement.
PR Checklist