Allow authenticating with a username to redis #1488
Conversation
|
Thank you @maddymeows ! Could you please rebase the PR and also explain if you tested with new and old redis. |
70e9221 to
875358d
Compare
|
Sorry for tagging the regression fix alongside this PR, came up in testing after rebasing.
Done
I've tested on debian 10 which has redis dated before ACLs were introduced, works fine as long as you don't configure a username.
As far as I know, no |
| @@ -163,9 +167,6 @@ static Ryconninfo *RyconninfoParse(const char *userdb, char **errmsg) { | |||
| if (!(co->host)) { | |||
| co->host = strdup("127.0.0.1"); | |||
| } | |||
| if (!(co->password)) { | |||
There was a problem hiding this comment.
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.
| @@ -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'.
There was a problem hiding this comment.
co->dbname is never null because a default is given in RyconninfoParse, I don't know how to silence this.
|
Hi @maddymeows ! Could you please update the PR so I can merge it into upcoming version? Thanks |
875358d to
0bb2154
Compare
|
@eakraly done! |
|
This commit breaks unauthenticated redis connections ("select " is not sent if password is not set). |
|
Fixes a regression pointed out at <#1488 (comment)>. A regression was introduced in the last PR where the dbname parameter was not respected if using redis without authentication. The logic for sending the select command responsible for switching to the correct database was wrongly guarded behind authentication being provided. This PR flattens the control flow so the select command is always sent, whether using authentication or not.
I needed this
I just looked for occurrences of password and added username branches there. Let me know if there's anything wrong, I'm not proficient in C at all.