From 78fb0dc20c38770bf0c1d2eea14ea4be5d4e570d Mon Sep 17 00:00:00 2001 From: Kaxil Naik Date: Mon, 12 Oct 2020 14:21:42 +0100 Subject: [PATCH 1/4] Mask Password in Log table when creating users via CLI --- airflow/utils/cli.py | 9 ++++++++- tests/utils/test_cli_util.py | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/airflow/utils/cli.py b/airflow/utils/cli.py index dc0dbeb8ec403..40b1892afe09a 100644 --- a/airflow/utils/cli.py +++ b/airflow/utils/cli.py @@ -105,8 +105,15 @@ def _build_metrics(func_name, namespace): :param namespace: Namespace instance from argparse :return: dict with metrics """ + sensitive_fields = {'-p', '--password'} + full_command = list(sys.argv) + for sensitive_field in sensitive_fields: + if sensitive_field in full_command: + # Mask value passed to Password arg + full_command[full_command.index(sensitive_field) + 1] = "*" * 8 + metrics = {'sub_command': func_name, 'start_datetime': datetime.utcnow(), - 'full_command': '{}'.format(list(sys.argv)), 'user': getpass.getuser()} + 'full_command': '{}'.format(full_command), 'user': getpass.getuser()} if not isinstance(namespace, Namespace): raise ValueError("namespace argument should be argparse.Namespace instance," diff --git a/tests/utils/test_cli_util.py b/tests/utils/test_cli_util.py index ed17a4e4d1138..be7fa60797f5d 100644 --- a/tests/utils/test_cli_util.py +++ b/tests/utils/test_cli_util.py @@ -16,12 +16,16 @@ # specific language governing permissions and limitations # under the License. # - +import json import os +import sys import unittest from argparse import Namespace from contextlib import contextmanager from datetime import datetime +from unittest import mock + +from parameterized import parameterized from airflow import settings from airflow.exceptions import AirflowException @@ -85,6 +89,34 @@ def test_get_dags(self): with self.assertRaises(AirflowException): cli.get_dags(None, "foobar", True) + @parameterized.expand(["--password", "-p"]) + def test_cli_create_user_supplied_password_is_masked(self, password_variant): + + args = [ + 'airflow', 'users', 'create', '--username', 'test2', '--lastname', 'doe', + '--firstname', 'jon', + '--email', 'jdoe@apache.org', '--role', 'Viewer', password_variant, 'test' + ] + + expected_command = [ + 'airflow', 'users', 'create', '--username', 'test2', '--lastname', 'doe', + '--firstname', 'jon', + '--email', 'jdoe@apache.org', '--role', 'Viewer', password_variant, '********' + ] + + exec_date = datetime.utcnow() + namespace = Namespace(dag_id='foo', task_id='bar', subcommand='test', execution_date=exec_date) + with mock.patch.object(sys, "argv", args): + metrics = cli._build_metrics(args[1], namespace) + + self.assertTrue(metrics.get('start_datetime') <= datetime.utcnow()) + + log = metrics.get('log') + command = json.loads(log.extra).get('full_command') # type: str + # Replace single quotes to double quotes to avoid json decode error + command = json.loads(command.replace("'", '"')) + self.assertEqual(command, expected_command) + @contextmanager def fail_action_logger_callback(): From 40b1c0438857926bf7c1b32c70dba73a806edf24 Mon Sep 17 00:00:00 2001 From: Kaxil Naik Date: Mon, 12 Oct 2020 15:09:13 +0100 Subject: [PATCH 2/4] fixup! Mask Password in Log table when creating users via CLI --- airflow/utils/cli.py | 9 ++++++++- tests/utils/test_cli_util.py | 24 ++++++++++++------------ 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/airflow/utils/cli.py b/airflow/utils/cli.py index 40b1892afe09a..56a9186eaba6f 100644 --- a/airflow/utils/cli.py +++ b/airflow/utils/cli.py @@ -107,10 +107,17 @@ def _build_metrics(func_name, namespace): """ sensitive_fields = {'-p', '--password'} full_command = list(sys.argv) - for sensitive_field in sensitive_fields: + for sensitive_field in sensitive_fields: # pylint: disable=too-many-nested-blocks if sensitive_field in full_command: + # For cases when password is passed as "--password xyz" (with space between key and value) # Mask value passed to Password arg full_command[full_command.index(sensitive_field) + 1] = "*" * 8 + elif any(x.startswith(f'{sensitive_field}=') for x in full_command): + # For cases when password is passed as "--password=xyz" (with '=' between key and value) + for idx, command in enumerate(full_command): + if command.startswith(f'{sensitive_field}='): + full_command[idx] = f'{sensitive_field}={"*" * 8}' + break metrics = {'sub_command': func_name, 'start_datetime': datetime.utcnow(), 'full_command': '{}'.format(full_command), 'user': getpass.getuser()} diff --git a/tests/utils/test_cli_util.py b/tests/utils/test_cli_util.py index be7fa60797f5d..73f260939e19b 100644 --- a/tests/utils/test_cli_util.py +++ b/tests/utils/test_cli_util.py @@ -89,20 +89,20 @@ def test_get_dags(self): with self.assertRaises(AirflowException): cli.get_dags(None, "foobar", True) - @parameterized.expand(["--password", "-p"]) - def test_cli_create_user_supplied_password_is_masked(self, password_variant): - - args = [ - 'airflow', 'users', 'create', '--username', 'test2', '--lastname', 'doe', - '--firstname', 'jon', - '--email', 'jdoe@apache.org', '--role', 'Viewer', password_variant, 'test' + @parameterized.expand( + [ + ("--password test", "--password ********"), + ("-p test", "-p ********"), + ("--password=test", "--password=********"), + ("-p=test", "-p=********") ] + ) + def test_cli_create_user_supplied_password_is_masked(self, password_variant, expected_masked_pass): + create_command_w_password = 'airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin' + create_command = f'{create_command_w_password} {password_variant}' + args = create_command.split() - expected_command = [ - 'airflow', 'users', 'create', '--username', 'test2', '--lastname', 'doe', - '--firstname', 'jon', - '--email', 'jdoe@apache.org', '--role', 'Viewer', password_variant, '********' - ] + expected_command = f'{create_command_w_password} {expected_masked_pass}'.split() exec_date = datetime.utcnow() namespace = Namespace(dag_id='foo', task_id='bar', subcommand='test', execution_date=exec_date) From 3e1ce0eb8c6299156f0a475e3ef3f745ea14b98c Mon Sep 17 00:00:00 2001 From: Kaxil Naik Date: Mon, 12 Oct 2020 15:20:40 +0100 Subject: [PATCH 3/4] fixup! fixup! Mask Password in Log table when creating users via CLI --- airflow/utils/cli.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/airflow/utils/cli.py b/airflow/utils/cli.py index 56a9186eaba6f..179436007c560 100644 --- a/airflow/utils/cli.py +++ b/airflow/utils/cli.py @@ -107,17 +107,15 @@ def _build_metrics(func_name, namespace): """ sensitive_fields = {'-p', '--password'} full_command = list(sys.argv) - for sensitive_field in sensitive_fields: # pylint: disable=too-many-nested-blocks - if sensitive_field in full_command: + for idx, command in enumerate(full_command): # pylint: disable=too-many-nested-blocks + if command in sensitive_fields: # For cases when password is passed as "--password xyz" (with space between key and value) - # Mask value passed to Password arg - full_command[full_command.index(sensitive_field) + 1] = "*" * 8 - elif any(x.startswith(f'{sensitive_field}=') for x in full_command): + full_command[idx + 1] = "*" * 8 + else: # For cases when password is passed as "--password=xyz" (with '=' between key and value) - for idx, command in enumerate(full_command): + for sensitive_field in sensitive_fields: if command.startswith(f'{sensitive_field}='): full_command[idx] = f'{sensitive_field}={"*" * 8}' - break metrics = {'sub_command': func_name, 'start_datetime': datetime.utcnow(), 'full_command': '{}'.format(full_command), 'user': getpass.getuser()} From 35b10b7a0f1bc45e784eb7da632f6f42490cd0f5 Mon Sep 17 00:00:00 2001 From: Kaxil Naik Date: Mon, 12 Oct 2020 15:52:05 +0100 Subject: [PATCH 4/4] fixup! fixup! fixup! Mask Password in Log table when creating users via CLI --- airflow/utils/cli.py | 2 +- tests/utils/test_cli_util.py | 32 +++++++++++++++++++++++--------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/airflow/utils/cli.py b/airflow/utils/cli.py index 179436007c560..6e0ea25826cd6 100644 --- a/airflow/utils/cli.py +++ b/airflow/utils/cli.py @@ -105,7 +105,7 @@ def _build_metrics(func_name, namespace): :param namespace: Namespace instance from argparse :return: dict with metrics """ - sensitive_fields = {'-p', '--password'} + sensitive_fields = {'-p', '--password', '--conn-password'} full_command = list(sys.argv) for idx, command in enumerate(full_command): # pylint: disable=too-many-nested-blocks if command in sensitive_fields: diff --git a/tests/utils/test_cli_util.py b/tests/utils/test_cli_util.py index 73f260939e19b..dfa71e10c7413 100644 --- a/tests/utils/test_cli_util.py +++ b/tests/utils/test_cli_util.py @@ -91,18 +91,32 @@ def test_get_dags(self): @parameterized.expand( [ - ("--password test", "--password ********"), - ("-p test", "-p ********"), - ("--password=test", "--password=********"), - ("-p=test", "-p=********") + ( + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin --password test", + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin --password ********" + ), + ( + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin -p test", + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin -p ********" + ), + ( + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin --password=test", + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin --password=********" + ), + ( + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin -p=test", + "airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin -p=********" + ), + ( + "airflow connections add dsfs --conn-login asd --conn-password test --conn-type google", + "airflow connections add dsfs --conn-login asd --conn-password ******** --conn-type google", + ) ] ) - def test_cli_create_user_supplied_password_is_masked(self, password_variant, expected_masked_pass): - create_command_w_password = 'airflow users create -u test2 -l doe -f jon -e jdoe@apache.org -r admin' - create_command = f'{create_command_w_password} {password_variant}' - args = create_command.split() + def test_cli_create_user_supplied_password_is_masked(self, given_command, expected_masked_command): + args = given_command.split() - expected_command = f'{create_command_w_password} {expected_masked_pass}'.split() + expected_command = expected_masked_command.split() exec_date = datetime.utcnow() namespace = Namespace(dag_id='foo', task_id='bar', subcommand='test', execution_date=exec_date)