Fixing a critical problem in how CRLs were saved and moving SSL Store responsibilities to the SSL::Host class.

I was previously saving invalid CRLs unless they'd had a revocation
done in them; this commit fixes them so that they're always valid.

Also, I've added to SSL::Host the ability to generate a valid
SSL Store, suitable for validation.  This is now used by
Webrick and can be used by the http clients, too.

This should have been two commits, but I'm kind of down the
rabbit hole ATM.

7 files changed, 166 insertions, 84 deletions

diff --git a/lib/puppet/network/http/webrick.rb b/lib/puppet/network/http/webrick.rb
index 9bcf9958f..30085ec47 100644
--- a/lib/puppet/network/http/webrick.rb
+++ b/lib/puppet/network/http/webrick.rb
@@ -93,7 +93,7 @@ class Puppet::Network::HTTP::WEBrick
 
         host.generate unless host.key
 
-        raise Puppet::Error, "Could not retrieve certificate for %s" % host.name unless host.certificate
+        raise Puppet::Error, "Could not retrieve certificate for %s and not running on a valid certificate authority" % host.name unless host.certificate
 
         results[:SSLPrivateKey] = host.key.content
         results[:SSLCertificate] = host.certificate.content
@@ -107,25 +107,11 @@ class Puppet::Network::HTTP::WEBrick
         results[:SSLCACertificateFile] = Puppet[:localcacert]
         results[:SSLVerifyClient] = OpenSSL::SSL::VERIFY_PEER
 
-        results[:SSLCertificateStore] = setup_ssl_store if Puppet[:crl]
+        results[:SSLCertificateStore] = host.ssl_store if Puppet[:crl]
 
         results
     end
 
-    # Create our Certificate revocation list
-    def setup_ssl_store
-        unless crl = Puppet::SSL::CertificateRevocationList.find("ca")
-            raise Puppet::Error, "Could not find CRL; set 'crl' to 'false' to disable CRL usage"
-        end
-        store = OpenSSL::X509::Store.new
-        store.purpose = OpenSSL::X509::PURPOSE_ANY
-        store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK
-
-        store.add_file(Puppet[:localcacert])
-        store.add_crl(crl.content)
-        return store
-    end
-
   private
     
     def setup_handlers
diff --git a/lib/puppet/ssl/certificate_revocation_list.rb b/lib/puppet/ssl/certificate_revocation_list.rb
index 96b71c7a3..3029c14a4 100644
--- a/lib/puppet/ssl/certificate_revocation_list.rb
+++ b/lib/puppet/ssl/certificate_revocation_list.rb
@@ -9,12 +9,23 @@ class Puppet::SSL::CertificateRevocationList < Puppet::SSL::Base
     indirects :certificate_revocation_list, :terminus_class => :file
 
     # Knows how to create a CRL with our system defaults.
-    def generate(cert)
+    def generate(cert, cakey)
         Puppet.info "Creating a new certificate revocation list"
         @content = wrapped_class.new
         @content.issuer = cert.subject
         @content.version = 1
 
+        # Init the CRL number.
+        crlNum = OpenSSL::ASN1::Integer(0)
+        @content.extensions = [OpenSSL::X509::Extension.new("crlNumber", crlNum)]
+
+        # Set last/next update
+        @content.last_update = Time.now
+        # Keep CRL valid for 5 years
+        @content.next_update = Time.now + 5 * 365*24*60*60
+
+        @content.sign(cakey, OpenSSL::Digest::SHA1.new)
+
         @content
     end
 
diff --git a/lib/puppet/ssl/host.rb b/lib/puppet/ssl/host.rb
index 09086e0fa..105b39dc6 100644
--- a/lib/puppet/ssl/host.rb
+++ b/lib/puppet/ssl/host.rb
@@ -164,6 +164,24 @@ class Puppet::SSL::Host
     def public_key
         key.content.public_key
     end
