diff --git a/CHANGELOG.md b/CHANGELOG.md index 807bdc62..b1a8efb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -432,6 +432,16 @@ archived by series under [docs/changelog/](docs/changelog/); see the secure provider in the UDL and the integration guide, not only on the protocol-state provider: a custom secure store that reports a transient fault as `CorruptedData` loses every sealed protocol-state record. +- **Python `SecureStorage` warns on a host without a secret service.** Its + warning about a backend that cannot hold MLS keys checked only the class name, + and keyring's failing and null backends are both classes named `Keyring`, so a + headless Linux host or container (which gets `keyring.backends.fail.Keyring`) + was never warned. The check now also recognises those modules, and judges the + first backend inside keyring's `ChainerBackend` (the one writes go to) rather + than the chainer itself, which no check matched. The warning names the + backend in full (`keyring.backends.fail.Keyring`) and points a headless host + at the built-in file stores (`store_key`). Log output only; storage behaviour + is unchanged. - **Frames a bridge builds for the core carried a fixed application id.** The iOS, Android and Python bridges stamped `"offline-messenger"` on every relay answer, relay group frame and legacy plain-text DM they rebuilt for the diff --git a/bindings/python/README.md b/bindings/python/README.md index 07a3a303..9250eba6 100644 --- a/bindings/python/README.md +++ b/bindings/python/README.md @@ -167,9 +167,10 @@ MLS cryptographic key material is stored using the `keyring` library: | Linux | Secret Service (GNOME Keyring / KWallet) | | Windows | Windows Credential Locker | -On a host with none of those available, `keyring` falls back to a null or -plaintext backend. `SecureStorage` logs a warning when it detects one, but it -does not refuse to run — and on Python that warning is louder than it looks. +On a host with none of those available, `keyring` falls back to its failing +backend (every call raises), or to a plaintext one if `keyrings.alt` is +installed. `SecureStorage` logs a warning when it detects either, but it +does not refuse to run, and on Python that warning is louder than it looks. The credential store also holds `protocol_state_record_key`, the per-install key that seals pending messages, outbox entries, and media descriptors before they reach `AppStateStorage`. On a plaintext backend that key sits in a readable diff --git a/bindings/python/offline_protocol_sdk/secure_storage.py b/bindings/python/offline_protocol_sdk/secure_storage.py index b50f19d7..583a01ba 100644 --- a/bindings/python/offline_protocol_sdk/secure_storage.py +++ b/bindings/python/offline_protocol_sdk/secure_storage.py @@ -14,6 +14,7 @@ import threading import keyring +import keyring.backends.chainer from . import legacy_store_adoption from .offline_protocol import MlsStorageError, MlsStorageProvider @@ -21,6 +22,9 @@ logger = logging.getLogger(__name__) +#: keyring backends that cannot hold a secret: every call fails, or nothing is kept. +_INSECURE_KEYRING_MODULES = frozenset({"keyring.backends.fail", "keyring.backends.null"}) + # Base service name used for keyring entries _DEFAULT_SERVICE = "offline-protocol-mls-v2" @@ -96,16 +100,43 @@ def __init__( "ProtocolManager build the provider." ) - # Warn if keyring resolved to a plaintext or null backend — MLS key - # material would be stored unprotected. + # Warn if keyring resolved to a failing, null or plaintext backend: MLS + # key material would not be stored, or not stored securely. keyring's + # failing and null backends are both classes named plain ``Keyring`` + # (in ``keyring.backends.fail`` and ``keyring.backends.null``), so the + # module is checked as well as the class name; the failing one is what a + # host without a secret service gets. When two or more backends are + # viable, keyring hands back a ``ChainerBackend`` instead, and writes go + # to the first backend it chains, so that is the backend judged. (A host + # whose only viable backend is ``keyrings.alt``'s plaintext file gets + # that backend directly, not a chain; the unwrap is for a chain that + # happens to rank an insecure backend first.) try: backend = keyring.get_keyring() - backend_name = type(backend).__name__ - if "Fail" in backend_name or "Null" in backend_name or "PlaintextKeyring" in backend_name: + # A chain with nothing in it stores nothing, like the null backend. + empty_chain = False + if isinstance(backend, keyring.backends.chainer.ChainerBackend): + chained = backend.backends + empty_chain = not chained + if chained: + backend = chained[0] + backend_cls = type(backend) + backend_name = f"{backend_cls.__module__}.{backend_cls.__name__}" + if ( + empty_chain + or backend_cls.__module__ in _INSECURE_KEYRING_MODULES + # keyring's own failing and null backends are caught by module + # above; the name markers catch third-party backends. + or "Fail" in backend_cls.__name__ + or "Null" in backend_cls.__name__ + or "PlaintextKeyring" in backend_cls.__name__ + ): logger.warning( - "keyring backend is '%s' — MLS keys will NOT be stored " + "keyring backend is '%s': MLS keys will NOT be stored " "securely. Install a platform secret service (e.g. " - "gnome-keyring, kwallet) for production use.", + "gnome-keyring, kwallet), or on a headless host pass " + "store_key or store_key_env to ProtocolManager to use the " + "built-in file stores.", backend_name, ) except Exception: diff --git a/bindings/python/tests/test_secure_storage_backend_warning.py b/bindings/python/tests/test_secure_storage_backend_warning.py new file mode 100644 index 00000000..9bea461b --- /dev/null +++ b/bindings/python/tests/test_secure_storage_backend_warning.py @@ -0,0 +1,114 @@ +"""SecureStorage warns when keyring resolved to a backend that cannot hold secrets. + +On a host without a secret service (a headless Linux server, a container), the +keyring library selects ``keyring.backends.fail.Keyring``, whose every call +raises. Both it and ``keyring.backends.null.Keyring`` are classes named just +``Keyring``, so a check on the class name alone never recognised the case the +warning exists for. With two or more viable backends keyring returns a +``ChainerBackend`` instead, and the backend that matters is the first one it +chains, since writes go there. (A host whose only viable backend is +``keyrings.alt``'s plaintext file gets that backend directly; the chain cases +here are defensive.) +""" + +from __future__ import annotations + +import logging + +import keyring +import keyring.backends.chainer +import keyring.backends.fail +import keyring.backends.null +import pytest + +from offline_protocol_sdk.secure_storage import SecureStorage +from offline_protocol_sdk.storage_namespace import account_storage_namespace + +NAMESPACE = account_storage_namespace("backend-warning-test", "p") + + +def _plaintext_backend(): + # keyrings.alt is not a dependency; its file backend is a class of this name. + return type("PlaintextKeyring", (keyring.backends.null.Keyring,), {"__module__": "keyrings.alt.file"})() + + +def _named(name): + # A third-party backend: only the class name marks it as insecure. + return lambda: type(name, (keyring.backends.null.Keyring,), {"__module__": "keyrings.vendor"})() + + +def _platform_backend(): + # A real secret service backend: class name and module name carry no marker. + return type("Keyring", (keyring.backends.null.Keyring,), {"__module__": "keyring.backends.macOS"})() + + +def _chain(*backends): + # ``backends`` is a classproperty that discovers what is installed; pin it. + return type("ChainerBackend", (keyring.backends.chainer.ChainerBackend,), + {"__module__": "keyring.backends.chainer", "backends": list(backends)})() + + +def _plaintext_chain(): + return _chain(_plaintext_backend(), keyring.backends.null.Keyring()) + + +def _platform_chain(): + return _chain(_platform_backend(), _plaintext_backend()) + + +def _qualified(instance) -> str: + cls = type(instance) + return f"{cls.__module__}.{cls.__name__}" + + +@pytest.fixture +def backend(monkeypatch: pytest.MonkeyPatch): + def use(instance): + monkeypatch.setattr(keyring, "get_keyring", lambda: instance) + return instance + + return use + + +def _warnings(caplog) -> list[str]: + return [r.getMessage() for r in caplog.records + if r.levelno >= logging.WARNING and "will NOT be stored securely" in r.getMessage()] + + +@pytest.mark.parametrize("make", [keyring.backends.fail.Keyring, keyring.backends.null.Keyring, + _plaintext_backend, _named("FailKeyring"), _named("NullKeyring")], + ids=["fail", "null", "plaintext", "third_party_fail", "third_party_null"]) +def test_insecure_backend_is_warned_about(backend, caplog, make): + instance = backend(make()) + with caplog.at_level(logging.WARNING, logger="offline_protocol_sdk.secure_storage"): + SecureStorage(namespace=NAMESPACE, adopt_legacy_store=False) + messages = _warnings(caplog) + assert len(messages) == 1, caplog.text + assert f"'{_qualified(instance)}'" in messages[0] + assert "store_key" in messages[0] + + +def test_chain_led_by_an_insecure_backend_is_warned_about(backend, caplog): + chain = backend(_plaintext_chain()) + with caplog.at_level(logging.WARNING, logger="offline_protocol_sdk.secure_storage"): + SecureStorage(namespace=NAMESPACE, adopt_legacy_store=False) + messages = _warnings(caplog) + assert len(messages) == 1, caplog.text + assert f"'{_qualified(chain.backends[0])}'" in messages[0] + + +def test_empty_chain_is_warned_about(backend, caplog): + backend(_chain()) + with caplog.at_level(logging.WARNING, logger="offline_protocol_sdk.secure_storage"): + SecureStorage(namespace=NAMESPACE, adopt_legacy_store=False) + messages = _warnings(caplog) + assert len(messages) == 1, caplog.text + assert "'keyring.backends.chainer.ChainerBackend'" in messages[0] + + +@pytest.mark.parametrize("make", [_platform_backend, _platform_chain], ids=["platform", "chain"]) +def test_platform_backend_is_not_warned_about(backend, caplog, make): + backend(make()) + with caplog.at_level(logging.WARNING, logger="offline_protocol_sdk.secure_storage"): + SecureStorage(namespace=NAMESPACE, adopt_legacy_store=False) + assert _warnings(caplog) == []