search

cyberark / conjur-api-dotnet

.NET client for the CyberArk Conjur API
Apache License 2.0
15 stars 11 forks source link

Thread safe api key authenticator #5

Closed sashaCher closed 6 years ago

sashaCher commented 6 years ago

For VaultConjurSychronizer we need that Variable.AddValue action will be a thread safe. In general an AddValue flow looks thread safe except the part of token refresh. I simplified token expiration mechanism and add to thread safety.

dividedmind commented 6 years ago

Reading through the code now it appears to me that it was already mostly thread safe, except rarely a race would happen that would cause the token to be fetched twice. That can be solved just by locking and this is what's enough for this fix.

The other part of your fix (refactoring the expiration not to use a timer) actually breaks expiration in rare cases; I used a timer specifically because it provides a monotonic timer. Real time clock is by design not intended to measure elapsed time and not guaranteed monotonic or accurate for this purpose.

dividedmind commented 6 years ago

Now that I think about it some more, the lock is overkill as well. The race can be removed simply by using Interlocked.CompareExchange when accessing tokenExpired boolean. Let me know if you need any guidance on how to do that (or if you'd rather I did it :) ).

BTW, thanks for noticing the problem!

sashaCher commented 6 years ago

Suppose we have two threads. What will happened when the first thread gets a token and then the second replaces a token before the first one uses it. Is the "old" token still valid?

dividedmind commented 6 years ago

Yes, perfectly valid.

sashaCher commented 6 years ago

How long token actually is valid from Conjur point of view after "expiration" (7 minutes 30 seconds)?

dividedmind commented 6 years ago

Tokens are valid for eight minutes; dotnet API refreshes after 7'30", some other libraries (notably Ruby API) refreshes after 4'. Either way there's no risk that a thread will end up with an invalid token due to this race (unless it can connect for 30 seconds, at which point it probably has bigger problems). The only (relatively harmless) issue might be unnecessarily refreshing the token twice on 7'30" point (by two separate threads); using atomic exchange for the boolean will ensure that can't happen.

sashaCher commented 6 years ago

Interlocked.CompareExchange has no bool version. I will need to change tokenExpired to int. Am I right?