Additional options for SSL certificate verification #84

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants

As part of the project I'm working on, I added a couple of extra options to eventmachine evma_set_tls_parms for dealing with SSL certificate verification, to more easily allow our thin server to act as a client to a remote server over SSL.

I added an optional private key file password parameter that can be used to unlock a password protected private key file.

I also added an optional ca certificate file parameter that can be used to register additional CAs with SSL_CTX_load_verify_locations. The remote server we connect to has a certificate issued by our company CA which is not in a standard trust hierarchy, and I thought with such relatively simply verification requirements it would be better to handle this internally within eventmachine, rather than pushing the logic up to ruby's verify_peer every time, just to do the same thing. Ruby's verify_peer method is still called in this case, but our server doesn't do any additional verification.

If the implementation of these changes is acceptable, I thought they might make a useful addition.

This is the first time I've played around with eventmachine so apologies if this isn't the right approach to solving this problem - any more elegent solutions would be welcome! but I suppose at the very least, this pull request is a good way to articulate what I'm trying to achieve, and find the right solution to it.

Any feedback/suggestions are greatly appreciated,

Mike

Hello,

Any chance this change can be merged into master? It's really useful for passphrase-protected keyfiles, and has been working for us in production for 6 months. :-)

Thanks,
James

mbulat commented Sep 15, 2011

I'd like to second the request. We really need to use some passphrase protected key files, and I'd rather keep the mainline eventmachine in my gemfile than a patch or fork.

Thanks,
Mike

Contributor

ibc commented Nov 8, 2011

This would be really great!

Contributor

ibc commented Nov 8, 2011

Hi @miketheiron, I asked in eventmachine maillist about this pull request and Aman has replied:

"I am happy to merge that pull, but it has conflicts so I cannot use the merge button on github."

Maybe it's due to oother changes in the affected files. Would it be possible to refactor this pull request based on current master branch?

Contributor

ibc commented Nov 8, 2011

I will try to update the patch.

Contributor

ibc commented Nov 8, 2011

Hi, I've applied this pull request into my fork (updated to master branch today 11-11-08): https://github.com/ibc/eventmachine

It compiles and so, but fails in cases in which it worked. ssl_verify_peer(cert) is called never!

I've made a script that performs the TLS certificate(s) retrieval from a server. You can get it from here: http://public.aliax.net/check-remote-tls.rb

If I try my script with not patched eventmachine I get the expected output:

~# ./check-remote-tls.rb github.com 443

<< connection_completed

<< ssl_verify_peer => cert:

#<OpenSSL::X509::Certificate subject=/C=US/O=DigiCert
Inc/OU=www.digicert.com/CN=DigiCert High Assurance EV CA-1,
issuer=/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert High
Assurance EV Root CA, serial=11608356136354358657469651816937576901,
not_before=2006-11-10 00:00:00 UTC, not_after=2021-11-10 00:00:00 UTC>

<< ssl_verify_peer => cert:

#<OpenSSL::X509::Certificate subject=/businessCategory=Private
Organization/1.3.6.1.4.1.311.60.2.1.3=US/1.3.6.1.4.1.311.60.2.1.2=California/serialNumber=C3268102/C=US/ST=California/L=San
Francisco/O=GitHub, Inc./CN=github.com, issuer=/C=US/O=DigiCert
Inc/OU=www.digicert.com/CN=DigiCert High Assurance EV CA-1,
serial=19229479553765830569163959417678365365, not_before=2011-05-27
00:00:00 UTC, not_after=2013-07-29 12:00:00 UTC>

<< ssl_handshake_completed

However if I run the script with the pull request applied, I get this:

~# ./check-remote-tls.rb github.com 443

<< connection_completed

(no more). So ssl_verify_peer(cert) is never called.

Contributor

ibc commented Nov 8, 2011

Found the problem; it occurs in case I don't fill :ca_file in start_tls() !!!
Of course such field should NOT be mandatory!

Contributor

ibc commented Nov 8, 2011

By looking into ext/ssl.cpp it seems that some of the features of this pull request are just for TLS client mode. This is not good, they should be valid for both TLS client and server modes.

Contributor

ibc commented Nov 8, 2011

Ok, now I understand that the code in this pull request does not call ssl_verify_peer(cert) in case validation fails. I've added key password and ca_file also to SSL client and server side.

Contributor

ibc commented Nov 8, 2011

IMHO there should be a new callback EM::Connection#ssl_verification_failed(Fixnum error_code, String error_string).

Is there still interest in putting support for verifying SSL certs into EM? According to the current docs, "It is up to user defined code to perform a check on the certificates." However, after a discussion with some authors of em-http-request it seems that it is pretty much impossible for the user defined code to perform a proper SSL handshake.

I would love to help in any way I can on this, but most of the relevant code is in C, and my ability to use C is pretty limited.

Contributor

ibc commented Mar 31, 2012

Nothing will change in EventMachine at C++ level, never.

Hi, very sorry for not replying to this sooner, especially considering you've taken the time to investigate and comment on the patch. Annoyingly I'd used an old email address and forgot to check until recently.

This patch was a very noob attempt to integrate the features we wanted into em, and so I am more than happy to try and incorporate your very constructive feedback to make do it right.

A suggestion has been made to me from our side to perhaps split this into two separate pulls so that I'm not conflating two different changes:

  1. Support for pwd protected private key files
  2. Support for custom cafile, use of SSL_CTX_load_verify_locations to verify certificates against the cafile, and addition of the suggested EM::Connection#ssl_verification_failed(Fixnum error_code, String error_string) to make available any such errors

But I'm more than happy to break this down however you guys prefer.

Thanks for all your help, and constructive feedback, it's very much appreciated

Mike

@burke burke pushed a commit to burke/eventmachine that referenced this pull request May 22, 2012

Burke Libbey Merged #84. This improves support for providing credentials to OpenSSL
Conflicts:
	lib/em/connection.rb
ea3f730

burke commented May 24, 2012

I'm working on improving SSL support in eventmachine. I've merged these changes and made some other improvements.

In particular, the ssl_verify_wrapper can now check against an expected CN, and passes this result (pass or fail) to ruby's ssl_verify_peer if the method arity is > 1.

I still have some work to do on documentation, testing, and such, but would appreciate any feedback anyone has.

https://github.com/burke/eventmachine/blob/ssl_improvements/ext/ssl.cpp#L433

https://github.com/burke/eventmachine/commits/ssl_improvements

rud commented May 22, 2014

Poking this zombie issue - is this completely at a stalemate? Where have everybody gone with this in their projects?

burke commented May 22, 2014

We rewrote everything in Go. :)

@sodabrew sodabrew modified the milestone: v1.3.0 Dec 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment