As part of Redis 7 we introduced keyspces which marked each key as having a "read" or "write" flag. We are hoping to use this information for the new ACL v2 functionality, but there is some ambiguity that needs to be resolved. Whether a key is written to is pretty well defined, is it possible to mutate the data or not. However, there is ambiguity about what constitutes "reading from a key".

To help clarify the discussion, I'm going to split all of the key/value data into 3 parts. 1. Key information: The key string representation. 2. Value user data: The user supplied data stored in the value. This also includes TTL information, since it's explicitly provided by users. 3. Value metadata: The value type and metadata that is used to make user data available, but aren't directly supplied by users. Examples include dict entries for storing hashes and LRU/FLU data for handling evictions.

I think we have to assume that every operation has to have access to the key information, otherwise it wouldn't be able to look it up in the dictionary. So, I see two possible options for categorizing the remainder of the data.

With that background, I think we have 4 options for what we consider "reading" from a key. They are illustrated with a couple of commands.

Option 1: Any value data access counts as a read

  • LPUSH: Marked as read, since it needs to access metadata to push.
  • APPEND: Marked as read, since string length is metadata.
  • SINTERCARD: Marked as read, since set values must be read to compute

Option 2: Any value user data copied or returned counts as a read

  • LPUSH: Not marked as read, since it returns no value user data.
  • APPEND: Not marked as a read, since it does not return any of the user data.
  • SINTERCARD: Not marked as a read, since it does not return any of the user data, just an aggregate result.

Option 3: Any processing, storing, or returning of value user data counts as a read

  • LPUSH: Not marked as read, since it returns no value user data.
  • APPEND: Not marked as read, since length is not user data
  • SINTERCARD: Marked as a read, since it processes user data to compute the intersection.

Option 4: Any processing, storing, or returning of user data or metadata about user data counts as a read

  • LPUSH: Marked as read, since it returns the underlying length of the list after the push.
  • APPEND: Marked as a read, since it returns metadata (length) about the total string length.
  • SINTERCARD: Marked as a read, since it accesses user data to perform cardinality checks

I'm on the fence between option 3 and option 4. Option 4 seems the most correct, but it also places a lot of unnecessary restrictions on what counts as a read for ACLs. Intuitively it makes sense to me that APPEND and LPUSH shouldn't count as read commands and SINTERCARD should.

Misc

We also need to audit the all of the command specs to make sure they make sense. I noticed that LMOVE marked the first key as read + write, but does not also mark LPOP as read + write.

Comment From: madolson

@redis/core-team @guybe7 Would appreciate your input.

Comment From: oranagra

The problem here is probably not the read-only or the read+write combo, it's the write-only feature that's causing the trouble, right?

Maybe we should start by defining the purpose of write-only again, so it'l be easier to reason with. IIUC one common use case is many publishers can can modify a list containing sensitive information, so they should not be allowed to read data put there by other publishers. Another examples is replacing a password, you can override the password, but not read the old one.

So from that perspective a write-only access is aiming to avoid returning (or leaking in another way, like COPY to another key) data from the server to the unprivileged user. And i wanna remind us that we decided to ignore a similar requirement of preventing deletion of data (i.e we initially considered different flags for data insertion and data deletion).

I'm not entirely sure if you meant to separate server server metadata (LRU), from "metadata about the user data". i.e. LRU could be server metadata, but STRLEN, LLEN, SCARD, and TYPE, are all metadata about the user data (or did you consider these the data itself?). We could argue that we don't mind leaking these (list length and such) to the unprivileged user.

At first, i didn't like your use of the them "processing", and wanted to argue that SCARD and SINTERCARD should have the same flags. i.e. if SCARD is permitted to read the number of elements of a set to a user, then SINTERCARD could to that too. and that it doesn't matter if for the purpose of LLEN we can just return a cached length, or if we had to "process" the list to count it (what matters is what's returned, and not the work we needed to do to return it). However, discussing this with @yoav-steinberg he pointed out that user can use SINTERCARD to discover the contents of a key by writing to another one and issuing SINTERCARD (i'll say that this means we "leak" the data, not the metadata about the data). For the sake of argument, if we had to use strlen() in STRLEN, (to match the "processing" act), would it mean the same? i don't think so. so maybe it's just because SINTERCARD is a multi-key command? Anyway, i still don't like the term "processing". one could argue that if we don't process the data, we won't leak it, but i can imagine cases were we do process the data, but can't leak it (any dict lookup or modification or listpack insertion can be counted as "processing")

So, we're aiming for not requiring read privileges for LLEN, SCARD, STRLEN, TYPE, LPUSH, right? (they read or leak the metadata about the user data, but not the user data itself) But SINTERCARD (and also HSET!!) do require read since they could be used to "leak" data (not just metadata about the data)

The problem is that many redis commands leak data, in many we sought the opportunity for returning something other than "OK" which could be useful, and now we pay that price.

I think that if we go with option 4, we'll soon be required to add write-only variants for LPUSH, APPEND and others (and meanwhile, maybe people will create modules, or avoid using an incomplete feature). Even If we go with options 3, we'll be required to do the same, at least for HSET.

I see two ways around this: * One is by introducing HSET_WO (and maybe LPUSH_WO, depending on if we take option 3 or 4) * Another is to somehow add a generic mechanism that modifies the responses in some way so that they don't leak data. e.g. HSET could return the number of updated fields, rather than added fields (or maybe "OK"?). * The last is making a distinction between lower case r in the ACL rule, and an uppercase R, so that users can decide what they're ok to read (and leak)

bottom line:

Same as you, i don't define the key name as part of the data for read purpose, and i don't consider the metadata about the key, or metadata about the actual data as a concern. so KEYS, EXIST, OBJECT, LLEN, STRLEN, SCARD are all not considered read for that matter (won't be restricted if read is prohibited). I'm even willing to except TTL form read restriction (although it is explicitly specified by the user).

I'm but i'm afraid that even with that liberal approach, write-only configurations will be very limited since we'll have to block HSET (and there are no other ways to write to a hash).

We could for now take the write-only feature out, and only support read-only and read-write, but if we wanna support write-only, we'll need to either define that we don't prevent data leakage, or add some mechanism to restrict the verbose response from write commands.

P.s. Sadly, we recently added a GET argument to SET, which makes this command a read command too. Now we have to add a write-only variant (SET_WO, similarly to the BITFIELD_RO, SORT_RO commands)

Lastly, Regardless of ACL (what counts as read), i think it makes sense to have the key-specs flags more verbose and explicit, and have more than just read or write flags, since we don't know what they're gonna be used for (both inside redis, and by clients), so it would be nice to have more meta data there than just modifying it to fit the ACL specific needs. i'm not sure what that means, but from the above discussion (and all the compromises we're gonna make), it seems that we'll need to adjust the key-spec flags, so i wanna extend them, not just modify them.

Comment From: yossigo

This is what I was concerned about in the past, when I mentioned potential issues mapping an abstract operations model to what Redis commands do in practice.

I think we should be more obsessed with keeping things simple and be willing to give up the "completeness" of the model. For a start, we should ignore side channel issues. Even if we solved some of the obvious ones mentioned above, there will always be more (ask Intel...).

I lean towards completely avoiding "write" as a discrete operation and only refer to "read-only" vs "read-write". Because selectors also include specific commands, this can still be used to solve the problem of LMOVE and such where we want to specify the mutable and non-mutable keys.

If we end up with something simpler, I think it's OK to expect data to be modelled in certain way if certain policies need to be enforced. For example:

IIUC one common use case is many publishers can can modify a list containing sensitive information, so they should not be allowed to read data put there by other publishers.

In this case, the list can point to string keys which will hold the actual confidential value.

Another examples is replacing a password, you can override the password, but not read the old one.

We won't be able to use SET, but we could expect the secret to be stored in a hash and only allow HSET on these keys (or better yet, sent the user to RTFM about PBKDF2).

