Remove expired session from principal index

[kinsersh]

Despite RedisIndexedSessionRepository's efforts to automatically remove expired sessions from its principal index, it is possible for expired sessions to remain in that index due to misses from at least these causes:

• application failure due to no event redelivery (see https://docs.spring.io/spring-data/redis/reference/redis/redis-repositories/expirations.html)

  Redis Pub/Sub messages are not persistent. If a key expires while the application is down, the expiry event is not processed, which may lead to secondary indexes containing references to the expired object.

• application only gets events from one redis node in a cluster (see Keyspace Events in Redis Cluster get lost [DATAREDIS-534] spring-data-redis#1111)

The findByIndexNameAndIndexValue method in RedisIndexedSessionRepository reads every session in the principal index including the expired ones, only returning the live ones. As expired sessions stack up, it slows down the findByIndexNameAndIndexValue method. Even though RedisIndexedSessionRepository encounters expired sessions, it doesn't take the opportunity to delete them from the principal index. The effect is that expired sessions are read over and over, never getting cleaned up. This was observed negatively affecting Spring Authorization Server's token endpoint due to its reliance on findByIndexNameAndIndexValue via SessionRegistry's getAllSessions method (see https://github.com/spring-projects/spring-authorization-server/blob/29a8321ab2d65f2f479311c28dd72b3809353722/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java#L323).

This commit improves RedisIndexedSessionRepository to delete the expired session from the principal index when findByIndexNameAndIndexValue sees that a session is expired. This mitigates the impact of expired sessions in that they will only be read once by findByIndexNameAndIndexValue before being removed out of the index.

Mitigates #2230.

[marcusdacoregio]

Hi, @kinsersh. Thanks for the PR.

Have you already read this issue and checked if the workaround satisfies your need for now?

[kinsersh]

Hi, @kinsersh. Thanks for the PR.

Have you already read this issue and checked if the workaround satisfies your need for now?

I wasn’t aware of that issue. Funny that @wash1983 suggested the same workaround.

If the session still exists in redis, that workaround you note would allow RedisIndexedSessionRepository’s cleanup to do its job.

My remaining question is what to do if the key isn’t found in any spring:session:expirations:* entries? If not found, I assume that means redis likely already eliminated the spring:session:sessions:<key> and spring:session:sessions:expires:<key> entries, so there’d be no way to trigger the delete event on the expires key. Would you consider deleting the key out of the principal index where this scan comes up dry? The desire is to mitigate the redis memory leak and the slowdown to every call to findByIndexNameAndIndexValue due to it calling redis with HGETALL for stale sessions that aren’t cleaned out if the app missed the expired event.

[marcusdacoregio]

Yes, I believe that the session can be deleted from the principal index when that happens.

For the reactive indexed implementation of Spring Session Redis, we adopted a different strategy to track session expirations, see the details here. Do you think that approach is better than the current one in the servlet implementation?

[kinsersh]

Yes, I believe that the session can be deleted from the principal index when that happens.

For the reactive indexed implementation of Spring Session Redis, we adopted a different strategy to track session expirations, see the details here. Do you think that approach is better than the current one in the servlet implementation?

Yes, I see a benefit with the new approach. If the app is unavailable for a time, instead of skipping some spring:session:expirations:<expiration_minute> entries, the reactive implementation will go through the oldest entries in spring:session:sessions:expirations the next time cleanup runs.

Redis memory leaks from spring:session:sessions:index:PRINCIPAL_NAME_INDEX_NAME:<principal name> and spring:session:sessions:<session id>:idx are still possible if the app isn’t running between when redis attempts to deliver the expired event for spring:session:sessions:expires:<session id> and redis removes the session data at spring:session:sessions:<session id>.

As for handling the leak affecting spring:session:sessions:index:PRINCIPAL_NAME_INDEX_NAME:<principal name>, we can still clean up orphaned entries when they're encountered (caused by app downtime or using clustered redis). One weakness of this approach is that the principal indexes that aren't visited aren't ever cleaned up.

If Spring Session supported a configurable max session expiration with a default of say 24 hours, we'd be able to set an upper EXPIRES value on spring:session:sessions:<session id>:idx. Similarly, an upper EXPIRES could be set on spring:session:sessions:index:PRINCIPAL_NAME_INDEX_NAME:<principal name> and re-extend with any new session that's created for a particular principal ensuring redis would eventually clean it up. This feature would both help with this Redis memory leak and better support OWASP's recommendation on absolute session timeouts.

[wash1983]

The exact cause has not been identified, but it has indeed been observed in production that the PRINCIPAL_NAME_INDEX_NAME contains a vast number of entries, reaching tens of thousands or even more, which results in significantly degraded performance for org.springframework.session.security.SpringSessionBackedSessionRegistry<? extends Session> when limiting the number of user sessions.

Currently, the temporary solution provided in the pull request (PR) is being employed to address the issue.

[kinsersh]

@marcusdacoregio, I added 833e1b6 to this PR, implementing the what was discussed at #2869 (comment).

[Penghui-liu]

@marcusdacoregio，我将833e1b6添加到此 PR，查看#2869 (评论)中讨论的内容。

I found a new problem. If session concurrency control is enabled, when a new user accesses, the old user's sessionAttr:org.springframework.session.security.SpringSessionBackedSessionInformation.EXPIRED is set to true, but the expiration time does not change, so it will not be deleted from the main index.
So in your changes, you also need to add verification of sessionAttr:org.springframework.session.security.SpringSessionBackedSessionInformation.EXPIRE

[marcusdacoregio]

Hi everyone. I believe that the problem can be resolved by allowing a customization of the redis expiration policy used by the repository, which will be supported via #2906. The sorted set implementation can be useful for most cases and, if there is any additional need, users can customize the strategy.

[marcusdacoregio]

I don't think the problem should be addressed this way because we are just working around the issue that should have not happen in the first place. The best alternative would be to figure out a better way to manage the expired sessions (see #2906) so the problem won't happen and there will be a more reliable mechanism to detect expired sessions.

With that said I'll close this issue in favor of #2906 which will be prioritized for 3.4.