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 verify_mode option to verify client certs #37

Closed

Conversation

joemiller
Copy link
Contributor

Adds verify_mode option that can be used to authenticate client TLs
certificates.

Options: peer, force_peer, none.

  • peer: Configures https server to request client certificate, but not
    require it.
  • force_peer: Require client certificate to be trusted by one of the CA's in
    the keystore.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@ph
Copy link
Contributor

ph commented Feb 26, 2016

Hello @joemiller Would you mind rebasing this PR?

I think this will also fixe this issue #46 that might be related to the JRUBY changes that got merged in 2.16 ?:)

@ph
Copy link
Contributor

ph commented Feb 26, 2016

nevermind since this is a blocker I will rebase it and test it.

@joemiller
Copy link
Contributor Author

I had it on my list to do this weekend or next week since puma accepted the
patch to support certification verification when run in jruby yesterday -
puma/puma#833 - but if you would like to do it
first, that would be awesome!!!

On Fri, Feb 26, 2016 at 11:35 AM Pier-Hugues Pellerin <
notifications@github.com> wrote:

nevermind since this is a blocker I will rebase it and test it.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@joemiller joemiller force-pushed the client-auth-verify_mode branch 3 times, most recently from 7774b86 to 1b041bc Compare February 27, 2016 22:04
@joemiller
Copy link
Contributor Author

rebased against master, passed unit tests. Note I bumped puma to 3.x which should include the patch to honor the verify_mode flags.

@ph
Copy link
Contributor

ph commented Feb 29, 2016

@joemiller 2.16 should include the patch too, we are about to ship a web api with the next version of logstash I would prefer if we stick with 2.X. until 3.0 is released for some time ;)

https://github.com/puma/puma/blob/v2.16.0/History.txt#L11

@joemiller
Copy link
Contributor Author

I agree. I did not see that 2.16.0 would also have the patch =) I will
adjust the PR.

On Mon, Feb 29, 2016 at 6:54 AM Pier-Hugues Pellerin <
notifications@github.com> wrote:

@joemiller https://github.com/joemiller 2.16 should include the patch
too, we are about to ship a web api with the next version of logstash I
would prefer if we stick with 2.X. until 3.0 is released for some time ;)

https://github.com/puma/puma/blob/v2.16.0/History.txt#L11


Reply to this email directly or view it on GitHub
#37 (comment)
.

@joemiller
Copy link
Contributor Author

@ph adjusted puma dep to 2.16

@@ -69,6 +69,9 @@ class LogStash::Inputs::Http < LogStash::Inputs::Base
# Set the truststore password
config :keystore_password, :validate => :password

# Set the client certificate verification method. Valid methods: none, peer, force_peer
config :verify_mode, :validate => :string, :default => 'none'
Copy link
Member

Choose a reason for hiding this comment

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

thankfully the config dsl in logstash allows us to specify a short list of possible values, so we can validate the method setting here instead of checking else in line 109/110. what do you think?

suggestion:
config :verify_mode, :validate => ['none', 'peer', 'force_peer'], :default => 'none'

Copy link
Contributor

Choose a reason for hiding this comment

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

oops I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can make this change.

@ph
Copy link
Contributor

ph commented Mar 4, 2016

@jsvd I tested it manually and it fixed the issues people were encountering in #46

I have also created this #47
So we can have more integration tests concerning SSL scenarios.

@elasticsearch-bot
Copy link

João Duarte merged this into the following branches!

Branch Commits
master b7506fe

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.

None yet

5 participants