Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Jake doesn't support wheel-only installation #72

Closed
matthewdeanmartin opened this issue Dec 4, 2021 · 12 comments · Fixed by #74 or #80
Closed

[BUG] Jake doesn't support wheel-only installation #72

matthewdeanmartin opened this issue Dec 4, 2021 · 12 comments · Fixed by #74 or #80
Assignees
Labels
bug Something isn't working

Comments

@matthewdeanmartin
Copy link

matthewdeanmartin commented Dec 4, 2021

Describe the bug
Python packages come in two sorts, sdist and wheels. Sdist runs on installation setup.py, which allows for running malicious code. Wheels do not run setup.py on install, they just unpack the code & a user would have to invoke the malicious code intentionally via import or the like.

Jake has a dependency on termcolor, which doesn't not have a wheel. https://pypi.org/project/termcolor/#files

To Reproduce

export PIP_ONLY_BINARY=:all:
pipenv install jake --skip-lock --verbose

Or
pip install jake --only-binary=:all:
(The flag names are misleading, because when the flag is active, it installs only the wheel version & will ignore sdist for the package and ALL dependencies, even if they are all pure python)

Expected behavior
Jake should install without having to run setup.py for it or any dependency. The audience of jake is enterprises who are taking supply chain risks serious, probably because they have something valuable to protect. If I were a malicious hacker, I'd target termcolor on pypi (just need to guess their password), upload a malicious sdist and then steal valuables from jake users when setup.py runs.

Screenshots

[pipenv.exceptions.InstallError]: Collecting jake==1.1.3
[pipenv.exceptions.InstallError]:   Using cached jake-1.1.3-py3-none-any.whl (25 kB)
[pipenv.exceptions.InstallError]: Collecting PyYAML<6.0.0,>=5.4.1
[pipenv.exceptions.InstallError]:   Using cached PyYAML-5.4.1-cp39-cp39-win_amd64.whl (213 kB)
[pipenv.exceptions.InstallError]: ERROR: Could not find a version that satisfies the requirement termcolor<2.0.0,>=1.1.0 (from jake) (from versions: none)
[pipenv.exceptions.InstallError]: ERROR: No matching distribution found for termcolor<2.0.0,>=1.1.0
ERROR: Couldn't install package: jake
 Package installation failed...

I would recommend vendorizing your entire dependency chain (or at least the wheel-less one), but that is just because I'm paranoid about supply chain risks.

@matthewdeanmartin matthewdeanmartin added the bug Something isn't working label Dec 4, 2021
@matthewdeanmartin
Copy link
Author

You should still vendorize that 2 function lib, but in the meanwhile, I've forked it (the maintainer has been gone for 10 years) and published a version with wheels, here: https://pypi.org/project/termcolor-whl/

@matthewdeanmartin
Copy link
Author

And the other dependency with no wheels is terminaltables a project that has been idle for 5 years and the author has archived the github repo. I'll fork it and publish a wheel for it shortly, but again, y'all should just vendorize that.

@matthewdeanmartin
Copy link
Author

And terminaltables-whl is now up, but again, y'all should vendorize it, or at least switch to the whl versions.
Thanks.

@madpah
Copy link
Collaborator

madpah commented Dec 6, 2021

Hi @matthewdeanmartin - thanks for the report. A very important report indeed.

We'll get this worked on to remove this dependency!

Thanks for your input.

@madpah madpah self-assigned this Dec 6, 2021
@matthewdeanmartin
Copy link
Author

I got ahold of the previous maintainer of terminaltables and we got that one updated with a wheel dist - https://pypi.org/project/terminaltables/#files

One down, one to go.

@madpah
Copy link
Collaborator

madpah commented Dec 7, 2021

Thanks @matthewdeanmartin - to get a quick fix for this, we'll take the termcolor-whl and the updated terminaltables packages.

After this, we'll assess:

  • Their use in jake; and
  • If their use is pertinent, consider vendorising them

madpah added a commit that referenced this issue Dec 7, 2021
Signed-off-by: Paul Horton <phorton@sonatype.com>
@madpah madpah closed this as completed in #74 Dec 7, 2021
madpah added a commit that referenced this issue Dec 7, 2021
…installation

fix: ensure dependencies can be installed from binary packages #72
@madpah
Copy link
Collaborator

madpah commented Dec 7, 2021

@matthewdeanmartin - jake 1.1.4 has been published - can you confirm this resolves the immediate issue you reported?

Thanks

@matthewdeanmartin
Copy link
Author

I'll skip the analysis, but here is the output of pipenv graph

  - termcolor-whl [required: >=1.1.2,<2.0.0, installed: 1.1.2]
  - yaspin [required: >=2.1.0,<3.0.0, installed: 2.1.0]
    - termcolor [required: >=1.1.0,<2.0.0, installed: 1.1.0]

yaspin is pulling in the non-wheel termcolor. I thought you had a direct dependency on termcolor, but because it is transitive that means you'd need to vendorize yaspin.

The maintainer of termcolor is just gone, so that blocks some more elegant solutions.

I'm sorry I didn't notice that earlier, so you can drop the dep on termcolor-whl-- it doesn't help anyhow-- and I recommend vendorizing yaspin (or convincing yaspin to stop using termcolor). Yes vendorizing has some challenges of its own.

@bollwyvl
Copy link

bollwyvl commented Dec 7, 2021

As a downstream packager of jake on conda-forge, we generally have to hustle rather hard to keep track of vendored code once introduced for license/distribution compliance purposes... sometimes thornier than "pure" technical concerns! So i guess i would just request that if the vendoring path is taken, please include the LICENSE files of the upstreams.

Perhaps having the core install_requires cover the most boring cases (json, stdlib terminal) wherever possible, and offering the pretties as extras (or even separate packages linked with entry_points) would provide more flexibility and robustness. Selfishly, this jake-core would meet my need, as I generally use it as part of longer an analysis pipeline, so the more machine-readable, the better!

As for the concrete problem of terminal pretties: rich is quite good, and has an excellent table. a quick pip wheel --only-binary :all: rich reveals only wheel dependencies:

Collecting rich
  Using cached rich-10.15.2-py3-none-any.whl (214 kB)
Collecting commonmark<0.10.0,>=0.9.0
  Using cached commonmark-0.9.1-py2.py3-none-any.whl (51 kB)
Collecting colorama<0.5.0,>=0.4.0
  Using cached colorama-0.4.4-py2.py3-none-any.whl (16 kB)
Collecting pygments<3.0.0,>=2.6.0
  Using cached Pygments-2.10.0-py3-none-any.whl (1.0 MB)
Saved ./rich-10.15.2-py3-none-any.whl
Saved ./colorama-0.4.4-py2.py3-none-any.whl
Saved ./commonmark-0.9.1-py2.py3-none-any.whl
Saved ./Pygments-2.10.0-py3-none-any.whl

@madpah madpah reopened this Dec 8, 2021
@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

Thanks for the suggestion @bollwyvl of rich - sounds like it's worth us moving to using this in favour of both yaspin and terminaltables.

@madpah
Copy link
Collaborator

madpah commented Dec 9, 2021

See #73

@dvzrv
Copy link

dvzrv commented Aug 8, 2022

I would like to point out, that from a security point of view it does not make a difference whether a call to setup.py (during build or install) or an include of files from a wheel compromises your system. The outcome is the same: Your system is compromised because a Python upstream project has been compromised.

The solution can not be to create a fork of all projects out there, but to actually become the new upstream and improve its security workflow. @matthewdeanmartin it would be great if you could look into becoming the termcolor maintainer (also on pypi.org), if it is indeed unmaintained and replace its sources with your new project.
Forking the project just to create a wheel is problematic for downstreams (e.g. Arch Linux in my case), as it means that we now have to package two projects that ship the same package and conflict with one another.
Additionally, please note, that we rely on sdist tarballs (or the upstream project's release tarballs), as we build projects from them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants