okhttp: use new APIs for configuring TLS whenever possible (Android Q+) #6912

voidzcy · 2020-04-08T18:59:55Z

Use Android Q+'s new APIs for configuring TLS whenever possible. The new APIs are

Calling the hidden methods SSLSocket#setAlpnProtocols(...) and SSLSocket#getAlpnProtocol() in Android R throws an UnsupportedAppUsage exception in internal version of Android R (annotated on Android's Conscrypt). This PR migrates to invoke new public APIs for configuring Conscrypt whenever possible. Only fall back to invoke the old reflective APIs in when new APIs are not available (and for work around Conscrypt's bug as mentioned in #6912 (comment)).

There is also a new API for enabling SNI on SSLParameters introduced from Android 24. It requires constructing SNIHostName (also introduced from API 24). This PR also migrates to use that whenever possible.

I tested code execution for configuring Conscrypt under different environments:
(Note SSLSockets#setUseSessionTickets(...) is only supported for com.android.org.conscrypt sockets).

Android API version	Play Service	App-bundled Conscrypt	Socket type	Enable session tickets	Enable SNI	Configure ALPN
R	Yes	No	"com.google.android.gms.org.conscrypt"	Old API	New API	Both due to Conscrypt bug
R	No	No	"com.android.org.conscrypt"	New API	New API	New API
R	No	Yes	"org.conscrypt"	Old API	New API	Both due to Conscrypt bug
27	Yes	No	"com.google.android.gms.org.conscrypt	Old API	New API	Old API
27	No	No	"com.android.org.conscrypt"	Old API	New API	Old API
27	No	Yes	"org.conscrypt"	Old API	New API	Old API

…10.0+

…ypt is bundled with apps.

…tApplicationProtocols is not available.

ejona86

com.google.android.gms.org.conscrypt on R should support the new API. I'm concerned to hear it doesn't. We should probably discuss, since there may be something that invalidated your test.

okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java

ejona86 · 2020-04-14T20:28:46Z

Note: we'd ideally not do this work in the Android Platform and instead use JdkAlpnPlatform. We should be thinking about swapping to the new SNI APIs as well.

ejona86 · 2020-04-15T00:34:02Z

With some testing @voidzcy confirmed that Conscrypt was truly not throwing UnsupportedOperationException for getApplicationProtocols and also failing to do ALPN. On investigating, it appears it is simply because Conscrypt ignores the ALPN SSLParameters on Android: google/conscrypt#832 .

This feels like it will be a thorn in our side for a good while. We will want to figure out a workaround if we can. From the code it looks like we can getSSLParamaters after calling setSSLParameters to see if the ALPN configuration is still present.

…blic ones are not available.

okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java

ejona86 · 2020-04-15T16:21:08Z

okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java

+      sslSocket.setSSLParameters(sslParams);
+      // Check protocol configuration through SSLParameters succeeds. If not, fall back to
+      // configure with the old reflective method.
+      // Workaround for Conscrypt issue: https://github.com/google/conscrypt/issues/832


As written, this is not working around Conscrypt. This is checking whether the API is supported in a Conscrypt-compatible way. For this to be a workaround we would need to do the does-GET_APPLICATION_PROTOCOL-throw check before this, or we would need to say "we aren't checking getApplicationProtocol as a Conscrypt workaround".

I guess you are really referring to SSLParameters#getApplicationProtocols() instead of SSLSocket#getApplicationProtocol() here. The former never throw (we aren't doing the latter during configuration). The getApplicationProtocols() check really is to see if protocols configured on SSLParameters are propagated to SSLSocket, which is only because Conscrypt on Android does this incorrectly. Normally we wouldn't need to do this. Isn't this a workaround?

I agree now it looks more like checking whether the API is supported in a Conscrypt-compatible way due to using duck-type method calls. After changing to normal Method and only do this when APIs are available, it would be clearer.

I was referring to SSLSocket.getApplicationProtocol().

An API was added in Java 9 and Android API 27 to configure ALPN. But that API is implemented by multiple implementations. So we can't assume that if the API is there that it works. I was suggesting to use SSLSocket.getApplicationProtocol() before any negotiation has occurred to see if ALPN is supported in this fashion. If the method throws UnsupportedOperationException then the implementation does not support that API and we should try other APIs. That is the way we decided to handle it in Netty.

I see. That was what we used to find the Conscrypt bug. Due to that bug, even the invocation of SSLSocket.getApplicationProtocol() before any negotiation has occurred does not throw UnsupportedOperationException, ALPN is still not configured correctly. Here, we are working around that bug. Consequently, this check also helps us verifying the SSLSocket is configured with ALPN, isn't it? So do we still need the does-GET_APPLICATION_PROTOCOL-throw check?

voidzcy · 2020-04-15T19:10:45Z

Addressed all the comments and tested locally with different Conscrypt settings. PTAL. Thanks.

…). It cannot be verified now as we are using reflections loaded statically.

… if setApplicationProtocols() can be used.

ejona86

Thank you!

okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java

…N via setApplicationProtocols().

…ndroid Q+) (grpc#6912)" This reverts commit 5803dfd.

…#6959) * Revert "okhttp: Skip enabling SNI and session ticket for fake/test host names (#6949)" This reverts commit eb8e314. * Revert "okhttp: use new APIs for configuring TLS whenever possible (Android Q+) (#6912)" This reverts commit 5803dfd.

…+) (grpc#6912) Use new APIs for configuring TLS in Android environment. Starting from Android 29, there is a new set of public APIs for configuring ALPN (and starting from Android 24, there is API for enabling SNI). This change migrates to use these new APIs whenever possible. Only fallback to call the old hidden APIs if new ones do not exist (or do not work).

…grpc#6959) * Revert "okhttp: Skip enabling SNI and session ticket for fake/test host names (grpc#6949)" This reverts commit eb8e314. * Revert "okhttp: use new APIs for configuring TLS whenever possible (Android Q+) (grpc#6912)" This reverts commit 5803dfd.

voidzcy force-pushed the bugfix/okhttp_config_tls_with_public_apis_on_android_r branch from c47f4dd to f035d6e Compare April 8, 2020 19:00

Use new APIs for configuring TLS whenever possible.

aa9b7ec

voidzcy force-pushed the bugfix/okhttp_config_tls_with_public_apis_on_android_r branch from f035d6e to aa9b7ec Compare April 8, 2020 19:01

Ignore checkstyle warning for empty catch block

0e6e2d0

voidzcy force-pushed the bugfix/okhttp_config_tls_with_public_apis_on_android_r branch from b005895 to 0e6e2d0 Compare April 8, 2020 20:16

voidzcy added 8 commits April 8, 2020 14:30

Add test for verifying invoking getApplicationProtocol() for android …

8e0e641

…10.0+

Fix a wrong reflection call.

741247b

Add comment for explanation.

612225a

Fix checking wrong protocol type.

b29cf26

Should still calls old version (reflective) methods in case of Conscr…

284c51e

…ypt is bundled with apps.

Remove unused method.

745d447

Should still call the old version when using platform socket while se…

197c325

…tApplicationProtocols is not available.

Clean up unused import

ccc3bbe

voidzcy marked this pull request as ready for review April 14, 2020 18:56

voidzcy requested review from ericgribkoff and ejona86 April 14, 2020 18:57

ejona86 reviewed Apr 14, 2020

View reviewed changes

voidzcy added 6 commits April 14, 2020 20:43

Use public API whenever possible, only use old reflective API when pu…

cf373b5

…blic ones are not available.

Use new API for setting SNI whenever possible.

46db165

Better exception propagations.

8e4e3a2

Format

678bbc7

Eliminate ignored exception.

66f0b8e

Format

1fb9b20

ejona86 reviewed Apr 15, 2020

View reviewed changes

Use normal Method for using public APIs.

1eca84a

voidzcy added 2 commits April 15, 2020 12:33

Revert unit test for verifying invocation of getApplicationProtocols(…

7d69401

…). It cannot be verified now as we are using reflections loaded statically.

Check if SSLSocket.getApplicationProtocol() is supported to determine…

be1867e

… if setApplicationProtocols() can be used.

Fix exception catching for UnsupportedOperationException.

a5b7124

voidzcy requested a review from ejona86 April 16, 2020 16:58

ejona86 approved these changes Apr 16, 2020

View reviewed changes

okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java Outdated Show resolved Hide resolved

okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java Show resolved Hide resolved

voidzcy added 3 commits April 16, 2020 14:02

No need to load classes, they are already publicly available.

03e81ae

Add comment for checking if underlying provider supports enabling ALP…

cc435c0

…N via setApplicationProtocols().

Reword comment.

54eff36

voidzcy merged commit 5803dfd into grpc:master Apr 17, 2020

voidzcy added a commit to voidzcy/grpc-java that referenced this pull request Apr 20, 2020

Revert "okhttp: use new APIs for configuring TLS whenever possible (A…

88a4739

…ndroid Q+) (grpc#6912)" This reverts commit 5803dfd.

voidzcy added a commit to voidzcy/grpc-java that referenced this pull request Apr 21, 2020

Revert "okhttp: use new APIs for configuring TLS whenever possible (A…

948f01f

…ndroid Q+) (grpc#6912)" This reverts commit 5803dfd.

github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

okhttp: use new APIs for configuring TLS whenever possible (Android Q+) #6912

okhttp: use new APIs for configuring TLS whenever possible (Android Q+) #6912

voidzcy commented Apr 8, 2020 •

edited

Loading

ejona86 left a comment

ejona86 commented Apr 14, 2020

ejona86 commented Apr 15, 2020

ejona86 Apr 15, 2020

voidzcy Apr 15, 2020 •

edited

Loading

ejona86 Apr 15, 2020

voidzcy Apr 15, 2020 •

edited

Loading

voidzcy commented Apr 15, 2020

ejona86 left a comment

okhttp: use new APIs for configuring TLS whenever possible (Android Q+) #6912

okhttp: use new APIs for configuring TLS whenever possible (Android Q+) #6912

Conversation

voidzcy commented Apr 8, 2020 • edited Loading

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 commented Apr 14, 2020

ejona86 commented Apr 15, 2020

ejona86 Apr 15, 2020

Choose a reason for hiding this comment

voidzcy Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

ejona86 Apr 15, 2020

Choose a reason for hiding this comment

voidzcy Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

voidzcy commented Apr 15, 2020

ejona86 left a comment

Choose a reason for hiding this comment

voidzcy commented Apr 8, 2020 •

edited

Loading

voidzcy Apr 15, 2020 •

edited

Loading

voidzcy Apr 15, 2020 •

edited

Loading