From f601cfccf1d8e2e314a270943d91e8aa1932f2a4 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Thu, 31 Aug 2017 11:06:28 +1000 Subject: [PATCH] Support RFC1738 quoted chars in passwords In the bug, a user tried setting a devstack password with a "@" in it. As it turns out, sqlalchmey turns the connection-string into a sqlalchemy.engine.url.URL object [1] which returns a RFC1738 quoted string. However, alembic's set_main_option [2] uses python string-interpolation which interprets '%' characters. This means you end up with an interpolation traceback when using any quoted character (':@/') in a user/password (more likely password). Avoid this by ensuring the URL is safe for python interpolation in set_main_option by replacing '%' -> '%%'. I convinced myself this is safe because sqlalchemy correctly parses the quoted and unquoted versions just the same --- >>> str(sqlalchemy.engine.url.make_url('mysql+pymysql://foo:crazy:@/pw@/moo')) 'mysql+pymysql://foo:crazy%3A%40%2Fpw@/moo' >>> str(sqlalchemy.engine.url.make_url('mysql+pymysql://foo:crazy%3A%40%2Fpw@/moo')) 'mysql+pymysql://foo:crazy%3A%40%2Fpw@/moo' --- A test is added [1] https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/engine/url.py [2] http://alembic.zzzcomputing.com/en/latest/api/config.html#alembic.config.Config.set_main_option Change-Id: I3ef7e3e539e35ce040573f2044ab6eb3c990200a Closes-Bug: #1695299 --- glance/db/sqlalchemy/alembic_migrations/__init__.py | 10 +++++++++- glance/tests/unit/test_manage.py | 13 +++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/glance/db/sqlalchemy/alembic_migrations/__init__.py b/glance/db/sqlalchemy/alembic_migrations/__init__.py index f35e47d0a0..ef9faf38cd 100644 --- a/glance/db/sqlalchemy/alembic_migrations/__init__.py +++ b/glance/db/sqlalchemy/alembic_migrations/__init__.py @@ -35,7 +35,15 @@ def get_alembic_config(engine=None): config = alembic_config.Config(os.path.abspath(ini_path)) if engine is None: engine = db_api.get_engine() - config.set_main_option('sqlalchemy.url', str(engine.url)) + # str(sqlalchemy.engine.url.URL) returns a RFC-1738 quoted URL. + # This means that a password like "foo@" will be turned into + # "foo%40". This causes a problem for set_main_option() here + # because that uses ConfigParser.set, which (by design) uses + # *python* interpolation to write the string out ... where "%" is + # the special python interpolation character! Avoid this + # mis-match by quoting all %'s for the set below. + quoted_engine_url = str(engine.url).replace('%', '%%') + config.set_main_option('sqlalchemy.url', quoted_engine_url) return config diff --git a/glance/tests/unit/test_manage.py b/glance/tests/unit/test_manage.py index b92c9a4a39..4da916bdad 100644 --- a/glance/tests/unit/test_manage.py +++ b/glance/tests/unit/test_manage.py @@ -21,9 +21,11 @@ from six.moves import StringIO from glance.cmd import manage from glance.common import exception +from glance.db.sqlalchemy import alembic_migrations from glance.db.sqlalchemy import api as db_api from glance.db.sqlalchemy import metadata as db_metadata from glance.tests import utils as test_utils +from sqlalchemy.engine.url import make_url as sqlalchemy_make_url class TestManageBase(test_utils.BaseTestCase): @@ -166,6 +168,17 @@ class TestManage(TestManageBase): self.output = StringIO() self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.output)) + def test_db_complex_password(self): + engine = mock.Mock() + # See comments in get_alembic_config; make an engine url with + # password characters that will be escaped, to ensure the + # resulting value makes it into alembic unaltered. + engine.url = sqlalchemy_make_url( + 'mysql+pymysql://username:pw@%/!#$()@host:1234/dbname') + alembic_config = alembic_migrations.get_alembic_config(engine) + self.assertEqual(str(engine.url), + alembic_config.get_main_option('sqlalchemy.url')) + @mock.patch('glance.db.sqlalchemy.api.get_engine') @mock.patch( 'glance.db.sqlalchemy.alembic_migrations.data_migrations.'