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

Redis authentication support #11

Closed
wants to merge 1 commit into from

Conversation

shantanuthatte
Copy link

Support for authentication with Redis support with 'requirepass' set.

@timcosta
Copy link

timcosta commented Jun 2, 2014

+1 this is super necessary.

@rauchg
Copy link
Contributor

rauchg commented Jun 2, 2014

Thoughts about just passing in the clients that you call .auth on yourself?

@rauchg
Copy link
Contributor

rauchg commented Jun 2, 2014

I want to avoid adding every single option redis could take.

@timcosta
Copy link

timcosta commented Jun 2, 2014

Understood, but from my point of view this is a fairly small change looking at the PR above, but saves a lot of time for anyone using this in a production instance, because essentially all redis production instances should be behind auth. I agree that not all options should be supported, but I feel that this one is a must.

@shantanuthatte
Copy link
Author

What tjsail33 said. +1

@reqshark
Copy link
Contributor

reqshark commented Jun 2, 2014

I think with all the script kiddies out there trying to get into our remote machines having an auth option encourages people to think about more auth, and that's a good thing, so I'm +1ing this

@rauchg
Copy link
Contributor

rauchg commented Jun 2, 2014

On the surface it looks like a simple, 3 line patch, no-brainer to add. Enhances security and a clear developer need. But in reality, it makes a crucial error handling decision on behalf of the user.

In this case you decide to throw an uncatchable exception that could bring down the process when authentication fails. This might be ok in many scenarios, but some users might want to catch that and handle it in a particular way…

It's hard to encode all these scenarios into options passed in a hash. Now we need auth but we also need an optional authHandler function. And we need to document that the default behavior is to throw the error.

I think creating the redis clients on behalf of the user is already a mistake though. I did it in the interest of usability, but I'm considering removing it now.

@reqshark
Copy link
Contributor

reqshark commented Jun 2, 2014

i'm very intersted in the socket.io adapter and I'm wondering how it would work without creating a client on behalf of the user. So you would pass in a connection object instead?

@rauchg
Copy link
Contributor

rauchg commented Jun 3, 2014

Yep you can already do that by supplying pubClient and subClient.

@rauchg
Copy link
Contributor

rauchg commented Jun 3, 2014

Something we could do is expose the clients we create on behalf of the user as part of the adapter properties, pass along the auth option, and then let them attach error listeners manually. I believe node-redis fires an error event if no callback is supplied to the command. The issue there is that you lose the context of the error.

@toddbluhm
Copy link

I think just passing in clients is the way to go, instead of creating them for the user. I say that because some users already use the redis clients for other things too.

@BrandonCopley
Copy link

How would I pass in the client? is there a sample you can show me? my redis server is remote and needs authentication.

@shantanuthatte
Copy link
Author

Here's how I do it:

var redisApp = require("redis");
var socketpub = redisApp.createClient(6379, "127.0.0.1", {auth_pass: "my password", return_buffers: true});
var socketsub = redisApp.createClient(6379, "127.0.0.1", {auth_pass: "my password", return_buffers: true});

OR

socketpub.auth("my password");
socketsub.auth("my password");

Finally,

var redis = require('socket.io-redis');
io.adapter(redis({pubClient: socketpub, subClient: socketsub}));

@BrandonCopley
Copy link

@shantanuthatte thanks!

@geoffrey
Copy link

geoffrey commented Aug 7, 2014

+1 need this !

@rauchg
Copy link
Contributor

rauchg commented Aug 8, 2014

Please look at how @shantanuthatte did it.

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 this pull request may close these issues.

None yet

7 participants