Skip to content

bpo-46119: Add hash test for ensurepip packages #30393

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.

Sign up for GitHub

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

Closed
kumaraditya303 wants to merge 1 commit into from
Closed

bpo-46119: Add hash test for ensurepip packages #30393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Lib/test/support/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,8 +548,7 @@ def open_urlresource(url, *args, **kw):

check = kw.pop('check', None)

filename = urllib.parse.urlparse(url)[2].split('/')[-1] # '/': it's URL!

filename = urllib.parse.urlparse(url).path.replace('/', '_') # '/': it's URL!
fn = os.path.join(TEST_DATA_DIR, filename)

def check_valid_file(fn):
Expand Down
25 changes: 23 additions & 2 deletions Lib/test/test_ensurepip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,33 @@
import test.support
import unittest
import unittest.mock

import json
import ensurepip
import ensurepip._uninstall

import hashlib

class TestPackages(unittest.TestCase):

def test_packages_hash(self):
Copy link
Copy Markdown
Member

@pablogsal pablogsal Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is possible that this test is executed from an environment where it cannot reach the internet so it should be marked as using the network (with the @requires_resource('network') decorator).

Copy link
Copy Markdown
Member

@pablogsal pablogsal Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to maybe move this test to the GitHub Actions target, as having it in the test suite can be a bit problematic but I would like to know what other RM think

CC: @ambv @zooba @ned-deily

Copy link
Copy Markdown
Member

@ned-deily ned-deily Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this test? And, if it fails, what action should be taken? And what happens with those third-party distributors who do not use the bundled pip? In other words, is this going to cause more problems than it might solve?

Copy link
Copy Markdown
Member

@merwok merwok Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that same point, but noticed the code uses test.support.open_urlresource so I expected it to do the skipping. I checked just now and it does indeed: https://github.com/kumaraditya303/cpython/blob/0c6c596ac9c935564998c5a666ea8cd9dd6ef927/Lib/test/support/__init__.py#L570

Copy link
Copy Markdown
Member

@pablogsal pablogsal Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this test? And, if it fails, what action should be taken?

@ned-deily The idea is to make sure the bundled pip and setuptools binaries are exactly what is upstream. This is because in PRs changing these files we cannot easily tell from the PR that the binary blobs don't contain malicious code.

Copy link
Copy Markdown
Member

@ned-deily ned-deily Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ned-deily The idea is to make sure the bundled pip and setuptools binaries are exactly what is upstream. This is because in PRs changing these files we cannot easily tell from the PR that the binary blobs don't contain malicious code.

Then the test should only run on a merge into the repo, perhaps via GitHub Actions as you suggested. And/or as part of the release process, i.e. when producing the release materials. Otherwise, attempting to run an integrity test every time someone runs the test suite seems both problematic and misleading: if we can't trust what's in the repo we've downloaded from GitHub or in the release materials we've downloaded from python.org, shouldn't there be an integrity test of the whole source package or repo? But for release materials, there already is.

So, instead of adding a test case to the normal test suite, how about adding it as a command line option to the ensurepip module itself, then add running that option as part of the release process, and adding a GitHub action to run that option whenever any changes are merged to the wheel? That also gives downstream users the opportunity to easily run it if they so desire perhaps after running an ensurepip --update. (Side issue, ideally we should find a better way to separate out the wheels from the source repo so that they don't unnecessarily contribute to the repo's size over time.)

packages = ensurepip._get_packages()
for name, package in packages.items():
with test.support.open_urlresource(f"https://pypi.org/pypi/{name}/{package.version}/json") as data:
expected_sha256 = json.load(data)["releases"][package.version][0]["digests"]["sha256"]
if package.wheel_name:
# Use bundled wheel package
from ensurepip import _bundled
from importlib import resources
wheel_name = package.wheel_name
whl = (resources.files(_bundled) / wheel_name).read_bytes()
else:
# Use the wheel package directory
with open(package.wheel_path, "rb") as fp:
whl = fp.read()
self.assertEqual(
hashlib.sha256(whl).hexdigest(),
expected_sha256,
)

def touch(self, directory, filename):
fullname = os.path.join(directory, filename)
open(fullname, "wb").close()
Expand Down