Fixed CA race condition (#693)

3 files changed, 49 insertions, 9 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 27aa0fa5e..2e9fd1b34 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,6 @@
 0.23.2 (misspiggy)
+    Fixed CA race condition. (#693)
+
     Added shortname support to config.rb and refactored addargs
 
     Fixed the problem in cron jobs where environment settings
diff --git a/lib/puppet/sslcertificates/ca.rb b/lib/puppet/sslcertificates/ca.rb
index 018640d36..5e355f7fe 100644
--- a/lib/puppet/sslcertificates/ca.rb
+++ b/lib/puppet/sslcertificates/ca.rb
@@ -1,3 +1,5 @@
+require 'sync'
+
 class Puppet::SSLCertificates::CA
     include Puppet::Util::Warnings
 
@@ -226,6 +228,33 @@ class Puppet::SSLCertificates::CA
         }
     end
 
+    # Create an exclusive lock for reading and writing, and do the
+    # writing in a tmp file.
+    def readwritelock(file, mode = 0600)
+        tmpfile = file + ".tmp"
+        sync = Sync.new
+        unless FileTest.directory?(File.dirname(tmpfile))
+            raise Puppet::DevError, "Cannot create %s; directory %s does not exist" %
+                [file, File.dirname(file)]
+        end
+        sync.synchronize(Sync::EX) do
+            File.open(file, "r+", mode) do |rf|
+                rf.lock_exclusive do
+                    File.open(tmpfile, "w", mode) do |tf|
+                        yield tf
+                    end
+                    begin
+                        File.rename(tmpfile, file)
+                    rescue => detail
+                        Puppet.err "Could not rename %s to %s: %s" %
+                            [file, tmpfile, detail]
+                    end
+                end
+            end
+        end
+    end
+
+
     # Sign a given certificate request.
     def sign(csr)
         unless csr.is_a?(OpenSSL::X509::Request)
@@ -238,7 +267,14 @@ class Puppet::SSLCertificates::CA
             raise Puppet::Error, "CSR sign verification failed"
         end
 
-        serial = File.read(@config[:serial]).chomp.hex
+        serial = nil
+        readwritelock(@config[:serial]) { |f|
+            serial = File.read(@config[:serial]).chomp.hex
+
+            # increment the serial
+            f << "%04X" % (serial + 1)
+        }
+
         newcert = Puppet::SSLCertificates.mkcert(
             :type => :server,
             :name => csr.subject,
@@ -248,10 +284,6 @@ class Puppet::SSLCertificates::CA
             :publickey => csr.public_key
         )
 
-        # increment the serial
-        Puppet.config.write(:serial) do |f|
-            f << "%04X" % (serial + 1)
-        end
 
         sign_with_key(newcert)
 
diff --git a/test/certmgr/certmgr.rb b/test/certmgr/certmgr.rb
index ff0a3b61b..fb1611d7f 100755
--- a/test/certmgr/certmgr.rb
+++ b/test/certmgr/certmgr.rb
@@ -239,14 +239,20 @@ class TestCertMgr < Test::Unit::TestCase
 
         ca.revoke(h1.serial)
 
+        oldcert = File.read(Puppet.config[:cacert])
+        oldserial = File.read(Puppet.config[:serial])
+
         # Recreate the CA from disk
         ca = mkCA()
+        newcert = File.read(Puppet.config[:cacert])
+        newserial = File.read(Puppet.config[:serial])
+        assert_equal(oldcert, newcert, "The certs are not equal after making a new CA.")
+        assert_equal(oldserial, newserial, "The serials are not equal after making a new CA.")
         store = mkStore(ca)
-        assert( store.verify(ca.cert))
-        assert(!store.verify(h1, [ca.cert]))
-        assert( store.verify(h2, [ca.cert]))
+        assert( store.verify(ca.cert), "Could not verify CA certs after reloading certs.")
+        assert(!store.verify(h1, [ca.cert]), "Incorrectly verified revoked cert.")
+        assert( store.verify(h2, [ca.cert]), "Could not verify certs with reloaded CA.")
         
-        Puppet.err :yay
         ca.revoke(h2.serial)
         assert_equal(1, ca.crl.extensions.size)