This is an automated email from the ASF dual-hosted git repository.
sijie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 40c8919 ISSUE #1109: Error out pending ops on TLS key mismatch
exception
40c8919 is described below
commit 40c89198af603d94815711da40698399f8f16c75
Author: Kishore Udayashankar <[email protected]>
AuthorDate: Sat Feb 17 01:04:02 2018 +0800
ISSUE #1109: Error out pending ops on TLS key mismatch exception
Descriptions of the changes in this PR:
(W-4479117)
- Add Testcase to test certificate mismatch scenario
- On tls handshake failure, replace future.get() with channel.
- Print a warning message on exception caught in a netty context
on connection close, drain pendingOps
rev charan
Master Issue: #1109
Author: Kishore Udayashankar <[email protected]>
Reviewers: Enrico Olivelli <[email protected]>, Sijie Guo
<[email protected]>
This closes #1110 from kishorekasi/tls-keymismatch, closes #1109
---
.../bookkeeper/proto/PerChannelBookieClient.java | 36 +++++++----
.../java/org/apache/bookkeeper/tls/TestTLS.java | 70 +++++++++++++++++-----
2 files changed, 80 insertions(+), 26 deletions(-)
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 a486d57..75b3193 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
@@ -938,6 +938,23 @@ public class PerChannelBookieClient extends
ChannelInboundHandlerAdapter {
}
/**
+ * Errors out pending ops from per channel bookie client. As the channel
+ * is being closed, all the operations waiting on the connection
+ * will be sent to completion with error
+ */
+ void errorOutPendingOps(int rc) {
+ Queue<GenericCallback<PerChannelBookieClient>> oldPendingOps;
+ synchronized (this) {
+ oldPendingOps = pendingOps;
+ pendingOps = new ArrayDeque<>();
+ }
+
+ for (GenericCallback<PerChannelBookieClient> pendingOp :
oldPendingOps) {
+ pendingOp.operationComplete(rc, PerChannelBookieClient.this);
+ }
+ }
+
+ /**
* Errors out pending entries. We call this method from one thread to avoid
* concurrent executions to QuorumOpMonitor (implements callbacks). It
seems
* simpler to call it from BookieHandle instead of calling directly from
@@ -978,6 +995,7 @@ public class PerChannelBookieClient extends
ChannelInboundHandlerAdapter {
}
errorOutOutstandingEntries(BKException.Code.BookieHandleNotAvailableException);
+ errorOutPendingOps(BKException.Code.BookieHandleNotAvailableException);
synchronized (this) {
if (this.channel == ctx.channel()
@@ -1014,9 +1032,7 @@ public class PerChannelBookieClient extends
ChannelInboundHandlerAdapter {
}
if (cause instanceof IOException) {
- // these are thrown when a bookie fails, logging them just pollutes
- // the logs (the failure is logged from the listeners on the write
- // operation), so I'll just ignore it here.
+ LOG.warn("Exception caught on:{} cause:", ctx.channel(), cause);
ctx.close();
return;
}
@@ -1225,7 +1241,7 @@ public class PerChannelBookieClient extends
ChannelInboundHandlerAdapter {
if (future.isSuccess() && state ==
ConnectionState.CONNECTING) {
LOG.error("Connection state changed before TLS
handshake completed {}/{}", addr, state);
rc =
BKException.Code.BookieHandleNotAvailableException;
- closeChannel(future.get());
+ closeChannel(channel);
channel = null;
if (state != ConnectionState.CLOSED) {
state = ConnectionState.DISCONNECTED;
@@ -1241,20 +1257,20 @@ public class PerChannelBookieClient extends
ChannelInboundHandlerAdapter {
} else if (future.isSuccess()
&& (state == ConnectionState.CLOSED || state
== ConnectionState.DISCONNECTED)) {
LOG.warn("Closed before TLS handshake completed,
clean up: {}, current state {}",
- future.get(), state);
- closeChannel(future.get());
+ channel, state);
+ closeChannel(channel);
rc =
BKException.Code.BookieHandleNotAvailableException;
channel = null;
} else if (future.isSuccess() && state ==
ConnectionState.CONNECTED) {
LOG.debug("Already connected with another
channel({}), so close the new channel({})",
- channel, future.get());
- closeChannel(future.get());
+ channel, channel);
+ closeChannel(channel);
return; // pendingOps should have been completed
when other channel connected
} else {
LOG.error("TLS handshake failed with bookie:
{}/{}, current state {} : ",
- future.get(), addr, state, future.cause());
+ channel, addr, state, future.cause());
rc = BKException.Code.SecurityException;
- closeChannel(future.get());
+ closeChannel(channel);
channel = null;
if (state != ConnectionState.CLOSED) {
state = ConnectionState.DISCONNECTED;
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 0f51dcf..31ea7bd 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
@@ -52,7 +52,9 @@ import org.apache.bookkeeper.proto.BookieConnectionPeer;
import org.apache.bookkeeper.proto.ClientConnectionPeer;
import org.apache.bookkeeper.proto.TestPerChannelBookieClient;
import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
+import org.apache.bookkeeper.tls.TLSContextFactory.KeyStoreType;
import org.junit.After;
+import org.junit.Assume;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -75,28 +77,28 @@ public class TestTLS extends BookKeeperClusterTestCase {
private static boolean secureBookieSideChannel = false;
private static Collection<Object> secureBookieSideChannelPrincipals = null;
- private TLSContextFactory.KeyStoreType clientKeyStoreFormat;
- private TLSContextFactory.KeyStoreType clientTrustStoreFormat;
- private TLSContextFactory.KeyStoreType serverKeyStoreFormat;
- private TLSContextFactory.KeyStoreType serverTrustStoreFormat;
+ private KeyStoreType clientKeyStoreFormat;
+ private KeyStoreType clientTrustStoreFormat;
+ private KeyStoreType serverKeyStoreFormat;
+ private KeyStoreType serverTrustStoreFormat;
@Parameters
public static Collection<Object[]> data() {
return Arrays.asList(new Object[][] {
- { "JKS", "JKS" },
- { "PEM", "PEM" },
- { "PKCS12", "PKCS12" },
- { "JKS", "PEM" },
- { "PEM", "PKCS12" },
- { "PKCS12", "JKS" }
+ { "JKS", "JKS" },
+ { "PEM", "PEM" },
+ { "PKCS12", "PKCS12" },
+ { "JKS", "PEM" },
+ { "PEM", "PKCS12" },
+ { "PKCS12", "JKS" }
});
}
public TestTLS(String keyStoreFormat, String trustStoreFormat) {
super(3);
- this.clientKeyStoreFormat =
TLSContextFactory.KeyStoreType.valueOf(keyStoreFormat);
- this.clientTrustStoreFormat =
TLSContextFactory.KeyStoreType.valueOf(trustStoreFormat);
- this.serverKeyStoreFormat =
TLSContextFactory.KeyStoreType.valueOf(keyStoreFormat);
- this.serverTrustStoreFormat =
TLSContextFactory.KeyStoreType.valueOf(trustStoreFormat);
+ this.clientKeyStoreFormat = KeyStoreType.valueOf(keyStoreFormat);
+ this.clientTrustStoreFormat = KeyStoreType.valueOf(trustStoreFormat);
+ this.serverKeyStoreFormat = KeyStoreType.valueOf(keyStoreFormat);
+ this.serverTrustStoreFormat = KeyStoreType.valueOf(trustStoreFormat);
}
private String getResourcePath(String resource) throws Exception {
@@ -217,7 +219,7 @@ public class TestTLS extends BookKeeperClusterTestCase {
/**
* Verify that a server will not start if tls is enabled but no cert is
specified.
*/
- @Test(timeout = 60000)
+ @Test
public void testStartTLSServerNoKeyStore() throws Exception {
ServerConfiguration bookieConf =
newServerConfiguration().setTLSKeyStore(null);
@@ -230,7 +232,43 @@ public class TestTLS extends BookKeeperClusterTestCase {
}
/**
- * Verify that a server will not start if tls is enabled but the cert
password is incorrect.
+ * Verify handshake failure with a bad cert.
+ */
+ @Test
+ public void testKeyMismatchFailure() throws Exception {
+ // Valid test case only for PEM format keys
+ Assume.assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
+
+ ClientConfiguration clientConf = new
ClientConfiguration(baseClientConf);
+
+ // restart a bookie with bad cert
+ int restartBookieIdx = 0;
+ ServerConfiguration bookieConf = bsConfs.get(restartBookieIdx)
+ .setTLSCertificatePath(getResourcePath("client-cert.pem"));
+ killBookie(restartBookieIdx);
+ LOG.info("Sleeping for 1s before restarting bookie with bad cert");
+ Thread.sleep(1000);
+ bs.add(startBookie(bookieConf));
+ bsConfs.add(bookieConf);
+
+ // Create ledger and write entries
+ BookKeeper client = new BookKeeper(clientConf);
+ byte[] passwd = "testPassword".getBytes();
+ int numEntries = 2;
+ byte[] testEntry = "testEntry".getBytes();
+
+ try (LedgerHandle lh = client.createLedger(numBookies, numBookies,
DigestType.CRC32, passwd)) {
+ for (int i = 0; i <= numEntries; i++) {
+ lh.addEntry(testEntry);
+ }
+ fail("Should have failed with not enough bookies to write");
+ } catch (BKException.BKNotEnoughBookiesException bke) {
+ // expected
+ }
+ }
+
+ /**
+ * Verify that a server will not start if ssl is enabled but the cert
password is incorrect.
*/
@Test
public void testStartTLSServerBadPassword() throws Exception {
--
To stop receiving notification emails like this one, please contact
[email protected].
