Merge pull request #7501 from joshcooper/crl_refresh_2310

(PUP-2310) Allow agent to refresh CRL periodically

lib/puppet/defaults.rb

@@ -1098,6 +1098,23 @@ def self.default_vendormoduledir
       :desc       => "The default TTL for new certificates.
       #{AS_DURATION}",
     },
+    :crl_refresh_interval => {
+      :type       => :duration,
+      :desc       => "How often the Puppet agent refreshes its local CRL. By
+         default the CRL is only downloaded once, and never refreshed. If a
+         duration is specified, then the agent will refresh its CRL whenever it
+         next runs and the elapsed time since the CRL was last refreshed exceeds
+         the duration.
+
+         In general, the duration should be greater than the `runinterval`.
+         Setting it to an equal or lesser value will cause the CRL to be
+         refreshed on every run.
+
+         If the agent downloads a new CRL, the agent will use it for subsequent
+         network requests. If the refresh request fails or if the CRL is
+         unchanged on the server, then the agent run will continue using the
+         local CRL it already has.#{AS_DURATION}",
+    },
     :keylength => {
       :default    => 4096,
       :desc       => "The bit length of keys.",

lib/puppet/file_system.rb

@@ -214,10 +214,12 @@ def self.writable?(path)
 
   # Touches the file. On most systems this updates the mtime of the file.
   #
+  # @param mtime [Time] The last modified time or nil to use the current time
+  #
   # @api public
   #
-  def self.touch(path)
-    @impl.touch(assert_path(path))
+  def self.touch(path, mtime: nil)
+    @impl.touch(assert_path(path), mtime: mtime)
   end
 
   # Creates directories for all parts of the given path.

lib/puppet/file_system/file_impl.rb

@@ -108,8 +108,8 @@ def writable?(path)
     path.writable?
   end
 
-  def touch(path)
-    ::FileUtils.touch(path)
+  def touch(path, mtime: nil)
+    ::FileUtils.touch(path, mtime: mtime)
   end
 
   def mkpath(path)

lib/puppet/rest/routes.rb

@@ -1,10 +1,10 @@
+require 'time'
 require 'puppet/rest/route'
 require 'puppet/network/http_pool'
 require 'puppet/network/http/compression'
 
 module Puppet::Rest
   module Routes
-
     extend Puppet::Network::HTTP::Compression.module
 
     ACCEPT_ENCODING = 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3'
@@ -16,9 +16,14 @@ def self.ca
                         srv_service: :ca)
     end
 
-    # Make an HTTP request to fetch the named certificate
-    # @param [String] name the name of the certificate to fetch
-    # @param [Puppet::SSL::SSLContext] ssl_context the ssl content to use when making the request
+    def self.clear
+      @ca = nil
+    end
+
+    # Make an HTTP request to fetch the named certificate.
+    #
+    # @param name [String] the name of the certificate to fetch
+    # @param ssl_context [Puppet::SSL::SSLContext] the ssl content to use when making the request
     # @raise [Puppet::Rest::ResponseError] if the response status is not OK
     # @return [String] the PEM-encoded certificate or certificate bundle
     def self.get_certificate(name, ssl_context)
@@ -41,14 +46,19 @@ def self.get_certificate(name, ssl_context)
       end
     end
 
-    # Make an HTTP request to fetch the named crl
-    # @param [String] name the crl to fetch
-    # @param [Puppet::SSL::SSLContext] ssl_context the ssl content to use when making the request
+    # Make an HTTP request to fetch the named crl.
+    #
+    # @param name [String] name of the crl to fetch
+    # @param ssl_context [Puppet::SSL::SSLContext] the ssl content to use when making the request
+    # @param if_modified_since [Time, nil] If non-nil, then only download the CRL if it has been
+    #   modified since the specified time.
     # @raise [Puppet::Rest::ResponseError] if the response status is not OK
     # @return [String] the PEM-encoded crl
-    def self.get_crls(name, ssl_context)
+    def self.get_crls(name, ssl_context, if_modified_since: nil)
       ca.with_base_url(Puppet::Network::Resolver.new) do |url|
         header = { 'Accept' => 'text/plain', 'Accept-Encoding' => ACCEPT_ENCODING }
+        header['If-Modified-Since'] = if_modified_since.httpdate if if_modified_since
+
         url.path += "certificate_revocation_list/#{name}"
 
         use_ssl = url.is_a? URI::HTTPS
@@ -66,11 +76,12 @@ def self.get_crls(name, ssl_context)
       end
     end
 
-    # Make an HTTP request to send the named CSR
-    # @param [String] csr_pem the contents of the CSR to sent to the CA
-    # @param [String] name the name of the host whose CSR is being submitted
-    # @param [Puppet::SSL::SSLContext] ssl_context the ssl content to use when making the request
-    # @rasies [Puppet::Rest::ResponseError] if the response status is not OK
+    # Make an HTTP request to send the named CSR.
+    #
+    # @param csr_pem [String] the contents of the CSR to sent to the CA
+    # @param name [String] the name of the host whose CSR is being submitted
+    # @param ssl_context [Puppet::SSL::SSLContext] the ssl content to use when making the request
+    # @raise [Puppet::Rest::ResponseError] if the response status is not OK
     def self.put_certificate_request(csr_pem, name, ssl_context)
       ca.with_base_url(Puppet::Network::Resolver.new) do |url|
         header = { 'Accept' => 'text/plain',
@@ -91,11 +102,13 @@ def self.put_certificate_request(csr_pem, name, ssl_context)
       end
     end
 
-    # Make an HTTP request to get the named CSR
-    # @param [String] name the name of the host whose CSR is being queried
-    # @param [Puppet::SSL::SSLContext] ssl_context the ssl content to use when making the request
-    # @rasies [Puppet::Rest::ResponseError] if the response status is not OK
+    # Make an HTTP request to get the named CSR.
+    #
+    # @param name [String] the name of the host whose CSR is being queried
+    # @param ssl_context [Puppet::SSL::SSLContext] the ssl content to use when making the request
+    # @raise [Puppet::Rest::ResponseError] if the response status is not OK
     # @return [String] the PEM encoded certificate request
+    # @deprecated
     def self.get_certificate_request(name, ssl_context)
       ca.with_base_url(Puppet::Network::Resolver.new) do |url|
         header = { 'Accept' => 'text/plain', 'Accept-Encoding' => ACCEPT_ENCODING }

lib/puppet/ssl/state_machine.rb

@@ -69,12 +69,19 @@ def next_state
         crls = @cert_provider.load_crls
         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
+          end
         else
-          pem = Puppet::Rest::Routes.get_crls(Puppet::SSL::CA_NAME, @ssl_context)
-          crls = @cert_provider.load_crls_from_pem(pem)
-          # verify crls before saving
-          next_ctx = @ssl_provider.create_root_context(cacerts: ssl_context[:cacerts], crls: crls)
-          @cert_provider.save_crls(crls)
+          next_ctx = download_crl(@ssl_context, nil)
         end
       else
         Puppet.info("Certificate revocation is disabled, skipping CRL download")
@@ -89,6 +96,39 @@ def next_state
         raise Puppet::Error.new(_('Could not download CRLs: %{message}') % { message: e.message }, e)
       end
     end
+
+    private
+
+    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)
+    rescue Puppet::Rest::ResponseError => e
+      if e.response.code.to_i == 304
+        Puppet.info(_("CRL is unmodified, using existing CRL"))
+      else
+        Puppet.info(_("Failed to refresh CRL, using existing CRL: %{message}") % {message: e.message})
+      end
+
+      # return the original ssl_ctx
+      ssl_ctx
+    rescue SystemCallError => e
+      Puppet.warning(_("Failed to refresh CRL, using existing CRL: %{message}") % {message: e.message})
+
+      # return the original ssl_ctx
+      ssl_ctx
+    end
+
+    def download_crl(ssl_ctx, last_update)
+      pem = Puppet::Rest::Routes.get_crls(Puppet::SSL::CA_NAME, ssl_ctx, if_modified_since: last_update)
+      crls = @cert_provider.load_crls_from_pem(pem)
+      # verify crls before saving
+      next_ctx = @ssl_provider.create_root_context(cacerts: ssl_ctx[:cacerts], crls: crls)
+      @cert_provider.save_crls(crls)
+
+      next_ctx
+    end
   end
 
   # Load or generate a private key. If the key exists, try to load the client cert

lib/puppet/test/test_helper.rb

@@ -145,6 +145,7 @@ def self.before_each_test()
       Puppet::Util::Profiler.clear
 
       Puppet::SSL::Host.reset
+      Puppet::Rest::Routes.clear
 
       Puppet.clear_deprecation_warnings
     end

lib/puppet/x509/cert_provider.rb

@@ -110,6 +110,25 @@ def load_crls_from_pem(pem)
     end
   end
 
+  # Return the time when the CRL was last updated.
+  #
+  # @return [Time, nil] Time when the CRL was last updated, or nil if we don't
+  #   have a CRL
+  def crl_last_update
+    stat = Puppet::FileSystem.stat(@crlpath)
+    Time.at(stat.mtime)
+  rescue Errno::ENOENT
+    nil
+  end
+
+  # Set the CRL last updated time.
+  #
+  # @param time [Time] The last updated time
+  #
+  def crl_last_update=(time)
+    Puppet::FileSystem.touch(@crlpath, mtime: time)
+  end
+
   # Save named private key in the configured `privatekeydir`. For
   # historical reasons, names are case insensitive.
   #

spec/unit/file_system_spec.rb

@@ -1089,4 +1089,33 @@ def increment_counter_in_multiple_processes(file, num_procs, options)
       end
     end
   end
