Port missing X509Certificate members by steveharter · Pull Request #12240 · dotnet/corefx

steveharter · 2016-09-30T17:34:56Z

Per issue https://github.com/dotnet/corefx/issues/9986

Also converted X509 project to ns1.7.

@bartonjs please review

bartonjs · 2016-09-30T17:39:06Z

+            if (cert == null)
+                throw new ArgumentNullException(nameof(cert));
+
+            Pal = CertificatePal.FromHandle(cert.Handle); // Let FromHandle throw if certHandle is 0


This doesn't work for Linux (it loses private keys). You'll need to do a PAL clone.

bartonjs · 2016-09-30T17:45:56Z

                Assert.Equal(expectedThumbPrint, actualThumbprint);
+            };
+
+            using (X509Certificate2 c = new X509Certificate2(TestData.MsCertificate))


We'll need to add a copy constructor test for a cert which has a private key.

We should also add a test that proves they have independent lifetimes.

Create cert A from PFX

Create cert B from PFX

Get private key from B

Dispose A

Dispose key

Get another private key from B (and make sure it works)

bartonjs · 2016-09-30T17:53:45Z

        }

        [Theory, MemberData(nameof(BrainpoolCurvesPfx))]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp1_0, "dotnet/corefx#12235")]


What's the actual problem here? release/1.0.0 had this code in it. Did we register netcoreapp10 against netstandard1.5 instead of netstandard1.6? If so, is that being tracked as a problem? If it's by design, should we be testing as netstandard1.6 instead of netcoreapp10? (But don't we currently test as netcoreapp10? So why is this passing today?)

It is passing today because we x-compile the X509Certificates project against ns1.6 but don't do that anymore with this set of changes (so the assembly is coming from public nuget). My understanding is that the tests require code\fixes added in the 4.1.1 version of that assembly (corefx 1.1.0 branch). Once ns1.6 gets updates the tests should work again.

bartonjs · 2016-09-30T17:54:15Z

        }

        [Fact]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp1_0, "dotnet/corefx#12235")]


What about this test warrants the suppression?

Note that I removed these attributes as 1.0 tests are not currently running any longer. I need to follow up on this however they were not broken because of anything done in this PR

bartonjs · 2016-09-30T17:56:16Z

    "System.Runtime.InteropServices.RuntimeInformation": "4.3.0-beta-24522-03",
    "System.Security.Cryptography.Algorithms": "4.3.0-beta-24522-03",
    "System.Security.Cryptography.Cng": "4.3.0-beta-24522-03",
+    "System.Security.Cryptography.Csp": "4.3.0-beta-24522-03",


I don't see anything in this change which should require adding a reference to CSP... what's this for?

Note Csp is included in the implementation assembly (and was before)

Without it:

Internal\Cryptography\Pal.Windows\CertificatePal.PrivateKey.cs(61,41): error CS0246: The type or namespace name 'CspParameters' could not be found (are you missing a using directive or an assembly reference?) [C:\git\api22\src\System.Security.Cryptography.X509Certificates\src\System.Security.Cryptography.X509Certificates.csproj] Internal\Cryptography\Pal.Windows\CertificatePal.PrivateKey.cs(154,17): error CS0246: The type or namespace name 'CspParameters' could not be found (are you missing a using directive or an assembly reference?) [C:\git\api22\src\System.Security.Cryptography.X509Certificates\src\System.Security.Cryptography.X509Certificates.csproj]

justinvp · 2016-10-02T16:41:56Z


        [Fact]
-        [PlatformSpecific(PlatformID.Windows)]
+        [PlatformSpecific(Xunit.PlatformID.Windows)]


PlatformID has been renamed TestPlatforms. See #12284. Sorry for the merge conflicts.

steveharter · 2016-10-05T19:41:53Z

@morganbr please review (Jeremy OOF). Main change from initial review was to support copy ctor

morganbr · 2016-10-05T22:53:14Z

+        }
+
+        [Fact]
+        public static void TestCopyConstructor_Lifetime()


Can you please add a test for disposing in the reverse order as well?

It also doesn't look like you added the test Jeremy suggested (creating two separate objects from the same PFX and confirming their lifetimes are independent)

Is it also possible to test that when everything is finalized after duplication that native objects do get cleaned up?

Can you please add a test for disposing in the reverse order as well?

Done

It also doesn't look like you added the test Jeremy suggested (creating two separate objects from the same PFX and confirming their lifetimes are independent)

I read Jeremy's comment as being what I added as TestCopyConstructor_Lifetime() meaning the second cert was created with the first cert otherwise I don't see how I could have broken that. Note that some of the commit history was lost due to rebasing (I added TestCopyConstructor_Lifetime() after Jeremy's comment). However, I did add a new test verbatim that will test completely independent objects.

Is it also possible to test that when everything is finalized after duplication that native objects do get cleaned up?

Good suggestion however I don't think I have a good way to test that without reflection and\or using a gc.collect pattern.

Commit migrated from dotnet/corefx@91e4c88

dnfclas added the cla-already-signed label Sep 30, 2016

steveharter force-pushed the PortDsaCertEx branch from 9229785 to d16deb7 Compare September 30, 2016 17:56

bartonjs suggested changes Sep 30, 2016

View reviewed changes

steveharter force-pushed the PortDsaCertEx branch 4 times, most recently from 9419bf9 to 078cd35 Compare September 30, 2016 19:32

justinvp reviewed Oct 2, 2016

View reviewed changes

justinvp mentioned this pull request Oct 2, 2016

Remove Xunit.PlatformID enum dotnet/buildtools#1084

Merged

steveharter force-pushed the PortDsaCertEx branch from 6663c27 to ecfae1a Compare October 4, 2016 18:19

iamjasonp mentioned this pull request Oct 4, 2016

Change to use of X509Certificate2 copy constructor dotnet/wcf#1574

Closed

2 tasks

steveharter force-pushed the PortDsaCertEx branch 3 times, most recently from 0fd2fb7 to e6114b6 Compare October 5, 2016 18:13

steveharter force-pushed the PortDsaCertEx branch from e6114b6 to 5a3b905 Compare October 5, 2016 21:22

morganbr suggested changes Oct 5, 2016

View reviewed changes

morganbr approved these changes Oct 6, 2016

View reviewed changes

steveharter force-pushed the PortDsaCertEx branch 2 times, most recently from 960bda6 to db7d189 Compare October 6, 2016 00:17

steveharter added 2 commits October 5, 2016 19:56

Port missing X509Certificate members

cbe19fe

Support copy ctor; additional tests

67c5958

steveharter force-pushed the PortDsaCertEx branch from db7d189 to 63b2782 Compare October 6, 2016 00:56

Add additional tests

8831b7e

steveharter force-pushed the PortDsaCertEx branch from fbc27d5 to 8831b7e Compare October 6, 2016 13:47

steveharter merged commit 91e4c88 into dotnet:master Oct 6, 2016

karelz modified the milestone: 1.2.0 Dec 3, 2016

steveharter deleted the PortDsaCertEx branch April 15, 2019 18:20

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

Port missing X509Certificate members (dotnet/corefx#12240)

f120be5

Commit migrated from dotnet/corefx@91e4c88

Conversation

steveharter commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

steveharter commented Sep 30, 2016 •

edited

Loading

steveharter Sep 30, 2016 •

edited

Loading

steveharter Oct 5, 2016 •

edited

Loading