Skip to content

Upgrade password hashes on authentication#152

Closed
KlausTrainer wants to merge 2 commits intoapache:masterfrom
KlausTrainer:1780-upgrade-password-hashes-on-authentication
Closed

Upgrade password hashes on authentication#152
KlausTrainer wants to merge 2 commits intoapache:masterfrom
KlausTrainer:1780-upgrade-password-hashes-on-authentication

Conversation

@KlausTrainer
Copy link
Copy Markdown
Contributor

We now upgrade user docs to the new PBKDF2 password scheme on successful
authentication if the password hash is still from the old days where we
only used plain SHA-1 for hashing salted passwords.

Closes COUCHDB-1780.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should upgrade their hash after they've authenticated and proved they got the password right. Won't this just save whatever password they tried and make it the users actual password?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're totally right. My intention was to implement it exactly the way you suggest, and I have no idea why I didn't ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just realized that there should also be a test case that checks that no upgrade is being done when the password is wrong. I'll add that later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Btw., I also realized that the way I did the password hash upgrade here would totally introduce a security hole that would random people allow to choose an arbitrary new password if the existing password would be hashed with the old password scheme.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, that's what I was saying in my comment. :)

@KlausTrainer
Copy link
Copy Markdown
Contributor Author

Updated it according to the results of my previous discussion with @rnewson.

This removes client-side password crypto from the JavaScript tests.

In some JavaScript tests, it has been assumed that SHA-1 is used for the
password hash in user docs.  Those tests should, however, not rely on
implementation details of the user authentication hash function, as it
isn't the goal of those tests to check these.  Furthermore, this causes
problems when a password scheme is changed, or a new one is introduced.
We now upgrade user docs to the new PBKDF2 password scheme on successful
authentication if the password hash is still from the old days where we
only used plain SHA-1 for hashing salted passwords.

Closes COUCHDB-1780.
@rnewson
Copy link
Copy Markdown
Member

rnewson commented Feb 20, 2014

+1, looks good to me. Thanks!

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 20, 2014

Klaus Trainer on dev@couchdb.apache.org replies:
Thank you for the great review :)

@KlausTrainer KlausTrainer deleted the 1780-upgrade-password-hashes-on-authentication branch March 4, 2014 00:30
nickva pushed a commit to cloudant/couchdb that referenced this pull request Apr 21, 2017
This closes apache#152

Signed-off-by: ILYA Khlopotov <iilyak@ca.ibm.com>
lag-linaro pushed a commit to lag-linaro/couchdb that referenced this pull request Oct 25, 2018
janl pushed a commit that referenced this pull request Jan 5, 2020
nickva pushed a commit to nickva/couchdb that referenced this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants