Add support for parsing common/alt names

[rmccue]

This appears to follow both the specification and browser behaviour, but this should be double-checked before merge.

Pinging @mdawaffe for a review on this. Appears to follow both the RFC and Firefox's implementation.

Notably, doesn't support double wildcards (*.*.x.com), but I'm not sure that browsers support this any more. Google has migrated away from it for App Engine, I believe due to browser support.

[rmccue]

Crap, 5.2 support. Working on this now.

[coveralls]

Coverage decreased (-0%) when pulling 727de7b on common-name into 08e3638 on master.

[coveralls]

Coverage decreased (-0%) when pulling af873c1 on common-name into 08e3638 on master.

[coveralls]

Coverage decreased (-0%) when pulling 7c3c967 on common-name into 08e3638 on master.

[rmccue]

Tests are now passing completely, but 5.2 requires some manual testing here, since Travis skips them (it doesn't have OpenSSL enabled, alas). I'll attempt this tomorrow if no one beats me to it. :)

[coveralls]

Coverage decreased (-0%) when pulling 4502755 on common-name into 08e3638 on master.

[rmccue]

Also to test: self-signed certificates. They should be handled by the built in verification, but we should formalise this with a test.

[rmccue]

Does not support IP address certificates, possibly vulnerable to MITM attacks with those? (CN = google.com, subjectAltName = IP: 127.0.0.1)

[mdawaffe]

Two thoughts about the wildcard matching.

1. From RFC 2818 Section 3.1:

   Names may contain the wildcard character * which is considered to match any single domain name component or component fragment. E.g., .a.com matches foo.a.com but not bar.foo.a.com. f.com matches foo.com but not bar.com.

   So, technically, the matching needs to be fancier than what the code here does. Whether anyone actually does something like "f*.com" (which seems like a silly thing to do), I don't know.

2. If the host is "example.com", the code will test a wildcard match for "*.com", which seems odd.

[mdawaffe]

Some of the test cases have no assertions.

[rmccue]

Some of the test cases have no assertions.

Handled by the @expectedException Requests_Exception tag instead for those.

So, technically, the matching needs to be fancier than what the code here does. Whether anyone actually does something like "f*.com" (which seems like a silly thing to do), I don't know.

@dd32 used regular expressions for this in the WP port, I'll switch to doing the same.

If the host is "example.com", the code will test a wildcard match for "*.com", which seems odd.

Without using the public suffix list and doing extra validation, can't really catch this. Also IMO a problem for the upstream CAs; if they want to issue a certificate for *.com, that's their policy (although I'd expect to see them dropped from the bundle).

Thanks for the feedback! 🍰

[mdawaffe]

Handled by the @expectedException Requests_Exception tag instead for those.

Oh, oops. Nice :)

[dd32]

So, technically, the matching needs to be fancier than what the code here does. Whether anyone actually does something like "f*.com" (which seems like a silly thing to do), I don't know.

@dd32 used regular expressions for this in the WP port, I'll switch to doing the same.

Originally I was using a regular expression for matching, but switched it to only allowing .example.com, no f.com funkyness.

[rmccue]

Originally I was using a regular expression for matching, but switched it to only allowing .example.com, no f.com funkyness.

Noted. I'm wary of allowing f*.com personally, lest it gets exploited.

[rmccue]

For reference, all implementations of Firefox (NSS for desktop versions and httpclientandroidlib on Android) allow f*.com, but only with the wildcard as the final character. They also only allow them for >3 components, as per the following comment from NSS:

/* For a cn pattern to be considered valid, the wildcard character...
 * - may occur only in a DNS name with at least 3 components, and
 * - may occur only as last character in the first component, and
 * - may be preceded by additional characters
 */

I'm happy with those restrictions for Requests.

[rmccue]

To complete before merging:

• Make the pattern checking stricter
• Rearrange certificates to catch bug with old OpenSSL caught by @dd32