ecamp
Replace PHP sessions with JWT sessions when storing the OAuth state parameter
Replaces #2927. The goal of this PR is to completely remove native PHP sessions from the application again. The PHP sessions had to be re-introduced when we implemented OAuth login via hitobito and Google in #2030.
Why did we need PHP sessions? One security mechanism of a standard OAuth flow is to use a "state" parameter. Our API has to generate a random value for the state parameter, pass it on to the client, the client passes it to Google or hitobito, which then passes it back via the client to our API server. If the value of the state parameter is still the same after all these steps, we can be sure that the user was not impersonated, replayed or misled by some attacker. But in order to remember what the state value was back when our API created it, the KnpOauthBundle requires activating PHP sessions.
Why don't we want PHP sessions? As soon as PHP sessions are activated, Symfony assumes we want to use that as our storage system for user / login management. This has effects in various places in the application, e.g. omitting the PHPSESSID cookie breaks the functionality of the LexikJWTBundle, as @usu noticed in #2927. We don't want to have to deal with any side effects of PHP sessions when we already have a nicely working JWT cookie system which is completely stateless. Additionally, our JWT system is a lot easier to scale once we start scaling up by running more than one API container instance.
How does this work? Instead of storing data into the PHP session on the server, we encode the data (the state parameter) into a short-lived (5mins) JWT token and send that token as a cookie to the user. The browser will then send that cookie to our API on every request during the next 5 minutes, allowing us to extract the ground truth value of the state parameter again, once we need to check the validity.
Could we omit the state parameter entirely instead? @simfeld and me have read up on the exact attack vectors which are actually prevented by the state parameter. It's surprisingly hard to find properly explained attacks for this, but we found some (e.g. page 21 in this OWASP PDF). Also, Google requires us to use the state parameter anyways.
Isn't this insecure? We already do the same thing for our normal login system. A valid JWT token can only be generated (signed) by our server, because only there is the private key which is needed for signing the tokens. So this makes it impossible for attackers to forge their own tokens. Additionally, in this PR we create short-lived tokens and store them in a very secure Cookie. This even makes it more secure than our normal login system. All the data in this "session storage substitute" is publicly available to the user though, so it isn't suited for secret information. But the only things we need to store in there are the randomly generated state parameter and the callback URL where the user should return after the OAuth flow. Both these pieces of information are already public to the user.
TODO:
- [x] Tests for
JWTStateOAuth2Client - [x] Tests for our custom logic in
Kernel#process
Nice, code-wise looks good to me. Shall we deploy to check?
Yes, but we probably need to remove another deployment, or else our environment will become unstable again...
Under construction: I'm currently debugging why the JWT token read from the Cookie is null on the deployment (it works locally).
Update: OAuth now works great locally and on deployment, but only on the deployment the normal authentication via our JWT token does not work as long as no PHPSESSID cookie is sent. Apparently this is still some issue with Symfony... Looks like it's time for https://github.com/ecamp/ecamp3/wiki/debugging-locally-reproducing-production-error
One Problem with a stateless JWT Token as state is that we cannot invalidate it as soon at it's used. A whitelist with the issued state Tokens in the database would help with that. And after a Token is used, we remove it from the whitelist.
Else we follow the recommendations for Oauth2.
Update: OAuth now works great locally and on deployment, but only on the deployment the normal authentication via our JWT token does not work as long as no PHPSESSID cookie is sent. Apparently this is still some issue with Symfony... Looks like it's time for https://github.com/ecamp/ecamp3/wiki/debugging-locally-reproducing-production-error
@carlobeltrame: The issue was that we're using dots within cookie names, introduced with #2914. PHP replaces dots to underscores for variable names and this messes up the JWT authentication. No idea why login is working currently on devel, but I was able to debug it to this specific issue. Currently fixed by introducing a separate environment variable for cookie prefixes (https://github.com/ecamp/ecamp3/pull/2931/commits/d7f74e2a4c82a40b5de6e9cb6f4936d617775503).
From https://www.php.net/manual/en/language.variables.external.php:
Dots and spaces in variable names are converted to underscores. For example becomes $_REQUEST["a_b"].
Wow, nice. I even tried on localhost to add a dot to the cookie name and it still worked, so I assumed it couldn't be that. But glad to hear you found the problem!
A whitelist with the issued state Tokens in the database would help with that. And after a Token is used, we remove it from the whitelist.
Good idea. What information would you store in the database along with the state value? We will have a hard time to identify a single malicious state value to revoke if we ever need to, but at least we could invalidate all the currently valid state values at once by truncating that table. I would also store the creation timestamp of the state value, so we can also clean up any old expired state values which have never been used because the user didn't finish the OAuth flow.
A whitelist with the issued state Tokens in the database would help with that. And after a Token is used, we remove it from the whitelist.
Good idea. What information would you store in the database along with the state value? We will have a hard time to identify a single malicious state value to revoke if we ever need to, but at least we could invalidate all the currently valid state values at once by truncating that table. I would also store the creation timestamp of the state value, so we can also clean up any old expired state values which have never been used because the user didn't finish the OAuth flow.
Creation time stamp and jwt_hp would be enough. When the signature is not in the db, no one can retrieve a valid token from the database.
This is ready for merging now from my side. @simfeld what do you think of this? I'd like to get your feedback on this as well if possible.
While the user still passes the plain, random state value to the OAuth provider (as before), we now no longer store the state value in a PHP session, but in a JWT token inside a cookie AND in our database. Only if all three values (the plain one which did the round-trip via the OAuth provider, the JWT one and the DB one) match do we accept the login as legitimate. There were previously no "secret" values stored in the PHP sessions, so it does not matter that we expose all "session data" in the JWT token now. And we still have the option to invalidate all state tokens by truncating the o_auth_states database table (previously we could have done that by dropping all PHP session storage). Can you think of another way in which this system would be inferior to PHP sessions, security-wise?
@BacLuc I have edited a migration which has been added in an earlier commit of this PR. What do I have to do to make the "validate migrations" check pass? I can run bin/console doctrine:schema:update --force locally which fixes the "problem" locally, but there are no changed files to commit after doing so...
@BacLuc I have edited a migration which has been added in an earlier commit of this PR. What do I have to do to make the "validate migrations" check pass? I can run
bin/console doctrine:schema:update --forcelocally which fixes the "problem" locally, but there are no changed files to commit after doing so...
With docker-compose exec bin/console doctrine:migrations:diff you get the following migration:
<?php
declare(strict_types=1);
namespace DoctrineMigrations;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;
/**
* Auto-generated Migration: Please modify to your needs!
*/
final class Version20220904140709 extends AbstractMigration
{
public function getDescription(): string
{
return '';
}
public function up(Schema $schema): void
{
// this up() migration is auto-generated, please modify it to your needs
$this->addSql('DROP INDEX idx_466ef70c9d468a55');
}
public function down(Schema $schema): void
{
// this down() migration is auto-generated, please modify it to your needs
$this->addSql('CREATE SCHEMA public');
$this->addSql('CREATE INDEX idx_466ef70c9d468a55 ON o_auth_state (createtime)');
}
}
It does not make sense, but that's the migration doctrine wants.
If you set the name on the expireTime + state index, then doctrine creates the createTime index. Probably there is a bug in doctrine.
@ecamp/core
After merging this PR, i had to rm frontend/public/environment.js
That i had the SHARED_COOKIE_DOMAIN env variable in the frontend.