Initial implementation of X509Certificates, HttpClient, and SslStream for macOS #16445
Conversation
… for macOS Broken by this change: * 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. * The X.509 My store for CurrentUser and LocalMachine are the default (user) and System keychains. * The X.509 Root store is an interpretation of the Apple SecTrustSettings data. * The X.509 Disallowed store hasn't been implemented yet, but should be a very small change. * Other X.509 stores cannot be created due to keychain complexity. HttpClient: * Initialization no longer wakes up OpenSSL SslStream: * New implementation based on Apple SecureTransport. * Currently has support for SNI (for AuthenticateAsClient)
out SafeKeychainHandle keychain); | ||
|
||
[DllImport(Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SecKeychainCreate")] | ||
private static extern int AppleCryptoNative_SecKeychainCreateTemporary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the "Temporary" here refer to? Asking since it's not in the actual entrypoint name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The out parameter. (SafeTemporaryKeychainHandle deletes the underlying keychain when it's done). So if we added "really create a keychain" it shouldn't use a temporary handle.
|
||
internal static SafeKeychainHandle SecKeychainItemCopyKeychain(SafeKeychainItemHandle item) | ||
{ | ||
var handle = SecKeychainItemCopyKeychain(item.DangerousGetHandle()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert that item is valid? Do we know that no one is potentially disposing it concurrently with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added the DangerousAddRef / DangerousRelease template to it. While it should be safe, may as well just guarantee it.
|
||
if (osStatus == 0) | ||
{ | ||
return keychain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for osStatus to be 0 even if keychain.IsInvalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it successfully reports "this key/certificate/identity does not belong to a keychain" (aka "belongs to the null keychain").
{ | ||
lock (s_lookup) | ||
{ | ||
s_lookup.Remove(handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible we may end up removing an item whose value still has dangerous refs added to it? Wondering if there may be the possibility of a leak here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean managed (DangerousAddRef()), the count is 0 when ReleaseHandle is called.
If you mean native, that's tracked by the CFRelease/CFRetain (and I've seen that a keychain being deleted while someone has a handle to it results in OSStatus errors, not crashes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't clear on what the value was associated with the key. But I looked more closely and realized you're using this table as a way of getting from the IntPtr to the SafeHandle that wraps it, so this ReleaseHandle will only be called when there are no references to the SafeHandle stored as the value.
case 1: | ||
return true; | ||
default: | ||
Debug.Fail($"AppleCryptoNative_SslIsHostnameMatch returned {result}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should never happen, i.e. we expect the subsequent throw to be dead in reality? I'm wondering if we should be propagating result info in the exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the shim function returns 1 on success, 0 when there's an OSStatus value to report, and any other value when the shim believes that the universe makes no sense. Since we're (we believe) off the rails when a default value is returned, it's good to say we're dead in the water.
The actual non-normalized-bool return code only makes sense to the shim function that's being called, to track down where we went off the rails. It's not stable codes, and doesn't have messages.
case ChannelBindingKind.Endpoint: | ||
bindingHandle = new SafeChannelBindingHandle(bindingType); | ||
QueryEndPointChannelBinding(context, bindingHandle); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What allows this case to be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs
Endpoint tokens were factored out to share code between macOS and other-Unix.
return false; | ||
|
||
return _data[_position] == expectedTag; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: any reason this isn't expressed more concisely as:
return HasData && _data[_position] == expectedTag;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy pasta. Simplified. 😄
// If any other backend is used and revocation is requested, we can't guarantee | ||
// that assertion. | ||
if (easy._handler.CheckCertificateRevocationList && | ||
!CurlSslVersionDescription.Equals("SecureTransport")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a P/Invoke. Can you change it to cache the value behind the property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I already changed it to cache :)
SetSslVersion(easy); | ||
} | ||
|
||
private static void SetSslVersion(EasyRequest easy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks the same as for the Unix version (I think). Are they the same? If so, could we factor this out into a partial file that's shared with both versions?
@@ -242,6 +277,8 @@ public static IEnumerable<object[]> UseCallback_ValidCertificate_ExpectedValuesD | |||
|
|||
[OuterLoop] // TODO: Issue #11345 | |||
[ConditionalFact(nameof(BackendDoesNotSupportCustomCertificateHandling))] | |||
// For macOS the "custom handling" means that revocation can't be *disabled*. So this test does not apply. | |||
[PlatformSpecific(TestPlatforms.Any & ~TestPlatforms.OSX)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be [PlatformSpecific(~TestPlatforms.OSX)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I think I did it to make the ~
more obvious, but I'll go ahead and fold it.
{ | ||
while (toWrite > 0) | ||
{ | ||
_toConnection.Enqueue(*readFrom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch... does this mean all reading and writing is done byte by byte, i.e. an Enqueue/Dequeue per byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I went with Queue so I didn't have to manage my own growing/shrinking producer/consumer model. It would be better with a locally managed circular buffer which is optimized for/by Buffer.BlockCopy; but with everything else that was changing I went with the KISS paradigm on the data transfer between native and managed.
return null; | ||
} | ||
|
||
byte[] data = _toConnection.ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I don't understand the design here... but there seems to be a bunch of things here that could be very expensive.
} | ||
} | ||
|
||
private static readonly Dictionary<TlsCipherSuite, TlsMapping> s_tlsLookup = new Dictionary<TlsCipherSuite, TlsMapping> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: consider passing a capacity in to the ctor
|
||
AppleCertificatePal applePal = (AppleCertificatePal)cert; | ||
|
||
ptrs[0] = applePal.CertificateHandle.DangerousGetHandle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My standard question/comment every time I see DangerousGetHandle: let's just make sure we've audited all of these to ensure we've either not yet given out the SafeHandle so we know it couldn't have been disposed, or it's been appropriately addref'd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cost an extra array, but I made this more defensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge piece of work. Nice job. My primary concern is that some of the operations that appear to be hot path also appear to be expensive, e.g. per-byte operations, array allocations, etc. That can be addressed subsequently, though.
|
||
// A whole lot of NULL is expected from this. | ||
// Any key or cert which isn't keychain-backed, and this is the primary way we'd find that out. | ||
if (keychain.IsInvalid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keychain could be null here (if item is null - from looking at the impl of the method).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Marshaller guarantees that it never returns a (C#) null SafeHandle. If it returns nullptr/NULL then it returns an object whose handle value is 0. In this case, it will report as IsInvalid.
So this is a check for "did the native method return nullptr".
return matches; | ||
} | ||
|
||
matches.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe matches can be null here in edge cases (if invalid data passed); not checking additional cases
} | ||
} | ||
|
||
internal static void ReleaseItem(IntPtr keychainItem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call it UntrackItem instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. It was "ReleaseItem" since it got called from "ReleaseHandle", and itself calls DangerousRelease; but "UntrackItem" is a better parallel to "TrackItem". And makes it more clear that I'm not (necessarily) freeing anything.
{ | ||
} | ||
|
||
protected override bool ReleaseHandle() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this only tracks the array allocation, and does not free individual items, correct?
return Array.Empty<string>(); | ||
} | ||
|
||
using (SafeCFArrayHandle dnArray = Interop.AppleCrypto.SslCopyCADistinguishedNames(sslContext)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear on how individual native array elements are cleanup up here (or whether they should be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, a CFArray CFRetain()s (AddRef) items added to it, and CFRelease()s (DownRef) those items when they are removed (including the array itself being CFRelease()d for the last time).
Further, OSX has a Get vs Create/Copy convention. If you "Get" an item, it has not been CFRetain()ed by the caller. If you Copy it, it has (therefore it's yours to CFRelease).
So, in this method we "Copy" the names (we own the array, all the items were up-reffed when the array was created to track that the array knows about them), then we "Get" the values (so we don't own them), we then extract the data we need, and don't (because ownsHandle is false) CFRelease the individal members. We do, at the end, CFRelease (via ReleaseHandle) the array, which down-refs all of the data elements.
If SecureTransport gave us shared elements they'll still be alive. If they were uniquely constructed for this return they'll get cleaned up by CFRelease.
Most of that is conveyed in Apple documentation by "follows The Create Rule"
Debug.Assert(certificate != null, "certificate != null"); | ||
Debug.Assert(certificate.HasPrivateKey, "certificate.HasPrivateKey"); | ||
|
||
X509Chain chain = TLSCertificateExtensions.BuildNewChain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be disposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like yes. Done.
size_t bufSize = static_cast<size_t>(bufLen); | ||
|
||
OSStatus status = SSLRead(sslContext, buf, bufSize, &writtenSize); | ||
*written = static_cast<uint32_t>(writtenSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm we should be compiling for 64-bit mode right, size_t should be 64-bit then (not 32)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we only support amd64 for macOS. I don't know if we'd ever be supporting x86 (or arm32, if they have one), but I think taking the conservative (32-bit) approach is safer.
I'm hardening this a bit to ensure that we don't lose data... though if SSLRead
doesn't respect bufSize (populated from a uint32_t) as a boundary then there's not a lot we can do for application safety.
@bartonjs Good to go it appears. FYI do not merge this branch to master yet. We don't yet have the capacity to handle a bunch of stuff moving to 10.12. |
… for macOS (dotnet/corefx#16445) Broken by this change: * 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. * The X.509 My store for CurrentUser and LocalMachine are the default (user) and System keychains. * The X.509 Root store is an interpretation of the Apple SecTrustSettings data. * The X.509 Disallowed store hasn't been implemented yet, but should be a very small change. * Other X.509 stores cannot be created due to keychain complexity. HttpClient: * Initialization no longer wakes up OpenSSL SslStream: * New implementation based on Apple SecureTransport. * Currently has support for SNI (for AuthenticateAsClient) Commit migrated from dotnet/corefx@3b19899
Broken by this change:
Tls | Tls12
is an invalid choice.In this change:
General:
X509Certificates:
due to the significant amount of data that has no public export in Apple's libraries.
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:
SslStream: