refactor(security): add join point to start negotiation #626
Conversation
| rpc_session::on_rpc_session_connected.put_back(negotiation_service::on_rpc_connected, | ||
| "security"); | ||
| rpc_session::on_rpc_session_disconnected.put_back(negotiation_service::on_rpc_disconnected, | ||
| "security"); |
| if (negotiation_succeed) { | ||
| return negotiation_succeed; |
There was a problem hiding this comment.
Why these sentense out of lock?
There was a problem hiding this comment.
this is kind of double check. the first one didn't lock the _lock.
Because negotiation_succeed only transfered from false to true. So if it is true now, it will not change in the later. but if it is false now, maybe it will change soon. So we should use lock to protect it.
Why I use double check here is to be efficient
There was a problem hiding this comment.
OK, better to add some comments here
There was a problem hiding this comment.
Is on_recv_message in the same lock?
There was a problem hiding this comment.
on_recv_message will not lock any locks. It only calls is_negotiation_succeed to judge whether the negotiation is succeed. if negotiation is not succeed, the received message will be ignored.
|
|
||
| static zrwlock_nr _lock; // [ | ||
| static negotiation_map _negotiations; | ||
| //] |
There was a problem hiding this comment.
//[ and //] indicates that the _lock is used for protecting the members locates between [ and ]
This reverts commit 8128c53.
levy5307
changed the title
| bool rpc_session::is_auth_success(message_ex *msg) | ||
| { | ||
| if (security::FLAGS_enable_auth && !_negotiation->negotiation_succeed()) { | ||
| if (security::FLAGS_enable_auth && !is_negotiation_succeed()) { |
There was a problem hiding this comment.
From my point, if you want to decouple the negotiation entirely with rpc_session, you should remove this function and modify rpc_session::on_recv_message, where a session rejects the incoming requests when the negotiation is unfinished.
Nevertheless, there's no joint point for recv_message as far as I see. (There's one joint point at task_spec::on_rpc_call, but not suitable since it targets at customization of tasks, rather than general rpc control).
Maybe you can add such joint point later.
There was a problem hiding this comment.
Yes, I will do it in the next pull request. I split the refactor of network into 3 pull requests.
Previously, the network module is coupled from security, because we should start negotiation when rpc session is connected. To make network decoupled from security, in this pr, I use joint point for starting negotiation. And if you enable authentiation, a joint point, which is implemented in security module, is put_back to
on_rpc_session_connected.And to make network module simply, I removed the
SS_NEGOTIATING. Because I think the authentication code should not invade network modules