(PUP-10639) Refresh CA certs

Refresh CA certs if the last update time plus the `:ca_refresh_interval`
is in the past.

If the certs are updated, then force the CRLs to be updated too, since if the CA
bundle includes a new root or intermediate CA, we'll need its corresponding
non-expired CRL, to check the full chain.

If the certs aren't updated or there's an networking error, then continue using
the CA started with, and try again based on the next `:ca_refresh_interval`.

Author: joshcooper

lib/puppet/ssl/state_machine.rb

@@ -50,9 +50,23 @@ def initialize(machine)
     def next_state
       Puppet.debug("Loading CA certs")
 
+      force_crl_refresh = false
+
       cacerts = @cert_provider.load_cacerts
       if cacerts
         next_ctx = @ssl_provider.create_root_context(cacerts: cacerts, revocation: false)
+
+        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.
+          next_ctx, force_crl_refresh = refresh_ca(next_ctx, last_update)
+        end
       else
         route = @machine.session.route_to(:ca, ssl_context: @ssl_context)
         _, pem = route.get_certificate(Puppet::SSL::CA_NAME, ssl_context: @ssl_context)
@@ -74,7 +88,7 @@ def next_state
         @cert_provider.save_cacerts(cacerts)
       end
 
-      NeedCRLs.new(@machine, next_ctx)
+      NeedCRLs.new(@machine, next_ctx, force_crl_refresh)
     rescue OpenSSL::X509::CertificateError => e
       Error.new(@machine, e.message, e)
     rescue Puppet::HTTP::ResponseError => e
@@ -84,6 +98,49 @@ def next_state
         to_error(_('Could not download CA certificate: %{message}') % { message: e.message }, e)
       end
     end
+
+    private
+
+    def needs_refresh?(now, last_update)
+      return true if last_update.nil?
+
+      ca_ttl = Puppet[:ca_refresh_interval]
+      return false unless ca_ttl
+
+      now.to_i > last_update.to_i + ca_ttl
+    end
+
+    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]
+    rescue Puppet::HTTP::ResponseError => e
+      if e.response.code == 304
+        Puppet.info(_("CA certificate is unmodified, using existing CA certificate"))
+      else
+        Puppet.info(_("Failed to refresh CA certificate, using existing CA certificate: %{message}") % {message: e.message})
+      end
+
+      # return the original ssl_ctx
+      [ssl_ctx, false]
+    rescue Puppet::HTTP::HTTPError => e
+      Puppet.warning(_("Failed to refresh CA certificate, using existing CA certificate: %{message}") % {message: e.message})
+
+      # return the original ssl_ctx
+      [ssl_ctx, false]
+    end
+
+    def download_ca(ssl_ctx, last_update)
+      route = @machine.session.route_to(:ca, ssl_context: ssl_ctx)
+      _, pem = route.get_certificate(Puppet::SSL::CA_NAME, if_modified_since: last_update, ssl_context: ssl_ctx)
+      cacerts = @cert_provider.load_cacerts_from_pem(pem)
+      # verify cacerts before saving
+      next_ctx = @ssl_provider.create_root_context(cacerts: cacerts, revocation: false)
+      @cert_provider.save_cacerts(cacerts)
+
+      next_ctx
+    end
   end
 
   # If revocation is enabled, load CRLs or download them, using the CA bundle
@@ -93,6 +150,13 @@ def next_state
   # for which we don't have a CRL
   #
   class NeedCRLs < SSLState
+    attr_reader :force_crl_refresh
+
+    def initialize(machine, ssl_context, force_crl_refresh = false)
+      super(machine, ssl_context)
+      @force_crl_refresh = force_crl_refresh
+    end
+
     def next_state
       Puppet.debug("Loading CRLs")
 
@@ -102,15 +166,12 @@ def next_state
         if crls
           next_ctx = @ssl_provider.create_root_context(cacerts: ssl_context[:cacerts], crls: crls)
 
-          crl_ttl = Puppet[:crl_refresh_interval]
-          if crl_ttl
-            last_update = @cert_provider.crl_last_update
-            now = Time.now
-            if last_update.nil? || now.to_i > last_update.to_i + crl_ttl
-              # 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
+          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
           next_ctx = download_crl(@ssl_context, nil)
@@ -133,6 +194,15 @@ def next_state
 
     private
 
