diff --git a/NEWS b/NEWS index fe3d6cf1..5e48a5e2 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ * #1003: In the Zabbix monitoring hook, support Zabbix 7.2's authentication changes. * #1009: Send database passwords to MariaDB and MySQL via anonymous pipe, which is more secure than using an environment variable. + * #1013: Send database passwords to MongoDB via anonymous pipe, which is more secure than using + "--password" on the command-line. * Add a "verify_tls" option to the Uptime Kuma monitoring hook for disabling TLS verification. 1.9.12 diff --git a/borgmatic/hooks/data_source/mongodb.py b/borgmatic/hooks/data_source/mongodb.py index 59811101..37cc63dd 100644 --- a/borgmatic/hooks/data_source/mongodb.py +++ b/borgmatic/hooks/data_source/mongodb.py @@ -89,12 +89,36 @@ def dump_data_sources( return processes +def make_password_config_file(password): + ''' + Given a database password, write it as a MongoDB configuration file to an anonymous pipe and + return its filename. The idea is that this is a more secure way to transmit a password to + MongoDB than providing it directly on the command-line. + + Do not use the returned value for multiple different command invocations. That will not work + because each pipe is "used up" once read. + ''' + logger.debug('Writing MongoDB password to configuration file pipe') + + read_file_descriptor, write_file_descriptor = os.pipe() + os.write(write_file_descriptor, f'password: {password}'.encode('utf-8')) + os.close(write_file_descriptor) + + # This plus subprocess.Popen(..., close_fds=False) in execute.py is necessary for the database + # client child process to inherit the file descriptor. + os.set_inheritable(read_file_descriptor, True) + + return f'/dev/fd/{read_file_descriptor}' + + def build_dump_command(database, config, dump_filename, dump_format): ''' Return the mongodump command from a single database configuration. ''' all_databases = database['name'] == 'all' + password = borgmatic.hooks.credential.parse.resolve_credential(database.get('password'), config) + return ( ('mongodump',) + (('--out', shlex.quote(dump_filename)) if dump_format == 'directory' else ()) @@ -112,18 +136,7 @@ def build_dump_command(database, config, dump_filename, dump_format): if 'username' in database else () ) - + ( - ( - '--password', - shlex.quote( - borgmatic.hooks.credential.parse.resolve_credential( - database['password'], config - ) - ), - ) - if 'password' in database - else () - ) + + (('--config', make_password_config_file(password)) if password else ()) + ( ('--authenticationDatabase', shlex.quote(database['authentication_database'])) if 'authentication_database' in database @@ -251,7 +264,7 @@ def build_restore_command(extract_process, database, config, dump_filename, conn if username: command.extend(('--username', username)) if password: - command.extend(('--password', password)) + command.extend(('--config', make_password_config_file(password))) if 'authentication_database' in database: command.extend(('--authenticationDatabase', database['authentication_database'])) if 'restore_options' in database: diff --git a/tests/unit/hooks/data_source/test_mongodb.py b/tests/unit/hooks/data_source/test_mongodb.py index ea22e824..0cef73f8 100644 --- a/tests/unit/hooks/data_source/test_mongodb.py +++ b/tests/unit/hooks/data_source/test_mongodb.py @@ -127,6 +127,9 @@ def test_dump_data_sources_runs_mongodump_with_username_and_password(): flexmock(module.borgmatic.hooks.credential.parse).should_receive( 'resolve_credential' ).replace_with(lambda value, config: value) + flexmock(module).should_receive('make_password_config_file').with_args('trustsome1').and_return( + '/dev/fd/99' + ) flexmock(module.dump).should_receive('create_named_pipe_for_dump') flexmock(module).should_receive('execute_command').with_args( @@ -134,8 +137,8 @@ def test_dump_data_sources_runs_mongodump_with_username_and_password(): 'mongodump', '--username', 'mongo', - '--password', - 'trustsome1', + '--config', + '/dev/fd/99', '--authenticationDatabase', 'admin', '--db', @@ -243,6 +246,22 @@ def test_dump_data_sources_runs_mongodumpall_for_all_databases(): ) == [process] +def test_make_password_config_file_writes_password_to_pipe(): + read_file_descriptor = 99 + write_file_descriptor = flexmock() + + flexmock(module.os).should_receive('pipe').and_return( + (read_file_descriptor, write_file_descriptor) + ) + flexmock(module.os).should_receive('write').with_args( + write_file_descriptor, b'password: trustsome1' + ).once() + flexmock(module.os).should_receive('close') + flexmock(module.os).should_receive('set_inheritable') + + assert module.make_password_config_file('trustsome1') == '/dev/fd/99' + + def test_build_dump_command_with_username_injection_attack_gets_escaped(): database = {'name': 'test', 'username': 'bob; naughty-command'} flexmock(module.borgmatic.hooks.credential.parse).should_receive( @@ -345,6 +364,9 @@ def test_restore_data_source_dump_runs_mongorestore_with_username_and_password() flexmock(module.borgmatic.hooks.credential.parse).should_receive( 'resolve_credential' ).replace_with(lambda value, config: value) + flexmock(module).should_receive('make_password_config_file').with_args('trustsome1').and_return( + '/dev/fd/99' + ) flexmock(module).should_receive('execute_command_with_processes').with_args( [ 'mongorestore', @@ -352,8 +374,8 @@ def test_restore_data_source_dump_runs_mongorestore_with_username_and_password() '--drop', '--username', 'mongo', - '--password', - 'trustsome1', + '--config', + '/dev/fd/99', '--authenticationDatabase', 'admin', ], @@ -399,6 +421,9 @@ def test_restore_data_source_dump_with_connection_params_uses_connection_params_ flexmock(module.borgmatic.hooks.credential.parse).should_receive( 'resolve_credential' ).replace_with(lambda value, config: value) + flexmock(module).should_receive('make_password_config_file').with_args( + 'clipassword' + ).and_return('/dev/fd/99') flexmock(module).should_receive('execute_command_with_processes').with_args( [ 'mongorestore', @@ -410,8 +435,8 @@ def test_restore_data_source_dump_with_connection_params_uses_connection_params_ 'cliport', '--username', 'cliusername', - '--password', - 'clipassword', + '--config', + '/dev/fd/99', '--authenticationDatabase', 'admin', ], @@ -457,6 +482,9 @@ def test_restore_data_source_dump_without_connection_params_uses_restore_params_ flexmock(module.borgmatic.hooks.credential.parse).should_receive( 'resolve_credential' ).replace_with(lambda value, config: value) + flexmock(module).should_receive('make_password_config_file').with_args( + 'restorepass' + ).and_return('/dev/fd/99') flexmock(module).should_receive('execute_command_with_processes').with_args( [ 'mongorestore', @@ -468,8 +496,8 @@ def test_restore_data_source_dump_without_connection_params_uses_restore_params_ 'restoreport', '--username', 'restoreuser', - '--password', - 'restorepass', + '--config', + '/dev/fd/99', '--authenticationDatabase', 'admin', ],