change hashing algorithm from SHA-512 to bcrypt #638

C0rby · 2020-10-01T13:13:49Z

Even though SHA-512 is currently considered a secure algorithm it is not the best choice for password hashing. As this change introduces a breaking change it is beast to introduce it as early as possible to prevent us from having to implement a migration strategy

Closes: owncloud/product#195

kulmann

Please add the changelog in ~~/docs~~ /changelog on the top level of the repo. Extensions are not supposed to have their own changelog anymore. :-)

C0rby · 2020-10-01T13:51:23Z

Ah thanks! Wasn't sure about that. 👍

kulmann

👍

micbar · 2020-10-02T19:01:21Z

please rebase

kulmann

Please rebase again :-)

accounts/pkg/service/v0/accounts.go

phil-davis · 2020-10-09T13:56:06Z

I rebased - CI might be kinder this time.

C0rby · 2020-10-09T14:25:35Z

I really don't get the error of phoenixWebUI2... It's a bit frustrating... I need to dig deeper on monday...

kulmann · 2020-10-09T14:59:09Z

ocis-server logs in drone look like you might have a wrong password for the reva user...


2020-10-09T14:14:32Z ERR Login failed binddn=cn=reva,ou=sysusers,dc=example,dc=org handler=ocis service=glauth src={"IP":"127.0.0.1","Port":36030,"Zone":""} username=reva
--
19413 | 2020-10-09T14:14:32Z ERR bind with system user failed error="LDAP Result Code 49 \"Invalid Credentials\": " pkg=rgrpc service=storage traceid=0cbbb590ef485c1067ef84cff93ab30d

kulmann · 2020-10-09T15:03:51Z

... which is weird, because when I delete my local accounts data dir and run ocis from your branch, I can login as reva:reva.

C0rby · 2020-10-13T22:04:12Z

Seems like the requests from glauth to the accounts service timeout.

2020-10-13T21:27:49Z ERR Could not list accounts error="{\"id\":\"go.micro.client\",\"code\":408,\"detail\":\"context deadline exceeded\",\"status\":\"Request Timeout\"}" basedn=dc=example,dc=org binddn=cn=konnectd,ou=sysusers,dc=example,dc=org filter=(objectClass=posixaccount) handler=ocis query="on_premises_sam_account_name eq 'user1'" service=glauth src={"IP":"127.0.0.1","Port":55198,"Zone":""}

Maybe the accounts service crashes? But why is there no hint in the logs?

C0rby · 2020-11-10T11:03:41Z

I lowered the difficulty to 11 now which should half the hashing time.

C0rby · 2020-11-10T12:03:04Z

It looks better now but I'm still not happy about some tests taking 40+ minutes.

phil-davis · 2020-11-10T12:03:54Z

I am putting together some run-time stats. Will post here when CI finishes...

I changed the drone timeout from 1 to 2 hours for this repo - hopefully the currently running pipelines do not get killed after 1 hour. I want to see how long they run.

C0rby · 2020-11-10T12:08:32Z

And it got killed...

C0rby · 2020-11-10T12:14:28Z

@phil-davis, how hard would it be to make the api tests use OpenID?

phil-davis · 2020-11-10T12:15:36Z

Comparison of any earlier "typical" CI run today vs this PR with _hashDifficulty = 11

https://drone.owncloud.com/owncloud/ocis/1322 vs https://drone.owncloud.com/owncloud/ocis/1326
local-ownc 07:08 19:40
local-ocis 05:26 14:40
           13    34 = about 2.5 times longer

core-ownc1 11:01 27:05 - 
core-ocis1 11:28 25:54
core-ownc2 04:57 14:40
core-ocis2 06:40 24:50
core-ownc3 24:51 >60
core-ocis3 27:34 >60
core-ownc4 23:32 >60
core-ocis4 21:28 >60
core-ownc5 12:42 33:01
core-ocis5 12:12 31:58
core-ownc6 18:55 46:11
core-ocis6 23:02 >60
          198   >504 = at least 2.5 times longer

phoenix-1  12:11 19:07
phoenix-2  09:14 15:12 F
phoenix-3  12:51 17:02
phoenix-4  06:01 08:58
phoenix-5  19:56 31:42 F
phoenix-6  21:56 27:07
phoenix-7  12:16 13:24
phoenix-8  08:59 11:07
          103   143 + about 40% slower

