Skip to content

[Cache] Add support for ACL auth in RedisAdapter #45313

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

Jump to bottom
Merged
nicolas-grekas merged 1 commit into from
Feb 8, 2022
Merged

[Cache] Add support for ACL auth in RedisAdapter #45313

nicolas-grekas merged 1 commit into from
Feb 8, 2022

Conversation

@gam6itko
Copy link
Contributor

@gam6itko gam6itko commented Feb 4, 2022

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #45305
License MIT
Doc PR symfony/symfony-docs#...

Added support of DSN username:password section in RedisAdapter which allows using ACL AUTH

@carsonbot carsonbot added this to the 5.4 milestone Feb 4, 2022
@carsonbot carsonbot changed the title [cache] RedisAdapter supports acl username:password [Cache] RedisAdapter supports acl username:password Feb 4, 2022
nicolas-grekas
nicolas-grekas reviewed Feb 4, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

That's a new feature, please target 6.1.

nicolas-grekas
nicolas-grekas reviewed Feb 5, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I have a concern: right now, the user part of the credentials is simply ignored.
On eg heroku, the provided DSN always uses h as username. This PR would break such setups, isn't it? Or does Redis ignore the username when the password is correct (no-ACL)?
What about going with a user param in the query string instead?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 5, 2022 •
edited
Loading

I can answer part of my questions with https://redis.io/commands/auth:

When ACLs are used, the single argument form of the command, where only the password is specified, assumes that the implicit username is "default".

right now, the user part of the credentials is simply ignored.
could you please give my case a try and see if AUTH user pwd is able to ignore the user part if pwd matched the requirepass option?

Could you also please see how your patch behaves on older versions of Redis?

@gam6itko gam6itko marked this pull request as draft February 5, 2022 19:55
@gam6itko gam6itko changed the base branch from 5.4 to 6.1 February 5, 2022 19:56
@gam6itko
Copy link
Contributor Author

gam6itko commented Feb 6, 2022 •
edited
Loading

This feature not breaks BC with redis v5 and redis v4.
I've added a few tests for redis versions bellow 6. If you pass username:password when the server will throw exception like AUTH failed: ERR wrong number of arguments for 'auth' command .

@gam6itko gam6itko marked this pull request as ready for review February 6, 2022 08:37
@carsonbot carsonbot modified the milestones: 5.4, 6.1 Feb 6, 2022
@nicolas-grekas nicolas-grekas changed the title [Cache] RedisAdapter supports acl username:password [Cache] Add support for ACL auth in RedisAdapter Feb 7, 2022
@nicolas-grekas nicolas-grekas force-pushed the cache_redis_acl branch 3 times, most recently from 52ba5d4 to 9de08db Compare February 7, 2022 18:29
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 7, 2022
Merged
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 7, 2022

I just pushed some changes (see 2nd commit).
Please fetch + reset --hard before continuing.

Tests fails, see CI. I'm able to reproduce the failures locally by running docker run --rm -p 16379:6379 --name redis6 redis:6.0.0 in one term, then launching tests in another:
REDIS_HOST=localhost:16379 ./phpunit src/Symfony/Component/Cache/ --filter edis --stop-on-failure

Failures were already there before my changes ;)
Can you please have a look?

@gam6itko
Copy link
Contributor Author

gam6itko commented Feb 7, 2022 •
edited
Loading

@nicolas-grekas I neet to fix tests for AppVeyor CI?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 7, 2022

On Linux, see "Integration / Tests" line.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 7, 2022

An easy fix might be to remove testDefaultUserPasswordAuth
I'd be fine with it.

nicolas-grekas
nicolas-grekas approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I removed the offending test case.

nicolas-grekas added a commit that referenced this pull request Feb 8, 2022
@nicolas-grekas
bug #45339 [Cache] fix error handling when using Redis (nicolas-grekas)
8f7420c 
This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] fix error handling when using Redis

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #45325
| License       | MIT
| Doc PR        | -

Spotted while working on #45313

I won't be able to add tests here, the situations are too edgy.

Commits
-------

3c59e0f [Cache] fix error handling
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 8, 2022

Thank you @gam6itko.

@nicolas-grekas nicolas-grekas merged commit 1211039 into symfony:6.1 Feb 8, 2022
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 8, 2022
Closed
@gam6itko gam6itko deleted the cache_redis_acl branch February 8, 2022 10:31
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request May 11, 2022
Closed
@MatTheCat MatTheCat mentioned this pull request Aug 17, 2022
Closed
@julienloizelet julienloizelet mentioned this pull request Jun 12, 2024
Closed
@xabbuh xabbuh mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees

No one assigned

Labels

Cache Feature Status: Reviewed

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

[cache] RedisAdapter posibility to set ACL username and password

3 participants

@gam6itko @nicolas-grekas @carsonbot