AliBazzi / IdentityServer4.Contrib.RedisStore

A persistence layer using Redis DB for operational data and for caching capability for Identity Server 4
https://www.nuget.org/packages/IdentityServer4.Contrib.RedisStore
MIT License
137 stars 48 forks source link

RedisCommandException: Multi-key operations must involve a single slot while storing PersistedGrant #28

Open vadimdr opened 4 years ago

vadimdr commented 4 years ago

Hi,

I am getting the following exception while trying to perform the Authorization code flow with IdentityServer4 configured to use the Redis as an operational store:

RedisCommandException: Multi-key operations must involve a single slot; keys can use 'hash tags' to help this, i.e. '{/users/12345}/account' and '{/users/12345}/contacts' will always be in the same slot
IdentityServer4.Contrib.RedisStore.Stores.PersistedGrantStore.StoreAsync(PersistedGrant grant)
IdentityServer4.Stores.DefaultGrantStore<T>.StoreItemAsync(string key, T item, string clientId, string subjectId, DateTime created, Nullable<DateTime> expiration)
IdentityServer4.Stores.DefaultGrantStore<T>.CreateItemAsync(T item, string clientId, string subjectId, DateTime created, int lifetime)
IdentityServer4.ResponseHandling.AuthorizeResponseGenerator.CreateCodeFlowResponseAsync(ValidatedAuthorizeRequest request)
...

My instance of IdentityServer4 based server is configured to use Redis as an operational store:

.AddOperationalStore(options =>
                {
                    options.KeyPrefix = IdSrvConstants.IdentityServerSolution;
                    options.RedisConnectionMultiplexer = scope.ServiceProvider.GetRequiredService<IConnectionMultiplexer>();
                });

and the Redis is AWS ElasticCache clustered Redis instance. The same code works fine on a single-node Redis.

Based on my understanding, the problem is related to the way the PersistedGrantStore generates the key's names - it does not use the keys hash tags (see https://redis.io/topics/cluster-spec#keys-hash-tags) in order to make sure all the keys will involve a single slot while performing a single Redis transaction.

AliBazzi commented 4 years ago

Can you elaborate more on the mode of Elasticache instance, is Cluster mode On or Off ?

AliBazzi commented 4 years ago

Another question, what is the value of IdSrvConstants.IdentityServerSolution ?

vadimdr commented 4 years ago

Elasticache instance is in clustered mode. (Cluster mode on). There are 3 nodes and 1 shard. The constant value is IdM. We have switched the ElastiCache to non-clustered mode to test (Cluster mode off) and now it's working fine ...

I can certainly live with that in non-production environments, but for production, a cluster is a mandatory requirement ...

AliBazzi commented 4 years ago

I'm trying to figure out why it's not working in Cluster mode on. I will keep digging, I though IdSrvConstants.IdentityServerSolution may have { } as a value, but looks it's not.

vadimdr commented 4 years ago

I was actually thinking that putting the KeyPrefix with { } might resolve the issue because then all the keys will be hashed based on KeyPrefix only and will fall in the same slot. I will try to change the code and see if it has any impact...

AliBazzi commented 4 years ago

is it possible that the SubjectId format on your side is formatted like {...} ? maybe it's a GUID that you convert into a string by calling .ToString() which will result in default formatting as {....} ? To clarify SubjectId is sub claim.

vadimdr commented 4 years ago

This is how the Redis store looks like when I run it locally: image

As you can see, all sub are GUIDs, but they are formated as a GUID without any {}. The name of the client also doesn't contain the {}

vadimdr commented 4 years ago

I have tried to wrap the PrefixKey with {}, so it will be {IdM} and it's actually working with clustered ElastiCache

AliBazzi commented 4 years ago

but what will happens is that hashing algo will result in storing all the key/values in one shard only of your cluster, so I don't think this is the outcome you want.

I'm looking at the code, it will be complicated to do this as a fix, rather it will need big investment from my side to implement that here.

I will try to dedicate a time for this, but I can't promise to be anytime soon, sorry for that, feel free to PR if you want to drill into that.

vadimdr commented 4 years ago

Understood, thank you for your efforts. In the meanwhile, since I have only 1 shard in my ElastiCache cluster anyway, this resolution will work fine in this use case.

simonpinn commented 3 years ago

I've also just run into this issue, similar AWS config - has there been any update to this @AliBazzi ?

simonpinn commented 3 years ago

Can confirm - works when prefix is wrapped in curlies

AliBazzi commented 3 years ago

Hi @simonpinn wrapping the prefix will solve the problem but that will leave your Redis cluster unbalanced and not taking advantage of the clustered Redis

maybe you can use non-clustered instance so you don't waste money/resources on unused nodes, I found out that "fixing" this will leave current data in Redis unreachable, was thinking of wrapping the sub with curlies last year but found out that will leave current users in "logged-out" state cause their old data are no longer reachable.

ShoSashko commented 2 years ago

@AliBazzi Any updates on the issue?