Skip to content

[Security] Do not overwrite already stored tokens for REMOTE_USER authentication #43992

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

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

stlrnz
Copy link
Contributor

@stlrnz stlrnz commented Nov 10, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #43648
License MIT

As described in #43648 the user is currently loaded on every request for REMOTE_USER authentication.
Thanks to @wouterj for confirming me on Slack that this seems weird. So, I looked deeper into this.

I found out that other Authenticators tell the AuthenticatorManager only under special conditions (like matching route etc.) that they support the current request. However, the AbstractPreAuthenticatedAuthenticator is not so picky. In consequence, the user is authenticated again on every request.

Inspired by RememberMeAuthenticator, this PR adds an addition check to AbstractPreAuthenticatedAuthenticator to solve this issue.

// do not overwrite already stored tokens (i.e. from the session)

@carsonbot carsonbot added this to the 5.3 milestone Nov 10, 2021
@stlrnz stlrnz changed the title do not overwrite already stored tokens (i.e. from the session) [Security] Do not overwrite already stored tokens for REMOTE_USER authentication Nov 10, 2021
@wouterj
Copy link
Member

wouterj commented Nov 10, 2021

This looks like a correct change, although I'm wondering if we should apply this always (i.e. never check if authenticators support the request if there already is a token) instead of letting the authenticator decide. But I'm not sure if this would be the correct fix (happy to hear other ideas on this topic).

Is it possible for you to add a test for this behavior? (e.g. in the RemoteUserAuthenticatorTest)

@wouterj wouterj linked an issue Nov 10, 2021 that may be closed by this pull request
@stlrnz
Copy link
Contributor Author

stlrnz commented Nov 11, 2021

Is it possible for you to add a test for this behavior? (e.g. in the RemoteUserAuthenticatorTest)

I've done that. The test is inspired by RememberMeAuthenticatorTest.

This looks like a correct change, although I'm wondering if we should apply this always (i.e. never check if authenticators support the request if there already is a token) instead of letting the authenticator decide. But I'm not sure if this would be the correct fix (happy to hear other ideas on this topic).

I think this is the right question to ask. Looking at HttpBasicAuthenticator I assume there is the same check missing.

public function supports(Request $request): ?bool

It's easy to forget that scenario when writing an Authenticator. In my opinion the Authenticator is lying when returning false for the question "do you support this request?". It can support/authenticate this request, but we dont want it to do.
So for me it would be a better solution to check for the token on a more centralized position. But I dont know if there is any scenario where this would be wrong. The current solution is more flexible.

@Nyholm
Copy link
Member

Nyholm commented Nov 11, 2021

Thank you for the PR issue and the reproducer. Great job!

Im also hesitant that this is the correct fix. But Im not sure what else is. Im wondering what a system like kerberos would do on logout and if the user changes without logging out first. Then this fix will break applications (depending on kerberos or others behavior).

Im not sure who would like to read this but one can do a workaround by creating your own RemoteUserAuthentication and override the supports method. See here for more info: https://symfony.com/doc/current/security/custom_authenticator.html

@stlrnz
Copy link
Contributor Author

stlrnz commented Nov 15, 2021

Thank you for the feedback, @Nyholm!

Im wondering what a system like kerberos would do on logout and if the user changes without logging out first.

That's a really good question. In our environment, this cannot happen. But I mocked this scenario in my demo application and found a flaw in the original PR. So I pushed an updated version which checks for the user identifier before making a decision.
Now the new firewall does the same as the old: if the user changes, the new user is logged in.

But this implementation does not invalidate the session of the first user. In consequence the second user may have access to sensible data and/or the application will break anyway.
However, I checked this scenario using the old firewall, too. The same thing is happening here. So this PR neither worsen nor improved that special case. But since this never seemed to be an issue, I dont know how critical it really is.

I'm wondering if we should apply this always (i.e. never check if authenticators support the request if there already is a token) instead of letting the authenticator decide

I think this is still a really good idea, @wouterj! It may solve the problem with the session, too?
But this would be a more dangerous change. Maybe we should separate that discussion from this PR?

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this!

I've created a minimal app and checked your findings. The legacy security system indeed did not load the user if the token contained the same user (checked by username and firewall name).

If we add the precise checks of the old system, let's merge this 👍 . We can always improve things as a feature in an upcoming version.

@carsonbot carsonbot changed the title [Security] Do not overwrite already stored tokens for REMOTE_USER authentication Do not overwrite already stored tokens for REMOTE_USER authentication Nov 28, 2021
@carsonbot carsonbot changed the title Do not overwrite already stored tokens for REMOTE_USER authentication [Security] Do not overwrite already stored tokens for REMOTE_USER authentication Nov 28, 2021
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @stlrnz and @wouterj

@nicolas-grekas
Copy link
Member

Thank you @stlrnz.

@nicolas-grekas nicolas-grekas merged commit 7fd34c4 into symfony:5.3 Nov 29, 2021
This was referenced Nov 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security] User is loaded on every request
5 participants