Skip to content
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.

Sign up for GitHub

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

Jump to bottom

support new TCPConnector param fingerprint #366

Merged
merged 1 commit into from
May 20, 2015
Merged

support new TCPConnector param fingerprint #366

merged 1 commit into from
May 20, 2015

Conversation

requiredfield
Copy link
Contributor

@requiredfield requiredfield commented May 15, 2015 •
edited
Loading

Fixes #350

asvetlov
asvetlov reviewed May 15, 2015
View reviewed changes
docs/client.rst
>>> conn = aiohttp.TCPConnector(ssl_context=sslcontext)
>>> r = yield from aiohttp.request(
... 'get', 'https://example.com', connector=conn)

You may also verify certificates via fingerprint::

>>> fp = '...' # hex str of md5, sha1, or sha256 of expected cert (in DER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace '...' with example of arbitrary fingerprint. It may help to newcomers very well.

@requiredfield
Copy link
Contributor Author

@asvetlov Thanks, implemented all your suggestions. Please let me know if latest patch looks better.

Just wasn't sure what you wanted here. Yes, the property returns a string (or None if not set). Happy to rename this (something like expected_fingerprint?) everywhere rather than verify_fingerprint if you prefer?

@requiredfield requiredfield force-pushed the verify_fingerprint branch 2 times, most recently from e93fabc to df23fce Compare May 16, 2015 04:21
@requiredfield requiredfield changed the title support new TCPConnector param verify_fingerprint support new TCPConnector param expect_fingerprint May 16, 2015
@requiredfield
Copy link
Contributor Author

pushed a new version with verify_fingerprint renamed to expect_fingerprint, as well as improvements to weirdness cargo-culted from the urllib3 code

asvetlov
asvetlov reviewed May 17, 2015
View reviewed changes
aiohttp/connector.py
:param bool resolve: Set to True to do DNS lookup for host name.
:param family: socket address family
:param args: see :class:`BaseConnector`
:param kwargs: see :class:`BaseConnector`
"""

def __init__(self, *, verify_ssl=True,
def __init__(self, *, verify_ssl=True, expect_fingerprint=None,
resolve=False, family=socket.AF_INET, ssl_context=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a way to pass bytes fingerprint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a way to pass bytes fingerprint?

Something like 8661202?

@requiredfield requiredfield force-pushed the verify_fingerprint branch 5 times, most recently from ddfc5ef to 8661202 Compare May 18, 2015 03:40
asvetlov
asvetlov reviewed May 18, 2015
View reviewed changes
aiohttp/connector.py
ssl=sslcontext, family=hinfo['family'],
proto=hinfo['proto'], flags=hinfo['flags'],
server_hostname=hinfo['hostname'] if sslcontext else None))
server_hostname=hinfo['hostname'] if sslcontext else None)
if req.ssl and self._expect_fingerprint:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for bad guess, req.ssl may clash with ProxyConnector.

Please use transport.get_extra_info('sslcontext') instead as flag that transport.get_extra_info('socket') has certificate info.

@asvetlov
Copy link
Member

Sorry for long review, I'm striving to make code perfect.
Thanks you for patiently fixing my comments.

After looking on the last version I incline to accept bytes-only fingerprint as connector parameter.
I guess to rename it to just fingerprint -- short name is better.

We need also for str hexlified fingerprints -- both with : inside string and without it.
I guess to add function for converting str fingerprints to bytes ones. aiohttp/helpers.py is good place for that function.

Library users can find the helper function easy if you mention it in TCPConnector documentation with cross-reference.

Clean separation to byte-ish fingerprint for usage in connector and helper for converting user-readable str to bytes required by connector makes sense I feel.

FingerprintMismatch should operates with bytes for got and expected, they are have good enough repr(). We can unhexlify those sometimes in future if we'll find the repr is not readable well.

Both fingerprint property and ctor parameter should be documented in docs/client_reference.rst with ..versionadded: 0.16 tag.

We are in the middle on getting rid of sphinx autodoc generated documentation (docstrings in connector.py still not converted to plain text for example), you may add autodoc docstring to helper function.

If you are too boring in writing documentation please left it to me -- I'll accept your PR after code and tests done but very appreciate your help in documentation also.

@requiredfield requiredfield changed the title support new TCPConnector param expect_fingerprint support new TCPConnector param fingerprint May 19, 2015
@requiredfield requiredfield force-pushed the verify_fingerprint branch 2 times, most recently from 6a8e019 to 0e1219a Compare May 19, 2015 15:52
@requiredfield
support new TCPConnector param fingerprint
173ef3f 
enables ssl certificate pinning
@requiredfield
Copy link
Contributor Author

@asvetlov Happy to put the extra time in on my first contribution. I can always relate to striving to make the code perfect.

Looks like Travis and Appveyor checks completed and are both green. Hope this latest revision does the trick. And thanks for all the careful review and great work on aiohttp.

asvetlov added a commit that referenced this pull request May 20, 2015
@asvetlov
Merge pull request #366 from requiredfield/verify_fingerprint
8309cba 
support new TCPConnector param `fingerprint`
@asvetlov asvetlov merged commit 8309cba into aio-libs:master May 20, 2015
@asvetlov
Copy link
Member

Thank you very much!

@asvetlov asvetlov mentioned this pull request May 20, 2015
Closed
@requiredfield
Copy link
Contributor Author

Thanks for merging!

@requiredfield requiredfield deleted the verify_fingerprint branch May 20, 2015 19:03
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support pinning to ssl certificate via fingerprint
2 participants
@requiredfield @asvetlov