From 37274a9e54b828a8d6dc8eca3db99c024167efed Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 19 Jun 2019 21:33:25 +0200 Subject: [PATCH 01/11] Add redacting mechanism for secrets, using a custom Formatter --- src/yunohost/log.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index dd3bbd8b3..553822ff3 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -289,6 +289,19 @@ def is_unit_operation(entities=['app', 'domain', 'service', 'user'], return decorate +class RedactingFormatter(Formatter): + + def __init__(self, format_string, data_to_redact): + super(RedactingFormatter, self).__init__(format_string) + self.data_to_redact = data_to_redact + + def format(self, record): + msg = super(RedactingFormatter, self).format(record) + for data in self.data_to_redact: + msg = msg.replace(data, "**********") + return msg + + class OperationLogger(object): """ @@ -309,6 +322,7 @@ class OperationLogger(object): self.ended_at = None self.logger = None self._name = None + self.data_to_redact = [] self.path = OPERATIONS_PATH @@ -345,9 +359,12 @@ class OperationLogger(object): Register log with a handler connected on log system """ - # TODO add a way to not save password on app installation self.file_handler = FileHandler(self.log_path) - self.file_handler.formatter = Formatter('%(asctime)s: %(levelname)s - %(message)s') + # We use a custom formatter that's able to redact all stuff in self.data_to_redact + # N.B. : the stubtle thing here is that the class will remember a pointer to the list, + # so we can directly append stuff to self.data_to_redact and that'll be automatically + # propagated to the RedactingFormatter + self.file_handler.formatter = RedactingFormatter('%(asctime)s: %(levelname)s - %(message)s', self.data_to_redact) # Listen to the root logger self.logger = getLogger('yunohost') From b01bf434750faddabe97b44b063a2a3a8b536af2 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 19 Jun 2019 22:03:52 +0200 Subject: [PATCH 02/11] Change _parse_action_args to also return the arg type :/ --- src/yunohost/app.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 1f172a267..aac7455dd 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -362,10 +362,10 @@ def app_info(app, show_status=False, raw=False): ret['upgradable'] = upgradable ret['change_url'] = os.path.exists(os.path.join(app_setting_path, "scripts", "change_url")) - + with open(os.path.join(APPS_SETTING_PATH, app, 'manifest.json')) as json_manifest: manifest = json.load(json_manifest) - + ret['version'] = manifest.get('version', '-') return ret @@ -490,7 +490,7 @@ def app_change_url(operation_logger, app, domain, path): # Retrieve arguments list for change_url script # TODO: Allow to specify arguments args_odict = _parse_args_from_manifest(manifest, 'change_url') - args_list = args_odict.values() + args_list = [ value[0] for value in args_odict.values() ] args_list.append(app) # Prepare env. var. to pass to script @@ -639,7 +639,7 @@ def app_upgrade(app=[], url=None, file=None): # Retrieve arguments list for upgrade script # TODO: Allow to specify arguments args_odict = _parse_args_from_manifest(manifest, 'upgrade') - args_list = args_odict.values() + args_list = [ value[0] for value in args_odict.values() ] args_list.append(app_instance_name) # Prepare env. var. to pass to script @@ -797,7 +797,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu args_dict = {} if not args else \ dict(urlparse.parse_qsl(args, keep_blank_values=True)) args_odict = _parse_args_from_manifest(manifest, 'install', args=args_dict) - args_list = args_odict.values() + args_list = [ value[0] for value in args_odict.values() ] args_list.append(app_instance_name) # Prepare env. var. to pass to script @@ -1537,7 +1537,7 @@ def app_action_run(app, action, args=None): # Retrieve arguments list for install script args_dict = dict(urlparse.parse_qsl(args, keep_blank_values=True)) if args else {} args_odict = _parse_args_for_action(actions[action], args=args_dict) - args_list = args_odict.values() + args_list = [ value[0] for value in args_odict.values() ] app_id, app_instance_nb = _parse_app_instance_name(app) @@ -2272,7 +2272,7 @@ def _parse_action_args_in_yunohost_format(args, action_args): if arg.get("optional", False): # Argument is optional, keep an empty value # and that's all for this arg ! - args_dict[arg_name] = '' + args_dict[arg_name] = ('', arg_type) continue else: # The argument is required ! @@ -2310,22 +2310,20 @@ def _parse_action_args_in_yunohost_format(args, action_args): raise YunohostError('pattern_password_app', forbidden_chars=forbidden_chars) from yunohost.utils.password import assert_password_is_strong_enough assert_password_is_strong_enough('user', arg_value) - args_dict[arg_name] = arg_value + args_dict[arg_name] = (arg_value, arg_type) # END loop over action_args... # If there's only one "domain" and "path", validate that domain/path # is an available url and normalize the path. - domain_args = [arg["name"] for arg in action_args - if arg.get("type", "string") == "domain"] - path_args = [arg["name"] for arg in action_args - if arg.get("type", "string") == "path"] + domain_args = [ (name, value[0]) for name, value in args_dict.items() if value[1] == "domain" ] + path_args = [ (name, value[0]) for name, value in args_dict.items() if value[1] == "path" ] if len(domain_args) == 1 and len(path_args) == 1: - domain = args_dict[domain_args[0]] - path = args_dict[path_args[0]] + domain = domain_args[0][1] + path = path_args[0][1] domain, path = _normalize_domain_path(domain, path) # Check the url is available @@ -2344,7 +2342,7 @@ def _parse_action_args_in_yunohost_format(args, action_args): # (We save this normalized path so that the install script have a # standard path format to deal with no matter what the user inputted) - args_dict[path_args[0]] = path + args_dict[path_args[0][0]] = (path, "path") return args_dict @@ -2359,8 +2357,8 @@ def _make_environment_dict(args_dict, prefix="APP_ARG_"): """ env_dict = {} - for arg_name, arg_value in args_dict.items(): - env_dict["YNH_%s%s" % (prefix, arg_name.upper())] = arg_value + for arg_name, arg_value_and_type in args_dict.items(): + env_dict["YNH_%s%s" % (prefix, arg_name.upper())] = arg_value_and_type[0] return env_dict From e42ce996603818438fc026dd7f1009a83982430a Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 19 Jun 2019 22:24:06 +0200 Subject: [PATCH 03/11] Add all password-type argument as data to redact in app install --- src/yunohost/app.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index aac7455dd..0af562add 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -808,6 +808,9 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu # Start register change on system operation_logger.extra.update({'env': env_dict}) + # Tell the operation_logger to redact all password-type args + data_to_redact = [ value[0] for value in args_odict.values() if value[1] == "password" ] + operation_logger.data_to_redact.extend(data_to_redact) operation_logger.related_to = [s for s in operation_logger.related_to if s[0] != "app"] operation_logger.related_to.append(("app", app_id)) operation_logger.start() From 4269c10b80cdab105725cae870de61daa899b99a Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 19 Jun 2019 22:24:41 +0200 Subject: [PATCH 04/11] Also redact metadata file --- src/yunohost/log.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index 553822ff3..0caae534f 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -375,8 +375,11 @@ class OperationLogger(object): Write or rewrite the metadata file with all metadata known """ + dump = yaml.safe_dump(self.metadata, default_flow_style=False) + for data in self.data_to_redact: + dump = dump.replace(data, "**********") with open(self.md_path, 'w') as outfile: - yaml.safe_dump(self.metadata, outfile, default_flow_style=False) + outfile.write(dump) @property def name(self): From b9e4b91f1bbeb93c4dfbe3cd9088212afd685186 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 19 Jun 2019 23:11:24 +0200 Subject: [PATCH 05/11] Add /etc/yunohost/mysql as data to redact --- src/yunohost/log.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index 0caae534f..3631f6bf0 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -323,6 +323,7 @@ class OperationLogger(object): self.logger = None self._name = None self.data_to_redact = [] + self.data_to_redact.append(open("/etc/yunohost/mysql", "r").read().strip()) self.path = OPERATIONS_PATH From b26ec9c2fc9482b0c97bb3e432c08ccb7529dfa0 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 19 Jun 2019 23:14:48 +0200 Subject: [PATCH 06/11] Find data to redact on-the-fly corresponding to common stuff --- src/yunohost/log.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index 3631f6bf0..6bb1a4445 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -25,6 +25,7 @@ """ import os +import re import yaml import collections @@ -297,10 +298,22 @@ class RedactingFormatter(Formatter): def format(self, record): msg = super(RedactingFormatter, self).format(record) + self.identify_data_to_redact(msg) for data in self.data_to_redact: msg = msg.replace(data, "**********") return msg + def identify_data_to_redact(self, record): + + # Wrapping this in a try/except because we don't want this to + # break everything in case it fails miserably for some reason :s + try: + match = re.search(r'(db_pwd|password)=(\S{3,})$', record.strip()) + if match and match.group(2) not in self.data_to_redact: + self.data_to_redact.append(match.group(2)) + except Exception as e: + logger.warning("Failed to parse line to try to identify data to redact ... : %s" % e) + class OperationLogger(object): From 6a00aac13acff6faebe60f55a4c59fcb554093ef Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 27 Jun 2019 03:16:46 +0200 Subject: [PATCH 07/11] Catch pwd, pass and password as patterns for password --- src/yunohost/log.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index 6bb1a4445..32b62cb98 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -308,7 +308,9 @@ class RedactingFormatter(Formatter): # Wrapping this in a try/except because we don't want this to # break everything in case it fails miserably for some reason :s try: - match = re.search(r'(db_pwd|password)=(\S{3,})$', record.strip()) + # This matches stuff like db_pwd=the_secret or admin_password=other_secret + # (the secret part being at least 3 chars to avoid catching some lines like just "db_pwd=") + match = re.search(r'(pwd|pass|password)=(\S{3,})$', record.strip()) if match and match.group(2) not in self.data_to_redact: self.data_to_redact.append(match.group(2)) except Exception as e: From 5c7bad84728d0f9790f7ebbabe592ad46fdafbf4 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 27 Jun 2019 03:18:08 +0200 Subject: [PATCH 08/11] Why not use the global variable here ... --- data/helpers.d/postgresql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data/helpers.d/postgresql b/data/helpers.d/postgresql index 2b0a86308..3126639a6 100644 --- a/data/helpers.d/postgresql +++ b/data/helpers.d/postgresql @@ -266,7 +266,7 @@ ynh_psql_test_if_first_run() { echo "PostgreSQL is already installed, no need to create master password" else local pgsql="$(ynh_string_random)" - echo "$pgsql" >/etc/yunohost/psql + echo "$pgsql" >$PSQL_ROOT_PWD_FILE if [ -e /etc/postgresql/9.4/ ]; then local pg_hba=/etc/postgresql/9.4/main/pg_hba.conf From 5f1988c2ca195822ef5d742ab4d4b25ec7f7a574 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 27 Jun 2019 03:31:03 +0200 Subject: [PATCH 09/11] Change the variable name so that it's catched by the redacting mechanism --- data/helpers.d/postgresql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/data/helpers.d/postgresql b/data/helpers.d/postgresql index 3126639a6..2c1882b8e 100644 --- a/data/helpers.d/postgresql +++ b/data/helpers.d/postgresql @@ -265,8 +265,8 @@ ynh_psql_test_if_first_run() { if [ -f "$PSQL_ROOT_PWD_FILE" ]; then echo "PostgreSQL is already installed, no need to create master password" else - local pgsql="$(ynh_string_random)" - echo "$pgsql" >$PSQL_ROOT_PWD_FILE + local psql_root_password="$(ynh_string_random)" + echo "$psql_root_password" >$PSQL_ROOT_PWD_FILE if [ -e /etc/postgresql/9.4/ ]; then local pg_hba=/etc/postgresql/9.4/main/pg_hba.conf @@ -280,7 +280,7 @@ ynh_psql_test_if_first_run() { ynh_systemd_action --service_name=postgresql --action=start - sudo --login --user=postgres psql -c"ALTER user postgres WITH PASSWORD '$pgsql'" postgres + sudo --login --user=postgres psql -c"ALTER user postgres WITH PASSWORD '$psql_root_password'" postgres # force all user to connect to local database using passwords # https://www.postgresql.org/docs/current/static/auth-pg-hba-conf.html#EXAMPLE-PG-HBA.CONF From aa621abf5775644d6a8c8714601e456fc6848f0d Mon Sep 17 00:00:00 2001 From: Bram Date: Thu, 27 Jun 2019 05:11:04 +0200 Subject: [PATCH 10/11] [doc] typo Co-Authored-By: decentral1se --- src/yunohost/log.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index 32b62cb98..c228c2853 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -377,7 +377,7 @@ class OperationLogger(object): self.file_handler = FileHandler(self.log_path) # We use a custom formatter that's able to redact all stuff in self.data_to_redact - # N.B. : the stubtle thing here is that the class will remember a pointer to the list, + # N.B. : the subtle thing here is that the class will remember a pointer to the list, # so we can directly append stuff to self.data_to_redact and that'll be automatically # propagated to the RedactingFormatter self.file_handler.formatter = RedactingFormatter('%(asctime)s: %(levelname)s - %(message)s', self.data_to_redact) From 18f3540e2f89bf2631f147a19092efee0030ed9f Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 2 Jul 2019 15:55:31 +0200 Subject: [PATCH 11/11] Also redact psql password if it exists --- src/yunohost/log.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/yunohost/log.py b/src/yunohost/log.py index c228c2853..decb546f4 100644 --- a/src/yunohost/log.py +++ b/src/yunohost/log.py @@ -338,7 +338,10 @@ class OperationLogger(object): self.logger = None self._name = None self.data_to_redact = [] - self.data_to_redact.append(open("/etc/yunohost/mysql", "r").read().strip()) + + for filename in ["/etc/yunohost/mysql", "/etc/yunohost/psql"]: + if os.path.exists(filename): + self.data_to_redact.append(read_file(filename).strip()) self.path = OPERATIONS_PATH