-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: add configurable options #1546
Conversation
1. Secret Length: * Default: 32 characters (160 bits) * Minimum: 26 characters (130 bits) * Maximum: 128 characters (640 bits) Adjust the secret length using: occ config:app:set --value=64 -- twofactor_totp secret_length 2. Hash Algorithm: * Default: SHA-1 (sha1) * Optionally use SHA-256 (sha256) or SHA-512 (sha512): occ config:app:set --value=sha512 -- twofactor_totp hash_algorithm 3. **Token Length:** * Default: 6 digits * Minimum: 6 digits * Maximum: 12 digits Set the token length using: occ config:app:set --value=9 -- twofactor_totp token_length Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
CI has found a few issues. See the workflow output :) |
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
Got it! I've cleared everything on my end that I could. If there are any remaining issues or specific actions needed, please let me know. Thanks! |
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
Codecov keeps failing:
|
Sorry, this is becaues of a fork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested & works
Changing algorithm breaks existing setups. We have to make the very clear.
Sorry for the late review
### Considerations | ||
* The secret length affects only the initial generation of secrets for users. Once generated, secrets are encrypted and stored in the database and unencrypted stored in the TOTP app/device. Changing the secret length afterwards does not affect previously generated secrets. | ||
|
||
* Similarly, token length and hash algorithm can be changed at any time using the provided commands. However, these changes must be synchronized with all users of TOTP-enabled accounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Similarly, token length and hash algorithm can be changed at any time using the provided commands. However, these changes must be synchronized with all users of TOTP-enabled accounts. | |
* Similarly, token length and hash algorithm can be changed at any time using the provided commands. However, these changes must be synchronized with all users of TOTP-enabled accounts. |
I think "synchronized" should be phrazed a bit clearer. Can users actually change/upgrade their TOTP app entries when the admin changes these settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can users actually change/upgrade their TOTP app entries when the admin changes these settings?
Yes. If the token length and/or the hash algorithm are changed on the server, existing secrets can continue to be used as soon as these values are set to the same in the TOTP app.
This opens up the possibility of a completely new security strategy. For example, by agreeing on alternating token lengths or hash algorithms according to a predetermined time frame.
Yes, I know, that sounds a bit exaggerated, but that definitely makes Nextcloud's TOTP future proof.
Unfortunately, I cannot contribute any screenshots of my Aegis app because the app prevents screenshots for security reasons.
How would you like to explain it more clearly?
An admin will certainly first try to figure out how it works and quickly find out.
But I'm always in favor of a better explanation, but I couldn't think of anything better right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If the token length and/or the hash algorithm are changed on the server, existing secrets can continue to be used as soon as these values are set to the same in the TOTP app.
I use FreeOTP and it does not allow me to change this.
How about we persist the algorithm and token length with the secret? Then an admin can change the default, but they will only affect new registrations, old ones will just continue to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we persist the algorithm and token length with the secret? Then an admin can change the default, but they will only affect new registrations, old ones will just continue to work?
I think that would be very difficult to implement.
Of course everything is possible but then different instances of the app would have to run alongside each other. Basically for every registered TOTP user and then it wouldn't be a second factor app but a beast
Or can you think of an elegant way to implement such?
I would like to encourage you to install Aegis and migrate your existing secrets. Then you can try it out by yourself and see how it feels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
different instances of the app would have to run alongside each other.
Well, I just realize that this is nonsense what I wrote.. it won't be that bad, but data will have to be stored about which user registered with which algorithm/token length.
I can imagine that, but the implementation is certainly beyond my coding skills
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The time window of 30 seconds can also be changed, both with the nextcloud twofactor_totp app as with Aegis. However, after a lot of trying, I was unable to create effective, working OTPs with a time window other than 30 seconds. That is why I did not implement that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data will have to be stored about which user registered with which algorithm/token length.
Which would open up completely new possibilities, where every user can set and change their TOTP values themselves.
Changes should only be applied if an OTP with the new settings has been correctly generated, as a sign and counter-proof that it works (as with the first setup).
If you can already see how this could be implemented, then that is of course all the better and then this PR can be closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to encourage you to install Aegis and migrate your existing secrets. Then you can try it out by yourself and see how it feels.
Fair, but not everyone uses this app so I would like to find a way that is fool proof ;-)
it won't be that bad, but data will have to be stored about which user registered with which algorithm/token length.
We have the table twofactor_totp_secrets, which we can amend by columns for algorithm etc. At registration time we persist the options used, and continue to use those even when the admins have set new default options (with better security.
To achieve this
- Add a migration to add the columns
- Adjust
\OCA\TwoFactorTOTP\Db\TotpSecretMapper
and\OCA\TwoFactorTOTP\Db\TotpSecret
for the new new attributes (mapped from database rows) - Write options when a secret is generated
- Read options when a secret is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChristophWurst
Please take a look at the new PR #1549 which addresses your security concerns, we should continue to work on that one and close this PR if it is okay for you.
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Otherwise, the newly added chapter in the README.md applies to itself.
ernolf