From 15c716e5c5888b6c336c5124b39ae75e35420b0e Mon Sep 17 00:00:00 2001
From: Patrick Carlisle <patrick@puppetlabs.com>
Date: Mon, 2 Jul 2012 14:52:28 -0700
Subject: [PATCH] (#13563) Verify CSRs against the embedded public key

This adds a verification check when signing a certificate to ensure proof of
possession of the private key used to sign the CSR.
---
 lib/puppet/ssl/certificate_authority.rb        |  4 ++++
 .../ssl/certificate_authority_spec.rb          | 18 ++++++++++++++++++
 spec/unit/ssl/certificate_authority_spec.rb    |  3 ++-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/ssl/certificate_authority.rb b/lib/puppet/ssl/certificate_authority.rb
index d76a88bcd29..47ad60d0536 100644
--- a/lib/puppet/ssl/certificate_authority.rb
+++ b/lib/puppet/ssl/certificate_authority.rb
@@ -323,6 +323,10 @@ def check_internal_signing_policies(hostname, csr, allow_dns_alt_names)
       raise CertificateSigningError.new(hostname), "CSR subject contains a wildcard, which is not allowed: #{csr.content.subject.to_s}"
     end
 
+    unless csr.content.verify(csr.content.public_key)
+      raise CertificateSigningError.new(hostname), "CSR contains a public key that does not correspond to the signing key"
+    end
+
     unless csr.subject_alt_names.empty?
       # If you alt names are allowed, they are required. Otherwise they are
       # disallowed. Self-signed certs are implicitly trusted, however.
diff --git a/spec/integration/ssl/certificate_authority_spec.rb b/spec/integration/ssl/certificate_authority_spec.rb
index 634ff3289ef..50bb58f34fa 100755
--- a/spec/integration/ssl/certificate_authority_spec.rb
+++ b/spec/integration/ssl/certificate_authority_spec.rb
@@ -122,5 +122,23 @@
         $CHILD_STATUS.should == 0
       end
     end
+
+    it "should verify proof of possession when signing certificates" do
+      csr = @host.certificate_request
+      wrong_key = Puppet::SSL::Key.new(@host.name)
+      wrong_key.generate
+
+      csr.content.public_key = wrong_key.content.public_key
+      # The correct key has to be removed so we can save the incorrect one
+      Puppet::SSL::CertificateRequest.indirection.destroy(@host.name)
+      Puppet::SSL::CertificateRequest.indirection.save(csr)
+
+      expect {
+        @ca.sign(@host.name)
+      }.to raise_error(
+        Puppet::SSL::CertificateAuthority::CertificateSigningError,
+        "CSR contains a public key that does not correspond to the signing key"
+      )
+    end
   end
 end
diff --git a/spec/unit/ssl/certificate_authority_spec.rb b/spec/unit/ssl/certificate_authority_spec.rb
index cb57bece3c9..26ee891f5eb 100755
--- a/spec/unit/ssl/certificate_authority_spec.rb
+++ b/spec/unit/ssl/certificate_authority_spec.rb
@@ -243,8 +243,9 @@ def stub_ca_host
       # Stub out the factory
       Puppet::SSL::CertificateFactory.stubs(:build).returns "my real cert"
 
-      @request_content = stub "request content stub", :subject => OpenSSL::X509::Name.new([['CN', @name]])
+      @request_content = stub "request content stub", :subject => OpenSSL::X509::Name.new([['CN', @name]]), :public_key => stub('public_key')
       @request = stub 'request', :name => @name, :request_extensions => [], :subject_alt_names => [], :content => @request_content
+      @request_content.stubs(:verify).returns(true)
 
       # And the inventory
       @inventory = stub 'inventory', :add => nil