Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReactiveHashCommands.hMSet calls HSETNX if map contains a single tuple [DATAREDIS-791] #1365

Closed
spring-projects-issues opened this issue Mar 21, 2018 · 7 comments

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Mar 21, 2018

magd opened DATAREDIS-791 and commented

In ReactiveHashCommands#hMSet, I believe the method should not execute an ifValueNotExists()? By removing that, it should be OK I believe.

A method hMSetNX could be created too, if relevant.

I came to this conclusion after checking in Spring Boot 2, Spring Security component was not logging out correctly when using Spring Data Redis as the backend for the session. The problem was basically it's using this method to remove the SPRING_SECURITY_CONTEXT information, but as it's doing an HSETNX internally, it's not really updating the session information (as that key already exists). So basically, a user is never really logged out, even though it seems like it is.

If that's the case and I'm not mistaken, it's obviously an important security issue when using Redis as the session backend.

I'd be happy to provide a PR on GitHub if you think I'm right about this


Affects: 2.0.5 (Kay SR5)

Referenced from: pull request #325

Backported to: 2.0.6 (Kay SR6)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

Mark Paluch commented

Good catch. The bug becomes visible only if hMset is called with a map that contains a single entry. I'm already on a fix

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

magd commented

Thanks Mark. Once the fix is applied, are you going to let the Spring Security team know about this, or shall I open an issue myself on that project? They should upgrade the dependency as soon as possible, I assume.

Thank you

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

Mark Paluch commented

I'll talk to the Spring Security folks. In which bit of Spring Security does that happen (Spring Security Core, OAuth, Spring Session, …)?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

magd commented

I think it's a chain. Spring Session with Redis driver should update the version.

But then, I assume for projects like Spring Boot, there should be a full update to make it happen (Spring Boot should update version of Spring Security, and Spring Security should update version of Spring Session?).

Cheers!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

Mark Paluch commented

That's not an answer to the question which Spring Security component invokes hMSet.

When you're using Spring Boot, then you'll get the upgrade anyways with Spring Boot 2.0.1, see https://spring-calendar.cfapps.io/

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

magd commented

Sorry about that. It's Spring Session. ReactiveRedisOperationsSessionRepository::saveDelta (private) uses DefaultReactiveHashOperations::putAll, which in turn uses the method we're discussing

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 22, 2018

Mark Paluch commented

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants