Support for non blocking SSL handshake

[casperisfine]

I'm working on a new hiredis binding for Ruby, and I'm running into what seem to be a limitation to handle SSL handshake timeout.

The Ruby binding use redisConnectNonBlock so that it can use the Ruby VM APIs to wait on IO and release the GVL (like Python's GIL). This works well for raw TCP or UNIX sockets, but it seems to me that the SSL APis exposed by hiredis are incomplete.

My code look like this:

hiredis_init_ssl(...) {
    if (redisInitiateSSLWithContext(redis_context, ssl_context)) {
      // handle error
    }
}

redisInitiateSSLWithContext does handle non blocking IOs by returning OK:

hiredis/ssl.c

Lines 334 to 339 in d7683f3

	rv = SSL_get_error(rssl->ssl, rv);
	if (((c->flags & REDIS_BLOCK) == 0) &&
	(rv == SSL_ERROR_WANT_READ || rv == SSL_ERROR_WANT_WRITE)) {
	c->privctx = rssl;
	return REDIS_OK;
	}

However if the error was SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE, the caller is expected to call SSL_connect again:

If the underlying BIO is non-blocking, SSL_connect() will also return when the underlying BIO could not satisfy the needs of SSL_connect() to continue the handshake, indicating the problem by the return value -1. In this case a call to SSL_get_error() with the return value of SSL_connect() will yield SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE . The calling process then must repeat the call after taking appropriate action to satisfy the needs of SSL_connect(). The action depends on the underlying BIO . When using a non-blocking socket, nothing is to be done, but select() can be used to check for the required condition.

The problem is that the reference to the SSL pointer is entirely hidden, and that calling redisInitiateSSLWithContext again would return an error because it's not meant to be repeated.

I feel like it's missing a small API to retry the SSL_connect call after waiting on select(2).

cc @michael-grunder

[michael-grunder]

Hi, @casperisfine thanks for the report.

Your fix looks good, but I'm wondering if there is a way to provide this without exposing the SSL context, as it would put more stringent limitations on the library moving forward. I kind of like that it's encapsulated.

@yossigo I'll play around with it but let me know if you have any thoughts.

[yossigo]

@michael-grunder I agree with you that internal structures should not be exposed, can't see why it's really necessary. We do need to remember redisSSLConnect() was incomplete and let the caller know, so it can call the resume function. @casperisfine Do you agree?

BTW Looking at the code again, I realize there's something a lot bigger that's still missing: right now, there's no proper handling of redisSSLConnect() for async contexts. In an async context, redisInitiateSSL() or redisredisInitiateSSLWithContext() should handle the cases where blocking for IO is required, use an adapter to register on the event loop, re-try and eventually fire a success/failure callback.

[casperisfine]

I kind of like that it's encapsulated.

Yes me too, my "fix" is only meant as a short term hack.

do need to remember redisSSLConnect() was incomplete and let the caller know, so it can call the resume function.

Yes, I think such API would be preferable. However unless I'm mistaken, the caller have to know if it needs to wait for read or for writes (or both), so I believe that information has to be exposed.