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 the application 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/application.py

@@ -17,10 +17,12 @@
 from cleo.formatters.style import Style
 
 from poetry.__version__ import __version__
+from poetry.config.config import Config
 from poetry.console.command_loader import CommandLoader
 from poetry.console.commands.command import Command
 from poetry.utils.helpers import directory
 from poetry.utils.helpers import ensure_path
+from poetry.utils.password_manager import PoetryKeyring
 
 
 if TYPE_CHECKING:
@@ -218,6 +220,26 @@ def create_io(
 
         return io
 
+    @staticmethod
+    def preflight(io: IO) -> None:
+        config = Config.create()
+        if config.get("keyring.enabled"):
+            if io.is_verbose():
+                io.write("Checking keyring availability ... ")
+
+            keyring_available = PoetryKeyring.is_available()
+            if io.is_verbose():
+                if keyring_available:
+                    io.write(
+                        "<fg=green;options=bold>Available</>",
+                    )
+                else:
+                    io.write(
+                        "<fg=yellow;options=bold>Unavailable</>",
+                    )
+
+                io.write_line("")
+
     def _run(self, io: IO) -> int:
         # we do this here and not inside the _configure_io implementation in order
         # to ensure the users are not exposed to a stack trace for providing invalid values to
@@ -228,6 +250,7 @@ def _run(self, io: IO) -> int:
         self._load_plugins(io)
 
         with directory(self._working_directory):
+            self.preflight(io)
             exit_code: int = super()._run(io)
 
         return exit_code

src/poetry/utils/password_manager.py

@@ -7,9 +7,11 @@
 from contextlib import suppress
 from typing import TYPE_CHECKING
 
+from poetry.utils.threading import atomic_cached_property
+
 
 if TYPE_CHECKING:
-    from keyring.backend import KeyringBackend
+    import keyring.backend
 
     from poetry.config.config import Config
 
@@ -62,9 +64,9 @@ def get_password(self, name: str, username: str) -> str | None:
 
         try:
             return keyring.get_password(name, username)
-        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:
@@ -96,20 +98,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")
@@ -130,20 +134,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

@@ -42,6 +42,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
 
@@ -193,29 +194,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()
@@ -226,7 +227,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()
@@ -239,7 +240,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
@@ -582,3 +583,8 @@ def project_context(project: str | Path, in_place: bool = False) -> Iterator[Pat
             yield path
 
     return project_context
+
+
+@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,6 +18,7 @@
 
 from poetry.utils.authenticator import Authenticator
 from poetry.utils.authenticator import RepositoryCertificateConfig
+from poetry.utils.password_manager import PoetryKeyring
 
 
 if TYPE_CHECKING:
@@ -95,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

@@ -286,16 +286,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(