Skip to content

Add ssl_context to TCPConnector#211

Merged
asvetlov merged 4 commits intomasterfrom
Issue_206_ssl_context
Dec 29, 2014
Merged

Add ssl_context to TCPConnector#211
asvetlov merged 4 commits intomasterfrom
Issue_206_ssl_context

Conversation

@asvetlov
Copy link
Copy Markdown
Member

See #206 for PR reasons

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@fafhrd91
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@asvetlov
Copy link
Copy Markdown
Member Author

ok, will do. Thanks.

@asvetlov
Copy link
Copy Markdown
Member Author

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

@fafhrd91
Copy link
Copy Markdown
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
Copy Markdown

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.

3 participants