SessionManagementFilter thread safety

[daniloarcidiacono]

I'm facing some problems with concurrent session management, in particular when multiple concurrent requests are performed. Using this configuration:

 http
    .sessionManagement()
        .sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)
        .maximumSessions(1)
            .sessionRegistry(sessionRegistry)
            .maxSessionsPreventsLogin(true)

a given user can create only one session at a time. It works, but when I try to login multiple times with many threads, the checks performed by ConcurrentSessionControlAuthenticationStrategy are bypassed. Looking at the implementation it seems that this class is not designed with thread-safety in mind:

    final List<SessionInformation> sessions = sessionRegistry.getAllSessions(
            authentication.getPrincipal(), false);

    int sessionCount = sessions.size();
    int allowedSessions = getMaximumSessionsForThisUser(authentication);

    if (sessionCount < allowedSessions) {
        // They haven't got too many login sessions running at present
        return;
    }

    if (allowedSessions == -1) {
        // We permit unlimited logins
        return;
    }

Using an aspect to wrap the session authentication strategy inside a synchronized block fixes the problem, but I wonder if there is a better approach (or some configuration I've missed).

I've created a Spring Boot sample application reproducing the bug, hosted in this GitHub repository. The main class is PeakTest along with force-sync property (please note that due to the non-deterministic nature of the test, results change between runs).

Thanks

[vpavic]

Just to note that this was originally reported as spring-projects/spring-session#1181.

[jzheaux]

@daniloarcidiacono One thing to note is that the list of actions that are taken when a session is created is in the following order:

1. Check the session registry for a concurrent session violation (via ConcurrentSessionControlAuthenticationStrategy)
2. Mitigate session fixation (via ChangeSessionIdAuthenticationStrategy)
3. Register the session with the session registry (via RegisterSessionAuthenticationStrategy)

The reason your test is failing is not really due to a race condition, but simply the fact that this order was designed to protect a human from logging in twice, not an automated test.

Note that the check is performed first, which means if two requests come in close together and before the cap is hit, they will both pass the check (Step 1) since their sessions have not yet been registered (Step 3).

So, one thing that you could try would be to add the check at the end of the list:

1. Check
2. Mitigate
3. Register
4. Check

I made this change myself in your sample app and now if two requests come in close together, then both are rejected (it fails with Expected: 1 Actual: 0 instead of Expected: 1 Actual: 2 or more). My change looks like this:

protected void configure(HttpSecurity http) {
    http
        .sessionManagement()
            .sessionAuthenticationStrategy(sessionAuthenticationStrategy())
            .maximumSessions(1)
                .sessionRegistry(sessionRegistry)
            // ... your other configs
}

SessionAuthenticationStrategy sessionAuthenticationStrategy() {
    SessionAuthenticationStrategy mitigate = new ChangeSessionIdAuthenticationStrategy();

    ConcurrentSessionControlAuthenticationStrategy check =
            new ConcurrentSessionControlAuthenticationStrategy(sessionRegistry);
    check.setMaximumSessions(1);
    check.setExceptionIfMaximumExceeded(true);

    RegisterSessionAuthenticationStrategy register =
            new RegisterSessionAuthenticationStrategy(sessionRegistry);

    return new CompositeSessionAuthenticationStrategy(
            Arrays.asList(check, mitigate, register, check));
}

This isn't quite what you were asking for (your test will now fail occasionally because all requests were rejected), but it might be an effective workaround.

[lrozenblyum]

@jzheaux the workaround might help however it may cause the deny of access in legal situations... Ideally we need a 100% bullet-proof solution that will allow and deny access atomically depending on the current session registry state

[lrozenblyum]

The solution I've applied in my code is decorating the whole CompositeSessionAuthenticationStrategy with a synchronized decorator to ensure logical atomicity of check + register

[rwinch]

@lrozenblyum please keep in mind that won't work when you are working with multiple application servers. The only way I see around this is to check every request which adds a performance cost.

[lrozenblyum]

@rwinch about multiple application servers, do you mean a scenario when there is something like load balancing which selects target server to handle the request, so that the sessions live not in a single JVM/app. server?

[rwinch]

Yes

[lrozenblyum]

Wondering: is the issue still valid for non-cluster environments?
#3189 which was one of its reasons was fixed.