This is an automated email from the ASF dual-hosted git repository. nicoloboschi pushed a commit to branch ds-4.14 in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
commit 11bb11c9a52e97563fcba46434846c5fe6d4d97c Author: Michael Marshall <[email protected]> AuthorDate: Sun Jun 5 01:39:35 2022 -0500 [Client] Switch to rely on Netty for Hostname Verification (#3310) ### Motivation Currently, we initiate hostname verification for the Bookkeeper Client in the `PerChannelBookieClient` class. In order to simplify the code, I propose that we refactor the client so it relies on Netty, its SslHandler/SslEngine, and the JVM, to perform the hostname verification. When HTTPS is configured as the endpoint verification algorithm, it uses [RFC 2818](https://datatracker.ietf.org/doc/html/rfc2818) to perform hostname verification. This is defined by the Java Security Standard Algorithm Names documentation for JDK versions 8, 11, and 17. Here are the official docs: * https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html * https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html * https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html ### Changes * Rely on Netty and the SslEngine to perform hostname verification. With this change, CN matching is now deprecated, which brings the bookkeeper client in alignment with RFC 2818. * Add new method to the `SecurityHandlerFactory` interface. It is named `newTLSHandler` and takes the `host` and `port` of the remote peer when creating a new SslEngine. To ensure backwards compatibility, the default implementation will call the original method. Note that the remote host and port are only needed when a client is using them for hostname verification. (cherry picked from commit 6b22f852d31c6e325e0ac5019a34c36acc271ed3) (cherry picked from commit 315195e7c468f82f7802d05c38ac7a961a90d446) --- .../org/apache/bookkeeper/proto/AuthHandler.java | 35 ---------------------- .../bookkeeper/proto/PerChannelBookieClient.java | 22 +++++--------- .../bookkeeper/tls/SecurityHandlerFactory.java | 4 +++ .../apache/bookkeeper/tls/TLSContextFactory.java | 19 +++++++++++- .../java/org/apache/bookkeeper/tls/TestTLS.java | 3 +- 5 files changed, 30 insertions(+), 53 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java index e05846e061..f13d3a4c49 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java @@ -29,30 +29,24 @@ import io.netty.channel.ChannelDuplexHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; import io.netty.channel.ChannelPromise; -import io.netty.handler.ssl.SslHandler; import java.io.IOException; -import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicLong; -import javax.net.ssl.SSLSession; - import org.apache.bookkeeper.auth.AuthCallbacks; import org.apache.bookkeeper.auth.AuthToken; import org.apache.bookkeeper.auth.BookieAuthProvider; import org.apache.bookkeeper.auth.ClientAuthProvider; import org.apache.bookkeeper.client.BKException; import org.apache.bookkeeper.proto.BookkeeperProtocol.AuthMessage; -import org.apache.http.conn.ssl.DefaultHostnameVerifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; class AuthHandler { static final Logger LOG = LoggerFactory.getLogger(AuthHandler.class); - private static final DefaultHostnameVerifier HOSTNAME_VERIFIER = new DefaultHostnameVerifier(); static class ServerSideHandler extends ChannelInboundHandlerAdapter { volatile boolean authenticated = false; @@ -444,35 +438,6 @@ class AuthHandler { } } } - - public boolean verifyTlsHostName(Channel channel) { - SslHandler sslHandler = channel.pipeline().get(SslHandler.class); - if (sslHandler == null) { - if (LOG.isDebugEnabled()) { - LOG.debug("can't perform hostname-verification on non-ssl channel {}", channel); - } - return true; - } - SSLSession sslSession = sslHandler.engine().getSession(); - String hostname = null; - if (channel.remoteAddress() instanceof InetSocketAddress) { - hostname = ((InetSocketAddress) channel.remoteAddress()).getHostName(); - } else { - if (LOG.isDebugEnabled()) { - LOG.debug("can't get remote hostName on ssl session {}", channel); - } - return true; - } - if (LOG.isDebugEnabled()) { - LOG.debug("Verifying HostName for {}, Cipher {}, Protocols {}, on {}", hostname, - sslSession.getCipherSuite(), sslSession.getProtocol(), channel); - } - boolean verification = HOSTNAME_VERIFIER.verify(hostname, sslSession); - if (!verification) { - LOG.warn("Failed to validate hostname verification {} on {}", hostname, channel); - } - return verification; - } } @SuppressWarnings("serial") diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java index f248a40607..12209a636d 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java @@ -59,6 +59,7 @@ import io.netty.util.Recycler.Handle; import io.netty.util.concurrent.Future; import io.netty.util.concurrent.GenericFutureListener; import java.io.IOException; +import java.net.InetSocketAddress; import java.net.SocketAddress; import java.net.UnknownHostException; import java.security.cert.Certificate; @@ -1480,7 +1481,8 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter { void initTLSHandshake() { // create TLS handler PerChannelBookieClient parentObj = PerChannelBookieClient.this; - SslHandler handler = parentObj.shFactory.newTLSHandler(); + InetSocketAddress address = (InetSocketAddress) channel.remoteAddress(); + SslHandler handler = parentObj.shFactory.newTLSHandler(address.getHostName(), address.getPort()); channel.pipeline().addFirst(parentObj.shFactory.getHandlerName(), handler); handler.handshakeFuture().addListener(new GenericFutureListener<Future<Channel>>() { @Override @@ -1504,14 +1506,8 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter { state = ConnectionState.CONNECTED; AuthHandler.ClientSideHandler authHandler = future.get().pipeline() .get(AuthHandler.ClientSideHandler.class); - if (conf.getHostnameVerificationEnabled() && !authHandler.verifyTlsHostName(channel)) { - // add HostnameVerification or private classes not - // for validation - rc = BKException.Code.UnauthorizedAccessException; - } else { - authHandler.authProvider.onProtocolUpgrade(); - activeTlsChannelCounter.inc(); - } + authHandler.authProvider.onProtocolUpgrade(); + activeTlsChannelCounter.inc(); } else if (future.isSuccess() && (state == ConnectionState.CLOSED || state == ConnectionState.DISCONNECTED)) { LOG.warn("Closed before TLS handshake completed, clean up: {}, current state {}", @@ -2462,12 +2458,8 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter { state = ConnectionState.CONNECTED; AuthHandler.ClientSideHandler authHandler = future.channel().pipeline() .get(AuthHandler.ClientSideHandler.class); - if (conf.getHostnameVerificationEnabled() && !authHandler.verifyTlsHostName(channel)) { - rc = BKException.Code.UnauthorizedAccessException; - } else { - authHandler.authProvider.onProtocolUpgrade(); - activeTlsChannelCounter.inc(); - } + authHandler.authProvider.onProtocolUpgrade(); + activeTlsChannelCounter.inc(); } else if (future.isSuccess() && (state == ConnectionState.CLOSED || state == ConnectionState.DISCONNECTED)) { LOG.warn("Closed before connection completed, clean up: {}, current state {}", diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/SecurityHandlerFactory.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/SecurityHandlerFactory.java index 5b43744fd1..5d4bc3b088 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/SecurityHandlerFactory.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/SecurityHandlerFactory.java @@ -41,4 +41,8 @@ public interface SecurityHandlerFactory { void init(NodeType type, AbstractConfiguration conf, ByteBufAllocator allocator) throws SecurityException; SslHandler newTLSHandler(); + + default SslHandler newTLSHandler(String host, int port) { + return this.newTLSHandler(); + } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java index fdb2d019c3..280c4dccf1 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java @@ -46,6 +46,7 @@ import java.util.Arrays; import java.util.concurrent.TimeUnit; import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLParameters; import javax.net.ssl.TrustManagerFactory; import lombok.extern.slf4j.Slf4j; @@ -143,6 +144,7 @@ public class TLSContextFactory implements SecurityHandlerFactory { } private static final String TLSCONTEXT_HANDLER_NAME = "tls"; + private NodeType type; private String[] protocols; private String[] ciphers; private volatile SslContext sslContext; @@ -475,6 +477,7 @@ public class TLSContextFactory implements SecurityHandlerFactory { throws SecurityException { this.allocator = allocator; this.config = conf; + this.type = type; final String enabledProtocols; final String enabledCiphers; certRefreshTime = TimeUnit.SECONDS.toMillis(conf.getTLSCertFilesRefreshDurationSeconds()); @@ -522,7 +525,12 @@ public class TLSContextFactory implements SecurityHandlerFactory { @Override public SslHandler newTLSHandler() { - SslHandler sslHandler = getSSLContext().newHandler(allocator); + return this.newTLSHandler(null, -1); + } + + @Override + public SslHandler newTLSHandler(String peer, int port) { + SslHandler sslHandler = getSSLContext().newHandler(allocator, peer, port); if (protocols != null && protocols.length != 0) { sslHandler.engine().setEnabledProtocols(protocols); @@ -538,6 +546,15 @@ public class TLSContextFactory implements SecurityHandlerFactory { log.debug("Enabled cipher suites: {} ", Arrays.toString(sslHandler.engine().getEnabledCipherSuites())); } + if (type == NodeType.Client && ((ClientConfiguration) config).getHostnameVerificationEnabled()) { + SSLParameters sslParameters = sslHandler.engine().getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + sslHandler.engine().setSSLParameters(sslParameters); + if (log.isDebugEnabled()) { + log.debug("Enabled endpointIdentificationAlgorithm: HTTPS"); + } + } + return sslHandler; } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java index bc3fbcc3ba..2f398b0044 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java @@ -44,7 +44,6 @@ import org.apache.bookkeeper.auth.AuthToken; import org.apache.bookkeeper.auth.BookieAuthProvider; import org.apache.bookkeeper.auth.ClientAuthProvider; import org.apache.bookkeeper.client.BKException; -import org.apache.bookkeeper.client.BKException.BKUnauthorizedAccessException; import org.apache.bookkeeper.client.BookKeeper; import org.apache.bookkeeper.client.BookKeeper.DigestType; import org.apache.bookkeeper.client.BookKeeperAdmin; @@ -1020,7 +1019,7 @@ public class TestTLS extends BookKeeperClusterTestCase { try { testClient(clientConf, numBookies); fail("should have failed with unauthorized exception"); - } catch (BKUnauthorizedAccessException e) { + } catch (BKException.BKNotEnoughBookiesException e) { // Ok. } }
