Merge pull request #10751 from chef/fix-failures-in-ssl-handler

Signed-off-by: Tim Smith <tsmith@chef.io>

4 files changed, 133 insertions, 92 deletions

diff --git a/lib/chef/http/ssl_policies.rb b/lib/chef/http/ssl_policies.rb
index d36e747ff6..bc688f13a6 100644
--- a/lib/chef/http/ssl_policies.rb
+++ b/lib/chef/http/ssl_policies.rb
@@ -85,28 +85,41 @@ class Chef
           http_client.cert_store.set_default_paths
         end
         if config.trusted_certs_dir
-          certs = Dir.glob(File.join(Chef::Util::PathHelper.escape_glob_dir(config.trusted_certs_dir), "*.{crt,pem}"))
+          certs = Dir.glob(::File.join(Chef::Util::PathHelper.escape_glob_dir(config.trusted_certs_dir), "*.{crt,pem}"))
           certs.each do |cert_file|
-            cert = OpenSSL::X509::Certificate.new(File.read(cert_file))
+            cert = begin
+              OpenSSL::X509::Certificate.new(::File.binread(cert_file))
+                   rescue OpenSSL::X509::CertificateError => e
+                     raise Chef::Exceptions::ConfigurationError, "Error reading cert file '#{cert_file}', original error '#{e.class}: #{e.message}'"
+            end
             add_trusted_cert(cert)
           end
         end
       end
 
       def set_client_credentials
-        if config[:ssl_client_cert] || config[:ssl_client_key]
-          unless config[:ssl_client_cert] && config[:ssl_client_key]
-            raise Chef::Exceptions::ConfigurationError, "You must configure ssl_client_cert and ssl_client_key together"
-          end
-          unless ::File.exists?(config[:ssl_client_cert])
-            raise Chef::Exceptions::ConfigurationError, "The configured ssl_client_cert #{config[:ssl_client_cert]} does not exist"
-          end
-          unless ::File.exists?(config[:ssl_client_key])
-            raise Chef::Exceptions::ConfigurationError, "The configured ssl_client_key #{config[:ssl_client_key]} does not exist"
-          end
+        return unless config[:ssl_client_cert] || config[:ssl_client_key]
+
+        unless config[:ssl_client_cert] && config[:ssl_client_key]
+          raise Chef::Exceptions::ConfigurationError, "You must configure ssl_client_cert and ssl_client_key together"
+        end
+        unless ::File.exists?(config[:ssl_client_cert])
+          raise Chef::Exceptions::ConfigurationError, "The configured ssl_client_cert #{config[:ssl_client_cert]} does not exist"
+        end
+        unless ::File.exists?(config[:ssl_client_key])
+          raise Chef::Exceptions::ConfigurationError, "The configured ssl_client_key #{config[:ssl_client_key]} does not exist"
+        end
+
+        begin
+          http_client.cert = OpenSSL::X509::Certificate.new(::File.binread(config[:ssl_client_cert]))
+        rescue OpenSSL::X509::CertificateError => e
+          raise Chef::Exceptions::ConfigurationError, "Error reading cert file '#{config[:ssl_client_cert]}', original error '#{e.class}: #{e.message}'"
+        end
 
-          http_client.cert = OpenSSL::X509::Certificate.new(::File.read(config[:ssl_client_cert]))
-          http_client.key = OpenSSL::PKey::RSA.new(::File.read(config[:ssl_client_key]))
+        begin
+          http_client.key = OpenSSL::PKey::RSA.new(::File.binread(config[:ssl_client_key]))
+        rescue OpenSSL::PKey::RSAError => e
+          raise Chef::Exceptions::ConfigurationError, "Error reading key file '#{config[:ssl_client_key]}', original error '#{e.class}: #{e.message}'"
         end
       end
 
diff --git a/spec/data/ssl/binary/chef-rspec-der.cert b/spec/data/ssl/binary/chef-rspec-der.cert
new file mode 100644
index 0000000000..e49df6252a
--- /dev/null
+++ b/spec/data/ssl/binary/chef-rspec-der.cert
diff --git a/spec/data/ssl/binary/chef-rspec-der.key b/spec/data/ssl/binary/chef-rspec-der.key
new file mode 100644
index 0000000000..d8adadc5c9
--- /dev/null
+++ b/spec/data/ssl/binary/chef-rspec-der.key
diff --git a/spec/unit/http/ssl_policies_spec.rb b/spec/unit/http/ssl_policies_spec.rb
index 614b5018d1..6fc00b5fd9 100644
--- a/spec/unit/http/ssl_policies_spec.rb
+++ b/spec/unit/http/ssl_policies_spec.rb
@@ -29,91 +29,83 @@ describe "HTTP SSL Policy" do
     ENV["SSL_CERT_FILE"]           = nil
   end
 
