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

Cleanup hash checking #3423

Closed

Conversation

FlorianLudwig
Copy link

@FlorianLudwig FlorianLudwig commented Nov 27, 2020

Pull Request Check List

Depends on python-poetry/poetry-core#113
Further improves: #2422

  • Added tests for changed code.
  • Updated documentation for changed code.

What is this about

There are two points where checksum are (supposed to be*) validated in poetry:

  1. When choosing a download link the hash sum is queried from the repo and checked against a list of known hashes
  2. After downloading the downloaded package is checked to have one of the known hashes

This MR changes a few things:

  1. Only check against the hash for the corresponding file name instead of all hashes - this improves security and makes it possible to improve output to the user.
  2. Improve output: I the current version* if the hash mismatches this could be for two reasons, the hash is for a different file (e.g. the lock file was created for the .tar.gz but it was deleted and a wheel was uploaded instead) or because the hash of the file does not match. These are two different scenarios which the user cannot differentiate. This MR makes it transparent for the user what is actually happening.
  3. Code Clean-up: When selecting which link to use for download, if a matching hash exists for the link is considered. But the code filters out all links with mismatching hashes before the sorting is even reached. So that part was removed.
* The current version does not validate hashes at all. I am referring to how the code is intended to do it.

Open TODOs

  1. This really needs proper tests - and I am willing to contribute them but I had some difficulties, see other MR
  2. I am not sure about how to properly handle the error/warning output when a mismatching hash occurs. I python logging - which I guess is okay but not as nice to look at as the usual poetry output. On the other hand, mismatching hashes means something ugly is going on... 🤷‍♂️
  3. Docs need to be added - but I would welcome input on code and CLI output before updating the docs.

Previously a downloaded package was checked against all available hashes.
This is now changed to only allow the hash from the lock file matching the
current archive name.

The user experience benefits with more precise error messages:
The use can now differenciate if the hash does not match
or if the file is not present in the lock file.
Links without matching hash are already yanked before sorting.
@JonZeolla
Copy link

@FlorianLudwig can you elaborate on the current hash validation gap? Wondering if there's a separate issue for that

@FlorianLudwig
Copy link
Author

FlorianLudwig commented Feb 1, 2021

@JonZeolla the current gap is not a gap, in the current version hashes are not validated at all. So yeah, there is more than one issue

See this mr:

https://github.com/python-poetry/poetry-core/pull/113/files#diff-4930d6de384074a402a5209bfc6d61ad5a11aabc4f98abf3b112f394c3778d09R409

The missing clone.files = self.files results in the current situation where no hashes are checked at all.

This MR here is an improvement for the validation.

@JonZeolla
Copy link

Thank you for the details

@FlorianLudwig
Copy link
Author

FlorianLudwig commented Feb 1, 2021

@JonZeolla also see #2422

edit, I just updated the other MRs title to better reflect the current situation

@neersighted
Copy link
Member

Feel free to correct me if I'm wrong, but I'm reasonably sure that #4740 obsoletes this PR for the most part. Pointing out follow-up work that is not included in that PR but is in this one would be appreciated.

I'm currently in the process of working out some more test coverage for that PR -- it will be ready soon.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants