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

Implement SSL certificate verification #129

Merged
merged 2 commits into from Jul 31, 2020
Merged

Implement SSL certificate verification #129

merged 2 commits into from Jul 31, 2020

Conversation

jcoglan
Copy link
Collaborator

@jcoglan jcoglan commented Jul 19, 2020

EventMachine supports making TCP connections via the
EM.connect method. Once the connection is established, the
client can initiate a TLS session over the socket by calling
EM::Connection#start_tls. This method does not verify the
server'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 the
caller 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:

  • The caller can set the option tls.verify_peer, which defaults to true
  • The caller can set the option tls.cert_authority_file, a path to a file
    containing a set of certificate authorities to use in place of the system
    default set
  • When connecting through a proxy, only verifying the origin server's identity
    is supported

We have considered that enabling verify_peer by default may be a breaking
change, and considered two situations where this might apply:

  • The client is not talking to the server it thinks it is, because verification
    is turned off and the client is being attacked
  • The client is intentionally talking to a server that the system CAs would not
    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 their
server'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 if cert_authority_file is
set. As in the Faraday implementation, I have split the functionality between
the ssl_verify_peer and ssl_handshake_completed callbacks. I am not
completely sure why this done, but my assumption is that as ssl_verify_peer is
called 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 know
all 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.

@jcoglan jcoglan force-pushed the ssl-verification branch 2 times, most recently from 81f4801 to 74de500 Compare Jul 20, 2020
@jcoglan jcoglan force-pushed the ssl-verification branch 3 times, most recently from eb2cccb to c92cd0c Compare Jul 20, 2020
@jcoglan jcoglan merged commit f41e67c into master Jul 31, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant