launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:https-mirror-tls-version into launchpad:master.

Commit message:
Changing TLS version used on HTTPS mirrors (using 1.2 instead of 1.1), which should be compatible with most of HTTPS servers.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/380728

TLS version 1.1 is quite old, and not supported by some servers. We should be using 1.2. Ideally, we might want to implement some negotiation mechanism in the future, to detect which TLS version should be used on each HTTPS server.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:https-mirror-tls-version into launchpad:master.

diff --git a/lib/lp/registry/scripts/distributionmirror_prober.py b/lib/lp/registry/scripts/distributionmirror_prober.py
index fd40f8d..5b5d424 100644
--- a/lib/lp/registry/scripts/distributionmirror_prober.py
+++ b/lib/lp/registry/scripts/distributionmirror_prober.py
@@ -15,7 +15,7 @@ from StringIO import StringIO
 import OpenSSL
 from OpenSSL.SSL import (
     Context,
-    TLSv1_1_METHOD,
+    TLSv1_2_METHOD,
     )
 import requests
 from six.moves import http_client
@@ -350,7 +350,16 @@ class ProberFactory(protocol.ClientFactory):
                 reactor=reactor, contextFactory=self.https_agent_policy())
         else:
             contextFactory = self.https_agent_policy()
-            contextFactory.getContext = lambda: Context(TLSv1_1_METHOD)
+            # XXX: pappacena 2020-03-16
+            # TLS version 1.2 should work for most servers. But if it
+            # doesn't, we should implement a negotiation mechanism to test
+            # which version should be used before doing the actual probing
+            # request.
+            # One way to debug which version a given server is compatible
+            # with using curl is issuing the following command:
+            # curl -v --head https://<server-host> --tlsv1.2 --tls-max 1.2
+            # (changing 1.2 with other version numbers)
+            contextFactory.getContext = lambda: Context(TLSv1_2_METHOD)
             agent = TunnelingAgent(
                 reactor, (self.connect_host, self.connect_port, None),
                 contextFactory=contextFactory)
@@ -917,14 +926,17 @@ def should_skip_host(host):
         return ratio < MIN_REQUEST_TIMEOUT_RATIO
 
 
-def _parse(url, defaultPort=80):
+def _parse(url, defaultPort=None):
     """Parse the given URL returning the scheme, host, port and path."""
     scheme, host, path, dummy, dummy, dummy = urlparse(url)
-    port = defaultPort
     if ':' in host:
         host, port = host.split(':')
         assert port.isdigit()
         port = int(port)
+    elif defaultPort is None:
+        port = 443 if scheme == 'https' else 80
+    else:
+        port = defaultPort
     return scheme, host, port, path
 
 
diff --git a/lib/lp/registry/tests/test_distributionmirror_prober.py b/lib/lp/registry/tests/test_distributionmirror_prober.py
index 46a8cae..cb38388 100644
--- a/lib/lp/registry/tests/test_distributionmirror_prober.py
+++ b/lib/lp/registry/tests/test_distributionmirror_prober.py
@@ -43,6 +43,7 @@ from lp.registry.model.distributionmirror import DistributionMirror
 from lp.registry.scripts import distributionmirror_prober
 from lp.registry.scripts.distributionmirror_prober import (
     _get_cdimage_file_list,
+    _parse,
     ArchiveMirrorProberCallbacks,
     BadResponseCode,
     ConnectionSkipped,
@@ -110,7 +111,6 @@ class HTTPServerTestSetup(TacTestSetup):
         return os.path.join(self.root, 'distributionmirror_http_server.log')
 
 
-
 class LocalhostWhitelistedHTTPSPolicy(BrowserLikePolicyForHTTPS):
     """HTTPS policy that bypasses SSL certificate check when doing requests
     to localhost.
@@ -125,6 +125,25 @@ class LocalhostWhitelistedHTTPSPolicy(BrowserLikePolicyForHTTPS):
             hostname, port)
 
 
+class TestURLParser(TestCase):
+    def test_defined_port(self):
+        url = 'http://foo.com:37/bar'
+        self.assertEqual(('http', 'foo.com', 37, '/bar'), _parse(url))
+
+    def test_default_port_http(self):
+        url = 'http://foo.com/bar'
+        self.assertEqual(('http', 'foo.com', 80, '/bar'), _parse(url))
+
+    def test_default_port_https(self):
+        url = 'https://foo.com/bar'
+        self.assertEqual(('https', 'foo.com', 443, '/bar'), _parse(url))
+
+    def test_given_default_port(self):
+        url = 'https://foo.com/bar'
+        self.assertEqual(
+            ('https', 'foo.com', 99, '/bar'), _parse(url, defaultPort=99))
+
+
 class TestProberHTTPSProtocolAndFactory(TestCase):
     layer = TwistedLayer
     run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
@@ -153,6 +172,13 @@ class TestProberHTTPSProtocolAndFactory(TestCase):
         RedirectAwareProberFactory.https_agent_policy = (
             LocalhostWhitelistedHTTPSPolicy)
 
+        def restore_policy():
+            ProberFactory.https_agent_policy = original_probefactory_policy
+            RedirectAwareProberFactory.https_agent_policy = (
+                original_redirect_policy)
+
+        self.addCleanup(restore_policy)
+
         for factory in (ProberFactory, RedirectAwareProberFactory):
             self.useFixture(MockPatchObject(
                 factory, "https_agent_policy",
@@ -219,7 +245,7 @@ class TestProberHTTPSProtocolAndFactory(TestCase):
     def test_https_prober_uses_proxy(self):
         proxy_port = 6654
         self.pushConfig(
-            'launchpad', http_proxy='http://localhost:%s'% proxy_port)
+            'launchpad', http_proxy='http://localhost:%s' % proxy_port)
 
         url = 'https://localhost:%s/valid-mirror/file' % self.port
         prober = RedirectAwareProberFactory(url, timeout=0.5)