+    def needs_refresh?(now, last_update)
+      return true if @force_crl_refresh || last_update.nil?
+
+      crl_ttl = Puppet[:crl_refresh_interval]
+      return false unless crl_ttl
+
+      now.to_i > last_update.to_i + crl_ttl
+    end
+
     def refresh_crl(ssl_ctx, last_update)
       Puppet.info(_("Refreshing CRL"))
 

spec/unit/ssl/state_machine_spec.rb

@@ -30,7 +30,9 @@
     Puppet[:daemonize] = false
     Puppet[:ssl_lockfile] = tmpfile('ssllock')
     allow(Kernel).to receive(:sleep)
-    allow_any_instance_of(Puppet::X509::CertProvider).to receive(:crl_last_update).and_return(Time.now + (5 * 60))
+    future = Time.now + (5 * 60)
+    allow_any_instance_of(Puppet::X509::CertProvider).to receive(:crl_last_update).and_return(future)
+    allow_any_instance_of(Puppet::X509::CertProvider).to receive(:ca_last_update).and_return(future)
   end
 
   def expected_digest(name, content)
@@ -396,6 +398,16 @@ def expect_lockfile_to_contain(pid)
       expect(File).to_not exist(Puppet[:localcacert])
     end
 
+    it 'skips CA refresh if it has not expired' do
+      Puppet[:ca_refresh_interval] = '1y'
+      Puppet::FileSystem.touch(Puppet[:localcacert], mtime: Time.now)
+
+      allow_any_instance_of(Puppet::X509::CertProvider).to receive(:load_cacerts).and_return(cacerts)
+
+      # we're expecting a net/http request to never be made
+      state.next_state
+    end
+
     context 'when verifying CA cert bundle' do
       before :each do
         allow(cert_provider).to receive(:load_cacerts).and_return(nil)
@@ -436,6 +448,61 @@ def expect_lockfile_to_contain(pid)
         expect(st.message).to eq("CA bundle with digest (SHA256) #{fingerprint} did not match expected digest WR:ON:G!")
       end
     end
+
+    context 'when refreshing a CA bundle' do
+      before :each do
+        Puppet[:ca_refresh_interval] = '1s'
+        allow_any_instance_of(Puppet::X509::CertProvider).to receive(:load_cacerts).and_return(cacerts)
+
+        yesterday = Time.now - (24 * 60 * 60)
+        allow_any_instance_of(Puppet::X509::CertProvider).to receive(:ca_last_update).and_return(yesterday)
+      end
+
+      let(:new_ca_bundle) do
+        # add 'unknown' cert to the bundle
+        [cacert, cert_fixture('intermediate.pem'), cert_fixture('unknown-ca.pem')].map(&:to_pem)
+      end
+
+      it 'uses the local CA if it has not been modified' do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 304)
+
+        expect(state.next_state.ssl_context.cacerts).to eq(cacerts)
+      end
+
+      it 'uses the local CA if refreshing fails in HTTP layer' do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 503)
+
+        expect(state.next_state.ssl_context.cacerts).to eq(cacerts)
+      end
+
+      it 'uses the local CA if refreshing fails in TCP layer' do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_raise(Errno::ECONNREFUSED)
+
+        expect(state.next_state.ssl_context.cacerts).to eq(cacerts)
+      end
+
+      it 'uses the updated crl for the future requests' do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 200, body: new_ca_bundle.join)
+
+        expect(state.next_state.ssl_context.cacerts.map(&:to_pem)).to eq(new_ca_bundle)
+      end
+
+      it 'updates the `last_update` time' 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 'forces the NeedCRLs to refresh' do
+        stub_request(:get, %r{puppet-ca/v1/certificate/ca}).to_return(status: 200, body: new_ca_bundle.join)
+
+        st = state.next_state
+        expect(st).to be_an_instance_of(Puppet::SSL::StateMachine::NeedCRLs)
+        expect(st.force_crl_refresh).to eq(true)
+      end
+    end
   end
 
   context 'NeedCRLs' do
@@ -533,6 +600,7 @@ def expect_lockfile_to_contain(pid)
 
       allow_any_instance_of(Puppet::X509::CertProvider).to receive(:load_crls).and_return(crls)
 
+      # we're expecting a net/http request to never be made
       state.next_state
     end