- Notifications
You must be signed in to change notification settings - Fork35
Pw hashing rework#1114
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Pw hashing rework#1114
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This change modifies the database and requires a database upgradefrom within the lansuite admin area.After the database upgrade, the old MD5 user passwords will beautomatically converted to PBKDF2-SHA1 on the next user login.Clan and team passwords will only be upgraded, when new passwordsare set.PBKDF2-SHA1 was choosen for compatibility with the ejabberd XMPPserver password storage.
@M4LuZ What is missing for this PR to move out of the draft status?
|
M4LuZ commentedMay 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@andygrunwald ,
Also I was thinking about extending cases with Unit-tests for the hashing function itself are missing and as this is one of the most crucial bits that must work I would like to see them defined and working first But as for the original features and functionality this is feature-complete, tested and has unit test cases working |
andygrunwald left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had a quick review and it seems we need to put a bit more work into this.@M4LuZ Let me know if you have time to do it or if you require some help.
Overall: I don't like the "global static" aspect of the class. This makes it pretty hard to re-use. I would rather love to see an object oriented approach with inheritance where each algorithm is its own sub class.
| @@ -332,7 +332,7 @@ public function login($email, $password, $show_confirmation = 1) | |||
| $func->log_event(t('Login fehlgeschlagen. Email (%1) nicht verifiziert', $user['email']), "2", "Authentifikation"); | |||
| // User login and wrong password? | |||
| } elseif ($user["user_login"] and$tmp_login_pass !=$user["password"]) { | |||
| } elseif ($user["user_login"] and!PasswordHash::verify($password,$user["password"])) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is the variable$tmp_login_pass still relevant?
In line 236, we still do md5 hashing, but continue to use the clear text password.
| $hashProperties = self::getInfo($hash); | ||
| // rehashing needed if algorithm does not match or amount of iterations is different than system setting | ||
| // I was thinking about keeping higher iteration counts, but this would prevent any recovery from setting value too high by mistake | ||
| return ($hashProperties['hash'] != $systemConfig['hash']) || ($hashProperties['iterations'] != $systemConfig['iterations']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I login, i see
| @@ -8,8 +10,8 @@ function CheckClanPW($clanpw) | |||
| { | |||
| global $database; | |||
| $clan = $database->queryWithOnlyFirstRow("SELECT password FROM %prefix%clan WHERE clanid = ?", [$_GET['clanid']]); | |||
| if ($clan['password'] and $clan['password'] == md5($clanpw)) { | |||
| $clan = $db->qry_first("SELECT password FROM %prefix%clan WHERE clanid = %int%", $_GET['clanid']); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is this query changed?
This can't work, as the global variable$db is not present.
My suggestion: Using the old database query code here
| $clan =$db->qry_first("SELECT password FROM %prefix%clan WHERE clanid =%int%",$_GET['clanid']); | |
| $clan =$database->queryWithOnlyFirstRow("SELECT password FROM %prefix%clan WHERE clanid =?",[$_GET['clanid']]); |
| <entries> | ||
| <entry> | ||
| <value>default</value> | ||
| <description>Standard</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is "Standard"?
Should we write "Standard (Algo)"?
Uh oh!
There was an error while loading.Please reload this page.
| case 'md5': | ||
| return md5($password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is the rational still supporting md5?
| protected function setUp() : void | ||
| { | ||
| $GLOBALS['cfg'] = []; | ||
| error_reporting(E_ALL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| error_reporting(E_ALL); |
PHPUnit has its own error handling and we should use this:https://docs.phpunit.de/en/11.2/error-handling.html
| protected function setUp() : void | ||
| { | ||
| $GLOBALS['cfg'] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lets try to avoid globals in unit tests at all.
If this is needed, we should work on a bit of a refactoring.
| error_reporting(E_ALL); | ||
| // the following test hashes will be used for all functions and tests, thus defined once and shared globally | ||
| $GLOBALS['hashTestArray'] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lets use data providers for this:https://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers
| /* | ||
| * @covers PasswordHash::hash | ||
| public function testHash() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this dead code and can be removed?
Co-authored-by: Andy Grunwald <andygrunwald@gmail.com>
|
What is this PR doing?
Recreation of PR#83 on current master with quite a good amount of further work done.
This is not yet complete, but close enough for a wider audience to have a look and provide feedback.
pbkdf2pbkdf2-sha512as defaultmd5hashes to an intermediate custom formatmd5-sha512via update script with transparent changeover on login topbkdf2-sha512, depends on a module framework rework or CLI updater to be fully usablehashand implementation code be done)Which issue(s) this PR fixes:
refs#83
Checklist
CHANGELOG.mdentry