Skip to content
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

Adding support for newer SSL options in the net/http adapter #339

Closed

Conversation

aetherknight
Copy link

In recent Rubies, net/http supports setting the list of ciphers, as well as an SSL timeout and a callback for additional server certificate verification. This PR adds support for those attributes to the net/http adapter.

Let me know if there are any changes you would like made. Thanks!

BJ Orvis added 3 commits February 5, 2014 18:57
I considered gating with a respond_to? to support older versions of
Ruby where net/http does not support all of the attributes, but doing so
would make Faraday silently ignore those values instead of notifying the
developer that they are trying to do something with an older version of
Ruby that it does not support.
…net_http

* lostisland/master:
  Add full url for get /nigiri in README
  Update Travis settings for Rubinius
  Only try to cache dependencies when Amazon secret is available
  Add HTTPClient to list of adapters in README
  Retry only if the method is idempotent
  Adds a retry_if option to the retry middleware
  Handle Patron "Connection time-out" properly
  Cache Bundler dependencies on Travis CI
  Bump net-http-persistent
  Allow jruby-XYmode failures on Travis due to RVM bug
  Avoid broken net-http-persistent v2.9.2/3
Needed to add ciphers, verify_callback, and timeout (for ssl) to the
SSLOptions.
@aetherknight
Copy link
Author

All the Travis tests except for the Ruby 1.8 tests are now green. The errored tests are failing because the version of Rake pulled in by faraday no longer supports Ruby 1.8.

BJ Orvis and others added 3 commits April 9, 2014 17:17
…net_http

* lostisland/master:
  Don't require SimpleCov on Ruby 1.8
  Update README with env usage in stubs
  Lock rest-client to ~> 1.6.0 on Ruby 1.8.7
  Fix ActiveSupport::SafeBuffer#gsub bug
  Fix response not changing with parallel requests
…net_http

* lostisland/master:
  clarify what the default stack is
  Increase test coverage for parameter encoders
  Fix passing ActiveSupport::SafeBuffer to param encoders
  Remove timeout from request autoloads, since it was removed in 43343e9
  use to_str to check for strings and added #pretty_inspect to non-strings
  use an array instead of a hash for testing pretty_inspect of objects
  added options hash to Logger to support logging bodies
  log request bodies in logger middleware
@aetherknight
Copy link
Author

I merged master into my PR and now Travis-ci is happy again. No changes to my branch.

@stevehill1981
Copy link

Is anything else required before this pull request gets merged?

@mislav
Copy link
Contributor

mislav commented Oct 6, 2015

Sorry for the late review. PR looks good, but can we provide the same options to other adapters as well? Generally, request options should work the same across all adapters.

@mislav mislav added the feature label Oct 6, 2015
@aetherknight
Copy link
Author

I don't think it is possible for every adapter to provide all of these parameters without significantly patching the underlying libraries. net::http::persistent is straightforward, but the others are not.

Furthermore, the existing adapters do not consistently implement the existing SSL options. For example, the net::http and net::http::persistent expect OpenSSL::X509::Certificate and OpenSSL::PKey::* objects for :client_cert and :client_key, but libcurl-based clients can only accept filenames for the certificate and private key (and the file type required varies based on implementation of libcurl), and their adapters treat :client_cert and :client_key as filenames.

Another example is that the excon adapter confusingly uses both :client_cert and :client_key, but also :certificate and :private_key.

@aetherknight
Copy link
Author

Closing this in favor of #544, which comes from my own fork of faraday instead of an old organization fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants