Wrapping OpenSSL global state initialisation?

[carlosabalde]

Current TLS support in hiredis offers two ways to trigger the OpenSSL global state initialisation:

• Do it implicitly when calling redisSecureConnection().
• Do it explicitly calling SSL_library_init() on your own (if that's not already done by another component) and then use redisInitiateSSL() to set up Redis connections.

In order to avoid unwanted dependencies, it would be very convenient to hide the explicit call to SSL_library_init() in a public hiredis symbol. In other words, wrap the following logic + state from ssl.c in a function and expose it via hiredis.h.

    static int isInit = 0;
    if (!isInit) {
        isInit = 1;
        SSL_library_init();
#ifdef HIREDIS_USE_CRYPTO_LOCKS
        initOpensslLocks();
#endif

I'm not very familiar with hiredis internals, but if you agree with the proposal I could try to submit a PR :)

[michael-grunder]

cc: @yossigo as you wrote the SSL layer and know it much better than I do 😄

[carlosabalde]

I've just corrected a couple of wrong function names in my initial comment. Sorry for the confusion!

[carlosabalde]

I've spent some more time playing with hiredis & TLS in a personal project. Although I still think this could be a nice addition, now I see it won't be as useful as I initially thought. I'm closing the issue. Sorry for the noise and thank you all for the great work :)

[michael-grunder]

It wasn't too much noise at all!

[yossigo]

@carlosabalde Thanks for your input, just saw this now. I personally don't like redisSecureConnection() so much and would be happy to see it deprecated eventually in favor of redisInitiateSSL().

But I'm not sure I understand your proposal -- can you please explain nevertheless?

[carlosabalde]

@yossigo, I think redisSecureConnection() is very nice for simple use cases. It favors encapsulation hiding a lot of OpenSSL details. Main problem I see is how redisSecureConnection() implicitly and unconditionally calls SSL_library_init().

For example, I'm adding TLS support to a Varnish Cache module using hiredis. I'd prefer to ignore OpenSSL details and simply use redisSecureConnection(), but I cannot. Two reasons for that:

• Under certain circumstances OpenSSL global state is initialized while bootstrapping Varnish Cache. Using redisSecureConnection() in the Varnish Cache module would trigger an additional initialization. This is the scenario described in the documentation.

• Even assuming Varnish Cache never triggers OpenSSL initialization during bootstrap, the implicit OpenSSL initialization triggered by redisSecureConnection() still is problematic. Varnish Cache is highly multi-threaded and that could result in multiple OpenSSL initializations. Some control about when the initialization is executed is needed.

I could use redisInitiateSSL()... and that's what I did :). It's very convenient if you need full control, but that means you'll need to deal with OpenSSL stuff like SSL_library_init(), SSL_CTX_new(), SSL_new(), etc. I'd rather prefer to avoid that and simply use redisSecureConnection(). In order to do that some minor changes would be required:

• Adding an additional parameter to disable implicit OpenSSL auto-initialization when calling redisSecureConnection().

• Adding an additional entry point to trigger the hiredis OpenSSL initialization when is ok (i.e. Varnish Cache didn't trigger it during bootstrap) and safe (i.e. when multi-threading is not going to be a problem) to do it.

That was the original motivation for this case. Hope it helps to understand a little bit the scenario.

[yossigo]

@carlosabalde Thanks! This is all clear of course, and it's exactly why I tend to think the apparent simplicity of redisSecureConnection() is misleading and can be dangerous.

Note that it's not only the fact that it performs its own OpenSSL initialization, but also how this initialization is performed. For example, on older versions of OpenSSL you need to register locks callbacks for thread safety.

An alternative approach could be:

1. Drop support for redisSecureConnection() altogether. Not sure if backwards compatibility is an issue because it was never part of a released version!
2. Add something like a hiredisInitOpenSSL() helper functions which apps should call if they don't do their own SSL initialization. It will be explicit, but just one simple call.
3. Add something like a hiredisCreateSSLContext() helper function which takes a set of parameters similar to redisSecureConnection() and produces an SSL context that can be fed to redisInitiateSSL().

Does this make sense?
@michael-grunder @mnunberg What do you think?

[carlosabalde]

I like it. A single entry point (redisInitiateSSL()) + a couple of helpers (hiredisInitOpenSSL() & hiredisCreateSSLContext()) looks more consistent that current redisSecureConnection().

[michael-grunder]

Sounds good to me, and like you said there are no backward compatibility issues since we haven't yet released a version with SSL support.

If we do decide to make the changes we should do them before v1.0.0 when our options become far more limited.

[yossigo]

@michael-grunder 1.0.0 is definitely the point of no return on this, I wasn't sure about the amount of exposure the current SSL API has even as it was never officially released.

@valentinogeron As we've also discussed this ourselves, does that make sense to you as well?

[michael-grunder]

I can't imagine it would cause too much pain, but I'm confident our users will let us know if it does.

Seems worth it to nail down an API we're happy with.

[yossigo]

@michael-grunder I'll give it a shot then.

[valentinogeron]

@yossigo - sounds good to me.

[yossigo]

I've opened ticket #813 with a proposal, please let me know if this looks good to you.