Skip to content

Commit 0d7e7f5

Browse files
GhatageGhatage
committed
Don't allow non-secure client connections
(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.
1 parent 6a19e0e commit 0d7e7f5

File tree

6 files changed

+138
-5
lines changed

6 files changed

+138
-5
lines changed

‎bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,9 @@ public class ServerConfiguration extends AbstractConfiguration<ServerConfigurati
302302
// Certificate role based authorization
303303
protected static final String AUTHORIZED_ROLES = "authorizedRoles";
304304

305+
// Is true if only Secure Client Connections are allowed.
306+
protected static final String ONLY_SECURE_CLIENTS_ALLOWED = "onlySecureClientsAllowed";
307+
305308
/**
306309
* Construct a default configuration object.
307310
*/
@@ -3557,4 +3560,19 @@ public ServerConfiguration setAuthorizedRoles(String roles) {
35573560
this.setProperty(AUTHORIZED_ROLES, roles);
35583561
return this;
35593562
}
3563+
3564+
/*
3565+
* True if only secured client connections are allowed.
3566+
*/
3567+
public boolean isOnlySecureClientConnectionAllowed() {
3568+
return this.getBoolean(ONLY_SECURE_CLIENTS_ALLOWED, false);
3569+
}
3570+
3571+
/**
3572+
* Enable/Disable the feature to allow only Secure Client Connections.
3573+
*/
3574+
public ServerConfiguration setOnlySecureClientConnectionAllowed(boolean onlySecureClientConnectionAllowed) {
3575+
this.setProperty(ONLY_SECURE_CLIENTS_ALLOWED, Boolean.toString(onlySecureClientConnectionAllowed));
3576+
return this;
3577+
}
35603578
}

