10000 Support security encoder and added support for argon2 and co. by Toflar · Pull Request #536 · contao/contao · GitHub
[go: up one dir, main page]
Skip to content

Support security encoder and added support for argon2 and co. #536

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
merged 5 commits into from
Jun 27, 2019
Merged

Support security encoder and added support for argon2 and co. #536

merged 5 commits into from
Jun 27, 2019

Conversation

Toflar
Copy link
Member
@Toflar Toflar commented Jun 27, 2019 •
edited
Loading

In Symfony 4.3, a new security encoder named auto was introduced. It will choose the best password hashing algorithm based on what Symfony and/or the PHP community decides.

Currently this would mean as of PHP 7.2 if you did compile your PHP using libsodium, you'll have argon2 available. I wanted to make sure my setup benefits from the strongest algorithms available so I've implemented the auto configuration by default and it will fall back to the current bcrypt configuration in case you are not yet on Symfony 4.3. Of course auto may still take bcrypt in case your system does not support libsodium.

While implementing I noticed that we still use the password hashing API directly instead of relying on the Symfony components. This means you can configure a different algorithm in the security configuration but it will never work 😄 So I fixed this by replacing all our direct password hashing API calls with the underlying Symfony security components.

BTW: I had to remove the usage of password_needs_rehash() to completely rely on the Symfony encoder configured. This means right now you cannot automatically upgrade which is why currently the SodiumPasswordEncoder will also verify bcrypt passwords. Symfony will address this in 4.4 so we can (symfony/symfony#31843) completely rely on what Symfony is doing very soon. As of now, there are only bcrypt and argon2 anyway and those will be handled properly.

@Toflar Toflar added the feature label Jun 27, 2019
@Toflar Toflar requested review from bytehead, aschempp and leofeyer June 27, 2019 09:46
@Toflar Toflar self-assigned this Jun 27, 2019
@Toflar Toflar requested a review from ausi June 27, 2019 09:58
aschempp
aschempp approved these changes Jun 27, 2019
View reviewed changes
Copy link
Member
@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

LGTM except for the class usages

@Toflar Toflar marked this pull request as ready for review June 27, 2019 10:37
@Toflar Toflar added this to the 4.8 milestone Jun 27, 2019
leofeyer
leofeyer requested changes Jun 27, 2019
View reviewed changes
Copy link
Member
@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

First of all, this change implies dropping support for Symfony 4.2, so the composer.json file needs to be adjusted.

BTW: I had to remove the usage of password_needs_rehash()

So how to old password hashes get updated and persisted now?

@Toflar
Copy link
Member Author
Toflar commented Jun 27, 2019

First of all, this change implies dropping support for Symfony 4.2, so the composer.json file needs to be adjusted.

That's not true. It continues using bcrypt if you have < 4.3.

So how to old password hashes get updated and persisted now?

They don't. We can only implement that when Symfony 4.4 is released. But we're talking about bcrypt to something else and "something else" does not exist in PHP yet. The PASSWORD_DEFAULT constant is still configured to bcrypt so nothing had to be upgraded so far. This PR would only mean a regression if PHP updated their constant which they did not yet and there are no plans for 7.4 either. So by the time they'll do that we'll have Symfony 4.4 and changed already :)

@leofeyer
Copy link
Member

That's clever 🦊

Then the only thing that we need to discuss is the class usage. There are still a lot of getEncoder(BackendUser::class) and getEncoder(FrontendUser::class) calls, although both use the same algorithm and we could use getEncoder(User::class) everywhere.

@Toflar
Copy link
Member Author
Toflar commented Jun 27, 2019

Yeah in fact it doesn't really matter. It's just "more correct" the way it is now 😄

@leofeyer
Copy link
Member
leofeyer commented Jun 27, 2019 •
edited
Loading

Note to myself: After merging this, create a ticket for Contao 4.9 reminding us to implement the PasswordUpgraderInterface interface (see symfony/symfony#31843).

@leofeyer leofeyer force-pushed the feature/auto-security-encoder branch from b270ec6 to 62b6531 Compare June 27, 2019 14:08
@leofeyer leofeyer merged commit a9d5000 into master Jun 27, 2019
@leofeyer
Copy link
Member

Thank you @Toflar.

@leofeyer leofeyer deleted the feature/auto-security-encoder branch June 27, 2019 14:27
@leofeyer leofeyer mentioned this pull request Jun 27, 2019
Closed
@leofeyer
Copy link
Member

Reminder issue: #538

@richardhj
Copy link
Member

contao/core-bundle/src/Resources/contao/library/Contao/Encryption.php

Lines 183 to 185 in f5748db

public static function test($strHash)
{
if (strncmp($strHash, '$2y$', 4) === 0)

Encryption::test() provides a method to test if a given string is a hash.

Recently I've been using if (strncmp($strHash, '$2y$', 4) === 0) to check if a string is already hashed.

  1. Should Encryption::test() be updated, even though it is deprecated?
  2. How do I check if a string is already hashed?

@Toflar
Copy link
Member Author
Toflar commented Jun 28, 2019 
2\. How do I check if a string is already hashed?

Why do you need that anyway? :)

@richardhj
Copy link
Member

Why do you need that anyway? :)

In the save_callback on the password field (tl_member/tl_user) it might be the case that the omitted password is unhashed. If so, I need to hash the password. For that, I need to check if the password is already hashed :)

@Toflar
Copy link
Member Author
Toflar commented Jun 28, 2019

I don't understand that use case, sorry :) But maybe you could do this in PHP when using the password hashing API using password_get_info(). I don't think you can in Symfony though.

@richardhj
Copy link
Member

password_get_info() is totally fine! ☺️

@Toflar
Copy link
Member Author
Toflar commented Jun 28, 2019

Yeah so there's no way to do so as of today. Time for a SF PR if you need that ;)

@ausi
Copy link
Member
ausi commented Jun 30, 2019

How do I check if a string is already hashed?

Doing this is probably be a security issue because a password that looks like a hash must not be handled differently. What if my actual password starts with $2y$?

I think you have to design your code so that you know in advance if you are working with a hash or with a plaintext password.

richardhj added a commit to terminal42/contao-password-validation that referenced this pull request Jul 2, 2019
@richardhj
Follow-up of contao/contao#536.
0ac49fe
richardhj added a commit to terminal42/contao-password-validation that referenced this pull request Jul 2, 2019
@richardhj
Follow-up of contao/contao#536.
98b7682
richardhj added a commit to terminal42/contao-password-validation that referenced this pull request Jul 2, 2019
@richardhj
Follow-up of contao/contao#536.
e7562da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschempp aschempp aschempp approved these changes

@leofeyer leofeyer leofeyer approved these changes

@bytehead bytehead Awaiting requested review from bytehead

@ausi ausi Awaiting requested review from ausi

Assignees

@Toflar Toflar

Labels
feature
Projects
None yet
Milestone
4.8
Development

Successfully merging this pull request may close these issues.
5 participants
@Toflar @leofeyer @richardhj @ausi @aschempp
0