Skip to content

Commit

Permalink
Don't allow non-secure client connections
Browse files Browse the repository at this point in the history 
(1) Introduced a new config option onlySecureClientsAllowed in
    the Bookie Configuration. Default value is considered false.
(2) If onlySecureClientsAllowed is set to True, the Bookies only
    allow the Clients to communicate over TLS. Any requests
    from the Clients without TLS enabled will be rejected
    and Client gets the Security Exception.
  • Loading branch information
@Ghatage
Ghatage committed Jun 28, 2021
1 parent 6a19e0e commit 515e652
Show file tree
Hide file tree
Showing 6 changed files with 142 additions and 5 deletions.
18 changes: 18 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,9 @@ public class ServerConfiguration extends AbstractConfiguration<ServerConfigurati
// Certificate role based authorization
protected static final String AUTHORIZED_ROLES = "authorizedRoles";

// Is true if only Secure Client Connections are allowed.
protected static final String ONLY_SECURE_CLIENTS_ALLOWED = "onlySecureClientsAllowed";

/**
* Construct a default configuration object.
*/
Expand Down Expand Up @@ -3557,4 +3560,19 @@ public ServerConfiguration setAuthorizedRoles(String roles) {
this.setProperty(AUTHORIZED_ROLES, roles);
return this;
}

/*
* True if only secured client connections are allowed.
*/
public boolean isOnlySecureClientConnectionAllowed() {
return this.getBoolean(ONLY_SECURE_CLIENTS_ALLOWED, false);
}

