[appletls]: Use SecIdentityCreate() to avoid using the Mac keychain. by baulig · Pull Request #4671 · mono/mono

baulig · 2017-04-11T15:01:32Z

Reading Certificates from the Mac Keychain

Reading the private key from the keychain is a new feature introduced with
AppleTls on XamMac and iOS. On Desktop Mono, this new feature has several
known issues and it also did not received any testing yet. We go back to the old
way of doing things, which is to explicitly provide an X509Certificate2 with a
private key.

Keychain Dialog Popups

When using Xamarin.Mac or Xamarin.iOS, we try to search the keychain
for the certificate and private key.

On Xamarin.iOS, this is easy because each app has its own keychain.

On Xamarin.Mac, the .app package needs to be trusted via code-sign
to get permission to access the user's keychain. [FIXME: I still have to
research how to actually do that.] Without this, you will get a popup
message each time, asking you whether you want to allow the app to access
the keychain, but you can make these go away by selecting "Trust always".

On Desktop Mono, this is problematic because selecting "Trust always"
give the 'mono' binary (and thus everything you'll ever run with Mono)
permission to retrieve the private key from the keychain.

This code would also trigger constant keychain popup messages,
which could only be suppressed by granting full trust. It also makes it
impossible to run Mono in headless mode.

SecIdentityCreate

To avoid these problems, we are currently using an undocumented API
called SecIdentityRef() to avoid using the Mac keychain whenever a
X509Certificate2 with a private key is used.

On iOS and XamMac, you can still provide the X509Certificate without
a private key - in this case, a keychain search will be performed (and you
may get a popup message on XamMac).

chamons · 2017-04-11T15:17:33Z

mcs/class/System/Mono.AppleTls/AppleCertificateHelper.cs

 				return SecKeyChain.FindIdentity (secCert, true);
 			}
+#else
+			return null;


Are internal users of this API going to check for null or could this cause NRE?

Null is a valid return value - the caller is responsible for throwing an exception.

chamons · 2017-04-11T15:24:39Z

mcs/class/System/Mono.AppleTls/Certificate.cs

 				CFObject.CFRetain (handle);
 		}

