fix(bindings): warn when Python SecureStorage gets keyring's failing or null backend - #469
Merged
Merged
Conversation
…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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SecureStoragewarns when keyring has resolved to a backend that cannot hold MLS keys. However, the check looked only at the class name. keyring's failing backend (keyring.backends.fail.Keyring) and null backend (keyring.backends.null.Keyring) are both classes named plainKeyring, so neither matched. A headless Linux host or container without a secret service gets the failing backend, which is exactly the case the warning exists for. On such a host, no warning was logged.This PR:
keyring.backends.failandkeyring.backends.null;keyring.backends.fail.Keyring);Fail,Null,PlaintextKeyring), which now matter for third-party backends;ChainerBackend(the one writes go to) rather than the chainer itself, and warns about an empty chain. This is defensive: a host whose only viable backend iskeyrings.alt's plaintext file getsPlaintextKeyringdirectly, not a chain;store_key/store_key_env).Before, on a host whose keyring resolves to the failing backend: no warning.
After: one warning per
SecureStorageconstruction:Related issues
None.
Type of change
fix: bug fixChecklist
fix(bindings): ...)CHANGELOG.mdupdated (under Fixed)cargo fmt,cargo clippy,cargo testandcargo-denyare unaffected; CI runs them.unsafeValidation completed before opening
bindings/python/tests/test_secure_storage_backend_warning.py, 9 cases:FailKeyring/NullKeyring, are each warned about exactly once, and the message names the backend in full and thestore_keyremedy.main: the failing and null cases fail (no warning). The plaintext and platform cases pass.Failmarker,Nullmarker) fails at least one test.SecureStorage. The warning is logged and nameskeyring.backends.fail.Keyring.Breaking changes
None. This changes log output only:
Notes for reviewers
keyring.get_keyring()with instances of keyring's ownfailandnullbackends. The plaintext and platform backends are stand-ins built from a class name and module, becausekeyrings.altis not a dependency.