search

DarkGhostHunter / Laraguard

"On-premises 2FA Authentication for all your users out-of-the-box
MIT License
266 stars 24 forks source link

[4.x] Option to encrypt Shared Secret and Recovery Codes #56

Closed srichter closed 3 years ago

srichter commented 3 years ago

The RFC for TOTP recommends encrypting the shared secret when storing it. I feel that is an important option to have on this package.

I had previously implemented this for my own use of the package using accessors/mutators, but with Laravel 8 we can take advantage of the encrypted attribute cast type. I've made a proof of concept doing this.

There are some considerations:

  1. In order to maintain backward-compatibility with existing uses of the package, I dynamically adjust the $casts property based on the laraguard.encrypted config value. The default value is false. This makes the change completely invisible to existing uses.
  2. Additionally, to maintain compatibility with the current table schema, I continue to use an accessor/mutator for the recovery codes so they are able to be stored as json. If we modify the column to text we can use the cast type for them as well.
  3. With the current table schema the maximum secret length that can be stored encrypted is 25. If we change the column back to text we can support longer encrypted secret lengths.

I'm interested in hearing your thoughts on this and how you would want to handle backward-compatibility

DarkGhostHunter commented 3 years ago

We also RECOMMEND storing the keys securely in the validation system, and, more specifically, encrypting them using tamper-resistant hardware encryption and exposing them only when required: for example, the key is decrypted when needed to verify an OTP value, and re-encrypted immediately to limit exposure in the RAM to a short period of time.

Well, about that RAM part, we can't do nothing since we would need to kill the model instance. But yes, I think that encrypting the secret could be feasible without BC, and applied globally.

As I see it, encryption will be global and opt-in. If you enable it, there is no way to easily check if the user has encrypted secret or not. It could be done by checking the string start, but that depends on the encrypter itself. Adding a column would break compatibility in someway, but thanks to the ORM, we can check if $totpModel->is_encrypted returns false or null.

About the secret length, sometime ago I tested with an authenticator about a length of 512-bits and didn't work, can't remember which but I could bet it was Google Authenticator. Anyway, any database schema change it's BC unless it's additive and we can know the value as null with the ORM.

I think it's a good idea to encrypt both secret and recovery keys, but because the length constraint on the secret column, I humbly think it's just better and safer to create a major version following this:

This way

srichter commented 3 years ago

Yeah, we'll never meet their full recommendations for encryption, but storing the keys encrypted is a massive improvement.

I agree with your ideas for implementation. I will start on a draft PR.

As an aside, I saw your PR adding attemptWith() to the framework; nice job! Do you have any plans to make use of that for this package? That could resolve #46, but would require a full refactor of the package to accept the code along with the credentials upon initial login.

DarkGhostHunter commented 3 years ago

Yeah. I'm also waiting for that to be approved so I can star refactoring.

With that, the only thing the user would need will be to use a package callable.

DarkGhostHunter commented 3 years ago

It's been approved. Time to work on this.

DarkGhostHunter commented 3 years ago

I landed a solution for this.

For new projects

Both shared secret and recovery codes are encrypted/decrypted by default.

For old projects upgrading from v4.0

A upgrade migration must be run. It changes the table type, and encrypts all values from the database by chunks.

DarkGhostHunter commented 3 years ago

Done in v4.0.