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.

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

Add ssl_context to TCPConnector #211

Merged
merged 4 commits into from Dec 29, 2014
Merged

Add ssl_context to TCPConnector #211

merged 4 commits into from Dec 29, 2014

Conversation

asvetlov
Copy link
Member

See #206 for PR reasons

sslcontext = ssl.create_default_context()
else: # pragma: no cover
# Fallback for Python 3.3.
sslcontext = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
Copy link
Member

Choose a reason for hiding this comment

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

ssl.create_default_context() does a little bit more than his fallback version, like compression disable and check hostnames.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just borrowed the fallback code from selector_events.py:_SelectorSslTransport.__init__: what's good for asyncio is good for aiohttp I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Hm..while it sounds good I wonder it also good if aiohttp will uncanny make client/server affected to CRIME attack depending on under which Python version it runs. I think it's wise to synchronize behaviour for 3.3 with 3.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

Python 3.3 has no ssl.create_default_context() function, and I see no reasons to avoid it if the function is present.

Copy link
Member

Choose a reason for hiding this comment

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

Well, all the need is to sync the behavior. Currently it's different from the fallback case by lines 413, 418 and 437 which gives us next additional code:

import _ssl
sslcontext.options |= getattr(_ssl, "OP_NO_COMPRESSION", 0)
sslcontext.check_hostname = True
sslcontext.load_default_certs('1.3.6.1.5.5.7.3.1')

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, except I like to try use ssl.Purpose first if we run on Python 3.4 (Python 3.3 has not Purpose IMHO).

Copy link
Member

Choose a reason for hiding this comment

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

I didn't found ssl.Purpose for 3.3 quickly, so just picked raw enum value instead. Might worth to add some comment about where it comes from.

@asvetlov
Copy link
Member Author

The problem is: we have no tests for disabling SSLv{bad} etc.
Can we solve it by checking self-generated ssl certificates?
Personally I can pay for, say, Verisign certificate -- but the cert will be expired, maintaining valid certs for obsolete formats makes a mess.
Should we do it at all or better to accurate follow CPython changes (I like the second).
The question is not for PR, though.

sslcontext.options |= ssl.OP_NO_SSLv2
sslcontext.options |= ssl.OP_NO_SSLv3
sslcontext.set_default_verify_paths()
elif hasattr(ssl, 'create_default_context'):
Copy link
Member

Choose a reason for hiding this comment

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

i think it should be module global variable

@fafhrd91
Copy link
Member

i remember there was a way how to get free cert for open source projects

@fafhrd91
Copy link
Member

lgtm

asvetlov added a commit that referenced this pull request Dec 29, 2014
@asvetlov asvetlov merged commit 980c0d0 into master Dec 29, 2014
@asvetlov asvetlov deleted the Issue_206_ssl_context branch December 29, 2014 18:14
@fafhrd91
Copy link
Member

@asvetlov you should move "hasattr(ssl, 'create_default_context')" code to module level

@asvetlov
Copy link
Member Author

ok, will do. Thanks.

@asvetlov
Copy link
Member Author

Done in master.
Do you think it worth 0.13.1 bugfix release?
Personally I guess "no".

@fafhrd91
Copy link
Member

No

On Monday, December 29, 2014, Andrew Svetlov notifications@github.com
wrote:

Done in master.
Do you think it worth 0.13.1 bugfix release?
Personally I guess "no".


Reply to this email directly or view it on GitHub
#211 (comment).

@lock
Copy link

lock bot commented Oct 30, 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.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants