Fixes #5948 Adds keyring support

[zooba]

Fixes #5948 Adds keyring support
Uses keyring to resolve credentials for indexes when a password is not provided.
Also uses the (soon to be added) keyring API to resolve usernames.

[zooba]

See jaraco/keyring#351 for the new keyring API for resolving usernames.

[cjerdonek]

Thanks for the PR! Some comments.

[zooba]

Thanks for the feedback, @cjerdonek

The only one I haven't addressed (I hope) is the debug logging. I would like to have something there, but logging credentials is a bit interesting. Are you thinking checks for successfully retrieving credentials without actually displaying them?

[cjerdonek]

Thanks for the quick turnaround, @zooba. Re: the debug logging, what I had in mind was more showing enough to know what code path was happening (e.g. to troubleshoot whether the keyring was working or not for some case). So yeah, just "credentials found in keyring for <description>" or along those lines. But I don't know if it would be too verbose.

[zooba]

Considering I added my own print statements for debugging, it's probably worth having :)

Is logger.debug the right call?

[cjerdonek]

Okay, cool. Yeah, I think so.

[dstufft]

Is it not possible to bundle the keyring module like we've bundled all of pip's other dependencies? If not why?

If it's not possible to bundle keyring, we should probably guard the entire keyring section with a try except Exception bit to protect against changes in the keyring module from rendering pip un-usable (we'd just be unable to fetch from a repository that relied on the keyring module functioning).

[zooba]

It's possible to bundle it (@jaraco would happily put it in the stdlib if not for 1-2 concerns), but I don't have a particular need for that. My users are going to have to install my custom backend anyway, and that needs to pull down the dependency.

I'll add the catch-all for this section though, as that's a good idea.

[zooba]

Um, it looks like someone did the refactoring work suggested here and merged it already, so now my PR has conflicts.

It could be a while before I get a chance to understand the new code well enough to rewrite mine around it, so if it's easy enough feel free to just push the merge to my branch. Otherwise I'll get to this as soon as I can.

[cjerdonek]

@zooba The merge was done by @pradyunsg, but I wouldn't worry. The rebase should be trivial. Only two lines were changed, and shouldn't affect any of the surrounding code (they are in-line replacements preserving the same return value).

[zooba]

Inspired by your "rebase should be trivial" comment (and not at all because I'm procrastinating writing a report...), I started the rebase. Wasn't quite trivial, but I made it happen and it seems good (though it'll be easier to see in the diff view)

[pradyunsg]

now my PR has conflicts.

Whoooops! I hadn't noticed that there was an overlap between the PRs. I thought it was independent.

Sorry about that!

[cjerdonek]

Glad it worked out seemingly without too much trouble. Thanks for doing that, @zooba.

[zooba]

What else can/should I do to help get this merged?

[cjerdonek]

I don't have further review comments on the code myself, but I'll defer to @dstufft and the other committers on the PR as a whole since this isn't my area of expertise.

[xavfernandez]

I'm uncomfortable with the idea of using unvendored library in pip...

[pfmoore]

I agree with @xavfernandez - we have a policy of vendoring all our dependencies, which I'd prefer we stick with. Otherwise I foresee people raising issues around keyring support "not working" when they simply don't have the required bits present, or there's a problem with the version they have installed, or similar.

From what I can see, keyring and its dependency entrypoints are only 45K in size, and while I'm increasingly concerned about the size of the pip wheel (85% of it is vendored dependencies) this is a drop in the ocean (about 3-4%).

Is there any reason not to vendor the additional dependencies here? "I don't have a particular need for that" isn't really the point here 😄

[jaraco]

I think you'll run into problems vendoring keyring because keyring relies on its metadata (entry points) to discover the backends. I forget how vendoring works in pip, but someone should verify that when vendoring that the pertinent metadata is present and loadable by the entrypoints package.

[zooba]

And honestly, if keyring in pip doesn't support loading backends from entry points, I have no interest in this PR anymore. The whole point is to make authentication extensible without having to modify pip each time :)

For my use case (a custom keyring backend for certain index servers - not released yet because the team doesn't want to release something that the tools don't support), it would likely lead to us releasing a package that monkey-patches pip. I would hate to do that, but the alternative is giving out plain-text credentials and telling people to copy/paste them into build scripts, which everyone hates. (I guess another alternative is to bake specific support for our auth method into pip, which everyone would also hate and so I haven't even seriously suggested it.)

[zooba]

Also, backends are going to have dependencies on keyring as well, which will be hard if it has to be the vendored keyring. As it's a protocol rather than a stand-alone library or tools, I don't think it makes sense to vendor keyring.

[dstufft]

From a practical standpoint, I think that this is something where at least the backends are going to have to be allowed to be installed normally.

I don't have an opinion on if keyring itself should be bundled or not. I guess it would come down to whether it can be bundled (e.g. it uses entry points... can we even do that with bundled deps? We haven't had to before) and wether it has any stand alone functionality (e.g. does it ship with any backends of it's own that we'd want to have used by default?).

I haven't gotten a chance to check the PR again after my comments, but depending on the answers above, I don't have a problem with it being unbundled assuming we're very careful that even unexpected errors won't render pip unusable, and will instead just act as if that dependency isn't installed.

It wouldn't be our first optional, unbundled dependency. We allow people to install pyopenssl, etc in order to get better TLS on older Pythons as well.

[dstufft]

Oh, just then it reminds me, one possible problem is if anything pip is importing is a C extension on Windows, because importing a C extension locks it in Windows we can't delete it to do an upgrade. Is there any concern that a Keyring backend or dependency will be a C extension?

[zooba]

Is there any concern that a Keyring backend or dependency will be a C extension?

Um, yeah, probably :) But there are ways around that, so I think it is okay to document the potential interaction somewhere and let backends figure out their own approach to handling this case. The number of backend authors is going to be quite small, and they'll all have to read keyring's readme, which seems like a convenient place to put the note.

(For example, not importing the extension module until absolutely required, which presumably will never be the case for public PyPI. Or temporarily copying the pyd to another location and using it is pretty simple.)

[dstufft]

@zooba So I don't really know how keyring functions off the top of my head, so I may be wrong here, but I assume some backend authors may not have any idea that their thing is going to be imported by pip and know they need to do those work arounds.

I'm unfortunately not very familiar with the locking semantics on Windows. Are those work arounds something that pip can apply generically to all C extensions it might import? If not, then it may make sense to add a flag to disable keyring support that would prevent any C extensions in that import tree from being imported (as well as obviously disabling keyring functionality).

If it helps, if I recall the error was on trying to delete the... whatever a .so is on Windows (a .pyd?) after importing it.

[jaraco]

entry points... can we even do that with bundled deps?

Yes, if the relevant tools (in the case of keyring, entrypoints) can find the metadata for the vendored application. You could do this one of a couple of ways:

• Supply the keyring metadata with references rewritten to point at the the vendored package and install that metadata alongside the pip metadata (i.e. a pip_vendored_keyring.dist-info).
• Have pip present the keyring entry points as if it was its own, still rewritten.

This latter technique might be as simple as defining the entry points in setup.cfg:

[options.entry_points]
keyring.backends =
    Windows = pip._vendor.keyring.backends.Windows
    macOS = pip._vendor.keyring.backends.OS_X
    SecretService = pip._vendor.keyring.backends.SecretService
    KWallet = pip._vendor.keyring.backends.kwallet

Either of these techniques would have the nasty effect of supplying duplicate keyrings if keyring were installed properly.

If you're also proposing vendoring other backends, you'd want to include the entry points for those packages also.

All things considered, while this seems technically possible, I wouldn't recommend it. The potential for difficult-to-debug issues is too high.

[zooba]

Assuming the linting issues are fixed, this PR is awaiting maintainer feedback/merge.

[BrownTruck]

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!

[Seairth]

please get this merged in. this feature is long overdue.

[skaughtx0r]

@pradyunsg Can this be merged yet? It's getting annoying installing this from @zooba's branch.

[fbirchmore]

Hello,
Can this feature please be merged soon? My colleagues and I would like to be able to update repos without typing in the password repeatedly. However, the workplace policy prohibits storing passwords in plaintext. Thanks!

[dstufft]

I wonder if it makes sense to also catch Exception here, perhaps printing a warning since it's an "unexpected" error, and then still setting keyring to None. That way something like say a SyntaxError in keyring doesn't kill pip entirely?

[zooba]

@dstufft I think this is ready for another look

[pradyunsg]

Thanks @zooba for doing this and @dstufft for making time to review this!

[infin8x]

We're looking forward to recommending this to all of our Azure Artifacts users who are storing Python packages with us. Apologies for the newbie question: it looks like this is in master but not yet released. Is there a cadence for releases that we can plan around? We'll be adding some guidance to our product and docs for users to install keyring.

[pfmoore]

It's in the docs (although a bit hard to find) - basically we release on a quarterly cycle so the next release should be in July.

[infin8x]

Ah, thanks for the pointer! We'll look forward to the July release, that works well with what we were planning.

[pradyunsg]

@infin8x if you don't mind answering a few user research questions, could you please tell me the following:

Did you look somewhere for this information? If so, where did you look?

Did you try googling this? If so, what terms did you try googling with?

It's perfectly fine if you don't want to respond or if you didn't look around. I'm just trying to figure out how we could make it easier for users to discover such information more easily. :)

[infin8x]

Happy to help :).

I first looked at the historical releases to see if I could pattern-match a cadence based on the last few releases. Looking again now, if I'd thought to ignore the patch releases, there's actually a clear quarterly cadence. But, the patch releases threw me off. I then looked at the README to see if there was some sort of note there.

(A bit ashamed to admit this) I didn't think to Google. In retrospect, it makes total sense to search for pip release cadence but I guess I didn't think of that as information that would be documented somewhere. (Probably an old Microsoft habit of assuming stuff like that is tribal knowledge).

Hope this helps.

[pfmoore]

It does - thanks. @pradyunsg Maybe we could add a brief paragraph in the README - something simple like "pip usually comes installed with Python, and updates are released regularly, with new versions every 3 months" plus a link to the relevant part of the docs?

[pradyunsg]

Makes sense to me. I'm not sure where it should go though. None the less, filing a new issue to discuss this.

[pradyunsg]

Thanks for the honest answer @infin8x! :D

No worries about not googling. I think there's enough folks who don't Google for stuff like this and we should take that into account. FWIW, the query you pointed out, does surface the required details as the first result so that's good to know for us. :D

Let's continue the discussion in the above linked issue.