Merge pull request #8899 from joshcooper/reload_crls_11428

(PUP-11428) Reload ssl context for each agent run

Author: cthorn42

lib/puppet.rb

@@ -242,20 +242,7 @@ def self.base_context(settings)
     {
       :environments => Puppet::Environments::Cached.new(Puppet::Environments::Combined.new(*loaders)),
       :http_pool => proc { Puppet.runtime[:http].pool },
-      :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], 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 })
-          # TRANSLATORS: `puppet agent -t` is a command and should not be translated
-          Puppet.err(_("Run `puppet agent -t`"))
-          raise e
-        end
-      },
+      :ssl_context => proc { Puppet.runtime[:http].default_ssl_context },
       :ssl_host => proc { Puppet::SSL::Host.localhost(true) },
       :http_session => proc { Puppet.runtime[:http].create_session },
       :plugins => proc { Puppet::Plugins::Configuration.load_plugins },

lib/puppet/agent.rb

@@ -45,19 +45,29 @@ def run(client_options = {})
     result = nil
     wait_for_lock_deadline = nil
     block_run = Puppet::Application.controlled_run do
-      splay client_options.fetch :splay, Puppet[:splay]
+      # splay may sleep for awhile!
+      splay(client_options.fetch(:splay, Puppet[:splay]))
+
+      # waiting for certs may sleep for awhile depending on onetime, waitforcert and maxwaitforcert!
+      # this needs to happen before forking so that if we fail to obtain certs and try to exit, then
+      # we exit the main process and not the forked child.
+      ssl_context = wait_for_certificates(client_options)
+
       result = run_in_fork(should_fork) do
         with_client(client_options[:transaction_uuid], client_options[:job_id]) do |client|
           client_args = client_options.merge(:pluginsync => Puppet::Configurer.should_pluginsync?)
           begin
+            # lock may sleep for awhile depending on waitforlock and maxwaitforlock!
             lock do
               # NOTE: Timeout is pretty heinous as the location in which it
               # throws an error is entirely unpredictable, which means that
               # it can interrupt code blocks that perform cleanup or enforce
               # sanity. The only thing a Puppet agent should do after this
               # error is thrown is die with as much dignity as possible.
               Timeout.timeout(Puppet[:runtimeout], RunTimeoutError) do
-                client.run(client_args)
+                Puppet.override(ssl_context: ssl_context) do
+                  client.run(client_args)
+                end
               end
             end
           rescue Puppet::LockError
@@ -84,6 +94,8 @@ def run(client_options = {})
           rescue StandardError => detail
             Puppet.log_exception(detail, _("Could not run %{client_class}: %{detail}") % { client_class: client_class, detail: detail })
             nil
+          ensure
+            Puppet.runtime[:http].close
           end
         end
       end
@@ -137,4 +149,10 @@ def with_client(transaction_uuid, job_id = nil)
   ensure
     @client = nil
   end
+
+  def wait_for_certificates(options)
+    waitforcert = options[:waitforcert] || (Puppet[:onetime] ? 0 : Puppet[:waitforcert])
+    sm = Puppet::SSL::StateMachine.new(waitforcert: waitforcert, onetime: Puppet[:onetime])
+    sm.ensure_client_certificate
+  end
 end

lib/puppet/application/agent.rb

@@ -383,15 +383,11 @@ def run_command
 
       log_config if Puppet[:daemonize]
 
-      # run ssl state machine, waiting if needed
-      ssl_context = wait_for_certificates
-
       # Each application is responsible for pushing loaders onto the context.
       # Use the current environment that has already been established, though
       # it may change later during the configurer run.
       env = Puppet.lookup(:current_environment)
-      Puppet.override(ssl_context: ssl_context,
-                      current_environment: env,
+      Puppet.override(current_environment: env,
                       loaders: Puppet::Pops::Loaders.new(env, true)) do
         if Puppet[:onetime]
           onetime(daemon)
@@ -434,7 +430,7 @@ def fingerprint
 
   def onetime(daemon)
     begin
-      exitstatus = daemon.agent.run({:job_id => options[:job_id], :start_time => options[:start_time]})
+      exitstatus = daemon.agent.run({:job_id => options[:job_id], :start_time => options[:start_time], :waitforcert => options[:waitforcert]})
     rescue => detail
       Puppet.log_exception(detail)
     end
@@ -524,10 +520,4 @@ def daemonize_process_when(should_daemonize)
 
     daemon
   end
-
-  def wait_for_certificates
-    waitforcert = options[:waitforcert] || (Puppet[:onetime] ? 0 : Puppet[:waitforcert])
-    sm = Puppet::SSL::StateMachine.new(waitforcert: waitforcert)
-    sm.ensure_client_certificate
-  end
 end

lib/puppet/http/client.rb

@@ -25,7 +25,7 @@ class Puppet::HTTP::Client
   #   used if :include_system_store is set to true
   # @param [Integer] redirect_limit default number of HTTP redirections to allow
   #   in a given request. Can also be specified per-request.
-  # @param [Integer] retry_limit number of HTTP reties allowed in a given
+  # @param [Integer] retry_limit number of HTTP retries allowed in a given
   #   request
   #
   def initialize(pool: Puppet::Network::HTTP::Pool.new(Puppet[:http_keepalive_timeout]), ssl_context: nil, system_ssl_context: nil, redirect_limit: 10, retry_limit: 100)
@@ -272,6 +272,24 @@ def delete(url, headers: {}, params: {}, options: {})
   #
   def close
     @pool.close
+    @default_ssl_context = nil
+    @default_system_ssl_context = nil
+  end
+
+  def default_ssl_context
+    cert = Puppet::X509::CertProvider.new
+    password = cert.load_private_key_password
+
+    ssl = Puppet::SSL::SSLProvider.new
+    ctx = ssl.load_context(certname: Puppet[:certname], password: password)
+    ssl.print(ctx)
+    ctx
+  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 })
+    # TRANSLATORS: `puppet agent -t` is a command and should not be translated
+    Puppet.err(_("Run `puppet agent -t`"))
+    raise e
   end
 
   protected
@@ -409,6 +427,8 @@ def system_ssl_context
 
     ssl = Puppet::SSL::SSLProvider.new
     @default_system_ssl_context = ssl.create_system_context(cacerts: cacerts)
+    ssl.print(@default_system_ssl_context)
+    @default_system_ssl_context
   end
 
   def apply_auth(request, basic_auth)

lib/puppet/ssl/ssl_provider.rb

@@ -174,6 +174,27 @@ def verify_request(csr, public_key)
     csr
   end
 
+  def print(ssl_context, alg = 'SHA256')
+    if Puppet::Util::Log.sendlevel?(:debug)
+      chain = ssl_context.client_chain
+      # print from root to client
+      chain.reverse.each_with_index do |cert, i|
+        digest = Puppet::SSL::Digest.new(alg, cert.to_der)
+        if i == chain.length - 1
+          Puppet.debug(_("Verified client certificate '%{subject}' fingerprint %{digest}") % {subject: cert.subject.to_utf8, digest: digest})
+        else
+          Puppet.debug(_("Verified CA certificate '%{subject}' fingerprint %{digest}") % {subject: cert.subject.to_utf8, digest: digest})
+        end
+      end
+      ssl_context.crls.each do |crl|
+        oid_values = Hash[crl.extensions.map { |ext| [ext.oid, ext.value] }]
+        crlNumber = oid_values['crlNumber'] || 'unknown'
+        authKeyId = (oid_values['authorityKeyIdentifier'] || 'unknown').chomp!
+        Puppet.debug("Using CRL '#{crl.issuer.to_utf8}' authorityKeyIdentifier '#{authKeyId}' crlNumber '#{crlNumber }'")
+      end
+    end
+  end
+
   private
 
   def default_flags

lib/puppet/ssl/state_machine.rb

@@ -27,6 +27,15 @@ def to_error(message, cause)
       detail.set_backtrace(cause.backtrace)
       Error.new(@machine, message, detail)
     end
+
+    def log_error(message)
+      # When running daemonized we set stdout to /dev/null, so write to the log instead
+      if Puppet[:daemonize]
+        Puppet.err(message)
+      else
+        $stdout.puts(message)
+      end
+    end
   end
 
   # Load existing CA certs or download them. Transition to NeedCRLs.
@@ -270,15 +279,15 @@ def initialize(machine)
     def next_state
       time = @machine.waitforcert
       if time < 1
-        puts _("Exiting now because the waitforcert setting is set to 0.")
+        log_error(_("Exiting now because the waitforcert setting is set to 0."))
         exit(1)
       elsif Time.now.to_i > @machine.wait_deadline
-        puts _("Couldn't fetch certificate from CA server; you might still need to sign this agent's certificate (%{name}). Exiting now because the maxwaitforcert timeout has been exceeded.") % {name: Puppet[:certname] }
+        log_error(_("Couldn't fetch certificate from CA server; you might still need to sign this agent's certificate (%{name}). Exiting now because the maxwaitforcert timeout has been exceeded.") % {name: Puppet[:certname] })
         exit(1)
       else
         Puppet.info(_("Will try again in %{time} seconds.") % {time: time})
 
-        # close persistent connections and session state before sleeping
+        # close http/tls and session state before sleeping
         Puppet.runtime[:http].close
         @machine.session = Puppet.runtime[:http].create_session
 
@@ -417,20 +426,7 @@ def ensure_ca_certificates
   def ensure_client_certificate
     final_state = run_machine(NeedLock.new(self), Done)
     ssl_context = final_state.ssl_context
-
-    if Puppet::Util::Log.sendlevel?(:debug)
-      chain = ssl_context.client_chain
-      # print from root to client
-      chain.reverse.each_with_index do |cert, i|
-        digest = Puppet::SSL::Digest.new(@digest, cert.to_der)
-        if i == chain.length - 1
-          Puppet.debug(_("Verified client certificate '%{subject}' fingerprint %{digest}") % {subject: cert.subject.to_utf8, digest: digest})
-        else
-          Puppet.debug(_("Verified CA certificate '%{subject}' fingerprint %{digest}") % {subject: cert.subject.to_utf8, digest: digest})
-        end
-      end
-    end
-
+    @ssl_provider.print(ssl_context, @digest)
     ssl_context
   end
 

spec/integration/application/agent_spec.rb

@@ -3,6 +3,7 @@
 require 'puppet_spec/puppetserver'
 require 'puppet_spec/compiler'
 require 'puppet_spec/https'
+require 'puppet/application/agent'
 
 describe "puppet agent", unless: Puppet::Util::Platform.jruby? do
   include PuppetSpec::Files
@@ -737,4 +738,111 @@ def with_another_agent_running(&block)
       end
     end
   end
+
+  context "ssl" do
+    context "bootstrapping" do
+      before :each do
+        # reconfigure ssl to non-existent dir and files to force bootstrapping
+        dir = tmpdir('ssl')
+        Puppet[:ssldir] = dir
+        Puppet[:localcacert] = File.join(dir, 'ca.pem')
+        Puppet[:hostcrl] = File.join(dir, 'crl.pem')
+        Puppet[:hostprivkey] = File.join(dir, 'cert.pem')
+        Puppet[:hostcert] = File.join(dir, 'key.pem')
+
+        Puppet[:daemonize] = false
+        Puppet[:logdest] = 'console'
+        Puppet[:log_level] = 'info'
+      end
+
+      it "exits if the agent is not allowed to wait" do
+        Puppet[:waitforcert] = 0
+
+        server.start_server do |port|
+          Puppet[:serverport] = port
+          expect {
+            agent.run
+          }.to exit_with(1)
+           .and output(%r{Exiting now because the waitforcert setting is set to 0}).to_stdout
+           .and output(%r{Failed to submit the CSR, HTTP response was 404}).to_stderr
+        end
+      end
+
+      it "exits if the maxwaitforcert time is exceeded" do
+        Puppet[:waitforcert] = 1
+        Puppet[:maxwaitforcert] = 1
+
+        server.start_server do |port|
+          Puppet[:serverport] = port
+          expect {
+            agent.run
+          }.to exit_with(1)
+            .and output(%r{Couldn't fetch certificate from CA server; you might still need to sign this agent's certificate \(127.0.0.1\). Exiting now because the maxwaitforcert timeout has been exceeded.}).to_stdout
+            .and output(%r{Failed to submit the CSR, HTTP response was 404}).to_stderr
+        end
+      end
+    end
+
+    def copy_fixtures(sources, dest)
+      ssldir = File.join(PuppetSpec::FIXTURE_DIR, 'ssl')
+      File.open(dest, 'w') do |f|
+        sources.each do |s|
+          f.write(File.read(File.join(ssldir, s)))
+        end
+      end
+    end
+
+    it "reloads the CRL between runs" do
+      Puppet[:localcacert] = ca = tmpfile('ca')
+      Puppet[:hostcrl] = crl = tmpfile('crl')
+      Puppet[:hostcert] = cert = tmpfile('cert')
+      Puppet[:hostprivkey] = key = tmpfile('key')
+
+      copy_fixtures(%w[ca.pem intermediate.pem], ca)
+      copy_fixtures(%w[crl.pem intermediate-crl.pem], crl)
+      copy_fixtures(%w[127.0.0.1.pem], cert)
+      copy_fixtures(%w[127.0.0.1-key.pem], key)
+
+      revoked = cert_fixture('revoked.pem')
+      revoked_key = key_fixture('revoked-key.pem')
+
+      mounts = {}
+      mounts[:catalog] = -> (req, res) {
+        catalog = compile_to_catalog(<<~MANIFEST, node)
+          file { '#{cert}':
+            ensure => file,
+            content => '#{revoked}'
+          }
+          file { '#{key}':
+            ensure => file,
+            content => '#{revoked_key}'
+          }
+        MANIFEST
+
+        res.body = formatter.render(catalog)
+        res['Content-Type'] = formatter.mime
+      }
+
+      server.start_server(mounts: mounts) do |port|
+        Puppet[:serverport] = port
+        Puppet[:daemonize] = false
+        Puppet[:runinterval] = 1
+        Puppet[:waitforcert] = 1
+        Puppet[:maxwaitforcert] = 1
+
+        # simulate two runs of the agent, then return so we don't infinite loop
+        allow_any_instance_of(Puppet::Daemon).to receive(:run_event_loop) do |instance|
+          instance.agent.run(splay: false)
+          instance.agent.run(splay: false)
+        end
+
+        agent.command_line.args << '--verbose'
+        expect {
+          agent.run
+        }.to exit_with(1)
+         .and output(%r{Exiting now because the maxwaitforcert timeout has been exceeded}).to_stdout
+         .and output(%r{Certificate 'CN=revoked' is revoked}).to_stderr
+      end
+    end
+  end
 end