Support SSL Redis connections

[zbarnes757]

Is your feature request related to a problem? Please describe.
We connect to our Redis instances with SSL but this client does not support that. I would like to see the ability to connect via SSL using the same TLS options I pass to the LD client.

Describe the solution you'd like
I believe the issue lies here: https://github.com/launchdarkly/erlang-server-sdk/blob/main/src/ldclient_storage_redis_server.erl#L46-L51

A simple solution would be to propagate the TLS options into eredis. Example:

    Host = ldclient_config:get_value(Tag, redis_host),
    Port = ldclient_config:get_value(Tag, redis_port),
    Database = ldclient_config:get_value(Tag, redis_database),
    Password = ldclient_config:get_value(Tag, redis_password),
    Prefix = ldclient_config:get_value(Tag, redis_prefix),
    HTTPOptions = ldclient_config:get_value(Tag, http_options),
    TLS = maps:get(tls_options, HTTPOptions, []),
    EredisOpts = [
        {host, Host}, {port, Port}, {database, Database}, {password, Password}, {tls, TLS}
    ],
    {ok, Client} = eredis:start_link(EredisOpts),

[kinyoklion]

Hello @zbarnes757,

Thank you for the issue, and suggestion.

Filed internally as 211045

Thank you,
Ryan

[kinyoklion]

Hey @zbarnes757,

As long as this approach meets the requirements of your infrastructure, I think it is a place that we can start.

Generally speaking I have some concerns that the certificates and verification appropriate for redis may not also be the configuration most appropriate for other connections. If that turns out to be a problem, then it wouldn't hurt to later allow more specific settings for the redis client. (At that time it could default to the http_options, and then use more specific redis_options if they are available.)

I'm starting to look at this, it will mostly be a case of validating configurations and the behavior of our version of eredis. If possible I would rather not require a new eredis version at the same time. (Though that does need to be done.)

Thanks,
Ryan

[kinyoklion]

@zbarnes757,

I think this will require at least a bit of extra configuration. The primary problem with directly using the settings is that eredis will attempt to upgrade to tls if TLS options are provided.

This means, at a minimum, a setting to toggle.

The secondary problem I have found is with TLS negotiation. The same set of cipher suites may not be appropriate for general connections as well as redis.

So, I think, I will add some additional redis configuration that allows specific TLS configuration. If the same configuration works for you in both places, then you can easily provide that config to both.

Thank you,
Ryan

[zbarnes757]

@kinyoklion that makes sense! Probably best to have separate tls configurations.

[kinyoklion]

@zbarnes757,

Support has been released in version 2.1 of the SDK.

In my individual testing I ended up using a different configuration for redis than that used for HTTPS connections. This is because host name validation and ciphers for the redis configuration I was using were not easily compatible. If you only care about the encryption, versus the authenticity, then you could omit 'verify_peer'.

Additionally the redis configuraton supports both client and server options. This allows for client verification mode of redis.

Thank you,
Ryan

[kinyoklion]

Basic example, where TLS is enabled, but it doesn't verify authenticity.

    ldclient:start_instance("LD_SDK_KEY", #{
        feature_store => ldclient_storage_redis,
        redis_password => "some password",
        redis_host => "myurl",
        redis_port => 18749,
        redis_tls => [
            {cacertfile, "/some/path/redis_ca.pem"}
        ],
        http_options => #{
            tls_options => ldclient_config:tls_basic_options()
        }
    }),

If you omit redis_tls, or provide an empty list, then it will not use TLS.

Thank you,
Ryan

[zbarnes757]

@kinyoklion I was able to get it to work! It took some finagling but I got it.

My setup:

redis_tls_options = [
      cacertfile: '/etc/ssl/certs/ca-certificates.crt',
      verify: :verify_none,
      customize_hostname_check: [match_fun: :public_key.pkix_verify_hostname_match_fun(:https)]
    ]

[zbarnes757]

the only other thing I would recommend is adding the new option to the specs. My dialyzer is throwing warnings because of it.

[kinyoklion]

@zbarnes757
That is good news that you have things working.

In regards to the dialyzer, where are you getting errors? The settings should be in the spec, and should allow any tls_option. It is possibly I missed something.

Thanks,
Ryan

[zbarnes757]

It looks like it is needed here https://github.com/launchdarkly/erlang-server-sdk/blob/main/src/ldclient_instance.erl#L16-L45C1

[kinyoklion]

Ah, thank you, yes. I see that typing is duplicated, I should sort that.

[kinyoklion]

@zbarnes757 2.1.1 has been released that fixes the specs for the instance typing.

Thanks!
Ryan

[zbarnes757]

perfect! Works like a charm. Shall I close the issue?

[kinyoklion]

Ill go ahead and close it. If you encounter any other problems, then let us know.

Thank you,
Ryan