Merge pull request #8118 from MsysTechnologiesllc/nimesh/MSYS-937_windows_certificate_idempotency

windows_certificate: Ensure all actions are fully idempotent

6 files changed, 389 insertions, 4 deletions

diff --git a/lib/chef/resource/windows_certificate.rb b/lib/chef/resource/windows_certificate.rb
index 2057855b2c..45f959ac9f 100644
--- a/lib/chef/resource/windows_certificate.rb
+++ b/lib/chef/resource/windows_certificate.rb
@@ -58,7 +58,19 @@ class Chef
 
       action :create do
         description "Creates or updates a certificate."
-        add_cert(OpenSSL::X509::Certificate.new(raw_source))
+
+        cert_obj = OpenSSL::X509::Certificate.new(raw_source) # A certificate object in memory
+        thumbprint = OpenSSL::Digest::SHA1.new(cert_obj.to_der).to_s # Fetch its thumbprint
+
+        # Need to check if return value is Boolean:true
+        # If not then the given certificate should be added in certstore
+        if verify_cert(thumbprint) == true
+          Chef::Log.debug("Certificate is already present")
+        else
+          converge_by("Adding certificate #{new_resource.source} into Store #{new_resource.store_name}") do
+            add_cert(cert_obj)
+          end
+        end
       end
 
       # acl_add is a modify-if-exists operation : not idempotent
@@ -94,6 +106,8 @@ class Chef
           converge_by("Deleting certificate #{new_resource.source} from Store #{new_resource.store_name}") do
             delete_cert
           end
+        else
+          Chef::Log.debug("Certificate not found")
         end
       end
 
@@ -104,7 +118,7 @@ class Chef
         if cert_obj
           show_or_store_cert(cert_obj)
         else
-          Chef::Log.info("Certificate not found")
+          Chef::Log.debug("Certificate not found")
         end
       end
 
@@ -134,9 +148,14 @@ class Chef
           store.get(new_resource.source)
         end
 
-        def verify_cert
+        # 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
+        def verify_cert(thumbprint = new_resource.source)
           store = ::Win32::Certstore.open(new_resource.store_name)
-          store.valid?(new_resource.source)
+          store.valid?(thumbprint)
         end
 
         def show_or_store_cert(cert_obj)
@@ -240,11 +259,15 @@ class Chef
           set_acl_script
         end
 
+        # Returns the certificate string of the given
+        # input certificate in PEM format
         def raw_source
           ext = ::File.extname(new_resource.source)
           convert_pem(ext, new_resource.source)
         end
 
+        # Uses powershell command to convert crt/der/cer/pfx & p7b certificates
+        # In PEM format and returns its certificate content
         def convert_pem(ext, source)
           out = case ext
                 when ".crt", ".der"
@@ -260,6 +283,7 @@ class Chef
           format_raw_out(out)
         end
 
+        # Returns the certificate content
         def format_raw_out(out)
           begin_cert = "-----BEGIN CERTIFICATE-----"
           end_cert = "-----END CERTIFICATE-----"
