-
Notifications
You must be signed in to change notification settings - Fork 56
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
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fe09582
add configurable options
ernolf 4d2e7b7
make hash algorithm case insensitive
ernolf 46b903f
Fix constructor indentation (Lint)
ernolf 591847f
Add type declarations for class properties in Totp.php
ernolf 1abf35c
Update return types to string in config methods
ernolf 8251d0f
incorporate sugested changes from riview
ernolf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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.
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.
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.
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 beastOr 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.
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.
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.
Fair, but not everyone uses this app so I would like to find a way that is fool proof ;-)
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
\OCA\TwoFactorTOTP\Db\TotpSecretMapper
and\OCA\TwoFactorTOTP\Db\TotpSecret
for the new new attributes (mapped from database rows)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.