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

Add expiration based persistent rememberme token #483

Merged
merged 15 commits into from Jun 12, 2019

Conversation

Projects
None yet
3 participants
@aschempp
Copy link
Contributor

commented May 10, 2019

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:

  • make the expiration time configurable
  • update and add unit tests

@leofeyer leofeyer added the defect label May 13, 2019

@leofeyer leofeyer added this to the 4.8.0 milestone May 13, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 13, 2019

make the expiration time configurable

Can't this be done in the services.yml file through the $options argument?

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

make the expiration time configurable

Can't this be done in the services.yml file through the $options argument?

Theoretically, and it would be the best place imho. Unfortunately, the RemembermeFactory only merges known keys, so we would need to overwrite the factory…

@Toflar Toflar referenced this pull request Jun 4, 2019

Merged

Reworked caching for proxies #389

{
try {
$rows = $this->connection->executeQuery(
'SELECT * FROM tl_remember_me WHERE series=:series AND (expires IS NULL OR expires>=:expires) ORDER BY expires IS NULL DESC',

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 4, 2019

Member

For the sake of readability I would go with expires IS NULL OR expires>:now and then 'now' => new \DateTime()

This comment has been minimized.

Copy link
@aschempp

aschempp Jun 5, 2019

Author Contributor

You mean now instead of expires?

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 5, 2019

Member

Yes. It's easer to read when you write queries like this :) If you read it out loud, right now it is where series equals series AND (expires is null or expires is greater than or equal to expires). It doesn't really say much. This would read much better: where series equals series AND (expires is null or expires is greater than or equal to now).

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jun 6, 2019

Member

There is also a built-in NOW() function in MySQL.

Show resolved Hide resolved ...urity/Authentication/RememberMe/ExpiringTokenBasedRememberMeServices.php Outdated
@Toflar
Copy link
Member

left a comment

Just a few minor things but in general the concept looks good to me!

/**
* Clean up expired tokens on successful login (lazy behavior).
* This will also clean other series that have expired, e.g. if a cookie was never used again within the lifetime.

This comment has been minimized.

Copy link
@Toflar

Toflar Jun 4, 2019

Member

Can you also add a comment about the 5 minutes timespan here? That it seems to be a reasonable value. Just so that new \DateInterval('PT300S') does not look like a randomly chosen value if anybody looks into the code.

@aschempp aschempp marked this pull request as ready for review Jun 5, 2019

@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from 6c52109 to 03f6899 Jun 5, 2019

@aschempp aschempp force-pushed the aschempp:feature/expiring-rememberme branch from 6716c6a to 14bef3f Jun 6, 2019

@leofeyer leofeyer referenced this pull request Jun 6, 2019

Closed

REMEMBERME only works once #400

@aschempp aschempp force-pushed the aschempp:feature/expiring-rememberme branch from 334d841 to 642cfe7 Jun 7, 2019

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

I have update the implementation to use the ORM entity. While doing so, I also found an implementation issue that I was luckily able to solve:
As described in symfony/symfony#27910, the rememberme value (= password) should be hased in the database. This would make it impossible to create a cookie if the database was compromised. However, this also creates the issue that we have no way of updating the cookie, if we receive the expired one and should send the already-existing new one to the client (within the one minute timeout). Because the database value is encoded, we can't use it to send it to the client.
To solve this problem, I have now encoded the series in the database, which basically means the username. Because the username is always send with the cookie and remains the same across multiple requests, we can reuse it to update the cookie on the client side.

@aschempp aschempp force-pushed the aschempp:feature/expiring-rememberme branch from e98ce36 to 424a265 Jun 11, 2019

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

This should now be complete. Requires contao/phpstan#4 to fix the phpstan tests.

@leofeyer leofeyer force-pushed the aschempp:feature/expiring-rememberme branch from 0e1e286 to 1f1875e Jun 12, 2019

@leofeyer leofeyer removed the tests missing label Jun 12, 2019

@leofeyer leofeyer force-pushed the aschempp:feature/expiring-rememberme branch from 597403f to 97881a5 Jun 12, 2019

@leofeyer leofeyer merged commit 7c0c440 into contao:master Jun 12, 2019

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.8%) to 87.528%
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Thank you @aschempp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.