backport Windows Certificate fixes to Chef-17

Signed-off-by: John McCrae <jmccrae@chf.io>

4 files changed, 78 insertions, 64 deletions

diff --git a/chef-universal-mingw32.gemspec b/chef-universal-mingw32.gemspec
index 6d9497486c..a001bdc46c 100644
--- a/chef-universal-mingw32.gemspec
+++ b/chef-universal-mingw32.gemspec
@@ -14,7 +14,7 @@ gemspec.add_dependency "win32-service", ">= 2.1.5", "< 3.0"
 gemspec.add_dependency "wmi-lite", "~> 1.0"
 gemspec.add_dependency "win32-taskscheduler", "~> 2.0"
 gemspec.add_dependency "iso8601", ">= 0.12.1", "< 0.14" # validate 0.14 when it comes out
-gemspec.add_dependency "win32-certstore", "~> 0.6.2"
+gemspec.add_dependency "win32-certstore", "~> 0.6.14"
 gemspec.add_dependency "chef-powershell", "~> 1.0.12" # 0.5+ required for specifying user vs. system store
 gemspec.extensions << "ext/win32-eventlog/Rakefile"
 gemspec.files += Dir.glob("{distro,ext}/**/*")
diff --git a/kitchen-tests/cookbooks/end_to_end/recipes/_chef_client_trusted_certificate.rb b/kitchen-tests/cookbooks/end_to_end/recipes/_chef_client_trusted_certificate.rb
index 94e6cedde8..e719a01837 100644
--- a/kitchen-tests/cookbooks/end_to_end/recipes/_chef_client_trusted_certificate.rb
+++ b/kitchen-tests/cookbooks/end_to_end/recipes/_chef_client_trusted_certificate.rb
@@ -1,10 +1,10 @@
 chef_client_trusted_certificate "self-signed.badssl.com" do
   certificate <<~CERT
 -----BEGIN CERTIFICATE-----
-MIIDeTCCAmGgAwIBAgIJAMnA8BB8xT6wMA0GCSqGSIb3DQEBCwUAMGIxCzAJBgNV
+MIIDeTCCAmGgAwIBAgIJALvxdCPEMG1VMA0GCSqGSIb3DQEBCwUAMGIxCzAJBgNV
 BAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNp
 c2NvMQ8wDQYDVQQKDAZCYWRTU0wxFTATBgNVBAMMDCouYmFkc3NsLmNvbTAeFw0y
-MTEwMTEyMDAzNTRaFw0yMzEwMTEyMDAzNTRaMGIxCzAJBgNVBAYTAlVTMRMwEQYD
+MjA1MTcyMTE1MjVaFw0yNDA1MTYyMTE1MjVaMGIxCzAJBgNVBAYTAlVTMRMwEQYD
 VQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2NvMQ8wDQYDVQQK
 DAZCYWRTU0wxFTATBgNVBAMMDCouYmFkc3NsLmNvbTCCASIwDQYJKoZIhvcNAQEB
 BQADggEPADCCAQoCggEBAMIE7PiM7gTCs9hQ1XBYzJMY61yoaEmwIrX5lZ6xKyx2
@@ -14,12 +14,12 @@ xPxTuW1CrbV8/q71FdIzSOciccfCFHpsKOo3St/qbLVytH5aohbcabFXRNsKEqve
 ww9HdFxBIuGa+RuT5q0iBikusbpJHAwnnqP7i/dAcgCskgjZjFeEU4EFy+b+a1SY
 QCeFxxC7c3DvaRhBB0VVfPlkPz0sw6l865MaTIbRyoUCAwEAAaMyMDAwCQYDVR0T
 BAIwADAjBgNVHREEHDAaggwqLmJhZHNzbC5jb22CCmJhZHNzbC5jb20wDQYJKoZI
-hvcNAQELBQADggEBAC4DensZ5tCTeCNJbHABYPwwqLUFOMITKOOgF3t8EqOan0CH
-ST1NNi4jPslWrVhQ4Y3UbAhRBdqXl5N/NFfMzDosPpOjFgtifh8Z2s3w8vdlEZzf
-A4mYTC8APgdpWyNgMsp8cdXQF7QOfdnqOfdnY+pfc8a8joObR7HEaeVxhJs+XL4E
-CLByw5FR+svkYgCbQGWIgrM1cRpmXemt6Gf/XgFNP2PdubxqDEcnWlTMk8FCBVb1
-nVDSiPjYShwnWsOOshshCRCAiIBPCKPX0QwKDComQlRrgMIvddaSzFFTKPoNZjC+
-CUspSNnL7V9IIHvqKlRSmu+zIpm2VJCp1xLulk8=
+hvcNAQELBQADggEBAI8cPgdGNaXwomKxzksJPMCHQC3zkTKQBqGAk4yWp3w7/WHV
+1dTz/ezCH1UpxRUqTIZ/jS7OwrERRJCw6wr84WrKj0TKgZI00LEoRg3eK+U5QJj1
+4HZ8UTVkYL7OhPBanGgACw0eOvFtLdwizBTAw+B79Uzx0j84babX8HFq8UJR997H
+FN+Fo9w4+ObXiA6BB9+fsqhRKPcIvt4HI6eQ4S/5lBkZbhHIPSR5oy4y7AsnZeb8
+hFNhP/WPdRvk4za0cAkuhoY5/3A/U4eFrXYL4N8mhqiRWvqbSaBT2YnlbSVxEDQM
+CJWRCL+Lq4ZHGYgXSi1T0/LJHSlOa1F5qhZpk9A=
 -----END CERTIFICATE-----
   CERT
 end
diff --git a/lib/chef/resource/windows_certificate.rb b/lib/chef/resource/windows_certificate.rb
index 528b0c53f6..79abfa4c19 100644
--- a/lib/chef/resource/windows_certificate.rb
+++ b/lib/chef/resource/windows_certificate.rb
@@ -29,7 +29,6 @@ require "chef-utils/dist" unless defined?(ChefUtils::Dist)
 class Chef
   class Resource
     class WindowsCertificate < Chef::Resource
-      unified_mode true
 
       provides :windows_certificate
 
@@ -129,14 +128,14 @@ class Chef
       end
 
       action :delete, description: "Deletes a certificate." do
-        cert_obj = fetch_cert
+        cert_is_valid = verify_cert
 
-        if cert_obj
+        if cert_is_valid == true
           converge_by("Deleting certificate #{new_resource.source} from Store #{new_resource.store_name}") do
             delete_cert
           end
         else
-          Chef::Log.debug("Certificate not found")
+          Chef::Log.debug("Certificate Not Found")
         end
       end
 
@@ -146,17 +145,25 @@ class Chef
         end
 
         if ::File.extname(new_resource.output_path) == ".pfx"
-          powershell_exec!(pfx_ps_cmd(resolve_thumbprint(new_resource.source), store_location: ps_cert_location, store_name: new_resource.store_name, output_path: new_resource.output_path, password: new_resource.pfx_password ))
+
+          validated_thumbprint = validate_thumbprint(new_resource.source)
+          if validated_thumbprint != false # is the thumbprint valid
+            cert_obj = powershell_exec!(pfx_ps_cmd(validate_thumbprint(new_resource.source), store_location: ps_cert_location, store_name: new_resource.store_name, output_path: new_resource.output_path, password: new_resource.pfx_password ))
+          else
+            message = "While fetching the certificate, was passed the following invalid certificate thumbprint : #{new_resource.source}\n"
+            raise Chef::Exceptions::InvalidKeyAttribute, message
+          end
+
         else
           cert_obj = fetch_cert
         end
 
-        if cert_obj
+        if cert_obj != false && cert_obj != "Certificate Not Found"
           converge_by("Fetching certificate #{new_resource.source} from Store \\#{ps_cert_location}\\#{new_resource.store_name}") do
             export_cert(cert_obj, output_path: new_resource.output_path, store_name: new_resource.store_name , store_location: ps_cert_location, pfx_password: new_resource.pfx_password)
           end
         else
-          Chef::Log.debug("Certificate not found")
+          Chef::Log.debug("Certificate Not Found")
         end
       end
 
@@ -187,7 +194,7 @@ class Chef
 
         def delete_cert
           store = ::Win32::Certstore.open(new_resource.store_name, store_location: native_cert_location)
-          store.delete(resolve_thumbprint(new_resource.source))
+          store.delete(validate_thumbprint(new_resource.source))
         end
 
         def fetch_cert
@@ -196,17 +203,16 @@ class Chef
             fetch_key
 
           else
-            store.get(resolve_thumbprint(new_resource.source), store_name: new_resource.store_name, store_location: native_cert_location)
+            store.get(validate_thumbprint(new_resource.source))
           end
         end
 
         def fetch_key
           require "openssl" unless defined?(OpenSSL)
           file_name = ::File.basename(new_resource.output_path, ::File.extname(new_resource.output_path))
-          directory = ::File.dirname(new_resource.output_path)
           pfx_file = file_name + ".pfx"
           new_pfx_output_path = ::File.join(Chef::FileCache.create_cache_path("pfx_files"), pfx_file)
-          powershell_exec(pfx_ps_cmd(resolve_thumbprint(new_resource.source), store_location: ps_cert_location, store_name: new_resource.store_name, output_path: new_pfx_output_path, password: new_resource.pfx_password ))
+          powershell_exec(pfx_ps_cmd(validate_thumbprint(new_resource.source), store_location: ps_cert_location, store_name: new_resource.store_name, output_path: new_pfx_output_path, password: new_resource.pfx_password ))
           pkcs12 = OpenSSL::PKCS12.new(::File.binread(new_pfx_output_path), new_resource.pfx_password)
           f = ::File.open(new_resource.output_path, "w")
           f.write(pkcs12.key.to_s)
@@ -245,10 +251,6 @@ class Chef
           ::File.file?(source)
         end
 
-        def is_file?(source)
-          ::File.file?(source)
-        end
-
         # Thumbprints should be exactly 40 Hex characters
         def valid_thumbprint?(string)
           string.match?(/[0-9A-Fa-f]/) && string.length == 40
@@ -261,29 +263,29 @@ class Chef
           GETTHUMBPRINTCODE
         end
 
-        def resolve_thumbprint(thumbprint)
-          return thumbprint if valid_thumbprint?(thumbprint)
-
-          powershell_exec!(get_thumbprint(new_resource.store_name, ps_cert_location, new_resource.source)).result
+        def validate_thumbprint(thumbprint)
+          # valid_thumbprint can return false under at least 2 conditions:
+          # one is that the thumbprint is in fact busted
+          # the second is that the thumbprint is valid but belongs to an expired certificate already installed
+          results = valid_thumbprint?(thumbprint)
+          results == true ? thumbprint : false
         end
 
-        # Checks whether a certificate with the given thumbprint
-        # is already present and valid in certificate store
-        # If the certificate is not present, verify_cert returns a String: "Certificate not found"
-        # But if it is present but expired, it returns a Boolean: false
-        # Otherwise, it returns a Boolean: true
-        # updated this method to accept either a subject name or a thumbprint - 1/29/2021
-
+        # Checks to make sure whether the cert is found or not
+        # if it IS found, is it still valid - has it expired?
         def verify_cert(thumbprint = new_resource.source)
           store = ::Win32::Certstore.open(new_resource.store_name, store_location: native_cert_location)
-          if new_resource.pfx_password.nil?
-            store.valid?(resolve_thumbprint(thumbprint), store_location: native_cert_location, store_name: new_resource.store_name )
+          validated_thumbprint = validate_thumbprint(thumbprint)
+          if validated_thumbprint != false
+            result = store.valid?(thumbprint)
+            result == ( "Certificate Not Found" || "Certificate Has Expired" ) ? false : true
           else
