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

Jump to bottom

Redis AUTH support #748

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Redis AUTH support #748

wants to merge 3 commits into from
+318 −4

Conversation

reimannf
Copy link

This superseeds #576 as I cannot push to the original feature branch.
@axelfauvel if you are not ok with this, can you please merge the current state of dynomite in your branch, so I could pick from there?
@smukil I think I have all your remarks covered in the origanl PR. Hopefully we can take it from here.

This commit is based on
orange-cloudfoundry@7aa41a4
from @axelfauvel in #576 and tries to
close #46.

Unfortunatelly the initial commit was already so old and the dynomite code base already evolved,
that it was easier to not jump directly on this. Especically as there were some refactorings
requested.

Redis Datastore Authentification

If Dynomite is configured to require a password via config option requirepass the following
behaviour will be applied:

  1. On Dynomite startup, the server authenticates with the backend itself
    by calling the datastore agnostic function g_datastore_auth.
  2. The corresponding Redis response will be handeled in g_is_authenticated.
    Dynomite will exit if authentification to the datatstore was not successful.
  3. Each newly created client connection will require authentification.
  4. Clients can authentificate itself by issue the AUTH command against dynomite.
  5. Dynomite will check the password and simulate an AUTH response.
  6. If AUTH was successful, the auth_required flag on the connection is reset and
    the client can process further commands through this connection.

@reimannf
Redis AUTH support
2069c3a 
This commit is based on
orange-cloudfoundry@7aa41a4
from @axelfauvel in Netflix#576 and tries to
close Netflix#46.

Unfortunatelly the initial commit was already so old and the dynomite code base already evolved,
that it was easier to not jump directly on this. Especically as there were some refactorings
requested.

Redis Datastore Authentification
If Dynomite is configured to require a password via config option `requirepass` the following
behaviour will be applied:

1. On Dynomite startup, the server authenticates with the backend itself
   by calling the datastore agnostic function g_datastore_auth.
2. The corresponding Redis response will be handeled in g_is_authenticated.
   Dynomite will exit if authentification to the datatstore was not successful.
3. Each newly created client connection will require authentification.
4. Clients can authentificate itself by issue the AUTH command against dynomite.
5. Dynomite will check the password and simulate an AUTH response.
6. If AUTH was successful, the auth_required flag on the connection is reset and
   the client can process further commands through this connection.
@reimannf
fix whitespace
62909c9
@reimannf
Correct forward definition
4b0defc
@axelfauvel
Copy link

Hi @shailesh33 @ipapapa

Do you think you can merge this PR ? Many thanks :)

@jyriok
Copy link

jyriok commented Apr 2, 2020

any news about this pr ? :)

@yedongxin
Copy link

hello,any news about this merging ?

@ArthurHlt
Copy link

@smukil could you merge this ? it actually looks like author take all your remarks from original PR and do the changes and you needed only those changes before merging on original PR.

Not supporting auth is a real show blocker for many company, thanks for the time took on merging it

@mcouillard
Copy link

I've proven this PR against my fork of dev and the basics appear fine. It correctly requires the auth password & fails when given the wrong one, or none.

I'd love to see this PR get updated & re-reviewed for inclusion.

@WenningQiu WenningQiu mentioned this pull request Oct 31, 2022
Closed
@WenningQiu
Copy link

@smukil could you merge this ? it actually looks like author take all your remarks from original PR and do the changes and you needed only those changes before merging on original PR.

Not supporting auth is a real show blocker for many company, thanks for the time took on merging it

+1

@WenningQiu
Copy link

WenningQiu commented Nov 1, 2022 •
edited
Loading

@reimannf

I tried applied your PR (682daa3) to v0.8, the code builds but has runtime error on AUTH command:

qiuwen01-pa@accdevxacpx0001> redis-cli -p 5010
127.0.0.1:5010> auth WenningQiu password
Error: Server closed the connection
127.0.0.1:5010>

[2022-11-01 09:51:17.399] server_connected:297 <CONN_SERVER 0x13c3a30 5 to '127.0.0.1:5000:1'> connected
[2022-11-01 09:51:36.102] proxy_accept:203 <CONN_PROXY 0x13cdfa0 7 listening on '0.0.0.0:5010'> accepted <CONN_CLIENT 0x13d0a20 9 from '127.0.0.1:52448'>
[2022-11-01 09:51:36.103] redis_parse_req:1590 parsed unsupported command 'COMMAND'
[2022-11-01 09:51:36.103] redis_parse_req:2394 parsed bad req 3 res 1 type 0 state 5
00000000  2a 31 0d 0a 24 37 0d 0a  43 4f 4d 4d 41 4e 44 0d   |*1..$7..COMMAND.|
00000010  0a                                                 |.|
[2022-11-01 09:51:36.103] core_close:418 close <CONN_CLIENT 0x13d0a20 9 from '127.0.0.1:52448'> on event 00FF eof 0 done 0 rb 17 sb 0: Invalid argument
[2022-11-01 09:51:36.103] client_unref_internal_try_put:97 <CONN_CLIENT 0x13d0a20 -1 from '127.0.0.1:52448'> unref owner <POOL 0x13c2bb0 'dyn_o_mite'>
[2022-11-01 09:51:47.410] core_print_peer_status:62 0)0x13ce5d0 omaha rack5000 0.0.0.0:5100 NORMAL
[2022-11-01 09:52:23.096] proxy_accept:203 <CONN_PROXY 0x13cdfa0 7 listening on '0.0.0.0:5010'> accepted <CONN_CLIENT 0x13d0a20 9 from '127.0.0.1:52460'>
[2022-11-01 09:52:23.096] redis_parse_req:2394 parsed bad req 4 res 1 type 150 state 10
00000000  2a 33 0d 0a 24 34 0d 0a  61 75 74 68 0d 0a 24 31   |*3..$4..auth..$1|
00000010  30 0d 0a 57 65 6e 6e 69  6e 67 51 69 75 0d 0a 24   |0..WenningQiu..$|
00000020  38 0d 0a 70 61 73 73 77  6f 72 64 0d 0a            |8..password..|
[2022-11-01 09:52:23.096] core_close:418 close <CONN_CLIENT 0x13d0a20 9 from '127.0.0.1:52460'> on event FFFF eof 0 done 0 rb 45 sb 0: Invalid argument
[2022-11-01 09:52:23.097] client_unref_internal_try_put:97 <CONN_CLIENT 0x13d0a20 -1 from '127.0.0.1:52460'> unref owner <POOL 0x13c2bb0 'dyn_o_mite'>

So my questions for @reimannf:

1. What branch was your PR created for? 
2. If it was for a codebase before v0.8, will you be able to bring it up-to-date against v0.8?

@reimannf
Copy link
Author

reimannf commented Nov 2, 2022

You can see the original code that PR was prepared here: https://github.com/reimannf/dynomite/tree/redis-auth

Nevertheless, it seems that this PR is not wanted by the repo owners and I will not dump more time into it, feel free to pick it up.

@WenningQiu
Copy link

WenningQiu commented Nov 3, 2022 •
edited
Loading

You can see the original code that PR was prepared here: https://github.com/reimannf/dynomite/tree/redis-auth

Nevertheless, it seems that this PR is not wanted by the repo owners and I will not dump more time into it, feel free to pick it up.

@reimannf It turns out your change works with "AUTH <password>" command, even when it is applied to v0.8. The failure that I ran into was on "AUTH <username> <password>" command. I will see if I can adjust the patch to make it work with AUTH <username> <password> command to support ACLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Redis AUTH command
7 participants
@reimannf @axelfauvel @jyriok @yedongxin @ArthurHlt @mcouillard @WenningQiu