-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Add Yescrypt support to crypt() and password API #16452
base: master
Are you sure you want to change the base?
Conversation
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.
Some remarks, didn't look at everything and especially not in detail.
ext/standard/yescrypt/sha256.c
Outdated
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.
We probably want to reuse the existing SHA-256 implementation (for SHA-NI support).
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'll try; it has some extra stuff here like PBKDF2 etc, but the main loop can invoke the existing code probably.
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.
Based on the license comment, this is the SHA-256 implementation from libcperciva, thus it's the same author as the SHA-NI implementation. The API should be compatible.
As for PBKDF2, PHP necessarily also contains an existing implementation of that, but I didn't check if the API matches.
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.
Did another pass, looking a bit more in detail. It might make sense to loop in Solar Designer for him to verify if this integration is any good. From what I have seen he was somewhat involved in PHP before.
ext/standard/tests/password/password_needs_rehash_yescrypt.phpt
Outdated
Show resolved
Hide resolved
--FILE-- | ||
<?php | ||
|
||
// TODO: what to do with \0 ??? |
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.
For visibility: TODO.
return NULL; | ||
} | ||
|
||
/* PHP change: code to parse the settings extracted from yescrypt_r */ |
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 something that would make sense to upstream?
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.
The question is if the split will be kept.
There is the weird limitation now that you can't pass NUL bytes into the yescrypt password_hash() implementation. This happens because under the hood this uses crypt(), and when system crypt() is used instead of PHP crypt() it cannot deal with the NUL byte. To be consistent with system crypt I implemented rejecting NUL bytes. However, if we always make use of bundled yescrypt instead of relaying to system crypt's yescrypt, then we no longer have the NUL byte limitation; and then we also don't need to split the yescrypt code in a common and config part.
This would need to be discussed on the ML though.
5ca25c2
to
1584a36
Compare
@nielsdos thank you for your work! This would be incredibly useful for control panel projects like HestiaCP and ISPConfig that for now have to hack their way around this. Is there any way I can help out get this in php-src? |
@rjd22 notice that yescript hash is available when using system libxcrypt
And for password_hashing, using the xpass extension (libxcrypt bindings) without having to bundle new algo in PHP. |
I really don't like more deps for standard extension. I think the whole crypt stuff should move to the separate extension. See #17093 for more context. |
Implements #12911
Might need an RFC to decide on some non-trivial details