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

Handle one time password and very large passwords #33110

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

CarlSchwan
Copy link
Member

3 seperate commits, see description of the individual commits

This adds an option to disable storing passwords in the database. This
might be desirable when using single use token as passwords or very
large passwords.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
For passwords bigger than 250 characters, use a bigger key since the
performance impact is minor (around one second to encrypt the password).

For passwords bigger than 470 characters, give up earlier and throw
exeception recommanding admin to either enable the previously enabled
configuration or use smaller passwords.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Jul 5, 2022
@CarlSchwan CarlSchwan added this to the Nextcloud 25 milestone Jul 5, 2022
@CarlSchwan CarlSchwan requested a review from a team July 5, 2022 09:49
@CarlSchwan CarlSchwan self-assigned this Jul 5, 2022
@CarlSchwan CarlSchwan requested review from ArtificialOwl, blizzz and come-nc and removed request for a team July 5, 2022 09:49
@CarlSchwan CarlSchwan added the pending documentation This pull request needs an associated documentation update label Jul 5, 2022
@blizzz
Copy link
Member

blizzz commented Jul 5, 2022

About the second commit: explanation? Does it just take too long?

On the third commit: should not this be dependent on the introduced setting? Otherwise, is it not backend-dependent?

@CarlSchwan
Copy link
Member Author

About the second commit: explanation? Does it just take too long?

Yes it takes 30 seconds to then login if we have a 8192 key. Probably best to abort.

On the third commit: should not this be dependent on the introduced setting? Otherwise, is it not backend-dependent?

IHMO big passwords with 400 characters shouldn't be allowed as it created a tradeoff and the security argument is not valid since even 100 characters are big enough that it is uncrackable.

With the DB backend, we can ensure that the user won't shoot itself in the foot, with the other backends we need to handle the case there the user already shoot itself in the foot. Generally, though so big passwords are actually no passwords but just one-time token generated by the authentication provider.

@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 8, 2022
@blizzz blizzz merged commit 74ebb72 into master Jul 8, 2022
@blizzz blizzz deleted the feat/handle-onetime-password-large branch July 8, 2022 17:54
@PVince81
Copy link
Member

@CarlSchwan did you test with LDAP ?

as far as I remember there's logic that periodically pings the LDAP server with the user's password to check if the user was disabled on LDAP side. and if the auth fails, the user gets disabled.

now if the password is not stored, that logic should fail gracefully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants