Skip to content

v23: allow admin password to be stored in config.php without hashing … #2222

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.

Sign up for GitHub

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

Jump to bottom
Merged
monkeyiq merged 1 commit into from
Aug 28, 2024
Merged

v23: allow admin password to be stored in config.php without hashing … #2222

monkeyiq merged 1 commit into from
Aug 28, 2024

Conversation

@monkeyiq
Copy link
Contributor

@monkeyiq monkeyiq commented Aug 27, 2024

Given that this touches the admin login route directly, some more eyes to review are appreciated.

The code uses the result of password_get_info and if the algo is null then we know the auth.adminpassword from config.php is not set to a hashed password. At that stage the new lines do a direct string compare in the same way that pwValid does in 2.2 and used to in 2.3. More explicitly:

simplesamlphp/src/SimpleSAML/Utils/Crypto.php

Line 379 in a9dc957

return $hash === $password;

This is the intent of the code. Admins can again put plaintext passwords in config.php for auth.adminpassword if desired. Hashing is recommended but not required any more and functionality returns to that of v2.2.

We can see a null for the password for example in the following test

php > var_dump(password_get_info("123"));
array(3) {
  ["algo"]=>
  NULL
  ["algoName"]=>
  string(7) "unknown"
  ["options"]=>
  array(0) {
  }
}

This was requested here #2216 (comment)

More information about the changes around the admin password hashing and how we got where we are in 2.3 is at #2212 (comment)

@monkeyiq monkeyiq requested review from thijskh and tvdijen August 27, 2024 23:22
tvdijen
tvdijen reviewed Aug 28, 2024
View reviewed changes
Copy link
Member

@tvdijen tvdijen left a comment

Choose a reason for hiding this comment

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

This looks good @monkeyiq !
I would like to add a deprecation warning to the logs when the password is plain-text, and the upgrade notes need a touch up.

@thijskh
Copy link
Member

thijskh commented Aug 28, 2024

Would it be better to use hash_equals() instead of ===?

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

I was thinking about using something like hash_equals. The first PR is using the same thing that 2.2 uses to do the comparison. I think if we want to improve that then we probably want to move to hash_equals in 2.2, 2.3+ as a logical unit.

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

The deprecation warning is a great idea. It is much better to continue to work and offer a suggestion to improve security and at some stage remove the old acceptance of plain password from config.php.

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

I think I'll make the deprecation warning and upgrade notes PR separate linking back to this.

@monkeyiq monkeyiq merged commit 5930652 into simplesamlphp:simplesamlphp-2.3 Aug 28, 2024
@codecov
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.92%. Comparing base (b46c8da) to head (fbb290e).
Report is 1 commits behind head on simplesamlphp-2.3.

Additional details and impacted files 
@@                   Coverage Diff                   @@
##             simplesamlphp-2.3    #2222      +/-   ##
=======================================================
- Coverage                44.92%   44.92%   -0.01%     
- Complexity                3892     3893       +1     
=======================================================
  Files                      162      162              
  Lines                    12972    12974       +2     
=======================================================
  Hits                      5828     5828              
- Misses                    7144     7146       +2

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

I was about the cherry pick this forward to master but I am not sure we want it that far out?

monkeyiq added a commit that referenced this pull request Aug 28, 2024
@monkeyiq
v23: allow admin password to be stored in config.php without hashing …
c29efa9 
…again (#2222)
@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

I picked it into 2.4

@tvdijen
Copy link
Member

tvdijen commented Aug 28, 2024

No, master is where we break stuff and force hashed passwords!

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

sounds good!

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 28, 2024

ok, I'll take a look at another PR for deprecation warning and release notes update.

@monkeyiq monkeyiq mentioned this pull request Aug 28, 2024
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tvdijen tvdijen tvdijen left review comments

@thijskh thijskh Awaiting requested review from thijskh

Assignees

No one assigned

Labels

security

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@monkeyiq @thijskh @tvdijen