Add support for custom macOS keychains exposed using X509Store API #30603

filipnavara · 2018-06-22T14:08:36Z

Rationale: The X509Store API is heavily used by Mono unit tests and it is helpful to have it available on all supported platforms. There are already comments in the CoreFX unit tests referencing a similar need.

bartonjs

The idea seems reasonable, since I noticed that it prevents breaking out of the one directory that the UI would use by default.

bartonjs · 2018-06-22T14:56:51Z

Documentation/architecture/cross-platform-cryptography.md

-Keychain creation on macOS requires more input than is captured with the X509Store API, so attempting to create a new store will fail with a `PlatformNotSupportedException`.
-If a keychain is opened by P/Invoke to SecKeychainOpen, the resulting `IntPtr` can be passed to `new X509Store(IntPtr)` to obtain a read/write-capable store (subject to the current user's permissions).
+Custom store creation on macOS with the X509Store API will create a new keychain with no password. To create a keychain with password a P/Invoke to SecKeychainCreate could be used.
+The resulting `IntPtr` can be passed to `new X509Store(IntPtr)` to obtain a read/write-capable store (subject to the current user's permissions).


The IntPtr based sentence should still mention SecKeychainOpen, since this code will only find it in the user Keychains library.

And presumably you'd want to guard the "this works" to say for StoreLocation.CurrentUser

bartonjs · 2018-06-22T14:58:13Z

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs

+                // If it doesn't work (errSecAuthFailed) then fail silently and fallback to the
+                // default behavior of user interaction.
+                osStatus = AppleCryptoNative_SecKeychainUnlock(keychain, 0, Array.Empty<byte>());
+                if (osStatus == 0 || osStatus == -25293)


Use a const instead of just a comment.

const int errSecAuthFailed = -25293; if (osStatus == 0 || osStatus == errSecAuthFailed)

bartonjs · 2018-06-22T14:59:50Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

@@ -109,7 +109,18 @@ public static IStorePal FromSystemStore(string storeName, StoreLocation storeLoc
                    if (ordinalIgnoreCase.Equals("Disallowed", storeName))
                        return AppleTrustStore.OpenStore(StoreName.Disallowed, storeLocation, openFlags);

-                    break;
+                    if (!IsValidStoreName(storeName))


Since the ifs from here on out aren't part of the same "case insensitive switch", please switch to explicit bracing.

bartonjs · 2018-06-22T15:00:27Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

@@ -109,7 +109,18 @@ public static IStorePal FromSystemStore(string storeName, StoreLocation storeLoc
                    if (ordinalIgnoreCase.Equals("Disallowed", storeName))
                        return AppleTrustStore.OpenStore(StoreName.Disallowed, storeLocation, openFlags);

-                    break;
+                    if (!IsValidStoreName(storeName))


Is the "store creation is not supported on this platform" message really the best message for invalid store names?

The message is actually "The platform does not have a definition for an X509 certificate store named '{0}' with a StoreLocation of '{1}', and does not support creating it.", which may not be perfect, but it is not wrong either.

Well, and then it also carries a PlatformNotSupportedException within it.

The Linux version of not being a valid store name is at https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/DirectoryBasedStoreProvider.cs#L313-L325

Which suggests that there's an IOException hiding here.

Ah, I was sure I saw it in the Linux version, but I couldn't find now. I will follow that one then. Thanks!

bartonjs · 2018-06-22T15:01:01Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

+                    if (!IsValidStoreName(storeName))
+                        break;
+
+                    var storePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Library", storeName + ".keychain");


Please chop this line up, and var => string per the style rules.

bartonjs · 2018-06-22T15:02:35Z

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs

+        internal static SafeKeychainHandle CreateKeychain(string keychainPath)
+        {
+            SafeKeychainHandle keychain;
+            int osStatus = AppleCryptoNative_SecKeychainCreate(


I know I made sure my temporaries don't show up in Keychain.App. Will these? Presumably they should, since they'd show up in MMC on Windows; but you would want to check and see if you need to add them to the enumerate list, or not unadd them, or whatever.

Currently they don't, but they are under the default location in the "Add Keychain" option. Not sure if there's a reliable way to accomplish this, but I will investigate.

I looked at the SecKeychainSetSearchList API and it doesn't look very safe for concurrent access. Moreover it changes the search behavior when looking for passwords in the system. I don't think that is desirable, but I am open to second opinion.

Hm, I thought something did it by default. But I'm not seeing that, and I'll accept the reasoning.

bartonjs · 2018-06-22T15:03:43Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

@@ -131,6 +142,11 @@ public static IStorePal FromSystemStore(string storeName, StoreLocation storeLoc
            throw new CryptographicException(message, new PlatformNotSupportedException(message));
        }

+        private static bool IsValidStoreName(string storeName)
+        {
+            return !String.IsNullOrWhiteSpace(storeName) && Path.GetFileName(storeName) == storeName;


String => string, per the style rules

bartonjs · 2018-06-22T15:04:44Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

+                    if ((openFlags & OpenFlags.OpenExistingOnly) == OpenFlags.OpenExistingOnly)
+                        throw new CryptographicException(SR.Cryptography_X509_StoreNotFound);
+
+                    return AppleKeychainStore.CreateKeychain(storePath, openFlags);


This is going to need tests. Since it will modify the keychain display list they should be [OuterLoop]; and the tests will have to P/Invoke to SecKeychainDelete since we don't have public API for that.

X509FilesystemTests.Unix.cs is the spiritual equivalent for Linux.

bartonjs · 2018-06-22T15:10:29Z

There are already comments in the CoreFX unit tests referencing a similar need.

Where? Creating new stores during tests (aside from "store creation tests") is a bad idea. It taints the system.

filipnavara · 2018-06-22T15:15:49Z

Thanks for the throughout review. I will address the comments and add unit tests.

There are already comments in the CoreFX unit tests referencing a similar need.

Where? Creating new stores during tests (aside from "store creation tests") is a bad idea. It taints the system.

I was referring to this comment in corefx/src/System.Security.Cryptography.X509Certificates/tests/X509StoreTests.cs:

        /* Placeholder information for these tests until they can be written to run reliably.
         * Currently such tests would create physical files (Unix) and\or certificates (Windows)
         * which can collide with other running tests that use the same cert, or from a
         * test suite running more than once at the same time on the same machine.
         * Ideally, we use a GUID-named store to aoiv collitions with proper cleanup on Unix and Windows
         * and\or have lower testing hooks or use Microsoft Fakes Framework to redirect
         * and encapsulate the actual storage logic so it can be tested, along with mock exceptions
         * to verify exception handling.
         * See issue https://github.com/dotnet/corefx/issues/12833
         * and https://github.com/dotnet/corefx/issues/12223

        [Fact]
        public static void TestAddAndRemove() {}

        [Fact]
        public static void TestAddRangeAndRemoveRange() {}
        */

bartonjs · 2018-06-26T14:05:13Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

+            if (!IsValidStoreName(storeName))
+                throw new CryptographicException(SR.Format(SR.Security_InvalidValue, nameof(storeName)));
+
+            storePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Library", "Keychains", storeName + ".keychain");


Hm. A concern I have with this approach is that on Windows and Linux the store name is case insensitive. And on macOS it will now become case sensitive. Perhaps this should normalize the store name to ToLowerInvariant()?

I can do that. However it won't be possible to access third-party stores with non-lower-case names. My particular use case doesn't require that and it will still be possible using native APIs, so I am fine with that... (will write a unit test for it)

… stores them in memory temporarily until they are closed. Also make the file names always lower-case to preserve case-insensitive behavior from other platforms.

filipnavara · 2018-06-28T21:12:08Z

Any more comments? Anything I forgot to test?

bartonjs · 2018-06-28T21:30:51Z

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs

@@ -161,6 +174,52 @@ internal static SafeCFArrayHandle KeychainEnumerateIdentities(SafeKeychainHandle
            throw new CryptographicException();
        }

+        internal static SafeKeychainHandle CreateOrOpenKeychain(string keychainPath, bool crateAllowed)


crate => create

Eh, good catch...a search for "crate" in CoreFX sources actually shows a couple more occurrences. Most of them are in tests, but one of them is in XmlMelformMapping string. How ironic :-)

bartonjs · 2018-06-28T21:33:10Z

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/StorePal.cs

+            if (!IsValidStoreName(storeName))
+                throw new CryptographicException(SR.Format(SR.Security_InvalidValue, nameof(storeName)));
+
+            storePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "Library", "Keychains", storeName.ToLowerInvariant() + ".keychain");


Please chop the long line (my VS code segment width and Github's default width are pretty similar)

string storePath = Path.Combine( Environment.GetFolderPath(etc), "Library", "Keychains", storeName.ToLowerInvariant() + ".keychain");

bartonjs · 2018-06-28T22:35:47Z

src/System.Security.Cryptography.X509Certificates/tests/X509StoreMutableTests.OSX.cs

@@ -269,5 +346,21 @@ private static int GetStoreCertificateCount(X509Store store)
                return coll.Collection.Count;
            }
        }
+
+        class TemporaryX509Store : IDisposable


private class

bartonjs · 2018-06-28T22:40:26Z

Looks like I'm down to spelling and formatting, so probably good on the next iteration :)

filipnavara · 2018-07-10T07:31:09Z

Any chance to get this merged?

bartonjs · 2018-07-10T13:21:16Z

@dotnet-bot Test this please

filipnavara added 3 commits June 22, 2018 16:07

Add support for custom keychains exposed using X509Store API.

4189578

Update documentation.

e3a8d9f

Support the custom keychains only for StoreLocation.CurrentUser.

3b7a415

bartonjs reviewed Jun 22, 2018

View reviewed changes

filipnavara closed this Jun 22, 2018

filipnavara reopened this Jun 22, 2018

filipnavara added 2 commits June 22, 2018 17:44

Address couple of review comments.

3b2e7bd

Improve exception handling.

628b4b7

bartonjs reviewed Jun 26, 2018

View reviewed changes

filipnavara added 2 commits June 27, 2018 13:07

Do not depend on the Keychains being backed by files. macOS sometimes…

e0295b3

… stores them in memory temporarily until they are closed. Also make the file names always lower-case to preserve case-insensitive behavior from other platforms.

Add unit tests.

59e6685

filipnavara changed the title ~~WIP: Add support for custom macOS keychains exposed using X509Store API~~ Add support for custom macOS keychains exposed using X509Store API Jun 27, 2018

bartonjs reviewed Jun 28, 2018

View reviewed changes

Fix typos and coding style.

2886a82

bartonjs approved these changes Jun 29, 2018

View reviewed changes

bartonjs merged commit 188e174 into dotnet:master Jul 10, 2018

karelz added this to the 3.0 milestone Jul 19, 2018

vcsjones mentioned this pull request Feb 12, 2021

certificates added to user CertificateAuthority store are ignored by X509Chain.Build on macOS dotnet/runtime#48207

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom macOS keychains exposed using X509Store API #30603

Add support for custom macOS keychains exposed using X509Store API #30603

filipnavara commented Jun 22, 2018

bartonjs left a comment

bartonjs Jun 22, 2018

bartonjs Jun 22, 2018

bartonjs Jun 22, 2018

bartonjs Jun 22, 2018

filipnavara Jun 22, 2018

bartonjs Jun 22, 2018

filipnavara Jun 22, 2018

bartonjs Jun 22, 2018

bartonjs Jun 22, 2018

filipnavara Jun 22, 2018

filipnavara Jun 26, 2018

bartonjs Jun 26, 2018

bartonjs Jun 22, 2018

bartonjs Jun 22, 2018

bartonjs commented Jun 22, 2018

filipnavara commented Jun 22, 2018

bartonjs Jun 26, 2018

filipnavara Jun 26, 2018

filipnavara commented Jun 28, 2018

bartonjs Jun 28, 2018

filipnavara Jun 28, 2018

bartonjs Jun 28, 2018

bartonjs Jun 28, 2018

bartonjs commented Jun 28, 2018

filipnavara commented Jul 10, 2018

bartonjs commented Jul 10, 2018

Add support for custom macOS keychains exposed using X509Store API #30603

Add support for custom macOS keychains exposed using X509Store API #30603

Conversation

filipnavara commented Jun 22, 2018

bartonjs 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

bartonjs commented Jun 22, 2018

filipnavara commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipnavara commented Jun 28, 2018

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 Jun 28, 2018

filipnavara commented Jul 10, 2018

bartonjs commented Jul 10, 2018