You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2018/02/16 17:04:17 UTC
[GitHub] sijie closed pull request #1110: Issue 1109: Error out pending ops on TLS key mismatch exception
sijie closed pull request #1110: Issue 1109: Error out pending ops on TLS key mismatch exception
URL: https://github.com/apache/bookkeeper/pull/1110
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 a486d5716..75b319308 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
@@ -937,6 +937,23 @@ void errorOut(final CompletionKey key, final int rc) {
}
}
+ /**
+ * 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
@@ -978,6 +995,7 @@ public void channelInactive(ChannelHandlerContext ctx) throws Exception {
}
errorOutOutstandingEntries(BKException.Code.BookieHandleNotAvailableException);
+ errorOutPendingOps(BKException.Code.BookieHandleNotAvailableException);
synchronized (this) {
if (this.channel == ctx.channel()
@@ -1014,9 +1032,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
}
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 void operationComplete(Future<Channel> future) throws Exception {
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 void operationComplete(Future<Channel> future) throws Exception {
} 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 0f51dcfaa..31ea7bdf7 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.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 @@
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 void tearDown() throws Exception {
/**
* 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 void testStartTLSServerNoKeyStore() throws Exception {
}
/**
- * 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 {
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services