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 2020/01/07 20:15:25 UTC
[GitHub] [bookkeeper] rdhabalia opened a new pull request #2235:
[bookie-server] add support of client auto cert refresh
rdhabalia opened a new pull request #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235
### Motivation
as described at: https://github.com/apache/pulsar/issues/6010
Bookkeeper-client caches the tls certificates when it first tries to create a cnx with a given bookie and after that it never reloads certs even when valid certs changes on the file-system or new bookie-connection is created. Because of that as soon as client certs expires and bk-client disconnects from bookie then bk-client is not able to reconnect to bookie until we restart the bk-client process. and we see below TLS exception at bk-client.
```
19:43:03.983 [bookkeeper-io-12-45] ERROR org.apache.bookkeeper.proto.PerChannelBookieClient - Unexpected exception caught by bookie client channel handler
io.netty.handler.codec.DecoderException: javax.net.ssl.SSLHandshakeException: General OpenSslEngine problem
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:472) ~[netty-codec-4.1.31.Final.jar:4.1.31.Final]
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:278) ~[netty-codec-4.1.31.Final.jar:4.1.31.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1434) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:965) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:799) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:433) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:330) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:909) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-all-4.1.32.Final.jar:4.1.32.Final]
at java.lang.Thread.run(Thread.java:834) [?:?]
Caused by: javax.net.ssl.SSLHandshakeException: General OpenSslEngine problem
at io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$OpenSslClientCertificateCallback.handle(ReferenceCountedOpenSslClientContext.java:273) ~[netty-all-4.1.32.Final.jar
:4.1.32.Final]
at io.netty.internal.tcnative.SSL.readFromSSL(Native Method) ~[netty-tcnative-boringssl-static-2.0.20.Final.jar:2.0.20.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.readPlaintextData(ReferenceCountedOpenSslEngine.java:575) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1124) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1236) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1279) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.SslHandler$SslEngineType$1.unwrap(SslHandler.java:217) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1301) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.SslHandler.decodeNonJdkCompatible(SslHandler.java:1215) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1249) ~[netty-all-4.1.32.Final.jar:4.1.32.Final]
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:502) ~[netty-codec-4.1.31.Final.jar:4.1.31.Final]
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:441) ~[netty-codec-4.1.31.Final.jar:4.1.31.Final]
... 14 more
```
### Modification
Add support at bk-client to reload certs once they have changed on file-system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2235:
[bookie-server] add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235#discussion_r363996369
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/TLSContextFactory.java
##########
@@ -267,29 +277,33 @@ private void createClientContext()
}
sslContext = sslContextBuilder.build();
+ certLastRefreshTime = System.currentTimeMillis();
}
private void createServerContext()
throws SecurityException, KeyStoreException, NoSuchAlgorithmException, CertificateException, IOException,
UnrecoverableKeyException, InvalidKeySpecException, IllegalArgumentException {
isServerCtx = true;
- ServerConfiguration serverConf = (ServerConfiguration) config;
- tTLSCertificatePath = new FileModifiedTimeUpdater(serverConf.getTLSCertificatePath());
- tLSKeyStoreFilePath = new FileModifiedTimeUpdater(serverConf.getTLSKeyStore());
- tLSKeyStorePasswordFilePath = new FileModifiedTimeUpdater(serverConf.getTLSKeyStorePasswordPath());
- tLSTrustStoreFilePath = new FileModifiedTimeUpdater(serverConf.getTLSTrustStore());
- tLSTrustStorePasswordFilePath = new FileModifiedTimeUpdater(serverConf.getTLSTrustStorePasswordPath());
+ ServerConfiguration clientConf = (ServerConfiguration) config;
+ markAutoCertRefresh(clientConf.getTLSCertificatePath(), clientConf.getTLSKeyStore(),
+ clientConf.getTLSKeyStorePasswordPath(), clientConf.getTLSTrustStore(),
+ clientConf.getTLSTrustStorePasswordPath());
updateServerContext();
}
private synchronized SslContext getSSLContext() {
long now = System.currentTimeMillis();
- if (isServerCtx && (certRefreshTime > 0 && now > (certLastRefreshTime + certRefreshTime))) {
- if (tTLSCertificatePath.checkAndRefresh() || tLSKeyStoreFilePath.checkAndRefresh()
- || tLSKeyStorePasswordFilePath.checkAndRefresh() || tLSTrustStoreFilePath.checkAndRefresh()
- || tLSTrustStorePasswordFilePath.checkAndRefresh()) {
+ if ((certRefreshTime > 0 && now > (certLastRefreshTime + certRefreshTime))) {
+ if (tlsCertificateFilePath.checkAndRefresh() || tlsKeyStoreFilePath.checkAndRefresh()
+ || tlsKeyStorePasswordFilePath.checkAndRefresh() || tlsTrustStoreFilePath.checkAndRefresh()
+ || tlsTrustStorePasswordFilePath.checkAndRefresh()) {
try {
- updateServerContext();
+ LOG.info("Updating tls certs");
Review comment:
What about writing the path of the file?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] eolivelli commented on issue #2235: [bookie-server]
add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235#issuecomment-574037642
Sure. I will
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] eolivelli merged pull request #2235: [bookie-server]
add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] rdhabalia commented on issue #2235: [bookie-server]
add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on issue #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235#issuecomment-573980341
@eolivelli I don't have merging access. can someone please merge this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] rdhabalia commented on issue #2235: [bookie-server]
add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on issue #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235#issuecomment-572801788
run bookkeeper-server bookie tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2235:
[bookie-server] add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235#discussion_r363996687
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
##########
@@ -386,6 +389,67 @@ public void testRefreshDurationForBookieCerts() throws Exception {
newTlsKeyFile.delete();
}
+ /**
+ * Verify Bookkeeper-client refreshes certs at configured duration.
+ */
+ @Test
+ public void testRefreshDurationForBookkeeperClientCerts() throws Exception {
+ Assume.assumeTrue(serverKeyStoreFormat == KeyStoreType.PEM);
+
+ ClientConfiguration clientConf = new ClientConfiguration(baseClientConf);
+ String originalTlsCertFilePath = baseClientConf.getTLSCertificatePath();
+ String invalidClientCert = getResourcePath("server-cert.pem");
+ File originalTlsCertFile = new File(originalTlsCertFilePath);
+ File newTlsCertFile = IOUtils.createTempFileAndDeleteOnExit(originalTlsCertFilePath, "refresh");
+ // clean up temp file even if test fails
+ newTlsCertFile.deleteOnExit();
+ File invalidClientCertFile = new File(invalidClientCert);
+ // copy invalid cert to new temp file
+ FileUtils.copyFile(invalidClientCertFile, newTlsCertFile);
+ long refreshDurationInSec = 2;
+ clientConf.setTLSCertFilesRefreshDurationSeconds(1);
+ clientConf.setTLSCertificatePath(newTlsCertFile.getAbsolutePath());
+
+ // create a bookkeeper-client
+ BookKeeper client = new BookKeeper(clientConf);
Review comment:
Can we close this handle in a try-with-resources block? (Maybe fixing other cases in this file?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [bookkeeper] rdhabalia commented on issue #2235: [bookie-server]
add support of client auto cert refresh
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on issue #2235: [bookie-server] add support of client auto cert refresh
URL: https://github.com/apache/bookkeeper/pull/2235#issuecomment-572308490
@eolivelli addressed both the comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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