+
+    # Create/return a store that uses our SSL info to validate
+    # connections.
+    def ssl_store(purpose = OpenSSL::X509::PURPOSE_ANY)
+        store = OpenSSL::X509::Store.new
+        store.purpose = purpose
+
+        store.add_file(Puppet[:localcacert])
+
+        if Puppet[:crl]
+            unless crl = Puppet::SSL::CertificateRevocationList.find("ca")
+                raise ArgumentError, "Could not find CRL; set 'crl' to 'false' to disable CRL usage"
+            end
+            store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK
+            store.add_crl(crl.content)
+        end
+        return store
+    end
 end
 
 require 'puppet/ssl/certificate_authority'
diff --git a/spec/integration/ssl/certificate_revocation_list.rb b/spec/integration/ssl/certificate_revocation_list.rb
new file mode 100755
index 000000000..74e45b239
--- /dev/null
+++ b/spec/integration/ssl/certificate_revocation_list.rb
@@ -0,0 +1,45 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2008-5-5.
+#  Copyright (c) 2008. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/ssl/certificate_revocation_list'
+require 'tempfile'
+
+describe Puppet::SSL::CertificateRevocationList do
+    before do
+        # Get a safe temporary file
+        file = Tempfile.new("ca_integration_testing")
+        @dir = file.path
+        file.delete
+
+        Puppet.settings[:confdir] = @dir
+        Puppet.settings[:vardir] = @dir
+
+        Puppet::SSL::Host.ca_location = :local
+    end
+
+    after {
+        Puppet::SSL::Host.ca_location = :none
+
+        system("rm -rf %s" % @dir)
+        Puppet.settings.clear
+
+        # This is necessary so the terminus instances don't lie around.
+        Puppet::SSL::Key.indirection.clear_cache
+        Puppet::SSL::Certificate.indirection.clear_cache
+        Puppet::SSL::CertificateRequest.indirection.clear_cache
+        Puppet::SSL::CertificateRevocationList.indirection.clear_cache
+    }
+
+    it "should be able to read in written out CRLs with no revoked certificates" do
+        ca = Puppet::SSL::CertificateAuthority.new
+
+        raise "CRL not created" unless FileTest.exist?(Puppet[:hostcrl])
+
+        crl = Puppet::SSL::CertificateRevocationList.new("crl_int_testing")
+        crl.read(Puppet[:hostcrl])
+    end
+end
diff --git a/spec/unit/network/http/webrick.rb b/spec/unit/network/http/webrick.rb
index b59dc9f13..6bd3c2785 100644
--- a/spec/unit/network/http/webrick.rb
+++ b/spec/unit/network/http/webrick.rb
@@ -61,6 +61,7 @@ describe Puppet::Network::HTTP::WEBrick, "when turning on listening" do
         @server.expects(:setup_logger).returns(:Logger => :mylogger)
 
         WEBrick::HTTPServer.expects(:new).with {|args|
+            p args
             args[:Logger] == :mylogger
         }.returns(@mock_webrick)
 
@@ -209,62 +210,6 @@ describe Puppet::Network::HTTP::WEBrick do
         @server = Puppet::Network::HTTP::WEBrick.new
     end
 
