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

@@ -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

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('''[/ :;'"]''')

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

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)

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'):