Setting SSLContext still results in TrustEverythingTrustManager warning

[welsh]

When creating a connection using the RMQConnectionFactory, if you have passed your own SSLContext you will still get the warning:

14:50:54.068 [main] WARN com.rabbitmq.client.TrustEverythingTrustManager - SECURITY ALERT: this trust manager trusts every certificate, effectively disabling peer verification. This is convenient for local development but offers no protection against man-in-the-middle attacks. Please see https://www.rabbitmq.com/ssl.html to learn more about peer certificate verification.

Which is not accurate and was initially throwing me off as I was expecting it to either use:

1. My own TrustManager specified via config
2. The systems default TrustManager

And when I ended up digging into it, I did find it was actually working properly contrary to the warning message that was being logged. (Which was rather annoying because I wasn't sure if it was my own trust manager, the system or the TrustEverythingTrustManager that was making it work)

It looks like this is because when you create a connection you call:

setRabbitUri(logger, this, cf, getUri());

Which then calls:

com.rabbitmq.client.ConnectionFactory#setUri(String uriString)

And due to support the amQPs scheme and then Don't blindly overwrite SSLContext when specifying URI it will initialize the default TrustEverythingTrustManager, but then we override that when you subsequently call:

maybeEnableTLS(cf);

So, I believe this warning can be cleaned up by swapping those two lines (As the sslContext will properly be set by then), and then potentially adding a log / comment to:

RMQConnectionFactory#setUri(String uriString)

Indicating that if they can ignore the warnings generated from this as to not cause confusion as this can cause it to print out twice if your Uri starts with amqps.

I mean ideally, it would be great if the rabbitmq-java-client didn't by default set that TrustEverythingTrustManager but thats another story...

[michaelklishin]

Most users probably would expect the same thing: that the trust manager provided in the config would be used. Please submit a PR.

It has been explained multiple times why changing the default trust manager implementation in the Java client is not an easy decision. We wish we could switch to something else without consequences. Unfortunately even after years of improving RabbitMQ TLS guide we see over and over again that a lot of developers have very little clue as to what all those certificates are and what the heck is certificate chain verification. Guess who they take their questions and complaints to and how much they are willing to pay for that time.

[welsh]

PR submitted and I do understand why the decision was made the way it was, and I approve of the warning to be there if you are indeed using the TrustEverythingTrustManager however it just makes sense that it shouldn't appear if you aren't actually using it.

It's mainly for when support looks at the logging output of the application, it will prompt questions about why am I not doing it securely when in fact I am.

[michaelklishin]

@welsh I understand how that's very annoying and can attract a lot of scrutiny in some environments. @acogoluegnes is away for a few days but I'll try to QA your PR shortly.

[acogoluegnes]

Ironically this reverses a previous fix made obsolete by the change in the Java client @welsh mentioned.

[acogoluegnes]

Fixed in #75.