From d8a82f5aaa1a04b695c19b546767718b832559cb Mon Sep 17 00:00:00 2001
From: Casey Bodley <cbodley@redhat.com>
Date: Wed, 28 Jun 2023 16:49:33 -0400
Subject: [PATCH] rgw: BlockDecrypt filter parses manifest parts before
 construction

users now call a static read_manifest_parts() function, and pass the
resulting vector into the BlockDecrypt constructor

Signed-off-by: Casey Bodley <cbodley@redhat.com>
---
 src/rgw/rgw_crypt.cc            | 16 ++++----
 src/rgw/rgw_crypt.h             |  8 ++--
 src/rgw/rgw_rest_s3.cc          | 68 +++++++++++++++++++--------------
 src/test/rgw/test_rgw_crypto.cc | 64 +++++++++++++++----------------
 4 files changed, 85 insertions(+), 71 deletions(-)

diff --git a/src/rgw/rgw_crypt.cc b/src/rgw/rgw_crypt.cc
index d2c3edb0d55a9..9fdd9819ea177 100644
--- a/src/rgw/rgw_crypt.cc
+++ b/src/rgw/rgw_crypt.cc
@@ -661,6 +661,7 @@ RGWGetObj_BlockDecrypt::RGWGetObj_BlockDecrypt(const DoutPrefixProvider *dpp,
                                                CephContext* cct,
                                                RGWGetObj_Filter* next,
                                                std::unique_ptr<BlockCrypt> crypt,
+                                               std::vector<size_t> parts_len,
                                                optional_yield y)
     :
     RGWGetObj_Filter(next),
@@ -671,7 +672,8 @@ RGWGetObj_BlockDecrypt::RGWGetObj_BlockDecrypt(const DoutPrefixProvider *dpp,
     ofs(0),
     end(0),
     cache(),
-    y(y)
+    y(y),
+    parts_len(std::move(parts_len))
 {
   block_size = this->crypt->get_block_size();
 }
@@ -679,8 +681,10 @@ RGWGetObj_BlockDecrypt::RGWGetObj_BlockDecrypt(const DoutPrefixProvider *dpp,
 RGWGetObj_BlockDecrypt::~RGWGetObj_BlockDecrypt() {
 }
 
-int RGWGetObj_BlockDecrypt::read_manifest(const DoutPrefixProvider *dpp, bufferlist& manifest_bl) {
-  parts_len.clear();
+int RGWGetObj_BlockDecrypt::read_manifest_parts(const DoutPrefixProvider *dpp,
+                                                const bufferlist& manifest_bl,
+                                                std::vector<size_t>& parts_len)
+{
   RGWObjManifest manifest;
   if (manifest_bl.length()) {
     auto miter = manifest_bl.cbegin();
@@ -697,10 +701,8 @@ int RGWGetObj_BlockDecrypt::read_manifest(const DoutPrefixProvider *dpp, bufferl
       }
       parts_len.back() += mi.get_stripe_size();
     }
-    if (cct->_conf->subsys.should_gather<ceph_subsys_rgw, 20>()) {
-      for (size_t i = 0; i<parts_len.size(); i++) {
-        ldpp_dout(dpp, 20) << "Manifest part " << i << ", size=" << parts_len[i] << dendl;
-      }
+    for (size_t i = 0; i<parts_len.size(); i++) {
+      ldpp_dout(dpp, 20) << "Manifest part " << i << ", size=" << parts_len[i] << dendl;
     }
   }
   return 0;
diff --git a/src/rgw/rgw_crypt.h b/src/rgw/rgw_crypt.h
index b0c4886011276..48e4530da8d2c 100644
--- a/src/rgw/rgw_crypt.h
+++ b/src/rgw/rgw_crypt.h
@@ -101,16 +101,16 @@ class RGWGetObj_BlockDecrypt : public RGWGetObj_Filter {
   bufferlist cache; /**< stores extra data that could not (yet) be processed by BlockCrypt */
   size_t block_size; /**< snapshot of \ref BlockCrypt.get_block_size() */
   optional_yield y;
+  std::vector<size_t> parts_len; /**< size of parts of multipart object, parsed from manifest */
 
   int process(bufferlist& cipher, size_t part_ofs, size_t size);
 
-protected:
-  std::vector<size_t> parts_len; /**< size of parts of multipart object, parsed from manifest */
 public:
   RGWGetObj_BlockDecrypt(const DoutPrefixProvider *dpp,
                          CephContext* cct,
                          RGWGetObj_Filter* next,
                          std::unique_ptr<BlockCrypt> crypt,
+                         std::vector<size_t> parts_len,
                          optional_yield y);
   virtual ~RGWGetObj_BlockDecrypt();
 
@@ -121,7 +121,9 @@ public:
                           off_t bl_len) override;
   virtual int flush() override;
 
-  int read_manifest(const DoutPrefixProvider *dpp, bufferlist& manifest_bl);
+  static int read_manifest_parts(const DoutPrefixProvider *dpp,
+                                 const bufferlist& manifest_bl,
+                                 std::vector<size_t>& parts_len);
 }; /* RGWGetObj_BlockDecrypt */
 
 
diff --git a/src/rgw/rgw_rest_s3.cc b/src/rgw/rgw_rest_s3.cc
index d777b52d7f2d3..794d52dcf62ef 100644
--- a/src/rgw/rgw_rest_s3.cc
+++ b/src/rgw/rgw_rest_s3.cc
@@ -593,21 +593,28 @@ int RGWGetObj_ObjStore_S3::get_decrypt_filter(std::unique_ptr<RGWGetObj_Filter>
     return 0;
   }
 
-  int res = 0;
   std::unique_ptr<BlockCrypt> block_crypt;
-  res = rgw_s3_prepare_decrypt(s, attrs, &block_crypt, crypt_http_responses);
-  if (res == 0) {
-    if (block_crypt != nullptr) {
-      auto f = std::make_unique<RGWGetObj_BlockDecrypt>(s, s->cct, cb, std::move(block_crypt), s->yield);
-      if (manifest_bl != nullptr) {
-        res = f->read_manifest(this, *manifest_bl);
-        if (res == 0) {
-          *filter = std::move(f);
-        }
-      }
-    }
+  int res = rgw_s3_prepare_decrypt(s, attrs, &block_crypt, crypt_http_responses);
+  if (res < 0) {
+    return res;
   }
-  return res;
+  if (block_crypt == nullptr) {
+    return 0;
+  }
+
+  // in case of a multipart upload, we need to know the part lengths to
+  // correctly decrypt across part boundaries
+  std::vector<size_t> parts_len;
+  res = RGWGetObj_BlockDecrypt::read_manifest_parts(this, *manifest_bl,
+                                                    parts_len);
+  if (res < 0) {
+    return res;
+  }
+
+  *filter = std::make_unique<RGWGetObj_BlockDecrypt>(
+      s, s->cct, cb, std::move(block_crypt),
+      std::move(parts_len), s->yield);
+  return 0;
 }
 int RGWGetObj_ObjStore_S3::verify_requester(const rgw::auth::StrategyRegistry& auth_registry, optional_yield y) 
 {
@@ -2745,23 +2752,28 @@ int RGWPutObj_ObjStore_S3::get_decrypt_filter(
 {
   std::map<std::string, std::string> crypt_http_responses_unused;
 
-  int res = 0;
   std::unique_ptr<BlockCrypt> block_crypt;
-  res = rgw_s3_prepare_decrypt(s, attrs, &block_crypt, crypt_http_responses_unused);
-  if (res == 0) {
-    if (block_crypt != nullptr) {
-      auto f = std::unique_ptr<RGWGetObj_BlockDecrypt>(new RGWGetObj_BlockDecrypt(s, s->cct, cb, std::move(block_crypt), s->yield));
-      if (f != nullptr) {
-        if (manifest_bl != nullptr) {
-          res = f->read_manifest(this, *manifest_bl);
-          if (res == 0) {
-            *filter = std::move(f);
-          }
-        }
-      }
-    }
+  int res = rgw_s3_prepare_decrypt(s, attrs, &block_crypt, crypt_http_responses_unused);
+  if (res < 0) {
+    return res;
   }
-  return res;
+  if (block_crypt == nullptr) {
+    return 0;
+  }
+
+  // in case of a multipart upload, we need to know the part lengths to
+  // correctly decrypt across part boundaries
+  std::vector<size_t> parts_len;
+  res = RGWGetObj_BlockDecrypt::read_manifest_parts(this, *manifest_bl,
+                                                    parts_len);
+  if (res < 0) {
+    return res;
+  }
+
+  *filter = std::make_unique<RGWGetObj_BlockDecrypt>(
+      s, s->cct, cb, std::move(block_crypt),
+      std::move(parts_len), s->yield);
+  return 0;
 }
 
 int RGWPutObj_ObjStore_S3::get_encrypt_filter(
diff --git a/src/test/rgw/test_rgw_crypto.cc b/src/test/rgw/test_rgw_crypto.cc
index b8148882d4aa3..88104ebefd3de 100644
--- a/src/test/rgw/test_rgw_crypto.cc
+++ b/src/test/rgw/test_rgw_crypto.cc
@@ -428,7 +428,7 @@ TEST(TestRGWCrypto, verify_RGWGetObj_BlockDecrypt_ranges)
     ut_get_sink get_sink;
     auto cbc = AES_256_CBC_create(&no_dpp, g_ceph_context, &key[0], 32);
     ASSERT_NE(cbc.get(), nullptr);
-    RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink, std::move(cbc), null_yield);
+    RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink, std::move(cbc), {}, null_yield);
 
     //random ranges
     off_t begin = (r/3)*r*(r+13)*(r+23)*(r+53)*(r+71) % test_range;
@@ -474,7 +474,7 @@ TEST(TestRGWCrypto, verify_RGWGetObj_BlockDecrypt_chunks)
     ut_get_sink get_sink;
     auto cbc = AES_256_CBC_create(&no_dpp, g_ceph_context, &key[0], 32);
     ASSERT_NE(cbc.get(), nullptr);
-    RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink, std::move(cbc), null_yield);
+    RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink, std::move(cbc), {}, null_yield);
 
     //random
     off_t begin = (r/3)*r*(r+13)*(r+23)*(r+53)*(r+71) % test_range;
@@ -520,7 +520,7 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup)
   ut_get_sink get_sink;
   auto nonecrypt = std::unique_ptr<BlockCrypt>(new BlockCryptNone);
   RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
-                                 std::move(nonecrypt), null_yield);
+                                 std::move(nonecrypt), {}, null_yield);
   ASSERT_EQ(fixup_range(&decrypt,0,0),     range_t(0,255));
   ASSERT_EQ(fixup_range(&decrypt,1,256),   range_t(0,511));
   ASSERT_EQ(fixup_range(&decrypt,0,255),   range_t(0,255));
