From c5bb14a48e07fc77928d84d9440e713ffca07a41 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 10:22:02 +1100 Subject: [PATCH 01/10] fix(config): refuse an invalid assignment to the loaded settings and write nothing CachekitConfig took assignment unvalidated, so assigning the current key into previous_master_keys (or a fourth key, or a malformed one) after load skipped every keyring rule. validate_assignment alone is not enough: pydantic writes the value before the model validator runs, so a refused keyring assignment stays on the instance. __setattr__ now validates the assignment on a copy, under a lock so two assignments cannot combine unchecked, and writes only if the copy passes. The refusal is redacted and the assigned value is dropped from the frame in a finally. Also corrects comments and docs that said settings take assignments unvalidated, and states that a built encryption wrapper keeps its keyring. --- docs/configuration.md | 2 +- docs/features/zero-knowledge-encryption.md | 11 ++- src/cachekit/cache_handler.py | 14 +-- src/cachekit/config/settings.py | 29 +++++- .../serializers/encryption_wrapper.py | 14 +-- tests/unit/config/test_redacting_settings.py | 34 +++++++ .../test_encryption_master_key_vectors.py | 5 +- tests/unit/test_key_rotation_keyring.py | 98 ++++++++++++++++--- 8 files changed, 174 insertions(+), 33 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 3d231534..32b65e07 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -87,7 +87,7 @@ CACHEKIT_MASTER_KEY=<64-hex-char-key; use exactly 32 bytes> # same per-key requirements as CACHEKIT_MASTER_KEY). Entries written under a # listed key stay readable through the rotation window; writes always use # CACHEKIT_MASTER_KEY. More than 3 keys, or the current master key re-appearing -# in this list, is rejected at load (a master_key= argument: when its cache is built). +# in this list, is rejected at load (a master_key= on an encrypting cache: when it is built). CACHEKIT_PREVIOUS_MASTER_KEYS=, # Fail closed on decrypt authentication failures (default: false = fail open/recompute). # When true, AES-GCM auth failures and key-fingerprint mismatches raise diff --git a/docs/features/zero-knowledge-encryption.md b/docs/features/zero-knowledge-encryption.md index 35c3f845..1fb6d412 100644 --- a/docs/features/zero-knowledge-encryption.md +++ b/docs/features/zero-knowledge-encryption.md @@ -406,7 +406,7 @@ export CACHEKIT_MASTER_KEY= export CACHEKIT_PREVIOUS_MASTER_KEYS= ``` -Rules enforced at config load — rejected, never truncated or silently fixed: +Rules enforced at config load or cache build — rejected, never truncated or silently fixed: - **Cap**: at most 3 decrypt-only keys. - **Per-key validation**: identical to `CACHEKIT_MASTER_KEY` (hex-encoded, at least 32 bytes; use exactly 32). @@ -680,9 +680,12 @@ config = EncryptionConfig(enabled=True, master_key=secret_key, raises `KeyringConfigurationError` (a `ValueError` subclass, exported from `cachekit.serializers`) when the decrypt-only keyring is unusable: a previous master key passed directly that is not exactly 32 bytes, more than three previous keys, or the current -key repeated among them. Settings check all three at load, and an encrypting cache checks its -current key against the previous keys when it is built, so behind the decorators this surfaces -only when settings are assigned after that, or on a config-drift read (below). Outside config-drift reads, the fault never +key repeated among them. Settings check all three at load and refuse an assignment that breaks +them, leaving the settings as they were, and an encrypting cache checks its current key against +the previous keys when it is built, so behind the decorators this surfaces only on a config-drift +read (below). A cache that has already built its encryption wrapper keeps the keyring it built: +a key change reaches only caches built after it, and a running process applies it on restart. +Outside config-drift reads, the fault never evicts and is not counted on `cachekit_decrypt_failures_total`. Direct `EncryptionWrapper` users and callers of the `CacheOperationHandler` read and write methods receive it in both fail modes. Behind the `@cache` decorators, a read of an existing encrypted entry raises it too, from L1 or L2 and diff --git a/src/cachekit/cache_handler.py b/src/cachekit/cache_handler.py index bc15c398..7fff596a 100644 --- a/src/cachekit/cache_handler.py +++ b/src/cachekit/cache_handler.py @@ -698,8 +698,9 @@ def __init__( None: no prefix, for a handler with no backend behind it. Raises: - ConfigurationError: If encryption config is invalid (missing mode or both modes), or a - master key is present while encryption is None. + ConfigurationError: If encryption config is invalid (missing mode or both modes), a + master key is present while encryption is None, or an encrypting handler's current key + (master_key= or CACHEKIT_MASTER_KEY) is also among the previous master keys. TypeError: If serializer_name is not a string or SerializerProtocol instance, or master_key is bytes (pass ``key.hex()``). @@ -774,11 +775,10 @@ def __init__( encryption = False # The keyring's forward-only rule, refused here at decoration before any backend call, whichever route the - # current key took. Settings run the same check at load, but never see master_key= and take later assignments - # unvalidated; without this, the native keyring refuses only when the first read or write builds it. An - # encryption-disabled handler never encrypts, so it spends no nonce budget: its keyring is built only for a - # config-drift read, whose fault is a miss by design. Nor does it load settings here: a plaintext handler must - # not fail on a malformed keyring setting it never uses. + # current key took. Settings run the same check at load and on assignment, but never see master_key=; without + # this, the native keyring refuses only when the first read or write builds it. An encryption-disabled handler + # never encrypts, so it spends no nonce budget: its keyring is built only for a config-drift read, whose fault + # is a miss by design. This check loads no settings for a plaintext handler. if encryption: settings = get_settings() try: diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index 85b68ee1..de4034e5 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -17,6 +17,8 @@ from __future__ import annotations +import functools +import threading from typing import Annotated, Any, Literal, Optional from pydantic import ( @@ -25,9 +27,9 @@ field_validator, model_validator, ) -from pydantic_settings import NoDecode, SettingsConfigDict +from pydantic_settings import BaseSettings, NoDecode, SettingsConfigDict -from .validation import RedactingSettings, refuse_current_key_in_previous_keys +from .validation import RedactingSettings, _redacting, refuse_current_key_in_previous_keys # Keyring cap from the protocol spec (spec/encryption.md → "Key Rotation (Keyring)"): # at most 3 decrypt-only previous keys. Exceeding the cap is a configuration error, @@ -35,6 +37,11 @@ # MAX_DECRYPT_ONLY_KEYS, which re-validates behind the FFI boundary. MAX_PREVIOUS_MASTER_KEYS = 3 +# Serializes CachekitConfig assignments: each validates the whole state it would leave, so two +# assignments that are each valid alone (a new master_key, and that key added to previous_master_keys) +# cannot land together unchecked. +_ASSIGNMENT_LOCK = threading.Lock() + class CachekitConfig(RedactingSettings): """Backend-agnostic cache configuration. @@ -122,6 +129,8 @@ class CachekitConfig(RedactingSettings): # logs. errors()/json() ignore this flag; RedactingSettings sanitizes # those surfaces. hide_input_in_errors=True, + # Assignment after load runs every field and model validator; __setattr__ makes a refusal write nothing. + validate_assignment=True, ) # Generic cache configuration (backend-agnostic) @@ -277,6 +286,22 @@ def validate_previous_master_keys(self) -> CachekitConfig: return self + def __setattr__(self, name: str, value: Any) -> None: + """Validate an assignment on a copy first, and write it only if the copy passes. + + validate_assignment alone writes the new value before the model validator runs, so a refused keyring + assignment would stay on the instance, readable by other threads until the error surfaces. The value + is dropped in a finally: the raised error's traceback holds this frame (CWE-532). + """ + candidate = None + try: + with _ASSIGNMENT_LOCK: + candidate = self.model_copy() + _redacting(functools.partial(BaseSettings.__setattr__, candidate, name, value), type(self).__name__) + super().__setattr__(name, value) + finally: + del value, candidate # the copy holds the refused value + def __repr__(self) -> str: """Return string representation with sensitive information masked. diff --git a/src/cachekit/serializers/encryption_wrapper.py b/src/cachekit/serializers/encryption_wrapper.py index a997320e..a9970d9e 100644 --- a/src/cachekit/serializers/encryption_wrapper.py +++ b/src/cachekit/serializers/encryption_wrapper.py @@ -311,10 +311,12 @@ def _setup_encryption(self, master_key: SecretBytes | None, previous_master_keys # Decrypt-only previous keys from settings if not provided (key rotation, # spec/encryption.md → "Key Rotation (Keyring)"). Settings enforce the cap # of 3, per-key hex/length validation, and the forward-only subset check - # at load, and CacheSerializationHandler repeats the subset check for an - # encrypting cache's master_key= when it is built; the Rust Keyring - # re-validates all three behind the FFI boundary for wrappers constructed - # with explicit parameters and for settings assigned after those checks. + # at load and on every assignment, and CacheSerializationHandler repeats the + # subset check for an encrypting cache's master_key= when it is built; the + # Rust Keyring re-validates all three behind the FFI boundary for wrappers + # constructed with explicit parameters, and as defence in depth for keys + # read from settings. This wrapper keeps the keyring it builds: a later + # change to the settings reaches only wrappers built after it. # Keyring config errors below raise KeyringConfigurationError, NEVER # EncryptionError: EncryptionError is a SerializationError, which the # read-path policy (handle_decrypt_failure) classifies as corruption → @@ -331,8 +333,8 @@ def _setup_encryption(self, master_key: SecretBytes | None, previous_master_keys settings = get_settings() previous_master_keys = [SecretBytes(bytes.fromhex(key.get_secret_value())) for key in settings.previous_master_keys] - # The hex rule again, for keys read from settings: settings validate them at load, but the settings - # object takes later assignments unvalidated, and the Rust Keyring below checks only 16 bytes. + # The hex rule again, for keys read from settings: settings validate them at load and on assignment, and + # this stays as defence in depth because the Rust Keyring below checks only 16 bytes. for position, previous_key in enumerate(previous_master_keys): if len(previous_key) < 32: raise KeyringConfigurationError( diff --git a/tests/unit/config/test_redacting_settings.py b/tests/unit/config/test_redacting_settings.py index 2f3d9bd7..6dcd3794 100644 --- a/tests/unit/config/test_redacting_settings.py +++ b/tests/unit/config/test_redacting_settings.py @@ -42,6 +42,14 @@ from cachekit.serializers.encryption_wrapper import EncryptionError, EncryptionWrapper _KEY_HEX = "ab" * 32 +# No two 16-character windows of it are alike, so a truncated rendering of it is still found. +_DISTINCT_KEY_HEX = bytes(range(32)).hex() + +# Post-load assignments to the settings that are refused: one per field, one by the model validator. +_SETTINGS_ASSIGNMENT_REFUSALS: dict[str, tuple[str, object]] = { + "field-level": ("previous_master_keys", {_DISTINCT_KEY_HEX: 1}), + "model-level": ("previous_master_keys", [_DISTINCT_KEY_HEX, "01" * 32, "02" * 32, "03" * 32]), +} BACKEND_CONFIGS: list[type[BaseBackendConfig]] = [ RedisBackendConfig, @@ -633,6 +641,23 @@ def _cachekit_locals_holding(exc: BaseException, secret: str | bytes, *, below_c return found +@pytest.mark.unit +class TestSettingsAssignmentRedaction: + """A refused assignment to the loaded settings carries no route to the key it was given, whole or truncated.""" + + @pytest.mark.parametrize( + ("field", "value"), _SETTINGS_ASSIGNMENT_REFUSALS.values(), ids=_SETTINGS_ASSIGNMENT_REFUSALS.keys() + ) + def test_refused_assignment_redacts_the_key(self, field: str, value: object) -> None: + with pytest.raises(ValidationError) as exc_info: + setattr(singleton.get_settings(), field, value) + + _assert_no_route_to(exc_info.value, _DISTINCT_KEY_HEX) + rendered = str(exc_info.value) + repr(exc_info.value.errors()) + exc_info.value.json() + fragments = {_DISTINCT_KEY_HEX[i : i + 16] for i in range(len(_DISTINCT_KEY_HEX) - 15)} + assert [fragment for fragment in fragments if fragment in rendered] == [] + + @pytest.mark.unit class TestRedactingSettingsFrameLocals: """No cachekit frame on a raised config error's traceback keeps the raw input (CWE-532).""" @@ -1073,6 +1098,15 @@ def _api_key_rows(form: Callable[[str], Any]) -> dict[str, _EntryPointRow]: ConfigurationError, _SHORT_KEY_HEX, ), + **{ + f"settings-assignment-{name}": ( + {}, + lambda field=field, value=value: setattr(singleton.get_settings(), field, value), + ValidationError, + _DISTINCT_KEY_HEX, + ) + for name, (field, value) in _SETTINGS_ASSIGNMENT_REFUSALS.items() + }, } diff --git a/tests/unit/protocol/test_encryption_master_key_vectors.py b/tests/unit/protocol/test_encryption_master_key_vectors.py index 833ccf2b..e8a50da6 100644 --- a/tests/unit/protocol/test_encryption_master_key_vectors.py +++ b/tests/unit/protocol/test_encryption_master_key_vectors.py @@ -75,7 +75,8 @@ # One refusal each: a non-hex string, or one that decodes short. Anything else (a missing key) fails the match. DECORATOR_REFUSAL = r"CACHEKIT_MASTER_KEY must be (hex-encoded|at least 32 bytes)" WRAPPER_REFUSAL = r"Invalid master key format|Master key must be at least 32 bytes" -# The refusal each keyring reject row gets at decoration, from the settings validator, on both current-key routes. +# The refusal each keyring reject row gets at decoration from the settings validator; _keyring_refusal names the one +# exception, a repeat row on master_key_argument. _SETTINGS_REFUSAL = r"(?s)^1 validation error for CachekitConfig\n.*Value error, " _REPEAT_REFUSAL = "master_key must not appear in previous_master_keys" KEYRING_REFUSALS = { @@ -88,7 +89,7 @@ def _keyring_refusal(name: str, route: str) -> tuple[type[Exception], str]: - """The settings never see a master_key=, so the decorator's handler refuses its repeat, as it does a bad key.""" + """The settings never see a master_key=, so the decorator's handler refuses its repeat when it is built.""" if name in REPEAT_ROWS and route == "master_key_argument": return ConfigurationError, f"^{_REPEAT_REFUSAL}: " return ValidationError, KEYRING_REFUSALS[name] diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index 2c89393d..83c18710 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -152,6 +152,77 @@ def test_validation_errors_never_echo_key_material(self, monkeypatch): monkeypatch.delenv(key) +_REFUSED_ASSIGNMENTS = { + "previous-is-current": ("previous_master_keys", [SecretStr(K2.hex())], "must not appear in previous_master_keys"), + "previous-is-current-uppercase": ( + "previous_master_keys", + [SecretStr(K2.hex().upper())], + "must not appear in previous_master_keys", + ), + "current-is-previous": ("master_key", SecretStr(K1.hex()), "must not appear in previous_master_keys"), + "fourth-previous": ( + "previous_master_keys", + [SecretStr(f"{i:02x}" * 32) for i in range(4, 8)], + f"at most {MAX_PREVIOUS_MASTER_KEYS}", + ), + "previous-not-hex": ("previous_master_keys", [SecretStr("zz" * 32)], "not valid hex"), + "previous-short": ("previous_master_keys", [SecretStr("aa" * 31)], "at least 32 bytes"), +} + + +class TestSettingsAssignment: + """An assignment to the loaded settings runs the load-time rules, and a refused one writes nothing.""" + + @pytest.fixture + def settings(self, monkeypatch) -> CachekitConfig: + monkeypatch.setenv("CACHEKIT_MASTER_KEY", K2.hex()) + monkeypatch.setenv("CACHEKIT_PREVIOUS_MASTER_KEYS", K1.hex()) + reset_settings() + return get_settings() + + @pytest.mark.parametrize(("field", "value", "reason"), _REFUSED_ASSIGNMENTS.values(), ids=_REFUSED_ASSIGNMENTS.keys()) + def test_refused_assignment_leaves_settings_as_they_were(self, settings, field, value, reason): + before = getattr(settings, field) + + with pytest.raises(ValidationError, match=reason): + setattr(settings, field, value) + + assert getattr(settings, field) is before + + @pytest.mark.parametrize("case", ["previous-is-current", "previous-is-current-uppercase", "current-is-previous"]) + def test_refused_value_is_never_written(self, settings, monkeypatch, case): + """The keyring rules run in the model validator, which pydantic's validate_assignment calls only after it has + written the new value. The repeat check is the last of them, so a spy there sees what a concurrent reader of + the live settings would see while they run: the old values.""" + import cachekit.config.settings as settings_module + + field, value, reason = _REFUSED_ASSIGNMENTS[case] + before = (settings.master_key, settings.previous_master_keys) + seen: list[tuple[Any, Any]] = [] + refuse = settings_module.refuse_current_key_in_previous_keys + + def spy(*args: Any) -> None: + seen.append((settings.master_key, settings.previous_master_keys)) + refuse(*args) + + monkeypatch.setattr(settings_module, "refuse_current_key_in_previous_keys", spy) + + with pytest.raises(ValidationError, match=reason): + setattr(settings, field, value) + + assert len(seen) == 1 + assert seen[0][0] is before[0] + assert seen[0][1] is before[1] + + def test_valid_assignment_lands_validated(self, settings): + settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] + settings.master_key = K1.hex() # type: ignore[assignment] + + assert settings.previous_master_keys == [SecretStr(K3.hex())] + assert isinstance(settings.master_key, SecretStr) + assert settings.master_key.get_secret_value() == K1.hex() + + class _CallRecordingBackend: """Backend that records every call, to prove a refusal came before any.""" @@ -220,7 +291,8 @@ def test_refused_at_decoration(self, monkeypatch, decorate, previous): assert backend.calls == [] def test_reads_previous_keys_as_they_stand_at_decoration(self, monkeypatch): - """Settings validate no assignment, so a check made only at load would miss this one.""" + """Settings hold no current key here, so they accept this assignment; only a check made when the cache is + built sees it repeat the master_key= argument.""" backend = _CallRecordingBackend() cache.secure(backend=backend, master_key=KA.hex())(lambda: None) # accepted: no previous keys yet @@ -229,18 +301,22 @@ def test_reads_previous_keys_as_they_stand_at_decoration(self, monkeypatch): cache.secure(backend=backend, master_key=KA.hex())(lambda: None) assert backend.calls == [] - def test_reads_an_assigned_settings_master_key(self, monkeypatch): - """The same holds for the current key itself, when it comes from settings assigned after load.""" - monkeypatch.setenv("CACHEKIT_PREVIOUS_MASTER_KEYS", KA.hex()) - reset_settings() - monkeypatch.setattr(get_settings(), "master_key", SecretStr(KA.hex())) + def test_reads_the_settings_master_key_as_it_stands_at_build(self, monkeypatch): + """The same holds for the current key itself, when it comes from settings. Settings refuse the repeat at load + and on assignment, so unvalidated settings stand in for it: the handler's check stays as defence in depth.""" + import cachekit.cache_handler as cache_handler + + unvalidated = CachekitConfig.model_construct( + master_key=SecretStr(KA.hex()), previous_master_keys=[SecretStr(KA.hex())], encryption_fail_closed=False + ) + monkeypatch.setattr(cache_handler, "get_settings", lambda: unvalidated) with pytest.raises(ConfigurationError, match=REPEAT_REFUSAL): CacheSerializationHandler(encryption=True, single_tenant_mode=True) - def test_plaintext_handler_reads_no_settings(self, monkeypatch): - """A handler that neither encrypts nor defers fail-closed to settings never loads them, so a malformed - keyring setting cannot stop it being built.""" + def test_plaintext_handler_construction_reads_no_settings(self, monkeypatch): + """Building a handler that neither encrypts nor defers fail-closed to settings never loads them, so a malformed + keyring setting cannot stop it being built. Its later operations may still read settings.""" monkeypatch.setenv("CACHEKIT_PREVIOUS_MASTER_KEYS", "not-hex") reset_settings() @@ -575,8 +651,8 @@ def encryption_fingerprints(self, tenant_id: str) -> list[bytes]: def _settings_previous_keys(monkeypatch: pytest.MonkeyPatch, keys: list[bytes]) -> None: - """Hand the wrapper previous keys that skip settings-load validation, as a settings - object assigned to after load does (it validates no assignment). A key passed + """Hand the wrapper previous keys that skip settings validation, which settings refuse at + load and on assignment: the wrapper's own check is defence in depth. A key passed explicitly never gets this far: __init__ holds it to exactly 32 bytes.""" import cachekit.serializers.encryption_wrapper as ew From 17729fb33dd98ebada2dee25c11ebee20d4fcefb Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 10:30:47 +1100 Subject: [PATCH 02/10] fix(config): hold previous_master_keys as a tuple so it cannot be edited in place An in-place edit runs no validator, so appending the current key to the list bypassed the assignment guard. A tuple makes every change an assignment. --- docs/features/zero-knowledge-encryption.md | 3 ++- src/cachekit/config/settings.py | 5 +++-- tests/unit/test_key_rotation_keyring.py | 13 ++++++++++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/docs/features/zero-knowledge-encryption.md b/docs/features/zero-knowledge-encryption.md index 1fb6d412..ef466fd4 100644 --- a/docs/features/zero-knowledge-encryption.md +++ b/docs/features/zero-knowledge-encryption.md @@ -681,7 +681,8 @@ raises `KeyringConfigurationError` (a `ValueError` subclass, exported from `cachekit.serializers`) when the decrypt-only keyring is unusable: a previous master key passed directly that is not exactly 32 bytes, more than three previous keys, or the current key repeated among them. Settings check all three at load and refuse an assignment that breaks -them, leaving the settings as they were, and an encrypting cache checks its current key against +them, leaving the settings as they were (`previous_master_keys` is a tuple, so a change has to +be an assignment), and an encrypting cache checks its current key against the previous keys when it is built, so behind the decorators this surfaces only on a config-drift read (below). A cache that has already built its encryption wrapper keeps the keyring it built: a key change reaches only caches built after it, and a running process applies it on restart. diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index de4034e5..a1f5696e 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -214,8 +214,9 @@ class CachekitConfig(RedactingSettings): default=None, description="Master encryption key (hex-encoded; use exactly 32 bytes, 64 hex characters)", ) - previous_master_keys: Annotated[list[SecretStr], NoDecode] = Field( - default_factory=list, + # A tuple, so the keyring cannot be edited in place, where no validator runs: a change is an assignment. + previous_master_keys: Annotated[tuple[SecretStr, ...], NoDecode] = Field( + default_factory=tuple, description=( "Decrypt-only previous master keys for key rotation (env: " "CACHEKIT_PREVIOUS_MASTER_KEYS, comma-separated hex). Entries written " diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index 83c18710..3133d479 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -76,9 +76,9 @@ def test_env_blank_segments_ignored(self, monkeypatch): monkeypatch.setenv("CACHEKIT_PREVIOUS_MASTER_KEYS", f" {K1.hex()} ,, ") assert len(CachekitConfig().previous_master_keys) == 1 - def test_default_is_empty_list(self, monkeypatch): + def test_default_is_empty(self, monkeypatch): monkeypatch.delenv("CACHEKIT_PREVIOUS_MASTER_KEYS", raising=False) - assert CachekitConfig().previous_master_keys == [] + assert CachekitConfig().previous_master_keys == () def test_more_than_three_keys_raises_never_truncates(self): four = [SecretStr(f"{i:02x}" * 32) for i in range(1, 5)] @@ -214,11 +214,18 @@ def spy(*args: Any) -> None: assert seen[0][0] is before[0] assert seen[0][1] is before[1] + def test_previous_keys_cannot_be_edited_in_place(self, settings): + """No validator runs on an in-place edit, so the keyring takes none: a change has to be an assignment.""" + with pytest.raises(AttributeError): + settings.previous_master_keys.append(SecretStr(K2.hex().upper())) # type: ignore[attr-defined] + + assert settings.previous_master_keys == (SecretStr(K1.hex()),) + def test_valid_assignment_lands_validated(self, settings): settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] settings.master_key = K1.hex() # type: ignore[assignment] - assert settings.previous_master_keys == [SecretStr(K3.hex())] + assert settings.previous_master_keys == (SecretStr(K3.hex()),) assert isinstance(settings.master_key, SecretStr) assert settings.master_key.get_secret_value() == K1.hex() From f37e000e8810a83ea1c96e4fbd2d16f991872143 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 10:52:39 +1100 Subject: [PATCH 03/10] fix(config): store the value the copy validated, and skip the guard for non-fields The guard validated the assigned value on a copy, then passed the raw value to pydantic again. A generator was spent by then, so the live settings stored an empty keyring with no error, and an iterable that changes between passes could store a value the copy never saw. It now stores the copy's validated value. Assignments to names that are not fields bypass the guard, so a validator that sets a private attribute cannot deadlock on its lock. The docs now describe keyring pickup per encryption wrapper, which a cache builds per tenant. --- docs/features/zero-knowledge-encryption.md | 7 ++- src/cachekit/config/settings.py | 10 +++- .../serializers/encryption_wrapper.py | 4 +- tests/unit/config/test_redacting_settings.py | 11 ++++ tests/unit/test_key_rotation_keyring.py | 53 +++++++++++++++++++ 5 files changed, 81 insertions(+), 4 deletions(-) diff --git a/docs/features/zero-knowledge-encryption.md b/docs/features/zero-knowledge-encryption.md index ef466fd4..64cff6fc 100644 --- a/docs/features/zero-knowledge-encryption.md +++ b/docs/features/zero-knowledge-encryption.md @@ -684,8 +684,11 @@ key repeated among them. Settings check all three at load and refuse an assignme them, leaving the settings as they were (`previous_master_keys` is a tuple, so a change has to be an assignment), and an encrypting cache checks its current key against the previous keys when it is built, so behind the decorators this surfaces only on a config-drift -read (below). A cache that has already built its encryption wrapper keeps the keyring it built: -a key change reaches only caches built after it, and a running process applies it on restart. +read (below). Each encryption wrapper keeps the keyring it built. A cache builds one wrapper per +tenant, on that tenant's first use and again after the wrapper is evicted from the cache's LRU, and +each build reads the settings as they stand then. So a key change reaches every wrapper built after +it, and one multi-tenant cache can run the old keyring for some tenants and the new one for others. +Restart the process to apply a key change everywhere at once. Outside config-drift reads, the fault never evicts and is not counted on `cachekit_decrypt_failures_total`. Direct `EncryptionWrapper` users and callers of the `CacheOperationHandler` read and write methods receive it in both fail modes. Behind diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index a1f5696e..c8a885c7 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -294,12 +294,20 @@ def __setattr__(self, name: str, value: Any) -> None: assignment would stay on the instance, readable by other threads until the error surfaces. The value is dropped in a finally: the raised error's traceback holds this frame (CWE-532). """ + # Not a field (a subclass's private attribute, say): nothing to validate, and a validator that sets one + # must not re-enter the lock below. + if name not in type(self).model_fields: + super().__setattr__(name, value) + return candidate = None try: with _ASSIGNMENT_LOCK: candidate = self.model_copy() + # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy only. _redacting(functools.partial(BaseSettings.__setattr__, candidate, name, value), type(self).__name__) - super().__setattr__(name, value) + # The value the copy validated, never `value` again: a generator is spent by now, and an iterable + # may yield something else on a second pass. + super().__setattr__(name, getattr(candidate, name)) finally: del value, candidate # the copy holds the refused value diff --git a/src/cachekit/serializers/encryption_wrapper.py b/src/cachekit/serializers/encryption_wrapper.py index a9970d9e..7b44de29 100644 --- a/src/cachekit/serializers/encryption_wrapper.py +++ b/src/cachekit/serializers/encryption_wrapper.py @@ -316,7 +316,9 @@ def _setup_encryption(self, master_key: SecretBytes | None, previous_master_keys # Rust Keyring re-validates all three behind the FFI boundary for wrappers # constructed with explicit parameters, and as defence in depth for keys # read from settings. This wrapper keeps the keyring it builds: a later - # change to the settings reaches only wrappers built after it. + # change to the settings reaches only wrappers built after it, and + # CacheSerializationHandler builds one per tenant, on first use and after + # LRU eviction. # Keyring config errors below raise KeyringConfigurationError, NEVER # EncryptionError: EncryptionError is a SerializationError, which the # read-path policy (handle_decrypt_failure) classifies as corruption → diff --git a/tests/unit/config/test_redacting_settings.py b/tests/unit/config/test_redacting_settings.py index 6dcd3794..a774af28 100644 --- a/tests/unit/config/test_redacting_settings.py +++ b/tests/unit/config/test_redacting_settings.py @@ -657,6 +657,17 @@ def test_refused_assignment_redacts_the_key(self, field: str, value: object) -> fragments = {_DISTINCT_KEY_HEX[i : i + 16] for i in range(len(_DISTINCT_KEY_HEX) - 15)} assert [fragment for fragment in fragments if fragment in rendered] == [] + @pytest.mark.parametrize( + ("field", "value"), _SETTINGS_ASSIGNMENT_REFUSALS.values(), ids=_SETTINGS_ASSIGNMENT_REFUSALS.keys() + ) + def test_refused_assignment_leaves_no_frame_local(self, field: str, value: object) -> None: + """Pydantic's own assignment frames hold the raw value too, so the guard must re-raise from above them: the + cachekit-only walk of TestEntryPointFrameLocals would pass without that.""" + with pytest.raises(ValidationError) as exc_info: + setattr(singleton.get_settings(), field, value) + + assert _cachekit_locals_holding(exc_info.value, _DISTINCT_KEY_HEX, below_caller=True) == [] + @pytest.mark.unit class TestRedactingSettingsFrameLocals: diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index 3133d479..81c8d662 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -221,6 +221,59 @@ def test_previous_keys_cannot_be_edited_in_place(self, settings): assert settings.previous_master_keys == (SecretStr(K1.hex()),) + @pytest.mark.parametrize( + "make", + [ + pytest.param(lambda keys: (key for key in keys), id="generator"), + pytest.param(lambda keys: map(str, keys), id="map"), + ], + ) + def test_one_shot_iterable_lands_whole(self, settings, make): + """The guard validates the value on a copy, so the live settings must store what the copy validated: a second + pass over a one-shot iterable would find it empty and drop every previous key without an error.""" + settings.previous_master_keys = make([K1.hex(), K3.hex()]) + + assert settings.previous_master_keys == (SecretStr(K1.hex()), SecretStr(K3.hex())) + + def test_iterable_that_changes_between_passes_cannot_land_unchecked(self, settings): + """An iterable that yields a valid keyring once and the current key after must not reach the live settings.""" + passes = iter([[K3.hex()], [K2.hex()]]) + + class _Shifting: + def __iter__(self): + return iter(next(passes)) + + settings.previous_master_keys = _Shifting() # type: ignore[assignment] + + assert settings.previous_master_keys == (SecretStr(K3.hex()),) + + def test_validator_that_sets_a_private_attribute_does_not_deadlock(self, monkeypatch): + """A subclass's validator may set a private attribute while the guard holds its lock. A throwaway lock keeps a + regression from blocking every later assignment in this process.""" + import threading + + from pydantic import PrivateAttr, model_validator + + import cachekit.config.settings as settings_module + + monkeypatch.setattr(settings_module, "_ASSIGNMENT_LOCK", threading.Lock()) + + class _Marking(CachekitConfig): + _validated: bool = PrivateAttr(default=False) + + @model_validator(mode="after") + def _mark(self) -> _Marking: + self._validated = True + return self + + config = _Marking() + assigning = threading.Thread(target=setattr, args=(config, "l1_max_size_mb", 7), daemon=True) + assigning.start() + assigning.join(timeout=5) + + assert not assigning.is_alive() + assert config.l1_max_size_mb == 7 + def test_valid_assignment_lands_validated(self, settings): settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] settings.master_key = K1.hex() # type: ignore[assignment] From 57eb8a12ae4691cf81020ad719fc8cf701e4b729 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 11:04:11 +1100 Subject: [PATCH 04/10] fix(config): let only private attributes skip the assignment guard Every name that was not a field skipped the guard, so a value assigned to a mistyped field name was refused outside the redaction path and kept on the traceback. Only private attributes skip it now; any other name is refused inside the guard. --- src/cachekit/config/settings.py | 6 +++--- tests/unit/config/test_redacting_settings.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index c8a885c7..b3cdb579 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -294,9 +294,9 @@ def __setattr__(self, name: str, value: Any) -> None: assignment would stay on the instance, readable by other threads until the error surfaces. The value is dropped in a finally: the raised error's traceback holds this frame (CWE-532). """ - # Not a field (a subclass's private attribute, say): nothing to validate, and a validator that sets one - # must not re-enter the lock below. - if name not in type(self).model_fields: + # A private attribute: nothing to validate, and a validator that sets one must not re-enter the lock below. + # Any other name goes through the guard, so a mistyped field's value is refused inside _redacting too. + if name in type(self).__private_attributes__: super().__setattr__(name, value) return candidate = None diff --git a/tests/unit/config/test_redacting_settings.py b/tests/unit/config/test_redacting_settings.py index a774af28..0a5432a1 100644 --- a/tests/unit/config/test_redacting_settings.py +++ b/tests/unit/config/test_redacting_settings.py @@ -49,6 +49,7 @@ _SETTINGS_ASSIGNMENT_REFUSALS: dict[str, tuple[str, object]] = { "field-level": ("previous_master_keys", {_DISTINCT_KEY_HEX: 1}), "model-level": ("previous_master_keys", [_DISTINCT_KEY_HEX, "01" * 32, "02" * 32, "03" * 32]), + "mistyped-field": ("previous_master_key", _DISTINCT_KEY_HEX), } BACKEND_CONFIGS: list[type[BaseBackendConfig]] = [ From d7f18662c01e84678c2da5978850d819d4f60aaa Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 11:41:25 +1100 Subject: [PATCH 05/10] fix(config): make the assignment lock reentrant and correct the keyring docs A subclass property setter that assigns a field runs while the guard holds its lock, so the lock is now an RLock. The docs restore the case where a previous key assigned to the settings repeats the master_key= of a cache already built, and say that a cache given master_key= keeps that current key when the settings change. --- docs/features/zero-knowledge-encryption.md | 20 ++++++------ src/cachekit/config/settings.py | 9 +++--- .../serializers/encryption_wrapper.py | 9 ++---- tests/unit/config/test_redacting_settings.py | 3 +- tests/unit/test_key_rotation_keyring.py | 31 +++++++++++++++++-- 5 files changed, 50 insertions(+), 22 deletions(-) diff --git a/docs/features/zero-knowledge-encryption.md b/docs/features/zero-knowledge-encryption.md index 64cff6fc..a963b9b6 100644 --- a/docs/features/zero-knowledge-encryption.md +++ b/docs/features/zero-knowledge-encryption.md @@ -682,15 +682,11 @@ raises `KeyringConfigurationError` (a `ValueError` subclass, exported from passed directly that is not exactly 32 bytes, more than three previous keys, or the current key repeated among them. Settings check all three at load and refuse an assignment that breaks them, leaving the settings as they were (`previous_master_keys` is a tuple, so a change has to -be an assignment), and an encrypting cache checks its current key against -the previous keys when it is built, so behind the decorators this surfaces only on a config-drift -read (below). Each encryption wrapper keeps the keyring it built. A cache builds one wrapper per -tenant, on that tenant's first use and again after the wrapper is evicted from the cache's LRU, and -each build reads the settings as they stand then. So a key change reaches every wrapper built after -it, and one multi-tenant cache can run the old keyring for some tenants and the new one for others. -Restart the process to apply a key change everywhere at once. -Outside config-drift reads, the fault never -evicts and is not counted on `cachekit_decrypt_failures_total`. Direct `EncryptionWrapper` users +be an assignment), and an encrypting cache checks its current key against the previous keys when +it is built. Behind the decorators this surfaces on a config-drift read (below), or when a +previous key assigned to the settings repeats the `master_key=` of a cache already built: the +settings never see that key, so its next wrapper build refuses it. Outside config-drift reads, the +fault never evicts and is not counted on `cachekit_decrypt_failures_total`. Direct `EncryptionWrapper` users and callers of the `CacheOperationHandler` read and write methods receive it in both fail modes. Behind the `@cache` decorators, a read of an existing encrypted entry raises it too, from L1 or L2 and from the re-read after a distributed-lock wait, so the function does not run and no @@ -702,6 +698,12 @@ or one of the wrong length, raises `EncryptionError`, and an encryption-disabled claims encryption treats the fault as corruption (miss + evict), because only the unauthenticated header sent it down the decrypt path. +A wrapper keeps the keyring it built. A cache builds one per tenant, on that tenant's first use and +after LRU eviction, so a key change in the settings reaches only wrappers built after it: their +previous keys, and their current key too unless the cache was given `master_key=`. One multi-tenant +cache can therefore run the old keyring for some tenants and the new one for others. To apply a key +change everywhere at once, change the environment and restart the process. + > **⚠️ Key rotation under fail-closed:** with `fail_closed` enabled there is no > silent self-heal — rotating `CACHEKIT_MASTER_KEY` **without retaining the old key > in `CACHEKIT_PREVIOUS_MASTER_KEYS`** makes every pre-rotation entry raise diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index b3cdb579..5ce82932 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -39,8 +39,9 @@ # Serializes CachekitConfig assignments: each validates the whole state it would leave, so two # assignments that are each valid alone (a new master_key, and that key added to previous_master_keys) -# cannot land together unchecked. -_ASSIGNMENT_LOCK = threading.Lock() +# cannot land together unchecked. Reentrant: a subclass's validator or property setter may assign a field +# while the lock is held. +_ASSIGNMENT_LOCK = threading.RLock() class CachekitConfig(RedactingSettings): @@ -303,10 +304,10 @@ def __setattr__(self, name: str, value: Any) -> None: try: with _ASSIGNMENT_LOCK: candidate = self.model_copy() - # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy only. + # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy. _redacting(functools.partial(BaseSettings.__setattr__, candidate, name, value), type(self).__name__) # The value the copy validated, never `value` again: a generator is spent by now, and an iterable - # may yield something else on a second pass. + # may yield something else on a second pass. Pydantic validates it once more as it writes. super().__setattr__(name, getattr(candidate, name)) finally: del value, candidate # the copy holds the refused value diff --git a/src/cachekit/serializers/encryption_wrapper.py b/src/cachekit/serializers/encryption_wrapper.py index 7b44de29..4e795bf2 100644 --- a/src/cachekit/serializers/encryption_wrapper.py +++ b/src/cachekit/serializers/encryption_wrapper.py @@ -313,12 +313,9 @@ def _setup_encryption(self, master_key: SecretBytes | None, previous_master_keys # of 3, per-key hex/length validation, and the forward-only subset check # at load and on every assignment, and CacheSerializationHandler repeats the # subset check for an encrypting cache's master_key= when it is built; the - # Rust Keyring re-validates all three behind the FFI boundary for wrappers - # constructed with explicit parameters, and as defence in depth for keys - # read from settings. This wrapper keeps the keyring it builds: a later - # change to the settings reaches only wrappers built after it, and - # CacheSerializationHandler builds one per tenant, on first use and after - # LRU eviction. + # Rust Keyring re-validates all three behind the FFI boundary. This wrapper + # keeps the keyring it builds: a later change to the settings reaches only + # wrappers built after it. # Keyring config errors below raise KeyringConfigurationError, NEVER # EncryptionError: EncryptionError is a SerializationError, which the # read-path policy (handle_decrypt_failure) classifies as corruption → diff --git a/tests/unit/config/test_redacting_settings.py b/tests/unit/config/test_redacting_settings.py index 0a5432a1..f1ba18c2 100644 --- a/tests/unit/config/test_redacting_settings.py +++ b/tests/unit/config/test_redacting_settings.py @@ -45,7 +45,8 @@ # No two 16-character windows of it are alike, so a truncated rendering of it is still found. _DISTINCT_KEY_HEX = bytes(range(32)).hex() -# Post-load assignments to the settings that are refused: one per field, one by the model validator. +# Post-load assignments to the settings that are refused: by a field, by the model validator, and to a name +# that is no field. _SETTINGS_ASSIGNMENT_REFUSALS: dict[str, tuple[str, object]] = { "field-level": ("previous_master_keys", {_DISTINCT_KEY_HEX: 1}), "model-level": ("previous_master_keys", [_DISTINCT_KEY_HEX, "01" * 32, "02" * 32, "03" * 32]), diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index 81c8d662..85aa85bf 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -248,7 +248,7 @@ def __iter__(self): assert settings.previous_master_keys == (SecretStr(K3.hex()),) def test_validator_that_sets_a_private_attribute_does_not_deadlock(self, monkeypatch): - """A subclass's validator may set a private attribute while the guard holds its lock. A throwaway lock keeps a + """A subclass's validator may set a private attribute while the guard holds its lock. A fresh lock of the guard's kind keeps a regression from blocking every later assignment in this process.""" import threading @@ -256,7 +256,7 @@ def test_validator_that_sets_a_private_attribute_does_not_deadlock(self, monkeyp import cachekit.config.settings as settings_module - monkeypatch.setattr(settings_module, "_ASSIGNMENT_LOCK", threading.Lock()) + monkeypatch.setattr(settings_module, "_ASSIGNMENT_LOCK", type(settings_module._ASSIGNMENT_LOCK)()) # same kind, fresh class _Marking(CachekitConfig): _validated: bool = PrivateAttr(default=False) @@ -274,6 +274,33 @@ def _mark(self) -> _Marking: assert not assigning.is_alive() assert config.l1_max_size_mb == 7 + def test_property_setter_that_assigns_a_field_does_not_deadlock(self, monkeypatch): + """A subclass property setter that assigns a field runs on the copy while the guard holds its lock, then + assigns again through the guard. A fresh lock of the guard's kind keeps a regression from blocking later + assignments.""" + import threading + + import cachekit.config.settings as settings_module + + monkeypatch.setattr(settings_module, "_ASSIGNMENT_LOCK", type(settings_module._ASSIGNMENT_LOCK)()) # same kind, fresh + + class _Sized(CachekitConfig): + @property + def size(self) -> int: + return self.l1_max_size_mb + + @size.setter + def size(self, value: int) -> None: + self.l1_max_size_mb = value + + config = _Sized() + assigning = threading.Thread(target=setattr, args=(config, "size", 5), daemon=True) + assigning.start() + assigning.join(timeout=5) + + assert not assigning.is_alive() + assert config.l1_max_size_mb == 5 + def test_valid_assignment_lands_validated(self, settings): settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] settings.master_key = K1.hex() # type: ignore[assignment] From 50b0685e94c894d07c4cd12c273243f3b0c20860 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 11:47:33 +1100 Subject: [PATCH 06/10] fix(config): commit the validated state once and run a property setter once The guard committed an assignment by validating the copy's value a second time, so a validator that is not idempotent ran twice. It now swaps in the state the copy validated, as pydantic itself commits. A subclass property setter now runs once on the live settings; replaying it through a copy applied a non-idempotent setter twice and could not assign a write-only property. --- src/cachekit/config/settings.py | 12 ++++++--- tests/unit/test_key_rotation_keyring.py | 36 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index 5ce82932..20b1464d 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -303,12 +303,18 @@ def __setattr__(self, name: str, value: Any) -> None: candidate = None try: with _ASSIGNMENT_LOCK: + # A subclass property: run its setter once, on this instance. Each field it assigns comes back + # through this guard; replaying it on a copy would apply a non-idempotent setter twice. + if isinstance(getattr(type(self), name, None), property): + super().__setattr__(name, value) + return candidate = self.model_copy() # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy. _redacting(functools.partial(BaseSettings.__setattr__, candidate, name, value), type(self).__name__) - # The value the copy validated, never `value` again: a generator is spent by now, and an iterable - # may yield something else on a second pass. Pydantic validates it once more as it writes. - super().__setattr__(name, getattr(candidate, name)) + # Commit the state the copy validated, with no second validation: `value` may be a spent generator, + # and a non-idempotent validator would change it again. One swap of __dict__, as pydantic commits. + object.__setattr__(self, "__pydantic_fields_set__", candidate.__pydantic_fields_set__) + object.__setattr__(self, "__dict__", candidate.__dict__) finally: del value, candidate # the copy holds the refused value diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index 85aa85bf..aec2caa7 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -301,6 +301,42 @@ def size(self, value: int) -> None: assert not assigning.is_alive() assert config.l1_max_size_mb == 5 + def test_property_setter_runs_once_on_the_live_settings(self): + """A setter that is not idempotent must apply once, a write-only property must still be assignable, and what the + setter writes to a private attribute must land on the live settings, not on a discarded copy.""" + from pydantic import PrivateAttr + + class _Growing(CachekitConfig): + _grown_by: int = PrivateAttr(default=0) + + def _grow(self, by: int) -> None: + self._grown_by += by + self.l1_max_size_mb += by + + grow = property(fset=_grow) + + config = _Growing(l1_max_size_mb=10) + config.grow = 5 + + assert config.l1_max_size_mb == 15 + assert config._grown_by == 5 + + def test_assigned_value_is_validated_once(self): + """The live settings take the state the copy validated, so a validator that is not idempotent runs once.""" + from pydantic import field_validator + + class _Bumping(CachekitConfig): + @field_validator("l1_max_size_mb") + @classmethod + def _bump(cls, value: int) -> int: + return value + 1 + + config = _Bumping() + config.l1_max_size_mb = 5 + + assert config.l1_max_size_mb == 6 + assert "l1_max_size_mb" in config.model_fields_set + def test_valid_assignment_lands_validated(self, settings): settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] settings.master_key = K1.hex() # type: ignore[assignment] From 2623920c90c8cbd7de38eb9c7834a58ce7474ef1 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 11:51:02 +1100 Subject: [PATCH 07/10] fix(config): redact a refusal raised through a property setter A subclass property setter runs on the live settings, outside the copy path, so a key it assigned and the settings refused stayed in pydantic's frame locals. The property path now runs inside the same redaction boundary. --- src/cachekit/config/settings.py | 5 +++-- tests/unit/config/test_redacting_settings.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index 20b1464d..013870e1 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -304,9 +304,10 @@ def __setattr__(self, name: str, value: Any) -> None: try: with _ASSIGNMENT_LOCK: # A subclass property: run its setter once, on this instance. Each field it assigns comes back - # through this guard; replaying it on a copy would apply a non-idempotent setter twice. + # through this guard; replaying it on a copy would apply a non-idempotent setter twice. Redacted like + # the copy path, so a refused key leaves no pydantic frame holding it. if isinstance(getattr(type(self), name, None), property): - super().__setattr__(name, value) + _redacting(functools.partial(BaseSettings.__setattr__, self, name, value), type(self).__name__) return candidate = self.model_copy() # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy. diff --git a/tests/unit/config/test_redacting_settings.py b/tests/unit/config/test_redacting_settings.py index f1ba18c2..b6f2a484 100644 --- a/tests/unit/config/test_redacting_settings.py +++ b/tests/unit/config/test_redacting_settings.py @@ -670,6 +670,23 @@ def test_refused_assignment_leaves_no_frame_local(self, field: str, value: objec assert _cachekit_locals_holding(exc_info.value, _DISTINCT_KEY_HEX, below_caller=True) == [] + def test_refused_assignment_through_a_property_leaves_no_frame_local(self) -> None: + """A subclass property setter that assigns a key runs outside the copy, so it needs the same boundary.""" + + class _Rotating(CachekitConfig): + def _rotate(self, key: str) -> None: + self.master_key = key # type: ignore[assignment] + + rotate = property(fset=_rotate) + + config = _Rotating(previous_master_keys=(_DISTINCT_KEY_HEX,)) # type: ignore[arg-type] + with pytest.raises(ValidationError) as exc_info: + config.rotate = _DISTINCT_KEY_HEX + + assert config.master_key is None + _assert_no_route_to(exc_info.value, _DISTINCT_KEY_HEX) + assert _cachekit_locals_holding(exc_info.value, _DISTINCT_KEY_HEX, below_caller=True) == [] + @pytest.mark.unit class TestRedactingSettingsFrameLocals: From 90a01a4f12e8270871b3f6353378dd5714136f4e Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 12:05:55 +1100 Subject: [PATCH 08/10] fix(config): commit the private state a model validator derives on the copy Model validators run on the copy, and the commit swapped in its fields but not its private attributes, so private state a subclass validator derived was discarded with the copy. --- src/cachekit/config/settings.py | 4 +++- tests/unit/test_key_rotation_keyring.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index 013870e1..7026407c 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -313,7 +313,9 @@ def __setattr__(self, name: str, value: Any) -> None: # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy. _redacting(functools.partial(BaseSettings.__setattr__, candidate, name, value), type(self).__name__) # Commit the state the copy validated, with no second validation: `value` may be a spent generator, - # and a non-idempotent validator would change it again. One swap of __dict__, as pydantic commits. + # and a non-idempotent validator would change it again. One swap of __dict__, as pydantic commits, plus + # the private state a subclass's model validator may have derived on the copy. + object.__setattr__(self, "__pydantic_private__", candidate.__pydantic_private__) object.__setattr__(self, "__pydantic_fields_set__", candidate.__pydantic_fields_set__) object.__setattr__(self, "__dict__", candidate.__dict__) finally: diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index aec2caa7..ff1e0070 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -337,6 +337,23 @@ def _bump(cls, value: int) -> int: assert config.l1_max_size_mb == 6 assert "l1_max_size_mb" in config.model_fields_set + def test_private_state_a_model_validator_derives_lands(self): + """Model validators run on the copy, so what one writes to a private attribute must be committed with it.""" + from pydantic import PrivateAttr, model_validator + + class _Derived(CachekitConfig): + _l1_bytes: int = PrivateAttr(default=0) + + @model_validator(mode="after") + def _derive(self) -> _Derived: + self._l1_bytes = self.l1_max_size_mb * 1024 * 1024 + return self + + config = _Derived() + config.l1_max_size_mb = 7 + + assert config._l1_bytes == 7 * 1024 * 1024 + def test_valid_assignment_lands_validated(self, settings): settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] settings.master_key = K1.hex() # type: ignore[assignment] From 04252c68f130361ef67bc1a6a429fc238be1c083 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 13:15:50 +1100 Subject: [PATCH 09/10] fix(config): take the assignment lock for private writes and deletes An assignment commits by swapping in its validated copy's whole state. A private attribute written, or any attribute deleted, from another thread between the copy and the swap skipped the lock, so the swap undid it. Both now take the lock; it is reentrant, so a validator or property setter that writes a private attribute under the guard still does not deadlock. --- src/cachekit/config/settings.py | 18 +++++--- tests/unit/test_key_rotation_keyring.py | 57 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index 7026407c..9288e165 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -39,8 +39,9 @@ # Serializes CachekitConfig assignments: each validates the whole state it would leave, so two # assignments that are each valid alone (a new master_key, and that key added to previous_master_keys) -# cannot land together unchecked. Reentrant: a subclass's validator or property setter may assign a field -# while the lock is held. +# cannot land together unchecked. An assignment commits by swapping in its copy's whole state, so every +# other change (a private attribute, a delete) takes the lock too, or the swap would undo it. Reentrant: +# a subclass's validator or property setter may assign a field or a private attribute while it is held. _ASSIGNMENT_LOCK = threading.RLock() @@ -295,10 +296,12 @@ def __setattr__(self, name: str, value: Any) -> None: assignment would stay on the instance, readable by other threads until the error surfaces. The value is dropped in a finally: the raised error's traceback holds this frame (CWE-532). """ - # A private attribute: nothing to validate, and a validator that sets one must not re-enter the lock below. - # Any other name goes through the guard, so a mistyped field's value is refused inside _redacting too. + # A private attribute: nothing to validate, so no copy. It still takes the lock, so it cannot land between + # another thread's model_copy() and swap and be lost. Any other name goes through the guard, so a mistyped + # field's value is refused inside _redacting too. if name in type(self).__private_attributes__: - super().__setattr__(name, value) + with _ASSIGNMENT_LOCK: + super().__setattr__(name, value) return candidate = None try: @@ -321,6 +324,11 @@ def __setattr__(self, name: str, value: Any) -> None: finally: del value, candidate # the copy holds the refused value + def __delattr__(self, name: str) -> None: + """Delete under the assignment lock, so an assignment's swap cannot undo the delete.""" + with _ASSIGNMENT_LOCK: + super().__delattr__(name) + def __repr__(self) -> str: """Return string representation with sensitive information masked. diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index ff1e0070..47df0ae0 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -301,6 +301,63 @@ def size(self, value: int) -> None: assert not assigning.is_alive() assert config.l1_max_size_mb == 5 + @pytest.mark.parametrize( + ("change", "landed"), + [ + pytest.param(lambda config: setattr(config, "_note", "set"), lambda config: config._note == "set", id="set"), + pytest.param(lambda config: delattr(config, "_note"), lambda config: not hasattr(config, "_note"), id="delete"), + ], + ) + def test_private_change_from_another_thread_is_not_undone(self, monkeypatch, change, landed): + """An assignment commits by swapping in its copy's private state, so a private attribute another thread sets or + deletes between the copy and the commit must wait for the lock instead of being undone by the swap.""" + import threading + + from pydantic import PrivateAttr, model_validator + + import cachekit.config.settings as settings_module + + lock = type(settings_module._ASSIGNMENT_LOCK)() # same kind, fresh + progressed = threading.Event() # the other thread reached the lock, or made its change without it + + class _Reporting: + """The guard's lock, reporting when the other thread reaches it.""" + + def __enter__(self) -> None: + if threading.current_thread() is changing: + progressed.set() + lock.acquire() + + def __exit__(self, *exc_info: object) -> None: + lock.release() + + monkeypatch.setattr(settings_module, "_ASSIGNMENT_LOCK", _Reporting()) + + class _Noted(CachekitConfig): + _note: str = PrivateAttr(default="") + + @model_validator(mode="after") + def _change_meanwhile(self) -> _Noted: + if armed: # on the copy, between model_copy() and the commit + changing.start() + progressed.wait(timeout=5) + return self + + def change_live() -> None: + change(config) + progressed.set() + + changing = threading.Thread(target=change_live, daemon=True) + armed = False + config = _Noted() + armed = True + config.l1_max_size_mb = 7 + changing.join(timeout=5) + + assert not changing.is_alive() + assert config.l1_max_size_mb == 7 + assert landed(config) + def test_property_setter_runs_once_on_the_live_settings(self): """A setter that is not idempotent must apply once, a write-only property must still be assignable, and what the setter writes to a private attribute must land on the live settings, not on a discarded copy.""" From cab3cc31640c62fcca96c8d967cbba0bd6328a07 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 10 Oct 2026 15:01:31 +1100 Subject: [PATCH 10/10] fix(config): commit an extra="allow" subclass's undeclared names with the assignment Pydantic's validated assignment stores an undeclared name in __pydantic_extra__, not __dict__. The guard committed __dict__, the fields set and the private state from its validated copy, but not the extras, so on a subclass with extra="allow" an assignment raised nothing and the value was gone. The commit now carries every slot BaseModel keeps instance state in, the same set model_copy() fills. --- src/cachekit/config/settings.py | 9 +++++---- tests/unit/test_key_rotation_keyring.py | 13 +++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/cachekit/config/settings.py b/src/cachekit/config/settings.py index 9288e165..bc9b17fd 100644 --- a/src/cachekit/config/settings.py +++ b/src/cachekit/config/settings.py @@ -22,6 +22,7 @@ from typing import Annotated, Any, Literal, Optional from pydantic import ( + BaseModel, Field, SecretStr, field_validator, @@ -316,11 +317,11 @@ def __setattr__(self, name: str, value: Any) -> None: # BaseSettings.__setattr__, not this override: pydantic's validated assignment, on the copy. _redacting(functools.partial(BaseSettings.__setattr__, candidate, name, value), type(self).__name__) # Commit the state the copy validated, with no second validation: `value` may be a spent generator, - # and a non-idempotent validator would change it again. One swap of __dict__, as pydantic commits, plus + # and a non-idempotent validator would change it again. Every slot pydantic keeps instance state in, as + # model_copy() fills them: __dict__, the fields set, an extra="allow" subclass's undeclared names, and # the private state a subclass's model validator may have derived on the copy. - object.__setattr__(self, "__pydantic_private__", candidate.__pydantic_private__) - object.__setattr__(self, "__pydantic_fields_set__", candidate.__pydantic_fields_set__) - object.__setattr__(self, "__dict__", candidate.__dict__) + for slot in BaseModel.__slots__: + object.__setattr__(self, slot, getattr(candidate, slot)) finally: del value, candidate # the copy holds the refused value diff --git a/tests/unit/test_key_rotation_keyring.py b/tests/unit/test_key_rotation_keyring.py index 47df0ae0..47029bd7 100644 --- a/tests/unit/test_key_rotation_keyring.py +++ b/tests/unit/test_key_rotation_keyring.py @@ -411,6 +411,19 @@ def _derive(self) -> _Derived: assert config._l1_bytes == 7 * 1024 * 1024 + def test_extra_attribute_on_an_extra_allow_subclass_lands(self): + """Pydantic stores an undeclared name in __pydantic_extra__, not __dict__, so the commit must carry it.""" + from pydantic_settings import SettingsConfigDict + + class _Open(CachekitConfig): + model_config = SettingsConfigDict(extra="allow") + + config = _Open() + config.custom = 1 + + assert config.custom == 1 + assert config.model_extra == {"custom": 1} + def test_valid_assignment_lands_validated(self, settings): settings.previous_master_keys = [K3.hex()] # type: ignore[list-item] settings.master_key = K1.hex() # type: ignore[assignment]