Manual specification of hostname to verify against #140

Merged
merged 9 commits into from Mar 25, 2013

3 participants

@t-8ch

This allows people to manually specify the hostname they want to verify their ssl certs against.
This should help if SNI insn't available, the server returns the wrong cert or one connects directly to IP addresses.

(Reference: kennethreitz/requests#1124, kennethreitz/requests#749)
This could be enhanced to verify against fingerprints.

@shazow shazow commented on an outdated diff Jan 22, 2013
urllib3/util.py
@@ -24,6 +26,7 @@
import ssl
from ssl import wrap_socket, CERT_NONE, SSLError, PROTOCOL_SSLv23
+ from .exceptions import SSLError
@shazow
Owner
shazow added a note Jan 22, 2013

Add this to the .exceptions import below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Jan 22, 2013
urllib3/util.py
@@ -302,6 +305,43 @@ def resolve_ssl_version(candidate):
return candidate
+
+def match_fingerprint(remote, local):
+ """
+ Compares if both supplied fingerprints match.
+
+ remote -- binary
@shazow
Owner
shazow added a note Jan 22, 2013

Can we keep this consistent with urllib3's style?

:param remote: <Description>

etc

@shazow
Owner
shazow added a note Jan 22, 2013

Not completely clear what "binary" means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Jan 22, 2013
urllib3/util.py
@@ -302,6 +305,43 @@ def resolve_ssl_version(candidate):
return candidate
+
+def match_fingerprint(remote, local):
+ """
+ Compares if both supplied fingerprints match.
+
+ remote -- binary
+ local -- hexstring, can be separated by colons
+ """
+
+ # maps the raw byte length of a digest to its hash function
@shazow
Owner
shazow added a note Jan 22, 2013

Sentence-case comments please (capitalize first letter, period if it's a sentence).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow and 1 other commented on an outdated diff Jan 22, 2013
urllib3/util.py
@@ -302,6 +305,43 @@ def resolve_ssl_version(candidate):
return candidate
+
+def match_fingerprint(remote, local):
@shazow
Owner
shazow added a note Jan 22, 2013

Since this function doesn't return anything but just raises errors, perhaps we should call it something like assert_fingerprint?

@t-8ch
t-8ch added a note Jan 22, 2013

I named it after match_hostname.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Jan 22, 2013
urllib3/connectionpool.py
@@ -80,13 +80,15 @@ class VerifiedHTTPSConnection(HTTPSConnection):
ca_certs = None
ssl_version = None
- def set_cert(self, key_file=None, cert_file=None,
- cert_reqs=None, ca_certs=None):
+ def set_cert(self, key_file=None, cert_file=None, cert_reqs=None,
+ ca_certs=None, verify_hostname=None, verify_fingerprint=None):
@shazow
Owner
shazow added a note Jan 22, 2013

PEP8 indent on (

Ideally, group similar params (e.g. cert_, verify_) per line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Jan 22, 2013
urllib3/connectionpool.py
self.key_file = key_file
self.cert_file = cert_file
self.cert_reqs = cert_reqs
self.ca_certs = ca_certs
+ self.verify_hostname = verify_hostname
@shazow
Owner
shazow added a note Jan 22, 2013

Maybe call these s/verify/assert/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Jan 22, 2013
CONTRIBUTORS.txt
@@ -46,7 +46,11 @@ In chronological order:
* Support for explicitly closing pooled connections
* hartator <hartator@gmail.com>
- * Corrected multipart behavior for params
+ * Corrected multipart behavior for params
+
+* Thomas Weißschuh <thomas@t-8ch.de>
+ * various SSL patches
@shazow
Owner
shazow added a note Jan 22, 2013

Could you change this to "Various SSL patches, with tests"?

Or better yet, "SSL fingerprint checking, with tests"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Jan 22, 2013
urllib3/util.py
@@ -302,6 +305,43 @@ def resolve_ssl_version(candidate):
return candidate
+
+def match_fingerprint(remote, local):
+ """
+ Compares if both supplied fingerprints match.
@shazow
Owner
shazow added a note Jan 22, 2013

Could you give a better description of what this method is for and when it should be used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@t-8ch

@shazow This doesn't really work for Pools to multiple hosts. Are those "supported" and worth effort?

@shazow
Owner

PoolManager? Absolutely, it's the most-used use case for urllib3 (requests depends on it, for example).

@shazow
Owner

Or do you mean multiple hosts in a single HTTP(S)ConnectionPool object? That should not happen.

@t-8ch

I am thinking about assert_same_host in in urlopen()
If this is false a connection can be used with a different host than constructed.
Am I getting this wrong?

@shazow
Owner

Oh, by host you mean literally the Host: header? Yes. But the IPs must match.

@t-8ch

Then the current behaviour of of this PR should be sufficient.
I hope the style is acceptably now, that was really not my day -.-

@t-8ch

@shazow If you are happy with this I'll add the docs.

@shazow shazow commented on an outdated diff Feb 6, 2013
urllib3/util.py
+ fingerprint = fingerprint.replace(':', '').lower()
+
+ digest_length, rest = divmod(len(fingerprint), 2)
+
+ if rest or digest_length not in hashfunc_map:
+ raise SSLError('Fingerprint is of invalid length')
+
+ # We need encode() here for py32, works on py2 and p33
+ fingerprint_bytes = unhexlify(fingerprint.encode())
+
+ hashfunc = hashfunc_map[digest_length]
+
+ cert_digest = hashfunc(cert).digest()
+
+ if not cert_digest == fingerprint_bytes:
+ raise SSLError('Fingerprints did not match!\n'
@shazow
Owner
shazow added a note Feb 6, 2013

We don't have multiline exception messages anywhere. Perhaps make this consistent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on an outdated diff Feb 6, 2013
urllib3/util.py
+ Fingerprint as string of hexdigits, can be interspersed by colons.
+ """
+
+ # Maps the length of a digest to a possible hash function producing
+ # this digest
+ hashfunc_map = {
+ 16: md5,
+ 20: sha1
+ }
+
+ fingerprint = fingerprint.replace(':', '').lower()
+
+ digest_length, rest = divmod(len(fingerprint), 2)
+
+ if rest or digest_length not in hashfunc_map:
+ raise SSLError('Fingerprint is of invalid length')
@shazow
Owner
shazow added a note Feb 6, 2013

I believe we put periods at the end of message sentences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on the diff Feb 6, 2013
urllib3/util.py
@@ -302,6 +304,45 @@ def resolve_ssl_version(candidate):
return candidate
+
+def assert_fingerprint(cert, fingerprint):
+ """
+ Checks if given fingerprint matches the supplied certificate.
+
+ :param cert:
+ Certificate as bytes object.
+ :param fingerprint:
+ Fingerprint as string of hexdigits, can be interspersed by colons.
+ """
+
+ # Maps the length of a digest to a possible hash function producing
+ # this digest
@shazow
Owner
shazow added a note Feb 6, 2013

Periods at the end of sentence comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow
Owner

Looks good to me. :)

Docs appreciated.

@t-8ch

Here we go. Any opinions to my other PRs?

@shazow
Owner

Sorry I haven't had much time to do OSS stuff these last couple of weeks. Next week should be way better though. :)

@t-8ch

@shazow While you are at it: 'ping' 😎

@shazow
Owner

Thank you. :)

Reviewed the latest version of the changes, looks good! I'm happy to merge it next cycle (probably next week).

I wonder if all of the assert_* configuration is getting out of hand and if we should think about a different place or API for those kinds of things. Happy to leave that off for the future though.

@t-8ch

What about a configuration class Verification, like Retries and Timeout?

@shazow
Owner

Hmm might be getting out of hand, or maybe not. Out of curiosity, what do you imagine such a class would look like? :)

@t-8ch

This was just a reference to those other config objects you mentioned on another issue.
I had a use case like this in mind:

pool = HTTPSConnectionpool(host, port,
           verify=Verification(fingerprint='abc', key_file='/foo', cert_file='/bar')

Like some sort of algebraic datatype:

verify = None
verify = Verification(hostname='foo')
verify = Verification(callback=some_func)

My motivation for a fine grained (bloated) API like this is the elimination of all excuses for not verifying certificates in a reasonable manner.

@piotr-dobrogost

I like the idea of grouping parameters pertaining to some aspect of configuration.

@shazow
Owner

Hmm interesting indeed. Then the verification logic can live in the Verification object, which would leave HTTPSConnectionPool much cleaner. Will need to think about what objects the Verification logic would need access to. Looks like just the VerifiedHTTPSConnection should do the trick.

@t-8ch

ping @shazow
Rebased to trigger travis. Added two more tests to keep coverage at 100%.

@shazow shazow merged commit 7f9d8ee into shazow:master Mar 25, 2013

1 check passed

Details default The Travis build passed
@shazow
Owner

Fantastic. @t-8ch, thank you so much for following through with this. :)

@shazow
Owner

Hmm. How would you describe this for the CHANGES.rst?

@t-8ch

"Added mechanism to verify ssl certificates by fingerprint (md5, sha1) or against an arbitrary hostname (when connecting by IP or for misconfigured servers)"

@shazow
Owner

+1

@shazow
Owner

Cool. I think this is a pretty sizeable chunk for v1.6. Maybe I should version bump soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment