Refresh Certs every X minutes #3568

jai1 · 2019-02-11T18:07:47Z

Motivation

If certs are refreshed on the file system we need to refresh the certs and recreate the TLS Channel.

Modifications

Created a class SslContextRefresher.java which uses java.nio.file.WatchEvents to detect file system changes and refresh the certs.
As soon as the java.nio.file.WatchEvents detects file system changes it will refresh the sslcontext

jai1 · 2019-02-11T20:18:11Z

rerun java8 tests

merlimat

This seems to be targeting only discovery service, while it should also be in broker, proxy and client lib

merlimat · 2019-02-13T00:18:27Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/SslContextRefresher.java

+        this.sslContext = sslContext;
+    }
+
+    public SslContext get() {


Instead of using a background thread, couldn't we just check a timestamp here to check whether the cached context is still valid?

We could even just check the stat of the file each time, without having to configure a caching time. If the file hasn't changed since last time, then reuse context.

Agreed - will make the changes accordingly.

merlimat · 2019-02-13T00:19:34Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/SslContextRefresher.java

+        for (String file : files) {
+            LOG.info("File {}", file);
+            Path p = Paths.get(file).getParent();
+            WatchService watchService = p.getFileSystem().newWatchService();


Why to do we need to poll if we're using a "watch-service"? Isn't this supposed to notify when the files are changed?

I believe Watch service doesn't have a callback, it just has a polling mechanism.
https://docs.oracle.com/javase/8/docs/api/java/nio/file/WatchService.html

merlimat

Left a comment on the caching part. Also I think the same logic should be done in client lib

merlimat · 2019-02-13T23:47:10Z

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/AuthenticationDataProvider.java

+     * 
+     * @return a Client Certificate File Path, or null if data is not available
+     */
+    default String getCertFilePath() {


Again, I don't think it's a good option to have these 2 here, since the auth data provider is not specific to TLS.

I added refresh() instead

merlimat · 2019-02-14T15:05:43Z

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/AuthenticationDataProvider.java

+     * Refresh Authentication Data.
+     * 
+     */
+    default void refresh() {


But shouldn't this be dealt internally by implementations? Why should we triggering it from outside?

For example: when reading token credential you have different ways to refresh the token data (eg: passing a Supplier<String> or by refreshing the token file).

The implementation will have more information about when it has to refresh, so it would be better to leave the decision inside there.

My point is that TLS plugin should do the caching in its own implementation, for example by checking the last modified of cert file when client lib calls getTlsCertificates()

jai1 · 2019-02-18T18:17:16Z

@merlimat and @rdhabalia - Can you review this?

rdhabalia

LGTM.. few minor comments.

rdhabalia · 2019-02-18T18:12:31Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ServiceChannelInitializer.java

        }
    }

    @Override
    protected void initChannel(SocketChannel ch) throws Exception {
-        if (serverSslCtx != null) {
-            ch.pipeline().addLast(TLS_HANDLER, serverSslCtx.newHandler(ch.alloc()));
+        if (this.enableTls) {


can we check null for serverSslCtxRefresher to avoid NPE in future.

rdhabalia · 2019-02-18T18:13:40Z

...ery-service/src/main/java/org/apache/pulsar/discovery/service/ServiceChannelInitializer.java

        }
    }

    @Override
    protected void initChannel(SocketChannel ch) throws Exception {
-        if (sslCtx != null) {
-            ch.pipeline().addLast(TLS_HANDLER, sslCtx.newHandler(ch.alloc()));
+        if (this.enableTls) {


same here.. can we add null check for sslCtxRefresher.get()

rdhabalia · 2019-02-18T18:18:13Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/FileModifiedTimeUpdater.java

+        FileTime newLastModifiedTime = updateLastModifiedTime();
+        if (newLastModifiedTime != null && !newLastModifiedTime.equals(lastModifiedTime)) {
+            this.lastModifiedTime = newLastModifiedTime;
+            ret = true;


instead having boolean var, can we just return true here and return false in the next line.

rdhabalia · 2019-02-18T18:20:24Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/FileModifiedTimeUpdater.java

+            try {
+                return Files.getLastModifiedTime(p);
+            } catch (IOException e) {
+                LOG.error("Unable to fetch lastModified time for file {}: ", fileName, e);


instead catching exception, should we log and throw up to top level and let it be handled at caller?

rdhabalia · 2019-02-18T18:23:46Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/ServerSslContextRefresher.java

+                tlsCiphers, tlsProtocols, tlsRequireTrustedClientCertOnConnect);
+    }
+
+    public SslContext get() {


do we need this method thread-safe?

jai1 · 2019-02-18T20:42:59Z

rerun integration tests

rdhabalia · 2019-02-18T21:51:53Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarChannelInitializer.java

        }
    }

    @Override
    protected void initChannel(SocketChannel ch) throws Exception {
-        if (sslCtx != null) {
-            ch.pipeline().addLast(TLS_HANDLER, sslCtx.newHandler(ch.alloc()));
+        if (this.enableTls) {


can you also add null check for sslCtxRefresher here.

rdhabalia

👍 LGTM.

jai1 added the area/client label Feb 11, 2019

jai1 self-assigned this Feb 11, 2019

jai1 requested review from maskit, rdhabalia and massakam February 11, 2019 18:07

jai1 added area/broker and removed area/client labels Feb 11, 2019

jai1 force-pushed the expiry branch from 05acedc to 1afc501 Compare February 11, 2019 18:28

merlimat reviewed Feb 11, 2019

View reviewed changes

Refresh Certs every X minutes

810e91d

jai1 force-pushed the expiry branch from 1afc501 to 810e91d Compare February 12, 2019 20:42

jai1 added 2 commits February 12, 2019 13:48

Added SSL Context Refresher to Broker.

3fdd209

Added Cert Refreshing mechanism to ProxyService.

77c31b9

merlimat reviewed Feb 13, 2019

View reviewed changes

Refactored the code to reload client certs as well.

97acba5

jai1 force-pushed the expiry branch from 466119b to 97acba5 Compare February 13, 2019 01:57

merlimat reviewed Feb 13, 2019

View reviewed changes

jai1 force-pushed the expiry branch 2 times, most recently from 810cf2e to c55ac9b Compare February 14, 2019 07:29

merlimat reviewed Feb 14, 2019

View reviewed changes

jai1 force-pushed the expiry branch from c55ac9b to ced4407 Compare February 14, 2019 18:29

Refactored and Added Test Case

77c766b

jai1 force-pushed the expiry branch from ced4407 to 77c766b Compare February 14, 2019 21:39

Removed unused constructor from AuthenticationTls.java

81a5d6d

rdhabalia reviewed Feb 18, 2019

View reviewed changes

Handled Rajan's Comments.

0931a2f

rdhabalia reviewed Feb 18, 2019

View reviewed changes

rdhabalia approved these changes Feb 18, 2019

View reviewed changes

jai1 merged commit a0f7661 into apache:master Feb 18, 2019

jai1 deleted the expiry branch February 18, 2019 22:53

merlimat mentioned this pull request Feb 22, 2019

[pulsar-broker] auto refresh new tls certs for jetty webserver #3645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh Certs every X minutes #3568

Refresh Certs every X minutes #3568

jai1 commented Feb 11, 2019

jai1 commented Feb 11, 2019

merlimat left a comment

merlimat Feb 13, 2019

merlimat Feb 13, 2019

jai1 Feb 13, 2019

merlimat Feb 13, 2019

jai1 Feb 13, 2019

merlimat left a comment

merlimat Feb 13, 2019

jai1 Feb 14, 2019

merlimat Feb 14, 2019

jai1 commented Feb 18, 2019

rdhabalia left a comment

rdhabalia Feb 18, 2019

rdhabalia Feb 18, 2019

rdhabalia Feb 18, 2019

rdhabalia Feb 18, 2019

rdhabalia Feb 18, 2019

jai1 commented Feb 18, 2019

rdhabalia Feb 18, 2019

rdhabalia left a comment

Refresh Certs every X minutes #3568

Refresh Certs every X minutes #3568

Conversation

jai1 commented Feb 11, 2019

Motivation

Modifications

jai1 commented Feb 11, 2019

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jai1 commented Feb 18, 2019

rdhabalia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jai1 commented Feb 18, 2019

Choose a reason for hiding this comment

rdhabalia left a comment

Choose a reason for hiding this comment