search

italia / spid-cie-oidc-django

The SPID/CIE OIDC Federation SDK, written in Python
Apache License 2.0
27 stars 28 forks source link

Reason behind upper version bounds? #298

Closed sallner closed 9 months ago

sallner commented 9 months ago

Hello together,

I use this project in an application and I am very happy that this exists, so thanks for the work. Unfortunately, I just realised today, that there are some upper bound version constraints in the setup.py and no clear explanation to that or corresponding issues to remove those version constraints.

In particular, this hits me with the current CVEs of aiohttp (CVE-2023-49081 and CVE-2023-49082 ) which have been found by dependabot, but I cannot install them, because of the aforementioned constraints in this package. The same, less critical is the upper constraint for Django, which was recently introduced (#291) without further explanation.

I want to point you to read this article on upper bound version constraints and think about, whether it is possible to release those upper version constraints, as I see this package mainly as a library, which should not dictate the use of future releases of a peer-dependency.

peppelinux commented 9 months ago

there might be some cases where a major release introduces breaking changes

we assume that LTS release are used, where bug and security patches are resolved as a patch or minor release

which changes do you suggest in the dependencies?

sallner commented 9 months ago

there might be some cases where a major release introduces breaking changes

I often hear this argument, but as currently with aiohttp == 3.8 containing 2 CVE with one rated high, this is also valid for security changes. We even pinned it to a minor release here, so the fixes in aiohttp == 3.9 cannot be applied without a release of this spid-cie-oidc-django package. Having upper bound version constraints (without an actual reason is the code) for a library basically prevents developer, who use the library, to live in the future. This does not prevent breaking changes (as the spid-cie-oidc-django just introduced a breaking change for not supporting Django 5 in a patch version (1.2.2) even without mentioning it in the change log. I do not want to point fingers as this also happened to me, I am just stating reality.

we assume that LTS release are used, where bug and security patches are resolved as a patch or minor release

If there really is a reason, why a library is not compatible with another framework it makes sense to introduce an upper bound version constraint in order to highlight the need for a fix. If I know, what is missing for spid-cie-oidc-django to support Django 5 I would be able to help, work on it and file an PR. But as it was just stated, that there is no support, it is hard for me to estimate if the library is future proof.

which changes do you suggest in the dependencies?

Thanks for asking that. I would suggest to remove the upper bound version constraints in this setup.py of this library. This library will not be installed directly on its own in a pip environment. If it is installed in a production release, it will be necessary and is good practise to use version pinning of all dependencies to ensure a reliable installation. But this is in the hand and responsibility of the developer or devops using this library. It might be helpful for the full integration project to suggest a set of known good versions in a requirements.txt or constraints.txt.

If there is a known incompatibility there would be preferably a comment and/or link to an issue here, which explains the reason of the version constraint and the things which have to be done to resolve this problem. This way, we can split the work and ensure that the package can be used in the future and for immediate security updates.

I really suggest reading the article as the reasons and details are explained more in detail then I did it here. It actually changed my point of view on this topic.

Thanks for reading so long, I would really appreciate, if you could follow my suggestion. At least now for the next release, a removal of aiohttp < 3.9 is necessary to have the security fixes installed.

peppelinux commented 9 months ago

I agree @sallner

could you please provide a PR to fix this dependency misconfiguration?

I really appreciate your contribution

peppelinux commented 9 months ago

@sallner see https://github.com/italia/spid-cie-oidc-django/pull/302

sallner commented 9 months ago

Thank you I was still on holiday, so I could not add come to this earlier.