/**
* Enable/Disable the feature to allow only Secure Client Connections.
*/
public ServerConfiguration setOnlySecureClientConnectionAllowed(boolean onlySecureClientConnectionAllowed) {
this.setProperty(ONLY_SECURE_CLIENTS_ALLOWED, Boolean.toString(onlySecureClientConnectionAllowed));
return this;
}
}
24 changes: 23 additions & 1 deletion 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 @@ -45,6 +45,7 @@
import org.apache.bookkeeper.auth.BookieAuthProvider;
import org.apache.bookkeeper.auth.ClientAuthProvider;
import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.conf.ServerConfiguration;
import org.apache.bookkeeper.proto.BookkeeperProtocol.AuthMessage;
import org.apache.http.conn.ssl.DefaultHostnameVerifier;
import org.slf4j.Logger;
Expand All @@ -58,9 +59,12 @@ static class ServerSideHandler extends ChannelInboundHandlerAdapter {
volatile boolean authenticated = false;
final BookieAuthProvider.Factory authProviderFactory;
final BookieConnectionPeer connectionPeer;
final ServerConfiguration serverCfg;
BookieAuthProvider authProvider;

ServerSideHandler(BookieConnectionPeer connectionPeer, BookieAuthProvider.Factory authProviderFactory) {
ServerSideHandler(ServerConfiguration serverCfg, BookieConnectionPeer connectionPeer,
BookieAuthProvider.Factory authProviderFactory) {
this.serverCfg = serverCfg;
this.authProviderFactory = authProviderFactory;
this.connectionPeer = connectionPeer;
authProvider = null;
Expand Down Expand Up @@ -92,6 +96,24 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
}

if (authenticated) {
if (msg instanceof BookkeeperProtocol.Request) {
Channel c = ctx.channel();
BookkeeperProtocol.Request req = (BookkeeperProtocol.Request) msg;
// If onlySecureClientsAllowed is set to True, Allow operations from only the Secure Clients.
// The Secure Clients have the tls handler installed in their Channel Pipeline.
if (serverCfg.isOnlySecureClientConnectionAllowed()
&& req.getHeader().getOperation() != BookkeeperProtocol.OperationType.START_TLS
&& c.pipeline().get(SslHandler.class) == null) {
LOG.error("Request from Non-secure connection is not allowed",
req.getHeader().getOperation());
// We return Authentication Failure(EAUTH) error to Client
BookkeeperProtocol.Response.Builder builder = BookkeeperProtocol.Response.newBuilder()
.setHeader(req.getHeader())
.setStatus(BookkeeperProtocol.StatusCode.EUA);
ctx.channel().writeAndFlush(builder.build());
return;
}
}
super.channelRead(ctx, msg);
} else if (msg instanceof BookieProtocol.AuthRequest) { // pre-PB-client
BookieProtocol.AuthRequest req = (BookieProtocol.AuthRequest) msg;
Expand Down
4 changes: 2 additions & 2 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieNettyServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ protected void initChannel(SocketChannel ch) throws Exception {

pipeline.addLast("bookieProtoDecoder", new BookieProtoEncoding.RequestDecoder(registry));
pipeline.addLast("bookieProtoEncoder", new BookieProtoEncoding.ResponseEncoder(registry));
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(conf,
contextHandler.getConnectionPeer(), authProviderFactory));

ChannelInboundHandler requestHandler = isRunning.get()
Expand Down Expand Up @@ -398,7 +398,7 @@ protected void initChannel(LocalChannel ch) throws Exception {

pipeline.addLast("bookieProtoDecoder", new BookieProtoEncoding.RequestDecoder(registry));
pipeline.addLast("bookieProtoEncoder", new BookieProtoEncoding.ResponseEncoder(registry));
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(conf,
contextHandler.getConnectionPeer(), authProviderFactory));

ChannelInboundHandler requestHandler = isRunning.get()
Expand Down
6 changes: 6 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,12 @@ public void processRequest(Object msg, Channel c) {
MDC.clear();
}
} else {
if (serverCfg.isOnlySecureClientConnectionAllowed()) {
LOG.error("Client request of an older protocol version " + BookieProtocol.CURRENT_PROTOCOL_VERSION
+ " denied because server config option \"onlySecureClientsAllowed=true\" and this version "
+ "of the protocol does not support TLS.");
return;
}
BookieProtocol.Request r = (BookieProtocol.Request) msg;
// process packet
switch (r.getOpCode()) {
Expand Down
91 changes: 89 additions & 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 @@ -54,16 +54,20 @@
import org.apache.bookkeeper.conf.ClientConfiguration;
import org.apache.bookkeeper.conf.ServerConfiguration;
import org.apache.bookkeeper.net.BookieId;
import org.apache.bookkeeper.net.BookieSocketAddress;
import org.apache.bookkeeper.proto.BookieConnectionPeer;
import org.apache.bookkeeper.proto.BookieServer;
import org.apache.bookkeeper.proto.ClientConnectionPeer;
import org.apache.bookkeeper.proto.TestPerChannelBookieClient;
import org.apache.bookkeeper.server.conf.BookieConfiguration;
import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
import org.apache.bookkeeper.test.TestStatsProvider;
import org.apache.bookkeeper.tls.TLSContextFactory.KeyStoreType;
import org.apache.bookkeeper.util.IOUtils;
import org.apache.bookkeeper.util.PortManager;
import org.apache.bookkeeper.util.TestUtils;
import org.apache.commons.io.FileUtils;
import org.eclipse.jetty.server.Server;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
Expand Down Expand Up @@ -498,9 +502,11 @@ public void testConnectToTLSClusterTLSClientWithAuthentication() throws Exceptio

/**
* Verify that a client without tls enabled can connect to a cluster with TLS.
* In the Default Bookie config, ONLY_SECURE_CLIENTS_ALLOWED is set to false.
* Therefore, the Bookie allows non-secure client connection.
*/
@Test
public void testConnectToTLSClusterNonTLSClient() throws Exception {
public void testConnectToTLSClusterNonTLSClient1() throws Exception {
ClientConfiguration conf = new ClientConfiguration(baseClientConf);
conf.setTLSProviderFactoryClass(null);
try {
Expand All @@ -510,6 +516,87 @@ public void testConnectToTLSClusterNonTLSClient() throws Exception {
}
}

/**
* Verify that a client without tls enabled can NOT connect to a cluster with TLS,
* if ONLY_SECURE_CLIENTS_ALLOWED is set in the Bookie config.
*/
@Test
public void testConnectToTLSClusterNonTLSClient2() throws Exception {

// Set client without TLS
ClientConfiguration conf = new ClientConfiguration(baseClientConf);
conf.setTLSProviderFactoryClass(null);
try {
// Enable the feature to only allow secure clients.
restartBookies(c -> {
c.setOnlySecureClientConnectionAllowed(true);
return c;
});
testClient(conf, numBookies);
fail("non tls client should not be able to connect to tls enabled bookies");
} catch (BKException.BKNotEnoughBookiesException nnbe) {
// Correct response.
} catch (BKException.BKUnauthorizedAccessException ue) {
// Correct response.
}
}

/**
* Verify that the Read from the non-Secure Client throws BKSecurityException
* if ONLY_SECURE_CLIENTS_ALLOWED is set in the Bookie config.
*/
@Test
public void testReadwithNonTLSBookie() throws Exception {

/* TestTLS tests for V3 and V2 protocols together
* We should be able to create a ledger when its v3 and setOnlySecureClientConnectionAllowed(true)
* We shouldn't be able to do any create / read when its v2 and setOnlySecureClientConnectionAllowed(true)
* since v2 does not support TLS.
*/
if (useV2Protocol) {
return;
}

// Enable the feature to only allow secure clients.
ClientConfiguration conf = new ClientConfiguration(baseClientConf);
long lid = 0;
try {
restartBookies(c -> {
c.setOnlySecureClientConnectionAllowed(true);
return c;
});
// Create the Ledger
BookKeeper client = new BookKeeper(conf);
byte[] passwd = "testPassword".getBytes();
int numEntries = 100;
byte[] testEntry = "testEntry".getBytes();
try (LedgerHandle lh = client.createLedger(numBookies, numBookies, DigestType.CRC32, passwd);) {
for (int i = 0; i <= numEntries; i++) {
lh.addEntry(testEntry);
}
lid = lh.getId();
}
} catch (BKException.BKNotEnoughBookiesException nnbe) {
fail("tls client could not create the ledger");
}
// nonTLS client should not be able to read the ledger
conf.setTLSProviderFactoryClass(null);
try {
BookKeeper client = new BookKeeper(conf);
byte[] passwd = "testPassword".getBytes();
int numEntries = 100;
byte[] testEntry = "testEntry".getBytes();
try (LedgerHandle lh = client.openLedger(lid, DigestType.CRC32, passwd);) {
Enumeration<LedgerEntry> entries = lh.readEntries(0, numEntries);
fail("The Read should've failed with BKSecurityException");
}
} catch (BKException.BKSecurityException se) {
// Correct Response.
} catch (BKException.BKUnauthorizedAccessException ue) {
// Correct response.
}
}

/**
* Verify that a client will fail to connect to a server if it has asked for TLS, but it is not available.
*/
Expand All @@ -534,7 +621,7 @@ public void testClientWantsTLSNoServersHaveIt() throws Exception {
* bookies with TLS enabled in the cluster, although few bookies do not have TLS enabled.
*/
@Test
public void testTLSClientButOnlyFewTLSServers() throws Exception {
public void testTLSClientButOnlyFewTLSServers1() throws Exception {
// disable TLS on initial set of bookies
restartBookies(c -> {
c.setTLSProviderFactoryClass(null);
Expand Down
4 changes: 4 additions & 0 deletions conf/bk_server.conf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,10 @@ httpServerClass=org.apache.bookkeeper.http.vertx.VertxHttpServer
# Tls certificate files refresh duration in seconds.
# tlsCertFilesRefreshDurationSeconds=0

# Set true if only Secure Client Connection is allowed
# onlySecureClientsAllowed=


############################################## Bookie Storage ##############################################


Expand Down

0 comments on commit 515e652

Please sign in to comment.