Fix for cross-tenant package and class isolation

The database constraints which were present were enforcing the global
uniqueness of package FQNs and the names of classes defined in them.
This behavior was not correct, as the uniqueness should be enforced per
tenant, so the same package may be uploaded into two isolated tenants
without affecting each other.

This behavior lead to a very serious security issue: any tenant could
upload a package, leave it private and thus block all other tenants of
the cloud from uploading the package with the same name or even other
packages which contain at least one class in common with it. This could
be used to intentionally block all the operations of Murano on any
public environments.

This fix modifies the package name constraint to be unique only in
combination with owner_id, i.e. makes packages unique per tenant. Also
it removes the class name uniquness check from database (as there is no
cross-DB way to check it in a proper way) and adds a check method in
db.api module instead.

As the packages may be made public, this introduces a potential
collision: if the user owns some package, and there is a public package
with the same fully-qualified-name (or defining same class(es)) then the
class loader of the engine will have to choise between these packages
and/or classes defined in them.
To resolve this collision this commit adds a logic to fetch all the
patching packages and then pick the best match. Packages owned by the
current tenant are the most preferred, then the engine will pick public
packages, and non-owned non-public packages are the least preferred
(there may be no such packages now, they may appear when we add other
ways of package sharing).

Closes-bug: #1440094
Change-Id: I5c9b49642dfb6e955cf0c98b42f418da3b82060a

murano/api/v1/catalog.py

@@ -149,7 +149,6 @@ class Controller(object):
                     policy.check('publicize_package', req.context)
                 break
         package = db_api.package_update(package_id, body, req.context)
-
         return package.to_dict()
 
     def get(self, req, package_id):

murano/common/engine.py

@@ -139,7 +139,8 @@ class TaskExecutor(object):
             murano_client_factory = lambda: \
                 self._environment.clients.get_murano_client(self._environment)
             with package_loader.ApiPackageLoader(
-                    murano_client_factory) as pkg_loader:
+                    murano_client_factory,
+                    self._environment.tenant_id) as pkg_loader:
                 return self._execute(pkg_loader)
         finally:
             if self._model['Objects'] is None:

murano/db/catalog/api.py

@@ -235,11 +235,22 @@ def package_update(pkg_id_or_name, changes, context):
     session = db_session.get_session()
     with session.begin():
         pkg = _package_get(pkg_id_or_name, session)
+        was_private = not pkg.is_public
         if not context.is_admin:
             _authorize_package(pkg, context)
 
         for change in changes:
             pkg = operation_methods[change['op']](pkg, change)
+        became_public = pkg.is_public
+        class_names = [clazz.name for clazz in pkg.class_definitions]
+        if was_private and became_public:
+            with db_session.get_lock("public_packages", session):
+                _check_for_existing_classes(session, class_names, None,
+                                            check_public=True,
+                                            ignore_package_with_id=pkg.id)
+                _check_for_public_packages_with_fqn(session,
+                                                    pkg.fully_qualified_name,
+                                                    pkg.id)
         session.add(pkg)
     return pkg
 
@@ -359,16 +370,37 @@ def package_upload(values, tenant_id):
     composite_attr_to_func = {'categories': _get_categories,
                               'tags': _get_tags,
                               'class_definitions': _get_class_definitions}
-    with session.begin():
+    is_public = values.get('is_public', False)
+
+    if is_public:
+        public_lock = db_session.get_lock("public_packages", session)
+    else:
+        public_lock = None
+    tenant_lock = db_session.get_lock("classes_of_" + tenant_id, session)
+    try:
+        _check_for_existing_classes(session, values.get('class_definitions'),
+                                    tenant_id, check_public=is_public)
+        if is_public:
+            _check_for_public_packages_with_fqn(
+                session,
+                values.get('fully_qualified_name'))
         for attr, func in composite_attr_to_func.iteritems():
             if values.get(attr):
                 result = func(values[attr], session)
                 setattr(package, attr, result)
                 del values[attr]
