-
Notifications
You must be signed in to change notification settings - Fork 424
Add HTTP client options for SSL/TLS methods #530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,20 +14,31 @@ | |
boost::network::http::impl::ssl_delegate::ssl_delegate( | ||
asio::io_service &service, bool always_verify_peer, | ||
optional<std::string> certificate_filename, | ||
optional<std::string> verify_path, optional<std::string> certificate_file, | ||
optional<std::string> private_key_file) | ||
optional<std::string> verify_path, | ||
optional<std::string> certificate_file, | ||
optional<std::string> private_key_file, | ||
optional<std::string> ciphers, | ||
long ssl_options) | ||
: service_(service), | ||
certificate_filename_(certificate_filename), | ||
verify_path_(verify_path), | ||
certificate_file_(certificate_file), | ||
private_key_file_(private_key_file), | ||
ciphers_(ciphers), | ||
ssl_options_(ssl_options), | ||
always_verify_peer_(always_verify_peer) {} | ||
|
||
void boost::network::http::impl::ssl_delegate::connect( | ||
asio::ip::tcp::endpoint &endpoint, std::string host, | ||
function<void(system::error_code const &)> handler) { | ||
context_.reset( | ||
new asio::ssl::context(service_, asio::ssl::context::sslv23_client)); | ||
new asio::ssl::context(service_, asio::ssl::context::tlsv1_client)); | ||
if (ciphers_) { | ||
::SSL_CTX_set_cipher_list(context_->native_handle(), ciphers_->c_str()); | ||
} | ||
if (ssl_options_ != 0) { | ||
context_->set_options(ssl_options_); | ||
} | ||
if (certificate_filename_ || verify_path_) { | ||
context_->set_verify_mode(asio::ssl::context::verify_peer); | ||
if (certificate_filename_) | ||
|
@@ -36,9 +47,10 @@ void boost::network::http::impl::ssl_delegate::connect( | |
} else { | ||
if (always_verify_peer_) { | ||
context_->set_verify_mode(asio::ssl::context::verify_peer); | ||
context_->set_default_verify_paths(); // use openssl default verify paths. uses openssl environment variables SSL_CERT_DIR, SSL_CERT_FILE | ||
} | ||
else | ||
// use openssl default verify paths. uses openssl environment variables | ||
// SSL_CERT_DIR, SSL_CERT_FILE | ||
context_->set_default_verify_paths(); | ||
} else | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This formatting looks weird to me. Please make consistent with the surrounding code, and don't use tab characters... if you can run clang-format on it using the .clang-format configuration in the root of the package. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. I used the cpp-netlib's .clang-format and clang-format version 3.5.0 (tags/RELEASE_350/final), it should have picked up "UseTab: false". |
||
context_->set_verify_mode(asio::ssl::context::verify_none); | ||
} | ||
if (certificate_file_) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ struct client_options { | |
openssl_verify_path_(), | ||
openssl_certificate_file_(), | ||
openssl_private_key_file_(), | ||
openssl_ciphers_(), | ||
openssl_options_(0), | ||
io_service_(), | ||
always_verify_peer_(false), | ||
timeout_(0) {} | ||
|
@@ -38,6 +40,8 @@ struct client_options { | |
openssl_verify_path_(other.openssl_verify_path_), | ||
openssl_certificate_file_(other.openssl_certificate_file_), | ||
openssl_private_key_file_(other.openssl_private_key_file_), | ||
openssl_ciphers_(other.openssl_ciphers_), | ||
openssl_options_(other.openssl_options_), | ||
io_service_(other.io_service_), | ||
always_verify_peer_(other.always_verify_peer_), | ||
timeout_(other.timeout_) {} | ||
|
@@ -55,6 +59,8 @@ struct client_options { | |
swap(openssl_verify_path_, other.openssl_verify_path_); | ||
swap(openssl_certificate_file_, other.openssl_certificate_file_); | ||
swap(openssl_private_key_file_, other.openssl_private_key_file_); | ||
swap(openssl_ciphers_, other.openssl_ciphers_); | ||
swap(openssl_options_, other.openssl_options_); | ||
swap(io_service_, other.io_service_); | ||
swap(always_verify_peer_, other.always_verify_peer_); | ||
swap(timeout_, other.timeout_); | ||
|
@@ -90,6 +96,16 @@ struct client_options { | |
return *this; | ||
} | ||
|
||
client_options& openssl_ciphers(string_type const& v) { | ||
openssl_ciphers_ = v; | ||
return *this; | ||
} | ||
|
||
client_options& openssl_options(long o) { | ||
openssl_options_ ^= o; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get this. Why aren't you just setting the options_ and letting users explicitly OR the options they want as input? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, thanks! |
||
return *this; | ||
} | ||
|
||
client_options& io_service(boost::shared_ptr<boost::asio::io_service> v) { | ||
io_service_ = v; | ||
return *this; | ||
|
@@ -125,6 +141,12 @@ struct client_options { | |
return openssl_private_key_file_; | ||
} | ||
|
||
boost::optional<string_type> openssl_ciphers() const { | ||
return openssl_ciphers_; | ||
} | ||
|
||
long openssl_options() const { return openssl_options_; } | ||
|
||
boost::shared_ptr<boost::asio::io_service> io_service() const { | ||
return io_service_; | ||
} | ||
|
@@ -140,6 +162,8 @@ struct client_options { | |
boost::optional<string_type> openssl_verify_path_; | ||
boost::optional<string_type> openssl_certificate_file_; | ||
boost::optional<string_type> openssl_private_key_file_; | ||
boost::optional<string_type> openssl_ciphers_; | ||
long openssl_options_; | ||
boost::shared_ptr<boost::asio::io_service> io_service_; | ||
bool always_verify_peer_; | ||
int timeout_; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change the default from SSLv23 to TLSv1. This will be a silent breaking change and I'm not convinced this is the best default for users of the library anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing with Apple's provided OpenSSL 0.9.8 the
::SSLv23_method()
should use the options to decide what type of protocol it negotiates. To disable SSL3 you could use thesslv23_client
method and:options.openssl_options(SSL_OP_NO_SSLv3 | SSL_OP_NO_SSLv2 | SSL_OP_ALL);
. However, I'm still seeing an SSLv3 client hello. On newer versions of openssl this works as expected and thesslv23_client
with theSSL_OP_NO_SSLv3
option forces the client to send a TLSv1 client hello.I'll give this more thought, but using
tlsv1_client
forces Apple's OpenSSL to TLSv1 client hello (and negotiate it's best supported TLS1.0) while other newer OpenSSLs will do the same and negotiate TLS1.1/TLS1.2 if supported by the server. This is the best possible scenario without considering supporting older clients.Thoughts on a less-silent but still-default breaking change?