Add flag to force pip to always fetch password from keyring

[rittneje]

What's the problem this feature will solve?

I am trying to fetch modules from a private server. In order to avoid storing this password in filesystem or environment, I have added it to keyring instead. I have also set https://[email protected] as global.index-url in my pip.conf.

Based upon the verbose logs from pip install, it seems that pip will first do the request without a password, get a 401, and then fetch the password from keyring.

Describe the solution you'd like

There should be a way to force pip to always fetch the password from keyring in the first place. This would prevent it from spamming the server with unauthenticated requests.

Alternative Solutions

I don't want to put the password in plaintext in my pip.conf or environment variables, so I'm not aware of any alternative solution.

Additional context

n/a

[uranusjr]

Hi! Would you be interested in working on a solution to this? We can first produce a prototype that makes pip switch behaviour (maybe based on an environment variable first) and work out a good way to present the functionality to users from there.

[rittneje]

I'll give it a try. One thing I'm wondering is whether it would make more sense to have a different env var for PIP_USERNAME or something to that effect, since as I understand it, just putting it in PIP_INDEX_URL is ambiguous between it being a (secret) token, or a username. Then if PIP_INDEX_URL includes no auth info, but PIP_USERNAME is set, it would know to fetch the password from keyring in the first place. That could also simplify things with pipenv (assuming it incorporates the same general change), since that way the Pipfile would just include the index url verbatim rather than having to interpolate env vars in it. Meanwhile for CI/CD cases the build server can set PIP_USERNAME and PIP_PASSWORD (or PIP_TOKEN, depending on the auth style). Thoughts?

By the way, for context our private repo is hosted on Artifactory. Due to a bug on their end, the repeated unauthenticated requests from pip cause it to return 403 at random when it shouldn't. So this fix would also resolve that issue.

[uranusjr]

The problem is you can have index URLs, and when you do that PIP_USERNAME won’t make sense. The config must be per-index.

[rittneje]

Hmm, wouldn't you have the same general problem with some "always auth with keyring" flag then?

[uranusjr]

My understanding is we store index URL auths per-domain in keyring, and uses keyring.get_password()’s service key to get the corresponding one for an index. An “always use keyring” flag would make pip ask keyring for every URL, but keyring can return different answers for them.

You can find this logic in pip._internal.network.auth.

[rittneje]

I see. Actually, I do have another question after trying to read through the code. In my case, the index url only contained a username, not a password (or even a colon). But the code seems to have checks of the form if username is not None and password is not None. So then, why didn't it fallthrough to keyring under the existing logic? It appears to be due to this:

pip/src/pip/_internal/network/auth.py

Line 180 in 330d0aa

username, password = self._get_new_credentials(original_url)

When the request is first sent, keyring isn't even consulted, because for some reason allow_keyring=True is not passed the first time. Instead, it only allows keyring after a 401. Do you know why that is? I can't think of any reason that it shouldn't try to fetch from keyring if the index url contains a username with no password. I would think changing that behavior to simply fetch from keyring unconditionally would resolve my issue without any need for a new flag.

pip/src/pip/_internal/network/auth.py

Lines 150 to 161 in 330d0aa

	# If we don't have a password and keyring is available, use it.
	if allow_keyring:
	# The index url is more specific than the netloc, so try it first
	# fmt: off
	kr_auth = (
	get_keyring_auth(index_url, username) or
	get_keyring_auth(netloc, username)
	)
	# fmt: on
	if kr_auth:
	logger.debug("Found credentials in keyring for %s", netloc)
	return kr_auth

[uranusjr]

That line is for cases where the file URL contains auth. The index part is after the self._get_index_url(url) line.

Edit: Oops, sorry, I misread the comment.

The first 401 pass is intentional so keyring is not hit when the URL does not actually need authentication, which is the vast majority of cases. The keyring can pretty slow (depending on the back-end), and we want to make it as nonintrusive as possible for most people. If pip is a corporate tool we would probably always go through keyring, but pip is FOSS, and 99% (impression number without evidence, don’t quote me) of our users are only using FOSS indexes, so we prioritise them.

[rittneje]

Upon further review of the code, I think I've found a bug that, if fixed, will mostly resolve my issue without needing any new flags, and without incurring a performance hit for the majority case.

After fetching the password from keyring, it caches the credentials for later.

pip/src/pip/_internal/network/auth.py

Lines 265 to 268 in 330d0aa

	# Store the new username and password to use for future requests
	self._credentials_to_save = None
	if username is not None and password is not None:
	self.passwords[parsed.netloc] = (username, password)

However, the cache is only consulted if the index url has no credentials at all.

pip/src/pip/_internal/network/auth.py

Lines 182 to 184 in 330d0aa

	# If credentials not found, use any stored credentials for this netloc
	if username is None and password is None:
	username, password = self.passwords.get(netloc, (None, None))

Instead, the cache lookup should be this:

if (username is None or password is None) and netloc in self.passwords:
    un, pw = self.passwords[netloc]
    if username is None or username == un:
        username, password = un, pw

Separately, it would be nice to avoid the initial 401 by always consulting keyring in the first place. But that seems more complicated to do correctly, and less necessary if the cache lookup is fixed.

[uranusjr]

Again, always checking keyring unconditionally would not be an option, because we don’t want to acces keyring unless necessary. Checking the cached entry sounds like a doable change, but I think we need to make self.passwords smarter, otherwise you won’t be able to cache the username-password set you got when username != un is not None. This goes back to the point that keyring can be pretty slow…

[rittneje]

Right, that is one of the reasons I was saying that being able to force it to consult keyring in the first place is more complicated.

I think we need to make self.passwords smarter, otherwise you won’t be able to cache the username-password set you got when username != un is not None.

I didn't understand this point. My modified condition for using cached credentials will handle it fine. The only time it doesn't work is when there are two index urls in play for the same hostname with different embedded usernames. But that seems like a pretty bizarre use case, and it would already not be using the cache today anyway.

[uranusjr]

The only time it doesn't work is when there are two index urls in play for the same hostname with different embedded usernames

Isn’t this what your username == un part is checking? I think I am becoming confused whith the snippets now. Maybe it would be easier to discuss this if you could submit this as a PR with some tests to illustrate its usage.

[rittneje]

Yes, by "doesn't work", I just meant that it won't really function as a cache. It won't mix up the passwords though.

I've opened #10288 as a draft PR. I have not tried to run it myself, or even run the unit tests.

[rittneje]

@uranusjr Please take a look at that PR when you have a chance.