ComplianceAsCode / auditree-arboretum

The Auditree common fetchers, checks and harvest reports library.
https://auditree.github.io/
Apache License 2.0
17 stars 10 forks source link

PyYaml version needs an update #68

Closed markuszoeller closed 1 year ago

markuszoeller commented 1 year ago

Overview

Dependency PyYaml is declared as pyyaml<5.4 and the latest version before that upper bound is over 3y old: https://pypi.org/project/PyYAML/5.3.1/ That version also has vulnerabilities https://github.com/advisories/GHSA-6757-jp84-gxfx which got only fixed with 5.4.1, see commit message in https://github.com/kubernetes-client/python/commit/cd1507671cab33d2b467064449497c5438212a4b

In general I was wondering why that dependency has an upper bound while all others use a lower bound. Was the PR review suggestion at https://github.com/ComplianceAsCode/auditree-arboretum/pull/54#discussion_r622274837 maybe a typo and it should have been pyyaml>5.4? @alfinkel @cletomartin

Requirements

N/A

Approach

N/A

Security and Privacy

N/A

Test Plan

N/A

jhart1685 commented 1 year ago

To echo @markuszoeller 's comments, it's not clear to me why arboretum needs pyyaml<5.4

Not only is 5.3.1 old and vulnerable, it causes dependency problems for any projects that consume arboretum and require a newer version of pyyaml. Moreover, it looks like the pyyaml developers are no longer backporting fixes to the 5.x release. For example, versions 5.4 & 5.4.1 don't work with Python 3.10 and above (due to a fix around cython sources not getting backported).

Ideally, I think that explicit version dependency should be removed, allowing it to pick up the latest (currently 6.0.1). @alfinkel @cletomartin - will you accept a PR for that change? Are successful unit tests enough evidence that it does not cause a regression?

alfinkel commented 1 year ago

I don't think https://github.com/ComplianceAsCode/auditree-arboretum/pull/54#discussion_r622274837 was a typo but it was 2.5 years ago when I made the comment. There may have been an issue with syntactical usage around the package and the code but again, I can't say definitively as it was quite a while ago.

Also, I'm not sure that passing unit tests is an indicator that all would be OK as I don't think we had optimal unit test coverage (at least in the past).

But I'll defer to @cletomartin since he's probably more up to date with auditree/arboretum these days.

jhart1685 commented 1 year ago

Ok - thanks @alfinkel .. I might be missing something here, but I can't even see where or why that original PR required pyyaml. Certainly it's not used directly anywhere, and there's no obvious new indirect dependency added via that PR.

A pip inspect (or pip show pyyaml) in arboretum shows pyyaml being indirectly needed only by bandit, pre-commit and markdown-it-py.

markuszoeller commented 1 year ago

Certainly it's not used directly anywhere

It's used here: https://github.com/ComplianceAsCode/auditree-arboretum/blob/7ac86cd9b9a17ff687ccc74cce2e6b94eb9f57f4/arboretum/ibm_cloud/fetchers/fetch_cluster_resource.py#L111 That's the only place I could find and I would be surprised if that doesn't work anymore with a newer version.

jhart1685 commented 1 year ago

Ah - thanks for putting me straight @markuszoeller !

cletomartin commented 1 year ago

Thanks @markuszoeller! I don't think there is a good reason for keeping that restriction. I vaguely remember we had a problem with that specific version of pyyaml long time ago so this could be the case that we forgot to release the restriction when the issue was fixed.

I have created #69 for fixing this.

cletomartin commented 1 year ago

@markuszoeller I'm publishing now the new version.

markuszoeller commented 1 year ago

@markuszoeller I'm publishing now the new version.

@cletomartin JFYI, I don't see it yet on PyPi, so I cannot consume it yet. Could you take a look at the release please?

cletomartin commented 1 year ago

@markuszoeller it should be available now https://pypi.org/project/auditree-arboretum/.