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
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 15 additions & 24 deletions src/pip/_internal/network/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class Credentials(NamedTuple):


class KeyRingCredential(NamedTuple):
username: str
username: Optional[str]
password: str


Expand All @@ -46,40 +46,31 @@ class KeyRingCli:
PATH.
"""

@staticmethod
def _quote(string: Optional[str]) -> str:
return f"'{string}'"

@classmethod
def get_credential(
self, service_name: str, username: Optional[str]
cls, service_name: str, username: Optional[str]
) -> 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

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.

if res.returncode:
return None
return KeyRingCredential(username=username, password=res.stdout)

def set_password(self, service_name: str, username: str, password: str) -> None:
cmd = [
"echo",
self._quote(password),
"|",
"keyring",
"set",
self._quote(service_name),
self._quote(username),
]
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

return KeyRingCredential(username=username, password=password)

@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, input=input_)
res.check_returncode()
return None


try:
import keyring
except ImportError:
if shutil.which("keyring") is not None:
keyring = KeyRingCli()
keyring = KeyRingCli # type: ignore[assignment]
keyring = None # type: ignore[assignment]
except Exception as exc:
logger.warning(
Expand Down