From Redis 6, if auth command fails on existing authenticated connection, it will continue to be authenticated on existing username. This is due to authenticated flag not being reset if there is invalid auth here: https://github.com/redis/redis/blob/6.0/src/acl.c#L2038. If a user with higher permissions is authenticated and wants to switch to a user with lower permissions and fails to auth. It will fail but fallback to user with higher permissions which can cause side effects.
To reproduce
Redis-cli -h Redis6-cachename auth user1 correctpassword OK acl whoami user1 auth user2 incorrectpassword WRONGPASS invalid username-password pair acl whoami user1
Expected behavior
Redis-cli -h Redis6-cachename auth user1 correctpassword OK acl whoami user1 auth user2 incorrectpassword WRONGPASS invalid username-password pair acl whoami NOAUTH authentication required
Additional information
For redis 5, authenticated flag is reset here: https://github.com/redis/redis/blob/5.0/src/server.c#L2954 Redis 5 repro:
Redis-cli -h Redis4-cachename auth correctpassword OK ping pong auth incorrectpassword Invalid password ping NOAUTH authentication required
Comment From: hwware
Hello, thanks for your report. I think in this case both can be acceptable ways to handle this case, it depends what the community wants to decide or has decided on how to tackle this. @oranagra do you think this is a bug or is it intended to work this way?
Comment From: oranagra
in an attempt to check if it was intentional or an innocent bug, i tried fixing that and to see if there's a test for it, and there isn't. so looks unintentional to me.
@redis/core-team anyone can think of a reason not to fix it? i suppose breaking the current behavior won't cause much harm since i expect this pattern is not commonly used.
Comment From: yossigo
@oranagra Can't think of a reason not to fix that.
Comment From: soloestoy
It's a known issue for a long time, I used to think it's by designed, if we agree it's a bug, we should fix it.
Comment From: madolson
Oh, I know about this. I talked with salvatore about this at some point and we agreed on the current behavior, but this was like 3 years ago so not sure I remember the nuance of the conversation.
The problem I had seen was that most clients can't handle the state transition from "Authenticated -> De-authenticated" in that they fail in interesting ways like unable to do health checks (StackExchange.Redis comes to mind in that it sends health checks with info). So consider the case where you do want to switch authentication, if it fails you want to catch the error and try again, but if the underlying client falls apart the application will be unable to retry.
So my vote is to leave it, and maybe document the behavior?
Comment From: enjoy-binbin
I personally prefer the current behavior... (like i just try a new user with a wrong password, then you kick me out)
Comment From: oranagra
ok, good to know it was intended, or at least not overlooked, but i'm not sure i understand the problems that would be caused by flagging the connection as unauthenticated.
@enjoy-binbin you said "kick me out", unlike a deleted user, which closes the connection, and failed connection attempt should be similar to the RESET command in that respect, i.e. bring you back to the same state as a newly established connection (requiring AUTH / HELLO in order to run any command).
@madolson i don't know StackExchange.Redis, can you describe the problem in more detail? how is it different from a newly established connection that fails (or didn't attempt) an AUTH?
Note that one disadvantage of the current approach is that if someone tries to perform a series of operations on behalf of different users in a pipeline, some operations could be executed with the wrong user / permissions.
i.e. AUTH user1 pw; set x y; AUTH user2 pw FLUSHDB; AUTH user3 pw; client kill;
Comment From: madolson
The issue I was talking about with Salvatore was a cluster client which supported multiple underlying connections per client connection (to do fanout for commands like mset). Most of these clients differentiate the transport layer from the client link. On transport establishment, there is normally a series of "onConnect" checks, that if they fail they throw an error. If there was an error, it would propagate it up to the client, but it was assuming it was a client side error so the connection was still healthy and it would keep using it.
With that said, if we are going to change the contract again, I think the most rational thing to do is to disconnect, since that forces the client to correctly establish a new connection, which is well tested. An aside is that we also made the explicit decision when implementing the authentication APIs to disconnect instead of de-authenticate. Also found this, https://github.com/redis/redis/pull/5916#issuecomment-556126180 conversation we had online, which is basically restating the same position, clients don't handle de-authenticate well.
@oranagra I don't think that use case is correct anyways, since it's assuming that AUTH will fully reset the client state, which isn't necessarily true anyways. I would expect them to execute some pipeline like:
CLIENT RESET; AUTH; SET X Y
CLIENT RESET; AUTH; GET A B
Comment From: madolson
@oranagra Honestly sort of feels like we should support something like multiple sessions in your example, since I know a lot of clients do multiplexing and there are a lot of Redis proxies floating around that have a couple of backend connections. Then you can switch between them without the re-auth penalty. QUIC might be a better solution there though.
Comment From: oranagra
too much on my mind right now, and too late on the release process. since this was an intentional decision, and it's not a recent one, i suggest to leave it aside for redis 7.0, and maybe start discussing it again towards redis 8.0. if it was like that since 6.0, it's ok to let it stay the same for another version.
Comment From: oranagra
I suppose another aspect of this could be that when redis isn't set with a password, and the client attempts to connect with a password, the auth error is ignored and the connection is working. (one of them complaints in #10558). Fixing this may be a little less trivial since it'll need to have a client unauthenticated, even if redis does not require authentication.