paketo-buildpacks / cpython

Apache License 2.0
10 stars 16 forks source link

Optionally remove setuptools using BP_CPYTHON_RM_SETUPTOOLS env var #705

Closed jericop closed 6 months ago

jericop commented 6 months ago

Summary

This change addresses https://github.com/paketo-buildpacks/cpython/issues/579 and also cleans up broken pip executables in the cpython layer. The main issue is that the current pre-compiled installations of python include the setuptools package which has a known vulnerability. This change removes that package.

More reasons why this is helpful:

Current workaround

When upgrading is not an option, my current workaround is to add a project.toml file with an inline build pack to remove the offending setuptools package. In the words of a fellow engineer, "YUCK." This is the last thing I want to do or encourage users to do.

Use Cases

The issue has a lot of good information in it and rightly encourages users to upgrade to a new version of python. Unfortunately this is not always an option, so this change adds a way to remove packages from the python site-packages folder in the layer as needed. It also checks for and removes broken pip executables from the installation if there are any.

The PipCleanup function will do nothing if setuptools is not installed. It will also do nothing if there are no broken pip executables.

Checklist

jericop commented 6 months ago

@arjun024 Thanks for the feedback. I did read through the issue prior to creating this PR and wrote it in such a way to be in line with @robdimsdale comment.

The main driver for this is to address CVEs that cannot be fixed by changing dependencies in requirement.txt files. The only way to get around this is issue is to modify the cpython layer with an inline buildpack (at build time) or to modify the container after build. Both are not good solutions.

I will comment on his main points.

I agree that generally buildpacks shouldn't address CVEs, but this is a case where the CVE can't be addressed without modifying the buildpack or by modifying the image created with the buildpack. No changes to requirements.txt will prevent the CVE if you are using older versions of python.

arjun024 commented 6 months ago
brayanhenao commented 6 months ago

As @arjun024 mentioned previously, I don't think the default behavior of this Buildpack is to have no setuptools. This could affect many users who use this Buildpack and force them to create a requirements.txt to add this package.

It is out of the responsibility of the Buildpack to remove this package. I think the best way to implement this feature is to use some environment variables such as BP_CPYTHON_RM_SETUPTOOLS as an alternate behavior that does not affect the default one.

The motivation for this change is clear but I feel that the expected impact outweighs the reasons to leave it as default behavior and not as an alternative.

jericop commented 6 months ago

Thanks for the comment @brayanhenao. In the interest of moving forward I think the environment opt-in approach is fine and I will update the code to only opt in when BP_CPYTHON_RM_SETUPTOOLS has been been set.

Here is the behavior I plan to implement:

Any thoughts or preference on the environment variable approach?

jericop commented 6 months ago

I have gone ahead and implemented BP_CPYTHON_RM_SETUPTOOLS as proposed above.

jericop commented 6 months ago

As requested, I have removed the logic to cleanup broken pip executables and updated all unit tests to verify the changes. I have also added an integration test to verify that python works as expected when BP_CPYTHON_RM_SETUPTOOLS is set and setuptools is removed. This will confirm that the change works for all support dependencies versions and builders.

After giving it some thought and reading through some online discussions about how some packages can fall back to using setuptools when installing packages, I understand the hesitation to remove it altogether as I had originally proposed. Although I haven't encountered that myself, I can't say that I have built so many unique python containers to be sure it wouldn't be a problem. Thanks again for your feedback and patience.

jericop commented 6 months ago

LGTM. Thanks for the changes @jericop. I'll merge this now and make a release soon. Could you please as a follow up, document the env var & the rationale in the README and the paketo-website repo?

Apologies for forgetting to update the README. The release can wait for the readme change, which I will do later today. Thank you!