valkey-io / valkey

A new project to resume development on the formerly open-source Redis project. We're calling it Valkey, since it's a twist on the key-value datastore.
https://valkey.io
Other
14.65k stars 520 forks source link

ACL behavior on keyless commands #370

Open madolson opened 2 months ago

madolson commented 2 months ago

There are 5 commands that don't declare keys and operate on the keyspace: SCAN, RANDOMKEY, KEYS, FLUSHALL, FLUSHDB. It would make sense to assume that executing these commands would require access to "all" keys. However, they don't behave that way. A user that has +@all nokeys can still execute FLUSHALL. This perhaps makes sense, but I think it's actually much weirder you could do +@readonly ~app1:* and be able to do a scan across the entire keyspace.

I see two paths forward:

  1. These five special commands can only be executed if you have all keys permissions. This is a backwards breaking change and seems like the most "reasonable" thing to change in Valkey 8.
  2. We introduce an @all-keys category, which we can recommend you explicitly remove. We could also allow modules to opt-in to this API functionality as well.

This has been reported a few times on the old Redis thread, https://github.com/redis/redis/issues/8152.

PingXie commented 2 months ago

As much as I don't like it, I have to say that option 2 is safer. There is just no way to evaluate the risk of "unknown unknowns". That said, I still hope someone would come out and tell me that going with option 1 will actually break them.

Semantically speaking though, there might be a third option for SCAN, RANDOMKEY, and KEYS through (expensive) filtering but that wouldn't work for FLUSHALL and FLUSHDB.

hpatro commented 2 months ago

I think we should look these 5 commands in two different sense:

Set 1: SCAN, KEYS, RANDOMKEY Set 2: FLUSHALL, FLUSHDB

Regarding the option we should proceed with, I feel option 2 is the safer choice but it's confusing with allkeys key permission. How about @keyless/ @nokey category for set 1 and leave the set 2 as is.