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

[AndroidCrypto] Basic SSL stream implementation #50519

Merged
merged 13 commits into from
Apr 8, 2021

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Mar 31, 2021

Get basic (system-default-only) functionality working for SSL stream on Android.

  • Rework Android native implementation of SSLStream that @EgorBo added to align with managed APIs
  • Hook up AndroidCrypto_SSLStream* functions to managed
  • Add native implementations for getting connection info (protocol, certs)

This enables the basic scenario called out in #45739:

using (var client = new HttpClient())
{
    client.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue("TestApp","1.0"));

    string s = await client.GetStringAsync("https://api.github.com/zen");
    s = await client.GetStringAsync("https://mail.google.com/mail");
}

Any options other than system default (e.g. specifying ApplicationProtocols, CipherSuitesPolicy, SslProtocols) still throw NotImplementedException.

The main difference with the implementation versus other platforms is that Android always verifies the certificates received during a handshake using the default trust manager.

  • Any RemoteCertificateValidationCallback will only get an opportunity to validate certificates that have already been accepted by the system's built-in trust manager.
  • The only way to use something other than a built-in trust manager is to implement our own. Android doesn't allow defining a class via JNI, so doing this would involve actually creating/shipping a Java class. If deemed necessary, this could be investigated/invested in as a future improvement.

Most tests are still failing, but some outerloop tests that hit an external server (where the certs are trusted by the system) and just use system defaults pass (e.g. CertificateValidationRemoteServer, some PostScenarioTest)

cc @jkoritzinsky @steveisok @AaronRobinsonMSFT @bartonjs @wfurt

@ghost
Copy link

ghost commented Mar 31, 2021

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Get basic (system-default-only) functionality working for SSL stream on Android.

  • Rework Android native implementation of SSLStream to align with managed APIs
  • Hook up AndroidCrypto_SSLStream* functions to managed
  • Add native implementations for getting connection info (protocol, certs)

This enables the basic scenario called out in #45739:

using (var client = new HttpClient())
{
    client.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue("TestApp","1.0"));

    string s = await client.GetStringAsync("https://api.github.com/zen");
    s = await client.GetStringAsync("https://mail.google.com/mail");
}

Any options other than system default (e.g. specifying ApplicationProtocols, CipherSuitesPolicy, SslProtocols) still throw NotImplementedException.

The main difference with the implementation versus other platforms is that Android always verifies the certificates received during a handshake using the default trust manager.

  • Any RemoteCertificateValidationCallback will only get an opportunity to validate certificates that have already been accepted by the system's built-in trust manager.
  • The only way to use something other than a built-in trust manager is to implement our own. Android doesn't allow defining a class via JNI, so doing this would involve actually creating/shipping a Java class. If deemed necessary, this could be investigated/invested in as a future improvement.

Most tests are still failing, but some outerloop tests that hit an external server (where the certs are trusted by the system) and just use system defaults pass (e.g. CertificateValidationRemoteServer, some PostScenarioTest)

Author: elinor-fung
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@bartonjs
Copy link
Member

bartonjs commented Apr 5, 2021

I mostly skimmed through the shim code and PAL status transitions, hoping that @wfurt will do the deeper review there.

@elinor-fung elinor-fung requested a review from wfurt April 6, 2021 17:03
Comment on lines +411 to +420
jmethodID g_ByteBufferAllocate;
jmethodID g_ByteBufferCompact;
jmethodID g_ByteBufferFlip;
jmethodID g_ByteBufferGet;
jmethodID g_ByteBufferLimit;
jmethodID g_ByteBufferPosition;
jmethodID g_ByteBufferPutBuffer;
jmethodID g_ByteBufferPutByteArray;
jmethodID g_ByteBufferPutByteArrayWithLength;
jmethodID g_ByteBufferRemaining;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use some of the JNI NIO apis to make the byte buffers instead of calling the byte buffer methods through JNI?

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 stuck with the simplicity of going through these methods, since they handle all the mark / position / limit tracking. If we do the direct access through the JNI NIO support, we'd have to update those ourselves which would make things a bit more complex.

I'd prefer to go with the simpler way right now and note it as a possible future improvement/investigation that could improve perf.

@elinor-fung
Copy link
Member Author

@wfurt could you take a look, please?

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.

LGTM.
Looks like good improvement.

The lack of validation callback is somewhat concerning. Most users would either stick to system default or use it to override decision on self-signed certificates or use it for custom trust. if I read the comments right, neither use case will really work. If this is difficult, we should document it properly.

It seems like SslProtocols could be implemented. I think it is ok if the platform does not support some older version.

I check HttpClient and it sets ApplicationProtocols only if intending to use HTTP/2+
So the base use case should be OK.

CipherSuitesPolicy throws PNSP on Windows so I would see that is nice to have but certainly not critical.

@elinor-fung
Copy link
Member Author

Thanks @wfurt! The context you provided around the different options is very helpful.

The lack of validation callback is somewhat concerning. Most users would either stick to system default or use it to override decision on self-signed certificates or use it for custom trust. if I read the comments right, neither use case will really work. If this is difficult, we should document it properly.

Yes, neither use of the callback for self-signed certificates or custom trust will really work. The only thing that would work (aside from just using the system default) is if a user wants to reject a certificate that is accepted by the system default. The current plan is to make sure it is properly documented.

It seems like SslProtocols could be implemented.

Agreed - planning on doing SslProtocols once this change goes in.

@elinor-fung
Copy link
Member Author

Merging, since all these changes are Android-only and the Android CI legs have gone through (and build infrastructure for Windows is still having stress).

@elinor-fung elinor-fung merged commit 4d2fc1c into dotnet:main Apr 8, 2021
@elinor-fung elinor-fung deleted the ssl-android branch April 8, 2021 17:20
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 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