ESI requests break persistent remember me tokens #31078
Comments
|
Related: #27910 @paragonie-scott |
|
No, they should not, as that's not how ESI works when using Varnish or other reverse proxies. Reading response cookies to update the cookies sent in subsequent requests is something done by the browser, not by ESI-capable reverse proxies. |
|
#27910 is not related to this. Storing a hash of the value rather than the value itself is entirely orthogonal to the fact of changing the value on each usage. |
|
Ok, but then the ESI request will trigger the auto-login process again and delete the "remember me" cookie (see above). Are you implying that we cannot use persistent tokens together with ESI? |
|
Any updates on this one? @nicolas-grekas Can you confirm that Symfony intentionally does not support persistent rememberme tokens with series in combination with ESI? |
as @stof explained, Varnish doesn't do that. Maybe you could make it do so but that's out of scope. Base on your description above, I would look for a way to accept already consumed token from ESI responses (for some duration). Since they're signed, that'd be safe. All that is on you, nothing we can do in Symfony to me. |
|
Agreed. Thank you for your explanation. 😊 |
|
Also see #18384 |
2 tasks
leofeyer
pushed a commit
to contao/contao
that referenced
this issue
Jun 12, 2019
Description ----------- **Warning: This is currently POC and completely untested!** This is our current take at fixing #400 As suggested by @nicolas-grekas in symfony/symfony#31078 (comment), we need to add a short expiration time to the persistent rememberme tokens. This also fixes concurrent requests mentioned in symfony/symfony#18384. **Background Info:** - A compiler pass replaces the existing rememberme service implementation. This could be replaced by symfony/symfony#31309, but that would not allow new constructor arguments to the service. - Because we have our own service implementation, there's not need for a token provider. Also, `PersistentToken` is only used to communicate between the service and token provider. **Authentication flow:** 1. A request with rememberme cookie is authenticated in the firewall. 2. The rememberme service will mark all existing database tokens of the current series as *to expire* in 60 seconds. 3. Also adds a new token without an expiration time to the database. 4. The new cookie is added to the request attribute. If the request is ESI, the response cookie will never make it to the browser, but that's fine. On the second parallel request, there will be two tokens in the database (thanks to table locking). If the expiring one is still valid, it will be accepted. Instead of generating another new one, it will however send the already created new one back to the client. As long as there is one browser request (non-ESI) within that minute, the new rememberme cookie as well as the session will correctly make it to the client. This will be accomplised through: 1. The main response will never be in the cache (because there are cookies). 2. Should we implement an option to cache sites regardless of cookies (e.g. via checkbox as in #389), we can add a 1px blank image to the page that will fetch the session/rememberme cookie. I have also implemented symfony/symfony#27910 and now hashed the token value instead of the series. Both ways would work. Changing this will break existing rememberme cookies in Contao, but as everything is broken atm that should be fine. **TODO:** - [x] ~make the expiration time configurable~ - [x] update and add unit tests Commits ------- 92bcb51 Add expiration based persistent rememberme token 9a6a8ba Expire old rememberme series after 5 minutes 5121541 Code adjustments from PR feedback 3807ca3 Use Doctrine ORM entity to define tl_remember_me table b1b334b Rewrite rememberme services to use ORM a6b675f Implemented unit tests for everything except rememberme services 438f614 Implemented unit tests for ExpiringTokenBasedRememberMeServices a5fb20e Add ORM config for travis tests 4bce3e6 Fixed RemembermeServicesPass not correctly overriding arguments f81c940 Fixed composer.json validation 9ad2251 Fixed PHPStan issues 004489b Fixed several DQL query issues 1f1875e Delete database records of same series before migration 5709870 Fix the coding style 97881a5 Fix the --prefer-lowest tests
leofeyer
pushed a commit
to contao/manager-bundle
that referenced
this issue
Jun 12, 2019
Description ----------- **Warning: This is currently POC and completely untested!** This is our current take at fixing contao/contao#400 As suggested by @nicolas-grekas in symfony/symfony#31078 (comment), we need to add a short expiration time to the persistent rememberme tokens. This also fixes concurrent requests mentioned in symfony/symfony#18384. **Background Info:** - A compiler pass replaces the existing rememberme service implementation. This could be replaced by symfony/symfony#31309, but that would not allow new constructor arguments to the service. - Because we have our own service implementation, there's not need for a token provider. Also, `PersistentToken` is only used to communicate between the service and token provider. **Authentication flow:** 1. A request with rememberme cookie is authenticated in the firewall. 2. The rememberme service will mark all existing database tokens of the current series as *to expire* in 60 seconds. 3. Also adds a new token without an expiration time to the database. 4. The new cookie is added to the request attribute. If the request is ESI, the response cookie will never make it to the browser, but that's fine. On the second parallel request, there will be two tokens in the database (thanks to table locking). If the expiring one is still valid, it will be accepted. Instead of generating another new one, it will however send the already created new one back to the client. As long as there is one browser request (non-ESI) within that minute, the new rememberme cookie as well as the session will correctly make it to the client. This will be accomplised through: 1. The main response will never be in the cache (because there are cookies). 2. Should we implement an option to cache sites regardless of cookies (e.g. via checkbox as in contao/contao#389), we can add a 1px blank image to the page that will fetch the session/rememberme cookie. I have also implemented symfony/symfony#27910 and now hashed the token value instead of the series. Both ways would work. Changing this will break existing rememberme cookies in Contao, but as everything is broken atm that should be fine. **TODO:** - [x] ~make the expiration time configurable~ - [x] update and add unit tests Commits ------- 92bcb515 Add expiration based persistent rememberme token 9a6a8ba8 Expire old rememberme series after 5 minutes 5121541c Code adjustments from PR feedback 3807ca38 Use Doctrine ORM entity to define tl_remember_me table b1b334b2 Rewrite rememberme services to use ORM a6b675f3 Implemented unit tests for everything except rememberme services 438f6144 Implemented unit tests for ExpiringTokenBasedRememberMeServices a5fb20e6 Add ORM config for travis tests 4bce3e64 Fixed RemembermeServicesPass not correctly overriding arguments f81c9403 Fixed composer.json validation 9ad2251e Fixed PHPStan issues 004489b2 Fixed several DQL query issues 1f1875ef Delete database records of same series before migration 57098706 Fix the coding style 97881a50 Fix the --prefer-lowest tests
leofeyer
pushed a commit
to contao/core-bundle
that referenced
this issue
Jun 12, 2019
Description ----------- **Warning: This is currently POC and completely untested!** This is our current take at fixing contao/contao#400 As suggested by @nicolas-grekas in symfony/symfony#31078 (comment), we need to add a short expiration time to the persistent rememberme tokens. This also fixes concurrent requests mentioned in symfony/symfony#18384. **Background Info:** - A compiler pass replaces the existing rememberme service implementation. This could be replaced by symfony/symfony#31309, but that would not allow new constructor arguments to the service. - Because we have our own service implementation, there's not need for a token provider. Also, `PersistentToken` is only used to communicate between the service and token provider. **Authentication flow:** 1. A request with rememberme cookie is authenticated in the firewall. 2. The rememberme service will mark all existing database tokens of the current series as *to expire* in 60 seconds. 3. Also adds a new token without an expiration time to the database. 4. The new cookie is added to the request attribute. If the request is ESI, the response cookie will never make it to the browser, but that's fine. On the second parallel request, there will be two tokens in the database (thanks to table locking). If the expiring one is still valid, it will be accepted. Instead of generating another new one, it will however send the already created new one back to the client. As long as there is one browser request (non-ESI) within that minute, the new rememberme cookie as well as the session will correctly make it to the client. This will be accomplised through: 1. The main response will never be in the cache (because there are cookies). 2. Should we implement an option to cache sites regardless of cookies (e.g. via checkbox as in contao/contao#389), we can add a 1px blank image to the page that will fetch the session/rememberme cookie. I have also implemented symfony/symfony#27910 and now hashed the token value instead of the series. Both ways would work. Changing this will break existing rememberme cookies in Contao, but as everything is broken atm that should be fine. **TODO:** - [x] ~make the expiration time configurable~ - [x] update and add unit tests Commits ------- 92bcb515 Add expiration based persistent rememberme token 9a6a8ba8 Expire old rememberme series after 5 minutes 5121541c Code adjustments from PR feedback 3807ca38 Use Doctrine ORM entity to define tl_remember_me table b1b334b2 Rewrite rememberme services to use ORM a6b675f3 Implemented unit tests for everything except rememberme services 438f6144 Implemented unit tests for ExpiringTokenBasedRememberMeServices a5fb20e6 Add ORM config for travis tests 4bce3e64 Fixed RemembermeServicesPass not correctly overriding arguments f81c9403 Fixed composer.json validation 9ad2251e Fixed PHPStan issues 004489b2 Fixed several DQL query issues 1f1875ef Delete database records of same series before migration 57098706 Fix the coding style 97881a50 Fix the --prefer-lowest tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We are using the
PersistentTokenBasedRememberMeServicesservice together with theDoctrineTokenProviderclass to manage "remember me" tokens. The idea behind the service is to update the token hash after each usage, so the same token cannot be used twice.Now, if you have a "remember me" cookie and request a page that includes an ESI fragment, both the main request and the ESI request will trigger the auto-login process. However, after the main request has used the token, the hash is changed and the cookie is regenerated. By the time the ESI request is processed, the "remember me" token can no longer be verified (CookieTheftException) and the cookie is deleted in the
loginFailmethod.If the user was successfully authenticated based on the "remember me" cookie in the main request, the response will contain the updated "remember me" cookie as well as a session cookie. Should these cookies not be passed to the ESI request?
The text was updated successfully, but these errors were encountered: