Fix for #2890 (the cached certificates that would not die)

This patch implements the two-part suggestion from the ticket;

1) a client that receives a certificate that doesn't match its current
private key does not accept, store or use the certificate--instead it
removes any locally cached copies and acts as if the certificate had
never been found.

2) a puppetmaster that receives a csr from a client for whom it already
has a signed certificate now honors the request and considers it to
supercede any previously signed certificates.

In order to make the cache expiration work as expected, I changed a few
assumptions in the caching system:

* The expiration of a cached certificate is the earlier of the envelope
  expiration and the certificate's expiration, as opposed to just overriding
  the cache value
* Telling the cache to expire an item now removes it from the cache if
  possible, rather than just setting an expiration date in the past and
  hoping that somebody notices.

Signed-off-by: Markus Roberts <Markus@reality.com>

6 files changed, 38 insertions, 60 deletions

diff --git a/lib/puppet/indirector/envelope.rb b/lib/puppet/indirector/envelope.rb
index ef7952ef6..5fdea7081 100644
--- a/lib/puppet/indirector/envelope.rb
+++ b/lib/puppet/indirector/envelope.rb
@@ -6,8 +6,6 @@ module Puppet::Indirector::Envelope
     attr_accessor :expiration
 
     def expired?
-        return false unless expiration
-        return false if expiration >= Time.now
-        return true
+        expiration and expiration < Time.now
     end
 end
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index dc7e58f36..d762701f5 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -161,22 +161,19 @@ class Puppet::Indirector::Indirection
         end
     end
 
-    # Expire a cached object, if one is cached.  Note that we don't actually
-    # remove it, we expire it and write it back out to disk.  This way people
-    # can still use the expired object if they want.
+    # Expire a cached object, if one is cached.  Note that we now actually
+    # remove it if possible, and only mark it as expired if destroy isn't 
+    # supported.
     def expire(key, *args)
-        request = request(:expire, key, *args)
-
-        return nil unless cache?
-
-        return nil unless instance = cache.find(request(:find, key, *args))
-
-        Puppet.info "Expiring the %s cache of %s" % [self.name, instance.name]
-
-        # Set an expiration date in the past
-        instance.expiration = Time.now - 60
-
-        cache.save(request(:save, instance, *args))
+        if cache? and instance = cache.find(request(:find, key, *args))
+            Puppet.info "Expiring the #{name} cache of #{instance.name}"
+            if cache.respond_to? :destroy
+                cache.destroy(request(:destroy, instance, *args))
+            else
+                instance.expiration = Time.now - 1
+                cache.save(request(:save,instance,*args))
+            end
+        end
     end
 
     # Search for an instance in the appropriate terminus, caching the
@@ -216,7 +213,7 @@ class Puppet::Indirector::Indirection
             return nil
         end
 
-        Puppet.debug "Using cached %s for %s" % [self.name, request.key]
+        Puppet.debug "Using cached #{name} for #{request.key}, good until #{cached.expiration}"
         return cached
     end
 
diff --git a/lib/puppet/indirector/ssl_file.rb b/lib/puppet/indirector/ssl_file.rb
index 67202699d..fc1e65d22 100644
--- a/lib/puppet/indirector/ssl_file.rb
+++ b/lib/puppet/indirector/ssl_file.rb
@@ -91,7 +91,7 @@ class Puppet::Indirector::SslFile < Puppet::Indirector::Terminus
     def save(request)
         path = path(request.key)
         dir = File.dirname(path)
-
+ 
         raise Puppet::Error.new("Cannot save %s; parent directory %s does not exist" % [request.key, dir]) unless FileTest.directory?(dir)
         raise Puppet::Error.new("Cannot save %s; parent directory %s is not writable" % [request.key, dir]) unless FileTest.writable?(dir)
 
diff --git a/lib/puppet/ssl/certificate.rb b/lib/puppet/ssl/certificate.rb
index f9297f380..b6cba99a7 100644
--- a/lib/puppet/ssl/certificate.rb
+++ b/lib/puppet/ssl/certificate.rb
@@ -28,7 +28,8 @@ class Puppet::SSL::Certificate < Puppet::SSL::Base
     end
 
     def expiration
