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

Add an interface to allow calling system keyring #11589

Merged
merged 19 commits into from
Nov 10, 2022

Conversation

judahrand
Copy link
Contributor

@judahrand judahrand commented Nov 9, 2022

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

@judahrand judahrand marked this pull request as draft November 9, 2022 20:06
@judahrand judahrand marked this pull request as ready for review November 9, 2022 20:29
@judahrand
Copy link
Contributor Author

judahrand commented Nov 9, 2022

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.

Copy link
Member

@pfmoore pfmoore left a comment

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"
Copy link
Member

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.

Copy link
Contributor Author

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")
Copy link
Member

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.

Copy link
Contributor Author

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)]
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@pfmoore
Copy link
Member

pfmoore commented Nov 9, 2022

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...

@judahrand
Copy link
Contributor Author

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]
Copy link
Member

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.

Copy link
Contributor Author

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"),
Copy link
Member

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.

Copy link
Contributor Author

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

@judahrand
Copy link
Contributor Author

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

@judahrand
Copy link
Contributor Author

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
Copy link
Member

@uranusjr uranusjr Nov 10, 2022

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.

Copy link
Contributor Author

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.

@pfmoore
Copy link
Member

pfmoore commented Nov 10, 2022

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.

@judahrand
Copy link
Contributor Author

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.

@pfmoore
Copy link
Member

pfmoore commented Nov 10, 2022

It's been a pleasure 🙂

@uranusjr uranusjr merged commit 1b9cc0f into pypa:main Nov 10, 2022
@pradyunsg
Copy link
Member

We didn't update the documentation -- @judahrand would you be interested in covering this in https://pip.pypa.io/en/stable/topics/authentication/#keyring-support?

I guess that section would need to be modified to have subheaders for the two mechanisms and the header for subprocess calls can get a

```{versionadded} 23.0
```

at the start of it.

@judahrand
Copy link
Contributor Author

We didn't update the documentation -- @judahrand would you be interested in covering this in https://pip.pypa.io/en/stable/topics/authentication/#keyring-support?

I guess that section would need to be modified to have subheaders for the two mechanisms and the header for subprocess calls can get a

```{versionadded} 23.0

at the start of it.

I'll try to get to this in the next day or two.

@pradyunsg
Copy link
Member

Awesome, thank you! ^.^

judahrand added a commit to judahrand/pip that referenced this pull request Nov 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support keyring when installed with pipx
4 participants