OpenSSL additions + HTTP::Client support for more ssl options #2343
Conversation
- Added LibSSL functions for CA and certificate files - Added OpenSSL::SSL::Method from @datanoise with added SSLv3 and TLSv1 - Added ssl methods to HTTP::Client for specifying: - CA file path - Private key file path - Certificate file path - SSL method to use
|
Thank you for this! We'll probably take a look at this the next week. (Just to let you know that we saw this, just didn't have time yet because of the new releases :-)) |
|
What's happening with this? I'd be happy to rebase to fix the potential merge conflicts. However, I won't put the time if this won't be accepted. I'm not expecting this to be accepted as-is, I'm trying to raise de awareness of the OpenSSL issues in the standard library. SSL is a very important feature. |
| cl.ssl_method = OpenSSL::SSL::Method::TLSv1 | ||
| cl.ssl_cert_file = "/path/to/cert" | ||
| cl.ssl_key_file = "/path/to/key" | ||
| cl.ssl_ca_file = "/path/to/ca" |
There was a problem hiding this comment.
I think this calls for a ssl accessor. Or just having: Client.new(uri, ssl_context: ctx) that can then be tweaked through client.ssl_context?
There was a problem hiding this comment.
One that allows passing an OpenSSL::SSL::Context to use for requests? I think that would make sense. I have that in my experimental docker lib: https://github.com/jeromegn/docker.cr/blob/master/src/core_ext/http/client.cr#L3-L5
|
The current state of OpenSSL integration is very basic. It works, but should be improved, mostly to comply with better security and options. Please see https://github.com/ysbaddaden/http2/blob/master/src/core_ext/openssl.cr for the HTTP server counterpart needed for HTTP2. It's awaiting a proper pull request. BTW: shouldn't we default to TLS 1.2, instead of the long deprecated SSL2 and SSL3 ? |
| @@ -0,0 +1,18 @@ | |||
| enum OpenSSL::SSL::Method | |||
There was a problem hiding this comment.
I"m not sure we need an enum. LibSSL::SSLMethod is already a type. Also, there are a lot more methods in OpenSSL, thought I'm sure why there are so many.
|
@jeromegn Sorry, didn't have time, and I don't know much about OpenSSL to decide what's good or bad. @ysbaddaden Since you are reviewing this, if you find these additions/changes OK, and possibly others that come in the future, you are free to merge them even without my consent because, well, you seem to know this subject very well :-) |
|
If only I knew OpenSSL very well 😭 As far as I understand: we should default to a TLS1 handshake. SSL2 is long deprecated, and SSL3 is deprecated too. I believe only TLS 1.2 is really secure, but TLS1 is still acceptable. |
|
I also believe we should not default to a deprecated SSL method :) That might fix most issues. I'm by no means an expert with OpenSSL either. I'm learning though. I opened this to fix a very specific issue with my own library. @ysbaddaden regarding the |
| @@ -41,4 +44,7 @@ lib LibSSL | |||
| fun ssl_ctx_use_certificate_chain_file = SSL_CTX_use_certificate_chain_file(ctx : SSLContext, file : UInt8*) : Int | |||
| fun ssl_ctx_use_privatekey_file = SSL_CTX_use_PrivateKey_file(ctx : SSLContext, file : UInt8*, filetype : SSLFileType) : Int | |||
| fun ssl_set_bio = SSL_set_bio(handle : SSL, rbio : LibCrypto::Bio*, wbio : LibCrypto::Bio*) | |||
| fun ssl_ctx_load_verify_locations = SSL_CTX_load_verify_locations(ctx: SSLContext, ca_file: UInt8*, ca_path: UInt8*) : Int32 | |||
| fun ssl_ctx_use_certificate_file = SSL_CTX_use_certificate_file(ctx: SSLContext, file: UInt8*, type: Int32) : Int32 | |||
There was a problem hiding this comment.
No particular reason. It's been copy pasted.
|
This code contains a lot of copy/paste from @datanoise's datanoise/openssl.cr library. I'm not particularly well-versed in SSL stuff. I needed parts of it for my own use (that docker library,) but I only took enough to make it work. I think Crystal needs to seriously consider a more robust SSL offering. I'm happy if anybody wants to tackle this. I'm pretty sure I'm not the person that should do it :) |
|
Closing in favor to #2671 (merged) and #2689 (soon to be merged) that are doing just that: always validate the certificates, propose good defaults (good ciphers, only enable TLS protocols), ability to set CA certificates, etc. The approach will be a little different. Instead of having properties for the certificates, we prefer to build a There is no Enum for LibSSL::SSLMethod because we (almost) always want the |
OpenSSL::SSL::MethodonOpenSSL::SSL::Contextconstructorssl_methods to HTTP::Client for specifying:I'm in need of these changes to SSL for my Docker API client (https://github.com/jeromegn/docker.cr). For now, I've extended the various libs and classes within the shard, but I think these small changes could be beneficial to a lot of people.
I'm not entirely sure how to test these, for now I've just tested that it's possible to set ssl options on the
HTTP::Clientand since OpenSSL was vastly untested, I didn't bother adding more there.I did some manual testing, with a slightly modified version of my shard and using this version of crystal's binaries and it worked.
I inspired from Ruby's Net::HTTP methods for ssl, but I don't think they were quite right and I don't think this syntax I created is quite right either. I was debating adding a lot of initialization options, but then I would've had to add them on every method initializing or calling an initializer (there are 3 of them, that got quite verbose.) So I decided to let users set the SSL options after initialization.
I also had to explicitly
require "openssl"inHTTP::Clientdue to the property. This could probably be avoided by not specifying the type there.My goal is to make this mergeable, so if there's anything, please let me know and I'll be happy to discuss and fix :)