Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added TOTP MFA support per user. #756

Merged
merged 2 commits into from
May 18, 2024
Merged

Added TOTP MFA support per user. #756

merged 2 commits into from
May 18, 2024

Conversation

KuJoe
Copy link
Contributor

@KuJoe KuJoe commented May 17, 2024

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

KuJoe added 2 commits May 17, 2024 11:56
Added the option to enable/disable TOTP MFA per user using QR code or manually entering a key.
@danpros
Copy link
Owner

danpros commented May 17, 2024

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

@KuJoe
Copy link
Contributor Author

KuJoe commented May 17, 2024

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 danpros merged commit 71418fa into danpros:master May 18, 2024
@danpros
Copy link
Owner

danpros commented May 18, 2024

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
Copy link
Contributor

vdbhb59 commented May 19, 2024

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
Copy link
Owner

danpros commented May 19, 2024

@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
Copy link
Contributor

vdbhb59 commented May 20, 2024

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

@KuJoe
Copy link
Contributor Author

KuJoe commented May 20, 2024

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

@danpros
Copy link
Owner

danpros commented May 20, 2024

@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
Copy link
Contributor Author

KuJoe commented May 20, 2024

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
Copy link
Contributor

vdbhb59 commented May 20, 2024

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
Copy link
Contributor Author

KuJoe commented May 20, 2024

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
Copy link
Contributor

vdbhb59 commented May 20, 2024

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
Copy link
Owner

danpros commented May 20, 2024

@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
Copy link
Owner

danpros commented May 20, 2024

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
Copy link
Contributor

vdbhb59 commented May 20, 2024

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
Copy link
Contributor Author

KuJoe commented May 20, 2024

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
Copy link
Owner

danpros commented Jun 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants