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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
