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

feat(acl): add support of multiple passwords #3189

Merged
merged 3 commits into from
Jun 21, 2024
+190 −74
Merged

feat(acl): add support of multiple passwords #3189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d364a73
feat(acl): add support of multiple passwords
kostasrim Jun 18, 2024
3d284e7
fix test
kostasrim Jun 18, 2024
1264468
fix bugs
kostasrim Jun 21, 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
18 changes: 9 additions & 9 deletions src/server/acl/acl_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ void AclFamily::List(CmdArgList args, ConnectionContext* cntx) {

for (const auto& [username, user] : registry) {
std::string buffer = "user ";
const std::string_view pass = user.Password();
const std::string password = pass == "nopass" ? "nopass" : PrettyPrintSha(pass);
const std::string password = PasswordsToString(user.Passwords(), user.HasNopass(), false);

const std::string acl_keys = AclKeysToString(user.Keys());
const std::string maybe_space_com = acl_keys.empty() ? "" : " ";
Expand All @@ -75,7 +74,7 @@ void AclFamily::List(CmdArgList args, ConnectionContext* cntx) {

using namespace std::string_view_literals;

absl::StrAppend(&buffer, username, " ", user.IsActive() ? "on "sv : "off "sv, password, " ",
absl::StrAppend(&buffer, username, " ", user.IsActive() ? "on "sv : "off "sv, password,
acl_keys, maybe_space_com, acl_cat_and_commands);

cntx->SendSimpleString(buffer);
Expand Down Expand Up @@ -196,9 +195,7 @@ std::string AclFamily::RegistryToString() const {
std::string result;
for (auto& [username, user] : registry) {
std::string command = "USER ";
const std::string_view pass = user.Password();
const std::string password =
pass == "nopass" ? "nopass " : absl::StrCat("#", PrettyPrintSha(pass, true), " ");
const std::string password = PasswordsToString(user.Passwords(), user.HasNopass(), true);

const std::string acl_keys = AclKeysToString(user.Keys());
const std::string maybe_space = acl_keys.empty() ? "" : " ";
Expand Down Expand Up @@ -495,7 +492,10 @@ void AclFamily::GetUser(CmdArgList args, ConnectionContext* cntx) {
}
auto& user = registry.find(username)->second;
std::string status = user.IsActive() ? "on" : "off";
auto pass = user.Password();
auto pass = PasswordsToString(user.Passwords(), user.HasNopass(), false);
if (!pass.empty()) {
pass.pop_back();
}
Comment on lines +495 to +498
Copy link
Contributor

Choose a reason for hiding this comment

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

A little bit unusual interface with the leftover space 🙂 But yes, it's only used twice, so not important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Soooo, it's used in 3 places and two of them needed the extra space, so I thought this was the minimal set of changes required 🤣 I promise I will clean this, just like the other two places that almost have identical printing (acl list and when we serialize the registry to write it via acl save)


auto* rb = static_cast<facade::RedisReplyBuilder*>(cntx->reply_builder());
rb->StartArray(8);
Expand All @@ -509,7 +509,7 @@ void AclFamily::GetUser(CmdArgList args, ConnectionContext* cntx) {
}

rb->SendSimpleString("passwords");
if (pass != "nopass") {
if (pass != "nopass" && !pass.empty()) {
rb->SendSimpleString(pass);
} else {
rb->SendEmptyArray();
Expand Down Expand Up @@ -647,7 +647,7 @@ void AclFamily::Init(facade::Listener* main_listener, UserRegistry* registry) {
registry_ = registry;
config_registry.RegisterMutable("requirepass", [this](const absl::CommandLineFlag& flag) {
User::UpdateRequest rqst;
rqst.password = flag.CurrentValue();
rqst.passwords.push_back({flag.CurrentValue()});
registry_->MaybeAddAndUpdate("default", std::move(rqst));
return true;
});
Expand Down
83 changes: 66 additions & 17 deletions src/server/acl/acl_family_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,67 @@ TEST_F(AclFamilyTest, AclSetUser) {
EXPECT_THAT(resp, "OK");
resp = Run({"ACL", "LIST"});
auto vec = resp.GetVec();
EXPECT_THAT(
vec, UnorderedElementsAre("user default on nopass ~* +@all", "user vlad off nopass -@all"));
EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass ~* +@all", "user vlad off -@all"));

resp = Run({"ACL", "SETUSER", "vlad", "+ACL"});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "LIST"});
vec = resp.GetVec();
EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass ~* +@all",
"user vlad off nopass -@all +acl"));
EXPECT_THAT(vec,
UnorderedElementsAre("user default on nopass ~* +@all", "user vlad off -@all +acl"));

resp = Run({"ACL", "SETUSER", "vlad", "on", ">pass", ">temp"});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "LIST"});
vec = resp.GetVec();
EXPECT_THAT(vec.size(), 2);
auto contains_vlad = [](const auto& vec) {
const std::string default_user = "user default on nopass ~* +@all";
const std::string a_permutation = "user vlad on #a6864eb339b0e1f #d74ff0ee8da3b98 -@all +acl";
const std::string b_permutation = "user vlad on #d74ff0ee8da3b98 #a6864eb339b0e1f -@all +acl";
std::string_view other;
if (vec[0] == default_user) {
other = vec[1].GetView();
} else if (vec[1] == default_user) {
other = vec[0].GetView();
} else {
return false;
}

return other == a_permutation || other == b_permutation;
};

EXPECT_THAT(contains_vlad(vec), true);

resp = Run({"AUTH", "vlad", "pass"});
EXPECT_THAT(resp, "OK");

resp = Run({"AUTH", "vlad", "temp"});
EXPECT_THAT(resp, "OK");

resp = Run({"AUTH", "default", R"("")"});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "SETUSER", "vlad", ">another"});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "SETUSER", "vlad", "<another"});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "LIST"});
vec = resp.GetVec();
EXPECT_THAT(vec.size(), 2);
EXPECT_THAT(contains_vlad(vec), true);

resp = Run({"ACL", "SETUSER", "vlad", "resetpass"});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "LIST"});
vec = resp.GetVec();
EXPECT_THAT(vec,
UnorderedElementsAre("user default on nopass ~* +@all", "user vlad on -@all +acl"));
}

TEST_F(AclFamilyTest, AclDelUser) {
Expand Down Expand Up @@ -105,8 +156,8 @@ TEST_F(AclFamilyTest, AclList) {
resp = Run({"ACL", "LIST"});
auto vec = resp.GetVec();
EXPECT_THAT(vec, UnorderedElementsAre("user default on nopass ~* +@all",
"user kostas off d74ff0ee8da3b98 -@all +@admin",
"user adi off d74ff0ee8da3b98 -@all +@fast"));
"user kostas off #d74ff0ee8da3b98 -@all +@admin",
"user adi off #d74ff0ee8da3b98 -@all +@fast"));
}

TEST_F(AclFamilyTest, AclAuth) {
Expand Down Expand Up @@ -154,19 +205,17 @@ TEST_F(AclFamilyTest, TestAllCategories) {
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "LIST"});
EXPECT_THAT(resp.GetVec(),
UnorderedElementsAre("user default on nopass ~* +@all",
absl::StrCat("user kostas off nopass -@all ", "+@",
absl::AsciiStrToLower(cat))));
EXPECT_THAT(resp.GetVec(), UnorderedElementsAre("user default on nopass ~* +@all",
absl::StrCat("user kostas off -@all ", "+@",
absl::AsciiStrToLower(cat))));

resp = Run({"ACL", "SETUSER", "kostas", absl::StrCat("-@", cat)});
EXPECT_THAT(resp, "OK");

