Merge "Add credential quoting to Swift's StoreLocation."

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)

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(':')
-
+            if len(cred_parts) != 2:
-            # 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, key = cred_parts
-                user = ':'.join(cred_parts[0:2])
+            self.user = urllib.unquote(user)
-            else:
+            self.key = urllib.unquote(key)
-                user = cred_parts[0]
-            key = cred_parts[-1]
-            self.user = user
-            self.key = key
         else:
             self.user = None
         path_parts = path.split('/')

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)

glance/tests/unit/test_store_location.py

@@ -46,8 +46,8 @@ class TestStoreLocation(base.StoreClearingUnitTest):
         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://account%3Auser:pass@authurl.com/container/obj-id',
-            'swift+https://account:user: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',
@@ -178,7 +178,8 @@ class TestStoreLocation(base.StoreClearingUnitTest):
         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)
@@ -186,8 +187,8 @@ class TestStoreLocation(base.StoreClearingUnitTest):
         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("a:user@example.com", loc.user)
-        self.assertEqual("pass", loc.key)
+        self.assertEqual("p@ss", loc.key)
         self.assertEqual(uri, loc.get_uri())
 
         bad_uri = 'swif://'

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
@@ -190,12 +191,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)
@@ -203,7 +208,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)
@@ -223,7 +228,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)
 
@@ -240,7 +245,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)
@@ -252,7 +257,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)
 
@@ -305,7 +310,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 = \
@@ -372,7 +377,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)
 
@@ -410,7 +415,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)
 
@@ -464,7 +469,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)
 
@@ -549,7 +554,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)
 
@@ -561,7 +566,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)