You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "acelyc111 (via GitHub)" <gi...@apache.org> on 2023/03/21 07:35:23 UTC
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1402: feat(Ranger): Use Apache Ranger for meta access controller
acelyc111 commented on code in PR #1402:
URL: https://github.com/apache/incubator-pegasus/pull/1402#discussion_r1142959186
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -187,6 +202,70 @@ ranger_resource_policy_manager::ranger_resource_policy_manager(
_ac_type_of_database_rpcs);
}
+void ranger_resource_policy_manager::start()
+{
+ tasking::enqueue_timer(LPC_USE_RANGER_ACCESS_CONTROL,
+ &_tracker,
+ [this]() { this->update_policies_from_ranger_service(); },
+ std::chrono::seconds(FLAGS_update_ranger_policy_interval_sec),
+ 0,
+ std::chrono::milliseconds(1));
+}
+
+bool ranger_resource_policy_manager::allowed(const int rpc_code,
Review Comment:
Add some unit tests.
##########
src/runtime/security/meta_access_controller.cpp:
##########
@@ -61,15 +100,45 @@ bool meta_access_controller::allowed(message_ex *msg)
return false;
}
-void meta_access_controller::register_allowed_list(const std::string &rpc_code)
+bool meta_access_controller::allowed(message_ex *msg, const std::string &app_name)
{
- auto code = task_code::try_get(rpc_code, TASK_CODE_INVALID);
- CHECK_NE_MSG(code,
- TASK_CODE_INVALID,
- "invalid task code({}) in rpc_code_white_list of security section",
- rpc_code);
+ const auto rpc_code = msg->rpc_code().code();
+ const auto user_name = msg->io_session->get_client_username();
Review Comment:
```suggestion
const auto &user_name = msg->io_session->get_client_username();
```
##########
src/meta/meta_service.h:
##########
@@ -444,9 +481,27 @@ bool meta_service::check_status_with_msg(message_ex *req, TRespType &response_st
reply(req, response_struct);
return false;
}
-
+ if (!_access_controller->allowed(req, app_name)) {
+ response_struct.err = ERR_ACL_DENY;
+ LOG_INFO("not authorized {} to operate on app({}) for user({})",
Review Comment:
Same, log level.
##########
src/meta/server_state.cpp:
##########
@@ -1438,14 +1439,17 @@ void server_state::recall_app(dsn::message_ex *msg)
}
void server_state::list_apps(const configuration_list_apps_request &request,
- configuration_list_apps_response &response)
+ configuration_list_apps_response &response,
+ dsn::message_ex *msg)
Review Comment:
how about mark this function as `const`? then use const in the loop in line 1447.
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -187,6 +202,70 @@ ranger_resource_policy_manager::ranger_resource_policy_manager(
_ac_type_of_database_rpcs);
}
+void ranger_resource_policy_manager::start()
+{
+ tasking::enqueue_timer(LPC_USE_RANGER_ACCESS_CONTROL,
+ &_tracker,
+ [this]() { this->update_policies_from_ranger_service(); },
+ std::chrono::seconds(FLAGS_update_ranger_policy_interval_sec),
+ 0,
+ std::chrono::milliseconds(1));
+}
+
+bool ranger_resource_policy_manager::allowed(const int rpc_code,
+ const std::string &user_name,
+ const std::string &database_name)
+{
+ do {
+ const auto &ac_type = _ac_type_of_global_rpcs.find(rpc_code);
+ // It's not a GLOBAL rpc code.
+ if (ac_type == _ac_type_of_global_rpcs.end()) {
+ break;
+ }
+
+ // Check if it is allowed by any GLOBAL policy.
+ utils::auto_read_lock l(_global_policies_lock);
+ for (const auto &policy : _global_policies_cache) {
+ if (policy.policies.allowed(ac_type->second, user_name)) {
+ return true;
+ }
+ }
+
+ // It's not allowed to access except list_app.
+ // list_app rpc code is in both GLOBAL and DATABASE policies, check the DATABASE policies
+ // later.
+ if (rpc_code != RPC_CM_LIST_APPS.code()) {
+ return false;
+ }
+ } while (false);
+
+ do {
+ const auto &ac_type = _ac_type_of_database_rpcs.find(rpc_code);
+ // It's not a DATABASE rpc code.
+ if (ac_type == _ac_type_of_database_rpcs.end()) {
+ break;
+ }
+
+ // Check if it is allowed by any DATABASE policy.
+ utils::auto_read_lock l(_database_policies_lock);
+ for (const auto &policy : _database_policies_cache) {
+ if (!policy.policies.allowed(ac_type->second, user_name)) {
+ continue;
+ }
+ // Legacy tables may don't contain database section.
+ if (database_name.empty() && policy.database_names.count("*") != 0) {
Review Comment:
Will the legacy tables be skipped in the logic above and will never reach here?
##########
src/runtime/security/meta_access_controller.cpp:
##########
@@ -34,21 +35,59 @@ DSN_DEFINE_string(security,
"",
"allowed list of rpc codes for meta_access_controller");
-meta_access_controller::meta_access_controller()
+DSN_DECLARE_bool(enable_acl);
+DSN_DECLARE_bool(enable_ranger_acl);
+
+meta_access_controller::meta_access_controller(
+ const std::shared_ptr<ranger::ranger_resource_policy_manager> &policy_manager)
{
// MetaServer serves the allow-list RPC from all users. RPCs unincluded are accessible to only
// superusers.
if (utils::is_empty(FLAGS_meta_acl_rpc_allow_list)) {
- register_allowed_list("RPC_CM_LIST_APPS");
- register_allowed_list("RPC_CM_LIST_NODES");
- register_allowed_list("RPC_CM_CLUSTER_INFO");
- register_allowed_list("RPC_CM_QUERY_PARTITION_CONFIG_BY_INDEX");
+ register_allowed_rpc_code_list({"RPC_CM_LIST_APPS",
+ "RPC_CM_LIST_NODES",
+ "RPC_CM_CLUSTER_INFO",
+ "RPC_CM_QUERY_PARTITION_CONFIG_BY_INDEX"});
} else {
std::vector<std::string> rpc_code_white_list;
utils::split_args(FLAGS_meta_acl_rpc_allow_list, rpc_code_white_list, ',');
- for (const auto &rpc_code : rpc_code_white_list) {
- register_allowed_list(rpc_code);
- }
+ register_allowed_rpc_code_list(rpc_code_white_list);
+ }
+ _ranger_resource_policy_manager = policy_manager;
Review Comment:
How about use initialize-list to init `_ranger_resource_policy_manager`?
##########
src/meta/meta_service.h:
##########
@@ -414,20 +435,36 @@ bool meta_service::check_status(TRpcHolder rpc, rpc_address *forward_address)
LOG_INFO("reject request with {}", rpc.response().err);
return false;
}
-
return true;
}
-template <typename TRespType>
-bool meta_service::check_status_with_msg(message_ex *req, TRespType &response_struct)
+// when the Ranger ACL is enabled, only the leader meta_server will pull Ranger policy, so if it is
+// not the leader, _access_controller may be a null pointer, or a new leader is elected, and the
+// above policy information may be out of date.
+template <typename TRpcHolder>
+bool meta_service::check_status_and_authz(TRpcHolder rpc,
+ rpc_address *forward_address,
+ const std::string &app_name)
{
- if (!_access_controller->allowed(req)) {
- LOG_INFO("reject request with ERR_ACL_DENY");
- response_struct.err = ERR_ACL_DENY;
- reply(req, response_struct);
+ if (!check_leader_status(rpc, forward_address)) {
+ return false;
+ }
+ if (!_access_controller->allowed(rpc.dsn_request(), app_name)) {
+ rpc.response().err = ERR_ACL_DENY;
+ LOG_INFO("not authorized {} to operate on app({}) for user({})",
+ rpc.dsn_request()->rpc_code(),
+ app_name,
+ rpc.dsn_request()->io_session->get_client_username());
return false;
}
+ return true;
+}
+template <typename TRespType>
+bool meta_service::check_status_and_authz_with_reply(message_ex *req,
+ TRespType &response_struct,
+ const std::string &app_name)
+{
int result = check_leader(req, nullptr);
Review Comment:
Could you please to refactor `check_leader` in another patch firstly to make it return a enum class type? return -1, 0 and 1 is easy to make mistake.
##########
src/meta/meta_service.h:
##########
@@ -414,20 +435,36 @@ bool meta_service::check_status(TRpcHolder rpc, rpc_address *forward_address)
LOG_INFO("reject request with {}", rpc.response().err);
return false;
}
-
return true;
}
-template <typename TRespType>
-bool meta_service::check_status_with_msg(message_ex *req, TRespType &response_struct)
+// when the Ranger ACL is enabled, only the leader meta_server will pull Ranger policy, so if it is
+// not the leader, _access_controller may be a null pointer, or a new leader is elected, and the
+// above policy information may be out of date.
+template <typename TRpcHolder>
+bool meta_service::check_status_and_authz(TRpcHolder rpc,
+ rpc_address *forward_address,
+ const std::string &app_name)
{
- if (!_access_controller->allowed(req)) {
- LOG_INFO("reject request with ERR_ACL_DENY");
- response_struct.err = ERR_ACL_DENY;
- reply(req, response_struct);
+ if (!check_leader_status(rpc, forward_address)) {
+ return false;
+ }
+ if (!_access_controller->allowed(rpc.dsn_request(), app_name)) {
+ rpc.response().err = ERR_ACL_DENY;
+ LOG_INFO("not authorized {} to operate on app({}) for user({})",
Review Comment:
Make this log a DEBUG level, otherwise tens of thousands logs will be printed in log file if a user without authz try to access some resouses frequently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org