Comment From: madolson

@oranagra @yossigo I agree with a lot of what was mentioned. I suppose let me try to distill this to a recommendation we can agree/disagree on.

  • The write flag will be defined as indicating that the data stored in the key may be modified or created.
  • The read flag will be defined as indicating that the data stored in the key may will be accessed.
  • ACL key based permissions will be simplified to just include READ and READ+WRITE. This covers the most important use case.
  • In the future, we might add more flags to indicate different ways keys are accessed.

In the future I think we should work to include fine grain read/write permission (perhaps in a RC, or perhaps not). The "pure write commands", (DEL, COPY, RENAME, commands that don't need to read the current contents) should be rather easy to add. The different variants of reads (metadata/userdata) will likely be harder.

Comment From: oranagra

I'm sorry, i don't have anything constructive to suggest, just concerns, and maybe some perspective.

i wanna mention that in LMOVE, both keys are modified. the difference is that one of them is also being read from. so if we just have read-only and read+write, then both of LMOVE keys are flagged the same (with read+write).

i also wanna remind us that all of this started with a request for combining multiple rules for one user, in which we can grant some commands access to one group of keys, and other commands access to another group of keys, and that we wanna restrict the second group of commands access to the fist group of keys and vice versa.

and that the design in #9974 has both that (command based selectors), and also read/write based selectors. IIRC, the second part was meant to make the rule definition easier, but also meant to solve the multi-key command problem like SUNIONSTORE, and also LMOVE.

So just wanted to point out that: 1. even if we get that read/write thing wrong, users can still use the command selectors for many things. 2. if we drop write-only thing, we're not gonna solve the problem of LMOVE at all.

I'm not sure if the write-only use case is an important one, i see it was raised in the discussion at a rather stage. @theDogOfPavlov wrote this:

I can see that there may be a need for the ability to write to a key, but not read from it. Say multiple nodes sending sensitive data to a list, and the nodes shouldn't be able to see each others output.

I also wanna quote myself saying:

This is maybe another indication that the distinction between Write / Insert / Delete is too complex (we'll either have to give that up, or modify the keyspecs metadata, before it's too late to change).

I do still think i wanna give as much metadata as i can to the key-spec flags, even if we don't use it (yet).

Comment From: madolson

Based off the conversation we had with the core group, we decided that we are going to come up with a special keyspec flag for reads that will use for determining permissions. This may be independent of other types of reads.

We still want to come up with a list of all the useful flags that we are currently aware of, so that they don't change later. The current proposed keyspec flags: * write: Modifies the data located in the key. * delete: Explicitly deletes the content within the key, may not delete all data within the key. (Example, SPOP and DEL) * upsert: Adds or updates data to the value, but doesn't delete it. Hard to differentiate these two, since XADD may insert or overwrite) (Example, SADD and APPEND). * read: Accesses the data located in the key. (examples, GET and LPUSH but not DEL) * data-read: Returns user data located in the key, but excludes metadata access. (example, LPOP but not LPUSH) We'll use this for ACL.

Once we have a comprehensive list of flags, we will go and apply them to the commands. Please comment with others.

Comment From: oranagra

I went over it again, and here's what i propose to do with key-spec flags:

/* Key-spec flags *
 * -------------- */
/* The following refer what the command actually does with the value or metadata
 * of the key, and not necessarily the user data or how it affects it.
 * Each key-spec may must have exaclty one of these. Any operation that's not
 * distinctly deletion, overwrite or read-only would be marked as RW. */
#define CMD_KEY_RO (1ULL<<0)     /* Read-Only - Reads the value of the key, but
                                  * doesn't necessarily returns it. */
#define CMD_KEY_RW (1ULL<<1)     /* Read-Write - Modifies the data stored in the
                                  * value of the key or its metadata. */
#define CMD_KEY_OW (1ULL<<2)     /* Overwrite - Overwrites the data stored in
                                  * the value of the key. */
#define CMD_KEY_RM (1ULL<<3)     /* Deletes the key. */
/* The follwing refer to user data inside the value of the key, not the metadata
 * like LRU, type, cardinality. It refers to the logical operation on the user's
 * data (actual input strings / TTL), being used / returned / copied / changed,
 * It doesn't refer to modification or returning of metadata (like type, count,
 * presence of data). Any write that's not INSERT or DELETE, would be an UPADTE.
 * Each key-spec may have one of the writes with or without access, or none: */
#define CMD_KEY_ACCESS (1ULL<<4) /* Returns, copies or uses the user data from
                                  * the value of the key. */
#define CMD_KEY_UPDATE (1ULL<<5) /* Updates data to the value, new value may
                                  * depend on the old value. */
#define CMD_KEY_INSERT (1ULL<<6) /* Adds data to the value with no chance of,
                                  * modification or deletion of existing data. */
#define CMD_KEY_DELETE (1ULL<<7) /* Explicitly deletes some content
                                  * from the value of the key. */

I went over all the commands i could find being mentioned in the various related discussions, and here's how i think they would be mapped: | Spec | RO | RW | OW | RM | ACCESS | UPDATE | INSERT | DELETE | comments | | --------------- | -- | -- | -- | -- | ------ | ------ | ------ | ------ | ----------------------------------------------------- | | GET | x | | | | x | | | | | | SET | | x | | | x | x | | | RW and ACCESS due to GET | | SET_WO | | | x | | | x | | | write-only SET, if we add one | | GETEX | | x | | | x | x | | | writing to ttl | | GETDEL | | x | | | x | | | x | | | DEL | | | | x | | | | x | | | TYPE | x | | | | | | | | no user data fags | | EXIST | | | | | | | | | no flags - no access do data | | OBJECT | x | | | | | | | | no user data fags | | EXPIRE | | x | | | | x | | | ttl is user data, and the value can get deleted | | TTL | x | | | | x | | | | ttl is user data | | STRLEN | x | | | | | | | | length is not user data (string) | | INCR | | x | | | x | x | | | | | SETNX | | | x | | | x | | | | | APPEND | | x | | | | | x | | | | SCARD | x | | | | | | | | cardinality is not user data | | SINTERCARD | x | | | | | | | | cardinality is not user data | | SADD | | x | | | | x | | | | | ZADD | | x | | | | x | | | | | LPUSH | | x | | | | | x | | | | LPOP | | x | | | x | | | x | | | SPOP | | x | | | x | | | x | | | XADD | | x | | | | x | | | auto trimming makes this an update rather than insert | | LLEN | x | | | | | | | | length is not user data | | BITFIELD | | x | | | x | x | | | | | BITFIELD_RO | x | | | | x | | | | | | ZRANGE | x | | | | x | | | | | | HSET | | x | | | | x | | | | | HDEL | | x | | | | | | x | | | HMSET | | x | | | | x | | | overrides, means it's an update not insert | | EVAL | | x | | | x | x | | | we don't know, so default is RW + ACCESS | | EVAL_RO | x | | | | x | | | | we don't know (could be LLEN), so default is access | | XTRIM | | x | | | | | | x | | | FPADD | | x | | | | x | | | | | SORT src | x | | | | x | | | | | | SORT dst | | | x | | | x | | | | | SORT_RO | x | | | | x | | | | | | LPUSHX | | x | | | | | x | | | | SREM | | x | | | | | | x | | | ZPOPMIN | | x | | | x | | | x | | | ZREM | | x | | | | | | x | | | MSETNX | | | x | | | x | | | | | HSETNX | | x | | | | x | | | | | LMPOP | | x | | | x | | | x | | | COPY src | x | | | | x | | | | src unmodified | | CPOY dst | | | x | | | x | | | can override dest | | RENAME src | | x | | | x | | | x | src modified (unlike COPY) | | RENAME dst | | | x | | | x | | | can override dest | | LMOVE src | | x | | | x | | | x | | | LMOVE dst | | x | | | | | x | | unlike RENAME, can't override dest | | SINTERSTORE src | x | | | | x | | | | | | SINTERSTORE dst | | | x | | | x | | | | | ZRANGESTORE src | x | | | | x | | | | | | ZRANGESTORE dst | | | x | | | x | | | | | BITOP src | x | | | | x | | | | | | BITOP dst | | | x | | | x | | | | | PFMERGE src | x | | | | x | | | | | | PFMERGE dst | | | x | | | x | | | | | CLIENT | | | | | | | | | no keys | | DBSIZE | | | | | | | | | no keys | | SCRIPT | | | | | | | | | no keys | | FLUSHDB | | | | | | | | | no keys | | KEYS | | | | | | | | | no keys |

Comment From: oranagra

i've pushed a PR with this change applied to all commands: #10122 was a little bit unsure about these:

Spec RO RW OW RM ACCESS UPDATE INSERT DELETE comment
HSTRLEN x doesn't read user data, like STRLEN, LLEN
HEXISTS x doesn't read user data, like EXISTS
PFCOUNT x x doesn't read user data? like SCARD
SMOVE src x x x maybe another interesting case to discuss
SMOVE dst x x same
XGROUP CREATE x x writes to user data?
XGROUP SETID x x writes to user data?

Comment From: madolson

I'm mostly onboard with these flags. Explicitly defining the two separate categories makes a lot of sense to me.

This might just be mis-categorizations, but I don't quite follow update vs insert. Two examples are SADD and SETNEX, neither of which can update the existing data in a key, but you mark them as update.

I agree that the XGROUP commands do write to customer data.

I think PFCount should be marked the same as SCARD, and not be marked as ACCESS.

Perhaps we should add an access_metadata flag? Make it clear that the command is accessing the key but not returning user data.

Comment From: oranagra

@madolson you are right, SADD, SETNX, and maybe even APPEND, should be marked with INSERT, not UPDATE.

The distinction i was trying to make is that if the command has no chance to modify existing data (possibly change, overwrite or delete it), then it is marked as INSERT. if it has no chance of adding data, just deleting data, it is marked as DELETE, and everything else (or if we're uncertain), then UPDATE is the default.

Same with RW, if we can't safely categorize something as RO, or RM, it becomes an RW (even if it is mostly read or mostly remove).

RW, and UPDATE are always a safe choice, since they don't grantee anything to the user (like not deleting data or not inserting data).

I think i'll change PFCOUNT too.. not sure if i like to define another ACCESS type.

My thinking was that ACL could choose to use these distinctions. or it can choose to treat INSERT and UPDATE (or maybe even DELETE) as the same.

Comment From: sundb

  1. Whether XADD should be INSERT, it does not modify other data in the stream.
  2. Why FCALL_RO is RW and UPDATE, should it be RO and ACCESS?
  3. Should SET be OW? Like SETEX.

Comment From: oranagra

  1. XADD is UPDATE because of the auto-trimming it can do (can possibly wipe out the stream)
  2. FCALL_RO seems to be mislabeled, should indeed be RO and ACCESS, i'll fix it.
  3. unlike MSET and SETEX, SET can't be OW, because it has the GET feature (and the first 4 flags are mutually exclusive). i'm considering to add write only version: #10125

Comment From: oranagra

Summary of discussion in a core-team meeting. We considered letting the new ACL mechanism provide distinction between RW, and INSERT / DELETE, and concluded that for now we wanna avoid that (i.e. the key-spec flag will have that detail, but ACL won't use it) We tried to conclude if we wanna expose these key-spec flags in COMMAND command, or keep the internal for redis for now. The reason is that we're not 100% yet certain about the flags definition, and might some day conclude we wanna define them differently. and we don't like to be obligated to the old definition and avoid introducing breaking changes.

we eventually decide to let them be exposed in COMMAND command as they are, but still reserve the right to drop / modify them in the future.

Considering that, i'm gonna merge the PR soon. If anyone finds a problem with the flagging of some commands, please report or open a PR.