Uses "@dangerous" commands even when allowAdmin=false (default)

[seppo498573908457]

Hi, I've been working to set up a Redis cluster to serve multiple clients in our organization. The default set of allowed ACL commands that I set is +@all -@admin -@dangerous. When these clients connect, they cause ACL LOG entries for trying "INFO" and "CONFIG|GET" commands. These commands are @dangerous and can be used to resolve usernames and passwords. I also would like to avoid unnecessary warnings in the log. So in a way, this is a security issue.

I propose to not use any @dangerous commands for normal operation in any point, unless explicitly configured to do so. I'm sure you can make a few good connection string options to indicate such allowance and to compensate if you cannot use such commands. For example allowDangerous=(bool). For another example, if the client is read-only, it would be quite easy to set as a flag in the connection string.

For the very least, you should pinpoint the data that you need and use the least exposing functionality to get that data. For example, if you need info about the cluster, do not use the general "INFO", but a more specific "INFO CLUSTER" or "CLUSTER INFO". That could be an acceptable workaround and allowed me to add some specific commands to users.

[mgravell]

In the general case, we rely on that info extensively to configure the client for the environment, and CLUSTER and HELLO only apply in some cases. If you absolutely don't want INFO and CONFIG to run, you should be able to use whateverhost,$info=,$config= in your configuration string to disable them at the command map level.

[mgravell]

Please let us know whether the configuration-string (or command-map) approach meets your requirements.

[seppo498573908457]

I tested adding $info=,$config= into the connection string and that indeed stops use of info and config|get commands. But it also resulted in new strange ACL LOG entries.

My test user and connection string on Windows hosted Redis connectivity and basic key-value and pubsub test features:
testuser acl: user testuser on sanitize-payload #hash resetchannels ~testuser-* &testuser-* +@all -@admin -@dangerous
connection string: "six-cluster-hosts,ssl=true,user=testuser,password=mypass,channelPrefix=testuser-,$info=,$config="
I also set the key prefix with IDatabase.WithKeyPrefix(string) when handling key-values.

I used the ping feature of SE.Redis-library, I got the following ACL entries (some info snipped, but number of distinct entries is as-is):

1)  1) "count"
    2) (integer) 2
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "__Booksleeve_TieBreak"
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=86 fd=30 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=152 qbuf-free=20322 argv-mem=24 multi-mem=0 rbs=16384 rbp=16384 obl=1082 oll=0 omem=0 tot-mem=37800 events=r cmd=get user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"
2)  1) "count"
    2) (integer) 2
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "\xc9X4\x82\xc1\x81\x8aB\x85%\xd3o\xbfc\x8b."
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=86 fd=30 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=152 qbuf-free=20322 argv-mem=41 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=37849 events=r cmd=set user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"

I reset the log and tried key-value set and get. ACL LOG:

1)  1) "count"
    2) (integer) 4
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "__Booksleeve_TieBreak"
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=83 fd=29 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=41 qbuf-free=20433 argv-mem=24 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=37800 events=r cmd=get user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"
2)  1) "count"
    2) (integer) 2
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "\xb9\xe6\a\xd9\b\x9e2J\xb9<\xf8X\xd6\xe0\xa7\x84"
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=83 fd=29 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=111 qbuf-free=20363 argv-mem=41 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=37849 events=r cmd=set user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"
   20) (integer) 1760509566111
3)  1) "count"
    2) (integer) 2
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "\xfa\x0e\xe4s\xdc\xf1:A\x88e\x8en\xf1z\xee&"
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=79 fd=29 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=480 qbuf-free=19994 argv-mem=41 multi-mem=0 rbs=16384 rbp=16384 obl=96 oll=0 omem=0 tot-mem=37849 events=r cmd=set user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"

Again reset and pubsub subscribe and message, log:

1)  1) "count"
    2) (integer) 2
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "__Booksleeve_TieBreak"
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=68 fd=29 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=480 qbuf-free=19994 argv-mem=24 multi-mem=0 rbs=16384 rbp=16384 obl=1178 oll=0 omem=0 tot-mem=37800 events=r cmd=get user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"
2)  1) "count"
    2) (integer) 2
    3) "reason"
    4) "key"
    5) "context"
    6) "toplevel"
    7) "object"
    8) "'$p\xd9\x18\xb83N\x99\r\xc1\xacd9\x84a"
    9) "username"
   10) "testuser"
   13) "client-info"
   14) "id=68 fd=29 age=0 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=480 qbuf-free=19994 argv-mem=41 multi-mem=0 rbs=16384 rbp=16384 obl=96 oll=0 omem=0 tot-mem=37849 events=r cmd=set user=testuser redir=-1 resp=2 lib-name=SE.Redis lib-ver=2.8.31.52602"

