From 26b44699ba379f5c73bec7fa0abd0f8555997a34 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 09:35:20 -0800 Subject: [PATCH 1/7] Add a passphrase hook so borgmatic can collect the encryption passphrase once and pass it to Borg multiple times (#961). --- borgmatic/borg/environment.py | 21 +++++++- borgmatic/execute.py | 6 +++ borgmatic/hooks/credential/__init__.py | 0 borgmatic/hooks/credential/passcommand.py | 61 +++++++++++++++++++++++ borgmatic/hooks/dispatch.py | 14 +++--- docs/reference/source-code.md | 1 + 6 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 borgmatic/hooks/credential/__init__.py create mode 100644 borgmatic/hooks/credential/passcommand.py diff --git a/borgmatic/borg/environment.py b/borgmatic/borg/environment.py index 5dc353e5..9244f162 100644 --- a/borgmatic/borg/environment.py +++ b/borgmatic/borg/environment.py @@ -1,5 +1,7 @@ import os +import borgmatic.hooks.dispatch + OPTION_TO_ENVIRONMENT_VARIABLE = { 'borg_base_directory': 'BORG_BASE_DIR', 'borg_config_directory': 'BORG_CONFIG_DIR', @@ -7,7 +9,6 @@ OPTION_TO_ENVIRONMENT_VARIABLE = { 'borg_files_cache_ttl': 'BORG_FILES_CACHE_TTL', 'borg_security_directory': 'BORG_SECURITY_DIR', 'borg_keys_directory': 'BORG_KEYS_DIR', - 'encryption_passcommand': 'BORG_PASSCOMMAND', 'encryption_passphrase': 'BORG_PASSPHRASE', 'ssh_command': 'BORG_RSH', 'temporary_directory': 'TMPDIR', @@ -36,6 +37,24 @@ def make_environment(config): if value: environment[environment_variable_name] = str(value) + passphrase = borgmatic.hooks.dispatch.call_hook( + function_name='load_credential', + config=config, + hook_name='passcommand', + credential_name='encryption_passphrase', + ) + + # If the passcommand produced a passphrase, send it to Borg via an anonymous pipe. + if passphrase: + read_file_descriptor, write_file_descriptor = os.pipe() + os.write(write_file_descriptor, passphrase.encode('utf-8')) + os.close(write_file_descriptor) + + # This, plus subprocess.Popen(..., close_fds=False) in execute.py, is necessary for the Borg + # child process to inherit the file descriptor. + os.set_inheritable(read_file_descriptor, True) + environment['BORG_PASSPHRASE_FD'] = str(read_file_descriptor) + for ( option_name, environment_variable_name, diff --git a/borgmatic/execute.py b/borgmatic/execute.py index 67a6aac3..8d850496 100644 --- a/borgmatic/execute.py +++ b/borgmatic/execute.py @@ -307,6 +307,8 @@ def execute_command( shell=shell, env=environment, cwd=working_directory, + # Necessary for the passcommand credential hook to work. + close_fds=not bool((extra_environment or {}).get('BORG_PASSPHRASE_FD')), ) if not run_to_completion: return process @@ -354,6 +356,8 @@ def execute_command_and_capture_output( shell=shell, env=environment, cwd=working_directory, + # Necessary for the passcommand credential hook to work. + close_fds=not bool((extra_environment or {}).get('BORG_PASSPHRASE_FD')), ) except subprocess.CalledProcessError as error: if ( @@ -414,6 +418,8 @@ def execute_command_with_processes( shell=shell, env=environment, cwd=working_directory, + # Necessary for the passcommand credential hook to work. + close_fds=not bool((extra_environment or {}).get('BORG_PASSPHRASE_FD')), ) except (subprocess.CalledProcessError, OSError): # Something has gone wrong. So vent each process' output buffer to prevent it from hanging. diff --git a/borgmatic/hooks/credential/__init__.py b/borgmatic/hooks/credential/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/borgmatic/hooks/credential/passcommand.py b/borgmatic/hooks/credential/passcommand.py new file mode 100644 index 00000000..23cf5845 --- /dev/null +++ b/borgmatic/hooks/credential/passcommand.py @@ -0,0 +1,61 @@ +import functools +import logging +import shlex + + +import borgmatic.config.paths +import borgmatic.execute + + +logger = logging.getLogger(__name__) + + +@functools.cache +def run_passcommand(passcommand, passphrase_configured, working_directory): + ''' + Run the given passcommand using the given working directory and return the passphrase produced + by the command. But bail first if a passphrase is already configured; this mimics Borg's + behavior. + + Cache the results so that the passcommand only needs to run—and potentially prompt the user—once + per borgmatic invocation. + ''' + if passcommand and passphrase_configured: + logger.warning( + 'Ignoring the "encryption_passcommand" option because "encryption_passphrase" is set' + ) + return None + + return borgmatic.execute.execute_command_and_capture_output( + shlex.split(passcommand), + working_directory=working_directory, + ) + + +def load_credential(hook_config, config, credential_name): + ''' + Given the hook configuration dict, the configuration dict, and a credential name to load, call + the configured passcommand to produce and return an encryption passphrase. In effect, we're + doing an end-run around Borg by invoking its passcommand ourself. This allows us to pass the + resulting passphrase to multiple different Borg invocations without the user having to be + prompted multiple times. + + If no passcommand is configured, then return None. + + The credential name must be "encryption_passphrase"; that's the only supported credential with + this particular hook. + ''' + if credential_name != 'encryption_passphrase': + raise ValueError( + f'Credential name "{credential_name}" is not supported for the passphrase credential hook' + ) + + passcommand = config.get('encryption_passcommand') + + if not passcommand: + return None + + passphrase = config.get('encryption_passphrase') + working_directory = borgmatic.config.paths.get_working_directory(config) + + return run_passcommand(passcommand, bool(passphrase), working_directory) diff --git a/borgmatic/hooks/dispatch.py b/borgmatic/hooks/dispatch.py index d806fa68..d122f75d 100644 --- a/borgmatic/hooks/dispatch.py +++ b/borgmatic/hooks/dispatch.py @@ -3,6 +3,7 @@ import importlib import logging import pkgutil +import borgmatic.hooks.credential import borgmatic.hooks.data_source import borgmatic.hooks.monitoring @@ -10,6 +11,7 @@ logger = logging.getLogger(__name__) class Hook_type(enum.Enum): + CREDENTIAL = 'credential' DATA_SOURCE = 'data_source' MONITORING = 'monitoring' @@ -40,7 +42,7 @@ def call_hook(function_name, config, hook_name, *args, **kwargs): module_name = hook_name.split('_databases')[0] # Probe for a data source or monitoring hook module corresponding to the hook name. - for parent_module in (borgmatic.hooks.data_source, borgmatic.hooks.monitoring): + for parent_module in (borgmatic.hooks.credential, borgmatic.hooks.data_source, borgmatic.hooks.monitoring): if module_name not in get_submodule_names(parent_module): continue @@ -62,8 +64,8 @@ def call_hook(function_name, config, hook_name, *args, **kwargs): def call_hooks(function_name, config, hook_type, *args, **kwargs): ''' Given a configuration dict, call the requested function of the Python module corresponding to - each hook of the given hook type (either "data_source" or "monitoring"). Supply each call with - the configuration for that hook, and any given args and kwargs. + each hook of the given hook type ("credential", "data_source", or "monitoring"). Supply each + call with the configuration for that hook, and any given args and kwargs. Collect any return values into a dict from module name to return value. Note that the module name is the name of the hook module itself, which might be different from the hook configuration @@ -88,9 +90,9 @@ def call_hooks(function_name, config, hook_type, *args, **kwargs): def call_hooks_even_if_unconfigured(function_name, config, hook_type, *args, **kwargs): ''' Given a configuration dict, call the requested function of the Python module corresponding to - each hook of the given hook type (either "data_source" or "monitoring"). Supply each call with - the configuration for that hook and any given args and kwargs. Collect any return values into a - dict from hook name to return value. + each hook of the given hook type ("credential", "data_source", or "monitoring"). Supply each + call with the configuration for that hook and any given args and kwargs. Collect any return + values into a dict from hook name to return value. Raise AttributeError if the function name is not found in the module. Raise anything else that a called function raises. An error stops calls to subsequent functions. diff --git a/docs/reference/source-code.md b/docs/reference/source-code.md index 5eab1a90..e15482e6 100644 --- a/docs/reference/source-code.md +++ b/docs/reference/source-code.md @@ -18,6 +18,7 @@ you get started. Starting at the top level, we have: * [commands](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/commands): Looking to add a new flag or action? Start here. This contains borgmatic's entry point, argument parsing, and shell completion. * [config](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/config): Code responsible for loading, normalizing, and validating borgmatic's configuration. Interested in adding a new configuration option? Check out `schema.yaml` here. * [hooks](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/hooks): Looking to add a new database, filesystem, or monitoring integration? Start here. + * [credential](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/hooks/credential): Credential hooks—for loading passphrases and secrets from external providers. * [data_source](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/hooks/data_source): Database and filesystem hooks—anything that produces data or files to go into a backup archive. * [monitoring](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/hooks/monitoring): Monitoring hooks—integrations with third-party or self-hosted monitoring services. * [docs](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/docs): How-to and reference documentation, including the document you're reading now. From 52189490a287f278a72b0ea59e4f7a9c6fb77d87 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 09:48:55 -0800 Subject: [PATCH 2/7] Docstring typo (#961). --- borgmatic/hooks/credential/passcommand.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/borgmatic/hooks/credential/passcommand.py b/borgmatic/hooks/credential/passcommand.py index 23cf5845..8ffe6ad5 100644 --- a/borgmatic/hooks/credential/passcommand.py +++ b/borgmatic/hooks/credential/passcommand.py @@ -36,7 +36,7 @@ def load_credential(hook_config, config, credential_name): ''' Given the hook configuration dict, the configuration dict, and a credential name to load, call the configured passcommand to produce and return an encryption passphrase. In effect, we're - doing an end-run around Borg by invoking its passcommand ourself. This allows us to pass the + doing an end-run around Borg by invoking its passcommand ourselves. This allows us to pass the resulting passphrase to multiple different Borg invocations without the user having to be prompted multiple times. From 8fed8e0695b6d6683fa4cdc2c9656e5464a66950 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 09:55:32 -0800 Subject: [PATCH 3/7] Add a passcommand hook to NEWS (#961). --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 13785b52..e0013d24 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,7 @@ 1.9.9.dev0 * #635: Log the repository path or label on every relevant log message, not just some logs. + * #961: Add a passcommand hook so borgmatic can collect the encryption passphrase once and pass it + to Borg multiple times. * #981: Fix a "spot" check file count delta error. * #982: Fix for borgmatic "exclude_patterns" and "exclude_from" recursing into excluded subdirectories. From c392e4914c2c0fd5f7d6031d618fb0c851b0d576 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 10:20:24 -0800 Subject: [PATCH 4/7] Add documentation (#961). --- docs/how-to/provide-your-passwords.md | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/docs/how-to/provide-your-passwords.md b/docs/how-to/provide-your-passwords.md index 57803864..e5e43539 100644 --- a/docs/how-to/provide-your-passwords.md +++ b/docs/how-to/provide-your-passwords.md @@ -24,11 +24,29 @@ or security reasons, read on. borgmatic supports calling another application such as a password manager to obtain the Borg passphrase to a repository. -For example, to ask the *Pass* password manager to provide the passphrase: +For example, to ask the [Pass](https://www.passwordstore.org/) password manager +to provide the passphrase: + ```yaml -encryption_passcommand: pass path/to/borg-repokey +encryption_passcommand: pass path/to/borg-passphrase ``` +Another example for [KeePassXC](https://keepassxc.org/): + +```yaml +encryption_passcommand: keepassxc-cli show --show-protected --attributes Password credentials.kdbx borg_passphrase +``` + +... where `borg_passphrase` is the title of the KeePassXC entry containing your +Borg encryption passphrase in its `Password` field. + +New in version 1.9.9 Instead of +letting Borg run the passcommand—potentially mulitple times since borgmatic runs +Borg multiple times—borgmatic now runs the passcommand itself and passes it +securely to Borg via an anonymous pipe. This means you should only ever get +prompted for your password manager's passphrase at most once per borgmatic run. + + ### Using systemd service credentials Borgmatic supports using encrypted [credentials](https://systemd.io/CREDENTIALS/). From 355eef186ee966b814b35b0cde07a6cd6ac8014a Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 20:18:03 -0800 Subject: [PATCH 5/7] Get existing tests passing again (#961). --- borgmatic/hooks/dispatch.py | 6 +- tests/unit/borg/test_environment.py | 29 +++++- tests/unit/hooks/test_dispatch.py | 18 ++++ tests/unit/test_execute.py | 147 ++++++++++++++++++++++++++-- 4 files changed, 189 insertions(+), 11 deletions(-) diff --git a/borgmatic/hooks/dispatch.py b/borgmatic/hooks/dispatch.py index d122f75d..26b0832c 100644 --- a/borgmatic/hooks/dispatch.py +++ b/borgmatic/hooks/dispatch.py @@ -42,7 +42,11 @@ def call_hook(function_name, config, hook_name, *args, **kwargs): module_name = hook_name.split('_databases')[0] # Probe for a data source or monitoring hook module corresponding to the hook name. - for parent_module in (borgmatic.hooks.credential, borgmatic.hooks.data_source, borgmatic.hooks.monitoring): + for parent_module in ( + borgmatic.hooks.credential, + borgmatic.hooks.data_source, + borgmatic.hooks.monitoring, + ): if module_name not in get_submodule_names(parent_module): continue diff --git a/tests/unit/borg/test_environment.py b/tests/unit/borg/test_environment.py index 30b45746..33b4417e 100644 --- a/tests/unit/borg/test_environment.py +++ b/tests/unit/borg/test_environment.py @@ -3,13 +3,22 @@ from flexmock import flexmock from borgmatic.borg import environment as module -def test_make_environment_with_passcommand_should_set_environment(): +def test_make_environment_with_passcommand_should_call_it_and_set_passphrase_file_descriptor_in_environment(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return('passphrase') + flexmock(module.os).should_receive('pipe').and_return((3, 4)) + flexmock(module.os).should_receive('write') + flexmock(module.os).should_receive('close') + flexmock(module.os).should_receive('set_inheritable') + environment = module.make_environment({'encryption_passcommand': 'command'}) - assert environment.get('BORG_PASSCOMMAND') == 'command' + assert not environment.get('BORG_PASSCOMMAND') + assert environment.get('BORG_PASSPHRASE_FD') == '3' def test_make_environment_with_passphrase_should_set_environment(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'encryption_passphrase': 'pass'}) @@ -17,6 +26,8 @@ def test_make_environment_with_passphrase_should_set_environment(): def test_make_environment_with_ssh_command_should_set_environment(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'ssh_command': 'ssh -C'}) @@ -24,6 +35,8 @@ def test_make_environment_with_ssh_command_should_set_environment(): def test_make_environment_without_configuration_sets_certain_environment_variables(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({}) @@ -36,6 +49,8 @@ def test_make_environment_without_configuration_sets_certain_environment_variabl def test_make_environment_without_configuration_does_not_set_certain_environment_variables_if_already_set(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').with_args( 'BORG_RELOCATED_REPO_ACCESS_IS_OK' ).and_return('yup') @@ -48,6 +63,8 @@ def test_make_environment_without_configuration_does_not_set_certain_environment def test_make_environment_with_relocated_repo_access_true_should_set_environment_yes(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'relocated_repo_access_is_ok': True}) @@ -55,6 +72,8 @@ def test_make_environment_with_relocated_repo_access_true_should_set_environment def test_make_environment_with_relocated_repo_access_false_should_set_environment_no(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'relocated_repo_access_is_ok': False}) @@ -62,6 +81,8 @@ def test_make_environment_with_relocated_repo_access_false_should_set_environmen def test_make_environment_check_i_know_what_i_am_doing_true_should_set_environment_YES(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'check_i_know_what_i_am_doing': True}) @@ -69,6 +90,8 @@ def test_make_environment_check_i_know_what_i_am_doing_true_should_set_environme def test_make_environment_check_i_know_what_i_am_doing_false_should_set_environment_NO(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'check_i_know_what_i_am_doing': False}) @@ -76,6 +99,8 @@ def test_make_environment_check_i_know_what_i_am_doing_false_should_set_environm def test_make_environment_with_integer_variable_value(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'borg_files_cache_ttl': 40}) assert environment.get('BORG_FILES_CACHE_TTL') == '40' diff --git a/tests/unit/hooks/test_dispatch.py b/tests/unit/hooks/test_dispatch.py index 80f8c046..8f3f8ecf 100644 --- a/tests/unit/hooks/test_dispatch.py +++ b/tests/unit/hooks/test_dispatch.py @@ -17,6 +17,9 @@ def test_call_hook_invokes_module_function_with_arguments_and_returns_value(): config = {'super_hook': flexmock(), 'other_hook': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -39,6 +42,9 @@ def test_call_hook_probes_config_with_databases_suffix(): config = {'super_hook_databases': flexmock(), 'other_hook': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -61,6 +67,9 @@ def test_call_hook_strips_databases_suffix_from_hook_name(): config = {'super_hook_databases': flexmock(), 'other_hook_databases': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -83,6 +92,9 @@ def test_call_hook_without_hook_config_invokes_module_function_with_arguments_an config = {'other_hook': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -104,6 +116,9 @@ def test_call_hook_without_hook_config_invokes_module_function_with_arguments_an def test_call_hook_without_corresponding_module_raises(): config = {'super_hook': flexmock(), 'other_hook': flexmock()} test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -121,6 +136,9 @@ def test_call_hook_without_corresponding_module_raises(): def test_call_hook_skips_non_hook_modules(): config = {'not_a_hook': flexmock(), 'other_hook': flexmock()} + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) diff --git a/tests/unit/test_execute.py b/tests/unit/test_execute.py index 7741526f..8de49912 100644 --- a/tests/unit/test_execute.py +++ b/tests/unit/test_execute.py @@ -185,6 +185,7 @@ def test_execute_command_calls_full_command(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -207,6 +208,7 @@ def test_execute_command_calls_full_command_with_output_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stderr=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -221,7 +223,14 @@ def test_execute_command_calls_full_command_without_capturing_output(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('Popen').with_args( - full_command, stdin=None, stdout=None, stderr=None, shell=False, env=None, cwd=None + full_command, + stdin=None, + stdout=None, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(wait=lambda: 0)).once() flexmock(module).should_receive('interpret_exit_code').and_return(module.Exit_status.SUCCESS) flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) @@ -245,6 +254,7 @@ def test_execute_command_calls_full_command_with_input_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -266,6 +276,7 @@ def test_execute_command_calls_full_command_with_shell(): shell=True, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -287,6 +298,7 @@ def test_execute_command_calls_full_command_with_extra_environment(): shell=False, env={'a': 'b', 'c': 'd'}, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -308,6 +320,7 @@ def test_execute_command_calls_full_command_with_working_directory(): shell=False, env=None, cwd='/working', + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -317,6 +330,28 @@ def test_execute_command_calls_full_command_with_working_directory(): assert output is None +def test_execute_command_with_BORG_PASSPHRASE_FD_leaves_file_descriptors_open(): + full_command = ['foo', 'bar'] + flexmock(module).should_receive('log_command') + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('Popen').with_args( + full_command, + stdin=None, + stdout=module.subprocess.PIPE, + stderr=module.subprocess.STDOUT, + shell=False, + env={'a': 'b', 'BORG_PASSPHRASE_FD': '4'}, + cwd=None, + close_fds=False, + ).and_return(flexmock(stdout=None)).once() + flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) + flexmock(module).should_receive('log_outputs') + + output = module.execute_command(full_command, extra_environment={'BORG_PASSPHRASE_FD': '4'}) + + assert output is None + + def test_execute_command_without_run_to_completion_returns_process(): full_command = ['foo', 'bar'] process = flexmock() @@ -330,6 +365,7 @@ def test_execute_command_without_run_to_completion_returns_process(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(process).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -343,7 +379,12 @@ def test_execute_command_and_capture_output_returns_stdout(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd=None + full_command, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output(full_command) @@ -357,7 +398,12 @@ def test_execute_command_and_capture_output_with_capture_stderr_returns_stderr() flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=module.subprocess.STDOUT, shell=False, env=None, cwd=None + full_command, + stderr=module.subprocess.STDOUT, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output(full_command, capture_stderr=True) @@ -372,7 +418,12 @@ def test_execute_command_and_capture_output_returns_output_when_process_error_is flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd=None + full_command, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_raise(subprocess.CalledProcessError(1, full_command, err_output)).once() flexmock(module).should_receive('interpret_exit_code').and_return( module.Exit_status.SUCCESS @@ -389,7 +440,12 @@ def test_execute_command_and_capture_output_raises_when_command_errors(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd=None + full_command, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_raise(subprocess.CalledProcessError(2, full_command, expected_output)).once() flexmock(module).should_receive('interpret_exit_code').and_return( module.Exit_status.ERROR @@ -405,7 +461,12 @@ def test_execute_command_and_capture_output_returns_output_with_shell(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - 'foo bar', stderr=None, shell=True, env=None, cwd=None + 'foo bar', + stderr=None, + shell=True, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output(full_command, shell=True) @@ -424,6 +485,7 @@ def test_execute_command_and_capture_output_returns_output_with_extra_environmen shell=False, env={'a': 'b', 'c': 'd'}, cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output( @@ -439,7 +501,12 @@ def test_execute_command_and_capture_output_returns_output_with_working_director flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd='/working' + full_command, + stderr=None, + shell=False, + env=None, + cwd='/working', + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output( @@ -449,6 +516,29 @@ def test_execute_command_and_capture_output_returns_output_with_working_director assert output == expected_output +def test_execute_command_and_capture_output_with_BORG_PASSPHRASE_FD_leaves_file_descriptors_open(): + full_command = ['foo', 'bar'] + expected_output = '[]' + flexmock(module).should_receive('log_command') + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('check_output').with_args( + full_command, + stderr=None, + shell=False, + env={'a': 'b', 'BORG_PASSPHRASE_FD': '4'}, + cwd=None, + close_fds=False, + ).and_return(flexmock(decode=lambda: expected_output)).once() + + output = module.execute_command_and_capture_output( + full_command, + shell=False, + extra_environment={'BORG_PASSPHRASE_FD': '4'}, + ) + + assert output == expected_output + + def test_execute_command_with_processes_calls_full_command(): full_command = ['foo', 'bar'] processes = (flexmock(),) @@ -462,6 +552,7 @@ def test_execute_command_with_processes_calls_full_command(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -484,6 +575,7 @@ def test_execute_command_with_processes_returns_output_with_output_log_level_non shell=False, env=None, cwd=None, + close_fds=True, ).and_return(process).once() flexmock(module).should_receive('log_outputs').and_return({process: 'out'}) @@ -506,6 +598,7 @@ def test_execute_command_with_processes_calls_full_command_with_output_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stderr=None)).once() flexmock(module).should_receive('log_outputs') @@ -520,7 +613,14 @@ def test_execute_command_with_processes_calls_full_command_without_capturing_out flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('Popen').with_args( - full_command, stdin=None, stdout=None, stderr=None, shell=False, env=None, cwd=None + full_command, + stdin=None, + stdout=None, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(wait=lambda: 0)).once() flexmock(module).should_receive('interpret_exit_code').and_return(module.Exit_status.SUCCESS) flexmock(module).should_receive('log_outputs') @@ -546,6 +646,7 @@ def test_execute_command_with_processes_calls_full_command_with_input_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -567,6 +668,7 @@ def test_execute_command_with_processes_calls_full_command_with_shell(): shell=True, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -588,6 +690,7 @@ def test_execute_command_with_processes_calls_full_command_with_extra_environmen shell=False, env={'a': 'b', 'c': 'd'}, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -611,6 +714,7 @@ def test_execute_command_with_processes_calls_full_command_with_working_director shell=False, env=None, cwd='/working', + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -621,6 +725,32 @@ def test_execute_command_with_processes_calls_full_command_with_working_director assert output is None +def test_execute_command_with_processes_with_BORG_PASSPHRASE_FD_leaves_file_descriptors_open(): + full_command = ['foo', 'bar'] + processes = (flexmock(),) + flexmock(module).should_receive('log_command') + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('Popen').with_args( + full_command, + stdin=None, + stdout=module.subprocess.PIPE, + stderr=module.subprocess.STDOUT, + shell=False, + env={'a': 'b', 'BORG_PASSPHRASE_FD': '4'}, + cwd=None, + close_fds=False, + ).and_return(flexmock(stdout=None)).once() + flexmock(module).should_receive('log_outputs') + + output = module.execute_command_with_processes( + full_command, + processes, + extra_environment={'BORG_PASSPHRASE_FD': '4'}, + ) + + assert output is None + + def test_execute_command_with_processes_kills_processes_on_error(): full_command = ['foo', 'bar'] flexmock(module).should_receive('log_command') @@ -637,6 +767,7 @@ def test_execute_command_with_processes_kills_processes_on_error(): shell=False, env=None, cwd=None, + close_fds=True, ).and_raise(subprocess.CalledProcessError(1, full_command, 'error')).once() flexmock(module).should_receive('log_outputs').never() From cf326a98a527794db813601350b52a88cf4cc496 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 21:29:52 -0800 Subject: [PATCH 6/7] Add test coverage for new code (#961). --- borgmatic/hooks/credential/passcommand.py | 2 - docs/how-to/provide-your-passwords.md | 9 +-- tests/unit/hooks/credential/__init__.py | 0 .../unit/hooks/credential/test_passcommand.py | 65 +++++++++++++++++++ 4 files changed, 70 insertions(+), 6 deletions(-) create mode 100644 tests/unit/hooks/credential/__init__.py create mode 100644 tests/unit/hooks/credential/test_passcommand.py diff --git a/borgmatic/hooks/credential/passcommand.py b/borgmatic/hooks/credential/passcommand.py index 8ffe6ad5..d84dacc9 100644 --- a/borgmatic/hooks/credential/passcommand.py +++ b/borgmatic/hooks/credential/passcommand.py @@ -2,11 +2,9 @@ import functools import logging import shlex - import borgmatic.config.paths import borgmatic.execute - logger = logging.getLogger(__name__) diff --git a/docs/how-to/provide-your-passwords.md b/docs/how-to/provide-your-passwords.md index e5e43539..60aa33b7 100644 --- a/docs/how-to/provide-your-passwords.md +++ b/docs/how-to/provide-your-passwords.md @@ -41,10 +41,11 @@ encryption_passcommand: keepassxc-cli show --show-protected --attributes Passwor Borg encryption passphrase in its `Password` field. New in version 1.9.9 Instead of -letting Borg run the passcommand—potentially mulitple times since borgmatic runs -Borg multiple times—borgmatic now runs the passcommand itself and passes it -securely to Borg via an anonymous pipe. This means you should only ever get -prompted for your password manager's passphrase at most once per borgmatic run. +letting Borg run the passcommand—potentially multiple times since borgmatic runs +Borg multiple times—borgmatic now runs the passcommand itself and passes the +resulting passprhase securely to Borg via an anonymous pipe. This means you +should only ever get prompted for your password manager's passphrase at most +once per borgmatic run. ### Using systemd service credentials diff --git a/tests/unit/hooks/credential/__init__.py b/tests/unit/hooks/credential/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/hooks/credential/test_passcommand.py b/tests/unit/hooks/credential/test_passcommand.py new file mode 100644 index 00000000..be67b368 --- /dev/null +++ b/tests/unit/hooks/credential/test_passcommand.py @@ -0,0 +1,65 @@ +import pytest +from flexmock import flexmock + +from borgmatic.hooks.credential import passcommand as module + + +def test_run_passcommand_with_passphrase_configured_bails(): + flexmock(module.borgmatic.execute).should_receive('execute_command_and_capture_output').never() + + assert ( + module.run_passcommand('passcommand', passphrase_configured=True, working_directory=None) + is None + ) + + +def test_run_passcommand_without_passphrase_configured_executes_passcommand(): + flexmock(module.borgmatic.execute).should_receive( + 'execute_command_and_capture_output' + ).and_return('passphrase').once() + + assert ( + module.run_passcommand('passcommand', passphrase_configured=False, working_directory=None) + == 'passphrase' + ) + + +def test_load_credential_with_unknown_credential_name_errors(): + with pytest.raises(ValueError): + module.load_credential(hook_config={}, config={}, credential_name='wtf') + + +def test_load_credential_with_configured_passcommand_runs_it(): + flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return( + '/working' + ) + flexmock(module).should_receive('run_passcommand').with_args( + 'command', False, '/working' + ).and_return('passphrase').once() + + assert ( + module.load_credential( + hook_config={}, + config={'encryption_passcommand': 'command'}, + credential_name='encryption_passphrase', + ) + == 'passphrase' + ) + + +def test_load_credential_with_configured_passphrase_and_passcommand_detects_passphrase(): + flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return( + '/working' + ) + flexmock(module).should_receive('run_passcommand').with_args( + 'command', True, '/working' + ).and_return(None).once() + + assert ( + module.load_credential( + hook_config={}, + config={'encryption_passphrase': 'passphrase', 'encryption_passcommand': 'command'}, + credential_name='encryption_passphrase', + ) + is None + ) From 13acaa47e451087896e048fa08179c245bd1a73e Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 22:50:13 -0800 Subject: [PATCH 7/7] Add an end-to-end test for the passcommand hook (#961). --- tests/end-to-end/hooks/credential/__init__.py | 0 .../hooks/credential/test_passcommand.py | 65 +++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 tests/end-to-end/hooks/credential/__init__.py create mode 100644 tests/end-to-end/hooks/credential/test_passcommand.py diff --git a/tests/end-to-end/hooks/credential/__init__.py b/tests/end-to-end/hooks/credential/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/end-to-end/hooks/credential/test_passcommand.py b/tests/end-to-end/hooks/credential/test_passcommand.py new file mode 100644 index 00000000..e6c1c810 --- /dev/null +++ b/tests/end-to-end/hooks/credential/test_passcommand.py @@ -0,0 +1,65 @@ +import json +import os +import shutil +import subprocess +import sys +import tempfile + +import pytest + + +def generate_configuration(config_path, repository_path): + ''' + Generate borgmatic configuration into a file at the config path, and update the defaults so as + to work for testing, including updating the source directories, injecting the given repository + path, and tacking on an encryption passcommand. + ''' + subprocess.check_call(f'borgmatic config generate --destination {config_path}'.split(' ')) + config = ( + open(config_path) + .read() + .replace('ssh://user@backupserver/./sourcehostname.borg', repository_path) + .replace('- path: /mnt/backup', '') + .replace('label: local', '') + .replace('- /home/user/path with spaces', '') + .replace('- /home', f'- {config_path}') + .replace('- /etc', '') + .replace('- /var/log/syslog*', '') + + '\nencryption_passcommand: "echo test"' + ) + config_file = open(config_path, 'w') + config_file.write(config) + config_file.close() + + +def test_borgmatic_command(): + # Create a Borg repository. + temporary_directory = tempfile.mkdtemp() + repository_path = os.path.join(temporary_directory, 'test.borg') + extract_path = os.path.join(temporary_directory, 'extract') + + original_working_directory = os.getcwd() + os.mkdir(extract_path) + os.chdir(extract_path) + + try: + config_path = os.path.join(temporary_directory, 'test.yaml') + generate_configuration(config_path, repository_path) + + subprocess.check_call( + f'borgmatic -v 2 --config {config_path} repo-create --encryption repokey'.split(' ') + ) + + # Run borgmatic to generate a backup archive, and then list it to make sure it exists. + subprocess.check_call(f'borgmatic -v 2 --config {config_path}'.split(' ')) + output = subprocess.check_output( + f'borgmatic --config {config_path} list --json'.split(' ') + ).decode(sys.stdout.encoding) + parsed_output = json.loads(output) + + assert len(parsed_output) == 1 + assert len(parsed_output[0]['archives']) == 1 + archive_name = parsed_output[0]['archives'][0]['archive'] + finally: + os.chdir(original_working_directory) + shutil.rmtree(temporary_directory)