Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACL behavior on keyless commands #370

Open
madolson opened this issue Apr 24, 2024 · 2 comments
Open

ACL behavior on keyless commands #370

madolson opened this issue Apr 24, 2024 · 2 comments

Comments

@madolson
Copy link
Member

madolson commented Apr 24, 2024

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, redis/redis#8152.

@PingXie
Copy link
Member

PingXie commented Apr 26, 2024

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
Copy link
Contributor

hpatro commented Apr 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants