CacheServiceTicketManager threading issues

[sunnymoon]

For instance, when updating the sliding expiration, this is done by revoking and then reinserting the ticket into the Cache. If the thread gets interrupted while another thread is verifying the ticket, it will find the ticket is not in the cache and as such, fails and sends ASPXAUTH clear cookie to the browser, invalidating the authentication ticket.

[phantomtypist]

@sunnymoon when you say "updating the sliding expiration", do you mean in the web.config file?

[phantomtypist]

@sunnymoon Any chance you can get me a reproduction case (detailed/steps)?

[lanorkin]

@phantomtypist I think @sunnymoon is talking about these lines

https://github.com/apereo/dotnet-cas-client/blob/master/DotNetCasClient/CasAuthentication.cs#L950

// Extend the expiration of the cookie if FormsAuthentication is configured to do so.
if (FormsAuthentication.SlidingExpiration)
{
    FormsAuthenticationTicket newTicket = FormsAuthentication.RenewTicketIfOld(formsAuthenticationTicket);
    if (newTicket != null && newTicket != formsAuthenticationTicket)
    {
        SetAuthCookie(newTicket);
        if (ServiceTicketManager != null)
        {
            ServiceTicketManager.UpdateTicketExpiration(casTicket, newTicket.Expiration);
        }
    }
}

and subsequently about this implementation https://github.com/apereo/dotnet-cas-client/blob/master/DotNetCasClient/State/CacheServiceTicketManager.cs#L111 of UpdateTicketExpiration:

public void UpdateTicketExpiration(CasAuthenticationTicket casAuthenticationTicket, DateTime newExpiration)
{
    CommonUtils.AssertNotNull(casAuthenticationTicket, "casAuthenticationTicket parameter cannot be null.");

    RevokeTicket(casAuthenticationTicket.ServiceTicket);
    InsertTicket(casAuthenticationTicket, newExpiration);
}

I would not say it is threading issue (as Cache I believe is thread-safe per se), it's more like "transaction" issue - if thread aborted between revoke and insert, ticket will be lost

Not sure about real-life case here though, and from my perspective it is really minor issue.

It can be easily fixed: do not Revoke before Insert - key, if any exists, will be overwritten https://msdn.microsoft.com/en-us/library/4y13wyk9(v=vs.110).aspx

This method will overwrite an existing Cache item with the same key parameter.

[phantomtypist]

@lanorkin Yeah, I'm not sure why the call to revoke is in there. I'll take a look at it after the 1.1.0 release and possibly just remove the call to Revoke like you said. The Insert behavior is the same in both System.Web.Caching and System.Runtime.Caching.

As for the thread-safety comment... I agree there shouldn't be an issue, but I could never find definitive verbiage in the documentation that said that you didn't have to worry about threading. As you do, I also assume it to be thread-safe.

[lanorkin]

Re Cache thread safety from MSDN

System.Web.Caching, https://msdn.microsoft.com/en-us/library/system.web.caching.cache(v=vs.110).aspx

This type is thread safe.

System.Runtime.Caching, https://msdn.microsoft.com/en-us/library/system.runtime.caching.memorycache(v=vs.110).aspx

This type is thread safe.

[sunnymoon]

Imagine any browser makes two parallel requests to a .net application. One handled by thread 1 and another by thread 2.

Now thread 1 verifies the ticket is in the service ticket cache and proceeds to the revoke/insert code and actually executes the revoke operation, in fact removing the ticket from the cache. Then thread 1 is put asleep from the kernel threading subsystem and thread 2 actually tries to verify that the ticket exists in the cache and won't find it there because of the previous removal by thread 1. Then thread 2 actually thinks the user is NOT authenticated so proceeds to clearing the aspxauth cookie and flushes the response to the client.

This means that once thread 1 resumes its processing and actually reinserts the ticket into the cache it's already too late because thread 2 already cleared the user's authenticated session.

This, at least where I come from is called a race condition and it's clearly a multithreading issue.

It's true the cache is atomic for each operation... This code is NOT atomic in terms of the actual remove/put operation which is in fact the business function that should be atomic.

Call it transactionality if you wish... the bug is still the same.

I don't see how you never ran into this as any simple load test shows it very clearly and often.

[lanorkin]

@sunnymoon To be on the same page - is it correct that case you are describing is about requests which affects the same ticket, so real-life case is concurrent requests from the same user?

[sunnymoon]

Y3 s

On 10 Aug 2017 04:14, "Vladimir Vedeneev" notifications@github.com wrote:

@sunnymoon https://github.com/sunnymoon To be on the same page - is it correct that case you are describing is about requests which affects the same ticket, so real-life case is concurrent requests from the same user?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apereo/dotnet-cas-client/issues/40#issuecomment-321439169, or mute the thread https://github.com/notifications/unsubscribe-auth/AEJ6KNpqRzQtYfw1QuKDDpGfX_uOyNJ1ks5sWnWWgaJpZM4HqSOw .

[phantomtypist]

So we (internally at my company) ran into this issue you describe @sunnymoon.

I believe I mitigated this for .NET 4.x applications in the 1.1.0 beta release. You can review the code in the release branch for this version (linked below.) There is a cache manager (as a singleton) that wraps access to the System.Runtime.Caching calls and also includes some locking... that way even in a multi-threaded environment this scenario should be mitigated.

I didn't touch the .NET 2/3.x code route (System.Web.Caching) because I wanted to leave it alone as there was no problem with that code (see issue #54)

• https://github.com/apereo/dotnet-cas-client/blob/release/1.1.0/DotNetCasClient/State/MemoryCacheManager.cs
• https://github.com/apereo/dotnet-cas-client/blob/release/1.1.0/DotNetCasClient/State/MemoryCacheProxyTicketManager.cs
• https://github.com/apereo/dotnet-cas-client/blob/release/1.1.0/DotNetCasClient/State/MemoryCacheServiceTicketManager.cs

[phantomtypist]

I could hypothetically do the same thing with the .NET 2/3.x code route.

[sunnymoon]

None of the links you posted seem to work @phantomtypist...

[phantomtypist]

Too fast for ya ;)

Check the master branch now.

[lanorkin]

@phantomtypist

I don't see how you address the problem in question

Here https://github.com/apereo/dotnet-cas-client/blob/master/DotNetCasClient/State/MemoryCacheServiceTicketManager.cs#L107 in MemoryCacheServiceTicketManager you still have the same logic:

public void UpdateTicketExpiration(CasAuthenticationTicket casAuthenticationTicket, DateTime newExpiration)
{
    CommonUtils.AssertNotNull(casAuthenticationTicket, "casAuthenticationTicket parameter cannot be null.");

    RevokeTicket(casAuthenticationTicket.ServiceTicket);
    InsertTicket(casAuthenticationTicket, newExpiration);
}

So if thread A now is between Revoke and Insert, and thread B is querying cache for existence of ticket - thread B will not found it in cache and will clear auth cookie regardless thread A logic, so we still have race condition here if client browser issues two concurrent requests.

Moreover, I don't see logic in MemoryCacheManager class. It uses System.Runtime.Caching.MemoryCache which is thread-safe according to msdn, so there's not reason to protected simple read/write to it with lock. If it were not thread safe, then it is not correct to lock only for Set operations and not to lock on others like Get, ToList, Count, Contains etc

Also, as it is now, MemoryCacheManager used by MemoryCacheServiceTicketManager and System.Web.Caching.Cache used by CacheServiceTicketManager are behaves differently: MemoryCacheManager in Set methods doesn't update expiration if it already has item in cache. Right now it seems it should not impact overall behavior, as you try to Revoke before Insert, but that can be confusing in long perspective.