Enhance ssl verification to make hostname matching possible

[jtulley]

A critical part of SSL verification is matching the host name of
the certificate, and this is not done automatically by the
OpenSSL libraries. Also, simply calling ssl_verify_peer with
no context but the certificate means that the ruby code needs to
fully reimplement a certificate store and validation. Even then,
there is not enough information to know when to assert the host
name match.

This commit adds "ca-cert-file", and "ca-path", very similar to
the standard libcurl way of handing ca certificates. The current
cert-chain-file is very insufficient since it can only contain
one chain. This patch does not get rid of that, but merely adds
these two new options, and uses them in a call to
SSL_Ctx_load_verify_locations.

Also, ssl_verify_peer is now passed the error code of the
OpenSSL verification that has already occurred, as well as the
certificate depth. At depth 0, the ruby code can do a domain /
host name check.

[jtulley]

I realize that there is already a similar pull request, #334 Mine has the following differences:

1. With mine, 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.

2. 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.

The other pull request is superior in having eventmachine verify the host name. In that way, the developer does not end up skipping this critical step. But, I think the ca-cert-file, and ca-path options are better than bundling a cert file. Bundling a cert file is a non-starter, since certificates need to be maintained. Expiring certificates, revoked certificates, new trusted root certificates all would create a maintenance problem.

[jtulley]

I'd like to propose we merge the two, and I'd be willing to do that work if needed.

Besides my concern about pull 334's bundling of certs, I only saw one concern in the discussion on that change -- that was how to match the domain name. Are there reasons why the following would not be sufficient?:

if depth == 0
cert = OpenSSL::X509::Certificate.new(pem) rescue nil
return_val = OpenSSL::SSL.verify_certificate_identity(cert, @Domain) rescue false
end

This would rely on the domain / host name being passed in as per pull 334's technique. (my pull request leaves this completely up to ssl_verify_peer)

[tmm1]

This will break backwards compatibility with any code accepting only one argument.

[jtulley]

Yeah, that was definitely one concern of mine. On the other hand, ssl_verify_peer is pretty much useless at this point.

[jtulley]

Without any context, there is no way to make a correct determination in ssl_verify_peer, especially whether the domain name match (or lack of a match) is important enough to cause a connection failure. The error, and the depth level are the minimum items required to do this. The other patch, 334, does the host name check internally (which should be enhanced as ibc suggests), but there should still be more information passed to ssl_verify_peer.

[jtulley]

One other concern is ensuring that the calling code can figure out what went wrong. So, we'd still need to send enough information to ssl_verify_peer to make this possible.

[tmm1]

Yuck.

[jtulley]

Yeah, but the mechanism only allows passing one thing back, and this is certainly not sufficient in this case. Everything doesn't fit into a "one string and one string only" methodology. I'm open to alternatives, of course.

[jtulley]

Would it be better to change the event mechanism to support a "void *" and document this as an opaque pointer? Then have the code cast it to one or the other on the other side?
Or
Some sort of object hierarchy, once again with most operations ending up with an object that contains a char *, but others with the enhanced data fields. I don't like this very much, since it ends up being almost the same thing as the other objects, with reinterpret_casts or dynamic_casts there as well.

[ibc]

@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.

Regards.

[jtulley]

Is that OpenSSL verify_certificate_identity call not sufficient?
That might be an argument for the check to be completely done by ssl_verify_peer, like with my pull request. The downside being, of course, that this causes many to completely skip this check, resulting in no real SSL security.

[jtulley]

Iñaki, is the license of OverSIP such that we could just copy the validation code from there? Would that be sufficient if we did that, to still do the host checking internally to event machine? I'd still want to augment ssl_verify_peer since the current implementation does not give enough context, and also because sometimes that method will want to ignore host name mismatches. (Or, that could be yet another tls_option).

[ibc]

@jtulley OverSIP has MIT license so yes. But OverSIP implements procedures for validating a certificate following SIP protocol rules. Each application protocol may decide its own rules for inspecting the fields of a certificate. Please don't do that at low level in any way, that's up to the application on top of EventMachine. If you code a XMPP server on top of EM you need another way for inspecting the certificate fields, and so on.

Maybe EventMachine could validate the certificate by checking if it's correctly signed, not expired and so on, that could be a configurable option in start_tls. But please, again, don't try to include the CommonName or AltSubjectName fields of the certificate at C++ level (or within the EM code itself), that's UP to the application being coded on top of EventMachine.

[jtulley]

Ok. Well, the good news is that my pull request defers that validation to ssl_verify_peer, but simply provides enough context in that method via the error and depth arguments for that validation to be able to succeed. It was the other pull request that did internal host name validation.

So, that leaves Aman's concerns.

1. ssl_verify_peer backwards compatibility. I don't think there is any way around this. The current technique of only passing in the pem just isn't sufficient. Currently in ssl_verify_peer, you have no idea whether openssl has already validated the certificate, and you have no idea if you are at a certificate level at which host name validation is possible ("depth 0" certificates).

2. Passing that increased amount of information from ed.cpp's VerifySslPeer through the EventCallback mechanism to the event_callback method in rubymain.cpp. Currently this mechanism only allows for a const char* and a size. That had been sufficient for all events in the past but once you have to send more than one item through, it is no longer enough. Right now I'm creating a struct that has all the information, casting that to a const char* and then casting it back to the struct on the other side in the EM_SSL_VERIFY case. I'm open to suggestions on alternatives.

[jtulley]

Any further opinions on that last patch I uploaded, which contained an alternate way to pass information from VerifySslPeer to the event_callback EM_SSL_VERIFY case? I just added an additional integer field to the callbacks and the struct that is in use. Not a general case solution, but it solves this in a fairly clean way without any casts of a different structure to const char * and then back to that structure.

[rud]

So the competing PR #334 was closed but not merged. This PR was never merged either. Is this not still a problem with the SSL hostname verification?