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>

diff --git a/qa/tasks/cephfs/test_admin.py b/qa/tasks/cephfs/test_admin.py
index f109e24020514e5fed7a6ccf07c4cdc0525a0cf2..98548df8cd3cdd4beb4bec9f3a16f53aff5b3b26 100644 (file)
--- a/qa/tasks/cephfs/test_admin.py
+++ b/qa/tasks/cephfs/test_admin.py
@@ -1480,6 +1480,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 c1d410eaf76fb174dcdc4ec2c34b5316c87e5a98..2b696085d5b91f4730fe3ca24d123fd6800eadab 100644 (file)
--- a/src/mds/MDSAuthCaps.h
+++ b/src/mds/MDSAuthCaps.h
@@ -259,7 +259,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,