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.

Sign up for GitHub

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

Authentication: Fix CME immediate update_connection_password to use username #3330

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Authentication: Fix CME immediate update_connection_password to use username #3330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2280e9d
fixed CME immediate auth
liorsve Feb 27, 2025
91e4835
fixed CMD immediate auth
liorsve Feb 28, 2025
9e0d8ae
added auth with username CME tests
liorsve Mar 2, 2025
f7c5cdc
more CME tests
liorsve Mar 3, 2025
1bbaf67
revert CMD changes
liorsve Mar 3, 2025
b4f8420
added node CME tests
liorsve Mar 5, 2025
d19562d
fix lint
liorsve Mar 5, 2025
d497b0e
fix test
liorsve Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions glide-core/redis-rs/redis/src/cluster_async/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,11 @@ where
.await
}

/// Get the username used to authenticate with all cluster servers
pub async fn get_username(&mut self) -> RedisResult<Value> {
self.route_operation_request(Operation::GetUsername).await
}

/// Routes an operation request to the appropriate handler.
async fn route_operation_request(
&mut self,
Expand Down Expand Up @@ -620,6 +625,7 @@ enum CmdArg<C> {
#[derive(Clone)]
enum Operation {
UpdateConnectionPassword(Option<String>),
GetUsername,
}

fn route_for_pipeline(pipeline: &crate::Pipeline) -> RedisResult<Option<Route>> {
Expand Down Expand Up @@ -2276,6 +2282,15 @@ where
.expect(MUTEX_WRITE_ERR);
Ok(Response::Single(Value::Okay))
}
Operation::GetUsername => {
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,
}))
}
},
}
}
Expand Down
10 changes: 10 additions & 0 deletions glide-core/src/client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,16 @@ impl Client {
Some(ResponsePolicy::AllSucceeded),
));
let mut cmd = redis::cmd("AUTH");
match self.internal_client {
ClientWrapper::Cluster { ref mut client } => {
if let Ok(Value::SimpleString(username)) = client.get_username().await {
cmd.arg(username);
}
}
ClientWrapper::Standalone(_) => {
//TODO: get username and add it as arg in standalone
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you plan to implement this in the current PR?
If no, create a task

}
}
cmd.arg(password);
self.send_command(&cmd, Some(routing)).await
}
Expand Down
245 changes: 242 additions & 3 deletions node/tests/AuthTest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ import {

type BaseClient = GlideClient | GlideClusterClient;

const USERNAME = "username";
const INITIAL_PASSWORD = "initial_password";
const NEW_PASSWORD = "new_password";
const WRONG_PASSWORD = "wrong_password";
const TIMEOUT = 50000;

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

async function setNewAclUsernameWithPassword(
client: BaseClient,
username: string,
password: string,
) {
await client.customCommand([
"ACL",
"SETUSER",
username,
"on",
`>${password}`,
"~*",
"+@all",
]);
}

async function deleteAclUsernameAndPassword(
client: BaseClient,
username: string,
) {
return await client.customCommand(["ACL", "DELUSER", username]);
}

afterEach(async () => {
if (managementClient) {
try {
Expand All @@ -89,6 +116,8 @@ describe("Auth tests", () => {
}
}

await deleteAclUsernameAndPassword(managementClient, USERNAME);

if (cmdCluster) {
await flushAndCloseClient(false, cmdCluster.getAddresses());
}
Expand Down Expand Up @@ -118,6 +147,12 @@ describe("Auth tests", () => {
);
}

await setNewAclUsernameWithPassword(
managementClient,
USERNAME,
INITIAL_PASSWORD,
);

const ClientClass = isStandaloneMode ? GlideClient : GlideClusterClient;
const addresses = formatAddresses(activeCluster.getAddresses());

Expand All @@ -137,9 +172,6 @@ describe("Auth tests", () => {
describe.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
"update_connection_password_%p",
(protocol) => {
const NEW_PASSWORD = "new_password";
const WRONG_PASSWORD = "wrong_password";

/**
* Test replacing connection password with immediate re-authentication using a non-valid password.
* Verifies that immediate re-authentication fails when the password is not valid.
Expand Down Expand Up @@ -325,6 +357,213 @@ describe("Auth tests", () => {
).rejects.toThrow(RequestError);
}, protocol);
});
/*
* Test replacing the connection password without immediate re-authentication, when the client is pre-authenticated as an acl user.
* Verifies that:
* 1. The client can update its internal password
* 2. The client remains connected with current auth after non-immediate password update.
* 3. The client can reconnect using the new password after the user was deleted and reset with the new password on the server side (which causes the server to kill connections).
* Currently, this test is only supported for cluster mode,
* since standalone mode dont have multiple connections to manage,
* and the client will try to reconnect and will not listen to new tasks.
*/
it(
"test_update_connection_password_with_acl_user",
async () => {
await runTest(
async (client: BaseClient) => {
if (client instanceof GlideClient) {
return;
}

// Update password without re-authentication
const result =
await client.updateConnectionPassword(
NEW_PASSWORD,
false,
);
expect(result).toEqual("OK");

// Verify client still works with old auth
await client.set("test_key", "test_value");
const value = await client.get("test_key");
expect(value).toEqual("test_value");

// Update server password - this also kills the connection
await deleteAclUsernameAndPassword(
managementClient,
USERNAME,
);
await setNewAclUsernameWithPassword(
managementClient,
USERNAME,
NEW_PASSWORD,
);

// Verify client auto-reconnects with new password
await client.set("test_key2", "test_value2");
const value2 = await client.get("test_key2");
expect(value2).toEqual("test_value2");
},
protocol,
{
credentials: {
username: USERNAME,
password: INITIAL_PASSWORD,
},
},
);
},
TIMEOUT,
);
/**
* Test replacing connection password with immediate re-authentication using a non-valid password, with an acl user.
* Verifies that immediate re-authentication fails when the password is not valid.
*/
it("test_update_connection_password_auth_non_valid_pass_acl_user", async () => {
await runTest(
async (client: BaseClient) => {
await expect(
client.updateConnectionPassword(null, true),
).rejects.toThrow(RequestError);
await expect(
client.updateConnectionPassword("", true),
).rejects.toThrow(RequestError);
},
protocol,
{
credentials: {
username: USERNAME,
password: INITIAL_PASSWORD,
},
},
);
});
/**
* Test that re-authentication fails when using wrong password with an acl user.
*/
it("test_replace_password_immediateAuth_wrong_password_acl_user", async () => {
await runTest(
async (client: BaseClient) => {
await deleteAclUsernameAndPassword(
managementClient,
USERNAME,
);
await setNewAclUsernameWithPassword(
managementClient,
USERNAME,
NEW_PASSWORD,
);

await expect(
client.updateConnectionPassword(
WRONG_PASSWORD,
true,
),
).rejects.toThrow(RequestError);
await expect(
client.updateConnectionPassword(NEW_PASSWORD, true),
).resolves.toBe("OK");
},
protocol,
{
credentials: {
username: USERNAME,
password: INITIAL_PASSWORD,
},
},
);
});
/**
* Test deleting the user and reseting it with new password, which kills the connections, then re-authenticating with immediate re-authentication.
*/
it(
"test_update_connection_password_with_immediateAuth_acl_user",
async () => {
await runTest(
async (client: BaseClient) => {
// Delete user and reset it with new password
await deleteAclUsernameAndPassword(
managementClient,
USERNAME,
);
await setNewAclUsernameWithPassword(
managementClient,
USERNAME,
NEW_PASSWORD,
);

// Update client password with re-auth - make sure it reconnects successfuly with new password
expect(
await client.updateConnectionPassword(
NEW_PASSWORD,
true,
),
).toEqual("OK");

// Verify client works with new auth
await client.set("test_key", "test_value");
const value = await client.get("test_key");
expect(value).toEqual("test_value");
},
protocol,
{
credentials: {
username: USERNAME,
password: INITIAL_PASSWORD,
},
},
);
},
TIMEOUT,
);
/**
* Test changing server password when connection is lost before password update.
* Verifies that the client will not be able to reach the connection under the abstraction and return an error.
*
* **Note: This test is only supported for standalone mode, bellow explanation why*
*/
it("test_update_connection_password_connection_lost_before_password_update_acl_user", async () => {
await runTest(
async (client: BaseClient) => {
if (client instanceof GlideClusterClient) {
return;
}

// Set a key to ensure connection is established
await client.set("test_key", "test_value");

// Delete user and reset it with new password. This also kills the server conneciton.
await deleteAclUsernameAndPassword(
managementClient,
USERNAME,
);
await setNewAclUsernameWithPassword(
managementClient,
USERNAME,
NEW_PASSWORD,
);

// Try updating client password without immediate re-auth and with, both should fail
await expect(
client.updateConnectionPassword(
NEW_PASSWORD,
false,
),
).rejects.toThrow(RequestError);
await expect(
client.updateConnectionPassword(NEW_PASSWORD, true),
).rejects.toThrow(RequestError);
},
protocol,
{
credentials: {
username: USERNAME,
password: INITIAL_PASSWORD,
},
},
);
});
},
);
});
Loading
Loading