Merge lp:~stub/charm-helpers/fix-configure_sources into lp:charm-helpers

[Stuart Bishop]

fetch.add_source() is blindly retrieving a key by GPG key id from a remote keyserver, which is in most cases insecure.

This branch addresses this security problem by allowing add_source() to accept a full GPG key, rather than just a GPG key id. This is Bug #1341527.

I've also fixed Bug #1269718 while I'm here. I elected to raise an exception rather than log an error and continue, as other parts of the code where already raising exceptions when invalid sources are requested.

[Chris Glass (tribaal)]

That is a great addition to the mechanism IMO.

Could you please add a short docstring to the function explaining what the key parameter should be like? Example:

"""This function adds an entry to the sources.list file.

@param source: The archive's URL. Example: "http://archive.ubuntu.com"
@param key: The GPG key to verify the archive packages with. Can be either a
    key ID or a full public key block. If only the key ID is specified the
    key will be fetched from the network via HKP."""

[David Britton (dpb)]

+1, tested and works great. Thanks for adding this.

Agreed with @tribaal on the docstring. Now that the functionality is expanding, I think it sort of needs the docstring, and it's a good chance to add it.

Marking as approved since this is a minor non-functional change suggested.

review: Approve

[lp:~stub/charm-helpers/fix-configure_sources]

165. By Stuart Bishop on 2014-08-21

Add docstrings

=== modified file 'charmhelpers/fetch/__init__.py'
--- charmhelpers/fetch/__init__.py	2014-06-27 08:11:50 +0000
+++ charmhelpers/fetch/__init__.py	2014-07-14 14:54:10 +0000
@@ -1,4 +1,5 @@
 import importlib
+from tempfile import NamedTemporaryFile
 import time
 from yaml import safe_load
 from charmhelpers.core.host import (
@@ -225,10 +226,23 @@
         release = lsb_release()['DISTRIB_CODENAME']
         with open('/etc/apt/sources.list.d/proposed.list', 'w') as apt:
             apt.write(PROPOSED_POCKET.format(release))
+    else:
+        raise SourceConfigError("Unknown source: {!r}".format(source))
+
     if key:
-        subprocess.check_call(['apt-key', 'adv', '--keyserver',
-                               'hkp://keyserver.ubuntu.com:80', '--recv',
-                               key])
+        if '-----BEGIN PGP PUBLIC KEY BLOCK-----' in key:
+            with NamedTemporaryFile() as key_file:
+                key_file.write(key)
+                key_file.flush()
+                key_file.seek(0)
+                subprocess.check_call(['apt-key', 'add', '-'], stdin=key_file)
+        else:
+            # Note that hkp: is in no way a secure protocol. Using a
+            # GPG key id is pointless from a security POV unless you
+            # absolutely trust your network and DNS.
+            subprocess.check_call(['apt-key', 'adv', '--keyserver',
+                                   'hkp://keyserver.ubuntu.com:80', '--recv',
+                                   key])
 
 
 def configure_sources(update=False,

=== modified file 'tests/fetch/test_fetch.py'
--- tests/fetch/test_fetch.py	2014-06-05 10:47:46 +0000
+++ tests/fetch/test_fetch.py	2014-07-14 14:54:10 +0000
@@ -1,3 +1,4 @@
+from cStringIO import StringIO
 import subprocess
 
 from tests.helpers import patch_open
@@ -177,26 +178,54 @@
             mock_file.write.assert_called_with(result)
 
     @patch('subprocess.check_call')
+    def test_add_source_http_and_key_id(self, check_call):
+        source = "http://archive.ubuntu.com/ubuntu raring-backports main"
+        key_id = "akey"
+        fetch.add_source(source=source, key=key_id)
+        check_call.assert_has_calls([
+            call(['add-apt-repository', '--yes', source]),
+            call(['apt-key', 'adv', '--keyserver',
+                  'hkp://keyserver.ubuntu.com:80', '--recv', key_id])
+        ])
+
+    @patch('subprocess.check_call')
+    def test_add_source_https_and_key_id(self, check_call):
+        source = "https://USER:[e-mail]/project/awesome"
+        key_id = "GPGPGP"
+        fetch.add_source(source=source, key=key_id)
+        check_call.assert_has_calls([
+            call(['add-apt-repository', '--yes', source]),
+            call(['apt-key', 'adv', '--keyserver',
+                  'hkp://keyserver.ubuntu.com:80', '--recv', key_id])
+        ])
+
+    @patch('subprocess.check_call')
     def test_add_source_http_and_key(self, check_call):
         source = "http://archive.ubuntu.com/ubuntu raring-backports main"
-        key = "akey"
-        fetch.add_source(source=source, key=key)
-        check_call.assert_has_calls([
-            call(['add-apt-repository', '--yes', source]),
-            call(['apt-key', 'adv', '--keyserver', 'hkp://keyserver.ubuntu.com:80',
-                  '--recv', key])
-        ])
-
-    @patch('subprocess.check_call')
-    def test_add_source_https_and_key(self, check_call):
-        source = "http://USER:[e-mail]/project/awesome"
-        key = "GPGPGP"
-        fetch.add_source(source=source, key=key)
-        check_call.assert_has_calls([
-            call(['add-apt-repository', '--yes', source]),
-            call(['apt-key', 'adv', '--keyserver', 'hkp://keyserver.ubuntu.com:80',
-                  '--recv', key])
-        ])
+        key = '''
+            -----BEGIN PGP PUBLIC KEY BLOCK-----
+            [...]
+            -----END PGP PUBLIC KEY BLOCK-----
+            '''
+
+        received_args = []
+        received_key = StringIO()
+        def _check_call(arg, stdin=None):
+            '''side_effect to store the stdin passed to check_call process.'''
+            if stdin is not None:
+                received_args.extend(arg)
+                received_key.write(stdin.read())
+
+        with patch('subprocess.check_call',
+                   side_effect=_check_call) as check_call:
+            fetch.add_source(source=source, key=key)
+            check_call.assert_any_call(['add-apt-repository', '--yes', source])
+            self.assertEqual(['apt-key', 'add', '-'], received_args)
+            self.assertEqual(key, received_key.getvalue())
+
+    def test_add_unparsable_source(self):
+        source = "propsed"  # Minor typo
+        self.assertRaises(fetch.SourceConfigError, fetch.add_source, source)
 
     @patch.object(fetch, 'config')
     @patch.object(fetch, 'add_source')