Add an interface to allow calling system `keyring` by judahrand · Pull Request #11589 · pypa/pip (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation34 Commits19 Checks0 Files changed

Conversation

This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Closes #11588

This PR allows pip to use system wide keyring installations which appear on PATH.

In order to avoid breaking peoples current systems this will still default to using keyring imported from
the local environment. A different keyring installation will only be used if no local keyring is found.

Edit: this may also partially address #8485 as it delays the import of keyring

I'm not sure there's much to be tested here since we don't actually install keyring for testing if I understand correctly? 🤔

I haven't added any code branches outside KeyRingCli and testing the class itself is somewhat a pointless endeavor - it doesn't do much without keyring. But I'm happy to add whatever you feel is worthwhile.

Choose a reason for hiding this comment

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

I've made some general comments, but to be perfectly honest I don't use keyring at all, and I have little experience with what else might be needed here. So just to set expectations, I'm happy to review the code in general, but I'd want other maintainers to comment on whether this is an acceptable feature to add, and approve the implementation.

@classmethod
def set_password(cls, service_name: str, username: str, password: str) -> None:
cmd = ["keyring", "set", service_name, username]
input_ = password.encode() + b"\n"

Choose a reason for hiding this comment

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

I can imagine encoding issues here, especially on Windows.

Choose a reason for hiding this comment

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

I think I've solved this using PYTHONIOENCODING

res = subprocess.run(cmd)
if res.returncode:
raise RuntimeError(res.stderr)
password = res.stdout.decode().strip("\n")

Choose a reason for hiding this comment

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

Encoding issues possible here. On Windows, at least, it's not necessarily true that keywring will write its output in the same encoding as the pip process' default encoding.

Choose a reason for hiding this comment

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

I think I've solved this using PYTHONIOENCODING

) -> Optional[KeyRingCredential]:
cmd = ["keyring", "get", self._quote(service_name), self._quote(username)]
res = subprocess.run(cmd)
cmd = ["keyring", "get", service_name, str(username)]

Choose a reason for hiding this comment

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

Why str() round username? You don't account for the possibility of None (allowed by the type signature) and str() will do nothing if it's a string.

Choose a reason for hiding this comment

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

I've avoided this by only supporting get_password. I think this is the more correct thing to do as this is actually the function which is being called by the CLI.

Choose a reason for hiding this comment

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

I've moved to mirroring keyring's default implementation of get_credential which just wraps get_password

cmd = ["keyring", "get", self._quote(service_name), self._quote(username)]
res = subprocess.run(cmd)
cmd = ["keyring", "get", service_name, str(username)]
res = subprocess.run(cmd, capture_output=True)

Choose a reason for hiding this comment

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

You need to support the --no-input option here, so don't try to read stdin if the user supplied that flag.

Choose a reason for hiding this comment

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

I don't think I understand this. I'm not reading from stdin here.

Did you mean this line res = subprocess.run(cmd, input=input_)? If so the reason I'm doing this is that the user has already supplied the password through interaction. We're just saving the result here in a callback. Unfortunately, keyring doesn't support passing the password in any other way as far as I can see.

Choose a reason for hiding this comment

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

No, the keyring command you run might try to read from stdin. The --no-input flag is specifically to ensure that people can run pip without getting prompted for input, so you need to ensure that if --no-input is specified, you stop the keyring subprocess from reading stdin - probably by passing stdin=subprocess.DEVNULL to the call, assuming the keyring process works correctly if you do that.

I'm not sure there's much to be tested here since we don't actually install keyring for testing if I understand correctly?

I think we should test this. If the existing keyring code isn't tested, then I see that as a flaw in the original implementation, not a precedent we should follow...

I'm not sure there's much to be tested here since we don't actually install keyring for testing if I understand correctly?

I think we should test this. If the existing keyring code isn't tested, then I see that as a flaw in the original implementation, not a precedent we should follow...

The existing code is tested but keyring is patched to return expected results. Are you suggesting adding some tests which require keyring to be installed? Happy to do that - is there a pattern to follow for tests which rely on external dependencies?

It shouldn't need to ever so no reason to allow it and have to jiggle around the --no-input option in pip.


try:
import keyring
except ImportError:
keyring = None # type: ignore[assignment]
keyring_path = shutil.which("keyring")
if keyring_path is not None:
keyring = KeyRingCli(keyring_path) # type: ignore[assignment]

Choose a reason for hiding this comment

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

Instead of trying to fake keyring’s Python interface, it’d probably be easier if we introduce a common abstraction. We can have KeyRingCliProvider and KeyRingPythonProvider wrapping each implementation. This should also help get rid of the ImportError block.

Choose a reason for hiding this comment

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

I've acted on this feedback

cmd,
stdin=subprocess.DEVNULL,
capture_output=True,
env=dict(PYTHONIOENCODING="utf-8"),

Choose a reason for hiding this comment

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

I believe setting ENV will remove all other environment variables, including ones that the user might have set to control the keyring command (which might be the right thing to do - I don't know about that) and some essential system variables on Windows.

Rather than using env like this, you need to take a copy of os.environ, modify it, and pass the modified copy into env.

Choose a reason for hiding this comment

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

I've acted on this feedback

I'll need to overhaul the tests for this area of the code to fit in with the new abstraction.

I'm not sure there's much to be tested here since we don't actually install keyring for testing if I understand correctly?

I think we should test this. If the existing keyring code isn't tested, then I see that as a flaw in the original implementation, not a precedent we should follow...

I've added some tests by mocking subprocess.run with the expected behavior of the keyring cli.

Comment on lines 161 to 167

python_keyring = KeyRingPythonProvider()
if python_keyring.is_available():
return python_keyring

cli_keyring = KeyRingCliProvider()
if cli_keyring.is_available():
return cli_keyring

Choose a reason for hiding this comment

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

Since we only return a provider if the backend is available anyway, I wonder if it’d be easier to do something like

try: import keyring except ImportError: keyring = None # type: ignore[assignment]

class KeyRingCliProvider: def init(self, cmd: str) -> None: self.keyring = cmd

def get_keyring_provider() -> Optional[KeyRingBaseProvider]: if keyring is not None: return KeyRingPythonProvider() cli = shutil.which("keyring") if cli: return KeyRingCliProvider(cli) return None

This avoids a few is_available checks.

I also wonder whether it’d be a good idea to have a KeyRingNullProvider that always fails; this should help localise the KEYRING_DISABLED logic in get_keyring_provider and eliminate some is None checks.

Choose a reason for hiding this comment

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

I think there is an advantage to delaying the import keyring until get_keyring_provider is called. It means that if the user has provided a password we won't bother trying to import keyring (which from what I've heard can be very slow!).

I've implemented your idea of having a KeyRingNullProvider, doing away with is_available, and simplifying the KEYRING_DISABLED logic. This necessitates removing the cache on get_keyring_provider but that's probably fine.

I've reached the point now where this looks good to me, and I'm assuming that the way it uses the keyring client is OK (I have very limited knowledge of the keyring module). So I've approved it but I'll wait for @uranusjr's further review.

I've reached the point now where this looks good to me, and I'm assuming that the way it uses the keyring client is OK (I have very limited knowledge of the keyring module). So I've approved it but I'll wait for @uranusjr's further review.

Thanks very much, Paul. Appreciate your patience with me. I'm excited to be (potentially -- still early days) making my first contribution to pip. Massive FOSS supporter and very grateful for your work.