ACL V2 - Selectors and key based permissions by madolson · Pull Request #9974 · redis/redis

madolson · 2021-12-21T06:42:13Z

Implementation based off of #7291

Implementation details

This feature adds two major features to ACLs:

Key based permissions

We will introduce three sub-permissions on keys, read + write. The goal here is to make it easy to restrict specific types of operations on key values and to allow defining permissions that work with modules. This functionality also lets you reason about "future proofing operations". Key based permissions do not replace command permissions.

~<pattern> is just a shorthand for full permission on the pattern.
%(R|W)~<pattern> will grant the specified permissions for that pattern.
To reset, use resetkeys just as before. Patterns are merged within a selector.
%(R|W~*) is valid syntax, and maps to granting the specified access to the entire keyspace.

Multiple selectors

A selector is a set of allowed commands + first args + a set of key patterns that match against Redis commands. Each user can now have 1 or more selectors, and as long as 1 selector matches against the user, the command will be allowed. We are introducing the concept of the "root permissions" which is the selector that is applied to users when they are created. This selector is mutable in order to be maximally backwards compatible.

All new selectors by default are -@ALL and resetkeys. Selectors are not currently deduplicated if they are identical, but could be in the future.

All created selectors are immutable after initialization. The only way to update them is with a new keyword, clearselectors. Therefor to make minor changes requires fully resetting them and adding a new ones. Calling clearselectors does not remove permissions from the root segment. This dramatically reduces the chance of accidentally updating a selector to something you don't expect, since you are being explicit about the new state.

With selectors, you still only need to have channel access to maintain access for subscribe. We aggregate all of the allowed channels across all selectors when doing verification you can still access the channel.

A selector is defined by a set of ACL operations surrounded by parentheses. (e.g. (+@all ~*). The canonical form of a selector is that it should be a single string that starts with ( and ends with ) with no spacing between the boundary characters and the ACL operators. This is how it will be written out in ACL LIST. However, while testing I found it was easy to mess up, and it's a little weird in the ACL file. So Redis will try to transform selectors into the canonical form by stripping whitespace and searching for valid selectors across arguments. This applies to startup args, ACL list, config file, and ACL SETUSER. This code isn't strictly needed, but I think it's a nice usability issue.

ACL DRYRUN command

A new dryrun command was added that allows users to "test" whether or not a given user will be able to execute a command. I was using this mostly for testing, but Itamer suggested I commit it. It also makes some tests much easier to orchestrate.

Syntax

ACL DRYRUN <username> <command> [<arg> ...]

Returns:
OK: The command can be executed.
Bulk String: The error code for the command.
Errors on invalid command or user names.

Module APIs

The RM_CheckKeyPermission API now also takes in a flag argument describing how the key is going to be modified.

ACL List

Nothing specifically interesting here, key permissions and selectors are included naturally here.

ACL GETUSER

Two changes have been made to the getuser response. The first is the new selector field, which was mentioned earlier. The second is that the GETUSER now responds with DSL for keys and channels. This was a required decision for keys, since they now had more precise definitions. I also updated channels since now everything is using DSL as opposed to channels being the one thing that wasn't.
Note that there's a breaking change here!

Example:

>acl list
1) "user foo off %R~foo1 %W~bar2 ~whatever resetchannels &channel1 -@all +get (%R~selector1 &* -@all +get)"
> acl getuser foo
 1) "flags"
 2) 1) "off"
 3) "passwords"
 4) (empty array)
 5) "commands"
 6) "-@all +get"
 7) "keys"
 8) "%R~foo1 %W~bar2 ~whatever"
 9) "channels"
10) "&channel1"
11) "selectors"
12) 1) 1) "commands"
       2) "-@all +get"
       3) "keys"
       4) "%R~selector1"
       5) "channels"
       6) "&*"
    2) 1) "commands"
       2) "-@all"
       3) "keys"
       4) ""
       5) "channels"
       6) "&*"

Keyspecs and key references

The getKeySpecs functionality returns a keyReference instead of just a position. This is used so that the ACLs can fetch the flags corresponding to the key.

The set/get problem

Set is currently marked as R+W command, because it can also do GET on the data. This pops in a couple of other commands that have both RW flags even though they normally only do one, this was the result of us building complex commands. I just wanted to raise this as a downside. This seems like a gap, but my guess is it's not worth fixing now.

I don't believe strongly this needs to be solved right now. It would require another big push into updating the keyspecs to have multiple different variants with different keys. This is being investigated in #10040.

Related conversations

Sort out the mess with read keys on keyspecs #10040
Implement a dryrun command for testing
Change ACL permissions for sort/sort_ro
Probably worth doing some performance testing
create redis-doc PR (including new sub-command) Documentation for ACL V2 + ACL DRYRUN redis-doc#1756
Modules + channel keyspecs (Sort out the mess of modules, channels, keyspecs, and ACLs #10156)

oranagra · 2021-12-21T15:25:31Z

LPUSH returns metadata when it returns the length of the list.
SCARD processes the data to return the cardinality.

I'm a bit uncomfortable with calling list length "metadata" in LPUSH, and the cardinality in SCARD "data"
in that sense is LLEN data or metadata?

I think we should clearly state where INCR, and SETNX falls into, and maybe even a hypothetical case where INCR would have returned +OK (i.e the new data is a derivative of the existing one).
in that case (ignoring the return value), if we mark INCR as R+W, would that mean that APPEND is R+W too?

RENAME: currently marks both keys as write, now the first key will also be a read.

i think that in the past, the read / write command flags where about getting data out of redis (i.e. that's why RENAME is read), but now for ACL and keyspace restrictions, we must consider the source key as a read one.
so the key-spec flags can say the first key is has read flag, even if the command flag doesn't have the read flag.

LPOP: Currently marks the key as only write. (This is also inconsistent with LMOVE, which marks the popped key as read and write)

I think this is just a bug (missing read flag).
P.s. in the distinct the r meant "will never modify the key space" (so SELECT had it too), and in v3.2 i pushed to clean this and change the meaning of the flag.
But it still refers to keyspace, i.e. dbsize is read.

The set/get problem

That's an interesting downside (maybe not too late to change).
another bad option is to add a "write-only" variant, like we have for BITFIELD_RO.
I'm not sure what do do. i'm uncomfortable with changing it now, but also think it's a serious limitation.
maybe the way out is to perform some ACL permission checks inside the command proc itself?
i.e. it'll not get blocked by processCommand, and instead fail internally?

Modules and key based permissions

Regarding ACLCheckKey, let's make that change ASAP (API not released yet)

src/acl.c

src/db.c

src/acl.c

tests/unit/acl-v2.tcl

oranagra · 2021-12-23T20:33:50Z

I'm sorry. I was so happy that I finally finished reviewing the code, that I posted my comments without a summary.
all in all it looks great.
I think most of the comments are minor (or a few major ones that are easy to resolve).
I have a feeling that the tests should be enhanced, maybe test some odd commands and corner cases. As well as ACL SAVE and LOAD

madolson · 2021-12-23T20:39:51Z

@oranagra Thanks for looking through, it's not as polished as I normally like to put up PRs but knew I was going to be busy this week, so probably worth getting feedback. Tests was one thing I mentioned needs more work, so I'll definitely revamp them.

yossigo

@madolson Looks good to me. Did not do a very deep code review but I did play with that bit and it's cool.

Do you want to update the top comment with everything you have in mind that's still pending? I imagine there's a long tail and maybe we can parallelize this work somehow.

src/server.h

itamarhaber · 2021-12-30T18:49:01Z

As a chore/aside to this, would it make/be against common sense to add a capability to introspect the effective ACL permissions for a given user?

Ideally, this would be supported by an interface that, for a given user, would:

Report the verbatim allowed commands (and subcommand)
Report the allowed key patterns
Optionally, allow a "dry run" to test the possible ability to execute a specific command in terms the context's permissions alone

Mock API:

redis> HELLO 3 AUTH foo bar
OK
redis> COMMAND ALLOWED
1# "ping" => null
2# "echo" => null
3# "client|myid" => null
4# "get" =>
   1) "foo:*"
redis> COMMAND ALLOWED default
(error) NOPERM ...
redis> AUTH default ""
OK
redis> COMMAND ALLOWED foo
....

# Optional part
redis> AUTH foo bar
OK
redis> COMMAND DRYRUN get bar:123
(error) NOPERM ...
redis> COMMAND DRYRUN get foo:123
OK

madolson · 2022-01-02T05:56:26Z

@itamarhaber I have a dry-run command actually implemented locally for testing, I've been flip flopping if it's worth committing. My variant has the form:

ACL DRYRUN USER <command> <args>

Will add it as a follow-up. Not sure about the super detailed listing of commands though.

New test

madolson · 2022-01-13T06:48:16Z

NOTE: I didn't fix all the db.c stuff, I still need to spend some more time to fully understand the right way to fetch the keyspecs. However, I don't think the prevents reviewing the rest of the decisions.

oranagra · 2022-01-13T13:43:25Z

@madolson i'd rather avoid rebase and force-push (please just merge from unstable), it makes it hard to review changes.
if we have to rebase (like in the original functions PR that was merged without squash), we must use a separate force-push for the rebase from any other commit pushes.

anyway, for now, i'll assume the fist commit of this PR is identical to what i already reviewed. and i should review the two after it. right?

oranagra

only looked at the narrow diff of the last two commits, so might have comments that are out of context or may be missing something.
didn't review the tests.

redis.conf

src/acl.c

src/db.c

madolson · 2022-01-13T20:23:28Z

@oranagra sorry about the rebase, I wanted to pull in the changes from the pubsub v2 PR which included shard channels. There was quite a bit of changes all over the place, so I just decided to rebase. I normally agree, and wait until the end to rebase.

redis.conf

src/acl.c

oranagra

minor comments on last commit

src/db.c

src/acl.c

src/db.c

- getKeysUsingKeySpecs avoids argv access violation in case keynum index is out of range - doesCommandHaveKeys used getkeys_proc without checking that it's not a module command - doesCommandHaveKeys support modules that declared keys via CMD_MODULE_GETKEYS - doesCommandHaveKeys skips keyspecs that are marked with CHANNEL - doesCommandHaveChannels uses key-specs for shard channels, will make it possible for modules to join that game other minor things: - getKeysUsingKeySpecs - unify error handling in a goto label - ACLSelectorCheckKey rename flags to indicate they're keyspec flags and ACL's - ACLDoesCommandHaveChannels moved to db.c (next to doesCommandHaveKeys) - ACLCheckAllUserCommandPerm has to modify both error output parameters together (we can't have one come from one error and the other from another) - aclCommand has multiple if-else cases. name the goto label appropriately (specific to one of them)

oranagra · 2022-01-20T14:25:46Z

@madolson i've reviewed the whole thing again top to bottom, found a few bugs and fixed them, please CR the last commit.

i've also re-run the tests with the code that asserts both key extraction functions give the same results (put the assert in processCommand, since putting it in call will miss many of the test suite calls)

I have a feeling that the Test various odd commands for key permissions could be extended for more edge cases (we don't have anything that verifies the keyspecs and flags), but i don't wanna dive into it now.

I think this is ready for merge.

src/acl.c

hpatro · 2022-01-20T10:29:26Z

src/acl.c

-    listRelease(u->patterns);
-    listRelease(u->channels);
-    ACLResetFirstArgs(u);
+    listRelease(u->selectors);


Don't we need to privately free each selector first via ACLFreeSelector ?

No, listRelease will call the FreeMethod automatically, which is ACLFreeSelector.

src/acl.c

hpatro · 2022-01-20T10:36:38Z

src/acl.c

+                if (toupper(op[offset]) == 'R' && !(flags & ACL_READ_PERMISSION)) {
+                    flags |= ACL_READ_PERMISSION;
+                } else if (toupper(op[offset]) == 'W' && !(flags & ACL_WRITE_PERMISSION)) {
+                    flags |= ACL_WRITE_PERMISSION;


Why do we need to the flag check if at the end there is only one bit operation ?

To prevent having something like '%~WWWWWWWW' be accepted, it's just tighter input validation.

src/acl.c

hpatro · 2022-01-20T12:11:21Z

src/acl.c

+ * ACL_DENIED_CHANNEL. */
+static int ACLSelectorCheckPubsubArguments(aclSelector *s, robj **argv, int idx, int count, int literal, int *idxptr) {
+    for (int j = idx; j < idx+count; j++) {
+        if (ACLCheckChannelAgainstList(s->channels, argv[j]->ptr, sdslen(argv[j]->ptr), literal != ACL_OK)) {


Do we split at 80 chars per line ?

I don't split, and I've seen others not explicitly try to split it as well, when it doesn't really help readability to split.

i think it damages readability.

splitting at arbitrary position (offset, rather than a logical one).

adds more lines so we get less lines fit in our screen.

these days people have very wide monitors (and vertical screen space is needed, and there's an abundance of unused horizontal space)

madolson · 2022-01-20T17:05:57Z

src/db.c

+
+/* Returns if a given command may possibly access channels. For this context. */
+int doesCommandHaveChannels(struct redisCommand *cmd) {
+    return (getAllKeySpecsFlags(cmd, 0) & CMD_KEY_CHANNEL) || /* has at least one key-spec marked as CHANNEL */


@oranagra This grabs SUNSUBSCRIBE as well, which we don't want. I'm going to add an explicit check for that if that works. I'm also going to test this.

Actually, this is missing other support for modules + channels that was also removed. We can add it back, but the functionality isn't here right now.

I'd still argue this function belongs in db.c next to the other one.
Also, even if modules can't yet use it (so my commit comment was false), I think conceptually it's nicer to count on key spec flags (or any other flags), rather than explicitly list function names.
I.e. The transition we're making in other places like processCommand, to stop maintaining lists of functions, and instead define their behavior in flags.

* Minor renaming * Removed refactoring for "get all channels" since we don't have the corresponding code to let modules use it

madolson · 2022-01-20T17:38:50Z

@oranagra Just wanted to clarify the shard-channel keyspec stuff. In an earlier revision there was code to support both channels and sharded-channels in the keyspecs, but there wasn't clarity of it we wanted to do that, so I removed support for both. Right now modules have to do their own manual ACL checks for channels and there is an item in the follow up list to think this behavior through more.

The issues that I saw with modules support:

Should modules be able to also exempt themselves from ACL checks, like UNSUBSCRIBE and SUNSUBSCRIBE does.
Should we be able to differentiate channels and shard channels in key specs so modules can define either. Shard channels route clients while regular channels can be sent to any node (for stuff like keyspec notifications).

oranagra · 2022-01-20T17:56:23Z

Let's discuss modules later.
What I didn't like (or actually didn't understand) in your previous attempt, was that you added key-spec flags for the old pubsub, but these commands don't declare key-specs (and I think they shouldn't, since it'll confuse cluster clients).
So it's only ACL that has that requirement, and I think ACL should be the one to handle that (not key-specs)
I don't like to see key-spec flags to suppose non-key-spec usages.

yossigo

No CR, but played a bit with the latest version - LGTM!

SET is a R+W command, because it can also do `GET` on the data. SET without GET, is a WRITE command. SET with GET, is a READ + WRITE command. In redis#9974, we added ACL to let users define write-only access. So when the user uses SET with GET option, and the user doesn't have the READ permission on the key, we need to reject it. In this commit, we add a `getkeys_proc` function to control key flags in SET command. We also add a new key spec flag means that some keys might have different flags depending on arguments. Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Oran Agra <[email protected]>

…ad-only ACL (#10148) SET is a R+W command, because it can also do `GET` on the data. SET without GET is a write-only command. SET with GET is a read+write command. In #9974, we added ACL to let users define write-only access. So when the user uses SET with GET option, and the user doesn't have the READ permission on the key, we need to reject it, but we rather not reject users with write-only permissions from using the SET command when they don't use GET. In this commit, we add a `getkeys_proc` function to control key flags in SET command. We also add a new key spec flag (VARIABLE_FLAGS) means that some keys might have different flags depending on arguments. We also handle BITFIELD command, add a `bitfieldGetKeys` function. BITFIELD GET is a READ ONLY command. BITFIELD SET or BITFIELD INCR are READ WRITE commands. Other changes: 1. SET GET was added in 6.2, add the missing since in set.json 2. Added tests to cover the changes in acl-v2.tcl 3. Fix some typos in server.h and cleanups in acl-v2.tcl Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Oran Agra <[email protected]>

oranagra mentioned this pull request Dec 21, 2021

ACL: a plan for meta-users to allow more complex ACLs without making the rules themselves more complex. #7291

Closed

oranagra linked an issue Dec 21, 2021 that may be closed by this pull request

ACL: a plan for meta-users to allow more complex ACLs without making the rules themselves more complex. #7291

Closed

oranagra mentioned this pull request Dec 21, 2021

ACL Role #9195

Closed

5 tasks

oranagra added 7.0-must-have state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository labels Dec 21, 2021

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 21, 2021

oranagra reviewed Dec 23, 2021

View reviewed changes

yossigo reviewed Dec 30, 2021

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

This was referenced Jan 2, 2022

Redis Functions Flags #10025

Closed

Sort out the mess with read keys on keyspecs #10040

Closed

madolson added 2 commits January 9, 2022 18:48

ACL V2 draft implementation

08f1810

New test

PR ready version

3f359d9

madolson force-pushed the acl-v2 branch from 74144c1 to 3f359d9 Compare January 13, 2022 05:53

madolson marked this pull request as ready for review January 13, 2022 06:47

madolson requested review from oranagra and yossigo January 13, 2022 06:48

Fixed some minor wording updates

9f424d4

oranagra reviewed Jan 13, 2022

View reviewed changes

Addressed some comments

7bd95a9

oranagra reviewed Jan 16, 2022

View reviewed changes

redis.conf Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

oranagra reviewed Jan 16, 2022

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

madolson requested review from itamarhaber and oranagra January 20, 2022 04:00

Fix segfault

d528823

oranagra added the breaking-change This change can potentially break existing application label Jan 20, 2022

oranagra added 2 commits January 20, 2022 11:54

Merge remote-tracking branch 'origin/unstable' into acl-v2

603dd92

oranagra approved these changes Jan 20, 2022

View reviewed changes

hpatro reviewed Jan 20, 2022

View reviewed changes

madolson commented Jan 20, 2022

View reviewed changes

Addressed follow up comments

35dfbd1

* Minor renaming * Removed refactoring for "get all channels" since we don't have the corresponding code to let modules use it

yossigo approved these changes Jan 20, 2022

View reviewed changes

madolson merged commit 55c81f2 into redis:unstable Jan 20, 2022

soloestoy mentioned this pull request Jan 21, 2022

fix missing transform from key-spec to acl permission #10159

Closed

This was referenced Jan 25, 2022

Address ACL V2 changes redis/jedis#2840

Closed

Address ACL V2 changes redis/jedis#2841

Merged

enjoy-binbin mentioned this pull request Jan 26, 2022

Allow SET without GET arg on write-only ACL. Allow BITFIELD GET on read-only ACL #10148

Merged

oranagra mentioned this pull request Jan 26, 2022

RedisModule_KeyAtPos and COMMAND GETKEYS should be able to handle key-spec flags. #10189

Closed

madolson mentioned this pull request Jan 27, 2022

Documentation for ACL V2 + ACL DRYRUN redis/redis-doc#1756

Merged

filipecosta90 mentioned this pull request Feb 24, 2022

[BUG] ZREVRANGE 50% slower after upgrading from 5.0.7 to 6.2.6 #10310

Closed

filipecosta90 mentioned this pull request Mar 21, 2022

5-7% Performance regression from v5 to v6.2 to unstable due to added features ( more visible on pipeline ) #10460

Open

filipecosta90 mentioned this pull request Jul 15, 2022

Performance degrade 7.0.3 vs 6.2.7 #10981

Closed

filipecosta90 mentioned this pull request Dec 13, 2023

[QUESTION]Performance degrade of GET command on version 7.x & 6.x compare with 5.x #12748

Open

Conversation

madolson commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation details

Key based permissions

Multiple selectors

ACL DRYRUN command

Module APIs

ACL List

ACL GETUSER

Keyspecs and key references

The set/get problem

Related conversations

Uh oh!

oranagra commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Dec 23, 2021

Uh oh!

madolson commented Dec 23, 2021

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itamarhaber commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Jan 2, 2022

Uh oh!

madolson commented Jan 13, 2022

Uh oh!

oranagra commented Jan 13, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented Jan 13, 2022

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jan 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

madolson commented Dec 21, 2021 •

edited

Loading

oranagra commented Dec 21, 2021 •

edited

Loading

itamarhaber commented Dec 30, 2021 •

edited

Loading