-
         package.update(values)
         package.owner_id = tenant_id
         package.save(session)
+        tenant_lock.commit()
+        if public_lock is not None:
+            public_lock.commit()
+    except Exception:
+        tenant_lock.rollback()
+        if public_lock is not None:
+            public_lock.rollback()
+        raise
+
     return package
 
 
@@ -442,3 +474,49 @@ def category_delete(category_id):
             LOG.error(msg)
             raise exc.HTTPNotFound(msg)
         session.delete(category)
+
+
+def _check_for_existing_classes(session, class_names, tenant_id,
+                                check_public=False,
+                                ignore_package_with_id=None):
+    if not class_names:
+        return
+    q = session.query(models.Class.name).filter(
+        models.Class.name.in_(class_names))
+    private_filter = None
+    public_filter = None
+    predicate = None
+    if tenant_id is not None:
+        private_filter = models.Class.package.has(
+            models.Package.owner_id == tenant_id)
+    if check_public:
+        public_filter = models.Class.package.has(
+            models.Package.is_public)
+    if private_filter is not None and public_filter is not None:
+        predicate = sa.or_(private_filter, public_filter)
+    elif private_filter is not None:
+        predicate = private_filter
+    elif public_filter is not None:
+        predicate = public_filter
+    if predicate is not None:
+        q = q.filter(predicate)
+    if ignore_package_with_id is not None:
+        q = q.filter(models.Class.package_id != ignore_package_with_id)
+    if q.first() is not None:
+        msg = _('Class with the same full name is already '
+                'registered in the visibility scope')
+        LOG.error(msg)
+        raise exc.HTTPConflict(msg)
+
+
+def _check_for_public_packages_with_fqn(session, fqn,
+                                        ignore_package_with_id=None):
+    q = session.query(models.Package.id).\
+        filter(models.Package.is_public).\
+        filter(models.Package.fully_qualified_name == fqn)
+    if ignore_package_with_id is not None:
+        q = q.filter(models.Package.id != ignore_package_with_id)
+    if q.first() is not None:
+        msg = _('Package with the same Name is already made public')
+        LOG.error(msg)
+        raise exc.HTTPConflict(msg)

murano/db/migration/alembic_migrations/versions/008_fix_unique_constraints.py

