SSL Peer Verification and other SSL improvements #334
Conversation
This will enable ruby to optionally override ssl_verify_peer to change the result, or to leave it and accept whatever OpenSSL decided. This will be more useful once EM is modified to accept real certificated and CAs, and it can intelligently determine this itself.
…ls to OpenSSL Conflicts: lib/em/connection.rb
This extracts an error-handling routine repeated 5 times into an inline function.
This is mostly a no-op, with two notable exceptions: 1) Before, a private key password would never be set unless a private key was also supplied. Now it will be. This should never really have any noticeable effect. 2) ca_file previously had no effect when supplied to a client connection. Now it works on either client or server.
This reduces the use of global variables a bit, and moves certs into a new directory called "certs". This also adds some certs for a test CA and a client signed by it. These will be used in soon-to-be-written tests.
This is now consistent with the existing file parameters to start_tls.
This hostname is passed in as a :hostname option to start_tls. It's stored on the SslBox_t, using ex_data. I'm not 100% using sk_X509_shift is the best way to go about getting the correct certificate to check against, but it seems to work in my test cases. I will shortly rewrite these test cases to not hit the network.
1) Raise an error if called with unsupported arguments 2) Default ca_file to the mozilla default CA file
|
One thing I would still like to add at some point is a message indicating to ruby why C-level verification failed. I haven't decided quite how to implement that yet. One potential option would be to add a third parameter to I'll think about this some more and do that in the future, but I don't think it should be a blocker for this pull request. |
If a server is using SNI, we need to add the hostname to the client SSL connection.
Previously, the code did not take into account subjectAltName extensions, multiple CN attributes, or wildcard domains.
|
HI, about:
That is a limitation. TLS certificates can include subjectAltName fields which take preference over the Subject CN, and there are protocol-dependant rules to inspect the value of those subjectAltName fields, so I don't think it's a good idea. For example check http://tools.ietf.org/html/rfc5922. |
|
I didn't implement any protocol-dependent rules, but I do check the subjectAltName: Hostname verification seemed like an important feature, and it can trivially be skipped by not passing a hostname. I realize it's not perfect. |
|
Hi @burke, please check my TLS inspection code in OverSIP (a SIP proxy on top of EM): https://github.com/versatica/OverSIP/blob/master/lib/oversip/tls.rb#L130 The method |
|
I've got a change set prepared which does almost exactly this. I should've looked here first. :) One big difference with mine, is that I allow you to pass in a full ca-cert file (not a cert chain, which is limited), and / or a capath. So, in this regard it is rather like libcurl or command-line curl. No bundling of certs. Then, I pass to ssl_verify_peer the pem(as usual), an error code (with "0" / X509_V_OK for succes), and a certificate depth, which is useful for the method to know when to do a domain name match. Then it is completely up to ssl_verify_peer to do the domain name match. I like yours because it is less likely that an implementer will miss that crucial step (as is common with OpenSSL-based implementations). I'll still submit mine, and I'm not at all clued into the EM community (if there is one), so I'm not sure how easy it is to get something into the code. Definitely this is a big hole in the current code that needs to be filled by your or my changes. |
|
I'm proposing that we merge this change with mine, keeping the internal host name validation (is using ruby's OpenSSL library, OpenSSL::SSL.verify_certificate_identity sufficient?) of this pull request, getting rid of the bundled certs (this is a big maintenance issue), and instead opting for my method of allowing you to specify a ca cert path, or a ca-cert file that has multiple (not chained, necessarily) certificates. Also, I'd like to keep the augmented ssl_verify_peer method, so that the ruby code has some context when adding on additional certificate validation through that method. |
|
Copy&paste of the same comment writen in #378: @jtulley, as I described here and here, the world is bigger than just HTTP. Comparing the certificate Common Name with a given domain is NOT enough in many cases, so please don't try to include such a limited and incorrect comparison at core level since it's the application layer the only that can decide how the certificate must be inspected for validating anything related to the underlying application protocol. |
The commit log is quite descriptive, but here are some highlights:
:hostnameargument is passed tostart_tls, verify that the Subject CN matches this value.preverify_okparameter tossl_verify_peer:ssl_verify_peernow returns preverify_ok, ie. the openssl verification result. This is normally the right thing to do: this method no longer requires overriding for security.preverify_okand determine validity on their own based on the still-passedcertparameter or any other criteria.ssl_verify_peerdefinitions will continue to work with no modifications.start_tls.Very open to discussion on this. Feel free to hit me up on email or gtalk at burke@burkelibbey.org.