search

nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

SSL support for connections to Redis #202

Closed jessie-murray closed 2 years ago

jessie-murray commented 2 years ago

This PR adds support for encrypted connections to Redis, using Hiredis's SSL support which was added in v1.0.0. This fixes issue #201. I started by upgrading Hiredis to v1.0.2 and re-adding the macros that mark some functions as unused. A few fields had also changed types, so I had to update Webdis in 2 places to use the correct sprintf format. Once this was integrated I added a block in the Makefile to enable SSL support, which is done with a pre-processor macro. The reason for this is to avoid linking against libssl and libcrypto by default due to the frequent security vulnerabilities that are found in these libraries. With the macro in place I added SSL initialization code in server.c and handshake in pool.c, as well as a new config block read from conf.c. Lastly I documented the configuration and added a troubleshooting section for some issues.

@nicolasff I see there has been no response to your Hiredis question but after trying the different approaches we talked about and with the sentinel example I'm more confident now that this is the right way to use this feature.

The main question left is how this would be packaged with Docker: if the goal of this build option is to not expose a binary built with SSL by default, then how would someone connect to Redis securely with this image? The issue is not even to have the binary in the image, but also to include all the PEM files. Maybe Webdis can have a Docker image with SSL and one without?

I used this guide to setup SSL and ran all the tests including Websockets with SSL enabled.

nicolasff commented 2 years ago

I forgot to mention, I'm not really sure how to provide this feature. I'm kind of weary of having it enabled by default, I feel like there has been so many CVEs on Alpine due to openssl vulnerabilities that I would really prefer not to link against it if it's not needed. I mean just look at the last few releases, almost all of them bump the version of the Alpine base image due to vulnerabilities, many of them in openssl.

Maybe 2 binaries could work? e.g. /usr/bin/webdis and /usr/bin/webdis-ssl? Then since users who want to use SSL have to build their own image anyway with a custom config file that points to the right certs and keys, they can also overwrite webdis with webdis-ssl or use it as their entry point.

I'm also not so sure about doing it that way, because maybe it's not a big deal if there's an openssl vulnerability in a binary where it's unused. So yeah, none of this really changes anything about this PR, but I think there's still a bit more to do once it's merged and it's not completely clear to me yet.

If anyone reading this has a strong opinion about it, suggestions would be welcome :-)