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 CLI options for enhancing requests with HTTP headers #8078

Closed
wants to merge 7 commits into from

Conversation

amancevice
Copy link

Adressess issue #8042. Originally proposed in PR #8030 but more discussion was requested. This PR is a new attempt to add the barebones functionality requested without compromising on security.

This change adds a cURL-like -H / --header option to the CLI to allow users to enhance requests with custom HTTP headers.

To prevent pip from sending sensitive headers to unintended hosts, this option is ignored when extra index URLs are provided. The CLI will emit a warning to the user and any provided headers will be ignored if pip is configured to use multiple index URLs.

Example:

pip install \
  --index-url http://pypi.index/simple/ \
  --trusted-host pypi.index \
  -H 'X-Spam: ~*~ SPAM ~*~' \
  requests

@wisechengyi
Copy link

I am unable to stamp this, but verified this works great. Thanks!

@amancevice
Copy link
Author

Any thoughts on this? @uranusjr @pradyunsg @NoahGorny ?

@NoahGorny
Copy link
Contributor

I really like the idea, but I am only a simple commenter and can not approve this PR in any way :(

jsirois pushed a commit to pex-tool/pip that referenced this pull request May 11, 2020
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

The logic looks good to me 🙂 A couple of notes on implementation.

amancevice pushed a commit to amancevice/pip that referenced this pull request May 14, 2020
- Condense changelog with reference to discussion pypa#8042
- Change warning to hard error with reference to pypa#8042 on multiple index URLs
- Change critical to warning on header parsing errors
@amancevice
Copy link
Author

Thank you for your comments, @uranusjr ! I've made the requested changes; hope they are to your satisfaction.

@pradyunsg pradyunsg added C: finder PackageFinder and index related code state: needs discussion This needs some more discussion type: feature request Request for a new feature labels May 31, 2020
@amancevice
Copy link
Author

Any updates on this? Happy to discuss

@amancevice
Copy link
Author

Hi, just pinging to inquire about where this PR stands.

@sbidoul
Copy link
Member

sbidoul commented Jul 11, 2020

I am personally not entirely comfortable with this, mostly for security reasons.

To prevent pip from sending sensitive headers to unintended hosts, this option is ignored when extra index URLs are provided.
The CLI will emit a warning to the user and any provided headers will be ignored if pip is configured to use multiple index URLs.

pip can do HTTP requests to many other hosts than indexes (--find-links, --requirements, and PEP 508 direct URLs come to mind).

So I tend to think this would require a per-host configuration. Or per index if we restrict this to indexes only, but that is probably too restrictive? I'm not sure how to do that elegantly (without going all the way to specifying a fully pluggable http session mechanism).

@pfmoore
Copy link
Member

pfmoore commented Jul 11, 2020

without going all the way to specifying a fully pluggable http session mechanism

... and frankly, a fully pluggable mechanism would enable solutions to many other open issues as well (Kerberos and NTLM support, integration with platform native networking). I don't want to derail a quick win by expanding the scope to something unachievable, but I do think it's time that we had a serious look at pushing the complexity and limited flexibility of pip's current network handling options out to a pluggable mechanism of some sort.

@amancevice
Copy link
Author

I'm definitely sensitive to concerns about security, but by the same token (no pun intended) it would also be great to see pip support something more advanced than basic auth in the near term.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 16, 2020
Alexander Mancevice added 2 commits July 16, 2020 09:15
-H, --header <key:val>      HTTP header to include in all requests. This option
                            can be used multiple times. Conflicts with
                            --extra-index-url.

Example:

```
pip install \
  --index-url http://pypi.index/simple/ \
  --trusted-host pypi.index \
  -H 'X-Spam: ~*~ SPAM ~*~' \
  requests
```
Alexander Mancevice added 3 commits July 16, 2020 09:17
- Condense changelog with reference to discussion pypa#8042
- Change warning to hard error with reference to pypa#8042 on multiple index URLs
- Change critical to warning on header parsing errors
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 16, 2020
@connorlwilkes
Copy link

This is really useful for me. I am surprised that it doesn't currently exist within pip. Does anyone have any workarounds in the meantime?

What is the latest on this? Is this going to be merged or is it stuck? @amancevice Thanks

@amancevice
Copy link
Author

Some security concerns were raised and because of that I think the devs want to take this feature in a different direction.

My personal feeling is that this is a case of "letting perfect be the enemy of good" but the security concerns are valid.

I'm happy to revise this PR if the devs decide they would like to move forward on it, but my guess is this is a dead end.

@pfmoore
Copy link
Member

pfmoore commented Nov 10, 2020

I'd say you should ignore the digression about plugins, but look at how to address the point that pip doesn't just make HTTP requests to indexes. So if we don't want to expose sensitive information to (say) the host serving a direct URL, how should we do that? Modelling the interface on curl may be a mistake, as curl only does a single request each run, whereas pip does a lot of requests as part of an install. Per-host extra headers seems more secure/reliable.

It's also worth noting that we haven't had many requests for a feature like this, so risking a security hole for something that may be of limited benefit is something we have to take seriously.

@amancevice
Copy link
Author

thanks for the reply @pfmoore — I originally opened #8042 to open up the conversation as to what the interface could look like but it wasn't getting much traction (ie, I got antsy waiting for feedback 🙂) and opened this as a sort of minimum-viable implementation.

One of the proposals I put forward in #8042 was providing a command-line arg that accepted JSON in the form:

'{ "pypi.private.com": { "Authorization": "Bearer …", … } }'

And then pip would enhance any requests to pypi.private.com with the provided headers. Do you think this would be an acceptable second pass at this feature?

@pfmoore
Copy link
Member

pfmoore commented Nov 14, 2020

-1 on needing to enter JSON on the command line.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 18, 2021
@pradyunsg
Copy link
Member

Closing since this is fairly out of date and bitrotten.

Let's continue the discussion in #8042, since it's unclear if adding this functionality to pip would make sense. :)

@pradyunsg pradyunsg closed this Apr 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: finder PackageFinder and index related code needs rebase or merge PR has conflicts with current master state: needs discussion This needs some more discussion type: feature request Request for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants