Skip to content

Commit

Permalink
Don't use verify_callback.
Browse files Browse the repository at this point in the history
The OpenSSL verify_callback isn't supported on jruby [1], and behaves in
somewhat surprising ways on OS X due to Apple monkey patching OpenSSL.

We probably want to move in the direction of just passing through the
OpenSSL exceptions anyway.

[1] jruby/jruby#597

Fixes: rest-client#165
See also: rest-client#168, e03e5e6
  • Loading branch information
ab committed Apr 2, 2014
1 parent eef44c5 commit b59e2e5
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 30 deletions.
44 changes: 25 additions & 19 deletions lib/restclient/request.rb
Expand Up @@ -334,19 +334,10 @@ def transmit uri, req, payload, & block
net.use_ssl = uri.is_a?(URI::HTTPS)
net.ssl_version = ssl_version if ssl_version
net.ciphers = ssl_ciphers if ssl_ciphers
err_msg = nil
if verify_ssl
net.verify_mode = verify_ssl
net.verify_callback = lambda do |preverify_ok, ssl_context|
if (!preverify_ok) || ssl_context.error != 0
err_msg = "SSL Verification failed -- Preverify: #{preverify_ok}, Error: #{ssl_context.error_string} (#{ssl_context.error})"
return false
end
true
end
else
net.verify_mode = verify_ssl
end

# we no longer set net.verify_callback because it's not well supported on
# all platforms (see comments below)
net.verify_mode = verify_ssl

net.cert = ssl_client_cert if ssl_client_cert
net.key = ssl_client_key if ssl_client_key
Expand Down Expand Up @@ -393,16 +384,31 @@ def transmit uri, req, payload, & block
process_result res, & block
end
end
rescue OpenSSL::SSL::SSLError => e
if err_msg
raise SSLCertificateNotVerified.new(err_msg)
else
raise e
end
rescue EOFError
raise RestClient::ServerBrokeConnection
rescue Timeout::Error, Errno::ETIMEDOUT
raise RestClient::RequestTimeout

rescue OpenSSL::SSL::SSLError => error
# TODO: deprecate and remove RestClient::SSLCertificateNotVerified and just
# pass through OpenSSL::SSL::SSLError directly.
#
# Exceptions in verify_callback are ignored [1], and jruby doesn't support
# it at all [2]. RestClient has to catch OpenSSL::SSL::SSLError and either
# re-throw it as is, or throw SSLCertificateNotVerified based on the
# contents of the message field of the original exception.
#
# The client has to handle OpenSSL::SSL::SSLError exceptions anyway, so
# we shouldn't make them handle both OpenSSL and RestClient exceptions.
#
# [1] https://github.com/ruby/ruby/blob/89e70fe8e7/ext/openssl/ossl.c#L238
# [2] https://github.com/jruby/jruby/issues/597

if error.message.include?("certificate verify failed")
raise SSLCertificateNotVerified.new(error.message)
else
raise error
end
end

def setup_credentials(req)
Expand Down
13 changes: 3 additions & 10 deletions spec/integration/request_spec.rb
Expand Up @@ -28,16 +28,9 @@
expect { request.execute }.to_not raise_error
end

# I don' think this feature is useful anymore (under 1.9.3 at the very least).
#
# Exceptions in verify_callback are ignored; RestClient has to catch OpenSSL::SSL::SSLError
# and either re-throw it as is, or throw SSLCertificateNotVerified
# based on the contents of the message field of the original exception
#.
# The client has to handle OpenSSL::SSL::SSLError exceptions anyway,
# why make them handle both OpenSSL *AND* RestClient exceptions???
#
# also see https://github.com/ruby/ruby/blob/trunk/ext/openssl/ossl.c#L237
# TODO: deprecate and remove RestClient::SSLCertificateNotVerified and just
# pass through OpenSSL::SSL::SSLError directly. See note in
# lib/restclient/request.rb.
it "is unsuccessful with an incorrect ca_file" do
request = RestClient::Request.new(
:method => :get,
Expand Down
1 change: 0 additions & 1 deletion spec/unit/request_spec.rb
Expand Up @@ -549,7 +549,6 @@
:payload => 'payload',
:verify_ssl => mode )
@net.should_receive(:verify_mode=).with(mode)
@net.should_receive(:verify_callback=)
@http.stub(:request)
@request.stub(:process_result)
@request.stub(:response_log)
Expand Down

0 comments on commit b59e2e5

Please sign in to comment.