Authentication: Fix immediate update_connection_password to use username #3330

liorsve · 2025-03-05T11:57:57Z

Issue link

This Pull Request is linked to issue: #3289

This PR fixes the update connection password to add the username as arg in update_connection_password(immediateAuth = True) (i.e. send "AUTH [username] [password]") in case username was configured in ServerCredentials during client creation.
Currently, only "AUTH [password]" is sent.

Additionally, it fixes the implementation of update_connection_password in CMD - previously, the updated password would be saved only in the multiplexed_connection struct, and then upon disconnection the authentication would still be with the old password. Now, the password is updated in the ReconnectingConnection struct which is used in reconnecitons.

NOTES-

Deleting the user with delete_acl_username_and_password is triggering a disconnect from clients authenticated with that username.
Unlike the previous tests, calling update_connection_password with non-immediate authentication when disconnected from server (for example after deleting the user or actively killing the connection) is supposed to succeed both for CME and CMD. That's because this is an internal operation and doesn't require an active connection. The difference is, when calling with immediate auth, CME can reconnect with that supplied password while CMD tries to reconnect with the initial password and thus won't succeed.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.

glide-core/src/client/mod.rs

barshaul · 2025-03-09T14:37:50Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

+                    let username = core
+                        .get_cluster_param(|params| params.username.clone())
+                        .expect(MUTEX_WRITE_ERR);
+                    Ok(Response::Single(match username {
+                        Some(u) => Value::SimpleString(u),
+                        None => Value::Nil,
+                    }))


Suggested change

let username = core

.get_cluster_param(|params| params.username.clone())

.expect(MUTEX_WRITE_ERR);

Ok(Response::Single(match username {

Some(u) => Value::SimpleString(u),

None => Value::Nil,

}))

let username = match core

.get_cluster_param(|params| params.username.clone())

.expect(MUTEX_READ_ERR)

{

Some(username) => Value::SimpleString(username),

None => Value::Nil,

};

Ok(Response::Single(username))

barshaul · 2025-03-09T14:40:57Z

glide-core/src/client/mod.rs

+                    ClientWrapper::Cluster { ref mut client } => {
+                        if let Ok(Value::SimpleString(username)) = client.get_username().await {
+                            cmd.arg(username);
+                        }
+                    }
+                    ClientWrapper::Standalone(_) => {


move this logic into get_username function in glide_core's Client struct

barshaul · 2025-03-10T09:34:30Z

node/tests/AuthTest.test.ts

@@ -71,6 +75,29 @@ describe("Auth tests", () => {
    ): { host: string; port: number }[] =>
        addresses.map(([host, port]) => ({ host, port }));

+    async function setNewAclUsernameWithPassword(


This and the deleteAclUsernameAndPassword functions should handle the result of the requests - if the command fails you should catch it and panic, otherwise we won't know why the test is failing

I added an assertion everywhere besides delete_acl_username_and_password in the python tests - there the acl user is set only for the acl specific tests that I added, so asserting that a client was deleted fails on the non acl-user tests. So instead, in python I had the delete user function return the command's responds (1 if a user was deleted, 0 if not) and i'm asserting inside the relevant tests themselves that it's returning 1 when it should.

liorsve · 2025-03-10T16:57:07Z

Changes -

addressed previous comments about the CME changes.
added CMD fix in core + CMD tests.
In tests, I had to introduce some sleep wherever a server disconnection + password change was made, otherwise they were flaky. I encountered the same timing issue that was talked about here Remove outdated test for updating connection password in cluster mode #3311, where the client sometimes wouldn't have enough time to try and reconnect, probably because it takes some time for the new password to register in the server as well, and if the two are not well-synchronized in their timing it results in a connectionError. Introducing some sleep after the disconnection seems to have resolved it, so for now I re-added the test that was removed in this PR.

@barshaul @ikolomi @avifenesh

barshaul

Awesome job! added comments inline.
What about Java's tests?

barshaul · 2025-03-11T10:31:37Z

glide-core/src/client/reconnecting_connection.rs

@@ -119,7 +120,11 @@ async fn create_connection(
    discover_az: bool,
    connection_timeout: Duration,
 ) -> Result<ReconnectingConnection, (ReconnectingConnection, RedisError)> {
-    let client = &connection_backend.connection_info;
+    let client = {
+        let guard = connection_backend.connection_info.read().unwrap();


Instead unwrap use except with read lock err

barshaul · 2025-03-11T10:35:13Z

glide-core/src/client/reconnecting_connection.rs

@@ -363,4 +392,14 @@ impl ReconnectingConnection {
            log_error("disconnect notifier", "BUG! Disconnect notifier is not set");
        }
    }
+
+    pub fn update_connection_password(&self, new_password: Option<String>) {
+        let mut client = self.inner.backend.connection_info.write().unwrap();


Same regarding unwrap - —please ensure that all unwrap calls on locks are changed to expect. If I missed any instances where unwrap is used in a different context, keep in mind that we should avoid using unwrap in the code altogether

barshaul · 2025-03-11T10:37:48Z

glide-core/src/client/standalone_client.rs

+    }
+
+    pub async fn get_username(&self) -> Option<String> {
+        self.get_primary_connection().get_username()


Please add a comment explaining that all nodes in the client should have the same username and password thus we can get it from any of the nodes

barshaul · 2025-03-11T10:39:00Z

glide-core/src/client/standalone_client.rs

+        Ok(Value::Okay)
+    }
+
+    pub async fn get_username(&self) -> Option<String> {


please add documentation

also, why is it 'async'?

No reason, I think a leftover from a prev version where the inner get_username function was async. Anyways made it sync

barshaul · 2025-03-12T11:26:58Z

glide-core/src/client/mod.rs

+    async fn get_username(&mut self) -> Option<String> {
+        match &mut self.internal_client {
+            ClientWrapper::Cluster { client } => {
+                if let Ok(Value::SimpleString(username)) = client.get_username().await {


If someone would change get_username to return a different Value (e.g. not SimpleString, but for example bytes), we will always return None and we won't know what's the issue.
Use match instead to make sure you panic if you get Ok with Value which isn't SimpleString

barshaul · 2025-03-12T13:23:46Z

node/tests/AuthTest.test.ts

+                    protocol,
+                    clusterMode,
+                );
+            });


nit: missing newline

barshaul · 2025-03-12T13:32:18Z

python/python/tests/conftest.py

+        request,
+        cluster_mode,
+        protocol=protocol,
+        request_timeout=5000,


is it needed for the ACL tests? I think you can remove this and use the default

barshaul · 2025-03-12T13:34:38Z

python/python/tests/conftest.py

+        return await client.custom_command(
+            ["ACL", "DELUSER", username], route=AllNodes()
+        )
+


else: raise error

Do you mean else if it's not an instance of glideClient/glideClusterClient? but It's a TGlideCLient

barshaul · 2025-03-12T13:35:21Z

python/python/tests/conftest.py

+        raise RuntimeError(f"Failed to set ACL user: {e}")
+
+
+async def delete_acl_username_and_password(client: TGlideClient, username):


missing typing for username and password across the file

move this function to python/python/tests/utils/utils.py

barshaul · 2025-03-12T13:36:19Z

python/python/tests/conftest.py

@@ -330,6 +364,37 @@ async def config_set_new_password(client: TGlideClient, password):
        await client.config_set({"requirepass": password}, route=AllNodes())


+async def set_new_acl_username_with_password(client: TGlideClient, username, password):


move this function to python/python/tests/utils/utils.py

barshaul · 2025-03-23T08:47:59Z

glide-core/src/client/mod.rs

+                Ok(Value::SimpleString(username)) => Some(username),
+                Ok(Value::Nil) => None,
+                Ok(other) => panic!("Expected SimpleString, got: {:?}", other),
+                Err(e) => panic!("Got error trying to get username: {:?}", e),


We don't want to panic if get_username fails, as it would crash the whole client. Instead, change this function (get_username in mod.rs) to return Result<Option<String>> and return the error if we get one.

glide-core/src/client/mod.rs

barshaul · 2025-03-23T09:22:13Z

glide-core/src/client/reconnecting_connection.rs

+        let guard = connection_backend
+            .connection_info
+            .read()
+            .expect("READ_LOCK_ERR");


you should define consts and use them:
const WRITE_LOCK_ERR: &str = "Failed to acquire the write lock";
const READ_LOCK_ERR: &str = "Failed to acquire the read lock";

barshaul · 2025-03-23T09:23:29Z

glide-core/src/client/reconnecting_connection.rs

+impl ConnectionBackend {
+    /// Returns a read-only reference to the client's connection information
+    fn get_backend_client(&self) -> RwLockReadGuard<'_, redis::Client> {
+        self.connection_info.read().expect("READ_LOCK_ERR")


same regarding the error msg

barshaul · 2025-03-23T09:26:33Z

glide-core/src/client/reconnecting_connection.rs

+            .backend
+            .connection_info
+            .write()
+            .expect("WRITE_LOCK_ERR");


same reg. excpect err msg

barshaul · 2025-03-23T09:27:26Z

glide-core/src/client/reconnecting_connection.rs

+            .backend
+            .connection_info
+            .read()
+            .expect("READ_LOCK_ERR");


barshaul · 2025-03-23T09:27:51Z

glide-core/src/client/reconnecting_connection.rs

+        let client = self
+            .inner
+            .backend
+            .connection_info


why don't you use get_backend_client here?

barshaul · 2025-03-23T09:28:49Z

glide-core/src/client/standalone_client.rs

@@ -604,18 +604,23 @@ impl StandaloneClient {
        });
    }

-    /// Update the password used to authenticate with the servers.
+    /// Update the password used to authenticate with the server.


Suggested change

/// Update the password used to authenticate with the server.

/// Update the password used to authenticate with the servers.

barshaul · 2025-03-23T09:34:09Z

java/integTest/src/test/java/glide/cluster/ClusterClientTests.java

+            Thread.sleep(1000);
+
+            // Validate client reconnected succsessfuly
+            assertNotNull(testClient.info().get());


If you deleted the ACL user and haven't updated the client yet, how does it work? shouldn't we get an error here?

barshaul

See comments inline

Signed-off-by: Lior Sventitzky <[email protected]>

Co-authored-by: Bar Shaul <[email protected]> Signed-off-by: Lior Sventitzky <[email protected]>

liorsve requested a review from a team as a code owner March 5, 2025 11:57

liorsve force-pushed the add-username-auth branch from 225ff28 to d19562d Compare March 5, 2025 12:41

liorsve added Rust core redis-rs/glide-core matter Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. labels Mar 5, 2025

Yury-Fridlyand reviewed Mar 5, 2025

View reviewed changes

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

Yury-Fridlyand added the node Node.js wrapper label Mar 5, 2025

barshaul reviewed Mar 10, 2025

View reviewed changes

liorsve changed the title ~~Authentication: Fix CME immediate update_connection_password to use username~~ Authentication: Fix immediate update_connection_password to use username Mar 10, 2025

liorsve force-pushed the add-username-auth branch 7 times, most recently from eeecc18 to 6d91343 Compare March 10, 2025 16:17

ikolomi mentioned this pull request Mar 12, 2025

Python: Add back test_update_connection_password #3357

Draft

6 tasks

barshaul requested changes Mar 12, 2025

View reviewed changes

liorsve force-pushed the add-username-auth branch 4 times, most recently from afd12bd to 44ae581 Compare March 20, 2025 11:56

barshaul reviewed Mar 23, 2025

View reviewed changes

glide-core/src/client/mod.rs Outdated Show resolved Hide resolved

barshaul reviewed Mar 23, 2025

View reviewed changes

glide-core/src/client/reconnecting_connection.rs

.backend

.connection_info

.read()

.expect("READ_LOCK_ERR");

Copy link

Collaborator

barshaul Mar 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

barshaul reviewed Mar 23, 2025

View reviewed changes

barshaul requested changes Mar 23, 2025

View reviewed changes

liorsve and others added 15 commits March 23, 2025 12:13

fixed CME immediate auth

359d37b

Signed-off-by: Lior Sventitzky <[email protected]>

fixed CMD immediate auth

2ab845f

Signed-off-by: Lior Sventitzky <[email protected]>

added auth with username CME tests

d948aa5

Signed-off-by: Lior Sventitzky <[email protected]>

more CME tests

9eed07a

Signed-off-by: Lior Sventitzky <[email protected]>

revert CMD changes

f1a616c

Signed-off-by: Lior Sventitzky <[email protected]>

added node CME tests

5b6630b

Signed-off-by: Lior Sventitzky <[email protected]>

fix lint

924c5ca

Signed-off-by: Lior Sventitzky <[email protected]>

fix test

5ea2c40

Signed-off-by: Lior Sventitzky <[email protected]>

fix cmd update password, add cmd tests, fix previous tests

9e94c7a

Signed-off-by: Lior Sventitzky <[email protected]>

addressed comments

bcc85f0

Signed-off-by: Lior Sventitzky <[email protected]>

Addressed comments, added Java tests

2998728

Signed-off-by: Lior Sventitzky <[email protected]>

fix lint

df2b0a2

Signed-off-by: Lior Sventitzky <[email protected]>

add sleep to acl test

89f7242

Signed-off-by: Lior Sventitzky <[email protected]>

fix python test

1913dcd

Signed-off-by: Lior Sventitzky <[email protected]>

Update glide-core/src/client/mod.rs

9a07e5d

Co-authored-by: Bar Shaul <[email protected]> Signed-off-by: Lior Sventitzky <[email protected]>

liorsve force-pushed the add-username-auth branch from e456154 to 9a07e5d Compare March 23, 2025 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication: Fix immediate update_connection_password to use username #3330

Authentication: Fix immediate update_connection_password to use username #3330

liorsve commented Mar 5, 2025 •

edited

Loading

barshaul Mar 9, 2025

barshaul Mar 9, 2025

barshaul Mar 10, 2025

liorsve Mar 10, 2025

liorsve commented Mar 10, 2025 •

edited

Loading

barshaul left a comment

barshaul Mar 11, 2025

barshaul Mar 11, 2025 •

edited

Loading

barshaul Mar 11, 2025

barshaul Mar 11, 2025

barshaul Mar 12, 2025

liorsve Mar 13, 2025

barshaul Mar 12, 2025

barshaul Mar 12, 2025

barshaul Mar 12, 2025

barshaul Mar 12, 2025

liorsve Mar 12, 2025

barshaul Mar 12, 2025

barshaul Mar 12, 2025

barshaul Mar 12, 2025

barshaul Mar 23, 2025 •

edited

Loading

barshaul Mar 23, 2025

barshaul Mar 23, 2025

barshaul Mar 23, 2025

barshaul Mar 23, 2025

barshaul Mar 23, 2025

barshaul Mar 23, 2025

barshaul Mar 23, 2025

barshaul left a comment

		raise RuntimeError(f"Failed to set ACL user: {e}")


		async def delete_acl_username_and_password(client: TGlideClient, username):

		@@ -330,6 +364,37 @@ async def config_set_new_password(client: TGlideClient, password):
		await client.config_set({"requirepass": password}, route=AllNodes())


		async def set_new_acl_username_with_password(client: TGlideClient, username, password):

	/// Update the password used to authenticate with the server.
	/// Update the password used to authenticate with the servers.

Authentication: Fix immediate update_connection_password to use username #3330

Are you sure you want to change the base?

Authentication: Fix immediate update_connection_password to use username #3330

Conversation

liorsve commented Mar 5, 2025 • edited Loading

Issue link

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorsve commented Mar 10, 2025 • edited Loading

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

liorsve commented Mar 5, 2025 •

edited

Loading

liorsve commented Mar 10, 2025 •

edited

Loading

barshaul Mar 11, 2025 •

edited

Loading

barshaul Mar 23, 2025 •

edited

Loading