The tests are quite intensive with setup and teardown (creating/deleting users/groups...). So the API tests show a bigger slowdown because the UI tests already spend more of their time waiting for the browser to be ready.

"the system" is a lot slower with this change.

C0rby · 2020-11-10T12:36:46Z

What we could do is to make the difficulty configurable and to lower it for the tests.

C0rby · 2020-11-10T12:55:13Z

This should have similar times to the master now.

EDIT: I lowered the difficulty even more to 4. This is the lowest possible value and takes around 1.5ms to compute a hash value on my machine. Let's see if it makes any differences in the pipelines.

C0rby · 2020-11-10T13:56:22Z

Ok, looks better now.

phil-davis · 2020-11-10T14:15:47Z

What we could do is to make the difficulty configurable and to lower it for the tests.

That was a good idea. And if someone wants to test with a bigger hash difficulty they can easily change (or remove) the environment variable ACCOUNTS_HASH_DIFFICULTY and measure the time/CPU dfference.

phil-davis · 2020-11-10T14:17:12Z

There were some UI test fails - those are annoying. I restarted drone CI.

kulmann · 2020-11-10T14:19:12Z

What we could do is to make the difficulty configurable and to lower it for the tests.

That was a good idea. And if someone wants to test with a bigger hash difficulty they can easily change (or remove) the environment variable ACCOUNTS_HASH_DIFFICULTY and measure the time/CPU dfference.

Also needs to be used when updating ocis in phoenix CI. Good solution 👍
cc @dpakach

phil-davis · 2020-11-10T15:23:07Z

@C0rby other good stuff for fixing the intermittent fails issue #820 has been merged.
If you rebase this, it might pass!

C0rby · 2020-11-10T16:25:10Z

The scenario tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:359 does also fail quite consistently.
For some reason the method prepareCollaborators in apps/files/src/components/Collaborators/SharedFilesList.vue gets called multiple times. The first time with the correct shares array and then multiple times with undefined.
But only in the tests not when doing it manually...

shares  Array(1) files.bundle.js:301 
shares  undefined core.bundle.js:17
TypeError: Cannot read property 'forEach' of undefined
    at s.prepareCollaborators (files.bundle.js:301)
    at fn (files.bundle.js:301)
    at r (core.bundle.js:17)
    at s.ge [as _t] (core.bundle.js:17)
    at fn (files.bundle.js:301)
    at r (core.bundle.js:17)
    at s.ge [as _t] (core.bundle.js:17)
    at files.bundle.js:296
    at s.ve [as _l] (core.bundle.js:17)
    at s.w (files.bundle.js:296) files.bundle.js:301 
shares  undefined core.bundle.js:17
TypeError: Cannot read property 'forEach' of undefined
    at s.prepareCollaborators (files.bundle.js:301)
    at fn (files.bundle.js:301)
    at r (core.bundle.js:17)
    at s.ge [as _t] (core.bundle.js:17)
    at fn (files.bundle.js:301)
    at r (core.bundle.js:17)
    at s.ge [as _t] (core.bundle.js:17)
    at files.bundle.js:296
    at s.ve [as _l] (core.bundle.js:17)
    at s.w (files.bundle.js:296) files.bundle.js:301
shares  undefined core.bundle.js:17
TypeError: Cannot read property 'forEach' of undefined
    at s.prepareCollaborators (files.bundle.js:301)
    at fn (files.bundle.js:301)
    at r (core.bundle.js:17)
    at s.ge [as _t] (core.bundle.js:17)
    at fn (files.bundle.js:301)
    at r (core.bundle.js:17)
    at s.ge [as _t] (core.bundle.js:17)
    at files.bundle.js:296
    at s.ve [as _l] (core.bundle.js:17)
    at s.w (files.bundle.js:296)

C0rby · 2020-11-10T16:40:45Z

The UI tests are something for tomorrow...

C0rby · 2020-11-10T22:41:39Z

Waiting for owncloud/web#4297

