Implement documented --force-keyring

[odormond]

This PR provide an implementation of --force-keyring that is documented in authentication.

It was initially introduced in #11698 and accidentally left behind in the documentation when addressing comments and merging the functionality into specifying an explicit keyring provider.

I'm reviving it in order to address #11721.

My implementation simply allows usage of keyring to retrieve the credentials on the initial request. This let us skip the initial request that fails with 401 Unauthorized and which also temporarily suspend the user on Artifactory. Keyring is actually only queried if a username is found in the configured (extra-)index-url and does not result in an attempt to unlock the keyring when simply talking to pypi thanks to the logic already in place.

The last three commits are not related to --force-keyring but just boyscout work:

• Add test for the --keyring-provider option
• Fix the help string of --keyring-provider
• Fix meaningless typo dowbload -> download

I've not included any news fragment for these three commits. I'll happily do so if this is considered the right thing do or split them into separate PRs if needed.

[Darsstar]

Looks good to me.

Hopefully it wasn't to hard to grok test_prompt_for_keyring_if_needed. I probably should have gone through the effort to refactor out the common stuff and made multiple easier to understand tests. Sorry.

Good luck!

[Darsstar]

Please remove this warning. It is identical to the one on line 159, which is entirely my fault.

[odormond]

Done. :-)

[odormond]

Hopefully it wasn't to hard to grok test_prompt_for_keyring_if_needed.

It was OK. I was just surprised by how much tests the combinatorial produced! 😆

I probably should have gone through the effort to refactor out the common stuff and made multiple easier to understand tests.

I actually like the extensive testing it produces and how the xfail marks are applied. I'm used to have many more independent test functions but I don't think that is really more readable in the end as you must scroll through pages of tests to see if you're covering all cases.

[odormond]

@Darsstar, is there is anything else I should do for this to get merged?

[Darsstar]

I'm not part of any pypa team. I just contributed the --keyring-provide implementation and occasionally see if any keyring related PRs exist that might break things.

I suggest to be patient. Maybe after two months or some ping a team member. That has been my tactic, anyway.

[odormond]

I'm not part of any pypa team. I just contributed the --keyring-provide implementation and occasionally see if any keyring related PRs exist that might break things.

I suggest to be patient. Maybe after two months or some ping a team member. That has been my tactic, anyway.

OK. Thanks for tips!

[pfmoore]

is there is anything else I should do for this to get merged?

