Merge ~jslarraz/qa-regression-testing:urllib3_tests into qa-regression-testing:master

[Jorge Sancho Larraz]

    This commit expands the existent python-urllib3 test suite with tests for CVE-2020-26137, CVE-2018-25091, CVE-2023-43804, CVE-2023-45803, and some additional tests designed to detect potential regressions related to the fixes for those CVEs. Those tests rely on dummyserver, which is obtained in run-time via `apt source python-urllib3` from -release pocket and patched (patchs in $QRT/scripts/python-urllib3) to ensure that requirements introduced in newer versions of the package by CVE patches are meet. Those test have been included in .launchpad and it has been tested that they run smoothly in a local lpci installation based on snap.

    It has also been included one test "test_run_package_tests" that runs urllib3 package tests (obtained via `apt source python-urllib3`) and compared the test result (# of tests failed, passed, skipped and deselected) with the expected outcome for each release. This test has been included more as a way to detect potential regressions between versions (caused by a patch) than as a representative overview of the package status. This test is not run on lpci as it requires connection to pip to install dependencies and it is more prone to cause false positives in the future.

    This commit also include some common functionality in testlib.py required by test-python-urllib3.py to install latest pip version (specially helpful in older releases as the pip version bundled in python3-pip does not work properly) and manage python packages (install and uninstall) via pip. All this functionality is provided with with a common interface across releases, which is really handy to hide per-release specialties from the tests.

[Alex Murray (alexmurray)]

LGTM but could you please add a docstring comment for each of the new functions added to testlib.py to explain what they do (particularly `get_pip()` and `pip_install_r()` - and I wonder if perhaps `pip_install_r()` should be renamed `pip_install_from_requirements()` to make it clearer what this function does just from its name?

[Jorge Sancho Larraz (jslarraz)]

Thanks for the review Alex!

-----

Docstring comments added.
pip_install_r function rename to from_requirements as suggested.
test-python-urllib3 calls to pip_install_r updated accordingly.

[Alex Murray (alexmurray)]

LGTM! Thanks.

review: Approve

diff --git a/.launchpad.yaml b/.launchpad.yaml
index 242b9b8..9e75672 100644
--- a/.launchpad.yaml
+++ b/.launchpad.yaml
@@ -11,6 +11,7 @@ pipeline:
     - busybox
     - coreutils
     - util-linux
+    - python-urllib3
 
 jobs:
   imagemagick:
@@ -155,3 +156,16 @@ jobs:
       - sudo
     run: |
       ./lpcraft-runner util-linux
+
+  python-urllib3:
+    matrix:
+      - series: jammy
+        architectures: amd64
+      - series: focal
+        architectures: amd64
+      - series: bionic
+        architectures: amd64
+    packages:
+      - sudo
+    run: |
+      ./lpcraft-runner python-urllib3
diff --git a/scripts/python-urllib3/CVE-2023-45803.patch b/scripts/python-urllib3/CVE-2023-45803.patch
new file mode 100644
index 0000000..b66a445
--- /dev/null
+++ b/scripts/python-urllib3/CVE-2023-45803.patch
@@ -0,0 +1,23 @@
+diff --git a/dummyserver/handlers.py b/dummyserver/handlers.py
+index c90c2fce20..acd181d2db 100644
+--- a/dummyserver/handlers.py
++++ b/dummyserver/handlers.py
+@@ -186,6 +186,8 @@ def redirect(self, request):
+         status = request.params.get("status", "303 See Other")
+         if len(status) == 3:
+             status = "%s Redirect" % status.decode("latin-1")
++        elif isinstance(status, bytes):
++            status = status.decode("latin-1")
+ 
+         headers = [("Location", target)]
+         return Response(status=status, headers=headers)
+@@ -264,4 +266,9 @@ def encodingrequest(self, request):
+     def headers(self, request):
+         return Response(json.dumps(dict(request.headers)))
+ 
++    def headers_and_params(self, request):
++        return Response(
++            json.dumps({"headers": dict(request.headers), "params": request.params})
++        )
++
+     def successful_retry(self, request):
diff --git a/scripts/python-urllib3/fix_HTTPHeaderDict_not_json_serializable.patch b/scripts/python-urllib3/fix_HTTPHeaderDict_not_json_serializable.patch
new file mode 100644
index 0000000..fd5d20d
--- /dev/null
+++ b/scripts/python-urllib3/fix_HTTPHeaderDict_not_json_serializable.patch
@@ -0,0 +1,13 @@
+diff --git a/dummyserver/handlers.py b/dummyserver/handlers.py
+index ddf5c49f..83c28a68 100644
+--- a/dummyserver/handlers.py
++++ b/dummyserver/handlers.py
+@@ -227,7 +227,7 @@ class TestingApp(RequestHandler):
+         return Response(data, headers=headers)
+ 
+     def headers(self, request):
+-        return Response(json.dumps(request.headers))
++        return Response(json.dumps(dict(request.headers)))
+ 
+     def successful_retry(self, request):
+         """ Handler which will return an error and then success
diff --git a/scripts/test-python-urllib3.py b/scripts/test-python-urllib3.py
index 6a4d779..c664c0c 100755
--- a/scripts/test-python-urllib3.py
+++ b/scripts/test-python-urllib3.py
@@ -24,9 +24,9 @@
 
 '''
 
-# QRT-Depends: testlib_httpd.py testlib_ssl.py ssl
-# QRT-Packages: python3-urllib3 ssl-cert openssl lsb-release
-# QRT-Alternates: apache2:!precise apache2-mpm-prefork:precise
+# QRT-Depends: testlib_httpd.py testlib_ssl.py ssl python-urllib3
+# QRT-Packages: python3-urllib3 ssl-cert openssl lsb-release apache2 git python3-tornado python3-pytest
+# QRT-Alternates: python3-trustme:!xenial python3-trustme:!bionic
 # QRT-Privilege: root
 
 import unittest, subprocess
@@ -38,6 +38,8 @@ import testlib_ssl
 import tempfile
 import urllib3
 import sys
+import json
+import tarfile
 
 class BasicTest(testlib_httpd.HttpdCommon):
     '''Test basic functionality'''
@@ -328,11 +330,288 @@ SSLCACertificateFile /etc/ssl/certs/ca.crt
                        cert_file = clientcert_pem,
                        key_file = clientkey_pem)
 
+class PackageTest(testlib.TestlibCase):
+
+    @classmethod
+    def setUpClass(cls):
+        # Check that patches exists
+        if not os.path.isdir('python-urllib3'):
+            print("python-urllib3 folder was not copied to target. Ensure it is added to QRT-Depends"); exit(-1)
+
+        # Download sources
+        testlib._ensure_deb_src_entries()
+        testlib._run_apt_command(["python-urllib3"], "source")
+
+        # Unzip sources
+        for file in os.scandir('.'):
+            if file.name.startswith("python-urllib3") and file.name.endswith("orig.tar.gz"):
+                source_package = tarfile.open(file.name)
+                source_package.extractall('.')
+                source_package.close()
+
+        # Change directory
+        for folder in os.scandir('.'):
+            if folder.name.startswith("urllib3"):
+                os.chdir(folder.name)
+
+        # Remove urllib3 sources to ensure we are not using them accidentally
+        for file in os.scandir('.'):
+            if not file.name in ["test", "dummyserver", "dev-requirements.txt", "test-requirements.txt"]:
+                if file.is_file():
+                    os.remove(file.name)
+                elif file.is_dir():
+                    shutil.rmtree(file.name)
+
+        # Six is no longer embedded in urllib3 version packaged by debian. We need to use system wide six module instead.
+        # This lines substitude patch 01_do-not-use-embedded-python-six.patch but are version independent.
+        os.system("grep -r -l 'from urllib3.packages.six' * | xargs sed -i 's/from urllib3.packages.six/from six/g'")
+        os.system("grep -r -l 'from urllib3.packages import six' * | xargs sed -i 's/from urllib3.packages import six/import six/g'")
+
+        # Apply patches
+        patches = []
+        if testlib.TestlibManager().lsb_release['Release'] <= 18.04:
+            patches += ["fix_HTTPHeaderDict_not_json_serializable.patch"]
+            os.system("sed -i s/\\\"/\\\'/g ../python-urllib3/CVE-2023-45803.patch")
+        patches += ["CVE-2023-45803.patch"]
+
+        for patch in patches:
+            rc, report = testlib.cmd(["git", "apply", "--reject", '../python-urllib3/' + patch.strip()])
+            if not rc != 0:
+                print(report)
+
+        # Add current directory to path so that dummyserver can be imported and start a dummyserver instance
+        sys.path += [os.getcwd()]
+
+        from tornado import ioloop, web
+        from dummyserver.handlers import TestingApp
+        from dummyserver.server import (
+            DEFAULT_CERTS,
+            run_loop_in_thread,
+            run_tornado_app,
+        )
+
+        # Start test server
+        cls.io_loop = ioloop.IOLoop.current()
+        app = web.Application([(r".*", TestingApp)])
+        cls.server, cls.port = run_tornado_app(
+            app, cls.io_loop, DEFAULT_CERTS, "http", "localhost"
+        )
+        cls.server_thread = run_loop_in_thread(cls.io_loop)
+
+        cls.base_url = "http://%s:%d" % ("localhost", cls.port)
+        cls.base_url_alt = "http://%s:%d" % ("127.0.0.1", cls.port)
+
+    @classmethod
+    def tearDownClass(cls):
+        # Stop test server
+        cls.io_loop.add_callback(cls.server.stop)
+        cls.io_loop.add_callback(cls.io_loop.stop)
+        cls.server_thread.join()
+
+    def test_invalid_method_not_allowed(self):
+        ''' CVE-2020-26137 '''
+        for char in [" ", "\r", "\n", "\x00"]:
+            exception_caught = False
+            with urllib3.HTTPConnectionPool("localhost", self.port) as pool:
+                try:
+                    pool.request("GET" + char, "/")
+                except ValueError:
+                    exception_caught = True
+            self.assertTrue(exception_caught, "Invalid method with character " + char + ". It may be caused by CVE-2020-26137")
+
+    def test_retry_default_remove_headers_on_redirect(self):
+        ''' CVE-2018-25091 or CVE-2023-43804 '''
+        retry = urllib3.util.retry.Retry()
+
+        self.assertEqual(retry.remove_headers_on_redirect, {"authorization", "cookie"}, "Wrong default value of remove_headers_on_redirect. It is probably caused by CVE-2018-25091 or CVE-2023-43804")
+
+    def test_retry_set_remove_headers_on_redirect(self):
+        ''' Functional test related to potential CVE-2018-25091 and CVE-2023-43804 regressions. It is not expected to fail even if the package is vulnerable. '''
+        retry = urllib3.util.retry.Retry(remove_headers_on_redirect=["X-API-Secret"])
+
+        self.assertEqual(retry.remove_headers_on_redirect, {"x-api-secret"}, "urllib3 didn't honored the value of the remove_headers_on_redirect passed to the retry function. This is probably caused by a regression.")
+
+    def test_redirect_cross_host_remove_headers(self):
+        ''' CVE-2018-25091 or CVE-2023-43804 '''
+        with urllib3.poolmanager.PoolManager() as http:
+            r = http.request(
+                "GET",
+                "%s/redirect" % self.base_url,
+                fields={"target": "%s/headers" % self.base_url_alt},
+                headers={"Authorization": "foo", "Cookie": "foo=bar"},
+            )
+
+            self.assertEqual(r.status, 200, "Something went wrong with dummyserver, it returned wrong status code. It is probably a problem in the test and not in the library")
+
+            data = json.loads(r.data.decode("utf-8"))
+
+            self.assertNotIn("Authorization", data, "Authorization header was not properly removed when redirected to a different origin. It is probably caused by CVE-2018-25091")
+            self.assertNotIn("Cookie", data, "Cookie header was not properly removed when redirected to a different origin. It is probably caused by CVE-2023-43804")
+
+            r = http.request(
+                "GET",
+                "%s/redirect" % self.base_url,
+                fields={"target": "%s/headers" % self.base_url_alt},
+                headers={"authorization": "foo", "cookie": "foo=bar"},
+            )
+
+            self.assertEqual(r.status, 200, "Something went wrong with dummyserver, it returned wrong status code. It is probably a problem in the test and not in the library")
+
+            data = json.loads(r.data.decode("utf-8"))
+
+            self.assertNotIn("authorization", data, "authorization header was not properly removed when redirected to a different origin. It is probably caused by CVE-2018-20060.")
+            self.assertNotIn("Authorization", data, "Authorization header was not properly removed when redirected to a different origin. It is probably caused by CVE-2018-25091")
+            self.assertNotIn("cookie", data, "cookie header was not properly removed when redirected to a different origin. It is probably caused by CVE-2023-43804")
+            self.assertNotIn("Cookie", data, "Cookie header was not properly removed when redirected to a different origin. It is probably caused by CVE-2023-43804")
+
+    def test_redirect_cross_host_no_remove_headers(self):
+        ''' Functional test related to potential CVE-2018-25091 and CVE-2023-43804 regressions. It is not expected to fail even if the package is vulnerable. '''
+        with urllib3.poolmanager.PoolManager() as http:
+            r = http.request(
+                "GET",
+                "%s/redirect" % self.base_url,
+                fields={"target": "%s/headers" % self.base_url_alt},
+                headers={"Authorization": "foo", "Cookie": "foo=bar"},
+                retries=urllib3.util.retry.Retry(remove_headers_on_redirect=[]),
+            )
+
+            self.assertEqual(r.status, 200, "Something went wrong with dummyserver, it returned wrong status code. It is probably a problem in the test and not in the library")
+
+            data = json.loads(r.data.decode("utf-8"))
+
+            self.assertEqual(data["Authorization"], "foo", "The Authorization header value changed or was removed when explicitly stating the contraty. This is probably caused by a regression")
+            self.assertEqual(data["Cookie"], "foo=bar", "The Cookie header value changed or was removed when explicitly stating the contraty. This is probably caused by a regression")
+
+    def test_redirect_cross_host_set_removed_headers(self):
+        ''' Functional test related to potential CVE-2018-25091 and CVE-2023-43804 regressions. It is not expected to fail even if the package is vulnerable. '''
+        with urllib3.poolmanager.PoolManager() as http:
+            r = http.request(
+                "GET",
+                "%s/redirect" % self.base_url,
+                fields={"target": "%s/headers" % self.base_url_alt},
+                headers={
+                    "X-API-Secret": "foo",
+                    "Authorization": "bar",
+                    "Cookie": "foo=bar",
+                },
+                retries=urllib3.util.retry.Retry(remove_headers_on_redirect=["X-API-Secret"]),
+            )
+
+            self.assertEqual(r.status, 200, "Something went wrong with dummyserver, it returned wrong status code. It is probably a problem in the test and not in the library")
+
+            data = json.loads(r.data.decode("utf-8"))
+
+            self.assertNotIn("X-API-Secret", data, "X-API-Secret header was not properly removed when explicitly requested. It is probably caused by a regression.")
+            self.assertEqual(data["Authorization"], "bar", "The Authorization header value changed or was removed when explicitly stating the contraty. This is probably caused by a regression")
+            self.assertEqual(data["Cookie"], "foo=bar", "The Cookie header value changed or was removed when explicitly stating the contraty. This is probably caused by a regression")
+
+            r = http.request(
+                "GET",
+                "%s/redirect" % self.base_url,
+                fields={"target": "%s/headers" % self.base_url_alt},
+                headers={
+                    "x-api-secret": "foo",
+                    "authorization": "bar",
+                    "cookie": "foo=bar",
+                },
+                retries=urllib3.util.retry.Retry(remove_headers_on_redirect=["X-API-Secret"]),
+            )
+
+            self.assertEqual(r.status, 200, "Something went wrong with dummyserver, it returned wrong status code. It is probably a problem in the test and not in the library")
+
+            data = json.loads(r.data.decode("utf-8"))
+
+            self.assertNotIn("x-api-secret", data, "x-api-secret header was not properly removed when explicitly requested. It is probably caused by a regression.")
+            self.assertNotIn("X-API-Secret", data, "X-API-Secret header was not properly removed when explicitly requested. It is probably caused by a regression.")
+            self.assertEqual(data["Authorization"], "bar", "The Authorization header value changed or was removed when explicitly stating the contraty. This is probably caused by a regression")
+            self.assertEqual(data["Cookie"], "foo=bar", "The Cookie header value changed or was removed when explicitly stating the contraty. This is probably caused by a regression")
+
+    def test_303_redirect_makes_request_lose_body(self):
+        ''' CVE-2023-45803 '''
+        with urllib3.poolmanager.PoolManager() as http:
+            response = http.request(
+                "POST",
+                "%s/redirect" % self.base_url,
+                fields={
+                    "target": "%s/headers_and_params" % self.base_url,
+                    "status": "303 See Other",
+                },
+            )
+        data = json.loads(response.data.decode("utf-8"))
+        self.assertEqual(data["params"], {}, "Request body was not properly removed when 303 redirected to a different origin. It is probably caused by CVE-2023-45803")
+        self.assertNotIn("Content-Type", urllib3._collections.HTTPHeaderDict(data["headers"]), "Request body was not properly removed when 303 redirected to a different origin. It is probably caused by CVE-2023-45803")
+
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'External URLs not allowed in lpci environ'
+    )
+    def test_run_package_tests(self):
+        ''' Run urllib3 package test for the version included in the release and search for the expected results. '''
+
+        # Install dependencies
+        testlib.get_pip()
+
+        release = testlib.TestlibManager().lsb_release['Release']
+        if release == 18.04:
+            # Required to build psutils, which is only required in bionic
+            testlib.install_packages(['build-essential', 'python3-dev'])
+
+        if release == 14.04:
+            testlib.pip_install_from_requirements('test-requirements.txt')
+        else:
+            testlib.pip_install_from_requirements('dev-requirements.txt')
+
+        # Remove pip, it is needed to test the urllib3 module bundled inside pip
+        testlib.pip_uninstall('pip')
+
+        # Fix broken 'six' dependencies caused by patch 01_do-not-use-embedded-python-six.patch. ssltransport seems to only be used by old tests.
+        os.system("sudo sed -i 's/from ..packages import six/import six/g' /usr/lib/python3/dist-packages/urllib3/util/ssltransport.py")
+        os.system("sudo sed -i 's/from urllib3.packages import six/import six/g' /usr/lib/python3/dist-packages/urllib3/util/ssltransport.py")
+
+
+        # Run package test
+        command = [sys.executable, "-m", "pytest", "test", "-k", "not test_retry_default_remove_headers_on_redirect and "
+                                                             "not test_retry_set_remove_headers_on_redirect and "
+                                                             "not test_redirect_cross_host_remove_headers and "
+                                                             "not test_redirect_cross_host_no_remove_headers and "
+                                                             "not test_redirect_cross_host_no_remove_headers and "
+                                                             "not test_redirect_cross_host_set_removed_headers and "
+                                                             "not test_303_redirect_makes_request_lose_body",
+                                                             "--ignore=test/test_retry_deprecated.py"]
+        if release == 20.04: # There is an error in the version used in focal. Test using dummyserver don't work properly
+            command += ['--ignore=test/with_dummyserver']
+        rc, report = testlib.cmd(command)
+
+        # Check expected results
+        if release == 16.04:
+            self.assertIn("29 failed, 264 passed, 7 skipped",
+                          report, "Packet test execution result different than expected: " + report)
+        elif release == 18.04:
+            self.assertIn("35 failed, 763 passed, 52 skipped",
+                          report, "Packet test execution result different than expected: " + report)
+        elif release == 20.04:
+            self.assertIn("3 failed, 585 passed, 378 skipped, 2 deselected",
+                          report, "Packet test execution result different than expected: " + report)
+        elif release == 22.04:
+            self.assertIn("4 failed, 1226 passed, 588 skipped, 5 deselected",
+                          report, "Packet test execution result different than expected: " + report)
+        elif release == 23.04:
+            self.assertIn("3 failed, 1276 passed, 611 skipped, 5 deselected",
+                          report, "Packet test execution result different than expected: " + report)
+        elif release == 23.10:
+            self.assertIn("3 failed, 1289 passed, 611 skipped, 5 deselected",
+                          report, "Packet test execution result different than expected: " + report)
+        else:
+            print("Expected outcome is not defined for release: " + str(release))
 
 if __name__ == '__main__':
     testlib.require_root()
     suite = unittest.TestSuite()
     suite.addTest(unittest.TestLoader().loadTestsFromTestCase(BasicTest))
+    # Fixes for CVE-2018-25091, CVE-2023-43804 and CVE-2023-45803 are only available in esm for xenial and bionic,
+    # skip those tests when run in LPCI
+    if not ('LPCRAFT_QRT_RUNNER' in os.environ) or (testlib.TestlibManager().lsb_release['Release'] >= 20.04):
+        suite.addTest(unittest.TestLoader().loadTestsFromTestCase(PackageTest))
 
     rc = unittest.TextTestRunner(verbosity=2).run(suite)
     if not rc.wasSuccessful():
diff --git a/scripts/testlib.py b/scripts/testlib.py
index 8ed2353..9191a4a 100644
--- a/scripts/testlib.py
+++ b/scripts/testlib.py
@@ -915,7 +915,79 @@ def _run_snap_remove(snapname):
     rc, report = cmd(command)
     return rc, report
 
+def get_pip():
+    '''Install the latest compatible version of pip via the get-pip.py scirpt.'''
+    tempdir = tempfile.mkdtemp(dir='/tmp', prefix="testlib-")
+
+    # Download boostrap script, get-pip.py
+    if TestlibManager().lsb_release['Release'] == 14.04:
+        get_pip_url = 'https://bootstrap.pypa.io/pip/3.4/get-pip.py'
+    elif TestlibManager().lsb_release['Release'] == 16.04:
+        get_pip_url = 'https://bootstrap.pypa.io/pip/3.5/get-pip.py'
+    elif TestlibManager().lsb_release['Release'] == 18.04:
+        install_package('python3-distutils')
+        get_pip_url = 'https://bootstrap.pypa.io/pip/3.6/get-pip.py'
+    else:
+        install_package('python3-distutils')
+        get_pip_url = 'https://bootstrap.pypa.io/pip/get-pip.py'
+    command = ['wget', get_pip_url, '-O', tempdir+'/get-pip.py']
+    rc, report = cmd(command)
+    if rc != 0:
+        return rc, report
+
+    # Run the script to install the latest supported version of pip
+    command = [sys.executable, tempdir+'/get-pip.py']
+    if TestlibManager().lsb_release['Release'] == 14.04:
+        command += ['--user']
+        sys.path = [os.path.expanduser("~/.local/bin")] + sys.path
+    if TestlibManager().lsb_release['Release'] >= 23.04:
+        command += ['--break-system-packages']
+    rc, report = cmd(command)
+
+    shutil.rmtree(tempdir)
+    return rc, report
+
+def pip_install(pkg):
+    '''Installs a python package or a list of packages using pip'''
+    command = [sys.executable, '-m', 'pip', 'install']
+    if isinstance(pkg, str):
+        command += [pkg]
+    elif isinstance(pkg, list):
+        command += pkg
+
+    if TestlibManager().lsb_release['Release'] == 14.04:
+        command += ['--user']
+        sys.path = [os.path.expanduser("~/.local/bin")] + sys.path
+    if TestlibManager().lsb_release['Release'] >= 23.04:
+        command += ['--break-system-packages']
+    rc, report = cmd(command)
+    return rc, report
 
+def pip_uninstall(pkg):
+    '''Uninstalls a python package or a list of packages using pip'''
+    command = [sys.executable, '-m', 'pip', 'uninstall', '--yes']
+    if isinstance(pkg, str):
+        command += [pkg]
+    elif isinstance(pkg, list):
+        command += pkg
+
+    if TestlibManager().lsb_release['Release'] == 14.04:
+        command += ['--user']
+    if TestlibManager().lsb_release['Release'] >= 23.04:
+        command += ['--break-system-packages']
+    rc, report = cmd(command)
+    return rc, report
+
+def pip_install_from_requirements(requirements):
+    '''Install a set of python packages from a requirements file using pip'''
+    command = [sys.executable, '-m', 'pip', 'install', '-r', requirements]
+    if TestlibManager().lsb_release['Release'] == 14.04:
+        command += ['--user']
+        sys.path = [os.path.expanduser("~/.local/bin")] + sys.path
+    if TestlibManager().lsb_release['Release'] >= 23.04:
+        command += ['--break-system-packages']
+    rc, report = cmd(command)
+    return rc, report
 
 class TestDaemon:
     '''Helper class to manage daemons consistently'''