SEC-1174: Race condition with stateless ticket cache #1422

Closed
spring-issuemaster opened this Issue Jun 2, 2009 · 11 comments

1 participant

@spring-issuemaster

Eric Bottard (Migrated from SEC-1174) said:

I believe there is a race condition when using the "stateless" feature of CasAuthenticationProvider. The current code looks like this :
boolean stateless = false;

    if (authentication instanceof UsernamePasswordAuthenticationToken
        && CasProcessingFilter.CAS_STATELESS_IDENTIFIER.equals(authentication.getPrincipal())) {
        stateless = true;
    }

    CasAuthenticationToken result = null;

    if (stateless) {
        // Try to obtain from cache
        result = statelessTicketCache.getByTicketId(authentication.getCredentials().toString());
    }

    if (result == null) {
        result = this.authenticateNow(authentication);
        result.setDetails(authentication.getDetails());
    }

    if (stateless) {
        // Add to cache
        statelessTicketCache.putTicketInCache(result);
    }

and although the underlying statelessTicketCache (eg EhCache based) is certainly "synchronized", it is the whole find/authenticate/store scenario that should be atomic, or else you'll get InvalidTicketExceptions from CAS since a service ticket can get "burned" before it is put in the cache.

I would suggest to create two code paths (guarded by if(stateless)) and only synchronize in the stateless case so as to not penalize stateful attemps. Moreover, the ideal synchronization scenario would synchronize per ServiceTicket, but synchronization on Strings is known to be difficult (synchronizing on theTicket.intern() could seem like a nice idea but 1) would deadlock if CAS were to to the same thing in the same jvm/different thread and 2) would waste PermGen space).
I suppose the very correct fix could involve a synchronized WeakHashMap from ST to lock objects, or just synchronize on the Provider.

Note that this problem (although very real) can only be witnessed if you have many concurrent requests coming from a client which has not yet fulfilled a successful roundtrip for that particular ST, which it should cache, eg many concurrent ajax requests

@spring-issuemaster

Eric Bottard said:

Well, obviously I wasn't fully awake when speaking about WeakHashMap which can't use a String as a key and behave as expected.

The thing is that a serviceTicket will be burned by CAS anyway, so we can use a simple map and remove the entry after authentication is attempted
I guess one can use the following template for locking :

if (statefull) {
Object lock = getLockFor(serviceTicket);
try {
synchronized (lock) {
// Try to obtain from cache
result = statelessTicketCache.getByTicketId(authentication
.getCredentials().toString());
if (result == null) {
result = authenticateNow(authentication);
result.setDetails(authentication.getDetails());
}
// Add to cache
statelessTicketCache.putTicketInCache(result);

} finally {
removeLockFor(serviceTicket)
}
}

with getLockFor() and removeLockFor() synchronizing on this (which is ok since the lock on this is not held long, as opposed to a call to authenticateNow())

@spring-issuemaster

Luke Taylor said:

I'm afraid I don't really understand this, particularly your locking solution (implementation of getLockFor()) or the relevance/meaning of tickets being "burned by CAS anyway". It's a bit stream-of-consciousness for me :-).

I don't really see how this solves the problem you're describing. Could you explain how you expect the locks to work for individual tickets, preferably with a patch? How are the locks created and maintained? What happens if there is no lock for a ticket and the method returns null?

@spring-issuemaster

Eric Bottard said:

Tentative patch

@spring-issuemaster

Eric Bottard said:

Hello,

I have attached a tentative patched (sorry I had to copy/paste from our original patched file which had totally different formatting rules, so to avoid messing the diff I copy/pasted the interesting bits, hope I haven't messed it up, but you should get the idea).

About CAS "burning" tickets, I meant that once you ask CAS to authenticate a service ticket, if "forgets" the ticket and will thus pretend to know nothing about it afterwards

About the problem (only matters when you're in the "stateless" case) :
it can occur iff

  • on the very first attempt at authentication (before anything has been cached), you have multiple concurrent requests (particularly nasty with eg Ajax requests initiating from the front end)
  • Assuming there are two threads T1 and T2 handling requests initiated for the same service ticket "ST", let's assume the scheduler orders them like so : T1 : get from cache (ST) ==> null T2 : get from cache (ST) ==> null T1 : result = authenticateNow(ST) ==> ok, result is "ltaylor" (at this point, CAS forgets about the ticket ST) T2 : result = authenticateNow(ST) ==> InvalidTicketException since the ticket has been validated already T1 : put in cache

hence, thread T2 got kicked out although it had a perfectly valid ST.

So the solution (and the intuition confirms this) is to make the whole (cache get / do auth / cache put) interaction atomic. To do this, you can either synchronize on the CasAuthenticationManager (but this is a no go since the authenticateNow() method involves a WS call) or on the service ticket itself. But the thing is, you can't actually synchronize on the ST because they will be different String instances, even for the same ST "value". So you have to synchronize on something, key'ed by the ST value, hence my proposed solution. It does create and store a lock object for each given ST value, using a HashMap (uses String equals() internally, not ==). Note that the getLockFor() method has to be synchronized on this, but this is ok since this does not last long (as opposed to authenticateNow()).

I hope all this makes sense,
eb.

@spring-issuemaster

Eric Bottard said:

I should mention we DID witness this bug, in the setup explained above (very first interaction involving multiple requests (ajax))

@spring-issuemaster

Luke Taylor said:

Thanks for the update patch. It still seems to be missing some bits, but I get the picture. I was confused by the reference to CAS removing the tickets, since if you are using locks to prevent multiple calls to authenticateNow(), then that is really independent of CAS's treatment of the service ticket and the fact that it is only (normally) valid for a single use. I still don't see any connection.

I guess some information on the application would also be useful. What kind of application is it and why does it have multiple Ajax requests hitting the CAS provider at the same time with the same service ticket?

@spring-issuemaster

Eric Bottard said:

Thanks for the update patch. It still seems to be missing some bits, but I get the picture.
You're welcome :)

I was confused by the reference to CAS removing the tickets, since if you are using locks to prevent multiple calls to authenticateNow(), then that is really independent of CAS's treatment of the service ticket and the fact that it is only (normally) valid for a single use. I still don't see any connection.
The point is that if you allow the cache.get() and cache.put() to be interspeced, the code is not "Thread Safe", particularly because CAS's answer is not idempotent (because it forgets about the ST)

I guess some information on the application would also be useful. What kind of application is it and why does it have multiple Ajax requests hitting the CAS provider at the same time with the same service ticket?

It's a standard MVC application, but with the particularity that there can be multiple "simultaneous" requests that need to be authenticated at once. Maybe this shows a deeper "problem" which is that there should be some synchronization on the HttpSession when Spring Security stores the Authentication Object ?

@spring-issuemaster

Luke Taylor said:

OK, I thought you meant that the choice of locking strategy - "so we can use a simple map" was connected with the CAS behaviour. I understand the thread safety issue.

If it's a standard MVC app, and you are using an HttpSession, why are you using the stateless ticket cache (which is really a substitute session mechanism for stateless clients)?

The issue of concurrency and authentication has come up before (see SEC-1007 for example) and we would generally say that it isn't Spring Security's responsibility to deal with concurrent requests but up to the client to serialize them in order to establish an authenticated session before it proceeds. This doesn't just concern authentication, but raises issues about starting multiple concurrent sessions, for example.

@spring-issuemaster

Eric Bottard said:

Actually, we're using the stateless cache for web service calls (which are indeed stateless), triggered by multiple ajax requests from a first app.
I see what you mean regarding external session synchronization, I was just wondering if this particular problem could be adressed at a higher level in Spring Security.

It seems like I can't make myself understood about our particular multi application architecture (sorry), but I can guarantee you that the the race condition is actually present (the patched CasAuthenticationProvider does indeed prevent it, but a second pair of eyes would be useful to make sure that it does not introduce other problems). As you can see, I tried to minimize contention and we should be alright regarding memory leaks, but you never know.

@spring-issuemaster

Luke Taylor said:

Postponing. I'm still not sure that this is something that should be addressed at the server side. If you have a client which is sending multiple concurrent requests with the same service ticket (before it has been validated by the server), then that doesn't seem right. It should really be the client's responsibility to establish the authenticated session (in this case, the caching of the session ticket by Spring Security) before proceeding.

@spring-issuemaster

Luke Taylor said:

Leaving for now, as I still think it is up to the client to establish a secure connection (via an authentication protocol) without sending parallel requests to the secured URL.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.M1 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment