valkey icon indicating copy to clipboard operation
valkey copied to clipboard

ACL behavior on keyless commands

Open madolson opened this issue 1 year ago • 2 comments

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.

madolson avatar Apr 24 '24 23:04 madolson

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.

PingXie avatar Apr 26 '24 23:04 PingXie

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

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

  • Set 1 here exposes actual key(s) (data) to an user and is comparable to row level security in DBMS world (https://www.postgresql.org/docs/current/ddl-rowsecurity.html). Hence, I think set 1 should be handled by key permissions.
  • Set 2 is command level operation which doesn't expose(s) any data to the user and decision should be taken on basis of command based permissions supported by the user. I would map it to https://www.postgresql.org/docs/current/sql-grant.html

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.

hpatro avatar Apr 29 '24 23:04 hpatro