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

Make it easier to perform peer verification when TLS is enabled #394

Closed
michaelklishin opened this issue Aug 17, 2018 · 2 comments
Closed

Comments

@michaelklishin
Copy link
Member

michaelklishin commented Aug 17, 2018

Background

A discussion about a more secure TrustManager implementation used by default when ConnectionFactory#useSslProtocol() (the 0-arity version) is used has been going on for a few years now, see e.g. in #226. @acogoluegnes and I have some specific ideas that won't trip up beginners and open the flood gates of TLS configuration questions on our tiny team.

Certificate Validity Checks

Default TrustManager and any future versions should perform certificate validity checks. Most TLS implementations would reject expired certificates by default. This should not make it any harder for beginners to get started and IMO is a no-brainer improvement.

Default JDK Certificate Store Option

As suggested in #226, it is possible to use default JDK certificate store instead of a custom one.
Our research suggests that with a null TrustManager the default/installed security provider will be used by default. In any case, we could provide a convenient way to do this.

Server Hostname Verification using SAN (Subject Alternative Name) and CN (Common Name)

Peer verification means at least 2 different types of checks in practice. One common thing that's
now covered in an HTTPS RFC is verification of server certificate's' SAN and CN (as a fallback) against the target hostname.
As of JDK 7 there is a way to enable such checks without implementing a TrustManager. Also, before JDK 7 TrustManagers didn't have access to the TLS socket information. It remains to be
decided whether we want to provide a custom implementation for JDK 6 which RabbitMQ Java client 4.x still supports 😭.

Unfortunately the peer verification algorithm has subtle differences between implementations, which can greatly complicate things for the user and increase support load on our team.

Documentation Improvements

Some parts of the RabbitMQ TLS guide have been reviewed very recently but others haven't been touched for a few years. It's worth revisiting the parts that cover peer verification in Java.

@michaelklishin michaelklishin added this to the 4.9.0 milestone Aug 17, 2018
@michaelklishin michaelklishin changed the title More secure TrustManager implementations Make it easier to perform peer verification when TLS is enabled Aug 20, 2018
acogoluegnes added a commit that referenced this issue Aug 20, 2018
Hostname verification isn't performed by default in the TLS handshake,
this commit makes it easier to enable server hostname verification for
both blocking and non-blocking IO modes.

References #394
@acogoluegnes
Copy link
Contributor

acogoluegnes commented Aug 21, 2018

After some investigations, it appears that the TLS handshake performed by Java doesn't enforce hostname verification by default. Hostname verification must be explicitly enabled, so #398 provides a way to easily enable it (this isn't the default to preserve backward compatibility). This works for Java 7 and more, for Java 6, an extra hostname verification step must be added after the handshake.

Regarding server certificate validity, the check is performed by JDK TrustManager implementation.

@acogoluegnes acogoluegnes modified the milestones: 4.9.0, 4.8.0 Aug 21, 2018
acogoluegnes added a commit that referenced this issue Aug 21, 2018
Hostname verification isn't performed by default in the TLS handshake,
this commit makes it easier to enable server hostname verification for
both blocking and non-blocking IO modes.

References #394

(cherry picked from commit fcc3dbb)

Conflicts:
	src/main/java/com/rabbitmq/client/ConnectionFactory.java
	src/test/java/com/rabbitmq/client/test/ChannelRpcTimeoutIntegrationTest.java
acogoluegnes added a commit that referenced this issue Aug 21, 2018
This is a solution for Java 6. For Java 7 and more, setting the server
validation algorithm on the SSLParameters is more appropriate.

This commit tries to make the activation of hostname verification as
simpler as possible for the developer. The client detects it's running
on Java 6 and sets up the hostname verifier from the Commons
HttpClient project (it's still possible to provide a customer
HostnameVerifier). The client uses the SSLParameters solution if
possible.

References #394
acogoluegnes added a commit that referenced this issue Aug 22, 2018
Not costly to test both IO modes and makes iterating easier.

References #394
acogoluegnes added a commit to rabbitmq/rabbitmq-website that referenced this issue Aug 24, 2018
acogoluegnes added a commit that referenced this issue Aug 24, 2018
@michaelklishin
Copy link
Member Author

While working on this issue we've discovered that hostname verification doesn't play very well with #138 since hostnames are resolved to IP addresses and x509 certificates usually are not issued with IP addresses in subject alternative names.

We will disabled the resolved from #138 for TLS connections in 5.x and for all connections in 6.0. Note that the the resolver won't be deleted as we don't think there's anything wrong with it, it just doesn't play well with TLS with peer verification :(

acogoluegnes added a commit that referenced this issue Aug 27, 2018
acogoluegnes added a commit that referenced this issue Aug 27, 2018
Hostname verification isn't performed by default in the TLS handshake,
this commit makes it easier to enable server hostname verification for
both blocking and non-blocking IO modes.

References #394

(cherry picked from commit fcc3dbb)
acogoluegnes added a commit that referenced this issue Aug 27, 2018
Not costly to test both IO modes and makes iterating easier.

References #394

(cherry picked from commit 7da0fd3)
acogoluegnes added a commit that referenced this issue Aug 27, 2018
References #394

(cherry picked from commit b5079b4)
acogoluegnes added a commit that referenced this issue Aug 27, 2018
acogoluegnes added a commit that referenced this issue Aug 28, 2018
localhost is now part of the SAN of the generated server certificate, so
the hostname verification succeeds.

References #394

(cherry picked from commit f73b80a)

Conflicts:
	src/test/java/com/rabbitmq/client/test/ssl/HostnameVerification.java
acogoluegnes added a commit that referenced this issue Aug 28, 2018
localhost is now part of the SAN of the generated server certificate, so
the hostname verification succeeds.

References #394

(cherry picked from commit f73b80a)
acogoluegnes added a commit that referenced this issue Aug 28, 2018
localhost is now part of the SAN of the generated server certificate, so
the hostname verification succeeds.

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

No branches or pull requests

2 participants