From f8303d860504ecc2a1e469385a364edc8856c107 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 14 Oct 2021 08:38:34 -0600 Subject: [PATCH 01/15] wip - remove (depreciated) apt-key --- cloudinit/apt.py | 62 ++++++++++++++++++++++++++++ cloudinit/config/cc_apt_configure.py | 8 ++-- cloudinit/gpg.py | 31 ++++++++++++++ 3 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 cloudinit/apt.py diff --git a/cloudinit/apt.py b/cloudinit/apt.py new file mode 100644 index 00000000000..7d22b705821 --- /dev/null +++ b/cloudinit/apt.py @@ -0,0 +1,62 @@ +# Copyright (C) 2021 Canonical Ltd. +# +# Author: Brett Holman +# +# This file is part of cloud-init. See LICENSE file for license information. + +"""apt.py - Collection of apt related functions""" + +import os + +from cloudinit import log as logging +from cloudinit import gpg +from cloudinit import util + +LOCAL_KEYS='/etc/apt/trusted.gpg' +KEY_DIR='/etc/apt/trusted.gpg.d/' + +LOG = logging.getLogger(__name__) + + +def key(command, input_file=None, output_file=None, data=None): + """apt-key commands implemented: 'add', 'list', 'finger' + + @param input_file: '-' or file name to read from + @param output_file: name of output gpg file (without .gpg or .asc) + @param data: key contents + """ + + def _get_key_files(): + """return all apt keys + + /etc/apt/trusted.gpg (if it exists) and all keyfiles (and symlinks to + keyfiles) in /etc/apt/trusted.gpg.d/ are returned + """ + key_files = [LOCAL_KEYS] if not os.path.isfile(LOCAL_KEYS) else [] + + for file in os.listdir(KEY_DIR): + if file.endswith('.gpg') or file.endswith('.asc'): + key_files.append(file) + return key_files + + def add(input_file, output_file, data): + """apt-key add + """ + if input_file == '-': + stdout = gpg.dearmor(data) + util.write_file(KEY_DIR + '{}.gpg'.format(output_file), stdout) + else: + raise NotImplementedError + + def list(): + """apt-key list + """ + for key_file in _get_key_files(): + gpg.list(key_file) + + if command == 'add': + add(input_file, output_file, data) + elif command == 'finger' or command == 'list': + list() + else: + raise NotImplementedError diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 0c9c7925416..69e9bee9332 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -18,6 +18,7 @@ from cloudinit import gpg from cloudinit import log as logging from cloudinit import subp +from cloudinit import apt from cloudinit import templater from cloudinit import util from cloudinit.settings import PER_INSTANCE @@ -714,14 +715,15 @@ def generate_sources_list(cfg, release, mirrors, cloud): util.write_file(aptsrc, disabled, mode=0o644) -def add_apt_key_raw(key, target=None): +def add_apt_key_raw(key, file_name, target=None): """ actual adding of a key as defined in key argument to the system """ LOG.debug("Adding key:\n'%s'", key) try: - subp.subp(['apt-key', 'add', '-'], data=key.encode(), target=target) + name = file_name if not file_name.endswith('.list') else file_name[:-5] + apt.key('add', input_file='-', output_file=name, data=key) except subp.ProcessExecutionError: LOG.exception("failed to add apt GPG Key to apt keyring") raise @@ -741,7 +743,7 @@ def add_apt_key(ent, target=None): ent['key'] = gpg.getkeybyid(ent['keyid'], keyserver) if 'key' in ent: - add_apt_key_raw(ent['key'], target) + add_apt_key_raw(ent['key'], ent['filename'], target) def update_packages(cloud): diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py index 3780326c1c8..39555b803c8 100644 --- a/cloudinit/gpg.py +++ b/cloudinit/gpg.py @@ -27,6 +27,37 @@ def export_armour(key): return armour +def dearmor(key): + """Dearmor gpg key, dearmored key gets returned + + note: man gpg(1) makes no mention of an --armour spelling, only --armor + """ + + (stdout, _) = subp.subp(["gpg", "--dearmor"], data=key, decode=False, + capture=True) + return stdout + + +def list(key_file, human_output=False): + """List keys from a keyring with fingerprints. Default to a stable machine + parseable format. + + @param key_file: a string containing a filepath to a key + @param human_output: return output intended for human parsing + """ + + cmd = ['gpg', '--with-fingerprint', '--no-default-keyring', '--list-keys', + '--keyring'] + if not human_output: + cmd.append('--with-colons') + + cmd.append(key_file) + (stdout, stderr) = subp.subp(cmd, capture=True) + if stderr: + LOG.debug('Failed to export armoured key "%s": %s', key_file, stderr) + return stdout + + def recv_key(key, keyserver, retries=(1, 1)): """Receive gpg key from the specified keyserver. From 2fde927cf3941546ec5d65c20142bb04310f09f1 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Fri, 15 Oct 2021 15:10:09 -0600 Subject: [PATCH 02/15] remove apt-key from tests --- cloudinit/apt.py | 62 --------------- cloudinit/config/cc_apt_configure.py | 75 ++++++++++++++++++- cloudinit/gpg.py | 8 +- tests/integration_tests/modules/test_apt.py | 18 ++++- .../test_handler_apt_source_v1.py | 45 ++++++----- .../test_handler_apt_source_v3.py | 59 ++++++++------- 6 files changed, 153 insertions(+), 114 deletions(-) delete mode 100644 cloudinit/apt.py diff --git a/cloudinit/apt.py b/cloudinit/apt.py deleted file mode 100644 index 7d22b705821..00000000000 --- a/cloudinit/apt.py +++ /dev/null @@ -1,62 +0,0 @@ -# Copyright (C) 2021 Canonical Ltd. -# -# Author: Brett Holman -# -# This file is part of cloud-init. See LICENSE file for license information. - -"""apt.py - Collection of apt related functions""" - -import os - -from cloudinit import log as logging -from cloudinit import gpg -from cloudinit import util - -LOCAL_KEYS='/etc/apt/trusted.gpg' -KEY_DIR='/etc/apt/trusted.gpg.d/' - -LOG = logging.getLogger(__name__) - - -def key(command, input_file=None, output_file=None, data=None): - """apt-key commands implemented: 'add', 'list', 'finger' - - @param input_file: '-' or file name to read from - @param output_file: name of output gpg file (without .gpg or .asc) - @param data: key contents - """ - - def _get_key_files(): - """return all apt keys - - /etc/apt/trusted.gpg (if it exists) and all keyfiles (and symlinks to - keyfiles) in /etc/apt/trusted.gpg.d/ are returned - """ - key_files = [LOCAL_KEYS] if not os.path.isfile(LOCAL_KEYS) else [] - - for file in os.listdir(KEY_DIR): - if file.endswith('.gpg') or file.endswith('.asc'): - key_files.append(file) - return key_files - - def add(input_file, output_file, data): - """apt-key add - """ - if input_file == '-': - stdout = gpg.dearmor(data) - util.write_file(KEY_DIR + '{}.gpg'.format(output_file), stdout) - else: - raise NotImplementedError - - def list(): - """apt-key list - """ - for key_file in _get_key_files(): - gpg.list(key_file) - - if command == 'add': - add(input_file, output_file, data) - elif command == 'finger' or command == 'list': - list() - else: - raise NotImplementedError diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 69e9bee9332..965d3128ea7 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -18,7 +18,6 @@ from cloudinit import gpg from cloudinit import log as logging from cloudinit import subp -from cloudinit import apt from cloudinit import templater from cloudinit import util from cloudinit.settings import PER_INSTANCE @@ -28,6 +27,10 @@ # this will match 'XXX:YYY' (ie, 'cloud-archive:foo' or 'ppa:bar') ADD_APT_REPO_MATCH = r"^[\w-]+:\w" +APT_LOCAL_KEYS='/etc/apt/trusted.gpg' +APT_TRUSTED_DIR='/etc/apt/trusted.gpg.d/' +APT_CLOUD_INIT_DIR='/etc/apt/cloud-init.gpg.d/' + frequency = PER_INSTANCE distros = ["ubuntu", "debian"] mirror_property = { @@ -684,6 +687,8 @@ def add_mirror_keys(cfg, target): """Adds any keys included in the primary/security mirror clauses""" for key in ('primary', 'security'): for mirror in cfg.get(key, []): + if 'filename' not in mirror: + mirror['filename'] = key add_apt_key(mirror, target) @@ -723,7 +728,7 @@ def add_apt_key_raw(key, file_name, target=None): LOG.debug("Adding key:\n'%s'", key) try: name = file_name if not file_name.endswith('.list') else file_name[:-5] - apt.key('add', input_file='-', output_file=name, data=key) + apt_key('add', output_file=name, data=key) except subp.ProcessExecutionError: LOG.exception("failed to add apt GPG Key to apt keyring") raise @@ -1008,7 +1013,7 @@ def get_arch_mirrorconfig(cfg, mirrortype, arch): # select the specification matching the target arch default = None for mirror_cfg_elem in mirror_cfg_list: - arches = mirror_cfg_elem.get("arches") + arches = mirror_cfg_elem.get("arches", []) if arch in arches: return mirror_cfg_elem if "default" in arches: @@ -1091,6 +1096,70 @@ def apply_apt_config(cfg, proxy_fname, config_fname): LOG.debug("no apt config configured, removed %s", config_fname) +def apt_key(command, output_file=None, data=None, hardened=False, + human_output=True): + """apt-key replacement + + commands implemented: 'add', 'list', 'finger' + + @param output_file: name of output gpg file (without .gpg or .asc) + @param data: key contents + @param human_output: list keys formatted for human parsing + @param hardened: NOT IMPLEMENTED - write keys to to + /etc/apt/cloud-init.gpg.d/ (refered to with [signed-by] in sources file) + """ + + def _get_key_files(): + """return all apt keys + + /etc/apt/trusted.gpg (if it exists) and all keyfiles (and symlinks to + keyfiles) in /etc/apt/trusted.gpg.d/ are returned + + based on apt-key implementation + """ + key_files = [APT_LOCAL_KEYS] if os.path.isfile(APT_LOCAL_KEYS) else [] + + for file in os.listdir(APT_TRUSTED_DIR): + print(file) + if file.endswith('.gpg') or file.endswith('.asc'): + key_files.append(APT_TRUSTED_DIR + file) + return key_files if key_files else '' + + def apt_key_add(): + """apt-key add + """ + if hardened: + raise NotImplementedError + try: + key_dir = APT_CLOUD_INIT_DIR if hardened else APT_TRUSTED_DIR + stdout = gpg.dearmor(data) + util.write_file(key_dir + '{}.gpg'.format(output_file), stdout) + except subp.ProcessExecutionError as error: + LOG.debug('Failed to add key "%s": %s', data, error) + except UnicodeDecodeError as error: + LOG.debug('Invalid key format "%s": %s', data, error) + + def apt_key_list(): + """apt-key list + """ + key_list = [] + for key_file in _get_key_files(): + try: + key_list.append(gpg.list(key_file, human_output=human_output)) + except subp.ProcessExecutionError as error: + LOG.debug('Failed to list key "%s": %s', key_file, error) + print("apt_key_list:\n{}".format(key_list)) + return '\n'.join(key_list) + + if command == 'add': + return apt_key_add() + elif command == 'finger' or command == 'list': + return apt_key_list() + else: + raise ValueError('apt_key() commands add, list, and finger are' + 'currently supported') + + CONFIG_CLEANERS = { 'cloud-init': clean_cloud_init, } diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py index 39555b803c8..6c20bbd65c1 100644 --- a/cloudinit/gpg.py +++ b/cloudinit/gpg.py @@ -14,6 +14,8 @@ LOG = logging.getLogger(__name__) +GPG_LIST = ['gpg', '--with-fingerprint', '--no-default-keyring', '--list-keys', + '--keyring'] def export_armour(key): """Export gpg key, armoured key gets returned""" @@ -33,6 +35,8 @@ def dearmor(key): note: man gpg(1) makes no mention of an --armour spelling, only --armor """ + if not key: + raise ValueError("invalid attempt to dearmor key") (stdout, _) = subp.subp(["gpg", "--dearmor"], data=key, decode=False, capture=True) return stdout @@ -46,8 +50,8 @@ def list(key_file, human_output=False): @param human_output: return output intended for human parsing """ - cmd = ['gpg', '--with-fingerprint', '--no-default-keyring', '--list-keys', - '--keyring'] + cmd = [] + cmd.extend(GPG_LIST) if not human_output: cmd.append('--with-colons') diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index 54711fc02cb..4f1498c2e74 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -1,8 +1,11 @@ """Series of integration tests covering apt functionality.""" +import os import re from tests.integration_tests.clouds import ImageSpecification import pytest +from cloudinit.config import cc_apt_configure +from cloudinit import gpg from tests.integration_tests.instances import IntegrationInstance @@ -152,7 +155,10 @@ def test_ppa_source(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu' ) in ppa_path_contents - keys = class_client.execute('apt-key finger') + list_keys = [] + list_keys.extend(gpg.GPG_LIST) + list_keys.append(cc_apt_configure.APT_LOCAL_KEYS) + keys = class_client.execute(' '.join(list_keys)) assert TEST_PPA_KEY in keys def test_key(self, class_client: IntegrationInstance): @@ -169,7 +175,10 @@ def test_key(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu' ) in test_archive_contents - keys = class_client.execute('apt-key finger') + list_keys = [] + list_keys.extend(gpg.GPG_LIST) + list_keys.append(cc_apt_configure.APT_LOCAL_KEYS) + keys = class_client.execute(' '.join(list_keys)) assert TEST_KEY in keys def test_keyserver(self, class_client: IntegrationInstance): @@ -186,7 +195,10 @@ def test_keyserver(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu' ) in test_keyserver_contents - keys = class_client.execute('apt-key finger') + list_keys = [] + list_keys.extend(gpg.GPG_LIST) + list_keys.append(cc_apt_configure.APT_LOCAL_KEYS) + keys = class_client.execute(' '.join(list_keys)) assert TEST_KEYSERVER_KEY in keys def test_os_pipelining(self, class_client: IntegrationInstance): diff --git a/tests/unittests/test_handler/test_handler_apt_source_v1.py b/tests/unittests/test_handler/test_handler_apt_source_v1.py index 367971cb794..0057fdd168c 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v1.py @@ -279,16 +279,16 @@ def apt_src_keyid(self, filename, cfg, keynum): """ cfg = self.wrapv1conf(cfg) - with mock.patch.object(subp, 'subp', - return_value=('fakekey 1234', '')) as mockobj: + with mock.patch.object(cc_apt_configure, 'add_apt_key') as mockobj: cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) - # check if it added the right ammount of keys + # check if it added the right number of keys calls = [] - for _ in range(keynum): - calls.append(call(['apt-key', 'add', '-'], - data=b'fakekey 1234', - target=None)) + sources = cfg['apt']['sources'] + for src in sources: + print(sources[src]) + calls.append(call(sources[src], None)) + mockobj.assert_has_calls(calls, any_order=True) self.assertTrue(os.path.isfile(filename)) @@ -364,11 +364,17 @@ def apt_src_key(self, filename, cfg): """ cfg = self.wrapv1conf([cfg]) - with mock.patch.object(subp, 'subp') as mockobj: + with mock.patch.object(cc_apt_configure, 'add_apt_key') as mockobj: cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) - mockobj.assert_called_with(['apt-key', 'add', '-'], - data=b'fakekey 4321', target=None) + # check if it added the right amount of keys + sources = cfg['apt']['sources'] + calls = [] + for src in sources: + print(sources[src]) + calls.append(call(sources[src], None)) + + mockobj.assert_has_calls(calls, any_order=True) self.assertTrue(os.path.isfile(filename)) @@ -405,12 +411,11 @@ def test_apt_src_keyonly(self): cfg = {'key': "fakekey 4242", 'filename': self.aptlistfile} cfg = self.wrapv1conf([cfg]) - - with mock.patch.object(subp, 'subp') as mockobj: + with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) - mockobj.assert_called_once_with(['apt-key', 'add', '-'], - data=b'fakekey 4242', target=None) + calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 4242'),) + mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -422,11 +427,13 @@ def test_apt_src_keyidonly(self): cfg = self.wrapv1conf([cfg]) with mock.patch.object(subp, 'subp', - return_value=('fakekey 1212', '')) as mockobj: - cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) + return_value=('fakekey 1212', '')): + with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: + cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) + + calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 1212'),) + mockobj.assert_has_calls(calls, any_order=True) - mockobj.assert_called_with(['apt-key', 'add', '-'], - data=b'fakekey 1212', target=None) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -448,7 +455,7 @@ def apt_src_keyid_real(self, cfg, expectedkey): None, None) mockgetkey.assert_called_with(key, keyserver) - mockkey.assert_called_with(expectedkey, None) + mockkey.assert_called_with(expectedkey, self.aptlistfile, None) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py index d4db610f850..46addde9b26 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v3.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py @@ -220,16 +220,15 @@ def _apt_src_keyid(self, filename, cfg, keynum): """ params = self._get_default_params() - with mock.patch("cloudinit.subp.subp", - return_value=('fakekey 1234', '')) as mockobj: + with mock.patch.object(cc_apt_configure, 'add_apt_key') as mockobj: self._add_apt_sources(cfg, TARGET, template_params=params, aa_repo_match=self.matcher) - # check if it added the right ammount of keys + # check if it added the right number of keys calls = [] - for _ in range(keynum): - calls.append(call(['apt-key', 'add', '-'], data=b'fakekey 1234', - target=TARGET)) + for key in cfg: + calls.append(call(cfg[key], TARGET)) + mockobj.assert_has_calls(calls, any_order=True) self.assertTrue(os.path.isfile(filename)) @@ -248,6 +247,7 @@ def test_apt_v3_src_keyid(self): 'http://ppa.launchpad.net/' 'smoser/cloud-init-test/ubuntu' ' xenial main'), + 'filename': self.aptlistfile, 'keyid': "03683F77"}} self._apt_src_keyid(self.aptlistfile, cfg, 1) @@ -268,6 +268,7 @@ def test_apt_v3_src_keyid_tri(self): 'http://ppa.launchpad.net/' 'smoser/cloud-init-test/ubuntu' ' xenial multiverse'), + 'filename': self.aptlistfile3, 'keyid': "03683F77"}} self._apt_src_keyid(self.aptlistfile, cfg, 3) @@ -293,15 +294,15 @@ def test_apt_v3_src_key(self): 'http://ppa.launchpad.net/' 'smoser/cloud-init-test/ubuntu' ' xenial main'), + 'filename': self.aptlistfile, 'key': "fakekey 4321"}} - with mock.patch.object(subp, 'subp') as mockobj: + with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: self._add_apt_sources(cfg, TARGET, template_params=params, aa_repo_match=self.matcher) - mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 4321', - target=TARGET) - + calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 4321'),) + mockobj.assert_has_calls(calls, any_order=True) self.assertTrue(os.path.isfile(self.aptlistfile)) contents = util.load_file(self.aptlistfile) @@ -317,12 +318,13 @@ def test_apt_v3_src_keyonly(self): params = self._get_default_params() cfg = {self.aptlistfile: {'key': "fakekey 4242"}} - with mock.patch.object(subp, 'subp') as mockobj: + + with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: self._add_apt_sources(cfg, TARGET, template_params=params, aa_repo_match=self.matcher) - mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 4242', - target=TARGET) + calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 4242'),) + mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -331,14 +333,15 @@ def test_apt_v3_src_keyidonly(self): """test_apt_v3_src_keyidonly - Test keyid without source""" params = self._get_default_params() cfg = {self.aptlistfile: {'keyid': "03683F77"}} - with mock.patch.object(subp, 'subp', - return_value=('fakekey 1212', '')) as mockobj: - self._add_apt_sources(cfg, TARGET, template_params=params, - aa_repo_match=self.matcher) + return_value=('fakekey 1212', '')): + with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: + self._add_apt_sources(cfg, TARGET, template_params=params, + aa_repo_match=self.matcher) + + calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 1212'),) + mockobj.assert_has_calls(calls, any_order=True) - mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 1212', - target=TARGET) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -361,7 +364,7 @@ def apt_src_keyid_real(self, cfg, expectedkey): mockgetkey.assert_called_with(keycfg['keyid'], keycfg.get('keyserver', 'keyserver.ubuntu.com')) - mockkey.assert_called_with(expectedkey, TARGET) + mockkey.assert_called_with(expectedkey, keycfg['keyfile'], TARGET) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -369,14 +372,16 @@ def apt_src_keyid_real(self, cfg, expectedkey): def test_apt_v3_src_keyid_real(self): """test_apt_v3_src_keyid_real - Test keyid including key add""" keyid = "03683F77" - cfg = {self.aptlistfile: {'keyid': keyid}} + cfg = {self.aptlistfile: {'keyid': keyid, + 'keyfile': self.aptlistfile}} self.apt_src_keyid_real(cfg, EXPECTEDKEY) def test_apt_v3_src_longkeyid_real(self): """test_apt_v3_src_longkeyid_real Test long keyid including key add""" keyid = "B59D 5F15 97A5 04B7 E230 6DCA 0620 BBCF 0368 3F77" - cfg = {self.aptlistfile: {'keyid': keyid}} + cfg = {self.aptlistfile: {'keyid': keyid, + 'keyfile': self.aptlistfile}} self.apt_src_keyid_real(cfg, EXPECTEDKEY) @@ -384,6 +389,7 @@ def test_apt_v3_src_longkeyid_ks_real(self): """test_apt_v3_src_longkeyid_ks_real Test long keyid from other ks""" keyid = "B59D 5F15 97A5 04B7 E230 6DCA 0620 BBCF 0368 3F77" cfg = {self.aptlistfile: {'keyid': keyid, + 'keyfile': self.aptlistfile, 'keyserver': 'keys.gnupg.net'}} self.apt_src_keyid_real(cfg, EXPECTEDKEY) @@ -393,6 +399,7 @@ def test_apt_v3_src_keyid_keyserver(self): keyid = "03683F77" params = self._get_default_params() cfg = {self.aptlistfile: {'keyid': keyid, + 'keyfile': self.aptlistfile, 'keyserver': 'test.random.com'}} # in some test environments only *.ubuntu.com is reachable @@ -405,7 +412,7 @@ def test_apt_v3_src_keyid_keyserver(self): aa_repo_match=self.matcher) mockgetkey.assert_called_with('03683F77', 'test.random.com') - mockadd.assert_called_with('fakekey', TARGET) + mockadd.assert_called_with('fakekey', self.aptlistfile, TARGET) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -1002,10 +1009,12 @@ def test_apt_v3_add_mirror_keys(self): 'primary': [ {'arches': [arch], 'uri': 'http://test.ubuntu.com/', + 'filename': 'primary', 'key': 'fakekey_primary'}], 'security': [ {'arches': [arch], 'uri': 'http://testsec.ubuntu.com/', + 'filename': 'security', 'key': 'fakekey_security'}] } @@ -1013,8 +1022,8 @@ def test_apt_v3_add_mirror_keys(self): 'add_apt_key_raw') as mockadd: cc_apt_configure.add_mirror_keys(cfg, TARGET) calls = [ - mock.call('fakekey_primary', TARGET), - mock.call('fakekey_security', TARGET), + mock.call('fakekey_primary', 'primary', TARGET), + mock.call('fakekey_security', 'security', TARGET), ] mockadd.assert_has_calls(calls, any_order=True) From ce9b5e0e41d3c0756dec57480c6b73641e4fb370 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 19 Oct 2021 11:37:33 -0600 Subject: [PATCH 03/15] fixup tests --- cloudinit/config/cc_apt_configure.py | 10 +++--- cloudinit/gpg.py | 3 +- tests/integration_tests/modules/test_apt.py | 33 +++++++++---------- .../test_handler_apt_source_v1.py | 20 ++++++++--- .../test_handler_apt_source_v3.py | 17 +++++++--- 5 files changed, 50 insertions(+), 33 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 965d3128ea7..7361aea83d1 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -27,9 +27,9 @@ # this will match 'XXX:YYY' (ie, 'cloud-archive:foo' or 'ppa:bar') ADD_APT_REPO_MATCH = r"^[\w-]+:\w" -APT_LOCAL_KEYS='/etc/apt/trusted.gpg' -APT_TRUSTED_DIR='/etc/apt/trusted.gpg.d/' -APT_CLOUD_INIT_DIR='/etc/apt/cloud-init.gpg.d/' +APT_LOCAL_KEYS = '/etc/apt/trusted.gpg' +APT_TRUSTED_DIR = '/etc/apt/trusted.gpg.d/' +APT_CLOUD_INIT_DIR = '/etc/apt/cloud-init.gpg.d/' frequency = PER_INSTANCE distros = ["ubuntu", "debian"] @@ -1156,8 +1156,8 @@ def apt_key_list(): elif command == 'finger' or command == 'list': return apt_key_list() else: - raise ValueError('apt_key() commands add, list, and finger are' - 'currently supported') + raise ValueError( + 'apt_key() commands add, list, and finger are currently supported') CONFIG_CLEANERS = { diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py index 6c20bbd65c1..da166172672 100644 --- a/cloudinit/gpg.py +++ b/cloudinit/gpg.py @@ -17,6 +17,7 @@ GPG_LIST = ['gpg', '--with-fingerprint', '--no-default-keyring', '--list-keys', '--keyring'] + def export_armour(key): """Export gpg key, armoured key gets returned""" try: @@ -38,7 +39,7 @@ def dearmor(key): if not key: raise ValueError("invalid attempt to dearmor key") (stdout, _) = subp.subp(["gpg", "--dearmor"], data=key, decode=False, - capture=True) + capture=True) return stdout diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index 4f1498c2e74..dc8af8cf820 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -1,5 +1,4 @@ """Series of integration tests covering apt functionality.""" -import os import re from tests.integration_tests.clouds import ImageSpecification @@ -100,6 +99,19 @@ @pytest.mark.ubuntu @pytest.mark.user_data(USER_DATA) class TestApt: + def get_keys(self, class_client: IntegrationInstance): + """Return all keys in /etc/apt/trusted.gpg.d/ and /etc/apt/trusted.gpg + in human readable format. + """ + list_cmd = ' '.join(gpg.GPG_LIST) + ' ' + keys = class_client.execute(list_cmd + cc_apt_configure.APT_LOCAL_KEYS) + print(keys) + files = class_client.execute('ls ' + cc_apt_configure.APT_TRUSTED_DIR) + for file in files.split(): + path = cc_apt_configure.APT_TRUSTED_DIR + file + keys += class_client.execute(list_cmd + path) or '' + return keys + def test_sources_list(self, class_client: IntegrationInstance): """Integration test for the apt module's `sources_list` functionality. @@ -155,11 +167,7 @@ def test_ppa_source(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu' ) in ppa_path_contents - list_keys = [] - list_keys.extend(gpg.GPG_LIST) - list_keys.append(cc_apt_configure.APT_LOCAL_KEYS) - keys = class_client.execute(' '.join(list_keys)) - assert TEST_PPA_KEY in keys + assert TEST_KEY in self.get_keys(class_client) def test_key(self, class_client: IntegrationInstance): """Test the apt key functionality. @@ -174,12 +182,7 @@ def test_key(self, class_client: IntegrationInstance): assert ( 'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu' ) in test_archive_contents - - list_keys = [] - list_keys.extend(gpg.GPG_LIST) - list_keys.append(cc_apt_configure.APT_LOCAL_KEYS) - keys = class_client.execute(' '.join(list_keys)) - assert TEST_KEY in keys + assert TEST_KEY in self.get_keys(class_client) def test_keyserver(self, class_client: IntegrationInstance): """Test the apt keyserver functionality. @@ -195,11 +198,7 @@ def test_keyserver(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu' ) in test_keyserver_contents - list_keys = [] - list_keys.extend(gpg.GPG_LIST) - list_keys.append(cc_apt_configure.APT_LOCAL_KEYS) - keys = class_client.execute(' '.join(list_keys)) - assert TEST_KEYSERVER_KEY in keys + assert TEST_KEY in self.get_keys(class_client) def test_os_pipelining(self, class_client: IntegrationInstance): """Test 'os' settings does not write apt config file. diff --git a/tests/unittests/test_handler/test_handler_apt_source_v1.py b/tests/unittests/test_handler/test_handler_apt_source_v1.py index 0057fdd168c..2776739dd2b 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v1.py @@ -414,7 +414,10 @@ def test_apt_src_keyonly(self): with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) - calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 4242'),) + calls = (call( + 'add', + output_file=self.aptlistfile[:-5], + data='fakekey 4242'),) mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only @@ -429,12 +432,19 @@ def test_apt_src_keyidonly(self): with mock.patch.object(subp, 'subp', return_value=('fakekey 1212', '')): with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: - cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) - - calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 1212'),) + cc_apt_configure.handle( + "test", + cfg, + self.fakecloud, + None, + None) + + calls = (call( + 'add', + output_file=self.aptlistfile[:-5], + data='fakekey 1212'),) mockobj.assert_has_calls(calls, any_order=True) - # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py index 46addde9b26..11ed090cd2b 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v3.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py @@ -301,7 +301,10 @@ def test_apt_v3_src_key(self): self._add_apt_sources(cfg, TARGET, template_params=params, aa_repo_match=self.matcher) - calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 4321'),) + calls = (call( + 'add', + output_file=self.aptlistfile[:-5], + data='fakekey 4321'),) mockobj.assert_has_calls(calls, any_order=True) self.assertTrue(os.path.isfile(self.aptlistfile)) @@ -318,12 +321,14 @@ def test_apt_v3_src_keyonly(self): params = self._get_default_params() cfg = {self.aptlistfile: {'key': "fakekey 4242"}} - with mock.patch.object(cc_apt_configure, 'apt_key') as mockobj: self._add_apt_sources(cfg, TARGET, template_params=params, aa_repo_match=self.matcher) - calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 4242'),) + calls = (call( + 'add', + output_file=self.aptlistfile[:-5], + data='fakekey 4242'),) mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only @@ -339,10 +344,12 @@ def test_apt_v3_src_keyidonly(self): self._add_apt_sources(cfg, TARGET, template_params=params, aa_repo_match=self.matcher) - calls = (call('add', output_file=self.aptlistfile[:-5], data='fakekey 1212'),) + calls = (call( + 'add', + output_file=self.aptlistfile[:-5], + data='fakekey 1212'),) mockobj.assert_has_calls(calls, any_order=True) - # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) From cd1e350a6f95a65c8b884bf063dd376fc3d9ac3f Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 19 Oct 2021 14:29:09 -0600 Subject: [PATCH 04/15] Add support for apt source "signed-by" option. --- cloudinit/config/cc_apt_configure.py | 38 ++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 7361aea83d1..de0daf1db58 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -143,7 +143,7 @@ source1: keyid: 'keyid' keyserver: 'keyserverurl' - source: 'deb http:/// xenial main' + source: 'deb [signed-by=$KEY_FILE] http:/// xenial main' source2: source: 'ppa:' source3: @@ -316,7 +316,8 @@ - ``$MIRROR`` - ``$RELEASE`` - ``$PRIMARY`` - - ``$SECURITY``""") + - ``$SECURITY`` + - ``$KEY_FILE``""") }, 'conf': { 'type': 'string', @@ -385,7 +386,8 @@ - ``$MIRROR`` - ``$PRIMARY`` - ``$SECURITY`` - - ``$RELEASE``""") + - ``$RELEASE`` + - ``$KEY_FILE``""") } } } @@ -720,7 +722,7 @@ def generate_sources_list(cfg, release, mirrors, cloud): util.write_file(aptsrc, disabled, mode=0o644) -def add_apt_key_raw(key, file_name, target=None): +def add_apt_key_raw(key, file_name, target=None, hardened=False): """ actual adding of a key as defined in key argument to the system @@ -728,13 +730,13 @@ def add_apt_key_raw(key, file_name, target=None): LOG.debug("Adding key:\n'%s'", key) try: name = file_name if not file_name.endswith('.list') else file_name[:-5] - apt_key('add', output_file=name, data=key) + return apt_key('add', output_file=name, data=key, hardened=hardened) except subp.ProcessExecutionError: LOG.exception("failed to add apt GPG Key to apt keyring") raise -def add_apt_key(ent, target=None): +def add_apt_key(ent, target=None, hardened=False): """ Add key to the system as defined in ent (if any). Supports raw keys or keyid's @@ -748,7 +750,11 @@ def add_apt_key(ent, target=None): ent['key'] = gpg.getkeybyid(ent['keyid'], keyserver) if 'key' in ent: - add_apt_key_raw(ent['key'], ent['filename'], target) + return add_apt_key_raw( + ent['key'], + ent['filename'], + target, + hardened=hardened) def update_packages(cloud): @@ -777,11 +783,16 @@ def add_apt_sources(srcdict, cloud, target=None, template_params=None, if 'filename' not in ent: ent['filename'] = filename - add_apt_key(ent, target) + if 'source' in ent and '$KEY_FILE' in ent['source']: + key_file = add_apt_key(ent, target, hardened=True) + template_params['KEY_FILE'] = key_file + else: + key_file = add_apt_key(ent, target) if 'source' not in ent: continue source = ent['source'] + LOG.debug("adding source/template: %s/%s", source, template_params) source = templater.render_string(source, template_params) if not ent['filename'].startswith("/"): @@ -1127,13 +1138,15 @@ def _get_key_files(): def apt_key_add(): """apt-key add + + returns filepath to new keyring """ - if hardened: - raise NotImplementedError try: key_dir = APT_CLOUD_INIT_DIR if hardened else APT_TRUSTED_DIR stdout = gpg.dearmor(data) - util.write_file(key_dir + '{}.gpg'.format(output_file), stdout) + file_name = '{}{}.gpg'.format(key_dir, output_file) + util.write_file(file_name, stdout) + return file_name except subp.ProcessExecutionError as error: LOG.debug('Failed to add key "%s": %s', data, error) except UnicodeDecodeError as error: @@ -1141,6 +1154,9 @@ def apt_key_add(): def apt_key_list(): """apt-key list + + returns string of all trusted keys (in /etc/apt/trusted.gpg and + /etc/apt/trusted.gpg.d/) """ key_list = [] for key_file in _get_key_files(): From f5c4817f91d097955d86c78c533e190113b310df Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 21 Oct 2021 16:45:02 -0600 Subject: [PATCH 05/15] Fix error handling code + various cleanups --- cloudinit/config/cc_apt_configure.py | 47 ++++++++++----------- cloudinit/gpg.py | 2 +- tests/integration_tests/modules/test_apt.py | 4 +- 3 files changed, 26 insertions(+), 27 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index de0daf1db58..56a02dda061 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -11,6 +11,7 @@ import glob import os import re +import pathlib from textwrap import dedent from cloudinit.config.schema import ( @@ -28,8 +29,8 @@ ADD_APT_REPO_MATCH = r"^[\w-]+:\w" APT_LOCAL_KEYS = '/etc/apt/trusted.gpg' -APT_TRUSTED_DIR = '/etc/apt/trusted.gpg.d/' -APT_CLOUD_INIT_DIR = '/etc/apt/cloud-init.gpg.d/' +APT_TRUSTED_GPG_DIR = '/etc/apt/trusted.gpg.d/' +CLOUD_INIT_GPG_DIR = '/etc/apt/cloud-init.gpg.d/' frequency = PER_INSTANCE distros = ["ubuntu", "debian"] @@ -689,9 +690,7 @@ def add_mirror_keys(cfg, target): """Adds any keys included in the primary/security mirror clauses""" for key in ('primary', 'security'): for mirror in cfg.get(key, []): - if 'filename' not in mirror: - mirror['filename'] = key - add_apt_key(mirror, target) + add_apt_key(mirror, target, file_name=key) def generate_sources_list(cfg, release, mirrors, cloud): @@ -722,21 +721,21 @@ def generate_sources_list(cfg, release, mirrors, cloud): util.write_file(aptsrc, disabled, mode=0o644) -def add_apt_key_raw(key, file_name, target=None, hardened=False): +def add_apt_key_raw(key, file_name, hardened=False, target=None): """ actual adding of a key as defined in key argument to the system """ LOG.debug("Adding key:\n'%s'", key) try: - name = file_name if not file_name.endswith('.list') else file_name[:-5] + name = pathlib.Path(file_name).stem return apt_key('add', output_file=name, data=key, hardened=hardened) except subp.ProcessExecutionError: LOG.exception("failed to add apt GPG Key to apt keyring") raise -def add_apt_key(ent, target=None, hardened=False): +def add_apt_key(ent, target=None, hardened=False, file_name=None): """ Add key to the system as defined in ent (if any). Supports raw keys or keyid's @@ -752,8 +751,7 @@ def add_apt_key(ent, target=None, hardened=False): if 'key' in ent: return add_apt_key_raw( ent['key'], - ent['filename'], - target, + file_name or ent['filename'], hardened=hardened) @@ -792,7 +790,6 @@ def add_apt_sources(srcdict, cloud, target=None, template_params=None, if 'source' not in ent: continue source = ent['source'] - LOG.debug("adding source/template: %s/%s", source, template_params) source = templater.render_string(source, template_params) if not ent['filename'].startswith("/"): @@ -1116,8 +1113,8 @@ def apt_key(command, output_file=None, data=None, hardened=False, @param output_file: name of output gpg file (without .gpg or .asc) @param data: key contents @param human_output: list keys formatted for human parsing - @param hardened: NOT IMPLEMENTED - write keys to to - /etc/apt/cloud-init.gpg.d/ (refered to with [signed-by] in sources file) + @param hardened: write keys to to /etc/apt/cloud-init.gpg.d/ (referred to + with [signed-by] in sources file) """ def _get_key_files(): @@ -1130,27 +1127,29 @@ def _get_key_files(): """ key_files = [APT_LOCAL_KEYS] if os.path.isfile(APT_LOCAL_KEYS) else [] - for file in os.listdir(APT_TRUSTED_DIR): - print(file) + for file in os.listdir(APT_TRUSTED_GPG_DIR): if file.endswith('.gpg') or file.endswith('.asc'): - key_files.append(APT_TRUSTED_DIR + file) + key_files.append(APT_TRUSTED_GPG_DIR + file) return key_files if key_files else '' def apt_key_add(): """apt-key add - returns filepath to new keyring + returns filepath to new keyring, or '/dev/null' when an error occurs """ + file_name = '/dev/null' + if not output_file: + util.logexc(LOG, 'Unknown filename for key import'.format(data)) try: - key_dir = APT_CLOUD_INIT_DIR if hardened else APT_TRUSTED_DIR + key_dir = CLOUD_INIT_GPG_DIR if hardened else APT_TRUSTED_GPG_DIR stdout = gpg.dearmor(data) file_name = '{}{}.gpg'.format(key_dir, output_file) util.write_file(file_name, stdout) - return file_name - except subp.ProcessExecutionError as error: - LOG.debug('Failed to add key "%s": %s', data, error) - except UnicodeDecodeError as error: - LOG.debug('Invalid key format "%s": %s', data, error) + except subp.ProcessExecutionError: + util.logexc(LOG, 'Failed to add key: {}'.format(data)) + except UnicodeDecodeError: + util.logexc(LOG, 'Failed to add key: {}'.format(data)) + return file_name def apt_key_list(): """apt-key list @@ -1163,7 +1162,7 @@ def apt_key_list(): try: key_list.append(gpg.list(key_file, human_output=human_output)) except subp.ProcessExecutionError as error: - LOG.debug('Failed to list key "%s": %s', key_file, error) + LOG.warning('Failed to list key "%s": %s', key_file, error) print("apt_key_list:\n{}".format(key_list)) return '\n'.join(key_list) diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py index da166172672..0bec241bae4 100644 --- a/cloudinit/gpg.py +++ b/cloudinit/gpg.py @@ -59,7 +59,7 @@ def list(key_file, human_output=False): cmd.append(key_file) (stdout, stderr) = subp.subp(cmd, capture=True) if stderr: - LOG.debug('Failed to export armoured key "%s": %s', key_file, stderr) + LOG.warning('Failed to export armoured key "%s": %s', key_file, stderr) return stdout diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index dc8af8cf820..385ba63554f 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -106,9 +106,9 @@ def get_keys(self, class_client: IntegrationInstance): list_cmd = ' '.join(gpg.GPG_LIST) + ' ' keys = class_client.execute(list_cmd + cc_apt_configure.APT_LOCAL_KEYS) print(keys) - files = class_client.execute('ls ' + cc_apt_configure.APT_TRUSTED_DIR) + files = class_client.execute('ls ' + cc_apt_configure.APT_TRUSTED_GPG_DIR) for file in files.split(): - path = cc_apt_configure.APT_TRUSTED_DIR + file + path = cc_apt_configure.APT_TRUSTED_GPG_DIR + file keys += class_client.execute(list_cmd + path) or '' return keys From 880568c0119d818c44d81d39ea7515e94c00b2a7 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Sun, 24 Oct 2021 21:31:56 -0600 Subject: [PATCH 06/15] add gpg unit tests --- tests/unittests/test_gpg.py | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/unittests/test_gpg.py diff --git a/tests/unittests/test_gpg.py b/tests/unittests/test_gpg.py new file mode 100644 index 00000000000..0277efa0c1c --- /dev/null +++ b/tests/unittests/test_gpg.py @@ -0,0 +1,65 @@ +from unittest import mock + +from cloudinit.tests import helpers +from cloudinit import gpg +from cloudinit import subp + +TEST_KEY_HUMAN = ''' +/etc/apt/cloud-init.gpg.d/my_key.gpg +-------------------------------------------- +pub rsa4096 2021-10-22 [SC] + 3A3E F34D FDED B3B7 F3FD F603 F83F 7712 9A5E BD85 +uid [ unknown] Brett Holman +sub rsa4096 2021-10-22 [A] +sub rsa4096 2021-10-22 [E] +''' + +TEST_KEY_MACHINE = ''' +tru::1:1635129362:0:3:1:5 +pub:-:4096:1:F83F77129A5EBD85:1634912922:::-:::scESCA::::::23::0: +fpr:::::::::3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85: +uid:-::::1634912922::64F1F1D6FA96316752D635D7C6406C52C40713C7::Brett Holman \ +::::::::::0: +sub:-:4096:1:544B39C9A9141F04:1634912922::::::a::::::23: +fpr:::::::::8BD901490D6EC986D03D6F0D544B39C9A9141F04: +sub:-:4096:1:F45D9443F0A87092:1634912922::::::e::::::23: +fpr:::::::::8CCCB332317324F030A45B19F45D9443F0A87092: +''' + +TEST_KEY_FINGERPRINT_HUMAN = \ + '3A3E F34D FDED B3B7 F3FD F603 F83F 7712 9A5E BD85' + +TEST_KEY_FINGERPRINT_MACHINE = \ + '3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85' + + +class TestGPGCommands(helpers.CiTestCase): + def test_dearmor_empty_value(self): + with self.assertRaises(ValueError): + gpg.dearmor(None) + + def test_dearmor_bad_value(self): + with mock.patch.object( + subp, + 'subp', + side_effect=subp.ProcessExecutionError): + with self.assertRaises(subp.ProcessExecutionError): + gpg.dearmor('garbage key value') + + def list_output_helper(self, fingerprint, key): + with mock.patch.object(subp, 'subp', return_value=(key, '')): + assert fingerprint in gpg.list('/path/to/key.gpg') + + def test_list_human_output(self): + self.list_output_helper(TEST_KEY_FINGERPRINT_HUMAN, TEST_KEY_HUMAN) + + def test_list_machine_output(self): + self.list_output_helper(TEST_KEY_FINGERPRINT_MACHINE, TEST_KEY_MACHINE) + + def test_list_bad_value(self): + with mock.patch.object( + subp, + 'subp', + side_effect=subp.ProcessExecutionError): + with self.assertRaises(subp.ProcessExecutionError): + gpg.list('/path/to/bad/key.gpg') From 393e835f1d30377716269409ee3033eb6562c6a6 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 25 Oct 2021 14:12:09 -0600 Subject: [PATCH 07/15] add apt-key tests --- .../test_handler/test_handler_apt_key.py | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 tests/unittests/test_handler/test_handler_apt_key.py diff --git a/tests/unittests/test_handler/test_handler_apt_key.py b/tests/unittests/test_handler/test_handler_apt_key.py new file mode 100644 index 00000000000..1f2d6360cc0 --- /dev/null +++ b/tests/unittests/test_handler/test_handler_apt_key.py @@ -0,0 +1,147 @@ +import os +import shutil +import tempfile +from unittest import mock + +from cloudinit.tests import helpers +from cloudinit.config import cc_apt_configure +from cloudinit import subp +from cloudinit import util + +TEST_KEY_HUMAN = ''' +/etc/apt/cloud-init.gpg.d/my_key.gpg +-------------------------------------------- +pub rsa4096 2021-10-22 [SC] + 3A3E F34D FDED B3B7 F3FD F603 F83F 7712 9A5E BD85 +uid [ unknown] Brett Holman +sub rsa4096 2021-10-22 [A] +sub rsa4096 2021-10-22 [E] +''' + +TEST_KEY_MACHINE = ''' +tru::1:1635129362:0:3:1:5 +pub:-:4096:1:F83F77129A5EBD85:1634912922:::-:::scESCA::::::23::0: +fpr:::::::::3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85: +uid:-::::1634912922::64F1F1D6FA96316752D635D7C6406C52C40713C7::Brett Holman \ +::::::::::0: +sub:-:4096:1:544B39C9A9141F04:1634912922::::::a::::::23: +fpr:::::::::8BD901490D6EC986D03D6F0D544B39C9A9141F04: +sub:-:4096:1:F45D9443F0A87092:1634912922::::::e::::::23: +fpr:::::::::8CCCB332317324F030A45B19F45D9443F0A87092: +''' + +TEST_KEY_FINGERPRINT_HUMAN = \ + '3A3E F34D FDED B3B7 F3FD F603 F83F 7712 9A5E BD85' + +TEST_KEY_FINGERPRINT_MACHINE = \ + '3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85' + + +class TestAptKey(helpers.FilesystemMockingTestCase): + """TestAptKey + Class to test apt-key commands + """ + def setUp(self): + super(TestAptKey, self).setUp() + self.tmp = tempfile.mkdtemp() + self.new_root = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp) + self.addCleanup(shutil.rmtree, self.new_root) + + def _apt_key_add_success_helper(self, directory, hardened=False): + with mock.patch.object( + subp, + 'subp', + return_value=('fakekey', '')): + with mock.patch.object( + util, + 'write_file'): + file = cc_apt_configure.apt_key( + 'add', + output_file='my-key', + data='fakekey', + hardened=hardened) + assert file == directory + '/my-key.gpg' + + def test_apt_key_add_success(self): + self._apt_key_add_success_helper('/etc/apt/trusted.gpg.d') + + def test_apt_key_add_success_hardened(self): + self._apt_key_add_success_helper( + '/etc/apt/cloud-init.gpg.d', + hardened=True) + + def test_apt_key_add_fail_no_file_name(self): + file = cc_apt_configure.apt_key( + 'add', + output_file=None, + data='') + assert '/dev/null' == file + + def _apt_key_fail_helper(self, exception): + with mock.patch.object( + subp, + 'subp', + side_effect=exception): + file = cc_apt_configure.apt_key( + 'add', + output_file='my-key', + data='fakekey') + assert file == '/dev/null' + + def test_apt_key_add_fail_no_file_name_subproc(self): + self._apt_key_fail_helper(subp.ProcessExecutionError) + + def test_apt_key_add_fail_no_file_name_unicode(self): + self._apt_key_fail_helper(UnicodeDecodeError('test', b'', 1, 1, '')) + + def _apt_key_list_success_helper(self, finger, key, human_output=True): + with mock.patch.object( + subp, + 'subp', + return_value=(key, '')): + + with mock.patch.object( + os, + 'listdir', + return_value=('/fake/dir/key.gpg',)): + keys = cc_apt_configure.apt_key('list', human_output) + assert finger in keys + + def test_apt_key_list_success_human(self): + self._apt_key_list_success_helper( + TEST_KEY_FINGERPRINT_HUMAN, + TEST_KEY_HUMAN) + + def test_apt_key_list_success_machine(self): + self._apt_key_list_success_helper( + TEST_KEY_FINGERPRINT_MACHINE, + TEST_KEY_MACHINE, human_output=False) + + def test_apt_key_list_fail_no_keys(self): + with mock.patch.object( + os, + 'listdir', + return_value=()): + keys = cc_apt_configure.apt_key('list') + assert not keys + + def test_apt_key_list_fail_no_keys_file(self): + with mock.patch.object( + os, + 'listdir', + return_value=('file_not_gpg_key.txt')): + keys = cc_apt_configure.apt_key('list') + assert not keys + + def test_apt_key_list_fail_bad_key_file(self): + with mock.patch.object( + subp, + 'subp', + side_effect=subp.ProcessExecutionError): + with mock.patch.object( + os, + 'listdir', + return_value=('bad_gpg_key.gpg')): + keys = cc_apt_configure.apt_key('list') + assert not keys From d8d6ba21585c66897706187dd2a37937f7c703c8 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 25 Oct 2021 15:20:44 -0600 Subject: [PATCH 08/15] add integration test --- tests/integration_tests/integration_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/integration_settings.py b/tests/integration_tests/integration_settings.py index e4a790c212e..d2ed3a34a53 100644 --- a/tests/integration_tests/integration_settings.py +++ b/tests/integration_tests/integration_settings.py @@ -66,7 +66,7 @@ # Install from a PPA. It MUST start with 'ppa:' # # A path to a valid package to be uploaded and installed -CLOUD_INIT_SOURCE = 'NONE' +CLOUD_INIT_SOURCE = 'IN_PLACE' # Before an instance is torn down, we run `cloud-init collect-logs` # and transfer them locally. These settings specify when to collect these From ef56c9e8c8e6d1f7d4767ba8a4026d58cef1f129 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 25 Oct 2021 15:22:37 -0600 Subject: [PATCH 09/15] if filename not known, log an error and skip - don't write a file named .gpg --- cloudinit/config/cc_apt_configure.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 56a02dda061..4d6310c9d1c 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -1139,16 +1139,20 @@ def apt_key_add(): """ file_name = '/dev/null' if not output_file: - util.logexc(LOG, 'Unknown filename for key import'.format(data)) - try: - key_dir = CLOUD_INIT_GPG_DIR if hardened else APT_TRUSTED_GPG_DIR - stdout = gpg.dearmor(data) - file_name = '{}{}.gpg'.format(key_dir, output_file) - util.write_file(file_name, stdout) - except subp.ProcessExecutionError: - util.logexc(LOG, 'Failed to add key: {}'.format(data)) - except UnicodeDecodeError: - util.logexc(LOG, 'Failed to add key: {}'.format(data)) + util.logexc(LOG, 'Unknown filename, failed to add key: {}'.format( + data)) + else: + try: + key_dir = CLOUD_INIT_GPG_DIR if hardened else APT_TRUSTED_GPG_DIR + stdout = gpg.dearmor(data) + file_name = '{}{}.gpg'.format(key_dir, output_file) + util.write_file(file_name, stdout) + except subp.ProcessExecutionError: + util.logexc(LOG, 'Gpg error, failed to add key: {}'.format( + data)) + except UnicodeDecodeError: + util.logexc(LOG, 'Decode error, failed to add key: {}'.format( + data)) return file_name def apt_key_list(): @@ -1163,7 +1167,6 @@ def apt_key_list(): key_list.append(gpg.list(key_file, human_output=human_output)) except subp.ProcessExecutionError as error: LOG.warning('Failed to list key "%s": %s', key_file, error) - print("apt_key_list:\n{}".format(key_list)) return '\n'.join(key_list) if command == 'add': From d1c7dcea7aaccad49c5df5f53f7f92406ca55bb4 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 25 Oct 2021 16:54:00 -0600 Subject: [PATCH 10/15] test fixup --- cloudinit/config/cc_apt_configure.py | 9 ++-- .../integration_tests/integration_settings.py | 2 +- tests/integration_tests/modules/test_apt.py | 35 +++++++++++++--- .../test_handler_apt_source_v1.py | 28 ++++++++----- .../test_handler_apt_source_v3.py | 41 ++++++++++++------- 5 files changed, 80 insertions(+), 35 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 4d6310c9d1c..c2141ae212c 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -750,9 +750,9 @@ def add_apt_key(ent, target=None, hardened=False, file_name=None): if 'key' in ent: return add_apt_key_raw( - ent['key'], - file_name or ent['filename'], - hardened=hardened) + ent['key'], + file_name or ent['filename'], + hardened=hardened) def update_packages(cloud): @@ -1143,7 +1143,8 @@ def apt_key_add(): data)) else: try: - key_dir = CLOUD_INIT_GPG_DIR if hardened else APT_TRUSTED_GPG_DIR + key_dir = \ + CLOUD_INIT_GPG_DIR if hardened else APT_TRUSTED_GPG_DIR stdout = gpg.dearmor(data) file_name = '{}{}.gpg'.format(key_dir, output_file) util.write_file(file_name, stdout) diff --git a/tests/integration_tests/integration_settings.py b/tests/integration_tests/integration_settings.py index d2ed3a34a53..e4a790c212e 100644 --- a/tests/integration_tests/integration_settings.py +++ b/tests/integration_tests/integration_settings.py @@ -66,7 +66,7 @@ # Install from a PPA. It MUST start with 'ppa:' # # A path to a valid package to be uploaded and installed -CLOUD_INIT_SOURCE = 'IN_PLACE' +CLOUD_INIT_SOURCE = 'NONE' # Before an instance is torn down, we run `cloud-init collect-logs` # and transfer them locally. These settings specify when to collect these diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index 385ba63554f..581e87614b8 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -45,6 +45,10 @@ keyid: 441614D8 keyserver: keyserver.ubuntu.com source: "ppa:simplestreams-dev/trunk" + test_signed_by: + keyid: 3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85 + keyserver: keyserver.ubuntu.com + source: "deb [signed-by=$KEY_FILE] http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu $RELEASE main" test_key: source: "deb http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu $RELEASE main" key: | @@ -92,7 +96,8 @@ TEST_PPA_KEY = "3552 C902 B4DD F7BD 3842 1821 015D 28D7 4416 14D8" -TEST_KEY = "1FF0 D853 5EF7 E719 E5C8 1B9C 083D 06FB E4D3 04DF" +TEST_KEY_1 = "1FF0 D853 5EF7 E719 E5C8 1B9C 083D 06FB E4D3 04DF" +TEST_KEY_2 = "3A3E F34D FDED B3B7 F3FD F603 F83F 7712 9A5E BD85" @pytest.mark.ci @@ -106,7 +111,8 @@ def get_keys(self, class_client: IntegrationInstance): list_cmd = ' '.join(gpg.GPG_LIST) + ' ' keys = class_client.execute(list_cmd + cc_apt_configure.APT_LOCAL_KEYS) print(keys) - files = class_client.execute('ls ' + cc_apt_configure.APT_TRUSTED_GPG_DIR) + files = class_client.execute( + 'ls ' + cc_apt_configure.APT_TRUSTED_GPG_DIR) for file in files.split(): path = cc_apt_configure.APT_TRUSTED_GPG_DIR + file keys += class_client.execute(list_cmd + path) or '' @@ -167,7 +173,26 @@ def test_ppa_source(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu' ) in ppa_path_contents - assert TEST_KEY in self.get_keys(class_client) + assert TEST_KEY_1 in self.get_keys(class_client) + + def test_signed_by(self, class_client: IntegrationInstance): + """Test the apt signed-by functionality. + """ + release = ImageSpecification.from_os_image().release + source = ( + "deb [signed-by=/etc/apt/cloud-init.gpg.d/test_signed_by.gpg] " + "http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu" + " {} main".format(release)) + + path_contents = class_client.read_from_file( + '/etc/apt/sources.list.d/test_signed_by.list') + assert path_contents == source + + key = class_client.execute( + 'gpg --no-default-keyring --with-fingerprint --list-keys ' + '--keyring /etc/apt/cloud-init.gpg.d/test_signed_by.gpg') + + assert TEST_KEY_2 in key def test_key(self, class_client: IntegrationInstance): """Test the apt key functionality. @@ -182,7 +207,7 @@ def test_key(self, class_client: IntegrationInstance): assert ( 'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu' ) in test_archive_contents - assert TEST_KEY in self.get_keys(class_client) + assert TEST_KEY_1 in self.get_keys(class_client) def test_keyserver(self, class_client: IntegrationInstance): """Test the apt keyserver functionality. @@ -198,7 +223,7 @@ def test_keyserver(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu' ) in test_keyserver_contents - assert TEST_KEY in self.get_keys(class_client) + assert TEST_KEY_1 in self.get_keys(class_client) def test_os_pipelining(self, class_client: IntegrationInstance): """Test 'os' settings does not write apt config file. diff --git a/tests/unittests/test_handler/test_handler_apt_source_v1.py b/tests/unittests/test_handler/test_handler_apt_source_v1.py index 2776739dd2b..2357d69958a 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v1.py @@ -9,6 +9,7 @@ import re import shutil import tempfile +import pathlib from unittest import mock from unittest.mock import call @@ -416,8 +417,9 @@ def test_apt_src_keyonly(self): calls = (call( 'add', - output_file=self.aptlistfile[:-5], - data='fakekey 4242'),) + output_file=pathlib.Path(self.aptlistfile).stem, + data='fakekey 4242', + hardened=False),) mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only @@ -441,14 +443,15 @@ def test_apt_src_keyidonly(self): calls = (call( 'add', - output_file=self.aptlistfile[:-5], - data='fakekey 1212'),) + output_file=pathlib.Path(self.aptlistfile).stem, + data='fakekey 1212', + hardened=False),) mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) - def apt_src_keyid_real(self, cfg, expectedkey): + def apt_src_keyid_real(self, cfg, expectedkey, is_hardened=None): """apt_src_keyid_real Test specification of a keyid without source including up to addition of the key (add_apt_key_raw mocked to keep the @@ -463,9 +466,14 @@ def apt_src_keyid_real(self, cfg, expectedkey): return_value=expectedkey) as mockgetkey: cc_apt_configure.handle("test", cfg, self.fakecloud, None, None) - + if is_hardened is not None: + mockkey.assert_called_with( + expectedkey, + self.aptlistfile, + hardened=is_hardened) + else: + mockkey.assert_called_with(expectedkey, self.aptlistfile) mockgetkey.assert_called_with(key, keyserver) - mockkey.assert_called_with(expectedkey, self.aptlistfile, None) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -476,7 +484,7 @@ def test_apt_src_keyid_real(self): cfg = {'keyid': keyid, 'filename': self.aptlistfile} - self.apt_src_keyid_real(cfg, EXPECTEDKEY) + self.apt_src_keyid_real(cfg, EXPECTEDKEY, is_hardened=False) def test_apt_src_longkeyid_real(self): """test_apt_src_longkeyid_real - Test long keyid including key add""" @@ -484,7 +492,7 @@ def test_apt_src_longkeyid_real(self): cfg = {'keyid': keyid, 'filename': self.aptlistfile} - self.apt_src_keyid_real(cfg, EXPECTEDKEY) + self.apt_src_keyid_real(cfg, EXPECTEDKEY, is_hardened=False) def test_apt_src_longkeyid_ks_real(self): """test_apt_src_longkeyid_ks_real - Test long keyid from other ks""" @@ -493,7 +501,7 @@ def test_apt_src_longkeyid_ks_real(self): 'keyserver': 'keys.gnupg.net', 'filename': self.aptlistfile} - self.apt_src_keyid_real(cfg, EXPECTEDKEY) + self.apt_src_keyid_real(cfg, EXPECTEDKEY, is_hardened=False) def test_apt_src_ppa(self): """Test adding a ppa""" diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py index 11ed090cd2b..202891211bb 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v3.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py @@ -10,6 +10,7 @@ import shutil import socket import tempfile +import pathlib from unittest import TestCase, mock from unittest.mock import call @@ -214,7 +215,7 @@ def test_apt_v3_src_replace_tri(self): self.aptlistfile3: {'source': 'deb $MIRROR $RELEASE universe'}} self._apt_src_replace_tri(cfg) - def _apt_src_keyid(self, filename, cfg, keynum): + def _apt_src_keyid(self, filename, cfg, keynum, is_hardened=None): """_apt_src_keyid Test specification of a source + keyid """ @@ -227,7 +228,10 @@ def _apt_src_keyid(self, filename, cfg, keynum): # check if it added the right number of keys calls = [] for key in cfg: - calls.append(call(cfg[key], TARGET)) + if is_hardened is not None: + calls.append(call(cfg[key], hardened=is_hardened)) + else: + calls.append(call(cfg[key], TARGET)) mockobj.assert_has_calls(calls, any_order=True) @@ -303,8 +307,9 @@ def test_apt_v3_src_key(self): calls = (call( 'add', - output_file=self.aptlistfile[:-5], - data='fakekey 4321'),) + output_file=pathlib.Path(self.aptlistfile).stem, + data='fakekey 4321', + hardened=False),) mockobj.assert_has_calls(calls, any_order=True) self.assertTrue(os.path.isfile(self.aptlistfile)) @@ -327,8 +332,9 @@ def test_apt_v3_src_keyonly(self): calls = (call( 'add', - output_file=self.aptlistfile[:-5], - data='fakekey 4242'),) + output_file=pathlib.Path(self.aptlistfile).stem, + data='fakekey 4242', + hardened=False),) mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only @@ -346,14 +352,15 @@ def test_apt_v3_src_keyidonly(self): calls = (call( 'add', - output_file=self.aptlistfile[:-5], - data='fakekey 1212'),) + output_file=pathlib.Path(self.aptlistfile).stem, + data='fakekey 1212', + hardened=False),) mockobj.assert_has_calls(calls, any_order=True) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) - def apt_src_keyid_real(self, cfg, expectedkey): + def apt_src_keyid_real(self, cfg, expectedkey, is_hardened=None): """apt_src_keyid_real Test specification of a keyid without source including up to addition of the key (add_apt_key_raw mocked to keep the @@ -371,7 +378,11 @@ def apt_src_keyid_real(self, cfg, expectedkey): mockgetkey.assert_called_with(keycfg['keyid'], keycfg.get('keyserver', 'keyserver.ubuntu.com')) - mockkey.assert_called_with(expectedkey, keycfg['keyfile'], TARGET) + if is_hardened is not None: + mockkey.assert_called_with( + expectedkey, + keycfg['keyfile'], + hardened=is_hardened) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -382,7 +393,7 @@ def test_apt_v3_src_keyid_real(self): cfg = {self.aptlistfile: {'keyid': keyid, 'keyfile': self.aptlistfile}} - self.apt_src_keyid_real(cfg, EXPECTEDKEY) + self.apt_src_keyid_real(cfg, EXPECTEDKEY, is_hardened=False) def test_apt_v3_src_longkeyid_real(self): """test_apt_v3_src_longkeyid_real Test long keyid including key add""" @@ -390,7 +401,7 @@ def test_apt_v3_src_longkeyid_real(self): cfg = {self.aptlistfile: {'keyid': keyid, 'keyfile': self.aptlistfile}} - self.apt_src_keyid_real(cfg, EXPECTEDKEY) + self.apt_src_keyid_real(cfg, EXPECTEDKEY, is_hardened=False) def test_apt_v3_src_longkeyid_ks_real(self): """test_apt_v3_src_longkeyid_ks_real Test long keyid from other ks""" @@ -419,7 +430,7 @@ def test_apt_v3_src_keyid_keyserver(self): aa_repo_match=self.matcher) mockgetkey.assert_called_with('03683F77', 'test.random.com') - mockadd.assert_called_with('fakekey', self.aptlistfile, TARGET) + mockadd.assert_called_with('fakekey', self.aptlistfile, hardened=False) # filename should be ignored on key only self.assertFalse(os.path.isfile(self.aptlistfile)) @@ -1029,8 +1040,8 @@ def test_apt_v3_add_mirror_keys(self): 'add_apt_key_raw') as mockadd: cc_apt_configure.add_mirror_keys(cfg, TARGET) calls = [ - mock.call('fakekey_primary', 'primary', TARGET), - mock.call('fakekey_security', 'security', TARGET), + mock.call('fakekey_primary', 'primary', hardened=False), + mock.call('fakekey_security', 'security', hardened=False), ] mockadd.assert_has_calls(calls, any_order=True) From 4371fd19d92861caef19d550b3c41358fd49f085 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 25 Oct 2021 20:19:35 -0600 Subject: [PATCH 11/15] mock out subp --- tests/integration_tests/modules/test_apt.py | 4 ++-- tests/unittests/test_handler/test_handler_apt_key.py | 12 ++++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index 581e87614b8..2c054489b45 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -1,11 +1,11 @@ """Series of integration tests covering apt functionality.""" import re -from tests.integration_tests.clouds import ImageSpecification import pytest + from cloudinit.config import cc_apt_configure from cloudinit import gpg - +from tests.integration_tests.clouds import ImageSpecification from tests.integration_tests.instances import IntegrationInstance diff --git a/tests/unittests/test_handler/test_handler_apt_key.py b/tests/unittests/test_handler/test_handler_apt_key.py index 1f2d6360cc0..3bd1c0830ac 100644 --- a/tests/unittests/test_handler/test_handler_apt_key.py +++ b/tests/unittests/test_handler/test_handler_apt_key.py @@ -123,7 +123,11 @@ def test_apt_key_list_fail_no_keys(self): os, 'listdir', return_value=()): - keys = cc_apt_configure.apt_key('list') + with mock.patch.object( + subp, + 'subp', + return_value=('', '')): + keys = cc_apt_configure.apt_key('list') assert not keys def test_apt_key_list_fail_no_keys_file(self): @@ -131,7 +135,11 @@ def test_apt_key_list_fail_no_keys_file(self): os, 'listdir', return_value=('file_not_gpg_key.txt')): - keys = cc_apt_configure.apt_key('list') + with mock.patch.object( + subp, + 'subp', + return_value=('', '')): + keys = cc_apt_configure.apt_key('list') assert not keys def test_apt_key_list_fail_bad_key_file(self): From 085c6888663f57731e2f98ef7be41ea86ddb64fb Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 26 Oct 2021 11:42:27 -0600 Subject: [PATCH 12/15] add example to docs --- doc/examples/cloud-config-apt.txt | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/doc/examples/cloud-config-apt.txt b/doc/examples/cloud-config-apt.txt index f4392326590..7baa141c4b3 100644 --- a/doc/examples/cloud-config-apt.txt +++ b/doc/examples/cloud-config-apt.txt @@ -149,6 +149,7 @@ apt: # security is optional, if not defined it is set to the same value as primary security: - uri: http://security.ubuntu.com/ubuntu + - arches: [default] # If search_dns is set for security the searched pattern is: # -security-mirror @@ -212,14 +213,14 @@ apt: # # The key of each source entry is the filename and will be prepended by # /etc/apt/sources.list.d/ if it doesn't start with a '/'. - # If it doesn't end with .list it will be appended so that apt picks up it's + # If it doesn't end with .list it will be appended so that apt picks up its # configuration. # # Whenever there is no content to be written into such a file, the key is # not used as filename - yet it can still be used as index for merging # configuration. # - # The values inside the entries consost of the following optional entries: + # The values inside the entries consist of the following optional entries: # 'source': a sources.list entry (some variable replacements apply) # 'keyid': providing a key to import via shortid or fingerprint # 'key': providing a raw PGP key @@ -276,13 +277,14 @@ apt: my-repo2.list: # 2.4 replacement variables # - # sources can use $MIRROR, $PRIMARY, $SECURITY and $RELEASE replacement - # variables. + # sources can use $MIRROR, $PRIMARY, $SECURITY, $RELEASE and $KEY_FILE + # replacement variables. # They will be replaced with the default or specified mirrors and the # running release. # The entry below would be possibly turned into: # source: deb http://archive.ubuntu.com/ubuntu xenial multiverse - source: deb $MIRROR $RELEASE multiverse + source: deb [signed-by=$KEY_FILE] $MIRROR $RELEASE multiverse + keyid: F430BBA5 my-repo3.list: # this would have the same end effect as 'ppa:curtin-dev/test-archive' @@ -310,9 +312,19 @@ apt: keyid: B59D 5F15 97A5 04B7 E230 6DCA 0620 BBCF 0368 3F77 keyserver: pgp.mit.edu + ignored5: + # 2.8 signed-by + # + # One can specify [signed-by=$KEY_FILE] in the source definition, which + # will make the key be installed in the directory /etc/cloud-init.gpg.d/ + # and the $KEY_FILE replacement variable will be replaced with the path + # to the specified key. If $KEY_FILE is used, but no key is specified, + # apt update will (rightfully) fail due to an invalid value. + source: deb [signed-by=$KEY_FILE] $MIRROR $RELEASE multiverse + keyid: B59D 5F15 97A5 04B7 E230 6DCA 0620 BBCF 0368 3F77 my-repo4.list: - # 2.8 raw key + # 2.9 raw key # # The apt signing key can also be specified by providing a pgp public key # block. Providing the PGP key this way is the most robust method for From 60679b1c219149d688a05d4df3d41d047fb393f9 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 27 Oct 2021 09:22:37 -0600 Subject: [PATCH 13/15] update docstring for add_apt_sources --- cloudinit/config/cc_apt_configure.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index c2141ae212c..d0e7ac1a91b 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -762,9 +762,28 @@ def update_packages(cloud): def add_apt_sources(srcdict, cloud, target=None, template_params=None, aa_repo_match=None): """ - add entries in /etc/apt/sources.list.d for each abbreviated - sources.list entry in 'srcdict'. When rendering template, also - include the values in dictionary searchList + install keys and repo source .list files defined in 'sources' + + for each 'source' entry in the config: + 1. expand template variables and write source .list file in + /etc/apt/sources.list.d/ + 2. install defined keys + 3. update packages via distro-specific method (i.e. apt-key update) + + + @param srcdict: a dict containing elements required + @param cloud: cloud instance object + + Example srcdict value: + { + 'rio-grande-repo': { + 'source': 'deb [signed-by=$KEY_FILE] $MIRROR $RELEASE main', + 'keyid': 'B59D 5F15 97A5 04B7 E230 6DCA 0620 BBCF 0368 3F77', + 'keyserver': 'pgp.mit.edu' + } + } + + Note: Deb822 format is not supported """ if template_params is None: template_params = {} From 3e03f92338deb839e704f957c1b588bd51cee96f Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 27 Oct 2021 10:26:14 -0600 Subject: [PATCH 14/15] Update comment about implementing apt-key, revert accidental gpg fingerprint change. --- tests/integration_tests/modules/test_apt.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index 2c054489b45..ee2a572647d 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -106,7 +106,7 @@ class TestApt: def get_keys(self, class_client: IntegrationInstance): """Return all keys in /etc/apt/trusted.gpg.d/ and /etc/apt/trusted.gpg - in human readable format. + in human readable format. Mimics the output of apt-key finger """ list_cmd = ' '.join(gpg.GPG_LIST) + ' ' keys = class_client.execute(list_cmd + cc_apt_configure.APT_LOCAL_KEYS) @@ -207,7 +207,7 @@ def test_key(self, class_client: IntegrationInstance): assert ( 'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu' ) in test_archive_contents - assert TEST_KEY_1 in self.get_keys(class_client) + assert TEST_KEYSERVER_KEY in self.get_keys(class_client) def test_keyserver(self, class_client: IntegrationInstance): """Test the apt keyserver functionality. @@ -223,7 +223,7 @@ def test_keyserver(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu' ) in test_keyserver_contents - assert TEST_KEY_1 in self.get_keys(class_client) + assert TEST_KEYSERVER_KEY in self.get_keys(class_client) def test_os_pipelining(self, class_client: IntegrationInstance): """Test 'os' settings does not write apt config file. From 6ac2fcb9c5c482717fc4ef73c44a16d6efa14a0c Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 28 Oct 2021 15:53:50 -0600 Subject: [PATCH 15/15] Address latest review comments: ------------------------------- - remove unnecessary base classes - use mock decorators - increase the value density of tests Additionally: ------------- - add comments to new tests to explain/justify purpose - add second integration test for failure mode - eliminated unnecessary ValueError exception from gpg.dearmor(): - empty value will lead to to ProcessExecutionError when subp.subp() gets called, and that's a more appropriate error handling path anyways (will trigger if user sets key: "") - idk I'm sure I forgot something --- cloudinit/config/cc_apt_configure.py | 4 +- cloudinit/gpg.py | 8 +- tests/integration_tests/modules/test_apt.py | 28 ++-- tests/unittests/test_gpg.py | 60 +++++--- .../test_handler/test_handler_apt_key.py | 144 ++++++++---------- 5 files changed, 123 insertions(+), 121 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index d0e7ac1a91b..c3c48bbd919 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -1158,8 +1158,8 @@ def apt_key_add(): """ file_name = '/dev/null' if not output_file: - util.logexc(LOG, 'Unknown filename, failed to add key: {}'.format( - data)) + util.logexc( + LOG, 'Unknown filename, failed to add key: "{}"'.format(data)) else: try: key_dir = \ diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py index 0bec241bae4..07d682d2fe8 100644 --- a/cloudinit/gpg.py +++ b/cloudinit/gpg.py @@ -35,12 +35,7 @@ def dearmor(key): note: man gpg(1) makes no mention of an --armour spelling, only --armor """ - - if not key: - raise ValueError("invalid attempt to dearmor key") - (stdout, _) = subp.subp(["gpg", "--dearmor"], data=key, decode=False, - capture=True) - return stdout + return subp.subp(["gpg", "--dearmor"], data=key, decode=False)[0] def list(key_file, human_output=False): @@ -50,7 +45,6 @@ def list(key_file, human_output=False): @param key_file: a string containing a filepath to a key @param human_output: return output intended for human parsing """ - cmd = [] cmd.extend(GPG_LIST) if not human_output: diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py index ee2a572647d..2c388047e9b 100644 --- a/tests/integration_tests/modules/test_apt.py +++ b/tests/integration_tests/modules/test_apt.py @@ -46,9 +46,12 @@ keyserver: keyserver.ubuntu.com source: "ppa:simplestreams-dev/trunk" test_signed_by: - keyid: 3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85 + keyid: A2EB2DEC0BD7519B7B38BE38376A290EC8068B11 keyserver: keyserver.ubuntu.com - source: "deb [signed-by=$KEY_FILE] http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu $RELEASE main" + source: "deb [signed-by=$KEY_FILE] http://ppa.launchpad.net/juju/stable/ubuntu $RELEASE main" + test_bad_key: + key: "" + source: "deb $MIRROR $RELEASE main" test_key: source: "deb http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu $RELEASE main" key: | @@ -96,8 +99,8 @@ TEST_PPA_KEY = "3552 C902 B4DD F7BD 3842 1821 015D 28D7 4416 14D8" -TEST_KEY_1 = "1FF0 D853 5EF7 E719 E5C8 1B9C 083D 06FB E4D3 04DF" -TEST_KEY_2 = "3A3E F34D FDED B3B7 F3FD F603 F83F 7712 9A5E BD85" +TEST_KEY = "1FF0 D853 5EF7 E719 E5C8 1B9C 083D 06FB E4D3 04DF" +TEST_SIGNED_BY_KEY = "A2EB 2DEC 0BD7 519B 7B38 BE38 376A 290E C806 8B11" @pytest.mark.ci @@ -173,7 +176,7 @@ def test_ppa_source(self, class_client: IntegrationInstance): 'http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu' ) in ppa_path_contents - assert TEST_KEY_1 in self.get_keys(class_client) + assert TEST_PPA_KEY in self.get_keys(class_client) def test_signed_by(self, class_client: IntegrationInstance): """Test the apt signed-by functionality. @@ -181,9 +184,9 @@ def test_signed_by(self, class_client: IntegrationInstance): release = ImageSpecification.from_os_image().release source = ( "deb [signed-by=/etc/apt/cloud-init.gpg.d/test_signed_by.gpg] " - "http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu" + "http://ppa.launchpad.net/juju/stable/ubuntu" " {} main".format(release)) - + print(class_client.execute('cat /var/log/cloud-init.log')) path_contents = class_client.read_from_file( '/etc/apt/sources.list.d/test_signed_by.list') assert path_contents == source @@ -192,7 +195,14 @@ def test_signed_by(self, class_client: IntegrationInstance): 'gpg --no-default-keyring --with-fingerprint --list-keys ' '--keyring /etc/apt/cloud-init.gpg.d/test_signed_by.gpg') - assert TEST_KEY_2 in key + assert TEST_SIGNED_BY_KEY in key + + def test_bad_key(self, class_client: IntegrationInstance): + """Test the apt signed-by functionality. + """ + with pytest.raises(OSError): + class_client.read_from_file( + '/etc/apt/trusted.list.d/test_bad_key.gpg') def test_key(self, class_client: IntegrationInstance): """Test the apt key functionality. @@ -207,7 +217,7 @@ def test_key(self, class_client: IntegrationInstance): assert ( 'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu' ) in test_archive_contents - assert TEST_KEYSERVER_KEY in self.get_keys(class_client) + assert TEST_KEY in self.get_keys(class_client) def test_keyserver(self, class_client: IntegrationInstance): """Test the apt keyserver functionality. diff --git a/tests/unittests/test_gpg.py b/tests/unittests/test_gpg.py index 0277efa0c1c..451ffa91761 100644 --- a/tests/unittests/test_gpg.py +++ b/tests/unittests/test_gpg.py @@ -1,6 +1,6 @@ +import pytest from unittest import mock -from cloudinit.tests import helpers from cloudinit import gpg from cloudinit import subp @@ -33,33 +33,49 @@ '3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85' -class TestGPGCommands(helpers.CiTestCase): - def test_dearmor_empty_value(self): - with self.assertRaises(ValueError): - gpg.dearmor(None) - +class TestGPGCommands: def test_dearmor_bad_value(self): + """This exception is handled by the callee. Ensure it is not caught + internally. + """ with mock.patch.object( subp, 'subp', side_effect=subp.ProcessExecutionError): - with self.assertRaises(subp.ProcessExecutionError): + with pytest.raises(subp.ProcessExecutionError): gpg.dearmor('garbage key value') - def list_output_helper(self, fingerprint, key): - with mock.patch.object(subp, 'subp', return_value=(key, '')): - assert fingerprint in gpg.list('/path/to/key.gpg') - - def test_list_human_output(self): - self.list_output_helper(TEST_KEY_FINGERPRINT_HUMAN, TEST_KEY_HUMAN) + def test_gpg_list_args(self): + """Verify correct command gets called to list keys + """ + no_colons = [ + 'gpg', + '--with-fingerprint', + '--no-default-keyring', + '--list-keys', + '--keyring', + 'key'] + colons = [ + 'gpg', + '--with-fingerprint', + '--no-default-keyring', + '--list-keys', + '--keyring', + '--with-colons', + 'key'] + with mock.patch.object(subp, 'subp', return_value=('', '')) as m_subp: + gpg.list('key') + assert mock.call(colons, capture=True) == m_subp.call_args - def test_list_machine_output(self): - self.list_output_helper(TEST_KEY_FINGERPRINT_MACHINE, TEST_KEY_MACHINE) + gpg.list('key', human_output=True) + test_calls = mock.call((no_colons), capture=True) + assert test_calls == m_subp.call_args - def test_list_bad_value(self): - with mock.patch.object( - subp, - 'subp', - side_effect=subp.ProcessExecutionError): - with self.assertRaises(subp.ProcessExecutionError): - gpg.list('/path/to/bad/key.gpg') + def test_gpg_dearmor_args(self): + """Verify correct command gets called to dearmor keys + """ + with mock.patch.object(subp, 'subp', return_value=('', '')) as m_subp: + gpg.dearmor('key') + test_call = mock.call( + ["gpg", "--dearmor"], data='key', decode=False) + assert test_call == m_subp.call_args diff --git a/tests/unittests/test_handler/test_handler_apt_key.py b/tests/unittests/test_handler/test_handler_apt_key.py index 3bd1c0830ac..00e5a38d278 100644 --- a/tests/unittests/test_handler/test_handler_apt_key.py +++ b/tests/unittests/test_handler/test_handler_apt_key.py @@ -1,9 +1,6 @@ import os -import shutil -import tempfile from unittest import mock -from cloudinit.tests import helpers from cloudinit.config import cc_apt_configure from cloudinit import subp from cloudinit import util @@ -37,119 +34,104 @@ '3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85' -class TestAptKey(helpers.FilesystemMockingTestCase): +class TestAptKey: """TestAptKey Class to test apt-key commands """ - def setUp(self): - super(TestAptKey, self).setUp() - self.tmp = tempfile.mkdtemp() - self.new_root = tempfile.mkdtemp() - self.addCleanup(shutil.rmtree, self.tmp) - self.addCleanup(shutil.rmtree, self.new_root) - - def _apt_key_add_success_helper(self, directory, hardened=False): - with mock.patch.object( - subp, - 'subp', - return_value=('fakekey', '')): - with mock.patch.object( - util, - 'write_file'): - file = cc_apt_configure.apt_key( - 'add', - output_file='my-key', - data='fakekey', - hardened=hardened) + @mock.patch.object(subp, 'subp', return_value=('fakekey', '')) + @mock.patch.object(util, 'write_file') + def _apt_key_add_success_helper(self, directory, *args, hardened=False): + file = cc_apt_configure.apt_key( + 'add', + output_file='my-key', + data='fakekey', + hardened=hardened) assert file == directory + '/my-key.gpg' def test_apt_key_add_success(self): + """Verify the correct directory path gets returned for unhardened case + """ self._apt_key_add_success_helper('/etc/apt/trusted.gpg.d') def test_apt_key_add_success_hardened(self): + """Verify the correct directory path gets returned for hardened case + """ self._apt_key_add_success_helper( '/etc/apt/cloud-init.gpg.d', hardened=True) def test_apt_key_add_fail_no_file_name(self): + """Verify that null filename gets handled correctly + """ file = cc_apt_configure.apt_key( 'add', output_file=None, data='') assert '/dev/null' == file - def _apt_key_fail_helper(self, exception): - with mock.patch.object( - subp, - 'subp', - side_effect=exception): - file = cc_apt_configure.apt_key( - 'add', - output_file='my-key', - data='fakekey') + def _apt_key_fail_helper(self): + file = cc_apt_configure.apt_key( + 'add', + output_file='my-key', + data='fakekey') assert file == '/dev/null' - def test_apt_key_add_fail_no_file_name_subproc(self): - self._apt_key_fail_helper(subp.ProcessExecutionError) + @mock.patch.object(subp, 'subp', side_effect=subp.ProcessExecutionError) + def test_apt_key_add_fail_no_file_name_subproc(self, *args): + """Verify that bad key value gets handled correctly + """ + self._apt_key_fail_helper() - def test_apt_key_add_fail_no_file_name_unicode(self): - self._apt_key_fail_helper(UnicodeDecodeError('test', b'', 1, 1, '')) + @mock.patch.object( + subp, 'subp', side_effect=UnicodeDecodeError('test', b'', 1, 1, '')) + def test_apt_key_add_fail_no_file_name_unicode(self, *args): + """Verify that bad key encoding gets handled correctly + """ + self._apt_key_fail_helper() def _apt_key_list_success_helper(self, finger, key, human_output=True): - with mock.patch.object( - subp, - 'subp', - return_value=(key, '')): - - with mock.patch.object( - os, - 'listdir', - return_value=('/fake/dir/key.gpg',)): - keys = cc_apt_configure.apt_key('list', human_output) - assert finger in keys + @mock.patch.object(os, 'listdir', return_value=('/fake/dir/key.gpg',)) + @mock.patch.object(subp, 'subp', return_value=(key, '')) + def mocked_list(*a): + + keys = cc_apt_configure.apt_key('list', human_output) + assert finger in keys + mocked_list() def test_apt_key_list_success_human(self): + """Verify expected key output, human + """ self._apt_key_list_success_helper( TEST_KEY_FINGERPRINT_HUMAN, TEST_KEY_HUMAN) def test_apt_key_list_success_machine(self): + """Verify expected key output, machine + """ self._apt_key_list_success_helper( TEST_KEY_FINGERPRINT_MACHINE, TEST_KEY_MACHINE, human_output=False) - def test_apt_key_list_fail_no_keys(self): - with mock.patch.object( - os, - 'listdir', - return_value=()): - with mock.patch.object( - subp, - 'subp', - return_value=('', '')): - keys = cc_apt_configure.apt_key('list') - assert not keys - - def test_apt_key_list_fail_no_keys_file(self): - with mock.patch.object( - os, - 'listdir', - return_value=('file_not_gpg_key.txt')): - with mock.patch.object( - subp, - 'subp', - return_value=('', '')): - keys = cc_apt_configure.apt_key('list') + @mock.patch.object(os, 'listdir', return_value=()) + @mock.patch.object(subp, 'subp', return_value=('', '')) + def test_apt_key_list_fail_no_keys(self, *args): + """Ensure falsy output for no keys + """ + keys = cc_apt_configure.apt_key('list') assert not keys - def test_apt_key_list_fail_bad_key_file(self): - with mock.patch.object( - subp, - 'subp', - side_effect=subp.ProcessExecutionError): - with mock.patch.object( - os, - 'listdir', - return_value=('bad_gpg_key.gpg')): - keys = cc_apt_configure.apt_key('list') - assert not keys + @mock.patch.object(os, 'listdir', return_value=('file_not_gpg_key.txt')) + @mock.patch.object(subp, 'subp', return_value=('', '')) + def test_apt_key_list_fail_no_keys_file(self, *args): + """Ensure non-gpg file is not returned. + + apt-key used file extensions for this, so we do too + """ + assert not cc_apt_configure.apt_key('list') + + @mock.patch.object(subp, 'subp', side_effect=subp.ProcessExecutionError) + @mock.patch.object(os, 'listdir', return_value=('bad_gpg_key.gpg')) + def test_apt_key_list_fail_bad_key_file(self, *args): + """Ensure bad gpg key doesn't throw exeption. + """ + assert not cc_apt_configure.apt_key('list')