It needs a pip maintainer to approve and merge it. Looking briefly back at the linked issue, the only core maintainer I see commenting is @uranusjr who (in effect) wanted some assurance that this wasn't simply to work around an issue with how Artifactory is handling permissions. I don't think that was ever really confirmed, beyond some discussion (that I didn't really follow, TBH) around whether a 401 response was correct.

In addition, the original implementation explicitly decided not to have a --force-keyring option (the docs were accidentally left after removing the option in #11698) and I don't see any real discussion of why the reasoning for that removal no longer applies.

Personally, as I said here, I'm not at all comfortable with the amount of complexity that the keyring option has resulted in, and this is yet another example of that. Therefore, I don't plan on approving or merging this PR myself¹, and I'd strongly advise any other pip maintainer considering doing so to ask for a better justification for why we now need this, what has changed since it was removed, and why there isn't a more "streamlined" and less complex way of controlling the whole keyring mechanism².

If @odormond, or anyone else interested in this PR, wants to put some effort into making it more likely that this functionality gets merged, I would suggest any or all of the following:

1. Document the various index authentication scenarios the keyring functionality is intended to address (there's Artifactory and Azure Artifacts that I've seen mentioned in various places, and probably others), and summarise which parts of the functionality are intended to support which scenarios. The idea here is to confirm that the existing complexity is, in at least some sense, "essential".
2. Review and simplify https://pip.pypa.io/en/stable/topics/authentication/#keyring-support so that it provides a more understandable explanation of this whole mess, and explains clearly why the various options are needed and when to use them.

Better still would be to contribute a simplified implementation - but that's a lot more work (more than I think it's reasonable to ask for just to get this feature added) as well as hitting exactly the same issue that no-one would be in a position to review or approve the change on anything but a "good enough for me" basis.

Footnotes

1. At least not without some much stronger justification that it solves a broad problem and isn't just working around Artifactory's limitations. ↩

2. After all, the reason this change isn't progressing is because the maintenance cost of the complexity of the existing feature is holding it up... ↩

[Darsstar]

and why there isn't a more "streamlined" and less complex way of controlling the whole keyring mechanism2.

The --force-keyring flag could be entirely removed, the right hand side of the or in the use_keyring property can be made into a force_keyring property.
That is already documented to force keyring usage: https://github.com/pypa/pip/pull/12473/files?diff=split&w=0#diff-9861226891ed3d81c117324f567ed9cc55eb39db78acadf64504546c1bde44ffL148-L149

That way you might, successfully, be able to argue that this PR has become a bugfix instead of a new feature, @odormond.

PS. Well redacted I should have written does not.

[odormond]

It needs a pip maintainer to approve and merge it. Looking briefly back at the linked issue, the only core maintainer I see commenting is @uranusjr who (in effect) wanted some assurance that this wasn't simply to work around an issue with how Artifactory is handling permissions. I don't think that was ever really confirmed, beyond some discussion (that I didn't really follow, TBH) around whether a 401 response was correct.

This PR was indeed prompted by how Artifactory handles permissions. I went with the --force-keyring option due to it's accidental presence in the doc and the relatively small code change needed to get it to work.

If @odormond, or anyone else interested in this PR, wants to put some effort into making it more likely that this functionality gets merged, I would suggest any or all of the following:

1. Document the various index authentication scenarios the keyring functionality is intended to address (there's Artifactory and Azure Artifacts that I've seen mentioned in various places, and probably others), and summarise which parts of the functionality are intended to support which scenarios. The idea here is to confirm that the existing complexity is, in at least some sense, "essential".

2. Review and simplify https://pip.pypa.io/en/stable/topics/authentication/#keyring-support so that it provides a more understandable explanation of this whole mess, and explains clearly _why_ the various options are needed and when to use them.

Better still would be to contribute a simplified implementation - but that's a lot more work (more than I think it's reasonable to ask for just to get this feature added) as well as hitting exactly the same issue that no-one would be in a position to review or approve the change on anything but a "good enough for me" basis.

I'll happily challenge the way pip is currently handling authentication in order to get a simpler implementation.

I went through a few RFCs in order to better understand what to expect from HTTP authentication and I think pip should simply not send spontaneously an Authorization header as it does now.
Why? Because RFC9110 Section 11: HTTP Authentication essentially says that which challenge the client can use to authenticate is up to the server to decide. The server will advertise the supported challenge through the WWW-Authenticate header returned with a 401 Unauthorized response. So pip should not assume that the server will require/support Basic auth for any particular resource and spontaneously send an Authentication header — but this is not forbidden by the RFC either.
Additionally, both RFC9110 Section 11.5 Establishing a Protection Space (Realm) and RFC7617 The 'Basic' HTTP Authentication Scheme indicates that credentials should be considered "realm" specific. And it turns out that the realm is only known after receiving the WWW-Authenticate response.

In my understanding, to stick to the RFCs, pip should send a first request without any Authentication header independently of the User Information being present or not in the index-url. If the server respond with a 401 Unauthorized then credentials lookup must be done and if a realm is provided by the server it should¹ be taken into account for the lookup.

With this in place, getting back to Artifactory, it shouldn't have enough information to temporarily suspend a user, as it wouldn't receive an Authentication header with a user-name but no valid credentials in the first place. That would fix the issue.

Wrapping it all up, I'll give it a try and start again from scratch, simply dropping the initial Authentication header. I'll report my finding later in this thread.

Footnotes

1. I've never seen the realm being seriously taken into account. I don't know of any password manager keeping track of it so I would not bother with it actually. ↩

[pfmoore]

@odormond Thanks for that analysis. I'm not an expert in HTTP authentication, so I can't comment on the details (and I'd be cautious about approving any PR without someone who does know about the subject) but your proposal does indeed sound cleaner and more in line with the sort of "take a step back and get the design right" approach I was advocating.

[odormond]

Wrapping it all up, I'll give it a try and start again from scratch, simply dropping the initial Authentication header. I'll report my finding later in this thread.

This fixes the issue with Artifactory as I expected.

I'll dig further to see how to handle the other quirks the code is currently taking care of, namely:

• the user-info part of the URI is actually an auth token (see Fix handling of tokens (single part credentials) in URLs #6818, pip >=19.2 cannot install packages via custom URLs with "one element" auth #6796, 6795)
• credentials need to be looked up from a matching index-url instead of the request url
• same netloc might be used with different credentials for different urls
• netrc support is actually disabled when retrying after the 401 (I don't see why it was done like this so I'll have to do some archaeology on that one too)

[odormond]

I've created PR #12496 to address the issue without introducing the --force-keyring.

I'm afraid the code is not significantly simpler due to the inherent complexity of supporting credentials coming from possibly 4 different places (original URL, index URL, keyring, netrc) and being possibly partially distributed (i.e. username in URL and password in keyring).

[pfmoore]

I've created PR #12496 to address the issue without introducing the --force-keyring.

I've made a few comments on that PR. For people following this PR, though, I'll note that my position is that I'm uncomfortable adding (maintenance) complexity to pip (which is 100% volunteer maintained) to patch over an issue that only affects a single (commercial) index implementation. I'm also concerned that the keyring support - which was added cautiously and on the basis that it "wouldn't be a big burden" - is becoming complex to maintain.

My preference would be to go back to the fundamental design in #6818 where we treat "URL with an auth token" and "URL with a username but no password" as the same. If that's an invalid assumption¹ then we should revisit the design there. Maybe "practicality beats purity" dictates that we just implement something that works, and don't get bogged down in principles, but if so then it'll be someone other than me who makes that call, I'm afraid.

Footnotes

1. And given that Artifactory assumes a single part credential is a username with no password, either they are making an unwarranted assumption, or pip is. ↩

[dougthor42]

there's Artifactory and Azure Artifacts that I've seen mentioned in various places, and probably others [that use keyring for auth]

Google Cloud Platform (GCP) Artifact Registry also recommends keyring as the preferred auth method. https://cloud.google.com/artifact-registry/docs/python/authentication#keyring. The index URL does not have any token or username/password information in it when using keyring - instead, a token is added to the request header.

[mikhail-mmlc]

Currently, the --keyring-provider value is not provided to the logic that handles the installation of build dependencies (issue #12423). The newly proposed --force-keyring option in this pull request also appears to lack integration with the building environment logic. I am unsure whether this issue should be addressed separately, within this pull request, or even addressed at all, but I wanted to highlight this concern.