Initial implementation of X509Certificates, HttpClient, and SslStream for macOS #15970

bartonjs · 2017-02-08T23:52:08Z

Broken by this change:

X509Store has no implementation yet on macOS.
Exporting as a PFX is not yet implemented on macOS.
A lot of TLS CipherSuites have no metadata defined.
macOS does not support version skipping in TLS. So Tls | Tls12 is an invalid choice.

In this change:
General:

All OSStatus related exceptions now look up the error message.

X509Certificates:

X509Certificate moves to using SecCertificateRef from OpenSSL's X509.
X509 metadata comes from a managed reader after being loaded by Security.framework,
due to the significant amount of data that has no public export in Apple's libraries.
Significant code was factored out to be shared by OpenSSL and Apple implementations for X500DistinguishedName and X509Certficate2Collection.Find.
Loading a PFX (or, rather, the private keys from a PFX) via Apple's platform
requires importing into a Keychain, and a Keychain requires a file on disk.
A temporary keychain is created during cert loading and erased when safe.
Like the perphemeral key load on Windows this can leak files due to
abnormal program termination.

HttpClient:

Initialization no longer wakes up OpenSSL

SslStream:

New implementation based on Apple SecureTransport.
Currently has support for SNI (for AuthenticateAsClient)

bartonjs · 2017-02-08T23:56:34Z

This is a bit of a Work In Progress. But I'd gone on long enough that I wanted to get something out for review.

X509Certificates should fully pass tests. HttpClient will probably explode due to a TODO I left myself, and SslClient I'm trying to see what places the official tests are having trouble with in case my machine is behaving oddly.

My current remaining cleanup notes (aside from "implement deferred functionality"):

Do a full headers scrub (add/document the shim functions)
Remove all the printfs (they're all #defined away, but get rid of them
Remove ForceLoadPrivateKey (it should be vestigial, but something breaks when I do, need to investigate, since it means there's a problem somewhere else)
Curl VERIFYPEER/VERIFYHOST problem.
Fix comments that say REVIEW
ChainPal::Execute => "Handle this failure"
Two different TLS Protocol ID enums are in use. Oops

Part of what I'm looking for at this juncture is macro-comments; things that might take a day or two of restructuring to fix. Micro-comments are also quite welcome.

bartonjs · 2017-02-09T03:06:54Z

@dotnet-bot Test Outerloop OSX Debug please

steveharter

Cursory review done. Will do a detailed review when we pull things back

steveharter · 2017-02-09T00:41:42Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.cpp

+        return -2;
+    }
+
+    //*pOSStatus = SecTrustSetNetworkFetchAllowed(chain, allowNetwork);


Oh, good catch.

steveharter · 2017-02-09T00:43:55Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.cpp

+    }
+
+    CFDictionaryRef detailsAndStuff = SecTrustCopyResult(chain);
+    CFTypeRef detailsPtr = CFDictionaryGetValue(detailsAndStuff, CFSTR("TrustResultDetails"));


No error checking against detailsAndStuff and detailsPtr here

steveharter · 2017-02-09T00:44:32Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.cpp

+    CFRetain(details);
+    CFRelease(detailsAndStuff);
+
+    printf("Just the details:\n");


More debugging? (I'll stop mentioning that regarding printfs)

steveharter · 2017-02-09T00:47:35Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.h

+    PAL_X509ChainCtlNotTimeValid = 0x00020000,
+    PAL_X509ChainCtlNotSignatureValid = 0x00040000,
+    PAL_X509ChainCtlNotValidForUsage = 0x00080000,
+    PAL_X509ChainOfflineRevocation = 0x01000000,


Went from 80000 to 1000000 (an extra 0)?

steveharter · 2017-02-09T00:49:46Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_x509chain.h

+    PAL_X509ChainNoIssuanceChainPolicy = 0x02000000,
+    PAL_X509ChainExplicitDistrust = 0x04000000,
+    PAL_X509ChainHasNotSupportedCriticalExtension = 0x08000000,
+    PAL_X509ChainHasWeakSignature = 0x00100000,


PAL_X509ChainHasWeakSignature out of order or need extra 0

Values and ordering should be the same as they are in X509ChainStatusFlags.

steveharter · 2017-02-09T17:22:10Z

src/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

+                {
+                    // This is as likely as anything.  No error conditions are defined for
+                    // the OS function, and our shim only adds a NULL if isServer isn't a normalized bool.
+                    throw new OutOfMemoryException();


Seems like we can have something better.... Just raw Exception?

steveharter · 2017-02-09T17:24:35Z

src/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

+
+        private unsafe int WriteToConnection(void* connection, byte* data, void** dataLength)
+        {
+            ulong toWrite = (ulong)*dataLength;


verify connection == _toConnection?

connection will be NULL since we don't give them one. (It's their callback's way of maintaining object notions; but we got that captured in the delegate)

steveharter · 2017-02-09T17:28:57Z

src/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

+            X509ChainElementCollection elements = chain.ChainElements;
+
+            // We need to leave off the EE (first) and root (last) certificate from the intermediates.
+            X509Certificate2[] intermediateCerts = elements.Count < 3 ?


How does <3 guarantee that we need to remove the first and last? Vs. 4, 5, etc.?

If 1, it's just the first cert.
If 2, it's a cert directly from a root.

In both 1 and 2 we've removed the items we care about by being Array.Empty. At 3 we have things left over after we remove up to two.

And, really, it's because we're about to do Count - 2 as an array size.

steveharter · 2017-02-09T17:33:54Z

...Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/ManagedCertificateFinder.cs

@@ -0,0 +1,363 @@
+// Licensed to the .NET Foundation under one or more agreements.


Didn't review. Refactoring of OpenSslCertificateFinder.cs ?

steveharter · 2017-02-09T17:39:51Z

src/System.Security.Cryptography.X509Certificates/tests/Configurations.props

@@ -2,12 +2,14 @@
 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


FWIW I'm working on cleaning up the .props, test .csproj because most of our newer tests are not running due build changes and renaming of netstandard :(

… for macOS Broken by this change: * X509Store has no implementation yet on macOS. * Exporting as a PFX is not yet implemented on macOS. * A lot of TLS CipherSuites have no metadata defined. * macOS does not support version skipping in TLS. So `Tls | Tls12` is an invalid choice. In this change: General: * All OSStatus related exceptions now look up the error message. X509Certificates: * X509Certificate moves to using SecCertificateRef from OpenSSL's X509. * X509 metadata comes from a managed reader after being loaded by Security.framework, due to the significant amount of data that has no public export in Apple's libraries. * Significant code was factored out to be shared by OpenSSL and Apple implementations for X500DistinguishedName and X509Certficate2Collection.Find. * Loading a PFX (or, rather, the private keys from a PFX) via Apple's platform requires importing into a Keychain, and a Keychain requires a file on disk. A temporary keychain is created during cert loading and erased when safe. Like the perphemeral key load on Windows this can leak files due to abnormal program termination. HttpClient: * Initialization no longer wakes up OpenSSL SslStream: * New implementation based on Apple SecureTransport. * Currently has support for SNI (for AuthenticateAsClient)

Arg_InvalidHandle takes an argument name. Cryptography_OpenInvalidHandle just describes the state of things.

bartonjs · 2017-02-13T14:14:42Z

Okay, it seems that Sierra (10.12) fixed some bugs in SecTrustEvaluate. On El Capitan (10.11) it segfaults 50% of the time when the evaluation doesn't involve the SSL policy (the other 50% it reports a corrupt keychain when no keychain should be involved).

Hmm.

bartonjs · 2017-02-24T17:59:26Z

Enough changed that I made a completely new PR (#16445).

To get an idea of the changes: bartonjs/corefx@apple_x509_part1...bartonjs:apple_x509_part2

bartonjs self-assigned this Feb 8, 2017

dnfclas added the cla-already-signed label Feb 8, 2017

bartonjs requested review from stephentoub and steveharter February 8, 2017 23:52

karelz added this to the 2.0.0 milestone Feb 9, 2017

karelz added area-System.Security os-mac-os-x OS-X aka Mac OS labels Feb 9, 2017

steveharter reviewed Feb 9, 2017

View reviewed changes

bartonjs force-pushed the apple_x509_part1 branch from fdb3ed5 to 8cf6f2c Compare February 11, 2017 07:01

bartonjs added 2 commits February 11, 2017 10:17

Disable failing tests in X509Certificates

071d141

bartonjs force-pushed the apple_x509_part1 branch from 8cf6f2c to 071d141 Compare February 11, 2017 18:18

Fix two InvalidHandle exceptions.

d4ba084

Arg_InvalidHandle takes an argument name. Cryptography_OpenInvalidHandle just describes the state of things.

Disable chain building on macOS 10.11 and lower.

e7effe4

bartonjs closed this Feb 24, 2017

bartonjs removed their assignment Mar 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of X509Certificates, HttpClient, and SslStream for macOS #15970

Initial implementation of X509Certificates, HttpClient, and SslStream for macOS #15970

bartonjs commented Feb 8, 2017

bartonjs commented Feb 8, 2017 •

edited

bartonjs commented Feb 9, 2017

steveharter left a comment

steveharter Feb 9, 2017

bartonjs Feb 9, 2017

steveharter Feb 9, 2017

steveharter Feb 9, 2017

steveharter Feb 9, 2017

steveharter Feb 9, 2017

bartonjs Feb 9, 2017

steveharter Feb 9, 2017

steveharter Feb 9, 2017

bartonjs Feb 9, 2017

steveharter Feb 9, 2017

bartonjs Feb 9, 2017

steveharter Feb 9, 2017

bartonjs Feb 9, 2017

steveharter Feb 9, 2017

bartonjs commented Feb 13, 2017

bartonjs commented Feb 24, 2017

		@@ -0,0 +1,363 @@
		// Licensed to the .NET Foundation under one or more agreements.

		@@ -2,12 +2,14 @@
		<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

Initial implementation of X509Certificates, HttpClient, and SslStream for macOS #15970

Initial implementation of X509Certificates, HttpClient, and SslStream for macOS #15970

Conversation

bartonjs commented Feb 8, 2017

bartonjs commented Feb 8, 2017 • edited

bartonjs commented Feb 9, 2017

steveharter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Feb 13, 2017

bartonjs commented Feb 24, 2017

bartonjs commented Feb 8, 2017 •

edited