Addresses Issue #5572: Implement PIP_TRUSTED_HOSTS logic... #5615
Conversation
|
Thnaks for opening this @kalebmckale -- we can port the change over to the new file structure -- I've opened a PR tonight that eliminates core.py because its been holding me back from fixing other bugs. #5616 |
|
I'm happy to look over your branch and make the adjustment there so you at least have one less thing to do, if you like.
Actually, I figured it out. 🙃 |
|
@matteius, I thought I'd look through Anyhoo, I made some refactoring notes before stopping. You can view them here: routines/install.py. |
|
Thank you @kalebmckale -- I'll review them when I have more time. The goal of the routines PR is to split apart the existing code without refactor, and that will make subsequent refactor PRs much more viable. I really appreciate you taking the time to read through the code and look for areas of improvement. |
|
No problem. I figured the current split was limited in scope because that's how I'd work on an issue, too. Keep it focused on specific tasks and iteratively make changes over multiple issues to ensure no breakage. 🙂 |
|
@kalebmckale Now that the other PR that splits apart core.py has been merged, could you resolve the conflicts? |
|
@kalebmckale I am hopeful to do a release come this weekend, if possible I would like to include this. |
|
Hey @matteius ! Sorry I’ve been spending a lot of extra time at work lately and haven’t had much time to do coding at home. I’ll see what I can do before this weekend. |
kalebmckale
force-pushed
the
issue-5572
branch
3 times, most recently
from
8d3ecae
to
d7dec99
Compare
|
I think that should do it, but I am not able to test it locally right now. |
| @@ -334,8 +334,21 @@ def do_install( | |||
| ) | |||
| # Add the package to the Pipfile. | |||
| if index_url: | |||
| trusted_hosts = get_trusted_hosts() | |||
| host_and_port = get_host_and_port(index_url) | |||
| require_valid_https = not any( | |||
There was a problem hiding this comment.
Just thinking through this change -- isn't this saying it requires valid https if any of the hosts in the PIP_TRUSTED_HOSTS? Wouldn't it be better to only check if the specific host of the index_url requires valid https?
There was a problem hiding this comment.
This is identical to the code for the requirements file. I initially assumed it was correct as-is and literally just copied it over. Let me look more into this.
POST-UPDATE: The logic seems correct because for each index_url, it checks if either its host with or without port appears in list of trusted_hosts. If it does, it will then set require_valid_https to False and hence verify_ssl=False, which is what we want here. The any() is checking all the trusted hosts against the one index_url host (with and without port) and doesn't set all index_urls. If an index_url's host doesn't appear in trusted_hosts, this will still set verify_ssl=True. In cases when an index_url doesn't have a port, I would assume that any() is just doing one comparison unless I'm missing something.
UPDATE: This is currently only checking against the set environment variable PIP_TRUSTED_HOSTS.
This was primarily a quick patch work-around to allow a way for a user to be able to specify that an index specified at the command line was a trusted host and should set the argument verify_ssl=False.
Truthfully, this has to be just step one since it should also use the data from the pip.conf file, which at the time, I didn't know how to do until you implemented your code.
Furthermore, really there is a need for refactoring the whole trusted-host code throughout the codebase to condense the logic into one source of truth for all the references to it. However, that's a much bigger project and this work-around (assuming the logic is correct) provides something that works until the larger overhaul can be completed.
Yes, unfortunately, pipenv has a very ill code base. But we are working to make it better :-) |
Duplicated logic from `import_requirements()` in requirements.py to `do_install()` in `install.py` to allow users to specify index via `--index` command-line option and validate against `PIP_TRUSTED_HOSTS` when determining `verify_ssl` value. Update news file to be more descriptive.
|
@oz123 No shame or shade intended. I am happy to help. This is my wheelhouse. |
|
Thanks for the updated documentation. Can you please fix the linting error? |
|
Sure! I think I understand what the error is, but just checking: is it saying I need an extra newline at the end of the news file? |
kalebmckale
requested review from
oz123 and
matteius
and removed request for
oz123 and
matteius
|
Sorry for all the confusing messages. This is literally my first time working on a big GitHub project. I’m used to a competitor’s product and am confused about some of the differences. So, I’m probably clicking on a lot of things I shouldn’t be because I don’t know what they do. |
|
At home, I have only my iPad to develop and run code. I've been trying to run |
Thank you for contributing to Pipenv!
The issue
What is the thing you want to fix? Is it associated with an issue on GitHub? Please mention it.
Opened issue #5572
The fix
How does this pull request fix your problem? Did you consider any alternatives? Why is this the best solution, in your opinion?
Duplicated logic from
import_requirements()todo_install()incore.pyto allow users to validate an index specified via--indexcommand-line option againstPIP_TRUSTED_HOSTSwhen determiningverify_sslvalue written to the Pipfile.This is the quickest fix to the problem, but ideally,
trusted_hostslogic throughout the package should be consolidated and made consistent.Along with the
pip.confchanges, a user can ensure Pipfile is created with correct indices from command-line the first time.The checklist
news/directory to describe this fix with the extension.bugfix.rst,.feature.rst,.behavior.rst,.doc.rst..vendor.rst. or.trivial.rst(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.