rgw/user: add 'active' flag to RGWAccessKey

inactive keys are removed from the key pool so can't be used to
authenticate. the 'key create' admin op now takes an 'active' option

Fixes: https://tracker.ceph.com/issues/59186
Signed-off-by: Casey Bodley <cbodley@redhat.com>
(cherry picked from commit 463f463d50943c6fe17c39e41f64b1df27944ff6)

diff --git a/doc/radosgw/adminops.rst b/doc/radosgw/adminops.rst
index f9e501515d0bd52901b8a48d47a7c522b68c98c9..643190123d3f6ea8ecbb9717fa6bc44d6d3de80d 100644 (file)
--- a/doc/radosgw/adminops.rst
+++ b/doc/radosgw/adminops.rst
@@ -1164,6 +1164,13 @@ Request Parameters
 :Example: True [True]
 :Required: No
 
+``active``
+
+:Description: Activate or deactivate a key.
+:Type: Boolean
+:Example: True [True]
+:Required: No
+
 
 Response Entities
 ~~~~~~~~~~~~~~~~~

diff --git a/src/rgw/driver/rados/rgw_rest_user.cc b/src/rgw/driver/rados/rgw_rest_user.cc
index 89712970fb2da92cb6550d556bd39a694ad72aeb..58623cd646895e84c541c64b4f6d2c72319e1060 100644 (file)
--- a/src/rgw/driver/rados/rgw_rest_user.cc
+++ b/src/rgw/driver/rados/rgw_rest_user.cc
@@ -665,7 +665,9 @@ void RGWOp_Key_Create::execute(optional_yield y)
   std::string secret_key;
   std::string key_type_str;
 
-  bool gen_key;
+  bool gen_key = true;
+  bool active = true;
+  bool active_specified = false;
 
   RGWUserAdminOpState op_state(driver);
 
@@ -676,12 +678,16 @@ void RGWOp_Key_Create::execute(optional_yield y)
   RESTArgs::get_string(s, "access-key", access_key, &access_key);
   RESTArgs::get_string(s, "secret-key", secret_key, &secret_key);
   RESTArgs::get_string(s, "key-type", key_type_str, &key_type_str);
-  RESTArgs::get_bool(s, "generate-key", true, &gen_key);
+  RESTArgs::get_bool(s, "generate-key", gen_key, &gen_key);
+  RESTArgs::get_bool(s, "active", active, &active, &active_specified);
 
   op_state.set_user_id(uid);
   op_state.set_subuser(subuser);
   op_state.set_access_key(access_key);
   op_state.set_secret_key(secret_key);
+  if (active_specified) {
+    op_state.access_key_active = active;
+  }
 
   if (gen_key)
     op_state.set_generate_key();

diff --git a/src/rgw/driver/rados/rgw_user.cc b/src/rgw/driver/rados/rgw_user.cc
index f13e087c436484faa58aba550c20fad9b1c84997..b282083c824c56709ce8477827a0bfd69081d45e 100644 (file)
--- a/src/rgw/driver/rados/rgw_user.cc
+++ b/src/rgw/driver/rados/rgw_user.cc
@@ -102,6 +102,7 @@ static void dump_access_keys_info(Formatter *f, RGWUserInfo &info)
     f->dump_format("user", "%s%s%s", s.c_str(), sep, subuser);
     f->dump_string("access_key", k.id);
     f->dump_string("secret_key", k.key);
+    f->dump_bool("active", k.active);
     f->close_section();
   }
   f->close_section();
@@ -120,6 +121,7 @@ static void dump_swift_keys_info(Formatter *f, RGWUserInfo &info)
     info.user_id.to_str(s);
     f->dump_format("user", "%s%s%s", s.c_str(), sep, subuser);
     f->dump_string("secret_key", k.key);
+    f->dump_bool("active", k.active);
     f->close_section();
   }
   f->close_section();
@@ -603,11 +605,6 @@ int RGWAccessKeyPool::modify_key(RGWUserAdminOpState& op_state, std::string *err
   std::string key = op_state.get_secret_key();
   int key_type = op_state.get_key_type();
 
-  RGWAccessKey modify_key;
-
-  pair<string, RGWAccessKey> key_pair;
-  map<std::string, RGWAccessKey>::iterator kiter;
-
   switch (key_type) {
   case KEY_TYPE_S3:
     id = op_state.get_access_key();
@@ -633,8 +630,8 @@ int RGWAccessKeyPool::modify_key(RGWUserAdminOpState& op_state, std::string *err
     return -ERR_INVALID_ACCESS_KEY;
   }
 
-  key_pair.first = id;
-
+  RGWAccessKey modify_key;
+  map<std::string, RGWAccessKey>::iterator kiter;
   if (key_type == KEY_TYPE_SWIFT) {
     modify_key.id = id;
     modify_key.subuser = op_state.get_subuser();
@@ -652,16 +649,13 @@ int RGWAccessKeyPool::modify_key(RGWUserAdminOpState& op_state, std::string *err
     key = secret_key_buf;
   }
 
-  if (key.empty()) {
-      set_err_msg(err_msg, "empty secret key");
-      return -ERR_INVALID_SECRET_KEY;
+  if (!key.empty()) {
+    // update the access key with the new secret key
+    modify_key.key = key;
+  }
+  if (op_state.access_key_active) {
+    modify_key.active = *op_state.access_key_active;
   }
-
-  // update the access key with the new secret key
-  modify_key.key = key;
-
-  key_pair.second = modify_key;
-
 
   if (key_type == KEY_TYPE_S3) {
     (*access_keys)[id] = modify_key;

diff --git a/src/rgw/driver/rados/rgw_user.h b/src/rgw/driver/rados/rgw_user.h
index 91c859feafca512dd7883d7e11b1733da63105f6..46500fb1acaead4ffb8c2721b16978cc037c66a0 100644 (file)
--- a/src/rgw/driver/rados/rgw_user.h
+++ b/src/rgw/driver/rados/rgw_user.h
@@ -135,6 +135,7 @@ struct RGWUserAdminOpState {
   std::map<std::string, RGWAccessKey> op_access_keys;
   int32_t key_type{-1};
   bool access_key_exist = false;
+  std::optional<bool> access_key_active;
 
   std::set<std::string> mfa_ids;
 
@@ -275,6 +276,10 @@ struct RGWUserAdminOpState {
     access_key_exist = true;
   }
 
+  void set_access_key_active(bool active) {
+    access_key_active = active;
+  }
+
   void set_suspension(__u8 is_suspended) {
     suspended = is_suspended;
     suspension_op = true;

diff --git a/src/rgw/rgw_acl_types.h b/src/rgw/rgw_acl_types.h
index b9866e9b289a1bae8de1885eaef7e7c389a7486f..3f9f1715aba435e18be25bf1fb8cea19a19c8602 100644 (file)
--- a/src/rgw/rgw_acl_types.h
+++ b/src/rgw/rgw_acl_types.h
@@ -45,24 +45,29 @@ struct RGWAccessKey {
   std::string id; // AccessKey
   std::string key; // SecretKey
   std::string subuser;
+  bool active = true;
 
   RGWAccessKey() {}
   RGWAccessKey(std::string _id, std::string _key)
     : id(std::move(_id)), key(std::move(_key)) {}
 
   void encode(bufferlist& bl) const {
-    ENCODE_START(2, 2, bl);
+    ENCODE_START(3, 2, bl);
     encode(id, bl);
     encode(key, bl);
     encode(subuser, bl);
+    encode(active, bl);
     ENCODE_FINISH(bl);
   }
 
   void decode(bufferlist::const_iterator& bl) {
-     DECODE_START_LEGACY_COMPAT_LEN_32(2, 2, 2, bl);
+     DECODE_START_LEGACY_COMPAT_LEN_32(3, 2, 2, bl);
      decode(id, bl);
      decode(key, bl);
      decode(subuser, bl);
+     if (struct_v >= 3) {
+       decode(active, bl);
+     }
      DECODE_FINISH(bl);
   }
   void dump(Formatter *f) const;

diff --git a/src/rgw/rgw_admin.cc b/src/rgw/rgw_admin.cc
index 8265852973f905716badf580c519d9ffe34453c6..2977933b2e1cf811a785023f6e3a81234d90d0a6 100644 (file)
--- a/src/rgw/rgw_admin.cc
+++ b/src/rgw/rgw_admin.cc
@@ -336,6 +336,7 @@ void usage()
   cout << "   --gen-access-key                  generate random access key (for S3)\n";
   cout << "   --gen-secret                      generate random secret key\n";
   cout << "   --key-type=<type>                 key type, options are: swift, s3\n";
+  cout << "   --key-active=<bool>               activate or deactivate a key\n";
   cout << "   --temp-url-key[-2]=<key>          temp url key\n";
   cout << "   --access=<access>                 Set access permissions for sub-user, should be one\n";
   cout << "                                     of read, write, readwrite, full\n";
@@ -3357,6 +3358,8 @@ int main(int argc, const char **argv)
   int commit = false;
   int staging = false;
   int key_type = KEY_TYPE_UNDEFINED;
+  int key_active = true;
+  bool key_active_specified = false;
   std::unique_ptr<rgw::sal::Bucket> bucket;
   uint32_t perm_mask = 0;
   RGWUserInfo info;
@@ -3610,6 +3613,8 @@ int main(int argc, const char **argv)
         cerr << "bad key type: " << key_type_str << std::endl;
         exit(1);
       }
+    } else if (ceph_argparse_binary_flag(args, i, &key_active, NULL, "--key-active", (char*)NULL)) {
+      key_active_specified = true;
     } else if (ceph_argparse_witharg(args, i, &val, "--job-id", (char*)NULL)) {
       job_id = val;
     } else if (ceph_argparse_binary_flag(args, i, &gen_access_key, NULL, "--gen-access-key", (char*)NULL)) {
@@ -6443,6 +6448,10 @@ int main(int argc, const char **argv)
   if (key_type != KEY_TYPE_UNDEFINED)
     user_op.set_key_type(key_type);
 
+  if (key_active_specified) {
+    user_op.access_key_active = key_active;
+  }
+
   // set suspension operation parameters
   if (opt_cmd == OPT::USER_ENABLE)
     user_op.set_suspension(false);

diff --git a/src/rgw/rgw_common.cc b/src/rgw/rgw_common.cc
index 125d2780183870d4c4716b1e53f5587007487aae..3ee98fa18ca16b68235adf639fcd20de970621e6 100644 (file)
--- a/src/rgw/rgw_common.cc
+++ b/src/rgw/rgw_common.cc
@@ -2913,6 +2913,7 @@ void RGWAccessKey::dump(Formatter *f) const
   encode_json("access_key", id, f);
   encode_json("secret_key", key, f);
   encode_json("subuser", subuser, f);
+  encode_json("active", active, f);
 }
 
 void RGWAccessKey::dump_plain(Formatter *f) const
@@ -2933,6 +2934,7 @@ void RGWAccessKey::dump(Formatter *f, const string& user, bool swift) const
     encode_json("access_key", id, f);
   }
   encode_json("secret_key", key, f);
+  encode_json("active", active, f);
 }
 
 void RGWAccessKey::decode_json(JSONObj *obj) {
@@ -2946,6 +2948,7 @@ void RGWAccessKey::decode_json(JSONObj *obj) {
       subuser = user.substr(pos + 1);
     }
   }
+  JSONDecoder::decode_json("active", active, obj);
 }
 
 void RGWAccessKey::decode_json(JSONObj *obj, bool swift) {
@@ -2962,6 +2965,7 @@ void RGWAccessKey::decode_json(JSONObj *obj, bool swift) {
     }
   }
   JSONDecoder::decode_json("secret_key", key, obj, true);
+  JSONDecoder::decode_json("active", active, obj);
 }
 
 void RGWStorageStats::dump(Formatter *f) const

diff --git a/src/rgw/services/svc_user_rados.cc b/src/rgw/services/svc_user_rados.cc
index dd48bc4c01efef492b5757d19877fc4c289197e0..0d01c96d481ff919250bf5285a0c5edcb0f9c225 100644 (file)
--- a/src/rgw/services/svc_user_rados.cc
+++ b/src/rgw/services/svc_user_rados.cc
@@ -151,6 +151,22 @@ int RGWSI_User_RADOS::read_user_info(RGWSI_MetaBackend::Context *ctx,
   return 0;
 }
 
+static bool s3_key_active(const RGWUserInfo* info, const std::string& id) {
+  if (!info) {
+    return false;
+  }
+  auto i = info->access_keys.find(id);
+  return i != info->access_keys.end() && i->second.active;
+}
+
+static bool swift_key_active(const RGWUserInfo* info, const std::string& id) {
+  if (!info) {
+    return false;
+  }
+  auto i = info->swift_keys.find(id);
+  return i != info->swift_keys.end() && i->second.active;
+}
+
 class PutOperation
 {
   RGWSI_User_RADOS::Svc& svc;
@@ -203,28 +219,28 @@ public:
       }
     }
 
-    for (auto iter = info.swift_keys.begin(); iter != info.swift_keys.end(); ++iter) {
-      if (old_info && old_info->swift_keys.count(iter->first) != 0)
+    for (const auto& [id, key] : info.swift_keys) {
+      if (!key.active || swift_key_active(old_info, id))
         continue;
-      auto& k = iter->second;
       /* check if swift mapping exists */
       RGWUserInfo inf;
-      int r = svc.user->get_user_info_by_swift(ctx, k.id, &inf, nullptr, nullptr, y, dpp);
+      int r = svc.user->get_user_info_by_swift(ctx, id, &inf, nullptr, nullptr, y, dpp);
       if (r >= 0 && inf.user_id != info.user_id &&
           (!old_info || inf.user_id != old_info->user_id)) {
-        ldpp_dout(dpp, 0) << "WARNING: can't store user info, swift id (" << k.id
+        ldpp_dout(dpp, 0) << "WARNING: can't store user info, swift id (" << id
           << ") already mapped to another user (" << info.user_id << ")" << dendl;
         return -EEXIST;
       }
     }
 
     /* check if access keys already exist */
-    for (auto iter = info.access_keys.begin(); iter != info.access_keys.end(); ++iter) {
-      if (old_info && old_info->access_keys.count(iter->first) != 0)
+    for (const auto& [id, key] : info.access_keys) {
+      if (!key.active) // new key not active
+        continue;
+      if (s3_key_active(old_info, id)) // old key already active
         continue;
-      auto& k = iter->second;
       RGWUserInfo inf;
-      int r = svc.user->get_user_info_by_access_key(ctx, k.id, &inf, nullptr, nullptr, y, dpp);
+      int r = svc.user->get_user_info_by_access_key(ctx, id, &inf, nullptr, nullptr, y, dpp);
       if (r >= 0 && inf.user_id != info.user_id &&
           (!old_info || inf.user_id != old_info->user_id)) {
         ldpp_dout(dpp, 0) << "WARNING: can't store user info, access key already mapped to another user" << dendl;
@@ -266,23 +282,25 @@ public:
     }
 
     const bool renamed = old_info && old_info->user_id != info.user_id;
-    for (auto iter = info.access_keys.begin(); iter != info.access_keys.end(); ++iter) {
-      auto& k = iter->second;
-      if (old_info && old_info->access_keys.count(iter->first) != 0 && !renamed)
+    for (const auto& [id, key] : info.access_keys) {
+      if (!key.active)
+        continue;
+      if (s3_key_active(old_info, id) && !renamed)
         continue;
 
-      ret = rgw_put_system_obj(dpp, svc.sysobj, svc.zone->get_zone_params().user_keys_pool, k.id,
+      ret = rgw_put_system_obj(dpp, svc.sysobj, svc.zone->get_zone_params().user_keys_pool, id,
                                link_bl, exclusive, NULL, real_time(), y);
       if (ret < 0)
         return ret;
     }
 
-    for (auto siter = info.swift_keys.begin(); siter != info.swift_keys.end(); ++siter) {
-      auto& k = siter->second;
-      if (old_info && old_info->swift_keys.count(siter->first) != 0 && !renamed)
+    for (const auto& [id, key] : info.swift_keys) {
+      if (!key.active)
+        continue;
+      if (swift_key_active(old_info, id) && !renamed)
         continue;
 
-      ret = rgw_put_system_obj(dpp, svc.sysobj, svc.zone->get_zone_params().user_swift_pool, k.id,
+      ret = rgw_put_system_obj(dpp, svc.sysobj, svc.zone->get_zone_params().user_swift_pool, id,
                                link_bl, exclusive, NULL, real_time(), y);
       if (ret < 0)
         return ret;
@@ -323,23 +341,21 @@ public:
       }
     }
 
-    for ([[maybe_unused]] const auto& [name, access_key] : old_info.access_keys) {
-      if (!new_info.access_keys.count(access_key.id)) {
-        ret = svc.user->remove_key_index(dpp, access_key, y);
+    for (const auto& [id, key] : old_info.access_keys) {
+      if (key.active && !s3_key_active(&new_info, id)) {
+        ret = svc.user->remove_key_index(dpp, key, y);
         if (ret < 0 && ret != -ENOENT) {
-          set_err_msg("ERROR: could not remove index for key " + access_key.id);
+          set_err_msg("ERROR: could not remove index for key " + id);
           return ret;
         }
       }
     }
 
-    for (auto old_iter = old_info.swift_keys.begin(); old_iter != old_info.swift_keys.end(); ++old_iter) {
-      const auto& swift_key = old_iter->second;
-      auto new_iter = new_info.swift_keys.find(swift_key.id);
-      if (new_iter == new_info.swift_keys.end()) {
-        ret = svc.user->remove_swift_name_index(dpp, swift_key.id, y);
+    for (const auto& [id, key] : old_info.swift_keys) {
+      if (key.active && !swift_key_active(&new_info, id)) {
+        ret = svc.user->remove_swift_name_index(dpp, id, y);
         if (ret < 0 && ret != -ENOENT) {
-          set_err_msg("ERROR: could not remove index for swift_name " + swift_key.id);
+          set_err_msg("ERROR: could not remove index for swift_name " + id);
           return ret;
         }
       }
@@ -432,24 +448,27 @@ int RGWSI_User_RADOS::remove_user_info(RGWSI_MetaBackend::Context *ctx,
 {
   int ret;
 
-  auto kiter = info.access_keys.begin();
-  for (; kiter != info.access_keys.end(); ++kiter) {
-    ldpp_dout(dpp, 10) << "removing key index: " << kiter->first << dendl;
-    ret = remove_key_index(dpp, kiter->second, y);
+  for (const auto& [id, key] : info.access_keys) {
+    if (!key.active) {
+      continue;
+    }
+    ldpp_dout(dpp, 10) << "removing key index: " << id << dendl;
+    ret = remove_key_index(dpp, key, y);
     if (ret < 0 && ret != -ENOENT) {
-      ldpp_dout(dpp, 0) << "ERROR: could not remove " << kiter->first << " (access key object), should be fixed (err=" << ret << ")" << dendl;
+      ldpp_dout(dpp, 0) << "ERROR: could not remove " << id << " (access key object), should be fixed (err=" << ret << ")" << dendl;
       return ret;
     }
   }
 
-  auto siter = info.swift_keys.begin();
-  for (; siter != info.swift_keys.end(); ++siter) {
-    auto& k = siter->second;
-    ldpp_dout(dpp, 10) << "removing swift subuser index: " << k.id << dendl;
+  for (const auto& [id, key] : info.swift_keys) {
+    if (!key.active) {
+      continue;
+    }
+    ldpp_dout(dpp, 10) << "removing swift subuser index: " << id << dendl;
     /* check if swift mapping exists */
-    ret = remove_swift_name_index(dpp, k.id, y);
+    ret = remove_swift_name_index(dpp, id, y);
     if (ret < 0 && ret != -ENOENT) {
-      ldpp_dout(dpp, 0) << "ERROR: could not remove " << k.id << " (swift name object), should be fixed (err=" << ret << ")" << dendl;
+      ldpp_dout(dpp, 0) << "ERROR: could not remove " << id << " (swift name object), should be fixed (err=" << ret << ")" << dendl;
       return ret;
     }
   }

diff --git a/src/test/cli/radosgw-admin/help.t b/src/test/cli/radosgw-admin/help.t
index 8f6fc36190ccb0af54c709269d3275e11208f214..cedc2e4bbf0dde2795b3ade50cc1db7d75aef755 100644 (file)
--- a/src/test/cli/radosgw-admin/help.t
+++ b/src/test/cli/radosgw-admin/help.t
@@ -204,6 +204,7 @@
      --gen-access-key                  generate random access key (for S3)
      --gen-secret                      generate random secret key
      --key-type=<type>                 key type, options are: swift, s3
+     --key-active=<bool>               activate or deactivate a key
      --temp-url-key[-2]=<key>          temp url key
      --access=<access>                 Set access permissions for sub-user, should be one
                                        of read, write, readwrite, full