Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Client] Switch to rely on SslEngine for Hostname Verification #3310

Merged
merged 1 commit into from
Jun 5, 2022
Merged

[Client] Switch to rely on SslEngine for Hostname Verification #3310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 0 additions & 35 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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