Skip to content
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.

Sign up for GitHub

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

fix: fix user ERR NOAUTH Authentication required #2939

Merged
merged 5 commits into from
Nov 22, 2024
Merged

fix: fix user ERR NOAUTH Authentication required #2939

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35b073b
fix authrequired bug
buzhimingyonghu Oct 31, 2024
0054e94
init pika.conf
buzhimingyonghu Oct 31, 2024
62754fa
fix InitUser
buzhimingyonghu Oct 31, 2024
e643c87
fix InitUser
buzhimingyonghu Nov 7, 2024
19c1c8b
fix no userblacklist
buzhimingyonghu Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conf/pika.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,4 +691,4 @@ dont-compact-sst-created-in-seconds : 20
# For OBD_Compact
# According to the number of sst files in rocksdb,
# compact every `compact-every-num-of-files` file.
best-delete-min-ratio : 10
best-delete-min-ratio : 10
7 changes: 3 additions & 4 deletions include/acl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class AclSelector {
friend User;

public:
explicit AclSelector() : AclSelector(0){};
explicit AclSelector() : AclSelector(0) {};
explicit AclSelector(uint32_t flag);
explicit AclSelector(const AclSelector& selector);
~AclSelector() = default;
Expand All @@ -138,7 +138,7 @@ class AclSelector {
inline bool HasFlags(uint32_t flag) const { return flags_ & flag; };
inline void AddFlags(uint32_t flag) { flags_ |= flag; };
inline void DecFlags(uint32_t flag) { flags_ &= ~flag; };
bool EqualChannel(const std::vector<std::string> &allChannel);
bool EqualChannel(const std::vector<std::string>& allChannel);

private:
pstd::Status SetSelector(const std::string& op);
Expand Down Expand Up @@ -224,8 +224,7 @@ class User {
~User() = default;

std::string Name() const;

// inline uint32_t Flags() const { return flags_; };
// inline uint32_t Flags() const { return flags_; };
inline bool HasFlags(uint32_t flag) const { return flags_ & flag; };
inline void AddFlags(uint32_t flag) { flags_ |= flag; };
inline void DecFlags(uint32_t flag) { flags_ &= ~flag; };
Expand Down
23 changes: 7 additions & 16 deletions include/pika_client_conn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,15 @@ struct TimeStat {
before_queue_ts_ = 0;
}

uint64_t start_ts() const {
return enqueue_ts_;
}
uint64_t start_ts() const { return enqueue_ts_; }

uint64_t total_time() const {
return process_done_ts_ > enqueue_ts_ ? process_done_ts_ - enqueue_ts_ : 0;
}
uint64_t total_time() const { return process_done_ts_ > enqueue_ts_ ? process_done_ts_ - enqueue_ts_ : 0; }

uint64_t queue_time() const {
return dequeue_ts_ > enqueue_ts_ ? dequeue_ts_ - enqueue_ts_ : 0;
}
uint64_t queue_time() const { return dequeue_ts_ > enqueue_ts_ ? dequeue_ts_ - enqueue_ts_ : 0; }

uint64_t process_time() const {
return process_done_ts_ > dequeue_ts_ ? process_done_ts_ - dequeue_ts_ : 0;
}
uint64_t process_time() const { return process_done_ts_ > dequeue_ts_ ? process_done_ts_ - dequeue_ts_ : 0; }

uint64_t before_queue_time() const {
return process_done_ts_ > dequeue_ts_ ? before_queue_ts_ - enqueue_ts_ : 0;
}
uint64_t before_queue_time() const { return process_done_ts_ > dequeue_ts_ ? before_queue_ts_ - enqueue_ts_ : 0; }

uint64_t enqueue_ts_;
uint64_t dequeue_ts_;
Expand Down Expand Up @@ -94,7 +84,7 @@ class PikaClientConn : public net::RedisConn {
void UnAuth(const std::shared_ptr<User>& user);

bool IsAuthed() const;

void InitUser();
bool AuthRequired() const;

std::string UserName() const;
Expand Down Expand Up @@ -123,6 +113,7 @@ class PikaClientConn : public net::RedisConn {
std::vector<std::shared_ptr<std::string>> resp_array;

std::shared_ptr<TimeStat> time_stat_;

private:
net::ServerThread* const server_thread_;
std::string current_db_;
Expand Down
17 changes: 7 additions & 10 deletions src/acl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,31 +489,28 @@ void Acl::InitLimitUser(const std::string& bl, bool limit_exist) {
auto u = GetUser(DefaultLimitUser);
if (limit_exist) {
if (!bl.empty()) {
for(auto& cmd : blacklist) {
for (auto& cmd : blacklist) {
cmd = pstd::StringTrim(cmd, " ");
u->SetUser("-" + cmd);
}
u->SetUser("on");
Comment on lines +496 to 500
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor duplicated blacklist processing logic.

The blacklist processing logic is duplicated between the two code blocks. Consider extracting this into a helper method to improve maintainability and reduce code duplication.

Apply this diff to extract the common logic:

+void ProcessBlacklist(std::shared_ptr<User>& user, const std::vector<std::string>& blacklist) {
+  for (auto& cmd : blacklist) {
+    cmd = pstd::StringTrim(cmd, " ");
+    user->SetUser("-" + cmd);
+  }
+}

 void Acl::InitLimitUser(const std::string& bl, bool limit_exist) {
   auto pass = g_pika_conf->userpass();
   std::vector<std::string> blacklist;
   pstd::StringSplit(bl, ',', blacklist);
   std::unique_lock wl(mutex_);
   auto u = GetUser(DefaultLimitUser);
   if (limit_exist) {
     if (!bl.empty()) {
-      for (auto& cmd : blacklist) {
-        cmd = pstd::StringTrim(cmd, " ");
-        u->SetUser("-" + cmd);
-      }
+      ProcessBlacklist(u, blacklist);
       u->SetUser("on");
     }
     if (!pass.empty()) {
       u->SetUser(">" + pass);
     }
   } else {
     if (pass.empty()) {
       u->SetUser("nopass");
     } else {
       u->SetUser(">" + pass);
     }
     u->SetUser("on");
     u->SetUser("+@all");
     u->SetUser("~*");
     u->SetUser("&*");
-    for (auto& cmd : blacklist) {
-      cmd = pstd::StringTrim(cmd, " ");
-      u->SetUser("-" + cmd);
-    }
+    ProcessBlacklist(u, blacklist);
   }
 }

Also applies to: 512-515

}
if (!pass.empty()) {
u->SetUser(">"+pass);
}
} else {
if (pass.empty()) {
u->SetUser("nopass");
} else {
u->SetUser(">"+pass);
}
u->SetUser("on");
u->SetUser("+@all");
u->SetUser("~*");
u->SetUser("&*");

for(auto& cmd : blacklist) {
for (auto& cmd : blacklist) {
cmd = pstd::StringTrim(cmd, " ");
u->SetUser("-" + cmd);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring duplicated blacklist processing logic.

The blacklist processing logic is duplicated between the two loops. Consider extracting this into a helper method.

+void ProcessBlacklist(const std::vector<std::string>& blacklist, std::shared_ptr<User>& user) {
+  for (auto& cmd : blacklist) {
+    cmd = pstd::StringTrim(cmd, " ");
+    user->SetUser("-" + cmd);
+  }
+}

 void Acl::InitLimitUser(const std::string& bl, bool limit_exist) {
   auto pass = g_pika_conf->userpass();
   std::vector<std::string> blacklist;
   pstd::StringSplit(bl, ',', blacklist);
   std::unique_lock wl(mutex_);
   auto u = GetUser(DefaultLimitUser);
   if (limit_exist) {
     if (!bl.empty()) {
-      for (auto& cmd : blacklist) {
-        cmd = pstd::StringTrim(cmd, " ");
-        u->SetUser("-" + cmd);
-      }
+      ProcessBlacklist(blacklist, u);
       u->SetUser("on");
     }
   } else {
     u->SetUser("on");
     u->SetUser("+@all");
     u->SetUser("~*");
     u->SetUser("&*");
-    for (auto& cmd : blacklist) {
-      cmd = pstd::StringTrim(cmd, " ");
-      u->SetUser("-" + cmd);
-    }
+    ProcessBlacklist(blacklist, u);
   }

Committable suggestion was skipped due to low confidence.

if (pass.empty()) {
u->SetUser("nopass");
} else {
u->SetUser(">" + pass);
}
}
// bool Acl::CheckUserCanExec(const std::shared_ptr<Cmd>& cmd, const PikaCmdArgsType& argv) { cmd->name(); }

Expand Down
46 changes: 25 additions & 21 deletions src/pika_client_conn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ PikaClientConn::PikaClientConn(int fd, const std::string& ip_port, net::Thread*
: RedisConn(fd, ip_port, thread, mpx, handle_type, max_conn_rbuf_size),
server_thread_(reinterpret_cast<net::ServerThread*>(thread)),
current_db_(g_pika_conf->default_db()) {
// client init, set client user is default, and authenticated = false
UnAuth(g_pika_server->Acl()->GetUserLock(Acl::DefaultUser));
InitUser();
time_stat_.reset(new TimeStat());
}

Expand Down Expand Up @@ -259,7 +258,7 @@ void PikaClientConn::ProcessMonitor(const PikaCmdArgsType& argv) {
}

bool PikaClientConn::IsInterceptedByRTC(std::string& opt) {
//currently we only Intercept: Get, HGet
// currently we only Intercept: Get, HGet
if (opt == kCmdNameGet && g_pika_conf->GetCacheString()) {
return true;
}
Expand Down Expand Up @@ -289,11 +288,9 @@ void PikaClientConn::ProcessRedisCmds(const std::vector<net::RedisCmdArgsType>&
bool is_slow_cmd = g_pika_conf->is_slow_cmd(opt);
bool is_admin_cmd = g_pika_conf->is_admin_cmd(opt);

//we don't intercept pipeline batch (argvs.size() > 1)
if (g_pika_conf->rtc_cache_read_enabled() &&
argvs.size() == 1 && IsInterceptedByRTC(opt) &&
PIKA_CACHE_NONE != g_pika_conf->cache_mode() &&
!IsInTxn()) {
// we don't intercept pipeline batch (argvs.size() > 1)
if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) &&
PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) {
Comment on lines +291 to +293
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider splitting complex condition for better readability.

The multiple conditions could be split into separate checks with meaningful variable names for better maintainability.

-    if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) &&
-        PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) {
+    bool is_cache_enabled = g_pika_conf->rtc_cache_read_enabled();
+    bool is_single_command = argvs.size() == 1;
+    bool is_cacheable_command = IsInterceptedByRTC(opt);
+    bool has_valid_cache_mode = PIKA_CACHE_NONE != g_pika_conf->cache_mode();
+    bool not_in_transaction = !IsInTxn();
+    
+    if (is_cache_enabled && is_single_command && is_cacheable_command && 
+        has_valid_cache_mode && not_in_transaction) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// we don't intercept pipeline batch (argvs.size() > 1)
if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) &&
PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) {
// we don't intercept pipeline batch (argvs.size() > 1)
bool is_cache_enabled = g_pika_conf->rtc_cache_read_enabled();
bool is_single_command = argvs.size() == 1;
bool is_cacheable_command = IsInterceptedByRTC(opt);
bool has_valid_cache_mode = PIKA_CACHE_NONE != g_pika_conf->cache_mode();
bool not_in_transaction = !IsInTxn();
if (is_cache_enabled && is_single_command && is_cacheable_command &&
has_valid_cache_mode && not_in_transaction) {

// read in cache
if (ReadCmdInCache(argvs[0], opt)) {
delete arg;
Expand Down Expand Up @@ -358,21 +355,17 @@ bool PikaClientConn::ReadCmdInCache(const net::RedisCmdArgsType& argv, const std
resp_num--;
return false;
}
//acl check
// acl check
int8_t subCmdIndex = -1;
std::string errKey;
auto checkRes = user_->CheckUserPermission(c_ptr, argv, subCmdIndex, &errKey);
std::string object;
if (checkRes == AclDeniedCmd::CMD ||
checkRes == AclDeniedCmd::KEY ||
checkRes == AclDeniedCmd::CHANNEL ||
checkRes == AclDeniedCmd::NO_SUB_CMD ||
checkRes == AclDeniedCmd::NO_AUTH
) {
//acl check failed
if (checkRes == AclDeniedCmd::CMD || checkRes == AclDeniedCmd::KEY || checkRes == AclDeniedCmd::CHANNEL ||
checkRes == AclDeniedCmd::NO_SUB_CMD || checkRes == AclDeniedCmd::NO_AUTH) {
// acl check failed
return false;
}
//only read command(Get, HGet) will reach here, no need of record lock
// only read command(Get, HGet) will reach here, no need of record lock
bool read_status = c_ptr->DoReadCommandInCache();
auto cmdstat_map = g_pika_cmd_table_manager->GetCommandStatMap();
resp_num--;
Expand Down Expand Up @@ -547,13 +540,24 @@ void PikaClientConn::UnAuth(const std::shared_ptr<User>& user) {
}

bool PikaClientConn::IsAuthed() const { return authenticated_; }

void PikaClientConn::InitUser() {
if (g_pika_conf->requirepass().empty()) {
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser);
}
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) &&
!user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED));
}
Copy link

@coderabbitai coderabbitai bot Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix logical error in InitUser implementation.

There's a critical bug in the control flow. The requirepass check is ineffective because user_ is always set to DefaultLimitUser regardless of the condition.

Apply this fix to correct the logic:

void PikaClientConn::InitUser() {
  if (g_pika_conf->requirepass().empty()) {
    user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser);
+  } else {
+    user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
  }
-  user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
  authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) &&
                  !user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED));
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void PikaClientConn::InitUser() {
if (g_pika_conf->requirepass().empty()) {
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser);
}
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) &&
!user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED));
}
void PikaClientConn::InitUser() {
if (g_pika_conf->requirepass().empty()) {
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser);
} else {
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
}
authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) &&
!user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buzhimingyonghu 谢谢您的修正！🐰🎉

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

bool PikaClientConn::AuthRequired() const {
// If the user does not have a password, and the user is valid, then the user does not need authentication
// Otherwise, you need to determine whether go has been authenticated
return (!user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) ||
user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED))) &&
!IsAuthed();
if (user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED))) {
return true;
}
if (user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS))) {
return false;
}
return !IsAuthed();
}
std::string PikaClientConn::UserName() const { return user_->Name(); }

Expand Down
Loading