-    describe "when configuring an x509 store" do
-        before do
-            @store = stub 'store'
-            @store.stub_everything
-
-            @crl = stub 'crl', :content => 'real_crl'
-            Puppet::SSL::CertificateRevocationList.stubs(:find).returns @crl
-
-            @cacert = mock 'cacert'
-            Puppet::SSL::Certificate.stubs(:find).with('ca').returns @crl
-
-            OpenSSL::X509::Store.stubs(:new).returns @store
-        end
-
-        it "should create a new x509 store" do
-            OpenSSL::X509::Store.expects(:new).returns @store
-
-            @server.setup_ssl_store
-        end
-
-        it "should fail if no CRL can be found" do
-            Puppet::SSL::CertificateRevocationList.stubs(:find).returns nil
-
-            lambda { @server.setup_ssl_store }.should raise_error(Puppet::Error)
-        end
-
-        it "should add the CRL to the store" do
-            @store.expects(:add_crl).with "real_crl"
-
-            @server.setup_ssl_store
-        end
-
-        it "should add the CA certificate file to the store" do
-            Puppet.settings.stubs(:value).with(:localcacert).returns "/ca/cert"
-            @store.expects(:add_file).with "/ca/cert"
-
-            @server.setup_ssl_store
-        end
-
-        it "should set the store's flags to 'OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK'" do
-            @store.expects(:flags=).with(OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK)
-
-            @server.setup_ssl_store
-        end
-
-        it "should set the store's purpose to 'OpenSSL::X509::PURPOSE_ANY'" do
-            @store.expects(:purpose=).with OpenSSL::X509::PURPOSE_ANY
-
-            @server.setup_ssl_store
-        end
-
-        it "should return the store" do
-            @server.setup_ssl_store.should equal(@store)
-        end
-    end
-
     describe "when configuring an http logger" do
         before do
             Puppet.settings.stubs(:value).returns "something"
@@ -347,11 +292,9 @@ describe Puppet::Network::HTTP::WEBrick do
 
     describe "when configuring ssl" do
         before do
-            @server.stubs(:setup_ssl_store)
-
             @key = stub 'key', :content => "mykey"
             @cert = stub 'cert', :content => "mycert"
-            @host = stub 'host', :key => @key, :certificate => @cert, :name => "yay"
+            @host = stub 'host', :key => @key, :certificate => @cert, :name => "yay", :ssl_store => "mystore"
 
             Puppet::SSL::Certificate.stubs(:find).with('ca').returns @cert
 
@@ -414,7 +357,7 @@ describe Puppet::Network::HTTP::WEBrick do
             Puppet.settings.stubs(:value).with(:crl).returns true
             Puppet.settings.stubs(:value).with(:hostcrl).returns '/my/crl'
 
-            @server.expects(:setup_ssl_store).returns("mystore")
+            @host.expects(:ssl_store).returns "mystore"
 
             @server.setup_ssl[:SSLCertificateStore].should == "mystore"
         end
@@ -423,7 +366,7 @@ describe Puppet::Network::HTTP::WEBrick do
             Puppet.settings.stubs(:value).returns "whatever"
             Puppet.settings.stubs(:value).with(:crl).returns false
 
-            @server.expects(:setup_ssl_store).never
+            @host.expects(:ssl_store).never
 
             @server.setup_ssl[:SSLCertificateStore].should be_nil
         end
diff --git a/spec/unit/ssl/certificate_revocation_list.rb b/spec/unit/ssl/certificate_revocation_list.rb
index 2efdd187a..13febf744 100755
--- a/spec/unit/ssl/certificate_revocation_list.rb
+++ b/spec/unit/ssl/certificate_revocation_list.rb
@@ -7,6 +7,7 @@ require 'puppet/ssl/certificate_revocation_list'
 describe Puppet::SSL::CertificateRevocationList do
     before do
         @cert = stub 'cert', :subject => "mysubject"
+        @key = stub 'key', :private? => true
 
         @class = Puppet::SSL::CertificateRevocationList
     end
@@ -58,28 +59,50 @@ describe Puppet::SSL::CertificateRevocationList do
         it "should set its issuer to the subject of the passed certificate" do
             @real_crl.expects(:issuer=).with(@cert.subject)
 
-            @crl.generate(@cert)
+            @crl.generate(@cert, @key)
         end
 
         it "should set its version to 1" do
             @real_crl.expects(:version=).with(1)
 
-            @crl.generate(@cert)
+            @crl.generate(@cert, @key)
         end
 
         it "should create an instance of OpenSSL::X509::CRL" do
             OpenSSL::X509::CRL.expects(:new).returns(@real_crl)
 
