Feature/23201/csr extension requests

lib/puppet/ssl/certificate.rb

@@ -44,4 +44,22 @@ def near_expiration?(interval = nil)
   def unmunged_name
     self.class.name_from_subject(content.subject)
   end
+
+  # Any extensions registered with custom OIDs as defined in module
+  # Puppet::SSL::Oids may be looked up here.
+  #
+  # A cert with a 'pp_uuid' extension having the value 'abcd' would return:
+  #
+  # [{ 'oid' => 'pp_uuid', 'value' => 'abcd'}]
+  #
+  # @return [Array<Hash{String => String}>] An array of two element hashes,
+  # with key/value pairs for the extension's oid, and its value.
+  def custom_extensions
+    custom_exts = content.extensions.select do |ext|
+      Puppet::SSL::Oids.subtree_of?('ppRegCertExt', ext.oid) or
+        Puppet::SSL::Oids.subtree_of?('ppPrivCertExt', ext.oid)
+    end
+
+    custom_exts.map { |ext| {'oid' => ext.oid, 'value' => ext.value} }
+  end
 end

lib/puppet/ssl/certificate_request.rb

@@ -66,6 +66,9 @@ def extension_factory
   #   Subject Alternative Names to include in the CSR extension request.
   # @options opts [Hash<String, String, Array<String>>] :csr_attributes A hash
   #   of OIDs and values that are either a string or array of strings.
+  # @options opts [Array<String, String>] :extension_requests A hash of
+  #   certificate extensions to add to the CSR extReq attribute, excluding
+  #   the Subject Alternative Names extension.
   #
   # @raise [Puppet::Error] If the generated CSR signature couldn't be verified
   #
@@ -90,16 +93,8 @@ def generate(key, options = {})
       add_csr_attributes(csr, options[:csr_attributes])
     end
 
-    if options[:dns_alt_names] then
-      names = options[:dns_alt_names].split(/\s*,\s*/).map(&:strip) + [name]
-      names = names.sort.uniq.map {|name| "DNS:#{name}" }.join(", ")
-      names = extension_factory.create_extension("subjectAltName", names, false)
-
-      extReq = OpenSSL::ASN1::Set([OpenSSL::ASN1::Sequence([names])])
-
-      # We only support the standard request extensions.  If you really need
-      # msExtReq support, let us know and we can restore them. --daniel 2011-10-10
-      csr.add_attribute(OpenSSL::X509::Attribute.new("extReq", extReq))
+    if (ext_req_attribute = extension_request_attribute(options))
+      csr.add_attribute(ext_req_attribute)
     end
 
     signer = Puppet::SSL::CertificateSigner.new
@@ -113,69 +108,82 @@ def generate(key, options = {})
   end
 
   # Return the set of extensions requested on this CSR, in a form designed to
-  # be useful to Ruby: a hash.  Which, not coincidentally, you can pass
+  # be useful to Ruby: an array of hashes.  Which, not coincidentally, you can pass
   # successfully to the OpenSSL constructor later, if you want.
+  #
+  # @return [Array<Hash{String => String}>] An array of two or three element
+  # hashes, with key/value pairs for the extension's oid, its value, and
+  # optionally its critical state.
   def request_extensions
     raise Puppet::Error, "CSR needs content to extract fields" unless @content
 
     # Prefer the standard extReq, but accept the Microsoft specific version as
     # a fallback, if the standard version isn't found.
-    ext = @content.attributes.find {|x| x.oid == "extReq" } or
+    attribute = @content.attributes.find {|x| x.oid == "extReq" } or
       @content.attributes.find {|x| x.oid == "msExtReq" }
-    return [] unless ext
+    return [] unless attribute
 
     # Assert the structure and extract the names into an array of arrays.
-    unless ext.value.is_a? OpenSSL::ASN1::Set
-      raise Puppet::Error, "In #{ext.oid}, expected Set but found #{ext.value.class}"
+    unless attribute.value.is_a? OpenSSL::ASN1::Set
+      raise Puppet::Error, "In #{attribute.oid}, expected Set but found #{attribute.value.class}"
     end
 
-    unless ext.value.value.is_a? Array
-      raise Puppet::Error, "In #{ext.oid}, expected Set[Array] but found #{ext.value.value.class}"
+    unless attribute.value.value.is_a? Array
+      raise Puppet::Error, "In #{attribute.oid}, expected Set[Array] but found #{attribute.value.value.class}"
     end
 
-    unless ext.value.value.length == 1
-      raise Puppet::Error, "In #{ext.oid}, expected Set[Array[...]], but found #{ext.value.value.length} items in the array"
-    end
-
-    san = ext.value.value.first
-    unless san.is_a? OpenSSL::ASN1::Sequence
-      raise Puppet::Error, "In #{ext.oid}, expected Set[Array[Sequence[...]]], but found #{san.class}"
-    end
-    san = san.value
+    extensions = attribute.value.value
 
-    # OK, now san should be the array of items, validate that...
     index = -1
-    san.map do |name|
+    extensions.map do |extension|
       index += 1
+      context = "#{attribute.oid} extension index #{index}"
+
+      unless extension.is_a? OpenSSL::ASN1::Sequence
+        raise Puppet::Error, "In #{context}, expected Set[Array[Sequence[...]]], but found #{extension.class}"
+      end
+
+      unless extension.value.is_a? Array
+        raise Puppet::Error, "In #{context}, expected Set[Array[Sequence[Array[...]]]], but found #{extension.value.class}"
+      end
+
+      unless extension.value.size == 1
+        raise Puppet::Error, "In #{context}, expected Set[Array[Sequence[Array[...]]]] with one value, but found #{extension.value.size} elements"
+      end
 
-      unless name.is_a? OpenSSL::ASN1::Sequence
-        raise Puppet::Error, "In #{ext.oid}, expected request extension record #{index} to be a Sequence, but found #{name.class}"
+      unless extension.value.first.is_a? OpenSSL::ASN1::Sequence
+        raise Puppet::Error, "In #{context}, expected Set[Array[Sequence[Array[Sequence[...]]]]] but found #{extension.value.first.class}"
       end
-      name = name.value
+
+      unless extension.value.first.value.is_a? Array
+        raise Puppet::Error, "In #{context}, expected Set[Array[Sequence[Array[Sequence[Array[...]]]]]], but found #{extension.value.first.value.class}"
+      end
+
+      ext_values = extension.value.first.value
 
       # OK, turn that into an extension, to unpack the content.  Lovely that
       # we have to swap the order of arguments to the underlying method, or
       # perhaps that the ASN.1 representation chose to pack them in a
       # strange order where the optional component comes *earlier* than the
       # fixed component in the sequence.
-      case name.length
+      case ext_values.length
       when 2
-        ev = OpenSSL::X509::Extension.new(name[0].value, name[1].value)
+        ev = OpenSSL::X509::Extension.new(ext_values[0].value, ext_values[1].value)
         { "oid" => ev.oid, "value" => ev.value }
 
       when 3
-        ev = OpenSSL::X509::Extension.new(name[0].value, name[2].value, name[1].value)
+        ev = OpenSSL::X509::Extension.new(ext_values[0].value, ext_values[2].value, ext_values[1].value)
         { "oid" => ev.oid, "value" => ev.value, "critical" => ev.critical? }
 
       else
-        raise Puppet::Error, "In #{ext.oid}, expected extension record #{index} to have two or three items, but found #{name.length}"
+        raise Puppet::Error, "In #{attribute.oid}, expected extension record #{index} to have two or three items, but found #{ext_values.length}"
       end
-    end.flatten
+    end
   end
 
   def subject_alt_names
     @subject_alt_names ||= request_extensions.
-      select {|x| x["oid"] = "subjectAltName" }.
+      select {|x| x["oid"] == "subjectAltName" }.
       map {|x| x["value"].split(/\s*,\s*/) }.
       flatten.
       sort.
@@ -232,4 +240,39 @@ def add_csr_attributes(csr, csr_attributes)
       Puppet.debug("Added csr attribute: #{oid} => #{attr_set.inspect}")
     end
   end
