From 9df750fd19c6c11562b7bf86464185cd63568310 Mon Sep 17 00:00:00 2001 From: Tom Barron Date: Thu, 28 Feb 2019 15:35:12 -0500 Subject: [PATCH] Only allow IP access type for CephFS NFS For the CephFS NFS back end only ``IP`` access type is valid so enforce this in the driver. Also validate access level since there is a utility routine to check both access type and access level. Closes-bug: #1816420 Change-Id: I6c96f861b30ef7ccac05a7c199a62f0d69044c3a --- manila/share/drivers/ganesha/__init__.py | 75 ++++++++++++------- manila/share/drivers/ganesha/utils.py | 2 +- manila/tests/db/fakes.py | 3 + manila/tests/share/drivers/test_ganesha.py | 53 +++++++++++++ ...ess-type-for-ganehas-c42ce6f859fa0c8c.yaml | 8 ++ 5 files changed, 114 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/bug-1816420-validate-access-type-for-ganehas-c42ce6f859fa0c8c.yaml diff --git a/manila/share/drivers/ganesha/__init__.py b/manila/share/drivers/ganesha/__init__.py index 075d72adf2..1a8127b126 100644 --- a/manila/share/drivers/ganesha/__init__.py +++ b/manila/share/drivers/ganesha/__init__.py @@ -38,7 +38,7 @@ class NASHelperBase(object): # drivers that use a helper derived from this class # should pass the following attributes to - # ganesha_utils.validate_acces_rule in their + # ganesha_utils.validate_access_rule in their # update_access implementation. supported_access_types = () supported_access_levels = () @@ -60,7 +60,8 @@ class GaneshaNASHelper(NASHelperBase): """Perform share access changes using Ganesha version < 2.4.""" supported_access_types = ('ip', ) - supported_access_levels = (constants.ACCESS_LEVEL_RW, ) + supported_access_levels = (constants.ACCESS_LEVEL_RW, + constants.ACCESS_LEVEL_RO) def __init__(self, execute, config, tag='', **kwargs): super(GaneshaNASHelper, self).__init__(execute, config, **kwargs) @@ -127,8 +128,9 @@ class GaneshaNASHelper(NASHelperBase): def _allow_access(self, base_path, share, access): """Allow access to the share.""" - if access['access_type'] != 'ip': - raise exception.InvalidShareAccess('Only IP access type allowed') + ganesha_utils.validate_access_rule( + self.supported_access_types, self.supported_access_levels, + access, abort=True) access = ganesha_utils.fixup_access_rule(access) @@ -157,15 +159,23 @@ class GaneshaNASHelper(NASHelperBase): def update_access(self, context, share, access_rules, add_rules, delete_rules, share_server=None): """Update access rules of share.""" + rule_state_map = {} if not (add_rules or delete_rules): add_rules = access_rules self.ganesha.reset_exports() self.ganesha.restart_service() for rule in add_rules: - self._allow_access('/', share, rule) + try: + self._allow_access('/', share, rule) + except (exception.InvalidShareAccess, + exception.InvalidShareAccessLevel): + rule_state_map[rule['id']] = {'state': 'error'} + continue + for rule in delete_rules: self._deny_access('/', share, rule) + return rule_state_map class GaneshaNASHelper2(GaneshaNASHelper): @@ -228,6 +238,7 @@ class GaneshaNASHelper2(GaneshaNASHelper): confdict = {} existing_access_rules = [] + rule_state_map = {} if self.ganesha.check_export_exists(share['name']): confdict = self.ganesha._read_export(share['name']) @@ -243,6 +254,16 @@ class GaneshaNASHelper2(GaneshaNASHelper): wanted_rw_clients, wanted_ro_clients = [], [] for rule in access_rules: + + try: + ganesha_utils.validate_access_rule( + self.supported_access_types, self.supported_access_levels, + rule, True) + except (exception.InvalidShareAccess, + exception.InvalidShareAccessLevel): + rule_state_map[rule['id']] = {'state': 'error'} + continue + rule = ganesha_utils.fixup_access_rule(rule) if rule['access_level'] == 'rw': wanted_rw_clients.append(rule['access_to']) @@ -263,28 +284,30 @@ class GaneshaNASHelper2(GaneshaNASHelper): 'Clients': ','.join(wanted_rw_clients) }) - if existing_access_rules: - # Update existing export. - ganesha_utils.patch(confdict, { - 'EXPORT': { - 'CLIENT': clients - } - }) - self.ganesha.update_export(share['name'], confdict) - else: - # Add new export. - ganesha_utils.patch(confdict, self.export_template, { - 'EXPORT': { - 'Export_Id': self.ganesha.get_export_id(), - 'Path': self._get_export_path(share), - 'Pseudo': self._get_export_pseudo_path(share), - 'Tag': share['name'], - 'CLIENT': clients, - 'FSAL': self._fsal_hook(None, share, None) - } - }) - self.ganesha.add_export(share['name'], confdict) + if clients: # Empty list if no rules passed validation + if existing_access_rules: + # Update existing export. + ganesha_utils.patch(confdict, { + 'EXPORT': { + 'CLIENT': clients + } + }) + self.ganesha.update_export(share['name'], confdict) + else: + # Add new export. + ganesha_utils.patch(confdict, self.export_template, { + 'EXPORT': { + 'Export_Id': self.ganesha.get_export_id(), + 'Path': self._get_export_path(share), + 'Pseudo': self._get_export_pseudo_path(share), + 'Tag': share['name'], + 'CLIENT': clients, + 'FSAL': self._fsal_hook(None, share, None) + } + }) + self.ganesha.add_export(share['name'], confdict) else: # No clients have access to the share. Remove export. self.ganesha.remove_export(share['name']) self._cleanup_fsal_hook(None, share, None) + return rule_state_map diff --git a/manila/share/drivers/ganesha/utils.py b/manila/share/drivers/ganesha/utils.py index 6208a98aed..bc9eb044f7 100644 --- a/manila/share/drivers/ganesha/utils.py +++ b/manila/share/drivers/ganesha/utils.py @@ -139,7 +139,7 @@ def validate_access_rule(supported_access_types, supported_access_levels, def fixup_access_rule(access_rule): """Adjust access rule as required for ganesha to handle it properly. - :param access_rule: Access rules to be validated. + :param access_rule: Access rules to be fixed up. :return: access_rule """ if access_rule['access_to'] == '0.0.0.0/0': diff --git a/manila/tests/db/fakes.py b/manila/tests/db/fakes.py index 2e45f6b338..89732731b8 100644 --- a/manila/tests/db/fakes.py +++ b/manila/tests/db/fakes.py @@ -42,6 +42,9 @@ class FakeModel(object): def __contains__(self, key): return self._getattr__(key) + def to_dict(self): + return self.values + def stub_out(stubs, funcs): """Set the stubs in mapping in the db api.""" diff --git a/manila/tests/share/drivers/test_ganesha.py b/manila/tests/share/drivers/test_ganesha.py index e3d890b017..8ac14a311f 100644 --- a/manila/tests/share/drivers/test_ganesha.py +++ b/manila/tests/share/drivers/test_ganesha.py @@ -238,6 +238,8 @@ class GaneshaNASHelperTestCase(test.TestCase): mock.Mock(return_value='fakefsal')) self.mock_object(ganesha.ganesha_utils, 'patch', mock.Mock(side_effect=fake_patch_run)) + self.mock_object(ganesha.ganesha_utils, 'validate_access_rule', + mock.Mock(return_value=True)) ret = self._helper._allow_access(fake_basepath, self.share, self.access) self._helper.ganesha.get_export_id.assert_called_once_with() @@ -308,6 +310,26 @@ class GaneshaNASHelperTestCase(test.TestCase): self.assertTrue(self._helper.ganesha.reset_exports.called) self.assertTrue(self._helper.ganesha.restart_service.called) + def test_update_access_invalid_share_access_type(self): + bad_rule = fake_share.fake_access(access_type='notip', id='fakeid') + expected = {'fakeid': {'state': 'error'}} + + result = self._helper.update_access(self._context, self.share, + access_rules=[bad_rule], + add_rules=[], delete_rules=[]) + + self.assertEqual(expected, result) + + def test_update_access_invalid_share_access_level(self): + bad_rule = fake_share.fake_access(access_level='RO', id='fakeid') + expected = {'fakeid': {'state': 'error'}} + + result = self._helper.update_access(self._context, self.share, + access_rules=[bad_rule], + add_rules=[], delete_rules=[]) + + self.assertEqual(expected, result) + @ddt.ddt class GaneshaNASHelper2TestCase(test.TestCase): @@ -445,6 +467,8 @@ class GaneshaNASHelper2TestCase(test.TestCase): mock.Mock(return_value='/fakepath')) self.mock_object(self._helper, '_fsal_hook', mock.Mock(return_value={'Name': 'fake'})) + self.mock_object(ganesha.ganesha_utils, 'validate_access_rule', + mock.Mock(return_value=True)) result_confdict = { 'EXPORT': { 'Export_Id': 100, @@ -484,6 +508,8 @@ class GaneshaNASHelper2TestCase(test.TestCase): mock_gh, '_read_export', mock.Mock(return_value={'EXPORT': {'CLIENT': client}}) ) + self.mock_object(ganesha.ganesha_utils, 'validate_access_rule', + mock.Mock(return_value=True)) result_confdict = { 'EXPORT': { 'CLIENT': [ @@ -541,3 +567,30 @@ class GaneshaNASHelper2TestCase(test.TestCase): self.assertFalse(mock_gh.update_export.called) self.assertFalse(mock_gh.remove_export.called) self.assertFalse(self._helper._cleanup_fsal_hook.called) + + def test_update_access_invalid_share_access_type(self): + mock_gh = self._helper.ganesha + self.mock_object(mock_gh, 'check_export_exists', + mock.Mock(return_value=False)) + bad_rule = fake_share.fake_access(access_type='notip', id='fakeid') + expected = {'fakeid': {'state': 'error'}} + + result = self._helper.update_access(self._context, self.share, + access_rules=[bad_rule], + add_rules=[], delete_rules=[]) + + self.assertEqual(expected, result) + + def test_update_access_invalid_share_access_level(self): + bad_rule = fake_share.fake_access(access_level='NOT_RO_OR_RW', + id='fakeid') + expected = {'fakeid': {'state': 'error'}} + mock_gh = self._helper.ganesha + self.mock_object(mock_gh, 'check_export_exists', + mock.Mock(return_value=False)) + + result = self._helper.update_access(self._context, self.share, + access_rules=[bad_rule], + add_rules=[], delete_rules=[]) + + self.assertEqual(expected, result) diff --git a/releasenotes/notes/bug-1816420-validate-access-type-for-ganehas-c42ce6f859fa0c8c.yaml b/releasenotes/notes/bug-1816420-validate-access-type-for-ganehas-c42ce6f859fa0c8c.yaml new file mode 100644 index 0000000000..255006392e --- /dev/null +++ b/releasenotes/notes/bug-1816420-validate-access-type-for-ganehas-c42ce6f859fa0c8c.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Access rule type for shares served via nfs-ganesha is now validated, + fixing `launchpad bug #1816420 `_ + where ``cephx`` access type was allowed though only ``ip`` access type + is effective. This fix also validates ``access_level`` to ensure that + it is set to ``RW`` or ``RO``.