From c071c69e1b5a0d897674a1f7adc6ff32f19400ff Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Wed, 27 May 2015 12:49:44 -0300 Subject: [bug] Use BrowserLikePolicyForHTTPS for checking While testing the way that its implemented now, I found out that no check is being made on certificate attributes against the host. I found this simple way of creating a BrowserLikePolicyForHTTPS using a self signed cert and it worked on my test. I used test_https from Soledad for checking this (which we are fixing on another branch). Also, we don't want to depend on twisted for other things than leap.common.http. --- pkg/requirements.pip | 2 -- setup.py | 7 +++++ src/leap/common/certs.py | 17 ++++++++++++ src/leap/common/http.py | 70 +++++++++++++----------------------------------- 4 files changed, 42 insertions(+), 54 deletions(-) diff --git a/pkg/requirements.pip b/pkg/requirements.pip index f875344..1977cc8 100644 --- a/pkg/requirements.pip +++ b/pkg/requirements.pip @@ -2,8 +2,6 @@ jsonschema #<=0.8 -- are we done with this conflict? dirspec pyopenssl python-dateutil -Twisted>=12.1 -zope.interface pyzmq txzmq diff --git a/setup.py b/setup.py index a7de8f9..776a477 100644 --- a/setup.py +++ b/setup.py @@ -138,4 +138,11 @@ setup( tests_require=tests_requirements, include_package_data=True, zip_safe=False, + + extras_require={ + # needed for leap.common.http + # service_identity needed for propper hostname identification, + # see http://twistedmatrix.com/documents/current/core/howto/ssl.html + 'Twisted': ["Twisted>=14.0.2", "service_identity", "zope.insterface"] + }, ) diff --git a/src/leap/common/certs.py b/src/leap/common/certs.py index db513f6..c8e0743 100644 --- a/src/leap/common/certs.py +++ b/src/leap/common/certs.py @@ -178,3 +178,20 @@ def should_redownload(certfile, now=time.gmtime): return True return False + + +def get_compatible_ssl_context_factory(cert_path=None): + import twisted + cert = None + if twisted.version.base() > '14.0.1': + from twisted.web.client import BrowserLikePolicyForHTTPS + from twisted.internet import ssl + if cert_path: + cert = ssl.Certificate.loadPEM(open(cert_path).read()) + policy = BrowserLikePolicyForHTTPS(cert) + return policy + else: + raise Exception((""" + Twisted 14.0.2 is needed in order to have secure Client Web SSL Contexts, not %s + See: http://twistedmatrix.com/trac/ticket/7647 + """) % (twisted.version.base())) diff --git a/src/leap/common/http.py b/src/leap/common/http.py index 39f01ba..1dc5642 100644 --- a/src/leap/common/http.py +++ b/src/leap/common/http.py @@ -18,22 +18,28 @@ Twisted HTTP/HTTPS client. """ -import os +try: + import twisted +except ImportError: + print "*******" + print "Twisted is needed to use leap.common.http module" + print "" + print "Install the extra requirement of the package:" + print "$ pip install leap.common[Twisted]" + import sys + sys.exit(1) -from zope.interface import implements -from OpenSSL.crypto import load_certificate -from OpenSSL.crypto import FILETYPE_PEM +from leap.common.certs import get_compatible_ssl_context_factory + +from zope.interface import implements from twisted.internet import reactor -from twisted.internet.ssl import ClientContextFactory -from twisted.internet.ssl import CertificateOptions from twisted.internet.defer import succeed from twisted.web.client import Agent from twisted.web.client import HTTPConnectionPool from twisted.web.client import readBody -from twisted.web.client import BrowserLikePolicyForHTTPS from twisted.web.http_headers import Headers from twisted.web.iweb import IBodyProducer @@ -55,33 +61,12 @@ class HTTPClient(object): self._pool = HTTPConnectionPool(reactor, persistent=True) self._pool.maxPersistentPerHost = 10 - if cert_file: - cert = self._load_cert(cert_file) - self._agent = Agent( - reactor, - HTTPClient.ClientContextFactory(cert), - pool=self._pool) - else: - # trust the system's CAs - self._agent = Agent( - reactor, - BrowserLikePolicyForHTTPS(), - pool=self._pool) - - def _load_cert(self, cert_file): - """ - Load a X509 certificate from a file. - - :param cert_file: The path to the certificate file. - :type cert_file: str + policy = get_compatible_ssl_context_factory(cert_file) - :return: The X509 certificate. - :rtype: OpenSSL.crypto.X509 - """ - if os.path.exists(cert_file): - with open(cert_file) as f: - data = f.read() - return load_certificate(FILETYPE_PEM, data) + self._agent = Agent( + reactor, + policy, + pool=self._pool) def request(self, url, method='GET', body=None, headers={}): """ @@ -106,25 +91,6 @@ class HTTPClient(object): d.addCallback(readBody) return d - class ClientContextFactory(ClientContextFactory): - """ - A context factory that will verify the server's certificate against a - given CA certificate. - """ - - def __init__(self, cacert): - """ - Initialize the context factory. - - :param cacert: The CA certificate. - :type cacert: OpenSSL.crypto.X509 - """ - self._cacert = cacert - - def getContext(self, hostname, port): - opts = CertificateOptions(verify=True, caCerts=[self._cacert]) - return opts.getContext() - class StringBodyProducer(object): """ A producer that writes the body of a request to a consumer. -- cgit v1.2.3 From bf18c2bc6e3f533187281a3b31febd37ef22f8c0 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Wed, 27 May 2015 17:19:13 -0300 Subject: [feat] Make it optional to have a dedicated pool As @meskio pointed out, some cases could need a dedicated pool with different parameters. This is a suggested implementation where the pool is reused by default, creating a dedicated one just if needed/asked. This way we ensure that resources are under control and special cases are still handled. --- changes/bug-browserlikepolicy-https | 1 + src/leap/common/http.py | 14 ++++++--- src/leap/common/tests/test_http.py | 62 +++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 changes/bug-browserlikepolicy-https create mode 100644 src/leap/common/tests/test_http.py diff --git a/changes/bug-browserlikepolicy-https b/changes/bug-browserlikepolicy-https new file mode 100644 index 0000000..bceb129 --- /dev/null +++ b/changes/bug-browserlikepolicy-https @@ -0,0 +1 @@ +- Makes https client use Twisted SSL validation and adds a reuse by default behavior on connection pool diff --git a/src/leap/common/http.py b/src/leap/common/http.py index 1dc5642..1e384e5 100644 --- a/src/leap/common/http.py +++ b/src/leap/common/http.py @@ -44,13 +44,21 @@ from twisted.web.http_headers import Headers from twisted.web.iweb import IBodyProducer +def createPool(maxPersistentPerHost=10, persistent=True): + pool = HTTPConnectionPool(reactor, persistent) + pool.maxPersistentPerHost = maxPersistentPerHost + return pool + +_pool = createPool() + + class HTTPClient(object): """ HTTP client done the twisted way, with a main focus on pinning the SSL certificate. """ - def __init__(self, cert_file=None): + def __init__(self, cert_file=None, pool=_pool): """ Init the HTTP client @@ -58,15 +66,13 @@ class HTTPClient(object): system's CAs will be used. :type cert_file: str """ - self._pool = HTTPConnectionPool(reactor, persistent=True) - self._pool.maxPersistentPerHost = 10 policy = get_compatible_ssl_context_factory(cert_file) self._agent = Agent( reactor, policy, - pool=self._pool) + pool=pool) def request(self, url, method='GET', body=None, headers={}): """ diff --git a/src/leap/common/tests/test_http.py b/src/leap/common/tests/test_http.py new file mode 100644 index 0000000..e240ca3 --- /dev/null +++ b/src/leap/common/tests/test_http.py @@ -0,0 +1,62 @@ +# -*- coding: utf-8 -*- +# test_http.py +# Copyright (C) 2013 LEAP +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +""" +Tests for: + * leap/common/http.py +""" +import os +try: + import unittest2 as unittest +except ImportError: + import unittest + +from leap.common import http +from leap.common.testing.basetest import BaseLeapTest + +TEST_CERT_PEM = os.path.join( + os.path.split(__file__)[0], + '..', 'testing', "leaptest_combined_keycert.pem") + + +class HTTPClientTest(BaseLeapTest): + + def setUp(self): + pass + + def tearDown(self): + pass + + def test_agents_sharing_pool_by_default(self): + client = http.HTTPClient() + client2 = http.HTTPClient(TEST_CERT_PEM) + self.assertNotEquals(client._agent, client2._agent, "Expected dedicated agents") + self.assertEquals(client._agent._pool, client2._agent._pool, "Pool was not reused by default") + + def test_agent_can_have_dedicated_custom_pool(self): + custom_pool = http.createPool(maxPersistentPerHost=42, persistent=False) + self.assertEquals(custom_pool.maxPersistentPerHost, 42, + "Custom persistent connections limit is not being respected") + self.assertFalse(custom_pool.persistent, + "Custom persistence is not being respected") + default_client = http.HTTPClient() + custom_client = http.HTTPClient(pool=custom_pool) + + self.assertNotEquals(default_client._agent, custom_client._agent, "No agent reuse is expected") + self.assertEquals(custom_pool, custom_client._agent._pool, "Custom pool usage was not respected") + +if __name__ == "__main__": + unittest.main() -- cgit v1.2.3