fix(acl): implemented ACL SETPASS command to let authenticated users … #14564
Conversation
…change their password without needing full ACL SETUSER permissions
|
|
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
shahsb
left a comment
shahsb
left a comment
There was a problem hiding this comment.
Thanks @RustyCoderX for your efforts on this.!
| addReplyBulkCString(c, "timestamp-last-updated"); | ||
| addReplyLongLong(c, le->ctime); | ||
| } | ||
| } else if (!strcasecmp(sub,"setpass") && c->argc == 3) { |
There was a problem hiding this comment.
Could you please add unit or component tests for this change?
You could add to tests/unit/acl.tcl (run via redis-server --test-coverage). It would be nice to cover basic + edge cases.
src/acl.c
Outdated
| } | ||
|
|
||
| /* Set the password for the current user. */ | ||
| sds password = c->argv[2]->ptr; |
There was a problem hiding this comment.
Could you please add a length check for the password (configurable via a potential future acl-password-min-length config, but it could be hardcoded to 4 for now -- aligns with common Redis ACL practices).
Something like:
/* Validate password length (min 4 chars for basic security). */
size_t pwlen = sdslen(c->argv[2]->ptr);
if (pwlen < 4) {
addReplyErrorFormat(c, "Password must be at least %zu characters long", (size_t)4);
return;
}
/* Set the password for the current user (hashed via ACLSetUser). */
sds password = c->argv[2]->ptr;
src/acl.c
Outdated
| /* Reset existing passwords and set the new one. */ | ||
| if (ACLSetUser(c->user, "resetpass", -1) != C_OK || | ||
| ACLSetUser(c->user, aclop, sdslen(aclop)) != C_OK) { | ||
| addReplyError(c, "Failed to set password"); |
There was a problem hiding this comment.
It would be nice to enhance error handling: Distinguish between auth failures (already good), validation errors, and ACLSetUser issues (e.g., read-only mode or OOM via a simple errno check if available).
like:
if (acl_err != C_OK) {
/* More specific error if read-only (extend as needed). */
if (server.masterhost && server.master) { /* Replica check example */
addReplyError(c, "Password update failed: instance is read-only");
} else {
addReplyError(c, "Failed to set password");
}
} else {
addReply(c, shared.ok);
}
|
i don't think OPS like you to modify the password even you have it. |
4 participants
…change their password without needing full ACL SETUSER permissions