@@ -529,16 +529,6 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup)
   ASSERT_EQ(fixup_range(&decrypt,513,1024), range_t(512,1024+255));
 }
 
-using parts_len_t = std::vector<size_t>;
-
-class TestRGWGetObj_BlockDecrypt : public RGWGetObj_BlockDecrypt {
-  using RGWGetObj_BlockDecrypt::RGWGetObj_BlockDecrypt;
-public:
-  void set_parts_len(parts_len_t&& other) {
-    parts_len = std::move(other);
-  }
-};
-
 std::vector<size_t> create_mp_parts(size_t obj_size, size_t mp_part_len){
   std::vector<size_t> parts_len;
   size_t part_size;
@@ -561,9 +551,10 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup_simple)
 
   ut_get_sink get_sink;
   auto nonecrypt = std::make_unique<BlockCryptNone>(4096);
-  TestRGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
-				     std::move(nonecrypt), null_yield);
-  decrypt.set_parts_len(create_mp_parts(obj_size, part_size));
+  RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
+				 std::move(nonecrypt),
+				 create_mp_parts(obj_size, part_size),
+				 null_yield);
   ASSERT_EQ(fixup_range(&decrypt,0,0),     range_t(0,4095));
   ASSERT_EQ(fixup_range(&decrypt,1,4096),   range_t(0,8191));
   ASSERT_EQ(fixup_range(&decrypt,0,4095),   range_t(0,4095));
@@ -590,12 +581,14 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup_non_aligned_obj_size)
 {
   const NoDoutPrefix no_dpp(g_ceph_context, dout_subsys);
 
+  const size_t na_obj_size = obj_size + 1;
+
   ut_get_sink get_sink;
   auto nonecrypt = std::make_unique<BlockCryptNone>(4096);
-  TestRGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
-				     std::move(nonecrypt), null_yield);
-  auto na_obj_size = obj_size + 1;
-  decrypt.set_parts_len(create_mp_parts(na_obj_size, part_size));
+  RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
+				 std::move(nonecrypt),
+				 create_mp_parts(na_obj_size, part_size),
+				 null_yield);
 
   // these should be unaffected here
   ASSERT_EQ(fixup_range(&decrypt, 0, part_size - 2), range_t(0, part_size -1));
@@ -618,12 +611,14 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup_non_aligned_part_size)
 {
   const NoDoutPrefix no_dpp(g_ceph_context, dout_subsys);
 
+  const size_t na_part_size = part_size + 1;
+
   ut_get_sink get_sink;
   auto nonecrypt = std::make_unique<BlockCryptNone>(4096);
-  TestRGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
-				     std::move(nonecrypt), null_yield);
-  auto na_part_size = part_size + 1;
-  decrypt.set_parts_len(create_mp_parts(obj_size, na_part_size));
+  RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
+				 std::move(nonecrypt),
+				 create_mp_parts(obj_size, na_part_size),
+				 null_yield);
 
   // na_part_size -2, ie. part_size -1  is aligned to 4095 boundary
   ASSERT_EQ(fixup_range(&decrypt, 0, na_part_size - 2), range_t(0, na_part_size -2));
@@ -652,13 +647,15 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup_non_aligned)
 {
   const NoDoutPrefix no_dpp(g_ceph_context, dout_subsys);
 
+  const size_t na_part_size = part_size + 1;
+  const size_t na_obj_size = obj_size + 7; // (6*(5MiB + 1) + 1) for the last 1B overflow
+
   ut_get_sink get_sink;
   auto nonecrypt = std::make_unique<BlockCryptNone>(4096);
-  TestRGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
-				     std::move(nonecrypt), null_yield);
-  auto na_part_size = part_size + 1;
-  auto na_obj_size = obj_size + 7; // (6*(5MiB + 1) + 1) for the last 1B overflow
-  decrypt.set_parts_len(create_mp_parts(na_obj_size, na_part_size));
+  RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
+				 std::move(nonecrypt),
+				 create_mp_parts(na_obj_size, na_part_size),
+				 null_yield);
 
   // na_part_size -2, ie. part_size -1  is aligned to 4095 boundary
   ASSERT_EQ(fixup_range(&decrypt, 0, na_part_size - 2), range_t(0, na_part_size -2));
@@ -684,10 +681,11 @@ TEST(TestRGWCrypto, check_RGWGetObj_BlockDecrypt_fixup_invalid_ranges)
 
   ut_get_sink get_sink;
   auto nonecrypt = std::make_unique<BlockCryptNone>(4096);
-  TestRGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
-				     std::move(nonecrypt), null_yield);
+  RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
+				 std::move(nonecrypt),
+				 create_mp_parts(obj_size, part_size),
+				 null_yield);
 
-  decrypt.set_parts_len(create_mp_parts(obj_size, part_size));
 
   // the ranges below would be mostly unreachable in current code as rgw
   // would've returned a 411 before reaching, but we're just doing this to make
@@ -799,7 +797,7 @@ TEST(TestRGWCrypto, verify_Encrypt_Decrypt)
     ut_get_sink get_sink;
     RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink,
                                    AES_256_CBC_create(&no_dpp, g_ceph_context, &key[0], 32),
-                                   null_yield);
+                                   {}, null_yield);
 
     off_t bl_ofs = 0;
     off_t bl_end = test_size - 1;
-- 
2.39.5