Adding prefix `namespace` to REDIS set keys messes up permissions

[yuvalhvim]

Describe the bug When setting a key in redis via keyv, keyv will also add the key to a set. This set will have a name consisting of a constant and unchangeable prefix: namespace:.

We enforce permissions by key prefixes, so process some_process could only operate on keys starting with some_process: prefix.

Adding namespace: to the set name throws an error since the process doesn't have permissions to this prefix.

Expected behavior The prefix of the set name should be configurable. It's already looks like this:

_getNamespace() {
    return `namespace:${this.namespace}`;
}

the prefix should be configurable via the options param in the constructor IMHO

[jaredwray]

@yuvalhvim - can you please provide a set of code that shows this issue and also an example of what you believe it should be like?

[yuvalhvim]

@yuvalhvim - can you please provide a set of code that shows this issue and also an example of what you believe it should be like?

The issue is not in code, it is the fact that we manage our REDIS permissions (and I assume others as well) by key prefixes, and having @keyv/redis set a key with the not-configurable name namespace:${this.namespace} throws an error for lack of permissions (I'm talking about the SET of keys used for clear).

After thinking about it a little bit, I do think it is a bit weird to have the user configure something which is purely internal implementation. The user should live happily without knowing that there is a set somewhere keeping track of keys.

So one solution I could think of is first change _getNamespace function name and make sure it puts the user namespace as prefix, and make it something like:

_getKeysSetPrefix() {
    return `${this.namespace}:keys.set.ctl`
}

This could potentially create collisions, but technically so does the current solution.

Another option is to forget everything I wrote at the beginning and do let the user configure it, keeping it a bit general, so maybe Keyv's constructor could have in its options param something like innerControlKeysPrefix: string.

@jaredwray wdyt?

[jaredwray]

@yuvalhvim - do you have example code that shows this conflict as I dont see how the namespace is causing issues with Redis.

[yuvalhvim]

It is redis configuration and not code which makes this happen. For example, say we have a redis.conf file containing this:

user my_user +@all -@dangerous ~my_user_* on >my_password

my_user will only have permissions to keys with my_user_ prefix.

This is a valid way of enforcing permissions with redis. Anyone who uses this method of permissions cannot use keyv as it is because of the set name

[jaredwray]

@yuvalhvim - interesting and is not something that we have supported before. I currently do not know what would make sense architecture wise to implement this. One thing I will say is that you can set your own namespace with each new Keyv instance which could make it possible to select for each user.

const joeCache = new Keyv('redis://user:pass@localhost:6379', { namespace: 'joe' });

I don't know if this is something I would support changing in the current system but happy to leave it here for an upvote over the next 30 days if more people see this as a major issue.

[yuvalhvim]

I'm using the { namespace: 'joe' } parameter, but it's not enough, as the name of the set keeping track of all the keys to support a clear operation has a constant "namespace" prefixed to it.

Meaning that while all keys that I set are named joe:key1, joe:key2, the set keeping track of these keys is named namespace:joe, to which I don't have permissions for

[jaredwray]

@yuvalhvim - I see what you are saying. I am wondering if it would be better to remove the namespace word moving forward which would solve some of this.

[yuvalhvim]

It will definitely solve it 😃

Maybe the responsibility for clearing keys should not reside in keyv code (tho removing it would be a breaking change, and a bad one for those counting on it 😅)

I think one of these options should be implemented IMHO:

1. The removal of 'namespace' from the set as you suggested
2. Make it configurable is some way, like: new Keyv('redis://usr:pass@host:6379', { namespace: 'joe', innerKeysPrefix: 'iKnowNotToUseThisPrefix' })
3. Name the set something like <actualNameSpace>:keyv.keys.ctl so permissions will be kept, and the chances of collision is low

@jaredwray WDYT? 🙂

[jaredwray]

Here is what I am thinking in the system is to allow the namespace to be modified which should allow clear also and be backwards compatible. Something similar such as what you outlined but a bit more programable so you can set a function to generate the namespace if you want.

const keyv = new Keyv('redis://usr:pass@host:6379', { namespace: 'joe', getKeyPrefix: (key) => { return `${this.opts.namespace}:${key}`});

This would allow to replace the entire function with what you want and make it much more scalable. If you just set the namespace then it will work the same.

Still looking into the ramifications of this and how that would scale across all adapters so I don't know if this would be accepted at this time. FYI.

We have seen little to no asks around this also which is concern to add in complexity around it. Should have a final decision if we are going to accept this change end of next week.

[jaredwray]

@yuvalhvim - wanted to get your thoughts on this? If so, will re-open.

[yuvalhvim]

Sorry, don't know how I missed all of this 😅 I think your solution is good, and would like to know what you have decided 🙂