fix(pushapk): Actually check apk signatures

pushapkscript/docker.d/worker.yml

@@ -180,6 +180,9 @@ products:
           skip_check_multiple_locales: true
           skip_check_same_locales: true
           skip_checks_fennec: true
+          # There is no dep signing for mozillavpn yet, so init_worker.sh imports no
+          # certificate and there is no alias to verify against.
+          skip_check_signature: true
           override_channel_model: "single_google_app"
           app:
             package_names:

pushapkscript/src/pushapkscript/jarsigner.py

@@ -1,7 +1,7 @@
 import logging
 import subprocess
 
-from pushapkscript.exceptions import SignatureError
+from pushapkscript.exceptions import ConfigValidationError, SignatureError
 
 log = logging.getLogger(__name__)
 
@@ -36,4 +36,11 @@ def _pluck_configuration(context, publish_config):
     # Uses jarsigner in PATH if config doesn't provide it
     binary_path = context.config.get("jarsigner_binary", "jarsigner")
 
-    return binary_path, keystore_path, publish_config["certificate_alias"]
+    certificate_alias = publish_config.get("certificate_alias")
+    if not certificate_alias:
+        raise ConfigValidationError(
+            'No "certificate_alias" is configured for the "{}" target store, so the signature cannot be verified. Either configure one or set '
+            '"skip_check_signature" on the product.'.format(publish_config.get("target_store"))
+        )
+
+    return binary_path, keystore_path, certificate_alias

pushapkscript/src/pushapkscript/publish_config.py

@@ -77,6 +77,7 @@ def _get_channel_publish_config(product_config, task):
         return {
             "target_store": target_store,
             "dry_run": _should_do_dry_run(task),
+            "certificate_alias": publish_config.get("certificate_alias"),
             "package_names": publish_config["package_names"],
             "rollout_percentage": rollout_percentage,
             "sgs_service_account_id": store_config["service_account_id"],

pushapkscript/src/pushapkscript/script.py

@@ -36,7 +36,10 @@ async def async_main(context):
         # Google Play won't accept both APK and AAB
         raise TaskVerificationError("The configuration is invalid: Unable to push both APK and AAB files for the same product.")
 
-    if not publish_config.get("skip_check_signature", True):
+    # `skip_check_signature` is a product-level option, and omitting it means "do verify".
+    if product_config.get("skip_check_signature", False):
+        log.warning('This product is configured with "skip_check_signature", so the signing of the files will not be verified.')
+    else:
         log.info("Verifying APKs' signatures...")
         for apk_path in all_apks_paths:
             jarsigner.verify(context, publish_config, apk_path)
@@ -45,8 +48,6 @@ async def async_main(context):
         for aab_path in all_aabs_paths:
             jarsigner.verify(context, publish_config, aab_path)
             manifest.verify(product_config, aab_path)
-    else:
-        log.info('This product is configured with "skip_check_signature", so the signing of the files will not be verified.')
 
     log.info("Delegating publication to mozapkpublisher...")
     with contextlib.ExitStack() as stack:

pushapkscript/tests/integration/test_integration_script.py

@@ -5,42 +5,114 @@
 import subprocess
 import tempfile
 import unittest
+from zipfile import ZipFile
+
 import pytest
 
-import pushapkscript
 from pushapkscript.script import main
 
 from ..helpers.mock_file import MockFile, mock_open
 from ..helpers.task_generator import TaskGenerator
 
 this_dir = os.path.dirname(os.path.realpath(__file__))
-project_dir = os.path.dirname(pushapkscript.__file__)
-project_data_dir = os.path.join(project_dir, "data")
 test_data_dir = os.path.join(this_dir, "..", "data")
 
+# The fixtures are re-signed with this digest, so the product configs have to expect it.
+DIGEST_ALGORITHM = "SHA-256"
+
+
+def _has_working_jdk():
+    # The binaries have to be run, not just located: macOS ships stubs at /usr/bin that
+    # exist but exit non-zero with "Unable to locate a Java Runtime".
+    for binary in ("keytool", "jarsigner"):
+        try:
+            if subprocess.run([binary, "-help"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL).returncode != 0:
+                return False
+        except OSError:
+            return False
+    return True
+
+
+requires_jdk = pytest.mark.skipif(
+    not _has_working_jdk(),
+    reason="signing and verifying the test APKs needs keytool and jarsigner from a JDK",
+)
+
+
+def _strip_v1_signature(apk_path):
+    # jarsigner adds to whatever signature is already present, and the fixtures ship a
+    # stale one that lists entries no longer in the archive, so drop META-INF first.
+    with ZipFile(apk_path) as original:
+        entries = [(info, original.read(info.filename)) for info in original.infolist() if not info.filename.startswith("META-INF/")]
+
+    with ZipFile(apk_path, "w") as stripped:
+        for info, data in entries:
+            stripped.writestr(info, data)
+
 
 class KeystoreManager(object):
+    STORE_PASSWORD = "12345678"
+
     def __init__(self, temp_dir):
         self.keystore_path = os.path.join(temp_dir, "keystore")
 
     def add_certificate(self, certificate_alias):
+        # A freshly generated keypair rather than the bundled android-nightly.cer. That
+        # certificate is SHA1withDSA with a 1024-bit key, and modern JDKs disable both, so
+        # jarsigner reports anything signed with it as unsigned and `-strict` exits 16.
         subprocess.run(
             [
                 "keytool",
-                "-import",
+                "-genkeypair",
                 "-noprompt",
                 # JDK 9 changes default type to PKCS12, which causes "jarsigner -verify" to fail
                 "-storetype",
                 "jks",
                 "-keystore",
                 self.keystore_path,
                 "-storepass",
-                "12345678",
-                "-file",
-                os.path.join(project_data_dir, "android-nightly.cer"),
+                self.STORE_PASSWORD,
+                "-keypass",
+                self.STORE_PASSWORD,
                 "-alias",
                 certificate_alias,
-            ]
+                "-keyalg",
+                "RSA",
+                "-keysize",
+                "2048",
+                "-sigalg",
+                "SHA256withRSA",
+                "-dname",
+                "CN=pushapkscript integration test",
+                "-validity",
+                "3650",
+            ],
+            check=True,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+        )
+
+    def sign(self, apk_path, certificate_alias):
+        _strip_v1_signature(apk_path)
+        subprocess.run(
+            [
+                "jarsigner",
+                "-keystore",
+                self.keystore_path,
+                "-storepass",
+                self.STORE_PASSWORD,
+                "-keypass",
+                self.STORE_PASSWORD,
+                "-digestalg",
+                DIGEST_ALGORITHM,
+                "-sigalg",
+                "SHA256withRSA",
+                apk_path,
+                certificate_alias,
+            ],
+            check=True,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
         )
 
 
@@ -69,7 +141,7 @@ def generate_fennec_config(self):
                 "products": [
                     {
                         "product_names": ["aurora", "beta", "release"],
-                        "digest_algorithm": "SHA1",
+                        "digest_algorithm": DIGEST_ALGORITHM,
                         "override_channel_model": "choose_google_app_with_scope",
                         "apps": {
                             "aurora": {
@@ -108,7 +180,7 @@ def generate_focus_config(self):
                 "products": [
                     {
                         "product_names": ["focus"],
-                        "digest_algorithm": "SHA1",
+                        "digest_algorithm": DIGEST_ALGORITHM,
                         "skip_check_ordered_version_codes": True,
                         "skip_checks_fennec": True,
                         "override_channel_model": "single_google_app",
@@ -134,7 +206,7 @@ def generate_fenix_config(self):
                 "products": [
                     {
                         "product_names": ["fenix"],
-                        "digest_algorithm": "SHA1",
+                        "digest_algorithm": DIGEST_ALGORITHM,
                         "skip_check_multiple_locales": True,
                         "skip_check_same_locales": True,
                         "skip_checks_fennec": True,
@@ -175,6 +247,7 @@ def generate_fenix_config(self):
         )
 
 
+@requires_jdk
 @unittest.mock.patch("pushapkscript.script.open", new=mock_open)
 @unittest.mock.patch("pushapkscript.publish.open", new=mock_open)
 class MainTest(unittest.TestCase):
@@ -196,15 +269,26 @@ def tearDown(self):
         self.test_temp_dir_fp.cleanup()
 
     def _copy_all_apks_to_test_temp_dir(self, task_generator):
-        for task_id in (task_generator.x86_task_id, task_generator.arm_task_id):
+        return [
             self._copy_single_file_to_test_temp_dir(task_id, origin_file_name="target-{}.apk".format(task_id), destination_path="public/build/target.apk")
+            for task_id in (task_generator.x86_task_id, task_generator.arm_task_id)
+        ]
 
     def _copy_single_file_to_test_temp_dir(self, task_id, origin_file_name, destination_path):
         original_path = os.path.join(test_data_dir, origin_file_name)
         target_path = os.path.abspath(os.path.join(self.test_temp_dir, "work", "cot", task_id, destination_path))
         target_dir = os.path.dirname(target_path)
         os.makedirs(target_dir)
         shutil.copy(original_path, target_path)
+        return target_path
+
+    def _prepare_apks(self, task_generator, certificate_alias):
+        """Copy the fixtures in and sign them with `certificate_alias`, which is the alias
+        the product config under test expects, so signature verification passes."""
+        apk_paths = self._copy_all_apks_to_test_temp_dir(task_generator)
+        self.keystore_manager.add_certificate(certificate_alias)
+        for apk_path in apk_paths:
+            self.keystore_manager.sign(apk_path, certificate_alias)
 
     def write_task_file(self, task):
         task_file = os.path.join(self.config_generator.work_dir, "task.json")
@@ -216,8 +300,7 @@ def test_main_fennec_style(self, push_apk):
         task_generator = TaskGenerator()
         self.write_task_file(task_generator.generate_task("aurora"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("nightly")
+        self._prepare_apks(task_generator, "nightly")
         main(config_path=self.config_generator.generate_fennec_config())
 
         push_apk.assert_called_with(
@@ -246,8 +329,7 @@ def test_main_focus_style(self, push_apk):
         task_generator = TaskGenerator()
         self.write_task_file(task_generator.generate_task("focus", "production"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("focus")
+        self._prepare_apks(task_generator, "focus")
         main(config_path=self.config_generator.generate_focus_config())
 
         push_apk.assert_called_with(
@@ -276,8 +358,7 @@ def test_main_fenix_style(self, push_apk):
         task_generator = TaskGenerator()
         self.write_task_file(task_generator.generate_task("fenix", "nightly"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("fenix-nightly")
+        self._prepare_apks(task_generator, "fenix-nightly")
         main(config_path=self.config_generator.generate_fenix_config())
 
         push_apk.assert_called_with(
@@ -306,8 +387,7 @@ def test_main_downloads_verifies_signature_and_gives_the_right_config_to_mozapkp
         task_generator = TaskGenerator()
         self.write_task_file(task_generator.generate_task("aurora"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("nightly")
+        self._prepare_apks(task_generator, "nightly")
         main(config_path=self.config_generator.generate_fennec_config())
 
         push_apk.assert_called_with(
@@ -336,8 +416,7 @@ def test_main_allows_rollout_percentage(self, push_apk):
         task_generator = TaskGenerator(rollout_percentage=25)
         self.write_task_file(task_generator.generate_task("aurora"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("nightly")
+        self._prepare_apks(task_generator, "nightly")
         main(config_path=self.config_generator.generate_fennec_config())
 
         push_apk.assert_called_with(
@@ -367,8 +446,7 @@ def test_main_allows_commit_transaction(self, push_apk):
 
         self.write_task_file(task_generator.generate_task("aurora"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("nightly")
+        self._prepare_apks(task_generator, "nightly")
         main(config_path=self.config_generator.generate_fennec_config())
 
         push_apk.assert_called_with(
@@ -398,8 +476,7 @@ def test_main_with_samsung_store(self, push_apk):
 
         self.write_task_file(task_generator.generate_task("fenix", channel="release"))
 
-        self._copy_all_apks_to_test_temp_dir(task_generator)
-        self.keystore_manager.add_certificate("nightly")
+        self._prepare_apks(task_generator, "fenix-production")
         main(config_path=self.config_generator.generate_fenix_config())
 
         push_apk.assert_called_with(

pushapkscript/tests/test_jarsigner.py

@@ -3,7 +3,7 @@
 from unittest.mock import MagicMock, patch
 
 from pushapkscript import jarsigner
-from pushapkscript.exceptions import SignatureError
+from pushapkscript.exceptions import ConfigValidationError, SignatureError
 
 
 class JarSignerTest(unittest.TestCase):
@@ -68,3 +68,15 @@ def test_pluck_configuration_sets_every_argument(self):
 
     def test_pluck_configuration_uses_defaults(self):
         self.assertEqual(jarsigner._pluck_configuration(self.minimal_context, {"certificate_alias": "nightly"}), ("jarsigner", "/path/to/keystore", "nightly"))
+
+    def test_pluck_configuration_raises_when_no_alias_is_configured(self):
+        for publish_config in ({"target_store": "samsung"}, {"target_store": "google", "certificate_alias": None}):
+            with self.assertRaises(ConfigValidationError):
+                jarsigner._pluck_configuration(self.context, publish_config)
+
+    def test_verify_raises_before_running_jarsigner_when_no_alias_is_configured(self):
+        with patch("subprocess.run") as run:
+            with self.assertRaises(ConfigValidationError):
+                jarsigner.verify(self.context, {"target_store": "samsung"}, "/path/to/apk")
+
+            run.assert_not_called()

pushapkscript/tests/test_publish_config.py

@@ -150,6 +150,7 @@ def test_target_samsung():
     assert get_publish_config(FENIX_CONFIG, payload, "fenix") == {
         "target_store": "samsung",
         "dry_run": True,
+        "certificate_alias": "fenix",
         "sgs_service_account_id": "123456",
         "sgs_access_token": "abcdef",
         "package_names": ["org.mozilla.fenix"],
@@ -164,6 +165,7 @@ def test_target_samsung_with_commit():
     assert get_publish_config(FENIX_CONFIG, payload, "fenix") == {
         "target_store": "samsung",
         "dry_run": False,
+        "certificate_alias": "fenix",
         "sgs_service_account_id": "123456",
         "sgs_access_token": "abcdef",
         "package_names": ["org.mozilla.fenix"],
@@ -178,6 +180,7 @@ def test_target_samsung_rollout():
     assert get_publish_config(FENIX_CONFIG, payload, "fenix") == {
         "target_store": "samsung",
         "dry_run": True,
+        "certificate_alias": "fenix",
         "sgs_service_account_id": "123456",
         "sgs_access_token": "abcdef",
         "package_names": ["org.mozilla.fenix"],
@@ -192,6 +195,7 @@ def test_target_samsung_submit():
     assert get_publish_config(FENIX_CONFIG, payload, "fenix") == {
         "target_store": "samsung",
         "dry_run": True,
+        "certificate_alias": "fenix",
         "sgs_service_account_id": "123456",
         "sgs_access_token": "abcdef",
         "package_names": ["org.mozilla.fenix"],
@@ -200,6 +204,24 @@ def test_target_samsung_submit():
     }
 
 
+def test_certificate_alias_does_not_depend_on_the_target_store():
+    # The alias identifies the certificate the incoming artifact was signed with, which is
+    # decided by the upstream signing task, so it is the same whichever store it goes to.
+    google = get_publish_config(FENIX_CONFIG, {"channel": "production", "target_store": "google"}, "fenix")
+    samsung = get_publish_config(FENIX_CONFIG, {"channel": "production", "target_store": "samsung"}, "fenix")
+
+    assert google["certificate_alias"] == "fenix"
+    assert samsung["certificate_alias"] == "fenix"
+
+
+def test_certificate_alias_is_none_when_nothing_configures_it():
+    config = {"apps": {"production": {"package_names": ["org.mozilla.fenix"], "samsung": {"service_account_id": "1", "access_token": "2"}}}}
+
+    publish_config = get_publish_config(config, {"channel": "production", "target_store": "samsung"}, "fenix")
+
+    assert publish_config["certificate_alias"] is None
+
+
 def test_should_do_dry_run():
     task_payload = {"commit": True}
     assert _should_do_dry_run(task_payload) is False