summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorIan Wienand <iwienand@redhat.com>2017-08-31 11:06:28 +1000
committerIan Wienand <iwienand@redhat.com>2018-08-13 14:44:32 +1000
commitf601cfccf1d8e2e314a270943d91e8aa1932f2a4 (patch)
tree6668e9bc9124841a9622ad1267d8dc6c617f06c4
parent52d032484882fbe2f139141bbf5d47e6f4688730 (diff)
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
Notes
Notes (review): Code-Review+2: Brian Rosmaita <rosmaita.fossdev@gmail.com> Code-Review+1: Leopard Ma <mabao@inspur.com> Code-Review+2: Erno Kuvaja <jokke@usr.fi> Workflow+1: Erno Kuvaja <jokke@usr.fi> Verified+2: Zuul Submitted-by: Zuul Submitted-at: Wed, 15 Aug 2018 21:48:06 +0000 Reviewed-on: https://review.openstack.org/499410 Project: openstack/glance Branch: refs/heads/master
-rw-r--r--glance/db/sqlalchemy/alembic_migrations/__init__.py10
-rw-r--r--glance/tests/unit/test_manage.py13
2 files changed, 22 insertions, 1 deletions
diff --git a/glance/db/sqlalchemy/alembic_migrations/__init__.py b/glance/db/sqlalchemy/alembic_migrations/__init__.py
index f35e47d..ef9faf3 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):
35 config = alembic_config.Config(os.path.abspath(ini_path)) 35 config = alembic_config.Config(os.path.abspath(ini_path))
36 if engine is None: 36 if engine is None:
37 engine = db_api.get_engine() 37 engine = db_api.get_engine()
38 config.set_main_option('sqlalchemy.url', str(engine.url)) 38 # str(sqlalchemy.engine.url.URL) returns a RFC-1738 quoted URL.
39 # This means that a password like "foo@" will be turned into
40 # "foo%40". This causes a problem for set_main_option() here
41 # because that uses ConfigParser.set, which (by design) uses
42 # *python* interpolation to write the string out ... where "%" is
43 # the special python interpolation character! Avoid this
44 # mis-match by quoting all %'s for the set below.
45 quoted_engine_url = str(engine.url).replace('%', '%%')
46 config.set_main_option('sqlalchemy.url', quoted_engine_url)
39 return config 47 return config
40 48
41 49
diff --git a/glance/tests/unit/test_manage.py b/glance/tests/unit/test_manage.py
index b92c9a4..4da916b 100644
--- a/glance/tests/unit/test_manage.py
+++ b/glance/tests/unit/test_manage.py
@@ -21,9 +21,11 @@ from six.moves import StringIO
21 21
22from glance.cmd import manage 22from glance.cmd import manage
23from glance.common import exception 23from glance.common import exception
24from glance.db.sqlalchemy import alembic_migrations
24from glance.db.sqlalchemy import api as db_api 25from glance.db.sqlalchemy import api as db_api
25from glance.db.sqlalchemy import metadata as db_metadata 26from glance.db.sqlalchemy import metadata as db_metadata
26from glance.tests import utils as test_utils 27from glance.tests import utils as test_utils
28from sqlalchemy.engine.url import make_url as sqlalchemy_make_url
27 29
28 30
29class TestManageBase(test_utils.BaseTestCase): 31class TestManageBase(test_utils.BaseTestCase):
@@ -166,6 +168,17 @@ class TestManage(TestManageBase):
166 self.output = StringIO() 168 self.output = StringIO()
167 self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.output)) 169 self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.output))
168 170
171 def test_db_complex_password(self):
172 engine = mock.Mock()
173 # See comments in get_alembic_config; make an engine url with
174 # password characters that will be escaped, to ensure the
175 # resulting value makes it into alembic unaltered.
176 engine.url = sqlalchemy_make_url(
177 'mysql+pymysql://username:pw@%/!#$()@host:1234/dbname')
178 alembic_config = alembic_migrations.get_alembic_config(engine)
179 self.assertEqual(str(engine.url),
180 alembic_config.get_main_option('sqlalchemy.url'))
181
169 @mock.patch('glance.db.sqlalchemy.api.get_engine') 182 @mock.patch('glance.db.sqlalchemy.api.get_engine')
170 @mock.patch( 183 @mock.patch(
171 'glance.db.sqlalchemy.alembic_migrations.data_migrations.' 184 'glance.db.sqlalchemy.alembic_migrations.data_migrations.'