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

Adding SSL Support #61

Merged
merged 3 commits into from Mar 21, 2018

Conversation

Projects
None yet
9 participants
@Matt-Lane
Copy link
Contributor

commented Oct 12, 2017

This pull request adds SSL support to the configuration with this option:
ssl: boolean, default false

Usage example:

input {
   redis {
   	host => "securehost-redis.example.org"
   	port => 6380
   	data_type => "list"
   	key => "keyname"
	ssl => true
   }
}

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

Adding SSL Support
Adding SSL support via configuration
@jordansissel

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2017

@Matt-Lane

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

Looks like while my pull request worked for my specific situation (connecting to a secure redis cache in Azure), without setting any additional params, that will probably not be true for everyone.
I can see in the docs:https://github.com/redis/redis-rb#ssltls-support that we'd need to pass in a hash of all the params. Would we need to individually verify each of the possible inputs to the SSLContext, or just accept a hash?
A note that I know exactly enough Ruby to have made this change, but not much more, so updates may be slow.

@jordansissel

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

@iremmats

This comment has been minimized.

Copy link

commented Oct 28, 2017

We are very interested in using this in production soon. Today we have to use stunnel to connect to azure redis with logstash.

@jstoja

This comment has been minimized.

Copy link

commented Jan 12, 2018

@jordansissel Any update on this?

@sstarcher

This comment has been minimized.

Copy link

commented Feb 14, 2018

Anything we can do to get this merged?

@CarlosSardo

This comment has been minimized.

Copy link

commented Mar 7, 2018

Hi @jordansissel ! Can we have this PR merged? According to travis-ci logs, build #73.4 passed! We're also experiencing the same, basically we want to reach Azure Redis via SSL.

Thank you!

@jstoja

This comment has been minimized.

Copy link

commented Mar 7, 2018

Maybe someone else from Elastic might be interested about this, perhaps @jsvd

@jsvd
Copy link
Contributor

left a comment

@Matt-Lane I think we should go the extra mile to provide a way to set the other ssl/tls related settings.
Looking at the documentation, a good starting point of supported options would be: cert, key, ca_file and verify_mode

@@ -29,6 +29,9 @@ module LogStash module Inputs class Redis < LogStash::Inputs::Threadable
# The port to connect on.
config :port, :validate => :number, :default => 6379

# SSL
config :ssl, :validate => :boolean, :default => false

This comment has been minimized.

Copy link
@jsvd

jsvd Mar 12, 2018

Contributor

this must be documented in the docs/index.asciidoc file, as that is the source of truth for the plugin's documentation

This comment has been minimized.

Copy link
@Matt-Lane

Matt-Lane Mar 15, 2018

Author Contributor

Added the requested documentation

This comment has been minimized.

Copy link
@Yariv-Hashai

@jsvd jsvd referenced this pull request Mar 13, 2018

Closed

adding ssl #65

@sstarcher

This comment has been minimized.

Copy link

commented Mar 15, 2018

@jsvd would it be acceptable to get this in without going the extra mile. As is it is already useful to people. Certainly the extra features could be added later.

@Matt-Lane any chance you would have time to update the documentation

@jsvd

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

I'm ok with not requiring other ssl settings, but it must have docs and at least a test that checks it's passing the right option to the redis library...
(and the PR currently needs to be rebased)

@sstarcher

This comment has been minimized.

Copy link

commented Mar 15, 2018

@jsvd fully agreed
@Matt-Lane If you don't think you will have time in the next week I can pick this up for you and add the docs and rebase it. Please let me know.

Matt-Lane added some commits Mar 15, 2018

@Matt-Lane

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2018

I added the documentation and I believe I've rebased this correctly. Please let me know if I did not, or if any other changes should be made.

@robinjoseph08

This comment has been minimized.

Copy link

commented Mar 20, 2018

@jsvd Any chance you could take another look? We'd love to be able to use this plugin instead of forking it to get this functionality. Thanks!

@jsvd

jsvd approved these changes Mar 21, 2018

Copy link
Contributor

left a comment

finally got around to test this manually. confirmed it works 👍
LGTM

@jsvd jsvd merged commit 52089f4 into logstash-plugins:master Mar 21, 2018

2 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Jrmcneil Jrmcneil referenced this pull request Apr 10, 2018

Open

Add ssl support #59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.