From 0428639746cc6a3b069fd600200feae6dfabaf53 Mon Sep 17 00:00:00 2001 From: Nimesh-Msys Date: Wed, 16 Jan 2019 20:02:14 +0530 Subject: Maintaining idempotency in windows_certificate resource - Minor fixes in :create action - DRYed up `verify_cert` action and reusing the same while :create - Chefstyle maintained Signed-off-by: Nimesh-Msys --- lib/chef/resource/windows_certificate.rb | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/chef/resource/windows_certificate.rb b/lib/chef/resource/windows_certificate.rb index cf0b46dd6b..40b20658e5 100644 --- a/lib/chef/resource/windows_certificate.rb +++ b/lib/chef/resource/windows_certificate.rb @@ -59,7 +59,21 @@ 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 + + # Check whether a certificate with this thumbprint + # is already present in certificate store + exists = verify_cert(thumbprint) + + if (!!exists == exists) && exists + Chef::Log.info("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 @@ -95,6 +109,8 @@ class Chef converge_by("Deleting certificate #{new_resource.source} from Store #{new_resource.store_name}") do delete_cert end + else + Chef::Log.info("Certificate not found") end end @@ -135,9 +151,9 @@ class Chef store.get(new_resource.source) end - def verify_cert + 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) -- cgit v1.2.1 From 6623054c8f276ba4c3633292df0e6d00b5c6afa4 Mon Sep 17 00:00:00 2001 From: Nimesh-Msys Date: Thu, 17 Jan 2019 15:15:42 +0530 Subject: Review comment fixes - Added comments and changed log level to debug at the required places. Signed-off-by: Nimesh-Msys --- lib/chef/resource/windows_certificate.rb | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/lib/chef/resource/windows_certificate.rb b/lib/chef/resource/windows_certificate.rb index 40b20658e5..8b6a52711a 100644 --- a/lib/chef/resource/windows_certificate.rb +++ b/lib/chef/resource/windows_certificate.rb @@ -63,12 +63,10 @@ class Chef 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 - # Check whether a certificate with this thumbprint - # is already present in certificate store - exists = verify_cert(thumbprint) - - if (!!exists == exists) && exists - Chef::Log.info("Certificate is already present") + # 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) @@ -110,7 +108,7 @@ class Chef delete_cert end else - Chef::Log.info("Certificate not found") + Chef::Log.debug("Certificate not found") end end @@ -121,7 +119,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 @@ -151,6 +149,11 @@ class Chef store.get(new_resource.source) 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 def verify_cert(thumbprint = new_resource.source) store = ::Win32::Certstore.open(new_resource.store_name) store.valid?(thumbprint) @@ -257,11 +260,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" @@ -277,6 +284,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-----" -- cgit v1.2.1 From a6a65d48a206571bb1c4467a41d9c2aa4f63f7d4 Mon Sep 17 00:00:00 2001 From: Nimesh-Msys Date: Thu, 24 Jan 2019 02:22:58 +0530 Subject: Adding Functional specs for windows_certificate. - Mostly focus on all major actions and their idempotency. Signed-off-by: Nimesh-Msys --- spec/data/windows_certificates/othertest.cer | Bin 0 -> 912 bytes spec/data/windows_certificates/test.cer | Bin 0 -> 900 bytes spec/data/windows_certificates/test.pem | 21 ++ spec/data/windows_certificates/test.pfx | Bin 0 -> 2701 bytes .../resource/windows_certificate_spec.rb | 340 +++++++++++++++++++++ 5 files changed, 361 insertions(+) create mode 100644 spec/data/windows_certificates/othertest.cer create mode 100644 spec/data/windows_certificates/test.cer create mode 100644 spec/data/windows_certificates/test.pem create mode 100644 spec/data/windows_certificates/test.pfx create mode 100644 spec/functional/resource/windows_certificate_spec.rb diff --git a/spec/data/windows_certificates/othertest.cer b/spec/data/windows_certificates/othertest.cer new file mode 100644 index 0000000000..353f792252 Binary files /dev/null and b/spec/data/windows_certificates/othertest.cer differ diff --git a/spec/data/windows_certificates/test.cer b/spec/data/windows_certificates/test.cer new file mode 100644 index 0000000000..0f32d742c1 Binary files /dev/null and b/spec/data/windows_certificates/test.cer differ 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 Binary files /dev/null and b/spec/data/windows_certificates/test.pfx differ diff --git a/spec/functional/resource/windows_certificate_spec.rb b/spec/functional/resource/windows_certificate_spec.rb new file mode 100644 index 0000000000..03de7a0ebb --- /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 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 -- cgit v1.2.1 From 8924f9a7e9e960af470a1b4bd22583574bd5c757 Mon Sep 17 00:00:00 2001 From: Nimesh-Msys Date: Thu, 24 Jan 2019 11:35:08 +0530 Subject: Should be run only on Appveyor, without affecting anyone elses machine Signed-off-by: Nimesh-Msys --- spec/functional/resource/windows_certificate_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/functional/resource/windows_certificate_spec.rb b/spec/functional/resource/windows_certificate_spec.rb index 03de7a0ebb..188a0dc28d 100644 --- a/spec/functional/resource/windows_certificate_spec.rb +++ b/spec/functional/resource/windows_certificate_spec.rb @@ -47,7 +47,7 @@ module WindowsCertificateHelper end end -describe Chef::Resource::WindowsCertificate, :windows_only do +describe Chef::Resource::WindowsCertificate, :windows_only, :appveyor_only do include WindowsCertificateHelper let(:stdout) { StringIO.new } -- cgit v1.2.1