pki: allowed_domains are compared case sensitive if they use glob patterns

[gnugnug]

Steps to Reproduce:

# Create a role with allow_glob_domains enabled
vault write pki/roles/foo allowed_domains="example.com, *.example.com" allow_glob_domains=true
# Issuing certificates works regardless of case
vault write pki/issue/foo common_name="example.com" #works
vault write pki/issue/foo common_name="EXAMPLE.COM" #works
# When using an allowed_domain with a glob pattern case must suddenly match
vault write pki/issue/foo common_name="sub.example.com" #works
vault write pki/issue/foo common_name="SUB.EXAMPLE.COM" #fails with
["common name SUB.EXAMPLE.COM not allowed by this role"]

I would expect that the last command is successful as well, so that their behaviour is consistent. The domain names in the certificate are not case sensitive.

[cipherboy]

Hmm, I'm thinking we'll have to do a strings.ToLower(...) on both the glob pattern and the supplied domain name. Thoughts @stevendpclark or @kitography? Is there something that I'm missing here? I think per RFC 5280, we'd expect puny-coded domains, but this doesn't apply to non-domain CNs IIRC. I don't think there's an approach using strings.EqualFold(...) without perhaps forking https://github.com/ryanuber/go-glob.

[kitography]

I don't see any reference to Punycode, or RFC3492 in RFC5280?

It doesn't look as easy to solve to me as "strings.ToLower()".

There is an incredibly intricate standard here for matching non-glob things: https://www.rfc-editor.org/rfc/rfc5280#section-7.1 which refers to : https://www.rfc-editor.org/rfc/rfc4518#section-2.2 which refers to: https://www.rfc-editor.org/rfc/rfc3454

As far as the RFCs go, I found very little on matching globs/wildcards, or more, I found ( https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.6 ), which effectively gives up:

   Finally, the semantics of subject alternative names that include
   wildcard characters (e.g., as a placeholder for a set of names) are
   not addressed by this specification.  Applications with specific
   requirements MAY use such names, but they must define the semantics.

So, the options I'm seeing are:

• keep being a bit more strict on wildcards (which is a little weird in this example, but probably fine), or
• find a string prep that ignore wildcards, then use wildcards to match? (if there's a library doing it, I'd happily upgrade. )
• write our own half-match thing: I'm kind of concerned that toLower (in particularly on the matcher string) is going to have some weird effects on international languages here, so I'd need a lot more time to go over examples to see that work - I'd be particularly concerned about, for instance, final sigma (two lower case letters mapping to a single upper-case letter). It looks like https://www.unicode.org/reports/tr21/tr21-5.html defines casefold separately than just matching on to lower.

[cipherboy]

@kitography In this context, glob is a Vault-specific mechanism that simply says "match anything"; this isn't on the time-of-use cert validation, but time-of-issuance-against-a-policy validation mechanism. (i.e., this isn't a wildcard in the certificate's common_name field, we've already trimmed that up, but is instead a glob in the policy to allow say, matching something.example.com and else.example.com by specifying *.example.com in the policy itself).

The reference in RFC 5280 is under section 7.2:

Internationalized Domain Names (IDNs) may be included in certificates and CRLs in the subjectAltName and issuerAltName extensions, name constraints extension, authority information access extension, subject information access extension, CRL distribution points extension, and issuing distribution point extension. Each of these extensions uses the GeneralName type; one choice in GeneralName is the dNSName field, which is defined as type IA5String.

...

&c. They seem to use IDNs here, rather than punycode.

Here, since certs are assumed to be in IDN form for CNs/DNs/... when referring to web addresses, using strings.ToLower(...) on them should be fine. But I'm not sure if it would introduce problems for non-domain CNs (e.g., a person's name).

While I agree nominally that domain names aren't case sensitive, I'm not sure if this is generalizable. E.g., RFC 5280 Section 8 includes:

...

While the local-part of an electronic mail address is case sensitive [RFC2821], emailAddress attribute values are not case sensitive [RFC2985].

...

Notably, RFC 4519 Section 2.3 (from which, I believe RFC 5280 inherits its subject binding typing) has this definition about CN:

      ( 2.5.4.3 NAME 'cn'
         SUP name )

Which means it then falls on RFC section 2.18 as that is the superclass (name):

      ( 2.5.4.41 NAME 'name'
         EQUALITY caseIgnoreMatch
         SUBSTR caseIgnoreSubstringsMatch
         SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 )

Which probably means we're probably OK to use strings.ToLower given that it appears to have been designed at least thinking about Unicode.

My 2c.?

Edited to add: #14238 was the PR that added the stricter validation of wildcards that you were mentioning :-)

[gnugnug]

Thanks for the valuable insights. My point is that Vault already case-foldes domains without a glob pattern before comparison, so I don't see why it shouldn't do the same for the static part of domains with glob patterns.

[cipherboy]

Yes, I think that's a valid observation, hence the bug label for this. :-)

Do you want to open a PR using strings.ToLower(...) here in PKI?

Thinking about this more, and looking at the code for go-glob, I think the shortcoming is here in the upstream by using strings.Index(...) as this is case sensitive and also potentially unicode-encoding sensitive... It doesn't appear that the Go standard library has an Index variant that's unicode encoding insensitive, so it'd probably be a much larger PR than the upstream is expecting, unless we agree strings.ToLower(...) is sufficient in all cases, which I'm not (personally) convinced of.

At any rate, I'd take the easy PR now and hopefully that will improve things for the majority of users :-)

[KushnerykPavel]

Hello! @cipherboy may I pick this bug ?

[stevendpclark]

@KushnerykPavel please feel free!

[KushnerykPavel]

PR: #22126

[cipherboy]

unless we agree strings.ToLower(...) is sufficient in all cases, which I'm not (personally) convinced of.

Just for future me and others: this was merged and should be fine when common_name is strictly a hostname (which should be punycode ASCII per RFC 5280 IIRC).

However, when using certs for client auth with general names, this could still not work as e.g., we'd compare:

[E'] -> [e'] != [e][combine][']

we've not made it any worse, and this wouldn't have worked before, but the shortcomings are still there.