Skip to content

chore: remove locks from acl validation in the hot path #1765

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 7 commits into from
Aug 31, 2023
Merged

chore: remove locks from acl validation in the hot path #1765

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
90b623f
chore: remove locks from acl validation in the hot path
kostasrim Aug 29, 2023
627581f
Merge branch 'main' into stream_acl_user_updates
kostasrim Aug 30, 2023
a3bfb88
chore: address gh comments
kostasrim Aug 31, 2023
d0b8752
Merge branch 'main' into stream_acl_user_updates
kostasrim Aug 31, 2023
ed4484c
fix: broken check causing segfault
kostasrim Aug 31, 2023
01efc46
chore: address gh comments
kostasrim Aug 31, 2023
739581e
Merge branch 'main' into stream_acl_user_updates
kostasrim Aug 31, 2023
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
42 changes: 39 additions & 3 deletions src/server/acl/acl_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include "server/acl/acl_family.h"

#include <glog/logging.h>

#include <cctype>
#include <optional>
#include <variant>
Expand All @@ -12,6 +14,7 @@
#include "absl/strings/match.h"
#include "absl/strings/str_cat.h"
#include "core/overloaded.h"
#include "facade/dragonfly_connection.h"
#include "facade/facade_types.h"
#include "server/acl/acl_commands_def.h"
#include "server/command_registry.h"
Expand Down Expand Up @@ -46,6 +49,9 @@ static std::string AclToString(uint32_t acl_category) {
return tmp;
}

AclFamily::AclFamily(util::ProactorPool& pp) : pp_(pp) {
}

void AclFamily::Acl(CmdArgList args, ConnectionContext* cntx) {
(*cntx)->SendError("Wrong number of arguments for acl command");
}
Expand Down Expand Up @@ -157,12 +163,31 @@ std::variant<User::UpdateRequest, ErrorReply> ParseAclSetUser(CmdArgList args) {

} // namespace

