Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses #27: Adds support for making SSL connections to redis via hiredis 1.0.0+ #36

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jcohen02
Copy link

Adds support to redux for making SSL connections to redis. Makes use of SSL support added in hiredis 1.0.0. Addresses #27.

Multi-platform support:

  • Windows: Known working in Windows 10 (based on updates included within this PR and Windows: update to hiredis 1.0.0 #35), using the updated binaries provided in rwinlib/hiredis.
  • Linux: Working when built against custom-compiled hiredis-1.0.0 with SSL support, fixes needed for automated build (see below)
  • MacOS: Working when built against custom-compiled hiredis-1.0.0 with SSL support, fixes needed for automated build (see below)

Known issues:

  • Linux: At present, based on the configure script, if a version of hiredis is not found using pkg-config or via INCLUDE_DIR and LIB_DIR being provided to point to a custom compiled version of hiredis, an error is shown with details of the package names to install on common distros. My understanding is that most (all?) distros don't yet have packages for hiredis 1.0.0 and there is no SSL support.
  • MacOS: The current Homebrew formula for hiredis is for v0.14 and again includes no SSL support. If building against a custom compiled version of hiredis 1.0.0 with SSL support, then things work OK on MacOS.

Question: @richfitz, @jeroen: How would you like to handle the issue of missing SSL support on standard configurations on Linux/MacOS? For now, should we simply wrap the content of the redux_redis_connect_ssl function in connection.c in a pre-processor directive that online includes the content of this function for a Windows build and returns an error saying that the library was not built with SSL support for other platforms? This would at least ensure the library continues to operate as before on Linux/MacOS under a standard build.

Copy link
Owner

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jeremy. Probably no need to tag Jeroen in this unless we need his help getting the underlying libraries working.

There are a bunch of things that will make this much easier:

  • We should set up github actions and drop appveyor/travis. Travis is queuing for hours for me for some reason, and this will make it easier to configure with-and-without SSL as different versions. I can do that on a separate branch once I get a moment.
  • From an interface point of view, if you can feature test from the headers that's fine. The important thing is that from the point of view of functions that are called from R - all must exist. At the moment we have different functions for different types of redis connections, so I guess we're getting a 3rd here? Make sure that a dummy one exists and throws an error in the case where SSL support is not present
  • Some docs will be needed for use with SSL. You can put it in the README, the existing vignette or a new one. It would ideally cover the gist of installation of hiredis+ssl support on the 3 main platforms, and any limitations/special considerations of use with ssl

@Enchufa2
Copy link

Enchufa2 commented Sep 1, 2021

Is this PR alive? Any plans on adding TLS support?

@jcohen02
Copy link
Author

jcohen02 commented Sep 1, 2021

Hi @Enchufa2, thanks for highlighting this PR, yes, I'm really keen to get this merged in - I think there's probably an action on me to do some further tests and resolve remaining issues and this got rather lost due to other work! I'll take another look and hopefully this can be resolved and @richfitz can merge.

Have you been able to test the implementation in the feature/ssl branch? Any feedback welcome.

@richfitz
Copy link
Owner

richfitz commented Sep 1, 2021

yes, comments to look at above. This will also need merging/rebasing against master so that it runs on gha

@jcohen02
Copy link
Author

jcohen02 commented Sep 1, 2021

yes, comments to look at above. This will also need merging/rebasing against master so that it runs on gha

Do you prefer rebasing vs. a merge commit of master into the branch? Just done the latter since a rebase at this stage will require a force push back to feature/ssl which might cause issues/nuisance for anyone actively using the code in this branch...if you're happy, I'll stick with the current approach - didn't want to go against any conventions used in the repo.

@richfitz
Copy link
Owner

richfitz commented Sep 1, 2021

I have no strong feelings - I tend to rebase mine, but don't mind what you do

@Enchufa2
Copy link

Enchufa2 commented Sep 1, 2021

Have you been able to test the implementation in the feature/ssl branch? Any feedback welcome.

Not yet. The need has arisen to connect to a TLS-only Azure Redis, and I'm a bit in a rush here. I've seen no docs and I'm unsure about how to use this feature. Is it enough to set scheme="rediss"? Are system cert authorities used if nothing else is specified?

@jcohen02
Copy link
Author

jcohen02 commented Sep 1, 2021

Ah, ok, apologies, just catching up with where things are with this. I'll make sure there's full documentation before this gets merged in but since you're in a rush, you should be able to get make a connection as an initial test as follows:

[this has been tested in a clean Ubuntu 20.04 container with R 4.1.1 installed from packages based on the info at https://cran.r-project.org/bin/linux/ubuntu/, and also on macOS 10.15 with R 4.0.5 - if you're not running on macOS or Linux, it should also be buildable on Windows, I can try and provide some Windows instructions if you're using the Windows platform - I seem to recall that there were some issues that we had to work around]

You'll need an install of hiredis with SSL support to build redux against, this should be straightforward to build from source:

  • Get hiredis release 1.0.0 and extract.
  • Build with USE_SSL=1 - if you're using macOS and have a version of openssl installed via macports/homebrew, you might need to add the relevant paths to CFLAGS/LDFLAGS, e.g.:
    $ make USE_SSL=1
    
    If you're installing hiredis to a custom location, you can set a prefix env var when you run make install, e.g.:
    $ sudo make install USE_SSL=1    # (note that you need the USE_SSL on the make install command too)
    
    or, as pointed out above, on macOS, you may need to prefix the build command with the include/lib paths, e.g.
    $ CFLAGS=-I/opt/local/include LDFLAGS=-L/opt/local/lib make USE_SSL=1
    

Now you should be able to clone and build redux with SSL support (from the fork with the feature/ssl branch)...

  • Clone and checkout branch:

    $ git clone https://github.com/jcohen02/redux.git; cd redux
    $ git checkout feature/ssl
    
  • Depending on whether you have a clean R environment or you already have a number of core dependencies installed, you may need to install the R packages R6 and storr

  • Now build/install the redux R library with SSL support this will use pkg-config to find the hiredis and hiredis_ssl libraries:

    $ R CMD INSTALL .
    

    (on macOS I had to prefix the INSTALL command with the PKG_CONFIG path to the hiredis pkg-config file, e.g. PKG_CONFIG_PATH=/usr/local/lib/pkgconfig R CMD INSTALL .)

You should now be ready to use redux to make an SSL connection to a redis instance with SSL support. Presumably for your target Azure Redis instance you have a client keypair and also access to the CA certificate required for the connection? In the following example you'll see that the client cert, key and CA cert paths are passed when setting up the redis_config. You should also be able to set the following environmnt variables for these values and the call to redis_config will pick them up:

  • REDIS_SSL_CERT_PATH: Full path to client certificate file.
  • REDIS_SSL_KEY_PATH: Full path to client key file.
  • REDIS_SSL_CA_PATH: Full path to CA certificate file.

It should also be possible to pass the various other connection parameters using the following env vars: REDIS_CONNECTION_SCHEME ("redis" or "rediss"), REDIS_HOST, REDIS_PORT.

At an interactive R prompt the following should now be possible:

> library(redux)
> config <- redux::redis_config(url = "rediss://<hostname:sslport>", certPath = "/path/to/certificate.crt", keyPath = "/path/to/key.key", CApath = "/path/to/cacert.pem")
> redis <- hiredis(config)
> redis$SET('test', '123')
 [Redis: OK]
> redis$GET('test')
 [1] "123"

Hope this helps, let me know if you're having problems getting things going or anything isn't clear. As I say, detailed documentation will be added soon.

@Enchufa2
Copy link

Enchufa2 commented Sep 1, 2021

Thanks for the details. The environment is a CentOS 7 docker image. I was using stunnel as a temporary fix. But will try and report any quirks.

@jcohen02
Copy link
Author

jcohen02 commented Sep 2, 2021

Just a quick update to the previous comments - thought I'd give this a quick test on CentOS 7, I was getting SSL connection issues, e.g.

> library(redux)
> config <- redux::redis_config(url = "rediss://localhost:6379", certPath = "/root/test_certs/client.crt", keyPath = "/root/test_certs/client.key", CApath = /root/test_certs/ca.crt")
> redis <- hiredis(config)
Error in redis_connect_tcp_ssl(config$host, config$port, config$CApath,  : 
  Failed to create SSL context: Unknown error code
>

Not clear quite what the cause of this is yet - it works fine on macOS and Ubuntu 20.04 but I found that as a workaround, turning on off optimisation on the build of redux resolves this! I did this by adding a ~/.R/Makevars file and including CFLAGS=-g (since it seems to be building with -g -O2 by default). This will be investigated further.

Also note that the redux SSL support has now also been tested and works successfully with a build of redis 6 with SSL support - previously this was being used with stunnel/redis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants