Allow client credentials secret to be hashed

[sebastiaanluca]

A part of the discussion in #14 was about having client credentials secrets securely stored in the database. While encrypting this value when storing and decrypting when retrieving would be ok to do, it doesn't provide any real value as it can be easily decrypted if the attacker already has access to the application.

Another, more viable option was to treat the secret like a user password and hash it. It's not necessary to know the actual value when authenticating, so it doesn't need to be stored in plaintext or have the ability to be decrypted. This increases the security of the API by removing the ability for attackers, abusive users, or other parties to get a hold of those secrets (that sometimes give access to critical API endpoints).

This PR introduces a lightweight, optional way of enabling hashed client secrets for existing or future applications using Laravel Passport.

In the boot method of your service provider:

Passport::useHashedClientSecrets();

Showing the secret

The main downside of this feature is that, with it enabled, the developer will only have one chance to show the secret to the user (unless they manually keep track of it). During the HTTP request the OAuth client was created in, it will have the plaintext secret set on the client's $client->plain_secret field. When subsequently retrieving the client, it will be null.

Towards the user, it's more secure to not have API secrets viewable, just like you wouldn't have user passwords shown on their detail page or profile "just in case they forgot".

Impact

By mainly changing the way the secret is verified in \Laravel\Passport\Bridge\ClientRepository, it enables hashed secrets throughout the entire flow. So besides the user-side and upgrade flow, there isn't really a big impact.

Upgrading

For existing applications that want to make use of hashed secrets, enabling the option will break their authentication flow. Therefore they need to enable the flag, add a migration that removes the length limit on the secret field, and hash their secrets.

For the latter, I'd suggest a migration that both removes the length limit and converts the plaintext secrets to hashed secrets. Note that this would be a one-time action and could not be undone, as the secrets will the irreversibly hashed.

Tests

I've opted to extend the existing test since the tests are identical, but the setup is different. This allows you to only maintain one test and have the other follow the lead of its parent. I didn't add tests for the client model, etc, as it seems some parts are not tested? I can add more tests if needed.

One remark here is that I do get a lot of PHPUnit 9.x deprecation warnings (I'm on PHP 7.4), but all tests pass.

[taylorotwell]

SHA-256 hashing is sufficient and much faster given the randomness and entropy of client secrets. Rainbow table attacks against them are not practical.

[sebastiaanluca]

Good point, client secrets are indeed already quite random. So replacing the default hasher with SHA hashing would be sufficient in this case? I'll find some time to update the PR. Everything else looking good?

[taylorotwell]

I haven't reviewed it mega thoroughly but it seems reasonable.

[taylorotwell]

Are there any other things in Passport that people will likely request to be hashed?

[sebastiaanluca]

Are there any other things in Passport that people will likely request to be hashed?

I went through the tables and as far as I know and can see, the client secrets are the only ones that are both a (small) "security issue" (or provide increased security —or the feeling of such) and do not require to be in plaintext when retrieving from the database.

People do want UUIDs instead of auto-incremented integers, but I feel that's for another PR. At the moment, that can be done without tweaking Passport using event listeners and the likes.

[sebastiaanluca]

Using password_hash($value, CRYPT_SHA256), but since there are 3 options here, I'm not sure if it's correct. The others were OPENSSL_ALGO_SHA256 and MHASH_SHA256. Using SHA-256 also allows us to simplify the code, dependencies, and tests.

Thanks for taking the time to reply!

[taylorotwell]

I think you can just do hash('sha256', $value)

[sebastiaanluca]

I've updated the PR as per your suggestion. Although there's no salt to add randomness to the strings now, I suppose it's still secure enough given the combination of unilateral hashing and the randomness of the secrets.

I do have to note that if someone creates an OAuth client with a word or sentence as secret (still possible/allowed), there are a lot of tools out there to find the hashed word in a hash/word database. For instance, the hashed value used in the tests ("secret") is found instantly. Using salts, you'd enforce increased randomness and wouldn't have this, but it would be slower.

[taylorotwell]

I'm unsure why you would need to remove the length on the secret field. All SHA-256 values are 64 characters long regardless of the original string length.

[sebastiaanluca]

In the migration? Was because of the length of the salted hash or some previous implementation I tried. Not present in the final result where we just use a SHA-256 hashed string.

Should we add a note somewhere (docs, upgrade guide?) to explain this feature and how to use it in existing applications? We can include some code (a migration) that converts existing secrets to hashes for convenience.

Thanks for merging!

[driesvints]

Thanks @sebastiaanluca! I'll include this PR in the upgrade guide once we release 9.0 (which might still be a while because we only recently did an 8.0 release). Once the release is done we can update the docs.

[sebastiaanluca]

Cheers! Upgrading would then just be the following:

For existing applications that want to make use of hashed secrets, enabling the option will break their authentication flow. Therefore they need to enable the flag and hash their secrets. To do so, I suggest including an example migration that gets all existing clients, hashes their current secrets, and saves them. With a big, bold note that this is an irreversible action.

Perhaps I'll also have a look at a PR that introduces optional UUIDs. Although it currently can be achieved by extending the client model and so on, having it integrated natively seems a better situation here.

[driesvints]

Hey @sebastiaanluca, definitely not to reprimand you (thank you for the pr!) but wanted to make you aware that this PR wasn't finished yet (it escaped us as well):

#1244
#1252

I think we got everything now.