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.

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

Unable to connect to Redis with correct credentials #300

Closed
iuioiua opened this issue Apr 6, 2022 · 12 comments · Fixed by #310
Closed

Unable to connect to Redis with correct credentials #300

iuioiua opened this issue Apr 6, 2022 · 12 comments · Fixed by #310

Comments

@iuioiua
Copy link

iuioiua commented Apr 6, 2022

I'm attempting to use a newly created Redis user. However, I'm getting the following error:

error: Uncaught (in promise) Error: -WRONGPASS invalid username-password pair

    throw new ErrorReplyError(line);
          ^
    at tryParseErrorReply (https://deno.land/x/redis@v0.25.4/protocol/reply.ts:205:11)
    at readReply (https://deno.land/x/redis@v0.25.4/protocol/reply.ts:55:7)
    at async RedisConnection.connectThunkified (https://deno.land/x/redis@v0.25.4/connection.ts:97:11)
    at async RedisConnection.connect (https://deno.land/x/redis@v0.25.4/connection.ts:126:5)
    at async connect (https://deno.land/x/redis@v0.25.4/redis.ts:2287:3)
    at async file://<location>

I'm essentially connecting to the following client via the following:

export const REDIS = await connect(parseURL(Deno.env.get("REDIS_URL")!));

When using my Redis default user, this code works fine. However, when using the new user, it doesn't. I know the REDIS_URL environmental variable is correct because I've used it to access Redis via redis-cli -u <url>.

Please let me know if there's any more information you need and thank you in advance.

@uki00a
Copy link
Member

uki00a commented Apr 12, 2022

@isyouaint Thanks for the report! Could you tell us the format of the URL set in the REDIS_URL variable so we can investigate the cause in more detail?

@iuioiua
Copy link
Author

iuioiua commented Apr 18, 2022

@isyouaint Thanks for the report! Could you tell us the format of the URL set in the REDIS_URL variable so we can investigate the cause in more detail?

As well, I tried connecting without using parseURL and got the same error. Format of the REDIS_URL variable is:

redis://<name>:<password>@<hostname>:<port>

@iuioiua
Copy link
Author

iuioiua commented Apr 18, 2022

@uki00a, I added a username parameter to RedisConnection.authenticate and was able to get this working. Reading this gave me the idea. Hopefully, this helps come to a solution.
Before:

private authenticate(password: string): Promise<RedisReply> {
  return sendCommand(this.writer, this.reader, "AUTH", password);
}

After:

private authenticate(username: string, password: string): Promise<RedisReply> {
  return sendCommand(this.writer, this.reader, "AUTH", username, password);
}

@Xirui
Copy link
Contributor

Xirui commented Apr 18, 2022

@isyouaint I guess you are using Redis 6.

The code in your Before block only works with Redis versions prior of 6.

@iuioiua
Copy link
Author

iuioiua commented Apr 19, 2022

Any issue with me adding this functionality in a PR?

@uki00a
Copy link
Member

uki00a commented Apr 19, 2022

@isyouaint A PR will be welcome! 👍

@OldManFroggy
Copy link
Contributor

OldManFroggy commented May 31, 2022

suggest you update RedisConnectionOptions to support username?: string and also update authenticate function to check for username and adjust redis command as needed for password/default auth "AUTH", password and if exists "AUTH", username, password

not sure if this is being progressed, if not I am more than willing push these changes as a PR let me know, as I am using a modified local repo for production currently with redis cloud.

also I am not sure where to add tests for this as there are connection tests in /tests/commands/general and /tests/commands/connection polite suggestions encouraged ;)

@uki00a @keroxp I can push a PR from my local branch anytime as its done and I am using in production. (ps. never done a PR request/push to opensource before, but my local version <branched as #300> is ready to be pushed as a PR, if I am added as a contributor.)

@uki00a
Copy link
Member

uki00a commented May 31, 2022

@scott-ling

suggest you update RedisConnectionOptions to support username?: string and also update authenticate function to check for username and adjust redis command as needed for password/default auth "AUTH", password and if exists "AUTH", username, password

Sounds good!

not sure if this is being progressed, if not I am more than willing push these changes as a PR let me know, as I am using a modified local repo for production currently with redis cloud.

I haven't worked on this yet, so you are welcome to submit a PR! 👍

also I am not sure where to add tests for this as there are connection tests in /tests/commands/general and /tests/commands/connection polite suggestions encouraged ;)

I think it would be better to add tests to /tests/commands/general

Please let me know on this issue or on Discord If you have any other questions or issues 🙂

@OldManFroggy
Copy link
Contributor

@uki00a all good, I will add a couple tests for the new situations

  1. password passed no username passed
  2. password and username passed

Will let ya know when its ready to roll, I should be able to complete this tonight once other work is done (I am on GMT)

Thanks for the quick turn-around, this was a breaking issue for me so assume its same for a few others.

-Scott

@iuioiua
Copy link
Author

iuioiua commented May 31, 2022

@scott-ling I haven’t yet started work on a PR, so I guess it’s yours 🤙🏾

@OldManFroggy
Copy link
Contributor

OldManFroggy commented May 31, 2022 via email

@OldManFroggy
Copy link
Contributor

Sorry for delay, PR made.

@uki00a uki00a linked a pull request Jun 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants