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

Add pip-hashes to complement pip-requirements. #10600

Closed
wants to merge 1 commit into from

Conversation

deeglaze
Copy link
Contributor

@deeglaze deeglaze commented Jan 9, 2025

For stronger build integrity, the build tools as downloaded should be verified against hashes that this repo's maintainers also concur are authentic.

Note the "unsafe" qualifier for setuptools is not troublesome [1], but still pinning pip itself could be a problem [2]

Not pinning pip means that container definitions that pip install --upgrade pip -r requiremenst.txt will fail. The base container's pip package should be sufficient to install the edk2 build dependencies.

There are many hashes this tool adds for single release versions because one tool version can get released on many platforms and Python versions.

The tool stores hashes in alphabetical order rather than in a release list order. I have not culled any since there is no "official" development platform to weed out unnecessary hashes. If we decide that only the CI toolchain containers matter, then we should cull after first showing that they can handle --require-hashes in the pip-install step.

[1] jazzband/pip-tools#806 (comment)
[2] pypa/pip#6459

Description

  • Breaking change? No, the file is not used by anything in this commit.
  • Impacts security? Yes, it improves build integrity.
  • Includes tests? No, this will be followed-up with patches to the container definitions.

How This Was Tested

I copied in the pip-hashes.txt instead of downloading the "master" pip-requirements in the tianocore ubuntu22 container definition:

COPY ./pip-hashes.txt /tmp/pip-hashes.txt                                                                                                                                                                                                                                                                                                                     
RUN pip install --require-hashes -r /tmp/pip-hashes.txt 

This was successful. Note this drops the --upgrade pip added in tianocore/containers@46802aa.

I recommend that any reviewer install pip-tools themselves and run

pip-compile --allow-unsafe --generate-hashes --output-file=pip-hashes.txt pip-requirements.txt

To compare digests to show there is no funny business.

Integration Instructions

As tested.

@github-actions github-actions bot added the impact:security This change has a direct security impact such as changing a crypto algorithm. label Jan 9, 2025
@deeglaze
Copy link
Contributor Author

deeglaze commented Jan 9, 2025

/cc @bexcran

For stronger build integrity, the build tools as downloaded should be
verified against hashes that this repo's maintainers also concur are
authentic.

Note the "unsafe" qualifier for setuptools is not troublesome [1], but
still pinning pip itself could be a problem [2]

Not pinning pip means that container definitions that pip install
--upgrade pip -r requiremenst.txt will fail. The base container's pip
package should be sufficient to install the edk2 build dependencies.

There are many hashes this tool adds for single release versions because
one tool version can get released on many platforms and Python versions.

The tool stores hashes in alphabetical order rather than in a release
list order. I have not culled any since there is no "official"
development platform to weed out unnecessary hashes. If we decide that
only the CI toolchain containers matter, then we should cull after first
showing that they can handle --require-hashes in the pip-install step.

[1] jazzband/pip-tools#806 (comment)
[2] pypa/pip#6459

Signed-off-by: Dionna Glaze <[email protected]>
@deeglaze
Copy link
Contributor Author

PR #10592 may need to get merged first. Please advise.

Copy link

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale Due to lack of updates, this item is pending deletion. label Mar 11, 2025
Copy link

This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions.

@github-actions github-actions bot closed this Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:security This change has a direct security impact such as changing a crypto algorithm. stale Due to lack of updates, this item is pending deletion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant