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

Don't use obsolete X509Certificate2 constructor on net9.0 #5911

Merged
merged 3 commits into from
Aug 1, 2024
Merged

Don't use obsolete X509Certificate2 constructor on net9.0 #5911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c56ec1
Don't use obsolete X509Certificate2 constructor on net9.0
akoeplinger Jul 10, 2024
085f2ff
Upgrade dotnet used in nuget.client source-build job
akoeplinger Jul 10, 2024
8e1a46c
Merge branch 'dev' into obsolete-x509certificate2
zivkan Jul 31, 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: 2 additions & 2 deletions eng/dotnet-build/global.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"tools": {
"dotnet": "9.0.100-preview.2.24157.14"
"dotnet": "9.0.100-preview.7.24357.2"
},
"msbuild-sdks": {
"Microsoft.DotNet.Arcade.Sdk": "9.0.0-beta.24208.8"
"Microsoft.DotNet.Arcade.Sdk": "9.0.0-beta.24352.2"
}
}
17 changes: 16 additions & 1 deletion src/NuGet.Core/NuGet.Commands/SignCommand/CertificateProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ internal static class CertificateProvider

private const int MACOS_INVALID_CERT = -25257;

#if NET9_0_OR_GREATER
private const int CRYPT_E_BAD_DECODE = unchecked((int)0x80092002);
#endif

#if IS_SIGNING_SUPPORTED && IS_CORECLR
//Generic exception ASN1 corrupted data
Expand Down Expand Up @@ -83,6 +86,9 @@ public static async Task<X509Certificate2Collection> GetCertificatesAsync(Certif
options.CertificatePath)));

case CRYPT_E_NO_MATCH_HRESULT:
#if NET9_0_OR_GREATER
case CRYPT_E_BAD_DECODE:
#endif
Comment on lines +89 to +91
Copy link
Contributor Author

@akoeplinger akoeplinger Jul 10, 2024 •
edited
Loading

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.

Copy link

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.

Copy link
Contributor Author

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 IS_SIGNING_SUPPORTED && IS_CORECLR
case OPENSSL_ASN1_CORRUPTED_DATA_ERROR:
#else
Expand Down Expand Up @@ -122,7 +128,12 @@ private static async Task<X509Certificate2> LoadCertificateFromFileAsync(Certifi

if (!string.IsNullOrEmpty(options.CertificatePassword))
{
cert = new X509Certificate2(options.CertificatePath, options.CertificatePassword); // use the password if the user provided it.
// use the password if the user provided it
#if NET9_0_OR_GREATER
cert = X509CertificateLoader.LoadPkcs12FromFile(options.CertificatePath, options.CertificatePassword);
#else
cert = new X509Certificate2(options.CertificatePath, options.CertificatePassword);
Copy link

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)

Copy link
Member

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.

#endif
}
else
{
Expand All @@ -147,8 +158,12 @@ private static async Task<X509Certificate2> LoadCertificateFromFileAsync(Certifi
throw;
}
}
#else
#if NET9_0_OR_GREATER
cert = X509CertificateLoader.LoadPkcs12FromFile(options.CertificatePath, null);
#else
cert = new X509Certificate2(options.CertificatePath);
#endif
#endif
}

Expand Down
4 changes: 4 additions & 0 deletions src/NuGet.Core/NuGet.Configuration/Settings/Items/FileClientCertItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,11 @@ public override IEnumerable<X509Certificate> Search()
Resources.FileCertItemPathFileNotExist));
}

#if NET9_0_OR_GREATER
return new[] { string.IsNullOrWhiteSpace(Password) ? X509CertificateLoader.LoadPkcs12FromFile(filePath, null) : X509CertificateLoader.LoadPkcs12FromFile(filePath, Password) };
#else
return new[] { string.IsNullOrWhiteSpace(Password) ? new X509Certificate2(filePath) : new X509Certificate2(filePath, Password) };
#endif
}

public void Update(string filePath, string? password, bool storePasswordInClearText)
Expand Down
4 changes: 4 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/Signing/Utility/CertificateChainUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,11 @@ public static IX509CertificateChain GetCertificateChain(X509Chain x509Chain)
// Return a new certificate object.
// This allows the chain and its chain element certificates to be disposed
// in both success and error cases.
#if NET9_0_OR_GREATER
certs.Add(X509CertificateLoader.LoadCertificate(item.Certificate.RawData));
#else
certs.Add(new X509Certificate2(item.Certificate.RawData));
#endif
}

return certs;
Expand Down
4 changes: 4 additions & 0 deletions test/NuGet.Core.Tests/NuGet.Configuration.Test/ClientCertificateProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,11 @@ private byte[] CreateCertificate()

