You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by si...@apache.org on 2020/03/30 00:14:16 UTC
[bookkeeper] branch master updated: Unable to enable TLS if using
v2 protocol
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 87981d4 Unable to enable TLS if using v2 protocol
87981d4 is described below
commit 87981d420d0ae9d472d4e92b7abc145069e17e61
Author: Sijie Guo <si...@apache.org>
AuthorDate: Sun Mar 29 17:14:05 2020 -0700
Unable to enable TLS if using v2 protocol
Descriptions of the changes in this PR:
*Motivation*
TLS is enabled using `startTLS` but the `startTLS`
is a v3 protobuf request. So if the client is
configured to use v2 protocol, the client is not
able to decode `startTLSResponse`.
*Modifications*
This change is to improve the v2 protocol response handling for
handling `startTLS` response.
Reviewers: Enrico Olivelli <eo...@gmail.com>
This closes #2300 from sijie/fix_tls_v2_protocol
---
.../bookkeeper/proto/BookieProtoEncoding.java | 46 +++++++++++++++++++---
.../bookkeeper/proto/PerChannelBookieClient.java | 2 +-
.../bookkeeper/proto/BookieProtoEncodingTest.java | 2 +-
.../java/org/apache/bookkeeper/tls/TestTLS.java | 36 +++++++++++------
4 files changed, 68 insertions(+), 18 deletions(-)
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
index 151a799..0c237c4 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
@@ -41,6 +41,8 @@ import java.io.IOException;
import java.security.NoSuchAlgorithmException;
import org.apache.bookkeeper.proto.BookieProtocol.PacketHeader;
+import org.apache.bookkeeper.proto.BookkeeperProtocol.OperationType;
+import org.apache.bookkeeper.proto.BookkeeperProtocol.Response;
import org.apache.bookkeeper.proto.checksum.MacDigestManager;
import org.apache.bookkeeper.util.ByteBufList;
import org.slf4j.Logger;
@@ -479,11 +481,20 @@ public class BookieProtoEncoding {
*/
@Sharable
public static class ResponseDecoder extends ChannelInboundHandlerAdapter {
- final EnDecoder rep;
+ final EnDecoder repPreV3;
+ final EnDecoder repV3;
+ final boolean useV2Protocol;
+ final boolean tlsEnabled;
+ boolean usingV3Protocol;
- ResponseDecoder(ExtensionRegistry extensionRegistry, boolean useV2Protocol) {
- rep = useV2Protocol
- ? new ResponseEnDeCoderPreV3(extensionRegistry) : new ResponseEnDecoderV3(extensionRegistry);
+ ResponseDecoder(ExtensionRegistry extensionRegistry,
+ boolean useV2Protocol,
+ boolean tlsEnabled) {
+ this.repPreV3 = new ResponseEnDeCoderPreV3(extensionRegistry);
+ this.repV3 = new ResponseEnDecoderV3(extensionRegistry);
+ this.useV2Protocol = useV2Protocol;
+ this.tlsEnabled = tlsEnabled;
+ usingV3Protocol = true;
}
@Override
@@ -499,7 +510,32 @@ public class BookieProtoEncoding {
}
ByteBuf buffer = (ByteBuf) msg;
buffer.markReaderIndex();
- ctx.fireChannelRead(rep.decode(buffer));
+
+ Object result;
+ if (!useV2Protocol) { // always use v3 protocol
+ result = repV3.decode(buffer);
+ } else { // use v2 protocol but
+ // if TLS enabled, the first message `startTLS` is a protobuf message
+ if (tlsEnabled && usingV3Protocol) {
+ try {
+ result = repV3.decode(buffer);
+ if (result instanceof Response
+ && OperationType.START_TLS == ((Response) result).getHeader().getOperation()) {
+ usingV3Protocol = false;
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Degrade bookkeeper to v2 after starting TLS.");
+ }
+ }
+ } catch (InvalidProtocolBufferException e) {
+ usingV3Protocol = false;
+ buffer.resetReaderIndex();
+ result = repPreV3.decode(buffer);
+ }
+ } else {
+ result = repPreV3.decode(buffer);
+ }
+ }
+ ctx.fireChannelRead(result);
} finally {
ReferenceCountUtil.release(msg);
}
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 c151426..dd3d130 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
@@ -565,7 +565,7 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter {
pipeline.addLast("bookieProtoEncoder", new BookieProtoEncoding.RequestEncoder(extRegistry));
pipeline.addLast(
"bookieProtoDecoder",
- new BookieProtoEncoding.ResponseDecoder(extRegistry, useV2WireProtocol));
+ new BookieProtoEncoding.ResponseDecoder(extRegistry, useV2WireProtocol, shFactory != null));
pipeline.addLast("authHandler", new AuthHandler.ClientSideHandler(authProviderFactory, txnIdGenerator,
connectionPeer, useV2WireProtocol));
pipeline.addLast("mainhandler", PerChannelBookieClient.this);
diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieProtoEncodingTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieProtoEncodingTest.java
index a7f0c92..537c9fc 100644
--- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieProtoEncodingTest.java
+++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieProtoEncodingTest.java
@@ -91,7 +91,7 @@ public class BookieProtoEncodingTest {
ResponseEnDeCoderPreV3 v2Encoder = new ResponseEnDeCoderPreV3(registry);
ResponseEnDecoderV3 v3Encoder = new ResponseEnDecoderV3(registry);
- ResponseDecoder v3Decoder = new ResponseDecoder(registry, false);
+ ResponseDecoder v3Decoder = new ResponseDecoder(registry, false, false);
try {
v3Decoder.channelRead(ctx,
v2Encoder.encode(v2Resp, UnpooledByteBufAllocator.DEFAULT)
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 94dec72..9d00c47 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
@@ -23,6 +23,7 @@ import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
import java.io.File;
import java.io.IOException;
@@ -66,7 +67,6 @@ import org.apache.bookkeeper.util.IOUtils;
import org.apache.commons.io.FileUtils;
import org.junit.After;
import org.junit.Assert;
-import org.junit.Assume;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -93,24 +93,29 @@ public class TestTLS extends BookKeeperClusterTestCase {
private KeyStoreType clientTrustStoreFormat;
private KeyStoreType serverKeyStoreFormat;
private KeyStoreType serverTrustStoreFormat;
+ private final boolean useV2Protocol;
@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", false },
+ { "PEM", "PEM", false },
+ { "PEM", "PEM", true },
+ { "PKCS12", "PKCS12", false },
+ { "JKS", "PEM", false },
+ { "PEM", "PKCS12", false },
+ { "PKCS12", "JKS", false }
});
}
- public TestTLS(String keyStoreFormat, String trustStoreFormat) {
+ public TestTLS(String keyStoreFormat,
+ String trustStoreFormat,
+ boolean useV2Protocol) {
super(3);
this.clientKeyStoreFormat = KeyStoreType.valueOf(keyStoreFormat);
this.clientTrustStoreFormat = KeyStoreType.valueOf(trustStoreFormat);
this.serverKeyStoreFormat = KeyStoreType.valueOf(keyStoreFormat);
this.serverTrustStoreFormat = KeyStoreType.valueOf(trustStoreFormat);
+ this.useV2Protocol = useV2Protocol;
}
private String getResourcePath(String resource) throws Exception {
@@ -123,6 +128,7 @@ public class TestTLS extends BookKeeperClusterTestCase {
/* client configuration */
baseClientConf.setTLSProviderFactoryClass(TLSContextFactory.class.getName());
baseClientConf.setTLSClientAuthentication(true);
+ baseClientConf.setUseV2WireProtocol(useV2Protocol);
switch (clientKeyStoreFormat) {
case PEM:
@@ -249,7 +255,7 @@ public class TestTLS extends BookKeeperClusterTestCase {
@Test
public void testKeyMismatchFailure() throws Exception {
// Valid test case only for PEM format keys
- Assume.assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
+ assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
ClientConfiguration clientConf = new ClientConfiguration(baseClientConf);
@@ -335,6 +341,10 @@ public class TestTLS extends BookKeeperClusterTestCase {
*/
@Test
public void testConnectToLocalTLSClusterTLSClient() throws Exception {
+ // skip test
+ if (useV2Protocol) {
+ return;
+ }
ServerConfiguration serverConf = new ServerConfiguration();
for (ServerConfiguration conf : bsConfs) {
conf.setDisableServerSocketBind(true);
@@ -351,7 +361,7 @@ public class TestTLS extends BookKeeperClusterTestCase {
*/
@Test
public void testRefreshDurationForBookieCerts() throws Exception {
- Assume.assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
+ assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
ServerConfiguration serverConf = new ServerConfiguration();
String originalTlsKeyFilePath = bsConfs.get(0).getTLSKeyStore();
String invalidServerKey = getResourcePath("client-key.pem");
@@ -393,7 +403,7 @@ public class TestTLS extends BookKeeperClusterTestCase {
*/
@Test
public void testRefreshDurationForBookkeeperClientCerts() throws Exception {
- Assume.assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
+ assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
ClientConfiguration clientConf = new ClientConfiguration(baseClientConf);
String originalTlsCertFilePath = baseClientConf.getTLSCertificatePath();
@@ -601,6 +611,10 @@ public class TestTLS extends BookKeeperClusterTestCase {
*/
@Test
public void testBookieAuthPluginRequireClientTLSAuthenticationLocal() throws Exception {
+ if (useV2Protocol) {
+ return;
+ }
+
ServerConfiguration serverConf = new ServerConfiguration(baseConf);
serverConf.setBookieAuthProviderFactoryClass(AllowOnlyClientsWithX509Certificates.class.getName());
serverConf.setDisableServerSocketBind(true);