search

tuupola / slim-jwt-auth

PSR-7 and PSR-15 JWT Authentication Middleware
https://appelsiini.net/projects/slim-jwt-auth
MIT License
821 stars 140 forks source link

chore: support firebase/php-jwt v6 #217

Open esetnik opened 2 years ago

esetnik commented 2 years ago

We should support the latest version of firebase/php-jwt which is v6

tuupola commented 2 years ago

It seems firebase/php-jwt:6.0.0 is not a throw in replacement. It is also possible that it requires dropping support for older versions since handling of keys is quite different. Will investigate bit more.

JimTools commented 2 years ago

This feature request becomes more important with the recent discovery of CVE-2021-46743 so it may be wise to drop support for previous versions but that would be a breaking change.

This is a non-trivial rework of how multiple algorithms are handled, along with changes in behaviour as it now throws more exceptions.

tuupola commented 2 years ago

CVE-2021-46743 is not a vulnerability per se but a footgun if user against all advice enables both RS256 and HS256 in the config.

JimTools commented 2 years ago

I had to look up what a footgun is 😆

I have taken a stab at upgrading to the recommended way. (branch)

but this isn't perfect.

zluiten commented 2 years ago

Would dropping support for older versions be a BC break? Composer will just prevent the consumer from upgrading when using non supported versions of firebase/php-jwt (assuming composer is the supported way of installing this package).

How much are older versions of firebase/php-jwt used anyway?

tuupola commented 2 years ago

No, dropping support for old versions of dependencies is not a BC break. It seem currently 5.x has most downloads, but 6.x is rising too.

https://packagist.org/packages/firebase/php-jwt/stats

townxelliot commented 2 years ago

It would be great to see some movement on this. While I understand that the bug is avoidable through configuration, the presence of this package in a composer.json file within a Docker image will cause CVE alerts if using scanning tools (e.g. docker scan/AWS ECR scanning). Are there any plans to apply the patch given above (https://github.com/tuupola/slim-jwt-auth/issues/217#issuecomment-1094250900) or similar to the project? I'd be happy to have a look at providing a patch myself if the team don't have capacity to deal with it right now, but don't want to duplicate any ongoing work. Thanks.

tuupola commented 2 years ago

One of my pet peeves is CVE scanners which blindly check for version numbers but not if code is actually vulnerable. That said it is a good idea to upgrade the firebase/php-jwt and silent CVE scanners are preferred.

Currently for me paid work comes first so I have not been hurrying this too much. Changes in https://github.com/tuupola/slim-jwt-auth/issues/217#issuecomment-1094250900 do look good, but I am not sure if it breaks BC because tests were changed. I will test that soon.

townxelliot commented 2 years ago

Agreed on blind scanning, but unfortunately it's ingrained in our culture here. I'm not sure I can get them turned off :) I appreciate you have plenty of other pressures, and am grateful for the swift response. In the meantime, I've suggested using a stripped down forked version of your code which works in our situation. I'll see what the rest of the team think. As soon as your very useful library is upgraded, we'll switch back to that and ditch our work-around. Thanks again.

tuupola commented 2 years ago

I know. Just last week I had to spend considerable time explaining a corporate that CentOS 7 servers are not vulnerable even when their CVE scanner warns about "vulnerable" versions of OpenSSH. To maintain stability server oriented distros patch their software instead of upgrading.

In any case I do not think it is possible to upgrade to latest firebase/php-jwt without breaking BC. This is because it now requires knowing beforehand the algorithm used with each secret. This means you cannot configure the middleware anymore like this:

$app->add(new Tuupola\Middleware\JwtAuthentication([
    "secret" => "supersecretkeyyoushouldnotcommittogithub"
]))
$app->add(new Tuupola\Middleware\JwtAuthentication([
    "secret" => [
        "acme" =>"supersecretkeyyoushouldnotcommittogithub",
        "beta" =>"anothersecretkeyfornevertocommittogithub"
     ]
]))

Or you could if you hardcode a default algrorithm like I did in a proof of concept here https://github.com/tuupola/slim-jwt-auth/pull/223. This will however break BC for anyone who was not using the default algorithm. So I probably bump the major version number and do some other BC breaking changes that have been in my todo list.

JimTools commented 2 years ago

It's slightly challenging to do this upgrade, but I'm more than willing to contribute in anyway I can.

mbolli commented 1 year ago

I just want to give a +1 on bumping the major version number to break BC there:

I would gladly help to do this.

josefsabl commented 7 months ago

More and more of my dependencies require firebase/php-jwt: ^6.0 and this library is becoming an obstacle, I am afraid I will have to replace it by something else. Any chance it will support 6.0 anytime soon?

Thank you!

esetnik commented 7 months ago

@josefsabl I think https://github.com/tuupola/slim-jwt-auth/pull/246 needs to be merged and a new major release issued.

egalley commented 3 months ago

Hello everybody,

Is there a way to fix this vulnerability issue : new release or new library?

Thank you

JimTools commented 3 months ago

@egalley I’ve forked and released the library but it’s not an ideal solution as in includes breaking changes. And with the recent events of XZ any maintainer is going to be highly suspicious of adding someone with write privileges

fork