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

Log a warning when TLS is enabled but peer verification is not #129

Open
michaelklishin opened this issue Dec 19, 2017 · 7 comments
Open
Labels

Comments

@michaelklishin
Copy link
Member

See the discussion with @andrewvc in #128. Bunny and a few other clients already do this.

@andrewvc
Copy link
Contributor

andrewvc commented Dec 19, 2017

@michaelklishin the java classes already do this now.

TBH, I think that it's very problematic that the setting tls: trueor tls: "TLSv1.2" turns on TLS without verification. If the setting was tls: "development" or something, sure, but it is reasonable to expect that given a boolean option this setting would actually enable full TLS.

It's a bit like a car where the ignition has two positions: "On", and "On2" where "On" doesn't enable the brakes.

@jordansissel
Copy link

I propose two changes:

  1. make "development" mode prompting yes/no to accept a server's certificate instead of blanket trusting everything. Prior art: SSH does this and is itself a wildly successful tool.
  2. Rename useSslProtocol(protocol) (which disables security by trusting everything) to be something more obviously-indicative of the dangerous behavior. TLS/SSL is too easy to misconfigure, so I strongly recommend renaming or eliminating this specific method.

@michaelklishin
Copy link
Member Author

michaelklishin commented Dec 20, 2017

@jordansissel this is a library, what would do the prompting? Or is it a Logstash feature? Likewise in 2, is it a Java client change? We certainly can rename the method to something like useSslProtocolUnsafelyWithoutPeerVerification in 4.5.0 and 5.2.0.

March Hare would have to emit its own warnings either way and at some point we'll probably have to revisit this decision (about only emitting a warning but not changing the default). I hope I explained why we haven't done this to date in #128.

@michaelklishin
Copy link
Member Author

@acogoluegnes FYI.

@michaelklishin
Copy link
Member Author

@andrewvc I was going to release March Hare 3.1.0 soon (possibly as soon as this issue is addressed) with Java client 4.4.1. We can bump the version to 4.0 next and consider some breaking changes around TLS options. I think they should be in a separate issue since it's the next step after this one.

What I'm trying to say is that I'm open to breaking changes in MH and even Java client but several helpful intermediate steps can be done as well and that's always my approach with libraries that are several years old.

@jordansissel
Copy link

@michaelklishin I am conflating issues, sorry.

My suggestion was unclear in scope and that my recommendation is to make it harder to disable trust, which includes not making it disabled by default (as the documentation does).

this is a library, what would do the prompting?

In Java, I do this with a callback into the X509TrustManager. Logstash can solve this by using the Java client directly and providing our own SSLContext (which we will do soon).

Intermediate steps are good for stability, so I have no complaints about that :)

As you said, developers generally have no knowledge of how to use TLS correctly, and I agree with you. I am working on a project (https://github.com/elastic/tealess) to improve this problem in Java, and when it's ready, I'd like to demo it to you as a possible default-alternative to the current trust-everything TrustManager available in the Java client today. No pressure.

@michaelklishin
Copy link
Member Author

Tealess looks awesome. OK, so let's emit a warning first in this issue and file a few others about renaming some options/Java client methods/making it easier to do the right thing without making it a breaking default immediately.

I am all for Logstash introducing more safety on top of what the clients do (if you are willing to answer all the x509 certificate chain verification questions :P).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants