chef client is successfully reading from the windows cert store and attempting to connect to the chef server. Added unit tests and corrected a test in the windows_hostname_spec to remove WMI calls

Signed-off-by: John McCrae <john.mccrae@progress.com>

4 files changed, 161 insertions, 159 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index b564285e86..25fa936c03 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -20,8 +20,6 @@
 
 require_relative "config"
 require_relative "mixin/params_validate"
-require "chef/mixin/powershell_exec"
-require "chef/mixin/shell_out"
 require "chef-utils/dsl/default_paths" unless defined?(ChefUtils::DSL::DefaultPaths)
 require_relative "log"
 require_relative "deprecated"
@@ -67,8 +65,6 @@ class Chef
   # syncs cookbooks if necessary, and triggers convergence.
   class Client
     extend Chef::Mixin::Deprecation
-    include Chef::Mixin::PowershellExec
-    include Chef::Mixin::ShellOut
 
     extend Forwardable
     #
@@ -233,7 +229,7 @@ class Chef
       start_profiling
 
       runlock = RunLock.new(Chef::Config.lockfile)
-      # TODO feels like acquire should have its own block arg for this
+      # TODO: feels like acquire should have its own block arg for this
       runlock.acquire
       # don't add code that may fail before entering this section to be sure to release lock
       begin
@@ -642,21 +638,16 @@ class Chef
     # @api private
     #
     def register(client_name = node_name, config = Chef::Config)
-      puts "\n cli.rb - This is the client name I am using : #{client_name}"
-      puts "\n cli.rb - This is my client key : #{config[:client_key]}"
-      if !config[:client_key]
-        events.skipping_registration(client_name, config)
-        logger.trace("Client key is unspecified - skipping registration")
-      elsif detect_certificate_key(client_name)
+      if Chef::HTTP::Authenticator.detect_certificate_key(client_name)
         events.skipping_registration(client_name, config)
         logger.trace("Client key #{client_name} is present in certificate repository - skipping registration")
-        puts "\n cli.rb - Did I correctly detect a client key in the certstore or keychain : #{detect_certificate_key(client_name)}"
-        puts "\n cli.rb - Hey, I found an appropriate key in the keychain or Certstore!"
+      elsif !config[:client_key]
+        events.skipping_registration(client_name, config)
+        logger.trace("Client key is unspecified - skipping registration")
       elsif File.exist?(config[:client_key])
         events.skipping_registration(client_name, config)
         logger.trace("Client key #{config[:client_key]} is present - skipping registration")
       else
-        puts "\n cli.rb - Did I correctly detect a client key in the certstore or keychain : #{detect_certificate_key(client_name)}"
         events.registration_start(node_name, config)
         logger.info("Client key #{config[:client_key]} is not present - registering")
         Chef::ApiClient::Registration.new(node_name, config[:client_key]).run
@@ -672,60 +663,6 @@ class Chef
     end
 
     #
