Revert the guts of #2890

This patch reverts the semantically significant parts of #2890 due to the
issues discussed on #3360 (security concerns when used with autosign,
inconsistency between REST & XMLRPC semantics) but leaves the semantically
neutral changes (code cleanup, added tests) in place.

This patch is intended for 0.25.x, but may also be applied as a step in the
resolution of #3450 (refactored #2890, add "remove_certs" flag) in Rolwf.

4 files changed, 40 insertions, 32 deletions

diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index 266758b84..35f17768e 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -161,19 +161,22 @@ class Puppet::Indirector::Indirection
         end
     end
 
-    # 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.
+    # 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.
     def expire(key, *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
+        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))
     end
 
     # Search for an instance in the appropriate terminus, caching the
@@ -213,7 +216,7 @@ class Puppet::Indirector::Indirection
             return nil
         end
 
-        Puppet.debug "Using cached #{name} for #{request.key}, good until #{cached.expiration}"
+        Puppet.debug "Using cached %s for %s" % [self.name, request.key]
         return cached
     end
 
diff --git a/lib/puppet/ssl/certificate.rb b/lib/puppet/ssl/certificate.rb
index b6cba99a7..f9297f380 100644
--- a/lib/puppet/ssl/certificate.rb
+++ b/lib/puppet/ssl/certificate.rb
@@ -28,8 +28,7 @@ class Puppet::SSL::Certificate < Puppet::SSL::Base
     end
 
     def expiration
-        # 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
+        return nil unless content
+        return content.not_after
     end
 end
diff --git a/lib/puppet/ssl/host.rb b/lib/puppet/ssl/host.rb
index 8d44ffe9c..225c9790f 100644
--- a/lib/puppet/ssl/host.rb
+++ b/lib/puppet/ssl/host.rb
@@ -154,19 +154,26 @@ class Puppet::SSL::Host
     end
 
     def certificate
-        @certificate ||= (
+        unless @certificate
+            generate_key unless key
+
             # get the CA cert first, since it's required for the normal cert
             # to be of any use.
-            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
+            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"
             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 f9efc02f7..f6bcbc1f7 100644
--- a/lib/puppet/sslcertificates/ca.rb
+++ b/lib/puppet/sslcertificates/ca.rb
@@ -278,13 +278,12 @@ class Puppet::SSLCertificates::CA
         host = thing2name(csr)
 
         csrfile = host2csrfile(host)
-        raise Puppet::Error, "Certificate request for #{host} already exists" if File.exists?(csrfile)
-        Puppet.settings.writesub(:csrdir, csrfile) { |f| f.print csr.to_pem }
+        if File.exists?(csrfile)
+            raise Puppet::Error, "Certificate request for %s already exists" % host
+        end
 
-        certfile = host2certfile(host)
-        if File.exists?(certfile)
-            Puppet.notice "Removing previously signed certificate #{certfile} for #{host}"
-            Puppet::SSLCertificates::Inventory::rebuild
+        Puppet.settings.writesub(:csrdir, csrfile) do |f|
+            f.print csr.to_pem
         end
     end