From 57a299d3ebdd16e2c8ce8d34cf03b91c5f1fc488 Mon Sep 17 00:00:00 2001 From: Sam Kramer Date: Mon, 24 Oct 2022 16:46:18 +0100 Subject: [PATCH 1/7] Provide better logging for endpoint verification by listing all endpoints checked --- .../keyvalue/cassandra/CassandraClientFactory.java | 14 ++++++++------ .../cassandra/CassandraClientFactoryTest.java | 8 ++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java index d12d1f6a279..a14ae5c4ae0 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java @@ -19,6 +19,7 @@ import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableSet; import com.palantir.atlasdb.cassandra.CassandraCredentialsConfig; import com.palantir.atlasdb.cassandra.CassandraKeyValueServiceConfig; import com.palantir.atlasdb.keyvalue.cassandra.ImmutableCassandraClientConfig.SocketTimeoutMillisBuildStage; @@ -44,7 +45,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Optional; -import java.util.stream.Stream; +import java.util.Set; import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory; import org.apache.cassandra.thrift.AuthenticationRequest; @@ -226,15 +227,16 @@ private static void login(Client client, CassandraCredentialsConfig config) thro @VisibleForTesting static void verifyEndpoint(CassandraServer cassandraServer, SSLSocket socket, boolean throwOnFailure) throws SafeSSLPeerUnverifiedException { - boolean endpointVerified = Stream.of( - socket.getInetAddress().getHostAddress(), cassandraServer.cassandraHostName()) - .anyMatch(address -> hostnameVerifier.verify(address, socket.getSession())); + Set endpointsToCheck = + ImmutableSet.of(socket.getInetAddress().getHostAddress(), cassandraServer.cassandraHostName()); + boolean endpointVerified = + endpointsToCheck.stream().anyMatch(address -> hostnameVerifier.verify(address, socket.getSession())); if (!endpointVerified) { - log.warn("Endpoint verification failed for host.", SafeArg.of("cassandraServer", cassandraServer)); + log.warn("Endpoint verification failed for host.", SafeArg.of("endpointsToCheck", endpointsToCheck)); if (throwOnFailure) { throw new SafeSSLPeerUnverifiedException( - "Endpoint verification failed for host.", SafeArg.of("cassandraServer", cassandraServer)); + "Endpoint verification failed for host.", SafeArg.of("endpointsToCheck", endpointsToCheck)); } } } diff --git a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java index 600df925e28..4c797d5b7e6 100644 --- a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java +++ b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java @@ -114,6 +114,14 @@ public void verifyEndpointDoesNotThrowWhenHostnameNotPresentButIpIs() { .doesNotThrowAnyException(); } + @Test + public void verifyEndpointDoesNotThrownWhenHostnameIpAreDuplicates() { + CassandraServer cassandraServer = CassandraServer.of(InetSocketAddress.createUnresolved("1.2.3.4", 4000)); + SSLSocket sslSocket = createSSLSocket(cassandraServer, DEFAULT_ADDRESS); + assertThatCode(() -> CassandraClientFactory.verifyEndpoint(cassandraServer, sslSocket, true)) + .doesNotThrowAnyException(); + } + @SuppressWarnings("ReverseDnsLookup") private static InetSocketAddress mockInetSocketAddress(String ipAddress) { InetAddress inetAddress = mockInetAddress(ipAddress); From 82459c7680babbf37ec8228c0f07d763b1da06c6 Mon Sep 17 00:00:00 2001 From: Sam Kramer Date: Mon, 24 Oct 2022 17:00:24 +0100 Subject: [PATCH 2/7] Test separate method for obtaining endpoints to verify --- .../keyvalue/cassandra/CassandraClientFactory.java | 8 ++++++-- .../keyvalue/cassandra/CassandraClientFactoryTest.java | 10 ++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java index a14ae5c4ae0..7ff4b43b719 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java @@ -227,8 +227,7 @@ private static void login(Client client, CassandraCredentialsConfig config) thro @VisibleForTesting static void verifyEndpoint(CassandraServer cassandraServer, SSLSocket socket, boolean throwOnFailure) throws SafeSSLPeerUnverifiedException { - Set endpointsToCheck = - ImmutableSet.of(socket.getInetAddress().getHostAddress(), cassandraServer.cassandraHostName()); + Set endpointsToCheck = getEndpointsToCheck(cassandraServer, socket); boolean endpointVerified = endpointsToCheck.stream().anyMatch(address -> hostnameVerifier.verify(address, socket.getSession())); @@ -241,6 +240,11 @@ static void verifyEndpoint(CassandraServer cassandraServer, SSLSocket socket, bo } } + @VisibleForTesting + static Set getEndpointsToCheck(CassandraServer cassandraServer, SSLSocket socket) { + return ImmutableSet.of(socket.getInetAddress().getHostAddress(), cassandraServer.cassandraHostName()); + } + @Override public boolean validateObject(PooledObject client) { try { diff --git a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java index 4c797d5b7e6..08f653c689f 100644 --- a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java +++ b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java @@ -115,11 +115,13 @@ public void verifyEndpointDoesNotThrowWhenHostnameNotPresentButIpIs() { } @Test - public void verifyEndpointDoesNotThrownWhenHostnameIpAreDuplicates() { - CassandraServer cassandraServer = CassandraServer.of(InetSocketAddress.createUnresolved("1.2.3.4", 4000)); + public void getEndpointsToCheckDeduplicatesMatchingHostnameIp() { + CassandraServer cassandraServer = + CassandraServer.of(InetSocketAddress.createUnresolved(DEFAULT_ADDRESS.getHostAddress(), 4000)); SSLSocket sslSocket = createSSLSocket(cassandraServer, DEFAULT_ADDRESS); - assertThatCode(() -> CassandraClientFactory.verifyEndpoint(cassandraServer, sslSocket, true)) - .doesNotThrowAnyException(); + assertThat(CassandraClientFactory.getEndpointsToCheck(cassandraServer, sslSocket)) + .isNotEmpty() + .containsExactly(DEFAULT_ADDRESS.getHostAddress()); } @SuppressWarnings("ReverseDnsLookup") From 992f020aa5742ce2ae507b89725426b1165a8227 Mon Sep 17 00:00:00 2001 From: Sam Kramer Date: Mon, 24 Oct 2022 17:07:34 +0100 Subject: [PATCH 3/7] Also test case where we should not expect deduplication --- .../keyvalue/cassandra/CassandraClientFactoryTest.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java index 08f653c689f..5d7e87b5ddf 100644 --- a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java +++ b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java @@ -124,6 +124,14 @@ public void getEndpointsToCheckDeduplicatesMatchingHostnameIp() { .containsExactly(DEFAULT_ADDRESS.getHostAddress()); } + @Test + public void getEndpointsToCheckPerformsNoDeduplicationWhenHostnameIpDiffer() { + SSLSocket sslSocket = createSSLSocket(DEFAULT_SERVER, DEFAULT_ADDRESS); + assertThat(CassandraClientFactory.getEndpointsToCheck(DEFAULT_SERVER, sslSocket)) + .isNotEmpty() + .containsExactlyInAnyOrder(DEFAULT_SERVER.cassandraHostName(), DEFAULT_ADDRESS.getHostAddress()); + } + @SuppressWarnings("ReverseDnsLookup") private static InetSocketAddress mockInetSocketAddress(String ipAddress) { InetAddress inetAddress = mockInetAddress(ipAddress); From 3e585dfdefe135f3ef1783e2b4d5c01002a9bb45 Mon Sep 17 00:00:00 2001 From: Sam Kramer Date: Tue, 25 Oct 2022 10:53:40 +0100 Subject: [PATCH 4/7] Throw socketexception when verifying endpoints if the socket is closed when configured to do so. --- .../cassandra/CassandraClientFactory.java | 13 ++++++++++--- .../cassandra/CassandraClientFactoryTest.java | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java index 7ff4b43b719..52718e387bd 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java @@ -180,8 +180,8 @@ private static Cassandra.Client getRawClient( try { SSLSocket socket = (SSLSocket) sslSocketFactory.createSocket( thriftSocket.getSocket(), addr.getHostString(), addr.getPort(), true); - verifyEndpoint(cassandraServer, socket, clientConfig.enableEndpointVerification()); thriftSocket = tSocketFactory.create(socket); + verifyEndpoint(cassandraServer, socket, clientConfig.enableEndpointVerification()); success = true; } catch (IOException e) { throw new TTransportException(e); @@ -223,14 +223,21 @@ private static void login(Client client, CassandraCredentialsConfig config) thro * This will check both ip address/hostname, and uses the IP address associated with the socket, rather * that what has been provided. Hostname/ip address are both need to be checked, as historically we've * connected to Cassandra directly using IP addresses, and therefore need to support such cases. + * + * Will only throw when throwOnFailure is true, even if the socket is closed during verification. */ @VisibleForTesting static void verifyEndpoint(CassandraServer cassandraServer, SSLSocket socket, boolean throwOnFailure) - throws SafeSSLPeerUnverifiedException { + throws SafeSSLPeerUnverifiedException, SocketException { Set endpointsToCheck = getEndpointsToCheck(cassandraServer, socket); boolean endpointVerified = endpointsToCheck.stream().anyMatch(address -> hostnameVerifier.verify(address, socket.getSession())); - + if (socket.isClosed()) { + if (throwOnFailure) { + throw new SocketException("Unable to verify hostnames as socket is closed."); + } + return; + } if (!endpointVerified) { log.warn("Endpoint verification failed for host.", SafeArg.of("endpointsToCheck", endpointsToCheck)); if (throwOnFailure) { diff --git a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java index 5d7e87b5ddf..eedb0b527a5 100644 --- a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java +++ b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java @@ -30,6 +30,7 @@ import com.palantir.exception.SafeSSLPeerUnverifiedException; import java.net.InetAddress; import java.net.InetSocketAddress; +import java.net.SocketException; import java.security.cert.Certificate; import java.security.cert.CertificateParsingException; import java.security.cert.X509Certificate; @@ -114,6 +115,22 @@ public void verifyEndpointDoesNotThrowWhenHostnameNotPresentButIpIs() { .doesNotThrowAnyException(); } + @Test + public void verifyEndpointThrowsWhenSocketIsClosed() { + SSLSocket sslSocket = createSSLSocket(DEFAULT_SERVER, DEFAULT_ADDRESS); + when(sslSocket.isClosed()).thenReturn(true); + assertThatThrownBy(() -> CassandraClientFactory.verifyEndpoint(DEFAULT_SERVER, sslSocket, true)) + .isInstanceOf(SocketException.class); + } + + @Test + public void verifyEndpointDoesNotThrowWhenSocketIsClosedAndThrowOnFailureIsFalse() { + SSLSocket sslSocket = createSSLSocket(DEFAULT_SERVER, DEFAULT_ADDRESS); + when(sslSocket.isClosed()).thenReturn(true); + assertThatCode(() -> CassandraClientFactory.verifyEndpoint(DEFAULT_SERVER, sslSocket, false)) + .doesNotThrowAnyException(); + } + @Test public void getEndpointsToCheckDeduplicatesMatchingHostnameIp() { CassandraServer cassandraServer = From 6253c910c34ef75468f8e8731bf4ead5ebf69cb1 Mon Sep 17 00:00:00 2001 From: Sam Kramer Date: Tue, 25 Oct 2022 10:55:35 +0100 Subject: [PATCH 5/7] Change log var to be past tense --- .../atlasdb/keyvalue/cassandra/CassandraClientFactory.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java index 52718e387bd..c40d8e63722 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java @@ -239,10 +239,10 @@ static void verifyEndpoint(CassandraServer cassandraServer, SSLSocket socket, bo return; } if (!endpointVerified) { - log.warn("Endpoint verification failed for host.", SafeArg.of("endpointsToCheck", endpointsToCheck)); + log.warn("Endpoint verification failed for host.", SafeArg.of("endpointsChecked", endpointsToCheck)); if (throwOnFailure) { throw new SafeSSLPeerUnverifiedException( - "Endpoint verification failed for host.", SafeArg.of("endpointsToCheck", endpointsToCheck)); + "Endpoint verification failed for host.", SafeArg.of("endpointsChecked", endpointsToCheck)); } } } From 1c20c4f3e3af11e81816ff126f7e803f3e2f08c2 Mon Sep 17 00:00:00 2001 From: Sam Kramer Date: Tue, 25 Oct 2022 11:32:09 +0100 Subject: [PATCH 6/7] Change exception type to SafeSSLPeerUnverifiedException --- .../atlasdb/keyvalue/cassandra/CassandraClientFactory.java | 6 ++++-- .../keyvalue/cassandra/CassandraClientFactoryTest.java | 3 +-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java index c40d8e63722..a91478a3843 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactory.java @@ -228,13 +228,15 @@ private static void login(Client client, CassandraCredentialsConfig config) thro */ @VisibleForTesting static void verifyEndpoint(CassandraServer cassandraServer, SSLSocket socket, boolean throwOnFailure) - throws SafeSSLPeerUnverifiedException, SocketException { + throws SafeSSLPeerUnverifiedException { Set endpointsToCheck = getEndpointsToCheck(cassandraServer, socket); boolean endpointVerified = endpointsToCheck.stream().anyMatch(address -> hostnameVerifier.verify(address, socket.getSession())); if (socket.isClosed()) { if (throwOnFailure) { - throw new SocketException("Unable to verify hostnames as socket is closed."); + throw new SafeSSLPeerUnverifiedException( + "Unable to verify endpoints as socket is closed.", + SafeArg.of("endpoint", socket.getInetAddress())); } return; } diff --git a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java index eedb0b527a5..175d9caacae 100644 --- a/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java +++ b/atlasdb-cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientFactoryTest.java @@ -30,7 +30,6 @@ import com.palantir.exception.SafeSSLPeerUnverifiedException; import java.net.InetAddress; import java.net.InetSocketAddress; -import java.net.SocketException; import java.security.cert.Certificate; import java.security.cert.CertificateParsingException; import java.security.cert.X509Certificate; @@ -120,7 +119,7 @@ public void verifyEndpointThrowsWhenSocketIsClosed() { SSLSocket sslSocket = createSSLSocket(DEFAULT_SERVER, DEFAULT_ADDRESS); when(sslSocket.isClosed()).thenReturn(true); assertThatThrownBy(() -> CassandraClientFactory.verifyEndpoint(DEFAULT_SERVER, sslSocket, true)) - .isInstanceOf(SocketException.class); + .isInstanceOf(SafeSSLPeerUnverifiedException.class); } @Test From 755df96158b43587aebc1df9f17205f41c3c2feb Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Tue, 25 Oct 2022 10:42:07 +0000 Subject: [PATCH 7/7] Add generated changelog entries --- changelog/@unreleased/pr-6314.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-6314.v2.yml diff --git a/changelog/@unreleased/pr-6314.v2.yml b/changelog/@unreleased/pr-6314.v2.yml new file mode 100644 index 00000000000..a84782dd20b --- /dev/null +++ b/changelog/@unreleased/pr-6314.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: If hostname/ip address are duplicates, only check one when performing + endpoint verification. + links: + - https://github.com/palantir/atlasdb/pull/6314