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

Add ssl_context to TCPConnector #211

Merged
merged 4 commits into from
Dec 29, 2014
Merged

Add ssl_context to TCPConnector #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdc6fbf
Add ssl_context to TCPConnector
asvetlov Dec 28, 2014
647b91a
Disable SSLv3
asvetlov Dec 28, 2014
7dd8a96
Set ssl NO_COMPRESSION if present
asvetlov Dec 29, 2014
4511918
Set ssl NO_COMPRESSION if present if verify_ssl=False
asvetlov Dec 29, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions aiohttp/connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,17 @@ class TCPConnector(BaseConnector):
"""

def __init__(self, *args, verify_ssl=True,
resolve=False, family=socket.AF_INET, **kwargs):
resolve=False, family=socket.AF_INET, ssl_context=None,
**kwargs):
if not verify_ssl and ssl_context is not None:
raise ValueError(
"Either disable ssl certificate validation by "
"verify_ssl=False or specify ssl_context, not both.")

super().__init__(*args, **kwargs)

self._verify_ssl = verify_ssl
self._ssl_context = ssl_context
self._family = family
self._resolve = resolve
self._resolved_hosts = {}
Expand All @@ -236,6 +243,33 @@ def verify_ssl(self):
"""Do check for ssl certifications?"""
return self._verify_ssl

@property
def ssl_context(self):
"""SSLContext instance for https requests.

Lazy property, creates context on demand.
"""
if self._ssl_context is None:
if not self._verify_ssl:
sslcontext = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
sslcontext.options |= ssl.OP_NO_SSLv2
Copy link
Member

Choose a reason for hiding this comment

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

Why only SSLv2?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's behavior of current aiohttp.TCPConnector, I didn't modified this code :)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, miss it below. May be worth to touch it then since it cause the way to allow SSLv3 while others forbids that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, SSLv3 has vulnerability issues IIRC.

sslcontext.options |= ssl.OP_NO_SSLv3
sslcontext.options |= getattr(ssl, "OP_NO_COMPRESSION", 0)
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

# Python 3.4+
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.

sslcontext.options |= ssl.OP_NO_SSLv2
sslcontext.options |= ssl.OP_NO_SSLv3
sslcontext.options |= getattr(ssl, "OP_NO_COMPRESSION", 0)
sslcontext.set_default_verify_paths()
sslcontext.verify_mode = ssl.CERT_REQUIRED
self._ssl_context = sslcontext
return self._ssl_context

@property
def family(self):
"""Socket family like AF_INET."""
Expand Down Expand Up @@ -288,11 +322,10 @@ def _create_connection(self, req, **kwargs):

Has same keyword arguments as BaseEventLoop.create_connection.
"""
sslcontext = req.ssl
if req.ssl and not self._verify_ssl:
sslcontext = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
sslcontext.options |= ssl.OP_NO_SSLv2
sslcontext.set_default_verify_paths()
if req.ssl:
sslcontext = self.ssl_context
else:
sslcontext = None

hosts = yield from self._resolve_host(req.host, req.port)

Expand Down
17 changes: 17 additions & 0 deletions tests/test_connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import time
import socket
import unittest
import ssl
from unittest import mock

import aiohttp
Expand Down Expand Up @@ -312,6 +313,22 @@ def test_tcp_connector_clear_resolved_hosts(self):
conn.clear_resolved_hosts()
self.assertEqual(conn.resolved_hosts, {})

def test_ambigous_verify_ssl_and_ssl_context(self):
with self.assertRaises(ValueError):
aiohttp.TCPConnector(
verify_ssl=False,
ssl_context=ssl.SSLContext(ssl.PROTOCOL_SSLv23))

def test_dont_recreate_ssl_context(self):
conn = aiohttp.TCPConnector(loop=self.loop)
ctx = conn.ssl_context
self.assertIs(ctx, conn.ssl_context)

def test_respect_precreated_ssl_context(self):
ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
conn = aiohttp.TCPConnector(loop=self.loop, ssl_context=ctx)
self.assertIs(ctx, conn.ssl_context)


class HttpClientConnectorTests(unittest.TestCase):

Expand Down