[macOS] Implement RSA, ECC through new macOS 10.12 APIs #51620

filipnavara · 2021-04-21T12:48:57Z

Key generation is updated to use SecKeyCreateRandomKey API to avoid need for temporary keychains
RSA encryption is converted to use SecKeyCreateEncryptedData
RSA decryption is converted to use SecKeyCreateDecryptedData

The new APIs are also available on iOS so the conditional blocks around the reimplemented APIs are removed. The implementation is not tested on iOS and may require some tweaks but it compiles at least.

…ta and bypass keychains

ghost · 2021-04-21T12:49:04Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

…ta and bypass keychains

NOT READY FOR REVIEW

Author:	filipnavara
Assignees:	-
Labels:	`area-System.Security`
Milestone:	-

filipnavara · 2021-04-21T12:51:10Z

Related to #38101 which I will likely try to revive at some point in some form.

…s truly ephemeral keys

vcsjones · 2021-04-21T14:27:21Z

Maybe thing's have gotten better since I last tried in macOS world (or maybe I did a bunch of things wrong 😄 ) but the problem with "data" keys as I call them is they don't seem to work fully in place of keychain-backed keys. e.g. CopyWithPrivateKey on X509Certificate2 I think was one place that I hit.

Oddly, if my recollection serves, ECDSA was more problematic than RSA.

filipnavara · 2021-04-21T14:30:56Z

Maybe thing's have gotten better since I last tried in macOS world (or maybe I did a bunch of things wrong 😄 ) but the problem with "data" keys as I call them is they don't seem to work fully in place of keychain-backed keys. e.g. CopyWithPrivateKey on X509Certificate2 I think was one place that I hit.

I fully expect that to be a problem, just scratching the unit test surface now to decide on a strategy. On iOS the keychain-backed APIs just don't exist so even if this works on a limited API surface it's still better than nothing.

bartonjs · 2021-04-21T15:36:29Z

I expect, given @vcsjones' previous investigation, that instead of editing the current APIs what'll be needed is parallel API for RSA macOS and RSA iOS.

filipnavara · 2021-04-21T15:39:18Z

There's still a lot of code that can be shared. For example, this PR passed the macOS tests with new APIs introduced in macOS 10.12 that exist on iOS too. I'm not necessarily saying the same approach will work everywhere but at least part of the code seems sharable.

Locally the same change for key generation works for ECC too. I expect the key imports to be somewhat more problematic but let's see. Worst case I will have to split the implementations.

bartonjs · 2021-04-21T15:44:18Z

For example, this PR passed the macOS tests with new APIs introduced in macOS 10.12 that exist on iOS too.

Huh. I saw they were still in progress, hadn't noticed that the OSX legs had already finished. Since I'm surprised by this (vaguely recalling the issues that @vcsjones ran into), I'll trigger an OuterLoop run to see if it's only some more complex tests that fail.

bartonjs · 2021-04-21T15:52:05Z

/azp run runtime-libraries-coreclr outerloop osx

azure-pipelines · 2021-04-21T15:52:11Z

No pipelines are associated with this pull request.

bartonjs · 2021-04-21T15:52:20Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-04-21T15:52:35Z

Azure Pipelines successfully started running 1 pipeline(s).

filipnavara · 2021-04-21T15:57:49Z

It's not that surprising that this part worked. It's still creating the old-style keys (as opposed to the iOS / data keys). The difference is that SecCreateRandomKey doesn't enforce permanent keys while SecKeyGenerateKeyPair does:

https://github.com/Apple-FOSS-Mirror/Security/blob/5bcad85836c8bbb383f660aaf25b555a805a48e4/OSX/libsecurity_keychain/lib/SecKey.cpp#L1667
https://github.com/Apple-FOSS-Mirror/Security/blob/5bcad85836c8bbb383f660aaf25b555a805a48e4/OSX/libsecurity_keychain/lib/SecKey.cpp#L1640-L1643

… obsolete code

filipnavara · 2021-04-22T05:14:22Z

The outerloop tests on CI have timed out. I tried to run them locally with .dotnet/dotnet build src/libraries/System.Security.Cryptography.Algorithms/tests/System.Security.Cryptography.Algorithms.Tests.csproj /t:Test /p:XUnitOptions="-noclass System.Security.Cryptography.Algorithms.Tests.AesGcmTests -noclass System.Security.Cryptography.Algorithms.Tests.AesCcmTests" /p:Outerloop=true but it seems to run only two more tests in comparison to the inner loop. Nevertheless all the tests succeeded locally.