‎bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import org.apache.bookkeeper.auth.BookieAuthProvider;
4646
import org.apache.bookkeeper.auth.ClientAuthProvider;
4747
import org.apache.bookkeeper.client.BKException;
48+
import org.apache.bookkeeper.conf.ServerConfiguration;
4849
import org.apache.bookkeeper.proto.BookkeeperProtocol.AuthMessage;
4950
import org.apache.http.conn.ssl.DefaultHostnameVerifier;
5051
import org.slf4j.Logger;
@@ -58,9 +59,12 @@ static class ServerSideHandler extends ChannelInboundHandlerAdapter {
5859
volatile boolean authenticated = false;
5960
final BookieAuthProvider.Factory authProviderFactory;
6061
final BookieConnectionPeer connectionPeer;
62+
final ServerConfiguration serverCfg;
6163
BookieAuthProvider authProvider;
6264

63-
ServerSideHandler(BookieConnectionPeer connectionPeer, BookieAuthProvider.Factory authProviderFactory) {
65+
ServerSideHandler(ServerConfiguration serverCfg, BookieConnectionPeer connectionPeer,
66+
BookieAuthProvider.Factory authProviderFactory) {
67+
this.serverCfg = serverCfg;
6468
this.authProviderFactory = authProviderFactory;
6569
this.connectionPeer = connectionPeer;
6670
authProvider = null;
@@ -92,6 +96,24 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
9296
}
9397

9498
if (authenticated) {
99+
if (msg instanceof BookkeeperProtocol.Request) {
100+
Channel c = ctx.channel();
101+
BookkeeperProtocol.Request req = (BookkeeperProtocol.Request) msg;
102+
// If onlySecureClientsAllowed is set to True, Allow operations from only the Secure Clients.
103+
// The Secure Clients have the tls handler installed in their Channel Pipeline.
104+
if (serverCfg.isOnlySecureClientConnectionAllowed()
105+
&& req.getHeader().getOperation() != BookkeeperProtocol.OperationType.START_TLS
106+
&& c.pipeline().get(SslHandler.class) == null) {
107+
LOG.error("Request from Non-secure connection is not allowed",
108+
req.getHeader().getOperation());
109+
// We return Authentication Failure(EAUTH) error to Client
110+
BookkeeperProtocol.Response.Builder builder = BookkeeperProtocol.Response.newBuilder()
111+
.setHeader(req.getHeader())
112+
.setStatus(BookkeeperProtocol.StatusCode.EUA);
113+
ctx.channel().writeAndFlush(builder.build());
114+
return;
115+
}
116+
}
95117
super.channelRead(ctx, msg);
96118
} else if (msg instanceof BookieProtocol.AuthRequest) { // pre-PB-client
97119
BookieProtocol.AuthRequest req = (BookieProtocol.AuthRequest) msg;

‎bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieNettyServer.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ protected void initChannel(SocketChannel ch) throws Exception {
337337

338338
pipeline.addLast("bookieProtoDecoder", new BookieProtoEncoding.RequestDecoder(registry));
339339
pipeline.addLast("bookieProtoEncoder", new BookieProtoEncoding.ResponseEncoder(registry));
340-
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(
340+
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(conf,
341341
contextHandler.getConnectionPeer(), authProviderFactory));
342342

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

399399
pipeline.addLast("bookieProtoDecoder", new BookieProtoEncoding.RequestDecoder(registry));
400400
pipeline.addLast("bookieProtoEncoder", new BookieProtoEncoding.ResponseEncoder(registry));
401-
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(
401+
pipeline.addLast("bookieAuthHandler", new AuthHandler.ServerSideHandler(conf,
402402
contextHandler.getConnectionPeer(), authProviderFactory));
403403

404404
ChannelInboundHandler requestHandler = isRunning.get()

‎bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,12 @@ public void processRequest(Object msg, Channel c) {
360360
MDC.clear();
361361
}
362362
} else {
363+
if (serverCfg.isOnlySecureClientConnectionAllowed()) {
364+
LOG.error("Client request of an older protocol version " + BookieProtocol.CURRENT_PROTOCOL_VERSION
365+
+ " denied because server config option \"onlySecureClientsAllowed=true\" and this version "
366+
+ "of the protocol does not support TLS.");
367+
return;
368+
}
363369
BookieProtocol.Request r = (BookieProtocol.Request) msg;
364370
// process packet
365371
switch (r.getOpCode()) {

‎bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -498,9 +498,11 @@ public void testConnectToTLSClusterTLSClientWithAuthentication() throws Exceptio
498498

499499
/**
500500
* Verify that a client without tls enabled can connect to a cluster with TLS.
501+
* In the Default Bookie config, ONLY_SECURE_CLIENTS_ALLOWED is set to false.
502+
* Therefore, the Bookie allows non-secure client connection.
501503
*/
502504
@Test
503-
public void testConnectToTLSClusterNonTLSClient() throws Exception {
505+
public void testConnectToTLSClusterNonTLSClient1() throws Exception {
504506
ClientConfiguration conf = new ClientConfiguration(baseClientConf);
505507
conf.setTLSProviderFactoryClass(null);
506508
try {
@@ -510,6 +512,87 @@ public void testConnectToTLSClusterNonTLSClient() throws Exception {
510512
}
511513
}
512514

515+
/**
516+
* Verify that a client without tls enabled can NOT connect to a cluster with TLS,
517+
* if ONLY_SECURE_CLIENTS_ALLOWED is set in the Bookie config.
518+
*/
519+
@Test
520+
public void testConnectToTLSClusterNonTLSClient2() throws Exception {
521+
522+
// Set client without TLS
523+
ClientConfiguration conf = new ClientConfiguration(baseClientConf);
524+
conf.setTLSProviderFactoryClass(null);
525+
try {
526+
// Enable the feature to only allow secure clients.
527+
restartBookies(c -> {
528+
c.setOnlySecureClientConnectionAllowed(true);
529+
return c;
530+
});
531+
testClient(conf, numBookies);
532+
fail("non tls client should not be able to connect to tls enabled bookies");
533+
} catch (BKException.BKNotEnoughBookiesException nnbe) {
534+
// Correct response.
535+
} catch (BKException.BKUnauthorizedAccessException ue) {
536+
// Correct response.
537+
}
538+
}
539+
540+
/**
541+
* Verify that the Read from the non-Secure Client throws BKSecurityException
542+
* if ONLY_SECURE_CLIENTS_ALLOWED is set in the Bookie config.
543+
*/
544+
@Test
545+
public void testReadwithNonTLSBookie() throws Exception {
546+
547+
/* TestTLS tests for V3 and V2 protocols together
548+
* We should be able to create a ledger when its v3 and setOnlySecureClientConnectionAllowed(true)
549+
* We shouldn't be able to do any create / read when its v2 and setOnlySecureClientConnectionAllowed(true)
550+
* since v2 does not support TLS.
551+
*/
552+
if (useV2Protocol) {
553+
return;
554+
}
555+
556+
// Enable the feature to only allow secure clients.
557+
ClientConfiguration conf = new ClientConfiguration(baseClientConf);
558+
long lid = 0;
559+
try {
560+
restartBookies(c -> {
561+
c.setOnlySecureClientConnectionAllowed(true);
562+
return c;
563+
});
564+
// Create the Ledger
565+
BookKeeper client = new BookKeeper(conf);
566+
byte[] passwd = "testPassword".getBytes();
567+
int numEntries = 100;
568+
byte[] testEntry = "testEntry".getBytes();
569+
try (LedgerHandle lh = client.createLedger(numBookies, numBookies, DigestType.CRC32, passwd);) {
570+
for (int i = 0; i <= numEntries; i++) {
571+
lh.addEntry(testEntry);
572+
}
573+
lid = lh.getId();
574+
}
575+
} catch (BKException.BKNotEnoughBookiesException nnbe) {
576+
fail("tls client could not create the ledger");
577+
}
578+
// nonTLS client should not be able to read the ledger
579+
conf.setTLSProviderFactoryClass(null);
580+
try {
581+
BookKeeper client = new BookKeeper(conf);
582+
byte[] passwd = "testPassword".getBytes();
583+
int numEntries = 100;
584+
byte[] testEntry = "testEntry".getBytes();
585+
try (LedgerHandle lh = client.openLedger(lid, DigestType.CRC32, passwd);) {
586+
Enumeration<LedgerEntry> entries = lh.readEntries(0, numEntries);
587+
fail("The Read should've failed with BKSecurityException");
588+
}
589+
} catch (BKException.BKSecurityException se) {
590+
// Correct Response.
591+
} catch (BKException.BKUnauthorizedAccessException ue) {
592+
// Correct response.
593+
}
594+
}
595+
513596
/**
514597
* Verify that a client will fail to connect to a server if it has asked for TLS, but it is not available.
515598
*/
@@ -534,7 +617,7 @@ public void testClientWantsTLSNoServersHaveIt() throws Exception {
534617
* bookies with TLS enabled in the cluster, although few bookies do not have TLS enabled.
535618
*/
536619
@Test
537-
public void testTLSClientButOnlyFewTLSServers() throws Exception {
620+
public void testTLSClientButOnlyFewTLSServers1() throws Exception {
538621
// disable TLS on initial set of bookies
539622
restartBookies(c -> {
540623
c.setTLSProviderFactoryClass(null);

‎conf/bk_server.conf

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,10 @@ httpServerClass=org.apache.bookkeeper.http.vertx.VertxHttpServer
278278
# Tls certificate files refresh duration in seconds.
279279
# tlsCertFilesRefreshDurationSeconds=0
280280

281+
# Set true if only Secure Client Connection is allowed
282+
# onlySecureClientsAllowed=
283+
284+
281285
############################################## Bookie Storage ##############################################
282286

283287

0 commit comments

Comments
 (0)