This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch v2-8-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 3d7ca2b0e08491f22b78029659b53ebe53194073 Author: Aritra Basu <[email protected]> AuthorDate: Mon Jan 29 17:53:45 2024 +0530 Secret masker ignores passwords with special chars (#36692) * Secret masker ignores passwords with special chars #36688 Connection uri's get connection uses quote to change the password and certain other fields to escape special chars due to this, when the connection object is passed through the masker this changed string is skipped. * Added a test for the logging change (cherry picked from commit e8538493a9f74f61834b19e840c7b9987269d45d) --- airflow/models/connection.py | 2 ++ tests/always/test_connection.py | 13 ++++++++----- tests/utils/log/test_secrets_masker.py | 18 +++++++++++++++++- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/airflow/models/connection.py b/airflow/models/connection.py index 0af3f13768..d622a221de 100644 --- a/airflow/models/connection.py +++ b/airflow/models/connection.py @@ -173,6 +173,7 @@ class Connection(Base, LoggingMixin): if self.password: mask_secret(self.password) + mask_secret(quote(self.password)) @staticmethod def _validate_extra(extra, conn_id) -> None: @@ -206,6 +207,7 @@ class Connection(Base, LoggingMixin): def on_db_load(self): if self.password: mask_secret(self.password) + mask_secret(quote(self.password)) def parse_from_uri(self, **uri): """Use uri parameter in constructor, this method is deprecated.""" diff --git a/tests/always/test_connection.py b/tests/always/test_connection.py index e01907a8d3..ab2e26c6ad 100644 --- a/tests/always/test_connection.py +++ b/tests/always/test_connection.py @@ -22,6 +22,7 @@ import os import re from collections import namedtuple from unittest import mock +from urllib.parse import quote import pytest import sqlalchemy @@ -362,6 +363,7 @@ class TestConnection: expected_calls = [] if test_config.test_conn_attributes.get("password"): expected_calls.append(mock.call(test_config.test_conn_attributes["password"])) + expected_calls.append(mock.call(quote(test_config.test_conn_attributes["password"]))) if test_config.test_conn_attributes.get("extra_dejson"): expected_calls.append(mock.call(test_config.test_conn_attributes["extra_dejson"])) @@ -592,7 +594,7 @@ class TestConnection: @mock.patch.dict( "os.environ", { - "AIRFLOW_CONN_TEST_URI": "postgresql://username:[email protected]:5432/the_database", + "AIRFLOW_CONN_TEST_URI": "postgresql://username:[email protected]:5432/the_database", }, ) def test_using_env_var(self): @@ -600,10 +602,10 @@ class TestConnection: assert "ec2.compute.com" == conn.host assert "the_database" == conn.schema assert "username" == conn.login - assert "password" == conn.password + assert "password!" == conn.password assert 5432 == conn.port - self.mask_secret.assert_called_once_with("password") + self.mask_secret.assert_has_calls([mock.call("password!"), mock.call(quote("password!"))]) @mock.patch.dict( "os.environ", @@ -717,7 +719,7 @@ class TestConnection: conn = Connection( conn_id=f"test-{os.getpid()}", conn_type="http", - password="s3cr3t", + password="s3cr3t!", extra='{"apikey":"masked too"}', ) session.add(conn) @@ -733,7 +735,8 @@ class TestConnection: assert self.mask_secret.mock_calls == [ # We should have called it _again_ when loading from the DB - mock.call("s3cr3t"), + mock.call("s3cr3t!"), + mock.call(quote("s3cr3t!")), mock.call({"apikey": "masked too"}), ] finally: diff --git a/tests/utils/log/test_secrets_masker.py b/tests/utils/log/test_secrets_masker.py index 4657a7c1f3..8478d01d2a 100644 --- a/tests/utils/log/test_secrets_masker.py +++ b/tests/utils/log/test_secrets_masker.py @@ -30,6 +30,7 @@ from unittest.mock import patch import pytest from airflow import settings +from airflow.models import Connection from airflow.utils.log.secrets_masker import ( RedactedIO, SecretsMasker, @@ -80,7 +81,6 @@ def logger(caplog): logger.addFilter(filt) filt.add_mask("password") - return logger @@ -340,6 +340,22 @@ class TestSecretsMasker: assert caplog.text == f"INFO State: {expected}\n" assert "TypeError" not in caplog.text + def test_masking_quoted_strings_in_connection(self, logger, caplog): + secrets_masker = [fltr for fltr in logger.filters if isinstance(fltr, SecretsMasker)][0] + with patch("airflow.utils.log.secrets_masker._secrets_masker", return_value=secrets_masker): + test_conn_attributes = dict( + conn_type="scheme", + host="host/location", + schema="schema", + login="user", + password="should_be_hidden!", + port=1234, + extra=None, + ) + conn = Connection(**test_conn_attributes) + logger.info(conn.get_uri()) + assert "should_be_hidden" not in caplog.text + class TestShouldHideValueForKey: @pytest.mark.parametrize(
