Add SSL context for RedisCluster connections

[robin-brabants]

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	no

Description

This small feature allows to pass an sslContext to the RedisCluster constructor from the phpredis extension.

Without this feature, one could not connect to a cluster using TLS. Now when specifying the correct protocol for the seed nodes (e.g.: 'seeds' => ['tls://nodeId-01.xxx:6379', 'tls://nodeId-02.xxx:6379']) and passing an sslContext one can connect to a cluster using TLS .

One can pass an empty array for the sslContext, but also explicitly specify non-default options for the TLS connection. For a full list of ssl context options see: https://www.php.net/manual/en/context.ssl.php.

This for example enables one to connect to an AWS elasticache redis cluster with in-transit encryption set to required due to security reasons.

[Ocramius]

TARGET THE develop BRANCH????

Is the template still foobar'd ?

[Ocramius]

I understand where this comes from, but the use case of verify_peer => false seems kinda anachronistic nowadays.

Possibly better to provision both host and client with a dummy certificate via https://github.com/FiloSottile/mkcert/blob/2a46726cebac0ff4e1f133d90b4e4c42f1edf44a/README.md ?

[robin-brabants]

TARGET THE develop BRANCH????

Is the template still foobar'd ?

The template mentions to make a PR for features against a development branch:

Pick the target branch based on the following criteria:

• New feature, or refactor of existing code: develop branch

[robin-brabants]

I understand where this comes from, but the use case of verify_peer => false seems kinda anachronistic nowadays.

Possibly better to provision both host and client with a dummy certificate via https://github.com/FiloSottile/mkcert/blob/2a46726cebac0ff4e1f133d90b4e4c42f1edf44a/README.md ?

it is just an example for a unit test

[robin-brabants]

Passing a ssl/tls context to the RedisCluster constructor from the phpredis extension is apparently only introduced in version 5.3.2: https://github.com/phpredis/phpredis/blob/develop/CHANGELOG.md
I noticed this due to the failing pipeline.
This would of course be a breaking change though.

[boesing]

I havent had this kind of change ever but I remember that I did modified some redis range in the past. IMO having a check during CD that all the required extension versions are fulfilled should be a good thing, so I do not have a problem with that change personally, but how is this kind of stuff handled in other OSS.
Do you have experience here @Ocramius?

[robin-brabants]

I added a few commits which should fix the failing integration tests. Could they be ran again?

[boesing]

I'd added some comments, would be decent if we can improve this a little bit.
Sorry for the late response, I am not doing too much OSS at the moment.

[boesing]

I wonder if we can add an integration test where we actually setup a redis cluster with SSL and self-signed certificate to verify if this is actually working. I don't expect this in this PR tho as I think it would already suffice and I can understand that taking time to implement this kind of setup would be too much.
It took me quite some time to even have redis cluster docker setup up and running, since the docker image we are using here does not support SSL right now, we would have to create an own docker image. Thus I think I will create an issue off this comment once we merged this PR.

[robin-brabants]

Implemented an sslContext class right now which contains the sslcontext options in the fields.

I omitted the 'capture_peer_cert' and 'capture_peer_cert_chain' options since they are not relevant for us in my opinion.

I wanted to make some of the default values clearly visible in the constructor. I made them correspond to the actual sslContext default options at least for more recent php versions with the exception of 'SNI_enabled' which would make sense in my opinion to enable by default if possible.

[robin-brabants]

@Ocramius @boesing
Are there still any changes required for this PR

[boesing]

Sorry, havent reviewed yet. Will probably do so either today or tomorrow evening.

[boesing]

I refactored the code a little bit, especially since I am not a fan from those abbreviations all round tech.
I renamed some properties but still preserve the context notation when converting the object to the ssl context array.

I'd say we should only allow 1:1 array notations which would be accepted by PHP as well, thats why I removed all that camel case handling and just have two dedicated methods, a named factory to instantiate our object from an SSL context array (in the notation supported by PHP) and a method which converts the object back to a PHP supported SSL context array.

I also do not allow RedisClusterOptions#setSslContext to consume an array. IMHO if that is being called from somewhere, it should be either null or the SslContext object.
I did override AdapterOptions#setFromArray to ensure that ssl_context or sslContext array keys are always converted to an object in case an array is passed.
So test is still passing.

I also removed all the ssl context defaults. That prevents us from having issues with several PHP versions where (whyever) defaults might change and then we have to see how to provide defaults for both versions. I do not really see that we should do that and thus I'd say lets allow passing null for any option and in case it is not explicitly set (i.e. null), we do not pass that in the array we pass to RedisCluster#__construct.

I also would say we should not require ext-openssl since it would be only required from upstream projects as this component does pretty much work without that extension (as I removed all the defaults which were using OPENSSL_* constants) and thus I'd say, that is a requirement which should be added by projects rather than this library.

---

I'd love to get some review of my changes and some feedback. If that works for you and you still see the feature is serving your needs, I'd love to get a quick 👍🏼 so that we can proceed.
Since I do not have a cluster serving via TLS, I am unable to verify if everything is working. I've created #88 to keep track that we should have integration tests up and running so that we do not accidentally break stuff.

But I'd be fine with releasing this without actual integration tests as its a new feature which needs to be explicitly implemented by projects and thus its okayish to depend on projects having integration testings up and running until we implement #88.

Thanks for your massive work here and I'm happy to see your feedback.

---

Had to rebase with latest 2.8.x as renovate updated composer.lock...

[robin-brabants]

Sorry that I only see your improvement right now, but was a bit preoccupied.
I would have some time again after the weekend and will give my feedback on Monday.

[robin-brabants]

@boesing
Your changes look great to me! I just had two minor comments.
For me it is fine to merge this PR as is.

[boesing]

Thanks @robin-brabants!