add support for TCPConnector param verify_fingerprint
#361
Conversation
|
Please go ahead! |
| cert = sock.getpeercert(True) | ||
| digest = self._hashfunc(cert).digest() | ||
| if digest != self._fingerprint_bytes: | ||
| raise FingerprintMismatch |
There was a problem hiding this comment.
Use at least FingerprintMismatch(). Maybe later you'll add info about failing host:port pair -- it is helpful for understanding the source of problem.
There was a problem hiding this comment.
Good idea. Addressed this in the latest commit. Also storing the expected vs. got fingerprint.
requiredfield
force-pushed
the
master
branch
2 times, most recently
from
62668ab to
2fbbb9c
Compare
|
@asvetlov Thanks for reviewing! Just pushed a new patch with tests. Would you mind taking a peek at the latest patch? |
| self.assertFalse(conn.resolve) | ||
| self.assertEqual(conn.family, socket.AF_INET) | ||
| self.assertEqual(conn.resolved_hosts, {}) | ||
|
|
||
| def test_tcp_connector_verify_fingerprint(self): | ||
| exc_handler = unittest.mock.Mock() | ||
| self.loop.set_exception_handler(exc_handler) |
There was a problem hiding this comment.
wasn't sure if I needed these two lines with the exc_handler
requiredfield
force-pushed
the
master
branch
2 times, most recently
from
b59208d to
c27f6fb
Compare
| @@ -4,6 +4,8 @@ CHANGES | |||
| 0.16.0 (XX-XX-XXXX) | |||
| ------------------ | |||
|
|
|||
| - Support `verify_fingerprint` param of TCPConnector | |||
|
|
|||
There was a problem hiding this comment.
updated changelog
requiredfield
changed the title
verify_fingerprint
requiredfield
force-pushed
the
master
branch
3 times, most recently
from
ae68bb1 to
a9674ce
Compare
|
Pushed another improved version. Please let me know if there's anything left to do before this is good to merge. Thanks again for reviewing! |
| if self._verify_fingerprint: | ||
| sock = conn[0]._sock | ||
| # gives DER-encoded cert as a sequence of bytes (or None) | ||
| cert = sock.getpeercert(binary_form=True) |
There was a problem hiding this comment.
@asvetlov Do these two lines (502 and 504) look good to you? The conn[0]._sock makes me feel the need to check because of the private member access. As for sock.getpeercert, if sock is a regular socket.socket rather than an SSLSocket this will cause AttributeError, but I think that would only happen if the user tries to use verify_fingerprint with a non-SSL connection. I can code this more defensively though if that would be better.
There was a problem hiding this comment.
conn is (transport, protocol) pair.
For getting socket object from transport please call transport.get_extra_info('socket').
Also you have skip certificate check for non-ssl socket (request.ssl is False).
Not yet ready to merge but starting a PR now to incorporate any early feedback. Thanks for any review!