fix(keyring): make keyring unlock thread safe

When using `poetry install`, Poetry executor prefers to execute
operations using a `concurrent.futures.ThreadPoolExecutor`. This can
lead to multiple credential store unlock requests to be dispatched
simultaneously.

If the credential store is locked, and a user either intentionally or
unintentionally dismisses the prompt to provide the store password; or
the dbus messages fail to launch the prompter the Poetry user can
experience, what can appear as a "hang". This in fact can be several
threads competing with each other waiting for a dbus event indefinitely;
this is a consequence of how Poetry uses keyring.

This change introduces the following:

- pre-flight check for installer commands that ensures keyring unlocks
  only once for the duration of the command
- improved logging of keyring unlock event and/or failure
- thread safe caching of `PasswordManager.<use_keyring|keyring>`

This change does not address the following:

- handling cases where dbus message fails or prompter is blocked
- authentication of a locked keyring in a non-tty session

Author: abn

src/poetry/console/commands/installer_command.py

@@ -4,9 +4,12 @@
 
 from poetry.console.commands.env_command import EnvCommand
 from poetry.console.commands.group_command import GroupCommand
+from poetry.utils.password_manager import PoetryKeyring
 
 
 if TYPE_CHECKING:
+    from cleo.io.io import IO
+
     from poetry.installation.installer import Installer
 
 
@@ -30,3 +33,7 @@ def installer(self) -> Installer:
 
     def set_installer(self, installer: Installer) -> None:
         self._installer = installer
+
+    def execute(self, io: IO) -> int:
+        PoetryKeyring.preflight_check(io)
+        return super().execute(io)

src/poetry/utils/password_manager.py

@@ -7,11 +7,15 @@
 from contextlib import suppress
 from typing import TYPE_CHECKING
 
+from poetry.config.config import Config
+from poetry.utils.threading import atomic_cached_property
+
 
 if TYPE_CHECKING:
-    from keyring.backend import KeyringBackend
+    import keyring.backend
+
+    from cleo.io.io import IO
 
-    from poetry.config.config import Config
 
 logger = logging.getLogger(__name__)
 
@@ -37,6 +41,35 @@ class PoetryKeyring:
     def __init__(self, namespace: str) -> None:
         self._namespace = namespace
 
+    @staticmethod
+    def preflight_check(io: IO | None = None, config: Config | None = None) -> None:
+        """
+        Performs a preflight check to determine the availability of the keyring service
+        and logs the status if verbosity is enabled. This method is used to validate
+        the configuration setup related to the keyring functionality.
+
+        :param io: An optional input/output handler used to log messages during the
+            preflight check. If not provided, logging will be skipped.
+        :param config: An optional configuration object. If not provided, a new
+            configuration instance will be created using the default factory method.
+        :return: None
+        """
+        config = config or Config.create()
+
+        if config.get("keyring.enabled"):
+            if io and io.is_verbose():
+                io.write("Checking keyring availability: ")
+
+            message = "<fg=yellow;options=bold>Unavailable</>"
+
+            with suppress(RuntimeError, ValueError):
+                if PoetryKeyring.is_available():
+                    message = "<fg=green;options=bold>Available</>"
+
+            if io and io.is_verbose():
+                io.write(message)
+                io.write_line("")
+
     def get_credential(
         self, *names: str, username: str | None = None
     ) -> HTTPAuthCredential:
@@ -70,9 +103,9 @@ def get_password(self, name: str, username: str) -> str | None:
 
         try:
             return keyring.get_password(name, username or self._EMPTY_USERNAME_KEY)
-        except (RuntimeError, keyring.errors.KeyringError):
+        except (RuntimeError, keyring.errors.KeyringError) as e:
             raise PoetryKeyringError(
-                f"Unable to retrieve the password for {name} from the key ring"
+                f"Unable to retrieve the password for {name} from the key ring {e}"
             )
 
     def set_password(self, name: str, username: str, password: str) -> None:
@@ -104,20 +137,22 @@ def get_entry_name(self, name: str) -> str:
         return f"{self._namespace}-{name}"
 
     @classmethod
+    @functools.cache
     def is_available(cls) -> bool:
         logger.debug("Checking if keyring is available")
         try:
             import keyring
             import keyring.backend
+            import keyring.errors
         except ImportError as e:
             logger.debug("An error occurred while importing keyring: %s", e)
             return False
 