Even though SHA-512 is currently considered a secure algorithm it is not the best choice for password hashing. As this change introduces a breaking change it is beast to introduce it as early as possible to prevent us from having to implement a migration strategy Signed-off-by: David Christofas <[email protected]>

phil-davis · 2020-11-11T04:44:02Z

owncloud/web#4297 has been merged in the phoenix repo.
#836 bumped the commit id used for the Phoenix UI tests and is now in master
I rebased this. Let's see what CI says.

phil-davis · 2020-11-11T05:45:04Z

https://drone.owncloud.com/owncloud/ocis/1348/29/7 failed.

Failures:

1) Scenario: upload new file with same name to see if different versions are shown (attempt 2) # tests/acceptance/features/webUIFiles/versions.feature:14
   ✔ Before # tests/acceptance/setup.js:28
   ✔ Before # tests/acceptance/setup.js:32
   ✔ Before # tests/acceptance/setup.js:36
   ✔ Before # tests/acceptance/setup.js:46
   ✔ Before # tests/acceptance/setup.js:68
   ✔ Before # tests/acceptance/setup.js:75
   ✔ Before # tests/acceptance/stepDefinitions/filesContext.js:18
   ✔ Before # tests/acceptance/stepDefinitions/generalContext.js:234
   ✔ Before # tests/acceptance/stepDefinitions/generalContext.js:273
   ✔ Given these users have been created with default attributes: # tests/acceptance/stepDefinitions/provisioningContext.js:199
       | username |
       | user0    |
       | user1    |
   ✔ Given user "user0" has logged in using the webUI # tests/acceptance/stepDefinitions/loginContext.js:67
   ✔ And the user has browsed to the files page # tests/acceptance/stepDefinitions/filesContext.js:82
   ✔ And user "user0" has uploaded file with content "lorem content" to "lorem.txt" # tests/acceptance/stepDefinitions/filesContext.js:92
   ✔ And user "user0" has uploaded file with content "new lorem content" to "lorem.txt" # tests/acceptance/stepDefinitions/filesContext.js:92
   ✔ When the user browses to display the "versions" details of file "lorem.txt" # tests/acceptance/stepDefinitions/filesContext.js:112
   ✔ Then the content of file "lorem.txt" for user "user0" should be "new lorem content" # tests/acceptance/stepDefinitions/filesContext.js:482
   ✖ And the versions list should contain 2 entries # tests/acceptance/stepDefinitions/filesContext.js:458
       AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

       2 !== 1

That's sad. Only one of the expected versions is displayed. There is likely some timing issue with the multiple uploads?

I restarted drone.

sonarcloud · 2020-11-11T05:50:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

phil-davis · 2020-11-11T06:43:13Z

@C0rby CI passed. IMO this PR is "a good thing" and does not make CI any less reliable. Feel free to merge if you agree.

Merge: ec8a45a 1406350 Author: David Christofas <[email protected]> Date: Wed Nov 11 10:14:36 2020 +0100 Merge pull request #638 from owncloud/accounts-change-hashing-algorithm change hashing algorithm from SHA-512 to bcrypt

C0rby requested review from IljaN, butonic and kulmann October 1, 2020 13:13

C0rby self-assigned this Oct 1, 2020

kulmann requested changes Oct 1, 2020

View reviewed changes

C0rby force-pushed the accounts-change-hashing-algorithm branch from 20ada19 to 60dd048 Compare October 1, 2020 13:55

kulmann approved these changes Oct 1, 2020

View reviewed changes

IljaN approved these changes Oct 5, 2020

View reviewed changes

C0rby force-pushed the accounts-change-hashing-algorithm branch 5 times, most recently from 3f2ff17 to c4d56ce Compare October 7, 2020 09:44

kulmann requested changes Oct 8, 2020

View reviewed changes

accounts/pkg/service/v0/accounts.go Outdated Show resolved Hide resolved

accounts/pkg/service/v0/accounts.go Show resolved Hide resolved

kulmann approved these changes Oct 8, 2020

View reviewed changes

accounts/pkg/service/v0/accounts.go Outdated Show resolved Hide resolved

accounts/pkg/service/v0/accounts.go Show resolved Hide resolved

C0rby force-pushed the accounts-change-hashing-algorithm branch from aede5ae to 2ecd62c Compare October 9, 2020 11:55

phil-davis force-pushed the accounts-change-hashing-algorithm branch from 2ecd62c to 356d1cf Compare October 9, 2020 13:55

C0rby force-pushed the accounts-change-hashing-algorithm branch from 356d1cf to 8b36256 Compare October 13, 2020 21:10

exalate-issue-sync bot mentioned this pull request Oct 22, 2020

Replace password hash algorithm - move Code to monorepo owncloud/product#195

Closed

micbar unassigned C0rby Oct 26, 2020

C0rby force-pushed the accounts-change-hashing-algorithm branch from 4d4b1cf to 231d354 Compare October 26, 2020 15:02

C0rby force-pushed the accounts-change-hashing-algorithm branch from 235ce51 to 01aae09 Compare November 10, 2020 11:08

C0rby force-pushed the accounts-change-hashing-algorithm branch from 363ce07 to 794eca1 Compare November 10, 2020 13:01

C0rby force-pushed the accounts-change-hashing-algorithm branch from 794eca1 to fe9919d Compare November 10, 2020 16:05

David Christofas added 2 commits November 11, 2020 10:26

make hash difficulty configurable

1406350

phil-davis force-pushed the accounts-change-hashing-algorithm branch from fe9919d to 1406350 Compare November 11, 2020 04:41

C0rby merged commit efe3702 into master Nov 11, 2020

delete-merged-branch bot deleted the accounts-change-hashing-algorithm branch November 11, 2020 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change hashing algorithm from SHA-512 to bcrypt #638

change hashing algorithm from SHA-512 to bcrypt #638

C0rby commented Oct 1, 2020

kulmann left a comment •

edited

Loading

C0rby commented Oct 1, 2020

kulmann left a comment

micbar commented Oct 2, 2020 •

edited

Loading

kulmann left a comment

phil-davis commented Oct 9, 2020

C0rby commented Oct 9, 2020

kulmann commented Oct 9, 2020

kulmann commented Oct 9, 2020 •

edited

Loading

C0rby commented Oct 13, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

phil-davis commented Nov 10, 2020 •

edited

Loading

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

phil-davis commented Nov 10, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020 •

edited

Loading

C0rby commented Nov 10, 2020

phil-davis commented Nov 10, 2020

phil-davis commented Nov 10, 2020

kulmann commented Nov 10, 2020

phil-davis commented Nov 10, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

phil-davis commented Nov 11, 2020

phil-davis commented Nov 11, 2020 •

edited

Loading

sonarcloud bot commented Nov 11, 2020

phil-davis commented Nov 11, 2020

change hashing algorithm from SHA-512 to bcrypt #638

change hashing algorithm from SHA-512 to bcrypt #638

Conversation

C0rby commented Oct 1, 2020

kulmann left a comment • edited Loading

Choose a reason for hiding this comment

C0rby commented Oct 1, 2020

kulmann left a comment

Choose a reason for hiding this comment

micbar commented Oct 2, 2020 • edited Loading

kulmann left a comment

Choose a reason for hiding this comment

phil-davis commented Oct 9, 2020

C0rby commented Oct 9, 2020

kulmann commented Oct 9, 2020

kulmann commented Oct 9, 2020 • edited Loading

C0rby commented Oct 13, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

phil-davis commented Nov 10, 2020 • edited Loading

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

phil-davis commented Nov 10, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020 • edited Loading

C0rby commented Nov 10, 2020

phil-davis commented Nov 10, 2020

phil-davis commented Nov 10, 2020

kulmann commented Nov 10, 2020

phil-davis commented Nov 10, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

C0rby commented Nov 10, 2020

phil-davis commented Nov 11, 2020

phil-davis commented Nov 11, 2020 • edited Loading

sonarcloud bot commented Nov 11, 2020

phil-davis commented Nov 11, 2020

kulmann left a comment •

edited

Loading

micbar commented Oct 2, 2020 •

edited

Loading

kulmann commented Oct 9, 2020 •

edited

Loading

phil-davis commented Nov 10, 2020 •

edited

Loading

C0rby commented Nov 10, 2020 •

edited

Loading

phil-davis commented Nov 11, 2020 •

edited

Loading