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

Support credentials from environment variables. #6723

Closed
wants to merge 3 commits into from

Conversation

lhupfeldt
Copy link

@lhupfeldt lhupfeldt commented Jul 17, 2019

Use PIP_USERNAME and PIP_PASSWORD for authentication if both are
defined in env.

Closes #4789.

Use PIP_USERNAME and PIP_PASSWORD for authentication if both are
defined in env.
- Fix line length failing py27 test.
@pradyunsg
Copy link
Member

Is it not possible to use keyring to do this? The upcoming version of pip will have support for keyring.

@lhupfeldt
Copy link
Author

lhupfeldt commented Jul 18, 2019 via email

@chrahunt
Copy link
Member

For #4789 (as indicated in news file).

@pradyunsg
Copy link
Member

Ah, good catch @chrahunt! I updated OP so that merging this PR will auto-close that issue. :D

@lhupfeldt -- While I don't know yet if we're going to merge this or not, thanks for for filing this PR! It's moving us forward on a long standing issue and is much appreciated. :)

@xavfernandez
Copy link
Member

This means that the specified PIP_USERNAME & PIP_PASSWORD would be sent to all indexes (compared to netrc & keyring solutions that match on the url).

A good side of having included keyring support means that you could now easily write a pip-env-credentials package (or some other name) defining a new keyring backend reading those env variables and once this package installed, pip would honor those env variables.

@lhupfeldt
Copy link
Author

lhupfeldt commented Jul 21, 2019 via email

- Documantation: Add note about same credentials used for all indexes.
@lhupfeldt
Copy link
Author

I have updated the documentation to mention that the same credentials will be used for all indexes.

@lhupfeldt
Copy link
Author

What are your thoughts on including this in the next release? I think this is really the simplest way of integrating with CI servers, while at the same time the complexity of the added code is extremely low.

@pradyunsg
Copy link
Member

pradyunsg commented Aug 2, 2019

twine also supports keyring.

I understand that the need for this but it is already possible to get equivalent (or superior) functionality, while requiring less functionality in pip.

It should be doable to have a keyring backend that reads from environment variables.

We worked on the keyring integration and one of the primary motivators for me supporting that work, was the fact that pip no longer has to do additional work to support different authentication workflows -- the end users can setup a keyring backend using whichever mechanism they prefer and pip knows how to use it.

@pradyunsg pradyunsg closed this Aug 2, 2019
@pradyunsg pradyunsg reopened this Aug 2, 2019
@pradyunsg
Copy link
Member

Oh, I don't love the GitHub mobile UI. 🙃

@pfmoore
Copy link
Member

pfmoore commented Aug 2, 2019

I don't really have much stake in this, but I agree with @pradyunsg - this is definitely something I'd have hoped the keyring integration would have covered, and if it doesn't I'd rather we addressed the deficiencies in that integration, rather than adding a second way of doing this.

If the problem is the bootstrapping issue that you can't use keyring until you've installed it, then that was something explicitly discussed when the support was added. Unless there's something new here that wasn't considered in that discussion, I don't see the point in having the same debate again.

@lhupfeldt
Copy link
Author

lhupfeldt commented Aug 3, 2019 via email

@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 Sep 21, 2019

with caplog.at_level(logging.DEBUG):
auth = MultiDomainBasicAuth()
get = functools.partial(
Copy link
Member

Choose a reason for hiding this comment

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

I would just call auth._get_new_credentials directly and store the result in an intermediate variable. If this ever fails the assertion output looks better as well since it would be retrieved_credentials == ('foo', 'bar') which is a bit easier to understand than get, IMO.

env_username = os.environ.get(user_var)
env_password = os.environ.get(passwd_var)
if env_username:
if env_password:
Copy link
Member

Choose a reason for hiding this comment

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

I would not put this restriction - there were many people in #6796 that have authentication consisting of a single token.

@pradyunsg
Copy link
Member

I'm gonna close this PR due to the merge conflicts, a lack of activity and because we have a open tracking issue for this request.

Thanks for filing this PR @lhupfeldt and feel free to file a new one if/when you fix the merge conflicts. :)

@pradyunsg pradyunsg closed this Jan 7, 2020
@lhupfeldt
Copy link
Author

I was under the impression that the idea was turned down.
If there is any interest, then I will be happy to fix merge conflicts.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 6, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2020
@pradyunsg
Copy link
Member

Well, because I'm looking at this discussion again today, and I noticed that you've said:

If I understand correctly you expect me to: 1) Install the python keyring package on my systems (without using pip) 2) Implement my own keyring backend and install that on my systems (without using pip) 3) Configure keyring to on all my systems to use my special backend.

You can use pip install --no-index {path-or-URL-to-an-accessible-wheel-for-keyring} {path-to-your-custom-credential-store}, in case you're wondering how to install a Python package offline.

There is no additional configuration to be done -- keyring will pick up that backend, assuming you have set the approreiate metadata. That said, let's continue the discussion in the motivating issue for this, to establish if there's still interest.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add support for credentials via environment variables
6 participants