This test was conducted on a test cluster with no other concurrent users or data. TieBreaks are understandable because of user permissions. Please explain the scrambled key usages. I have never seen those before.

[mgravell]

If you're not using the tie-breaker functionality, you can disable that by setting .TieBreaker = "" (code) or ...,tiebreaker= (config string).

The garbled keys ... I'm guessing this changes per run, in which case I think this is coming from the discovery logic, i.e. "is this a writable node, or a readonly replica"; without INFO to tell us that, we fall back to trying:

SET {guid} "replica_read_only" PX 1

i.e. try setting a random guid to a known traceable value with a near-instantaneous timeout. Vexingly, even HELLO 3 (RESP3 handshake) doesn't give us this information, so it is kinda hard to know what sort of server we're talking to without some kind of interaction.

Question: is this cluster, or non-cluster? If it is cluster, we can probably use the cluster topology for this information instead (and indeed, the SET approach isn't actually quite right as-is - it would need to consider slot logic)

---

for completeness only:

One additional possibility is that this is from the keep-alive/heartbeat. Is this a command connection, or a pub/sub connection? For command connections, the the heartbeat/keep-alive logic is:

• create a random token (this happens once only, basically: new guid, as bytes)
• are we allowed to use ECHO? if so: ECHO that token, and verify that we get that back
• if not: try the same with PING with that token
• if not: try using TIME (although we can't check the token in that case)
• finally, try an EXISTS against that token as a key, and check we get a response

so: you would need to have disabled ECHO, PING and TIME for that to be the case, which seems... unlikely.

(note to self: there may also be a niche cluster/-MOVED problem with that last option; I'll look into that)

However, on pub/sub connections, we have far fewer commands available - we use PING if possible (depends on the server version and command-map), falling back to UNSUBSCRIBE - as unsubscribing to a random guid (that we therefore aren't subscribed to) is a no-op that we can use to check health.

[mgravell]

crossref: #2970

[seppo498573908457]

It's a cluster of 3 masters and 3 replicas, total 6 nodes, end point of each is defined in the connection string that goes to the library.

In actual use we have multiple clients using the same cluster and these clients are not allowed to see each others data (for security and data integrity). So we are using key and channel prefixes that are named by the username and set permissions in Redis accordingly. This is mandatory, because accidental key-collision would corrupt data.

In SE.Redis library connection string, you can put the channelPrefix parameter, which seems to be honored at all times. There should also be "keyPrefix" parameter that you could use in cases like these.

ECHO, PING and TIME commands are allowed, I tested them individually with the same username as in above example.

And for keys and channels that start with __Booksleeve_* I'm a bit confused. Should all clients share common booksleeve key-value keys and pubsub channels or should each have their own? The aforementioned channelPrefix seems to cause the library use its own channel, but for key-values it doesn't seem to use a prefix, according to the ACL LOG provided in my previous post.

If I grant permissions to ~__Booksleeve_* I obviously no longer get ACL LOG entry from that, but the byte-keyed entries still keep coming.

In the previous post I have 3 scenarios: the librarys IServer.PingAsync() function, key-value database write and read using IDatabase, and pubsub channel subscription, post and capture of message using RedisChannel and ISubscriber. Each seemed to complete without problems, but generated ACL LOG entries. I cannot tell if they are command or pubsub connections, but I'd guess the first two are commands and the last one is pubsub.

[mgravell]

For your purposes, you don't need the tie-breaker; I would set that to "" - the cluster has a stronger vote. Unfortunately, at the time this is issued, we don't necessarily know yet whether we're talking to a cluster. I have updated #2970 to add a mention that - I'll see if we can nuke that automatically (especially under RESP3).

This would perhaps be pretty simple, so I would suggest that we look into this as the short-term mitigation here, i.e.

1. (me) verify that at least with RESP3, we know we're on a cluster (via HELLO) before we get to discovery
2. (me) skip this part of discovery for clusters, and publish a new library version (both tie-breaker and readonly-replica discovery)
3. (you) absorb the new library version, and make sure you've enabled RESP3

---

Re key prefix: this is currently configured via the WithKeyPrefix() API rather than the configuration API; this was to allow different scopes against the same connection (probably influenced heavily by the fact that IIRC the public part of Stack Overflow required this feature at one point; IIRC the enterprise/hosted version of SE had true tenant isolation). Also keep in mind that ACLs didn't exist in early versions of the Redis API ;p

So; a few options leap to mind here:

1. add a new KeyPrefix that applies at the top level, like ChannelPrefix, and would be combined with WithKeyPrefix (so if the config has "ABC/", the caller uses WithKeyPrefix("DEF/"), and they request key "GHI", we would actually fetch ABC/DEF/GHI), and ensure this is used from the discovery APIs
2. add a UniqueIdPattern API specifically for the niche discovery fallback scenarios

I think option 1 is far more enticing and useful. Thoughts @philon-msft @NickCraver ?

However: if we can remove the problem entirely for cluster environments on RESP3, that is even more enticing - zero API delta.

[mgravell]

Investigation results: the current HELLO is issued before the reader is active, and expects F+F logic; refactoring is possible but non-trivial

[seppo498573908457]

Thank you for your response.

I have updated my library to most recent version to date and now added protocol=resp3,tiebreaker= to my connection string per your suggestion. This is good info, we all want as smooth and fast operation as possible after all. I now get only the byte-keyed ACL LOG entries.

From my point of view "KeyPrefix" would be very good, that would be consistent with existing "ChannelPrefix" and easiest to configure.

The minimum number of nodes for the smallest possible Redis cluster is 6. The cluster does not start up with any less. When you got 6 or more nodes in your connection string, that's a good hint of a cluster. It's also acceptable if you required "cluster=true" in your connection string if that made things easier. I don't know how your connection procedure works, but HELLO command returns (among other things):

 9) "mode"
10) "cluster"
11) "role"
12) "master"

