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

hiredis now supports SSL #58

Open
tjwallace opened this issue Feb 27, 2019 · 21 comments
Open

hiredis now supports SSL #58

tjwallace opened this issue Feb 27, 2019 · 21 comments

Comments

@tjwallace
Copy link

redis/hiredis#645 - not sure if any changes need to be made here

@tjwallace tjwallace changed the title hiredis now supports ssl hiredis now supports SSL Feb 27, 2019
@degzcs
Copy link

degzcs commented Mar 6, 2019

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

@philipbjorge
Copy link

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
Copy link

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

@michael-grunder
Copy link
Collaborator

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
Copy link
Collaborator

michael-grunder commented Oct 15, 2019

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
Copy link
Collaborator

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
Copy link

sj26 commented Jan 30, 2020

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

@brian-kephart
Copy link

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

@kimyu92
Copy link

kimyu92 commented Oct 19, 2020

@michael-grunder possible 1.0 update with ssl support?

@johnnagro
Copy link

👍

@cmcinnes-mdsol
Copy link

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 #69 to bump hiredis to 1.0.0 which has ssl support.

@stanhu
Copy link
Contributor

stanhu commented Aug 10, 2022

@stanhu
Copy link
Contributor

stanhu commented Aug 10, 2022

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
Copy link

kimyu92 commented Aug 12, 2022

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

@michael-grunder
Copy link
Collaborator

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
Copy link

kimyu92 commented Aug 12, 2022

@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 #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
Copy link
Contributor

stanhu commented Aug 12, 2022

I'm waiting for redis/hiredis#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
Copy link

kimyu92 commented Aug 12, 2022

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
Copy link
Contributor

stanhu commented Aug 12, 2022

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 redis/hiredis#1085 merged and released, though.

@stanhu
Copy link
Contributor

stanhu commented Aug 12, 2022

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

@byroot
Copy link
Collaborator

byroot commented Aug 13, 2022

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: redis/redis-rb#1120

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