From: Casey Bodley Date: Wed, 20 Dec 2023 16:25:03 +0000 (-0500) Subject: rgw/user: add 'active' flag to RGWAccessKey X-Git-Tag: v20.0.0~2585^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=463f463d50943c6fe17c39e41f64b1df27944ff6;p=ceph.git 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 --- diff --git a/doc/radosgw/adminops.rst b/doc/radosgw/adminops.rst index f1faac6b1c2f..a7bdec431f51 100644 --- a/doc/radosgw/adminops.rst +++ b/doc/radosgw/adminops.rst @@ -1163,6 +1163,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 200f1c03d2e1..bd87e4e27e02 100644 --- a/src/rgw/driver/rados/rgw_rest_user.cc +++ b/src/rgw/driver/rados/rgw_rest_user.cc @@ -651,7 +651,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); @@ -662,12 +664,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 b5569e481c53..c2ae4d318088 100644 --- 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(); @@ -600,11 +602,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 key_pair; - map::iterator kiter; - switch (key_type) { case KEY_TYPE_S3: id = op_state.get_access_key(); @@ -630,8 +627,8 @@ int RGWAccessKeyPool::modify_key(RGWUserAdminOpState& op_state, std::string *err return -ERR_INVALID_ACCESS_KEY; } - key_pair.first = id; - + RGWAccessKey modify_key; + map::iterator kiter; if (key_type == KEY_TYPE_SWIFT) { modify_key.id = id; modify_key.subuser = op_state.get_subuser(); @@ -649,16 +646,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 a0cd7ed84fee..50ad28af24cf 100644 --- a/src/rgw/driver/rados/rgw_user.h +++ b/src/rgw/driver/rados/rgw_user.h @@ -135,6 +135,7 @@ struct RGWUserAdminOpState { std::map op_access_keys; int32_t key_type{-1}; bool access_key_exist = false; + std::optional access_key_active; std::set 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 b9866e9b289a..3f9f1715aba4 100644 --- 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 8265852973f9..2977933b2e1c 100644 --- 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= key type, options are: swift, s3\n"; + cout << " --key-active= activate or deactivate a key\n"; cout << " --temp-url-key[-2]= temp url key\n"; cout << " --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 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 f75ed2aca016..b1fa2fe64985 100644 --- a/src/rgw/rgw_common.cc +++ b/src/rgw/rgw_common.cc @@ -2912,6 +2912,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 @@ -2932,6 +2933,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) { @@ -2945,6 +2947,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) { @@ -2961,6 +2964,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 dd48bc4c01ef..0d01c96d481f 100644 --- 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 8f6fc36190cc..cedc2e4bbf0d 100644 --- 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= key type, options are: swift, s3 + --key-active= activate or deactivate a key --temp-url-key[-2]= temp url key --access= Set access permissions for sub-user, should be one of read, write, readwrite, full