Skip to content

Allow respecting PASSWORD_DEFAULT #19203

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.

Sign up for GitHub

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

Jump to bottom
Merged
rullzer merged 1 commit into from
Feb 4, 2020
Merged

Allow respecting PASSWORD_DEFAULT #19203

rullzer merged 1 commit into from
Feb 4, 2020
+100 −13

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Jan 29, 2020

@rullzer rullzer added the 1. to develop Accepted and waiting to be taken care of label Jan 29, 2020
@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 30, 2020

What do you think about this

makes sense for customization but what is the benefit?

@rullzer
Copy link
Member Author

rullzer commented Jan 30, 2020

What do you think about this

makes sense for customization but what is the benefit?

On some systems argon2 causes extremely high load.

@blizzz
Copy link
Member

blizzz commented Jan 30, 2020

What do you think about this

makes sense for customization but what is the benefit?

On some systems argon2 causes extremely high load.

Usually, you can tune this with the options now exposed. Already enabling a few threads helps. Or even go down on memory on weaker machines.

The core problem is rather to choose a configuration that flies. And admins might not be knowledgeable in the specifics or shy away from changing security related issues. Offering chossing hashing alghos between those to is rather a "turbo" button.

Like this it might break with a wrong setting and if PHP is not built with bcrypt.

Ain't it better to implement the Turbo button so that it takes into consideration the available threads and memory and sets the parameters accordingly? Or leave the button and do it in a repair step automatically.

@rullzer
Copy link
Member Author

rullzer commented Jan 31, 2020

Usually, you can tune this with the options now exposed. Already enabling a few threads helps. Or even go down on memory on weaker machines.

That doesn't really help with the load. It helps with speed. But if your machine 8 core machine is busy and gets a load of 13 then just having it run with more cores doesn't help.

The core problem is rather to choose a configuration that flies. And admins might not be knowledgeable in the specifics or shy away from changing security related issues. Offering chossing hashing alghos between those to is rather a "turbo" button.

Like this it might break with a wrong setting and if PHP is not built with bcrypt.

sure. That is why it says only change if you know what you are doing. We can make the warning more explicit. We have plenty of things that break your nextcloud if you set them wrong.

Ain't it better to implement the Turbo button so that it takes into consideration the available threads and memory and sets the parameters accordingly? Or leave the button and do it in a repair step automatically.

As said I'm not sure I know how to set it in such a way that it helps if your machine is under heavy load already.

@rullzer
Copy link
Member Author

rullzer commented Jan 31, 2020

Ok alternative suggestion.
We can make a (bool) switch. That says weather or not we should respect PASSWORD_DEFAULT.
That might be more error proof as it is just a bool. And in case of wrong stuff we just set it to false. So less change to brick your nextcloud.

At the same time if php changes the default they willhave a good reason.

@blizzz
Copy link
Member

blizzz commented Jan 31, 2020 •
edited
Loading

If it hammers the server so much, the memory can be capped drastically, this should shorten the runtime and this load, if i got it right. Still, we cannot judge this on setup.

+1 for PASSWORD_DEFAULT switch

@rullzer
Copy link
Member Author

rullzer commented Jan 31, 2020

Moved it to a password default switch

@rullzer rullzer changed the title Allow selecting the hashing algorithm Allow respecting PASSWORD_DEFAULT Feb 3, 2020
ChristophWurst
ChristophWurst approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise 👍

config/config.sample.php Outdated
* However if for whatever reason you want to stick with the PASSWORD_DEFAULT
* of your php version. Then set the setting to true.
*/
'hashingDefaultPassword' => false,
Copy link
Member

@ChristophWurst ChristophWurst Feb 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'hashingDefaultPassword' => false,
'hashing_default_password' => false,

we use snake_case for most config params

@rullzer rullzer added 3. to review Waiting for reviews backport-request enhancement and removed 1. to develop Accepted and waiting to be taken care of labels Feb 3, 2020
@rullzer rullzer added this to the Nextcloud 19 milestone Feb 3, 2020
@blizzz

This comment has been minimized.

Sign in to view
@blizzz blizzz force-pushed the enh/allow_selecting_hashing_algorithm branch 2 times, most recently from 772b06a to 170c57d Compare February 3, 2020 16:15
@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 Feb 3, 2020
@rullzer
Allow selecting the hashing algorithm
0d651f1 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the enh/allow_selecting_hashing_algorithm branch from 170c57d to 0d651f1 Compare February 3, 2020 20:41
@rullzer rullzer merged commit d63fc8e into master Feb 4, 2020
@rullzer rullzer deleted the enh/allow_selecting_hashing_algorithm branch February 4, 2020 10:30
@rullzer
Copy link
Member Author

rullzer commented Feb 4, 2020

/backport to stable18

@rullzer
Copy link
Member Author

rullzer commented Feb 4, 2020

/backport to stable17

@rullzer
Copy link
Member Author

rullzer commented Feb 4, 2020

/backport to stable16

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Feb 4, 2020

The backport to stable17 failed. Please do this backport manually.

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 4, 2020
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Feb 4, 2020

backport to stable18 in #19292

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Feb 4, 2020

The backport to stable16 failed. Please do this backport manually.

@maniackcrudelis maniackcrudelis mentioned this pull request Apr 15, 2020
Merged
8 tasks
@MorrisJobke
Copy link
Member

MorrisJobke commented May 25, 2020

@rullzer 18+ only I would say

@ChristophWurst ChristophWurst mentioned this pull request Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@blizzz blizzz blizzz approved these changes

@kesselb kesselb Awaiting requested review from kesselb

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement

Projects

None yet

Milestone

Nextcloud 19

Development

Successfully merging this pull request may close these issues.

6 participants

@rullzer @ChristophWurst @blizzz @MorrisJobke @juliusknorr