From 0252c182cf535d4f9db1dc1c0df144430c39b1a1 Mon Sep 17 00:00:00 2001 From: Maru Newby Date: Thu, 3 May 2012 16:09:52 -0700 Subject: [PATCH] Add credential quoting to Swift's StoreLocation. * Added credential quoting in glance.store.swift.StoreLocation to support usernames and passwords that contain '@' characters. Without quoting, '@' characters in credentials (e.g. using an email address as a username) would result in an exception being thrown by parse_uri. * Added a migration to support the change in location format. * Addresses bug 994296 Change-Id: I92c4f4af914394aada1b4415a42f5a1308187dc4 --- .../versions/015_quote_swift_credentials.py | 163 ++++++++++++++++++ glance/store/swift.py | 19 +- glance/tests/unit/test_migrations.py | 60 +++++++ glance/tests/unit/test_store_location.py | 11 +- glance/tests/unit/test_swift_store.py | 27 +-- 5 files changed, 251 insertions(+), 29 deletions(-) create mode 100644 glance/registry/db/migrate_repo/versions/015_quote_swift_credentials.py diff --git a/glance/registry/db/migrate_repo/versions/015_quote_swift_credentials.py b/glance/registry/db/migrate_repo/versions/015_quote_swift_credentials.py new file mode 100644 index 0000000000..653d7c7a68 --- /dev/null +++ b/glance/registry/db/migrate_repo/versions/015_quote_swift_credentials.py @@ -0,0 +1,163 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2012 OpenStack LLC. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import types +import urlparse + +import sqlalchemy + +from glance.common import exception +import glance.store.swift + + +def upgrade(migrate_engine): + migrate_location_credentials(migrate_engine, to_quoted=True) + + +def downgrade(migrate_engine): + migrate_location_credentials(migrate_engine, to_quoted=False) + + +def migrate_location_credentials(migrate_engine, to_quoted): + """ + Migrate location credentials for swift uri's between the quoted + and unquoted forms. + + :param migrate_engine: The configured db engine + :param to_quoted: If True, migrate location credentials from + unquoted to quoted form. If False, do the + reverse. + """ + meta = sqlalchemy.schema.MetaData() + meta.bind = migrate_engine + + images_table = sqlalchemy.Table('images', meta, autoload=True) + + images = images_table.select(images_table.c.location.startswith('swift')).\ + execute() + + for image in images: + fixed_uri = fix_uri_credentials(image['location'], to_quoted) + images_table.update().\ + where(images_table.c.id == image['id']).\ + values(location=fixed_uri).execute() + + +def fix_uri_credentials(uri, to_quoted): + """ + Fix the given uri's embedded credentials by round-tripping with + StoreLocation. + + If to_quoted is True, the uri is assumed to have credentials that + have not been quoted, and the resulting uri will contain quoted + credentials. + + If to_quoted is False, the uri is assumed to have credentials that + have been quoted, and the resulting uri will contain credentials + that have not been quoted. + """ + location = glance.store.swift.StoreLocation({}) + if to_quoted: + # The legacy parse_uri doesn't unquote credentials + location.parse_uri = types.MethodType(legacy_parse_uri, location) + else: + # The legacy _get_credstring doesn't quote credentials + location._get_credstring = types.MethodType(legacy__get_credstring, + location) + location.parse_uri(uri) + return location.get_uri() + + +def legacy__get_credstring(self): + if self.user: + return '%s:%s@' % (self.user, self.key) + return '' + + +def legacy_parse_uri(self, uri): + """ + Parse URLs. This method fixes an issue where credentials specified + in the URL are interpreted differently in Python 2.6.1+ than prior + versions of Python. It also deals with the peculiarity that new-style + Swift URIs have where a username can contain a ':', like so: + + swift://account:user:pass@authurl.com/container/obj + """ + # Make sure that URIs that contain multiple schemes, such as: + # swift://user:pass@http://authurl.com/v1/container/obj + # are immediately rejected. + if uri.count('://') != 1: + reason = _( + "URI cannot contain more than one occurrence of a scheme." + "If you have specified a URI like " + "swift://user:pass@http://authurl.com/v1/container/obj" + ", you need to change it to use the swift+http:// scheme, " + "like so: " + "swift+http://user:pass@authurl.com/v1/container/obj" + ) + raise exception.BadStoreUri(uri, reason) + + pieces = urlparse.urlparse(uri) + assert pieces.scheme in ('swift', 'swift+http', 'swift+https') + self.scheme = pieces.scheme + netloc = pieces.netloc + path = pieces.path.lstrip('/') + if netloc != '': + # > Python 2.6.1 + if '@' in netloc: + creds, netloc = netloc.split('@') + else: + creds = None + else: + # Python 2.6.1 compat + # see lp659445 and Python issue7904 + if '@' in path: + creds, path = path.split('@') + else: + creds = None + netloc = path[0:path.find('/')].strip('/') + path = path[path.find('/'):].strip('/') + if creds: + cred_parts = creds.split(':') + + # User can be account:user, in which case cred_parts[0:2] will be + # the account and user. Combine them into a single username of + # account:user + if len(cred_parts) == 1: + reason = (_("Badly formed credentials '%(creds)s' in Swift " + "URI") % locals()) + raise exception.BadStoreUri(uri, reason) + elif len(cred_parts) == 3: + user = ':'.join(cred_parts[0:2]) + else: + user = cred_parts[0] + key = cred_parts[-1] + self.user = user + self.key = key + else: + self.user = None + path_parts = path.split('/') + try: + self.obj = path_parts.pop() + self.container = path_parts.pop() + if not netloc.startswith('http'): + # push hostname back into the remaining to build full authurl + path_parts.insert(0, netloc) + self.authurl = '/'.join(path_parts) + except IndexError: + reason = _("Badly formed Swift URI") + raise exception.BadStoreUri(uri, reason) diff --git a/glance/store/swift.py b/glance/store/swift.py index c4b10e392b..0fe1a4488c 100644 --- a/glance/store/swift.py +++ b/glance/store/swift.py @@ -23,6 +23,7 @@ import hashlib import httplib import logging import math +import urllib import urlparse from glance.common import exception @@ -70,7 +71,7 @@ class StoreLocation(glance.store.location.StoreLocation): def _get_credstring(self): if self.user: - return '%s:%s@' % (self.user, self.key) + return '%s:%s@' % (urllib.quote(self.user), urllib.quote(self.key)) return '' def get_uri(self): @@ -133,21 +134,13 @@ class StoreLocation(glance.store.location.StoreLocation): path = path[path.find('/'):].strip('/') if creds: cred_parts = creds.split(':') - - # User can be account:user, in which case cred_parts[0:2] will be - # the account and user. Combine them into a single username of - # account:user - if len(cred_parts) == 1: + if len(cred_parts) != 2: reason = (_("Badly formed credentials '%(creds)s' in Swift " "URI") % locals()) raise exception.BadStoreUri(uri, reason) - elif len(cred_parts) == 3: - user = ':'.join(cred_parts[0:2]) - else: - user = cred_parts[0] - key = cred_parts[-1] - self.user = user - self.key = key + user, key = cred_parts + self.user = urllib.unquote(user) + self.key = urllib.unquote(key) else: self.user = None path_parts = path.split('/') diff --git a/glance/tests/unit/test_migrations.py b/glance/tests/unit/test_migrations.py index 0c749cf8f3..ab93ece6e3 100644 --- a/glance/tests/unit/test_migrations.py +++ b/glance/tests/unit/test_migrations.py @@ -316,3 +316,63 @@ class TestMigrations(unittest.TestCase): last_num_image_properties = conn.execute(sel).scalar() self.assertEqual(num_image_properties - 2, last_num_image_properties) + + def test_no_data_loss_14_to_15(self): + for key, engine in self.engines.items(): + conf = utils.TestConfigOpts({ + 'sql_connection': TestMigrations.TEST_DATABASES[key]}) + conf.register_opt(cfg.StrOpt('sql_connection')) + self._check_no_data_loss_14_to_15(engine, conf) + + def _check_no_data_loss_14_to_15(self, engine, conf): + """ + Check that migrating swift location credentials to quoted form + and back does not result in data loss. + """ + migration_api.version_control(conf, version=0) + migration_api.upgrade(conf, 14) + + conn = engine.connect() + images_table = Table('images', MetaData(), autoload=True, + autoload_with=engine) + + def get_locations(): + conn = engine.connect() + locations = [x[0] for x in + conn.execute( + select(['location'], from_obj=[images_table]))] + conn.close() + return locations + + unquoted_locations = [ + 'swift://acct:usr:pass@example.com/container/obj-id', + 'file://foo', + ] + quoted_locations = [ + 'swift://acct%3Ausr:pass@example.com/container/obj-id', + 'file://foo', + ] + + # Insert images with an unquoted image location + now = datetime.datetime.now() + kwargs = dict( + deleted=False, + created_at=now, + updated_at=now, + status='active', + is_public=True, + min_disk=0, + min_ram=0, + ) + for i, location in enumerate(unquoted_locations): + kwargs.update(location=location, id=i) + conn.execute(images_table.insert(), [kwargs]) + conn.close() + + migration_api.upgrade(conf, 15) + + self.assertEqual(get_locations(), quoted_locations) + + migration_api.downgrade(conf, 14) + + self.assertEqual(get_locations(), unquoted_locations) diff --git a/glance/tests/unit/test_store_location.py b/glance/tests/unit/test_store_location.py index 312b93601e..f0f81c0f18 100644 --- a/glance/tests/unit/test_store_location.py +++ b/glance/tests/unit/test_store_location.py @@ -40,8 +40,8 @@ class TestStoreLocation(unittest.TestCase): good_store_uris = [ 'https://user:pass@example.com:80/images/some-id', 'http://images.oracle.com/123456', - 'swift://account:user:pass@authurl.com/container/obj-id', - 'swift+https://account:user:pass@authurl.com/container/obj-id', + 'swift://account%3Auser:pass@authurl.com/container/obj-id', + 'swift+https://account%3Auser:pass@authurl.com/container/obj-id', 's3://accesskey:secretkey@s3.amazonaws.com/bucket/key-id', 's3://accesskey:secretwith/aslash@s3.amazonaws.com/bucket/key-id', 's3+http://accesskey:secret@s3.amazonaws.com/bucket/key-id', @@ -172,7 +172,8 @@ class TestStoreLocation(unittest.TestCase): self.assertEqual("pass", loc.key) self.assertEqual(uri, loc.get_uri()) - uri = 'swift+http://account:user:pass@authurl.com/v1/container/12345' + uri = ('swift+http://a%3Auser%40example.com:p%40ss@authurl.com/' + 'v1/container/12345') loc.parse_uri(uri) self.assertEqual("swift+http", loc.scheme) @@ -180,8 +181,8 @@ class TestStoreLocation(unittest.TestCase): self.assertEqual("http://authurl.com/v1", loc.swift_auth_url) self.assertEqual("container", loc.container) self.assertEqual("12345", loc.obj) - self.assertEqual("account:user", loc.user) - self.assertEqual("pass", loc.key) + self.assertEqual("a:user@example.com", loc.user) + self.assertEqual("p@ss", loc.key) self.assertEqual(uri, loc.get_uri()) bad_uri = 'swif://' diff --git a/glance/tests/unit/test_swift_store.py b/glance/tests/unit/test_swift_store.py index 743bd37849..976d132850 100644 --- a/glance/tests/unit/test_swift_store.py +++ b/glance/tests/unit/test_swift_store.py @@ -22,6 +22,7 @@ import hashlib import httplib import tempfile import unittest +import urllib import stubout import swift.common.client @@ -187,12 +188,16 @@ def stub_out_swift_common_client(stubs, conf): class SwiftTests(object): + @property + def swift_store_user(self): + return urllib.quote(self.conf['swift_store_user']) + def test_get_size(self): """ Test that we can get the size of an object in the swift store """ uri = "swift://%s:key@auth_address/glance/%s" % ( - self.conf['swift_store_user'], FAKE_UUID) + self.swift_store_user, FAKE_UUID) loc = get_location_from_uri(uri) image_size = self.store.get_size(loc) self.assertEqual(image_size, 5120) @@ -200,7 +205,7 @@ class SwiftTests(object): def test_get(self): """Test a "normal" retrieval of an image in chunks""" uri = "swift://%s:key@auth_address/glance/%s" % ( - self.conf['swift_store_user'], FAKE_UUID) + self.swift_store_user, FAKE_UUID) loc = get_location_from_uri(uri) (image_swift, image_size) = self.store.get(loc) self.assertEqual(image_size, 5120) @@ -220,7 +225,7 @@ class SwiftTests(object): """ loc = get_location_from_uri("swift+http://%s:key@auth_address/" "glance/%s" % ( - self.conf['swift_store_user'], FAKE_UUID)) + self.swift_store_user, FAKE_UUID)) (image_swift, image_size) = self.store.get(loc) self.assertEqual(image_size, 5120) @@ -237,7 +242,7 @@ class SwiftTests(object): raises an error """ loc = get_location_from_uri("swift://%s:key@authurl/glance/noexist" % ( - self.conf['swift_store_user'])) + self.swift_store_user)) self.assertRaises(exception.NotFound, self.store.get, loc) @@ -249,7 +254,7 @@ class SwiftTests(object): expected_checksum = hashlib.md5(expected_swift_contents).hexdigest() expected_image_id = utils.generate_uuid() loc = 'swift+https://%s:key@localhost:8080/glance/%s' - expected_location = loc % (self.conf['swift_store_user'], + expected_location = loc % (self.swift_store_user, expected_image_id) image_swift = StringIO.StringIO(expected_swift_contents) @@ -302,7 +307,7 @@ class SwiftTests(object): for variation, expected_location in variations.items(): image_id = utils.generate_uuid() expected_location = expected_location % ( - self.conf['swift_store_user'], image_id) + self.swift_store_user, image_id) expected_swift_size = FIVE_KB expected_swift_contents = "*" * expected_swift_size expected_checksum = \ @@ -369,7 +374,7 @@ class SwiftTests(object): expected_checksum = hashlib.md5(expected_swift_contents).hexdigest() expected_image_id = utils.generate_uuid() loc = 'swift+https://%s:key@localhost:8080/noexist/%s' - expected_location = loc % (self.conf['swift_store_user'], + expected_location = loc % (self.swift_store_user, expected_image_id) image_swift = StringIO.StringIO(expected_swift_contents) @@ -407,7 +412,7 @@ class SwiftTests(object): expected_checksum = hashlib.md5(expected_swift_contents).hexdigest() expected_image_id = utils.generate_uuid() loc = 'swift+https://%s:key@localhost:8080/glance/%s' - expected_location = loc % (self.conf['swift_store_user'], + expected_location = loc % (self.swift_store_user, expected_image_id) image_swift = StringIO.StringIO(expected_swift_contents) @@ -461,7 +466,7 @@ class SwiftTests(object): expected_checksum = hashlib.md5(expected_swift_contents).hexdigest() expected_image_id = utils.generate_uuid() loc = 'swift+https://%s:key@localhost:8080/glance/%s' - expected_location = loc % (self.conf['swift_store_user'], + expected_location = loc % (self.swift_store_user, expected_image_id) image_swift = StringIO.StringIO(expected_swift_contents) @@ -546,7 +551,7 @@ class SwiftTests(object): Test we can delete an existing image in the swift store """ uri = "swift://%s:key@authurl/glance/%s" % ( - self.conf['swift_store_user'], FAKE_UUID) + self.swift_store_user, FAKE_UUID) loc = get_location_from_uri(uri) self.store.delete(loc) @@ -558,7 +563,7 @@ class SwiftTests(object): raises an error """ loc = get_location_from_uri("swift://%s:key@authurl/glance/noexist" % ( - self.conf['swift_store_user'])) + self.swift_store_user)) self.assertRaises(exception.NotFound, self.store.delete, loc)