+		/*


Should ^ ctor set this.owner explicitly?

Hmm, we could also make owner the object that's CFRelease()d and set it to IntPtr.Zero if we don't "own" it.

chamons · 2017-04-11T15:28:49Z

mcs/class/System/Mono.AppleTls/Certificate.cs

+			if (owner != IntPtr.Zero) {
+				CFObject.CFRelease (owner);
+				owner = handle = IntPtr.Zero;
+			} else if (handle != IntPtr.Zero) {


It seems a tad weird to have this else in the Dispose. Not necessary wrong, but not expected.

chamons · 2017-04-11T15:29:17Z

mcs/class/System/Mono.AppleTls/Certificate.cs

+		{
+			var descriptorHandle = CFString.Create (descriptor);
+			if (descriptorHandle == null)
+				throw new InvalidOperationException ();


Is this the right exception?

Maybe OutOfMemoryException()?

If that method is used (I did not see it used in the diff) then it seems this should be a null check on descriptor to throw an ArgumentNullException

chamons · 2017-04-11T15:29:41Z

mcs/class/System/Mono.AppleTls/Certificate.cs

+
+				return new SecAccess (accessRef, true);
+			} finally {
+				descriptorHandle.Dispose ();


Could we use a using block here instead?

Bad idea, but we're incorrectly doing it in so many other places. We'd have to make all those CFString.Create() etc. functions throw instead of possibly returning null.

The idea is not to change CFString.Create() but keep the code as it but replace try/finally with using

chamons · 2017-04-11T15:32:00Z

mcs/class/System/Mono.AppleTls/ImportExport.cs

+
+			IntPtr result = IntPtr.Zero;
+			SecStatusCode status = SecStatusCode.Success;
+			for (var test = 0; test < 13; test++) {


Why 13 here?

Oops - that entire loop must be removed!

chamons · 2017-04-11T15:32:16Z

mcs/class/System/Mono.AppleTls/ImportExport.cs

+			using (var cert = new SecCertificate (certificate)) {
+				var identity = SecIdentityCreate (IntPtr.Zero, cert.Handle, key.Handle);
+				if (CFType.GetTypeID (identity) != SecIdentity.GetTypeID ())
+					throw new InvalidOperationException ();


Right exception here?

It should never happen - do we have some kind of "internal error" exception?

marek-safar · 2017-04-11T15:49:21Z

mcs/class/System/Mono.AppleTls/ImportExport.cs

+
+			IntPtr result = IntPtr.Zero;
+			SecStatusCode status = SecStatusCode.Success;
+			keyParams = new SecItemImportExportKeyParameters ();


What is this line for?

marek-safar · 2017-04-11T15:50:12Z

mcs/class/System/Mono.AppleTls/ImportExport.cs

+				Marshal.StructureToPtr (keyParams.Value, keyParamsPtr, false);
+			}
+
+			IntPtr result = IntPtr.Zero;


You don't need to explicitly set there when they are set by SecItemImport

marek-safar · 2017-04-11T15:52:45Z

mcs/class/System/Mono.AppleTls/ImportExport.cs

+		// Native enum; don't change.
+		enum SecExternalFormat : int {
+			Unknown = 0,
+			OpenSSL,


If it's native enum, set OpenSSL value explicitly

marek-safar · 2017-04-11T15:52:58Z

mcs/class/System/Mono.AppleTls/ImportExport.cs

+
+		// Native enum; don't change.
+		enum SecExternalItemType : int {
+			Unknown,


Explicitly set values

marek-safar · 2017-04-11T15:55:31Z

mcs/class/System/Mono.AppleTls/Items.cs

+
+			IntPtr ptr;
+			result = SecItem.SecItemCopyMatching (query.Handle, out ptr);
+			if ((result == SecStatusCode.Success) && (ptr != IntPtr.Zero)) {


Extra parens

marek-safar · 2017-04-11T15:55:57Z

mcs/class/System/Mono.AppleTls/Items.cs

+			IntPtr ptr;
+			result = SecItem.SecItemCopyMatching (query.Handle, out ptr);
+			if ((result == SecStatusCode.Success) && (ptr != IntPtr.Zero)) {
+				var array = CFArray.ArrayFromHandle<INativeObject> (ptr, p => {


why not just return CFArray.ArrayFromHandle...

Not my code, I only moved it around a little bit. I just made this private as it's only supported as called from FindIdentity().

In some situations, the result array may contain elements of different types.

marek-safar · 2017-04-11T15:56:22Z

mcs/class/System/Mono.AppleTls/Items.cs

+					IntPtr cfType = CFType.GetTypeID (p);
+					if (cfType == SecCertificate.GetTypeID ())
+						return new SecCertificate (p, true);
+					else if (cfType == SecKey.GetTypeID ())


Redundant else(s)

spouliot

in progress...

spouliot · 2017-04-11T15:50:13Z

mcs/class/System/Mono.AppleTls/Certificate.cs

+		[DllImport (AppleTlsContext.SecurityLibrary)]
+		extern static /* OSStatus */ SecStatusCode SecAccessCreate (/* CFStringRef */ IntPtr descriptor,  /* CFArrayRef */ IntPtr trustedList, /* SecAccessRef _Nullable * */ out IntPtr accessRef);
+
+		public static SecAccess Create (string descriptor)


where is that method used ?

It will be used later on - after we re-enabled the keychain on mobile.

spouliot · 2017-04-11T15:51:12Z

mcs/class/System/Mono.AppleTls/Certificate.cs

