feat(acl): add validation for acl keys #2272
Conversation
src/server/acl/validator.cc
Outdated
| const size_t step = (id.opt_mask() & CO::CommandOpt::INTERLEAVED_KEYS) ? 2 : 1; | ||
| const bool var_keys = (id.opt_mask() & CO::CommandOpt::VARIADIC_KEYS); | ||
|
|
There was a problem hiding this comment.
you should use DetermineKeys() from transactio.h which gives you a key index
src/server/acl/validator.cc
Outdated
| const size_t end = var_keys ? tail_args.size() : id.last_key_pos() + 1; | ||
| for (size_t i = id.first_key_pos(); i < end; i += step) { |
There was a problem hiding this comment.
there is something called a bonus key, you'll see when you use DetermineKeys()
There was a problem hiding this comment.
I forgot about this and the bonus keys. Nice catch!
| const bool var_keys = (id.opt_mask() & CO::CommandOpt::VARIADIC_KEYS); | ||
|
|
||
| auto match = [](const auto& pattern, const auto& target) { | ||
| return stringmatchlen(pattern.data(), pattern.size(), target.data(), target.size(), 0); |
There was a problem hiding this comment.
wow wtf this is a built-in redis function
| #include "server/server_state.h" | ||
| extern "C" { | ||
| #include "redis/util.h" |
There was a problem hiding this comment.
please add its for stringmatchlen because it's hard to find otherwise
src/server/acl/validator.cc
Outdated
| return stringmatchlen(pattern.data(), pattern.size(), target.data(), target.size(), 0); | ||
| }; | ||
|
|
||
| bool key_allowed = true; |
There was a problem hiding this comment.
key_allowed is a confusing name for a resulting variable, you probably had command_allowed in mind
There was a problem hiding this comment.
I meant keys_allowed, because this part of the code checks if the command key arguments are allowed by the authed user
src/server/acl/validator.cc
Outdated
| const bool command = | ||
| (acl_cat & cat_credentials) != 0 || (acl_commands[index] & command_mask) != 0; | ||
|
|
||
| return command && key_allowed; |
There was a problem hiding this comment.
you probably want to check that first before you go over all the keys
src/server/acl/validator.cc
Outdated
| const bool is_read_command = id.opt_mask() & CO::CommandOpt::READONLY; | ||
| const bool is_write_command = id.opt_mask() & CO::CommandOpt::WRITE; |
There was a problem hiding this comment.
- Didn't you add IsReadOnly/IsWriteOnly below for that?
- Why did you use
::CommandOpt::... I understand it's the full path, but first it's not easier to read, second it's harder to search. I always useCO::Xto search for places in code that rely on some flag
(2) actually also applies to (1) 😆
There was a problem hiding this comment.
uh I thought it was a scoped enum
| bool IsReadOnly() const { | ||
| return opt_mask_ & CO::CommandOpt::READONLY; | ||
| } | ||
|
|
||
| bool IsWriteOnly() const { | ||
| return opt_mask_ & CO::CommandOpt::WRITE; | ||
| } | ||
|
|
There was a problem hiding this comment.
I suggest that whenever we add a useful utility, let's replace old code with it, because otherwise we just spread non conformity and make it harder to search for uses
PR5
part of #2229