Or:

 9) "mode"
10) "cluster"
11) "role"
12) "replica"

So if you use that, you can poll each node for their role. You also have CLUSTER INFO and especially CLUSTER NODES commands that return info about the cluster. An example of CLUSTER NODES (with simplified IPs and hosts):

61e69b0c13a229c534c87460e04eace4a5e5570c 10.10.10.12:6379@16379,redis2.mydomain.com slave 2b5a4c428c0aebb34d1359d21d9cd6e32778c55d 0 1760535521044 7 connected
34e27336ed435ba3ca945b07bcb46e95f64e80cf 10.10.10.15:6379@16379,redis5.mydomain.com slave 44cee9ad96018ea206375e66305dd2480a4f8983 0 1760535522050 1 connected
2b5a4c428c0aebb34d1359d21d9cd6e32778c55d 10.10.10.16:6379@16379,redis6.mydomain.com master - 0 1760535520000 7 connected 5461-10922
44cee9ad96018ea206375e66305dd2480a4f8983 10.10.10.11:6379@16379,redis1.mydomain.com master - 0 1760535519031 1 connected 0-5460
22100b40647453d4a4fcbe5ca9e9e1efc844e4ab 10.10.10.14:6379@16379,redis4.mydomain.com slave 221a9f49dce63fc3a95d52c0b876ab50a1084804 0 1760535520036 3 connected
221a9f49dce63fc3a95d52c0b876ab50a1084804 10.10.10.13:6379@16379,redis3.mydomain.com myself,master - 0 0 3 connected 10923-16383

CLUSTER commands are not admin or dangerous, they are runnable with my normal user permissions. Whether this is acceptable method for your library is up to you of course.

I also whined to Redis about a few things that are related to this, I'll report back if they respond with relevant information.

[mgravell]

I am very familiar with HELLO and CLUSTER; the problem here is timing; we issue the handshake and discovery as a pipeline - we don't currently have access to the result of (say) HELLO at the time we need to make decisions about feature discovery, because it probably hasn't even reached the server yet. We could potentially change that, at the cost of some latency - it wouldn't be a huge problem - but: it requires a non-trivial amount of related changes, since the machinery to process replies hasn't been started at that point.