(PUP-11869) Retry failed CA/CRL refreshes sooner

Prior to this commit, any time a node attempted to refresh its
certificate authority (CA) or certificate revocation list (CRL),
regardless of whether it was successful in doing so, Puppet would update
the modified timestamp (mtime) on the CA or CRL file.

Puppet uses the mtime to decide when to attempt to update a CA or CRL.
As a result, if Puppet attempted but failed to updates either of those
file, it would need to wait the full refresh interval before trying
again.

This commit changes Puppet to only update the modified timestamp on the
CA or CRL files if they have been successfully refreshed. This means
that if Puppet fails to update its CA or CRL, it will try again the next
run interval instead of waiting for the full CA or CRL refresh interval
(which should be greater than the run interval).

Author: mhashizume

lib/puppet/ssl/state_machine.rb

@@ -59,9 +59,6 @@ def next_state
         now = Time.now
         last_update = @cert_provider.ca_last_update
         if needs_refresh?(now, last_update)
-          # set last updated time first, then make a best effort to refresh
-          @cert_provider.ca_last_update = now
-
           # If we refresh the CA, then we need to force the CRL to be refreshed too,
           # since if there is a new CA in the chain, then we need its CRL to check
           # the full chain for revocation status.
@@ -114,7 +111,12 @@ def refresh_ca(ssl_ctx, last_update)
       Puppet.info(_("Refreshing CA certificate"))
 
       # return the next_ctx containing the updated ca
-      [download_ca(ssl_ctx, last_update), true]
+      next_ctx = [download_ca(ssl_ctx, last_update), true]
+
+      # After a successful refresh, update ca_last_update
+      @cert_provider.ca_last_update = Time.now
+
+      next_ctx
     rescue Puppet::HTTP::ResponseError => e
       if e.response.code == 304
         Puppet.info(_("CA certificate is unmodified, using existing CA certificate"))
@@ -171,8 +173,6 @@ def next_state
           now = Time.now
           last_update = @cert_provider.crl_last_update
           if needs_refresh?(now, last_update)
-            # set last updated time first, then make a best effort to refresh
-            @cert_provider.crl_last_update = now
             next_ctx = refresh_crl(next_ctx, last_update)
           end
         else
@@ -209,7 +209,12 @@ def refresh_crl(ssl_ctx, last_update)
       Puppet.info(_("Refreshing CRL"))
 
       # return the next_ctx containing the updated crl
-      download_crl(ssl_ctx, last_update)
+      next_ctx = download_crl(ssl_ctx, last_update)
+
+      # After a successful refresh, update crl_last_update
+      @cert_provider.crl_last_update = Time.now
+
+      next_ctx
     rescue Puppet::HTTP::ResponseError => e
       if e.response.code == 304
         Puppet.info(_("CRL is unmodified, using existing CRL"))

spec/unit/ssl/state_machine_spec.rb

@@ -487,14 +487,22 @@ def expect_lockfile_to_contain(pid)
         expect(state.next_state.ssl_context.cacerts.map(&:to_pem)).to eq(new_ca_bundle)
       end
 
-      it 'updates the `last_update` time' do
+      it 'updates the `last_update` time on successful CA refresh' do
         stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 200, body: new_ca_bundle.join)
 
         expect_any_instance_of(Puppet::X509::CertProvider).to receive(:ca_last_update=).with(be_within(60).of(Time.now))
 
         state.next_state
       end
 
+      it "does not update the `last_update` time when CA refresh fails" do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_raise(Errno::ECONNREFUSED)
+
+        expect_any_instance_of(Puppet::X509::CertProvider).to receive(:ca_last_update=).never
+
+        state.next_state
+      end
+
       it 'forces the NeedCRLs to refresh' do
         stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 200, body: new_ca_bundle.join)