From 6228162cbda19a8d5093f1d06e08a30c2fc33ad6 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 7 Dec 2017 17:32:14 +0100 Subject: [PATCH] handle jarsigner/apksigner output cleanly for rational logging These were both spamming the output with lots of confusing messages, even when --verbose was not used. Jarsigner especially has confusing messages, since it has warnings that do not pertain to APK signatures at all, like the ones about timestamps and missing Certificate Authority. closes #405 --- .gitignore | 1 + fdroidserver/common.py | 49 +++++++++++++++++++++++++++++++++++------- fdroidserver/index.py | 1 + tests/common.TestCase | 45 ++++++++++++++++++++++++++++++++++++++ tests/index.TestCase | 12 ----------- 5 files changed, 88 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index dda5ae97..b92ef773 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ tmp/ makebuildserver.config.py /tests/.fdroid.keypass.txt /tests/.fdroid.keystorepass.txt +/tests/.java.security /tests/fdroid-icon.png /tests/OBBMainOldVersion.apk /tests/OBBMainPatchCurrent.apk diff --git a/fdroidserver/common.py b/fdroidserver/common.py index 6511181f..5d35f3c7 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -2544,8 +2544,16 @@ def verify_jar_signature(jar): """ - if subprocess.call([config['jarsigner'], '-strict', '-verify', jar]) != 4: - raise VerificationException(_("The repository's index could not be verified.")) + error = _('JAR signature failed to verify: {path}').format(path=jar) + try: + output = subprocess.check_output([config['jarsigner'], '-strict', '-verify', jar], + stderr=subprocess.STDOUT) + raise VerificationException(error + '\n' + output.decode('utf-8')) + except subprocess.CalledProcessError as e: + if e.returncode == 4: + logging.debug(_('JAR signature verified: {path}').format(path=jar)) + else: + raise VerificationException(error + '\n' + e.output.decode('utf-8')) def verify_apk_signature(apk, min_sdk_version=None): @@ -2561,14 +2569,24 @@ def verify_apk_signature(apk, min_sdk_version=None): args = [config['apksigner'], 'verify'] if min_sdk_version: args += ['--min-sdk-version=' + min_sdk_version] - return subprocess.call(args + [apk]) == 0 + if options.verbose: + args += ['--verbose'] + try: + output = subprocess.check_output(args + [apk]) + if options.verbose: + logging.debug(apk + ': ' + output.decode('utf-8')) + return True + except subprocess.CalledProcessError as e: + logging.error('\n' + apk + ': ' + e.output.decode('utf-8')) else: - logging.warning("Using Java's jarsigner, not recommended for verifying APKs! Use apksigner") + if not config.get('jarsigner_warning_displayed'): + config['jarsigner_warning_displayed'] = True + logging.warning(_("Using Java's jarsigner, not recommended for verifying APKs! Use apksigner")) try: verify_jar_signature(apk) return True - except Exception: - pass + except Exception as e: + logging.error(e) return False @@ -2589,8 +2607,23 @@ def verify_old_apk_signature(apk): with open(_java_security, 'w') as fp: fp.write('jdk.jar.disabledAlgorithms=MD2, RSA keySize < 1024') - return subprocess.call([config['jarsigner'], '-J-Djava.security.properties=' + _java_security, - '-strict', '-verify', apk]) == 4 + try: + cmd = [ + config['jarsigner'], + '-J-Djava.security.properties=' + _java_security, + '-strict', '-verify', apk + ] + output = subprocess.check_output(cmd, stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: + if e.returncode != 4: + output = e.output + else: + logging.debug(_('JAR signature verified: {path}').format(path=apk)) + return True + + logging.error(_('Old APK signature failed to verify: {path}').format(path=apk) + + '\n' + output.decode('utf-8')) + return False apk_badchars = re.compile('''[/ :;'"]''') diff --git a/fdroidserver/index.py b/fdroidserver/index.py index d013b319..82b5ff33 100644 --- a/fdroidserver/index.py +++ b/fdroidserver/index.py @@ -691,6 +691,7 @@ def download_repo_index(url_str, etag=None, verify_fingerprint=True): jar = zipfile.ZipFile(fp) # verify that the JAR signature is valid + logging.debug(_('Verifying index signature:')) common.verify_jar_signature(fp.name) # get public key and its fingerprint from JAR diff --git a/tests/common.TestCase b/tests/common.TestCase index 9f4b3ada..a6cc9d87 100755 --- a/tests/common.TestCase +++ b/tests/common.TestCase @@ -22,6 +22,7 @@ print('localmodule: ' + localmodule) if localmodule not in sys.path: sys.path.insert(0, localmodule) +import fdroidserver.index import fdroidserver.signindex import fdroidserver.common import fdroidserver.metadata @@ -275,12 +276,56 @@ class CommonTest(unittest.TestCase): config['jarsigner'] = fdroidserver.common.find_sdk_tools_cmd('jarsigner') fdroidserver.common.config = config + self.assertTrue(fdroidserver.common.verify_apk_signature('bad-unicode-πÇÇ现代通用字-български-عربي1.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('org.bitbucket.tickytacky.mirrormirror_1.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('org.bitbucket.tickytacky.mirrormirror_2.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('org.bitbucket.tickytacky.mirrormirror_3.apk')) + self.assertFalse(fdroidserver.common.verify_apk_signature('org.bitbucket.tickytacky.mirrormirror_4.apk')) + self.assertTrue(fdroidserver.common.verify_apk_signature('org.dyndns.fules.ck_20.apk')) self.assertTrue(fdroidserver.common.verify_apk_signature('urzip.apk')) self.assertFalse(fdroidserver.common.verify_apk_signature('urzip-badcert.apk')) self.assertFalse(fdroidserver.common.verify_apk_signature('urzip-badsig.apk')) self.assertTrue(fdroidserver.common.verify_apk_signature('urzip-release.apk')) self.assertFalse(fdroidserver.common.verify_apk_signature('urzip-release-unsigned.apk')) + def test_verify_old_apk_signature(self): + fdroidserver.common.config = None + config = fdroidserver.common.read_config(fdroidserver.common.options) + config['jarsigner'] = fdroidserver.common.find_sdk_tools_cmd('jarsigner') + fdroidserver.common.config = config + + self.assertTrue(fdroidserver.common.verify_old_apk_signature('bad-unicode-πÇÇ现代通用字-български-عربي1.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('org.bitbucket.tickytacky.mirrormirror_1.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('org.bitbucket.tickytacky.mirrormirror_2.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('org.bitbucket.tickytacky.mirrormirror_3.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('org.bitbucket.tickytacky.mirrormirror_4.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('org.dyndns.fules.ck_20.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('urzip.apk')) + self.assertFalse(fdroidserver.common.verify_old_apk_signature('urzip-badcert.apk')) + self.assertFalse(fdroidserver.common.verify_old_apk_signature('urzip-badsig.apk')) + self.assertTrue(fdroidserver.common.verify_old_apk_signature('urzip-release.apk')) + self.assertFalse(fdroidserver.common.verify_old_apk_signature('urzip-release-unsigned.apk')) + + def test_verify_jar_signature_succeeds(self): + fdroidserver.common.config = None + config = fdroidserver.common.read_config(fdroidserver.common.options) + config['jarsigner'] = fdroidserver.common.find_sdk_tools_cmd('jarsigner') + fdroidserver.common.config = config + source_dir = os.path.join(self.basedir, 'signindex') + for f in ('testy.jar', 'guardianproject.jar'): + testfile = os.path.join(source_dir, f) + fdroidserver.common.verify_jar_signature(testfile) + + def test_verify_jar_signature_fails(self): + fdroidserver.common.config = None + config = fdroidserver.common.read_config(fdroidserver.common.options) + config['jarsigner'] = fdroidserver.common.find_sdk_tools_cmd('jarsigner') + fdroidserver.common.config = config + source_dir = os.path.join(self.basedir, 'signindex') + testfile = os.path.join(source_dir, 'unsigned.jar') + with self.assertRaises(fdroidserver.index.VerificationException): + fdroidserver.common.verify_jar_signature(testfile) + def test_verify_apks(self): fdroidserver.common.config = None config = fdroidserver.common.read_config(fdroidserver.common.options) diff --git a/tests/index.TestCase b/tests/index.TestCase index 671370c3..1fd89f7e 100755 --- a/tests/index.TestCase +++ b/tests/index.TestCase @@ -45,18 +45,6 @@ class IndexTest(unittest.TestCase): fdroidserver.common.config = config fdroidserver.signindex.config = config - def test_verify_jar_signature_succeeds(self): - source_dir = os.path.join(self.basedir, 'signindex') - for f in ('testy.jar', 'guardianproject.jar'): - testfile = os.path.join(source_dir, f) - fdroidserver.common.verify_jar_signature(testfile) - - def test_verify_jar_signature_fails(self): - source_dir = os.path.join(self.basedir, 'signindex') - testfile = os.path.join(source_dir, 'unsigned.jar') - with self.assertRaises(fdroidserver.index.VerificationException): - fdroidserver.common.verify_jar_signature(testfile) - def test_get_public_key_from_jar_succeeds(self): source_dir = os.path.join(self.basedir, 'signindex') for f in ('testy.jar', 'guardianproject.jar'):