From 0bd418836e01ac85ff5b60e4321e2d8881da20c0 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 27 Feb 2025 10:15:45 -0800 Subject: [PATCH] Send MariaDB passwords via anonymous pipe instead of environment variable (#1009) --- borgmatic/borg/environment.py | 2 +- borgmatic/execute.py | 35 +++++---- borgmatic/hooks/data_source/mariadb.py | 103 ++++++++++++++++--------- 3 files changed, 87 insertions(+), 53 deletions(-) diff --git a/borgmatic/borg/environment.py b/borgmatic/borg/environment.py index 19c8ad6e..344478c2 100644 --- a/borgmatic/borg/environment.py +++ b/borgmatic/borg/environment.py @@ -74,7 +74,7 @@ def make_environment(config): 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 + # 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) diff --git a/borgmatic/execute.py b/borgmatic/execute.py index df58c833..20eedb58 100644 --- a/borgmatic/execute.py +++ b/borgmatic/execute.py @@ -266,8 +266,8 @@ def log_command(full_command, input_file=None, output_file=None, environment=Non width=MAX_LOGGED_COMMAND_LENGTH, placeholder=' ...', ) - + (f" < {getattr(input_file, 'name', '')}" if input_file else '') - + (f" > {getattr(output_file, 'name', '')}" if output_file else '') + + (f" < {getattr(input_file, 'name', input_file)}" if input_file else '') + + (f" > {getattr(output_file, 'name', output_file)}" if output_file else '') ) @@ -315,8 +315,8 @@ def execute_command( shell=shell, env=environment, cwd=working_directory, - # Necessary for the passcommand credential hook to work. - close_fds=not bool((environment or {}).get('BORG_PASSPHRASE_FD')), + # Necessary for passing credentials via anonymous pipe. + close_fds=False, ) if not run_to_completion: return process @@ -333,6 +333,7 @@ def execute_command( def execute_command_and_capture_output( full_command, + input_file=None, capture_stderr=False, shell=False, environment=None, @@ -342,28 +343,30 @@ def execute_command_and_capture_output( ): ''' Execute the given command (a sequence of command/argument strings), capturing and returning its - output (stdout). If capture stderr is True, then capture and return stderr in addition to - stdout. If shell is True, execute the command within a shell. If an environment variables dict - is given, then pass it into the command. If a working directory is given, use that as the - present working directory when running the command. If a Borg local path is given, and the - command matches it (regardless of arguments), treat exit code 1 as a warning instead of an - error. But if Borg exit codes are given as a sequence of exit code configuration dicts, then use - that configuration to decide what's an error and what's a warning. + output (stdout). If an input file descriptor is given, then pipe it to the command's stdin. If + capture stderr is True, then capture and return stderr in addition to stdout. If shell is True, + execute the command within a shell. If an environment variables dict is given, then pass it into + the command. If a working directory is given, use that as the present working directory when + running the command. If a Borg local path is given, and the command matches it (regardless of + arguments), treat exit code 1 as a warning instead of an error. But if Borg exit codes are given + as a sequence of exit code configuration dicts, then use that configuration to decide what's an + error and what's a warning. Raise subprocesses.CalledProcessError if an error occurs while running the command. ''' - log_command(full_command, environment=environment) + log_command(full_command, input_file, environment=environment) command = ' '.join(full_command) if shell else full_command try: output = subprocess.check_output( command, + stdin=input_file, stderr=subprocess.STDOUT if capture_stderr else None, shell=shell, env=environment, cwd=working_directory, - # Necessary for the passcommand credential hook to work. - close_fds=not bool((environment or {}).get('BORG_PASSPHRASE_FD')), + # Necessary for passing credentials via anonymous pipe. + close_fds=False, ) except subprocess.CalledProcessError as error: if ( @@ -422,8 +425,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((environment or {}).get('BORG_PASSPHRASE_FD')), + # Necessary for passing credentials via anonymous pipe. + close_fds=False, ) except (subprocess.CalledProcessError, OSError): # Something has gone wrong. So vent each process' output buffer to prevent it from hanging. diff --git a/borgmatic/hooks/data_source/mariadb.py b/borgmatic/hooks/data_source/mariadb.py index b992a944..b6be8b30 100644 --- a/borgmatic/hooks/data_source/mariadb.py +++ b/borgmatic/hooks/data_source/mariadb.py @@ -26,11 +26,56 @@ def make_dump_path(base_directory): # pragma: no cover SYSTEM_DATABASE_NAMES = ('information_schema', 'mysql', 'performance_schema', 'sys') -def database_names_to_dump(database, config, environment, dry_run): +def make_defaults_file_pipe(username=None, password=None): ''' - Given a requested database config and a configuration dict, return the corresponding sequence of - database names to dump. In the case of "all", query for the names of databases on the configured - host and return them, excluding any system databases that will cause problems during restore. + Given a database username and/or password, write it to an anonymous pipe and return its file + descriptor for passing to an executed command. The idea is that this is a more secure way to + transmit credentials to a database client than using an environment variable. + + If no username or password are given, then return None. + + Do not and use this value for multiple different command invocations. That will not work because + each pipe is "used up" once read. + ''' + values = '\n'.join( + ( + (f'user={username}' if username is not None else ''), + (f'password={password}' if password is not None else ''), + ) + ).strip() + + if not values: + return None + + fields_message = ' and '.join( + field_name for field_name in + ( + (f'username ({username})' if username is not None else None), + ('password' if password is not None else None), + ) + if field_name is not None + ) + logger.debug(f'Writing database {fields_message} to defaults extra file pipe') + + read_file_descriptor, write_file_descriptor = os.pipe() + os.write( + write_file_descriptor, f'[client]\n{values}'.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 read_file_descriptor + + +def database_names_to_dump(database, config, username, password, environment, dry_run): + ''' + Given a requested database config, a configuration dict, a database username and password, an + environment dict, and whether this is a dry run, return the corresponding sequence of database + names to dump. In the case of "all", query for the names of databases on the configured host and + return them, excluding any system databases that will cause problems during restore. ''' if database['name'] != 'all': return (database['name'],) @@ -40,24 +85,20 @@ def database_names_to_dump(database, config, environment, dry_run): mariadb_show_command = tuple( shlex.quote(part) for part in shlex.split(database.get('mariadb_command') or 'mariadb') ) + defaults_file_descriptor = make_defaults_file_pipe(username, password) show_command = ( mariadb_show_command + + ((f'--defaults-extra-file=/dev/fd/{defaults_file_descriptor}',) if defaults_file_descriptor else ()) + (tuple(database['list_options'].split(' ')) if 'list_options' in database else ()) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) + (('--protocol', 'tcp') if 'hostname' in database or 'port' in database else ()) - + ( - ( - '--user', - borgmatic.hooks.credential.parse.resolve_credential(database['username'], config), - ) - if 'username' in database - else () - ) + ('--skip-column-names', '--batch') + ('--execute', 'show schemas') ) + logger.debug('Querying for "all" MariaDB databases to dump') + show_output = execute_command_and_capture_output(show_command, environment=environment) return tuple( @@ -68,7 +109,7 @@ def database_names_to_dump(database, config, environment, dry_run): def execute_dump_command( - database, config, dump_path, database_names, environment, dry_run, dry_run_label + database, config, username, password, dump_path, database_names, environment, dry_run, dry_run_label ): ''' Kick off a dump for the given MariaDB database (provided as a configuration dict) to a named @@ -95,21 +136,15 @@ def execute_dump_command( shlex.quote(part) for part in shlex.split(database.get('mariadb_dump_command') or 'mariadb-dump') ) + defaults_file_descriptor = make_defaults_file_pipe(username, password) dump_command = ( mariadb_dump_command + + ((f'--defaults-extra-file=/dev/fd/{defaults_file_descriptor}',) if defaults_file_descriptor else ()) + (tuple(database['options'].split(' ')) if 'options' in database else ()) + (('--add-drop-database',) if database.get('add_drop_database', True) else ()) + (('--host', database['hostname']) if 'hostname' in database else ()) + (('--port', str(database['port'])) if 'port' in database else ()) + (('--protocol', 'tcp') if 'hostname' in database or 'port' in database else ()) - + ( - ( - '--user', - borgmatic.hooks.credential.parse.resolve_credential(database['username'], config), - ) - if 'username' in database - else () - ) + ('--databases',) + database_names + ('--result-file', dump_filename) @@ -165,19 +200,10 @@ def dump_data_sources( for database in databases: dump_path = make_dump_path(borgmatic_runtime_directory) - environment = dict( - os.environ, - **( - { - 'MYSQL_PWD': borgmatic.hooks.credential.parse.resolve_credential( - database['password'], config - ) - } - if 'password' in database - else {} - ), - ) - dump_database_names = database_names_to_dump(database, config, environment, dry_run) + username = borgmatic.hooks.credential.parse.resolve_credential(database.get('username'), config) + password = borgmatic.hooks.credential.parse.resolve_credential(database.get('password'), config) + environment = dict(os.environ) + dump_database_names = database_names_to_dump(database, config, username, password, environment, dry_run) if not dump_database_names: if dry_run: @@ -193,6 +219,8 @@ def dump_data_sources( execute_dump_command( renamed_database, config, + username, + password, dump_path, (dump_name,), environment, @@ -205,6 +233,8 @@ def dump_data_sources( execute_dump_command( database, config, + username, + password, dump_path, dump_database_names, environment, @@ -296,8 +326,10 @@ def restore_data_source_dump( mariadb_restore_command = tuple( shlex.quote(part) for part in shlex.split(data_source.get('mariadb_command') or 'mariadb') ) + defaults_file_descriptor = make_defaults_file_pipe(username, password) restore_command = ( mariadb_restore_command + + ((f'--defaults-extra-file=/dev/fd/{defaults_file_descriptor}',) if defaults_file_descriptor else ()) + ('--batch',) + ( tuple(data_source['restore_options'].split(' ')) @@ -307,9 +339,8 @@ def restore_data_source_dump( + (('--host', hostname) if hostname else ()) + (('--port', str(port)) if port else ()) + (('--protocol', 'tcp') if hostname or port else ()) - + (('--user', username) if username else ()) ) - environment = dict(os.environ, **({'MYSQL_PWD': password} if password else {})) + environment = dict(os.environ) logger.debug(f"Restoring MariaDB database {data_source['name']}{dry_run_label}") if dry_run: