danpros / htmly

Simple and fast databaseless PHP blogging platform, and Flat-File CMS
https://www.htmly.com
GNU General Public License v2.0
1.01k stars 258 forks source link

Added TOTP MFA support per user. #756

Closed KuJoe closed 1 month ago

KuJoe commented 1 month ago

This PR adds the ability to enabled and disable TOTP MFA for individual users. You can add the MFA token by scanning a QR code or manually typing in the secret key. The secret key is saved in the user.ini file and can be reset by changing the value to "disabled". This can probably be done more cleanly but I wanted to implement it without having to rewrite too many core modules or functions.

The libraries used are "pragmarx/google2fa" and "bacon/bacon-qr-code", but they do require PHP 8.0 or higher to work properly making all 5.x and 7.x PHP versions incompatible.

Additionally the php-gd module is required to generate the QR code.

Should resolve issues #589 and #449

danpros commented 1 month ago

Thanks for the PR. I will test it in my local dev first before merge it.

KuJoe commented 1 month ago

Thanks for reviewing it!

Also, since this adds new functionality is it possible for other contributors to add to the documentation on https://docs.htmly.com/ or possibly adding the Wiki feature to this GitHub repo so we can build out more documentation as new things like this get added? Thanks again!

danpros commented 1 month ago

The bacon/bacon-qr-code need at least PHP 8.1. We release this version as version 3.0.0 (new major version). I will update the composer pretty soon, so we do not need to execute composer command for each new htmly installation.

vdbhb59 commented 1 month ago

Secret Key is in hash/encrypted format in user.ini or plain text? If the latter, then it needs to be secured.

Also, I will look into the wiki/docs in a few days time and add with proper snapshots.

Edit: I remember docs/wiki pages on Github. Unable to locate those now. Odd. @danpros please provide the code to add/update the guides.

danpros commented 1 month ago

@vdbhb59 this still in master branch and not yet released. And the development still ongoing so I will add the docs later after we tidy it up.

vdbhb59 commented 1 month ago

Also, why suggest only the foogle 2fa, why not some opensource app like FreeOTP+ from F-Droid? @KuJoe mate.

KuJoe commented 1 month ago

I can only suggest the things I know, never heard of that one. Feel free to use it as well.

danpros commented 1 month ago

@vdbhb59 yes we need to secure it using password_hash. Like already stated here #727

Currently the hash is bcrypt when saving the password to text, so its very strong and we need to use it either for the mfa code.

KuJoe commented 1 month ago

I think a good solution would be to encrypt the secret with the user's password so it's unencrypted at login with that password. I should be able to get this feature updated to make sure it's stored encrypted in the user.ini file.

vdbhb59 commented 1 month ago

I can only suggest the things I know, never heard of that one. Feel free to use it as well.

I know, just thought that with time people would have realized the demon foogle is. :( Been 9 years for me now. :)

KuJoe commented 1 month ago

You don't have to use Google, you can use any auth app to scan the QR code or enter the code manually. They all work with that library. You don't even need a Google account to implement it. I just picked that library because it's still being maintained (not by Google) and was easy to use.

vdbhb59 commented 1 month ago

You don't have to use Google, you can use any auth app to scan the QR code or enter the code manually. They all work with that library. You don't even need a Google account to implement it. I just picked that library because it's still being maintained (not by Google) and was easy to use.

No no. I know that. I was just trying to refer to the suggestion part of it. Nothing otherwise. I use FreeOTP+ and KeepassDX which makes it best solution for me. :)

danpros commented 1 month ago

@KuJoe Yes we should secure the secret key.

And you can remove the verify password actually. I try blank or different password and the password also changed. Remove or use password_verify($pass, $user_pass) before proceed.

During login we missing the variable $message (error message), try logout and use random MFA code.

This should fix the error:

$message['error'] = '';
$message['error'] .= '<li class="alert alert-danger">' . i18n('MFA_Error') . '</li>';

Thank you for this feature.

danpros commented 1 month ago

Reading a bunch of MFA article whether the MFA secret should encrypted or not.. its fine we leave the MFA secret as is. The MFA is for second line of defense, usually against brute force. So we should not use the user password to encrypt it because it the hacker can decrypt it than they can get the user password.

Edit: except we use another password/seed to encrypt it of course.

vdbhb59 commented 1 month ago

Reading a bunch of MFA article whether the MFA secret should encrypted or not.. its fine we leave the MFA secret as is. The MFA is for second line of defense, usually against brute force. So we should not use the user password to encrypt it because it the hacker can decrypt it than they can get the user password.

Edit: except we use another password/seed to encrypt it of course.

Ideally any 2FA/MFA is separate from the 1FA/OFA. Also, it would be beneficial to encrypt 1FA/OFA separately in itself. Probably by 256bits.

KuJoe commented 1 month ago

Doesn't the MFA secret need to be accessible to the MFA library to check the TOTP code? If you encrypt it, where will you store the decryption key?

danpros commented 1 month ago

@KuJoe docs added to github: https://github.com/danpros/htmly-docs