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

ssl_params: doesn't appear to support min_version #159

Closed
geoffharcourt opened this issue Jan 11, 2024 · 6 comments
Closed

ssl_params: doesn't appear to support min_version #159

geoffharcourt opened this issue Jan 11, 2024 · 6 comments

Comments

@geoffharcourt
Copy link

geoffharcourt commented Jan 11, 2024

While trying to programmatically enforce a minimum TLS version for our Sidekiq Redis connections, I ran into an issue that I think would need to be handled in redis-client.

If you pass a min_version parameter into ssl_params, an attempt to connect responds with an `unexpected eof while reading (Redis::CannotConnectError).

Here's what my code I used to check against a TLS-supporting (v1.2 or 1.3) Redis instance looked like:

# works
client = RedisClient.new(
  url: url, 
  ssl_params: { verify_mode: OpenSSL::SSL::VERIFY_PEER },
)
client.call("EXISTS", "hi") # works

# blows up
client = RedisClient.new(
  url: url, 
  ssl_params: { verify_mode: OpenSSL::SSL::VERIFY_PEER, min_version: OpenSSL::SSL::TLS1_3_VERSION },
)
client.call("EXISTS", "hi") # raises

The exception looks like this:

main:0> client.call("EXISTS", "hi")
/bundle/ruby/3.2.0/gems/redis-client-0.19.1/lib/redis_client/ruby_connection.rb:131:in `connect_nonblock': SSL_connect returned=1 errno=0 peeraddr=aa.bb.cc.dd:6379 state=error: unexpected eof while reading (RedisClient::CannotConnectError)

Passing this parameter works successfully with redis-rb's SSL setup. It wasn't at all clear from the code where the SSL context is built and then later passed on to OpenSSL::SSL::SSLSocket why this wouldn't be working, but unless I'm missing something I think this prevents setting a minimum SSL/TLS version for connections.

Thanks for all your work on the gem.

@byroot
Copy link
Member

byroot commented Jan 11, 2024

The handling of ssl_params happens here:

def ssl_context(ssl_params)
params = ssl_params.dup || {}
cert = params[:cert]
if cert.is_a?(String)
cert = File.read(cert) if File.exist?(cert)
params[:cert] = OpenSSL::X509::Certificate.new(cert)
end
key = params[:key]
if key.is_a?(String)
key = File.read(key) if File.exist?(key)
params[:key] = OpenSSL::PKey.read(key)
end
context = OpenSSL::SSL::SSLContext.new
context.set_params(params)
if context.verify_mode != OpenSSL::SSL::VERIFY_NONE
if context.respond_to?(:verify_hostname) # Missing on JRuby
context.verify_hostname
end
end
context
end

Passing this parameter works successfully with redis-rb's SSL setup.

By that you mean redis-rb 4.x? If so the code is pretty much the same:

https://github.com/redis/redis-rb/blob/0afcf1cad6f4fc74213f2b4c7a20819581c36897/lib/redis/connection/ruby.rb#L249

@byroot
Copy link
Member

byroot commented Jan 11, 2024

Also unexpected eof while reading sounds like redis/redis-rb#1106

What's the redis-server version? Older Redis would close the socket without shutting down the SSL session which would cause this issue (not saying it's your issue, but this may be hiding the real root cause)

@geoffharcourt
Copy link
Author

Hi @byroot we're connecting to AWS Elasticache, it's configured as Redis 7.0.7.

This is interesting, I didn't realize the redis gem now uses redis-client! In my testing last night I used the same ssl_params: arguments with RedisClient and Redis v5.0.8 and was able to make calls with Redis 5.0.8 successfully but not with RedisClient 0.19.1.

@byroot
Copy link
Member

byroot commented Jan 11, 2024

was able to make calls with Redis 5.0.8 successfully but not with RedisClient 0.19.1.

That very surprising. I'd recommend editing redis-client to see what ssl params it receive from redis-rb, but there is little to no transformation IIRC.

@geoffharcourt
Copy link
Author

I got something crossed up, apologies for using your time here.

@byroot
Copy link
Member

byroot commented Jan 12, 2024

Happens to the best of us 😄

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

2 participants