Merge pull request #9059 from joshcooper/renew_ca_cert_10639

(PUP-10639) Periodically refresh our CA bundle

Author: tvpartytonight

lib/puppet/defaults.rb

@@ -1212,6 +1212,24 @@ def self.initialize_default_settings!(settings)
       :desc       => "The default TTL for new certificates.
       #{AS_DURATION}",
     },
+    :ca_refresh_interval => {
+      :default    => "1d",
+      :type       => :duration,
+      :desc       => "How often the Puppet agent refreshes its local CA certs. By
+         default the CA certs are refreshed once every 24 hours. If a different
+         duration is specified, then the agent will refresh its CA certs whenever
+         it next runs and the elapsed time since the certs were last refreshed
+         exceeds the duration.
+
+         In general, the duration should be greater than the `runinterval`.
+         Setting it to 0 or an equal or lesser value than `runinterval`,
+         will cause the CA certs to be refreshed on every run.
+
+         If the agent downloads new CA certs, the agent will use it for subsequent
+         network requests. If the refresh request fails or if the CA certs are
+         unchanged on the server, then the agent run will continue using the
+         local CA certs it already has. #{AS_DURATION}",
+    },
     :crl_refresh_interval => {
       :default    => "1d",
       :type       => :duration,
@@ -1222,8 +1240,8 @@ def self.initialize_default_settings!(settings)
          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.
+         Setting it to 0 or an equal or lesser value than `runinterval`,
+         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

lib/puppet/http/service/ca.rb

@@ -28,16 +28,21 @@ def initialize(client, session, server, port)
   # Submit a GET request to retrieve the named certificate from the server.
   #
   # @param [String] name name of the certificate to request
+  # @param [Time] if_modified_since If not nil, only download the cert if it has
+  #   been modified since the specified time.
   # @param [Puppet::SSL::SSLContext] ssl_context
   #
   # @return [Array<Puppet::HTTP::Response, String>] An array containing the
   #   request response and the stringified body of the request response
   #
   # @api public
-  def get_certificate(name, ssl_context: nil)
+  def get_certificate(name, if_modified_since: nil, ssl_context: nil)
+    headers = add_puppet_headers(HEADERS)
+    headers['If-Modified-Since'] = if_modified_since.httpdate if if_modified_since
+
     response = @client.get(
       with_base_url("/certificate/#{name}"),
-      headers: add_puppet_headers(HEADERS),
+      headers: headers,
       options: {ssl_context: ssl_context}
     )
 

lib/puppet/ssl/state_machine.rb

@@ -50,14 +50,28 @@ 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)
         if @machine.ca_fingerprint
-          actual_digest = Puppet::SSL::Digest.new(@machine.digest, pem).to_hex
+          actual_digest = @machine.digest_as_hex(pem)
           expected_digest = @machine.ca_fingerprint.scan(/../).join(':').upcase
           if actual_digest == expected_digest
             Puppet.info(_("Verified CA bundle with digest (%{digest_type}) %{actual_digest}") %
@@ -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,51 @@ 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)
+
+      Puppet.info("Refreshed CA certificate: #{@machine.digest_as_hex(pem)}")
+
+      next_ctx
+    end
   end
 
   # If revocation is enabled, load CRLs or download them, using the CA bundle
@@ -93,6 +152,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 +168,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 +196,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"))
 
@@ -162,6 +234,8 @@ def download_crl(ssl_ctx, last_update)
       next_ctx = @ssl_provider.create_root_context(cacerts: ssl_ctx[:cacerts], crls: crls)
       @cert_provider.save_crls(crls)
 
+      Puppet.info("Refreshed CRL: #{@machine.digest_as_hex(pem)}")
+
       next_ctx
     end
   end
@@ -441,6 +515,10 @@ def unlock
     @lockfile.unlock
   end
 
+  def digest_as_hex(str)
+    Puppet::SSL::Digest.new(digest, str).to_hex
+  end
+
   private
 
   def run_machine(state, stop)

lib/puppet/x509/cert_provider.rb

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

spec/integration/application/agent_spec.rb

@@ -896,6 +896,55 @@ def copy_fixtures(sources, dest)
          .and output(%r{Certificate 'CN=revoked' is revoked}).to_stderr
       end
     end
+
+    it "refreshes the CA and CRL" do
+      Puppet[:localcacert] = ca = tmpfile('ca')
+      Puppet[:hostcrl] = crl = tmpfile('crl')
+      copy_fixtures(%w[ca.pem intermediate.pem], ca)
+      copy_fixtures(%w[crl.pem intermediate-crl.pem], crl)
+
+      now = Time.now
+      yesterday = now - (60 * 60 * 24)
+      Puppet::FileSystem.touch(ca, mtime: yesterday)
+      Puppet::FileSystem.touch(crl, mtime: yesterday)
+
+      server.start_server do |port|
+        Puppet[:serverport] = port
+        Puppet[:ca_refresh_interval] = 1
+
+        expect {
+          agent.command_line.args << '--test'
+          agent.run
+        }.to exit_with(0)
+         .and output(/Info: Refreshed CA certificate: /).to_stdout
+      end
+
+      # If the CA is updated, then the CRL must be updated too
+      expect(Puppet::FileSystem.stat(ca).mtime).to be >= now
+      expect(Puppet::FileSystem.stat(crl).mtime).to be >= now
+    end
+
+    it "refreshes only the CRL" do
+      Puppet[:hostcrl] = crl = tmpfile('crl')
+      copy_fixtures(%w[crl.pem intermediate-crl.pem], crl)
+
+      now = Time.now
+      yesterday = now - (60 * 60 * 24)
+      Puppet::FileSystem.touch(crl, mtime: yesterday)
+
+      server.start_server do |port|
+        Puppet[:serverport] = port
+        Puppet[:crl_refresh_interval] = 1
+
+        expect {
+          agent.command_line.args << '--test'
+          agent.run
+        }.to exit_with(0)
+         .and output(/Info: Refreshed CRL: /).to_stdout
+      end
+
+      expect(Puppet::FileSystem.stat(crl).mtime).to be >= now
+    end
   end
 
   context "legacy facts" do
@@ -994,6 +1043,7 @@ def mounts
         expect {
           agent.run
         }.to exit_with(1)
+          .and output(/Info: Loading facts/).to_stdout
           .and output(
             match(/Error: Evaluation Error: Unknown variable: 'osfamily'/)
               .and match(/Error: Could not retrieve catalog from remote server: Error 500 on SERVER:/)

spec/unit/application/lookup_spec.rb

@@ -668,6 +668,7 @@ def run_lookup(lookup)
           expect {
             lookup.run_command
           }.to exit_with(0)
+           .and output(/This is in facts hash/).to_stdout
         end
       end
     end

spec/unit/http/service/ca_spec.rb

@@ -95,6 +95,18 @@
         expect(err.response.code).to eq(404)
       end
     end
+
+    it 'raises a 304 response error if it is unmodified' do
+      stub_request(:get, url).to_return(status: [304, 'Not Modified'])
+
+      expect {
+        subject.get_certificate('ca', if_modified_since: Time.now)
+      }.to raise_error do |err|
+        expect(err).to be_an_instance_of(Puppet::HTTP::ResponseError)
+        expect(err.message).to eq("Not Modified")
+        expect(err.response.code).to eq(304)
+      end
+    end
   end
 
   context 'when getting CRLs' do