UPD: .dotnet/dotnet build src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj /t:Test /p:Outerloop=true: System.Security.Cryptography.X509Certificates.Tests Total: 1190, Errors: 0, Failed: 0, Skipped: 0, Time: 112.783s

marek-safar · 2021-04-22T07:28:25Z

/cc @MaximLipnin

MaximLipnin · 2021-04-22T08:14:59Z

#51098

filipnavara · 2021-04-22T08:25:38Z

It contributes to #51098 but doesn't quite solve it. I have another PR in the pipeline to enable import of ephemeral RSA keys using the iOS-compatible APIs. Note that I am focusing now on the part that can be shared with macOS to simplify testing. Some of the APIs have slightly different semantics on iOS which may need specific tweaks but this should lay down the groundwork to make that happen.

filipnavara · 2021-04-22T09:07:31Z

Maybe thing's have gotten better since I last tried in macOS world (or maybe I did a bunch of things wrong 😄 ) but the problem with "data" keys as I call them is they don't seem to work fully in place of keychain-backed keys. e.g. CopyWithPrivateKey on X509Certificate2 I think was one place that I hit.

I was playing with it today and it's a mixed bag situation.

First of all, the SecKeyCreateWithData API documentation states that it requires kSecAttrKeySizeInBits which is not true. The implementation doesn't look at it, never looked at it (as far as the published sources go), and even omits it in many of their own regression tests. It's much simpler to use when this is not required. I filed an Apple Feedback to clarify the situation.

Second, it does seem to work for RSA keys even with the item export. On ECC though it hits the problematic code paths in the native code which explain the issues you've hit.

I'll submit a PR later to convert the RSA code paths to use it.

filipnavara · 2021-04-22T13:31:16Z

I got the ECC somewhat working locally without duplicating the keys. The trick is to understand what is going on. All the RSA/ECC keys work with the new SecKeyCreateSignature/SecKeyVerifySignature APIs no matter how they were created. What is problematic is the import and export of the keys.

Private ECC keys created with SecKeyCreateWithData cannot be exported with SecItemExport. They can be exported with SecKeyCopyExternalRepresentation which can also avoid some re-parsing.

Private ECC keys created with SecItemImport cannot be exported with SecKeyCopyExternalRepresentation. It returns errSecPassphraseRequired error.

Public ECC keys created with either SecKeyCreateWithData or SecItemImport can be exported with SecKeyCopyExternalRepresentation.

I've come up with a code that converts most of the imports from ECParameters and other formats to use SecKeyCreateWithData. On export I try to use SecKeyCopyExternalRepresentation and fall back to SecItemExport if the errSecPassphraseRequired error is encountered. This approach seems to pass the unit tests on my machine. It's also easy to extend it to iOS by just disabling some of the code paths that rely on the old APIs.

I'll run the benchmarks from earlier Kevin's PR to see whether there is even significant benefit in using the SecKeyCreateWithData keys or whether it's the SecKeyCreateSignature/SecKeyVerifySignature APIs that make the difference.

bartonjs · 2021-04-22T15:50:23Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-04-22T15:50:38Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ecc.c

filipnavara · 2021-04-22T17:26:49Z

The failed test on the outer loop is likely unrelated:

    System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled [FAIL]
      System.TimeoutException : The operation has timed out.
      Stack Trace:
        /_/src/libraries/Common/tests/System/Threading/Tasks/TaskTimeoutExtensions.cs(49,0): at System.Threading.Tasks.TaskTimeoutExtensions.WhenAllOrAnyFailed(Task[] tasks, Int32 millisecondsTimeout)
        /_/src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs(172,0): at System.Net.Test.Common.Http2LoopbackServer.CreateClientAndServerAsync(Func`2 clientFunc, Func`2 serverFunc, Http2Options http2Options, Int32 timeout)
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs(3451,0): at System.Net.Http.Functional.Tests.HttpClientHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled()
        --- End of stack trace from previous location ---
    System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Http2.GetAsync_ServerDelaysSendingSettingsThenSetsLowerMaxConcurrentStreamsLimitThenIncreaseIt_ClientAppliesEachLimitChangeProperly [SKIP]

bartonjs · 2021-04-27T00:53:38Z

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

+            SafeSecKeyRefHandle keychainPublic;
+            SafeSecKeyRefHandle keychainPrivate;


Since these are not keychains (or even bound to them) the variables could probably use a better name.

Good catch. I will fix it up in a follow up PR.

bartonjs · 2021-04-27T01:02:15Z

Since the variable renames are the only thing I saw, and CI takes a long time, I wouldn't bother with them.

Thanks for spearheading this, @filipnavara!

