Move the passcommand logic out of a hook to prevent future security issues (e.g., passphrase exfiltration attacks) if a user invokes a credential hook from an arbitrary configuration value (#961).

borgmatic/borg/environment.py

@@ -1,6 +1,6 @@
 import os
 
-import borgmatic.hooks.dispatch
+import borgmatic.borg.passcommand
 
 OPTION_TO_ENVIRONMENT_VARIABLE = {
     'borg_base_directory': 'BORG_BASE_DIR',
@@ -37,12 +37,7 @@ 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',
-    )
+    passphrase = borgmatic.borg.passcommand.get_passphrase_from_passcommand(config)
 
     # If the passcommand produced a passphrase, send it to Borg via an anonymous pipe.
     if passphrase:

borgmatic/hooks/credential/passcommand.py → borgmatic/borg/passcommand.py

@@ -30,24 +30,15 @@ def run_passcommand(passcommand, passphrase_configured, working_directory):
     )
 
 
-def load_credential(hook_config, config, credential_name):
+def get_passphrase_from_passcommand(config):
     '''
-    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 ourselves. This allows us to pass the
-    resulting passphrase to multiple different Borg invocations without the user having to be
-    prompted multiple times.
+    Given the configuration dict, 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
+    ourselves. 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:

tests/unit/borg/test_environment.py

@@ -4,7 +4,9 @@ from borgmatic.borg import environment as module
 
 
 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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')
@@ -17,7 +19,9 @@ def test_make_environment_with_passcommand_should_call_it_and_set_passphrase_fil
 
 
 def test_make_environment_with_passphrase_should_set_environment():
-    flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None)
+    flexmock(module.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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'})
@@ -26,7 +30,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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'})
@@ -35,7 +41,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).and_return(None)
     flexmock(module.os).should_receive('pipe').never()
     flexmock(module.os.environ).should_receive('get').and_return(None)
     environment = module.make_environment({})
@@ -49,7 +57,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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'
@@ -63,7 +73,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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})
@@ -72,7 +84,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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})
@@ -81,7 +95,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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})
@@ -90,7 +106,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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})
@@ -99,7 +117,9 @@ 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.borgmatic.borg.passcommand).should_receive(
+        'get_passphrase_from_passcommand'
+    ).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})

tests/unit/hooks/credential/test_passcommand.py → tests/unit/borg/test_passcommand.py

@@ -1,7 +1,6 @@
-import pytest
 from flexmock import flexmock
 
-from borgmatic.hooks.credential import passcommand as module
+from borgmatic.borg import passcommand as module
 
 
 def test_run_passcommand_with_passphrase_configured_bails():
@@ -24,12 +23,7 @@ def test_run_passcommand_without_passphrase_configured_executes_passcommand():
     )
 
 
-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():
+def test_get_passphrase_from_passcommand_with_configured_passcommand_runs_it():
     flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(
         '/working'
     )
@@ -38,16 +32,14 @@ def test_load_credential_with_configured_passcommand_runs_it():
     ).and_return('passphrase').once()
 
     assert (
-        module.load_credential(
-            hook_config={},
-            config={'encryption_passcommand': 'command'},
-            credential_name='encryption_passphrase',
+        module.get_passphrase_from_passcommand(
+            {'encryption_passcommand': 'command'},
         )
         == 'passphrase'
     )
 
 
-def test_load_credential_with_configured_passphrase_and_passcommand_detects_passphrase():
+def test_get_passphrase_from_passcommand_with_configured_passphrase_and_passcommand_detects_passphrase():
     flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(
         '/working'
     )
@@ -56,10 +48,8 @@ def test_load_credential_with_configured_passphrase_and_passcommand_detects_pass
     ).and_return(None).once()
 
     assert (
-        module.load_credential(
-            hook_config={},
-            config={'encryption_passphrase': 'passphrase', 'encryption_passcommand': 'command'},
-            credential_name='encryption_passphrase',
+        module.get_passphrase_from_passcommand(
+            {'encryption_passphrase': 'passphrase', 'encryption_passcommand': 'command'},
         )
         is None
     )