-        def backend_name(backend: KeyringBackend) -> str:
+        def backend_name(backend: keyring.backend.KeyringBackend) -> str:
             name: str = backend.name
             return name.split(" ")[0]
 
-        def backend_is_valid(backend: KeyringBackend) -> bool:
+        def backend_is_valid(backend: keyring.backend.KeyringBackend) -> bool:
             name = backend_name(backend)
             if name in ("chainer", "fail", "null"):
                 logger.debug(f"Backend {backend.name!r} is not suitable")
@@ -138,20 +173,30 @@ def backend_is_valid(backend: KeyringBackend) -> bool:
         if valid_backend is None:
             logger.debug("No valid keyring backend was found")
             return False
-        else:
-            logger.debug(f"Using keyring backend {backend.name!r}")
-            return True
+
+        logger.debug(f"Using keyring backend {backend.name!r}")
+
+        try:
+            # unfortunately there is no clean of checking if keyring is unlocked
+            keyring.get_password("python-poetry-check", "python-poetry")
+        except (RuntimeError, keyring.errors.KeyringError):
+            logger.debug(
+                "Accessing keyring failed during availability check", exc_info=True
+            )
+            return False
+
+        return True
 
 
 class PasswordManager:
     def __init__(self, config: Config) -> None:
         self._config = config
 
-    @functools.cached_property
+    @atomic_cached_property
     def use_keyring(self) -> bool:
         return self._config.get("keyring.enabled") and PoetryKeyring.is_available()
 
-    @functools.cached_property
+    @atomic_cached_property
     def keyring(self) -> PoetryKeyring:
         if not self.use_keyring:
             raise PoetryKeyringError(

tests/conftest.py

@@ -43,6 +43,7 @@
 from tests.helpers import http_setup_redirect
 from tests.helpers import isolated_environment
 from tests.helpers import mock_clone
+from tests.helpers import set_keyring_backend
 from tests.helpers import switch_working_directory
 from tests.helpers import with_working_directory
 
@@ -204,29 +205,29 @@ def dummy_keyring() -> DummyBackend:
 
 @pytest.fixture()
 def with_simple_keyring(dummy_keyring: DummyBackend) -> None:
-    keyring.set_keyring(dummy_keyring)
+    set_keyring_backend(dummy_keyring)
 
 
 @pytest.fixture()
 def with_fail_keyring() -> None:
-    keyring.set_keyring(FailKeyring())  # type: ignore[no-untyped-call]
+    set_keyring_backend(FailKeyring())  # type: ignore[no-untyped-call]
 
 
 @pytest.fixture()
 def with_locked_keyring() -> None:
-    keyring.set_keyring(LockedBackend())  # type: ignore[no-untyped-call]
+    set_keyring_backend(LockedBackend())  # type: ignore[no-untyped-call]
 
 
 @pytest.fixture()
 def with_erroneous_keyring() -> None:
-    keyring.set_keyring(ErroneousBackend())  # type: ignore[no-untyped-call]
+    set_keyring_backend(ErroneousBackend())  # type: ignore[no-untyped-call]
 
 
 @pytest.fixture()
 def with_null_keyring() -> None:
     from keyring.backends.null import Keyring
 
-    keyring.set_keyring(Keyring())  # type: ignore[no-untyped-call]
+    set_keyring_backend(Keyring())  # type: ignore[no-untyped-call]
 
 
 @pytest.fixture()
@@ -237,7 +238,7 @@ def with_chained_fail_keyring(mocker: MockerFixture) -> None:
     )
     from keyring.backends.chainer import ChainerBackend
 
-    keyring.set_keyring(ChainerBackend())  # type: ignore[no-untyped-call]
+    set_keyring_backend(ChainerBackend())  # type: ignore[no-untyped-call]
 
 
 @pytest.fixture()
@@ -250,7 +251,7 @@ def with_chained_null_keyring(mocker: MockerFixture) -> None:
     )
     from keyring.backends.chainer import ChainerBackend
 
-    keyring.set_keyring(ChainerBackend())  # type: ignore[no-untyped-call]
+    set_keyring_backend(ChainerBackend())  # type: ignore[no-untyped-call]
 
 
 @pytest.fixture
@@ -633,3 +634,8 @@ def handle(self) -> int:
         return MockCommand()
 
     return _command_factory
+
+
+@pytest.fixture(autouse=True)
+def default_keyring(with_null_keyring: None) -> None:
+    pass

tests/helpers.py

@@ -8,6 +8,8 @@
 from pathlib import Path
 from typing import TYPE_CHECKING
 
