From ff1e460ad50c49e74e1baf8db93255d69178ae01 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 21 Apr 2020 16:15:53 -0700 Subject: [PATCH 1/3] Revert "okhttp: revert changes for using new APIs to configure TLS in Android (#6959)" This reverts commit ee8b395f79a1154df602e7add4d4e2828799e5e1. --- .../grpc/okhttp/OkHttpProtocolNegotiator.java | 173 +++++++++++++++++- 1 file changed, 168 insertions(+), 5 deletions(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java index d981ee2845f..5443675549a 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java @@ -19,17 +19,25 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.annotations.VisibleForTesting; +import io.grpc.internal.GrpcUtil; import io.grpc.okhttp.internal.OptionalMethod; import io.grpc.okhttp.internal.Platform; import io.grpc.okhttp.internal.Platform.TlsExtensionType; import io.grpc.okhttp.internal.Protocol; import io.grpc.okhttp.internal.Util; import java.io.IOException; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.net.Socket; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nullable; +import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLSocket; /** @@ -133,6 +141,69 @@ static final class AndroidNegotiator extends OkHttpProtocolNegotiator { private static final OptionalMethod SET_NPN_PROTOCOLS = new OptionalMethod<>(null, "setNpnProtocols", byte[].class); + // Non-null on Android 10.0+. + // SSLSockets.isSupportedSocket(SSLSocket) + private static final Method SSL_SOCKETS_IS_SUPPORTED_SOCKET; + // SSLSockets.setUseSessionTickets(SSLSocket, boolean) + private static final Method SSL_SOCKETS_SET_USE_SESSION_TICKET; + // SSLParameters.setApplicationProtocols(String[]) + private static final Method SET_APPLICATION_PROTOCOLS; + // SSLParameters.getApplicationProtocols() + private static final Method GET_APPLICATION_PROTOCOLS; + // SSLSocket.getApplicationProtocol() + private static final Method GET_APPLICATION_PROTOCOL; + + // Non-null on Android 7.0+. + // SSLParameters.setServerNames(List) + private static final Method SET_SERVER_NAMES; + // SNIHostName(String) + private static final Constructor SNI_HOST_NAME; + + static { + // Attempt to find Android 10.0+ APIs. + Method setApplicationProtocolsMethod = null; + Method getApplicationProtocolsMethod = null; + Method getApplicationProtocolMethod = null; + Method sslSocketsIsSupportedSocketMethod = null; + Method sslSocketsSetUseSessionTicketsMethod = null; + try { + Class sslParameters = SSLParameters.class; + setApplicationProtocolsMethod = + sslParameters.getMethod("setApplicationProtocols", String[].class); + getApplicationProtocolsMethod = sslParameters.getMethod("getApplicationProtocols"); + getApplicationProtocolMethod = SSLSocket.class.getMethod("getApplicationProtocol"); + Class sslSockets = Class.forName("android.net.ssl.SSLSockets"); + sslSocketsIsSupportedSocketMethod = + sslSockets.getMethod("isSupportedSocket", SSLSocket.class); + sslSocketsSetUseSessionTicketsMethod = + sslSockets.getMethod("setUseSessionTickets", SSLSocket.class, boolean.class); + } catch (ClassNotFoundException e) { + logger.log(Level.FINER, "Failed to find Android 10.0+ APIs", e); + } catch (NoSuchMethodException e) { + logger.log(Level.FINER, "Failed to find Android 10.0+ APIs", e); + } + SET_APPLICATION_PROTOCOLS = setApplicationProtocolsMethod; + GET_APPLICATION_PROTOCOLS = getApplicationProtocolsMethod; + GET_APPLICATION_PROTOCOL = getApplicationProtocolMethod; + SSL_SOCKETS_IS_SUPPORTED_SOCKET = sslSocketsIsSupportedSocketMethod; + SSL_SOCKETS_SET_USE_SESSION_TICKET = sslSocketsSetUseSessionTicketsMethod; + + // Attempt to find Android 7.0+ APIs. + Method setServerNamesMethod = null; + Constructor sniHostNameConstructor = null; + try { + setServerNamesMethod = SSLParameters.class.getMethod("setServerNames", List.class); + sniHostNameConstructor = + Class.forName("javax.net.ssl.SNIHostName").getConstructor(String.class); + } catch (ClassNotFoundException e) { + logger.log(Level.FINER, "Failed to find Android 7.0+ APIs", e); + } catch (NoSuchMethodException e) { + logger.log(Level.FINER, "Failed to find Android 7.0+ APIs", e); + } + SET_SERVER_NAMES = setServerNamesMethod; + SNI_HOST_NAME = sniHostNameConstructor; + } + AndroidNegotiator(Platform platform) { super(platform); } @@ -152,21 +223,79 @@ public String negotiate(SSLSocket sslSocket, String hostname, List pro /** * Override {@link Platform}'s configureTlsExtensions for Android older than 5.0, since OkHttp * (2.3+) only support such function for Android 5.0+. + * + *

Note: Prior to Android Q, the standard way of accessing some Conscrypt features was to + * use reflection to call hidden APIs. Beginning in Q, there is public API for all of these + * features. We attempt to use the public API where possible. Otherwise, fall back to use the + * old reflective API. */ @Override protected void configureTlsExtensions( SSLSocket sslSocket, String hostname, List protocols) { - // Enable SNI and session tickets. - if (hostname != null) { - SET_USE_SESSION_TICKETS.invokeOptionalWithoutCheckedException(sslSocket, true); - SET_HOSTNAME.invokeOptionalWithoutCheckedException(sslSocket, hostname); + String[] protocolNames = protocolIds(protocols); + SSLParameters sslParams = sslSocket.getSSLParameters(); + try { + // Enable SNI and session tickets. + // Hostname is normally validated in the builder (see checkAuthority) and it should + // virtually always succeed. Check again here to avoid troubles (e.g., hostname with + // underscore) enabling SNI, which works around cases where checkAuthority is disabled. + // See b/154375837. + if (hostname != null && isValidHostName(hostname)) { + if (SSL_SOCKETS_IS_SUPPORTED_SOCKET != null + && (boolean) SSL_SOCKETS_IS_SUPPORTED_SOCKET.invoke(null, sslSocket)) { + SSL_SOCKETS_SET_USE_SESSION_TICKET.invoke(null, sslSocket, true); + } else { + SET_USE_SESSION_TICKETS.invokeOptionalWithoutCheckedException(sslSocket, true); + } + if (SET_SERVER_NAMES != null && SNI_HOST_NAME != null) { + SET_SERVER_NAMES + .invoke(sslParams, Collections.singletonList(SNI_HOST_NAME.newInstance(hostname))); + } else { + SET_HOSTNAME.invokeOptionalWithoutCheckedException(sslSocket, hostname); + } + } + boolean alpnEnabled = false; + if (GET_APPLICATION_PROTOCOL != null) { + try { + // If calling SSLSocket.getApplicationProtocol() throws UnsupportedOperationException, + // the underlying provider does not implement operations for enabling + // ALPN in the fashion of SSLParameters.setApplicationProtocols(). Fall back to + // use old hidden methods. + GET_APPLICATION_PROTOCOL.invoke(sslSocket); + SET_APPLICATION_PROTOCOLS.invoke(sslParams, (Object) protocolNames); + alpnEnabled = true; + } catch (InvocationTargetException e) { + Throwable targetException = e.getTargetException(); + if (targetException instanceof UnsupportedOperationException) { + logger.log(Level.FINER, "setApplicationProtocol unsupported, will try old methods"); + } else { + throw e; + } + } + } + sslSocket.setSSLParameters(sslParams); + // Check application protocols are configured correctly. If not, configure again with + // old methods. + // Workaround for Conscrypt bug: https://github.com/google/conscrypt/issues/832 + if (alpnEnabled && GET_APPLICATION_PROTOCOLS != null) { + String[] configuredProtocols = + (String[]) GET_APPLICATION_PROTOCOLS.invoke(sslSocket.getSSLParameters()); + if (Arrays.equals(protocolNames, configuredProtocols)) { + return; + } + } + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } catch (InvocationTargetException e) { + throw new RuntimeException(e); + } catch (InstantiationException e) { + throw new RuntimeException(e); } Object[] parameters = {Platform.concatLengthPrefixed(protocols)}; if (platform.getTlsExtensionType() == TlsExtensionType.ALPN_AND_NPN) { SET_ALPN_PROTOCOLS.invokeWithoutCheckedException(sslSocket, parameters); } - if (platform.getTlsExtensionType() != TlsExtensionType.NONE) { SET_NPN_PROTOCOLS.invokeWithoutCheckedException(sslSocket, parameters); } else { @@ -177,6 +306,23 @@ protected void configureTlsExtensions( @Override public String getSelectedProtocol(SSLSocket socket) { + if (GET_APPLICATION_PROTOCOL != null) { + try { + return (String) GET_APPLICATION_PROTOCOL.invoke(socket); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } catch (InvocationTargetException e) { + Throwable targetException = e.getTargetException(); + if (targetException instanceof UnsupportedOperationException) { + logger.log( + Level.FINER, + "Socket unsupported for getApplicationProtocol, will try old methods"); + } else { + throw new RuntimeException(e); + } + } + } + if (platform.getTlsExtensionType() == TlsExtensionType.ALPN_AND_NPN) { try { byte[] alpnResult = @@ -207,4 +353,21 @@ public String getSelectedProtocol(SSLSocket socket) { return null; } } + + private static String[] protocolIds(List protocols) { + List result = new ArrayList<>(); + for (Protocol protocol : protocols) { + result.add(protocol.toString()); + } + return result.toArray(new String[0]); + } + + private static boolean isValidHostName(String name) { + try { + GrpcUtil.checkAuthority(name); + return true; + } catch (IllegalArgumentException e) { + return false; + } + } } From 9daa679841d2b8bf991cc2e6d0c5aa8b3c1f6aed Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 21 Apr 2020 16:30:12 -0700 Subject: [PATCH 2/3] Manually check underscore for hostname, as Android's URI implementation allows underscore in hostname. --- .../java/io/grpc/okhttp/OkHttpProtocolNegotiator.java | 8 +++++++- .../java/io/grpc/okhttp/OkHttpProtocolNegotiatorTest.java | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java index 5443675549a..2dc09438c30 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java @@ -362,7 +362,13 @@ private static String[] protocolIds(List protocols) { return result.toArray(new String[0]); } - private static boolean isValidHostName(String name) { + @VisibleForTesting + static boolean isValidHostName(String name) { + // GrpcUtil.checkAuthority() depends on URI implementation, while Android's URI implementation + // allows underscore in hostname. Manually disallow hostname with underscore to avoid troubles. + if (name.contains("_")) { + return false; + } try { GrpcUtil.checkAuthority(name); return true; diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpProtocolNegotiatorTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpProtocolNegotiatorTest.java index 61214f169da..fefa598fdd6 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpProtocolNegotiatorTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpProtocolNegotiatorTest.java @@ -18,6 +18,7 @@ import static com.google.common.base.Charsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; @@ -295,4 +296,9 @@ public void setWantClientAuth(boolean want) {} @Override public void startHandshake() throws IOException {} } + + @Test + public void isValidHostName_withUnderscore() { + assertFalse(OkHttpProtocolNegotiator.isValidHostName("test_cert_2")); + } } From b77f962ad7add3b74aa34fcd58cf0cb070cfb707 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 21 Apr 2020 16:37:56 -0700 Subject: [PATCH 3/3] Link related bug number. --- .../src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java | 1 + 1 file changed, 1 insertion(+) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java index 2dc09438c30..d09d6cccedd 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpProtocolNegotiator.java @@ -366,6 +366,7 @@ private static String[] protocolIds(List protocols) { static boolean isValidHostName(String name) { // GrpcUtil.checkAuthority() depends on URI implementation, while Android's URI implementation // allows underscore in hostname. Manually disallow hostname with underscore to avoid troubles. + // See b/154375837. if (name.contains("_")) { return false; }