Add SSL support to JedisCluster #1550

joroda · 2017-07-18T18:26:34Z

These changes add SSL support to JedisCluster. Tests have also been added. I've tried to solve issue of not being able to get SSL ports from a Redis server by introducing a configuration setting where users can specify a mapping between the ports reported by Redis and the actual SSL ports that should be used. This approach seemed the most flexible.

marcosnils · 2017-07-18T18:46:19Z

hey @joroda this is really amazing. I'll try to review whenever I have some time. Would love if @xetorthio @HeartSaVioR or @damiri-ts could also take a look.

damiri-ts · 2017-07-22T20:36:32Z

@joroda I've been looking at the code to try to understand it. I will outline my analysis of the code here just to make sure I understand it correctly.

The client maintains a local map of standard ports to TLS ports for each node in the cluster
When the client connects to a single node in the cluster, the node returns the hosts and standard ports of all nodes in the cluster to the client
The client uses the information from the server and it local map of ports to determine the TLS port to use when connecting to the other nodes in the cluster.

It seems that one limitation of this solution, is that the client needs to know all hosts, ports, and TLS ports in the cluster ahead of time. This means that if another node is added to the cluster, the client will not be immediately aware of this.

Did I understand this correctly?

joroda · 2017-07-24T15:23:18Z

@damiri-ts Your understanding is correct. I'm not sure that there is a way to avoid the client needing to know about all ports/TLS ports ahead of time. However, instead of using a map for these ports, it may be more flexible for the user to provide a Function<int, int> that does the mapping between normal and TLS ports.

In the case that the relationship between port and TLS port was easily predictable, the user could provide something like (int p) -> p + 1000 to adapt to nodes being added without having to change code.

Please let me know what you think about this.

Edit: I've realized that defining anonymous function is something that was only added in Java 8, so that may not be the best option. Alternatively, the client could have the user provide either a Map of the ports or just a integer representing the offset between a port and its TLS port.

damiri-ts · 2017-07-24T20:43:09Z

@joroda Providing a port mapping function is a clever idea. I believe that this would be the best solution, short of modifications to the server, because it would allow clients to have immediate knowledge of additional nodes on the cluster, provided some convention for calculating TLS ports from non-TLS ports.

Thank you for your contribution.

…SL ports.

joroda · 2017-07-25T16:19:28Z

@damiri-ts Sounds great. I've just pushed a change to implement this. In order to do this in a way supported by Java 7, I've added the JedisClusterPortMap inteface instead of using Java 8's lambda function feature. Here is the interface:

public interface JedisClusterPortMap {
  int getSSLPort(int port);
}

Users will have to provide an implementation of the interface which could look something like this:

JedisClusterPortMap portMap = new JedisClusterPortMap() {
    public int getSSLPort(int port) {
      return port + 1000;
    }
};

Please let me know what you think of this.

damiri-ts · 2017-08-01T04:37:59Z

@joroda It looks good to me. I think it's the most flexible and practical solution without server-side changes.

joroda · 2017-08-07T18:06:53Z

@damiri-ts @marcosnils Let me know if this needs anything else before it can be merged.

HeartSaVioR · 2017-08-22T23:59:01Z

I'll have a look when I have some time to catch up. Sorry to visit too late.

joroda · 2017-10-12T21:45:01Z

@HeartSaVioR Just wanted to check in to see if you've had the chance to take a look at this.

HeartSaVioR

LGTM.

HeartSaVioR · 2017-11-25T22:41:58Z

@marcosnils @sazzad16 Please review this. Thanks in advance.

mp911de · 2017-12-08T22:11:24Z

Looks pretty decent, it would also make sense to provide an address mapper if the IP address is different than the one reported by CLUSTER INFO/CLUSTER SLOTS (e.g. running the SSL tunnel on a different machine).

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

There are no usage of these methods. From redis#1550, it is evident that these are not handy even for ssl operations.

…isting one to fix backwards compatibility issue

# Conflicts: # src/main/java/redis/clients/jedis/JedisCluster.java # src/main/java/redis/clients/jedis/JedisClusterConnectionHandler.java # src/main/java/redis/clients/jedis/JedisClusterInfoCache.java # src/main/java/redis/clients/jedis/JedisSlotBasedConnectionHandler.java

