Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Jun 3, 2021
Merged

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

merged 1 commit into from
Jun 3, 2021

Conversation

filipnavara
Copy link
Member

Fixes #36107.
Ref: #36107 (comment)

@ghost
Copy link

ghost commented May 14, 2021

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

Issue Details

Fixes #36107.
Ref: #36107 (comment)

Author: filipnavara
Assignees: -
Labels:

area-System.Security
Milestone: -

@vcsjones
Copy link
Member

Did you actually see improvements in benchmarks?

I'm a bit behind in responding to issues, but I did give this a shot and I did not see an improvement in performance.

@filipnavara
Copy link
Member Author

I'll re-run the benchmarks to make double sure that it takes the right code path.

@filipnavara filipnavara marked this pull request as draft May 14, 2021 13:25
@filipnavara
Copy link
Member Author

filipnavara commented May 14, 2021 •
edited
Loading

BenchmarkDotNet=v0.12.1.20210514-develop, OS=macOS Big Sur 11.3 (20E232) [Darwin 20.4.0]
Apple M1 2.40GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=6.0.100-preview.3.21202.5
[Host] : .NET 6.0.0 (6.0.21.20104), X64 RyuJIT
Job-AAEMFC : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
Job-DJNIOL : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT

Method Job Toolchain Config Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
SignHash Job-AAEMFC branch nistP256, SHA256 376.8 us 1.03 us 0.86 us 0.07 - - - 248 B
SignHash Job-DJNIOL main nistP256, SHA256 5,700.9 us 53.82 us 47.71 us 1.00 - - - 253 B
VerifyHash Job-AAEMFC branch nistP256, SHA256 493.8 us 3.21 us 2.85 us 0.04 - - - 1,337 B
VerifyHash Job-DJNIOL main nistP256, SHA256 11,277.0 us 164.62 us 137.47 us 1.00 - - - 1,341 B
SignHash Job-AAEMFC branch nistP384, SHA384 1,497.6 us 20.16 us 17.88 us 0.13 - - - 313 B
SignHash Job-DJNIOL main nistP384, SHA384 11,503.5 us 105.37 us 93.41 us 1.00 - - - 322 B
VerifyHash Job-AAEMFC branch nistP384, SHA384 1,524.5 us 10.95 us 9.71 us 0.07 - - - 1,369 B
VerifyHash Job-DJNIOL main nistP384, SHA384 23,298.7 us 252.31 us 223.67 us 1.00 - - - 1,388 B
SignHash Job-AAEMFC branch nistP521, SHA512 1,540.2 us 20.36 us 19.05 us 0.07 - - - 393 B
SignHash Job-DJNIOL main nistP521, SHA512 23,319.9 us 123.77 us 109.72 us 1.00 - - - 404 B
VerifyHash Job-AAEMFC branch nistP521, SHA512 1,606.7 us 13.24 us 12.39 us 0.03 - - - 1,409 B
VerifyHash Job-DJNIOL main nistP521, SHA512 46,410.1 us 150.48 us 117.48 us 1.00 - - - 1,465 B

(ignore the "allocated" column; the branch is few commits apart from the main one; both are x64 Debug build running under Rosetta)

@filipnavara filipnavara marked this pull request as ready for review May 14, 2021 14:03
@vcsjones
Copy link
Member

Woohoo.

@filipnavara
Copy link
Member Author

filipnavara commented May 14, 2021 •
edited
Loading

The failures with iOS.Simulator.Aot.Test on iOS / MacCatalyst could be related but there's not enough info in the logs. If I remember correctly these tests were enabled quite recently (this week) so it's possible that it may be just a coincidence.

Looks like the tests fail on all the recent PRs, so the failures are unrelated.

bartonjs
bartonjs reviewed May 14, 2021
View reviewed changes
src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ecc.c
@@ -24,6 +24,10 @@ int32_t AppleCryptoNative_EccGenerateKey(int32_t keySizeBits,
{
CFDictionaryAddValue(attributes, kSecAttrKeyType, kSecAttrKeyTypeEC);
CFDictionaryAddValue(attributes, kSecAttrKeySizeInBits, cfKeySizeValue);
if (__builtin_available(macOS 10.15, iOS 13, tvOS 13, *))
{
CFDictionaryAddValue(attributes, kSecUseDataProtectionKeychain, kCFBooleanTrue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Apple docs for this are... lacking. What's the effect? Does this let us get rid of the temporary keychains, and therefore make ephemeral load work?

If someone does PersistKeySet (where we load it into the default keychain for them) does this do anything weird?

Copy link
Member Author

@filipnavara filipnavara May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary keychains were already removed here in previous PR (#51620) that unified the key generation between iOS and macOS. However, by default macOS still generates ephemeral legacy CSSM keys that could be imported into (legacy) keychains. This attribute causes the code to create the iOS-style data keys instead. They don't interoperate with the legacy keychain SecItem* APIs well but that's never directly used in .NET (X509Certificate2.CopyWithPrivateKey will go through export and re-import using old APIs).

Copy link
Member Author

@filipnavara filipnavara May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the attribute existed in earlier macOS versions under the name kSecAttrNoLegacy which was more fitting (but private API).

@filipnavara
Copy link
Member Author

RSA key generation benchmark

BenchmarkDotNet=v0.12.1.20210515-develop, OS=macOS Big Sur 11.3 (20E232) [Darwin 20.4.0]
Apple M1 2.40GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=6.0.100-preview.3.21202.5
[Host] : .NET 6.0.0 (6.0.21.20104), X64 RyuJIT
Job-VMVYPG : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
Job-FZFMXK : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT

Method Job Toolchain KeySize Mean Error StdDev Median Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
GenerateKey Job-VMVYPG branch 1024 22.68 ms 0.618 ms 1.813 ms 22.62 ms 0.47 0.09 - - - 4 KB
GenerateKey Job-FZFMXK main 1024 49.74 ms 3.082 ms 9.038 ms 49.04 ms 1.00 0.00 - - - 4 KB
GenerateKey Job-VMVYPG branch 2048 196.33 ms 21.201 ms 61.846 ms 185.20 ms 0.66 0.49 - - - 6 KB
GenerateKey Job-FZFMXK main 2048 427.20 ms 83.391 ms 241.932 ms 392.75 ms 1.00 0.00 - - - 7 KB
GenerateKey Job-VMVYPG branch 4096 2,437.88 ms 529.845 ms 1,537.176 ms 1,974.58 ms 0.90 1.43 - - - 12 KB
GenerateKey Job-FZFMXK main 4096 4,527.22 ms 894.447 ms 2,537.396 ms 4,167.44 ms 1.00 0.00 - - - 12 KB

@bartonjs
Copy link
Member

While many PRs are failing on building MacCatalyst and iOSSimulator, I can't bring myself to accept a merge here with that being true, since I can't convince myself that the __builtin_available / kSecUseDataProtectionKeychain aren't related.

@filipnavara
Copy link
Member Author

The problem with MacCatalyst / iOSSimulator should be fixed in main. Re-running the tests should succeeded.

@filipnavara
Copy link
Member Author

/azp run runtime-staging

@filipnavara
Copy link
Member Author

Apparently the build was still picking the broken Mono :-/ I expected it to build the PR merge commit and not the PR tip commit.

I rebased it, so let's try again...

@steveisok
Copy link
Member

@bartonjs Can you give this another review?

@bartonjs bartonjs merged commit 4275561 into dotnet:main Jun 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bartonjs bartonjs bartonjs approved these changes

Assignees
No one assigned
Labels
area-System.Security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ECDsa.VerifyData Performance Slow on Mac
4 participants
@filipnavara @vcsjones @bartonjs @steveisok