mds/MDSAuthCaps: allow adding multiple caps via "fs auth" cmd

When multiple caps are passed to "ceph fs authorize" command, the
command hangs because the MON crashes due to this command. Underlying
code assumes that only one cap can be passed at a time by the user
through the "ceph fs authorize" command. "ceph_assert()" checks if only
one cap is passed. If not, the code crashes.

Fixes: https://tracker.ceph.com/issues/64127
Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 56e0f73afe5fdebf38af339efea0dfde65a4de87)

diff --git a/qa/tasks/cephfs/test_admin.py b/qa/tasks/cephfs/test_admin.py
index a22fbb746e0eedf2347e68b63c4aab0b0c0dbd04..6c8e669376e1ad71ab9cf67dcabece932ed6edcc 100644 (file)
--- a/qa/tasks/cephfs/test_admin.py
+++ b/qa/tasks/cephfs/test_admin.py
@@ -1853,6 +1853,26 @@ class TestFsAuthorize(CephFSTestCase):
             FS_AUTH_CAPS[2], self.captesters[2], self.fs2.name, self.mount_b,
             keyring)
 
+    def test_adding_multiple_caps(self):
+        '''
+        Test that the command "ceph fs authorize" is successful in updating
+        the entity's caps when multiple caps are passed to it in one go.
+
+        Tests: https://tracker.ceph.com/issues/64127
+        '''
+        DIR = 'dir1'
+        self.mount_a.run_shell(f'mkdir {DIR}')
+        self.captesters = (CapTester(self.mount_a, '/'),
+                           CapTester(self.mount_a, f'/{DIR}'))
+        self.fs.authorize(self.client_id, ('/', 'rw'))
+
+        FS_AUTH_CAPS = ('/', 'rw', 'root_squash'), (f'/{DIR}', 'rw' )
+        # The fact that following line passes means
+        # https://tracker.ceph.com/issues/64127 has been fixed
+        keyring = self.fs.authorize(self.client_id, FS_AUTH_CAPS)
+
+        self._remount_and_run_tests(FS_AUTH_CAPS, keyring)
+
     def _remount_and_run_tests(self, fs_auth_caps, keyring):
         '''
         This method is specifically designed to meet needs of most of the

diff --git a/src/mds/MDSAuthCaps.cc b/src/mds/MDSAuthCaps.cc
index e5d8c4854017bec379baf109fa6c8060b894cb95..c65c7f86e2ea0f45a049d18f08e44780c81c14c5 100644 (file)
--- a/src/mds/MDSAuthCaps.cc
+++ b/src/mds/MDSAuthCaps.cc
@@ -382,25 +382,28 @@ bool MDSAuthCaps::parse(string_view str, ostream *err)
   }
 }
 
-bool MDSAuthCaps::merge(MDSAuthCaps newcap)
+/* Check if the "cap grant" is already present in this cap object. If it is,
+ * return false. If not, add it and return true.
+ *
+ * ng = new grant, new mds cap grant.
+ */
+bool MDSAuthCaps::merge_one_cap_grant(MDSCapGrant ng)
 {
-  ceph_assert(newcap.grants.size() == 1);
-  auto ng = newcap.grants[0];
-
+  // check if "ng" is already present in this cap object.
   for (auto& g : grants) {
     if (g.match.fs_name == ng.match.fs_name && g.match.path == ng.match.path) {
       if (g.spec.get_caps() == ng.spec.get_caps()) {
        // no update required. maintaining idempotency.
        return false;
        } else {
-       // cap for given fs name is present, let's update it.
+       // "ng" is present but perm/spec is different. update it.
        g.spec.set_caps(ng.spec.get_caps());
        return true;
       }
     }
   }
 
-  // cap for given fs name and/or path is absent, let's add a new cap for it.
+  // since "ng" is absent in this cap object, add it.
   grants.push_back(MDSCapGrant(
     MDSCapSpec(ng.spec.get_caps()),
     MDSCapMatch(ng.match.fs_name, ng.match.path, ng.match.root_squash),
@@ -409,6 +412,29 @@ bool MDSAuthCaps::merge(MDSAuthCaps newcap)
   return true;
 }
 
+/* User can pass one or MDS caps that it wishes to add to entity's keyring.
+ * Merge all of these caps one by one. Return value indicates whether or not
+ * AuthMonitor must update the entity's keyring.
+ *
+ * If all caps do not merge (that is, underlying helper method returns false
+ * after attempting merge), no update is required. Return false so that
+ * AuthMonitor doesn't run the update procedure for caps.
+ *
+ * If even one cap is merged (that is, underlying method returns true even
+ * once), an update to the entity's keyring is required. Return true so that
+ * AuthMonitor runs the update procedure.
+ */
+bool MDSAuthCaps::merge(MDSAuthCaps newcaps)
+{
+  bool were_caps_merged = false;
+
+  for (auto& ng : newcaps.grants) {
+      were_caps_merged |= merge_one_cap_grant(ng);
+  }
+
+  return were_caps_merged;
+}
+
 string MDSCapMatch::to_string()
 {
   string str = "";

diff --git a/src/mds/MDSAuthCaps.h b/src/mds/MDSAuthCaps.h
index 0be4f5ec13c6a2e4f8af944985c1826fd791005a..3c79b876aa68056cbed68e0a57f334a71e93e9fa 100644 (file)
--- a/src/mds/MDSAuthCaps.h
+++ b/src/mds/MDSAuthCaps.h
@@ -263,7 +263,8 @@ public:
 
   void set_allow_all();
   bool parse(std::string_view str, std::ostream *err);
-  bool merge(MDSAuthCaps newcap);
+  bool merge_one_cap_grant(MDSCapGrant ng);
+  bool merge(MDSAuthCaps newcaps);
 
   bool allow_all() const;
   bool is_capable(std::string_view inode_path,