private X509Certificate2 GetCertificate()
{
#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(CreateCertificate(), CertificatePassword);
#else
return new X509Certificate2(CreateCertificate(), CertificatePassword);
#endif
}

private void RemoveCertificateFromStorage()
Expand Down
4 changes: 4 additions & 0 deletions test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSource/ClientCertificatesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ private X509Certificate2 GetCertificate()
var end = start.AddYears(1);
var cert = request.CreateSelfSigned(start, end);
var data = cert.Export(X509ContentType.Pfx);
#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(data, null);
#else
return new X509Certificate2(data);
#endif
}
}
}
4 changes: 4 additions & 0 deletions test/TestUtilities/Test.Utility/SelfSignedCertificateMockServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,11 @@ private static X509Certificate2 GenerateSelfSignedCertificate()
var end = DateTime.UtcNow.AddYears(1);
var cert = request.CreateSelfSigned(start, end);
var certBytes = cert.Export(X509ContentType.Pfx, "password");
#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(certBytes, "password", X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(certBytes, "password", X509KeyStorageFlags.Exportable);
#endif
}
}

Expand Down
28 changes: 28 additions & 0 deletions test/TestUtilities/Test.Utility/Signing/SigningTestUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,11 @@ private static X509Certificate2 GenerateCertificate(
}
}

#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#endif
}

private static RSASignaturePadding ToPadding(this RSASignaturePaddingMode mode)
Expand Down Expand Up @@ -485,7 +489,11 @@ public static X509Certificate2 GenerateCertificate(string subjectName, RSA key)

var certResult = request.CreateSelfSigned(notBefore: DateTime.UtcNow.Subtract(TimeSpan.FromHours(1)), notAfter: DateTime.UtcNow.Add(TimeSpan.FromHours(1)));

#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#endif
}

public static X509Certificate2 GenerateCertificate(
Expand Down Expand Up @@ -522,7 +530,11 @@ public static X509Certificate2 GenerateCertificate(
using (var temp = request.Create(issuerDN, generator, notBefore, notAfter, serialNumber))
{
var certResult = temp.CopyWithPrivateKey(algorithm);
#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#endif
}
}

Expand Down Expand Up @@ -557,7 +569,11 @@ public static X509Certificate2 GenerateSelfIssuedCertificate(bool isCa)
var now = DateTime.UtcNow;
var certResult = request.CreateSelfSigned(notBefore: now, notAfter: now.AddHours(1));

#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(certResult.Export(X509ContentType.Pkcs12), password: (string)null, keyStorageFlags: X509KeyStorageFlags.Exportable);
#endif
}
}

Expand Down Expand Up @@ -620,7 +636,11 @@ public static SignedCms GenerateRepositoryCountersignedSignedCms(X509Certificate
/// </summary>
public static X509Certificate2 GetPublicCert(X509Certificate2 cert)
{
#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadCertificate(cert.Export(X509ContentType.Cert));
#else
return new X509Certificate2(cert.Export(X509ContentType.Cert));
#endif
}

/// <summary>
Expand All @@ -629,7 +649,11 @@ public static X509Certificate2 GetPublicCert(X509Certificate2 cert)
public static X509Certificate2 GetPublicCertWithPrivateKey(X509Certificate2 cert)
{
var password = new Guid().ToString();
#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(cert.Export(X509ContentType.Pfx, password), password, X509KeyStorageFlags.PersistKeySet | X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(cert.Export(X509ContentType.Pfx, password), password, X509KeyStorageFlags.PersistKeySet | X509KeyStorageFlags.Exportable);
#endif
}

public static TrustedTestCert<TestCertificate> GenerateTrustedTestCertificate()
Expand Down Expand Up @@ -726,7 +750,11 @@ public static X509Certificate2 GetCertificate(string name)
{
var bytes = GetResourceBytes(name);

#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadCertificate(bytes);
#else
return new X509Certificate2(bytes);
#endif
}

public static byte[] GetHash(X509Certificate2 certificate, NuGet.Common.HashAlgorithmName hashAlgorithm)
Expand Down
4 changes: 4 additions & 0 deletions test/TestUtilities/Test.Utility/TestServer/TcpListenerServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ private static X509Certificate2 GenerateSelfSignedCertificate()
var cert = request.CreateSelfSigned(start, end);
var certBytes = cert.Export(X509ContentType.Pfx, "password");

#if NET9_0_OR_GREATER
return X509CertificateLoader.LoadPkcs12(certBytes, "password", X509KeyStorageFlags.Exportable);
#else
return new X509Certificate2(certBytes, "password", X509KeyStorageFlags.Exportable);
#endif
}
}

Expand Down