Enforcing VERIFY_PEER on the client
#5
Conversation
ph
force-pushed
the
fix/clients-should-use-certificate
branch
from
759fb5f to
593355b
Compare
|
Small cleanups in the dependencies and how we specify the files in the gemspec. |
The ruby client did not enforce the validation of the certificate, opening possible man in the middle attacks on the client. This PR make sure that the `verify_mode` is set to `VERIFY_PEER` and add the certificate to the store for this specific connection. An integration test was added to validate this change. This change makes the ruby client handling of connection closer to the `logstash-forwarder` behavior, which does the verify peer per default. Fixes #4
ph
force-pushed
the
fix/clients-should-use-certificate
branch
from
593355b to
ea44add
Compare
| client = Lumberjack::Client.new(:port => port, | ||
| :host => host, | ||
| :addresses => host, | ||
| :ssl_certificate => certificate_file_crt) |
There was a problem hiding this comment.
no need for a ssl_key parameter on the client side?
There was a problem hiding this comment.
Well the client use the public key to encrypt which is available in the certificate and the server use the private key to decrypt it? The ssl_key was never a requirement in the logstash-output-lumberjack. But yes, the LSF has an optional key option in the config, I'll check it more.
There was a problem hiding this comment.
https://github.com/elastic/logstash-forwarder/blob/master/publisher1.go#L137
I'll check on the ruby side, I am +1 on having the same behavior, I need to investigate what it's actually do.
There was a problem hiding this comment.
http://www.hydrogen18.com/blog/your-own-pki-tls-golang.html A lot of nice information ;)
There was a problem hiding this comment.
we don't currently require a client certificate, it's optional.
|
Thanks @jsvd for the comment, I'll do a bit more thinking. |
| @@ -0,0 +1,57 @@ | |||
| # encoding: utf-8 | |||
| require "lib/lumberjack/client" | |||
There was a problem hiding this comment.
remove the 'lib/' part here
|
Added some inline comments. Tests pass ( Let's get expected-rejection cases added also (client connects to a server that is not trusted, etc) |
|
@jordansissel I have added another test to cover a not trusted server. |
|
updated with @jordansissel comments. |
|
Tests passing. |
Enforcing `VERIFY_PEER` on the client
The ruby client did not enforce the validation of the certificate,
opening possible man in the middle attacks on the client. This PR make sure that
the
verify_modeis set toVERIFY_PEERand add the certificate to the storefor this specific connection. An integration test was added to validate
this change.
This change makes the ruby client handling of connection closer to the
logstash-forwarderbehavior, which does the verify peer per default.Fixes #4