From 3520ef5b4ecc3ef382e41b8fa95fd0e1b9bc6a42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Priit=20L=C3=A4tt?= Date: Wed, 17 Feb 2021 16:25:31 +0200 Subject: [PATCH] Improvement: Add option to specify allowed apps for keychain add-certificates (#69) * Add options to keychain add-certificates to specify which applications have access to the imported certificate * Remove reference to undefined fixture * Accept just executable name for allowed app * Update docs * Move method definition within the same class --- CHANGELOG.md | 7 +++ doc.py | 2 + docs/keychain/add-certificates.md | 15 +++++ src/codemagic/__version__.py | 2 +- src/codemagic/cli/argument/argument.py | 4 ++ src/codemagic/tools/keychain.py | 86 +++++++++++++++++++++++--- tests/tools/test_keychain.py | 58 +++++++++++++++++ 7 files changed, 163 insertions(+), 11 deletions(-) create mode 100644 tests/tools/test_keychain.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 08a5e14f..d7d87af8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +Version 0.4.5 +------------- + +**Improvements** + +- Improvement: Add options to `keychain add-certificates` to specify which applications have access to the imported certificate without warning. + Version 0.4.4 ------------- diff --git a/doc.py b/doc.py index 35d52256..4073cadb 100755 --- a/doc.py +++ b/doc.py @@ -88,6 +88,8 @@ def _process_choice(choices): def _process_default(default): if not default: return '' + if isinstance(default, tuple) and len(default) > 1 and isinstance(default[0], Path): + default = ', '.join(str(p).replace(str(Path.home()), '$HOME') for p in default) if isinstance(default, tuple) and isinstance(default[0], Path): default = default[0] if isinstance(default, Path): diff --git a/docs/keychain/add-certificates.md b/docs/keychain/add-certificates.md index 1f4ba777..628e4b1f 100644 --- a/docs/keychain/add-certificates.md +++ b/docs/keychain/add-certificates.md @@ -10,6 +10,9 @@ keychain add-certificates [-h] [--log-stream STREAM] [--no-color] [--version] [- [-p PATH] [-c CERTIFICATE_PATHS] [--certificate-password CERTIFICATE_PASSWORD] + [-a ALLOWED_APPLICATIONS] + [-A] + [-D] ``` ### Optional arguments for action `add-certificates` @@ -21,6 +24,18 @@ Path to pkcs12 certificate. Can be either a path literal, or a glob pattern to m Encrypted p12 certificate password. Alternatively to entering CERTIFICATE_PASSWORD in plaintext, it may also be specified using a "@env:" prefix followed by a environment variable name, or "@file:" prefix followed by a path to the file containing the value. Example: "@env:" uses the value in the environment variable named "", and "@file:" uses the value from file at "". [Default: ''] +##### `-a, --allow-app=ALLOWED_APPLICATIONS` + + +Specify an application which may access the imported key without warning. Multiple arguments. Default: `codesign, productsign` +##### `-A, --allow-all-applications` + + +Allow any application to access the imported key without warning +##### `-D, --disallow-all-applications` + + +Do not allow any applications to access the imported key without warning ### Optional arguments for command `keychain` ##### `-p, --path=PATH` diff --git a/src/codemagic/__version__.py b/src/codemagic/__version__.py index b8316e61..941ac6f1 100644 --- a/src/codemagic/__version__.py +++ b/src/codemagic/__version__.py @@ -1,5 +1,5 @@ __title__ = "codemagic-cli-tools" __description__ = "CLI tools used in Codemagic builds" -__version__ = "0.4.4" +__version__ = "0.4.5" __url__ = 'https://github.com/codemagic-ci-cd/cli-tools' __licence__ = 'GNU General Public License v3.0' diff --git a/src/codemagic/cli/argument/argument.py b/src/codemagic/cli/argument/argument.py index a64d72ab..53014047 100644 --- a/src/codemagic/cli/argument/argument.py +++ b/src/codemagic/cli/argument/argument.py @@ -15,6 +15,10 @@ class Argument(ArgumentProperties, enum.Enum): + @property + def flag(self) -> str: + return sorted(self.value.flags, key=len, reverse=True)[0] + def register(self, argument_group: argparse._ArgumentGroup): kwargs = self.value.argparse_kwargs or {} if 'action' not in kwargs: diff --git a/src/codemagic/tools/keychain.py b/src/codemagic/tools/keychain.py index c7b98f1f..9fe81e6a 100755 --- a/src/codemagic/tools/keychain.py +++ b/src/codemagic/tools/keychain.py @@ -6,6 +6,7 @@ import pathlib import shutil from tempfile import NamedTemporaryFile +from typing import Iterable from typing import List from typing import Optional from typing import Sequence @@ -77,6 +78,32 @@ class KeychainArgument(cli.Argument): description='Encrypted p12 certificate password', argparse_kwargs={'required': False, 'default': ''}, ) + ALLOWED_APPLICATIONS = cli.ArgumentProperties( + flags=('-a', '--allow-app'), + key='allowed_applications', + description='Specify an application which may access the imported key without warning', + type=pathlib.Path, + argparse_kwargs={ + 'required': False, + 'default': (pathlib.Path('codesign'), pathlib.Path('productsign')), + 'nargs': '+', + 'metavar': 'allowed-app', + }, + ) + ALLOW_ALL_APPLICATIONS = cli.ArgumentProperties( + flags=('-A', '--allow-all-applications'), + key='allow_all_applications', + type=bool, + description='Allow any application to access the imported key without warning', + argparse_kwargs={'required': False, 'action': 'store_true'}, + ) + DISALLOW_ALL_APPLICATIONS = cli.ArgumentProperties( + flags=('-D', '--disallow-all-applications'), + key='disallow_all_applications', + type=bool, + description='Do not allow any applications to access the imported key without warning', + argparse_kwargs={'required': False, 'action': 'store_true'}, + ) @cli.common_arguments(KeychainArgument.PATH) @@ -245,24 +272,58 @@ def _generate_path(self): @cli.action('add-certificates', KeychainArgument.CERTIFICATE_PATHS, - KeychainArgument.CERTIFICATE_PASSWORD) - def add_certificates(self, - certificate_path_patterns: Sequence[pathlib.Path], - certificate_password: Password = Password('')): + KeychainArgument.CERTIFICATE_PASSWORD, + KeychainArgument.ALLOWED_APPLICATIONS, + KeychainArgument.ALLOW_ALL_APPLICATIONS, + KeychainArgument.DISALLOW_ALL_APPLICATIONS) + def add_certificates( + self, + certificate_path_patterns: Sequence[pathlib.Path], + certificate_password: Password = Password(''), + allowed_applications: Sequence[pathlib.Path] = KeychainArgument.ALLOWED_APPLICATIONS.get_default(), + allow_all_applications: Optional[bool] = KeychainArgument.ALLOW_ALL_APPLICATIONS.get_default(), + disallow_all_applications: Optional[bool] = KeychainArgument.DISALLOW_ALL_APPLICATIONS.get_default()): """ Add p12 certificate to specified keychain. """ - self.logger.info(f'Add certificates to keychain {self.path}') + add_for_all_apps = False + add_for_apps: List[str] = [] + if allow_all_applications and disallow_all_applications: + raise KeychainArgument.ALLOW_ALL_APPLICATIONS.raise_argument_error( + f'Using mutually exclusive options ' + f'{KeychainArgument.ALLOWED_APPLICATIONS.flag!r} and ' + f'{KeychainArgument.DISALLOW_ALL_APPLICATIONS.flag!r}') + elif allow_all_applications: + add_for_all_apps = True + elif not disallow_all_applications: + add_for_apps = list(self._get_certificate_allowed_applications(allowed_applications)) + + self.logger.info('Add certificates to keychain %s', self.path) certificate_paths = list(self.find_paths(*certificate_path_patterns)) if not certificate_paths: raise KeychainError('Did not find any certificates from specified locations') for certificate_path in certificate_paths: - self._add_certificate(certificate_path, certificate_password) + self._add_certificate(certificate_path, certificate_password, add_for_all_apps, add_for_apps) + + @classmethod + def _get_certificate_allowed_applications( + cls, given_allowed_applications: Sequence[pathlib.Path]) -> Iterable[str]: + for application in given_allowed_applications: + resolved_path = shutil.which(application) + if resolved_path is None: + # Only raise exception if user-specified path is not present + if application not in KeychainArgument.ALLOWED_APPLICATIONS.get_default(): + raise KeychainArgument.ALLOWED_APPLICATIONS.raise_argument_error( + f'Application "{application}" does not exist or is not in PATH') + else: + yield resolved_path def _add_certificate(self, certificate_path: pathlib.Path, - certificate_password: Optional[Password] = None): + certificate_password: Optional[Password] = None, + allow_for_all_apps: bool = False, + allowed_applications: Sequence[str] = tuple()): self.logger.info(f'Add certificate {certificate_path} to keychain {self.path}') # If case of no password, we need to explicitly set -P '' flag. Otherwise, # security tries to open an interactive dialog to prompt the user for a password, @@ -273,13 +334,18 @@ def _add_certificate(self, certificate_password = Password('') obfuscate_patterns = [] - process = self.execute([ + import_cmd = [ 'security', 'import', certificate_path, '-f', "pkcs12", '-k', self.path, - '-T', shutil.which('codesign'), '-P', certificate_password.value, - ], obfuscate_patterns=obfuscate_patterns) + ] + if allow_for_all_apps: + import_cmd.append('-A') + for allowed_application in allowed_applications: + import_cmd.extend(['-T', allowed_application]) + + process = self.execute(import_cmd, obfuscate_patterns=obfuscate_patterns) if process.returncode != 0: if 'The specified item already exists in the keychain' in process.stderr: diff --git a/tests/tools/test_keychain.py b/tests/tools/test_keychain.py new file mode 100644 index 00000000..37342c19 --- /dev/null +++ b/tests/tools/test_keychain.py @@ -0,0 +1,58 @@ +import argparse +import pathlib +from unittest import mock +from unittest.mock import Mock + +import pytest + +from codemagic.cli import CliProcess +from codemagic.tools.keychain import Keychain +from codemagic.tools.keychain import KeychainArgument + + +def mock_find_paths(_self, *args): + return [*args] + + +@pytest.mark.parametrize('allowed_apps, allow_all, disallow_all, expected_args, not_expected_args', [ + (['app1', 'app2'], False, False, ['app1', 'app2'], ['-A']), # Only allow specified apps + (['app1', 'app2'], True, False, ['-A'], ['app1', 'app2']), # Allow all apps + (['app1', 'app2'], False, True, [], ['-A', 'app1', 'app2']), # Do not allow any apps +]) +@mock.patch.object(Keychain, 'find_paths', mock_find_paths) +def test_add_certificates_allowed_applications( + allowed_apps, allow_all, disallow_all, expected_args, not_expected_args, cli_argument_group): + KeychainArgument.ALLOWED_APPLICATIONS.register(cli_argument_group) + keychain = Keychain(path=pathlib.Path('/tmp/keychain')) + + with mock.patch.object(keychain, 'execute') as mock_execute, \ + mock.patch('codemagic.tools.keychain.shutil') as mock_shutil: + mock_shutil.which = lambda path: str(path) + mock_execute.return_value = Mock(returncode=0, spec=CliProcess) + keychain.add_certificates( + [pathlib.Path('*.p12')], + allowed_applications=list(map(pathlib.Path, allowed_apps)), + allow_all_applications=allow_all, + disallow_all_applications=disallow_all, + ) + mock_call_args = mock_execute.call_args[0][0] + + for arg in expected_args: + assert arg in mock_call_args + for arg in not_expected_args: + assert arg not in mock_call_args + + +@mock.patch.object(Keychain, 'find_paths', mock_find_paths) +def test_add_certificates_all_apps_allowed_and_disallowed(cli_argument_group): + KeychainArgument.ALLOW_ALL_APPLICATIONS.register(cli_argument_group) + KeychainArgument.DISALLOW_ALL_APPLICATIONS.register(cli_argument_group) + with pytest.raises(argparse.ArgumentError) as argument_error: + Keychain(path=pathlib.Path('/tmp/keychain')).add_certificates( + [pathlib.Path('*.p12')], + allowed_applications=[], + allow_all_applications=True, + disallow_all_applications=True, + ) + assert KeychainArgument.ALLOW_ALL_APPLICATIONS.flag in argument_error.value.argument_name + assert 'mutually exclusive options' in argument_error.value.message