SSL/TLS: Add SslClientAuthenticationOptions configurability

[NickCraver]

Proposal for #2219, allowing explicit configuration of the SSL connection for advanced use cases. This needs some thought, but pitching the general idea here as an option available for the frameworks that support it.

[slorello89]

@NickCraver - this LGTM in principle, my initial thought was to avoid any public API surface area to make it a more palatable addition. But I think this is way more usable and enduring and will give users maximal flexibility moving forward.

[btchoum]

@NickCraver

I like this idea a lot. It solves the immediate issue while giving us more flexibility. I will give it a try in my environment and let you know how it goes.

Thank you so much for considering this issue.

[NickCraver]

@btchoum minor but important fix pushed if trying this...otherwise it'll double auth because I'm an idiot who didn't hit save.

[slorello89]

@NickCraver - I've confirmed this fix works for my purposes :) - one thing I will note that may or may not be worth addressing. The way the tls authentication works now, it actually swallows the authentication failures in tls.

When I was initially testing this I was trying to run something like:

var options = new ConfigurationOptions();
options.EndPoints.Add($"{host}:12000");
options.Ssl = true;
options.SslClientAuthenticationOptions = delegate(string s)
{
    var opts = new SslClientAuthenticationOptions();
    opts.TargetHost = host;
    opts.EnabledSslProtocols = SslProtocols.Tls12;
   
    opts.CipherSuitesPolicy = new CipherSuitesPolicy(new TlsCipherSuite[]
    {
        TlsCipherSuite.TLS_RSA_WITH_AES_128_CBC_SHA256,
        TlsCipherSuite.TLS_RSA_WITH_AES_256_CBC_SHA256
    });
    
    opts.RemoteCertificateValidationCallback += (sender, certificate, chain, errors) =>
    {
        Console.WriteLine(errors);
        return true;
    };
    return opts;
};

options.SslProtocols = SslProtocols.Tls12;
options.CertificateValidation += (sender, certificate, chain, errors) => true;

This is erroneous because options.CertificateValidation and opts.RemoteCertificateValidationCheck both assign the delegate, but the only error I was getting was:

It was not possible to connect to the redis server(s). There was an authentication failure; check that passwords (or client certificates) are configured correctly.

If you really dig though, you see that you get:

The 'RemoteCertificateValidationCallback' option was already set in the SslStream constructor.

Now if I dropped a text-writer into the constructor of the multiplexer, this of course lights up the issue, and maybe this is a broader discussion, but some of these failures - might it be worth adding them as inner-exceptions to the exceptions the muxer is already throwing?

[NickCraver]

@slorello89 hmmm I see the issue, but changing the flow is a bit more complicated because we don't throw from that method. I agree we should improve this for sure - let's get tests on this, commit as-is as an enhancement, then separately improve the error logging. I have general ideas of per-endpoint having the last exception and we can sum those up, etc. - lots we can improve here for sure in surfacing this.

[NickCraver]

Started to add tests here but remembered we have a secure (password) image in the docker-compose but not a SSL/certificate one...which we can now do for Redis 6+ and light up these tests. No expertise there so I'll need to investigate how to light up a new port that's SSL-enabled (self-signed/test cert - doesn't have to be valid).

[slorello89]

Hey @NickCraver - yeah so when it comes to building an environment for this, it gets a bit sticky, as you're aware TLS was added in Redis 6, what you might not know (depending how deep you are into the weeds of redis tls), is that it's not shipped in the standard binaries/distros of redis. Rather you need to build redis from source with the tls flag set. I'm not sure how you want to handle this - you're obviously a lot more familiar with how the StackExchange.Redis test-suite/environment is setup - I couldn't gather much beyond it looking like you run everything on a windows-server image?

I put together this repo which has an image that you can use to validate the fix (it uses the cipher AES256-SHA256 - that's TLS_RSA_WITH_AES_256_CBC_SHA256 in the IANA parlance that .NET uses), and ran this bit of code against it to validate the cipher-suite fix (as well as just generally insuring that the options worked :))

var options = new ConfigurationOptions();
options.EndPoints.Add("localhost:6379");
options.Ssl = true;
options.SslClientAuthenticationOptions = delegate(string s)
{
    var opts = new SslClientAuthenticationOptions();
    opts.TargetHost = s;
    opts.EnabledSslProtocols =  SslProtocols.Tls12 | SslProtocols.Tls13 ;
    // opts.CipherSuitesPolicy = new CipherSuitesPolicy(new[] {TlsCipherSuite.TLS_RSA_WITH_AES_128_CBC_SHA256}); // change to this and it will break
    opts.CipherSuitesPolicy = new CipherSuitesPolicy(new [] {TlsCipherSuite.TLS_RSA_WITH_AES_256_CBC_SHA256});
    opts.CertificateRevocationCheckMode = X509RevocationMode.NoCheck;
    opts.RemoteCertificateValidationCallback += (_, _, _, _) => true;
    return opts;
};

var muxer = await ConnectionMultiplexer.ConnectAsync(options);
var db = muxer.GetDatabase();

db.StringSet("foo", "bar");
Console.WriteLine("Completed. . .");

[NickCraver]

@slorello89 overall we're using the redis-latest docker image for most builds, so only looking to solve it there. I believe this is already built with support (https://github.com/docker-library/redis/blob/5c0bbb5dfce3d4999649cbc3ba8bf7c123bcadff/7.0/Dockerfile) so I think it's a matter of figuring out the docker bits to get any certificate on there and loaded (I don't care about validity). Am I reading that right?

I was thinking of how to generate it but I guess we can just put one 20 years out directly in the repo to build in - I'm perfectly okay with that long as it has a comment on re-generating.

[slorello89]

@NickCraver - yep looks like they are building the docker image with it (I was a bit lost in the forest for the trees) - I just pushed an update to that repo slimming it down so it uses the redis image and the bare minimum of what it would need to test things.

As far as validity is concerned, you can work around that with the configuration options - notice I just return true when it's trying to validate above. Or you could create a very long living cert.

[slorello89]

@NickCraver - I think I understand how to get it in - do you mind if I push to this branch?

[NickCraver]

@slorello89 Not at all - if you have the bandwidth I'd very much appreciate it. Don't worry about tests failing if you add a new instance and default config for SSL server/port for the new instance (I'd expect Windows to start failing the non-skipped tests) - can figure that out once it works at all.

[slorello89]

@NickCraver - that worked on my machine (granted I did not spin up the StackExchange.Redis build enviornment) heads up, GH might complain to the two of us about leaked cert files but those certs aren't real certs :)

[slorello89]

yeah no not quite :) ok I might see where I've gone wrong.

[slorello89]

@NickCraver - sorry for spamming this PR with commits looks like I needed to disaggregate the tls versions - probably a cipher-suite negotiation thing with the server it kept failing when given the choice of TLS 1.3 since I was passing it bogus pre-tls ciphers - lol - looks like the tests are passing (at least for now) - Is this something that should be skipped on windows since it's using Redis 3?

[NickCraver]

@slorello89 Thanks a ton for the docker setup work here, much appreciated! I'm taking a poke locally at getting tests cross-platform friendly, simplifying a bit, lighting up the existing SSL tests...and maybe improving those error messages a bit while in here. I think I need it to have test sanity anyway so seeing how hard that is to tackle.

[slorello89]

LGTM 👍

[NickCraver]

@slorello89 thanks for the eyes and help here <3

[btchoum]

@NickCraver @slorello89
Thank you so much for solving this issue. We’ve been using the latest version of the package and so far works perfectly. Much appreciated