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.

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

[iOS] Implement DSA, RSA, EC key import/export #51926

Merged
merged 5 commits into from
Apr 29, 2021

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Apr 27, 2021

No description provided.

@ghost
Copy link

ghost commented Apr 27, 2021

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

Issue Details

Depends on #51914 and #51923

Author: filipnavara
Assignees: -
Labels:

area-System.Security

Milestone: -

@filipnavara filipnavara marked this pull request as ready for review April 27, 2021 21:51
@filipnavara
Copy link
Member Author

filipnavara commented Apr 27, 2021

@bartonjs Now that the other PRs have been merged I suppose this is good enough for first round of review. It's the major remaining change that enables S.S.C.Algorithms to work on iOS.

(I am on different time zone so I try to keep few PRs open during my rest time to speed up the feedback process.)

@@ -139,3 +139,82 @@ uint64_t AppleCryptoNative_SecKeyGetSimpleKeySizeInBytes(SecKeyRef publicKey)

return SecKeyGetBlockSize(publicKey);
}

#if defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS)
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be conditional?

  • No: Great, let's remove the ifs
  • Yes, this uses API not on macOS: Sadface.
  • Yes, this is alternative implementations of functions already present for macOS: Can we get rid of all of these triple target references and use one or two high-level defines based on these? (e.g. FULL_MACOS and MOBILE_MACOS, or some names that get more than 3 seconds of thought)

Copy link
Member Author

@filipnavara filipnavara Apr 27, 2021

Choose a reason for hiding this comment

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

It doesn't have to be conditional. It's just not used on macOS yet in this PR which is why I excluded it. I'd be happy to remove the conditionals and add the references to entrypoints.c. I prototyped a managed implementation that uses both types of the keys (iOS / CSSM) on macOS but I don't intend to submit it as PR (at least not until the the iOS part is working).

Generally I would prefer to split the iOS / macOS code into different files in the native dylib but I didn't do it because of the preexisting code. I do think it would make sense though, especially with support for iOS keychain which is substantially different from the macOS one. Most of the #ifs would be removed and appear only once in the CMake script that way.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess for me it's "is this only for iOS" vs "is this only for iOS today". If future PRs (particularly in this release) are going to start calling these functions from macOS, the API shouldn't be conditional.

And if the API is conditional, I agree that I'd prefer the isolation be done at the file level.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it is iOS only today so I'll just include these new APIs unconditionally.

And if the API is conditional, I agree that I'd prefer the isolation be done at the file level.

If I get to do the iOS keychain work I'll introduce that pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened #51976 to get CI run for the implementation of data keys on macOS. I suspect it will need some tweaking but given how much code I'd move back and forth it would likely make sense to merge it into this PR if I can get the CI to pass.

@marek-safar
Copy link
Contributor

Thank you for the contribution.

@filipnavara filipnavara deleted the ios-crypto-wip branch April 29, 2021 07:43
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants