Skip to content

Commit

Permalink
[Client] Switch to rely on Netty for Hostname Verification (apache#3310)
Browse files Browse the repository at this point in the history 
### 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.
  • Loading branch information
@michaeljmarshall
michaeljmarshall committed Jun 5, 2022
1 parent 5f1b000 commit 6b22f85
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 53 deletions.
35 changes: 0 additions & 35 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,24 @@
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;
Expand Down Expand Up @@ -444,35 +438,6 @@ public void operationComplete(int rc, Void v) {
}
}
}

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")
Expand Down
22 changes: 7 additions & 15 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
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;
Expand Down Expand Up @@ -1488,7 +1489,8 @@ public String toString() {
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
Expand All @@ -1512,14 +1514,8 @@ public void operationComplete(Future<Channel> future) throws Exception {
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 {}",
Expand Down Expand Up @@ -2472,12 +2468,8 @@ public void operationComplete(ChannelFuture future) {
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 {}",
Expand Down
4 changes: 4 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/SecurityHandlerFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,8 @@ enum NodeType {
void init(NodeType type, AbstractConfiguration conf, ByteBufAllocator allocator) throws SecurityException;

SslHandler newTLSHandler();

default SslHandler newTLSHandler(String host, int port) {
return this.newTLSHandler();
}
}
19 changes: 18 additions & 1 deletion bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
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;
Expand Down Expand Up @@ -143,6 +144,7 @@ public String toString() {
}

private static final String TLSCONTEXT_HANDLER_NAME = "tls";
private NodeType type;
private String[] protocols;
private String[] ciphers;
private volatile SslContext sslContext;
Expand Down Expand Up @@ -475,6 +477,7 @@ public synchronized void init(NodeType type, AbstractConfiguration conf, ByteBuf
throws SecurityException {
this.allocator = allocator;
this.config = conf;
this.type = type;
final String enabledProtocols;
final String enabledCiphers;
certRefreshTime = TimeUnit.SECONDS.toMillis(conf.getTLSCertFilesRefreshDurationSeconds());
Expand Down Expand Up @@ -522,7 +525,12 @@ public synchronized void init(NodeType type, AbstractConfiguration conf, ByteBuf

@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);
Expand All @@ -538,6 +546,15 @@ public SslHandler newTLSHandler() {
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;
}

Expand Down
3 changes: 1 addition & 2 deletions bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
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;
Expand Down Expand Up @@ -1021,7 +1020,7 @@ public void testClientAuthPluginWithHostnameVerificationEnabled() throws Excepti
try {
testClient(clientConf, numBookies);
fail("should have failed with unauthorized exception");
} catch (BKUnauthorizedAccessException e) {
} catch (BKException.BKNotEnoughBookiesException e) {
// Ok.
}
}
Expand Down

0 comments on commit 6b22f85

Please sign in to comment.