+import keyring
+
 from poetry.core.packages.package import Package
 from poetry.core.packages.utils.link import Link
 from poetry.core.vcs.git import ParsedUrl
@@ -20,6 +22,7 @@
 from poetry.repositories import Repository
 from poetry.repositories.exceptions import PackageNotFoundError
 from poetry.utils._compat import metadata
+from poetry.utils.password_manager import PoetryKeyring
 
 
 if TYPE_CHECKING:
@@ -30,6 +33,7 @@
     import httpretty
 
     from httpretty.core import HTTPrettyRequest
+    from keyring.backend import KeyringBackend
     from poetry.core.constraints.version import Version
     from poetry.core.packages.dependency import Dependency
     from pytest_mock import MockerFixture
@@ -356,3 +360,9 @@ def with_working_directory(source: Path, target: Path | None = None) -> Iterator
 
     with switch_working_directory(target or source, remove=use_copy) as path:
         yield path
+
+
+def set_keyring_backend(backend: KeyringBackend) -> None:
+    """Clears availability cache and sets the specified keyring backend."""
+    PoetryKeyring.is_available.cache_clear()
+    keyring.set_keyring(backend)

tests/utils/test_authenticator.py

@@ -18,14 +18,14 @@
 
 from poetry.utils.authenticator import Authenticator
 from poetry.utils.authenticator import RepositoryCertificateConfig
+from poetry.utils.password_manager import PoetryKeyring
 
 
 if TYPE_CHECKING:
     from pytest import LogCaptureFixture
     from pytest import MonkeyPatch
     from pytest_mock import MockerFixture
 
-    from poetry.utils.password_manager import PoetryKeyring
     from tests.conftest import Config
     from tests.conftest import DummyBackend
 
@@ -96,29 +96,39 @@ def test_authenticator_ignores_locked_keyring(
     http: type[httpretty.httpretty],
     with_locked_keyring: None,
     caplog: LogCaptureFixture,
+    mocker: MockerFixture,
 ) -> None:
     caplog.set_level(logging.DEBUG, logger="poetry.utils.password_manager")
+    spy_get_credential = mocker.spy(PoetryKeyring, "get_credential")
+    spy_get_password = mocker.spy(PoetryKeyring, "get_password")
     authenticator = Authenticator()
     authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz")
 
     request = http.last_request()
     assert request.headers["Authorization"] is None
-    assert "Keyring foo.bar is locked" in caplog.messages
+    assert "Using keyring backend 'conftest LockedBackend'" in caplog.messages
+    assert spy_get_credential.call_count == spy_get_password.call_count == 0
 
 
 def test_authenticator_ignores_failing_keyring(
     mock_remote: None,
     http: type[httpretty.httpretty],
     with_erroneous_keyring: None,
     caplog: LogCaptureFixture,
+    mocker: MockerFixture,
 ) -> None:
     caplog.set_level(logging.DEBUG, logger="poetry.utils.password_manager")
+    spy_get_credential = mocker.spy(PoetryKeyring, "get_credential")
+    spy_get_password = mocker.spy(PoetryKeyring, "get_password")
     authenticator = Authenticator()
     authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz")
 
     request = http.last_request()
     assert request.headers["Authorization"] is None
-    assert "Accessing keyring foo.bar failed" in caplog.messages
+
+    assert "Using keyring backend 'conftest ErroneousBackend'" in caplog.messages
+    assert "Accessing keyring failed during availability check" in caplog.messages
+    assert spy_get_credential.call_count == spy_get_password.call_count == 0
 
 
 def test_authenticator_uses_password_only_credentials(

tests/utils/test_password_manager.py

@@ -285,16 +285,18 @@ def test_fail_keyring_should_be_unavailable(
     assert not key_ring.is_available()
 
 
-def test_locked_keyring_should_be_available(with_locked_keyring: None) -> None:
+def test_locked_keyring_should_not_be_available(with_locked_keyring: None) -> None:
     key_ring = PoetryKeyring("poetry")
 
-    assert key_ring.is_available()
+    assert not key_ring.is_available()
 
 
-def test_erroneous_keyring_should_be_available(with_erroneous_keyring: None) -> None:
+def test_erroneous_keyring_should_not_be_available(
+    with_erroneous_keyring: None,
+) -> None:
     key_ring = PoetryKeyring("poetry")
 
-    assert key_ring.is_available()
+    assert not key_ring.is_available()
 
 
 def test_get_http_auth_from_environment_variables(