+
+  context '#touch' do
+    let(:dest) { tmpfile('touch_file') }
+
+    it 'creates a file' do
+      Puppet::FileSystem.touch(dest)
+
+      expect(File).to be_file(dest)
+    end
+
+    it 'updates the mtime for an existing file' do
+      Puppet::FileSystem.touch(dest)
+
+      now = Time.now
+      allow(Time).to receive(:now).and_return(now)
+
+      Puppet::FileSystem.touch(dest)
+
+      expect(File.mtime(dest)).to be_within(1).of(now)
+    end
+
+    it 'allows the mtime to be passed in' do
+      tomorrow = Time.now + (24 * 60 * 60)
+
+      Puppet::FileSystem.touch(dest, mtime: tomorrow)
+
+      expect(File.mtime(dest)).to be_within(1).of(tomorrow)
+    end
+  end
 end

spec/unit/ssl/state_machine_spec.rb

@@ -7,13 +7,6 @@
 describe Puppet::SSL::StateMachine, unless: Puppet::Util::Platform.jruby? do
   include PuppetSpec::Files
 
-  before(:each) do
-    WebMock.disable_net_connect!
-
-    allow_any_instance_of(Net::HTTP).to receive(:start)
-    allow_any_instance_of(Net::HTTP).to receive(:finish)
-  end
-
   let(:machine) { described_class.new }
   let(:cacert_pem) { cacert.to_pem }
   let(:cacert) { cert_fixture('ca.pem') }
@@ -225,6 +218,69 @@
 
       expect(File).to_not exist(Puppet[:hostcrl])
     end
+
+    it 'skips CRL refresh by default' do
+      allow_any_instance_of(Puppet::X509::CertProvider).to receive(:load_crls).and_return(crls)
+
+      state.next_state
+    end
+
+    it 'skips CRL refresh if it has not expired' do
+      Puppet[:crl_refresh_interval] = '1y'
+      Puppet::FileSystem.touch(Puppet[:hostcrl], mtime: Time.now)
+
+      allow_any_instance_of(Puppet::X509::CertProvider).to receive(:load_crls).and_return(crls)
+
+      state.next_state
+    end
+
+    context 'when refreshing a CRL' do
+      before :each do
+        Puppet[:crl_refresh_interval] = '1s'
+        allow_any_instance_of(Puppet::X509::CertProvider).to receive(:load_crls).and_return(crls)
+
+        yesterday = Time.now - (24 * 60 * 60)
+        allow_any_instance_of(Puppet::X509::CertProvider).to receive(:crl_last_update).and_return(yesterday)
+      end
+
+      let(:new_crl_bundle) do
+        # add intermediate crl to the bundle
+        int_crl = crl_fixture('intermediate-crl.pem')
+        [crl, int_crl].map(&:to_pem)
+      end
+
+      it 'uses the local crl if it has not been modified' do
+        stub_request(:get, %r{puppet-ca/v1/certificate_revocation_list/ca}).to_return(status: 304)
+
+        expect(state.next_state.ssl_context.crls).to eq(crls)
+      end
+
+      it 'uses the local crl if refreshing fails in HTTP layer' do
+        stub_request(:get, %r{puppet-ca/v1/certificate_revocation_list/ca}).to_return(status: 503)
+
+        expect(state.next_state.ssl_context.crls).to eq(crls)
+      end
+
+      it 'uses the local crl if refreshing fails in TCP layer' do
+        stub_request(:get, %r{puppet-ca/v1/certificate_revocation_list/ca}).to_raise(Errno::ECONNREFUSED)
+
+        expect(state.next_state.ssl_context.crls).to eq(crls)
+      end
+
+      it 'uses the updated crl for the future requests' do
+        stub_request(:get, %r{puppet-ca/v1/certificate_revocation_list/ca}).to_return(status: 200, body: new_crl_bundle.join)
+
+        expect(state.next_state.ssl_context.crls.map(&:to_pem)).to eq(new_crl_bundle)
+      end
+
+      it 'updates the `last_update` time' do
+        stub_request(:get, %r{puppet-ca/v1/certificate_revocation_list/ca}).to_return(status: 200, body: new_crl_bundle.join)
+
+        expect_any_instance_of(Puppet::X509::CertProvider).to receive(:crl_last_update=).with(be_within(60).of(Time.now))
+
+        state.next_state
+      end
+    end
   end
 
   context 'when ensuring a client cert' do

spec/unit/x509/cert_provider_spec.rb

@@ -557,4 +557,30 @@ def expects_private_file(path)
       end
     end
   end
+
+  context 'CRL last update time' do
+    let(:crl_path) { tmpfile('pem_crls') }
+
+    it 'returns nil if the CRL does not exist' do
+      provider = create_provider(crlpath: '/does/not/exist')
+
+      expect(provider.crl_last_update).to be_nil
+    end
+
+    it 'returns the last update time' do
+      time = Time.now - 30
+      Puppet::FileSystem.touch(crl_path, mtime: time)
+      provider = create_provider(crlpath: crl_path)
+
+      expect(provider.crl_last_update).to be_within(1).of(time)
+    end
+
+    it 'sets the last update time' do
+      time = Time.now - 30
+      provider = create_provider(crlpath: crl_path)
+      provider.crl_last_update = time
+
+      expect(Puppet::FileSystem.stat(crl_path).mtime).to be_within(1).of(time)
+    end
+  end
 end