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

Managed classes for AndroidCrypto SSL and X509Certificates #48674

Merged
merged 8 commits into from
Feb 26, 2021

Conversation

elinor-fung
Copy link
Member

  • Add managed classes for SSL and X509Certificates when using AndroidCrypto
    • Mostly just throw NotImplementedException right now, but it is at least a clean split from the existing OpenSSL usage
  • Add SafeJObjectHandle for handling deletion of global references
  • Switch to managed parser for certificate data on Android
    • Needed to get extensions in the expected order, so it made sense to use it as the reader

cc @jkoritzinsky @steveisok @AaronRobinsonMSFT @bartonjs

@ghost
Copy link

ghost commented Feb 24, 2021

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

Issue Details
  • Add managed classes for SSL and X509Certificates when using AndroidCrypto
    • Mostly just throw NotImplementedException right now, but it is at least a clean split from the existing OpenSSL usage
  • Add SafeJObjectHandle for handling deletion of global references
  • Switch to managed parser for certificate data on Android
    • Needed to get extensions in the expected order, so it made sense to use it as the reader

cc @jkoritzinsky @steveisok @AaronRobinsonMSFT @bartonjs

Author: elinor-fung
Assignees: -
Labels:

area-System.Security

Milestone: -

@elinor-fung elinor-fung added this to the 6.0.0 milestone Feb 24, 2021

public static ICertificatePal FromBlob(ReadOnlySpan<byte> rawData, SafePasswordHandle password, X509KeyStorageFlags keyStorageFlags)
{
// TODO: [AndroidCrypto] Handle PKCS#12
Copy link
Member

Choose a reason for hiding this comment

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

The good news is that macOS and Unix-OpenSSL both use a managed PKCS#12 reader/writer to take away a lot of corner case inconsistencies.

But it probably requires that there already be support from the Algorithms layer 😄

@bartonjs bartonjs requested a review from wfurt February 24, 2021 16:20
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

The stubs look reasonable to me. Added a networking representative to weigh in on the System.Net.Security parts.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

generally looks OK to me. Left few comments to consider.

{
if (certificateContext != null)
{
// Make a defensive copy of the certificate. In some async cases the
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is really needed but I know macOS has it there. So it is probably OK to keep.


namespace System.Net
{
internal sealed class SafeDeleteSslContext : SafeDeleteContext
Copy link
Member

Choose a reason for hiding this comment

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

For Windows & Linux SafeDeleteSslContext lives in Common. OSX was here under System.Net.Security. It would make sense to ma to use consistent location. Do you have any preference @stephentoub? We can possibly move it later but since this is new file it may be peer to make sure it its where we want it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the Windows one is actually shared and the Linux one (like OSX and Android) is only used by System.Net.Security.

I'm going to leave this here for now. Happy to move it (and the OSX one) to common in a follow-up if that is what you / @stephentoub would like.

@elinor-fung elinor-fung merged commit bb9a9d1 into dotnet:master Feb 26, 2021
@elinor-fung elinor-fung deleted the androidCryptoStubs branch February 26, 2021 01:44
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 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.

4 participants