SSL Support Reorganization

[yossigo]

Here are some thoughts about reorganizing SSL support in hiredis.

1. Add a redisUseSSL(redisContext *, SSL*) function to let callers supply a pre-configured SSL* context and thus have full control over OpenSSL configuration options.
2. The above will also imply that hiredis does not need to initialize OpenSSL global state, as redisSecureConnection() does now and may cause conflicts with other users of OpenSSL.
3. Reorganize the code so the SSL-related functions will not be compiled and won't be available if hiredis was compiled without SSL support.
4. As an extension to the above, make it possible to compile only SSL support as a separate library (with its own header file) which can be used as an extension. This should make it easier to package hiredis and hiredis+ssl support separately if one wishes to do so (see Debian libevent/libevent-openssl as an example).

To achieve [3] and [4] I think sslio.c can become ssl.c and be self contained. The use of the REDIS_SSL flag to determine which read/write functions should be used can be replaced with simple function callbacks, and so can the use of struct redisSsl so the rest of hiredis is completely decoupled from the SSL implementation.

@mnunberg does this make sense to you?

[mnunberg]

As long as we make it possible (and make it the default) to have a single SSL library with SSL support, I'd be ok with it. I'm not sure though how you'd implement (4) without having a bunch of function pointers and using dlopen..

[yossigo]

A bunch of function pointers yes, to facilitate a kind of run-time polymorphism on top of redisContext. I don't see why dlopen will be necessary though.

[mnunberg]

Wouldn't the intent be to allow ssl support to be added while not making it a dependency tax?

[yossigo]

@mnunberg yes, but the way I see it you'll be able to build hiredis into hiredis.so (or a static lib of course) and hiredis_ssl.so.

If you never use SSL, just link with -lhiredis but you'll not have redisSecureConnection() etc.
If you want to use SSL, you need to link with -lhiredis -lhiredis_ssl -lssl -lcrypto and you'll have the SSL functions.

And as you indicate, by default we can just build a single library with/without SSL support. But thinking about how this can get packaged for Linux distros etc, I think the core + SSL support approach is a clear winner.

[mnunberg]

Oh, I see, that makes sense.

…

On Aug 28, 2019, at 11:37 AM, Yossi Gottlieb ***@***.***> wrote: @mnunberg <https://github.com/mnunberg> yes, but the way I see it you'll be able to build hiredis into hiredis.so (or a static lib of course) and hiredis_ssl.so. If you never use SSL, just link with -lhiredis but you'll not have redisSecureConnection() etc. If you want to use SSL, you need to link with -lhiredis -lhiredis_ssl -lssl -lcrypto and you'll have the SSL functions. And as you indicate, by default we can just build a single library with/without SSL support. But thinking about how this can get packaged for Linux distros etc, I think the core + SSL support approach is a clear winner. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#705?email_source=notifications&email_token=AADKOUMZ33GYXLVXKSDZNEDQG2LSPA5CNFSM4IRJRJK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5LRMBY#issuecomment-525800967>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADKOUIWOP5Y43MGMDO2YXTQG2LSPANCNFSM4IRJRJKQ>.

[yossigo]

@mnunberg I'm tempted to make this the only option rather than complicate the Makefile and live in a world where hiredis.so could be with or without SSL support. It basically means if you need SSL you always have an extra #include "hiredis_ssl" and -lhiredis_ssl, but then we core hiredis is never contaminated with SSL dependencies. Thoughts about it?

[mnunberg]

@yossigo I think this would complicate the installation of the library. Think about how an example using SSL would work-- You'd need to add the rpath properly, and so on.

I understand the concern that something like antirez/redis would have, but as I've mentioned before on other issues, hiredis is a simple enough library that people don't need to rely on our own Makefile-- it's provided there for convenience. In other words: if there is a more complex use case, you should handle it in redis, rather than push the solution "up" to hiredis.

I'm not sure of many e.g. debian packages that split ssl and non-ssl support, unless there is some clear licensing issue, or if there is a dual-option with gnutls or openssl.

[yossigo]

@mnunberg Why would splitting hiredis and hiredis_ssl add complication? If you want SSL, you'll need to #include an additional header file and link against an additional library. Simply adding another library won't affect rpath (you may need it anyway for libhiredis.so, but then the same would apply to libhiredis_ssl.so.

I think the pattern of creating separate packages for SSL support is quite common, if you browse Debian repos you'll see it quite often.

I also find it a good fit for hiredis because until the last official release it didn't support SSL, so providing it as a complementary library would be the natural thing to do from a backward compatibility point of view.

Did you take a look at the draft PR to get a feel of how it looks like? You can see how it affects the examples.

[yossigo]

Done in #708.