-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
SSL/TLS certificate verification disabled #352
Comments
There are several related issues: #146 and #197. Here are some interesting resources:
|
The fact that AHC works out of the box with self-signed certificates isn't a feature, it's a security bug. People who want their self-signed certificates to work should add them to their trust store (either programmatically, which I'm not sure is possible with AHC, or via There's a discussion on this problem here: http://security.stackexchange.com/questions/22965/what-is-the-potential-impact-of-these-ssl-certificate-validation-vulnerabilities Many developers using libraries like this assume that it has sensible security defaults. Unfortunately, it seems that AHC doesn't. This impacts a number of projects that uses this library. (To be honest, I don't use AHC myself, but I have noticed that an application I've used was able to connect to an URL that uses a certificate that wouldn't normally have been validated with my trust store.) |
I've just had a quick look at the fragment of code affected by the patch in issue #351. I think the expected default settings don't quite make sense.
Firstly, not specifying a keystore location is actually quite common, since, from a client point of view, the keystore is only used when using client certificates. There isn't even a default value for the keystore in the default JSSE settings (see JSSE Customization section). Assuming that when no client certificates are used, no server certificate validation should take places is a bid odd. Secondly, of course, assuming that, when no truststore location is specified, no server certificate validation should take place either (not even with the default JRE truststore/trustmanager) is certainly a default that will catch many library users by surprise. In addition, it's not always possible to find what the default trust manager uses (see: http://stackoverflow.com/questions/13657299/how-to-find-current-truststore-on-disk-programatically/13660336#13660336). |
Are you willing to contribute? I personally won't be able to work on this before several weeks at least. |
Hi AHC team, Thanks. |
I think the logic behind SSLUtils in creating only a single static SSLContext is incorrect, because it means that the system properties that were in effect at the time of creation are static. In reality, system properties can be changed after a program runs. In addition, Not only that, but SSLUtil relies on static methods for functionality. Even if the goal were to use a singleton, there's no way to reinitialize the method or override a static method without using reflection or Unsafe. Finally, getLooseSSLContext should not exist, at all. If people need to pass in a loose SSL context, they should do that by hand. |
I mostly agree, except regarding getLooseSSLContext that can perfectly sit there as a public helper. It's the one I'd use for Gatling, I don't really care about security there, I need raw performance. Do you think you could handle this? My hands are pretty full at the moment... |
Poking at it right now. |
@slandelle if you're using getLooseSSLContext for Gatling, then you're probably getting inaccurate numbers for your load tests, as adding / removing certificate verification is going to alter the CPU / memory times. I think it's much better to remove it -- the real pain isn't in TLS certificate verification, it's that creating certificates and sharing them is not something people know how to do. JEP 166 should let users set up multiple keystores like this: |
@slandelle PR submitted. |
Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves AsyncHttpClient#348
The SSL/TLS certificate verification is disabled. This is a security issue (which makes connections vulnerable to MITM attacks).
Here is a quick test that can be tested with a server that has a self-signed certificate or issued by a CA that's not in the default truststore.
URLConnection
will fail as expected (sun.security.validator.ValidatorException: PKIX path building failed
), whereasSimpleAsyncHttpClient
will work just fine.Please remove anything that has to do with the
TrustingSSLSocketFactory
incom.ning.http.client.providers.apache.TrustingSSLSocketFactory
.The text was updated successfully, but these errors were encountered: