From 6457415b423a54424e2f5f4546af95f8c6bd46f8 Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Fri, 12 Jan 2024 15:24:54 +0100 Subject: [PATCH 1/8] boyscout: add missing options test for --keyring-provider --- tests/unit/test_options.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/unit/test_options.py b/tests/unit/test_options.py index 22ff7f721d7..d450ac637f8 100644 --- a/tests/unit/test_options.py +++ b/tests/unit/test_options.py @@ -513,6 +513,16 @@ def test_no_input(self) -> None: assert options1.no_input assert options2.no_input + def test_keyring_provider(self) -> None: + # FakeCommand intentionally returns the wrong type. + options1, args1 = cast( + Tuple[Values, List[str]], main(["--keyring-provider", "import", "fake"]) + ) + options2, args2 = cast( + Tuple[Values, List[str]], main(["fake", "--keyring-provider", "import"]) + ) + assert options1.keyring_provider == options2.keyring_provider == "import" + def test_proxy(self) -> None: # FakeCommand intentionally returns the wrong type. options1, args1 = cast( From 12ea572c02b453e8214a90599ec7ee8f80d99f4b Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Fri, 12 Jan 2024 15:26:31 +0100 Subject: [PATCH 2/8] boyscout: fix --keyring-provider help string It's not listing all the choices and the default shown is hardcoded to the wrong value. --- src/pip/_internal/cli/cmdoptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index d05e502f908..6889918397f 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -260,8 +260,8 @@ class PipOption(Option): default="auto", help=( "Enable the credential lookup via the keyring library if user input is allowed." - " Specify which mechanism to use [disabled, import, subprocess]." - " (default: disabled)" + " Specify which mechanism to use [auto, disabled, import, subprocess]." + " (default: %default)" ), ) From 6badf4b0beb7ecebe6fde8d4b4cbf0b0d7755e68 Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Fri, 12 Jan 2024 15:26:06 +0100 Subject: [PATCH 3/8] boyscout: fix meaningless typo --- tests/unit/test_network_auth.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index 5c12d870156..d8aa3a83579 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -129,11 +129,11 @@ def test_prioritize_longest_path_prefix_match_organization() -> None: ) # Inspired by Azure DevOps URL structure, GitLab should look similar - assert get("http://example.com/org-name-alpha/repo-guid/dowbload/") == ( + assert get("http://example.com/org-name-alpha/repo-guid/download/") == ( "foo", "bar", ) - assert get("http://example.com/org-name-beta/repo-guid/dowbload/") == ("bar", "foo") + assert get("http://example.com/org-name-beta/repo-guid/download/") == ("bar", "foo") def test_prioritize_longest_path_prefix_match_project() -> None: @@ -149,10 +149,10 @@ def test_prioritize_longest_path_prefix_match_project() -> None: # Inspired by Azure DevOps URL structure, GitLab should look similar assert get( - "http://example.com/org-alpha/project-name-alpha/repo-guid/dowbload/" + "http://example.com/org-alpha/project-name-alpha/repo-guid/download/" ) == ("foo", "bar") assert get( - "http://example.com/org-alpha/project-name-beta/repo-guid/dowbload/" + "http://example.com/org-alpha/project-name-beta/repo-guid/download/" ) == ("bar", "foo") From 39db361b0729172e88955e9290671933183b626c Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Thu, 25 Jan 2024 09:09:01 +0100 Subject: [PATCH 4/8] auth: Don't use credentials until we know the server requires them --- src/pip/_internal/network/auth.py | 244 ++++++++++++++++-------------- 1 file changed, 132 insertions(+), 112 deletions(-) diff --git a/src/pip/_internal/network/auth.py b/src/pip/_internal/network/auth.py index 94a82fa6618..f014131562d 100644 --- a/src/pip/_internal/network/auth.py +++ b/src/pip/_internal/network/auth.py @@ -14,7 +14,7 @@ from functools import lru_cache from os.path import commonprefix from pathlib import Path -from typing import Any, Dict, List, NamedTuple, Optional, Tuple +from typing import Any, List, NamedTuple, Optional, Tuple from pip._vendor.requests.auth import AuthBase, HTTPBasicAuth from pip._vendor.requests.models import Request, Response @@ -230,7 +230,14 @@ def __init__( self.prompting = prompting self.index_urls = index_urls self.keyring_provider = keyring_provider # type: ignore[assignment] - self.passwords: Dict[str, AuthInfo] = {} + # In order to avoid prompting the user or its keyring repeatedly + # we cache the credentials required per URL "prefix" and not just by + # netloc. A single server might host multiple independent indexes with + # independent credentials for them so we must be more specific than the + # netloc, thus the URL "prefix". + # The list is kept sorted by decreasing prefix length in order to guarantee + # maximal specificity. + self._required_credentials: list[Tuple[str, AuthInfo]] = [] # When the user is prompted to enter credentials and keyring is # available, we will offer to save them. If the user accepts, # this value is set to the credentials they entered. After the @@ -279,8 +286,8 @@ def _get_keyring_auth( get_keyring_provider.cache_clear() return None - def _get_index_url(self, url: str) -> Optional[str]: - """Return the original index URL matching the requested URL. + def _get_index_url(self, url: str) -> Optional[Tuple[str, str]]: + """Return the common prefix and original index URL matching the requested URL. Cached or dynamically generated credentials may work against the original index URL rather than just the netloc. @@ -302,9 +309,10 @@ def _get_index_url(self, url: str) -> Optional[str]: for index in self.index_urls: index = index.rstrip("/") + "/" - parsed_index = urllib.parse.urlsplit(remove_auth_from_url(index)) + index_without_auth = remove_auth_from_url(index) + parsed_index = urllib.parse.urlsplit(index_without_auth) if parsed_url == parsed_index: - return index + return index_without_auth, index if parsed_url.netloc != parsed_index.netloc: continue @@ -325,119 +333,130 @@ def _get_index_url(self, url: str) -> Optional[str]: ).rfind("/"), ) - return urllib.parse.urlunsplit(candidates[0]) + index = urllib.parse.urlunsplit(candidates[0]) + common_path = ( + commonprefix([parsed_url.path, candidates[0].path]).rsplit("/", 1)[0] + "/" + ) + matching_prefix = urllib.parse.urlunsplit(parsed_url._replace(path=common_path)) + return matching_prefix, index def _get_new_credentials( self, original_url: str, - *, - allow_netrc: bool = True, - allow_keyring: bool = False, - ) -> AuthInfo: - """Find and return credentials for the specified URL.""" + ) -> Tuple[str, AuthInfo]: + """Find credentials for the specified URL and a key for caching them""" # Split the credentials and netloc from the url. - url, netloc, url_user_password = split_auth_netloc_from_url( + url_without_auth, netloc, (username, password) = split_auth_netloc_from_url( original_url, ) + cache_key = url_without_auth # Start with the credentials embedded in the url - username, password = url_user_password if username is not None and password is not None: - logger.debug("Found credentials in url for %s", netloc) - return url_user_password + logger.debug("Found credentials in url for %s", cache_key) + return cache_key, (username, password) # Find a matching index url for this request - index_url = self._get_index_url(url) - if index_url: + index_info = self._get_index_url(url_without_auth) + if index_info: + cache_key, index_url = index_info # Split the credentials from the url. - index_info = split_auth_netloc_from_url(index_url) - if index_info: - index_url, _, index_url_user_password = index_info - logger.debug("Found index url %s", index_url) - - # If an index URL was found, try its embedded credentials - if index_url and index_url_user_password[0] is not None: - username, password = index_url_user_password - if username is not None and password is not None: - logger.debug("Found credentials in index url for %s", netloc) - return index_url_user_password - - # Get creds from netrc if we still don't have them - if allow_netrc: - netrc_auth = get_netrc_auth(original_url) - if netrc_auth: - logger.debug("Found credentials in netrc for %s", netloc) - return netrc_auth - - # 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 = ( - self._get_keyring_auth(index_url, username) or - self._get_keyring_auth(netloc, username) - ) - # fmt: on - if kr_auth: - logger.debug("Found credentials in keyring for %s", netloc) - return kr_auth - - return username, password + ( + url_without_auth, + _, + (index_username, password), + ) = split_auth_netloc_from_url(index_url) + logger.debug("Found index url %s", url_without_auth) + + # If an index URL was found, try its embedded credentials as long as the + # original and index urls agree on the username. + if ( + index_username is not None + and password is not None + and username in (index_username, None) + ): + logger.debug("Found credentials in index url for %s", cache_key) + return cache_key, (index_username, password) + + # Use the username from the index only if the original does not specify one + if username is None: + username = index_username - def _get_url_and_credentials( - self, original_url: str - ) -> Tuple[str, Optional[str], Optional[str]]: - """Return the credentials to use for the provided URL. + if self.use_keyring: + # We still don't have a password so let's lookup up the url_without_auth in + # the keyring. Note that this is the original_url (without auth) if we did + # not find a matching index otherwise it's the index url (without auth). + kr_auth = self._get_keyring_auth(url_without_auth, username) + if kr_auth is None: + # Still no password so let's try with the netloc only. That also means + # we'll have to cache credentials for a minimal url, i.e.: + # scheme://netloc/. + scheme, netloc, *_ = urllib.parse.urlsplit(cache_key) + cache_key = urllib.parse.urlunsplit((scheme, netloc, "/", None, None)) + kr_auth = self._get_keyring_auth(netloc, username) + if kr_auth and ( # we found some credentials and + username is None # the URL does not constrain the username + or kr_auth[0] == username # or credentials are for the same user + ): + logger.debug("Found credentials in keyring for %s", cache_key) + return cache_key, kr_auth + + # As a last resort, try with netrc. This is as specific as the netloc so we + # have to cache credentials for a minimal url, i.e.: scheme://netloc/. + scheme, netloc, *_ = urllib.parse.urlsplit(cache_key) + cache_key = urllib.parse.urlunsplit((scheme, netloc, "/", None, None)) + netrc_auth = get_netrc_auth(original_url) + if netrc_auth: + logger.debug("Found credentials in netrc for %s", cache_key) + return cache_key, netrc_auth + + # Either username and password are None because neither the original_url nor any + # matching index had credentials and we did not find any entry in keyring or + # netrc. + # Or username is not None and password is None in case the original_url or any + # matching index_url has a userinfo without a `:` and we did not find + # any entry in keyring or netrc to complement the username with a password. + # If both a username and password where found (even if being empty strings) we + # would have exited earlier. + if username is None and password is None: + logger.debug("No credentials found for %s", cache_key) + else: + logger.debug( + "No password found for %s. Will assume username is a token.", cache_key + ) + return cache_key, (username, password) - If allowed, netrc and keyring may be used to obtain the - correct credentials. + def _get_required_credentials(self, url_without_auth: str) -> AuthInfo: + """Return the credentials that were found to be required for the given URL. - Returns (url_without_credentials, username, password). Note - that even if the original URL contains credentials, this - function may return a different username and password. + The URL must not contain any auth. """ - url, netloc, _ = split_auth_netloc_from_url(original_url) - - # Try to get credentials from original url - username, password = self._get_new_credentials(original_url) - - # If credentials not found, use any stored credentials for this netloc. - # Do this if either the username or the password is missing. - # This accounts for the situation in which the user has specified - # the username in the index url, but the password comes from keyring. - if (username is None or password is None) and netloc in self.passwords: - un, pw = self.passwords[netloc] - # It is possible that the cached credentials are for a different username, - # in which case the cache should be ignored. - if username is None or username == un: - username, password = un, pw - - if username is not None or password is not None: - # Convert the username and password if they're None, so that - # this netloc will show up as "cached" in the conditional above. - # Further, HTTPBasicAuth doesn't accept None, so it makes sense to - # cache the value that is going to be used. - username = username or "" - password = password or "" - - # Store any acquired credentials. - self.passwords[netloc] = (username, password) - - assert ( - # Credentials were found - (username is not None and password is not None) - # Credentials were not found - or (username is None and password is None) - ), f"Could not load credentials from url: {original_url}" - - return url, username, password + # Check if a similar URL is known to require credentials + for url_prefix, credentials in self._required_credentials: + if url_without_auth.startswith(url_prefix): + logger.debug("Found required credentials for %s", url_prefix) + return credentials + + # We're not aware of any required credentials for a similar URL. + # We might find out it is the case only after the server answered with + # a 401 Unauthorized response. + return None, None + + def _cache_required_credentials( + self, url_prefix: str, credentials: AuthInfo + ) -> None: + self._required_credentials = sorted( + self._required_credentials + [(url_prefix, credentials)], + reverse=True, + key=lambda v: len(v[0]), + ) def __call__(self, req: Request) -> Request: - # Get credentials for this request - url, username, password = self._get_url_and_credentials(req.url) - # Set the url of the request to the url without any credentials - req.url = url + req.url = remove_auth_from_url(req.url) + + # Get credentials for this request if they're required + username, password = self._get_required_credentials(req.url) if username is not None and password is not None: # Send the basic auth with this request @@ -478,15 +497,7 @@ def handle_401(self, resp: Response, **kwargs: Any) -> Response: if resp.status_code != 401: return resp - username, password = None, None - - # Query the keyring for credentials: - if self.use_keyring: - username, password = self._get_new_credentials( - resp.url, - allow_netrc=False, - allow_keyring=True, - ) + cache_key, (username, password) = self._get_new_credentials(resp.url) # We are not able to prompt the user so simply return the response if not self.prompting and not username and not password: @@ -498,14 +509,23 @@ def handle_401(self, resp: Response, **kwargs: Any) -> Response: save = False if not username and not password: username, password, save = self._prompt_for_password(parsed.netloc) + if username: + # As we prompted the user for credentials for the netloc and + # not a more specific URL, we will cache them for the least + # specific URL, i.e.: scheme://netloc/ + scheme, netloc, *_ = parsed + cache_key = urllib.parse.urlunsplit((scheme, netloc, "/", None, None)) # 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) - + if username is not None: + self._cache_required_credentials(cache_key, (username, password)) # Prompt to save the password to keyring - if save and self._should_save_password_to_keyring(): + if ( + save + and password is not None + and self._should_save_password_to_keyring() + ): self._credentials_to_save = Credentials( url=parsed.netloc, username=username, From 2b2fbf903d6b4ce4f9ac3a16dfb45ff7cc4db75d Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Thu, 25 Jan 2024 09:51:29 +0100 Subject: [PATCH 5/8] auth: adjust unit tests to new signature of _get_new_credentials --- tests/unit/test_network_auth.py | 120 ++++++++++++++++++++------------ 1 file changed, 75 insertions(+), 45 deletions(-) diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index d8aa3a83579..7a282f24336 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -108,13 +108,17 @@ def test_get_index_url_credentials() -> None: "http://foo:bar@example.com/path", ] ) - get = functools.partial( - auth._get_new_credentials, allow_netrc=False, allow_keyring=False - ) + get = functools.partial(auth._get_new_credentials) # Check resolution of indexes - assert get("http://example.com/path/path2") == ("foo", "bar") - assert get("http://example.com/path3/path2") == (None, None) + assert get("http://example.com/path/path2") == ( + "http://example.com/path/", + ("foo", "bar"), + ) + assert get("http://example.com/path3/path2") == ( + "http://example.com/", + (None, None), + ) def test_prioritize_longest_path_prefix_match_organization() -> None: @@ -124,16 +128,17 @@ def test_prioritize_longest_path_prefix_match_organization() -> None: "http://bar:foo@example.com/org-name-beta/repo-alias/simple", ] ) - get = functools.partial( - auth._get_new_credentials, allow_netrc=False, allow_keyring=False - ) + get = functools.partial(auth._get_new_credentials) # Inspired by Azure DevOps URL structure, GitLab should look similar assert get("http://example.com/org-name-alpha/repo-guid/download/") == ( - "foo", - "bar", + "http://example.com/org-name-alpha/", + ("foo", "bar"), + ) + assert get("http://example.com/org-name-beta/repo-guid/download/") == ( + "http://example.com/org-name-beta/", + ("bar", "foo"), ) - assert get("http://example.com/org-name-beta/repo-guid/download/") == ("bar", "foo") def test_prioritize_longest_path_prefix_match_project() -> None: @@ -143,17 +148,15 @@ def test_prioritize_longest_path_prefix_match_project() -> None: "http://bar:foo@example.com/org-alpha/project-name-beta/repo-alias/simple", ] ) - get = functools.partial( - auth._get_new_credentials, allow_netrc=False, allow_keyring=False - ) + get = functools.partial(auth._get_new_credentials) # Inspired by Azure DevOps URL structure, GitLab should look similar assert get( "http://example.com/org-alpha/project-name-alpha/repo-guid/download/" - ) == ("foo", "bar") + ) == ("http://example.com/org-alpha/project-name-alpha/", ("foo", "bar")) assert get( "http://example.com/org-alpha/project-name-beta/repo-guid/download/" - ) == ("bar", "foo") + ) == ("http://example.com/org-alpha/project-name-beta/", ("bar", "foo")) class KeyringModuleV1: @@ -178,19 +181,28 @@ def set_password(self, system: str, username: str, password: str) -> None: @pytest.mark.parametrize( "url, expect", ( - ("http://example.com/path1", (None, None)), + ("http://example.com/path1", ("http://example.com/", (None, None))), # path1 URLs will be resolved by netloc - ("http://user@example.com/path3", ("user", "user!netloc")), - ("http://user2@example.com/path3", ("user2", "user2!netloc")), + ( + "http://user@example.com/path3", + ("http://example.com/", ("user", "user!netloc")), + ), + ( + "http://user2@example.com/path3", + ("http://example.com/", ("user2", "user2!netloc")), + ), # path2 URLs will be resolved by index URL - ("http://example.com/path2/path3", (None, None)), - ("http://foo@example.com/path2/path3", ("foo", "foo!url")), + ("http://example.com/path2/path3", ("http://example.com/", (None, None))), + ( + "http://foo@example.com/path2/path3", + ("http://example.com/path2/", ("foo", "foo!url")), + ), ), ) def test_keyring_get_password( monkeypatch: pytest.MonkeyPatch, url: str, - expect: Tuple[Optional[str], Optional[str]], + expect: Tuple[str, Tuple[Optional[str], Optional[str]]], ) -> None: keyring = KeyringModuleV1() monkeypatch.setitem(sys.modules, "keyring", keyring) @@ -199,7 +211,7 @@ def test_keyring_get_password( keyring_provider="import", ) - actual = auth._get_new_credentials(url, allow_netrc=False, allow_keyring=True) + actual = auth._get_new_credentials(url) assert actual == expect @@ -247,12 +259,16 @@ def test_keyring_get_password_username_in_index( index_urls=["http://user@example.com/path2", "http://example.com/path4"], keyring_provider="import", ) - get = functools.partial( - auth._get_new_credentials, allow_netrc=False, allow_keyring=True - ) + get = functools.partial(auth._get_new_credentials) - assert get("http://example.com/path2/path3") == ("user", "user!url") - assert get("http://example.com/path4/path1") == (None, None) + assert get("http://example.com/path2/path3") == ( + "http://example.com/path2/", + ("user", "user!url"), + ) + assert get("http://example.com/path4/path1") == ( + "http://example.com/", + (None, None), + ) @pytest.mark.parametrize( @@ -346,13 +362,22 @@ def get_credential(self, system: str, username: str) -> Optional[Credential]: @pytest.mark.parametrize( "url, expect", ( - ("http://example.com/path1", ("username", "netloc")), - ("http://example.com/path2/path3", ("username", "url")), - ("http://user2@example.com/path2/path3", ("username", "url")), + ( + "http://example.com/", + ("http://example.com/", ("username", "netloc")), + ), + ( + "http://example.com/path2/path3", + ("http://example.com/path2/", ("username", "url")), + ), + ( + "http://user2@example.com/path2/path3", + ("http://example.com/", ("user2", None)), + ), ), ) def test_keyring_get_credential( - monkeypatch: pytest.MonkeyPatch, url: str, expect: Tuple[str, str] + monkeypatch: pytest.MonkeyPatch, url: str, expect: Tuple[str, Tuple[str, str]] ) -> None: monkeypatch.setitem(sys.modules, "keyring", KeyringModuleV2()) auth = MultiDomainBasicAuth( @@ -360,9 +385,7 @@ def test_keyring_get_credential( keyring_provider="import", ) - assert ( - auth._get_new_credentials(url, allow_netrc=False, allow_keyring=True) == expect - ) + assert auth._get_new_credentials(url) == expect class KeyringModuleBroken: @@ -387,9 +410,7 @@ def test_broken_keyring_disables_keyring(monkeypatch: pytest.MonkeyPatch) -> Non assert keyring_broken._call_count == 0 for i in range(5): url = "http://example.com/path" + str(i) - assert auth._get_new_credentials( - url, allow_netrc=False, allow_keyring=True - ) == (None, None) + assert auth._get_new_credentials(url) == ("http://example.com/", (None, None)) assert keyring_broken._call_count == 1 @@ -443,19 +464,28 @@ def check_returncode(self) -> None: @pytest.mark.parametrize( "url, expect", ( - ("http://example.com/path1", (None, None)), + ("http://example.com/path1", ("http://example.com/", (None, None))), # path1 URLs will be resolved by netloc - ("http://user@example.com/path3", ("user", "user!netloc")), - ("http://user2@example.com/path3", ("user2", "user2!netloc")), + ( + "http://user@example.com/path3/", + ("http://example.com/", ("user", "user!netloc")), + ), + ( + "http://user2@example.com/path3", + ("http://example.com/", ("user2", "user2!netloc")), + ), # path2 URLs will be resolved by index URL - ("http://example.com/path2/path3", (None, None)), - ("http://foo@example.com/path2/path3", ("foo", "foo!url")), + ("http://example.com/path2/path3", ("http://example.com/", (None, None))), + ( + "http://foo@example.com/path2/path3", + ("http://example.com/path2/", ("foo", "foo!url")), + ), ), ) def test_keyring_cli_get_password( monkeypatch: pytest.MonkeyPatch, url: str, - expect: Tuple[Optional[str], Optional[str]], + expect: Tuple[str, Tuple[Optional[str], Optional[str]]], ) -> None: monkeypatch.setattr(pip._internal.network.auth.shutil, "which", lambda x: "keyring") monkeypatch.setattr( @@ -466,7 +496,7 @@ def test_keyring_cli_get_password( keyring_provider="subprocess", ) - actual = auth._get_new_credentials(url, allow_netrc=False, allow_keyring=True) + actual = auth._get_new_credentials(url) assert actual == expect From ace0adefd7a49328874d71d57f080813c87ab02e Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Thu, 25 Jan 2024 16:27:01 +0100 Subject: [PATCH 6/8] auth: remove tests of removed _get_url_and_credentials --- tests/unit/test_network_auth.py | 84 --------------------------------- 1 file changed, 84 deletions(-) diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index 7a282f24336..cf1a35c6f94 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -19,88 +19,6 @@ def reset_keyring() -> Iterable[None]: pip._internal.network.auth.get_keyring_provider.cache_clear() -@pytest.mark.parametrize( - ["input_url", "url", "username", "password"], - [ - ( - "http://user%40email.com:password@example.com/path", - "http://example.com/path", - "user@email.com", - "password", - ), - ( - "http://username:password@example.com/path", - "http://example.com/path", - "username", - "password", - ), - ( - "http://token@example.com/path", - "http://example.com/path", - "token", - "", - ), - ( - "http://example.com/path", - "http://example.com/path", - None, - None, - ), - ], -) -def test_get_credentials_parses_correctly( - input_url: str, url: str, username: Optional[str], password: Optional[str] -) -> None: - auth = MultiDomainBasicAuth() - get = auth._get_url_and_credentials - - # Check URL parsing - assert get(input_url) == (url, username, password) - assert ( - # There are no credentials in the URL - (username is None and password is None) - or - # Credentials were found and "cached" appropriately - auth.passwords["example.com"] == (username, password) - ) - - -def test_get_credentials_not_to_uses_cached_credentials() -> None: - auth = MultiDomainBasicAuth() - auth.passwords["example.com"] = ("user", "pass") - - got = auth._get_url_and_credentials("http://foo:bar@example.com/path") - expected = ("http://example.com/path", "foo", "bar") - assert got == expected - - -def test_get_credentials_not_to_uses_cached_credentials_only_username() -> None: - auth = MultiDomainBasicAuth() - auth.passwords["example.com"] = ("user", "pass") - - got = auth._get_url_and_credentials("http://foo@example.com/path") - expected = ("http://example.com/path", "foo", "") - assert got == expected - - -def test_get_credentials_uses_cached_credentials() -> None: - auth = MultiDomainBasicAuth() - auth.passwords["example.com"] = ("user", "pass") - - got = auth._get_url_and_credentials("http://example.com/path") - expected = ("http://example.com/path", "user", "pass") - assert got == expected - - -def test_get_credentials_uses_cached_credentials_only_username() -> None: - auth = MultiDomainBasicAuth() - auth.passwords["example.com"] = ("user", "pass") - - got = auth._get_url_and_credentials("http://user@example.com/path") - expected = ("http://example.com/path", "user", "pass") - assert got == expected - - def test_get_index_url_credentials() -> None: auth = MultiDomainBasicAuth( index_urls=[ @@ -296,7 +214,6 @@ def test_keyring_set_password( keyring = KeyringModuleV1() monkeypatch.setitem(sys.modules, "keyring", keyring) auth = MultiDomainBasicAuth(prompting=True, keyring_provider="import") - monkeypatch.setattr(auth, "_get_url_and_credentials", lambda u: (u, None, None)) monkeypatch.setattr(auth, "_prompt_for_password", lambda *a: creds) if creds[2]: # when _prompt_for_password indicates to save, we should save @@ -526,7 +443,6 @@ def test_keyring_cli_set_password( keyring = KeyringSubprocessResult() monkeypatch.setattr(pip._internal.network.auth.subprocess, "run", keyring) auth = MultiDomainBasicAuth(prompting=True, keyring_provider="subprocess") - monkeypatch.setattr(auth, "_get_url_and_credentials", lambda u: (u, None, None)) monkeypatch.setattr(auth, "_prompt_for_password", lambda *a: creds) if creds[2]: # when _prompt_for_password indicates to save, we should save From 4c61538896e261dfe5e1a66a678d9fa8d1de3f31 Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Thu, 25 Jan 2024 16:27:41 +0100 Subject: [PATCH 7/8] auth: add test of _get/save_required_credentials --- tests/unit/test_network_auth.py | 42 +++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/unit/test_network_auth.py b/tests/unit/test_network_auth.py index cf1a35c6f94..beb3b71f5fd 100644 --- a/tests/unit/test_network_auth.py +++ b/tests/unit/test_network_auth.py @@ -19,6 +19,48 @@ def reset_keyring() -> Iterable[None]: pip._internal.network.auth.get_keyring_provider.cache_clear() +def test_cache_required_credentials_maintains_url_prefix_specificity() -> None: + auth = MultiDomainBasicAuth() + save = auth._cache_required_credentials + expected = [ + ("http://longest.example.com/prefix/", ("user1", "pass1")), + ("https://example.com/other/prefix/", ("user2", "pass2")), + ("http://shorter.example.com/", ("user3", "pass3")), + ("https://short.example.com/", ("user4", "pass4")), + ("http://example.com/short/", ("user5", "pass5")), + ] + + # Save credentials in any wrong order + wrongly_ordered = [expected[3], expected[1], expected[2], expected[0], expected[4]] + for url_prefix, credentials in wrongly_ordered: + save(url_prefix, credentials) + + assert auth._required_credentials == expected + + +@pytest.mark.parametrize( + ["input_url", "username", "password"], + [ + ("http://unknown.example.com/path", None, None), + ("http://required.example.com/path/subpath", "username", "password"), + ("http://required.example.com/different/subpath", "token", None), + ], +) +def test_get_required_credentials( + input_url: str, username: Optional[str], password: Optional[str] +) -> None: + auth = MultiDomainBasicAuth() + for url_prefix, credentials in ( + ("http://required.example.com/path/", ("username", "password")), + ("http://required.example.com/different/", ("token", None)), + ): + auth._cache_required_credentials(url_prefix, credentials) + + get = auth._get_required_credentials + + assert get(input_url) == (username, password) + + def test_get_index_url_credentials() -> None: auth = MultiDomainBasicAuth( index_urls=[ From ab7a0346edf68d9e8a761e3214406d97b4e59fca Mon Sep 17 00:00:00 2001 From: Olivier Dormond Date: Fri, 26 Jan 2024 11:21:50 +0100 Subject: [PATCH 8/8] auth: update documentation and news --- docs/html/topics/authentication.md | 98 ++++++++++++++++++++---------- news/11721.bugfix.rst | 2 + news/11721.doc.rst | 2 + 3 files changed, 71 insertions(+), 31 deletions(-) create mode 100644 news/11721.bugfix.rst create mode 100644 news/11721.doc.rst diff --git a/docs/html/topics/authentication.md b/docs/html/topics/authentication.md index a2649071762..a0eb8d56e17 100644 --- a/docs/html/topics/authentication.md +++ b/docs/html/topics/authentication.md @@ -2,8 +2,18 @@ ## Basic HTTP authentication -pip supports basic HTTP-based authentication credentials. This is done by -providing the username (and optionally password) in the URL: +pip supports basic HTTP-based authentication credentials. You can provide the +credentials directly in the URLs or using {pypi}`keyring` or a `.netrc` file. When +needed pip will search for credentials in the following order: + +1. package URL from requirement (if any) +2. index URLs +3. keyring (if available) +4. `.netrc` file (if present) + +## URL credentials support + +You can provide the username (and optionally password) directly in the URL: ``` https://username:password@pypi.company.com/simple @@ -34,6 +44,15 @@ https://user:he%2F%2Fo@pypi.company.com/simple [reserved-chars]: https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters + +### Securely storing password in keyring + +It is recommended to avoid storing password in clear text. For this purpose, you can +use {pypi}`keyring` to store the password securely while still mentioning the username +to use in your URL. pip will then extract the username from the URL and, as it did not +find a password, it will search for a corresponding one in your keyring. See [Keyring +support](#keyring-support) bellow. + ## netrc support pip supports loading credentials from a user's `.netrc` file. If no credentials @@ -63,14 +82,45 @@ man pages][netrc-docs]. [netrc-docs]: https://www.gnu.org/software/inetutils/manual/html_node/The-_002enetrc-file.html [netrc-std-lib]: https://docs.python.org/3/library/netrc.html -## Keyring Support +## Keyring support + +pip supports loading credentials stored in your keyring using the {pypi}`keyring` +library, which can be enabled by passing `--keyring-provider` with a value of `auto`, +`import`, or `subprocess`. The default value `auto` respects `--no-input` and does not +query keyring at all if the option is used; otherwise it tries the `import`, +`subprocess`, and `disabled` providers (in this order) and uses the first one that +works. + +You can explicitly disable keyring support by passing `--keyring-provider=disabled`. + +When looking for credentials, pip will first try to find a record in your keyring for +the corresponding URL and if none are found it will try with just the server hostname. + +### Storing credentials + +In interactive mode, when the keyring is available and the server requires the +user to authenticate, pip will prompt you for the credentials and then save +them in the keyring. In this case the credentials will be saved based on the server +hostname. + +You can also manually store your credentials in your keyring, either for an index URL +(note that the URL _must_ end with a `/`): + +``` +keyring set https://pypi.company.com/dev/simple/ user.name@company.com +``` + +Or for a server hostname: -pip supports loading credentials stored in your keyring using the -{pypi}`keyring` library, which can be enabled py passing `--keyring-provider` -with a value of `auto`, `disabled`, `import`, or `subprocess`. The default -value `auto` respects `--no-input` and does not query keyring at all if the option -is used; otherwise it tries the `import`, `subprocess`, and `disabled` -providers (in this order) and uses the first one that works. +``` +keyring set pypi.company.com user.name@company.com +``` + +In both cases, `keyring` will prompt you for the password to store. + +Note: For server requiring a token instead of a username and password, you can still +store it as the username with an empty password in keyring but due to the limitation of +the `subprocess` provider, this only make sense when using the `import` provider. ### Configuring pip's keyring usage @@ -103,6 +153,10 @@ $ export PIP_KEYRING_PROVIDER=disabled Setting `keyring-provider` to `import` makes pip communicate with `keyring` via its Python interface. +This is slightly faster than the `subprocess` provider and it makes it possible to use +URLs without any username as it can find it in your keyring. The downside is that you +have to install it in every virtualenv. + ```bash # install keyring from PyPI $ pip install keyring --index-url https://pypi.org/simple @@ -115,9 +169,10 @@ $ pip install your-package --keyring-provider import --index-url https://pypi.co Setting `keyring-provider` to `subprocess` makes pip look for and use the `keyring` command found on `PATH`. -For this use case, a username *must* be included in the URL, since it is -required by `keyring`'s command line interface. See the example below or the -basic HTTP authentication section at the top of this page. +The advantage is that you only need to install it once (and it can even be installed +system-wide for all users). +The disadvantage is that, a username *must* be included in the URL, since it is required +by `keyring`'s command line interface. See the example below. ```bash # Install keyring from PyPI using pipx, which we assume is installed properly @@ -163,25 +218,6 @@ from the subprocess in which they run Pip. You won't know whether the keyring backend is waiting the user input or not in such situations. ``` -pip is conservative and does not query keyring at all when `--no-input` is used -because the keyring might require user interaction such as prompting the user -on the console. You can force keyring usage by passing `--force-keyring` or one -of the following: - -```bash -# possibly with --user, --global or --site -$ pip config set global.force-keyring true -# or -$ export PIP_FORCE_KEYRING=1 -``` - -```{warning} -Be careful when doing this since it could cause tools such as pipx and Pipenv -to appear to hang. They show their own progress indicator while hiding output -from the subprocess in which they run Pip. You won't know whether the keyring -backend is waiting the user input or not in such situations. -``` - Note that `keyring` (the Python package) needs to be installed separately from pip. This can create a bootstrapping issue if you need the credentials stored in the keyring to download and install keyring. diff --git a/news/11721.bugfix.rst b/news/11721.bugfix.rst new file mode 100644 index 00000000000..0dbe55cc14a --- /dev/null +++ b/news/11721.bugfix.rst @@ -0,0 +1,2 @@ +Wait until the index server tells us that authentication is required before trying +to make sense of the userinfo section of the URLs and potentially query ``keyring``. diff --git a/news/11721.doc.rst b/news/11721.doc.rst new file mode 100644 index 00000000000..743278c169c --- /dev/null +++ b/news/11721.doc.rst @@ -0,0 +1,2 @@ +- Removed mention of un-implemented ``--force-keyring`` option. +- Detailed the pros and cons of using the ``import`` and ``subprocess`` keyring providers.