From bc551bd1f259a6965d4e7eb6eecc4f9f9b437469 Mon Sep 17 00:00:00 2001 From: Narsimham Chelluri Date: Thu, 1 Aug 2013 01:32:25 -0700 Subject: [PATCH] Made SSL connections use the system certificate store by default. - Set http.verify_mode to OpenSSL::SSL::VERIFY_PEER by default - Use OpenSSL::X509::Store.new as certificate store unless another is specified Adapted from Faraday. See: https://github.com/lostisland/faraday/blob/b25adc0ca6b47eb5150494a735721579862990f0/lib/faraday/adapter/net_http.rb#L100 --- lib/httparty/connection_adapter.rb | 13 ++++++++++++- spec/httparty/connection_adapter_spec.rb | 16 ++++++++++++++++ spec/httparty/ssl_spec.rb | 20 ++++++++++++++++---- spec/spec_helper.rb | 6 ++++++ spec/support/ssl_test_helper.rb | 8 ++++---- 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/lib/httparty/connection_adapter.rb b/lib/httparty/connection_adapter.rb index 469a58b..8b1a619 100644 --- a/lib/httparty/connection_adapter.rb +++ b/lib/httparty/connection_adapter.rb @@ -108,7 +108,18 @@ module HTTParty def attach_ssl_certificates(http, options) if http.use_ssl? - http.verify_mode = OpenSSL::SSL::VERIFY_NONE + if options.fetch(:verify, true) + http.verify_mode = OpenSSL::SSL::VERIFY_PEER + if options[:cert_store] + http.cert_store = options[:cert_store] + else + # Use the default cert store by default, i.e. system ca certs + http.cert_store = OpenSSL::X509::Store.new + http.cert_store.set_default_paths + end + else + http.verify_mode = OpenSSL::SSL::VERIFY_NONE + end # Client certificate authentication if options[:pem] diff --git a/spec/httparty/connection_adapter_spec.rb b/spec/httparty/connection_adapter_spec.rb index b532519..93d54e7 100644 --- a/spec/httparty/connection_adapter_spec.rb +++ b/spec/httparty/connection_adapter_spec.rb @@ -64,6 +64,22 @@ describe HTTParty::ConnectionAdapter do context "when dealing with ssl" do let(:uri) { URI 'https://foobar.com' } + context "uses the system cert_store, by default" do + let(:system_cert_store) do + system_cert_store = mock('default_cert_store') + system_cert_store.should_receive(:set_default_paths) + OpenSSL::X509::Store.should_receive(:new).and_return(system_cert_store) + system_cert_store + end + it { should use_cert_store(system_cert_store) } + end + + context "should use the specified cert store, when one is given" do + let(:custom_cert_store) { mock('custom_cert_store') } + let(:options) { {:cert_store => custom_cert_store} } + it { should use_cert_store(custom_cert_store) } + end + context "using port 443 for ssl" do let(:uri) { URI 'https://api.foo.com/v1:443' } it { should use_ssl } diff --git a/spec/httparty/ssl_spec.rb b/spec/httparty/ssl_spec.rb index 512762b..8d6ec6f 100644 --- a/spec/httparty/ssl_spec.rb +++ b/spec/httparty/ssl_spec.rb @@ -10,12 +10,24 @@ describe HTTParty::Request do FakeWeb.allow_net_connect = false end - it "should work when no trusted CA list is specified" do - ssl_verify_test(nil, nil, "selfsigned.crt").should == {'success' => true} + it "should fail when no trusted CA list is specified, by default" do + lambda do + ssl_verify_test(nil, nil, "selfsigned.crt") + end.should raise_error OpenSSL::SSL::SSLError end - it "should work when no trusted CA list is specified, even with a bogus hostname" do - ssl_verify_test(nil, nil, "bogushost.crt").should == {'success' => true} + it "should work when no trusted CA list is specified, when the verify option is set to false" do + ssl_verify_test(nil, nil, "selfsigned.crt", :verify => false).should == {'success' => true} + end + + it "should fail when no trusted CA list is specified, with a bogus hostname, by default" do + lambda do + ssl_verify_test(nil, nil, "bogushost.crt") + end.should raise_error OpenSSL::SSL::SSLError + end + + it "should work when no trusted CA list is specified, even with a bogus hostname, when the verify option is set to true" do + ssl_verify_test(nil, nil, "bogushost.crt", :verify => false).should == {'success' => true} end it "should work when using ssl_ca_file with a self-signed CA" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a85b2b1..a7aeb9a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -28,3 +28,9 @@ Spec::Matchers.define :use_ssl do connection.use_ssl? end end + +Spec::Matchers.define :use_cert_store do |cert_store| + match do |connection| + connection.cert_store == cert_store + end +end diff --git a/spec/support/ssl_test_helper.rb b/spec/support/ssl_test_helper.rb index 0aa6818..ba63000 100644 --- a/spec/support/ssl_test_helper.rb +++ b/spec/support/ssl_test_helper.rb @@ -2,11 +2,11 @@ require 'pathname' module HTTParty module SSLTestHelper - def ssl_verify_test(mode, ca_basename, server_cert_filename) + def ssl_verify_test(mode, ca_basename, server_cert_filename, options = {}) options = { :format => :json, :timeout => 30, - } + }.merge(options) if mode ca_path = File.expand_path("../../fixtures/ssl/generated/#{ca_basename}", __FILE__) @@ -24,9 +24,9 @@ module HTTParty if mode ca_path = File.expand_path("../../fixtures/ssl/generated/#{ca_basename}", __FILE__) raise ArgumentError.new("#{ca_path} does not exist") unless File.exist?(ca_path) - return HTTParty.get("https://localhost:#{test_server.port}/", :format => :json, :timeout => 30, mode => ca_path) + return HTTParty.get("https://localhost:#{test_server.port}/", options) else - return HTTParty.get("https://localhost:#{test_server.port}/", :format => :json, :timeout => 30) + return HTTParty.get("https://localhost:#{test_server.port}/", options) end ensure test_server.stop if test_server