permitted_dns_domain is applied inconsistently

[ericnorris]

Describe the bug
When configuring a CA with permitted_dns_domains and a domain without a leading ., Vault will disallow issuing certificates with a Common Name field that are subdomains for the domain. It will, however, allow SANs that it doesn't consider valid as a Common Name.

To Reproduce
Steps to reproduce the behavior:

1. Configure a root CA with a permitted_dns_domains of example.com
2. Attempt to issue a certificate with a CN of subdomain.example.com
3. Attempt to issue a certificate with a CN of example.com and a SAN of subdomain.example.com

Expected behavior
I expect both steps 2 and 3 should work, but only step 3 does.

Per the RFC:

DNS name restrictions are expressed as host.example.com. Any DNS
name that can be constructed by simply adding zero or more labels to
the left-hand side of the name satisfies the name constraint. For
example, www.host.example.com would satisfy the constraint but
host1.example.com would not.

My reading of this means that when evaluating a DNS name, any leading subdomains are valid. The Vault documentation and tests seem to be using the following from the RFC:

For URIs, the constraint applies to the host part of the name. The
constraint MUST be specified as a fully qualified domain name and MAY
specify a host or a domain. Examples would be "host.example.com" and
".example.com". When the constraint begins with a period, it MAY be
expanded with one or more labels. That is, the constraint
".example.com" is satisfied by both host.example.com and
my.host.example.com. However, the constraint ".example.com" is not
satisfied by "example.com".

This, however, is for URIs - not DNS names - and so in my opinion is not appropriate for validating DNS names in the CN field. It's entirely possible I'm mistaken, but I'd appreciate the dialogue regardless.

Environment:

• Vault Server Version: 0.10.3

[ericnorris]

Hey @jefferai, thanks so much for the speedy fix!

One follow-up comment, if I may: I have unfortunately already generated root CAs that have a permitted_dns_domain of .somedomain, and if I understand e8a740d correctly those will no longer validate. This is because strings.TrimSuffix('blah.somedomain.com', '.somedomain.com') will result in blah, and fail the strings.HasSuffix(pre, ".") check.

Additionally, if I am understanding correctly the current code would validate this is not a dns name .somedomain.com, although this is much less of an issue.

[jefferai]

@ericnorris OK so it turns out that the right thing to do is really actually get rid of that function entirely and let the Go standard library do its thing. I modified the code and tests and it turns out -- this works as expected. Which also means that it doesn't actually match against a self-signed root:

   Name constraints are not applied to self-issued certificates (unless
   the certificate is the final certificate in the path).  (This could
   prevent CAs that use name constraints from employing self-issued
   certificates to implement key rollover.)

The bit about "final certificate in the path" is rather confusing, because it is either always the last certificate in the path (according to standard validation logic), or it is the only certificate in which case it is first and last, but it's unclear then what the difference is. So I'm passing through the values provided, but leaving it up to downstream crypto stacks to interpret exactly what that actually means. This means test cases that failed before don't now because they're actually correct.

This does likely mean you'll need to reissue your root, unfortunately.

As for validating DNS names for validity, for the moment I'm leaving that up to the operator needing to provide a valid name.

[jefferai]

My tests aren't working as I expect yet, but I did want to comment after doing a dive into Go's built-in code: it looks like it actually applies constraints from both roots and intermediates, and it does use .foobar.com notation to allow only subdomains (but foobar.com will match allow the bare and subdomain both).

[jefferai]

OK, more followup.

The real issue here is that the earlier code was attempting to reject generation of child certificates that did not meet criteria. But really it's a verification/validation functionality to check the chain, so Vault was kind of going about this backwards. While the functionality Vault had could allow you to know immediately if something you're issuing is invalid, which I think was kind of the point, it also precludes rolling the issuing cert with one that contains new values (with the Key IDs matching).

So, Go doesn't constrain these on issuing, and we shouldn't either, because I think that's the right read. Accordingly, this value is going to basically just be a straight pass-through into the Go x509 issuing logic.

[ericnorris]

Thanks for following up and being thorough @jefferai! Appreciate it.

[jefferai]

No problem! I want this stuff to work, it can just be hard to grok all the RFCs right :-) (And it doesn't help that often even if you do various crypto stacks behave differently...)