-    # Detects if a private key exists in a certificate repository like Keychain (macOS) or Certificate Store (Windows)
-    #
-    # @param client_name - we're using the node name to store and retrieve any keys
-    # Returns true if a key is found, false if not. False will trigger a registration event which will lead to a certificate based key being created
-    #
-    #
-    def detect_certificate_key(client_name)
-      if ChefUtils.windows?
-        check_certstore_for_key(client_name)
-      elsif ChefUtils.macos?
-        puts "Made it to checking the macos key"
-        check_keychain_for_key(client_name)
-      else # generic return for Linux systemss
-        false
-      end
-    end
-
-    def check_certstore_for_key(client_name)
-      powershell_code = <<~CODE
-        $cert = Get-ChildItem -path cert:\\LocalMachine\\My -Recurse -Force  | Where-Object { $_.Subject -Match "#{client_name}" } -ErrorAction Stop
-        if ($cert) {
-          return $true
-        }
-        else{
-          return $false
-        }
-      CODE
-      powershell_exec!(powershell_code).result
-    end
-
-    # below we're checking Keychain for the presence of a cert.
-    # We compare the output we get because the code throws an error (127) if the cert isn't present
-    def check_keychain_for_key(client_name)
-      mine = shell_out! <<~EOH
-      getcert() {
-          CERTSTATUS=$(security find-certificate -c #{client_name} >/dev/null)
-          SUB='#{client_name}'
-          if [[ "$CERTSTATUS" == *"$SUB"* ]]; then
-              true
-          else
-              false
-          fi
-      }
-      getcert
-      getcertresult=$?
-      echo $getcertresult
-      EOH
-
-      # The bash script above checks for a cert in Keychain and returns a 127 if not present, 0 if present.
-      output = mine.run_command
-      output == '0' ? true : false
-    end
-
-    #
     # Converges all compiled resources.
     #
     # Fires the converge_start, converge_complete and converge_failed events.
@@ -817,7 +754,7 @@ class Chef
     end
 
     # Notification registration
-    class << self
+    class<<self
       #
       # Add a listener for the 'client run started' event.
       #
@@ -929,6 +866,12 @@ class Chef
     end
 
     def start_profiling
+      if Chef::Config[:slow_report]
+        require_relative "handler/slow_report"
+
+        Chef::Config.report_handlers << Chef::Handler::SlowReport.new(Chef::Config[:slow_report])
+      end
+
       return unless Chef::Config[:profile_ruby]
 
       profiling_prereqs!
@@ -947,7 +890,7 @@ class Chef
     end
 
     def empty_directory?(path)
-      !File.exists?(path) || (Dir.entries(path).size <= 2)
+      !File.exist?(path) || (Dir.entries(path).size <= 2)
     end
 
     def is_last_element?(index, object)
diff --git a/lib/chef/http/authenticator.rb b/lib/chef/http/authenticator.rb
index eda722b255..1fe406d209 100644
--- a/lib/chef/http/authenticator.rb
+++ b/lib/chef/http/authenticator.rb
@@ -17,6 +17,7 @@
 #
 
 require "chef/mixin/powershell_exec"
+require "chef/mixin/shell_out"
 require_relative "auth_credentials"
 require_relative "../exceptions"
 autoload :OpenSSL, "openssl"
@@ -24,11 +25,14 @@ autoload :OpenSSL, "openssl"
 class Chef
   class HTTP
     class Authenticator
-
       DEFAULT_SERVER_API_VERSION = "2".freeze
 
+      extend Chef::Mixin::PowershellExec
       include Chef::Mixin::PowershellExec
 
+      extend Chef::Mixin::ShellOut
+      include Chef::Mixin::ShellOut
+
       attr_reader :signing_key_filename
       attr_reader :raw_key
       attr_reader :attr_names
@@ -86,62 +90,50 @@ class Chef
         @auth_credentials.client_name
       end
 
-      # def load_signing_key(key_file, raw_key = nil)
-      #   if !!key_file
-      #     @raw_key = IO.read(key_file).strip
-      #   elsif !!raw_key
-      #     @raw_key = raw_key.strip
-      #   else
-      #     return nil
-      #   end
-      #   # Pass in '' as the passphrase to avoid OpenSSL prompting on the TTY if
-      #   # given an encrypted key. This also helps if using a single file for
-      #   # both the public and private key with ssh-agent mode.
-      #   @key = OpenSSL::PKey::RSA.new(@raw_key, "")
-      # rescue SystemCallError, IOError => e
-      #   Chef::Log.warn "Failed to read the private key #{key_file}: #{e.inspect}"
-      #   raise Chef::Exceptions::PrivateKeyMissing, "I cannot read #{key_file}, which you told me to use to sign requests!"
-      # rescue OpenSSL::PKey::RSAError
-      #   msg = "The file #{key_file} or :raw_key option does not contain a correctly formatted private key or the key is encrypted.\n"
-      #   msg << "The key file should begin with '-----BEGIN RSA PRIVATE KEY-----' and end with '-----END RSA PRIVATE KEY-----'"
-      #   raise Chef::Exceptions::InvalidPrivateKey, msg
-      # end
+      def detect_certificate_key(client_name)
+        self.class.detect_certificate_key(client_name)
+      end
+
+      def check_certstore_for_key(client_name)
+        self.class.check_certstore_for_key(client_name)
+      end
+
+      # Detects if a private key exists in a certificate repository like Keychain (macOS) or Certificate Store (Windows)
+      #
+      # @param client_name - we're using the node name to store and retrieve any keys
+      # Returns true if a key is found, false if not. False will trigger a registration event which will lead to a certificate based key being created
+      #
+      #
+      def self.detect_certificate_key(client_name)
+        if ChefUtils.windows?
+          check_certstore_for_key(client_name)
+        else # generic return for Mac and LInux clients
+          false
+        end
+      end
+
+      def self.check_certstore_for_key(client_name)
+        powershell_code = <<~CODE
+          $cert = Get-ChildItem -path cert:\\LocalMachine\\My -Recurse -Force  | Where-Object { $_.Subject -Match "#{client_name}" } -ErrorAction Stop
+          if ($cert) {
+            return $true
+          }
+          else{
+            return $false
+          }
+        CODE
+        powershell_exec!(powershell_code).result
+      end
 
       def load_signing_key(key_file, raw_key = nil)
+
         results = retrieve_certificate_key(Chef::Config[:node_name])
-        puts "\n"
-        puts " auth.rb - This is the key file name I am trying to load : #{key_file}"
-        puts " auth.rb - Is there a raw_key name? : #{raw_key ? raw_key : false}"
-        puts " auth.rb - Chef Config thinks this is my node name : #{Chef::Config[:node_name]}"
-        puts " auth.rb - Is the Global @node_name available here? : #{@node_Name ? @node_name : false}"
-        puts "\n"
+
         if key_file == nil? && raw_key == nil?
           puts "\nNo key detected\n"
         elsif results != false
-          # results variable holds 2 values - "False" or the contents of a key.
+          # results variable can be 1 of 2 values - "False" or the contents of a key.
           @raw_key = results
-          puts "\n"
-          puts "Hey. I think I got a key back! #{results}"
-        # first time chef-client runs, generate node-name and burn that to the client.rb
-        # use that node name to create a p12/pfx on the fly
-        # store that
-        # add in code to dump key from certstore
-        # what key am I looking for? Should it be named Chef-Client or the S/N of the node or what? - it is the node name as derived from what is listed in the client.rb
-        # - Open the client.rb, ::File.read(client.rb)
-        # - Hit that with a regex to get the node name out
-        #
-        #
-        #   contents = ::File.read(::File.join(ChefConfig::Config.etc_chef_dir, "client.rb"))
-        #   node_name = contents.match(/^node_name.*$/).to_s.split(" ")[1].split(/\W+/)
-        #
-        # - Extract the pfx that matches that from Progress Key hive in HKLM I created earlier, node_name : some_random_value
-        # - extract the key, delete anything on disk
-        # - connect to Chef Server
-        # key_file and raw_key nil, check the certstore if Windows, keychain if Mac?
-        #
-        # Chef::Logger("Could not find the key in cert store, falling back to client.pem on disk")
-        # Check for the presence of a validation.pem file and delete it after my new client key is registered with the chef server.
-        #
         elsif !!key_file
           @raw_key = IO.read(key_file).strip
         elsif !!raw_key
@@ -162,49 +154,59 @@ class Chef
         raise Chef::Exceptions::InvalidPrivateKey, msg
       end
 
-      def retrieve_certificate_key(client_name)
-        if ChefUtils.windows?
-          check_and_retrieve_certstore_for_key(client_name)
-        elsif ChefUtils.macos?
-          check_keychain_for_key(client_name)
-        else # generic return for Linux systemss
-          false
-        end
-      end
 
-      def check_and_retrieve_certstore_for_key(client_name)
+      # def retrieve_certificate_key(client_name)
+      #   if ChefUtils.windows?
+      #     check_and_retrieve_private_key(client_name)
+      #   else # generic return for Linux systemss
+      #     false
+      #   end
+      # end
+
+      def retrieve_certificate_key(client_name)
         # This code block assumes a certificate with a subject name like "Chef-<node-name>" is in the \LocalMachine\My store and
-        # that here is a password stored in the registry to be used to export the pfx with.
+        # that there is a password stored in the registry to be used to export the pfx with.
 
         require "openssl" unless defined?(OpenSSL)
 
-        powershell_password_code = <<~CODE
-            Try {
-                Get-ItemPropertyValue -Path "HKLM:\\Software\\Progress\\Authenticator" -Name "PfxPass" -ErrorAction Stop;
-            }
-            Catch {
-                return $false
-            }
-        CODE
-        password = powershell_exec!(powershell_password_code).result
-
-        powershell_code = <<~CODE
-            Try {
-                $pfspass = "#{password}"
-                $my_pwd = ConvertTo-SecureString -String $pfspass -Force -AsPlainText;
-                $cert = Get-ChildItem -path cert:\\LocalMachine\\My -Recurse | Where-Object { $_.Subject -match "#{client_name}" } -ErrorAction Stop;
-                $tempfile = [System.IO.Path]::GetTempPath() + "exportpfx.pfx";
-                Export-PfxCertificate -Cert $cert -Password $my_pwd -FilePath $tempfile;
-            }
-            Catch {
-                return $false
-            }
-        CODE
-        myresult = powershell_exec!(powershell_code).result
-
-        pkcs = OpenSSL::PKCS12.new(File.binread(myresult["PSPath"].split("::")[1]), password)
-        ::File.delete(myresult["PSPath"].split("::")[1])
-        return OpenSSL::PKey::RSA.new(pkcs.key.to_pem)
+        if ChefUtils.windows?
+          powershell_password_code = <<~CODE
+              Try {
+                  Get-ItemPropertyValue -Path "HKLM:\\Software\\Progress\\Authenticator" -Name "PfxPass" -ErrorAction Stop;
+              }
+              Catch {
+                  return $false
+              }
+          CODE
+          password = powershell_exec!(powershell_password_code).result
+
+          if password == false
+            return false
+          end
+
+          powershell_code = <<~CODE
+              Try {
+                  $my_pwd = ConvertTo-SecureString -String "#{password}" -Force -AsPlainText;
+                  $cert = Get-ChildItem -path cert:\\LocalMachine\\My -Recurse | Where-Object { $_.Subject -match "#{client_name}" } -ErrorAction Stop;
+                  $tempfile = [System.IO.Path]::GetTempPath() + "exportpfx.pfx";
+                  Export-PfxCertificate -Cert $cert -Password $my_pwd -FilePath $tempfile;
+              }
+              Catch {
+                  return $false
+              }
+          CODE
+          myresult = powershell_exec!(powershell_code).result
+
+          if myresult != false
+            pkcs = OpenSSL::PKCS12.new(File.binread(myresult["PSPath"].split("::")[1]), password)
+            ::File.delete(myresult["PSPath"].split("::")[1])
+            OpenSSL::PKey::RSA.new(pkcs.key.to_pem)
+          else
+            false
+          end
+        else # doing nothing for the Mac and Linux clients
+          false
+        end
       end
 
       def authentication_headers(method, url, json_body = nil, headers = nil)
diff --git a/spec/functional/resource/windows_hostname_spec.rb b/spec/functional/resource/windows_hostname_spec.rb
index a0a58b2919..98ef0f6964 100644
--- a/spec/functional/resource/windows_hostname_spec.rb
+++ b/spec/functional/resource/windows_hostname_spec.rb
@@ -22,8 +22,9 @@ require "chef/resource/hostname"
 describe Chef::Resource::Hostname, :windows_only do
   include Chef::Mixin::PowershellExec
 
+  # Returns either the name of the domain or the word "workgroup" if not domain-bound
   def get_domain_status
-    powershell_exec!("(Get-WmiObject -Class Win32_ComputerSystem).PartofDomain").result
+    powershell_exec!("(Get-CimInstance Win32_ComputerSystem).Domain").result
   end
 
   let(:new_hostname) { "New-Hostname" }
diff --git a/spec/integration/client/client_spec.rb b/spec/integration/client/client_spec.rb
index ecd8ec5bb5..595f2f2e80 100644
--- a/spec/integration/client/client_spec.rb
+++ b/spec/integration/client/client_spec.rb
@@ -4,6 +4,7 @@ require "chef/mixin/shell_out"
 require "tiny_server"
 require "tmpdir"
 require "chef-utils/dist"
+require "chef/mixin/powershell_exec"
 
 describe "chef-client" do
 
@@ -31,8 +32,36 @@ describe "chef-client" do
     @server = @api = nil
   end
 
+  def install_certificate_in_store
+    if ChefUtils.windows?
+      powershell_exec!("New-SelfSignedCertificate -certstorelocation cert:\\localmachine\\my -Subject #{client_name} -FriendlyName #{client_name}")
+    end
+  end
+
+  def create_registry_key
+    powershell_exec! <<~EOH
+      $pfx_password = New-Object -TypeName PSObject
+      $pfx_password | Add-Member -MemberType ScriptProperty -Name "Password" -Value { ("~!@#$%^&*_-+=`|\\(){}[<]:;'>,.?/0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz".tochararray() | Sort-Object { Get-Random })[0..14] -join '' }
+      if (-not (Test-Path HKLM:\\SOFTWARE\\Progress)){
+        New-Item -Path "HKLM:\\SOFTWARE\\Progress\\Authenticator" -Force
+        New-ItemProperty  -path "HKLM:\\SOFTWARE\\Progress\\Authenticator" -name "PfxPass" -value $pfxpassword -PropertyType String
+      }
+    EOH
+  end
+
+  def remove_certificate_from_store
+    powershell_exec! <<~EOH
+      Get-ChildItem -path cert:\\LocalMachine\\My -Recurse -Force  | Where-Object { $_.Subject -Match "#{client_name}" } -ErrorAction Stop | Remove-Item
+    EOH
+  end
+
+  def remove_registry_key
+    powershell_exec!("Remove-Item -Path HKLM:\\SOFTWARE\\Progress -Recurse")
+  end
+
   include IntegrationSupport
   include Chef::Mixin::ShellOut
+  include Chef::Mixin::PowershellExec
 
   let(:chef_dir) { File.join(__dir__, "..", "..", "..") }
 
@@ -47,6 +76,7 @@ describe "chef-client" do
   # cf. CHEF-4914
   let(:chef_client) { "bundle exec #{ChefUtils::Dist::Infra::CLIENT} --minimal-ohai" }
   let(:chef_solo) { "bundle exec #{ChefUtils::Dist::Solo::EXEC} --legacy-mode --minimal-ohai" }
+  let(:client_name) { "chef-973334" }
 
   when_the_repository "has a cookbook with a no-op recipe" do
     before { file "cookbooks/x/recipes/default.rb", "" }
@@ -115,6 +145,32 @@ describe "chef-client" do
       result.error!
     end
 
+    if ChefUtils.windows?
+      context "and the private key is in the Windows CertStore" do
+        before(:each) do
+          # install the p12/pfx and make sure the key and password are stored in the registry
+          install_certificate_in_store
+          create_registry_key
+        end
+
+        after(:each) do
+          # remove the p12/pfx and remove the registry key
+          remove_certificate_from_store
+          remove_registry_key
+        end
+
+        it "should verify that the cert is loaded in the LocalMachine\\My" do
+          expect(Chef::HTTP::Authenticator).to receive(:check_certstore_for_key).and_call_original
+          expect(Chef::HTTP::Authenticator.check_certstore_for_key(client_name)).to eq(true)
+        end
+
+        it "should verify that a private key is returned to me" do
+          expect(Chef::HTTP::Authenticator).to receive(:retrieve_certificate_key).and_call_original
+          expect(Chef::HTTP::Authenticator.retrieve_certificate_key(client_name)).to match(/Private/)
+        end
+      end
+    end
+
     context "and a private key" do
       before do
         file "mykey.pem", <<~EOM