+		{
+			var descriptorHandle = CFString.Create (descriptor);
+			if (descriptorHandle == null)
+				throw new InvalidOperationException ();


If that method is used (I did not see it used in the diff) then it seems this should be a null check on descriptor to throw an ArgumentNullException

spouliot · 2017-04-11T15:54:27Z

mcs/class/System/Mono.AppleTls/Enums.cs

 	// values are defined in Security.framework/Headers/SecBase.h 
 	enum SecStatusCode {
 		Success 							= 0,
-		DuplicateItem	 					= -25299,


It is also already public in Xamarin.*.dll so we'll end up shipping it twice (lots of string metadata that only grows over time).

I understand it's nice for debugging the internals but it's not exposed. Maybe it could live under a #if XXX ?

We could probably remove some of the more obscure values.

…hen we have a private key. Reading Certificates from the Mac Keychain ========================================== Reading the private key from the keychain is a new feature introduced with AppleTls on XamMac and iOS. On Desktop Mono, this new feature has several known issues and it also did not received any testing yet. We go back to the old way of doing things, which is to explicitly provide an X509Certificate2 with a private key. Keychain Dialog Popups ====================== When using Xamarin.Mac or Xamarin.iOS, we try to search the keychain for the certificate and private key. On Xamarin.iOS, this is easy because each app has its own keychain. On Xamarin.Mac, the .app package needs to be trusted via code-sign to get permission to access the user's keychain. [FIXME: I still have to research how to actually do that.] Without this, you will get a popup message each time, asking you whether you want to allow the app to access the keychain, but you can make these go away by selecting "Trust always". On Desktop Mono, this is problematic because selecting "Trust always" give the 'mono' binary (and thus everything you'll ever run with Mono) permission to retrieve the private key from the keychain. This code would also trigger constant keychain popup messages, which could only be suppressed by granting full trust. It also makes it impossible to run Mono in headless mode. SecIdentityCreate ================= To avoid these problems, we are currently using an undocumented API called SecIdentityRef() to avoid using the Mac keychain whenever a X509Certificate2 with a private key is used. On iOS and XamMac, you can still provide the X509Certificate without a private key - in this case, a keychain search will be performed (and you may get a popup message on XamMac).

The issues are now fixed.

baulig · 2017-04-11T18:54:21Z

I did some cleanups, fixed all the problems you guys addressed and added some documentation.

Could you please have another look at this to see whether this can now go in? @marek-safar @chamons

marek-safar · 2017-04-11T21:19:59Z

mcs/class/System/Mono.AppleTls/Items.cs

+		internal SecKeyChain (IntPtr handle, bool owns = false)
+		{
+			if (handle == IntPtr.Zero)
+				throw new Exception ("Invalid handle");


Should be ArgumentException

…4671) * [appletls]: Use SecIdentityCreate() to avoid using the Mac keychain when we have a private key. Reading Certificates from the Mac Keychain ========================================== Reading the private key from the keychain is a new feature introduced with AppleTls on XamMac and iOS. On Desktop Mono, this new feature has several known issues and it also did not received any testing yet. We go back to the old way of doing things, which is to explicitly provide an X509Certificate2 with a private key. Keychain Dialog Popups ====================== When using Xamarin.Mac or Xamarin.iOS, we try to search the keychain for the certificate and private key. On Xamarin.iOS, this is easy because each app has its own keychain. On Xamarin.Mac, the .app package needs to be trusted via code-sign to get permission to access the user's keychain. [FIXME: I still have to research how to actually do that.] Without this, you will get a popup message each time, asking you whether you want to allow the app to access the keychain, but you can make these go away by selecting "Trust always". On Desktop Mono, this is problematic because selecting "Trust always" give the 'mono' binary (and thus everything you'll ever run with Mono) permission to retrieve the private key from the keychain. This code would also trigger constant keychain popup messages, which could only be suppressed by granting full trust. It also makes it impossible to run Mono in headless mode. SecIdentityCreate ================= To avoid these problems, we are currently using an undocumented API called SecIdentityRef() to avoid using the Mac keychain whenever a X509Certificate2 with a private key is used. On iOS and XamMac, you can still provide the X509Certificate without a private key - in this case, a keychain search will be performed (and you may get a popup message on XamMac). (cherry picked from commit 1eb27c1)

Temporary PR, preparing the cherry-picking of Mono's PR #4671 (mono/mono#4671) into mono/2017-02.

* [BTLS]: Cleanup certificate store initialization. * Kill unused MonoBtlsX509Store.AddTrustedRoots(). * In server-mode, MonoBtlsProvider.SetupCertificateStore() now only adds certificates explicitly trused via MonoTlsSettings.TrustAnchors. * MonoTlsProvider.ValidateCertificate() - which is called from X509Certificate2.Verify() via X509CertificateImplBtls.Verify() - now uses MonoTlsSettings.DefaultSettings and assumes client-mode. * [appletls]: Actually use the new code that was added in PR #4671. * [btls]: Add new MonoTlsSettings.CertificateValidationTime property. This allows you to set a custom time for certificate expiration checks. * [appletls]: Bind SecTrustSetVerifyDate() and check MonoTlsSettings.CertificateValidationTime.

…4671) (#4677) * [appletls]: Use SecIdentityCreate() to avoid using the Mac keychain. (#4671) Reading Certificates from the Mac Keychain ========================================== Reading the private key from the keychain is a new feature introduced with AppleTls on XamMac and iOS. On Desktop Mono, this new feature has several known issues and it also did not received any testing yet. We go back to the old way of doing things, which is to explicitly provide an X509Certificate2 with a private key. Keychain Dialog Popups ====================== When using Xamarin.Mac or Xamarin.iOS, we try to search the keychain for the certificate and private key. On Xamarin.iOS, this is easy because each app has its own keychain. On Xamarin.Mac, the .app package needs to be trusted via code-sign to get permission to access the user's keychain. [FIXME: I still have to research how to actually do that.] Without this, you will get a popup message each time, asking you whether you want to allow the app to access the keychain, but you can make these go away by selecting "Trust always". On Desktop Mono, this is problematic because selecting "Trust always" give the 'mono' binary (and thus everything you'll ever run with Mono) permission to retrieve the private key from the keychain. This code would also trigger constant keychain popup messages, which could only be suppressed by granting full trust. It also makes it impossible to run Mono in headless mode. SecIdentityCreate ================= To avoid these problems, we are currently using an undocumented API called SecIdentityRef() to avoid using the Mac keychain whenever a X509Certificate2 with a private key is used. On iOS and XamMac, you can still provide the X509Certificate without a private key - in this case, a keychain search will be performed (and you may get a popup message on XamMac).

…4671) (#4677) * [appletls]: Use SecIdentityCreate() to avoid using the Mac keychain. (#4671) Reading Certificates from the Mac Keychain ========================================== Reading the private key from the keychain is a new feature introduced with AppleTls on XamMac and iOS. On Desktop Mono, this new feature has several known issues and it also did not received any testing yet. We go back to the old way of doing things, which is to explicitly provide an X509Certificate2 with a private key. Keychain Dialog Popups ====================== When using Xamarin.Mac or Xamarin.iOS, we try to search the keychain for the certificate and private key. On Xamarin.iOS, this is easy because each app has its own keychain. On Xamarin.Mac, the .app package needs to be trusted via code-sign to get permission to access the user's keychain. [FIXME: I still have to research how to actually do that.] Without this, you will get a popup message each time, asking you whether you want to allow the app to access the keychain, but you can make these go away by selecting "Trust always". On Desktop Mono, this is problematic because selecting "Trust always" give the 'mono' binary (and thus everything you'll ever run with Mono) permission to retrieve the private key from the keychain. This code would also trigger constant keychain popup messages, which could only be suppressed by granting full trust. It also makes it impossible to run Mono in headless mode. SecIdentityCreate ================= To avoid these problems, we are currently using an undocumented API called SecIdentityRef() to avoid using the Mac keychain whenever a X509Certificate2 with a private key is used. On iOS and XamMac, you can still provide the X509Certificate without a private key - in this case, a keychain search will be performed (and you may get a popup message on XamMac). (cherry picked from commit b9b2d23)

…4671) (#4677) (#4689) * [appletls]: Use SecIdentityCreate() to avoid using the Mac keychain. (#4671) Reading Certificates from the Mac Keychain ========================================== Reading the private key from the keychain is a new feature introduced with AppleTls on XamMac and iOS. On Desktop Mono, this new feature has several known issues and it also did not received any testing yet. We go back to the old way of doing things, which is to explicitly provide an X509Certificate2 with a private key. Keychain Dialog Popups ====================== When using Xamarin.Mac or Xamarin.iOS, we try to search the keychain for the certificate and private key. On Xamarin.iOS, this is easy because each app has its own keychain. On Xamarin.Mac, the .app package needs to be trusted via code-sign to get permission to access the user's keychain. [FIXME: I still have to research how to actually do that.] Without this, you will get a popup message each time, asking you whether you want to allow the app to access the keychain, but you can make these go away by selecting "Trust always". On Desktop Mono, this is problematic because selecting "Trust always" give the 'mono' binary (and thus everything you'll ever run with Mono) permission to retrieve the private key from the keychain. This code would also trigger constant keychain popup messages, which could only be suppressed by granting full trust. It also makes it impossible to run Mono in headless mode. SecIdentityCreate ================= To avoid these problems, we are currently using an undocumented API called SecIdentityRef() to avoid using the Mac keychain whenever a X509Certificate2 with a private key is used. On iOS and XamMac, you can still provide the X509Certificate without a private key - in this case, a keychain search will be performed (and you may get a popup message on XamMac). (cherry picked from commit b9b2d23)

[appletls]: Use SecIdentityCreate() to avoid using the Mac keychain.

d051ed2

dnfclas added the cla-already-signed label Apr 11, 2017

baulig requested a review from chamons April 11, 2017 15:02

chamons requested a review from spouliot April 11, 2017 15:09

chamons reviewed Apr 11, 2017

View reviewed changes

Kill debugging code.

83bd217

marek-safar previously requested changes Apr 11, 2017

View reviewed changes

spouliot reviewed Apr 11, 2017

View reviewed changes

Martin Baulig added 6 commits April 11, 2017 12:03

Actually kill all the leftovers from the old debugging code.

caa45dc

Cleanup.

d95a68d

Make private.

eb98fe1

Add documentation.

7859b62

Use the commit message of the next empty commit when merging.

fe26c68

baulig requested review from chamons and marek-safar April 11, 2017 21:08

baulig self-assigned this Apr 11, 2017

marek-safar reviewed Apr 11, 2017

View reviewed changes

marek-safar approved these changes Apr 11, 2017

View reviewed changes

Cosmetic.

8344a18

baulig merged commit 1eb27c1 into master Apr 11, 2017

baulig deleted the work-appletls-keychain-landing branch April 11, 2017 21:46

baulig pushed a commit to dotnet/macios that referenced this pull request Apr 12, 2017

[DONTMERGE]: Switch mono to work-4671-201702-merge.

7ac7f87

Temporary PR, preparing the cherry-picking of Mono's PR #4671 (mono/mono#4671) into mono/2017-02.

This was referenced Apr 12, 2017

[DONTMERGE]: Switch mono to work-4671-201702-merge. dotnet/macios#2001

Closed

[appletls]: Use SecIdentityCreate() to avoid using the Mac keychain. (#4671) #4677

Merged

baulig pushed a commit that referenced this pull request Apr 12, 2017

[appletls]: Actually use the new code that was added in PR #4671.

77f8567

baulig pushed a commit that referenced this pull request Apr 12, 2017

[appletls]: Actually use the new code that was added in PR #4671.

169ab66

baulig pushed a commit that referenced this pull request Apr 12, 2017

[appletls]: Actually use the new code that was added in PR #4671.

998f76f

baulig mentioned this pull request Apr 13, 2017

[appletls]: Use SecIdentityCreate() to avoid using the Mac keychain. (#4671)(#4677) #4689

Merged

Conversation

baulig commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reading Certificates from the Mac Keychain

Keychain Dialog Popups

SecIdentityCreate

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!

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!

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!

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!

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!

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!

spouliot left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baulig commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

baulig commented Apr 11, 2017 •

edited

Loading