@@ -0,0 +1,199 @@
+# Copyright 2015 OpenStack Foundation.
+#
+# 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.
+
+
+from alembic import op
+import sqlalchemy as sa
+
+import murano.db.migration.helpers as helpers
+from murano.db.sqla import types as st
+
+"""fix_unique_constraints
+
+Revision ID: 008
+Revises: 007
+Create Date: 2015-04-08 14:01:06.458512
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '008'
+down_revision = '007'
+
+MYSQL_ENGINE = 'InnoDB'
+MYSQL_CHARSET = 'utf8'
+
+
+def upgrade():
+    engine = op.get_bind()
+    if engine.dialect.dialect_description == 'mysql+mysqldb':
+        engine.execute('SET FOREIGN_KEY_CHECKS=0')
+    if engine.dialect.dialect_description == 'postgresql+psycopg2':
+        op.drop_constraint('package_to_tag_package_id_fkey',
+                           'package_to_tag', 'foreignkey')
+        op.drop_constraint('package_to_tag_tag_id_fkey',
+                           'package_to_tag', 'foreignkey')
+        op.drop_constraint('package_to_category_package_id_fkey',
+                           'package_to_category', 'foreignkey')
+        op.drop_constraint('class_definition_package_id_fkey',
+                           'class_definition', 'foreignkey')
+    helpers.transform_table(
+        'package', {}, {},
+        sa.Column('created', sa.DateTime(), nullable=False),
+        sa.Column('updated', sa.DateTime(), nullable=False),
+        sa.Column('id', sa.String(length=36), nullable=False),
+        sa.Column('archive', st.LargeBinary(), nullable=True),
+        sa.Column('fully_qualified_name',
+                  sa.String(length=128),
+                  nullable=False),
+        sa.Column('type', sa.String(length=20), nullable=False),
+        sa.Column('author', sa.String(length=80), nullable=True),
+        sa.Column('name', sa.String(length=80), nullable=False),
+        sa.Column('enabled', sa.Boolean(), nullable=True),
+        sa.Column('description', sa.Text(), nullable=False),
+        sa.Column('is_public', sa.Boolean(), nullable=True),
+        sa.Column('logo', st.LargeBinary(), nullable=True),
+        sa.Column('owner_id', sa.String(length=36), nullable=False),
+        sa.Column('ui_definition', sa.Text(), nullable=True),
+        sa.Column('supplier_logo', sa.types.LargeBinary),
+        sa.Column('supplier', sa.types.Text()),
+        sa.PrimaryKeyConstraint('id'),
+        mysql_engine=MYSQL_ENGINE,
+        mysql_charset=MYSQL_CHARSET
+    )
+
+    helpers.transform_table(
+        'class_definition', {}, {},
+        sa.Column('created', sa.DateTime(), nullable=False),
+        sa.Column('updated', sa.DateTime(), nullable=False),
+        sa.Column('id', sa.String(length=36), nullable=False),
+        sa.Column('name',
+                  sa.String(length=128),
+                  nullable=False),
+        sa.Column('package_id', sa.String(length=36), nullable=True),
+        sa.ForeignKeyConstraint(['package_id'], ['package.id'], ),
+        sa.PrimaryKeyConstraint('id'),
+        mysql_engine=MYSQL_ENGINE,
+        mysql_charset=MYSQL_CHARSET
+    )
+
+    if engine.dialect.dialect_description == 'mysql+mysqldb':
+        engine.execute('SET FOREIGN_KEY_CHECKS=1')
+
+    if engine.dialect.dialect_description == 'postgresql+psycopg2':
+        op.create_foreign_key('package_to_tag_package_id_fkey',
+                              'package_to_tag', 'package',
+                              ['package_id'], ['id'])
+
+        op.create_foreign_key('package_to_tag_tag_id_fkey',
+                              'package_to_tag', 'tag',
+                              ['tag_id'], ['id'])
+
+        op.create_foreign_key('package_to_category_package_id_fkey',
+                              'package_to_category', 'package',
+                              ['package_id'], ['id'])
+
+        op.create_foreign_key('class_definition_package_id_fkey',
+                              'class_definition', 'package',
+                              ['package_id'], ['id'])
+
+    op.create_index('ix_package_fqn_and_owner', table_name='package',
+                    columns=['fully_qualified_name', 'owner_id'], unique=True)
+    op.create_index('ix_class_definition_name',
+                    'class_definition',
+                    ['name'])
+
+
+def downgrade():
+    op.drop_index('ix_package_fqn_and_owner', table_name='package')
+    op.drop_index('ix_class_definition_name', table_name='class_definition')
+
+    engine = op.get_bind()
+    if engine.dialect.dialect_description == 'mysql+mysqldb':
+        engine.execute('SET FOREIGN_KEY_CHECKS=0')
+    if engine.dialect.dialect_description == 'postgresql+psycopg2':
+        op.drop_constraint('package_to_tag_package_id_fkey',
+                           'package_to_tag', 'foreignkey')
+        op.drop_constraint('package_to_tag_tag_id_fkey',
+                           'package_to_tag', 'foreignkey')
+        op.drop_constraint('package_to_category_package_id_fkey',
+                           'package_to_category', 'foreignkey')
+        op.drop_constraint('class_definition_package_id_fkey',
+                           'class_definition', 'foreignkey')
+
+    helpers.transform_table(
+        'class_definition', {}, {},
+        sa.Column('created', sa.DateTime(), nullable=False),
+        sa.Column('updated', sa.DateTime(), nullable=False),
+        sa.Column('id', sa.String(length=36), nullable=False),
+        sa.Column('name',
+                  sa.String(length=128),
+                  nullable=False,
+                  unique=True),
+        sa.Column('package_id', sa.String(length=36), nullable=True),
+        sa.PrimaryKeyConstraint('id'),
+        mysql_engine=MYSQL_ENGINE,
+        mysql_charset=MYSQL_CHARSET
+    )
+
+    helpers.transform_table(
+        'package', {}, {},
+        sa.Column('created', sa.DateTime(), nullable=False),
+        sa.Column('updated', sa.DateTime(), nullable=False),
+        sa.Column('id', sa.String(length=36), nullable=False),
+        sa.Column('archive', st.LargeBinary(), nullable=True),
+        sa.Column('fully_qualified_name',
+                  sa.String(length=128),
+                  nullable=False,
+                  unique=True),
+        sa.Column('type', sa.String(length=20), nullable=False),
+        sa.Column('author', sa.String(length=80), nullable=True),
+        sa.Column('name', sa.String(length=80), nullable=False),
+        sa.Column('enabled', sa.Boolean(), nullable=True),
+        sa.Column('description', sa.Text(), nullable=False),
+        sa.Column('is_public', sa.Boolean(), nullable=True),
+        sa.Column('logo', st.LargeBinary(), nullable=True),
+        sa.Column('owner_id', sa.String(length=36), nullable=False),
+        sa.Column('ui_definition', sa.Text(), nullable=True),
+        sa.Column('supplier_logo', sa.types.LargeBinary),
+        sa.Column('supplier', sa.types.Text()),
+        sa.PrimaryKeyConstraint('id'),
+        mysql_engine=MYSQL_ENGINE,
+        mysql_charset=MYSQL_CHARSET
+    )
+
+    if engine.dialect.dialect_description == 'mysql+mysqldb':
+        engine.execute('SET FOREIGN_KEY_CHECKS=1')
+
+    if engine.dialect.dialect_description == 'postgresql+psycopg2':
+        op.create_foreign_key('package_to_tag_package_id_fkey',
+                              'package_to_tag', 'package',
+                              ['package_id'], ['id'])
+
+        op.create_foreign_key('package_to_tag_tag_id_fkey',
+                              'package_to_tag', 'tag',
+                              ['tag_id'], ['id'])
+
+        op.create_foreign_key('package_to_category_package_id_fkey',
+                              'package_to_category', 'package',
+                              ['package_id'], ['id'])
+
+        op.create_foreign_key('class_definition_package_id_fkey',
+                              'class_definition', 'package',
+                              ['package_id'], ['id'])
+
+    op.create_index('ix_class_definition_name',
+                    'class_definition',
+                    ['name'])

murano/db/models.py

@@ -220,15 +220,15 @@ class Instance(Base):
 class Package(Base, TimestampMixin):
     """Represents a meta information about application package."""
     __tablename__ = 'package'
-
+    __table_args__ = (sa.Index('ix_package_fqn_and_owner',
+                               'fully_qualified_name',
+                               'owner_id', unique=True),)
     id = sa.Column(sa.String(36),
                    primary_key=True,
                    default=uuidutils.generate_uuid)
     archive = sa.Column(st.LargeBinary())
     fully_qualified_name = sa.Column(sa.String(128),
-                                     nullable=False,
+                                     nullable=False)
-                                     index=True,
-                                     unique=True)
     type = sa.Column(sa.String(20), nullable=False, default='class')
     author = sa.Column(sa.String(80), default='Openstack')
     supplier = sa.Column(st.JsonBlob(), nullable=True, default={})
@@ -251,7 +251,8 @@ class Package(Base, TimestampMixin):
                                      cascade='save-update, merge',
                                      lazy='joined')
     class_definitions = sa_orm.relationship(
-        "Class", cascade='save-update, merge, delete', lazy='joined')
+        "Class", cascade='save-update, merge, delete', lazy='joined',
+        backref='package')
 
     def to_dict(self):
         d = self.__dict__.copy()

murano/engine/package_loader.py

@@ -45,12 +45,13 @@ class PackageLoader(six.with_metaclass(abc.ABCMeta)):
 
 
 class ApiPackageLoader(PackageLoader):
-    def __init__(self, murano_client_factory):
+    def __init__(self, murano_client_factory, tenant_id):
         self._cache_directory = self._get_cache_directory()
         self._murano_client_factory = murano_client_factory
+        self.tenant_id = tenant_id
 
     def get_package_by_class(self, name):
-        filter_opts = {'class_name': name, 'limit': 1}
+        filter_opts = {'class_name': name}
         try:
             package_definition = self._get_definition(filter_opts)
         except LookupError:
@@ -59,7 +60,7 @@ class ApiPackageLoader(PackageLoader):
         return self._get_package_by_definition(package_definition)
 
     def get_package(self, name):
-        filter_opts = {'fqn': name, 'limit': 1}
+        filter_opts = {'fqn': name}
         try:
             package_definition = self._get_definition(filter_opts)
         except LookupError:
@@ -82,15 +83,19 @@ class ApiPackageLoader(PackageLoader):
 
     def _get_definition(self, filter_opts):
         try:
-            packages = self._murano_client_factory().packages.filter(
+            packages = list(self._murano_client_factory().packages.filter(
-                **filter_opts)
+                **filter_opts))
-            try:
+            if len(packages) > 1:
-                return packages.next()
+                LOG.debug('Ambiguous package resolution: '
-            except StopIteration:
+                          'more then 1 package found for query "{0}", '
+                          'will resolve based on the ownership'.
+                          format(filter_opts))
+                return get_best_package_match(packages, self.tenant_id)
+            elif len(packages) == 1:
+                return packages[0]
+            else:
                 LOG.debug('There are no packages matching filter '
                           '{0}'.format(filter_opts))
-                # TODO(smelikyan): This exception should be replaced with one
-                # defined in python-muranoclient
                 raise LookupError()
         except muranoclient_exc.HTTPException:
             LOG.debug('Failed to get package definition from repository')
@@ -187,3 +192,19 @@ class DirectoryPackageLoader(PackageLoader):
             self._packages_by_name[package.full_name] = package
 
             self._processed_entries.add(entry)
+
+
+def get_best_package_match(packages, tenant_id):
+    public = None
+    other = []
+    for package in packages:
+        if package.owner_id == tenant_id:
+            return package
+        elif package.is_public:
+            public = package
+        else:
+            other.append(package)
+    if public is not None:
+        return public
+    elif other:
+        return other[0]

murano/tests/unit/db/test_catalog.py

@@ -26,14 +26,16 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase):
     def setUp(self):
         super(CatalogDBTestCase, self).setUp()
         self.tenant_id = str(uuid.uuid4())
+        self.tenant_id_2 = str(uuid.uuid4())
         self.context = utils.dummy_context(tenant_id=self.tenant_id)
+        self.context_2 = utils.dummy_context(tenant_id=self.tenant_id_2)
 
     def _create_categories(self):
         api.category_add('cat1')
         api.category_add('cat2')
 
-    def _stub_package(self):
+    def _stub_package(self, **kwargs):
-        return {
+        base = {
             'archive': "archive blob here",
             'fully_qualified_name': 'com.example.package',
             'type': 'class',
@@ -46,6 +48,15 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase):
             'logo': "logo blob here",
             'ui_definition': '{}',
         }
+        base.update(**kwargs)
+        return base
+
+    def get_change(self, op, path, value):
+        return {
+            'op': op,
+            'path': path,
+            'value': value
+        }
 
     def test_list_empty_categories(self):
         res = api.category_get_names()
@@ -87,3 +98,97 @@ class CatalogDBTestCase(base.MuranoWithDBTestCase):
 
         self.assertRaises(exc.HTTPNotFound,
                           api.package_get, package.id, self.context)
+
+    def test_package_upload_to_different_tenants_with_same_fqn(self):
+        values = self._stub_package()
+
+        api.package_upload(values, self.tenant_id)
+        api.package_upload(values, self.tenant_id_2)
+
+    def test_package_upload_public_public_fqn_violation(self):
+        values = self._stub_package(is_public=True)
+        api.package_upload(values, self.tenant_id)
+        values = self._stub_package(is_public=True)
+        self.assertRaises(exc.HTTPConflict, api.package_upload,
+                          values, self.tenant_id_2)
+
+    def test_package_upload_public_private_no_fqn_violation(self):
+        values = self._stub_package(is_public=True)
+        api.package_upload(values, self.tenant_id)
+        values = self._stub_package(is_public=False)
+        api.package_upload(values, self.tenant_id_2)
+
+    def test_package_upload_private_public_no_fqn_violation(self):
+        values = self._stub_package()
+        api.package_upload(values, self.tenant_id)
+        values = self._stub_package(is_public=True)
+        api.package_upload(values, self.tenant_id_2)
+
+    def test_class_name_is_unique(self):
+        value = self._stub_package(class_definitions=('foo', 'bar'))
+        api.package_upload(value, self.tenant_id)
+        value = self._stub_package(class_definitions=('bar', 'baz'),
+                                   fully_qualified_name='com.example.package2')
+        self.assertRaises(exc.HTTPConflict, api.package_upload, value,
+                          self.tenant_id)
+
+    def test_class_name_uniqueness_not_enforced_in_different_tenants(self):
+        value = self._stub_package(class_definitions=('foo', 'bar'))
+        api.package_upload(value, self.tenant_id)
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   fully_qualified_name='com.example.package2')
+        api.package_upload(value, self.tenant_id_2)
+
+    def test_class_name_public_public_violation(self):
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   is_public=True)
+        api.package_upload(value, self.tenant_id)
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   is_public=True,
+                                   fully_qualified_name='com.example.package2')
+        self.assertRaises(exc.HTTPConflict, api.package_upload,
+                          value, self.tenant_id_2)
+
+    def test_class_name_public_private_no_violation(self):
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   is_public=True)
+        api.package_upload(value, self.tenant_id)
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   is_public=False,
+                                   fully_qualified_name='com.example.package2')
+        api.package_upload(value, self.tenant_id_2)
+
+    def test_class_name_private_public_no_violation(self):
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   is_public=False)
+        api.package_upload(value, self.tenant_id)
+        value = self._stub_package(class_definitions=('foo', 'bar'),
+                                   is_public=True,
+                                   fully_qualified_name='com.example.package2')
+        api.package_upload(value, self.tenant_id_2)
+
+    def test_package_make_public(self):
+        id = api.package_upload(self._stub_package(), self.tenant_id).id
+        patch = self.get_change('replace', ['is_public'], True)
+        api.package_update(id, [patch], self.context)
+        package = api.package_get(id, self.context)
+        self.assertEqual(True, package.is_public)
+
+    def test_package_update_public_public_fqn_violation(self):
+        id1 = api.package_upload(self._stub_package(), self.tenant_id).id
+        id2 = api.package_upload(self._stub_package(), self.tenant_id_2).id
+        patch = self.get_change('replace', ['is_public'], True)
+        api.package_update(id1, [patch], self.context)
+        self.assertRaises(exc.HTTPConflict, api.package_update,
+                          id2, [patch], self.context_2)
+
+    def test_package_update_public_public_class_name_violation(self):
+        id1 = api.package_upload(self._stub_package(
+            class_definitions=('foo', 'bar')), self.tenant_id).id
+        id2 = api.package_upload(self._stub_package(
+            class_definitions=('foo', 'bar'),
+            fully_qualified_name='com.example.package2'), self.tenant_id_2).id
+        patch = self.get_change('replace', ['is_public'], True)
+        api.package_update(id1, [patch], self.context)
+        self.assertRaises(exc.HTTPConflict, api.package_update,
+                          id2, [patch], self.context_2)