diff --git a/glide-core/redis-rs/redis/src/types.rs b/glide-core/redis-rs/redis/src/types.rs index 4b6cdbb150..2d8035d697 100644 --- a/glide-core/redis-rs/redis/src/types.rs +++ b/glide-core/redis-rs/redis/src/types.rs @@ -153,6 +153,10 @@ pub enum ErrorKind { /// Not all slots are covered by the cluster NotAllSlotsCovered, + + /// Used when an error occurs on when user perform wrong usage of management operation. + /// E.g. not allowed configuration change. + UserOperationError, } #[derive(PartialEq, Debug)] @@ -900,6 +904,7 @@ impl RedisError { ErrorKind::RESP3NotSupported => "resp3 is not supported by server", ErrorKind::ParseError => "parse error", ErrorKind::NotAllSlotsCovered => "not all slots are covered", + ErrorKind::UserOperationError => "Wrong usage of management operation", } } @@ -1095,6 +1100,7 @@ impl RedisError { ErrorKind::NotAllSlotsCovered => RetryMethod::NoRetry, ErrorKind::FatalReceiveError => RetryMethod::Reconnect, ErrorKind::FatalSendError => RetryMethod::ReconnectAndRetry, + ErrorKind::UserOperationError => RetryMethod::NoRetry, } } } diff --git a/glide-core/src/client/mod.rs b/glide-core/src/client/mod.rs index ffbdc60d4e..33258399ab 100644 --- a/glide-core/src/client/mod.rs +++ b/glide-core/src/client/mod.rs @@ -479,28 +479,63 @@ impl Client { /// Update the password used to authenticate with the servers. /// If None is passed, the password will be removed. - /// If `re_auth` is true, the new password will be used to re-authenticate with all of the nodes. + /// If `immediate_auth` is true, the password will be used to authenticate with the servers immediately using the `AUTH` command. + /// The default behavior is to update the password without authenticating immediately. + /// If the password is empty or None, and `immediate_auth` is true, the password will be updated and an error will be returned. pub async fn update_connection_password( &mut self, password: Option, - re_auth: bool, + immediate_auth: bool, ) -> RedisResult { - if re_auth { - let routing = RoutingInfo::MultiNode(( - MultipleNodeRoutingInfo::AllNodes, - Some(ResponsePolicy::AllSucceeded), - )); - let mut cmd = redis::cmd("AUTH"); - cmd.arg(&password); - self.send_command(&cmd, Some(routing)).await?; + let timeout = self.request_timeout; + // The password update operation is wrapped in a timeout to prevent it from blocking indefinitely. + // If the operation times out, an error is returned. + // Since the password update operation is not a command that go through the regular command pipeline, + // it is not have the regular timeout handling, as such we need to handle it separately. + match tokio::time::timeout(timeout, async { + match self.internal_client { + ClientWrapper::Standalone(ref mut client) => { + client.update_connection_password(password.clone()).await + } + ClientWrapper::Cluster { ref mut client } => { + client.update_connection_password(password.clone()).await + } + } + }) + .await + { + Ok(result) => { + if immediate_auth { + self.send_immediate_auth(password).await + } else { + result + } + } + Err(_elapsed) => Err(RedisError::from(( + ErrorKind::IoError, + "Password update operation timed out, please check the connection", + ))), } + } - match self.internal_client { - ClientWrapper::Standalone(ref mut client) => { - client.update_connection_password(password).await - } - ClientWrapper::Cluster { ref mut client } => { - client.update_connection_password(password).await + async fn send_immediate_auth(&mut self, password: Option) -> RedisResult { + match &password { + Some(pw) if pw.is_empty() => Err(RedisError::from(( + ErrorKind::UserOperationError, + "Empty password provided for authentication", + ))), + None => Err(RedisError::from(( + ErrorKind::UserOperationError, + "No password provided for authentication", + ))), + Some(password) => { + let routing = RoutingInfo::MultiNode(( + MultipleNodeRoutingInfo::AllNodes, + Some(ResponsePolicy::AllSucceeded), + )); + let mut cmd = redis::cmd("AUTH"); + cmd.arg(password); + self.send_command(&cmd, Some(routing)).await } } } diff --git a/glide-core/src/protobuf/command_request.proto b/glide-core/src/protobuf/command_request.proto index e50cdc8b3c..30b33362af 100644 --- a/glide-core/src/protobuf/command_request.proto +++ b/glide-core/src/protobuf/command_request.proto @@ -510,7 +510,7 @@ message ClusterScan { message UpdateConnectionPassword { optional string password = 1; - bool re_auth = 2; + bool immediate_auth = 2; } message CommandRequest { diff --git a/glide-core/src/socket_listener.rs b/glide-core/src/socket_listener.rs index b7f967e0bd..b9db4e6d99 100644 --- a/glide-core/src/socket_listener.rs +++ b/glide-core/src/socket_listener.rs @@ -529,7 +529,7 @@ fn handle_request(request: CommandRequest, mut client: Client, writer: Rc handleLcsIdxResponse(Map response) * that the internal reconnection mechanism can handle reconnection seamlessly, preventing the * loss of in-flight commands. * + * @param immediateAuth A boolean flag. If true, the client will + * authenticate immediately with the new password against all connections, Using AUTH + * command.
+ * If password supplied is an empty string, the client will not perform auth and a warning + * will be returned.
+ * The default is `false`. * @apiNote This method updates the client's internal password configuration and does not perform * password rotation on the server side. * @param password A new password to set. - * @param mode Password update mode, see {@link PasswordUpdateMode}. * @return "OK". * @example * 
{@code
@@ -799,9 +803,9 @@ protected Map handleLcsIdxResponse(Map response)
      * }
*/ public CompletableFuture updateConnectionPassword( - @NonNull String password, @NonNull PasswordUpdateMode mode) { + @NonNull String password, boolean immediateAuth) { return commandManager.submitPasswordUpdate( - Optional.of(password), mode, this::handleStringResponse); + Optional.of(password), immediateAuth, this::handleStringResponse); } /** @@ -815,16 +819,22 @@ public CompletableFuture updateConnectionPassword( * * @apiNote This method updates the client's internal password configuration and does not perform * password rotation on the server side. - * @param mode Password update mode, see {@link PasswordUpdateMode}. + * @param immediateAuth A boolean flag. If true, the client will + * authenticate immediately with the new password against all connections, Using AUTH + * command.
+ * If password supplied is an empty string, the client will not perform auth and a warning + * will be returned.
+ * The default is `false`. * @return "OK". * @example * 
{@code
-     * String response = client.resetConnectionPassword(RE_AUTHENTICATE).get();
+     * String response = client.resetConnectionPassword(true).get();
      * assert response.equals("OK");
      * }
*/ - public CompletableFuture updateConnectionPassword(@NonNull PasswordUpdateMode mode) { - return commandManager.submitPasswordUpdate(Optional.empty(), mode, this::handleStringResponse); + public CompletableFuture updateConnectionPassword(boolean immediateAuth) { + return commandManager.submitPasswordUpdate( + Optional.empty(), immediateAuth, this::handleStringResponse); } @Override diff --git a/java/client/src/main/java/glide/managers/CommandManager.java b/java/client/src/main/java/glide/managers/CommandManager.java index b1422a0eee..d069c6bd72 100644 --- a/java/client/src/main/java/glide/managers/CommandManager.java +++ b/java/client/src/main/java/glide/managers/CommandManager.java @@ -17,7 +17,6 @@ import glide.api.models.GlideString; import glide.api.models.Script; import glide.api.models.Transaction; -import glide.api.models.commands.PasswordUpdateMode; import glide.api.models.commands.scan.ClusterScanCursor; import glide.api.models.commands.scan.ScanOptions; import glide.api.models.configuration.RequestRoutingConfiguration.ByAddressRoute; @@ -224,16 +223,16 @@ public CompletableFuture submitClusterScan( * Submit a password update request to GLIDE core. * * @param password A new password to set or empty value to remove the password. - * @param mode Password update mode. + * @param immediateAuth immediately perform auth. * @param responseHandler A response handler. * @return A request promise. * @param Type of the response. */ public CompletableFuture submitPasswordUpdate( Optional password, - PasswordUpdateMode mode, + boolean immediateAuth, GlideExceptionCheckedFunction responseHandler) { - var builder = UpdateConnectionPassword.newBuilder().setReAuth(mode.getValue()); + var builder = UpdateConnectionPassword.newBuilder().setImmediateAuth(immediateAuth); password.ifPresent(builder::setPassword); var command = CommandRequest.newBuilder().setUpdateConnectionPassword(builder.build()); diff --git a/java/integTest/src/test/java/glide/SharedClientTests.java b/java/integTest/src/test/java/glide/SharedClientTests.java index 0ed48a6077..26a4144e96 100644 --- a/java/integTest/src/test/java/glide/SharedClientTests.java +++ b/java/integTest/src/test/java/glide/SharedClientTests.java @@ -27,11 +27,12 @@ import net.bytebuddy.utility.RandomString; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -// @Timeout(35) // seconds +@Timeout(35) // seconds public class SharedClientTests { private static GlideClient standaloneClient = null; diff --git a/java/integTest/src/test/java/glide/cluster/ClusterClientTests.java b/java/integTest/src/test/java/glide/cluster/ClusterClientTests.java index e907bee796..380cd1a2c0 100644 --- a/java/integTest/src/test/java/glide/cluster/ClusterClientTests.java +++ b/java/integTest/src/test/java/glide/cluster/ClusterClientTests.java @@ -5,15 +5,14 @@ import static glide.TestUtilities.commonClusterClientConfig; import static glide.TestUtilities.getRandomString; import static glide.api.BaseClient.OK; -import static glide.api.models.configuration.RequestRoutingConfiguration.SimpleMultiNodeRoute.ALL_NODES; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; import glide.api.GlideClusterClient; -import glide.api.models.commands.PasswordUpdateMode; import glide.api.models.configuration.ServerCredentials; import glide.api.models.exceptions.ClosingException; import glide.api.models.exceptions.RequestException; @@ -21,10 +20,9 @@ import java.util.UUID; import java.util.concurrent.ExecutionException; import lombok.SneakyThrows; +import org.apache.commons.lang3.RandomStringUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.EnumSource; @Timeout(10) // seconds public class ClusterClientTests { @@ -167,50 +165,117 @@ public void closed_client_throws_ExecutionException_with_ClosingException_as_cau } @SneakyThrows - @ParameterizedTest - @EnumSource(PasswordUpdateMode.class) - public void password_update(PasswordUpdateMode mode) { - GlideClusterClient client = + @Test + public void test_update_connection_password() { + GlideClusterClient adminClient = GlideClusterClient.createClient(commonClusterClientConfig().build()).get(); + String pwd = UUID.randomUUID().toString(); - var key = UUID.randomUUID().toString(); - var pwd = UUID.randomUUID().toString(); - client.set(key, "meow meow").get(); + try (GlideClusterClient testClient = + GlideClusterClient.createClient(commonClusterClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); + + // Update password without re-authentication + assertEquals(OK, testClient.updateConnectionPassword(pwd, false).get()); + + // Verify client still works with old auth + assertNotNull(testClient.info().get()); + + // Update server password + // Kill all other clients to force reconnection + assertEquals("OK", adminClient.configSet(Map.of("requirepass", pwd)).get()); + adminClient.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}).get(); + + Thread.sleep(1000); + + // Verify client auto-reconnects with new password + assertNotNull(testClient.info().get()); + } finally { + adminClient.configSet(Map.of("requirepass", "")).get(); + adminClient.close(); + } + } - try (var testClient = + @SneakyThrows + @Test + public void test_update_connection_password_auth_non_valid_pass() { + // Test Client fails on call to updateConnectionPassword with invalid parameters + try (GlideClusterClient testClient = GlideClusterClient.createClient(commonClusterClientConfig().build()).get()) { + var emptyPasswordException = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword("", true).get()); + assertInstanceOf(RequestException.class, emptyPasswordException.getCause()); + + var noPasswordException = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword(true).get()); + assertInstanceOf(RequestException.class, noPasswordException.getCause()); + } + } - // validate that we can get the value - assertEquals("meow meow", testClient.get(key).get()); + @SneakyThrows + @Test + public void test_update_connection_password_no_server_auth() { + var pwd = UUID.randomUUID().toString(); - // set the password and forcefully drop connection for the second client - assertEquals("OK", client.configSet(Map.of("requirepass", pwd)).get()); - if (mode == PasswordUpdateMode.RE_AUTHENTICATE) - testClient.customCommand(new String[] {"RESET"}, ALL_NODES).get(); - else client.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}, ALL_NODES).get(); + try (GlideClusterClient testClient = + GlideClusterClient.createClient(commonClusterClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); - // client should reconnect, but will receive NOAUTH error - var exception = assertThrows(ExecutionException.class, () -> testClient.get(key).get()); + // Test that immediate re-authentication fails when no server password is set. + var exception = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword(pwd, true).get()); assertInstanceOf(RequestException.class, exception.getCause()); - assertTrue(exception.getMessage().toLowerCase().contains("noauth")); + } + } + + @SneakyThrows + @Test + public void test_update_connection_password_long() { + var pwd = RandomStringUtils.randomAlphabetic(1000); + + try (GlideClusterClient testClient = + GlideClusterClient.createClient(commonClusterClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); - assertEquals("OK", testClient.updateConnectionPassword(pwd, mode).get()); + // Test replacing connection password with a long password string. + assertEquals(OK, testClient.updateConnectionPassword(pwd, false).get()); + } + } - // after setting new password we should be able to work with the server - assertEquals("meow meow", testClient.get(key).get()); + @Timeout(50) + @SneakyThrows + @Test + public void test_replace_password_immediateAuth_wrong_password() { + var pwd = UUID.randomUUID().toString(); + var notThePwd = UUID.randomUUID().toString(); - // unset the password and drop connection again - assertEquals("OK", client.configSet(Map.of("requirepass", "")).get()); + GlideClusterClient adminClient = + GlideClusterClient.createClient(commonClusterClientConfig().build()).get(); + try (GlideClusterClient testClient = + GlideClusterClient.createClient(commonClusterClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); - client.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}, ALL_NODES).get(); + // set the password to something else + adminClient.configSet(Map.of("requirepass", notThePwd)).get(); + + // Test that re-authentication fails when using wrong password. + var exception = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword(pwd, true).get()); + assertInstanceOf(RequestException.class, exception.getCause()); - // client should reconnect, but since no auth configured, able to get a value - assertEquals("meow meow", testClient.get(key).get()); - } catch (Exception e) { - e.printStackTrace(); + // But using something else password returns OK + assertEquals(OK, testClient.updateConnectionPassword(notThePwd, true).get()); } finally { - client.configSet(Map.of("requirepass", "")).get(); - client.close(); + adminClient.configSet(Map.of("requirepass", "")).get(); + adminClient.close(); } } } diff --git a/java/integTest/src/test/java/glide/standalone/StandaloneClientTests.java b/java/integTest/src/test/java/glide/standalone/StandaloneClientTests.java index 6e6c23263f..85b5cdd889 100644 --- a/java/integTest/src/test/java/glide/standalone/StandaloneClientTests.java +++ b/java/integTest/src/test/java/glide/standalone/StandaloneClientTests.java @@ -7,12 +7,12 @@ import static glide.api.BaseClient.OK; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; import glide.api.GlideClient; -import glide.api.models.commands.PasswordUpdateMode; import glide.api.models.configuration.ServerCredentials; import glide.api.models.exceptions.ClosingException; import glide.api.models.exceptions.RequestException; @@ -20,10 +20,11 @@ import java.util.UUID; import java.util.concurrent.ExecutionException; import lombok.SneakyThrows; +import org.apache.commons.lang3.RandomStringUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.ValueSource; @Timeout(10) // seconds public class StandaloneClientTests { @@ -167,42 +168,119 @@ public void closed_client_throws_ExecutionException_with_ClosingException_as_cau } @SneakyThrows - @ParameterizedTest - @EnumSource(PasswordUpdateMode.class) - public void password_update(PasswordUpdateMode mode) { - GlideClient client = GlideClient.createClient(commonClientConfig().build()).get(); - var key = UUID.randomUUID().toString(); + @Test + public void test_update_connection_password_auth_non_valid_pass() { + // Test Client fails on call to updateConnectionPassword with invalid parameters + try (var testClient = GlideClient.createClient(commonClientConfig().build()).get()) { + var emptyPasswordException = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword("", true).get()); + assertInstanceOf(RequestException.class, emptyPasswordException.getCause()); + + var noPasswordException = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword(true).get()); + assertInstanceOf(RequestException.class, noPasswordException.getCause()); + } + } + + @SneakyThrows + @Test + public void test_update_connection_password_no_server_auth() { var pwd = UUID.randomUUID().toString(); - client.set(key, "meow meow").get(); try (var testClient = GlideClient.createClient(commonClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); - // validate that we can get the value - assertEquals("meow meow", testClient.get(key).get()); + // Test that immediate re-authentication fails when no server password is set. + var exception = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword(pwd, true).get()); + assertInstanceOf(RequestException.class, exception.getCause()); + } + } - // set the password and forcefully drop connection for the second client - assertEquals("OK", client.configSet(Map.of("requirepass", pwd)).get()); - client.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}).get(); + @SneakyThrows + @Test + public void test_update_connection_password_long() { + var pwd = RandomStringUtils.randomAlphabetic(1000); - // client should reconnect, but will receive NOAUTH error - var exception = assertThrows(ExecutionException.class, () -> testClient.get(key).get()); - assertInstanceOf(RequestException.class, exception.getCause()); - assertTrue(exception.getMessage().toLowerCase().contains("noauth")); + try (var testClient = GlideClient.createClient(commonClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); - assertEquals("OK", testClient.updateConnectionPassword(pwd, mode).get()); + // Test replacing connection password with a long password string. + assertEquals(OK, testClient.updateConnectionPassword(pwd, false).get()); + } + } - // after setting new password we should be able to work with the server - assertEquals("meow meow", testClient.get(key).get()); + @Timeout(50) + @SneakyThrows + @Test + public void test_replace_password_immediateAuth_wrong_password() { + var pwd = UUID.randomUUID().toString(); + var notThePwd = UUID.randomUUID().toString(); + + GlideClient adminClient = GlideClient.createClient(commonClientConfig().build()).get(); + try (var testClient = GlideClient.createClient(commonClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); - // unset the password and drop connection again - assertEquals("OK", client.configSet(Map.of("requirepass", "")).get()); - client.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}).get(); + // set the password to something else + adminClient.configSet(Map.of("requirepass", notThePwd)).get(); - // client should reconnect, but since no auth configured, able to get a value - assertEquals("meow meow", testClient.get(key).get()); + // Test that re-authentication fails when using wrong password. + var exception = + assertThrows( + ExecutionException.class, () -> testClient.updateConnectionPassword(pwd, true).get()); + assertInstanceOf(RequestException.class, exception.getCause()); + + // But using something else password returns OK + assertEquals(OK, testClient.updateConnectionPassword(notThePwd, true).get()); + } finally { + adminClient.configSet(Map.of("requirepass", "")).get(); + adminClient.close(); + } + } + + @SneakyThrows + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void test_update_connection_password_connection_lost_before_password_update( + boolean immediateAuth) { + GlideClient adminClient = GlideClient.createClient(commonClientConfig().build()).get(); + var pwd = UUID.randomUUID().toString(); + + try (var testClient = GlideClient.createClient(commonClientConfig().build()).get()) { + // validate that we can use the client + assertNotNull(testClient.info().get()); + + // set the password and forcefully drop connection for the testClient + assertEquals("OK", adminClient.configSet(Map.of("requirepass", pwd)).get()); + adminClient.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}).get(); + + /* + * Some explanation for the curious mind: + * Our library is abstracting a connection or connections, with a lot of mechanism around it, making it behave like what we call a "client". + * When using standalone mode, the client is a single connection, so on disconnection the first thing it planned to do is to reconnect. + * + * There's no reason to get other commands and to take care of them since to serve commands we need to be connected. + * + * Hence, the client will try to reconnect and will not listen try to take care of new tasks, but will let them wait in line, + * so the update connection password will not be able to reach the connection and will return an error. + * For future versions, standalone will be considered as a different animal then it is now, since standalone is not necessarily one node. + * It can be replicated and have a lot of nodes, and to be what we like to call "one shard cluster". + * So, in the future, we will have many existing connection and request can be managed also when one connection is locked, + */ + var exception = + assertThrows( + ExecutionException.class, + () -> testClient.updateConnectionPassword(pwd, immediateAuth).get()); + assertInstanceOf(RequestException.class, exception.getCause()); } finally { - client.configSet(Map.of("requirepass", "")).get(); - client.close(); + adminClient.configSet(Map.of("requirepass", "")).get(); + adminClient.close(); } } }