-            @crl.generate(@cert)
+            @crl.generate(@cert, @key)
+        end
+
+        # The next three tests aren't good, but at least they
+        # specify the behaviour.
+        it "should add an extension for the CRL number" do
+            @real_crl.expects(:extensions=)
+            @crl.generate(@cert, @key)
+        end
+
+        it "should set the last update time" do
+            @real_crl.expects(:last_update=)
+            @crl.generate(@cert, @key)
+        end
+
+        it "should set the next update time" do
+            @real_crl.expects(:next_update=)
+            @crl.generate(@cert, @key)
+        end
+
+        it "should sign the CRL" do
+            @real_crl.expects(:sign).with { |key, digest| key == @key }
+            @crl.generate(@cert, @key)
         end
 
         it "should set the content to the generated crl" do
-            @crl.generate(@cert)
+            @crl.generate(@cert, @key)
             @crl.content.should equal(@real_crl)
         end
 
         it "should return the generated crl" do
-            @crl.generate(@cert).should equal(@real_crl)
+            @crl.generate(@cert, @key).should equal(@real_crl)
         end
     end
 
@@ -88,9 +111,10 @@ describe Puppet::SSL::CertificateRevocationList do
     describe "when revoking a certificate" do
         before do
             @class.wrapped_class.any_instance.stubs(:issuer=)
+            @class.wrapped_class.any_instance.stubs(:sign)
 
             @crl = @class.new("crl")
-            @crl.generate(@cert)
+            @crl.generate(@cert, @key)
             @crl.content.stubs(:sign)
 
             @crl.stubs :save
diff --git a/spec/unit/ssl/host.rb b/spec/unit/ssl/host.rb
index 233bede9b..fb239104f 100755
--- a/spec/unit/ssl/host.rb
+++ b/spec/unit/ssl/host.rb
@@ -380,4 +380,59 @@ describe Puppet::SSL::Host do
             end
         end
     end
+
+    it "should have a method for creating an SSL store" do
+        Puppet::SSL::Host.new("me").should respond_to(:ssl_store)
+    end
+
+    describe "when creating an SSL store" do
+        before do
+            @host = Puppet::SSL::Host.new("me")
+            @store = mock 'store'
+            @store.stub_everything
+            OpenSSL::X509::Store.stubs(:new).returns @store
+
+            Puppet.settings.stubs(:value).returns "ssl_host_testing"
+            Puppet.settings.stubs(:value).with(:crl).returns false
+        end
+
+        it "should accept a purpose" do
+            @store.expects(:purpose=).with "my special purpose"
+            @host.ssl_store("my special purpose")
+        end
+
+        it "should default to OpenSSL::X509::PURPOSE_ANY as the purpose" do
+            @store.expects(:purpose=).with OpenSSL::X509::PURPOSE_ANY
+            @host.ssl_store
+        end
+
+        it "should add the local CA cert file" do
+            Puppet.settings.stubs(:value).with(:localcacert).returns "/ca/cert/file"
+            @store.expects(:add_file).with "/ca/cert/file"
+            @host.ssl_store
+        end
+
+        describe "and the CRL is enabled" do
+            before do
+                Puppet.settings.stubs(:value).with(:crl).returns true
+                @crl = stub 'crl', :content => "real_crl"
+                Puppet::SSL::CertificateRevocationList.stubs(:find).returns @crl
+            end
+
+            it "should fail if no CRL can be found" do
+                Puppet::SSL::CertificateRevocationList.expects(:find).returns nil
+                lambda { @host.ssl_store }.should raise_error(ArgumentError)
+            end
+
+            it "should add the CRL" do
+                @store.expects(:add_crl).with "real_crl"
+                @host.ssl_store
+            end
+
+            it "should set the flags to OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK" do
+                @store.expects(:flags=).with OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK
+                @host.ssl_store
+            end
+        end
+    end
 end