Permalink
Browse files

Don't allow the creation of SSL objects with invalid certnames

This ensures we catch invalid certnames earlier in the process, such as
when an agent is starting or making a certificate request, rather than
at the very last step of signing the request. This should make for
better error messages to the user.
  • Loading branch information...
1 parent f341962 commit bd2820ec6ee8a45f57fcc57f79dddde0062cdca7 @nicklewis nicklewis committed Jun 21, 2012
View
@@ -5,6 +5,9 @@ class Puppet::SSL::Base
# For now, use the YAML separator.
SEPARATOR = "\n---\n"
+ # Only allow printing ascii characters, excluding /
+ VALID_CERTNAME = /\A[ -.0-~]+\Z/
@vojtekb

vojtekb Jul 18, 2012

why is '/' illegal?

We are using externally signed certificates for our puppet setup. Before this change it works perfectly, with this change we get the following error:

err: /File[/var/lib/puppet/lib]: Failed to generate additional resources using 'eval_generate: Certname "/c=nl/st=noordholland/l=amsterdam/o=some company/ou=tech ca0 (root ca)/emailaddress=tech@somecompany.com" must not contain unprintable or non-ASCII characters

There are indeed slashes in the name but they were added by legitimate openssl operations.

@jeffmccune

jeffmccune via email Jul 18, 2012

Contributor
@jeffmccune

jeffmccune Jul 18, 2012

Contributor

@vojtekb Please watch #15561 to receive notifications of our progress on fixing this issue. Please also add any impact data about this issue in your site to the ticket.

@vojtekb

vojtekb Jul 20, 2012

@jeffmccune
version 2.7.13 <- worked fine
version 2.7.18 <- not

+
def self.from_multiple_s(text)
text.split(SEPARATOR).collect { |inst| from_s(inst) }
end
@@ -22,6 +25,10 @@ def self.wrapped_class
@wrapped_class
end
+ def self.validate_certname(name)
+ raise "Certname #{name.inspect} must not contain unprintable or non-ASCII characters" unless name =~ VALID_CERTNAME
@vojtekb

vojtekb Jul 18, 2012

BTW. the error message does not mention that '/' is not allowed so users will go and look for really unprintable characters

+ end
+
attr_accessor :name, :content
# Is this file for the CA?
@@ -35,6 +42,7 @@ def generate
def initialize(name)
@name = name.to_s.downcase
+ self.class.validate_certname(@name)
end
# Read content from disk appropriately.
@@ -309,8 +309,7 @@ def check_internal_signing_policies(hostname, csr, allow_dns_alt_names)
raise CertificateSigningError.new(hostname), "CSR subject common name #{cn.inspect} does not match expected certname #{hostname.inspect}"
end
- # Only allow printing ascii characters, excluding /
- if hostname !~ /\A[ -.0-~]+\Z/
+ if hostname !~ Puppet::SSL::Base::VALID_CERTNAME
raise CertificateSigningError.new(hostname), "CSR #{hostname.inspect} subject contains unprintable or non-ASCII characters"
end
View
@@ -236,6 +236,7 @@ def generate
def initialize(name = nil)
@name = (name || Puppet[:certname]).downcase
+ Puppet::SSL::Base.validate_certname(@name)
@key = @certificate = @certificate_request = nil
@ca = (name == self.class.ca_name)
end
@@ -31,6 +31,7 @@
Puppet::SSL::CertificateAuthority.stubs(:ca?).returns false
csr = OpenSSL::X509::Request.new
+ csr.subject = OpenSSL::X509::Name.new([["CN", "anything"]])
subject.getcert(csr.to_pem).should == ''
end

1 comment on commit bd2820e

Contributor

djmitche commented on bd2820e Aug 31, 2012

c.f. gh-1101 for a proposed fix

Please sign in to comment.