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

Allow authenticating with a username to redis #1488

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

Allow authenticating with a username to redis #1488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a110d3
Allow authenticating with a username to redis
maddymeows Jun 2, 2024
875358d
Fix regression in edebb9a
maddymeows Jun 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 22 additions & 12 deletions src/apps/relay/dbdrivers/dbd_redis.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
struct _Ryconninfo {
char *host;
char *dbname;
char *user;
char *password;
unsigned int connect_timeout;
unsigned int port;
Expand All @@ -64,6 +65,9 @@
if (co->dbname) {
free(co->dbname);
}
if (co->user) {
free(co->user);
}
if (co->password) {
free(co->password);
}
Expand Down Expand Up @@ -118,13 +122,13 @@
} else if (!strcmp(s, "database")) {
co->dbname = strdup(seq + 1);
} else if (!strcmp(s, "user")) {
;
co->user = strdup(seq + 1);
} else if (!strcmp(s, "uname")) {
;
co->user = strdup(seq + 1);
} else if (!strcmp(s, "name")) {
;
co->user = strdup(seq + 1);
} else if (!strcmp(s, "username")) {
;
co->user = strdup(seq + 1);
} else if (!strcmp(s, "password")) {
co->password = strdup(seq + 1);
} else if (!strcmp(s, "pwd")) {
Expand Down Expand Up @@ -163,9 +167,6 @@
if (!(co->host)) {
co->host = strdup("127.0.0.1");
}
if (!(co->password)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

@maddymeows maddymeows Jun 3, 2024 •
edited

Choose a reason for hiding this comment

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

Originally during testing I created an extra branch if (!(co->user)) { co->user = strdup(""); }.

However, I found out that it made coturn always authenticate with a username and password, which would break for redis servers that don't have the ACL feature.

That's because the branches using password (which I ended up copying for username branches) checked if the pointer to password was set, which it always was with the statement I just removed, rather than if password contained a useful value. So the password conditionals haven't been working for a while, coturn just discarded the error that redis threw out and continued on (which is fine, but not ideal).

Hence also changes to make those branches co->password && strlen(co->password) rather than just co->password, just in case. I could effectively change them to co->password && co->password[0] but strlen felt nicer.

Error handling of missing auth doesn't seem to be handled fully correctly from what I grasp, but happens moreso as a side effect, the side effect of using the side effect is it never warns about if the password is needed which is why there are no logs complaining about use of AUTH when wasn't needed.

co->password = strdup("");
}
}

return co;
Expand Down Expand Up @@ -225,8 +226,12 @@
if (!rc) {
TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "Cannot initialize Redis DB async connection\n");
} else {
if (co->password) {
turnFreeRedisReply(redisCommand(rc, "AUTH %s", co->password));
if (co->password && strlen(co->password)) {
if (co->user && strlen(co->user)) {
turnFreeRedisReply(redisCommand(rc, "AUTH %s %s", co->user, co->password));
} else {
turnFreeRedisReply(redisCommand(rc, "AUTH %s", co->password));
}
}
if (co->dbname) {
turnFreeRedisReply(redisCommand(rc, "select %s", co->dbname));
Expand Down Expand Up @@ -268,7 +273,7 @@
}
}

ret = redisLibeventAttach(base, co->host, co->port, co->password, atoi(co->dbname));
ret = redisLibeventAttach(base, co->host, co->port, co->user, co->password, atoi(co->dbname));

Check warning

Code scanning / PREfast

'co->dbname' could be '0': this does not adhere to the specification for the function 'atoi'. Warning

'co->dbname' could be '0': this does not adhere to the specification for the function 'atoi'.
Copy link
Author

Choose a reason for hiding this comment

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

co->dbname is never null because a default is given in RyconninfoParse, I don't know how to silence this.


if (!ret) {
TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "Cannot initialize Redis DB connection\n");
Expand Down Expand Up @@ -348,8 +353,13 @@
}
redisFree(redisconnection);
redisconnection = NULL;
} else if (co->password) {
void *reply = redisCommand(redisconnection, "AUTH %s", co->password);
} else if (co->password && strlen(co->password)) {
void *reply;
if (co->user && strlen(co->user)) {
reply = redisCommand(redisconnection, "AUTH %s %s", co->user, co->password);
} else {
reply = redisCommand(redisconnection, "AUTH %s", co->password);
}
if (!reply) {
if (redisconnection->err && redisconnection->errstr[0]) {
TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "Redis: %s\n", redisconnection->errstr);
Expand Down
30 changes: 23 additions & 7 deletions src/apps/relay/hiredis_libevent2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ struct redisLibeventEvents {
int rev_set, wev_set;
char *ip;
int port;
char *user;
char *pwd;
int db;
};
Expand Down Expand Up @@ -213,7 +214,7 @@ void send_message_to_redis(redis_context_handle rch, const char *command, const

///////////////////////// Attach /////////////////////////////////

redis_context_handle redisLibeventAttach(struct event_base *base, char *ip0, int port0, char *pwd, int db) {
redis_context_handle redisLibeventAttach(struct event_base *base, char *ip0, int port0, char *user, char *pwd, int db) {

struct redisLibeventEvents *e = NULL;
redisAsyncContext *ac = NULL;
Expand Down Expand Up @@ -248,6 +249,9 @@ redis_context_handle redisLibeventAttach(struct event_base *base, char *ip0, int
e->base = base;
e->ip = strdup(ip);
e->port = port;
if (user) {
e->user = strdup(user);
}
if (pwd) {
e->pwd = strdup(pwd);
}
Expand Down Expand Up @@ -276,9 +280,15 @@ redis_context_handle redisLibeventAttach(struct event_base *base, char *ip0, int
e->wev_set = 1;

// Authentication
if (redis_le_valid(e) && pwd) {
if (redisAsyncCommand(ac, NULL, e, "AUTH %s", pwd) != REDIS_OK) {
e->invalid = 1;
if (redis_le_valid(e) && pwd && strlen(pwd)) {
if (user && strlen(user)) {
if (redisAsyncCommand(ac, NULL, e, "AUTH %s %s", e->user, e->pwd) != REDIS_OK) {
e->invalid = 1;
}
} else {
if (redisAsyncCommand(ac, NULL, e, "AUTH %s", e->pwd) != REDIS_OK) {
e->invalid = 1;
}
}
}

Expand Down Expand Up @@ -351,9 +361,15 @@ static void redis_reconnect(struct redisLibeventEvents *e) {
e->invalid = 0;

// Authentication
if (redis_le_valid(e) && e->pwd) {
if (redisAsyncCommand(ac, NULL, e, "AUTH %s", e->pwd) != REDIS_OK) {
e->invalid = 1;
if (redis_le_valid(e) && e->pwd && strlen(e->pwd)) {
if (e->user && strlen(e->user)) {
if (redisAsyncCommand(ac, NULL, e, "AUTH %s %s", e->user, e->pwd) != REDIS_OK) {
e->invalid = 1;
}
} else {
if (redisAsyncCommand(ac, NULL, e, "AUTH %s", e->pwd) != REDIS_OK) {
e->invalid = 1;
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/apps/relay/hiredis_libevent2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ typedef void *redis_context_handle;

#if !defined(TURN_NO_HIREDIS)

redis_context_handle redisLibeventAttach(struct event_base *base, char *ip, int port, char *pwd, int db);
redis_context_handle redisLibeventAttach(struct event_base *base, char *ip, int port, char *user, char *pwd, int db);

void send_message_to_redis(redis_context_handle rch, const char *command, const char *key, const char *format, ...);

Expand Down
2 changes: 1 addition & 1 deletion src/apps/relay/mainrelay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3045,7 +3045,7 @@ int main(int argc, char **argv) {
TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "System enable num is %lu\n", get_system_active_number_of_cpus());
}

memset(&turn_params.default_users_db, 0, sizeof(default_users_db_t));
memset(&turn_params.default_users_db.ram_db, 0, sizeof(ram_users_db_t));
turn_params.default_users_db.ram_db.static_accounts = ur_string_map_create(free);

struct uoptions uo;
Expand Down