-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(AclFamily): add AUTH for acl members #1732
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
Changes from 10 commits
f6d8d1e
f4dbe93
d37290a
0cf215e
20412b9
9d3b767
cc2432d
c0356d8
2251f8e
ff6e3f0
16aa85f
d98d270
d84ff8f
b4a9163
cdfdb29
dba320c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// Copyright 2022, DragonflyDB authors. All rights reserved. | ||
// See LICENSE for licensing terms. | ||
// | ||
// | ||
|
||
template <class... Ts> struct Overloaded : Ts... { using Ts::operator()...; }; | ||
|
||
template <class... Ts> Overloaded(Ts...) -> Overloaded<Ts...>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,25 +3,30 @@ | |
|
||
#include "server/acl/acl_family.h" | ||
|
||
#include <optional> | ||
#include <variant> | ||
|
||
#include "absl/strings/ascii.h" | ||
#include "absl/strings/match.h" | ||
#include "absl/strings/str_cat.h" | ||
#include "core/overloaded.h" | ||
#include "facade/facade_types.h" | ||
#include "server/acl/acl_commands_def.h" | ||
#include "server/command_registry.h" | ||
#include "server/conn_context.h" | ||
#include "server/server_state.h" | ||
|
||
namespace dfly::acl { | ||
|
||
constexpr uint32_t kList = acl::ADMIN | acl::SLOW | acl::DANGEROUS; | ||
|
||
static std::string AclToString(uint32_t acl_category) { | ||
std::string tmp; | ||
|
||
if (acl_category == acl::ALL) { | ||
return "+@all"; | ||
return "+@ALL"; | ||
} | ||
|
||
if (acl_category == acl::NONE) { | ||
return "+@none"; | ||
return "+@NONE"; | ||
} | ||
|
||
const std::string prefix = "+@"; | ||
|
@@ -32,7 +37,8 @@ static std::string AclToString(uint32_t acl_category) { | |
absl::StrAppend(&tmp, prefix, REVERSE_CATEGORY_INDEX_TABLE[step], postfix); | ||
} | ||
} | ||
tmp.erase(tmp.size()); | ||
|
||
tmp.pop_back(); | ||
|
||
return tmp; | ||
} | ||
|
@@ -61,10 +67,108 @@ void AclFamily::List(CmdArgList args, ConnectionContext* cntx) { | |
} | ||
} | ||
|
||
namespace { | ||
|
||
std::optional<std::string> MaybeParsePassword(std::string_view command) { | ||
if (command[0] != '>') { | ||
return {}; | ||
} | ||
|
||
return {std::string(command.substr(1))}; | ||
} | ||
|
||
std::optional<bool> MaybeParseStatus(std::string_view command) { | ||
if (command == "ON") { | ||
return true; | ||
} | ||
if (command == "OFF") { | ||
return false; | ||
} | ||
return {}; | ||
} | ||
|
||
using OptCat = std::optional<uint32_t>; | ||
|
||
// bool == true if + | ||
// bool == false if - | ||
std::pair<OptCat, bool> MaybeParseAclCategory(std::string_view command) { | ||
if (absl::StartsWith(command, "+@")) { | ||
auto res = CATEGORY_INDEX_TABLE.find(command.substr(2)); | ||
if (res == CATEGORY_INDEX_TABLE.end()) { | ||
return {}; | ||
} | ||
return {res->second, true}; | ||
} | ||
|
||
if (absl::StartsWith(command, "-@")) { | ||
auto res = CATEGORY_INDEX_TABLE.find(command.substr(2)); | ||
if (res == CATEGORY_INDEX_TABLE.end()) { | ||
return {}; | ||
} | ||
return {res->second, false}; | ||
} | ||
|
||
return {}; | ||
} | ||
|
||
using facade::ErrorReply; | ||
|
||
std::variant<User::UpdateRequest, ErrorReply> ParseAclSetUser(CmdArgList args) { | ||
User::UpdateRequest req; | ||
|
||
for (auto arg : args) { | ||
if (auto pass = MaybeParsePassword(facade::ToSV(arg)); pass) { | ||
if (req.password) { | ||
return ErrorReply("Only one password is allowed"); | ||
} | ||
req.password = std::move(pass); | ||
continue; | ||
} | ||
|
||
ToUpper(&arg); | ||
const auto command = facade::ToSV(arg); | ||
|
||
if (auto status = MaybeParseStatus(command); status) { | ||
if (req.is_active) { | ||
return ErrorReply("Multiple ON/OFF are not allowed"); | ||
} | ||
req.is_active = *status; | ||
continue; | ||
} | ||
|
||
auto [cat, add] = MaybeParseAclCategory(command); | ||
if (!cat) { | ||
return ErrorReply(absl::StrCat("Unrecognized paramter", command)); | ||
} | ||
|
||
auto* acl_field = add ? &req.plus_acl_categories : &req.minus_acl_categories; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit silly, but do we want to check no category appears both as a minus and as a plus? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is from the previous PR. What do you mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. like what happens if I do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I thought about this before, and for this specific case there won't be an issue because the ADD set is applied Before the REMOVE set and I thought that this redundancy is fine. However, now I am thinking about it, for the reverse case this is not true, that is Another solution is to apply them one by one in order which is slightly slower (since I can apply them all at once) but I don't expect it will affect the performance and IMO I think this is a better solution since it covers the ordering and we can allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will address this in a separate PR since this is about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait... What operations are permitted? If the string is SSO, then any operations on the map (rehashing, shrinking, etc) will invalidate the string_view 🤨
PS: I wanted to put this comment below the |
||
*acl_field = acl_field->value_or(0) | *cat; | ||
} | ||
|
||
return req; | ||
} | ||
|
||
} // namespace | ||
|
||
void AclFamily::SetUser(CmdArgList args, ConnectionContext* cntx) { | ||
std::string_view username = facade::ToSV(args[0]); | ||
auto req = ParseAclSetUser(args.subspan(1)); | ||
auto error_case = [cntx](ErrorReply&& error) { (*cntx)->SendError(error); }; | ||
auto update_case = [username, cntx](User::UpdateRequest&& req) { | ||
ServerState::tlocal()->user_registry->MaybeAddAndUpdate(username, std::move(req)); | ||
(*cntx)->SendOk(); | ||
}; | ||
|
||
std::visit(Overloaded{error_case, update_case}, std::move(req)); | ||
} | ||
|
||
using CI = dfly::CommandId; | ||
|
||
#define HFUNC(x) SetHandler(&AclFamily::x) | ||
|
||
constexpr uint32_t kList = acl::ADMIN | acl::SLOW | acl::DANGEROUS; | ||
constexpr uint32_t kSetUser = acl::ADMIN | acl::SLOW | acl::DANGEROUS; | ||
|
||
// We can't implement the ACL commands and its respective subcommands LIST, CAT, etc | ||
// the usual way, (that is, one command called ACL which then dispatches to the subcommand | ||
// based on the secocond argument) because each of the subcommands has different ACL | ||
|
@@ -76,6 +180,8 @@ void AclFamily::Register(dfly::CommandRegistry* registry) { | |
*registry << CI{"ACL", CO::NOSCRIPT | CO::LOADING, 0, 0, 0, 0, acl::kList}.HFUNC(Acl); | ||
*registry << CI{"ACL LIST", CO::ADMIN | CO::NOSCRIPT | CO::LOADING, 1, 0, 0, 0, acl::kList}.HFUNC( | ||
List); | ||
*registry << CI{"ACL SETUSER", CO::ADMIN | CO::NOSCRIPT | CO::LOADING, -2, 0, 0, 0, acl::kSetUser} | ||
.HFUNC(SetUser); | ||
} | ||
|
||
#undef HFUNC | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,6 +5,7 @@ | |||||
#include "server/acl/user_registry.h" | ||||||
|
||||||
#include "core/fibers.h" | ||||||
#include "facade/facade_types.h" | ||||||
#include "server/acl/acl_commands_def.h" | ||||||
|
||||||
namespace dfly::acl { | ||||||
|
@@ -44,14 +45,15 @@ bool UserRegistry::IsUserActive(std::string_view username) const { | |||||
return it->second.IsActive(); | ||||||
} | ||||||
|
||||||
bool UserRegistry::AuthUser(std::string_view username, std::string_view password) const { | ||||||
std::pair<bool, const std::string_view> UserRegistry::AuthUser(std::string_view username, | ||||||
std::string_view password) const { | ||||||
std::shared_lock<util::SharedMutex> lock(mu_); | ||||||
const auto& user = registry_.find(username); | ||||||
if (user == registry_.end()) { | ||||||
return false; | ||||||
return {false, {}}; | ||||||
} | ||||||
|
||||||
return user->second.HasPassword(password); | ||||||
return {user->second.HasPassword(password) && user->second.IsActive(), user->first}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personal preference, but I don't mind the suggestion. I think the second is more likely than the first but oh well 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a security perspective I don't want us to check the password if the user is disabled. It doesn't really matter but it's a bad practice IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmmm, what could go wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 'Reject illegal queries as soon as you can know they're illegal' is just a good security principal, even if it doesn't have immediate bad consequences. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but you don't always engineer around security in mind -- any obsession over one principle can backfire pretty fast, imagine if C++ was designed with this in mind, no UB etc, it would be monstrous (and I don't bite the arguments about Rust being a better safe alternative this is so easy to debunk). That being said for execution paths that are guaranteed to be handled I don't see any problem. Again, I like to consider suggestions and since you think that this is a good principle I happily applied it 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is code that handles authentication. It HAS to be designed with security in mind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤣 |
||||||
} | ||||||
|
||||||
UserRegistry::RegistryViewWithLock::RegistryViewWithLock(std::shared_lock<util::SharedMutex> mu, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,8 @@ class UserRegistry { | |
|
||
// Acquires a read lock | ||
// Used by Auth | ||
bool AuthUser(std::string_view username, std::string_view password) const; | ||
std::pair<bool, const std::string_view> AuthUser(std::string_view username, | ||
std::string_view password) const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment what the return value means (not obvious without looking at impl how to tell apart non-existing and existing but disabled user) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No needed anymore, we deep copy the username :) |
||
|
||
// Helper class for accessing the registry with a ReadLock outside the scope of UserRegistry | ||
class RegistryViewWithLock { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -196,6 +196,8 @@ class ConnectionContext : public facade::ConnectionContext { | |
// Reference to a FlowInfo for this connection if from a master to a replica. | ||
FlowInfo* replication_flow; | ||
|
||
std::optional<std::string_view> authed_username; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, the underlying There's an issue in general of what happens to connections when their user is deleted. But this especially looks like a UAF bug waiting to happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope. That's not true.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, but please document this next to the field. |
||
|
||
private: | ||
void EnableMonitoring(bool enable) { | ||
subscriptions++; // required to support the monitoring | ||
|
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.