diff --git a/lib/puppet/ssl/certificate_factory.rb b/lib/puppet/ssl/certificate_factory.rb
index 71c9a20fe..3d8190c88 100644
--- a/lib/puppet/ssl/certificate_factory.rb
+++ b/lib/puppet/ssl/certificate_factory.rb
@@ -1,160 +1,165 @@
 require 'puppet/ssl'
 
 # The tedious class that does all the manipulations to the
 # certificate to correctly sign it.  Yay.
 module Puppet::SSL::CertificateFactory
   # How we convert from various units to the required seconds.
   UNITMAP = {
     "y" => 365 * 24 * 60 * 60,
     "d" => 24 * 60 * 60,
     "h" => 60 * 60,
     "s" => 1
   }
 
   def self.build(cert_type, csr, issuer, serial)
     # Work out if we can even build the requested type of certificate.
     build_extensions = "build_#{cert_type.to_s}_extensions"
     respond_to?(build_extensions) or
       raise ArgumentError, "#{cert_type.to_s} is an invalid certificate type!"
 
     # set up the certificate, and start building the content.
     cert = OpenSSL::X509::Certificate.new
 
     cert.version    = 2 # X509v3
     cert.subject    = csr.content.subject
     cert.issuer     = issuer.subject
     cert.public_key = csr.content.public_key
     cert.serial     = serial
 
     # Make the certificate valid as of yesterday, because so many people's
     # clocks are out of sync.  This gives one more day of validity than people
     # might expect, but is better than making every person who has a messed up
     # clock fail, and better than having every cert we generate expire a day
     # before the user expected it to when they asked for "one year".
     cert.not_before = Time.now - (60*60*24)
     cert.not_after  = Time.now + ttl
 
     add_extensions_to(cert, csr, issuer, send(build_extensions))
 
     return cert
   end
 
   private
 
   def self.add_extensions_to(cert, csr, issuer, extensions)
     ef = OpenSSL::X509::ExtensionFactory.
       new(cert, issuer.is_a?(OpenSSL::X509::Request) ? cert : issuer)
 
     # Extract the requested extensions from the CSR.
     requested_exts = csr.request_extensions.inject({}) do |hash, re|
       hash[re["oid"]] = [re["value"], re["critical"]]
       hash
     end
 
     # Produce our final set of extensions.  We deliberately order these to
     # build the way we want:
     # 1. "safe" default values, like the comment, that no one cares about.
     # 2. request extensions, from the CSR
     # 3. extensions based on the type we are generating
     # 4. overrides, which we always want to have in their form
     #
     # This ordering *is* security-critical, but we want to allow the user
     # enough rope to shoot themselves in the foot, if they want to ignore our
     # advice and externally approve a CSR that sets the basicConstraints.
     #
     # Swapping the order of 2 and 3 would ensure that you couldn't slip a
     # certificate through where the CA constraint was true, though, if
     # something went wrong up there. --daniel 2011-10-11
     defaults = { "nsComment" => "Puppet Ruby/OpenSSL Internal Certificate" }
     override = { "subjectKeyIdentifier" => "hash" }
 
     exts = [defaults, requested_exts, extensions, override].
       inject({}) {|ret, val| ret.merge(val) }
 
     cert.extensions = exts.map do |oid, val|
       val, crit = *val
       val       = val.join(', ') unless val.is_a? String
 
+      # Enforce the X509v3 rules about subjectAltName being critical:
+      # specifically, it SHOULD NOT be critical if we have a subject, which we
+      # always do. --daniel 2011-10-18
+      crit = false if oid == "subjectAltName"
+
       # val can be either a string, or [string, critical], and this does the
       # right thing regardless of what we get passed.
       ef.create_ext(oid, val, crit)
     end
   end
 
   # TTL for new certificates in seconds. If config param :ca_ttl is set,
   # use that, otherwise use :ca_days for backwards compatibility
   def self.ttl
     ttl = Puppet.settings[:ca_ttl]
 
     return ttl unless ttl.is_a?(String)
 
     raise ArgumentError, "Invalid ca_ttl #{ttl}" unless ttl =~ /^(\d+)(y|d|h|s)$/
 
     $1.to_i * UNITMAP[$2]
   end
 
   # Woot! We're a CA.
   def self.build_ca_extensions
     {
       # This was accidentally omitted in the previous version of this code: an
       # effort was made to add it last, but that actually managed to avoid
       # adding it to the certificate at all.
       #
       # We have some sort of bug, which means that when we add it we get a
       # complaint that the issuer keyid can't be fetched, which breaks all
       # sorts of things in our test suite and, e.g., bootstrapping the CA.
       #
       # http://tools.ietf.org/html/rfc5280#section-4.2.1.1 says that, to be a
       # conforming CA we MAY omit the field if we are self-signed, which I
       # think gives us a pass in the specific case.
       #
       # It also notes that we MAY derive the ID from the subject and serial
       # number of the issuer, or from the key ID, and we definitely have the
       # former data, should we want to restore this...
       #
       # Anyway, preserving this bug means we don't risk breaking anything in
       # the field, even though it would be nice to have. --daniel 2011-10-11
       #
       # "authorityKeyIdentifier" => "keyid:always,issuer:always",
       "keyUsage"               => [%w{cRLSign keyCertSign}, true],
       "basicConstraints"       => ["CA:TRUE", true],
     }
   end
 
   # We're a terminal CA, probably not self-signed.
   def self.build_terminalsubca_extensions
     {
       "keyUsage"         => [%w{cRLSign keyCertSign}, true],
       "basicConstraints" => ["CA:TRUE,pathlen:0", true],
     }
   end
 
   # We're a normal server.
   def self.build_server_extensions
     {
       "keyUsage"         => [%w{digitalSignature keyEncipherment}, true],
       "extendedKeyUsage" => [%w{serverAuth clientAuth emailProtection}, true],
       "basicConstraints" => ["CA:FALSE", true],
     }
   end
 
   # Um, no idea.
   def self.build_ocsp_extensions
     {
       "keyUsage"         => [%w{nonRepudiation digitalSignature}, true],
       "extendedKeyUsage" => [%w{serverAuth OCSPSigning}, true],
       "basicConstraints" => ["CA:FALSE", true],
     }
   end
 
   # Normal client.
   def self.build_client_extensions
     {
       "keyUsage"         => [%w{nonRepudiation digitalSignature keyEncipherment}, true],
       "extendedKeyUsage" => [%w{clientAuth emailProtection}, true],
       "basicConstraints" => ["CA:FALSE", true],
       "nsCertType"       => "client,email",
     }
   end
 end
 
diff --git a/lib/puppet/ssl/certificate_request.rb b/lib/puppet/ssl/certificate_request.rb
index db9ab6109..657ed616e 100644
--- a/lib/puppet/ssl/certificate_request.rb
+++ b/lib/puppet/ssl/certificate_request.rb
@@ -1,151 +1,151 @@
 require 'puppet/ssl/base'
 
 # Manage certificate requests.
 class Puppet::SSL::CertificateRequest < Puppet::SSL::Base
   wraps OpenSSL::X509::Request
 
   extend Puppet::Indirector
 
   # If auto-signing is on, sign any certificate requests as they are saved.
   module AutoSigner
     def save(instance, key = nil)
       super
 
       # Try to autosign the CSR.
       if ca = Puppet::SSL::CertificateAuthority.instance
         ca.autosign
       end
     end
   end
 
   indirects :certificate_request, :terminus_class => :file, :extend => AutoSigner
 
   # Convert a string into an instance.
   def self.from_s(string)
     instance = wrapped_class.new(string)
     name = instance.subject.to_s.sub(/\/CN=/i, '').downcase
     result = new(name)
     result.content = instance
     result
   end
 
   # Because of how the format handler class is included, this
   # can't be in the base class.
   def self.supported_formats
     [:s]
   end
 
   def extension_factory
     @ef ||= OpenSSL::X509::ExtensionFactory.new
   end
 
   # How to create a certificate request with our system defaults.
   def generate(key, options = {})
     Puppet.info "Creating a new SSL certificate request for #{name}"
 
     # Support either an actual SSL key, or a Puppet key.
     key = key.content if key.is_a?(Puppet::SSL::Key)
 
     # If we're a CSR for the CA, then use the real ca_name, rather than the
     # fake 'ca' name.  This is mostly for backward compatibility with 0.24.x,
     # but it's also just a good idea.
     common_name = name == Puppet::SSL::CA_NAME ? Puppet.settings[:ca_name] : name
 
     csr = OpenSSL::X509::Request.new
     csr.version = 0
     csr.subject = OpenSSL::X509::Name.new([["CN", common_name]])
     csr.public_key = key.public_key
 
     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, true)
+      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))
     end
 
     csr.sign(key, OpenSSL::Digest::MD5.new)
 
     raise Puppet::Error, "CSR sign verification failed; you need to clean the certificate request for #{name} on the server" unless csr.verify(key.public_key)
 
     @content = csr
     Puppet.info "Certificate Request fingerprint (md5): #{fingerprint}"
     @content
   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
   # successfully to the OpenSSL constructor later, if you want.
   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
       @content.attributes.find {|x| x.oid == "msExtReq" }
     return [] unless ext
 
     # 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}"
     end
 
     unless ext.value.value.is_a? Array
       raise Puppet::Error, "In #{ext.oid}, expected Set[Array] but found #{ext.value.value.class}"
     end
 
     unless ext.value.value.count == 1
       raise Puppet::Error, "In #{ext.oid}, expected Set[Array[...]], but found #{ext.value.value.count} 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
 
     # OK, now san should be the array of items, validate that...
     index = -1
     san.map do |name|
       index += 1
 
       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}"
       end
       name = name.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.count
       when 2
         ev = OpenSSL::X509::Extension.new(name[0].value, name[1].value)
         { "oid" => ev.oid, "value" => ev.value }
 
       when 3
         ev = OpenSSL::X509::Extension.new(name[0].value, name[2].value, name[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.count}"
       end
     end.flatten
   end
 
   def subject_alt_names
     names = request_extensions.
       select {|x| x["oid"] = "subjectAltName" }.
       map {|x| x["value"].split(/\s*,\s*/) }.
       flatten.
       sort.
       uniq
 
     names.empty? ? nil : names
   end
 end