-
Notifications
You must be signed in to change notification settings - Fork 320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add nameserver auth #3835
Conversation
…nd all internal services
Linux Test Report 59 files + 59 249 suites +249 1h 47m 47s ⏱️ + 1h 47m 47s Results for commit 5debf64. ± Comparison against base commit 6b52ee5. ♻️ This comment has been updated with latest results. |
SDK Test Report102 files ±0 102 suites ±0 2m 15s ⏱️ -2s Results for commit 5debf64. ± Comparison against base commit 6b52ee5. This pull request removes 48 and adds 27 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
integration test will fail because old sdk is used |
bool ok = name_server_client.SendRequest(&::openmldb::nameserver::NameServer_Stub::ShowTable, &request, &response, | ||
FLAGS_request_timeout_ms, 1); | ||
ASSERT_TRUE(ok); | ||
ASSERT_EQ(response.table_info_size(), size); | ||
} | ||
|
||
TEST_F(SqlClusterTest, RecoverProcedure) { | ||
TEST_F(SqlClusterTest, DropProcedureBeforeDropTable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rename this test file? drop_procedure_before_drop_table_test.cc
is not a good name. You'd better make the file name be the same with the test class, not the test method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to separate the RecoverProcedure and DropProcedureBeforeDropTable for the tests to pass. There's probably some bugs in the tests that we can fix later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no docs about privileges, add in auth.md after #3879
|
||
void UserAccessManager::StartSyncTask() { | ||
sync_task_running_ = true; | ||
sync_task_thread_ = std::thread([this] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query table every 0.1 seconds ? Not good practice if my understand is correct, instead, you should lazy fetch the table with help of zk.
Register an event listener to zk > on table values update, notify zk > zk will send data updated event to every subscriber .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is just temporary solution to get latest user credentials. In the future we will use a distributed transaction to update user table cache on all nodes.
merge after 0.9.0 released, cherry-pick is heavy |
What kind of change does this PR introduce?
This PR introduces a significant security enhancement by implementing RPC authenticator functionality to ensure all connections are authenticated server-side. This is a feature change aimed at improving the security of server-client communications.
What is the current behavior?
Currently, there is no authentication performed on the server side for incoming connections. Authentication is solely handled within the SDK, leaving a potential security gap for server-client communications.
What is the new behavior (if this is a feature change)?
With this change, all connections to the server will undergo authentication at the beginning of the rpc connection. This server-side authentication is facilitated by the newly introduced Authenticator class, which supports two types of authentication: service authentication and user authentication. For the sake of maintaining backward compatibility, the system will initially allow all services carrying the "default" token and users with the username "root". This PR lays down the foundational framework for our enhanced security measures, focusing on authentication at the server side. Future PRs will expand on this by incorporating user verification against tablet data and authenticating service tokens via a Zookeeper service token record. This implementation is a critical step toward tightening security and providing a robust authentication mechanism for our system.