search

Yubico / php-u2flib-server

(OBSOLETE) U2F library in PHP
https://developers.yubico.com/php-u2flib-server/
BSD 2-Clause "Simplified" License
289 stars 68 forks source link

Code cleanup, PSR-2/4, security fixes, and PHP 7 only support #70

Closed Zae closed 2 years ago

Zae commented 6 years ago

Hiya,

I'm not sure if you accept merge requests, but I noticed the code of this library was getting very stale, so I had some fun with the code, I changed a lot of stuff.

This continues on the work I did in #69 but in this version I dropped all support for PHP < 7

PHP 7 fixes

klali commented 6 years ago

Hello,

Do you consider #69 or #70 to be the one that should stay open?

I consider this library to be in maintainance mode given webauthn happening and won't be merging style and structure changes.

With that said some of this might still qualify (or I can be persuaded) but I'd like to see smaller pull requests that are reviewable and easier to talk about.

Zae commented 6 years ago

Hi @klali ,

That really depends, do you want to keep PHP 5 (#69) support or do you want to use the new (strict) typing features of PHP 7.

Also which non style/structure-features would you like to keep, I could make a new PR with only those changes.

klali commented 6 years ago

Ah. I believe we should have this support php 5 while that is supported as a platform (which I read to be 2018-12-31).

Zae commented 6 years ago

Hey @klali ,

That doesn't really leave a lot then, especially if this lib is basically deprecated now.

Just wondering, does yubico have a library like this for the u2f part of webauthn?

emlun commented 6 years ago

Not for PHP at this time, but we do have them for Java, Python and C. We're considering making one for PHP, but we haven't planned or commited to it.

klali commented 6 years ago

I wouldn't say that this lib is deprecated (yet), I do however expect u2f to be deprecated by the browsers as the webauthn support stabilizes.

While I did state my thoughts on what type of changes to make here I'm more than happy to reason about what makes sense.

Zae commented 6 years ago

Hi @klali ,

My reasons for the changes are this:

psr-2/psr-4, big part of the community is switching or already using the psr-2 rules and a psr-4 compatible autoloader. I think this really adds to readability which is especially important in a security oriented library. Same for the array() vs [] change, I think it adds readability and consistency with other languages such as javascript. Same with the const, I think it adds readability and make the code somewhat easier to change later on, if at all possible to change the hashing algo.

namespacing the tests makes it more part of the codebase itself I think, also it makes it possible to use little tricks where you override built-in functions from php to make mock versions if needed.

Constant Time Encoding I added because I'm a bit paranoid about timing attacks lately :P, but I don't really know if it's really needed in this usecase. Maybe @paragonie-scott (does this tag even work?) knows if it's really necessary here?

php 7 change, php 5 is basically already deprecated, the dec 2018 date is for security fixes only. Also you are not really blocking php 5 users from using this library if you merge the php 7 only stuff. You could tag a 2.0.0 version with the php7 only stuff and php5 users will use the 1.0.1 version. I think the stricter types are very important for a security oriented library.

Other than that, I could create a rebased version with only the const / array and test namespace changes, but it feels a bit pointless tbh.

Spomky commented 6 years ago

Hi all,

I fully agree with @Zae. As this topic never moved forward (e.g. https://github.com/Yubico/php-u2flib-server/pull/49 in May 2016), I started to work on my own modern PHP implementation of this standard (and webauthn by the way).

emlun commented 2 years ago

Thank you for your contributions, and sorry this never went anywhere. Since the U2F API is now obsolete, we're archiving this repository and ceasing maintenance.