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.

2 files changed, 72 insertions, 45 deletions

diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index 0663fe55e..0f6fd13d5 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -545,44 +545,33 @@ describe Puppet::Indirector::Indirection do
                     @indirection.expire("/my/key")
                 end
 
-                describe "and the terminus supports removal of cache items with destroy" do
-                    it "should destroy the cached instance" do
-                        @cache.expects(:find).returns @cached
-                        @cache.expects(:destroy).with { |r| r.method == :destroy and r.key == "/my/key" }
-                        @cache.expects(:save).never
-                        @indirection.expire("/my/key")
-                    end
-                end
-
-                describe "and the terminus does not support removal of cache items with destroy" do
-                    it "should set the cached instance's expiration to a time in the past" do
-                        @cache.expects(:find).returns @cached
-                        @cache.stubs(:save)
+                it "should set the cached instance's expiration to a time in the past" do
+                    @cache.expects(:find).returns @cached
+                    @cache.stubs(:save)
 
-                        @cached.expects(:expiration=).with { |t| t < Time.now }
+                    @cached.expects(:expiration=).with { |t| t < Time.now }
 
-                        @indirection.expire("/my/key")
-                    end
+                    @indirection.expire("/my/key")
+                end
 
-                    it "should save the now expired instance back into the cache" do
-                        @cache.expects(:find).returns @cached
+                it "should save the now expired instance back into the cache" do
+                    @cache.expects(:find).returns @cached
 
-                        @cached.expects(:expiration=).with { |t| t < Time.now }
+                    @cached.expects(:expiration=).with { |t| t < Time.now }
 
-                        @cache.expects(:save)
+                    @cache.expects(:save)
 
-                        @indirection.expire("/my/key")
-                    end
+                    @indirection.expire("/my/key")
+                end
 
-                    it "should use a request to save the expired resource to the cache" do
-                        @cache.expects(:find).returns @cached
+                it "should use a request to save the expired resource to the cache" do
+                    @cache.expects(:find).returns @cached
 
-                        @cached.expects(:expiration=).with { |t| t < Time.now }
+                    @cached.expects(:expiration=).with { |t| t < Time.now }
 
-                        @cache.expects(:save).with { |r| r.is_a?(Puppet::Indirector::Request) and r.instance == @cached and r.method == :save }.returns(@cached)
+                    @cache.expects(:save).with { |r| r.is_a?(Puppet::Indirector::Request) and r.instance == @cached and r.method == :save }.returns(@cached)
 
-                        @indirection.expire("/my/key")
-                    end
+                    @indirection.expire("/my/key")
                 end
             end
         end
diff --git a/spec/unit/ssl/host.rb b/spec/unit/ssl/host.rb
index f6f06a993..36d2ed2e6 100755
--- a/spec/unit/ssl/host.rb
+++ b/spec/unit/ssl/host.rb
@@ -90,6 +90,55 @@ describe Puppet::SSL::Host do
         Puppet::SSL::Host.localhost.should equal(two)
     end
 
+    it "should be able to verify its certificate matches its key" do
+        Puppet::SSL::Host.new("foo").should respond_to(:certificate_matches_key?)
+    end
+
+    it "should consider the certificate invalid if it cannot find a key" do
+        host = Puppet::SSL::Host.new("foo")
+        host.expects(:key).returns nil
+
+        host.should_not be_certificate_matches_key
+    end
+
+    it "should consider the certificate invalid if it cannot find a certificate" do
+        host = Puppet::SSL::Host.new("foo")
+        host.expects(:key).returns mock("key")
+        host.expects(:certificate).returns nil
+
+        host.should_not be_certificate_matches_key
+    end
+
+    it "should consider the certificate invalid if the SSL certificate's key verification fails" do
+        host = Puppet::SSL::Host.new("foo")
+
+        key = mock 'key', :content => "private_key"
+        sslcert = mock 'sslcert'
+        certificate = mock 'cert', :content => sslcert
+
+        host.stubs(:key).returns key
+        host.stubs(:certificate).returns certificate
+
+        sslcert.expects(:check_private_key).with("private_key").returns false
+
+        host.should_not be_certificate_matches_key
+    end
+
+    it "should consider the certificate valid if the SSL certificate's key verification succeeds" do
+        host = Puppet::SSL::Host.new("foo")
+
+        key = mock 'key', :content => "private_key"
+        sslcert = mock 'sslcert'
+        certificate = mock 'cert', :content => sslcert
+
+        host.stubs(:key).returns key
+        host.stubs(:certificate).returns certificate
+
+        sslcert.expects(:check_private_key).with("private_key").returns true
+
+        host.should be_certificate_matches_key
+    end
+
     describe "when specifying the CA location" do
         before do
             [Puppet::SSL::Key, Puppet::SSL::Certificate, Puppet::SSL::CertificateRequest, Puppet::SSL::CertificateRevocationList].each do |klass|
@@ -359,11 +408,10 @@ describe Puppet::SSL::Host do
     describe "when managing its certificate" do
         before do
             @realcert = mock 'certificate'
-            @realcert.stubs(:check_private_key).with('private key').returns true
-
-            @cert = stub 'cert', :content => @realcert, :expired? => false
+            @cert = stub 'cert', :content => @realcert
 
-            @host.stubs(:key).returns stub("key",:content => 'private key' )
+            @host.stubs(:key).returns mock("key")
+            @host.stubs(:certificate_matches_key?).returns true
         end
 
         it "should find the CA certificate if it does not have a certificate" do
@@ -411,22 +459,12 @@ describe Puppet::SSL::Host do
             @host.certificate.should equal(@cert)
         end
 
-        it "should immediately expire the cached copy if the found certificate does not match the private key" do
-            @realcert.expects(:check_private_key).with('private key').returns false
-
-            Puppet::SSL::Certificate.stubs(:find).returns @cert
-            Puppet::SSL::Certificate.expects(:expire).with("myname")
-
-            @host.certificate
-        end
-
-        it "should not return a certificate if it does not match the private key" do
-            @realcert.expects(:check_private_key).with('private key').returns false
+        it "should fail if the found certificate does not match the private key" do
+            @host.expects(:certificate_matches_key?).returns false
 
             Puppet::SSL::Certificate.stubs(:find).returns @cert
-            Puppet::SSL::Certificate.stubs(:expire).with("myname")
 
-            @host.certificate.should == nil
+            lambda { @host.certificate }.should raise_error(Puppet::Error)
         end
 
         it "should return any previously found certificate" do