StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.88k stars 1.51k forks source link

Question about key rotation & AuthenticationFailure #2484

Open dxynnez opened 1 year ago

dxynnez commented 1 year ago

Hello team,

I have 'ssl=True,abortConnect=False' in the connection string. I have a few questions regarding the key rotation:

1) I was told that when the key getting rotated, a RedisConnectionException with FailureType equals to 'AuthenticationFailure' would get thrown. But in my case the failure type is alwasy 'SocketClosed' for a original connected mux.

2) I was told that the 'the ConfigurationOptions will be respected after the initial connection, updating User and Password at any point will be observed when reconnecting' , but that doesn't seem to be the case if the key was rotated before any connection was established successfully - I have the following code:

    private readonly IConnectionMultiplexer mux;
    private readonly ConfigurationOptions options;
    private readonly Lazy<LoadedLuaScript> lazyLoadedLua = new Lazy<LoadedLuaScript>(() => "#somescript#".Load(this.mux.GetServer(this.mux.GetDatabase().IdentifyEndpoint());

    mux = ConnectionMultiplexer.Connect(options);
    try
    {
        lazyLoadedLua.Value;
    }
    catch (RedisConnectionException redisConnectionException) when (redisConnectionException.FailureType == ConnectionFailureType.AuthenticationFailure)
    {
        this.options.Password = "#RotatedPassword#"
    }

When calling lazyLoadedLua.Value, the RedisConnectionException with FailureType equals to 'AuthenticationFailure' does get thrown, but changing the key on the fly doesn't seem to be respected by the mux as all the subsequent 'Lazy#Value' calls still failed with RedisConnectionException(AuthenticationFailure) - I did reset the Password to the right one.

3) regarding the setter of ConfigurationOptions#Password - the implementation seems to have bugs as the value set by one thread isn't guaranteed to be seen by other threads - don't we need some memory fence for it?

NickCraver commented 1 year ago

The config stuff works as described, and works cross-thread since there's only 1 instance in play...but that's not what your issue is :)

When a Lazy<T> throws an exception in the getter it will re-throw that exception forever. It's not trying until success, it just caches the exception and re-throws for every .Value access. That's what you're hitting here. You'd need to re-create the lazy in your catch or some other approach (I can't see your big picture here) to handle what's happening.

dxynnez commented 1 year ago

aha yeah I was expecting the Lazy to not cache exception but seems like it does.

How heavy is the Load operation? Would loading the same script multiple times essentially result in a noop as far as the redis server is concerned, since the loaded script is basically the sha1 hash? I am wondering if I can just set the Lazy to PublicationOnly. I don't really want to re-create the lazy on-the-fly as then I will need some memory fence to make sure the update is going to be seen by other threads.

What about the 1) and 3)?

dxynnez commented 1 year ago

@NickCraver sorry to ping you directly - am just trying to see if you could share more on the above questions. Thank you!

NickCraver commented 1 year ago

@mgravell ^ current state here? I know this has shifted a few times

dxynnez commented 1 year ago

Poking the thread here - seem like the managed identity support has been GA'ed (though I had no luck getting it to work) so secret rotation should no longer be needed.

But I am still quite interested in the above ask about loading the LUA script - would loading the same script multiple times a heavy operation on the server side, or only the first load is doing the actual work while the rest would not actually write anything to the server, even if the sciprt-load are issued by different clients (from different workers)?