void AclFamily::StreamUpdatesToAllProactorConnections(std::string_view user, uint32_t update_cat) {
auto update_cb = [user, update_cat]([[maybe_unused]] size_t id, util::Connection* conn) {
DCHECK(conn);
auto connection = static_cast<facade::Connection*>(conn);
auto ctx = static_cast<ConnectionContext*>(connection->cntx());
if (ctx && user == ctx->authed_username) {
ctx->acl_categories = update_cat;
}
};

Comment on lines +163 to +172
Copy link
Contributor

Choose a reason for hiding this comment

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

SetUser is called so rarely that we can neglect the cost of traversing all connections? (of which there are also just a few usually?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Damn... What if the context is performing squashing? 😭

We can copy the value (instead of referencing the parent), but then how do we propagate changes? On each batch? 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want you can drop support for squashing and I will think how to enable support for your features

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, because Redis is single-threaded it's impossible for ACL rules to change while its executing a script / a multi transaction, right?

Copy link
Contributor Author

@kostasrim kostasrim Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

SetUser is called so rarely that we can neglect the cost of traversing all connections? (of which there are also just a few usually?)

Yes that's true but it doesn't hurt to not traverse if the user was just created :)

Damn... What if the context is performing squashing? 😭 We can copy the value (instead of referencing the parent), but then how do we propagate changes? On each batch? 😕

I already thought of this 👨‍🍳 ! It won't affect us. Why? Because, sure we update the "stub" contexts as well BUT validation is done on the parent and not on the stub context. Therefore, we don't really care, since the update done to stub contexts won't affect anything(think of it a stub update -- we mocked the mock 🔪 ). That is before a batch executes we call ValidateCommandExecution which will see the updated changes since it promts the parent and not the stub context

If you want you can drop support for squashing and I will think how to enable support for your features

It's all fine and if it wasn't we would fix it 🚀 No rush here

Btw, because Redis is single-threaded it's impossible for ACL rules to change while its executing a script / a multi transaction, right?

Redis is single threaded indeed but connections can be multiplexed just like on a single thread you can run multiple tasks interleaved with each other (to the user this looks like multihreading, in reality it's just a single thread that interleaves task execution). That being said, this example works on redis:

time     client 1                          client 2
t1         redis-cli
t2         auth user user
t3         multi
t4         set key 23
t5                                              redis-cli
t6                                              setuser kostas -@string
t7         exec --> ERROR, categories changed between `multi/exec`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t1 to t7 is monotonically increasing

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to the case when acl rules change while exec is running

BUT validation is done on the parent and not on the stub context

Exactly. When you change a connection from its own thread, you assume no other thread has access to it. So it is safe to mutate variables. BUT with squashing, the parent connection is accessed from multiple threads for validation, so its not possible to mutate it (because ValidateCommandExecution might be running on another thread)

pp_.AwaitFiberOnAll([this, update_cb](util::ProactorBase* pb) {
for (auto& listener : listeners_) {
listener->TraverseConnections(update_cb);
}
});
}

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) {
auto update_case = [username, cntx, this](User::UpdateRequest&& req) {
ServerState::tlocal()->user_registry->MaybeAddAndUpdate(username, std::move(req));
auto cred = ServerState::tlocal()->user_registry->GetCredentials(username);
StreamUpdatesToAllProactorConnections(username, cred.acl_categories);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. maybe you want to do this only if this is not a new user?
  2. do we want to have a lock here so no 2 set user commands are done at the same time because if we have 2 set commands for the same user at the same time what will be set in the connections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. maybe you want to do this only if this is not a new user?
    That's true we can do this
  2. That's a good catch! I already have a pattern for this 😜

(*cntx)->SendOk();
};

Expand All @@ -171,8 +196,15 @@ void AclFamily::SetUser(CmdArgList args, ConnectionContext* cntx) {

using CI = dfly::CommandId;

#define HFUNC(x) SetHandler(&AclFamily::x)
using MemberFunc = void (AclFamily::*)(CmdArgList args, ConnectionContext* cntx);

inline CommandId::Handler HandlerFunc(AclFamily* acl, MemberFunc f) {
return [=](CmdArgList args, ConnectionContext* cntx) { return (acl->*f)(args, cntx); };
}

#define HFUNC(x) SetHandler(HandlerFunc(this, &AclFamily::x))

constexpr uint32_t kAcl = acl::CONNECTION;
constexpr uint32_t kList = acl::ADMIN | acl::SLOW | acl::DANGEROUS;
constexpr uint32_t kSetUser = acl::ADMIN | acl::SLOW | acl::DANGEROUS;

Expand All @@ -184,7 +216,7 @@ constexpr uint32_t kSetUser = acl::ADMIN | acl::SLOW | acl::DANGEROUS;
// easy to handle that case explicitly in `DispatchCommand`.

void AclFamily::Register(dfly::CommandRegistry* registry) {
*registry << CI{"ACL", CO::NOSCRIPT | CO::LOADING, 0, 0, 0, 0, acl::kList}.HFUNC(Acl);
*registry << CI{"ACL", CO::NOSCRIPT | CO::LOADING, 0, 0, 0, 0, acl::kAcl}.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}
Expand All @@ -193,4 +225,8 @@ void AclFamily::Register(dfly::CommandRegistry* registry) {

#undef HFUNC

void AclFamily::Init(std::vector<facade::Listener*> listeners) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not const& and than copy here?

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 like to delegate this to the callers. An example is:

std::vector<...> tmp;
Init(std::move(tmp)); //init moves internally so in total 2 moves

If we do what you suggest we end up with 1 move(fine const& will work) and then a deep copy.

Generally, I like to:

  1. Accept by value an delegate the choice to the user.
  2. Accept by && only if the function is supposed to be used as a sink.

listeners_ = std::move(listeners);
}

} // namespace dfly::acl
27 changes: 22 additions & 5 deletions src/server/acl/acl_family.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,39 @@

#pragma once

#include <cstdint>
#include <string_view>
#include <vector>

#include "facade/dragonfly_listener.h"
#include "helio/util/proactor_pool.h"
#include "server/common.h"

namespace dfly {

class ConnectionContext;
class CommandRegistry;

namespace acl {

class AclFamily {
class AclFamily final {
public:
static void Register(CommandRegistry* registry);
explicit AclFamily(util::ProactorPool& pp);

void Register(CommandRegistry* registry);
void Init(std::vector<facade::Listener*> listeners);

private:
static void Acl(CmdArgList args, ConnectionContext* cntx);
static void List(CmdArgList args, ConnectionContext* cntx);
static void SetUser(CmdArgList args, ConnectionContext* cntx);
void Acl(CmdArgList args, ConnectionContext* cntx);
void List(CmdArgList args, ConnectionContext* cntx);
void SetUser(CmdArgList args, ConnectionContext* cntx);

// Helper function that updates all open connections and their
// respective ACL fields on all the available proactor threads
void StreamUpdatesToAllProactorConnections(std::string_view user, uint32_t update_cat);

std::vector<facade::Listener*> listeners_;
util::ProactorPool& pp_;
};

} // namespace acl
Expand Down
6 changes: 1 addition & 5 deletions src/server/acl/validator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,12 @@

#include "server/acl/validator.h"

#include "server/server_state.h"

namespace dfly::acl {

[[nodiscard]] bool IsUserAllowedToInvokeCommand(const ConnectionContext& cntx,
const facade::CommandId& id) {
auto& registry = *ServerState::tlocal()->user_registry;
auto credentials = registry.GetCredentials(cntx.authed_username);
auto command_credentials = id.acl_categories();
return (credentials.acl_categories & command_credentials) != 0;
return (cntx.acl_categories & command_credentials) != 0;
}

} // namespace dfly::acl
2 changes: 0 additions & 2 deletions src/server/acl/validator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

#pragma once

#include <string_view>

#include "facade/command_id.h"
#include "server/conn_context.h"

Expand Down
2 changes: 2 additions & 0 deletions src/server/conn_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <absl/container/fixed_array.h>
#include <absl/container/flat_hash_set.h>

#include "acl/acl_commands_def.h"
#include "core/fibers.h"
#include "facade/conn_context.h"
#include "facade/reply_capture.h"
Expand Down Expand Up @@ -197,6 +198,7 @@ class ConnectionContext : public facade::ConnectionContext {
FlowInfo* replication_flow;

std::string authed_username{"default"};
uint32_t acl_categories{acl::ALL};

private:
void EnableMonitoring(bool enable) {
Expand Down
5 changes: 3 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 @@ -608,7 +608,7 @@ optional<ShardId> GetRemoteShardToRunAt(const Transaction& tx) {
} // namespace

Service::Service(ProactorPool* pp)
: pp_(*pp), server_family_(this), cluster_family_(&server_family_) {
: pp_(*pp), acl_family_(*pp), server_family_(this), cluster_family_(&server_family_) {
CHECK(pp);
CHECK(shard_set == NULL);

Expand Down Expand Up @@ -665,6 +665,7 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>

shard_set->Init(shard_num, !opts.disable_time_update);

acl_family_.Init(listeners);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we enforce ACL on all listeners? I think we should only do it for the main listener (and not uds and admin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, you are right.

request_latency_usec.Init(&pp_);
StringFamily::Init(&pp_);
GenericFamily::Init(&pp_);
Expand Down Expand Up @@ -2127,7 +2128,7 @@ void Service::RegisterCommands() {
BitOpsFamily::Register(&registry_);
HllFamily::Register(&registry_);
SearchFamily::Register(&registry_);
acl::AclFamily::Register(&registry_);
acl_family_.Register(&registry_);

server_family_.Register(&registry_);
cluster_family_.Register(&registry_);
Expand Down
2 changes: 2 additions & 0 deletions src/server/main_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/varz_value.h"
#include "core/interpreter.h"
#include "facade/service_interface.h"
#include "server/acl/acl_family.h"
#include "server/acl/user_registry.h"
#include "server/cluster/cluster_family.h"
#include "server/command_registry.h"
Expand Down Expand Up @@ -165,6 +166,7 @@ class Service : public facade::ServiceInterface {
util::ProactorPool& pp_;

acl::UserRegistry user_registry_;
acl::AclFamily acl_family_;
ServerFamily server_family_;
ClusterFamily cluster_family_;
CommandRegistry registry_;
Expand Down
2 changes: 2 additions & 0 deletions src/server/server_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,6 +1554,8 @@ void ServerFamily::Auth(CmdArgList args, ConnectionContext* cntx) {
auto is_authorized = registry->AuthUser(username, password);
if (is_authorized) {
cntx->authed_username = username;
auto cred = registry->GetCredentials(username);
cntx->acl_categories = cred.acl_categories;
return (*cntx)->SendOk();
}
return (*cntx)->SendError(absl::StrCat("Could not authorize user: ", username));
Expand Down