New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SSL certificate verification failed if no dNSName entry in subjectAltName #56209
Comments
When connecting to a SSL server, the certificate verification failed if Example: >>> cert = conn.getpeercert()
>>> cert
{'notAfter': 'May 15 14:31:42 2011 GMT',
'subject': ((('countryName', u'FR'),),
(('stateOrProvinceName', u'Ile-de-France'),),
(('localityName', u'Paris'),),
(('organizationName', 'xxx'),),
(('organizationalUnitName', 'xxx'),),
(('commonName', 'foobar.corp'),),
(('emailAddress', u'test@test.net'),)),
'subjectAltName': (('email', text@test.net'),)} This certificate is valid according to RFC 2818:
Even if the use of CommonName is deprecated, we should not break Current revision of Lib/ssl.py :
Proposed patch is: diff -r 513f6dfd3173 Lib/ssl.py
--- a/Lib/ssl.py Sun May 01 20:24:59 2011 -0500
+++ b/Lib/ssl.py Mon May 02 11:16:46 2011 +0200
@@ -122,8 +122,9 @@
if _dnsname_to_pat(value).match(hostname):
return
dnsnames.append(value)
- if not san:
- # The subject is only checked when subjectAltName is empty
+ if not san and not dnsnames:
+ # The subject is only checked when there is no dNSName entry
+ # in subjectAltName
for sub in cert.get('subject', ()):
for key, value in sub:
# XXX according to RFC 2818, the most specific Common Name |
Are you sure about "if not san and not dnsnames"? It is even more restrictive than the currently condition. "if not dnsnames" looks like it would fit the bill better. Also, better if you can provide a complete patch, including additional test(s) in Lib/test/test_ssl.py. |
P.S.: if you're really right ('have those RFC's, but didn't read Thanks, Steffen! |
Hello Antoine, Steffen, You are absolutely right about removing the 'not san' part. Here is the diff -r c22d5b37f6a4 Lib/ssl.py
--- a/Lib/ssl.py Fri May 06 09:31:02 2011 +0300
+++ b/Lib/ssl.py Fri May 06 12:47:14 2011 +0200
@@ -122,8 +122,9 @@
if _dnsname_to_pat(value).match(hostname):
return
dnsnames.append(value)
- if not san:
- # The subject is only checked when subjectAltName is empty
+ if not dnsnames:
+ # The subject is only checked when there is no dNSName entry
+ # in subjectAltName
for sub in cert.get('subject', ()):
for key, value in sub:
# XXX according to RFC 2818, the most specific Common Name
diff -r c22d5b37f6a4 Lib/test/test_ssl.py
--- a/Lib/test/test_ssl.py Fri May 06 09:31:02 2011 +0300
+++ b/Lib/test/test_ssl.py Fri May 06 12:47:14 2011 +0200
@@ -277,6 +277,24 @@
(('organizationName', 'Google Inc'),))}
fail(cert, 'mail.google.com')
+ # No DNS entry in subjectAltName but a commonName
+ cert = {'notAfter': 'Dec 18 23:59:59 2099 GMT',
+ 'subject': ((('countryName', 'US'),),
+ (('stateOrProvinceName', 'California'),),
+ (('localityName', 'Mountain View'),),
+ (('commonName', 'mail.google.com'),)),
+ 'subjectAltName': (('othername', 'blabla'), )}
+ ok(cert, 'mail.google.com')
+
+ # No DNS entry subjectAltName and no commonName
+ cert = {'notAfter': 'Dec 18 23:59:59 2099 GMT',
+ 'subject': ((('countryName', 'US'),),
+ (('stateOrProvinceName', 'California'),),
+ (('localityName', 'Mountain View'),),
+ (('organizationName', 'Google Inc'),)),
+ 'subjectAltName': (('othername', 'blabla'),)}
+ fail(cert, 'google.com')
+
# Empty cert / no cert
self.assertRaises(ValueError, ssl.match_hostname, None, 'example.com')
self.assertRaises(ValueError, ssl.match_hostname, {}, 'example.com') Steffen, I will submit a bug report to Mercurial as soon as this patch is expected to be integrate in py3k. |
New changeset d4c2a99d1bad by Antoine Pitrou in branch '3.2': New changeset 1b37827984ba by Antoine Pitrou in branch 'default': |
Patch committed in 3.2 and 3.x, thank you! |
In my opinion the RFCs are a bit unclear about how iPAddress subjectAltNames should be handled. (I also don't know if Python currently do the right thing by accepting and matching IP addresses if specified in commonName.) Until now Python failed to the safe side by not matching on subjectAltName iPAddress but also not falling back to commonName if they were specified. AFAICS, with this change it is possible to create strange certificates that Python would accept when an IP address matched commonName but other implementations would reject because of iPAddress mismatch. That is probably not a real problem, but I wanted to point it out as the biggest issue I could find with this fix. Nice catch. We could perhaps add IP addresses to dnsnames even though we don't match on them. |
Hello Mads
Good point! But I think we already have this issue with a certificate cert = { 'subject': ((('commonName', '192.168.1.1'),),)}
ok(cert, '192.168.1.1') Do you think this test should fail? |
Nicolas Bareil wrote, On 05/07/2011 09:48 AM:
Until now I have considered this behaviour OK but undocumented and But the more I look at it the more convinced I get that this test should A consequence of that is that my previous concern is invalid. There is |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: