launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/system-ca-certificates into lp:launchpad.

Commit message:
Add configurable CA certificates bundle path for urlfetch.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/system-ca-certificates/+merge/368993

Comparing https://launchpad.net/ubuntu/+source/ca-certificates with https://pypi.org/project/certifi/, I'm not actually convinced that the system package is a better way for Launchpad to get an up-to-date CA certificate package, particularly now that we can easily update certifi independently of requests; so in my opinion we probably shouldn't use this on production at the moment.  Still, it's useful to have the option.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/system-ca-certificates into lp:launchpad.

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2019-05-28 13:55:14 +0000
+++ configs/development/launchpad-lazr.conf	2019-06-18 17:35:58 +0000
@@ -98,6 +98,7 @@
 enable_test_openid_provider: True
 openid_canonical_root: https://testopenid.test/
 openid_provider_root: https://testopenid.test/
+ca_certificates_path: /etc/ssl/certs/ca-certificates.crt
 code_domain: code.launchpad.test
 default_batch_size: 5
 max_attachment_size: 2097152

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2019-05-28 13:55:14 +0000
+++ lib/lp/services/config/schema-lazr.conf	2019-06-18 17:35:58 +0000
@@ -940,6 +940,12 @@
 # datatype: string
 http_proxy: none
 
+# Path to CA certificate bundle, or "none" to use the one bundled with the
+# certifi Python package.
+#
+# datatype: string
+ca_certificates_path: none
+
 # Session cookies being sent to a subdomain of the parent
 # domains listed here will be sent to the parent domain,
 # allowing sessions to be shared between vhosts.

=== modified file 'lib/lp/services/tests/test_timeout.py'
--- lib/lp/services/tests/test_timeout.py	2019-06-07 21:12:13 +0000
+++ lib/lp/services/tests/test_timeout.py	2019-06-18 17:35:58 +0000
@@ -388,6 +388,24 @@
             {scheme: proxy for scheme in ('http', 'https')},
             fake_send.calls[0][1]['proxies'])
 
+    def test_urlfetch_no_ca_certificates(self):
+        """If ca_certificates_path is None, urlfetch uses bundled certs."""
+        self.pushConfig('launchpad', ca_certificates_path='none')
+        fake_send = FakeMethod(result=Response())
+        self.useFixture(
+            MonkeyPatch('requests.adapters.HTTPAdapter.send', fake_send))
+        urlfetch('http://example.com/')
+        self.assertIs(True, fake_send.calls[0][1]['verify'])
+
+    def test_urlfetch_ca_certificates_if_configured(self):
+        """urlfetch uses the configured ca_certificates_path if it is set."""
+        self.pushConfig('launchpad', ca_certificates_path='/path/to/certs')
+        fake_send = FakeMethod(result=Response())
+        self.useFixture(
+            MonkeyPatch('requests.adapters.HTTPAdapter.send', fake_send))
+        urlfetch('http://example.com/')
+        self.assertEqual('/path/to/certs', fake_send.calls[0][1]['verify'])
+
     def test_urlfetch_does_not_support_ftp_urls_by_default(self):
         """urlfetch() does not support ftp urls by default."""
         url = 'ftp://localhost/'

=== modified file 'lib/lp/services/timeout.py'
--- lib/lp/services/timeout.py	2019-06-14 14:26:30 +0000
+++ lib/lp/services/timeout.py	2019-06-18 17:35:58 +0000
@@ -378,6 +378,9 @@
                 request_kwargs["proxies"]["ftp"] = config.launchpad.http_proxy
         if output_file is not None:
             request_kwargs["stream"] = True
+        if config.launchpad.ca_certificates_path is not None:
+            request_kwargs.setdefault(
+                "verify", config.launchpad.ca_certificates_path)
         response = self.session.request(url=url, **request_kwargs)
         raise_for_status_redacted(response)
         if output_file is None: