feat: support explicit per-function encryption opt-out (tri-state)#151
Merged
Conversation
WalkthroughThe pull request implements tri-state encryption control across the cachekit library. Type signatures for ChangesTri-State Encryption Parameter Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Fleet-wide auto-detection (PR #127) enables encryption whenever CACHEKIT_MASTER_KEY is set. Because the encryption flag was a plain bool, the auto-detect guard could not distinguish "user left it unset" from "user explicitly disabled" — so a deliberate @cache(encryption=False) was silently promoted to encryption=True and the value was encrypted anyway. Make the encryption flag tri-state across the handler, decorator wrapper, and EncryptionConfig: - None (default): auto-detect from CACHEKIT_MASTER_KEY (fleet-wide opt-in) - True : force encryption ON - False : explicit hard opt-out, honored even when a master key is present The handler now only runs auto-detection (and auto-sets single_tenant_mode) when encryption is None; explicit False is never promoted. EncryptionConfig.enabled defaults to None so config-driven decorators still auto-detect, and its validate() resolves CACHEKIT_MASTER_KEY from the env so force-on (True) works without inlining the key. The bare @cache decorator now maps a flattened encryption= bool to a nested EncryptionConfig, leaving intent presets and the RORO config form untouched. Closes #128
27Bslash6
force-pushed
the
feat/encryption-explicit-opt-out
branch
from
e24dd7b to
55473e1
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cachekit/cache_handler.py`:
- Around line 317-326: The forced-on encryption path must also materialize the
env master key: when encryption is True (or config.encryption is set) and
master_key is None and tenant_extractor is None, read
get_settings().master_key.get_secret_value() and set master_key and
single_tenant_mode just like the encryption is None branch; ensure this change
touches the same symbols used in creation flow (master_key, encryption,
tenant_extractor) so that create_cache_wrapper() and EncryptionConfig.validate()
end up forwarding a non-None master_key to
_get_cached_encryption_wrapper()/EncryptionWrapper.
In `@src/cachekit/config/nested.py`:
- Around line 384-396: The helper _resolve_master_key currently calls
settings.master_key.get_secret_value() and returns plaintext; change
_resolve_master_key to return the SecretStr (or None) instead of unwrapping it
(i.e., return settings.master_key directly) and update EncryptionConfig.validate
to test presence/emptiness using the SecretStr API (or truthiness) without
calling get_secret_value(); leave any bytes.fromhex(...) or other plaintext
conversion to the crypto boundary code that actually performs
encryption/decryption so only those functions unwrap the SecretStr. Use
get_settings(), settings.master_key, _resolve_master_key, and
EncryptionConfig.validate to locate and update the logic accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07eba6b1-46b4-4c2e-91f3-fb3fc13a6e51
📒 Files selected for processing (8)
.secrets.baselinesrc/cachekit/cache_handler.pysrc/cachekit/config/nested.pysrc/cachekit/config/validation.pysrc/cachekit/decorators/intent.pysrc/cachekit/decorators/wrapper.pytests/unit/config/test_nested_configs.pytests/unit/test_cache_handler_encryption_autodetect.py
Comment on lines
+317
to
+326
| if encryption is None: | ||
| encryption = False | ||
| if master_key is None and tenant_extractor is None: | ||
| from cachekit.config.singleton import get_settings | ||
|
|
||
| settings = get_settings() | ||
| if settings.master_key: | ||
| encryption = True | ||
| master_key = settings.master_key.get_secret_value() | ||
| single_tenant_mode = True |
There was a problem hiding this comment.
Resolve CACHEKIT_MASTER_KEY for the forced-on path as well.
This branch only materialises master_key from settings when encryption is None. In the explicit encryption=True path, EncryptionConfig.validate() can succeed on an env key, but create_cache_wrapper() still forwards config.encryption.master_key=None, so self.master_key stays unset and _get_cached_encryption_wrapper() later builds the EncryptionWrapper without a key. That breaks the advertised encryption=True + env-only behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cachekit/cache_handler.py` around lines 317 - 326, The forced-on
encryption path must also materialize the env master key: when encryption is
True (or config.encryption is set) and master_key is None and tenant_extractor
is None, read get_settings().master_key.get_secret_value() and set master_key
and single_tenant_mode just like the encryption is None branch; ensure this
change touches the same symbols used in creation flow (master_key, encryption,
tenant_extractor) so that create_cache_wrapper() and EncryptionConfig.validate()
end up forwarding a non-None master_key to
_get_cached_encryption_wrapper()/EncryptionWrapper.
Comment on lines
+384
to
+396
| def _resolve_master_key(self) -> str | None: | ||
| """Resolve the master key from the inline value, falling back to env settings. | ||
|
|
||
| Inline master_key takes precedence; otherwise read CACHEKIT_MASTER_KEY via the | ||
| settings singleton. Mirrors validate_encryption_config so the config-level and | ||
| handler-level views of "is a key available?" stay consistent. | ||
| """ | ||
| if self.master_key: | ||
| return self.master_key | ||
| from cachekit.config.singleton import get_settings | ||
|
|
||
| settings = get_settings() | ||
| return settings.master_key.get_secret_value() if settings.master_key else None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Audit master-key typing and unwrapping sites before changing the surface to SecretStr.
rg -n -C2 '\bmaster_key\b|SecretStr|get_secret_value\(' src/cachekit testsRepository: cachekit-io/cachekit-py
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show EncryptionConfig type definitions + _resolve_master_key usage
sed -n '320,430p' src/cachekit/config/nested.py | cat -n
# Show validate_encryption_config master-key resolution
sed -n '1,140p' src/cachekit/config/validation.py | cat -n
# Show cache_handler master_key typing/autodetect and where it unwraps settings.master_key
sed -n '240,620p' src/cachekit/cache_handler.py | cat -nRepository: cachekit-io/cachekit-py
Length of output: 30483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\. _resolve_master_key\(|_resolve_master_key\(" src/cachekit/config/nested.py src/cachekit | head -n 200Repository: cachekit-io/cachekit-py
Length of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show CachekitConfig.master_key definition + type
rg -n "class CachekitConfig|master_key: Optional\\[SecretStr\\]" -n src/cachekit/config/settings.py
sed -n '200,260p' src/cachekit/config/settings.py | cat -n
# Also inspect get_settings/singleton for master_key typing
rg -n "def get_settings|master_key" src/cachekit/config/singleton.py src/cachekit/config/__init__.py 2>/dev/null || true
ls src/cachekit/config | headRepository: cachekit-io/cachekit-py
Length of output: 2719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect singleton typing for master_key
sed -n '1,120p' src/cachekit/config/singleton.py | cat -n
# Inspect EncryptionConfig definition in nested.py (imports + field types)
sed -n '1,120p' src/cachekit/config/nested.py | cat -n
sed -n '240,430p' src/cachekit/config/nested.py | cat -nRepository: cachekit-io/cachekit-py
Length of output: 15812
Avoid unwrapping settings.master_key to plaintext in _resolve_master_key()
src/cachekit/config/nested.py currently converts settings.master_key from SecretStr to str via get_secret_value() in _resolve_master_key() (used by EncryptionConfig.validate() for a presence check). This breaks the secret-handling rule by spreading plaintext key material across config/runtime layers. Change _resolve_master_key() (and validate()) to keep the resolved value as SecretStr (or avoid get_secret_value() entirely for the “is a key available?” check), and only unwrap to plaintext at the crypto boundary where bytes.fromhex(...) is actually required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cachekit/config/nested.py` around lines 384 - 396, The helper
_resolve_master_key currently calls settings.master_key.get_secret_value() and
returns plaintext; change _resolve_master_key to return the SecretStr (or None)
instead of unwrapping it (i.e., return settings.master_key directly) and update
EncryptionConfig.validate to test presence/emptiness using the SecretStr API (or
truthiness) without calling get_secret_value(); leave any bytes.fromhex(...) or
other plaintext conversion to the crypto boundary code that actually performs
encryption/decryption so only those functions unwrap the SecretStr. Use
get_settings(), settings.master_key, _resolve_master_key, and
EncryptionConfig.validate to locate and update the logic accordingly.
Source: Coding guidelines
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cachekit/cache_handler.py (1)
322-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMaterialise
CACHEKIT_MASTER_KEYforencryption=Trueas well.Only the
encryption is Nonebranch resolves the env key. If callers passencryption=Trueand rely on the upstreamEncryptionConfig.validate()env fallback,self.master_keystill staysNonehere, so_get_cached_encryption_wrapper()later buildsEncryptionWrapper(..., master_key=None). That leaves the documented force-on + env-only path broken.Suggested fix
- if encryption is None: - encryption = False - if master_key is None and tenant_extractor is None: - from cachekit.config.singleton import get_settings - - settings = get_settings() - if settings.master_key: - encryption = True - master_key = settings.master_key.get_secret_value() - single_tenant_mode = True + resolved_env_master_key: str | None = None + if master_key is None: + settings = get_settings() + resolved_env_master_key = settings.master_key.get_secret_value() if settings.master_key else None + + if encryption is None: + encryption = False + if resolved_env_master_key is not None and tenant_extractor is None: + encryption = True + master_key = resolved_env_master_key + single_tenant_mode = True + elif encryption and master_key is None: + master_key = resolved_env_master_key🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cachekit/cache_handler.py` around lines 322 - 331, The code only resolves CACHEKIT_MASTER_KEY when encryption is None, leaving callers who pass encryption=True with master_key=None; update the logic in the CacheHandler initializer so that when encryption is True (or encryption is None and resolved to True) and master_key is None and tenant_extractor is None you call get_settings(), set master_key = settings.master_key.get_secret_value(), set encryption = True and single_tenant_mode = True; ensure this change affects the same variables used by _get_cached_encryption_wrapper() / EncryptionWrapper so EncryptionConfig.validate() fallback and the force-on env-only path work correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/cachekit/cache_handler.py`:
- Around line 322-331: The code only resolves CACHEKIT_MASTER_KEY when
encryption is None, leaving callers who pass encryption=True with
master_key=None; update the logic in the CacheHandler initializer so that when
encryption is True (or encryption is None and resolved to True) and master_key
is None and tenant_extractor is None you call get_settings(), set master_key =
settings.master_key.get_secret_value(), set encryption = True and
single_tenant_mode = True; ensure this change affects the same variables used by
_get_cached_encryption_wrapper() / EncryptionWrapper so
EncryptionConfig.validate() fallback and the force-on env-only path work
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7a91b1f-d131-4f9a-ab90-96d90622fbcc
📒 Files selected for processing (1)
src/cachekit/cache_handler.py
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Make the
encryptionflag tri-state so a deliberate@cache(encryption=False)is honored even whenCACHEKIT_MASTER_KEYis set fleet-wide.None(default): auto-detect fromCACHEKIT_MASTER_KEY(fleet-wide opt-in)True: force encryption ONFalse: explicit hard opt-out — never encrypts, even with a master key presentWhy
Fleet-wide auto-detection (PR #127) turns encryption on whenever
CACHEKIT_MASTER_KEYis set. Becauseencryptionwas a plainbool, the auto-detect guard (if not encryption and master_key is None ...) could not tell "user left it unset" from "user explicitly disabled" — both wereFalse. The result: a deliberate@cache(encryption=False)was silently promoted toencryption=Trueand the value was encrypted anyway (issue #128). There was no per-function escape hatch from fleet-wide encryption.What changed
cache_handler.py—encryptionparam is nowbool | None(defaultNone). The auto-detect guard runs only whenencryption is None; it is the sole path that may flip encryption on and auto-setsingle_tenant_mode. An explicitFalseis never promoted.config/nested.py—EncryptionConfig.enableddefaults toNone(tri-state) so config-driven decorators still auto-detect.validate()now resolvesCACHEKIT_MASTER_KEYfrom the env (matching the handler), so force-on (True) works without inlining the key.None/Falseare both falsy and skip validation.decorators/wrapper.py— publicencryption: bool | None = None; the three-state value is threaded to the handler without collapsingNone→False.decorators/intent.py— the bare@cachedecorator maps a flattenedencryption=<bool>(plus related scalar kwargs) to a nestedEncryptionConfig. Intent presets (.secure,.io, …) and the ROROconfig=form are left untouched; an already-constructedEncryptionConfigpasses through without double-wrapping.config/validation.py—validate_encryption_configacceptsbool | None.How verified
tests/unit/test_cache_handler_encryption_autodetect.py(withCACHEKIT_MASTER_KEYset viamonkeypatch, since PR test: fix MASTER_KEY env leak that broke 14 integration tests post #127 #133 removed the autouse env): assert@cache(encryption=False)yields a non-encrypted envelope while bare@cacheencrypts, andencryption=Truestill forces on — checked at the handler level viahandler.encryptionand on the wire via the serialized envelope'smetadata.encryptedflag, plus end-to-end throughDecoratorConfig.EncryptionConfigdefault-value test to the new tri-state contract (enabled is None).uv run ruff check .✓ ·ruff format --check .✓ ·basedpyright src/✓ (0 errors) · doctests on all changed modules ✓pytest tests/critical/ tests/unit/→ 1713 passed (1 unrelated flaky file-backend FD test, passes in isolation; touches no files in this PR).Closes #128
Summary by CodeRabbit
New Features
Tests