Merge branch 'cert-pin' into 'master'

`AllowedAPKSigningKeys` cleanups

See merge request fdroid/fdroidserver!1343

fdroidserver/build.py

@@ -826,6 +826,15 @@ def force_halt_build(timeout):
     vm.destroy()
 
 
+def keep_when_not_allowed():
+    """Control if APKs signed by keys not in AllowedAPKSigningKeys are removed."""
+    return (
+        (options is not None and options.keep_when_not_allowed)
+        or (config is not None and config.get('keep_when_not_allowed'))
+        or common.default_config['keep_when_not_allowed']
+    )
+
+
 def parse_commandline():
     """Parse the command line.
 
@@ -863,6 +872,8 @@ def parse_commandline():
                         help=_("Force build of disabled apps, and carries on regardless of scan problems. Only allowed in test mode."))
     parser.add_argument("-a", "--all", action="store_true", default=False,
                         help=_("Build all applications available"))
+    parser.add_argument("--keep-when-not-allowed", default=False, action="store_true",
+                        help=argparse.SUPPRESS)
     parser.add_argument("-w", "--wiki", default=False, action="store_true",
                         help=argparse.SUPPRESS)
     metadata.add_metadata_arguments(parser)
@@ -1112,6 +1123,34 @@ def main():
                                              'supplied reference binary '
                                              'successfully')
 
+                            used_key = common.apk_signer_fingerprint(of)
+                            expected_keys = app['AllowedAPKSigningKeys']
+                            if used_key is None:
+                                logging.warn(_('reference binary missing '
+                                               'signature'))
+                            elif len(expected_keys) == 0:
+                                logging.warn(_('AllowedAPKSigningKeys missing '
+                                               'but reference binary supplied'))
+                            elif used_key not in expected_keys:
+                                if options.test or keep_when_not_allowed():
+                                    logging.warning(_('Keeping failed build "{apkfilename}"')
+                                                    .format(apkfilename=unsigned_apk))
+                                else:
+                                    logging.debug('removing %s', unsigned_apk)
+                                    os.remove(unsigned_apk)
+                                logging.debug('removing %s', of)
+                                os.remove(of)
+                                raise FDroidException('supplied reference '
+                                                      'binary signed with '
+                                                      '{signer} instead of '
+                                                      'with {expected}'.
+                                                      format(signer=used_key,
+                                                             expected=expected_keys))
+                            else:
+                                logging.info(_('supplied reference binary has '
+                                               'allowed signer {signer}').
+                                             format(signer=used_key))
+
                     build_succeeded_ids.append([app['id'], build.versionCode])
 
                     if not options.onserver:

fdroidserver/common.py

@@ -128,6 +128,7 @@ default_config = {
     'gradle': os.path.join(FDROID_PATH, 'gradlew-fdroid'),
     'sync_from_local_copy_dir': False,
     'allow_disabled_algorithms': False,
+    'keep_when_not_allowed': False,
     'per_app_repos': False,
     'make_current_version_link': False,
     'current_version_name_source': 'Name',

fdroidserver/lint.py

@@ -696,6 +696,25 @@ def check_updates_ucm_http_aum_pattern(app):  # noqa: D403
         yield _("AutoUpdateMode with UpdateCheckMode: HTTP must have a pattern.")
 
 
+def check_certificate_pinned_binaries(app):
+    if len(app.get('AllowedAPKSigningKeys')) > 0:
+        return
+    if app.get('Binaries') is not None:
+        yield _(
+            'App has Binaries but does not have corresponding AllowedAPKSigningKeys to pin certificate.'
+        )
+        return
+    builds = app.get('Builds')
+    if builds is None:
+        return
+    for build in builds:
+        if build.get('binary') is not None:
+            yield _(
+                'App version has binary but does not have corresponding AllowedAPKSigningKeys to pin certificate.'
+            )
+            return
+
+
 def main():
     global config, options
 
@@ -803,6 +822,7 @@ def main():
             check_current_version_code,
             check_updates_expected,
             check_updates_ucm_http_aum_pattern,
+            check_certificate_pinned_binaries,
         ]
 
         for check_func in app_check_funcs:

tests/build.TestCase

@@ -40,6 +40,10 @@ class FakeProcess:
         print('FakeFDroidPopen', args, kwargs)
 
 
+class Options:
+    keep_when_not_allowed = False
+
+
 class BuildTest(unittest.TestCase):
     '''fdroidserver/build.py'''
 
@@ -51,6 +55,7 @@ class BuildTest(unittest.TestCase):
         os.chdir(self.basedir)
         fdroidserver.common.config = None
         fdroidserver.build.config = None
+        fdroidserver.build.options = None
         self._td = mkdtemp()
         self.testdir = self._td.name
 
@@ -604,9 +609,9 @@ class BuildTest(unittest.TestCase):
         sdk_path = os.path.join(self.testdir, 'android-sdk')
         self.create_fake_android_home(sdk_path)
         with open('config.yml', 'w') as fp:
-            yaml.dump({'sdk_path': sdk_path}, fp)
+            yaml.dump({'sdk_path': sdk_path, 'keep_when_not_allowed': True}, fp)
         os.chmod('config.yml', 0o600)
-        fdroidserver.common.build = fdroidserver.common.read_config()
+        fdroidserver.build.config = fdroidserver.common.read_config()
 
         os.mkdir('metadata')
         appid = 'info.guardianproject.checkey'
@@ -696,6 +701,139 @@ class BuildTest(unittest.TestCase):
                 self.assertFalse(os.path.exists(production_result))
                 self.assertFalse(os.path.exists(production_compare_file))
 
+    def test_failed_allowedapksigningkeys_are_not_in_unsigned(self):
+        os.chdir(self.testdir)
+        os.mkdir('metadata')
+        appid = 'info.guardianproject.checkey'
+        metadata_file = os.path.join('metadata', appid + '.yml')
+        shutil.copy(os.path.join(self.basedir, metadata_file), 'metadata')
+        with open(metadata_file) as fp:
+            app = fdroidserver.metadata.App(yaml.safe_load(fp))
+        app['RepoType'] = 'git'
+        app[
+            'Binaries'
+        ] = 'https://example.com/fdroid/repo/info.guardianproject.checkey_%v.apk'
+        build = fdroidserver.metadata.Build(
+            {
+                'versionCode': 123,
+                'versionName': '1.2.3',
+                'commit': '1.2.3',
+                'disable': False,
+            }
+        )
+        app['Builds'] = [build]
+        expected_key = 'a' * 64
+        bogus_key = 'b' * 64
+        app['AllowedAPKSigningKeys'] = [expected_key]
+        fdroidserver.metadata.write_metadata(metadata_file, app)
+
+        os.makedirs(os.path.join('unsigned', 'binaries'))
+        production_result = os.path.join(
+            'unsigned', '%s_%d.apk' % (appid, build['versionCode'])
+        )
+        production_compare_file = os.path.join(
+            'unsigned', 'binaries', '%s_%d.binary.apk' % (appid, build['versionCode'])
+        )
+        os.makedirs(os.path.join('tmp', 'binaries'))
+        test_result = os.path.join('tmp', '%s_%d.apk' % (appid, build['versionCode']))
+        test_compare_file = os.path.join(
+            'tmp', 'binaries', '%s_%d.binary.apk' % (appid, build['versionCode'])
+        )
+        with mock.patch(
+            'fdroidserver.common.force_exit', lambda *args: None
+        ) as a, mock.patch(
+            'fdroidserver.common.get_android_tools_version_log', lambda: 'fake'
+        ) as b, mock.patch(
+            'fdroidserver.common.FDroidPopen', FakeProcess
+        ) as c, mock.patch(
+            'fdroidserver.build.FDroidPopen', FakeProcess
+        ) as d, mock.patch(
+            'fdroidserver.build.trybuild', lambda *args: True
+        ) as e, mock.patch(
+            'fdroidserver.net.download_file', lambda *args, **kwargs: None
+        ) as f:
+            a, b, c, d, e, f  # silence linters' "unused" warnings
+
+            with mock.patch('sys.argv', ['fdroid build', appid]):
+                # successful comparison, successful signer
+                open(production_result, 'w').close()
+                open(production_compare_file, 'w').close()
+                with mock.patch(
+                    'fdroidserver.common.verify_apks', lambda *args: None
+                ) as g, mock.patch(
+                    'fdroidserver.common.apk_signer_fingerprint',
+                    lambda *args: expected_key,
+                ) as h:
+                    g, h
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(production_result))
+                self.assertTrue(os.path.exists(production_compare_file))
+                # successful comparison, failed signer
+                open(production_result, 'w').close()
+                open(production_compare_file, 'w').close()
+                with mock.patch(
+                    'fdroidserver.common.verify_apks', lambda *args: None
+                ) as g, mock.patch(
+                    'fdroidserver.common.apk_signer_fingerprint',
+                    lambda *args: bogus_key,
+                ) as h:
+                    g, h
+                    fdroidserver.build.main()
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+                # failed comparison
+                open(production_result, 'w').close()
+                open(production_compare_file, 'w').close()
+                with mock.patch(
+                    'fdroidserver.common.verify_apks', lambda *args: 'failed'
+                ):
+                    fdroidserver.build.main()
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+
+            with mock.patch('sys.argv', ['fdroid build', '--test', appid]):
+                # successful comparison, successful signer
+                open(test_result, 'w').close()
+                open(test_compare_file, 'w').close()
+                with mock.patch(
+                    'fdroidserver.common.verify_apks', lambda *args: None
+                ) as g, mock.patch(
+                    'fdroidserver.common.apk_signer_fingerprint',
+                    lambda *args: expected_key,
+                ) as h:
+                    g, h
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(test_result))
+                self.assertTrue(os.path.exists(test_compare_file))
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+                # successful comparison, failed signer
+                open(test_result, 'w').close()
+                open(test_compare_file, 'w').close()
+                with mock.patch(
+                    'fdroidserver.common.verify_apks', lambda *args: None
+                ) as g, mock.patch(
+                    'fdroidserver.common.apk_signer_fingerprint',
+                    lambda *args: bogus_key,
+                ) as h:
+                    g, h
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(test_result))
+                self.assertFalse(os.path.exists(test_compare_file))
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+                # failed comparison
+                open(test_result, 'w').close()
+                open(test_compare_file, 'w').close()
+                with mock.patch(
+                    'fdroidserver.common.verify_apks', lambda *args: 'failed'
+                ):
+                    fdroidserver.build.main()
+                self.assertTrue(os.path.exists(test_result))
+                self.assertFalse(os.path.exists(test_compare_file))
+                self.assertFalse(os.path.exists(production_result))
+                self.assertFalse(os.path.exists(production_compare_file))
+
     @mock.patch('fdroidserver.vmtools.get_build_vm')
     @mock.patch('fdroidserver.vmtools.get_clean_builder')
     @mock.patch('paramiko.SSHClient')
@@ -780,6 +918,51 @@ class BuildTest(unittest.TestCase):
         test_flag = ('--skip-scan', True)
         fdroidserver.build.build_server(app, build, vcs, '', '', '', False)
 
+    def test_keep_when_not_allowed_default(self):
+        self.assertFalse(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_config_true(self):
+        fdroidserver.build.config = {'keep_when_not_allowed': True}
+        self.assertTrue(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_config_false(self):
+        fdroidserver.build.config = {'keep_when_not_allowed': False}
+        self.assertFalse(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_options_true(self):
+        fdroidserver.build.options = Options
+        fdroidserver.build.options.keep_when_not_allowed = True
+        self.assertTrue(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_options_false(self):
+        fdroidserver.build.options = Options
+        fdroidserver.build.options.keep_when_not_allowed = False
+        self.assertFalse(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_options_true_override_config(self):
+        fdroidserver.build.options = Options
+        fdroidserver.build.options.keep_when_not_allowed = True
+        fdroidserver.build.config = {'keep_when_not_allowed': False}
+        self.assertTrue(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_options_default_does_not_override(self):
+        fdroidserver.build.options = Options
+        fdroidserver.build.options.keep_when_not_allowed = False
+        fdroidserver.build.config = {'keep_when_not_allowed': True}
+        self.assertTrue(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_all_true(self):
+        fdroidserver.build.options = Options
+        fdroidserver.build.options.keep_when_not_allowed = True
+        fdroidserver.build.config = {'keep_when_not_allowed': True}
+        self.assertTrue(fdroidserver.build.keep_when_not_allowed())
+
+    def test_keep_when_not_allowed_all_false(self):
+        fdroidserver.build.options = Options
+        fdroidserver.build.options.keep_when_not_allowed = False
+        fdroidserver.build.config = {'keep_when_not_allowed': False}
+        self.assertFalse(fdroidserver.build.keep_when_not_allowed())
+
 
 if __name__ == "__main__":
     os.chdir(os.path.dirname(__file__))