Implement SSL certificate verification #129
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EventMachine supports making TCP connections via the
EM.connect
method. Once the connection is established, theclient can initiate a TLS session over the socket by calling
EM::Connection#start_tls
. This method does not verify theserver's identity by default, but it does accept an option named
verify_peer
.If this is set, the connection will invoke
EM::Connection#ssl_verify_peer
, a method supplied by thecaller which performs certificate validation. That is, EventMachine does not
implement such a validation routine itself; it requires the caller to supply
one.
This pull request implements such a validation routine using the OpenSSL module.
This implementation is derived from the one in Faraday, which
was adopted by em-http-request.
It changes the functionality of faye-websocket as follows:
tls.verify_peer
, which defaults totrue
tls.cert_authority_file
, a path to a filecontaining a set of certificate authorities to use in place of the system
default set
is supported
We have considered that enabling
verify_peer
by default may be a breakingchange, and considered two situations where this might apply:
is turned off and the client is being attacked
recognised, for example a server with a self-signed cert
We regard the first situation as a bug, not behaviour we intentionally support.
It's what this PR is designed to fix. We regard the second situation as a
possibly-breaking change but give such clients a means to get back to a working
state, either by setting the
tls.cert_authority_file
file to point to theirserver's certificate, or by switching off
tls.verify_peer
.In copying over the implementation from Faraday I have kept the same calls to
OpenSSL in place, with the addition of calling
OpenSSL::X509::Store#add_file
instead of
OpenSSL::X509::Store#set_default_paths
ifcert_authority_file
isset. As in the Faraday implementation, I have split the functionality between
the
ssl_verify_peer
andssl_handshake_completed
callbacks. I am notcompletely sure why this done, but my assumption is that as
ssl_verify_peer
iscalled in turn with every certificate in the server's chain, its only job is to
check that each new cert can be verified by the certs already in the chain,
forming a chain of trust from the client's trusted certs all the way down the
server's list of certificates. When
ssl_handshake_completed
is called, we knowall the certificates have now been processed and we need to verify the hostname
from the last one the server sent. I'd like some feedback on whether this is
correct and my approach is valid.
There is also the bigger question that this code needs reviewing and
maintaining, and I am not an expert on SSL/TLS, X509, or anything else that's
relevant. I feel that this functionality should be baked into EventMachine, or
at least have a default implementation, rather than EventMachine's users having
to write their own sections of important security code. I would like to know how
risky it is in practice for us to own this code, and whether we should push for
another approach. For example, we could request that EventMachine implement this
functionality, or we could gather support for a shared library that implements
it, where that library could be used by faye-websocket, em-http-request, and
anything else using EventMachine's TLS support.