Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fixing X509Certificate2Collection.Export on Unix with multiple certs and private key #26152

Merged
merged 3 commits into from
Jan 19, 2018
Merged

Fixing X509Certificate2Collection.Export on Unix with multiple certs and private key #26152

merged 3 commits into from
Jan 19, 2018

Conversation

ayende
Copy link
Contributor

@ayende ayende commented Jan 4, 2018

See previous discussion of this bug here: https://github.com/dotnet/corefx/issues/26125

This passes tests on Ubuntu 16.04 machine

@ayende
Fixing #26125 - When exporting a certificate collection with more tha…
000bb1e 
…n a single certificate and one of them has a private key, don't skip it
@ayende
Copy link
Contributor Author

ayende commented Jan 4, 2018

There are test failures on OSX, see:

   at Interop.AppleCrypto.X509Export(X509ContentType contentType, SafeCreateHandle cfPassphrase, IntPtr[] certHandles) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.cs:line 410
   at Interop.AppleCrypto.X509ExportPfx(IntPtr[] certHandles, SafePasswordHandle exportPassword) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.cs:line 443
   at Internal.Cryptography.Pal.StorePal.AppleCertificateExporter.ExportPkcs12(SafePasswordHandle password) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.ExportPal.cs:line 93
   at Internal.Cryptography.Pal.StorePal.AppleCertificateExporter.Export(X509ContentType contentType, SafePasswordHandle password) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.ExportPal.cs:line 45
   at System.Security.Cryptography.X509Certificates.X509Certificate2Collection.Export(X509ContentType contentType, String password) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Collection.cs:line 116
   at System.Security.Cryptography.X509Certificates.X509Certificate2Collection.Export(X509ContentType contentType) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Collection.cs:line 108
   at System.Security.Cryptography.X509Certificates.Tests.CollectionExportTests.CanAddMultipleCertsWithoutPrivate() in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs:line 37

This is for the test that test the existing (and correct) behavior, so I think that this was the case before my changed, but there was no test to trigger this behavior.
I also don't have an OSX available to test this.

There is also the Centos.73.Amd64.Open-Release-x64 failure, but I'm not sure if this is related, although the error is crypto related, so it might be, but any other Linux run was successful?

bartonjs
bartonjs suggested changes Jan 5, 2018
View reviewed changes
....Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/ExportProvider.cs Outdated
@@ -138,11 +140,17 @@ private byte[] ExportPfx(SafePasswordHandle password)
throw Interop.Crypto.CreateOpenSslCryptographicException();
}

return Interop.Crypto.OpenSslEncode(
var result = Interop.Crypto.OpenSslEncode(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improper use of var, please specify the type.

....Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/ExportProvider.cs Outdated
Interop.Crypto.GetPkcs12DerSize,
Interop.Crypto.EncodePkcs12,
pkcs12);

// ensure certs handle isn't finalized while raw handle(s) is in use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar: Something like "ensure cert handles aren't finalized while the raw handles are in use"

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
{
private static X509Certificate2 CreateCert(string name)
{
var rsa = new RSACryptoServiceProvider(2048);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSA.Create. Never RSACryptoServiceProvider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the new test moves to the other file I'd just continue using the test certificates that the rest of the library does, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the new test moves to the other file

I don't follow what you mean here?

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
{
var rsa = new RSACryptoServiceProvider(2048);
var csr = new CertificateRequest("CN=" + name, rsa, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);
var cert = csr.CreateSelfSigned(DateTime.Today.AddDays(-1), DateTime.Today.AddDays(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improper var (just return the value)

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
{
// without private keys
new X509Certificate2(CreateCert("one").Export(X509ContentType.Cert)),
new X509Certificate2(CreateCert("one").Export(X509ContentType.Cert)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using two different names seems better. Also, this test is redundant to CollectionTests::ExportUnrelatedPfx.

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 don't think that it tests the same thing, in particular, this test is testing the actual fix, and if it was duplicating the other, it wouldn't be needed, because ExportUnrelatedPfx would already be breaking on Linux.

In particular, ExportUnrelatedPfx isn't testing certs with private keys.

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
var col = new X509Certificate2Collection
{
// without private keys
new X509Certificate2(CreateCert("one").Export(X509ContentType.Cert)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I, personally, prefer reading RawData than calling Export(Cert). They're the same thing, though.

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
var col = new X509Certificate2Collection
{
// without private keys
new X509Certificate2(CreateCert("one").Export(X509ContentType.Cert)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All allocated certificates must be explicitly disposed (so that this test library can be run with finalization detection enabled to see if anything in the X509Certificates library is causing finalization)

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
}

[Fact]
public static void CanAddMultipleCertsWithSinglePrivateKey()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add this in CollectionTests.cs, at around line 867.

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
Assert.Equal(1, col.Cast<X509Certificate2>().Count(x => x.HasPrivateKey));
Assert.Equal(2, col.Count);

var buffer = col.Export(X509ContentType.Pfx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improper use of var, please specify the type.

src/System.Security.Cryptography.X509Certificates/tests/CollectionExportTests.cs Outdated
var buffer = col.Export(X509ContentType.Pfx);

var newCol = new X509Certificate2Collection();
newCol.Import(buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import via Cert.Import and the ImportedCollection disposable class. (See CollectionTests.ExportUnrelatedPfx for an example)

@bartonjs
Copy link
Member

bartonjs commented Jan 5, 2018

Thanks for jumping on this. Aside from a bad use of var it's just some test hygiene stuff to be changed.

There are test failures on OSX ... but there was no test to trigger this behavior.

Actually, there is 😄.

corefx/src/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs

Lines 735 to 737 in 71ff5e9

[Fact]
[ActiveIssue(16705, TestPlatforms.OSX)]
public static void ExportUnrelatedPfx()

@ayende
Copy link
Contributor Author

ayende commented Jan 8, 2018

Okay, updated the PR based on the review, thanks.

@bartonjs
Copy link
Member

bartonjs commented Jan 8, 2018

@dotnet-bot Test OSX x64 Debug Build please
Test Windows x86 Release Build please
Test CROSS Check please

@bartonjs
Copy link
Member

@ayende I pulled this down on a mac, and it looks like there's something different making it fail on macOS. If you want to add [ActiveIssue(26397, TestPlatforms.OSX)] to the test (to match the new issue that I just opened for macOS) then we should be able to merge this and at least get Linux fixed.

@ayende
Copy link
Contributor Author

ayende commented Jan 18, 2018

@bartonjs Done

@bartonjs
Copy link
Member

@dotnet-bot Test Linux x64 Release Build please
Test UWP CoreCLR x64 Debug Build please
Test Windows x64 Debug Build please
Test Windows x86 Release Build please

@bartonjs
Copy link
Member

@dotnet-bot Test Windows x64 Debug Build please (#26349)
@dotnet-bot Test Linux x64 Release Build please (drawing segfault?)

@bartonjs bartonjs merged commit ce3fe5d into dotnet:master Jan 19, 2018
@bartonjs
Copy link
Member

Thanks for taking care of this, @ayende.

@karelz karelz added this to the 2.1.0 milestone Jan 20, 2018
@bartonjs bartonjs removed their assignment Jan 29, 2018
@bartonjs bartonjs mentioned this pull request Jan 31, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bartonjs bartonjs bartonjs approved these changes

Assignees

@ayende ayende

Labels
area-System.Security
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ayende @bartonjs @karelz