Merge pull request #7502 from joshcooper/encrypted-private-keys

(PUP-9466) Readd encrypted private keys

lib/puppet.rb

@@ -209,8 +209,10 @@ def self.base_context(settings)
       },
       :ssl_context => proc {
         begin
+          cert = Puppet::X509::CertProvider.new
+          password = cert.load_private_key_password
           ssl = Puppet::SSL::SSLProvider.new
-          ssl.load_context(certname: Puppet[:certname])
+          ssl.load_context(certname: Puppet[:certname], password: password)
         rescue => e
           # TRANSLATORS: `message` is an already translated string of why SSL failed to initialize
           Puppet.log_exception(e, _("Failed to initialize SSL: %{message}") % { message: e.message })

lib/puppet/application/agent.rb

@@ -347,12 +347,12 @@ def run_command
       # Setup signal traps immediately after daemonization so we clean up the daemon
       daemon.set_signal_traps
 
-      wait_for_certificates
-
-      if Puppet[:onetime]
-        onetime(daemon)
-      else
-        main(daemon)
+      Puppet.override(ssl_context: wait_for_certificates) do
+        if Puppet[:onetime]
+          onetime(daemon)
+        else
+          main(daemon)
+        end
       end
     end
   end

lib/puppet/application/ssl.rb

@@ -206,7 +206,8 @@ def download_cert(ssl_context)
   end
 
   def verify(certname)
-    ssl_context = @ssl_provider.load_context(certname: certname)
+    password = @cert_provider.load_private_key_password
+    ssl_context = @ssl_provider.load_context(certname: certname, password: password)
 
     # print from root to client
     ssl_context.client_chain.reverse.each_with_index do |cert, i|

lib/puppet/ssl/ssl_provider.rb

@@ -90,13 +90,17 @@ def create_context(cacerts:, crls:, private_key:, client_cert:, revocation: Pupp
   # and private key. Connections made from the returned context will be mutually
   # authenticated.
   #
+  # @param certname [String] Which cert & key to load
   # @param revocation [:chain, :leaf, false] revocation mode
+  # @param password [String, nil] If the private key is encrypted, decrypt
+  #   it using the password. If the key is encrypted, but a password is
+  #   not specified, then the key cannot be loaded.
   # @return [Puppet::SSL::SSLContext] A context to use to create connections
   # @raise [Puppet::SSL::CertVerifyError] There was an issue with
   #   one of the certs or CRLs.
   # @raise [Puppet::Error] There was an issue with one of the required components.
   # @api private
-  def load_context(certname: Puppet[:certname], revocation: Puppet[:certificate_revocation])
+  def load_context(certname: Puppet[:certname], revocation: Puppet[:certificate_revocation], password: nil)
     cert = Puppet::X509::CertProvider.new
     cacerts = cert.load_cacerts(required: true)
     crls = case revocation
@@ -105,10 +109,12 @@ def load_context(certname: Puppet[:certname], revocation: Puppet[:certificate_re
            else
              []
            end
-    private_key = cert.load_private_key(certname, required: true)
+    private_key = cert.load_private_key(certname, required: true, password: password)
     client_cert = cert.load_client_cert(certname, required: true)
 
     create_context(cacerts: cacerts, crls: crls,  private_key: private_key, client_cert: client_cert, revocation: revocation)
+  rescue OpenSSL::PKey::PKeyError => e
+    raise Puppet::SSL::SSLError.new(_("Failed to load private key for host '%{name}': %{message}") % { name: certname, message: e.message }, e)
   end
 
   # Verify the `csr` was signed with a private key corresponding to the

lib/puppet/ssl/state_machine.rb

@@ -100,7 +100,8 @@ class NeedKey < SSLState
     def next_state
       Puppet.debug(_("Loading/generating private key"))
 
-      key = @cert_provider.load_private_key(Puppet[:certname])
+      password = @cert_provider.load_private_key_password
+      key = @cert_provider.load_private_key(Puppet[:certname], password: password)
       if key
         cert = @cert_provider.load_client_cert(Puppet[:certname])
         if cert
@@ -117,7 +118,8 @@ def next_state
           Puppet.info _("Creating a new RSA SSL key for %{name}") % { name: Puppet[:certname] }
           key = OpenSSL::PKey::RSA.new(Puppet[:keylength].to_i)
         end
-        @cert_provider.save_private_key(Puppet[:certname], key)
+
+        @cert_provider.save_private_key(Puppet[:certname], key, password: password)
       end
 
       NeedSubmitCSR.new(@machine, @ssl_context, key)

lib/puppet/x509/cert_provider.rb

@@ -115,11 +115,20 @@ def load_crls_from_pem(pem)
   #
   # @param name [String] The private key identity
   # @param key [OpenSSL::PKey::RSA] private key
+  # @param password [String, nil] If non-nil, derive an encryption key
+  #   from the password, and use that to encrypt the private key. If nil,
+  #   save the private key unencrypted.
   # @raise [Puppet::Error] if the private key cannot be saved
   # @api private
-  def save_private_key(name, key)
+  def save_private_key(name, key, password: nil)
+    pem = if password
+            cipher = OpenSSL::Cipher::AES.new(128, :CBC)
+            key.export(cipher, password)
+          else
+            key.to_pem
+          end
     path = to_path(@privatekeydir, name)
-    save_pem(key.to_pem, path, **permissions_for_setting(:hostprivkey))
+    save_pem(pem, path, **permissions_for_setting(:hostprivkey))
   rescue SystemCallError => e
     raise Puppet::Error.new(_("Failed to save private key for '%{name}'") % {name: name}, e)
   end
@@ -129,37 +138,46 @@ def save_private_key(name, key)
   #
   # @param name [String] The private key identity
   # @param required [Boolean] If true, raise if it is missing
+  # @param password [String, nil] If the private key is encrypted, decrypt
+  #   it using the password. If the key is encrypted, but a password is
+  #   not specified, then the key cannot be loaded.
   # @return (see #load_private_key_from_pem)
   # @raise (see #load_private_key_from_pem)
   # @raise [Puppet::Error] if the private key cannot be loaded
   # @api private
-  def load_private_key(name, required: false)
+  def load_private_key(name, required: false, password: nil)
     path = to_path(@privatekeydir, name)
     pem = load_pem(path)
     if !pem && required
       raise Puppet::Error, _("The private key is missing from '%{path}'") % { path: path }
     end
-    pem ? load_private_key_from_pem(pem) : nil
+    pem ? load_private_key_from_pem(pem, password: password) : nil
   rescue SystemCallError => e
     raise Puppet::Error.new(_("Failed to load private key for '%{name}'") % {name: name}, e)
   end
 
   # Load a PEM encoded private key.
   #
   # @param pem [String] PEM encoded private key
+  # @param password [String, nil] If the private key is encrypted, decrypt
+  #   it using the password. If the key is encrypted, but a password is
+  #   not specified, then the key cannot be loaded.
   # @return [OpenSSL::PKey::RSA, OpenSSL::PKey::EC] The private key
   # @raise [OpenSSL::PKey::PKeyError] The `pem` text does not contain a valid key
   # @api private
-  def load_private_key_from_pem(pem)
-    # set a non-nil passphrase to ensure openssl doesn't prompt
-    # but ruby 2.4.0 & 2.4.1 require at least 4 bytes, see
-    # https://github.com/ruby/ruby/commit/f012932218fd609f75f9268812df61fb26e2d0f1#diff-40e4270ec386990ac60d7ab5ff8045a4
+  def load_private_key_from_pem(pem, password: nil)
+    # set a non-nil password to ensure openssl doesn't prompt
+    # but ruby 2.4.0 & 2.4.1 require at least 4 bytes due to
+    # https://github.com/ruby/openssl/commit/f38501249f33bff7ca9d208670b8cde695ea8b7b
+    # and corrected in https://github.com/ruby/openssl/commit/a896c3d1dfa090e92dec1abf8ac12843af6af721
+    password ||= '    '
+
     if Puppet::Util::Platform.jruby?
       begin
         if pem =~ EC_HEADER
-          OpenSSL::PKey::EC.new(pem, '    ')
+          OpenSSL::PKey::EC.new(pem, password)
         else
-          OpenSSL::PKey::RSA.new(pem, '    ')
+          OpenSSL::PKey::RSA.new(pem, password)
         end
       rescue OpenSSL::PKey::PKeyError => e
         if e.message =~ /Neither PUB key nor PRIV key/
@@ -169,10 +187,20 @@ def load_private_key_from_pem(pem)
         end
       end
     else
-      OpenSSL::PKey.read(pem, '    ')
+      OpenSSL::PKey.read(pem, password)
     end
   end
 
+  # Load the private key password.
+  #
+  # @return [String, nil] The private key password as a binary string or nil
+  #   if there is none.
+  def load_private_key_password
+    Puppet::FileSystem.read(Puppet[:passfile], :encoding => Encoding::BINARY)
+  rescue Errno::ENOENT
+    nil
+  end
+
   # Save a named client cert to the configured `certdir`.
   #
   # @param name [String] The client cert identity

spec/unit/application/ssl_spec.rb

@@ -33,12 +33,12 @@
     Puppet[:vardir] = tmpdir("ssl_testing")
 
     # Host assumes ca cert and crl are present
-    File.open(Puppet[:localcacert], 'w') { |f| f.write(@ca_cert.to_pem) }
-    File.open(Puppet[:hostcrl], 'w') { |f| f.write(@crl.to_pem) }
+    File.write(Puppet[:localcacert], @ca_cert.to_pem)
+    File.write(Puppet[:hostcrl], @crl.to_pem)
 
     # Setup our ssl client
-    File.open(Puppet[:hostprivkey], 'w') { |f| f.write(@host[:private_key].to_pem) }
-    File.open(Puppet[:hostpubkey], 'w') { |f| f.write(@host[:private_key].public_key.to_pem) }
+    File.write(Puppet[:hostprivkey], @host[:private_key].to_pem)
+    File.write(Puppet[:hostpubkey], @host[:private_key].public_key.to_pem)
   end
 
   def expects_command_to_pass(expected_output = nil)
@@ -185,7 +185,7 @@ def expects_command_to_fail(message)
     end
 
     it 'overwrites an existing cert' do
-      File.open(Puppet[:hostcert], 'w') { |f| f.write "existing certificate" }
+      File.write(Puppet[:hostcert], "existing certificate")
 
       stub_request(:get, %r{puppet-ca/v1/certificate/#{name}}).to_return(status: 200, body: @host[:cert].to_pem)
 
@@ -195,12 +195,12 @@ def expects_command_to_fail(message)
     end
 
     it "reports an error if the downloaded cert's public key doesn't match our private key" do
-      File.open(Puppet[:hostcert], 'w') { |f| f.write "existing cert" }
+      File.write(Puppet[:hostcert], "existing cert")
 
       # generate a new host key, whose public key doesn't match the cert
       private_key = OpenSSL::PKey::RSA.new(512)
-      File.open(Puppet[:hostprivkey], 'w') { |f| f.write(private_key.to_pem) }
-      File.open(Puppet[:hostpubkey], 'w') { |f| f.write(private_key.public_key.to_pem) }
+      File.write(Puppet[:hostprivkey], private_key.to_pem)
+      File.write(Puppet[:hostpubkey], private_key.public_key.to_pem)
 
       stub_request(:get, %r{puppet-ca/v1/certificate/#{name}}).to_return(status: 200, body: @host[:cert].to_pem)
 
@@ -220,7 +220,7 @@ def expects_command_to_fail(message)
     before do
       ssl.command_line.args << 'verify'
 
-      File.open(Puppet[:hostcert], 'w') { |f| f.write(@host[:cert].to_pem) }
+      File.write(Puppet[:hostcert], @host[:cert].to_pem)
     end
 
     it 'reports if the key is missing' do
@@ -239,28 +239,44 @@ def expects_command_to_fail(message)
       # generate new keys
       private_key = OpenSSL::PKey::RSA.new(512)
       public_key = private_key.public_key
-      File.open(Puppet[:hostprivkey], 'w') { |f| f.write(private_key.to_pem) }
-      File.open(Puppet[:hostpubkey], 'w') { |f| f.write(public_key.to_pem) }
+      File.write(Puppet[:hostprivkey], private_key.to_pem)
+      File.write(Puppet[:hostpubkey], public_key.to_pem)
 
       expects_command_to_fail(%r{The certificate for 'CN=ssl-client' does not match its private key})
     end
 
     it 'reports if the cert verification fails' do
       # generate a new CA to force an error
       new_ca = Puppet::TestCa.new
-      File.open(Puppet[:localcacert], 'w') { |f| f.write(new_ca.ca_cert.to_pem) }
+      File.write(Puppet[:localcacert], new_ca.ca_cert.to_pem)
 
       # and CRL for that CA
-      File.open(Puppet[:hostcrl], 'w') { |f| f.write(new_ca.ca_crl.to_pem) }
+      File.write(Puppet[:hostcrl], new_ca.ca_crl.to_pem)
 
       expects_command_to_fail(%r{Invalid signature for certificate 'CN=ssl-client'})
     end
 
     it 'reports when verification succeeds' do
-      allow_any_instance_of(OpenSSL::X509::Store).to receive(:verify).and_return(true)
-
       expects_command_to_pass(%r{Verified client certificate 'CN=ssl-client' fingerprint})
     end
+
+    it 'reports when verification succeeds with a password protected private key' do
+      FileUtils.cp(File.join(PuppetSpec::FIXTURE_DIR, 'ssl', 'encrypted-key.pem'), Puppet[:hostprivkey])
+      FileUtils.cp(File.join(PuppetSpec::FIXTURE_DIR, 'ssl', 'signed.pem'), Puppet[:hostcert])
+
+      Puppet[:passfile] = file_containing('passfile', '74695716c8b6')
+
+      expects_command_to_pass(%r{Verified client certificate 'CN=signed' fingerprint})
+    end
+
+    it 'reports if the private key password is incorrect' do
+      FileUtils.cp(File.join(PuppetSpec::FIXTURE_DIR, 'ssl', 'encrypted-key.pem'), Puppet[:hostprivkey])
+      FileUtils.cp(File.join(PuppetSpec::FIXTURE_DIR, 'ssl', 'signed.pem'), Puppet[:hostcert])
+
+      Puppet[:passfile] = file_containing('passfile', 'wrongpassword')
+
+      expects_command_to_fail(/Failed to load private key for host 'ssl-client'/)
+    end
   end
 
   context 'when cleaning' do
@@ -269,32 +285,32 @@ def expects_command_to_fail(message)
     end
 
     it 'deletes the hostcert' do
-      File.open(Puppet[:hostcert], 'w') { |f| f.write(@host[:cert].to_pem) }
+      File.write(Puppet[:hostcert], @host[:cert].to_pem)
 
       expects_command_to_pass(%r{Removed certificate #{Puppet[:cert]}})
     end
 
     it 'deletes the private key' do
-      File.open(Puppet[:hostprivkey], 'w') { |f| f.write(@host[:private_key].to_pem) }
+      File.write(Puppet[:hostprivkey], @host[:private_key].to_pem)
 
       expects_command_to_pass(%r{Removed private key #{Puppet[:hostprivkey]}})
     end
 
     it 'deletes the public key' do
-      File.open(Puppet[:hostpubkey], 'w') { |f| f.write(@host[:private_key].public_key.to_pem) }
+      File.write(Puppet[:hostpubkey], @host[:private_key].public_key.to_pem)
 
       expects_command_to_pass(%r{Removed public key #{Puppet[:hostpubkey]}})
     end
 
     it 'deletes the request' do
       path = File.join(Puppet[:requestdir], "#{Puppet[:certname]}.pem")
-      File.open(path, 'w') { |f| f.write(@host[:csr].to_pem) }
+      File.write(path, @host[:csr].to_pem)
 
       expects_command_to_pass(%r{Removed certificate request #{path}})
     end
 
     it 'deletes the passfile' do
-      File.open(Puppet[:passfile], 'w') { |_| }
+      FileUtils.touch(Puppet[:passfile])
 
       expects_command_to_pass(%r{Removed private key password file #{Puppet[:passfile]}})
     end
@@ -323,13 +339,13 @@ def expects_command_to_fail(message)
       end
 
       it 'deletes the local CA cert' do
-        File.open(Puppet[:localcacert], 'w') { |f| f.write(@ca_cert.to_pem) }
+        File.write(Puppet[:localcacert], @ca_cert.to_pem)
 
         expects_command_to_pass(%r{Removed local CA certificate #{Puppet[:localcacert]}})
       end
 
       it 'deletes the local CRL' do
-        File.open(Puppet[:hostcrl], 'w') { |f| f.write(@crl.to_pem) }
+        File.write(Puppet[:hostcrl], @crl.to_pem)
 
         expects_command_to_pass(%r{Removed local CRL #{Puppet[:hostcrl]}})
       end
@@ -348,15 +364,15 @@ def expects_command_to_fail(message)
       end
 
       it "cleans the cert when the CA is local and the CA has already cleaned its cert" do
-        File.open(Puppet[:hostcert], 'w') { |f| f.write(@host[:cert].to_pem) }
+        File.write(Puppet[:hostcert], @host[:cert].to_pem)
 
         stub_request(:get, %r{puppet-ca/v1/certificate/puppetserver}).to_return(status: 404)
 
         expects_command_to_pass(%r{Removed certificate .*puppetserver.pem})
       end
 
       it "cleans the cert when run on a puppetserver that isn't the CA" do
-        File.open(Puppet[:hostcert], 'w') { |f| f.write(@host[:cert].to_pem) }
+        File.write(Puppet[:hostcert], @host[:cert].to_pem)
 
         Puppet[:ca_server] = 'caserver'
 
@@ -374,7 +390,7 @@ def expects_command_to_fail(message)
         device_cert_path = File.join(Puppet[:devicedir], 'device.example.com', 'ssl', 'certs')
         device_cert_file = File.join(device_cert_path, 'device.example.com.pem')
         FileUtils.mkdir_p(device_cert_path)
-        File.open(device_cert_file, 'w') { |f| f.write('device.example.com') }
+        File.write(device_cert_file, 'device.example.com')
         expects_command_to_pass(%r{Removed certificate #{device_cert_file}})
      end
     end