[NEW] module api for specifying alternate password hashing algorithms

[daniel-house]

Problem:

1. We cannot force our users to select strong passwords such as 64 byte hexadecimal strings.
2. Not everyone is content to use unsalted sha256 hashing.
3. Because machines grow more powerful and attacks grow more sophisticated, we can expect to eventually need newer and stronger algorithms.

New Feature:
A module API that will allow the password hashing algorithm to be replaced by passing a set of function pointers.
It should take full advantage of the existing ACL and should not require defining new commands such as HELLOACL.AUTH.
Changing the hashing algorithm should be transparent to the client.

Alternatives:

Looked at RedisModule_AuthenticateClientWithACLUser and related functions. Normally these don't get called until after the existing hashing algorithm is used at

redis/src/acl.c

Line 1125 in 9cb9f98

if (ACLCheckUserCredentials(username,password) == C_OK) {

Looked at adding a new command such as HELLOACL.AUTH. This would require that clients change their method of authenticating.

Looked at renaming the AUTH command so that it could be intercepted by a new implementation in a module. This would force the implementer to deal with issues that are unrelated to the hashing algorithm, such as checking the DISABLED and NOPASS flags. https://github.com/RedisLabsModules/pam_auth/blob/master/pam_auth.c overrides the AUTH command and we can see that it becomes responsible for generating appropriate responses to the client.

[madolson]

I'm not the biggest fan of building the callbacks you outlined. I think it's sort trying to optimize just this one use case, instead of building a good set of API interfaces around extending authentication. The current problems I am seeing:

1. It's hard to plug into existing ACL users. We could add more hooks around this, I don't see a problem with this.
2. It doesn't work with the HELLO command. I think this is worth doing something about.
3. Client support is bad, which is also fair. I think for your use case the renaming would be okay, but generally improving this would also be nice.

Also, I agree about the lack of salting and hashing, it's not great what Redis currently supports.

[yossigo]

I feel the same as @madolson.

Specifically regarding the hashing algorithm, I think the real issue here is not lack of flexibility but the use of SHA256 as it is. From a security standpoint, one would just expect a proper hash like bcrypt/scrypt/pbkdf2, but it was ruled out due to the performance impact on handling lots of incoming connections.

One goal of such an API should be to offload this to some process outside of Redis.

[daniel-house]

It seems to me that we need to preserve the external behaviors of the AUTH and HELLO commands while allowing them to use different authentication methods (either internally or by means of some process outside of Redis) so that the most that a client can observe is an increase in latency for the AUTH and HELLO commands.

Both authCommand and helloCommand call ACLAuthenticateUser which in turn calls ACLCheckUserCredentials, which does the heavy lifting, which consists of hashing the password and checking the result against all of the hashes on record for the user.

This makes me think that we should extend ACLCheckUserCredentials to support hooks that allow the definition of alternate authentication methods. We could do this in a way that allows the new authentication methods to block the client and make requests to external authentication services.

[daniel-house]

Here are some high level design thoughts. Please let me know what you think.

The current ACL code is built around two key functions: ACLCheckPasswordHash and ACLHashPassword.

ACLCheckPasswordHash is used when a client sets a password using ACL SETUSER <uname> #hash or deletes a password using ACL SETUSER <uname> !hash. I think this is a useful capability that should be preserved when it makes sense.

ACLHashPassword is for creating a new hash when processing ACL SETUSER <uname> >pword, for deleting a hash when processing ACL SETUSER <uname> <pword, and for authenticating a client.

If we only wanted to support alternate internal hashing methods such as PBKDF2, hooks that allowed a module to define the following 3 functions should be enough.

bool isWellFormedAclEntry(void* context, RedisModuleString *aclEntry)
bool doesPasswordMatchAclEntry(void* context, RedisModuleString *password, RedisModuleString *aclEntry)
RedisModuleString *createAclHashEntry(void *context, password)

To support external authentication such as LDAP the module could set the isWellFormed and Create hooks to NULL and provide an implementation for doesPasswordMatch that blocks the client and delegates the authentication work.

In this case the Module API might look roughly like this:

RM_setAuthHooks(isWellFormed, doesPasswordMatch, createEntry)

Extra features might be considered:

1. Redis could take responsibility for blocking the client and running a background thread so that doesPasswordMatch would only need to send and receive.
2. Redis could allow modules to define a list of authentication methods rather than an all-or-nothing replacement of SHA256. This would be particularly useful when upgrading a live Redis instance.

[madolson]

@daniel-house Sorry for ignoring you for so long, this has been rattling in the back of my head and I do think we need to address it.

I still want to hesitate on having a first class AUTH hook, but instead what I want to do is suggest the following:

1. Extend module command filtering to be able to intercept specific command calls and can choose either to execute the command or skip it. If you are going to skip it, you presumably will also send an error to the client.
2. In this intercepted context, you will also be able to manipulate the arguments before delegating to the real API call.
3. A note about this is you can chain interceptors if you really wanted, not sure if that is super useful or not.

For your specific flow, I would expect the module to do the following things:

1. Introduce a new SECUREAUTH.SETUSER command that can handle the setting of your secure passwords, and for everything else it delegates to the real ACL SETUSER. The only downside here is you have to maintain your own mapping between the two users, maybe we could also allow setting some custom metadata. You will leave the user as having an empty list of passwords.
2. Add an interceptor for the AUTH command, that checks the username and password in your secure context (potentially blocking). If the password is validated, you can call the module api to authenticate the user and skip the real AUTH call. Otherwise you can delegate the call the real implementation. In your case since the user has no passwords, it will fail and return the auth error.
3. For HELLO, you can add an interceptor for HELLO that will parse for the AUTH arguments. If it finds them, it will attempt to authenticate with the credentials and if it succeeds, remove them from the arguments and delegate to the real HELLO implementation.

I know I haven't really addressed that there isn't native support for passwords, but I think the above illustrates how it might work alright. The only missing link I see is ACL LIST and ACL GETUSER are still not supported great.

@yossigo Would also love your thoughts on this as well. I sort of want to try to prototype this, as I think the intercepting could solve some other problems as well.

[daniel-house]

Interceptors would be a major advancement for module writers and they deserve to be added to #8157. They would have so much power and flexibility, and such a wide range of applicability that I would vote to add them sooner than nearly any other enhancement.

It may be possible to use interceptors to enable alternate security solutions such as PBKDF2, but it seems like a LOT of very detailed effort that would have to be repeated by every organization that wanted to strengthen Redis security. If we do go this route we should consider publishing a configurable security module using the interceptors as an example and as proof that the interceptors are sufficient. Additionally, such a module would provide a place for community review and rapid dissemination of security patches.

[madolson]

Just as a proof of concept, I think interceptors can work. I built a simple POC using this existing module filters, madolson@e3a1bc5, and it seems to cover the AUTH case just fine.

[yossigo]

@daniel-house @madolson Apologies for taking so long, was pretty swamped as well lately.

This is a great showcase of why command filtering is so powerful (but maybe I'm biased), but this approach also has a few downsides:

• It's pretty complex
• It needs to reimplement and mimick a big chunk of Redis (AUTH, HELLO, ACL LIST, ACL GETUSER, ACL SETUSER).

It's tempting to say that adding a few API functions is a more viable option - but I don't see how this API can be so simple if we also consider the need to handle AUTH and HELLO asynchronously to avoid blocking the main loop.

Having a reference implementation / skeleton / first-party module that does this in a generic manner and can be extended makes sense.

[zeekling]

I think we should supoort auth module, that can be realized by developers who use redis for special scene.
Alse we can support some auth by default.

[madolson]

If someone has time, building a reference implementation would be helpful. I think we can investigate adding some hooks that help tactically make the experience more straightforward to implement.

[zuiderkwast]

Duplicate of or replaced by #11256 (close this one?)

[madolson]

I agree let's close this one. @daniel-house Do you want to check out the new issue and comment if it solves your use case?