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

@@ -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='<no name>', **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':
+        ganesha_utils.validate_access_rule(
-            raise exception.InvalidShareAccess('Only IP access type allowed')
+            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:
+            if clients:  # Empty list if no rules passed validation
-                # Update existing export.
+                if existing_access_rules:
-                ganesha_utils.patch(confdict, {
+                    # Update existing export.
-                    'EXPORT': {
+                    ganesha_utils.patch(confdict, {
-                        'CLIENT': clients
+                        'EXPORT': {
-                    }
+                            'CLIENT': clients
-                })
+                        }
-                self.ganesha.update_export(share['name'], confdict)
+                    })
-            else:
+                    self.ganesha.update_export(share['name'], confdict)
-                # Add new export.
+                else:
-                ganesha_utils.patch(confdict, self.export_template, {
+                    # Add new export.
-                    'EXPORT': {
+                    ganesha_utils.patch(confdict, self.export_template, {
-                        'Export_Id': self.ganesha.get_export_id(),
+                        'EXPORT': {
-                        'Path': self._get_export_path(share),
+                            'Export_Id': self.ganesha.get_export_id(),
-                        'Pseudo': self._get_export_pseudo_path(share),
+                            'Path': self._get_export_path(share),
-                        'Tag': share['name'],
+                            'Pseudo': self._get_export_pseudo_path(share),
-                        'CLIENT': clients,
+                            'Tag': share['name'],
-                        'FSAL': self._fsal_hook(None, share, None)
+                            'CLIENT': clients,
-                    }
+                            'FSAL': self._fsal_hook(None, share, None)
-                })
+                        }
-                self.ganesha.add_export(share['name'], confdict)
+                    })
+                    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

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':

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

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)

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 <https://bugs.launchpad.net/manila/+bug/1811680>`_
+    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``.