diff --git a/pushapkscript/docker.d/worker.yml b/pushapkscript/docker.d/worker.yml index 80da70915..c3ddde76d 100644 --- a/pushapkscript/docker.d/worker.yml +++ b/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: diff --git a/pushapkscript/src/pushapkscript/jarsigner.py b/pushapkscript/src/pushapkscript/jarsigner.py index adf0a321e..50d559b68 100644 --- a/pushapkscript/src/pushapkscript/jarsigner.py +++ b/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 diff --git a/pushapkscript/src/pushapkscript/publish_config.py b/pushapkscript/src/pushapkscript/publish_config.py index 3905bee77..91eea87f4 100644 --- a/pushapkscript/src/pushapkscript/publish_config.py +++ b/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"], diff --git a/pushapkscript/src/pushapkscript/script.py b/pushapkscript/src/pushapkscript/script.py index 0a2db40b5..dd6d0a9a5 100755 --- a/pushapkscript/src/pushapkscript/script.py +++ b/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: diff --git a/pushapkscript/tests/integration/test_integration_script.py b/pushapkscript/tests/integration/test_integration_script.py index 068f03fe8..0625787d8 100644 --- a/pushapkscript/tests/integration/test_integration_script.py +++ b/pushapkscript/tests/integration/test_integration_script.py @@ -5,29 +5,65 @@ 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", @@ -35,12 +71,48 @@ def add_certificate(self, certificate_alias): "-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,8 +269,10 @@ 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) @@ -205,6 +280,15 @@ def _copy_single_file_to_test_temp_dir(self, task_id, origin_file_name, destinat 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( diff --git a/pushapkscript/tests/test_jarsigner.py b/pushapkscript/tests/test_jarsigner.py index 9c66ec7f8..128be6ecf 100644 --- a/pushapkscript/tests/test_jarsigner.py +++ b/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() diff --git a/pushapkscript/tests/test_publish_config.py b/pushapkscript/tests/test_publish_config.py index 6b590c594..8255d3f54 100644 --- a/pushapkscript/tests/test_publish_config.py +++ b/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 diff --git a/pushapkscript/tests/test_script.py b/pushapkscript/tests/test_script.py index 059c65ae4..06e8f5922 100644 --- a/pushapkscript/tests/test_script.py +++ b/pushapkscript/tests/test_script.py @@ -74,6 +74,55 @@ async def assert_google_play_call_apk(_, __, all_apks_files, ___): await async_main(context) +@pytest.mark.asyncio +@pytest.mark.parametrize( + "product_config_extras, expected_verifications", + ( + # Omitting the option means the signature is verified. + ({}, 2), + ({"skip_check_signature": False}, 2), + ({"skip_check_signature": True}, 0), + ), +) +async def test_async_main_honours_skip_check_signature(monkeypatch, product_config_extras, expected_verifications): + apks = ["/some/path/to/one.apk", "/some/path/to/another.apk"] + jarsigner_calls = [] + manifest_calls = [] + + monkeypatch.setattr(artifacts, "get_upstream_artifacts_full_paths_per_task_id", lambda _: ({"someTaskId": apks}, {})) + monkeypatch.setattr(jarsigner, "verify", lambda _, __, apk_path: jarsigner_calls.append(apk_path)) + monkeypatch.setattr(manifest, "verify", lambda _, apk_path: manifest_calls.append(apk_path)) + monkeypatch.setattr(task, "extract_android_product_from_scopes", lambda _: "fenix") + + product_config = { + "apps": { + "release": { + "package_names": ["org.mozilla.fenix"], + "certificate_alias": "fenix-release", + "google": {"default_track": "production", "credentials_file": "fenix.json"}, + } + } + } + product_config.update(product_config_extras) + monkeypatch.setattr(pushapkscript.script, "_get_product_config", lambda _, __: product_config) + + async def noop(*args, **kwargs): + pass + + monkeypatch.setattr(publish, "publish", noop) + monkeypatch.setattr(publish, "publish_aab", noop) + + context = MagicMock() + context.config = {"do_not_contact_server": True} + context.task = {"payload": {"channel": "release"}} + + with patch("pushapkscript.script.open", new=mock_open): + await async_main(context) + + assert len(jarsigner_calls) == expected_verifications + assert len(manifest_calls) == expected_verifications + + def test_get_product_config_validation(): context = Context() context.config = {} diff --git a/taskcluster/docker/pushapkscript/Dockerfile b/taskcluster/docker/pushapkscript/Dockerfile index a02a342a5..8479be4bf 100644 --- a/taskcluster/docker/pushapkscript/Dockerfile +++ b/taskcluster/docker/pushapkscript/Dockerfile @@ -8,8 +8,10 @@ FROM $DOCKER_IMAGE_PARENT ADD --chown=app:app topsrcdir/pushapkscript /app/pushapkscript USER root +# jarsigner verifies the signature of the APKs/AABs before they are published, and it +# ships in the JDK rather than the JRE, so this needs default-jdk-headless. RUN apt-get update \ - && apt-get install --no-install-recommends -y curl default-jre-headless \ + && apt-get install --no-install-recommends -y curl default-jdk-headless \ && apt-get clean USER app diff --git a/taskcluster/kinds/docker-image/kind.yml b/taskcluster/kinds/docker-image/kind.yml index f587a61f7..2c09a19dc 100644 --- a/taskcluster/kinds/docker-image/kind.yml +++ b/taskcluster/kinds/docker-image/kind.yml @@ -96,7 +96,8 @@ tasks: args: PYTHON_VERSION: *py314 UV_VERSION: *uv_version - APT_PACKAGES: default-jre-headless + # default-jdk-headless, not the JRE: the tests shell out to jarsigner. + APT_PACKAGES: default-jdk-headless pushflatpakscript-test-py314: definition: base-test