Timer leak and potential JWT/OAuth2 expiry in TimerBasedCredentialRefresher [OAuth2] #1639
Description
Describe the bug
Using the TimerBasedCredentialRefresher in conjunction with the AutomaticRecoveryEnabled feature provided by ConnectionFactory results in System.Timers.Timer leakage everytime the connection is lost and recovered again afterwards.
This happens due to the following circumstances:
- On successful recovery a new Connection is instantiated which subsequently calls Open(...), then StartAndTune(), then MaybeStartCredentialRefresher() and finally the Register(...) method on the TimerBasedCredentialRefresher. The Register(...) method is passed the factories CredentialsProvider and the new Connection's notify callback.
- The TimerBasedCredentialRefresher will lookup if a registration already exists by using TryAdd onto the ConcurrentDictionary. However while doing so it will kick-off a new Timer by calling scheduleTimer(...) which is then scheduled to provide the new Connection callback with credentials while the old(er) ones are still running. Furthermore the _registration ConcurrentDictionary will only contain the oldest Timer because TryAdd will never succeed when a certain key is already present in the dictionary.
This has multiple consequences:
- Each time a connection is recovered an additional Timer object is created while the old ones are not cleaned up.
- Only the newly created timer will "serve" the current Connection and therefore will kick-off a new Timer cycle. This can potentially lead to expired tokens on the RabbitMQ side in certain timing conditions.
Reproduction steps
For consequence 2.:
Suppose your JWT has a lifetime of 15 minutes. Therefore the TimerBasedCredentialRefresher will refresh with a new JWT every 10 minutes (2/3 of the tokens lifetime).
You initially connect to the broker at time X. Connection is lost after X + 9 min and recovered after a few seconds. Due to the recovery a new Timer will be scheduled to provide credentials in X + 9 min + 10 min which is X + 19 min. Therefore the initial token will have expired for a time frame of 4 minutes until the new Timer makes a refresh. Actions in this time frame may lead to more serious consequences since the client no longer has valid auth. The old Timer has no effect because it will only call a notify callback of a Connection that has already been abandoned.
Expected behavior
Timer objects shall not created by each Register(...) call, but instead the initial Timer only passed the notify callback of the new Connection instance.
This will also ensure that tokens don't expire in certain time frames and timing conditions as described above.
Additional context
I implemented an own version of ICredentialsRefresher with a fix so if you're interested in that I might create a pull request if desired.