From 9fdaa3684369dd6112d887e3dea2893d4573f61f Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Wed, 2 Oct 2024 08:51:48 -0700 Subject: [PATCH 1/4] throw IllegalArgumentException in ProtoUtil. --- .../java/io/grpc/s2a/internal/handshaker/ProtoUtil.java | 5 +++-- .../grpc/s2a/internal/handshaker/SslContextFactory.java | 6 ++++-- .../io/grpc/s2a/internal/handshaker/ProtoUtilTest.java | 8 ++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java index 1d88d5a2b55..7cab06a85a5 100644 --- a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java +++ b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java @@ -27,7 +27,8 @@ final class ProtoUtil { * * @param tlsVersion the {@link TLSVersion} object to be converted. * @return a {@link String} representation of the TLS version. - * @throws AssertionError if the {@code tlsVersion} is not one of the supported TLS versions. + * @throws IllegalArgumentException if the {@code tlsVersion} is not one of + * the supported TLS versions. */ @VisibleForTesting static String convertTlsProtocolVersion(TLSVersion tlsVersion) { @@ -41,7 +42,7 @@ static String convertTlsProtocolVersion(TLSVersion tlsVersion) { case TLS_VERSION_1_0: return "TLSv1"; default: - throw new AssertionError( + throw new IllegalArgumentException( String.format("TLS version %d is not supported.", tlsVersion.getNumber())); } } diff --git a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java index 72ace2c7885..aec4ef6ebce 100644 --- a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java +++ b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java @@ -63,7 +63,8 @@ static SslContext createForClient( KeyStoreException, NoSuchAlgorithmException, UnrecoverableKeyException, - GeneralSecurityException { + GeneralSecurityException, + IllegalArgumentException { checkNotNull(stub, "stub should not be null."); checkNotNull(targetName, "targetName should not be null on client side."); GetTlsConfigurationResp.ClientTlsConfiguration clientTlsConfiguration; @@ -136,7 +137,8 @@ private static void configureSslContextWithClientTlsConfiguration( IOException, KeyStoreException, NoSuchAlgorithmException, - UnrecoverableKeyException { + UnrecoverableKeyException, + IllegalArgumentException { sslContextBuilder.keyManager(createKeylessManager(clientTlsConfiguration)); ImmutableSet tlsVersions = ProtoUtil.buildTlsProtocolVersionSet( diff --git a/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java b/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java index b685d0bc755..2b42002660f 100644 --- a/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java +++ b/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java @@ -46,9 +46,9 @@ public void convertTlsProtocolVersion_success() { @Test public void convertTlsProtocolVersion_withUnknownTlsVersion_fails() { - AssertionError expected = + IllegalArgumentException expected = assertThrows( - AssertionError.class, + IllegalArgumentException.class, () -> ProtoUtil.convertTlsProtocolVersion(TLSVersion.TLS_VERSION_UNSPECIFIED)); expect.that(expected).hasMessageThat().isEqualTo("TLS version 0 is not supported."); } @@ -79,9 +79,9 @@ public void buildTlsProtocolVersionSet_success() { @Test public void buildTlsProtocolVersionSet_failure() { - AssertionError expected = + IllegalArgumentException expected = assertThrows( - AssertionError.class, + IllegalArgumentException.class, () -> ProtoUtil.buildTlsProtocolVersionSet( TLSVersion.TLS_VERSION_UNSPECIFIED, TLSVersion.TLS_VERSION_1_3)); From f264c580cbeb6d9d4b131e812e7b3b55159c7f38 Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Wed, 2 Oct 2024 08:59:12 -0700 Subject: [PATCH 2/4] throw exception in TrustManager in more standard way. --- .../io/grpc/s2a/internal/handshaker/S2ATrustManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/S2ATrustManager.java b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/S2ATrustManager.java index 2f7e5750f88..406545b30bf 100644 --- a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/S2ATrustManager.java +++ b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/S2ATrustManager.java @@ -120,10 +120,10 @@ private void checkPeerTrusted(X509Certificate[] chain, boolean isCheckingClientC SessionResp resp; try { resp = stub.send(reqBuilder.build()); - } catch (IOException | InterruptedException e) { - if (e instanceof InterruptedException) { - Thread.currentThread().interrupt(); - } + } catch (IOException e) { + throw new CertificateException("Failed to send request to S2A.", e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); throw new CertificateException("Failed to send request to S2A.", e); } if (resp.hasStatus() && resp.getStatus().getCode() != 0) { From 0b1e0bf790f29aaa3b2e0ffd61d26945fa2d46db Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Wed, 2 Oct 2024 14:27:23 -0700 Subject: [PATCH 3/4] handle IllegalArgumentException in SslContextFactory. --- .../internal/handshaker/SslContextFactory.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java index aec4ef6ebce..270f586bed7 100644 --- a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java +++ b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java @@ -63,8 +63,7 @@ static SslContext createForClient( KeyStoreException, NoSuchAlgorithmException, UnrecoverableKeyException, - GeneralSecurityException, - IllegalArgumentException { + GeneralSecurityException { checkNotNull(stub, "stub should not be null."); checkNotNull(targetName, "targetName should not be null on client side."); GetTlsConfigurationResp.ClientTlsConfiguration clientTlsConfiguration; @@ -137,12 +136,16 @@ private static void configureSslContextWithClientTlsConfiguration( IOException, KeyStoreException, NoSuchAlgorithmException, - UnrecoverableKeyException, - IllegalArgumentException { + UnrecoverableKeyException { sslContextBuilder.keyManager(createKeylessManager(clientTlsConfiguration)); - ImmutableSet tlsVersions = - ProtoUtil.buildTlsProtocolVersionSet( - clientTlsConfiguration.getMinTlsVersion(), clientTlsConfiguration.getMaxTlsVersion()); + ImmutableSet tlsVersions; + try { + tlsVersions = + ProtoUtil.buildTlsProtocolVersionSet( + clientTlsConfiguration.getMinTlsVersion(), clientTlsConfiguration.getMaxTlsVersion()); + } catch (IllegalArgumentException e) { + throw new IOException("Received invalid TLS version information from S2A.", e); + } if (tlsVersions.isEmpty()) { throw new S2AConnectionException("Set of TLS versions received from S2A server is empty."); } From dfd8036380aed6376fd31ee0539e6256df49dbec Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Fri, 4 Oct 2024 15:20:04 -0700 Subject: [PATCH 4/4] Don't throw error on unknown TLS version. --- .../io/grpc/s2a/internal/handshaker/ProtoUtil.java | 6 +++++- .../s2a/internal/handshaker/SslContextFactory.java | 13 +++++-------- .../grpc/s2a/internal/handshaker/ProtoUtilTest.java | 12 +++++------- .../internal/handshaker/SslContextFactoryTest.java | 2 +- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java index 7cab06a85a5..1f24727a083 100644 --- a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java +++ b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/ProtoUtil.java @@ -63,7 +63,11 @@ static ImmutableSet buildTlsProtocolVersionSet( } if (versionNumber >= minTlsVersion.getNumber() && versionNumber <= maxTlsVersion.getNumber()) { - tlsVersions.add(convertTlsProtocolVersion(tlsVersion)); + try { + tlsVersions.add(convertTlsProtocolVersion(tlsVersion)); + } catch (IllegalArgumentException e) { + continue; + } } } return tlsVersions.build(); diff --git a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java index 270f586bed7..3e5481daa9e 100644 --- a/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java +++ b/s2a/src/main/java/io/grpc/s2a/internal/handshaker/SslContextFactory.java @@ -139,15 +139,12 @@ private static void configureSslContextWithClientTlsConfiguration( UnrecoverableKeyException { sslContextBuilder.keyManager(createKeylessManager(clientTlsConfiguration)); ImmutableSet tlsVersions; - try { - tlsVersions = - ProtoUtil.buildTlsProtocolVersionSet( - clientTlsConfiguration.getMinTlsVersion(), clientTlsConfiguration.getMaxTlsVersion()); - } catch (IllegalArgumentException e) { - throw new IOException("Received invalid TLS version information from S2A.", e); - } + tlsVersions = + ProtoUtil.buildTlsProtocolVersionSet( + clientTlsConfiguration.getMinTlsVersion(), clientTlsConfiguration.getMaxTlsVersion()); if (tlsVersions.isEmpty()) { - throw new S2AConnectionException("Set of TLS versions received from S2A server is empty."); + throw new S2AConnectionException("Set of TLS versions received from S2A server is" + + " empty or not supported."); } sslContextBuilder.protocols(tlsVersions); } diff --git a/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java b/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java index 2b42002660f..f60aa1a189b 100644 --- a/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java +++ b/s2a/src/test/java/io/grpc/s2a/internal/handshaker/ProtoUtilTest.java @@ -79,12 +79,10 @@ public void buildTlsProtocolVersionSet_success() { @Test public void buildTlsProtocolVersionSet_failure() { - IllegalArgumentException expected = - assertThrows( - IllegalArgumentException.class, - () -> - ProtoUtil.buildTlsProtocolVersionSet( - TLSVersion.TLS_VERSION_UNSPECIFIED, TLSVersion.TLS_VERSION_1_3)); - expect.that(expected).hasMessageThat().isEqualTo("TLS version 0 is not supported."); + expect + .that( + ProtoUtil.buildTlsProtocolVersionSet( + TLSVersion.TLS_VERSION_UNSPECIFIED, TLSVersion.TLS_VERSION_1_3)) + .isEqualTo(ImmutableSet.of("TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3")); } } \ No newline at end of file diff --git a/s2a/src/test/java/io/grpc/s2a/internal/handshaker/SslContextFactoryTest.java b/s2a/src/test/java/io/grpc/s2a/internal/handshaker/SslContextFactoryTest.java index fc3cfb5e441..17b834abf2a 100644 --- a/s2a/src/test/java/io/grpc/s2a/internal/handshaker/SslContextFactoryTest.java +++ b/s2a/src/test/java/io/grpc/s2a/internal/handshaker/SslContextFactoryTest.java @@ -142,7 +142,7 @@ public void createForClient_getsBadTlsVersionsFromServer_throwsError() throws Ex assertThat(expected) .hasMessageThat() - .contains("Set of TLS versions received from S2A server is empty."); + .contains("Set of TLS versions received from S2A server is empty or not supported."); } @Test