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.

Sign up for GitHub

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

Jump to bottom

Add verify_cert option #13

Closed
wants to merge 2 commits into from
Closed

Add verify_cert option #13

wants to merge 2 commits into from

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Aug 5, 2015

Fixes #12

suyograo
suyograo reviewed Aug 5, 2015
View reviewed changes
lib/logstash/plugin_mixins/http_client.rb
# If you need to use a custom X.509 CA (`.pem` certs) specify the path to that here
# Set this to false to disable SSL/TLS certificate validation
# Note: setting this to false is generally considered insecure!
config :verify_cert, :validate => :boolean, :default => true
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts about naming this :ssl_certificate_verification. Just so we can be consistent with es-output https://github.com/logstash-plugins/logstash-output-elasticsearch/blob/master/lib/logstash/outputs/elasticsearch.rb#L282

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@suyograo makes sense, will do (though I'll point out that SSL has been replaced by TLS at this point).

@suyograo
Copy link
Contributor

suyograo commented Aug 5, 2015

Is there a way we could warn (to console) the user when this option is used? May be in register ?

@andrewvc
Copy link
Contributor Author

andrewvc commented Aug 5, 2015

@suyograo I don't have access to #register from this since its a mixin. I personally think its fine not to warn since it is something the user has to explicitly set. What's the use case you're envisioning here?

@suyograo
Copy link
Contributor

suyograo commented Aug 5, 2015

@andrewvc i'm cool with that 👍

@andrewvc
Copy link
Contributor Author

andrewvc commented Aug 5, 2015

@suyograo I just fixed the issue with inconsistent parameter names. Looking better?

@suyograo
Copy link
Contributor

suyograo commented Aug 5, 2015

LGTM

@andrewvc
Copy link
Contributor Author

andrewvc commented Aug 5, 2015

Thanks @suyograo !

@elasticsearch-bot
Copy link

Merged sucessfully into master!

@andrewvc andrewvc closed this in c7106be Aug 5, 2015
andrewvc added a commit that referenced this pull request Aug 5, 2015
@andrewvc @jordansissel
Use a config option for SSL that's consistent with the ES output
c121c2a 
Fixes #13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewvc @suyograo @elasticsearch-bot