-        return nil unless content
-        return content.not_after
+        # Our expiration is either that of the cache or the content, whichever comes first
+        cache_expiration = @expiration
+        [(content and content.not_after), cache_expiration].compact.sort.first
     end
 end
diff --git a/lib/puppet/ssl/host.rb b/lib/puppet/ssl/host.rb
index 11e11c597..75e51e5c8 100644
--- a/lib/puppet/ssl/host.rb
+++ b/lib/puppet/ssl/host.rb
@@ -94,12 +94,7 @@ class Puppet::SSL::Host
 
     # Remove all traces of a given host
     def self.destroy(name)
-        [Key, Certificate, CertificateRequest].inject(false) do |result, klass|
-            if klass.destroy(name)
-                result = true
-            end
-            result
-        end
+        [Key, Certificate, CertificateRequest].collect { |part| part.destroy(name) }.any? { |x| x }
     end
 
     # Search for more than one host, optionally only specifying
@@ -107,12 +102,7 @@ class Puppet::SSL::Host
     # This just allows our non-indirected class to have one of
     # indirection methods.
     def self.search(options = {})
-        classes = [Key, CertificateRequest, Certificate]
-        if klass = options[:for]
-            classlist = [klass].flatten
-        else
-            classlist = [Key, CertificateRequest, Certificate]
-        end
+        classlist = [options[:for] || [Key, CertificateRequest, Certificate]].flatten
 
         # Collect the results from each class, flatten them, collect all of the names, make the name list unique,
         # then create a Host instance for each one.
@@ -127,8 +117,7 @@ class Puppet::SSL::Host
     end
 
     def key
-        return nil unless @key ||= Key.find(name)
-        @key
+        @key ||= Key.find(name)
     end
 
     # This is the private key; we can create it from scratch
@@ -146,8 +135,7 @@ class Puppet::SSL::Host
     end
 
     def certificate_request
-        return nil unless @certificate_request ||= CertificateRequest.find(name)
-        @certificate_request
+        @certificate_request ||= CertificateRequest.find(name)
     end
 
     # Our certificate request requires the key but that's all.
@@ -166,26 +154,19 @@ class Puppet::SSL::Host
     end
 
     def certificate
-        unless @certificate
-            generate_key unless key
-
+        @certificate ||= (
             # get the CA cert first, since it's required for the normal cert
             # to be of any use.
-            return nil unless Certificate.find("ca") unless ca?
-            return nil unless @certificate = Certificate.find(name)
-
-            unless certificate_matches_key?
-                raise Puppet::Error, "Retrieved certificate does not match private key; please remove certificate from server and regenerate it with the current key"
+            if not (key or generate_key) or not (ca? or Certificate.find("ca")) or not (cert = Certificate.find(name)) or cert.expired?
+                nil
+            elsif not cert.content.check_private_key(key.content)
+                Certificate.expire(name)
+                Puppet.warning "Retrieved certificate does not match private key"
+                nil
+            else
+                cert
             end
-        end
-        @certificate
-    end
-
-    def certificate_matches_key?
-        return false unless key
-        return false unless certificate
-
-        return certificate.content.check_private_key(key.content)
+            )
     end
 
     # Generate all necessary parts of our ssl host.
diff --git a/lib/puppet/sslcertificates/ca.rb b/lib/puppet/sslcertificates/ca.rb
index f6bcbc1f7..f9efc02f7 100644
--- a/lib/puppet/sslcertificates/ca.rb
+++ b/lib/puppet/sslcertificates/ca.rb
@@ -278,12 +278,13 @@ class Puppet::SSLCertificates::CA
         host = thing2name(csr)
 
         csrfile = host2csrfile(host)
-        if File.exists?(csrfile)
-            raise Puppet::Error, "Certificate request for %s already exists" % host
-        end
+        raise Puppet::Error, "Certificate request for #{host} already exists" if File.exists?(csrfile)
+        Puppet.settings.writesub(:csrdir, csrfile) { |f| f.print csr.to_pem }
 
-        Puppet.settings.writesub(:csrdir, csrfile) do |f|
-            f.print csr.to_pem
+        certfile = host2certfile(host)
+        if File.exists?(certfile)
+            Puppet.notice "Removing previously signed certificate #{certfile} for #{host}"
+            Puppet::SSLCertificates::Inventory::rebuild
         end
     end