This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new 84d034589d Clean up Thrift SSL code (#3787)
84d034589d is described below
commit 84d034589d4454cadcf72c0f668393d342f64325
Author: Christopher Tubbs <ctubb...@apache.org>
AuthorDate: Thu Sep 28 19:12:55 2023 -0400
Clean up Thrift SSL code (#3787)
* Remove unneeded ProtocolOverridingSSLSocketFactory that was used to
support older JDK versions that we no longer support
* Clean up SslConnectionParams class to remove a workaround for a bug
that was patched upstream in Thrift
* Use Thrift's provided TSSLTransportFactory.getClientSocket method that
allows us to provide custom TSSLTransportParameters and delete the
private methods copied from Thrift 0.9.1 that were used to implement
this ourselves (methods which were not kept up to date with the
improvements in upstream Thrift)
* Rename SslConnectionParams.getTTransportParams to
.getTSSLTransportParameters, because the old name for the getter
method did not reflect the actual type that it returned, and the
old name did not make it clear it was returning a parameters object
specifically for SSL / TLS
---------
* Update exception message to refer to the TSSLTransportParameters name
instead of TTransportParams
Co-authored-by: EdColeman <d...@etcoleman.com>
---
.../rpc/ProtocolOverridingSSLSocketFactory.java | 111 ---------------------
.../accumulo/core/rpc/SslConnectionParams.java | 20 +---
.../org/apache/accumulo/core/rpc/ThriftUtil.java | 108 +-------------------
.../apache/accumulo/server/rpc/TServerUtils.java | 2 +-
4 files changed, 8 insertions(+), 233 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/rpc/ProtocolOverridingSSLSocketFactory.java
b/core/src/main/java/org/apache/accumulo/core/rpc/ProtocolOverridingSSLSocketFactory.java
deleted file mode 100644
index 1f89306236..0000000000
---
a/core/src/main/java/org/apache/accumulo/core/rpc/ProtocolOverridingSSLSocketFactory.java
+++ /dev/null
@@ -1,111 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.core.rpc;
-
-import static com.google.common.base.Preconditions.checkArgument;
-import static java.util.Objects.requireNonNull;
-
-import java.io.IOException;
-import java.net.InetAddress;
-import java.net.Socket;
-import java.net.UnknownHostException;
-
-import javax.net.ssl.SSLSocket;
-import javax.net.ssl.SSLSocketFactory;
-
-/**
- * JDK6's SSLSocketFactory doesn't seem to properly set the protocols on the
Sockets that it creates
- * which causes an SSLv2 client hello message during handshake, even when only
TLSv1 is enabled.
- * This only appears to be an issue on the client sockets, not the server
sockets.
- *
- * This class wraps the SSLSocketFactory ensuring that the Socket is properly
configured.
- * https://www.coderanch.com/t/637177/Security/Disabling-handshake-message-Java
- *
- * This class can be removed when JDK6 support is officially unsupported by
Accumulo
- */
-class ProtocolOverridingSSLSocketFactory extends SSLSocketFactory {
-
- private final SSLSocketFactory delegate;
- private final String[] enabledProtocols;
-
- public ProtocolOverridingSSLSocketFactory(final SSLSocketFactory delegate,
- final String[] enabledProtocols) {
- requireNonNull(enabledProtocols);
- checkArgument(enabledProtocols.length != 0, "Expected at least one
protocol");
- this.delegate = delegate;
- this.enabledProtocols = enabledProtocols;
- }
-
- @Override
- public String[] getDefaultCipherSuites() {
- return delegate.getDefaultCipherSuites();
- }
-
- @Override
- public String[] getSupportedCipherSuites() {
- return delegate.getSupportedCipherSuites();
- }
-
- @Override
- public Socket createSocket(final Socket socket, final String host, final int
port,
- final boolean autoClose) throws IOException {
- final Socket underlyingSocket = delegate.createSocket(socket, host, port,
autoClose);
- return overrideProtocol(underlyingSocket);
- }
-
- @Override
- public Socket createSocket(final String host, final int port)
- throws IOException, UnknownHostException {
- final Socket underlyingSocket = delegate.createSocket(host, port);
- return overrideProtocol(underlyingSocket);
- }
-
- @Override
- public Socket createSocket(final String host, final int port, final
InetAddress localAddress,
- final int localPort) throws IOException, UnknownHostException {
- final Socket underlyingSocket = delegate.createSocket(host, port,
localAddress, localPort);
- return overrideProtocol(underlyingSocket);
- }
-
- @Override
- public Socket createSocket(final InetAddress host, final int port) throws
IOException {
- final Socket underlyingSocket = delegate.createSocket(host, port);
- return overrideProtocol(underlyingSocket);
- }
-
- @Override
- public Socket createSocket(final InetAddress host, final int port, final
InetAddress localAddress,
- final int localPort) throws IOException {
- final Socket underlyingSocket = delegate.createSocket(host, port,
localAddress, localPort);
- return overrideProtocol(underlyingSocket);
- }
-
- /**
- * Set the {@link javax.net.ssl.SSLSocket#getEnabledProtocols() enabled
protocols} to
- * {@link #enabledProtocols} if the <code>socket</code> is a {@link
SSLSocket}
- *
- * @param socket The Socket
- */
- private Socket overrideProtocol(final Socket socket) {
- if (socket instanceof SSLSocket) {
- ((SSLSocket) socket).setEnabledProtocols(enabledProtocols);
- }
- return socket;
- }
-}
diff --git
a/core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java
b/core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java
index c4a8bc11c6..27e579935b 100644
--- a/core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java
+++ b/core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java
@@ -222,26 +222,12 @@ public class SslConnectionParams {
return trustStoreType;
}
- // Work around THRIFT-3450 ... fixed with b9641e094...
- static class TSSLTransportParametersHack extends TSSLTransportParameters {
- TSSLTransportParametersHack(String clientProtocol) {
- super(clientProtocol, new String[] {});
- this.cipherSuites = null;
- }
- }
-
- public TSSLTransportParameters getTTransportParams() {
+ public TSSLTransportParameters getTSSLTransportParameters() {
if (useJsse) {
- throw new IllegalStateException("Cannot get TTransportParams for JSEE
configuration.");
- }
-
- TSSLTransportParameters params;
- if (cipherSuites != null) {
- params = new TSSLTransportParameters(clientProtocol, cipherSuites);
- } else {
- params = new TSSLTransportParametersHack(clientProtocol);
+ throw new IllegalStateException("Cannot get TSSLTransportParameters for
JSSE configuration.");
}
+ TSSLTransportParameters params = new
TSSLTransportParameters(clientProtocol, cipherSuites);
params.requireClientAuth(clientAuth);
if (keyStoreSet) {
params.setKeyStore(keyStorePath, keyStorePass, null, keyStoreType);
diff --git a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java
b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java
index a448908f75..6924b4c862 100644
--- a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java
+++ b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java
@@ -18,22 +18,14 @@
*/
package org.apache.accumulo.core.rpc;
-import java.io.FileInputStream;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.net.InetAddress;
import java.nio.channels.ClosedByInterruptException;
-import java.security.KeyStore;
import java.security.SecureRandom;
import java.util.HashMap;
import java.util.Map;
-import javax.net.ssl.KeyManagerFactory;
-import javax.net.ssl.SSLContext;
-import javax.net.ssl.SSLSocket;
-import javax.net.ssl.SSLSocketFactory;
-import javax.net.ssl.TrustManagerFactory;
-
import org.apache.accumulo.core.clientImpl.ClientContext;
import org.apache.accumulo.core.rpc.SaslConnectionParams.SaslMechanism;
import org.apache.accumulo.core.rpc.clients.ThriftClientTypes;
@@ -52,8 +44,6 @@ import org.apache.thrift.transport.TTransportFactory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-
/**
* Factory methods for creating Thrift client objects
*/
@@ -220,26 +210,10 @@ public class ThriftUtil {
transport =
TSSLTransportFactory.getClientSocket(address.getHost(),
address.getPort(), timeout);
} else {
- // JDK6's factory doesn't appear to pass the protocol onto the
Socket properly so we have
- // to do some magic to make sure that happens. Not an issue in JDK7
-
- // Taken from thrift-0.9.1 to make the SSLContext
- SSLContext sslContext = createSSLContext(sslParams);
-
- // Create the factory from it
- SSLSocketFactory sslSockFactory = sslContext.getSocketFactory();
-
- // Wrap the real factory with our own that will set the protocol on
the Socket before
- // returning it
- ProtocolOverridingSSLSocketFactory wrappingSslSockFactory =
- new ProtocolOverridingSSLSocketFactory(sslSockFactory,
- new String[] {sslParams.getClientProtocol()});
-
- // Create the TSocket from that
- transport =
- createClient(wrappingSslSockFactory, address.getHost(),
address.getPort(), timeout);
- // TSSLTransportFactory leaves transports open, so no need to open
here
+ transport = TSSLTransportFactory.getClientSocket(address.getHost(),
address.getPort(),
+ timeout, sslParams.getTSSLTransportParameters());
}
+ // TSSLTransportFactory leaves transports open, so no need to open here
transport = ThriftUtil.transportFactory().getTransport(transport);
} else if (saslParams != null) {
@@ -338,12 +312,12 @@ public class ThriftUtil {
transport = ThriftUtil.transportFactory().getTransport(transport);
}
success = true;
+ return transport;
} finally {
if (!success && transport != null) {
transport.close();
}
}
- return transport;
}
/**
@@ -400,80 +374,6 @@ public class ThriftUtil {
}
}
- /**
- * Lifted from TSSLTransportFactory in Thrift-0.9.1. The method to create a
client socket with an
- * SSLContextFactory object is not visible to us. Have to use
SslConnectionParams instead of
- * TSSLTransportParameters because no getters exist on
TSSLTransportParameters.
- *
- * @param params Parameters to use to create the SSLContext
- */
- @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN",
- justification = "code runs in same security context as user who
providing the keystore files")
- private static SSLContext createSSLContext(SslConnectionParams params)
- throws TTransportException {
- try {
- SSLContext ctx = SSLContext.getInstance(params.getClientProtocol());
- TrustManagerFactory tmf = null;
- KeyManagerFactory kmf = null;
-
- if (params.isTrustStoreSet()) {
- tmf =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
- KeyStore ts = KeyStore.getInstance(params.getTrustStoreType());
- try (FileInputStream fis = new
FileInputStream(params.getTrustStorePath())) {
- ts.load(fis, params.getTrustStorePass().toCharArray());
- }
- tmf.init(ts);
- }
-
- if (params.isKeyStoreSet()) {
- kmf =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
- KeyStore ks = KeyStore.getInstance(params.getKeyStoreType());
- try (FileInputStream fis = new
FileInputStream(params.getKeyStorePath())) {
- ks.load(fis, params.getKeyStorePass().toCharArray());
- }
- kmf.init(ks, params.getKeyStorePass().toCharArray());
- }
-
- if (params.isKeyStoreSet() && params.isTrustStoreSet()) {
- ctx.init(kmf.getKeyManagers(), tmf.getTrustManagers(), null);
- } else if (params.isKeyStoreSet()) {
- ctx.init(kmf.getKeyManagers(), null, null);
- } else {
- ctx.init(null, tmf.getTrustManagers(), null);
- }
- return ctx;
- } catch (Exception e) {
- throw new TTransportException("Error creating the transport", e);
- }
- }
-
- /**
- * Lifted from Thrift-0.9.1 because it was private. Create an SSLSocket with
the given factory,
- * host:port, and timeout.
- *
- * @param factory Factory to create the socket from
- * @param host Destination host
- * @param port Destination port
- * @param timeout Socket timeout
- */
- private static TSocket createClient(SSLSocketFactory factory, String host,
int port, int timeout)
- throws TTransportException {
- SSLSocket socket = null;
- try {
- socket = (SSLSocket) factory.createSocket(host, port);
- socket.setSoTimeout(timeout);
- return new TSocket(socket);
- } catch (Exception e) {
- try {
- if (socket != null) {
- socket.close();
- }
- } catch (IOException ioe) {}
-
- throw new TTransportException("Could not connect to " + host + " on port
" + port, e);
- }
- }
-
public static void checkIOExceptionCause(IOException e) {
if (e instanceof ClosedByInterruptException) {
Thread.currentThread().interrupt();
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
b/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
index ff139ac003..3a72250268 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java
@@ -399,7 +399,7 @@ public class TServerUtils {
TSSLTransportFactory.getServerSocket(port, timeout,
params.isClientAuth(), address);
} else {
tServerSock = TSSLTransportFactory.getServerSocket(port, timeout,
address,
- params.getTTransportParams());
+ params.getTSSLTransportParameters());
}
final ServerSocket serverSock = tServerSock.getServerSocket();
