Skip to content

[RFC] Keep old session after regeneration? #27395

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

Closed
umulmrum opened this issue May 28, 2018 · 22 comments
Closed

[RFC] Keep old session after regeneration? #27395

umulmrum opened this issue May 28, 2018 · 22 comments
Labels
HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed) Stalled

Comments

@umulmrum
Copy link
Contributor

In #13048 session migration was changed so that the old session data are deleted immediately.
The PHP docs in https://secure.php.net/manual/en/features.session.security.management.php state multiple times that this is not the right thing to do, but old session data should be kept for a short while to avoid side effects (concurrency/unstable network).

I did not perceive concrete problems with the current implementation but noted the contradictions between PHP docs and Symfony implementation and wondered if this was done on purpose (although I couldn't find any discussion on that topic).

In short: Can the current implementation be considered correct or good enough or should the regeneration process be improved?

@mattattui
Copy link
Contributor

mattattui commented May 29, 2018

This may be unrelated, but my custom guard authenticator now seems to regenerate the session on every request, and since most browsers make several concurrent requests, the ones that happen after the session has been destroyed all fail. To put it mildly, this is a bit of a problem :)

@fabpot
Copy link
Member

fabpot commented May 29, 2018

/cc @weaverryan

@leofeyer
Copy link
Contributor

leofeyer commented May 29, 2018

The changes from f6f2f97 (in particular the changes in the SimplePreAuthenticationListener) also break Contao. We have bound our authentication system to the PHP session ID, so whenever the PHP session changes, the user is logged out. And since the PHP session changes upon every request now, the user is logged out permanently.

BTW, should the session not only be regenerated after a successful login?

@tifabien
Copy link
Contributor

Same issue for me. Since I updated in 3.4.11 I have now all my form submissions which fails as it does not found csrf token in session.

@weaverryan
Copy link
Member

Ill check into this. For anyone having the issue, can it offer more details? The session should only regenerate after a successful login, not on every request. If you can post some of our authentication code, like the authenticator & firewall - that may help.

@stof
Copy link
Member

stof commented May 30, 2018

@weaverryan pre-authenticated listeners are authenticating successfully on each request, as they send the credentials with each request

@weaverryan
Copy link
Member

weaverryan commented May 30, 2018

But, at the same time, any authentication mechanism that authenticates on every request would not need to use the session. The new code basically says:

A) I just authenticated. Is there a session?
B) If yes, migrate the session

My initial instinct is that we're hitting on a situation where some projects are both authenticating on every request, but also relying on the session. I'm not sure exactly why, so I'd like more detail from users. Specifically: if you notice that your session is being regenerated on every request, I'd like to see your authentication + firewall code. If you have this situation, then, for some reason, some authentication listeners IS "authenticating you" on every request, which causes the session regeneration.

@leofeyer
Copy link
Contributor

@weaverryan I don't know if it is what you are looking for, but here is the authentication related stuff of Contao 4.4:

@weaverryan
Copy link
Member

@leofeyer yes - this is helpful :). Can you help me understand the ContaoAuthenticator? It creates an AnonymousToken then tries to find a user from by using the secret/providerKey passed to it? Can you describe the authentication flow that this class supports?

Also, I noticed that your firewalls are stateless. So, the Cantao authentication mechanism shouldn't care at all about the session, right? Btw, if you are available, I'm on the Symfony Slack if it's easier to dump some knowledge on me there (then I'd summarize here) :)

@leofeyer
Copy link
Contributor

@aschempp will contact you. Thank you for your help!

@weaverryan
Copy link
Member

Just chatted with @aschempp - PR proposal coming shortly.

For anyone else having issues, I would still like to see your authentication details. Contao has a very specific SimplePreAuthenticationInterface authentication situation. But, for those of you using Guard or something else, I still want to know your details.

@weaverryan
Copy link
Member

See #27427.

But, I want to know more about users having issues - especially with guard authentication - to make sure we understand the full scope of things.

@mattattui
Copy link
Contributor

I've solved this issue in my instance by learning one very important thing: custom Guard authenticators which rely on sessions do not need to handle authenticated users. Guard already handles existing authenticated sessions if you don't get in its way.

In Symfony 3.4.11 my supports() method returned true for a login attempt, and also returned true if $request->hasPreviousSession() && $request->getSession()->has('username'), then my getCredentials() method would handle both situations, essentially handling every request as a login, which confused Guard.

I fixed this by removing everything except the handling of new login attempts from my custom authenticator and returning false from the supports() method at all other times. (It's slightly different for previous versions of symfony which don't implement the supports() method; in those, you'd make your getCredentials() method return null instead).

Thanks to @weaverryan for helping out on this.

@weaverryan
Copy link
Member

Hi guys!

I now believe that this is entirely a documentation issue. Specifically, we should keep this session migration change. However, this does affect some authentication mechanisms. Specifically:

If you create an auth mechanism that you expect will be used by a browser, then your auth mechanism must be smart enough to not re-authenticate the user when they are already authenticated

If your browser-based authentication mechanism IS authenticating the user on every request, then you actually do need to migrate the session, else you're open to session fixation. That's exactly what the recent patches added. So, if you are in this situation, I believe the proper fix is to update your authenticator to only authenticate when necessary (i.e. not on every request).

I've created a docs PR - symfony/symfony-docs#9860 - for the 2.8 version with Guard. You can see an example of checking to see if the user is already authenticated.

@weaverryan
Copy link
Member

Hi again!

After a bit more chatting, I believe there is still a problem. Specifically, the session should not migrate if your firewall is stateless: true. The use-case is SSO - e.g. your auth mechanisms reads from $_SERVER on every request to auth the user.

With the recent changes, even if you set stateless: true, your session is still migrated. I believe that's wrong, as there should be no risk of session fixation. And, practically speaking, any stateless, SSO implementation will suffer this problem.

I think the only solution is to, somehow, make the session migration process aware of whether or not the firewall is stateless. Rolling back the changes it not really an option, as we would need to rollback the changes to Guard, but Guard truly does need to migrate the session in the vast majority of cases.

@leofeyer
Copy link
Contributor

I think the only solution is to, somehow, make the session migration process aware of whether or not the firewall is stateless.

I agree.

@weaverryan
Copy link
Member

See #27452

@umulmrum
Copy link
Contributor Author

umulmrum commented Jun 4, 2018

I hope everyone who kidnapped this ticket got a solution ;-)

My original concern was not referring to the latest changes in session migration which seems to have caused that problem, but to a change introduced in Symfony 2.3. Would you be so kind to have a look at that @weaverryan and/or possibly others? Thanks!

fabpot added a commit that referenced this issue Jun 10, 2018
This PR was squashed before being merged into the 2.8 branch (closes #27452).

Discussion
----------

Avoid migration on stateless firewalls

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | Related to #27395
| License       | MIT
| Doc PR        | symfony/symfony-docs#9860

This is a proof-of-concept. Once we agree / are happy, I need to add this to all of the other authentication mechanisms that recently got the session migration code & add tests.

Basically, this avoids migrating the session if the firewall is stateless. There were 2 options to do this:

A) Make the `SessionAuthenticationStrategy` aware of all stateless firewalls. **This is the current approach**
or
B) Make each individual authentication listener aware whether or not *its* firewall is stateless.

Commits
-------

cca73bb Avoid migration on stateless firewalls
fabpot added a commit that referenced this issue Jun 10, 2018
…PasswordJsonAuthenticationListener (weaverryan)

This PR was squashed before being merged into the 3.4 branch (closes #27556).

Discussion
----------

Avoiding session migration for stateless firewall UsernamePasswordJsonAuthenticationListener

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | Related to #27395
| License       | MIT
| Doc PR        | symfony/symfony-docs#9860

This is the sister PR to #27452, which covered all the other authentication listeners.

Commits
-------

c06f322 Avoiding session migration for stateless firewall UsernamePasswordJsonAuthenticationListener
weaverryan added a commit to symfony/symfony-docs that referenced this issue Jun 11, 2018
…averryan)

This PR was squashed before being merged into the 2.8 branch (closes #9860).

Discussion
----------

Avoiding authentication on every request with Guard

Hi guys!

See symfony/symfony#27395 and symfony/symfony#27427

When we're happy with this, I can merge and handle the changes on 3.4, because it will be almost entirely different (with `supports()`, autowiring & the `Security` class - booya for progress!)

Commits
-------

d2f062a Avoiding authentication on every request with Guard
@Simperfit
Copy link
Contributor

@weaverryan can this be closed ?

@javiereguiluz javiereguiluz added HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jul 29, 2019
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed) Stalled
Projects
None yet
Development

No branches or pull requests

10 participants