openstack/manila
Code Issues Proposed changes

Fix access control for single host addresses

Browse Source 
In CIDR notation, the max prefix-length is typically
used to denote individual host addresses, for example:
2620:52:0:13b8::fe:e7 and 2620:52:0:13b8::fe:e7/128
are semantically the same.

Fix the access-allow API to raise 400 Bad Request if
an address by either notation already exists in the
manila database for a given share.

Change-Id: I6e790fd0edd82064a3c5cda8a919c9eeb2da85d0
Closes-Bug: 1767430
This commit is contained in:
Goutham Pacha Ravi 2018-05-14 12:50:38 -07:00
parent 1095fb7478
commit 50f957eefd
6 changed files with 173 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
manila/db/api.py
View File

@ -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)

46
manila/db/sqlalchemy/api.py
View File

@ -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,

15
manila/share/api.py
View File

@ -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)

77
manila/tests/db/sqlalchemy/test_api.py
View File

@ -224,6 +224,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):
@ -1346,6 +1384,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'])

45
manila/tests/share/test_api.py
View File

@ -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()

7
releasenotes/notes/bug-1767430-access-control-raise-ip-address-conflict-on-host-routes-0c298125fee4a640.yaml Normal file
View File

@ -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.