slipenkomk/valkey
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't disconnect all clients in ACL LOAD (#12171)

Browse Source 
Previous implementation would disconnect _all_ clients when running `ACL
LOAD`, which wasn't very useful.

This change brings the behavior in line with that of `ACL SETUSER`, `ACL
DELUSER`, in that only clients whose user is deleted or clients
subscribed to channels which they no longer have access to will be
disconnected.

---------

Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
This commit is contained in:
Slava Koyfman 2023-12-24 11:56:44 +02:00 committed by GitHub
parent 09e0d338f5
commit 20214b26a4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 188 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

186
src/acl.c
View File

@ -539,12 +539,6 @@ void ACLCopyUser(user *dst, user *src) {
}
}
/* Free all the users registered in the radix tree 'users' and free the
* radix tree itself. */
void ACLFreeUsersSet(rax *users) {
raxFreeWithCallback(users,(void(*)(void*))ACLFreeUserAndKillClients);
}
/* Given a command ID, this function set by reference 'word' and 'bit'
* so that user->allowed_commands[word] will address the right word
* where the corresponding bit for the provided ID is stored, and
@ -1909,29 +1903,26 @@ int ACLCheckAllPerm(client *c, int *idxptr) {
return ACLCheckAllUserCommandPerm(c->user, c->cmd, c->argv, c->argc, idxptr);
}
/* Check if the user's existing pub/sub clients violate the ACL pub/sub
* permissions specified via the upcoming argument, and kill them if so. */
void ACLKillPubsubClientsIfNeeded(user *new, user *original) {
/* Do nothing if there are no subscribers. */
if (!dictSize(server.pubsub_patterns) &&
!dictSize(server.pubsub_channels) &&
!dictSize(server.pubsubshard_channels))
return;
int totalSubscriptions(void) {
return dictSize(server.pubsub_patterns) +
dictSize(server.pubsub_channels) +
dictSize(server.pubsubshard_channels);
}
/* If 'new' can access all channels 'original' could then return NULL;
Otherwise return a list of channels that the new user can access */
list *getUpcomingChannelList(user *new, user *original) {
listIter li, lpi;
listNode *ln, *lpn;
robj *o;
int kill = 0;
/* First optimization is we check if any selector has all channel
* permissions. */
/* Optimization: we check if any selector has all channel permissions. */
listRewind(new->selectors,&li);
while((ln = listNext(&li))) {
aclSelector *s = (aclSelector *) listNodeValue(ln);
if (s->flags & SELECTOR_FLAG_ALLCHANNELS) return;
if (s->flags & SELECTOR_FLAG_ALLCHANNELS) return NULL;
}
/* Second optimization is to check if the new list of channels
/* Next, check if the new list of channels
* is a strict superset of the original. This is done by
* created an "upcoming" list of all channels that are in
* the new user and checking each of the existing channels
@ -1969,58 +1960,87 @@ void ACLKillPubsubClientsIfNeeded(user *new, user *original) {
if (match) {
/* All channels were matched, no need to kill clients. */
listRelease(upcoming);
return;
return NULL;
}
return upcoming;
}
/* Check if the client should be killed because it is subscribed to channels that were
* permitted in the past, are not in the `upcoming` channel list. */
int ACLShouldKillPubsubClient(client *c, list *upcoming) {
robj *o;
int kill = 0;
if (getClientType(c) == CLIENT_TYPE_PUBSUB) {
/* Check for pattern violations. */
dictIterator *di = dictGetIterator(c->pubsub_patterns);
dictEntry *de;
while (!kill && ((de = dictNext(di)) != NULL)) {
o = dictGetKey(de);
int res = ACLCheckChannelAgainstList(upcoming, o->ptr, sdslen(o->ptr), 1);
kill = (res == ACL_DENIED_CHANNEL);
}
dictReleaseIterator(di);
/* Check for channel violations. */
if (!kill) {
/* Check for global channels violation. */
di = dictGetIterator(c->pubsub_channels);
while (!kill && ((de = dictNext(di)) != NULL)) {
o = dictGetKey(de);
int res = ACLCheckChannelAgainstList(upcoming, o->ptr, sdslen(o->ptr), 0);
kill = (res == ACL_DENIED_CHANNEL);
}
dictReleaseIterator(di);
}
if (!kill) {
/* Check for shard channels violation. */
di = dictGetIterator(c->pubsubshard_channels);
while (!kill && ((de = dictNext(di)) != NULL)) {
o = dictGetKey(de);
int res = ACLCheckChannelAgainstList(upcoming, o->ptr, sdslen(o->ptr), 0);
kill = (res == ACL_DENIED_CHANNEL);
}
dictReleaseIterator(di);
}
if (kill) {
return 1;
}
}
return 0;
}
/* Check if the user's existing pub/sub clients violate the ACL pub/sub
* permissions specified via the upcoming argument, and kill them if so. */
void ACLKillPubsubClientsIfNeeded(user *new, user *original) {
/* Do nothing if there are no subscribers. */
if (totalSubscriptions() == 0)
return;
list *channels = getUpcomingChannelList(new, original);
/* If the new user's pubsub permissions are a strict superset of the original, return early. */
if (!channels)
return;
listIter li;
listNode *ln;
/* Permissions have changed, so we need to iterate through all
* the clients and disconnect those that are no longer valid.
* Scan all connected clients to find the user's pub/subs. */
listRewind(server.clients,&li);
while ((ln = listNext(&li)) != NULL) {
client *c = listNodeValue(ln);
kill = 0;
if (c->user == original && getClientType(c) == CLIENT_TYPE_PUBSUB) {
/* Check for pattern violations. */
dictIterator *di = dictGetIterator(c->pubsub_patterns);
dictEntry *de;
while (!kill && ((de = dictNext(di)) != NULL)) {
o = dictGetKey(de);
int res = ACLCheckChannelAgainstList(upcoming, o->ptr, sdslen(o->ptr), 1);
kill = (res == ACL_DENIED_CHANNEL);
}
dictReleaseIterator(di);
/* Check for channel violations. */
if (!kill) {
/* Check for global channels violation. */
di = dictGetIterator(c->pubsub_channels);
while (!kill && ((de = dictNext(di)) != NULL)) {
o = dictGetKey(de);
int res = ACLCheckChannelAgainstList(upcoming, o->ptr, sdslen(o->ptr), 0);
kill = (res == ACL_DENIED_CHANNEL);
}
dictReleaseIterator(di);
}
if (!kill) {
/* Check for shard channels violation. */
di = dictGetIterator(c->pubsubshard_channels);
while (!kill && ((de = dictNext(di)) != NULL)) {
o = dictGetKey(de);
int res = ACLCheckChannelAgainstList(upcoming, o->ptr, sdslen(o->ptr), 0);
kill = (res == ACL_DENIED_CHANNEL);
}
dictReleaseIterator(di);
}
/* Kill it. */
if (kill) {
freeClient(c);
}
}
if (c->user != original)
continue;
if (ACLShouldKillPubsubClient(c, channels))
freeClient(c);
}
listRelease(upcoming);
listRelease(channels);
}
/* =============================================================================
@ -2427,11 +2447,43 @@ sds ACLLoadFromFile(const char *filename) {
ACLFreeUser(new_default);
raxInsert(Users,(unsigned char*)"default",7,DefaultUser,NULL);
raxRemove(old_users,(unsigned char*)"default",7,NULL);
ACLFreeUsersSet(old_users);
/* If there are some subscribers, we need to check if we need to drop some clients. */
rax *user_channels = NULL;
if (totalSubscriptions() > 0) {
user_channels = raxNew();
}
listIter li;
listNode *ln;
listRewind(server.clients,&li);
while ((ln = listNext(&li)) != NULL) {
client *c = listNodeValue(ln);
user *original = c->user;
list *channels = NULL;
user *new = ACLGetUserByName(c->user->name, sdslen(c->user->name));
if (new && user_channels) {
if (!raxFind(user_channels, (unsigned char*)(new->name), sdslen(new->name), (void**)&channels)) {
channels = getUpcomingChannelList(new, original);
raxInsert(user_channels, (unsigned char*)(new->name), sdslen(new->name), channels, NULL);
}
}
/* When the new channel list is NULL, it means the new user's channel list is a superset of the old user's list. */
if (!new || (channels && ACLShouldKillPubsubClient(c, channels))) {
freeClient(c);
continue;
}
c->user = new;
}
if (user_channels)
raxFreeWithCallback(user_channels, (void(*)(void*))listRelease);
raxFreeWithCallback(old_users,(void(*)(void*))ACLFreeUser);
sdsfree(errors);
return NULL;
} else {
ACLFreeUsersSet(Users);
raxFreeWithCallback(Users,(void(*)(void*))ACLFreeUser);
Users = old_users;
errors = sdscat(errors,"WARNING: ACL errors detected, no change to the previously active ACL rules was performed");
return errors;

2
src/adlist.c
View File

@ -76,6 +76,8 @@ void listEmpty(list *list)
* This function can't fail. */
void listRelease(list *list)
{
if (!list)
return;
listEmpty(list);
zfree(list);
}

1
tests/assets/user.acl
View File

@ -1,3 +1,4 @@
user alice on allcommands allkeys &* >alice
user bob on -@all +@set +acl ~set* &* >bob
user doug on resetchannels &test +@all ~* >doug
user default on nopass ~* &* +@all

74
tests/unit/acl.tcl
View File

@ -1005,16 +1005,76 @@ start_server [list overrides [list "dir" $server_path "acl-pubsub-default" "allc
set e
} {*NOPERM*set*}
test {ACL LOAD only disconnects affected clients} {
reconnect
r ACL SETUSER doug on nopass resetchannels &test* +@all ~*
set rd1 [redis_deferring_client]
set rd2 [redis_deferring_client]
$rd1 AUTH alice alice
$rd1 read
$rd1 SUBSCRIBE test1
$rd1 read
$rd2 AUTH doug doug
$rd2 read
$rd2 SUBSCRIBE test1
$rd2 read
r ACL LOAD
r PUBLISH test1 test-message
# Permissions for 'alice' haven't changed, so they should still be connected
assert_match {*test-message*} [$rd1 read]
# 'doug' no longer has access to "test1" channel, so they should get disconnected
catch {$rd2 read} e
assert_match {*I/O error*} $e
$rd1 close
$rd2 close
}
test {ACL LOAD disconnects clients of deleted users} {
reconnect
r ACL SETUSER mortimer on >mortimer ~* &* +@all
set rd1 [redis_deferring_client]
set rd2 [redis_deferring_client]
$rd1 AUTH alice alice
$rd1 read
$rd1 SUBSCRIBE test
$rd1 read
$rd2 AUTH mortimer mortimer
$rd2 read
$rd2 SUBSCRIBE test
$rd2 read
r ACL LOAD
r PUBLISH test test-message
# Permissions for 'alice' haven't changed, so they should still be connected
assert_match {*test-message*} [$rd1 read]
# 'mortimer' has been deleted, so their client should get disconnected
catch {$rd2 read} e
assert_match {*I/O error*} $e
$rd1 close
$rd2 close
}
test {ACL load and save} {
r ACL setuser eve +get allkeys >eve on
r ACL save
# ACL load will free user and kill clients
r ACL load
catch {r ACL LIST} e
assert_match {*I/O error*} $e
reconnect
# Clients should not be disconnected since permissions haven't changed
r AUTH alice alice
r SET key value
r AUTH eve eve
@ -1028,12 +1088,10 @@ start_server [list overrides [list "dir" $server_path "acl-pubsub-default" "allc
r ACL setuser harry on nopass resetchannels &test +@all ~*
r ACL save
# ACL load will free user and kill clients
r ACL load
catch {r ACL LIST} e
assert_match {*I/O error*} $e
reconnect
# Clients should not be disconnected since permissions haven't changed
r AUTH harry anything
r publish test bar
catch {r publish test1 bar} e