+
+  private
+
+  PRIVATE_EXTENSIONS = [
+    'subjectAltName', '2.5.29.17',
+  ]
+
+  # @api private
+  def extension_request_attribute(options)
+    extensions = []
+
+    if options[:extension_requests]
+      options[:extension_requests].each_pair do |oid, value|
+        if PRIVATE_EXTENSIONS.include? oid
+          raise Puppet::Error, "Cannot specify CSR extension request #{oid}: conflicts with internally used extension request"
+        end
+
+        ext = OpenSSL::X509::Extension.new(oid, value.to_s, false)
+        extensions << OpenSSL::ASN1::Sequence([ext])
+      end
+    end
+
+    if options[:dns_alt_names]
+      names = options[:dns_alt_names].split(/\s*,\s*/).map(&:strip) + [name]
+      names = names.sort.uniq.map {|name| "DNS:#{name}" }.join(", ")
+      alt_names_ext = extension_factory.create_extension("subjectAltName", names, false)
+
+      extensions << OpenSSL::ASN1::Sequence([alt_names_ext])
+    end
+
+    unless extensions.empty?
+      ext_req = OpenSSL::ASN1::Set(extensions)
+      OpenSSL::X509::Attribute.new("extReq", ext_req)
+    end
+  end
 end

lib/puppet/ssl/certificate_request_attributes.rb

@@ -7,11 +7,12 @@
 # @api private
 class Puppet::SSL::CertificateRequestAttributes
 
-  attr_reader :path, :custom_attributes
+  attr_reader :path, :custom_attributes, :extension_requests
 
   def initialize(path)
     @path = path
     @custom_attributes = {}
+    @extension_requests = {}
   end
 
   # Attempt to load a yaml file at the given @path.
@@ -22,6 +23,7 @@ def load
     if Puppet::FileSystem::File.exist?(path)
       hash = Puppet::Util::Yaml.load_file(path)
       @custom_attributes = hash.delete('custom_attributes') || {}
+      @extension_requests = hash.delete('extension_requests') || {}
       if not hash.keys.empty?
         raise Puppet::Error, "unexpected attributes #{hash.keys.inspect} in #{@path.inspect}"
       end

lib/puppet/ssl/host.rb

@@ -177,6 +177,7 @@ def generate_certificate_request(options = {})
     csr_attributes = Puppet::SSL::CertificateRequestAttributes.new(Puppet[:csr_attributes])
     if csr_attributes.load
       options[:csr_attributes] = csr_attributes.custom_attributes
+      options[:extension_requests] = csr_attributes.extension_requests
     end
 
     @certificate_request = CertificateRequest.new(name)

spec/integration/ssl/autosign_spec.rb

@@ -0,0 +1,90 @@
+require 'spec_helper'
+
+describe "autosigning" do
+  include PuppetSpec::Files
+
+  let(:puppet_dir) { tmpdir("ca_autosigning") }
+  let(:csr_attributes_content) do
+    {
+      'custom_attributes' => {
+        '1.3.6.1.4.1.34380.2.0' => 'hostname.domain.com',
+        '1.3.6.1.4.1.34380.2.1' => 'my passphrase',
+        '1.3.6.1.4.1.34380.2.2' => # system IPs in hex
+          [ 0xC0A80001, # 192.168.0.1
+            0xC0A80101 ], # 192.168.1.1
+      },
+      'extension_requests' => {
+        'pp_uuid' => 'abcdef',
+        '1.3.6.1.4.1.34380.1.1.2' => '1234', # pp_instance_id
+        '1.3.6.1.4.1.34380.1.2.1' => 'some-value', # private extension
+      },
+    }
+  end
+
+  let(:host) { Puppet::SSL::Host.new }
+
+  before do
+    Puppet.settings[:confdir] = puppet_dir
+    Puppet.settings[:vardir] = puppet_dir
+
+    # This is necessary so the terminus instances don't lie around.
+    Puppet::SSL::Key.indirection.termini.clear
+  end
+
+  context "with extension requests from csr_attributes file" do
+    let(:ca) { Puppet::SSL::CertificateAuthority.new }
+
+    def write_csr_attributes
+      File.open(Puppet.settings[:csr_attributes], 'w') do |file|
+        file.puts YAML.dump(csr_attributes_content)
+      end
+    end
+
+    context "and subjectAltName" do
+      it "raises an error if you include subjectAltName in csr_attributes" do
+        csr_attributes_content['extension_requests']['subjectAltName'] = 'foo'
+        write_csr_attributes
+        expect { host.generate_certificate_request }.to raise_error(Puppet::Error, /subjectAltName.*conflicts with internally used extension request/)
+      end
+
+      it "properly merges subjectAltName when in settings" do
+        Puppet.settings[:dns_alt_names] = 'althostname.nowhere'
+        write_csr_attributes
+        host.generate_certificate_request
+        csr = Puppet::SSL::CertificateRequest.indirection.find(host.name)
+        expect(csr.subject_alt_names).to include('DNS:althostname.nowhere')
+      end
+    end
+
+    context "without subjectAltName" do
+
+      before do
+        write_csr_attributes
+        host.generate_certificate_request
+      end
+
+      it "pulls extension attributes from the csr_attributes file into the certificate" do
+        csr = Puppet::SSL::CertificateRequest.indirection.find(host.name)
+        expect(csr.request_extensions).to have(3).items
+        expect(csr.request_extensions).to include('oid' => 'pp_uuid', 'value' => 'abcdef')
+        expect(csr.request_extensions).to include('oid' => 'pp_instance_id', 'value' => '1234')
+        expect(csr.request_extensions).to include('oid' => '1.3.6.1.4.1.34380.1.2.1', 'value' => 'some-value')
+      end
+
+      it "copies extension requests to certificate" do
+        cert = ca.sign(host.name)
+        expect(cert.custom_extensions).to include('oid' => 'pp_uuid', 'value' => 'abcdef')
+        expect(cert.custom_extensions).to include('oid' => 'pp_instance_id', 'value' => '1234')
+        expect(cert.custom_extensions).to include('oid' => '1.3.6.1.4.1.34380.1.2.1', 'value' => 'some-value')
+      end
+
+      it "does not copy custom attributes to certificate" do
+        cert = ca.sign(host.name)
+        cert.custom_extensions.each do |ext|
+          expect(Puppet::SSL::Oids.subtree_of?('1.3.6.1.4.1.34380.2', ext['oid'])).to be_false
+        end
+      end
+    end
+
+  end
+end

