Add support for importing PKCS7 files as X509Certificate2Collections. #2912

bartonjs · 2015-08-21T05:03:36Z

Several tests were added for X509Certificate2Collection Import, particularly regarding PKCS#7 content, and the new and existing tests were extracted to CollectionImportTests.

A few tests were added which have no file contents yet, issue 2635 tracks getting an updated testdata package to test reading the files by name. The tests were verified manually, and in general have their file contents included in the TestData class anyways.

Reading a PKCS#7 blob from X509Certificate2 will throw a CryptographicException whether or not the file is signed, issue 2910 tracks making it follow the Windows/MSDN behavior on the success path as well (there is no test asset available currently to see the success path on Windows).

All PKCS file structure interpretation was moved to PkcsFormatReader, and the functions were consolidated across the matrix of [File vs Blob] x [Collection vs Single]. This should prevent the behaviors from getting out of sync.

X509Certificate2Collection.Import now also works for X509-DER and X509-PEM inputs, matching the Windows behavior.

Fixes #2849.

mmitche · 2015-08-21T05:11:35Z

@dotnet-bot test this please (either GitHub or mac #2 is acting up)

bartonjs · 2015-08-21T07:15:47Z

@dotnet-bot test this please (and if you ask it nicely, I bet the Windows crypto intermittent failure will behave; @bartonjs should really figure out what's going on there...)

stephentoub · 2015-08-21T12:19:43Z

@dotnet-bot test this please

stephentoub · 2015-08-21T12:21:57Z

src/Common/src/Microsoft/Win32/SafeHandles/SafeX509Handles.Unix.cs

Why is the SetHandle necessary?

Or, maybe I've misunderstood what "Shared" means here. I was assuming it meant that multiple consumers could be holding on to the same instance, and none of them should free/dispose anything, so we make ReleaseHandle a nop. Not the case?

We have a SafeHandle tracking the PKCS#7 object. Then someone wants to make use of p7->d.signed->certs. We shouldn't free that value when we're done with it, because it may either cause problems with freeing the PKCS#7 later, or we merely have blocked ourselves out of using the data any longer.

So this is shared with another safehandle.

Having written out the explanation, I'm wondering if this SafeHandle needs a field reference to the owning SafeHandle (to ensure it's lifetime WRT finalization), have IsInvalid take that handle's IsInvalid into account, and ReleaseHandle just set that field to null.

Or maybe just downgrade it to IntPtr.

Since I've clearly caused confusion, I'm very open to feedback on making it easier to follow (and if it's just a comment that's certainly doable, too).

Thanks for the explanation.

I'm wondering if this SafeHandle needs a field reference to the owning SafeHandle (to ensure it's lifetime WRT finalization), have IsInvalid take that handle's IsInvalid into account, and ReleaseHandle just set that field to null.

I'm actually not sure what the best practice is for SafeHandles that wrap portions of other SafeHandles. @jkotas? Maybe the creation of the "child" handle AddRef's the parent and its ReleaseHandle ReleaseRef's the parent?

Maybe the creation of the "child" handle AddRef's the parent and its ReleaseHandle ReleaseRef's the parent?

Yes, this is the right patern to use for safe handles that point to container members, and where the container has to be kept alive for as long as the container member is alive.

(Windows crypto and networking APIs tend to have this problem too - I believe that you will find this in desktop reference sources if you look around.)

I found an example in SafeCredentialReference; then added some Interlocked.Increment and Interlocked.Decrement and Console.WriteLines to let me test this out:

Starting: System.Security.Cryptography.X509Certificates.Tests There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) There is/are 1 active SafePkcs7Handle(s) There is/are 0 remaining SafePkcs7Handle(s) Finished: System.Security.Cryptography.X509Certificates.Tests

So things work the way that I expected them to. Thanks @jkotas.

stephentoub · 2015-08-21T12:33:35Z

Some minor questions, but LGTM.

stephentoub · 2015-08-21T12:56:42Z

CI seems to be consistently failing in the TestPrivateKey test:

Discovering: System.Security.Cryptography.X509Certificates.Tests
  Discovered:  System.Security.Cryptography.X509Certificates.Tests
  Starting:    System.Security.Cryptography.X509Certificates.Tests
     System.Security.Cryptography.X509Certificates.Tests.PfxTests.TestPrivateKey [FAIL]
        System.Security.Cryptography.CryptographicException : OpenCSP failed with error code 2148073494.
        Stack Trace:
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.Csp\src\System\Security\Cryptography\CapiHelper.cs(309,0): at Internal.NativeCrypto.CapiHelper.CreateProvHandle(CspParameters parameters, Boolean randomKeyContainer)
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.Csp\src\System\Security\Cryptography\CapiHelper.cs(687,0): at Internal.NativeCrypto.CapiHelper.GetKeyPairHelper(CspAlgorithmType keyType, CspParameters parameters, Boolean randomKeyContainer, Int32 keySize, SafeProvHandle& safeProvHandle, SafeKeyHandle& safeKeyHandle)
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.Csp\src\System\Security\Cryptography\RSACryptoServiceProvider.cs(89,0): at System.Security.Cryptography.RSACryptoServiceProvider.GetKeyPair()
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.Csp\src\System\Security\Cryptography\RSACryptoServiceProvider.cs(69,0): at System.Security.Cryptography.RSACryptoServiceProvider..ctor(Int32 keySize, CspParameters parameters, Boolean useDefaultKeySize)
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.Csp\src\System\Security\Cryptography\RSACryptoServiceProvider.cs(49,0): at System.Security.Cryptography.RSACryptoServiceProvider..ctor(CspParameters parameters)
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Pal.Windows\CertificatePal.PrivateKey.cs(30,0): at Internal.Cryptography.Pal.CertificatePal.GetRSAPrivateKey()
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.X509Certificates\src\System\Security\Cryptography\X509Certificates\RSACertificateExtensions.cs(55,0): at System.Security.Cryptography.X509Certificates.RSACertificateExtensions.GetRSAPrivateKey(X509Certificate2 certificate)
           d:\j\workspace\dotnet_corefx_windows_debug_prtest\src\System.Security.Cryptography.X509Certificates\tests\PfxTests.cs(67,0): at System.Security.Cryptography.X509Certificates.Tests.PfxTests.TestPrivateKey()
  Finished:    System.Security.Cryptography.X509Certificates.Tests

stephentoub · 2015-08-21T12:56:59Z

@dotnet-bot test this please

bartonjs · 2015-08-21T15:14:27Z

I've never seen this particular error before:

07:00:07 + ./run-test.sh --configuration Debug --os Linux --coreclr-bins /mnt/j/workspace/dotnet_corefx_linux_debug_tst_prtest@2/bin/Product/Linux.x64.Release/ --mscorlib-bins /mnt/j/workspace/dotnet_corefx_linux_debug_tst_prtest@2/bin/Product/Linux.x64.Release/
07:00:07 Pulling down Microsoft.DotNet.CoreFx.Linux.TemporaryTestHost.0.0.2-prerelease.nupkg
07:00:08 Unzipping to packages/Microsoft.DotNet.CoreFx.Linux.TemporaryTestHost.0.0.2-prerelease
07:00:08 [Microsoft.DotNet.CoreFx.Linux.TemporaryTestHost.0.0.2-prerelease.nupkg]
07:00:08   End-of-central-directory signature not found.  Either this file is not
07:00:08   a zipfile, or it constitutes one disk of a multi-part archive.  In the
07:00:08   latter case the central directory and zipfile comment will be found on
07:00:08   the last disk(s) of this archive.
07:00:08 unzip:  cannot find zipfile directory in one of Microsoft.DotNet.CoreFx.Linux.TemporaryTestHost.0.0.2-prerelease.nupkg or
07:00:08         Microsoft.DotNet.CoreFx.Linux.TemporaryTestHost.0.0.2-prerelease.nupkg.zip, and cannot find Microsoft.DotNet.CoreFx.Linux.TemporaryTestHost.0.0.2-prerelease.nupkg.ZIP, period.
07:00:08 Package not laid out as expected
07:00:08 Build step 'Execute shell' marked build as failure

So it still hasn't quite made it to "run the tests on a machine other than mine".

bartonjs · 2015-08-21T15:15:36Z

@dotnet-bot test this please (fail things all you like, I just want to see the linux results)

mmitche · 2015-08-21T16:15:45Z

@bartonjs I saw that on OpenSuSE...let me poke around a bit.

bartonjs · 2015-08-21T18:24:53Z

New commits, for your viewing pleasure. I'll squash when people seem happy with the changes.

bartonjs · 2015-08-21T19:05:02Z

@dotnet-bot test this please (I pushed new commits while you were napping)

stephentoub · 2015-08-22T02:55:07Z

LGTM

Several tests were added for X509Certificate2Collection Import, particularly regarding PKCS#7 content, and the new and existing tests were extracted to CollectionImportTests. A few tests were added which have no file contents yet, issue 2635 tracks getting an updated testdata package to test reading the files by name. The tests were verified manually, and in general have their file contents included in the TestData class anyways. Reading a PKCS#7 blob from X509Certificate2 will throw a CryptographicException whether or not the file is signed, issue 2910 tracks making it follow the Windows/MSDN behavior on the success path as well (there is no test asset available currently to see the success path on Windows). All PKCS file structure interpretation was moved to PkcsFormatReader, and the functions were consolidated across the matrix of [File vs Blob] x [Collection vs Single]. This should prevent the behaviors from getting out of sync. X509Certificate2Collection.Import now also works for X509-DER and X509-PEM inputs, matching the Windows behavior.

Add support for importing PKCS7 files as X509Certificate2Collections.

Add support for importing PKCS7 files as X509Certificate2Collections. Commit migrated from dotnet/corefx@36969f1

joshfree added the 2 - In Progress label Aug 21, 2015

dnfclas added the cla-already-signed label Aug 21, 2015

stephentoub reviewed Aug 21, 2015
View reviewed changes

bartonjs force-pushed the unix_pkcs7 branch from 6482d63 to a2cef28 Compare August 22, 2015 21:20

bartonjs added a commit that referenced this pull request Aug 22, 2015

Merge pull request #2912 from bartonjs/unix_pkcs7

36969f1

Add support for importing PKCS7 files as X509Certificate2Collections.

bartonjs merged commit 36969f1 into dotnet:master Aug 22, 2015

joshfree removed the 2 - In Progress label Aug 22, 2015

bartonjs deleted the unix_pkcs7 branch August 22, 2015 22:22

karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#2912 from bartonjs/unix_pkcs7

fc790ab

Add support for importing PKCS7 files as X509Certificate2Collections. Commit migrated from dotnet/corefx@36969f1

Add support for importing PKCS7 files as X509Certificate2Collections. #2912

Add support for importing PKCS7 files as X509Certificate2Collections. #2912

Uh oh!

Conversation

bartonjs commented Aug 21, 2015

Uh oh!

mmitche commented Aug 21, 2015

Uh oh!

bartonjs commented Aug 21, 2015

Uh oh!

stephentoub commented Aug 21, 2015

Uh oh!

stephentoub Aug 21, 2015

Choose a reason for hiding this comment

Uh oh!

stephentoub Aug 21, 2015

Choose a reason for hiding this comment

Uh oh!

bartonjs Aug 21, 2015

Choose a reason for hiding this comment

Uh oh!

stephentoub Aug 21, 2015

Choose a reason for hiding this comment

Uh oh!

jkotas Aug 21, 2015

Choose a reason for hiding this comment

Uh oh!

bartonjs Aug 21, 2015

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Aug 21, 2015

Uh oh!

stephentoub commented Aug 21, 2015

Uh oh!

stephentoub commented Aug 21, 2015

Uh oh!

bartonjs commented Aug 21, 2015

Uh oh!

bartonjs commented Aug 21, 2015

Uh oh!

mmitche commented Aug 21, 2015

Uh oh!

bartonjs commented Aug 21, 2015

Uh oh!

bartonjs commented Aug 21, 2015

Uh oh!

stephentoub commented Aug 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants