Support ssl_verify_peer with wss.

[authorNari]

EM::Connection should implements a .ssl_verify_peer method when you use a verify_peer: true option.
http://www.rubydoc.info/github/eventmachine/eventmachine/EventMachine/Connection#ssl_verify_peer-instance_method
So I implement these code.

• add :trust_ca option
• implement .ssl_verify_peer on Connection

[SpComb]

I suspect that correctly implementing SSL cert validation via this EM ssl_verify_peer interface is kinda impossible, without EM changes to properly support cert chain validation, but the PR for that has been open since 2012 😱 eventmachine/eventmachine#378

[SpComb]

I didn't test this code yet, but immediate problems that I see with this:

• The EM OpenSSL verify callback ignores the OpenSSL cert preverify errors, i.e. it doesn't care if the server-supplied cert and private key do not match? ssl_verify_peer() not called on CA mismatch of cert and key eventmachine/eventmachine#275 https://github.com/eventmachine/eventmachine/blob/v1.2.3/ext/ssl.cpp#L589
• This callback gets called with each individual cert in the chain that the server sends? How will it work if the server sends an intermediate CA cert that's not in the cert store?
• This doesn't actually validate the depth=0 cert's CN against the URI host, so it will accept any valid cert for any domain?
• No cert validity (NotBefore/NotAfter) checks etc.

[jcoglan]

@authorNari Apologies it's taken me so long to respond to this; it's been a bit of a rough year so far. My initial gut reaction to this, which was confirmed by @SpComb's comments, is that I don't have the domain expertise to maintain SSL-related implementation and I'd rather not have it be baked into my libraries. I'm fine with passing options off to an underlying I/O system we're relying on (e.g. if EventMachine handled this) or, I could provide some hooks in the API for you to inject these checks yourself.

Is there an API design that we could add to this library that would let you add this SSL code into the connection process?

[SpComb]

Is there an API design that we could add to this library that would let you add this SSL code into the connection process?

AFAIK: no

I came to the conclusion that EM's SSL support is just fundamentally flawed, and there's no reasonable way to support SSL cert verification (client or server). It just doesn't expose the necessary information from libssl, so while you can make some crude attempts at trying to whitelist some specific certificates, it's never going work correctly without changes to EM (eventmachine/eventmachine#378).

We ended up getting rid of EM on the client side, and writing our own websocket client library with faye/websocket-driver-ruby and ruby stdlib OpenSSL sockets, which does support SSL certificate validation.

We still use EM on the server side, but that's with haproxy handling the SSL termination and proxying the websocket connections.

(EDIT: use more considerate terminology)

[jcoglan]

I suspect @SpComb's comments are how we should resolve this. If you have issues at the transport layer, then either:

• It's a bug in our integration with EventMachine
• Or, the problem is out of scope and you should integrate your I/O solution with websocket-driver

[jcoglan]

To clarify: given it looks like EventMachine does not support the functionality you want, then I consider it out of scope for this library and you would be better served integration a good TLS implementation with websocket-driver.

[jcoglan]

@SpComb I'd also be very grateful if you could avoid using the word 'sane' to refer to software designs you find agreeable. There are usually better terms you could use to refer more specifically to the problem at hand.

[sodabrew]

Hi! Maintainer of EventMachine here. I'd love to improve our TLS support, and whatever else I can do to support this project. Note that while I don't have a ton of time, I am always happy to review PRs and shepherd out EM releases.

[SpComb]

Hi @sodabrew,

I'm not entirely sure if this is the right PR/issue to discuss this on, but I can briefly summarize what EM support I think would be required for implementing SSL clients with server certificate verification,

Listed in terms of the exposed libssl APIs:

• SSL_CTX_load_verify_locations Supported in Enhance ssl verification to make hostname matching possible eventmachine/eventmachine#378 by extending EventMachine::set_tls_parms

• SSL_CTX_set_verify with SSL_VERIFY_PEER: already supported in EM: https://github.com/eventmachine/eventmachine/blob/bc9e2faefafb7c13c3b4baeacb9763878a26a348/ext/ssl.cpp#L341-L346

• The SSL_set_verify(..., ssl_verify_wrapper) callback MUST return false by default if called with !preverify_ok.

  The ssl_verify_wrapper ignoring the preverify_ok parameter is the most blatantly broken part of the implementation, because this effectively bypasses all of the libssl certificate validation logic 👿

  Based on my reading of the docs and issues like ssl_verify_peer() not called on CA mismatch of cert and key eventmachine/eventmachine#275, I suspect this even includes very fundamental things like "the private key used to sign the session key matches the public key in the certificate".

• SSL_get_peer_certificate: already supported in https://github.com/eventmachine/eventmachine/blob/bc9e2faefafb7c13c3b4baeacb9763878a26a348/ext/ssl.cpp#L532-L540 ?

I think the SSL_CTX_load_verify_locations + preverify_ok changes would be the bare minimum that would be required. These also match the changes dicussed/implemented in eventmachine/eventmachine#378

Additional bonus points for:

• SSL_get_verify_result + X509_verify_cert_error_string to allow the application to report more useful error messages than just "certificate verification failed"

• Some convenience wrapper for the cert subject/hostname validation - ideally there should be some kind of secure: true/false boolean that doesn't require each client developer to research and write their own certificate verification wrappers for the vast majority of usecases

  I think Ruby's OpenSSL::SSL.verify_certificate_identity can probably be used by applications together with the SSL_get_peer_certificate API, so it doesn't necessarily need to be part of EM itself.

[jcoglan]

If this is going to turn into discussing changes to EventMachine, I'd appreciate it if a thread could be opened on that project rather than being continued here. At such time as an acceptable API exists in EM I'm happy to take another look.

[jcoglan]

Thanks to @sodabrew for chiming in and offering help :)

[parhs]

However it is open to MITM attack so it is no secure at all.

[jcoglan]

@authorNari @sodabrew @SpComb we may now be able to move this issue forward. Last month, em-http-request took steps to address this same issue, prompting a discussion about how the Faye client should respond. Faye uses em-http-request and faye-websocket-ruby to talk to servers in its Ruby client implementation, so now that em-http-request is performing peer verification and has an implementation for it, we can look at fixing faye-websocket and Faye in the same way. Further details and discussion are in faye/faye#524.

I have opened #129 as an attempt to adapt the em-http-request solution to this package. I would appreciate a review, esp from @SpComb as you seem to know a lot about SSL. I am not very well-versed in the details and am going off code others have written. In terms of functionality, and how it differs from this PR:

• It either uses set_default_paths, or loads CA certificates from a user-supplied file, not both. This mirrors the behaviour of Node.js, which this project also supports.
• Each certificate received from the server is added to the cert store, whereas in this PR only certs already set up by the client can be used to verify server certificates. My PR allows the server to send a chain where later certs are verified by earlier ones.
• Once all certificates have been received, the last one has its hostname checked using OpenSSL::SSL.verify_certificate_identity, using the hostname from the request URI.

That last point means it performs additional verification to what's done here, but I don't know what other functionality it still lacks that we might be able to add.

I would add that I would still much rather that EventMachine integrated SSL verification into start_tls rather than making users implement it, since I would rather not be responsible for this code. However, if I can get my PR adequately reviewed, then I'd be happy to ship it and make this package safer by default.

[jcoglan]

This has now been superseded by the merging of #129.