Support REDIS AUTH command by axelfauvel · Pull Request #576 · Netflix/dynomite

axelfauvel · 2018-06-27T14:21:10Z

Hello,

According to this issue : #46

We have implemented AUTH command in Dynomite

Enjoy :)

blogumi · 2018-07-05T06:19:42Z

I support the push to get this integrated. Has this been pulled into dev branch?

LukeCarrier · 2018-10-11T16:54:29Z

I've rebased @axelfauvel's work on top of the style changes:

https://github.com/LukeCarrier/dynomite/tree/redis-auth

gsambasiva · 2018-12-17T18:13:37Z

@axelfauvel @smukil Is there any chance to rethink about this issue.

axelfauvel · 2018-12-18T08:24:05Z

@axelfauvel @smukil Is there any chance to rethink about this issue.

Hi @gsambasiva, i would love to see this implemented but i haven't received feedbacks :(

timvaillancourt · 2019-07-05T13:12:46Z

Bumping this. Would love to use this project but AUTH is a blocker

smukil · 2019-07-08T18:27:57Z

Reviewing this now. Will get back shortly with comments.

smukil

Thanks for submitting this patch. I've done a first pass. After these changes are addressed, I'll have another pass then approve.

smukil · 2019-07-08T19:39:37Z

 }

+static struct mbuf *
+get_mbuf(struct msg *msg)


I would call this get_or_create_mbuf(struct msg*) and move it to dyn_message.h/c.

smukil · 2019-07-08T19:44:31Z

+                }
+            }
+            auth_reply(ctx, conn, req, "-ERR invalid password\r\n");
+        } else {


You can remove this else condition and just do:

... ... auth_reply(ctx, conn, req, "-ERR invalid password\r\n"); } auth_reply(ctx, conn, req, "-NOAUTH Authentication required\r\n"); return true; } ... ...

smukil · 2019-07-08T19:57:59Z

+}
+
+static void
+auth_reply(struct context *ctx, struct conn *conn, struct msg *smsg, const char *usr_msg)


Could you move auth_reply() and get_password() to a new file called src/proto/dyn_redis_auth.c ?

And have a top level function called authenticate_conn(struct conn*), which can be called from the Handle "AUTH requirepass\r\n" case.

smukil · 2019-07-08T20:01:23Z

+    msg->done = 1;
+
+    conn_event_add_out(conn);
+    TAILQ_INSERT_TAIL(&conn->omsg_q, smsg, c_tqe);


This seems like a race. You'd need to insert it to the omsg_q before doing an event notification.

Also, prefer to use the conn_enqueue_outq() helper which would eventually call req_client_enqueue_omsgq().

smukil · 2019-07-08T20:06:00Z

 	server_failure(ctx, datastore);
 }

+static void


Move redis_auth() to src/dyn_redis_auth.c. We shouldn't be mixing redis specifc code in generic files. (There are other instances of this in existing code that need to be cleaned up as well, but when introducing new code, let's try to keep it as clean as possible).

smukil · 2019-07-08T20:11:44Z

    conn_pool_connected(conn->conn_pool, conn);

    log_notice("%s connected ", print_obj(conn));
+    redis_auth(ctx, conn);


Instead of calling redis_auth() directly here, we'd need to add some indirection to keep it datastore agnostic.
You'd need to add a function like datastore_auth() and call into redis_auth() if Dynomite is configured for Redis.

It's fairly simple to do, and you can follow this example of how I added a datastore agnostic rewrite_query() function:
68aad15

grep for g_rewrite_query and see all the places it's used to understand how to do this.

smukil · 2019-07-08T20:13:07Z

    log_info("%s %s RECEIVED %s", print_obj(c_conn), print_obj(req), print_obj(rsp));
-
+
+    if (c_conn->type == CONN_SERVER) {


Why is this block of code necessary?

smukil · 2019-07-08T20:18:02Z

+
+static void
+auth_reply(struct context *ctx, struct conn *conn, struct msg *smsg, const char *usr_msg)
+{


On the top of the new file src/proto/dyn_redis_auth.c, could you add some comments about how Dynomite performs this authentication.

Eg: On Dynomite startup, the server authenticates with the datastore by itself. And on each client connection, it authenticates on the first AUTH command from the client. (add detail as you see fit)

smukil · 2019-07-08T20:19:41Z

@axelfauvel I've submitted a review above.

axelfauvel · 2019-07-15T08:38:35Z

@smukil : unfortunately the person who made the code for this PR is gone and we don't have resources that can handle it anymore.

Nethertheless, I agree on letting you fix the code and merging it

nCore · 2019-10-10T09:31:38Z

@axelfauvel @smukil I can take this and try to merge it properly to current version of dynomite. Any objections?

axelfauvel · 2019-10-10T09:43:51Z

@nCore : Au contraire ! Go ahead :)

@axelfauvel

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.

yedongxin · 2020-04-30T02:54:32Z

hello,any news about this merging ?

yedongxin · 2020-04-30T03:10:51Z

@nCore：互惠生！前进：）

hello,any news about this merging ?

@axelfauvel

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.

@axelfauvel

…t/682daa32a80396f9522c390d9ffff277df3bd953.patch by W. Qiu) 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.

@axelfauvel

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.

implementing REDIS AUTH

7aa41a4

smukil self-requested a review October 12, 2018 22:18

smukil added the newfeature label Oct 12, 2018

axelfauvel mentioned this pull request Mar 27, 2019

Support Redis AUTH command #46

Open

smukil reviewed Jul 8, 2019

View reviewed changes

smukil assigned axelfauvel Jul 8, 2019

smukil mentioned this pull request Jul 9, 2019

Use 'ssh-keygen-t rsa-f' to generate the 'dynomite. Pem' file. Once the password is set, the startup fails #680

Closed

reimannf mentioned this pull request Dec 13, 2019

Redis AUTH support #748

Open

WenningQiu mentioned this pull request Oct 31, 2022

AUTH command not supported #814

Closed

LeonYanwj approved these changes Sep 20, 2023

View reviewed changes

		log_info("%s %s RECEIVED %s", print_obj(c_conn), print_obj(req), print_obj(rsp));


		if (c_conn->type == CONN_SERVER) {

Conversation

axelfauvel commented Jun 27, 2018

Uh oh!

blogumi commented Jul 5, 2018

Uh oh!

LukeCarrier commented Oct 11, 2018

Uh oh!

gsambasiva commented Dec 17, 2018

Uh oh!

axelfauvel commented Dec 18, 2018

Uh oh!

timvaillancourt commented Jul 5, 2019

Uh oh!

smukil commented Jul 8, 2019

Uh oh!

smukil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smukil commented Jul 8, 2019

Uh oh!

axelfauvel commented Jul 15, 2019

Uh oh!

nCore commented Oct 10, 2019

Uh oh!

axelfauvel commented Oct 10, 2019

Uh oh!

yedongxin commented Apr 30, 2020

Uh oh!

yedongxin commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants