feat: add configurable options #1546

ernolf · 2024-07-12T12:45:20Z

Solves: New hashing algorithm? #26
Related: Token unsafe in FreeOTP #1324
Background: https://help.nextcloud.com/t/lenght-of-totp-key/169226

Otherwise, the newly added chapter in the README.md applies to itself.

ernolf

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 <[email protected]>

Signed-off-by: ernolf <[email protected]>

ChristophWurst

Nice!

README.md

ChristophWurst · 2024-07-12T16:16:57Z

CI has found a few issues. See the workflow output :)

Signed-off-by: ernolf <[email protected]>

ernolf · 2024-07-12T20:23:53Z

CI has found a few issues. See the workflow output :)

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 <[email protected]>

ernolf · 2024-07-17T18:40:41Z

@ChristophWurst

Codecov keeps failing:

Error: There was an error fetching the storage URL during POST: 429 - {'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected time to availability: 3146s.', code='throttled')}

ChristophWurst · 2024-07-17T18:58:47Z

Sorry, this is becaues of a fork

ChristophWurst

Tested & works

Changing algorithm breaks existing setups. We have to make the very clear.

Sorry for the late review

ChristophWurst · 2024-07-17T19:16:24Z

README.md

+### 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.


Suggested change

* 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?

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.

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?

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.

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

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.

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.

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

@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.

github-actions · 2024-07-27T02:19:53Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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.)

ernolf added 2 commits July 12, 2024 09:56

make hash algorithm case insensitive

4d2e7b7

Signed-off-by: ernolf <[email protected]>

SystemKeeper requested review from ChristophWurst and st3iny July 12, 2024 12:52

SystemKeeper linked an issue Jul 12, 2024 that may be closed by this pull request

New hashing algorithm? #26

Open

ChristophWurst approved these changes Jul 12, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

ChristophWurst added enhancement 3. to review labels Jul 12, 2024

ernolf added 3 commits July 12, 2024 21:37

Fix constructor indentation (Lint)

46b903f

Signed-off-by: ernolf <[email protected]>

Add type declarations for class properties in Totp.php

591847f

Signed-off-by: ernolf <[email protected]>

Update return types to string in config methods

1abf35c

Signed-off-by: ernolf <[email protected]>

incorporate sugested changes from riview

8251d0f

Signed-off-by: ernolf <[email protected]>

ChristophWurst approved these changes Jul 17, 2024

View reviewed changes

ernolf mentioned this pull request Jul 22, 2024

feat: introducing configurable options #1549

Open

github-actions bot added the feedback-requested label Jul 27, 2024

ernolf closed this Aug 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add configurable options #1546

feat: add configurable options #1546

ernolf commented Jul 12, 2024

ChristophWurst left a comment

ChristophWurst commented Jul 12, 2024

ernolf commented Jul 12, 2024

ernolf commented Jul 17, 2024

ChristophWurst commented Jul 17, 2024

ChristophWurst left a comment

ChristophWurst Jul 17, 2024

ernolf Jul 17, 2024

ChristophWurst Jul 17, 2024

ernolf Jul 17, 2024 •

edited

Loading

ernolf Jul 17, 2024

ernolf Jul 17, 2024 •

edited

Loading

ernolf Jul 17, 2024

ChristophWurst Jul 18, 2024

ernolf Jul 22, 2024

github-actions bot commented Jul 27, 2024

	* 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.

feat: add configurable options #1546

feat: add configurable options #1546

Conversation

ernolf commented Jul 12, 2024

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst commented Jul 12, 2024

ernolf commented Jul 12, 2024

ernolf commented Jul 17, 2024

ChristophWurst commented Jul 17, 2024

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst Jul 17, 2024

Choose a reason for hiding this comment

ernolf Jul 17, 2024

Choose a reason for hiding this comment

ChristophWurst Jul 17, 2024

Choose a reason for hiding this comment

ernolf Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

ernolf Jul 17, 2024

Choose a reason for hiding this comment

ernolf Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

ernolf Jul 17, 2024

Choose a reason for hiding this comment

ChristophWurst Jul 18, 2024

Choose a reason for hiding this comment

ernolf Jul 22, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 27, 2024

ernolf Jul 17, 2024 •

edited

Loading

ernolf Jul 17, 2024 •

edited

Loading