Fix #2082 - puppetca shouldn't list revoked certificates

This patch does two things:
 * it enhance puppetca to list revoked certificates (prefixed by -)
 * it fixes the ca crl verification which was broken

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

6 files changed, 37 insertions, 10 deletions

diff --git a/lib/puppet/ssl/certificate_authority.rb b/lib/puppet/ssl/certificate_authority.rb
index 4a7d4615b..10d13c28e 100644
--- a/lib/puppet/ssl/certificate_authority.rb
+++ b/lib/puppet/ssl/certificate_authority.rb
@@ -17,6 +17,14 @@ class Puppet::SSL::CertificateAuthority
 
     require 'puppet/ssl/certificate_authority/interface'
 
+    class CertificateVerificationError < RuntimeError
+        attr_accessor :error_code
+
+        def initialize(code)
+            @error_code = code
+        end
+    end
+
     class << self
         include Puppet::Util::Cacher
 
@@ -276,9 +284,10 @@ class Puppet::SSL::CertificateAuthority
         store.add_file Puppet[:cacert]
         store.add_crl crl.content if self.crl
         store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
+        store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK
 
         unless store.verify(cert.content)
-            raise "Certificate for %s failed verification" % name
+            raise CertificateVerificationError.new(store.error), store.error_string
         end
     end
 
diff --git a/lib/puppet/ssl/certificate_authority/interface.rb b/lib/puppet/ssl/certificate_authority/interface.rb
index e4552950c..3f91434e3 100644
--- a/lib/puppet/ssl/certificate_authority/interface.rb
+++ b/lib/puppet/ssl/certificate_authority/interface.rb
@@ -60,8 +60,16 @@ class Puppet::SSL::CertificateAuthority::Interface
         end
 
         hosts.uniq.sort.each do |host|
-            if signed.include?(host)
+            invalid = false
+            begin
+                ca.verify(host) unless requests.include?(host)
+            rescue Puppet::SSL::CertificateAuthority::CertificateVerificationError => details
+                invalid = details.to_s
+            end
+            if not invalid and signed.include?(host)
                 puts "+ " + host
+            elsif invalid
+                puts "- " + host + " (" + invalid + ")"
             else
                 puts host
             end
diff --git a/sbin/puppetca b/sbin/puppetca
index 572f72c03..27ba916b5 100755
--- a/sbin/puppetca
+++ b/sbin/puppetca
@@ -55,7 +55,9 @@
 #
 # list::
 #   List outstanding certificate requests.  If '--all' is specified,
-#   signed certificates are also listed, prefixed by '+'.
+#   signed certificates are also listed, prefixed by '+', and revoked
+#   or invalid certificates are prefixed by '-' (the verification outcome
+#   is printed in parenthesis).
 #
 # print::
 #   Print the full-text version of a host's certificate.
diff --git a/spec/integration/ssl/certificate_authority.rb b/spec/integration/ssl/certificate_authority.rb
index 5f963f7f5..553c9b3b6 100755
--- a/spec/integration/ssl/certificate_authority.rb
+++ b/spec/integration/ssl/certificate_authority.rb
@@ -50,13 +50,11 @@ describe Puppet::SSL::CertificateAuthority do
     end
 
     it "should be able to revoke a host certificate" do
-        pending("This test doesn't actually work yet") do
-            @ca.generate("newhost")
+        @ca.generate("newhost")
 
-            @ca.revoke("newhost")
+        @ca.revoke("newhost")
 
-            lambda { @ca.verify("newhost") }.should raise_error
-        end
+        lambda { @ca.verify("newhost") }.should raise_error
     end
 
     it "should have a CRL" do
diff --git a/spec/unit/ssl/certificate_authority.rb b/spec/unit/ssl/certificate_authority.rb
index 4c2466d93..80114300e 100755
--- a/spec/unit/ssl/certificate_authority.rb
+++ b/spec/unit/ssl/certificate_authority.rb
@@ -585,7 +585,7 @@ describe Puppet::SSL::CertificateAuthority do
 
         describe "and verifying certificates" do
             before do
-                @store = stub 'store', :verify => true, :add_file => nil, :purpose= => nil, :add_crl => true
+                @store = stub 'store', :verify => true, :add_file => nil, :purpose= => nil, :add_crl => true, :flags= => nil
 
                 OpenSSL::X509::Store.stubs(:new).returns @store
 
@@ -631,6 +631,12 @@ describe Puppet::SSL::CertificateAuthority do
                 @ca.verify("me")
             end
 
+            it "should set the store flags to check the crl" do
+                @store.expects(:flags=).with OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK
+
+                @ca.verify("me")
+            end
+
             it "should use the store to verify the certificate" do
                 @cert.expects(:content).returns "mycert"
 
diff --git a/spec/unit/ssl/certificate_authority/interface.rb b/spec/unit/ssl/certificate_authority/interface.rb
index 784c6cf9a..d741ec400 100755
--- a/spec/unit/ssl/certificate_authority/interface.rb
+++ b/spec/unit/ssl/certificate_authority/interface.rb
@@ -176,6 +176,7 @@ describe Puppet::SSL::CertificateAuthority::Interface do
             describe "and an empty array was provided" do
                 it "should print a string containing all certificate requests" do
                     @ca.expects(:waiting?).returns %w{host1 host2}
+                    @ca.stubs(:verify)
 
                     @applier = @class.new(:list, [])
 
@@ -189,12 +190,14 @@ describe Puppet::SSL::CertificateAuthority::Interface do
                 it "should print a string containing all certificate requests and certificates" do
                     @ca.expects(:waiting?).returns %w{host1 host2}
                     @ca.expects(:list).returns %w{host3 host4}
+                    @ca.stubs(:verify)
+                    @ca.expects(:verify).with("host3").raises(Puppet::SSL::CertificateAuthority::CertificateVerificationError.new(23), "certificate revoked")
 
                     @applier = @class.new(:list, :all)
 
                     @applier.expects(:puts).with "host1"
                     @applier.expects(:puts).with "host2"
-                    @applier.expects(:puts).with "+ host3"
+                    @applier.expects(:puts).with "- host3 (certificate revoked)"
                     @applier.expects(:puts).with "+ host4"
 
                     @applier.apply(@ca)
@@ -205,6 +208,7 @@ describe Puppet::SSL::CertificateAuthority::Interface do
                 it "should print a string of all named hosts that have a waiting request" do
                     @ca.expects(:waiting?).returns %w{host1 host2}
                     @ca.expects(:list).returns %w{host3 host4}
+                    @ca.stubs(:verify)
 
                     @applier = @class.new(:list, %w{host1 host2 host3 host4})