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

Previously, we only loaded the agent's ssl state once when the agent started,
but not between runs. Since the agent may sleep for upwards of `runinterval`
seconds and those files may change on disk, reload them each time.

The `wait_for_certificates` behavior depends on whether we're running onetime or
not and whether `waitforcert` was specified on the `puppet agent` command line.
So pass those options through.

Enable pending test now that it passes.

Author: joshcooper

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
@@ -137,4 +147,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

spec/integration/application/agent_spec.rb

@@ -836,7 +836,6 @@ def copy_fixtures(sources, dest)
           instance.agent.run(splay: false)
         end
 
-        pending("PUP-11428: the second run should fail due to the revoked client cert")
         agent.command_line.args << '--verbose'
         expect {
           agent.run

spec/unit/agent_spec.rb

@@ -38,6 +38,10 @@ def controlled_run(&block)
         end
       end
     end
+
+    ssl_context = Puppet::SSL::SSLContext.new
+    machine = instance_double("Puppet::SSL::StateMachine", ensure_client_certificate: ssl_context)
+    allow(Puppet::SSL::StateMachine).to receive(:new).and_return(machine)
   end
 
   after do
@@ -197,7 +201,7 @@ def controlled_run(&block)
         @agent.run
       end
 
-      it "should inform that a run is already in progres and try to run every X seconds if waitforlock is used" do
+      it "should inform that a run is already in progress and try to run every X seconds if waitforlock is used" do
         # so the locked file exists
         allow(File).to receive(:file?).and_return(true)
         # so we don't have to wait again for the run to exit (default maxwaitforcert is 60)
@@ -226,7 +230,7 @@ def controlled_run(&block)
         @agent.run
       end
     end
-    
+
     describe "when should_fork is true", :if => Puppet.features.posix? && RUBY_PLATFORM != 'java' do
       before do
         @agent = Puppet::Agent.new(AgentTestClient, true)