-            store.valid?(resolve_thumbprint(thumbprint), store_location: native_cert_location, store_name: new_resource.store_name)
+            message = "While verifying the certificate, was passed the following invalid certificate thumbprint : #{thumbprint}\n"
+            raise Chef::Exceptions::InvalidKeyAttribute, message
           end
         end
 
-        # this array structure is solving 2 problems. The first is that we need to have support for both the CurrentUser AND LocalMachine stores
+        # this structure is solving 2 problems. The first is that we need to have support for both the CurrentUser AND LocalMachine stores
         # Secondly, we need to pass the proper constant name for each store to win32-certstore but also pass the short name to powershell scripts used here
         def ps_cert_location
           new_resource.user_store ? "CurrentUser" : "LocalMachine"
@@ -436,7 +438,7 @@ class Chef
         end
 
         def export_cert(cert_obj, output_path:, store_name:, store_location:, pfx_password:)
-          # Delete the cert if it exists. This is non-destructive in that it only removes the file and not the entire path.
+          # Delete the cert if it exists on disk already.
           # We want to ensure we're not randomly loading an old stinky cert.
           if ::File.exists?(output_path)
             ::File.delete(output_path)
@@ -460,7 +462,20 @@ class Chef
             cert_out = shell_out("openssl x509 -text -inform DER -in #{cert_obj} -outform CRT").stdout
             out_file.puts(cert_out)
           when ".pfx"
-            pfx_ps_cmd(resolve_thumbprint(new_resource.source), store_location: store_location, store_name: store_name, output_path: output_path, password: pfx_password )
+            validated_thumbprint = validate_thumbprint(new_resource.source)
+            if validated_thumbprint != false # is the thumbprint valid
+              store = ::Win32::Certstore.open(new_resource.store_name, store_location: native_cert_location)
+              result = store.valid?(new_resource.source) # is there a cert in the store matching that thumbprint
+              temp = result == ( "Certificate Not Found" || "Certificate Has Expired" ) ? false : true
+              if temp == true
+                pfx_ps_cmd(validate_thumbprint(new_resource.source), store_location: store_location, store_name: store_name, output_path: output_path, password: pfx_password )
+              else
+                Chef::Log.debug("The requested certificate is not found or has expired")
+              end
+            else
+              message = "While exporting the pfx, was passed the following invalid certificate thumbprint : #{new_resource.source}\n"
+              raise Chef::Exceptions::InvalidKeyAttribute, message
+            end
           when ".p7b"
             cert_out = shell_out("openssl pkcs7 -export -nokeys -in #{cert_obj.to_pem} -outform P7B").stdout
             out_file.puts(cert_out)
@@ -481,14 +496,11 @@ class Chef
         #
         def import_certificates(cert_objs, is_pfx, store_name: new_resource.store_name, store_location: native_cert_location)
           [cert_objs].flatten.each do |cert_obj|
-            # thumbprint = OpenSSL::Digest.new("SHA1", cert_obj.to_der).to_s
-            # pkcs = OpenSSL::PKCS12.new(cert_obj, new_resource.pfx_password)
-            # cert = OpenSSL::X509::Certificate.new(pkcs.certificate.to_pem)
             thumbprint = OpenSSL::Digest.new("SHA1", cert_obj.to_der).to_s
-            if is_pfx
-              if verify_cert(thumbprint) == true
-                Chef::Log.debug("Certificate is already present")
-              else
+            if verify_cert(thumbprint) == true
+              Chef::Log.debug("Certificate is already present")
+            elsif verify_cert(thumbprint) == false # Not found already in the CertStore
+              if is_pfx
                 if is_file?(new_resource.source)
                   converge_by("Creating a PFX #{new_resource.source} for Store #{new_resource.store_name}") do
                     add_pfx_cert(new_resource.source)
@@ -502,15 +514,14 @@ class Chef
                   message << exception.message
                   raise Chef::Exceptions::ArgumentError, message
                 end
