diff --git a/manila/db/api.py b/manila/db/api.py index 3d2af23a29..17ac1f220e 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -449,6 +449,13 @@ def share_access_get_all_by_type_and_access(context, share_id, access_type, context, share_id, access_type, access) +def share_access_check_for_existing_access(context, share_id, access_type, + access_to): + """Returns True if rule corresponding to the type and client exists.""" + return IMPL.share_access_check_for_existing_access( + context, share_id, access_type, access_to) + + def share_instance_access_create(context, values, share_instance_id): """Allow access to share instance.""" return IMPL.share_instance_access_create( @@ -595,6 +602,15 @@ def share_snapshot_access_get_all_for_share_snapshot(context, context, share_snapshot_id, filters) +def share_snapshot_check_for_existing_access(context, share_snapshot_id, + access_type, access_to): + """Returns True if rule corresponding to the type and client exists.""" + return IMPL.share_snapshot_check_for_existing_access(context, + share_snapshot_id, + access_type, + access_to) + + def share_snapshot_export_locations_get(context, snapshot_id): """Get all export locations for a given share snapshot.""" return IMPL.share_snapshot_export_locations_get(context, snapshot_id) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 4e6d7e98f1..0153a6dda0 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -21,6 +21,7 @@ import copy import datetime from functools import wraps +import ipaddress import sys import warnings @@ -2127,6 +2128,44 @@ def share_access_get_all_by_type_and_access(context, share_id, access_type, 'access_to': access}).all() +@require_context +def share_access_check_for_existing_access(context, share_id, access_type, + access_to): + return _check_for_existing_access( + context, 'share', share_id, access_type, access_to) + + +def _check_for_existing_access(context, resource, resource_id, access_type, + access_to): + + session = get_session() + if resource == 'share': + query_method = _share_access_get_query + access_to_field = models.ShareAccessMapping.access_to + else: + query_method = _share_snapshot_access_get_query + access_to_field = models.ShareSnapshotAccessMapping.access_to + + with session.begin(): + if access_type == 'ip': + rules = query_method( + context, session, {'%s_id' % resource: resource_id, + 'access_type': access_type}).filter( + access_to_field.startswith(access_to.split('/')[0])).all() + + matching_rules = [ + rule for rule in rules if + ipaddress.ip_network(six.text_type(access_to)) == + ipaddress.ip_network(six.text_type(rule['access_to'])) + ] + return len(matching_rules) > 0 + else: + return query_method( + context, session, {'%s_id' % resource: resource_id, + 'access_type': access_type, + 'access_to': access_to}).count() > 0 + + @require_context def share_access_delete_all_by_share(context, share_id): session = get_session() @@ -2608,6 +2647,13 @@ def share_snapshot_access_get_all_for_share_snapshot(context, return access_list +@require_context +def share_snapshot_check_for_existing_access(context, share_snapshot_id, + access_type, access_to): + return _check_for_existing_access( + context, 'share_snapshot', share_snapshot_id, access_type, access_to) + + @require_context def share_snapshot_access_get_all_for_snapshot_instance( context, snapshot_instance_id, filters=None, diff --git a/manila/share/api.py b/manila/share/api.py index d0ce4948ce..680d0dd97c 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1605,10 +1605,11 @@ class API(base.Base): if access_level not in constants.ACCESS_LEVELS + (None, ): msg = _("Invalid share access level: %s.") % access_level raise exception.InvalidShareAccess(reason=msg) - share_access_list = self.db.share_access_get_all_by_type_and_access( + + access_exists = self.db.share_access_check_for_existing_access( ctx, share['id'], access_type, access_to) - if len(share_access_list) > 0: + if access_exists: raise exception.ShareAccessExists(access_type=access_type, access=access_to) @@ -1846,14 +1847,10 @@ class API(base.Base): def snapshot_allow_access(self, context, snapshot, access_type, access_to): """Allow access to a share snapshot.""" + access_exists = self.db.share_snapshot_check_for_existing_access( + context, snapshot['id'], access_type, access_to) - filters = {'access_to': access_to, - 'access_type': access_type} - - access_list = self.db.share_snapshot_access_get_all_for_share_snapshot( - context, snapshot['id'], filters) - - if len(access_list) > 0: + if access_exists: raise exception.ShareSnapshotAccessExists(access_type=access_type, access=access_to) diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 8e123d719b..8cb01a7419 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -225,6 +225,44 @@ class ShareAccessDatabaseAPITestCase(test.TestCase): 'access_key'): self.assertEqual(with_share_access_data, key in instance_access) + @ddt.data({'existing': {'access_type': 'cephx', 'access_to': 'alice'}, + 'new': {'access_type': 'user', 'access_to': 'alice'}, + 'result': False}, + {'existing': {'access_type': 'user', 'access_to': 'bob'}, + 'new': {'access_type': 'user', 'access_to': 'bob'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': '10.0.0.10/32'}, + 'new': {'access_type': 'ip', 'access_to': '10.0.0.10'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': '10.10.0.11'}, + 'new': {'access_type': 'ip', 'access_to': '10.10.0.11'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': 'fd21::11'}, + 'new': {'access_type': 'ip', 'access_to': 'fd21::11'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': 'fd21::10'}, + 'new': {'access_type': 'ip', 'access_to': 'fd21::10/128'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': '10.10.0.0/22'}, + 'new': {'access_type': 'ip', 'access_to': '10.10.0.0/24'}, + 'result': False}, + {'existing': {'access_type': 'ip', 'access_to': '2620:52::/48'}, + 'new': {'access_type': 'ip', + 'access_to': '2620:52:0:13b8::/64'}, + 'result': False}) + @ddt.unpack + def test_share_access_check_for_existing_access(self, existing, new, + result): + share = db_utils.create_share() + db_utils.create_access(share_id=share['id'], + access_type=existing['access_type'], + access_to=existing['access_to']) + + rule_exists = db_api.share_access_check_for_existing_access( + self.ctxt, share['id'], new['access_type'], new['access_to']) + + self.assertEqual(result, rule_exists) + @ddt.ddt class ShareDatabaseAPITestCase(test.TestCase): @@ -1347,6 +1385,45 @@ class ShareSnapshotDatabaseAPITestCase(test.TestCase): self.assertSubDictMatch(values, actual_value[0].to_dict()) + @ddt.data({'existing': {'access_type': 'cephx', 'access_to': 'alice'}, + 'new': {'access_type': 'user', 'access_to': 'alice'}, + 'result': False}, + {'existing': {'access_type': 'user', 'access_to': 'bob'}, + 'new': {'access_type': 'user', 'access_to': 'bob'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': '10.0.0.10/32'}, + 'new': {'access_type': 'ip', 'access_to': '10.0.0.10'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': '10.10.0.11'}, + 'new': {'access_type': 'ip', 'access_to': '10.10.0.11'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': 'fd21::11'}, + 'new': {'access_type': 'ip', 'access_to': 'fd21::11'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': 'fd21::10'}, + 'new': {'access_type': 'ip', 'access_to': 'fd21::10/128'}, + 'result': True}, + {'existing': {'access_type': 'ip', 'access_to': '10.10.0.0/22'}, + 'new': {'access_type': 'ip', 'access_to': '10.10.0.0/24'}, + 'result': False}, + {'existing': {'access_type': 'ip', 'access_to': '2620:52::/48'}, + 'new': {'access_type': 'ip', + 'access_to': '2620:52:0:13b8::/64'}, + 'result': False}) + @ddt.unpack + def test_share_snapshot_check_for_existing_access(self, existing, new, + result): + db_utils.create_snapshot_access( + share_snapshot_id=self.snapshot_1['id'], + access_type=existing['access_type'], + access_to=existing['access_to']) + + rule_exists = db_api.share_snapshot_check_for_existing_access( + self.ctxt, self.snapshot_1['id'], new['access_type'], + new['access_to']) + + self.assertEqual(result, rule_exists) + def test_share_snapshot_access_get_all_for_snapshot_instance(self): access = db_utils.create_snapshot_access( share_snapshot_id=self.snapshot_1['id']) diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 3f36506737..3cea0c9dc0 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -2321,15 +2321,13 @@ class ShareAPITestCase(test.TestCase): status=constants.STATUS_AVAILABLE) access = db_utils.create_snapshot_access( share_snapshot_id=snapshot['id']) - filters = {'access_to': access_to, - 'access_type': access_type} values = {'share_snapshot_id': snapshot['id'], 'access_type': access_type, 'access_to': access_to} - access_get_all = self.mock_object( - db_api, 'share_snapshot_access_get_all_for_share_snapshot', - mock.Mock(return_value=[])) + existing_access_check = self.mock_object( + db_api, 'share_snapshot_check_for_existing_access', + mock.Mock(return_value=False)) access_create = self.mock_object( db_api, 'share_snapshot_access_create', mock.Mock(return_value=access)) @@ -2339,8 +2337,9 @@ class ShareAPITestCase(test.TestCase): access_type, access_to) self.assertEqual(access, out) - access_get_all.assert_called_once_with( - utils.IsAMatcher(context.RequestContext), snapshot['id'], filters) + existing_access_check.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), snapshot['id'], + access_type, access_to) access_create.assert_called_once_with( utils.IsAMatcher(context.RequestContext), values) @@ -2349,40 +2348,38 @@ class ShareAPITestCase(test.TestCase): access_type = 'ip' share = db_utils.create_share() snapshot = db_utils.create_snapshot(share_id=share['id']) - filters = {'access_to': access_to, - 'access_type': access_type} - - access_get_all = self.mock_object( - db_api, 'share_snapshot_access_get_all_for_share_snapshot', - mock.Mock(return_value=[])) + existing_access_check = self.mock_object( + db_api, 'share_snapshot_check_for_existing_access', + mock.Mock(return_value=False)) self.assertRaises(exception.InvalidShareSnapshotInstance, self.api.snapshot_allow_access, self.context, snapshot, access_type, access_to) - access_get_all.assert_called_once_with( - utils.IsAMatcher(context.RequestContext), snapshot['id'], filters) + existing_access_check.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), snapshot['id'], + access_type, access_to) def test_snapshot_allow_access_access_exists_exception(self): access_to = '1.1.1.1' access_type = 'ip' share = db_utils.create_share() snapshot = db_utils.create_snapshot(share_id=share['id']) - access = db_utils.create_snapshot_access( - share_snapshot_id=snapshot['id']) - filters = {'access_to': access_to, - 'access_type': access_type} + db_utils.create_snapshot_access( + share_snapshot_id=snapshot['id'], access_to=access_to, + access_type=access_type) - access_get_all = self.mock_object( - db_api, 'share_snapshot_access_get_all_for_share_snapshot', - mock.Mock(return_value=[access])) + existing_access_check = self.mock_object( + db_api, 'share_snapshot_check_for_existing_access', + mock.Mock(return_value=True)) self.assertRaises(exception.ShareSnapshotAccessExists, self.api.snapshot_allow_access, self.context, snapshot, access_type, access_to) - access_get_all.assert_called_once_with( - utils.IsAMatcher(context.RequestContext), snapshot['id'], filters) + existing_access_check.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), snapshot['id'], + access_type, access_to) def test_snapshot_deny_access(self): share = db_utils.create_share() diff --git a/releasenotes/notes/bug-1767430-access-control-raise-ip-address-conflict-on-host-routes-0c298125fee4a640.yaml b/releasenotes/notes/bug-1767430-access-control-raise-ip-address-conflict-on-host-routes-0c298125fee4a640.yaml new file mode 100644 index 0000000000..ea172d5c12 --- /dev/null +++ b/releasenotes/notes/bug-1767430-access-control-raise-ip-address-conflict-on-host-routes-0c298125fee4a640.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The access-allow API has now been fixed to validate duplicate IP + addresses by different notation styles. For example, if a host with IP + 172.16.21.24 already has access to an NFS share, access cannot be + requested for 172.16.21.24/32.