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

New proposed SSL API #813

Closed
yossigo opened this issue May 20, 2020 · 4 comments
Closed

New proposed SSL API #813

yossigo opened this issue May 20, 2020 · 4 comments

Comments

@yossigo
Copy link
Member

yossigo commented May 20, 2020

Raising this here as a last chance to address it before 1.0.0 is released and we'll want to avoid breaking API changes.

Currently there are two ways to use SSL connections with hiredis:

  1. Using redisInitiateSSL() and provide an SSL * object.

This offers the greatest flexibility but the user needs to take care of everything -- initialize OpenSSL, create an SSL_CTX *, configure it in a safe and secure way and with consideration to the OpenSSL version in use, etc.

  1. Using redisSecureConnection() which handles all of the above.

While easier to use, this approach does have a fair share of limitations and pitfalls:

  • It does not play well with others who may have initialized OpenSSL.
  • It creates an SSL_CTX * per connection and re-configures it, which is not very performent, depends on cert/key files to exist over time, etc.
  • It provides no additional control over OpenSSL features and configuration options.

The proposal is to abandon the redisSecureConnection() mechanism, stay with redisInitiateSSL() and add a new option that serves as a compromise - users have greater control over initialization, context lifetime, etc. but still don't have to work directly with OpenSSL for simple stuff.

Here's a first draft of how such API may look like:

typedef struct redisSSLContext redisSSLContext;

/**
 * Helper function to initialize the OpenSSL library.
 *
 * OpenSSL requires one-time initialization before it can be used. Callers should
 * call this function only once, and only if OpenSSL is not directly initialized
 * elsewhere.
 */
int redisInitOpenSSL(void);

/**
 * Helper function to initialize an OpenSSL context that can be used
 * to initiate SSL connections.
 */

redisSSLContext *redisCreateSSLContext(const char *cacert_filename, const char *capath,
        const char *cert_filename, const char *private_key_filename,
        const char *server_name, const char **errptr);

/**
 * Free a previously created OpenSSL context.
 */
void redisFreeSSLContext(redisSSLContext *redis_ssl_ctx);

/**
 * Initiate SSL on an existing redisContext.
 *
 * This is similar to redisInitiateSSL() but does not require the caller
 * to directly interact with OpenSSL, and instead uses a redisSSLContext
 * previously created using redisCreateSSLContext().
 */

int redisInitiateSSLWithContext(redisContext *c, redisSSLContext *redis_ssl_ctx);
@carlosabalde
Copy link

Looks great to me. I assume when initiating SSL via redisInitiateSSLWithContext() the same redisSSLContext instance could be shared among multiple connections and it will be the caller's responsibility to call redisFreeSSLContext() when all connections using it are terminated and the context is not needed anymore, right?

@yossigo
Copy link
Member Author

yossigo commented May 20, 2020

Right! It's really just a wrapper around SSL_CTX with the same semantics (refcounts and all).

@yossigo
Copy link
Member Author

yossigo commented May 22, 2020

Draft implementation in #821

@michael-grunder michael-grunder mentioned this issue May 22, 2020
7 tasks
@michael-grunder
Copy link
Collaborator

Closing via #821

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

No branches or pull requests

3 participants