From 3333c6623fa45bcbc7fabd061184a79b7b7f2fa6 Mon Sep 17 00:00:00 2001 From: Yassen Damyanov Date: Thu, 22 Sep 2022 12:12:28 -0400 Subject: [PATCH] Tighten password security by removing `URL.__str__` For improved security, the :class:`_url.URL` object will now use password obfuscation by default when ``str(url)`` is called. To stringify a URL with cleartext password, the :meth:`_url.URL.render_as_string` may be used, passing the :paramref:`_url.URL.render_as_string.hide_password` parameter as ``False``. Thanks to our contributors for this pull request. Fixes: #8567 Closes: #8563 Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/8563 Pull-request-sha: d1f1127f753849eb70b8d6cc64badf34e1b9219b Change-Id: If756c8073ff99ac83876d9833c8fe1d7c76211f9 --- doc/build/changelog/unreleased_20/8567.rst | 13 ++++ doc/build/changelog/whatsnew_20.rst | 37 ++++++++++ lib/sqlalchemy/engine/url.py | 3 - lib/sqlalchemy/testing/plugin/plugin_base.py | 5 +- lib/sqlalchemy/testing/provision.py | 4 +- test/dialect/oracle/test_dialect.py | 2 +- test/engine/test_parseconnect.py | 76 +++++++++++++++----- test/ext/asyncio/test_engine_py3k.py | 4 +- 8 files changed, 119 insertions(+), 25 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/8567.rst diff --git a/doc/build/changelog/unreleased_20/8567.rst b/doc/build/changelog/unreleased_20/8567.rst new file mode 100644 index 0000000000..e6c37c2b25 --- /dev/null +++ b/doc/build/changelog/unreleased_20/8567.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, engine + :tickets: 8567 + + For improved security, the :class:`_url.URL` object will now use password + obfuscation by default when ``str(url)`` is called. To stringify a URL with + cleartext password, the :meth:`_url.URL.render_as_string` may be used, + passing the :paramref:`_url.URL.render_as_string.hide_password` parameter + as ``False``. Thanks to our contributors for this pull request. + + .. seealso:: + + :ref:`change_8567` diff --git a/doc/build/changelog/whatsnew_20.rst b/doc/build/changelog/whatsnew_20.rst index 794f6d3cc3..7ac8a7cf9d 100644 --- a/doc/build/changelog/whatsnew_20.rst +++ b/doc/build/changelog/whatsnew_20.rst @@ -1038,6 +1038,43 @@ not expected to have significant effects on backwards compatibility. .. _Cython: https://cython.org/ +.. _change_8567: + +``str(engine.url)`` will obfuscate the password by default +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +To avoid leakage of database passwords, calling ``str()`` on a +:class:`.URL` will now enable the password obfuscation feature by default. +Previously, this obfuscation would be in place for ``__repr__()`` calls +but not ``__str__()``. This change will impact applications and test suites +that attempt to invoke :func:`_sa.create_engine` given the stringified URL +from another engine, such as:: + + >>> e1 = create_engine("postgresql+psycopg2://scott:tiger@localhost/test") + >>> e2 = create_engine(str(e1.url)) + +The above engine ``e2`` will not have the correct password; it will have the +obfuscated string ``"***"``. + +The preferred approach for the above pattern is to pass the +:class:`.URL` object directly, there's no need to stringify:: + + >>> e1 = create_engine("postgresql+psycopg2://scott:tiger@localhost/test") + >>> e2 = create_engine(e1.url) + +Otherwise, for a stringified URL with cleartext password, use the +:meth:`_url.URL.render_as_string` method, passing the +:paramref:`_url.URL.render_as_string.hide_password` parameter +as ``False``:: + + >>> e1 = create_engine("postgresql+psycopg2://scott:tiger@localhost/test") + >>> url_string = e1.url.render_as_string(hide_password=False) + >>> e2 = create_engine(url_string) + + +:ticket:`8567` + + .. _change_6980: "with_variant()" clones the original TypeEngine rather than changing the type diff --git a/lib/sqlalchemy/engine/url.py b/lib/sqlalchemy/engine/url.py index 8d80cfd1c8..cca31edd0f 100644 --- a/lib/sqlalchemy/engine/url.py +++ b/lib/sqlalchemy/engine/url.py @@ -642,9 +642,6 @@ class URL(NamedTuple): ) return s - def __str__(self) -> str: - return self.render_as_string(hide_password=False) - def __repr__(self) -> str: return self.render_as_string() diff --git a/lib/sqlalchemy/testing/plugin/plugin_base.py b/lib/sqlalchemy/testing/plugin/plugin_base.py index 494e7d5ab5..b90a2ec58c 100644 --- a/lib/sqlalchemy/testing/plugin/plugin_base.py +++ b/lib/sqlalchemy/testing/plugin/plugin_base.py @@ -436,7 +436,10 @@ def _engine_uri(options, file_config): if options.write_idents and provision.FOLLOWER_IDENT: with open(options.write_idents, "a") as file_: - file_.write(provision.FOLLOWER_IDENT + " " + db_url + "\n") + file_.write( + f"{provision.FOLLOWER_IDENT} " + f"{db_url.render_as_string(hide_password=False)}\n" + ) cfg = provision.setup_config( db_url, options, file_config, provision.FOLLOWER_IDENT diff --git a/lib/sqlalchemy/testing/provision.py b/lib/sqlalchemy/testing/provision.py index 12448b2fe1..7ba89b505a 100644 --- a/lib/sqlalchemy/testing/provision.py +++ b/lib/sqlalchemy/testing/provision.py @@ -167,7 +167,7 @@ def _generate_driver_urls(url, extra_drivers): extra_drivers.discard(main_driver) url = generate_driver_url(url, main_driver, "") - yield str(url) + yield url for drv in list(extra_drivers): @@ -183,7 +183,7 @@ def _generate_driver_urls(url, extra_drivers): if new_url: extra_drivers.remove(drv) - yield str(new_url) + yield new_url @register.init diff --git a/test/dialect/oracle/test_dialect.py b/test/dialect/oracle/test_dialect.py index e3101840af..9c2496b05d 100644 --- a/test/dialect/oracle/test_dialect.py +++ b/test/dialect/oracle/test_dialect.py @@ -100,7 +100,7 @@ class OracledbMode(fixtures.TestBase): def _run_in_process(self, fn): ctx = get_context("spawn") queue = ctx.Queue() - process = ctx.Process(target=fn, args=(str(config.db_url), queue)) + process = ctx.Process(target=fn, args=(config.db_url, queue)) try: process.start() process.join(10) diff --git a/test/engine/test_parseconnect.py b/test/engine/test_parseconnect.py index 23be61aaf8..f571b4bab7 100644 --- a/test/engine/test_parseconnect.py +++ b/test/engine/test_parseconnect.py @@ -83,36 +83,57 @@ class URLTest(fixtures.TestBase): "E:/work/src/LEM/db/hello.db", None, ), u.database - eq_(str(u), text) + eq_(u.render_as_string(hide_password=False), text) def test_rfc1738_password(self): u = url.make_url("dbtype://user:pass word + other%3Awords@host/dbname") eq_(u.password, "pass word + other:words") - eq_(str(u), "dbtype://user:pass word + other%3Awords@host/dbname") + eq_(str(u), "dbtype://user:***@host/dbname") + eq_( + u.render_as_string(hide_password=False), + "dbtype://user:pass word + other%3Awords@host/dbname", + ) u = url.make_url( "dbtype://username:apples%2Foranges@hostspec/database" ) eq_(u.password, "apples/oranges") - eq_(str(u), "dbtype://username:apples%2Foranges@hostspec/database") + eq_(str(u), "dbtype://username:***@hostspec/database") + eq_( + u.render_as_string(hide_password=False), + "dbtype://username:apples%2Foranges@hostspec/database", + ) u = url.make_url( "dbtype://username:apples%40oranges%40%40@hostspec/database" ) eq_(u.password, "apples@oranges@@") + eq_(str(u), "dbtype://username:***@hostspec/database") eq_( - str(u), + u.render_as_string(hide_password=False), "dbtype://username:apples%40oranges%40%40@hostspec/database", ) u = url.make_url("dbtype://username%40:@hostspec/database") eq_(u.password, "") eq_(u.username, "username@") - eq_(str(u), "dbtype://username%40:@hostspec/database") + eq_( + # Do not reveal an empty password + str(u), + "dbtype://username%40:***@hostspec/database", + ) + eq_( + u.render_as_string(hide_password=False), + "dbtype://username%40:@hostspec/database", + ) u = url.make_url("dbtype://username:pass%2Fword@hostspec/database") eq_(u.password, "pass/word") - eq_(str(u), "dbtype://username:pass%2Fword@hostspec/database") + eq_(str(u), "dbtype://username:***@hostspec/database") + eq_( + u.render_as_string(hide_password=False), + "dbtype://username:pass%2Fword@hostspec/database", + ) def test_password_custom_obj(self): class SecurePassword(str): @@ -128,58 +149,76 @@ class URLTest(fixtures.TestBase): ) eq_(u.password, "secured_password") - eq_(str(u), "dbtype://x:secured_password@localhost") + eq_( + u.render_as_string(hide_password=False), + "dbtype://x:secured_password@localhost", + ) # test in-place modification sp.value = "new_secured_password" eq_(u.password, sp) - eq_(str(u), "dbtype://x:new_secured_password@localhost") + eq_( + u.render_as_string(hide_password=False), + "dbtype://x:new_secured_password@localhost", + ) u = u.set(password="hi") eq_(u.password, "hi") - eq_(str(u), "dbtype://x:hi@localhost") + eq_(u.render_as_string(hide_password=False), "dbtype://x:hi@localhost") u = u._replace(password=None) is_(u.password, None) - eq_(str(u), "dbtype://x@localhost") + eq_(u.render_as_string(hide_password=False), "dbtype://x@localhost") def test_query_string(self): u = url.make_url("dialect://user:pass@host/db?arg1=param1&arg2=param2") eq_(u.query, {"arg1": "param1", "arg2": "param2"}) - eq_(str(u), "dialect://user:pass@host/db?arg1=param1&arg2=param2") + eq_( + u.render_as_string(hide_password=False), + "dialect://user:pass@host/db?arg1=param1&arg2=param2", + ) u = url.make_url("dialect://user:pass@host/?arg1=param1&arg2=param2") eq_(u.query, {"arg1": "param1", "arg2": "param2"}) eq_(u.database, "") - eq_(str(u), "dialect://user:pass@host/?arg1=param1&arg2=param2") + eq_( + u.render_as_string(hide_password=False), + "dialect://user:pass@host/?arg1=param1&arg2=param2", + ) u = url.make_url("dialect://user:pass@host?arg1=param1&arg2=param2") eq_(u.query, {"arg1": "param1", "arg2": "param2"}) eq_(u.database, None) - eq_(str(u), "dialect://user:pass@host?arg1=param1&arg2=param2") + eq_( + u.render_as_string(hide_password=False), + "dialect://user:pass@host?arg1=param1&arg2=param2", + ) u = url.make_url( "dialect://user:pass@host:450?arg1=param1&arg2=param2" ) eq_(u.port, 450) eq_(u.query, {"arg1": "param1", "arg2": "param2"}) - eq_(str(u), "dialect://user:pass@host:450?arg1=param1&arg2=param2") + eq_( + u.render_as_string(hide_password=False), + "dialect://user:pass@host:450?arg1=param1&arg2=param2", + ) u = url.make_url( "dialect://user:pass@host/db?arg1=param1&arg2=param2&arg2=param3" ) eq_(u.query, {"arg1": "param1", "arg2": ("param2", "param3")}) eq_( - str(u), + u.render_as_string(hide_password=False), "dialect://user:pass@host/db?arg1=param1&arg2=param2&arg2=param3", ) test_url = "dialect://user:pass@host/db?arg1%3D=param1&arg2=param+2" u = url.make_url(test_url) eq_(u.query, {"arg1=": "param1", "arg2": "param 2"}) - eq_(str(u), test_url) + eq_(u.render_as_string(hide_password=False), test_url) def test_comparison(self): common_url = ( @@ -727,7 +766,10 @@ class CreateEngineTest(fixtures.TestBase): assert e.url.drivername == e2.url.drivername == "mysql+mysqldb" assert e.url.username == e2.url.username == "scott" assert e2.url is u - assert str(u) == "mysql+mysqldb://scott:tiger@localhost/test" + eq_( + u.render_as_string(hide_password=False), + "mysql+mysqldb://scott:tiger@localhost/test", + ) assert repr(u) == "mysql+mysqldb://scott:***@localhost/test" assert repr(e) == "Engine(mysql+mysqldb://scott:***@localhost/test)" assert repr(e2) == "Engine(mysql+mysqldb://scott:***@localhost/test)" diff --git a/test/ext/asyncio/test_engine_py3k.py b/test/ext/asyncio/test_engine_py3k.py index 0269aaaeec..cdf70ca678 100644 --- a/test/ext/asyncio/test_engine_py3k.py +++ b/test/ext/asyncio/test_engine_py3k.py @@ -635,7 +635,9 @@ class AsyncEngineTest(EngineFixture): def test_async_engine_from_config(self): config = { - "sqlalchemy.url": str(testing.db.url), + "sqlalchemy.url": testing.db.url.render_as_string( + hide_password=False + ), "sqlalchemy.echo": "true", } engine = async_engine_from_config(config) -- 2.47.2