joroda · 2018-01-18T23:19:25Z

@sazzad16 I've added the requested changes and attempted to resolve merge conflicts.

src/main/java/redis/clients/jedis/JedisCluster.java

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

src/main/java/redis/clients/jedis/JedisSlotBasedConnectionHandler.java

src/test/java/redis/clients/jedis/tests/SSLJedisClusterTest.java

sazzad16 · 2018-01-19T02:55:40Z

src/test/java/redis/clients/jedis/tests/SSLJedisClusterTest.java

+      Assert.assertEquals(CertificateException.class, e.getCause().getCause().getCause().getClass());
+    }
+
+    if (jc != null) {


If jc != null, execution will not reach this point because of previous Assert.fail(). Put this block in finally and remove following Assert.fail(). Also, if this is only fail testing, try to hint that in method name (e.g. WithWrongSSLParameters).

I decided to go with connectByIpAddressFailsWithSSLParameters since in this case, there is not really a right SSLParameters that will work.

sazzad16 · 2018-01-19T03:04:56Z

src/test/java/redis/clients/jedis/tests/SSLJedisClusterTest.java

+      // hostname validation. When closing this Jedis object, the RedisOutputStream in the underlying 
+      // Connection object is null and causes this exception
+    }
+    if (jc2 != null) {


If jc2 != null, execution will not reach this point because of previous Assert.fail(). Put this block in finally and remove following Assert.fail().

sazzad16 · 2018-01-19T03:06:09Z

src/test/java/redis/clients/jedis/tests/SSLJedisClusterTest.java

+          InvalidAlgorithmParameterException.class, e.getCause().getCause().getCause().getCause().getClass());
+    }
+
+    if (jc != null) {


If jc != null, execution will not reach this point because of previous Assert.fail(). Put this block in finally and remove following Assert.fail().

sgopan · 2018-07-05T07:45:56Z

Any idea when these changes will be will merged. We badly need these changes.

akhiljain1986 · 2018-07-07T08:30:33Z

Is there any ETA to merge these changes ?

johnynek · 2018-07-20T18:48:41Z

Maintainers: is there any update? We are considering running our own build of jedis to get this feature. Of course, a release would be easier for us.

Is there anything blocking? It has a "ready to merge" tag, so maybe everything is ready to go?

sazzad16 · 2018-07-21T00:58:03Z

@johnynek There isn't any approximate date of release. So please use your own build for the time being. Thanks!

* Add SSL support to JedisCluster * Remove unused imports from SSLJedisClusterTest * Add JedisClusterPortMap interface for mapping between plaintext and SSL ports. * Fix casing in JedisClusterPortMap * Add new constructor for JedisClusterInfoCache instead of modifying existing one to fix backwards compatibility issue * Undo unintended formatting changes. * Improve name of test case in SSLJedisClusterTest * Fix logic for closing JedisCluster objects in SSLJedisClusterTest * Add option to specify mapping between redis and ssl hostnames * Remove unneeded public access modifiers

marcosnils · 2018-08-01T03:52:48Z

Downmerged

sazzad16 · 2018-08-01T04:08:45Z

@marcosnils Thank you for merging these PRs 👍 . I was planning to do this weekend :)

marcosnils · 2018-08-01T04:09:24Z

Np. Trying to understand why tests do not pass in travis but they do in my local machine 😕

sazzad16 · 2018-08-01T04:34:41Z

Hmm, that's odd. Because it did pass during merge request.

sazzad16 · 2018-08-01T04:35:23Z

@marcosnils Please revert this from 2.9.

marcosnils · 2018-08-01T13:30:41Z

Done.

marcosnils · 2018-08-11T18:38:44Z

I'll revert this PR because of two reasons:

Tests don't pass
There are some things in the design which I don't like and I'd like to change.

@joroda can you please open the PR again?

joroda · 2018-08-13T18:30:23Z

The tests were passing after my latest change to this PR, but that was a while ago so I wonder if other changes to the master branch since then have broken something. I'm not sure I'll have time to make additional changes, but please build off of my contribution if it is helpful. Otherwise, I will probably have time to work on this in a few months.

marcosnils · 2018-08-13T18:37:37Z

@joroda Apologize for the inconveniences and we really appreciate your effort. Would you mind re-opening the PR as-is so we can have a fresh start?. Thx!.

* Add SSL support to JedisCluster (#1550) * Add SSL support to JedisCluster * Remove unused imports from SSLJedisClusterTest * Add JedisClusterPortMap interface for mapping between plaintext and SSL ports. * Fix casing in JedisClusterPortMap * Add new constructor for JedisClusterInfoCache instead of modifying existing one to fix backwards compatibility issue * Undo unintended formatting changes. * Improve name of test case in SSLJedisClusterTest * Fix logic for closing JedisCluster objects in SSLJedisClusterTest * Add option to specify mapping between redis and ssl hostnames * Remove unneeded public access modifiers * Support for geo RO commands. * Support for geo RO commands. * Support for geo RO commands. * Change _ro to Readonly for better understanding. * Change _ro to Readonly for better understanding Update as per comments * Change _ro to Readonly for better understanding Update as per comments * Minor update. Update commands. https://redis.io/commands/georadius * Update as per comments. * Revert "Add SSL support to JedisCluster (#1550)" This reverts commit 17901ba.

joquijada · 2018-10-24T15:59:14Z

Is this solution already part of a Maven release of the jedis artifact? I see here it still lists 2.9.0, which is what I'm currently using and also facing the SSL issue while connecting to AWS Redis Cluster, which is using in-transit encryption, i.e. SSL enabled client required.

marcosnils · 2018-10-27T21:05:44Z

Nope 2.10.0 hasn't been released yet.

analytically · 2018-12-06T18:59:06Z

Doesn't seem part of 2.10.0 or 3.0.0.

…1841) * Add SSL support to JedisCluster (redis#1550) * Add SSL support to JedisCluster * Remove unused imports from SSLJedisClusterTest * Add JedisClusterPortMap interface for mapping between plaintext and SSL ports. * Fix casing in JedisClusterPortMap * Add new constructor for JedisClusterInfoCache instead of modifying existing one to fix backwards compatibility issue * Undo unintended formatting changes. * Improve name of test case in SSLJedisClusterTest * Fix logic for closing JedisCluster objects in SSLJedisClusterTest * Add option to specify mapping between redis and ssl hostnames * Remove unneeded public access modifiers * Support for geo RO commands. * Support for geo RO commands. * Support for geo RO commands. * Change _ro to Readonly for better understanding. * Change _ro to Readonly for better understanding Update as per comments * Change _ro to Readonly for better understanding Update as per comments * Minor update. Update commands. https://redis.io/commands/georadius * Update as per comments. * Revert "Add SSL support to JedisCluster (redis#1550)" This reverts commit 17901ba.

joroda added 2 commits July 14, 2017 15:24

Add SSL support to JedisCluster

0717942

Remove unused imports from SSLJedisClusterTest

db00d12

Add JedisClusterPortMap interface for mapping between plaintext and S…

a3b5c68

…SL ports.

Fix casing in JedisClusterPortMap

cce0dac

damiri-ts mentioned this pull request Aug 1, 2017

SSL/TLS Support for JedisCluster #1210

Closed

Merge branch 'master' into cluster_ssl

c6cb62f

HeartSaVioR requested review from HeartSaVioR and marcosnils October 22, 2017 10:13

HeartSaVioR added this to the 2.10.0 milestone Oct 22, 2017

HeartSaVioR added the wait for more reviews label Oct 22, 2017

HeartSaVioR approved these changes Oct 22, 2017

View reviewed changes

HeartSaVioR requested a review from sazzad16 November 25, 2017 22:41

sazzad16 requested changes Dec 9, 2017

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java Outdated Show resolved Hide resolved

sazzad16 added a commit to sazzad16/jedis that referenced this pull request Dec 9, 2017

Remove setupNodeIfNotExist for ssl

ebedba2

There are no usage of these methods. From redis#1550, it is evident that these are not handy even for ssl operations.

sazzad16 mentioned this pull request Dec 9, 2017

Remove setupNodeIfNotExist for ssl #1693

Closed

joroda added 2 commits January 18, 2018 12:44

Add new constructor for JedisClusterInfoCache instead of modifying ex…

ecd5d1d

…isting one to fix backwards compatibility issue

sazzad16 previously requested changes Jan 19, 2018

View reviewed changes

yuriyz mentioned this pull request Jul 4, 2018

Add Jedis SSL support for redis cache communication GluuFederation/oxCore#50

Open

sazzad16 added ready to merge and removed wait for more reviews labels Jul 5, 2018

marcosnils merged commit ba33d0f into redis:master Aug 1, 2018

marcosnils removed the ready to merge label Aug 1, 2018

wangxu0 mentioned this pull request Aug 13, 2018

Problem about PR #1845

Closed

joroda mentioned this pull request Aug 13, 2018

Add SSL support to JedisCluster #1849

Closed

sazzad16 mentioned this pull request Nov 20, 2018

Add SSL support to JedisCluster #1896

Merged

jusech5 mentioned this pull request Apr 3, 2020

Do JedisCluster has SSL/TLS support? #2175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSL support to JedisCluster #1550

Add SSL support to JedisCluster #1550

joroda commented Jul 18, 2017

marcosnils commented Jul 18, 2017 •

edited

damiri-ts commented Jul 22, 2017

joroda commented Jul 24, 2017 •

edited

damiri-ts commented Jul 24, 2017

joroda commented Jul 25, 2017 •

edited

damiri-ts commented Aug 1, 2017

joroda commented Aug 7, 2017

HeartSaVioR commented Aug 22, 2017

joroda commented Oct 12, 2017

HeartSaVioR left a comment

HeartSaVioR commented Nov 25, 2017

mp911de commented Dec 8, 2017

joroda commented Jan 18, 2018

sazzad16 Jan 19, 2018

joroda Jan 19, 2018

sazzad16 Jan 19, 2018

sazzad16 Jan 19, 2018

sgopan commented Jul 5, 2018

akhiljain1986 commented Jul 7, 2018

johnynek commented Jul 20, 2018

sazzad16 commented Jul 21, 2018

marcosnils commented Aug 1, 2018

sazzad16 commented Aug 1, 2018

marcosnils commented Aug 1, 2018

sazzad16 commented Aug 1, 2018

sazzad16 commented Aug 1, 2018

marcosnils commented Aug 1, 2018

marcosnils commented Aug 11, 2018 •

edited

joroda commented Aug 13, 2018

marcosnils commented Aug 13, 2018

joquijada commented Oct 24, 2018 •

edited

marcosnils commented Oct 27, 2018

analytically commented Dec 6, 2018

Add SSL support to JedisCluster #1550

Add SSL support to JedisCluster #1550

Conversation

joroda commented Jul 18, 2017

marcosnils commented Jul 18, 2017 • edited

damiri-ts commented Jul 22, 2017

joroda commented Jul 24, 2017 • edited

damiri-ts commented Jul 24, 2017

joroda commented Jul 25, 2017 • edited

damiri-ts commented Aug 1, 2017

joroda commented Aug 7, 2017

HeartSaVioR commented Aug 22, 2017

joroda commented Oct 12, 2017

HeartSaVioR left a comment

Choose a reason for hiding this comment

HeartSaVioR commented Nov 25, 2017

mp911de commented Dec 8, 2017

joroda commented Jan 18, 2018

sazzad16 Jan 19, 2018

Choose a reason for hiding this comment

joroda Jan 19, 2018

Choose a reason for hiding this comment

sazzad16 Jan 19, 2018

Choose a reason for hiding this comment

sazzad16 Jan 19, 2018

Choose a reason for hiding this comment

sgopan commented Jul 5, 2018

akhiljain1986 commented Jul 7, 2018

johnynek commented Jul 20, 2018

sazzad16 commented Jul 21, 2018

marcosnils commented Aug 1, 2018

sazzad16 commented Aug 1, 2018

marcosnils commented Aug 1, 2018

sazzad16 commented Aug 1, 2018

sazzad16 commented Aug 1, 2018

marcosnils commented Aug 1, 2018

marcosnils commented Aug 11, 2018 • edited

joroda commented Aug 13, 2018

marcosnils commented Aug 13, 2018

joquijada commented Oct 24, 2018 • edited

marcosnils commented Oct 27, 2018

analytically commented Dec 6, 2018

marcosnils commented Jul 18, 2017 •

edited

joroda commented Jul 24, 2017 •

edited

joroda commented Jul 25, 2017 •

edited

marcosnils commented Aug 11, 2018 •

edited

joquijada commented Oct 24, 2018 •

edited