From ca86c18e3351863ec25bf0448b05d3ed078777d9 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 24 Aug 2020 14:49:22 +0200 Subject: [PATCH 1/6] publish: reformat --- fdroidserver/publish.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fdroidserver/publish.py b/fdroidserver/publish.py index 425cba43..b766f273 100644 --- a/fdroidserver/publish.py +++ b/fdroidserver/publish.py @@ -156,14 +156,14 @@ def status_update_json(newKeyAliases, generatedKeys, signedApks): def main(): - global config, options # Parse command line... parser = ArgumentParser(usage="%(prog)s [options] " - "[APPID[:VERCODE] [APPID[:VERCODE] ...]]") + "[APPID[:VERCODE] [APPID[:VERCODE] ...]]") common.setup_global_opts(parser) - parser.add_argument("appid", nargs='*', help=_("applicationId with optional versionCode in the form APPID[:VERCODE]")) + parser.add_argument("appid", nargs='*', + help=_("applicationId with optional versionCode in the form APPID[:VERCODE]")) metadata.add_metadata_arguments(parser) options = parser.parse_args() metadata.warnings_action = options.W From eaca3d5faac78b9946563fa6cc1cac1affeb5955 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 24 Aug 2020 16:33:53 +0200 Subject: [PATCH 2/6] publish: better json reporting * newKeyAliases wasn't providing any useful information * generatedKeys now contains the used keyalias as well * signedApks now also records the used keyalias for each apk --- fdroidserver/publish.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/fdroidserver/publish.py b/fdroidserver/publish.py index b766f273..43a7fbaf 100644 --- a/fdroidserver/publish.py +++ b/fdroidserver/publish.py @@ -141,13 +141,11 @@ def store_stats_fdroid_signing_key_fingerprints(appids, indent=None): sign_sig_key_fingerprint_list(jar_file) -def status_update_json(newKeyAliases, generatedKeys, signedApks): +def status_update_json(generatedKeys, signedApks): """Output a JSON file with metadata about this run""" logging.debug(_('Outputting JSON')) output = common.setup_status_output(start_timestamp) - if newKeyAliases: - output['newKeyAliases'] = newKeyAliases if generatedKeys: output['generatedKeys'] = generatedKeys if signedApks: @@ -364,9 +362,7 @@ def main(): p = FDroidPopen(cmd, envs=env_vars) if p.returncode != 0: raise BuildException("Failed to generate key", p.output) - if appid not in generated_keys: - generated_keys[appid] = set() - generated_keys[appid].add(appid) + generated_keys[appid] = keyalias signed_apk_path = os.path.join(output_dir, apkfilename) if os.path.exists(signed_apk_path): @@ -379,13 +375,14 @@ def main(): common.sign_apk(apkfile, signed_apk_path, keyalias) if appid not in signed_apks: signed_apks[appid] = [] - signed_apks[appid].append(apkfile) + signed_apks[appid].append({"keyalias": keyalias, + "filename": apkfile}) publish_source_tarball(apkfilename, unsigned_dir, output_dir) logging.info('Published ' + apkfilename) store_stats_fdroid_signing_key_fingerprints(allapps.keys()) - status_update_json(new_key_aliases, generated_keys, signed_apks) + status_update_json(generated_keys, signed_apks) logging.info('published list signing-key fingerprints') From 7813a17cf8ad52dc856d19d3708dc93aaefa5cae Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 24 Aug 2020 16:35:50 +0200 Subject: [PATCH 3/6] publish: extract a few functions out of main publish is currently not reusable from other modules as everything is happening in main. It's also not testable from python unittests. There's already a function for getting the key_alias, so we can use that. Introduce tests for the split out functions. --- fdroidserver/publish.py | 67 ++++++++++++++++++----------------------- tests/publish.TestCase | 28 +++++++++++++++++ 2 files changed, 57 insertions(+), 38 deletions(-) diff --git a/fdroidserver/publish.py b/fdroidserver/publish.py index 43a7fbaf..8a6a3e81 100644 --- a/fdroidserver/publish.py +++ b/fdroidserver/publish.py @@ -153,6 +153,33 @@ def status_update_json(generatedKeys, signedApks): common.write_status_json(output) +def check_for_key_collisions(allapps): + """ + Make sure there's no collision in keyaliases from apps. + It was suggested at + https://dev.guardianproject.info/projects/bazaar/wiki/FDroid_Audit + that a package could be crafted, such that it would use the same signing + key as an existing app. While it may be theoretically possible for such a + colliding package ID to be generated, it seems virtually impossible that + the colliding ID would be something that would be a) a valid package ID, + and b) a sane-looking ID that would make its way into the repo. + Nonetheless, to be sure, before publishing we check that there are no + collisions, and refuse to do any publishing if that's the case... + :param allapps a dict of all apps to process + :return: a list of all aliases corresponding to allapps + """ + allaliases = [] + for appid in allapps: + m = hashlib.md5() # nosec just used to generate a keyalias + m.update(appid.encode('utf-8')) + keyalias = m.hexdigest()[:8] + if keyalias in allaliases: + logging.error(_("There is a keyalias collision - publishing halted")) + sys.exit(1) + allaliases.append(keyalias) + return allaliases + + def main(): global config, options @@ -199,29 +226,11 @@ def main(): logging.error("Config error - missing '{0}'".format(config['keystore'])) sys.exit(1) - # It was suggested at - # https://dev.guardianproject.info/projects/bazaar/wiki/FDroid_Audit - # that a package could be crafted, such that it would use the same signing - # key as an existing app. While it may be theoretically possible for such a - # colliding package ID to be generated, it seems virtually impossible that - # the colliding ID would be something that would be a) a valid package ID, - # and b) a sane-looking ID that would make its way into the repo. - # Nonetheless, to be sure, before publishing we check that there are no - # collisions, and refuse to do any publishing if that's the case... allapps = metadata.read_metadata() vercodes = common.read_pkg_args(options.appid, True) signed_apks = dict() - new_key_aliases = [] generated_keys = dict() - allaliases = [] - for appid in allapps: - m = hashlib.md5() # nosec just used to generate a keyalias - m.update(appid.encode('utf-8')) - keyalias = m.hexdigest()[:8] - if keyalias in allaliases: - logging.error(_("There is a keyalias collision - publishing halted")) - sys.exit(1) - allaliases.append(keyalias) + allaliases = check_for_key_collisions(allapps) logging.info(ngettext('{0} app, {1} key aliases', '{0} apps, {1} key aliases', len(allapps)).format(len(allapps), len(allaliases))) @@ -313,26 +322,8 @@ def main(): skipsigning = True # Now we sign with the F-Droid key. - - # Figure out the key alias name we'll use. Only the first 8 - # characters are significant, so we'll use the first 8 from - # the MD5 of the app's ID and hope there are no collisions. - # If a collision does occur later, we're going to have to - # come up with a new algorithm, AND rename all existing keys - # in the keystore! if not skipsigning: - if appid in config['keyaliases']: - # For this particular app, the key alias is overridden... - keyalias = config['keyaliases'][appid] - if keyalias.startswith('@'): - m = hashlib.md5() # nosec just used to generate a keyalias - m.update(keyalias[1:].encode('utf-8')) - keyalias = m.hexdigest()[:8] - else: - m = hashlib.md5() # nosec just used to generate a keyalias - m.update(appid.encode('utf-8')) - keyalias = m.hexdigest()[:8] - new_key_aliases.append(keyalias) + keyalias = key_alias(appid) logging.info("Key alias: " + keyalias) # See if we already have a key for this application, and diff --git a/tests/publish.TestCase b/tests/publish.TestCase index 88cdc48a..41863482 100755 --- a/tests/publish.TestCase +++ b/tests/publish.TestCase @@ -50,6 +50,9 @@ class PublishTest(unittest.TestCase): self.assertEqual('dc3b169e', publish.key_alias('org.test.testy')) self.assertEqual('78688a0f', publish.key_alias('org.org.org')) + self.assertEqual('ee8807d2', publish.key_alias("org.schabi.newpipe")) + self.assertEqual('b53c7e11', publish.key_alias("de.grobox.liberario")) + publish.config = {'keyaliases': {'yep.app': '@org.org.org', 'com.example.app': '1a2b3c4d'}} self.assertEqual('78688a0f', publish.key_alias('yep.app')) @@ -162,6 +165,31 @@ class PublishTest(unittest.TestCase): with mock.patch.object(sys, 'argv', ['fdroid fakesubcommand']): publish.main() + def test_check_for_key_collisions(self): + from fdroidserver.metadata import App + common.config = {} + common.fill_config_defaults(common.config) + publish.config = common.config + + # We cannot really test the negative case here as there doesn't seem + # to be a md5 collision with text input around. + # So we just test we don't trigger an exception here and get back the same number + # of aliases as we put in apps. + randomappids = [ + "org.fdroid.fdroid", + "a.b.c", + "u.v.w.x.y.z", + "lpzpkgqwyevnmzvrlaazhgardbyiyoybyicpmifkyrxkobljoz", + "vuslsm.jlrevavz.qnbsenmizhur.lprwbjiujtu.ekiho", + "w.g.g.w.p.v.f.v.gvhyz", + "nlozuqer.ufiinmrbjqboogsjgmpfks.dywtpcpnyssjmqz", + ] + allapps = {} + for appid in randomappids: + allapps[appid] = App() + allaliases = publish.check_for_key_collisions(allapps) + self.assertEqual(len(randomappids), len(allaliases)) + if __name__ == "__main__": os.chdir(os.path.dirname(__file__)) From a114c73c2d4f7f0117407e9fca43a2d5b221dd70 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 24 Aug 2020 19:29:57 +0200 Subject: [PATCH 4/6] publish: factor out the signing key creation into a method --- fdroidserver/publish.py | 65 ++++++++++++++++++++++++----------------- tests/publish.TestCase | 27 +++++++++++++++++ 2 files changed, 65 insertions(+), 27 deletions(-) diff --git a/fdroidserver/publish.py b/fdroidserver/publish.py index 8a6a3e81..50492fe2 100644 --- a/fdroidserver/publish.py +++ b/fdroidserver/publish.py @@ -180,6 +180,43 @@ def check_for_key_collisions(allapps): return allaliases +def create_key_if_not_existing(keyalias): + """ + Ensures a signing key with the given keyalias exists + :return: boolean, True if a new key was created, false otherwise + """ + # See if we already have a key for this application, and + # if not generate one... + env_vars = {'LC_ALL': 'C.UTF-8', + 'FDROID_KEY_STORE_PASS': config['keystorepass'], + 'FDROID_KEY_PASS': config.get('keypass', "")} + cmd = [config['keytool'], '-list', + '-alias', keyalias, '-keystore', config['keystore'], + '-storepass:env', 'FDROID_KEY_STORE_PASS'] + if config['keystore'] == 'NONE': + cmd += config['smartcardoptions'] + p = FDroidPopen(cmd, envs=env_vars) + if p.returncode != 0: + logging.info("Key does not exist - generating...") + cmd = [config['keytool'], '-genkey', + '-keystore', config['keystore'], + '-alias', keyalias, + '-keyalg', 'RSA', '-keysize', '2048', + '-validity', '10000', + '-storepass:env', 'FDROID_KEY_STORE_PASS', + '-dname', config['keydname']] + if config['keystore'] == 'NONE': + cmd += config['smartcardoptions'] + else: + cmd += '-keypass:env', 'FDROID_KEY_PASS' + p = FDroidPopen(cmd, envs=env_vars) + if p.returncode != 0: + raise BuildException("Failed to generate key", p.output) + return True + else: + return False + + def main(): global config, options @@ -326,33 +363,7 @@ def main(): keyalias = key_alias(appid) logging.info("Key alias: " + keyalias) - # See if we already have a key for this application, and - # if not generate one... - env_vars = {'LC_ALL': 'C.UTF-8', - 'FDROID_KEY_STORE_PASS': config['keystorepass'], - 'FDROID_KEY_PASS': config.get('keypass', "")} - cmd = [config['keytool'], '-list', - '-alias', keyalias, '-keystore', config['keystore'], - '-storepass:env', 'FDROID_KEY_STORE_PASS'] - if config['keystore'] == 'NONE': - cmd += config['smartcardoptions'] - p = FDroidPopen(cmd, envs=env_vars) - if p.returncode != 0: - logging.info("Key does not exist - generating...") - cmd = [config['keytool'], '-genkey', - '-keystore', config['keystore'], - '-alias', keyalias, - '-keyalg', 'RSA', '-keysize', '2048', - '-validity', '10000', - '-storepass:env', 'FDROID_KEY_STORE_PASS', - '-dname', config['keydname']] - if config['keystore'] == 'NONE': - cmd += config['smartcardoptions'] - else: - cmd += '-keypass:env', 'FDROID_KEY_PASS' - p = FDroidPopen(cmd, envs=env_vars) - if p.returncode != 0: - raise BuildException("Failed to generate key", p.output) + if create_key_if_not_existing(keyalias): generated_keys[appid] = keyalias signed_apk_path = os.path.join(output_dir, apkfilename) diff --git a/tests/publish.TestCase b/tests/publish.TestCase index 41863482..acd2996f 100755 --- a/tests/publish.TestCase +++ b/tests/publish.TestCase @@ -11,6 +11,7 @@ # import inspect +import jks, jks.util import logging import optparse import os @@ -190,6 +191,32 @@ class PublishTest(unittest.TestCase): allaliases = publish.check_for_key_collisions(allapps) self.assertEqual(len(randomappids), len(allaliases)) + def test_create_key_if_not_existing(self): + common.config = {} + common.fill_config_defaults(common.config) + publish.config = common.config + publish.config['keystorepass'] = '123456' + publish.config['keypass'] = '654321' + publish.config['keystore'] = "keystore.jks" + publish.config['keydname'] = 'CN=Birdman, OU=Cell, O=Alcatraz, L=Alcatraz, S=California, C=US' + testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir) + os.chdir(testdir) + keystore = jks.KeyStore.new("jks", []) + keystore.save(publish.config['keystore'], publish.config['keystorepass']) + + self.assertTrue(publish.create_key_if_not_existing("newalias")) + # The second time we try that, a new key should not be created + self.assertFalse(publish.create_key_if_not_existing("newalias")) + self.assertTrue(publish.create_key_if_not_existing("anotheralias")) + + keystore = jks.KeyStore.load(publish.config['keystore'], publish.config['keystorepass']) + self.assertCountEqual(keystore.private_keys, ["newalias", "anotheralias"]) + for alias, pk in keystore.private_keys.items(): + self.assertFalse(pk.is_decrypted()) + pk.decrypt(publish.config['keypass']) + self.assertTrue(pk.is_decrypted()) + self.assertEqual(jks.util.RSA_ENCRYPTION_OID, pk.algorithm_oid) + if __name__ == "__main__": os.chdir(os.path.dirname(__file__)) From d9a6bfb0a9de7904ef1cf836cd09890e1e846a04 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 24 Aug 2020 19:56:08 +0200 Subject: [PATCH 5/6] CI: install pyjks as dependency for tests --- .gitlab-ci.yml | 2 +- setup.py | 4 +++- tests/publish.TestCase | 6 +++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a6480604..0de8ffaf 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -6,7 +6,7 @@ variables: test: image: registry.gitlab.com/fdroid/ci-images-server:latest script: - - $pip install -e . + - $pip install -e .[test] - cd tests - ./complete-ci-tests diff --git a/setup.py b/setup.py index bade76bb..04baa9eb 100755 --- a/setup.py +++ b/setup.py @@ -52,7 +52,6 @@ def get_data_files(): with open("README.md", "r") as fh: long_description = fh.read() - setup(name='fdroidserver', version='1.2a', description='F-Droid Server Tools', @@ -88,6 +87,9 @@ setup(name='fdroidserver', 'requests >= 2.5.2, != 2.11.0, != 2.12.2, != 2.18.0', 'yamllint', ], + extras_require={ + 'test': ['pyjks'], + }, classifiers=[ 'Development Status :: 4 - Beta', 'Intended Audience :: Developers', diff --git a/tests/publish.TestCase b/tests/publish.TestCase index acd2996f..300ca422 100755 --- a/tests/publish.TestCase +++ b/tests/publish.TestCase @@ -11,7 +11,6 @@ # import inspect -import jks, jks.util import logging import optparse import os @@ -192,6 +191,11 @@ class PublishTest(unittest.TestCase): self.assertEqual(len(randomappids), len(allaliases)) def test_create_key_if_not_existing(self): + try: + import jks + import jks.util + except ImportError: + self.skipTest("pyjks not installed") common.config = {} common.fill_config_defaults(common.config) publish.config = common.config From 882f8cfe19fe7660ec759c32bf46cbff1cf63551 Mon Sep 17 00:00:00 2001 From: Marcus Hoffmann Date: Mon, 24 Aug 2020 21:19:59 +0200 Subject: [PATCH 6/6] test_check_for_key_collisions: test with an actual collision Genrated with this script: https://gitlab.com/fdroid/fdroidserver/-/merge_requests/787#note_401275883 --- tests/publish.TestCase | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/publish.TestCase b/tests/publish.TestCase index 300ca422..38691f85 100755 --- a/tests/publish.TestCase +++ b/tests/publish.TestCase @@ -171,10 +171,6 @@ class PublishTest(unittest.TestCase): common.fill_config_defaults(common.config) publish.config = common.config - # We cannot really test the negative case here as there doesn't seem - # to be a md5 collision with text input around. - # So we just test we don't trigger an exception here and get back the same number - # of aliases as we put in apps. randomappids = [ "org.fdroid.fdroid", "a.b.c", @@ -190,6 +186,10 @@ class PublishTest(unittest.TestCase): allaliases = publish.check_for_key_collisions(allapps) self.assertEqual(len(randomappids), len(allaliases)) + allapps = {'tof.cv.mpp': App(), 'j6mX276h': App()} + self.assertEqual(publish.key_alias('tof.cv.mpp'), publish.key_alias('j6mX276h')) + self.assertRaises(SystemExit, publish.check_for_key_collisions, allapps) + def test_create_key_if_not_existing(self): try: import jks