Spring Security
  1. Spring Security
  2. SEC-1174

Race condition with stateless ticket cache

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: 3.1.0.M1
    • Component/s: CAS
    • Labels:
      None

      Description

      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

        Activity

        Hide
        Eric Bottard added a comment -

        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 ?

        Show
        Eric Bottard added a comment - 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 ?
        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.
        Hide
        Eric Bottard added a comment -

        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.

        Show
        Eric Bottard added a comment - 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.
        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.
        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Eric Bottard
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: