From 41593e9f3411dd1f32bafde09ac78de246cbad52 Mon Sep 17 00:00:00 2001
From: Rishabh Dave <ridave@redhat.com>
Date: Thu, 25 Jan 2024 16:50:29 +0530
Subject: [PATCH] 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)
---
 qa/tasks/cephfs/test_admin.py | 20 ++++++++++++++++++
 src/mds/MDSAuthCaps.cc        | 38 +++++++++++++++++++++++++++++------
 src/mds/MDSAuthCaps.h         |  3 ++-
 3 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/qa/tasks/cephfs/test_admin.py b/qa/tasks/cephfs/test_admin.py
index a22fbb746e0..6c8e669376e 100644
--- 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 e5d8c485401..c65c7f86e2e 100644
--- 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 0be4f5ec13c..3c79b876aa6 100644
--- 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,
-- 
2.39.5