-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Don't overuse the keyring #8687
Conversation
6ddd9ed
to
43213b1
Compare
OK, I've verified this works as expected via https://gist.github.com/encukou/7d227abb62b50af5ed3539dfb725331d (thanks @encukou). I have a video record of the behavior I can share with the reviewer if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, based on a desk-review and reading lots of related code in requests/keyring/pip.
The automated test seems "close enough" to me and I'm happy to trust @hroncok that this works based on their manual tests. :)
I'm wondering if we should slot this into 20.2.2 -- any thoughts from others on this front? It's technically not a bugfix for something added in 20.2, but it's a substantial enough improvement that we should release this ASAP IMO. |
I agree, it’s better to get this out sooner than later. |
src/pip/_internal/network/auth.py
Outdated
# Prompt the user for a new username and password | ||
username, password, save = self._prompt_for_password(parsed.netloc) | ||
save = False | ||
if not username or not password: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this have an impact on people that have stored API keys in keyring, or have only provided a username or password in response to a prompt? Not sure how prevalent that is but it can appear as only one of these being set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context on single-part login credentials, see #6796.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, this should probably be: if not username
only, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I took a closer look. Previously we did:
- Check up to 4 places (including keyring) for credentials based on the request URL (
_get_new_credentials
) - Make request
- Get 401 and unconditionally prompt user for credentials with the response URL (
handle_401
) - Make followup request (
handle_401
) - Get 401 and warn user (
warn_on_401
)
Now we do:
- Check up to 3 places (not keyring) for credentials based on the request URL (
_get_new_credentials
) - Make request
- Get 401 and check up to 4 places for credentials (including keyring) based on the response URL (
handle_401
) - If that doesn't return something reasonable, then prompt the user
- Make followup request (
handle_401
) - Get 401 and warn user (
warn_on_401
)
I think there's a few things to note with the changes we're making here:
- The response URL may be different from the request URL in case of redirects, so the second time getting the credentials may behave differently
- In the case where the response URL and request URL are the same, it may just try the same credentials again incorrectly
- The response URL shouldn't contain credentials, so we don't have to worry about retrying with the same embedded credentials
It seems pretty complicated and to make headway we'd probably want to refactor some of this class so it doesn't get worse. Maybe we can avoid that and just disable keyring for pypi URLs? Especially if this is a change people want to get in soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the last three points:
- I think this is the correct behaviour. Handling redirects feels like goodness (e.g. I can bit.ly an index URL), and if a redirect turns out to be malicious, we don't want to pass along legitimate credentials to a different site
- Unavoidable, probably? But we're in a failure case by then anyway, so an extra request is not the end of the world.
- Correct, though if
_get_new_credentials
can map it back to a user-specified index url that has embedded credentials, those will be used
It is complicated, but I think this all lines up.
For the single-part credential issue, we probably want if not username and not password:
- assuming that if we got a partial credential that we should still try it. _get_new_credentials
has the logic for trying to turn a partial credential into a full one.
@@ -110,7 +112,7 @@ def _get_index_url(self, url): | |||
return None | |||
|
|||
def _get_new_credentials(self, original_url, allow_netrc=True, | |||
allow_keyring=True): | |||
allow_keyring=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we looked at the impact this might have on keyring providers and associated package index assumptions? From #8103 it seems like Azure DevOps gives us a redirect, not a 401, when authentication is needed, and so may depend on our eager usage of artifacts-keyring
. Not that it should necessarily block us, but this may not be a low impact change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I have no idea. My drive is to fix this for users who use the default pypi.org index. While I've tried to make it work for the others as well, I don't have access to such setups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. Can we just disable the initial keyring support for pypi-related URLs? We keep them here. I think that would narrow the scope of this enough that we can defer investigating my previous concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, Azure DevOps should return 401 now, but it's filtered on user agents to make sure that people who get given the URL land on a useful page. So you can see it through an actual pip install, but likely not if you curl directly.
@chrahunt @hroncok Are there more changes necessary here on this PR right now? If yes, does anyone have a rough estimate about when they might be addressed? This is the only outstanding PR in the milestone right now. If this is gonna take more than a couple of days or so, I'd like to cut 20.2.2 release without this. |
I don't speak for Chris, but I'd certainly be OK with that. :) |
My only concern is that @chrahunt seems to be suggesting that we might break people using this for Azure DevOps. I don't have any particular opinion on the correct thing to do here, but if that is a risk (and not just my misunderstanding) do we have a plan for the possibility that we release 20.2.2 with this in, and immediately get cries from Azure users that we need a 20.2.3 to fix their issue? (I'm fine if that plan is "tell them they'll have to pin to 20.2.1 or wait for 20.3", I just think we should know up front what we expect to say). |
I'll mostly cut this tomorrow/day-after (~12/36 hours?) and I'll be around for at least 5 hours after that. If stuff goes down, I'll be able to cut a 20.2.3 that just reverts the offending commit. That's assuming I get a response from Chris here that he thinks adding both commits would be better than just one. If not, I think it'd be OK to force-push on this branch (I'm assuming Miro hasn't checked off the relevant box) and merge just the first commit. |
See #8744 for just the first commit. |
I'd be mostly OK with this PR if we just scoped it to PyPI URLs. If we go that route we may want to confirm that there are no plans to offer private repos on PyPI anytime soon.
Sorry if I worded it poorly. In my mind I think all my concerns would be addressed if we just set
I'm totally on board for that, I gave my 👍 on #8744.
Yes, I think that's a risk, based on what we've seen in a few recent issues. I have some instructions here and can test shortly. |
I was able to successfully install an artifact using this branch and the instructions from #8103 (comment). output
This doesn't show the behavior I'm concerned with (a redirect instead of a 401, as in #8103 (comment)), so it's a little reassuring. I think including just the first commit is still less of a risk. |
Hah, I can confirm this. PyPI runs on donated infrastructure, so trying to do something like this (which only makes sense if you have paying customers) would likely result in it losing access to that infrastructure. |
Alrighty! #8744 is merged. 20.2.2 coming up soon! :) |
Any chance to see this shipped? Current behavior creates deadlocks. |
Hmm, 20.2.3 was released without this change. Not sure if it’s intended or not @pradyunsg |
We broke out #8744, which was released in 20.2.2. Is there something else that I'm missing? |
I added this to 20.3. Rebasing needed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the slow reply, I'm trying to make more of an effort to dredge through my GH notifications.
I think this is an overall improvement, and don't have any concerns wrt the keyring integration or artifacts-keyring compat.
@@ -110,7 +112,7 @@ def _get_index_url(self, url): | |||
return None | |||
|
|||
def _get_new_credentials(self, original_url, allow_netrc=True, | |||
allow_keyring=True): | |||
allow_keyring=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, Azure DevOps should return 401 now, but it's filtered on user agents to make sure that people who get given the URL land on a useful page. So you can see it through an actual pip install, but likely not if you curl directly.
src/pip/_internal/network/auth.py
Outdated
# Prompt the user for a new username and password | ||
username, password, save = self._prompt_for_password(parsed.netloc) | ||
save = False | ||
if not username or not password: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the last three points:
- I think this is the correct behaviour. Handling redirects feels like goodness (e.g. I can bit.ly an index URL), and if a redirect turns out to be malicious, we don't want to pass along legitimate credentials to a different site
- Unavoidable, probably? But we're in a failure case by then anyway, so an extra request is not the end of the world.
- Correct, though if
_get_new_credentials
can map it back to a user-specified index url that has embedded credentials, those will be used
It is complicated, but I think this all lines up.
For the single-part credential issue, we probably want if not username and not password:
- assuming that if we got a partial credential that we should still try it. _get_new_credentials
has the logic for trying to turn a partial credential into a full one.
What's the status on the PR? The Debian freeze is approaching, and ideally the pip version included will be fully functional. |
@hroncok Would you be able to make time before pip 21.0 to rebase this PR? It'd be great if we could include this in that. |
43213b1
to
2ba3760
Compare
Rebased. But I'm not sure I am capable of incorporating all the feedback from @zooba. |
2ba3760
to
1ca4cbd
Compare
CI thinks there's something wrong with the news fragment, but I fail to understand it :( |
It is looking for a |
1ca4cbd
to
e586bb8
Compare
Thanks! File renamed. |
I think this looks OK. If @zooba agrees, I think we can go ahead and merge this? :) |
My only remaining comment is that we will still prompt for credentials if keyring provides a username or password but not both, even though that may be legitimate. Switching the "or" in auth:262 to an "and" should fix that, but I can't be 100% sure that it's better. Given that we don't reuse any partial information in the prompt, I'm inclined to say we should (log a message first and then) send the partial credential. If it's just a PAT, there's no good way for a user to reproduce that at the prompt. |
Happy to do that if that's the consensus. |
Go ahead! If someone complains, I'll likely @ mention both of you for help. 🙃 |
e586bb8
to
886275d
Compare
Let’s do this. |
See the individual commits and their messages.
Fixes #8090