redis / hiredis-rb

Ruby wrapper for hiredis
BSD 3-Clause "New" or "Revised" License
320 stars 90 forks source link

hiredis now supports SSL #58

Open tjwallace opened 5 years ago

tjwallace commented 5 years ago

https://github.com/redis/hiredis/pull/645 - not sure if any changes need to be made here

degzcs commented 5 years ago

Hi there, is there any chance that this gem is going to be updated to support SSL in the near future?

philipbjorge commented 5 years ago

Is there a status update on this? Any idea what level of work is required to make this available (and where it needs to happen)?

maxrosecollins commented 5 years ago

Any status on this? I really need this to be supported

michael-grunder commented 5 years ago

Hi everyone,

I'm not sure if there are any plans to add SSL support, but I think the current maintainers of hiredis-rb are @byroot and @fw42.

michael-grunder commented 5 years ago

Hi all,

I took a crack at adding this over in my fork, and it appears to work: https://github.com/michael-grunder/hiredis-rb/commits/ssl-support

Disclaimer: I've never used Ruby or the Ruby extension API, so I could be missing some obvious gotchas. There's quite a bit of arcane logic (temporary volatile variables, NULLing things) seemingly intended to trick the GC. My point is my code may be very wrong. 🤣

Instead of adding to the existing connect methods, I just replicated the workflow in hiredis itself. What you do is connect to Redis normally and then call conn.secure with certs and keys.

Quick example:

require 'hiredis'

conn = Hiredis::Connection.new
conn.connect("127.0.0.1", 6390)

# Prototype: secure(ca, [cert, key, servername])
conn.secure "/path/to/ca", "/path/to/cert", "/path/to/key", "servername"

conn.write ["SET", "foo", "bar"]
conn.write ["RPUSH", "list", "a", "b", "c", "d", "e"]
conn.write ["GET", "foo"]
conn.write ["LRANGE", "list", 0, -1]

puts conn.read
puts conn.read
puts conn.read
puts conn.read

Edit: If you want to test it out without gem installing the fork, it can be done like so

# Make sure you're on the ssl-support branch
hiredis-rb $ USE_SSL=1 rake compile
hiredis-rb $ ruby -Ilib your_ssh_test.rb

Edit2: I reworked the C function and got a Ruby fallback working although I'm sure it will need to rescue SSLWaitReadable in more places. Also, it seems like the SSL layer in Ruby should be optional as well but I don't know Ruby so am unsure of the proper way to do that.

Cheers! Mike

michael-grunder commented 5 years ago

If people are actually interested in this feature, I'm happy to formalize the API (at least with respect to the C extension).

I don't know Ruby, so I have no idea what the "Rubyist" way to do things is.

sj26 commented 4 years ago

Yes please! I'm happy to help with the ruby side of things.

brian-kephart commented 4 years ago

Just popping in to note that hiredis recently cut a 1.0 release with SSL support (previously support was merged but unreleased).

kimyu92 commented 4 years ago

@michael-grunder possible 1.0 update with ssl support?

johnnagro commented 3 years ago

:+1:

cmcinnes-mdsol commented 3 years ago

Just chiming in to say I'm really interested in this feature and also happy to help out with the ruby side. I see @artygus opened https://github.com/redis/hiredis-rb/pull/69 to bump hiredis to 1.0.0 which has ssl support.

stanhu commented 2 years ago

Some work towards this:

  1. https://github.com/redis/hiredis-rb/pull/87
  2. https://github.com/redis/hiredis/pull/1085
stanhu commented 2 years ago

It seems redis-client already supports SSL with hiredis: https://github.com/redis-rb/redis-client/blob/master/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c

I wonder if we should just deprecate this library in favor of that.

kimyu92 commented 2 years ago

@michael-grunder @stanhu Any insight on what is the ETA for v1 release that supports ssl?

michael-grunder commented 2 years ago

I can rebase the changes in my fork and get them merged but the underlying issue is just that I have basically no knowledge of ruby, so am not certain about the correctness of my changes.

I suppose I can get them merged and then people can test them before an actual release.

kimyu92 commented 2 years ago

@michael-grunder Why not we have some rc release for testing. I believe many of us will certainly try to provide feedback if necessary.

I think we also need to reopen https://github.com/redis/hiredis-rb/pull/87 and merge it

For those who uses Rails, I assume we can't simply swap out redis-rb with redis-client and uses hiredis-client instead.

stanhu commented 2 years ago

I'm waiting for https://github.com/redis/hiredis/pull/1085 to be merged.

I also stopped working on #87 because I think we should deprecate this gem and use redis-client (https://github.com/redis-rb/redis-client/blob/master/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c), which fixes all the issues I ran into with this gem:

  1. Supports SSL.
  2. Doesn't seg fault on peer SSL verification failures.
  3. Supports Ruby garbage compaction.

Plus, Sidekiq 7 already natively supports it, and redis-rb is going to start using it.

kimyu92 commented 2 years ago

Sidekiq supports it since v6.5 as beta feature but Actioncable of Rails still does not support the adapter out of the box

https://github.com/rails/rails/blame/main/actioncable/lib/action_cable/subscription_adapter/redis.rb

So, it would be awhile before we can deprecate this gem 🙊

stanhu commented 2 years ago

Ok, I may continue working on #87 since hiredis-client also mandates Redis 6, which may be an issue for some legacy clients.

I'd like to see https://github.com/redis/hiredis/pull/1085 merged and released, though.

stanhu commented 2 years ago

I should also mentioned that #87 requires a change in redis-rb, so that may also block adoption.

byroot commented 2 years ago

So, it would be awhile before we can deprecate this gem 🙊

I'm currently working on redis-rb 5.0, which will use redis-client under the hood. Once it's done hiredis-rb won't be relevant anymore.

Feel free to work on all that, but I figured I'd give you a heads up.

Ref: https://github.com/redis/redis-rb/pull/1120