Mutual TLS support by jon-wei · Pull Request #6076 · apache/druid

jon-wei · 2018-07-31T00:34:16Z

This PR adds support for mutual TLS (https://en.wikipedia.org/wiki/Mutual_authentication).

This adds a new "requireClientAuth" property and truststore properties on the server-side TLS config.

If "requireClientAuth" is true, Druid will require clients to identify themselves by providing a TLS certificate.

The server-side truststore needs to be configured with the certificates needed to validate the client certificates.

On the client side for internal communications (simple-client-sslcontext extension), new keystore properties are added, specifying the client certificate that a Druid service will use when communicating with other Druid services.

gianm

This functionality should have a test of some sort: client cert validation is an important thing and we want to make sure it keeps working even if the code is refactored later in some way.

jon-wei · 2018-08-01T21:24:06Z

@gianm hm, I don't think we have any tests for TLS in general, I'll add some integration tests

gianm · 2018-08-02T20:50:21Z

@jon-wei, thanks, I think it is really valuable especially as we add more features to the feature.

(I heard you like features so we put features in your features?)

nishantmonu51

LGTM.

gianm · 2018-08-09T20:52:34Z

@jon-wei Are you done working on this patch? I wasn't sure since the last few commits are named "WIP"

jon-wei · 2018-08-09T20:53:32Z

Not yet, I made the integration tests use mutual TLS, but there's a test for coordinator -> overlord proxying that's failing, haven't fixed that yet

gianm · 2018-08-09T20:56:57Z

Got it, thanks.

jon-wei · 2018-08-14T22:48:39Z

I updated the patch with the following:

Added mutual TLS to the integration tests. All druid services will have both plaintext and TLS port enabled with client cert checking enabled. A new test that accesses TLS and non-TLS endpoints on each service has been added.
Adjusted the timers in the integration tests: I shortened the retry periods and increased the number of retries
Fixed some timing issues in certain integration tests (like feeding events to a worker task before it's ready)
Fixes coordinator -> overlord proxy requests when TLS is enabled (the OverlordProxyServlet was not using a http client with the proper SSLContext)

fjy · 2018-08-14T22:49:02Z

👍

gianm

Thanks @jon-wei. Please also include a couple of other features in the tests (I didn't see these, but if I missed them let me know).

Different private keys and certs for the client and server. (Can be the same root CA, and same CN/altNames, but generate two different keys, and two different keyStores.)
A negative test (getting the connection rejected in two cases: client cert missing; and client cert present, but CN/altName don't match the client hostname).

These are intended to verify that the certificate validation is being done properly by the https server.

gianm · 2018-08-17T16:29:54Z

This won't be used for anything other than verifying client certificates?

yes, the truststore on the server configuration side is only used to check the client certificates

gianm · 2018-08-17T16:30:26Z

How about requireClientCertificate?

Changed to requireClientCertificate

jon-wei · 2018-08-21T19:56:08Z

Different private keys and certs for the client and server. (Can be the same root CA, and same CN/altNames, but generate two different keys, and two different keyStores.)

The services and clients have different keys/certs, I'll update the patch to rename the client files so that this is more apparent

gianm · 2018-08-26T01:09:39Z

@jon-wei is this still a WIP or is it ready for re-review?

jon-wei · 2018-09-04T23:07:08Z

@gianm Still WIP, adjusting the networking in the integration tests for hostname verification, also looking into cert revocation handling

gianm · 2018-09-12T03:19:45Z

+        sslContextFactory.setNeedClientAuth(tlsServerConfig.isRequireClientCertificate());
+        if (tlsServerConfig.isRequireClientCertificate()) {
+          if (tlsServerConfig.getCrlPath() != null) {
+            sslContextFactory.setValidatePeerCerts(true);


This is a pretty crazy API on Jetty's part: it looks like that the only thing setValidatePeerCerts does is turn on CRL checking. Is that right? If so, the method is definitely not named intuitively. Could you add a comment about this?

I'm asking because at first glance, it looks like a bug, even if it's not.

Added a comment here

gianm · 2018-09-12T15:07:00Z

+          if (tlsServerConfig.getTrustStorePath() != null) {
+            sslContextFactory.setTrustStorePath(tlsServerConfig.getTrustStorePath());
+            sslContextFactory.setTrustStoreType(
+                tlsServerConfig.getTrustStoreType() == null ?


nit: formatting is a little strange here, and in similar statements below.

Reformatted these areas

gianm · 2018-09-12T15:08:44Z

@@ -39,28 +44,77 @@ public static SSLContext createSSLContext(
      String trustStoreType,


Looks like the trust store parameters should be marked @Nullable.

Marked the nullable params @Nullable

gianm · 2018-09-12T15:09:25Z

-      PasswordProvider trustStorePasswordProvider
+      PasswordProvider trustStorePasswordProvider,
+      String keyStoreType,
+      String keyStorePath,


Looks like the key store parameters should be marked @Nullable too. Maybe some other parameters as well?

Marked the nullable params @Nullable

gianm · 2018-09-18T20:05:45Z

 @Path("/status")
 public class StatusResource
 {
+  private static Logger log = new Logger(StatusResource.class);


Doesn't look necessary.

gianm · 2018-09-18T20:07:01Z

+      String keyStoreAlgorithm,
+      String certAlias,
+      PasswordProvider keyStorePasswordProvider,
+      PasswordProvider keyManagerFactoryPasswordProvider


This method's arg list is getting super long. It would be nicer, although not required, to use a builder instead.

Added a builder and replaced constructor calls with builder use

gianm · 2018-09-18T20:09:34Z

+|`druid.server.https.trustStorePath`|The file path or URL of the trust store containing certificates used to validate client certificates. Not needed if `requireClientCertificate` is false.|none|yes, only if `requireClientCertificate` is true|
+|`druid.server.https.trustStoreAlgorithm`|Algorithm to be used by TrustManager to validate client certificate chains. Not needed if `requireClientCertificate` is false.|`javax.net.ssl.TrustManagerFactory.getDefaultAlgorithm()`|no|
+|`druid.server.https.trustStorePassword`|The [Password Provider](../../operations/password-provider.html) or String password for the Trust Store.  Not needed if `requireClientCertificate` is false.|none|no|
+|`druid.server.https.validateHostnames`|If set to true, perform hostname validation on client certificates.  Not used if `requireClientCertificate` is false.|false|no|


What does "hostname validation" mean in this context? If it means checking CN/altNames, then it should be true by default, since otherwise it's a trap waiting to be sprung on people.

(Once you turn on druid.server.https.requireClientCertificate, we should be secure as possible by default, and you should have to explicitly set options to be less secure.)

Changed default to true and added more details to the documentation

jon-wei · 2018-09-19T04:16:18Z

Addressed comments, also adjusted pom.xml to exclude the TLS certs/keys generated by the integration tests from Rat license chcking

gianm

Thanks @jon-wei!

* Mutual TLS support * Kafka test fixes * TeamCity fix * Split integration tests * Use localhost DOCKER_IP * Increase server thread count * Increase SSL handshake timeouts * Add broken pipe retries, use injected client config params * PR comments, Rat license check exclusion

jon-wei added Feature Security labels Jul 31, 2018

himanshug approved these changes Jul 31, 2018

View reviewed changes

asdf2014 approved these changes Aug 1, 2018

View reviewed changes

gianm reviewed Aug 1, 2018

View reviewed changes

nishantmonu51 approved these changes Aug 6, 2018

View reviewed changes

jon-wei changed the title ~~Mutual TLS support~~ [WIP] Mutual TLS support Aug 9, 2018

jon-wei added the WIP label Aug 9, 2018

fjy added this to the 0.13.0 milestone Aug 13, 2018

jon-wei force-pushed the mutual_tls branch 3 times, most recently from 29d43b4 to b159728 Compare August 14, 2018 01:11

jon-wei changed the title ~~[WIP] Mutual TLS support~~ [WIP] Aug 14, 2018

jon-wei changed the title ~~[WIP]~~ Mutual TLS support Aug 14, 2018

jon-wei removed the WIP label Aug 14, 2018

jon-wei force-pushed the mutual_tls branch from 4d0b4b3 to 9818eac Compare August 14, 2018 21:11

gianm reviewed Aug 17, 2018

View reviewed changes

jon-wei force-pushed the mutual_tls branch 3 times, most recently from cc3ae32 to d1f9820 Compare August 30, 2018 23:13

Mutual TLS support

f5fd385

jon-wei force-pushed the mutual_tls branch from fc25eaf to 0874bf1 Compare September 10, 2018 19:47

Kafka test fixes

0874bf1

jon-wei closed this Sep 10, 2018

jon-wei reopened this Sep 10, 2018

jon-wei added 2 commits September 10, 2018 14:10

TeamCity fix

028e201

Split integration tests

1f485a3

jon-wei force-pushed the mutual_tls branch from 71985d6 to 1f485a3 Compare September 10, 2018 22:06

jon-wei added 3 commits September 10, 2018 16:07

Use localhost DOCKER_IP

8bc7fb1

Increase server thread count

a0dbf68

Increase SSL handshake timeouts

247ca09

jon-wei force-pushed the mutual_tls branch from 733df5e to 5795db3 Compare September 11, 2018 04:11

Add broken pipe retries, use injected client config params

e36c2a6

jon-wei force-pushed the mutual_tls branch from 5795db3 to e36c2a6 Compare September 11, 2018 04:26

clintropolis mentioned this pull request Sep 12, 2018

allow 3 retries for failing tests #6324

Merged

Merge remote-tracking branch 'upstream/master' into mutual_tls

7404f13

gianm reviewed Sep 18, 2018

View reviewed changes

jon-wei added 2 commits September 18, 2018 21:02

PR comments, Rat license check exclusion

69cdb1c

Merge remote-tracking branch 'upstream/master' into mutual_tls

eb7bff0

gianm approved these changes Sep 19, 2018

View reviewed changes

gianm merged commit 8972244 into apache:master Sep 19, 2018

dclim mentioned this pull request Oct 10, 2018

Druid 0.13.0-incubating release notes #6442

Closed

		@@ -39,28 +44,77 @@ public static SSLContext createSSLContext(
		String trustStoreType,

Conversation

jon-wei commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Aug 1, 2018

Uh oh!

gianm commented Aug 2, 2018

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

gianm commented Aug 9, 2018

Uh oh!

jon-wei commented Aug 9, 2018

Uh oh!

gianm commented Aug 9, 2018

Uh oh!

jon-wei commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjy commented Aug 14, 2018

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm commented Aug 26, 2018

Uh oh!

jon-wei commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Sep 19, 2018

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Jul 31, 2018 •

edited

Loading

jon-wei commented Aug 14, 2018 •

edited

Loading

jon-wei commented Aug 21, 2018 •

edited

Loading

jon-wei Sep 19, 2018 •

edited

Loading

jon-wei Sep 19, 2018 •

edited

Loading