resp = Run({"ACL", "LIST"});
EXPECT_THAT(resp.GetVec(),
UnorderedElementsAre("user default on nopass ~* +@all",
absl::StrCat("user kostas off nopass -@all ", "-@",
absl::AsciiStrToLower(cat))));
EXPECT_THAT(resp.GetVec(), UnorderedElementsAre("user default on nopass ~* +@all",
absl::StrCat("user kostas off -@all ", "-@",
absl::AsciiStrToLower(cat))));

resp = Run({"ACL", "DELUSER", "kostas"});
EXPECT_THAT(resp, IntArg(1));
Expand Down Expand Up @@ -206,15 +255,15 @@ TEST_F(AclFamilyTest, TestAllCommands) {
resp = Run({"ACL", "LIST"});
EXPECT_THAT(resp.GetVec(),
UnorderedElementsAre("user default on nopass ~* +@all",
absl::StrCat("user kostas off nopass -@all ", "+",
absl::StrCat("user kostas off -@all ", "+",
absl::AsciiStrToLower(command_name))));

resp = Run({"ACL", "SETUSER", "kostas", absl::StrCat("-", command_name)});

resp = Run({"ACL", "LIST"});
EXPECT_THAT(resp.GetVec(),
UnorderedElementsAre("user default on nopass ~* +@all",
absl::StrCat("user kostas off nopass ", "-@all ", "-",
absl::StrCat("user kostas off ", "-@all ", "-",
absl::AsciiStrToLower(command_name))));

resp = Run({"ACL", "DELUSER", "kostas"});
Expand Down Expand Up @@ -272,7 +321,7 @@ TEST_F(AclFamilyTest, TestGetUser) {
resp = Run({"ACL", "GETUSER", "kostas"});
const auto& kvec = resp.GetVec();
EXPECT_THAT(kvec[0], "flags");
EXPECT_THAT(kvec[1].GetVec(), UnorderedElementsAre("off", "nopass"));
EXPECT_THAT(kvec[1].GetVec(), UnorderedElementsAre("off"));
EXPECT_THAT(kvec[2], "passwords");
EXPECT_TRUE(kvec[3].GetVec().empty());
EXPECT_THAT(kvec[4], "commands");
Expand Down
33 changes: 26 additions & 7 deletions src/server/acl/helpers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,22 @@ std::optional<ParseKeyResult> MaybeParseAclKey(std::string_view command) {
return ParseKeyResult{std::string(key), op};
}

std::optional<std::string> MaybeParsePassword(std::string_view command, bool hashed) {
std::optional<User::UpdatePass> MaybeParsePassword(std::string_view command, bool hashed) {
using UpPass = User::UpdatePass;
if (command == "nopass") {
return std::string(command);
return UpPass{"", false, true};
}

if (command == "resetpass") {
return UpPass{"", false, false, true};
}

if (command[0] == '>' || (hashed && command[0] == '#')) {
return std::string(command.substr(1));
return UpPass{std::string(command.substr(1))};
}

if (command[0] == '<') {
return UpPass{std::string(command.substr(1)), true};
}

return {};
Expand Down Expand Up @@ -261,10 +270,8 @@ std::variant<User::UpdateRequest, ErrorReply> ParseAclSetUser(facade::ArgRange a

for (std::string_view arg : args) {
if (auto pass = MaybeParsePassword(facade::ToSV(arg), hashed); pass) {
if (req.password) {
return ErrorReply("Only one password is allowed");
}
req.password = std::move(pass);
req.passwords.push_back(std::move(*pass));

if (hashed && absl::StartsWith(facade::ToSV(arg), "#")) {
req.is_hashed = hashed;
}
Expand Down Expand Up @@ -346,4 +353,16 @@ std::string AclKeysToString(const AclKeys& keys) {
return result;
}

std::string PasswordsToString(const absl::flat_hash_set<std::string>& passwords, bool nopass,
bool full_sha) {
if (nopass) {
return "nopass ";
}
std::string result;
for (const auto& pass : passwords) {
absl::StrAppend(&result, "#", PrettyPrintSha(pass, full_sha), " ");
}

return result;
}
Comment on lines +356 to +367
Copy link
Contributor

Choose a reason for hiding this comment

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

so it can very well be that a user doesn't have nopass set, but it's password set is empty. Maybe we should print some kind of warning in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am following redis/valkey here :) If there is no nopass or anything that starts with # it means that there is no password set and the field is left blank

} // namespace dfly::acl
7 changes: 6 additions & 1 deletion src/server/acl/helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <string_view>
#include <variant>

#include "absl/container/flat_hash_set.h"
#include "facade/facade_types.h"
#include "server/acl/acl_log.h"
#include "server/acl/user.h"
Expand All @@ -23,7 +24,7 @@ std::string AclCatAndCommandToString(const User::CategoryChanges& cat,
std::string PrettyPrintSha(std::string_view pass, bool all = false);

// When hashed is true, we allow passwords that start with both # and >
std::optional<std::string> MaybeParsePassword(std::string_view command, bool hashed = false);
std::optional<User::UpdatePass> MaybeParsePassword(std::string_view command, bool hashed = false);

std::optional<bool> MaybeParseStatus(std::string_view command);

Expand Down Expand Up @@ -55,4 +56,8 @@ struct ParseKeyResult {
std::optional<ParseKeyResult> MaybeParseAclKey(std::string_view command);

std::string AclKeysToString(const AclKeys& keys);

std::string PasswordsToString(const absl::flat_hash_set<std::string>& passwords, bool nopass,
bool full_sha);

} // namespace dfly::acl
48 changes: 34 additions & 14 deletions src/server/acl/user.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <limits>

#include "absl/container/flat_hash_set.h"
#include "absl/strings/escaping.h"
#include "core/overloaded.h"
#include "server/acl/helpers.h"
Expand All @@ -30,8 +31,20 @@ User::User() {
}

void User::Update(UpdateRequest&& req) {
if (req.password) {
SetPasswordHash(*req.password, req.is_hashed);
for (auto& pass : req.passwords) {
if (pass.nopass) {
SetNopass();
continue;
}
if (pass.unset) {
UnsetPassword(pass.password);
continue;
}
if (pass.reset_password) {
password_hashes_.clear();
continue;
}
SetPasswordHash(pass.password, req.is_hashed);
}

auto cat_visitor = [this](UpdateRequest::CategoryValueType cat) {
Expand Down Expand Up @@ -68,23 +81,23 @@ void User::Update(UpdateRequest&& req) {
}

void User::SetPasswordHash(std::string_view password, bool is_hashed) {
if (password == "nopass") {
return;
}

nopass_ = false;
if (is_hashed) {
password_hash_ = absl::HexStringToBytes(password);
password_hashes_.insert(absl::HexStringToBytes(password));
return;
}
password_hash_ = StringSHA256(password);
password_hashes_.insert(StringSHA256(password));
}

void User::UnsetPassword(std::string_view password) {
password_hashes_.erase(StringSHA256(password));
}

bool User::HasPassword(std::string_view password) const {
if (!password_hash_) {
if (nopass_) {
return true;
}
// hash password and compare
return *password_hash_ == StringSHA256(password);
return password_hashes_.contains(StringSHA256(password));
}

void User::SetAclCategoriesAndIncrSeq(uint32_t cat) {
Expand Down Expand Up @@ -174,10 +187,12 @@ bool User::IsActive() const {
return is_active_;
}

static const std::string_view default_pass = "nopass";
const absl::flat_hash_set<std::string>& User::Passwords() const {
return password_hashes_;
}

std::string_view User::Password() const {
return password_hash_ ? *password_hash_ : default_pass;
bool User::HasNopass() const {
return nopass_;
}

const AclKeys& User::Keys() const {
Expand Down Expand Up @@ -206,4 +221,9 @@ void User::SetKeyGlobs(std::vector<UpdateKey> keys) {
}
}

void User::SetNopass() {
nopass_ = true;
password_hashes_.clear();
}

} // namespace dfly::acl
Loading
Loading