-  let(:unconfigured_http_client) { Net::HTTP.new("example.com", 443) }
   let(:http_client) do
-    unconfigured_http_client.use_ssl = true
-    ssl_policy.apply
-    unconfigured_http_client
+    ssl_policy_class.apply_to(Net::HTTP.new("example.com"))
   end
 
   describe Chef::HTTP::DefaultSSLPolicy do
 
-    let(:ssl_policy) { Chef::HTTP::DefaultSSLPolicy.new(unconfigured_http_client) }
+    let(:ssl_policy_class) { Chef::HTTP::DefaultSSLPolicy }
 
-    describe "when configured with :ssl_verify_mode set to :verify peer" do
-      before do
-        Chef::Config[:ssl_verify_mode] = :verify_peer
-      end
-
-      it "configures the HTTP client to use SSL when given a URL with the https protocol" do
-        expect(http_client.use_ssl?).to be_truthy
-      end
-
-      it "sets the OpenSSL verify mode to verify_peer" do
-        expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_PEER)
-      end
-
-      it "raises a ConfigurationError if :ssl_ca_path is set to a path that doesn't exist" do
-        Chef::Config[:ssl_ca_path] = "/dev/null/nothing_here"
-        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
-      end
+    it "raises a ConfigurationError if :ssl_ca_path is set to a path that doesn't exist" do
+      Chef::Config[:ssl_ca_path] = "/dev/null/nothing_here"
+      expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
+    end
 
-      it "should set the CA path if that is set in the configuration" do
-        Chef::Config[:ssl_ca_path] = File.join(CHEF_SPEC_DATA, "ssl")
-        expect(http_client.ca_path).to eq(File.join(CHEF_SPEC_DATA, "ssl"))
-      end
+    it "should set the CA path if that is set in the configuration" do
+      Chef::Config[:ssl_ca_path] = File.join(CHEF_SPEC_DATA, "ssl")
+      expect(http_client.ca_path).to eq(File.join(CHEF_SPEC_DATA, "ssl"))
+    end
 
-      it "raises a ConfigurationError if :ssl_ca_file is set to a file that does not exist" do
-        Chef::Config[:ssl_ca_file] = "/dev/null/nothing_here"
-        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
-      end
+    it "raises a ConfigurationError if :ssl_ca_file is set to a file that does not exist" do
+      Chef::Config[:ssl_ca_file] = "/dev/null/nothing_here"
+      expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
+    end
 
-      it "should set the CA file if that is set in the configuration" do
-        Chef::Config[:ssl_ca_file] = CHEF_SPEC_DATA + "/ssl/5e707473.0"
-        expect(http_client.ca_file).to eq(CHEF_SPEC_DATA + "/ssl/5e707473.0")
-      end
+    it "should set the CA file if that is set in the configuration" do
+      Chef::Config[:ssl_ca_file] = CHEF_SPEC_DATA + "/ssl/5e707473.0"
+      expect(http_client.ca_file).to eq(CHEF_SPEC_DATA + "/ssl/5e707473.0")
+    end
 
-      it "should set the custom CA file if SSL_CERT_FILE environment variable is set" do
-        ENV["SSL_CERT_FILE"] = CHEF_SPEC_DATA + "/trusted_certs/intermediate.pem"
-        expect(http_client.ca_file).to eq(CHEF_SPEC_DATA + "/trusted_certs/intermediate.pem")
-      end
+    it "should set the custom CA file if SSL_CERT_FILE environment variable is set" do
+      ENV["SSL_CERT_FILE"] = CHEF_SPEC_DATA + "/trusted_certs/intermediate.pem"
+      expect(http_client.ca_file).to eq(CHEF_SPEC_DATA + "/trusted_certs/intermediate.pem")
+    end
 
-      it "raises a ConfigurationError if SSL_CERT_FILE environment variable is set to a file that does not exist" do
-        ENV["SSL_CERT_FILE"] = "/dev/null/nothing_here"
-        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
-      end
+    it "raises a ConfigurationError if SSL_CERT_FILE environment variable is set to a file that does not exist" do
+      ENV["SSL_CERT_FILE"] = "/dev/null/nothing_here"
+      expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
     end
 
-    describe "when configured with :ssl_verify_mode set to :verify peer" do
-      before do
-        @url = URI.parse("https://chef.example.com:4443/")
-        Chef::Config[:ssl_verify_mode] = :verify_none
-      end
+    it "sets the OpenSSL verify mode to verify_peer when configured with :ssl_verify_mode set to :verify_peer" do
+      Chef::Config[:ssl_verify_mode] = :verify_peer
+      expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_PEER)
+    end
 
-      it "sets the OpenSSL verify mode to :verify_none" do
-        expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_NONE)
-      end
+    it "sets the OpenSSL verify mode to :verify_none when configured with :ssl_verify_mode set to :verify_none" do
+      Chef::Config[:ssl_verify_mode] = :verify_none
+      expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_NONE)
     end
 
     describe "when configured with a client certificate" do
-      before { @url = URI.parse("https://chef.example.com:4443/") }
-
       it "raises ConfigurationError if the certificate file doesn't exist" do
         Chef::Config[:ssl_client_cert] = "/dev/null/nothing_here"
         Chef::Config[:ssl_client_key]  = CHEF_SPEC_DATA + "/ssl/chef-rspec.key"
-        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
+        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError, /ssl_client_cert .* does not exist/)
       end
 
-      it "raises ConfigurationError if the certificate file doesn't exist" do
+      it "raises ConfigurationError if the private key file doesn't exist" do
         Chef::Config[:ssl_client_cert] = CHEF_SPEC_DATA + "/ssl/chef-rspec.cert"
         Chef::Config[:ssl_client_key]  = "/dev/null/nothing_here"
-        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
+        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError, /ssl_client_key .* does not exist/)
       end
 
       it "raises a ConfigurationError if one of :ssl_client_cert and :ssl_client_key is set but not both" do
         Chef::Config[:ssl_client_cert] = "/dev/null/nothing_here"
         Chef::Config[:ssl_client_key]  = nil
-        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError)
+        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError, /configure ssl_client_cert and ssl_client_key together/)
+      end
+
+      it "raises a ConfigurationError with a bad cert file" do
+        Chef::Config[:ssl_client_cert] = __FILE__
+        Chef::Config[:ssl_client_key]  = CHEF_SPEC_DATA + "/ssl/chef-rspec.key"
+        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError, /Error reading cert file '#{__FILE__}'/)
+      end
+
+      it "raises a ConfigurationError with a bad key file" do
+        Chef::Config[:ssl_client_cert] = CHEF_SPEC_DATA + "/ssl/chef-rspec.cert"
+        Chef::Config[:ssl_client_key]  = __FILE__
+        expect { http_client }.to raise_error(Chef::Exceptions::ConfigurationError, /Error reading key file '#{__FILE__}'/)
       end
 
       it "configures the HTTP client's cert and private key" do
@@ -122,20 +114,31 @@ describe "HTTP SSL Policy" do
         expect(http_client.cert.to_s).to eq(OpenSSL::X509::Certificate.new(IO.read(CHEF_SPEC_DATA + "/ssl/chef-rspec.cert")).to_s)
         expect(http_client.key.to_s).to eq(OpenSSL::PKey::RSA.new(IO.read(CHEF_SPEC_DATA + "/ssl/chef-rspec.key")).to_s)
       end
-    end
 
-    context "when additional certs are located in the trusted_certs dir" do
-      let(:self_signed_crt_path) { File.join(CHEF_SPEC_DATA, "trusted_certs", "example.crt") }
-      let(:self_signed_crt) { OpenSSL::X509::Certificate.new(File.read(self_signed_crt_path)) }
+      it "configures the HTTP client's cert and private key with a DER encoded cert" do
+        Chef::Config[:ssl_client_cert] = CHEF_SPEC_DATA + "/ssl/binary/chef-rspec-der.cert"
+        Chef::Config[:ssl_client_key]  = CHEF_SPEC_DATA + "/ssl/chef-rspec.key"
+        expect(http_client.cert.to_s).to eq(OpenSSL::X509::Certificate.new(IO.read(CHEF_SPEC_DATA + "/ssl/chef-rspec.cert")).to_s)
+        expect(http_client.key.to_s).to eq(OpenSSL::PKey::RSA.new(IO.read(CHEF_SPEC_DATA + "/ssl/chef-rspec.key")).to_s)
+      end
 
-      let(:additional_pem_path) { File.join(CHEF_SPEC_DATA, "trusted_certs", "opscode.pem") }
-      let(:additional_pem) { OpenSSL::X509::Certificate.new(File.read(additional_pem_path)) }
+      it "configures the HTTP client's cert and private key with a DER encoded key" do
+        Chef::Config[:ssl_client_cert] = CHEF_SPEC_DATA + "/ssl/chef-rspec.cert"
+        Chef::Config[:ssl_client_key]  = CHEF_SPEC_DATA + "/ssl/binary/chef-rspec-der.key"
+        expect(http_client.cert.to_s).to eq(OpenSSL::X509::Certificate.new(IO.read(CHEF_SPEC_DATA + "/ssl/chef-rspec.cert")).to_s)
+        expect(http_client.key.to_s).to eq(OpenSSL::PKey::RSA.new(IO.read(CHEF_SPEC_DATA + "/ssl/chef-rspec.key")).to_s)
+      end
+    end
 
+    context "when additional certs are located in the trusted_certs dir" do
       before do
         Chef::Config.trusted_certs_dir = File.join(CHEF_SPEC_DATA, "trusted_certs")
       end
 
       it "enables verification of self-signed certificates" do
+        path = File.join(CHEF_SPEC_DATA, "trusted_certs", "example.crt")
+        self_signed_crt = OpenSSL::X509::Certificate.new(File.binread(path))
+
         expect(http_client.cert_store.verify(self_signed_crt)).to be_truthy
       end
 
@@ -148,39 +151,64 @@ describe "HTTP SSL Policy" do
         # If the machine running the test doesn't have ruby SSL configured correctly,
         # then the root cert also has to be loaded for the test to succeed.
         # The system under test **SHOULD** do both of these things.
+        path = File.join(CHEF_SPEC_DATA, "trusted_certs", "opscode.pem")
+        additional_pem = OpenSSL::X509::Certificate.new(File.binread(path))
+
         expect(http_client.cert_store.verify(additional_pem)).to be_truthy
       end
 
-      context "and some certs are duplicates" do
-        it "skips duplicate certs" do
-          # For whatever reason, OpenSSL errors out when adding a
-          # cert you already have to the certificate store.
-          ssl_policy.set_custom_certs
-          ssl_policy.set_custom_certs # should not raise an error
+      it "skips duplicate certs" do
+        # For whatever reason, OpenSSL errors out when adding a
+        # cert you already have to the certificate store.
+        ssl_policy = ssl_policy_class.new(Net::HTTP.new("example.com"))
+        ssl_policy.set_custom_certs
+        ssl_policy.set_custom_certs # should not raise an error
+      end
+
+      it "raises ConfigurationError with a bad cert file in the trusted_certs dir" do
+        ssl_policy = ssl_policy_class.new(Net::HTTP.new("example.com"))
+
+        Dir.mktmpdir do |dir|
+          bad_cert_file = File.join(dir, "bad_cert_file.crt")
+          File.write(bad_cert_file, File.read(__FILE__))
+
+          Chef::Config.trusted_certs_dir = dir
+          expect { ssl_policy.set_custom_certs }.to raise_error(Chef::Exceptions::ConfigurationError, /Error reading cert file/)
         end
       end
+
+      it "works with binary certs" do
+        Chef::Config.trusted_certs_dir = File.join(CHEF_SPEC_DATA, "ssl", "binary")
+
+        ssl_policy = ssl_policy_class.new(Net::HTTP.new("example.com"))
+        ssl_policy.set_custom_certs
+      end
     end
   end
 
   describe Chef::HTTP::APISSLPolicy do
 
-    let(:ssl_policy) { Chef::HTTP::APISSLPolicy.new(unconfigured_http_client) }
+    let(:ssl_policy_class) { Chef::HTTP::APISSLPolicy }
 
-    context "when verify_api_cert is set" do
-      before do
-        Chef::Config[:verify_api_cert] = true
-      end
+    it "sets the OpenSSL verify mode to verify_peer when configured with :ssl_verify_mode set to :verify_peer" do
+      Chef::Config[:ssl_verify_mode] = :verify_peer
+      expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_PEER)
+    end
 
-      it "sets the OpenSSL verify mode to verify_peer" do
-        expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_PEER)
-      end
+    it "sets the OpenSSL verify mode to :verify_none when configured with :ssl_verify_mode set to :verify_none" do
+      Chef::Config[:ssl_verify_mode] = :verify_none
+      expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_NONE)
     end
 
+    it "sets the OpenSSL verify mode to verify_peer when verify_api_cert is set" do
+      Chef::Config[:verify_api_cert] = true
+      expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_PEER)
+    end
   end
 
   describe Chef::HTTP::VerifyPeerSSLPolicy do
 
-    let(:ssl_policy) { Chef::HTTP::VerifyPeerSSLPolicy.new(unconfigured_http_client) }
+    let(:ssl_policy_class) { Chef::HTTP::VerifyPeerSSLPolicy }
 
     it "sets the OpenSSL verify mode to verify_peer" do
       expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_PEER)
@@ -190,7 +218,7 @@ describe "HTTP SSL Policy" do
 
   describe Chef::HTTP::VerifyNoneSSLPolicy do
 
-    let(:ssl_policy) { Chef::HTTP::VerifyNoneSSLPolicy.new(unconfigured_http_client) }
+    let(:ssl_policy_class) { Chef::HTTP::VerifyNoneSSLPolicy }
 
     it "sets the OpenSSL verify mode to verify_peer" do
       expect(http_client.verify_mode).to eq(OpenSSL::SSL::VERIFY_NONE)