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

Move to stronger password hash #2401

Merged
merged 12 commits into from Apr 17, 2018

Conversation

Projects
None yet
5 participants
@TheAspens
Copy link
Member

commented Mar 8, 2018

These changes will close issue #2353 and close issue #1644.

@TheAspens TheAspens changed the title WIP: Move to stronger password hash Move to stronger password hash Mar 9, 2018

@TheAspens TheAspens changed the title Move to stronger password hash [WIP] Move to stronger password hash Mar 9, 2018

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2018

@drshawnkwang and @tristanolive - can the two of you please review this pull request and see if anything needs to be changed in the Drupal code?

@TheAspens TheAspens changed the title [WIP] Move to stronger password hash Move to stronger password hash Mar 9, 2018

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2018

@davidpanderson - can you please review this as well but if you are ok with it, please don't merge until either @drshawnkwang and @tristanolive have chimed in.

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2018

Also @brevilo and @nicolas17 - since you guys reveiwed the original design - do you guys want to review the implementation real quick?

@brevilo brevilo requested review from brevilo and davidpanderson Mar 12, 2018

@brevilo

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

FYI, as I added myself as a reviewer David got dropped for an unknown reason, hence I added him back.

@brevilo
Copy link
Contributor

left a comment

A general note: please try to stick to one format/style for arguments lists. That is, where you use spaces around arguments, operators and commas. Ideally use a formatter with your preferred style rules, preferably matching (or closely following) BOINC's. Thanks.

function do_passwd_rehash($user,$passwd_hash) {
$database_passwd_hash = password_hash($passwd_hash , PASSWORD_DEFAULT);
$result = $user->update(
"passwd_hash='$database_passwd_hash'"

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

@@ -176,7 +177,8 @@ function success($x) {
$query .= " email_addr='$email_addr', ";
}
if ($password_hash) {
$query .= " passwd_hash='$password_hash', ";
$database_passwd_hash = password_hash($password_hash , PASSWORD_DEFAULT);
$query .= " passwd_hash='$database_passwd_hash', ";
}

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

$email_addr = BoincDb::escape_string($email_addr);
$result = $user->update(
"email_addr='$email_addr', passwd_hash='$passwd_hash', email_validated=0"
"email_addr='$email_addr', passwd_hash='$database_passwd_hash', email_validated=0"

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

@@ -45,7 +46,8 @@
}
$passwd_hash = md5($passwd.$user->email_addr);
$result = $user->update("passwd_hash='$passwd_hash'");
$database_passwd_hash = password_hash( $passwd_hash, PASSWORD_DEFAULT);
$result = $user->update("passwd_hash='$database_passwd_hash'");

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

function do_passwd_rehash($user,$passwd_hash) {
$database_passwd_hash = password_hash($passwd_hash , PASSWORD_DEFAULT);
$result = $user->update(
"passwd_hash='$database_passwd_hash'"

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

@@ -49,9 +57,19 @@ function login_with_email($email_addr, $passwd, $next_url, $perm) {
error_page("This account has been administratively disabled.");
}
// allow authenticator as password
if ($passwd != $user->authenticator) {
if ($passwd != $user->authenticator ) {

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Why the added space? It's inconsistent.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

removed.

function do_passwd_rehash($user,$passwd_hash) {
$database_passwd_hash = password_hash($passwd_hash , PASSWORD_DEFAULT);
$result = $user->update(
"passwd_hash='$database_passwd_hash'"

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

@@ -72,16 +80,28 @@
// if no password set, set password to account key
//
if (!strlen($user->passwd_hash)) {
$user->passwd_hash = $auth_hash;
$user->passwd_hash = password_hash($auth_hash , PASSWORD_DEFAULT);
$user->update("passwd_hash='$user->passwd_hash'");

This comment has been minimized.

Copy link
@brevilo

brevilo Mar 12, 2018

Contributor

Here you escape the password hash before its use as part of an SQL clause. If you think it's required, it should be done here as well.

This comment has been minimized.

Copy link
@TheAspens

TheAspens Mar 13, 2018

Author Member

After reviewing, the hash does not need to be escaped so I have removed it from the referenced line.

@TheAspens TheAspens requested a review from tristanolive Mar 13, 2018

TheAspens added some commits Mar 13, 2018

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2018

I fixed the style issues. Please let me know if you have any other feedback on the change. Thanks!

@drshawnkwang

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

I looked at the code and checked-out your branch to test it on my Drupal devel. system. My testing shows that there shouldn't be any serious inconsistency with your PR and the drupal codebase. But we should also see if Tristan has any comments. Additionally, we'll work independently to put similar code into the drupal codebase.

@tristanolive

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

I'd expect some problems to arise with RPCs on the Drupal side, as some were implemented as simple wrappers around BOINC RPCs and others were a bit more involved. In particular, the create_account implementation in Drupal will likely need your changes integrated into it.

As Shawn's tests show, though, there doesn't look to be anything major.

@brevilo
Copy link
Contributor

left a comment

Thanks!

@tristanolive

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

Update for clarity: This PR can go ahead without waiting for corresponding Drupal updates (which should follow before too long).

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2018

@tristanolive - thanks for the clarification. Since I added you as a reviewer can you either approve or remove yourself as a reviewer? Thanks!

@davidpanderson - this touches enough stuff that I would appreciate your review on this - and if you are ok with it, please go ahead and merge it. Thanks!

@drshawnkwang

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

I know this PR has 'pass review', but would it be possible to make two minor late changes:

  1. function do_passwd_rehash() - could this be moved into an .inc file, such as user_util, this way the Drupal-BOINC code could load the library and I can use this function?

  2. The file html/inc/password.php - could it be renamed html/inc/password.inc ? This would be make it consistent with the other libraries in that directory. The Drupal code selectively loads .inc files for use as libraries. We would want to require_once this file as you have.

Maybe do_passwd_rehash() could be moved into the password.inc library?

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

@drshawnkwang - yes - I can make those. I'm updating this to WIP while I make those changes.

@TheAspens TheAspens changed the title Move to stronger password hash [WIP] Move to stronger password hash Apr 3, 2018

TheAspens added some commits Apr 4, 2018

web: refactor password hashing changes to move compatibility library …
…as a

.inc file and to move common functions into user_util.inc

@TheAspens TheAspens changed the title [WIP] Move to stronger password hash Move to stronger password hash Apr 4, 2018

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2018

@drshawnkwang - please review again and let me know if this matches what you suggested and work for you. Thanks for the feedback.

@drshawnkwang

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

@TheAspens , thanks for the changes. I checked out this branch and was able to load the password_compat.inc and user_util.inc libraries in order to use the functions you wrote. Thanks again for making these changes.

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2018

I just found this #1644. Which will also be closed by this change.

@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

@drshawnkwang @brevilo @tristanolive @davidpanderson - This has been open long enough for any objections to be raised and so it should be good to merge. Can someone go ahead and merge this? Thanks!

@brevilo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

@davidpanderson hasn't signed-off yet so he should do the merge as soon as he's ok with it.

@davidpanderson

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

This looks fine, but it conflicts with the token PR. Kevin, feel free to merge after resolving that.

Merge branch 'master' into knr_passwd_hash
Conflicts:
	py/Boinc/setup_project.py
@TheAspens

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2018

Ok - I've fixed the merge conflict. Everyone has approved this pull request so if someone can merge it I would appreciate it! (by the rules, I cannot merge my own).

@davidpanderson davidpanderson merged commit ad28e83 into master Apr 17, 2018

3 checks passed

Scrutinizer 3 new issues, 10 updated code elements
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@TheAspens TheAspens deleted the knr_passwd_hash branch Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.