diff --git a/spec/data/windows_certificates/othertest.cer b/spec/data/windows_certificates/othertest.cer
new file mode 100644
index 0000000000..353f792252
--- /dev/null
+++ b/spec/data/windows_certificates/othertest.cer
diff --git a/spec/data/windows_certificates/test.cer b/spec/data/windows_certificates/test.cer
new file mode 100644
index 0000000000..0f32d742c1
--- /dev/null
+++ b/spec/data/windows_certificates/test.cer
diff --git a/spec/data/windows_certificates/test.pem b/spec/data/windows_certificates/test.pem
new file mode 100644
index 0000000000..cfbec5d188
--- /dev/null
+++ b/spec/data/windows_certificates/test.pem
@@ -0,0 +1,21 @@
+-----BEGIN CERTIFICATE-----
+MIIDgDCCAmigAwIBAgIQEyXvJXC8z6lBIxwnT7/d5jANBgkqhkiG9w0BAQsFADBD
+MRwwGgYDVQQDDBNBIER1bW15IENlcnRpZmljYXRlMSMwIQYJKoZIhvcNAQkBFhR0
+ZXN0Ynlyc3BlY0BjaGVmLmNvbTAeFw0xOTAxMjMxODEzNTBaFw0yMDAxMjMxODMz
+NTBaMEMxHDAaBgNVBAMME0EgRHVtbXkgQ2VydGlmaWNhdGUxIzAhBgkqhkiG9w0B
+CQEWFHRlc3RieXJzcGVjQGNoZWYuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A
+MIIBCgKCAQEA1IPsH+S+HKVsJJDuHsqgSQnWAWp7SsBqwnx/t/NZAM6g41mbwafP
+EZixFB5G6VAIiUosHcLhFwz00uPwVZIDND1Ez4TxACraF0iJQpy2kmriDq449ccu
+fn/d8k417Vj0Hm7mcNpv6uaQrjYhIYFHXKV5aQS/OROQGvwFuWe56uJI25ua9lWR
+8yBR621bgn6oW7elBZ8YDQAH88Y0LNo15FBeL2IDUXHBajEfkIRDE3BH+8zcuK4g
+RnRJYBBkzFCXvTXLcRyr1zXaow31TeECrUdPGgBO+nTpLqWYWTylAv36C1nMYBn2
+5ItKAsswVEpQMIeQ5ysfaab0Ei3DRZIEjQIDAQABo3AwbjAOBgNVHQ8BAf8EBAMC
+BaAwHQYDVR0lBBYwFAYIKwYBBQUHAwIGCCsGAQUFBwMBMB4GA1UdEQQXMBWCE3d3
+dy50ZXN0Ynlyc3BlYy5jb20wHQYDVR0OBBYEFMeiyQLCtZBHbmVnvCkoDnRkR+tB
+MA0GCSqGSIb3DQEBCwUAA4IBAQA1hy2yADJ9ULaQMduBt0PiVKP+UKD87OQj0pJK
+vFE7WVSxWaphA4XS15hityJt4eHmGF8R6tNxip7eS2mloGGMguijslqvQLICeeCN
+/7Ov9CsJJG3R8xVrbEZkPExUbV8swJX68GoVxPi4nSj2TFhizBScaOKLedzIXtv5
+hGSXpl3RfETckTq1wmIVEQE9CUoWkea74zvGc5wXTi3r2ZZxof6olGELqT8W/jyT
+vSzUDIC0iwuSVS0AyonBlAnA34ak3Q6a0RCZGK3l1IYz6Cb1JbHHpuCDZPPHooBi
+Hbd+SuvfCH9DLgDFJCAOg+X7WCMQAoy9gCY8Ne5oBTYyjmCz
+-----END CERTIFICATE-----
diff --git a/spec/data/windows_certificates/test.pfx b/spec/data/windows_certificates/test.pfx
new file mode 100644
index 0000000000..c774e12efd
--- /dev/null
+++ b/spec/data/windows_certificates/test.pfx
diff --git a/spec/functional/resource/windows_certificate_spec.rb b/spec/functional/resource/windows_certificate_spec.rb
new file mode 100644
index 0000000000..188a0dc28d
--- /dev/null
+++ b/spec/functional/resource/windows_certificate_spec.rb
@@ -0,0 +1,340 @@
+# Author: Nimesh Patni (nimesh.patni@msystechnologies.com)
+# Copyright: Copyright 2008-2018, Chef Software, Inc.
+# License: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "spec_helper"
+require "chef/mixin/powershell_out"
+require "chef/resource/windows_certificate"
+
+module WindowsCertificateHelper
+  include Chef::Mixin::PowershellOut
+
+  def create_store(store)
+    path = "Cert:\\LocalMachine\\" + store
+    command = <<~EOC
+      New-Item -Path #{path}
+    EOC
+    powershell_out(command)
+  end
+
+  def cleanup(store)
+    path = "Cert:\\LocalMachine\\" + store
+    command = <<~EOC
+      Remove-Item -Path #{path} -Recurse
+    EOC
+    powershell_out(command)
+  end
+
+  def no_of_certificates
+    path = "Cert:\\LocalMachine\\" + store
+    command = <<~EOC
+      Write-Host (dir #{path} | measure).Count;
+    EOC
+    powershell_out(command).stdout.to_i
+  end
+end
+
+describe Chef::Resource::WindowsCertificate, :windows_only, :appveyor_only do
+  include WindowsCertificateHelper
+
+  let(:stdout) { StringIO.new }
+  let(:username) { "ChefFunctionalTest" }
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:new_resource) { Chef::Resource::WindowsCertificate.new(username, run_context) }
+  let(:password) { "P@ssw0rd!" }
+  let(:store) { "Chef-Functional-Test" }
+  let(:certificate_path) { File.expand_path(File.join(CHEF_SPEC_DATA, "windows_certificates")) }
+  let(:cer_path) { File.join(certificate_path, "test.cer") }
+  let(:pem_path) { File.join(certificate_path, "test.pem") }
+  let(:out_path) { File.join(certificate_path, "testout.pem") }
+  let(:tests_thumbprint) { "3180B3E3217862600BD7B2D28067B03D41576A4F" }
+  let(:other_cer_path) { File.join(certificate_path, "othertest.cer") }
+  let(:others_thumbprint) { "AD393859B2D2D4161D224F16CBD3D16555753A20" }
+
+  before do
+    opts = { store_name: store }
+    key = :store_name
+    to_be = ["TRUSTEDPUBLISHER", "TrustedPublisher", "CLIENTAUTHISSUER",
+             "REMOTE DESKTOP", "ROOT", "TRUSTEDDEVICES", "WEBHOSTING",
+             "CA", "AUTHROOT", "TRUSTEDPEOPLE", "MY", "SMARTCARDROOT", "TRUST",
+             "DISALLOWED"]
+
+    # Byepassing the validation so that we may create a custom store
+    allow_any_instance_of(Chef::Mixin::ParamsValidate)
+          .to receive(:_pv_equal_to)
+          .with(opts, key, to_be)
+          .and_return(true)
+
+    # Creating a custom store for the testing
+    create_store(store)
+
+    allow(Chef::Log).to receive(:info) do |msg|
+      stdout.puts(msg)
+    end
+  end
+
+  after { cleanup(store) }
+
+  subject(:win_certificate) do
+    new_resource.store_name = store
+    new_resource
+  end
+
+  it "Initially there are no certificates" do
+    expect(no_of_certificates).to eq(0)
+  end
+
+  describe "action :create" do
+    before do
+      win_certificate.source = cer_path
+      win_certificate.run_action(:create)
+    end
+
+    context "Adding a certificate" do
+      it "Imports certificate into store" do
+        expect(no_of_certificates).to eq(1)
+      end
+
+      it "Converges while addition" do
+        expect(win_certificate).to be_updated_by_last_action
+      end
+    end
+
+    context "Again adding the same certificate" do
+      before do
+        win_certificate.run_action(:create)
+      end
+      it "Does not imports certificate into store" do
+        expect(no_of_certificates).to eq(1)
+      end
+      it "Idempotent: Does not converge while addition" do
+        expect(no_of_certificates).to eq(1)
+        expect(win_certificate).not_to be_updated_by_last_action
+      end
+    end
+
+    context "Again adding the same certificate of other format" do
+      before do
+        win_certificate.source = pem_path
+        win_certificate.run_action(:create)
+      end
+      it "Does not imports certificate into store" do
+        expect(no_of_certificates).to eq(1)
+      end
+      it "Idempotent: Does not converge while addition" do
+        expect(no_of_certificates).to eq(1)
+        expect(win_certificate).not_to be_updated_by_last_action
+      end
+    end
+
+    context "Adding another certificate" do
+      before do
+        win_certificate.source = other_cer_path
+        win_certificate.run_action(:create)
+      end
+      it "Imports certificate into store" do
+        expect(no_of_certificates).to eq(2)
+      end
+      it "Converges while addition" do
+        expect(no_of_certificates).to eq(2)
+        expect(win_certificate).to be_updated_by_last_action
+      end
+    end
+  end
+
+  describe "action: verify" do
+    context "When a certificate is not present" do
+      before do
+        win_certificate.source = tests_thumbprint
+        win_certificate.run_action(:verify)
+      end
+      it "Initial check if certificate is not present" do
+        expect(no_of_certificates).to eq(0)
+      end
+      it "Displays correct message" do
+        expect(stdout.string.strip).to eq("Certificate not found")
+      end
+      it "Does not converge while verifying" do
+        expect(win_certificate).not_to be_updated_by_last_action
+      end
+    end
+
+    context "When a certificate is present" do
+      before do
+        win_certificate.source = cer_path
+        win_certificate.run_action(:create)
+      end
+
+      context "For a valid thumbprint" do
+        before do
+          win_certificate.source = tests_thumbprint
+          win_certificate.run_action(:verify)
+        end
+        it "Initial check if certificate is present" do
+          expect(no_of_certificates).to eq(1)
+        end
+        it "Displays correct message" do
+          expect(stdout.string.strip).to eq("Certificate is valid")
+        end
+        it "Does not converge while verifying" do
+          expect(win_certificate).not_to be_updated_by_last_action
+        end
+      end
+
+      context "For an invalid thumbprint" do
+        before do
+          win_certificate.source = others_thumbprint
+          win_certificate.run_action(:verify)
+        end
+        it "Initial check if certificate is present" do
+          expect(no_of_certificates).to eq(1)
+        end
+        it "Displays correct message" do
+          expect(stdout.string.strip).to eq("Certificate not found")
+        end
+        it "Does not converge while verifying" do
+          expect(win_certificate).not_to be_updated_by_last_action
+        end
+      end
+    end
+  end
+
+  describe "action: fetch" do
+    context "When a certificate is not present" do
+      before do
+        win_certificate.source = tests_thumbprint
+        win_certificate.run_action(:fetch)
+      end
+      it "Initial check if certificate is not present" do
+        expect(no_of_certificates).to eq(0)
+      end
+      it "Does not show any content" do
+        expect(stdout.string.strip).to be_empty
+      end
+      it "Does not converge while fetching" do
+        expect(win_certificate).not_to be_updated_by_last_action
+      end
+    end
+
+    context "When a certificate is present" do
+      before do
+        win_certificate.source = cer_path
+        win_certificate.run_action(:create)
+      end
+
+      after do
+        if File.exists?(out_path)
+          File.delete(out_path)
+        end
+      end
+
+      context "For a valid thumbprint" do
+        before do
+          win_certificate.source = tests_thumbprint
+          win_certificate.cert_path = out_path
+          win_certificate.run_action(:fetch)
+        end
+        it "Initial check if certificate is present" do
+          expect(no_of_certificates).to eq(1)
+        end
+        it "Stores Certificate content at given path" do
+          expect(File.exists?(out_path)).to be_truthy
+        end
+        it "Does not converge while fetching" do
+          expect(win_certificate).not_to be_updated_by_last_action
+        end
+      end
+
+      context "For an invalid thumbprint" do
+        before do
+          win_certificate.source = others_thumbprint
+          win_certificate.cert_path = out_path
+          win_certificate.run_action(:fetch)
+        end
+        it "Initial check if certificate is present" do
+          expect(no_of_certificates).to eq(1)
+        end
+        it "Does not show any content" do
+          expect(stdout.string.strip).to be_empty
+        end
+        it "Does not store certificate content at given path" do
+          expect(File.exists?(out_path)).to be_falsy
+        end
+        it "Does not converge while fetching" do
+          expect(win_certificate).not_to be_updated_by_last_action
+        end
+      end
+    end
+  end
+
+  describe "action: delete" do
+    context "When a certificate is not present" do
+      before do
+        win_certificate.source = tests_thumbprint
+        win_certificate.run_action(:delete)
+      end
+      it "Initial check if certificate is not present" do
+        expect(no_of_certificates).to eq(0)
+      end
+      it "Does not delete any certificate" do
+        expect(stdout.string.strip).to be_empty
+      end
+    end
+
+    context "When a certificate is present" do
+      before do
+        win_certificate.source = cer_path
+        win_certificate.run_action(:create)
+      end
+      before { win_certificate.source = tests_thumbprint }
+      it "Initial check if certificate is present" do
+        expect(no_of_certificates).to eq(1)
+      end
+      it "Deletes the certificate" do
+        win_certificate.run_action(:delete)
+        expect(no_of_certificates).to eq(0)
+      end
+      it "Converges while deleting" do
+        win_certificate.run_action(:delete)
+        expect(win_certificate).to be_updated_by_last_action
+      end
+      it "Idempotent: Does not converge while deleting again" do
+        win_certificate.run_action(:delete)
+        win_certificate.run_action(:delete)
+        expect(no_of_certificates).to eq(0)
+        expect(win_certificate).not_to be_updated_by_last_action
+      end
+      it "Deletes the valid certificate" do
+        # Add another certificate"
+        win_certificate.source = other_cer_path
+        win_certificate.run_action(:create)
+        expect(no_of_certificates).to eq(2)
+
+        # Delete previously added certificate
+        win_certificate.source = tests_thumbprint
+        win_certificate.run_action(:delete)
+        expect(no_of_certificates).to eq(1)
+
+        # Verify another certificate still exists
+        win_certificate.source = others_thumbprint
+        win_certificate.run_action(:verify)
+        expect(stdout.string.strip).to eq("Certificate is valid")
+      end
+    end
+  end
+end