Skip to content

chore(acl): Implicit categories #3411

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

Merged
merged 3 commits into from
Nov 8, 2024
Merged

chore(acl): Implicit categories #3411

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
0789942
chore(acl): Implicit categories
dranikpg Jul 30, 2024
918e462
chore: add comment
dranikpg Nov 8, 2024
888fd5c
chore: fix fast categories
dranikpg Nov 8, 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
53 changes: 47 additions & 6 deletions src/server/command_registry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,48 @@ using absl::StrAppend;
using absl::StrCat;
using absl::StrSplit;

CommandId::CommandId(const char* name, uint32_t mask, int8_t arity, int8_t first_key,
int8_t last_key, uint32_t acl_categories)
: facade::CommandId(name, mask, arity, first_key, last_key, acl_categories) {
namespace {

uint32_t ImplicitCategories(uint32_t mask) {
if (mask & CO::ADMIN)
opt_mask_ |= CO::NOSCRIPT;
mask |= CO::NOSCRIPT;
return mask;
}

uint32_t ImplicitAclCategories(uint32_t mask) {
mask = ImplicitCategories(mask);
uint32_t out = 0;

if (mask & CO::WRITE)
out |= acl::WRITE;

if ((mask & CO::READONLY) && ((mask & CO::NOSCRIPT) == 0))
out |= acl::READ;

if (mask & CO::ADMIN)
out |= acl::ADMIN | acl::DANGEROUS;

// todo pubsub

if (mask & CO::FAST)
out |= acl::FAST;

if (mask & CO::BLOCKING)
out |= acl::BLOCKING;

if ((out & acl::FAST) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

A counter example to this is HGETALL which we mark as:

<< CI{"HGETALL", CO::FAST | CO::READONLY, 2, 1, 1, acl::kHGetAll}.HFUNC(HGetAll)

which according to the rules above would mark it as FAST instead of SLOW.

My concern here is that we have to manually check all of these commands and the CO:: variants actually do map to the rules implemented in ImplicitAclCategories (and if the above mapping has no mistakes as well)

Another concern I have is that if we have bugs in our CO:: variants the Implicit rules describe above will insert a bugs in our ACL rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another concern I have is that if we have bugs

You can apply that concern to any code, I've added an option to return acl categories from COMMAND, so we can compare before and after and see the differences

before.txt

A counter example to this is HGETALL which we mark as:

It's not a counter example, we just remove CO::FAST. Again, the CO tags don't mean anything now

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, in fact a diff on this will also hinter any errors. I like that

out |= acl::SLOW;

return out;
}

} // namespace

CommandId::CommandId(const char* name, uint32_t mask, int8_t arity, int8_t first_key,
int8_t last_key, std::optional<uint32_t> acl_categories)
: facade::CommandId(name, ImplicitCategories(mask), arity, first_key, last_key,
acl_categories.value_or(ImplicitAclCategories(mask))) {
implicit_acl_ = !acl_categories.has_value();
}

bool CommandId::IsTransactional() const {
Expand Down Expand Up @@ -152,6 +189,9 @@ CommandRegistry& CommandRegistry::operator<<(CommandId cmd) {
}

cmd.SetFamily(family_of_commands_.size() - 1);
if (acl_category_)
cmd.SetAclCategory(*acl_category_);

if (!is_sub_command || absl::StartsWith(cmd.name(), "ACL")) {
cmd.SetBitIndex(1ULL << bit_index_);
family_of_commands_.back().push_back(std::string(k));
Expand All @@ -165,9 +205,10 @@ CommandRegistry& CommandRegistry::operator<<(CommandId cmd) {
return *this;
}

void CommandRegistry::StartFamily() {
family_of_commands_.push_back({});
void CommandRegistry::StartFamily(std::optional<uint32_t> acl_category) {
family_of_commands_.emplace_back();
bit_index_ = 0;
acl_category_ = acl_category;
}

std::string_view CommandRegistry::RenamedOrOriginal(std::string_view orig) const {
Expand Down
11 changes: 9 additions & 2 deletions src/server/command_registry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class CommandId : public facade::CommandId {
// NOTICE: name must be a literal string, otherwise metrics break! (see cmd_stats_map in
// server_state.h)
CommandId(const char* name, uint32_t mask, int8_t arity, int8_t first_key, int8_t last_key,
uint32_t acl_categories);
std::optional<uint32_t> acl_categories = std::nullopt);

CommandId(CommandId&&) = default;

Expand Down Expand Up @@ -146,7 +146,13 @@ class CommandId : public facade::CommandId {
return command_stats_[thread_index];
}

void SetAclCategory(uint32_t mask) {
if (implicit_acl_)
acl_categories_ |= mask;
}

private:
bool implicit_acl_;
std::unique_ptr<CmdCallStats[]> command_stats_;
Handler handler_;
ArgValidator validator_;
Expand Down Expand Up @@ -194,7 +200,7 @@ class CommandRegistry {
}
}

void StartFamily();
void StartFamily(std::optional<uint32_t> acl_category = std::nullopt);

std::string_view RenamedOrOriginal(std::string_view orig) const;

Expand All @@ -212,6 +218,7 @@ class CommandRegistry {

FamiliesVec family_of_commands_;
size_t bit_index_;
std::optional<uint32_t> acl_category_; // category of family currently being built
};

} // namespace dfly
15 changes: 13 additions & 2 deletions src/server/main_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2487,8 +2487,8 @@ void Service::Command(CmdArgList args, Transaction* tx, SinkReplyBuilder* builde
});

auto* rb = static_cast<RedisReplyBuilder*>(builder);
auto serialize_command = [&rb](string_view name, const CommandId& cid) {
rb->StartArray(6);
auto serialize_command = [&rb, this](string_view name, const CommandId& cid) {
rb->StartArray(7);
rb->SendSimpleString(cid.name());
rb->SendLong(cid.arity());
rb->StartArray(CommandId::OptCount(cid.opt_mask()));
Expand All @@ -2504,6 +2504,17 @@ void Service::Command(CmdArgList args, Transaction* tx, SinkReplyBuilder* builde
rb->SendLong(cid.first_key_pos());
rb->SendLong(cid.last_key_pos());
rb->SendLong(cid.opt_mask() & CO::INTERLEAVED_KEYS ? 2 : 1);

{
const auto& table = acl_family_.GetRevTable();
vector<string> cats;
for (uint32_t i = 0; i < 32; i++) {
if (cid.acl_categories() & (1 << i)) {
cats.emplace_back("@" + table[i]);
}
}
rb->SendSimpleStrArr(cats);
}
};

// If no arguments are specified, reply with all commands
Expand Down
84 changes: 26 additions & 58 deletions src/server/string_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1530,68 +1530,36 @@ void StringFamily::ClThrottle(CmdArgList args, Transaction* tx, SinkReplyBuilder

#define HFUNC(x) SetHandler(&StringFamily::x)

namespace acl {
constexpr uint32_t kSet = WRITE | STRING | SLOW;
constexpr uint32_t kSetEx = WRITE | STRING | SLOW;
constexpr uint32_t kPSetEx = WRITE | STRING | SLOW;
constexpr uint32_t kSetNx = WRITE | STRING | FAST;
constexpr uint32_t kAppend = WRITE | STRING | FAST;
constexpr uint32_t kPrepend = WRITE | STRING | FAST;
constexpr uint32_t kIncr = WRITE | STRING | FAST;
constexpr uint32_t kDecr = WRITE | STRING | FAST;
constexpr uint32_t kIncrBy = WRITE | STRING | FAST;
constexpr uint32_t kIncrByFloat = WRITE | STRING | FAST;
constexpr uint32_t kDecrBy = WRITE | STRING | FAST;
constexpr uint32_t kGet = READ | STRING | FAST;
constexpr uint32_t kGetDel = WRITE | STRING | FAST;
constexpr uint32_t kGetEx = WRITE | STRING | FAST;
constexpr uint32_t kGetSet = WRITE | STRING | FAST;
constexpr uint32_t kMGet = READ | STRING | FAST;
constexpr uint32_t kMSet = WRITE | STRING | SLOW;
constexpr uint32_t kMSetNx = WRITE | STRING | SLOW;
constexpr uint32_t kStrLen = READ | STRING | FAST;
constexpr uint32_t kGetRange = READ | STRING | SLOW;
constexpr uint32_t kSubStr = READ | STRING | SLOW;
constexpr uint32_t kSetRange = WRITE | STRING | SLOW;
// ClThrottle is a module in redis. Therefore we introduce a new extension
// to the category. We should consider other defaults as well
constexpr uint32_t kClThrottle = THROTTLE;
} // namespace acl

void StringFamily::Register(CommandRegistry* registry) {
constexpr uint32_t kMSetMask =
CO::WRITE | CO::DENYOOM | CO::INTERLEAVED_KEYS | CO::NO_AUTOJOURNAL;

registry->StartFamily();
*registry
<< CI{"SET", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL, -3, 1, 1, acl::kSet}.HFUNC(Set)
<< CI{"SETEX", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL, 4, 1, 1, acl::kSetEx}.HFUNC(
SetEx)
<< CI{"PSETEX", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL, 4, 1, 1, acl::kPSetEx}.HFUNC(
PSetEx)
<< CI{"SETNX", CO::WRITE | CO::DENYOOM, 3, 1, 1, acl::kSetNx}.HFUNC(SetNx)
<< CI{"APPEND", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1, acl::kAppend}.HFUNC(Append)
<< CI{"PREPEND", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1, acl::kPrepend}.HFUNC(Prepend)
<< CI{"INCR", CO::WRITE | CO::FAST, 2, 1, 1, acl::kIncr}.HFUNC(Incr)
<< CI{"DECR", CO::WRITE | CO::FAST, 2, 1, 1, acl::kDecr}.HFUNC(Decr)
<< CI{"INCRBY", CO::WRITE | CO::FAST, 3, 1, 1, acl::kIncrBy}.HFUNC(IncrBy)
<< CI{"INCRBYFLOAT", CO::WRITE | CO::FAST, 3, 1, 1, acl::kIncrByFloat}.HFUNC(IncrByFloat)
<< CI{"DECRBY", CO::WRITE | CO::FAST, 3, 1, 1, acl::kDecrBy}.HFUNC(DecrBy)
<< CI{"GET", CO::READONLY | CO::FAST, 2, 1, 1, acl::kGet}.HFUNC(Get)
<< CI{"GETDEL", CO::WRITE | CO::FAST, 2, 1, 1, acl::kGetDel}.HFUNC(GetDel)
<< CI{"GETEX", CO::WRITE | CO::DENYOOM | CO::FAST | CO::NO_AUTOJOURNAL, -2, 1, 1, acl::kGetEx}
.HFUNC(GetEx)
<< CI{"GETSET", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1, acl::kGetSet}.HFUNC(GetSet)
<< CI{"MGET", CO::READONLY | CO::FAST | CO::IDEMPOTENT, -2, 1, -1, acl::kMGet}.HFUNC(MGet)
<< CI{"MSET", kMSetMask, -3, 1, -1, acl::kMSet}.HFUNC(MSet)
<< CI{"MSETNX", kMSetMask, -3, 1, -1, acl::kMSetNx}.HFUNC(MSetNx)
<< CI{"STRLEN", CO::READONLY | CO::FAST, 2, 1, 1, acl::kStrLen}.HFUNC(StrLen)
<< CI{"GETRANGE", CO::READONLY | CO::FAST, 4, 1, 1, acl::kGetRange}.HFUNC(GetRange)
<< CI{"SUBSTR", CO::READONLY | CO::FAST, 4, 1, 1, acl::kSubStr}.HFUNC(
GetRange) // Alias for GetRange
<< CI{"SETRANGE", CO::WRITE | CO::FAST | CO::DENYOOM, 4, 1, 1, acl::kSetRange}.HFUNC(SetRange)
<< CI{"CL.THROTTLE", CO::WRITE | CO::DENYOOM | CO::FAST, -5, 1, 1, acl::kClThrottle}.HFUNC(
ClThrottle);
registry->StartFamily(acl::STRING);
Copy link
Contributor

@kostasrim kostasrim Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we can create a class template that is implicitly convertible to CommandId and during that conversion it injects the acl family like acl::STRING here which will move away the logic from registry. But IMO I am mostly worried about correctness than syntactic sugar

*registry << CI{"SET", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL, -3, 1, 1}.HFUNC(Set)
<< CI{"SETEX", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL, 4, 1, 1}.HFUNC(SetEx)
<< CI{"PSETEX", CO::WRITE | CO::DENYOOM | CO::NO_AUTOJOURNAL, 4, 1, 1}.HFUNC(PSetEx)
<< CI{"SETNX", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1}.HFUNC(SetNx)
<< CI{"APPEND", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1}.HFUNC(Append)
<< CI{"PREPEND", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1}.HFUNC(Prepend)
<< CI{"INCR", CO::WRITE | CO::FAST, 2, 1, 1}.HFUNC(Incr)
<< CI{"DECR", CO::WRITE | CO::FAST, 2, 1, 1}.HFUNC(Decr)
<< CI{"INCRBY", CO::WRITE | CO::FAST, 3, 1, 1}.HFUNC(IncrBy)
<< CI{"INCRBYFLOAT", CO::WRITE | CO::FAST, 3, 1, 1}.HFUNC(IncrByFloat)
<< CI{"DECRBY", CO::WRITE | CO::FAST, 3, 1, 1}.HFUNC(DecrBy)
<< CI{"GET", CO::READONLY | CO::FAST, 2, 1, 1}.HFUNC(Get)
<< CI{"GETDEL", CO::WRITE | CO::FAST, 2, 1, 1}.HFUNC(GetDel)
<< CI{"GETEX", CO::WRITE | CO::DENYOOM | CO::FAST | CO::NO_AUTOJOURNAL, -2, 1, 1}.HFUNC(
GetEx)
<< CI{"GETSET", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1}.HFUNC(GetSet)
<< CI{"MGET", CO::READONLY | CO::FAST | CO::IDEMPOTENT, -2, 1, -1}.HFUNC(MGet)
<< CI{"MSET", kMSetMask, -3, 1, -1}.HFUNC(MSet)
<< CI{"MSETNX", kMSetMask, -3, 1, -1}.HFUNC(MSetNx)
<< CI{"STRLEN", CO::READONLY | CO::FAST, 2, 1, 1}.HFUNC(StrLen)
<< CI{"GETRANGE", CO::READONLY, 4, 1, 1}.HFUNC(GetRange)
<< CI{"SUBSTR", CO::READONLY, 4, 1, 1}.HFUNC(GetRange) // Alias for GetRange
<< CI{"SETRANGE", CO::WRITE | CO::DENYOOM, 4, 1, 1}.HFUNC(SetRange)
<< CI{"CL.THROTTLE", CO::WRITE | CO::DENYOOM | CO::FAST, -5, 1, 1, acl::THROTTLE}.HFUNC(
ClThrottle);
}

} // namespace dfly
Loading