All, I have added the tests and ensured that everything seems pretty clean. I have opened a Pull Request for this item as I would like to get more eyes on it (especially since it touches crypt()). Please review the PR and comment away.
https://github.com/php/php-src/pull/191/files Once it looks good, I'll merge it in. I just wanted to get more eyes on the diff first... Thanks Anthony On Wed, Sep 12, 2012 at 10:02 AM, Anthony Ferrara <ircmax...@gmail.com>wrote: > Hello all, > > I've closed the vote and it's been accepted with a vote total of 19:0, > unanimous. I've moved the RFC into Accepted. > > I'm going to add the remaining tests, and then move it into master later. > > As for the PECL extension route, I'll work on splitting it into a PECl > extension for 5.3/5.4 at the same time. > > Thanks all, > > Anthony > > On Tue, Sep 11, 2012 at 7:58 AM, Anthony Ferrara <ircmax...@gmail.com>wrote: > >> Hannes, >> >> > First off, this has been discussed on the list for literally months. >>> Why >>> > wait until the day before voting can end before bringing this up? >>> >>> So commenting is strictly forbidden during votes? >> >> >> Not in the least. Just pointing out that this discussion could have been >> better if it was started much earlier (prior to the time and effort being >> put into it). Just pointing it out. There's nothing "wrong" (procedurally) >> about bringing it up now, just that it would have been nice to hear this >> point earlier in the discussion... >> >> >>> > Secondly, the main reason for not developing this as an extension is >>> that >>> > there's really no benefit to it. There are little to no performance >>> gains to >>> > be had by the C implementation. It can live quite as easily as a PHP >>> > library. >>> >>> The benefit is that it can be tested properly and bugs discovered and >>> ironed out first. >>> >> >> I'm not so sure about that. If it was widely adopted as an extension, >> sure. But I would predict a very limited adoption. Almost purely academic. >> The reason is that there's no advantage to doing it in C other than to >> provide a working API for those who don't understand how to build it (or >> realize the benefits thereto). Those are the same people who wouldn't be >> able to install a PECL extension (know-how or access). And projects who >> realize this is needed are likely already using one of the many libraries >> available. >> >> So I'm not sure the install rate would be anywhere high enough to work >> any significant issues out. Additionally, there are testing phases for the >> release that would hopefully catch any major issues prior to 5.5 going >> gold... >> >> >>> This is not the sort of thing you want to get security bug reports the >>> day after its released in core. >>> >> >> Sure. Which is why I've been going around looking for security reviews >> (posted to the crypt-dev list on openwall, and to >> security.stackexchange.com). >> >> >>> If your ego is big enough you can guarantee you have tested this >>> thoroughly and want it to become the recommended way.. You have to be >>> damn sure you don't fuck it up. >>> >> >> I want to say that this is really unfair. By saying it this way you've >> pushed me into a corner. I disagree that your suggestion would have a >> result of any significant gain in security. Yet I cannot disagree with you >> without coming across as an egotist now that you said it that way. Please >> try to keep things civil without setting up traps to try to prove your >> point. I don't appreciate this remark at all... >> >> >>> This is exactly the sort of thing that doesn't need to be developed in >>> the core tree, but can later be merged in once proven successful. >>> >> >> As indicated before, if that's the case, then it would never be merged. >> Because this would never be successful as a stand alone PECL module. The >> primary reason for adding it is so that people who don't know any better >> would be able to use a secure API. That is the antithesis of a PECL >> extension. >> >> >>> >> Especially considering the patch is unfinished. >>> > >>> > >>> > Aside from adding a few more tests, what's unfinished? If you're >>> referring >>> > to the line in the RFC, I just haven't updated it. The patch has been >>> worked >>> > on and is in a place where I'd be comfortable submitting it... >>> >>> The test suite seems very limited, and the code seems to be waiting >>> for more algorithms to be implemented. >>> >> >> Yes, the test suite needs to be expanded. I tested it additionally with >> the PHPUnit tests that I wrote for the PHP fallback, so it's been tested >> fairly well... I just need to expand the phpt test suite included in core... >> >> As far as more algorithms, right now there's no reason to add more >> algorithms. The whole point was to never add anything but the strongest >> algorithm available. Which today is bcrypt. But as time goes on, and better >> algorithms become available, the API is designed so that they can be >> implemented. But today, there's no other algorithm that should be >> implemented. (SCrypt is a candidate, but since it lacks crypt(3) bindings, >> and is still very young, it's inclusion isn't prudent at this point). >> >> There's a section to the RFC for adding new algorithms as time goes on: >> https://wiki.php.net/rfc/password_hash#updating_password_default >> >> Anthony >> > >
