GEODE-9542: Enable SSL client authentication for Radish by jdeppe-pivotal · Pull Request #6826 · apache/geode

jdeppe-pivotal · 2021-09-01T15:45:14Z

When Geode's ssl-require-authentication is enabled, Redis clients must
authenticate with a valid certificate.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

- When Geode's ssl-require-authentication is enabled, Redis clients must authenticate with a valid certificate.

DonalEvans · 2021-09-01T16:03:53Z

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java

+            FileWatchingX509ExtendedTrustManager.newFileWatchingTrustManager(sslConfigForServer));
+      }
+
+      SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);


The javadoc for SslContextBuilder.forServer() states that the keyManagerFactory must be non-null, but it's possible that we get to this point with a null keyManagerFactory if sslConfigForServer.getKeystore() returns null. Is it actually possible for sslConfigForServer.getKeystore() to return null? If not, that check should probably be removed.

Good catch. I cannot think why no keystore would be provided and it seems like a misconfiguration. You need a keystore if you want to use SSL/TLS. To that end I think it's OK to throw an exception in that case. WDYT?

DonalEvans · 2021-09-01T17:49:24Z

...apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/SSLDUnitTest.java

+      assertThatThrownBy(jedis::ping).satisfiesAnyOf(
+          e -> assertThat(e.getMessage()).contains("SSLException"),
+          e -> assertThat(e.getMessage()).contains("SSLHandshakeException"));


These assertions could be combined to a single one:

assertThatThrownBy(jedis::ping).isInstanceOf(SSLException.class);

Since SSLHandshakeException extends SSLException. Alternately, you could use satisfiesAnyOf() with assertions on the message body rather than exception name, but this might be trickier if you don't know all the different messages that might be expected in exceptions here.

I've removed this block since exceptions are now just coming from the createClient call.

And... now I've added it back since it seems that the createClient can also sometime fail (I have a feeling it's platform or JDK specific). But if the client is created, then this ping will definitely fail.

The top level exception is always JedisConnectionException and the ones here are somewhere in the stack but not even the root cause. That's why I'm just looking at the message since that always contains one of these. I also have a strong suspicion that different plaforms and/or JDK produce slightly different messages or exception stacks. This seems to be one way to cover all bases.

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java

DonalEvans · 2021-09-02T18:07:53Z

...apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/SSLDUnitTest.java

+  }
+
+  @Test
+  public void givenMutualAuthentication_clientErrorsWithoutKeystore() throws Exception {


I may just be misunderstanding, but this test name implies mutual authentication, but the boolean mutualAuthentication argument passed to the createClient() method is false.

So the givenMutualAuthentication bit is supposed to refer to how the server is configured. Passing false to createClient in order to not set it up to perform mutual auth and thus error. Feel free to suggest some more expressive test method name or perhaps rename the parameters.

Maybe the test name could be "givenMutualAuthenticationOnServer..."? Not super important though.

...apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/SSLDUnitTest.java

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java

upthewaterspout

Overall, looks great! Looks like you also added support for watching the keystore and truststore files.

A few comments, inline.

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java

...apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/SSLDUnitTest.java

upthewaterspout · 2021-09-08T17:58:53Z

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java

+
+      if (!sslConfigForServer.isAnyProtocols()) {
+        sslContextBuilder.protocols(
+            Arrays.asList(sslConfigForServer.getProtocolsAsStringArray()));


I wonder if we should add some tests that our redis server is honoring the ciphers and protocols that the user is specifying.

I started down that road but I'm not sure how to best go about it. I started playing with some code I found that probes the port and tries to set various SSL options, but it seems a bit fragile since the JDKs are always adjusting their supported security. Any other ideas?

I was able to verify manually, but that doesn't help.

If it helps, I see lots of other tests checked in that are picking explicit protocols - eg RestAPIsWithSSLDUnitTest.

I wonder if netty's use of the protocol list exactly matches ours, for example. We allow "any" to show up in the list, and we also order the list with the highest priority protocol first.

So I added some test code, that I found, that probes a SSL-enabled server and informs what protocols, ciphers and certificates the server has. Originally it was just a standalone-app so I massaged it a bit for this use.

Ok. I also be fine with a test that just ensures the client can't connect with mismatched protocols, etc. to see that setting the protocol works. But if you like this TestSslServer approach better I'm ok with that.

upthewaterspout · 2021-09-13T18:04:50Z

...atible-with-redis/src/commonTest/java/org/apache/geode/redis/internal/ssl/TestSSLServer.java

@@ -0,0 +1,1505 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license


I don't think this file should have the apache license header, it should probably just have the original MIT license header.

upthewaterspout · 2021-09-13T18:07:08Z

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java

+
+      if (!sslConfigForServer.isAnyProtocols()) {
+        sslContextBuilder.protocols(
+            Arrays.asList(sslConfigForServer.getProtocolsAsStringArray()));


Ok. I also be fine with a test that just ensures the client can't connect with mismatched protocols, etc. to see that setting the protocol works. But if you like this TestSslServer approach better I'm ok with that.

GEODE-9542: Enable client authentication for Radish

0162c75

- When Geode's ssl-require-authentication is enabled, Redis clients must authenticate with a valid certificate.

jdeppe-pivotal requested review from DonalEvans, dschneider-pivotal, nonbinaryprogrammer, ringles and upthewaterspout as code owners September 1, 2021 15:45

jdeppe-pivotal added the redis Issues related to the geode-for-redis module label Sep 1, 2021

jdeppe-pivotal changed the title ~~GEODE-9542: Enable client authentication for Radish~~ GEODE-9542: Enable SSL client authentication for Radish Sep 1, 2021

Address inconsistent SSL error messages

8100790

DonalEvans reviewed Sep 1, 2021

View reviewed changes

Add tests for cert rotation

f934afa

jdeppe-pivotal requested review from Bill and echobravopapa as code owners September 1, 2021 20:58

jdeppe-pivotal marked this pull request as draft September 1, 2021 21:01

Review update - throw exception if no keystore is provided

2d454d8

jdeppe-pivotal marked this pull request as ready for review September 1, 2021 21:33

DonalEvans reviewed Sep 1, 2021

View reviewed changes

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java Outdated Show resolved Hide resolved

jdeppe-pivotal added 2 commits September 1, 2021 19:24

Chasing the Heisenbug

0eb2d49

More exception checking

f87ae2b

echobravopapa approved these changes Sep 2, 2021

View reviewed changes

DonalEvans reviewed Sep 2, 2021

View reviewed changes

...apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/SSLDUnitTest.java Outdated Show resolved Hide resolved

dschneider-pivotal requested changes Sep 2, 2021

View reviewed changes

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java Outdated Show resolved Hide resolved

...patible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java Outdated Show resolved Hide resolved

jdeppe-pivotal added 2 commits September 2, 2021 16:11

Review updates

204de9f

Account for JDK 11 exceptions

c9ec459

jdeppe-pivotal requested a review from kirklund as a code owner September 3, 2021 03:15

jdeppe-pivotal added 2 commits September 2, 2021 21:08

Handle reference to internal SunCertPathBuilderException

d662547

More of the same

6e2174b

DonalEvans approved these changes Sep 3, 2021

View reviewed changes

jdeppe-pivotal requested a review from dschneider-pivotal September 7, 2021 15:35

dschneider-pivotal approved these changes Sep 8, 2021

View reviewed changes

upthewaterspout reviewed Sep 8, 2021

View reviewed changes

Review updates

378cfe5

jdeppe-pivotal requested a review from upthewaterspout September 10, 2021 14:03

jdeppe-pivotal and others added 2 commits September 11, 2021 06:13

Add TTestSSLServer

ebbd645

Allow TestSSLServer to be called from tests

56d2d54

upthewaterspout reviewed Sep 13, 2021

View reviewed changes

Fix license header

6e05467

jdeppe-pivotal merged commit 81153cf into apache:develop Sep 14, 2021

		@@ -0,0 +1,1505 @@
		/*
		* Licensed to the Apache Software Foundation (ASF) under one or more contributor license

Conversation

jdeppe-pivotal commented Sep 1, 2021

For all changes:

Note:

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeppe-pivotal Sep 2, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

upthewaterspout left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jdeppe-pivotal Sep 2, 2021 •

edited

Loading