[4.x] Option to encrypt Shared Secret and Recovery Codes #56
Comments
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 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 I think it's a good idea to encrypt both secret and recovery keys, but because the length constraint on the
This way
|
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 |
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. |
It's been approved. Time to work on this. |
I landed a solution for this. For new projectsBoth shared secret and recovery codes are encrypted/decrypted by default. For old projects upgrading from v4.0A upgrade migration must be run. It changes the table type, and encrypts all values from the database by chunks. |
Done in v4.0. |
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:
$casts
property based on thelaraguard.encrypted
config value. The default value isfalse
. This makes the change completely invisible to existing uses.I'm interested in hearing your thoughts on this and how you would want to handle backward-compatibility
The text was updated successfully, but these errors were encountered: