PublickKeyTokenProvider: Fix password update routine with password hash #33898

marcelklehr · 2022-09-05T12:14:49Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

ldidry · 2022-09-12T14:26:18Z

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

lib/private/Authentication/Token/PublicKeyTokenProvider.php

marcelklehr · 2022-09-13T11:58:22Z

Let's wait till this is merged @ldidry

ldidry · 2022-09-13T12:58:34Z

Ok, thx. 🙂

lib/private/Authentication/Token/PublicKeyTokenProvider.php

icewind1991 · 2022-09-13T17:16:39Z

follow up from #33485

core/Migrations/Version25000Date20220905140840.php

lib/private/Authentication/Token/PublicKeyToken.php

core/Migrations/Version25000Date20220905140840.php

juliusknorr · 2022-09-21T19:07:48Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

+								ICrypto              $crypto,
+								IConfig              $config,
+								LoggerInterface      $logger,
+								ITimeFactory         $time,


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

lib/private/Authentication/Token/PublicKeyTokenProvider.php

apps/admin_audit/composer/autoload.php

juliusknorr

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

szaimen · 2022-12-01T18:43:21Z

/rebase

juliusknorr · 2022-12-02T12:15:51Z

Cypress failure looks related (500 error on the login)

artonge

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

juliusknorr · 2022-12-19T16:07:27Z

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 · 2022-12-27T10:32:23Z

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

artonge · 2022-12-27T10:34:11Z

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 · 2022-12-27T10:48:05Z

#35889 should fix the issue.

marcelklehr · 2022-12-27T14:01:38Z

Should be good to merge now

juliusknorr · 2023-01-03T18:02:52Z

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

Signed-off-by: Marcel Klehr <[email protected]>

Signed-off-by: Julius Härtl <[email protected]>

juliusknorr · 2023-01-05T07:01:36Z

Failure unrelated

nickvergessen · 2023-01-09T12:09:58Z

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

nickvergessen · 2023-01-09T12:14:05Z

lib/private/Authentication/Token/PublicKeyTokenProvider.php

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


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?

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

ChristophWurst · 2024-01-24T15:44:05Z

/backport to stable25

CarlSchwan reviewed Sep 5, 2022

View reviewed changes

lib/private/Authentication/Token/PublicKeyTokenProvider.php Outdated Show resolved Hide resolved

nickvergessen reviewed Sep 13, 2022

View reviewed changes

lib/private/Authentication/Token/PublicKeyTokenProvider.php Outdated Show resolved Hide resolved

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

nickvergessen reviewed Sep 13, 2022

View reviewed changes

lib/private/Authentication/Token/PublicKeyTokenProvider.php Outdated Show resolved Hide resolved

marcelklehr marked this pull request as ready for review September 14, 2022 14:16

marcelklehr requested review from ChristophWurst and miaulalala as code owners September 15, 2022 12:09

ChristophWurst reviewed Sep 15, 2022

View reviewed changes

core/Migrations/Version25000Date20220905140840.php Outdated Show resolved Hide resolved

lib/private/Authentication/Token/PublicKeyToken.php Show resolved Hide resolved

github-advanced-security bot found potential problems Sep 15, 2022

View reviewed changes

core/Migrations/Version25000Date20220905140840.php Fixed Show fixed Hide fixed

core/Migrations/Version25000Date20220905140840.php Fixed Show fixed Hide fixed

marcelklehr requested a review from ChristophWurst September 16, 2022 10:56

juliusknorr reviewed Sep 21, 2022

View reviewed changes

juliusknorr self-requested a review October 18, 2022 18:50

juliusknorr added 3. to review Waiting for reviews performance 🚀 labels Oct 18, 2022

juliusknorr added this to the Nextcloud 26 milestone Oct 18, 2022

marcelklehr force-pushed the fix/authtoken-password-update branch from 45b8964 to 3b10cdd Compare November 15, 2022 12:10

juliusknorr reviewed Nov 15, 2022

View reviewed changes

apps/admin_audit/composer/autoload.php Show resolved Hide resolved

juliusknorr approved these changes Nov 15, 2022

View reviewed changes

marcelklehr force-pushed the fix/authtoken-password-update branch from 3b10cdd to c74766e Compare November 16, 2022 12:49

marcelklehr closed this Dec 1, 2022

marcelklehr force-pushed the fix/authtoken-password-update branch from c74766e to 175ac79 Compare December 1, 2022 17:06

marcelklehr reopened this Dec 1, 2022

marcelklehr requested review from CarlSchwan and nickvergessen December 1, 2022 18:24

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

PVince81 requested review from artonge and icewind1991 December 16, 2022 17:01

artonge reviewed Dec 19, 2022

View reviewed changes

artonge approved these changes Dec 19, 2022

View reviewed changes

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

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

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

marcelklehr and others added 2 commits January 4, 2023 08:30

PublickKeyTokenProvider: Fix password update routine with password hash

adfe367

Signed-off-by: Marcel Klehr <[email protected]>

chore(version): Bump dev version to trigger database migration

bd6ac28

Signed-off-by: Julius Härtl <[email protected]>

juliusknorr force-pushed the fix/authtoken-password-update branch from f2d064a to bd6ac28 Compare January 4, 2023 07:31

juliusknorr merged commit 18164ae into master Jan 5, 2023

juliusknorr deleted the fix/authtoken-password-update branch January 5, 2023 07:01

nickvergessen reviewed Jan 9, 2023

View reviewed changes

PVince81 mentioned this pull request Jan 9, 2023

[Bug]: Password update routine slowness #36046

Closed

9 tasks

nickvergessen mentioned this pull request Jan 9, 2023

fix(authentication): Only verify each hash once #36048

Merged

4 tasks

ChristophWurst mentioned this pull request Jan 24, 2024

[stable25] Fix unnecessary authtoken updates #43087

Closed

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

Uh oh!

PublickKeyTokenProvider: Fix password update routine with password hash #33898

PublickKeyTokenProvider: Fix password update routine with password hash #33898

Uh oh!

Conversation

marcelklehr commented Sep 5, 2022

Uh oh!

Uh oh!

ldidry commented Sep 12, 2022

Uh oh!

Uh oh!

marcelklehr commented Sep 13, 2022

Uh oh!

ldidry commented Sep 13, 2022

Uh oh!

Uh oh!

icewind1991 commented Sep 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliusknorr Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

marcelklehr Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

szaimen commented Dec 1, 2022

Uh oh!

juliusknorr commented Dec 2, 2022

Uh oh!

artonge left a comment

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Dec 19, 2022

Uh oh!

marcelklehr commented Dec 27, 2022

Uh oh!

artonge commented Dec 27, 2022

Uh oh!

artonge commented Dec 27, 2022

Uh oh!

marcelklehr commented Dec 27, 2022

Uh oh!

juliusknorr commented Jan 3, 2023

Uh oh!

juliusknorr commented Jan 5, 2023

Uh oh!

nickvergessen commented Jan 9, 2023

Uh oh!

nickvergessen Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

nickvergessen Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Jan 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants