Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

chore: support firebase/php-jwt v6 #217

Open
esetnik opened this issue Feb 18, 2022 · 16 comments
Open

chore: support firebase/php-jwt v6 #217

esetnik opened this issue Feb 18, 2022 · 16 comments
Assignees

Comments

@esetnik
Copy link

esetnik commented Feb 18, 2022

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

@tuupola tuupola self-assigned this Feb 18, 2022
@tuupola
Copy link
Owner

tuupola commented Feb 18, 2022

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
Copy link
Contributor

JimTools commented Apr 6, 2022

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
Copy link
Owner

tuupola commented Apr 8, 2022

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
Copy link
Contributor

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
Copy link

zluiten commented May 4, 2022

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
Copy link
Owner

tuupola commented May 4, 2022

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
Copy link

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 (#217 (comment)) 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.

townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue May 19, 2022
slim-jwt-auth currently does not support firebase/php-jwt 6.
This raise critical security alerts when our containers are scanned
(namely CVE-2021-46743).

There is an issue on the slim-jwt-auth repo for this, which I
have commented on, asking for further info:

tuupola/slim-jwt-auth#217

In the meantime, I have taken a copy of the key part of that
package and reworked it slightly so that it functions with
php-jwt v6. I also removed a lot of the options we're not
using.

The existing middleware which functions with slim-jwt-auth is
commented out but still present. To reinstate that package,
we just need to remove this commit.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue May 19, 2022
slim-jwt-auth currently does not support firebase/php-jwt 6.
This raise critical security alerts when our containers are scanned
(namely CVE-2021-46743).

There is an issue on the slim-jwt-auth repo for this, which I
have commented on, asking for further info:

tuupola/slim-jwt-auth#217

In the meantime, I have taken a copy of the key part of that
package and reworked it slightly so that it functions with
php-jwt v6. I also removed a lot of the options we're not
using.

The existing middleware which functions with slim-jwt-auth is
commented out but still present. To reinstate that package,
we just need to remove this commit.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue May 19, 2022
slim-jwt-auth currently does not support firebase/php-jwt 6.
This raise critical security alerts when our containers are scanned
(namely CVE-2021-46743).

There is an issue on the slim-jwt-auth repo for this, which I
have commented on, asking for further info:

tuupola/slim-jwt-auth#217

In the meantime, I have taken a copy of the key part of that
package and reworked it slightly so that it functions with
php-jwt v6. I also removed a lot of the options we're not
using.

The existing middleware which functions with slim-jwt-auth is
commented out but still present. To reinstate that package,
we just need to remove this commit.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue May 19, 2022
slim-jwt-auth currently does not support firebase/php-jwt 6.
This raise critical security alerts when our containers are scanned
(namely CVE-2021-46743).

There is an issue on the slim-jwt-auth repo for this, which I
have commented on, asking for further info:

tuupola/slim-jwt-auth#217

In the meantime, I have taken a copy of the key part of that
package and reworked it slightly so that it functions with
php-jwt v6. I also removed a lot of the options we're not
using.

The existing middleware which functions with slim-jwt-auth is
commented out but still present. To reinstate that package,
we just need to remove this commit.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue May 19, 2022
slim-jwt-auth currently does not support firebase/php-jwt 6.
This raise critical security alerts when our containers are scanned
(namely CVE-2021-46743).

There is an issue on the slim-jwt-auth repo for this, which I
have commented on, asking for further info:

tuupola/slim-jwt-auth#217

In the meantime, I have taken a copy of the key part of that
package and reworked it slightly so that it functions with
php-jwt v6. I also removed a lot of the options we're not
using.

The existing middleware which functions with slim-jwt-auth is
commented out but still present. To reinstate that package,
we just need to remove this commit.
@tuupola
Copy link
Owner

tuupola commented May 19, 2022

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 #217 (comment) do look good, but I am not sure if it breaks BC because tests were changed. I will test that soon.

@townxelliot
Copy link

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
Copy link
Owner

tuupola commented May 20, 2022

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 #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
Copy link
Contributor

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

townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue May 23, 2022
* Remove unnecessary files from service-front docker image

* node_modules and the package.* files are only used for building
the JS, so remove them
* Remove any test files
* Remove any build-enabling files

This has the benefits of making the image smaller, at the same
time as improving security and reducing bogus security alerts
(e.g. alerts about nodejs libraries which don't have any effect
at runtime).

* Remove unnecessary files from service-api docker image

* Remove build-related and documentation-related files
* Remove test cases
* Exclude scripts which aren't used at runtime

This removes potential security issues caused by unused
components.

We keep the composer.lock as security scans use this to
decide whether there are vulnerabilities in our images.

* Update to latest Notify client

This upgrades the firebase/php-jwt library to a secure version.

* Remove unnecessary files from service-admin

* Tests must be in image for unit testing in CI

* Replace tuupola/slim-jwt-auth with our fork

slim-jwt-auth currently does not support firebase/php-jwt 6.
This raise critical security alerts when our containers are scanned
(namely CVE-2021-46743).

There is an issue on the slim-jwt-auth repo for this, which I
have commented on, asking for further info:

tuupola/slim-jwt-auth#217

In the meantime, I have taken a copy of the key part of that
package and reworked it slightly so that it functions with
php-jwt v6. I also removed a lot of the options we're not
using.

The existing middleware which functions with slim-jwt-auth is
commented out but still present. To reinstate that package,
we just need to remove this commit.

Co-authored-by: William Falconer <[email protected]>
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue Jun 8, 2022
Once the issue with slim-jwt-auth is fixed
(see tuupola/slim-jwt-auth#217),
we can reinstate that library as the JWT solution as follows:

* Add tuupola/slim-jwt-auth back into composer.json
* Remove tuupola/http-factory and tuupola/callable-handler
  (these are dependencies of tuupola/slim-jwt-auth
  and will be installed by that library; we no longer need to reference
  them directly once slim-jwt-auth is back in place)
* Uncomment body of App\Middleware\Session\JwtAuthenticationFactory class
* Remove App\Middleware\Session\JwtMiddleware file
* Remove App\Middleware\Session\JwtMiddlewareFactory file
* Uncomment lines 67-68 and remove lines 69-70 of ConfigProvider.php

For more context, see commit 3efaa3b in this repo.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue Jun 10, 2022
Once the issue with slim-jwt-auth is fixed
(see tuupola/slim-jwt-auth#217),
we can reinstate that library as the JWT solution as follows:

* Add tuupola/slim-jwt-auth back into composer.json
* Remove tuupola/http-factory and tuupola/callable-handler
  (these are dependencies of tuupola/slim-jwt-auth
  and will be installed by that library; we no longer need to reference
  them directly once slim-jwt-auth is back in place)
* Uncomment body of App\Middleware\Session\JwtAuthenticationFactory class
* Remove App\Middleware\Session\JwtMiddleware file
* Remove App\Middleware\Session\JwtMiddlewareFactory file
* Uncomment lines 67-68 and remove lines 69-70 of ConfigProvider.php

For more context, see commit 3efaa3b in this repo.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue Jun 13, 2022
Once the issue with slim-jwt-auth is fixed
(see tuupola/slim-jwt-auth#217),
we can reinstate that library as the JWT solution as follows:

* Add tuupola/slim-jwt-auth back into composer.json
* Remove tuupola/http-factory and tuupola/callable-handler
  (these are dependencies of tuupola/slim-jwt-auth
  and will be installed by that library; we no longer need to reference
  them directly once slim-jwt-auth is back in place)
* Uncomment body of App\Middleware\Session\JwtAuthenticationFactory class
* Remove App\Middleware\Session\JwtMiddleware file
* Remove App\Middleware\Session\JwtMiddlewareFactory file
* Uncomment lines 67-68 and remove lines 69-70 of ConfigProvider.php

For more context, see commit 3efaa3b in this repo.
townxelliot added a commit to ministryofjustice/opg-lpa that referenced this issue Jun 14, 2022
…965)

* Add psalm as service-admin dev dependency

* Update service-admin psalm config to match other components

* First pass of level 4 psalm check

* Implement simplified LoggerTrait

The MakeLogger LoggerTrait is designed to work with laminas-mvc
apps and will not work correctly in service-admin (which is
not a laminas-mvc app).

Implement a simple alternative logger which can be used within
service-admin as a drop-in replacement for MakeLogger.

As we are not so concerned about logging trace IDs here, we can
resort to logging errors directly and not as JSON. If this is
unacceptable, we will have to consider rewriting the MakeLogger
code so we can output JSON in non-laminas-mvc apps.

The code in this commit improves things, as at the moment I would
expect logging to fail completely for service-admin due to
missing laminas-mvc dependencies.

* Second pass of pass of level 4 psalm check

* Comment out currently-unused class

Once the issue with slim-jwt-auth is fixed
(see tuupola/slim-jwt-auth#217),
we can reinstate that library as the JWT solution as follows:

* Add tuupola/slim-jwt-auth back into composer.json
* Remove tuupola/http-factory and tuupola/callable-handler
  (these are dependencies of tuupola/slim-jwt-auth
  and will be installed by that library; we no longer need to reference
  them directly once slim-jwt-auth is back in place)
* Uncomment body of App\Middleware\Session\JwtAuthenticationFactory class
* Remove App\Middleware\Session\JwtMiddleware file
* Remove App\Middleware\Session\JwtMiddlewareFactory file
* Uncomment lines 67-68 and remove lines 69-70 of ConfigProvider.php

For more context, see commit 3efaa3b in this repo.

* Update to PHP 8.1 in composer config

* Set a default form element value of '' if it is null

* Upgrade to PHP 8.1 in service-admin docker container

* Update to version 14 of opg-lpa-datamodels

* psalm scan service-admin with PHP 8.1

* Update composer dependencies

* Upgrade front-app docker image to PHP 8.1

* Passing null to DateTime is deprecated

When we get the last login time for a user, we set this to null
if they have not logged in before. This value is then passed
to new DateTime($lastLogin). However, PHP 8.1 is not happy with
passing a null to the DateTime constructor and throws a deprecation
message. Default this value to 'now' if the user has never logged
in before to fix.

* Move simplified LoggerTrait into MakeLogger shared directory
contactjavas added a commit to usefulteam/slim-jwt-auth that referenced this issue Sep 28, 2022
> This was a fork of [tuupola/slim-jwt-auth](https://github.com/tuupola/slim-jwt-auth) by [Mika Tuupola](https://github.com/tuupola). The fork was taken from `3.x` branch (at [this state](https://github.com/tuupola/slim-jwt-auth/tree/a4d6b3857daccb393f885473a08b2ea25874ae6b)).
>
> Thanks to Mika Tuupola & the package's contributors for their hard work.
>
> We forked it because we wanted to use [firebase/php-jwt](https://github.com/firebase/php-jwt) version 6 which had not supported at that time. Related [issue](tuupola/slim-jwt-auth#217).
@mbolli
Copy link

mbolli commented Feb 21, 2023

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

  • The ^3.0|^4.0|^5.0 version range is not updated anymore by firebase (as in: since v6 came out)
  • Would be a good opportunity for PHP 8.0 or 8.1 as minimum version

I would gladly help to do this.

@josefsabl
Copy link

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
Copy link
Author

esetnik commented Dec 15, 2023

@josefsabl I think #246 needs to be merged and a new major release issued.

@egalley
Copy link

egalley commented Apr 8, 2024

Hello everybody,

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

Thank you

@JimTools
Copy link
Contributor

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants