From d8f172d13de48cc39553b397862f9a79e8c78e2d Mon Sep 17 00:00:00 2001 From: kivtxs <131401183+kivtxs@users.noreply.github.com> Date: Sun, 27 Sep 2026 16:40:27 -0400 Subject: [PATCH 1/3] fix(bindings): warn when Python SecureStorage gets keyring's failing or null backend SecureStorage warns when keyring resolved to a backend that cannot hold MLS keys, but checked only the class name. keyring's failing and null backends are both classes named Keyring (keyring.backends.fail, keyring.backends.null), so the case the warning exists for, a host without a secret service, never matched. Also check the backend's module, and name the backend in full in the warning. The existing class-name checks are kept. Log output only. --- CHANGELOG.md | 7 ++ .../offline_protocol_sdk/secure_storage.py | 24 +++++-- .../test_secure_storage_backend_warning.py | 65 +++++++++++++++++++ 3 files changed, 90 insertions(+), 6 deletions(-) create mode 100644 bindings/python/tests/test_secure_storage_backend_warning.py diff --git a/CHANGELOG.md b/CHANGELOG.md index de16a1bf7..b0cb849f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,13 @@ 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 the warning + names the backend in full (`keyring.backends.fail.Keyring`). 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/offline_protocol_sdk/secure_storage.py b/bindings/python/offline_protocol_sdk/secure_storage.py index b50f19d77..bff6af55e 100644 --- a/bindings/python/offline_protocol_sdk/secure_storage.py +++ b/bindings/python/offline_protocol_sdk/secure_storage.py @@ -21,6 +21,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,14 +99,23 @@ 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. 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: + backend_cls = type(keyring.get_keyring()) + backend_name = f"{backend_cls.__module__}.{backend_cls.__name__}" + if ( + backend_cls.__module__ in _INSECURE_KEYRING_MODULES + 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.", backend_name, 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 000000000..c22eff764 --- /dev/null +++ b/bindings/python/tests/test_secure_storage_backend_warning.py @@ -0,0 +1,65 @@ +"""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. +""" + +from __future__ import annotations + +import logging + +import keyring +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 _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"})() + + +@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], + ids=["fail", "null", "plaintext"]) +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 type(instance).__name__ in messages[0] + + +def test_platform_backend_is_not_warned_about(backend, caplog): + backend(_platform_backend()) + with caplog.at_level(logging.WARNING, logger="offline_protocol_sdk.secure_storage"): + SecureStorage(namespace=NAMESPACE, adopt_legacy_store=False) + assert _warnings(caplog) == [] From affd834b56cd6d8374cd77ab8d79f28659970f0f Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 1 Oct 2026 13:13:52 +0530 Subject: [PATCH 2/3] fix(bindings): see through keyring's chainer in the backend warning The previous commit taught the insecure-backend warning that keyring's failing and null backends are both classes named plain `Keyring`. It still judged whatever get_keyring() handed back by name, and that is not always the backend that holds the keys. It turns out that once two or more backends have a positive priority, keyring returns a ChainerBackend at priority 10 and lets it delegate. That is exactly what a headless host gets after the usual workaround, `pip install keyrings.alt`: a chain led by a plaintext backend, whose class name matches nothing. No warning, plaintext keys. The very case the warning exists for, again. So unwrap the chain and judge its first backend, the one writes reach. A chain with nothing in it stores nothing, so it warns too. While at it, the warning only told people to install gnome-keyring or kwallet. A server or container has a better answer now, the built-in file stores behind `store_key`, so say that. The README claimed keyring falls back to a null or plaintext backend; it falls back to the failing one. And the tests only checked that "Keyring" appeared in the message, which a warning that dropped the module also passes. They now pin the qualified name. --- CHANGELOG.md | 7 ++- bindings/python/README.md | 7 +-- .../offline_protocol_sdk/secure_storage.py | 23 +++++++-- .../test_secure_storage_backend_warning.py | 50 +++++++++++++++++-- 4 files changed, 74 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 365a79ed1..53fd8355a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -436,8 +436,11 @@ archived by series under [docs/changelog/](docs/changelog/); see the 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 the warning - names the backend in full (`keyring.backends.fail.Keyring`). Log output only; + was never warned. The check now also recognises those modules, and judges the + first backend inside keyring's `ChainerBackend`, which is what a host gets with + `keyrings.alt` installed and which hid a plaintext backend the same way. 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 diff --git a/bindings/python/README.md b/bindings/python/README.md index 07a3a3034..9250eba60 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 bff6af55e..f66ade52f 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 @@ -104,12 +105,24 @@ def __init__( # 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. + # host without a secret service gets. When two or more backends are + # viable, keyring hands back a ``ChainerBackend`` instead, which is how a + # plaintext backend from ``keyrings.alt`` usually arrives on such a host, + # so the backend judged is the first one it chains: the one writes reach. try: - backend_cls = type(keyring.get_keyring()) + backend = keyring.get_keyring() + # 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 ( - backend_cls.__module__ in _INSECURE_KEYRING_MODULES + empty_chain + or backend_cls.__module__ in _INSECURE_KEYRING_MODULES or "Fail" in backend_cls.__name__ or "Null" in backend_cls.__name__ or "PlaintextKeyring" in backend_cls.__name__ @@ -117,7 +130,9 @@ def __init__( logger.warning( "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 index c22eff764..bece8e368 100644 --- a/bindings/python/tests/test_secure_storage_backend_warning.py +++ b/bindings/python/tests/test_secure_storage_backend_warning.py @@ -4,7 +4,9 @@ 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. +warning exists for. With two or more viable backends (``keyrings.alt`` +installed on such a host, typically), keyring returns a ``ChainerBackend`` +instead, and the backend that matters is the first one it chains. """ from __future__ import annotations @@ -12,6 +14,7 @@ import logging import keyring +import keyring.backends.chainer import keyring.backends.fail import keyring.backends.null import pytest @@ -32,6 +35,25 @@ def _platform_backend(): 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): @@ -55,11 +77,31 @@ def test_insecure_backend_is_warned_about(backend, caplog, make): SecureStorage(namespace=NAMESPACE, adopt_legacy_store=False) messages = _warnings(caplog) assert len(messages) == 1, caplog.text - assert type(instance).__name__ in messages[0] + 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] -def test_platform_backend_is_not_warned_about(backend, caplog): - backend(_platform_backend()) +@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) == [] From 9db17840565e6148a31261cb70c49e0f49653ae5 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 1 Oct 2026 13:47:13 +0530 Subject: [PATCH 3/3] fix(bindings): stop claiming keyrings.alt arrives through the chainer The last round explained the ChainerBackend unwrap by saying that a headless host with keyrings.alt gets a chainer, which hid the plaintext backend. It doesn't. The chainer's priority is 10 only when two or more backends rank above zero, and -1 otherwise, so a host whose only viable backend is keyrings.alt's plaintext file gets PlaintextKeyring *directly*. The class-name check already caught that before this PR. With pycryptodome installed the chain leads with EncryptedKeyring, which is correctly not warned. Probed against keyring 25.7.0 and keyrings.alt 5.0.2 with the platform backend made unviable. The unwrap is still right, writes go to the first chained backend, but it is defensive, and the code comment, the changelog and the test docstring now say so instead of inventing a common case. While at it, the "Fail" and "Null" class-name markers no longer catch anything of keyring's own (the module check does), so they only matter for third-party backends, and nothing pinned them: deleting either one left the suite green. Say what they are for and add a third-party case for each. --- CHANGELOG.md | 10 +++++----- .../offline_protocol_sdk/secure_storage.py | 10 +++++++--- .../test_secure_storage_backend_warning.py | 17 ++++++++++++----- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53fd8355a..b1a8efb16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -437,11 +437,11 @@ archived by series under [docs/changelog/](docs/changelog/); see the 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`, which is what a host gets with - `keyrings.alt` installed and which hid a plaintext backend the same way. 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. + 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/offline_protocol_sdk/secure_storage.py b/bindings/python/offline_protocol_sdk/secure_storage.py index f66ade52f..583a01bae 100644 --- a/bindings/python/offline_protocol_sdk/secure_storage.py +++ b/bindings/python/offline_protocol_sdk/secure_storage.py @@ -106,9 +106,11 @@ def __init__( # (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, which is how a - # plaintext backend from ``keyrings.alt`` usually arrives on such a host, - # so the backend judged is the first one it chains: the one writes reach. + # 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() # A chain with nothing in it stores nothing, like the null backend. @@ -123,6 +125,8 @@ def __init__( 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__ diff --git a/bindings/python/tests/test_secure_storage_backend_warning.py b/bindings/python/tests/test_secure_storage_backend_warning.py index bece8e368..9bea461bd 100644 --- a/bindings/python/tests/test_secure_storage_backend_warning.py +++ b/bindings/python/tests/test_secure_storage_backend_warning.py @@ -4,9 +4,11 @@ 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 (``keyrings.alt`` -installed on such a host, typically), keyring returns a ``ChainerBackend`` -instead, and the backend that matters is the first one it chains. +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 @@ -30,6 +32,11 @@ def _plaintext_backend(): 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"})() @@ -69,8 +76,8 @@ def _warnings(caplog) -> list[str]: @pytest.mark.parametrize("make", [keyring.backends.fail.Keyring, keyring.backends.null.Keyring, - _plaintext_backend], - ids=["fail", "null", "plaintext"]) + _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"):