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

PublickKeyTokenProvider: Fix password update routine with password hash #33898

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

marcelklehr
Copy link
Member

fixes #33757

@ldidry
Copy link
Contributor

ldidry commented Sep 12, 2022

@marcelklehr Do you think this PR could be tested on production server? We have like 600GiB of WAL every 2 days (for a DB that weights less than 200GiB), it puts a real burden on our backup server.

@marcelklehr marcelklehr force-pushed the fix/authtoken-password-update branch from ed55e2d to 9a2a28f Compare September 13, 2022 11:57
@marcelklehr
Copy link
Member Author

Let's wait till this is merged @ldidry

@ldidry
Copy link
Contributor

ldidry commented Sep 13, 2022

Ok, thx. 🙂

@icewind1991
Copy link
Member

follow up from #33485

@marcelklehr marcelklehr marked this pull request as ready for review September 14, 2022 14:16
Comment on lines 68 to 71
ICrypto $crypto,
IConfig $config,
LoggerInterface $logger,
ITimeFactory $time,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe revert the additional spaces as the last line also doesn't have it ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@juliusknorr juliusknorr self-requested a review October 18, 2022 18:50
@juliusknorr juliusknorr added this to the Nextcloud 26 milestone Oct 18, 2022
@marcelklehr marcelklehr force-pushed the fix/authtoken-password-update branch from 45b8964 to 3b10cdd Compare November 15, 2022 12:10
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Apart from the comment, please squash the fixups and check the lint failure. Otherwise good to get in from my side 👍

@marcelklehr marcelklehr force-pushed the fix/authtoken-password-update branch from 3b10cdd to c74766e Compare November 16, 2022 12:49
@marcelklehr marcelklehr closed this Dec 1, 2022
@marcelklehr marcelklehr force-pushed the fix/authtoken-password-update branch from c74766e to 175ac79 Compare December 1, 2022 17:06
@marcelklehr marcelklehr reopened this Dec 1, 2022
@szaimen
Copy link
Contributor

szaimen commented Dec 1, 2022

/rebase

@nextcloud-command nextcloud-command force-pushed the fix/authtoken-password-update branch from c00f143 to 441d383 Compare December 1, 2022 18:47
@juliusknorr
Copy link
Member

Cypress failure looks related (500 error on the login)

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Maybe naive, but couldn't we compare the value of decrypted password ?

@juliusknorr
Copy link
Member

Maybe naive, but couldn't we compare the value of decrypted password ?

We cannot decrypt all tokens of a user to update them as we only got the user password when the update is called, not the actual cipher to decrypt the tokens (e.g. the app passwords).

@marcelklehr marcelklehr force-pushed the fix/authtoken-password-update branch from 441d383 to 3c7bae8 Compare December 27, 2022 09:56
@marcelklehr
Copy link
Member Author

For some reason the migration isn't executed before the cypress tests run. Help is appreciated.

@artonge
Copy link
Contributor

artonge commented Dec 27, 2022

For some reason the migration isn't executed before the cypress tests run. Help is appreciated.

I had the same issue in another PR. Let me see if I can come up with a generic solution

@artonge
Copy link
Contributor

artonge commented Dec 27, 2022

#35889 should fix the issue.

@marcelklehr marcelklehr force-pushed the fix/authtoken-password-update branch from 3c7bae8 to 3559da1 Compare December 27, 2022 13:38
@marcelklehr
Copy link
Member Author

Should be good to merge now

@juliusknorr juliusknorr force-pushed the fix/authtoken-password-update branch 2 times, most recently from 703a860 to f2d064a Compare January 3, 2023 18:02
@juliusknorr
Copy link
Member

Pushed another commit to bump the version and trigger the db upgrade on existing setups.

marcelklehr and others added 2 commits January 4, 2023 08:30
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the fix/authtoken-password-update branch from f2d064a to bd6ac28 Compare January 4, 2023 07:31
@juliusknorr
Copy link
Member

Failure unrelated

@juliusknorr juliusknorr merged commit 18164ae into master Jan 5, 2023
@juliusknorr juliusknorr deleted the fix/authtoken-password-update branch January 5, 2023 07:01
@nickvergessen
Copy link
Member

This basically breaks Talk integration tests.
On each login the password column as well as password_hash of oc_authtokens is changed, for all entries, although the user password never changes (admin:admin).

$encryptedPassword = $this->encryptPassword($password, $publicKey);
if ($t->getPassword() !== $encryptedPassword) {
$t->setPassword($encryptedPassword);
if ($t->getPasswordHash() === null || $this->hasher->verify(sha1($password) . $password, $t->getPasswordHash())) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you maybe mean:

Suggested change
if ($t->getPasswordHash() === null || $this->hasher->verify(sha1($password) . $password, $t->getPasswordHash())) {
if ($t->getPasswordHash() === null || !$this->hasher->verify(sha1($password) . $password, $t->getPasswordHash())) {

So the password is updated everytime it does NOT match the old stored password?

Copy link
Member

Choose a reason for hiding this comment

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

But the actual problem is $this->hasher->verify takes like ~0.2 seconds

@ChristophWurst
Copy link
Member

/backport to stable25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update password check not working as expected
9 participants