spec/integration/ssl/certificate_authority_spec.rb

@@ -21,10 +21,6 @@
     @ca = Puppet::SSL::CertificateAuthority.new
   end
 
-  after do
-    Puppet::SSL::CertificateAuthority.instance_variable_set("@instance", nil)
-  end
-
   it "should be able to generate a new host certificate" do
     ca.generate("newhost")
 

spec/unit/ssl/certificate_request_spec.rb

@@ -219,6 +219,62 @@
       end
     end
 
+    context "with extension requests" do
+      let(:extension_data) do
+        {
+          '1.3.6.1.4.1.34380.1.1.31415' => 'pi',
+          '1.3.6.1.4.1.34380.1.1.2718'  => 'e',
+        }
+      end
+
+      it "adds an extreq attribute to the CSR" do
+        request.generate(key, :extension_requests => extension_data)
+
+        exts = request.content.attributes.select { |attr| attr.oid = 'extReq' }
+        exts.length.should == 1
+      end
+
+      it "adds an extension for each entry in the extension request structure" do
+        request.generate(key, :extension_requests => extension_data)
+
+        exts = request.request_extensions
+
+        exts.should include('oid' => '1.3.6.1.4.1.34380.1.1.31415', 'value' => 'pi')
+        exts.should include('oid' => '1.3.6.1.4.1.34380.1.1.2718', 'value' => 'e')
+      end
+
+      it "defines the extensions as non-critical" do
+        request.generate(key, :extension_requests => extension_data)
+        request.request_extensions.each do |ext|
+          ext['critical'].should be_false
+        end
+      end
+
+      it "rejects the subjectAltNames extension" do
+        san_names = ['subjectAltName', '2.5.29.17']
+        san_field = 'DNS:first.tld, DNS:second.tld'
+
+        san_names.each do |name|
+          expect do
+            request.generate(key, :extension_requests => {name => san_field})
+          end.to raise_error Puppet::Error, /conflicts with internally used extension/
+        end
+      end
+
+      it "merges the extReq attribute with the subjectAltNames extension" do
+        request.generate(key,
+                         :dns_alt_names => 'first.tld, second.tld',
+                         :extension_requests => extension_data)
+        exts = request.request_extensions
+
+        exts.should include('oid' => '1.3.6.1.4.1.34380.1.1.31415', 'value' => 'pi')
+        exts.should include('oid' => '1.3.6.1.4.1.34380.1.1.2718', 'value' => 'e')
+        exts.should include('oid' => 'subjectAltName', 'value' => 'DNS:first.tld, DNS:myname, DNS:second.tld')
+
+        request.subject_alt_names.should eq ['DNS:first.tld', 'DNS:myname', 'DNS:second.tld']
+      end
+    end
+
     it "should sign the csr with the provided key" do
       request.generate(key)
       request.content.verify(key.content.public_key).should be_true