[macOS/iOS] Implement RSA through SecKeyCreate[Decrypted/Encrypted]Da…

e8df32c

…ta and bypass keychains

dotnet-issue-labeler bot added the area-System.Security label Apr 21, 2021

Replace SecKeyGenerateKeyPair with SecKeyCreateRandomKey which create…

a90f879

…s truly ephemeral keys

Update ECC key generation to use SecKeyCreateRandomKey API and remove…

d6dedb2

… obsolete code

filipnavara changed the title ~~[macOS/iOS] Implement RSA through SecKeyCreate[Decrypted/Encrypted]Da…~~ [macOS] Implement RSA, ECC through new macOS 10.12 APIs Apr 22, 2021

filipnavara marked this pull request as ready for review April 22, 2021 05:14

Remove unused ExportImportKey function

95f87cc

bartonjs reviewed Apr 22, 2021

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ecc.c Show resolved Hide resolved

akoeplinger mentioned this pull request Apr 23, 2021

Mark some S.S.C.Algorithms APIs as unsupported on ios/tvos #49762

Closed

filipnavara requested a review from bartonjs April 26, 2021 13:50

Consolidate the duplicated code in pal_rsa

f6da122

bartonjs reviewed Apr 27, 2021

View reviewed changes

bartonjs approved these changes Apr 27, 2021

View reviewed changes

marek-safar merged commit c8c3963 into dotnet:main Apr 27, 2021

MaximLipnin mentioned this pull request Apr 29, 2021

[iOS/tvOS] Enable RSA operations #51098

Closed

filipnavara mentioned this pull request May 15, 2021

[macOS] Specify kSecUseDataProtectionKeychain when generating RSA/ECC keys #52759

Merged

karelz added this to the 6.0.0 milestone May 20, 2021

MaximLipnin mentioned this pull request May 24, 2021

Annotate APIs throwing PNSE on iOS/tvOS/MacCatalyst #47910

Closed

27 tasks

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[macOS] Implement RSA, ECC through new macOS 10.12 APIs #51620

[macOS] Implement RSA, ECC through new macOS 10.12 APIs #51620

filipnavara commented Apr 21, 2021 •

edited

Loading

ghost commented Apr 21, 2021

filipnavara commented Apr 21, 2021

vcsjones commented Apr 21, 2021

filipnavara commented Apr 21, 2021

bartonjs commented Apr 21, 2021

filipnavara commented Apr 21, 2021 •

edited

Loading

bartonjs commented Apr 21, 2021

bartonjs commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

bartonjs commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

filipnavara commented Apr 21, 2021

filipnavara commented Apr 22, 2021 •

edited

Loading

marek-safar commented Apr 22, 2021

MaximLipnin commented Apr 22, 2021

filipnavara commented Apr 22, 2021

filipnavara commented Apr 22, 2021

filipnavara commented Apr 22, 2021 •

edited

Loading

bartonjs commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

filipnavara commented Apr 22, 2021

bartonjs Apr 27, 2021 •

edited

Loading

filipnavara Apr 27, 2021

bartonjs commented Apr 27, 2021

		SafeSecKeyRefHandle keychainPublic;
		SafeSecKeyRefHandle keychainPrivate;

[macOS] Implement RSA, ECC through new macOS 10.12 APIs #51620

[macOS] Implement RSA, ECC through new macOS 10.12 APIs #51620

Conversation

filipnavara commented Apr 21, 2021 • edited Loading

ghost commented Apr 21, 2021

filipnavara commented Apr 21, 2021

vcsjones commented Apr 21, 2021

filipnavara commented Apr 21, 2021

bartonjs commented Apr 21, 2021

filipnavara commented Apr 21, 2021 • edited Loading

bartonjs commented Apr 21, 2021

bartonjs commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

bartonjs commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

filipnavara commented Apr 21, 2021

filipnavara commented Apr 22, 2021 • edited Loading

marek-safar commented Apr 22, 2021

MaximLipnin commented Apr 22, 2021

filipnavara commented Apr 22, 2021

filipnavara commented Apr 22, 2021

filipnavara commented Apr 22, 2021 • edited Loading

bartonjs commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

filipnavara commented Apr 22, 2021

bartonjs Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

filipnavara Apr 27, 2021

Choose a reason for hiding this comment

bartonjs commented Apr 27, 2021

filipnavara commented Apr 21, 2021 •

edited

Loading

filipnavara commented Apr 21, 2021 •

edited

Loading

filipnavara commented Apr 22, 2021 •

edited

Loading

filipnavara commented Apr 22, 2021 •

edited

Loading

bartonjs Apr 27, 2021 •

edited

Loading