-              end
-            else
-              if verify_cert(thumbprint) == true
-                Chef::Log.debug("Certificate is already present")
               else
                 converge_by("Creating a certificate #{new_resource.source} for Store #{new_resource.store_name}") do
                   add_cert(cert_obj)
                 end
               end
+            else
+              message = "Certificate could not be imported"
+              raise Chef::Exceptions::CertificateNotImportable, message
             end
           end
         end
diff --git a/spec/functional/resource/windows_certificate_spec.rb b/spec/functional/resource/windows_certificate_spec.rb
index df2d1cbec8..9552d559b3 100644
--- a/spec/functional/resource/windows_certificate_spec.rb
+++ b/spec/functional/resource/windows_certificate_spec.rb
@@ -89,7 +89,9 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
 
   end
 
-  after { delete_store }
+  after do
+    delete_store
+  end
 
   describe "action: create" do
     it "starts with no certificates" do
@@ -195,7 +197,7 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
       create_store
     end
     it "fails with no certificates in the store" do
-      expect(Chef::Log).to receive(:info).with("Certificate not found")
+      expect(Chef::Log).to receive(:info).with("Certificate not valid")
 
       resource.source = tests_thumbprint
       resource.run_action(:verify)
@@ -219,7 +221,7 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
       end
 
       it "fails with an invalid thumbprint" do
-        expect(Chef::Log).to receive(:info).with("Certificate not found")
+        expect(Chef::Log).to receive(:info).with("Certificate not valid")
 
         resource.source = others_thumbprint
         resource.run_action(:verify)
@@ -253,7 +255,7 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
       end
 
       it "fails with an invalid thumbprint" do
-        expect(Chef::Log).to receive(:info).with("Certificate not found")
+        expect(Chef::Log).to receive(:info).with("Certificate not valid")
 
         resource.source = others_thumbprint
         resource.run_action(:verify)
@@ -265,11 +267,11 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
 
   describe "action: fetch" do
     context "with no certificate in the store" do
-      it "throws an error with no certificates in the store" do
-        expect(Chef::Log).not_to receive(:info)
+      it "logs a debug error with no certificates in the store" do
+        expect(Chef::Log).to receive(:debug).with("Certificate Not Found")
         resource.source = others_thumbprint
         resource.output_path = cert_output_path
-        expect { resource.run_action :fetch }.to raise_error(ArgumentError)
+        resource.run_action(:fetch)
       end
     end
 
@@ -288,7 +290,7 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
       end
 
       it "fails with an invalid thumbprint" do
-        expect(Chef::Log).not_to receive(:info)
+        expect(Chef::Log).to receive(:debug).with("Certificate Not Found")
 
         resource.source = others_thumbprint
 
@@ -296,7 +298,7 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
           path = File.join(dir, "test.pem")
 
           resource.output_path = path
-          expect { resource.run_action :fetch }.to raise_error(ArgumentError)
+          resource.run_action(:fetch)
         end
 
       end
@@ -340,9 +342,10 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
   end
 
   describe "action: delete" do
-    it "throws an argument error when attempting to delete a certificate that doesn't exist" do
+    it "logs an error when attempting to delete a certificate that doesn't exist" do
+      expect(Chef::Log).to receive(:debug).with("Certificate Not Found")
       resource.source = tests_thumbprint
-      expect { resource.run_action :delete }.to raise_error(ArgumentError)
+      resource.run_action(:delete)
     end
 
     it "deletes an existing certificate while leaving other certificates alone" do
@@ -360,7 +363,7 @@ describe Chef::Resource::WindowsCertificate, :windows_only do
       expect(certificate_count).to eq(1)
       expect(resource).to be_updated_by_last_action
 
-      expect { resource.run_action :delete }.to raise_error(ArgumentError)
+      expect { resource.run_action :delete }.not_to raise_error
       expect(certificate_count).to eq(1)
       expect(resource).not_to be_updated_by_last_action