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

Add possibility to establish SSL connection with SSL_VERIFY_NONE #72

Closed
wants to merge 5 commits into from
Closed

Add possibility to establish SSL connection with SSL_VERIFY_NONE #72

wants to merge 5 commits into from

Conversation

mtuleika-appcast
Copy link

This PR adds possibility to establish SSL connection to Redis with SSL_VERIFY_NONE(in my case to ElastiCache).

What was done:

  • hiredis submodule was bumped to version 1.0.0
  • C extension in hiredis-rb was extended to support SSL connection without certification
require 'hiredis'

conn = Hiredis::Connection.new
conn.connect_ssl('<host>', 6379)

conn.write %w[AUTH <password>]
puts conn.read
conn.write %w[SET speed awesome]
puts conn.read
conn.write %w[GET speed]
puts conn.read

@ssinghi
Copy link

ssinghi commented Sep 26, 2021

I think first PR #69 can be merged and then the ssl support added.

@milieu
Copy link

milieu commented Feb 16, 2022

Well PR #69 has been merged...

@ayoul3
Copy link

ayoul3 commented Mar 23, 2022

hey @ssinghi and team, would love for this to be merged if/when possible !

$LDFLAGS << " #{hiredis_dir}/libhiredis.a"
$LDFLAGS << " #{hiredis_dir}/libhiredis_ssl.a"
$LDFLAGS << " -L/usr/local/opt/openssl/lib"
Copy link
Contributor

@stanhu stanhu Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can assume that OpenSSL is here. We may want to use pkg_config to determine the LDFLAGS and CFLAGS.


redisInitOpenSSL();
ssl = redisCreateSSLContext(NULL, NULL, NULL, NULL, StringValuePtr(arg_host), &ssl_error);
SSL_CTX_set_verify(ssl->ssl_ctx, SSL_VERIFY_NONE, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think SSL_VERIFY_NONE should be the default. I'd say that the default should be verify is enabled, but you may pass a verify option.

@engwan
Copy link

engwan commented Aug 6, 2022

@stanhu I also saw https://github.com/michael-grunder/hiredis-rb/commits/ssl-support which is also an attempt to add SSL support.

I saw it has some code to conditionally link OpenSSL: michael-grunder@a9897ec#diff-573a6b1ffeebb872ab4255105affc087786e9e64b583ebcf28bca619c0a1227aR48-R49

@@ -2,6 +2,14 @@
#include <errno.h>
#include "hiredis_ext.h"

struct redisSSLContext {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of an ugly hack since it's reaching inside the hiredis internals to get this information.

I think we're better off modifying hiredis to support the setting of verify mode: redis/hiredis#1085

@YegorZdanovich
Copy link

@stanhu @ssinghi
@mtuleika-appcast doesn't work on ruby projects anymore, maybe someone can continue this PR

@appcast-inc appcast-inc closed this by deleting the head repository Dec 26, 2023
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

Successfully merging this pull request may close these issues.

8 participants