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.

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

Add support for ACL authentication #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ilyaBykonya
Copy link

Redis 6.2 introduced the function of user ACL lists, which limit access and increase the level of system security: https://redis.io/docs/management/security/acl/

The changes in the fork were aimed only at adding support for this functionality.

In the first commit, an overload of the authenticate method was added to work with the new ACL, and the old method simply called: authenticate("default", password); which for new versions of Redis is identical to authenticate(password);

The second commit was caused by the fact that the old version, being the only (old) authentication method for requirepass, was no longer compatible with it. And I've added code duplication to maintain backward compatibility with Redis < 6.2

@ilyaBykonya ilyaBykonya requested a review from rpj as a code owner March 11, 2024 22:19
@rpj
Copy link
Member

rpj commented Mar 12, 2024

This is fantastic, thank you @ilyaBykonya for submitting it! 🙇

Overall it looks really good, but we do require that a test be added for newly-added or changed functionality, which this PR definitely qualifies for.

Fortunately the GitHub Actions CI pipeline is using redis version 7.2.4 so it will be fully testable within the CI pipeline setup here.

I've just removed the Actions restriction so you will be able to run further pipelines without my approval, so you can be sure your tests succeed in the CI pipeline by just pushing the changes here (or to a separate branch if you prefer) and letting the pipeline run.

Please get in touch with any questions, I'm more than happy to help guide you in any way I can!

Thank you again, I really appreciate the contribution!

Copy link
Member

@rpj rpj left a comment

Choose a reason for hiding this comment

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

Needs added test(s)

* @param password The password with which to authenticate.
* @returns RedisReturnValue detailing the result
*/
RedisReturnValue authenticate(const char *password);
/**
* Authenticate with the given password.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Authenticate with the given password.
* Authenticate with the given password and client.

@@ -107,11 +107,18 @@ class Redis
Redis &operator=(const Redis &&) = delete;

/**
* Authenticate with the given password.
* Authenticate with the given password and client.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Authenticate with the given password and client.
* Authenticate with the given password.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants