Add FernetEncryptionWrapper and simplify setup #77
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe encryption wrapper architecture is refactored to separate generic encryption logic from Fernet-specific implementation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant FernetEncryptionWrapper
participant BaseEncryptionWrapper
participant encrypt_with_fernet
participant decrypt_with_fernet
participant Fernet
Note over Client,Fernet: Encryption Flow
Client->>FernetEncryptionWrapper: put(key, value)
FernetEncryptionWrapper->>BaseEncryptionWrapper: _encrypt_value(value)
BaseEncryptionWrapper->>encrypt_with_fernet: call encryption_fn(json_bytes)
encrypt_with_fernet->>Fernet: encrypt(data)
Fernet-->>encrypt_with_fernet: encrypted_bytes
encrypt_with_fernet-->>BaseEncryptionWrapper: base64(encrypted_bytes)
BaseEncryptionWrapper-->>FernetEncryptionWrapper: {__encrypted_data__, __encryption_version__}
Note over Client,Fernet: Decryption Flow
Client->>FernetEncryptionWrapper: get(key)
FernetEncryptionWrapper->>BaseEncryptionWrapper: _decrypt_value(encrypted_payload)
BaseEncryptionWrapper->>BaseEncryptionWrapper: _validate_encrypted_payload(payload)
BaseEncryptionWrapper->>decrypt_with_fernet: call decryption_fn(encrypted_bytes, version)
decrypt_with_fernet->>Fernet: decrypt(data)
Fernet-->>decrypt_with_fernet: decrypted_bytes
decrypt_with_fernet-->>BaseEncryptionWrapper: json_bytes
BaseEncryptionWrapper-->>FernetEncryptionWrapper: value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes This refactoring spans multiple files and introduces new abstractions (base class, derived class, new exceptions), but follows a consistent and cohesive pattern. The logic changes are concentrated in three areas: the base wrapper redesign (function callbacks), the Fernet wrapper implementation (key derivation and encryption wiring), and test updates (fixture refactoring). The changes are heterogeneous but methodical, with no extreme logic density in individual sections. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (4)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (18)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the encryption wrapper implementation by introducing a new FernetEncryptionWrapper class that extends a new BaseEncryptionWrapper class, simplifying initialization and improving the API design for encryption functionality.
Key changes:
- Introduced
BaseEncryptionWrapperandFernetEncryptionWrapperclasses, replacing the previousEncryptionWrapper - Added support for deriving encryption keys from source material and salt, rather than requiring pre-generated Fernet keys
- Removed redundant test setup by utilizing the shared
fernetfixture
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py |
Updated tests to use FernetEncryptionWrapper and shared fernet fixture, removing redundant setup code |
key-value/key-value-sync/src/key_value/sync/wrappers/encryption/__init__.py |
Updated exports to include BaseEncryptionWrapper and FernetEncryptionWrapper |
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py |
New file implementing FernetEncryptionWrapper with support for both Fernet instances and source material |
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py |
Refactored base encryption wrapper to accept encryption/decryption functions instead of Fernet key |
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/__init__.py |
Updated exports to include BaseEncryptionWrapper and FernetEncryptionWrapper |
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py |
Updated async tests to use FernetEncryptionWrapper and shared fernet fixture |
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py |
New file implementing async version of FernetEncryptionWrapper |
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py |
Refactored async base encryption wrapper with encryption/decryption functions |
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/__init__.py |
Updated exports for async encryption wrappers |
key-value/key-value-aio/pyproject.toml |
Modified pytest configuration to enable inline snapshot fixes and creation |
README.md |
Updated documentation example to show new initialization pattern |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
101-107: Honor raise_on_decryption_error for malformed typesIf "encrypted_data" exists but isn’t a str, this raises TypeError even when raise_on_decryption_error is False. That breaks the contract used elsewhere for corrupted data.
- if not isinstance(base64_str, str): - # Corrupted data, return as-is - msg = f"Corrupted data: expected str, got {type(base64_str)}" - raise TypeError(msg) + if not isinstance(base64_str, str): + msg = f"Corrupted data: expected str, got {type(base64_str)}" + if self.raise_on_decryption_error: + raise TypeError(msg) + return None
🧹 Nitpick comments (12)
key-value/key-value-aio/pyproject.toml (1)
52-52: Avoid auto-creating inline snapshots by defaultUsing --inline-snapshot=fix,create in addopts can silently rewrite snapshots (especially on CI). Recommend dropping create and running it only locally when intentionally updating.
-[tool.pytest.ini_options] -asyncio_mode = "auto" -addopts = ["--inline-snapshot=fix,create"] +[tool.pytest.ini_options] +asyncio_mode = "auto" +addopts = ["--inline-snapshot=fix"]Optionally, document running
pytest --inline-snapshot=createlocally when needed.key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
29-31: Docstring should be algorithm-agnosticBaseEncryptionWrapper now accepts arbitrary encryption/decryption functions; “using Fernet” is misleading. Reword to “using the provided encryption function.”
- This wrapper encrypts the JSON-serialized value using Fernet (symmetric encryption) + This wrapper encrypts the JSON‑serialized value using the provided symmetric encryption function
66-71: Prevent accidental double-encryptionThe guard is commented out. If callers pass an already-encrypted dict (or stack the wrapper twice), get() will only decrypt once and return an encrypted payload to users.
- # # Don't encrypt if it's already encrypted - # if _ENCRYPTED_DATA_KEY in value: - # return value + # Don't encrypt if it's already encrypted + if _ENCRYPTED_DATA_KEY in value: + return valueAdd a unit test for idempotence on already-encrypted inputs.
81-83: Clarify comment: not necessarily FernetThis encrypts via the injected function, not specifically Fernet.
- # Encrypt with Fernet + # Encrypt via provided encryption function
112-114: Clarify comment: not necessarily FernetThis decrypts via the injected function, not specifically Fernet.
- # Decrypt with Fernet + # Decrypt via provided decryption functionkey-value/key-value-aio/tests/stores/wrappers/test_encryption.py (1)
52-57: Avoid snapshotting exact encrypted token; assert structure insteadFernet tokens are non-deterministic (timestamp/IV). Exact snapshot will change across runs and, with --inline-snapshot=create, can mask issues. Assert shape/content invariants instead.
- raw_result = await memory_store.get(collection="test", key="test") - assert raw_result == snapshot( - { - "__encrypted_data__": "Z0FBQUFBQm8tc0ZsYWhZUmJqUnN0VGlyeGVoUWZuczlPUllyWWxyVEotTVNMVFMtd1hoalNTQk56eFdzNGVocEg0T0xDeEVkTHpJckc2Z0lGZGpCTWZpS3o3cmVWRmRUTl91RENvSW8zNnI3QTlJVmtrQ1FtNnc9", - "__encryption_version__": 1, - } - ) + raw_result = await memory_store.get(collection="test", key="test") + assert raw_result is not None + assert set(raw_result.keys()) == {"__encrypted_data__", "__encryption_version__"} + assert isinstance(raw_result["__encrypted_data__"], str) + assert raw_result["__encryption_version__"] == 1 + # sanity: ciphertext doesn't leak plaintext + assert "test" not in raw_result["__encrypted_data__"] + assert "value" not in raw_result["__encrypted_data__"]key-value/key-value-sync/src/key_value/sync/wrappers/encryption/__init__.py (1)
4-7: Potential breaking change: restore a deprecated alias for EncryptionWrapperDownstream code importing EncryptionWrapper from this module will break. Consider adding a temporary alias to ease migration.
Apply in the source (not this generated file):
from key_value.sync.code_gen.wrappers.encryption.base import BaseEncryptionWrapper from key_value.sync.code_gen.wrappers.encryption.fernet import FernetEncryptionWrapper -__all__ = ["BaseEncryptionWrapper", "FernetEncryptionWrapper"] +# Deprecated alias for backward compatibility. Remove in a future major. +EncryptionWrapper = FernetEncryptionWrapper +__all__ = ["BaseEncryptionWrapper", "FernetEncryptionWrapper", "EncryptionWrapper"]Please confirm if a hard break is intended; if so, update release notes and migration docs.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
35-53: KDF choice and defaults may encourage weak keys if source_material is password-likeHKDF is fine for key expansion from high-entropy input, but not for password hardening. If source_material may be a human password, prefer PBKDF2HMAC/Scrypt (with parameters per cryptography guidance) or explicitly document that source_material must be high-entropy.
Option A (doc/param rename): Rename to key_material and document “must be high-entropy; not a password.”
Option B (stronger derivation): Use Scrypt for password-like input:
-from cryptography.hazmat.primitives.kdf.hkdf import HKDF +from cryptography.hazmat.primitives.kdf.hkdf import HKDF +from cryptography.hazmat.primitives.kdf.scrypt import Scrypt @@ -def _generate_encryption_key(source_material: str, salt: str | None = None) -> bytes: +def _generate_encryption_key(source_material: str, salt: str | None = None, *, password_like: bool = False) -> bytes: @@ - raw_key = HKDF( - algorithm=hashes.SHA256(), - length=32, - salt=salt.encode(), - info=b"Fernet", - ).derive(key_material=source_material.encode()) + if password_like: + kdf = Scrypt(salt=salt.encode(), length=32, n=2**14, r=8, p=1) + raw_key = kdf.derive(source_material.encode()) + else: + raw_key = HKDF(algorithm=hashes.SHA256(), length=32, salt=salt.encode(), info=b"Fernet").derive( + key_material=source_material.encode() + )Confirm intended usage and we can finalize docs/tests accordingly.
Also applies to: 68-74
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (2)
65-70: Tidy up comments and remove dead code
- Remove the commented “already encrypted” block to avoid drift.
- Update comments to reflect pluggable crypto, not Fernet.
Apply:
- # # Don't encrypt if it's already encrypted - # if _ENCRYPTED_DATA_KEY in value: - # return value @@ - # Encrypt with Fernet + # Encrypt with the provided encryption function @@ - # Decrypt with Fernet + # Decrypt with the provided decryption functionAlso applies to: 80-82, 108-110
25-27: Remove unused EncryptionError (avoid confusion with shared errors)This local exception isn’t used; shared DecryptionError is used instead. Drop it or start using it consistently.
Apply:
-class EncryptionError(Exception): - """Exception raised when encryption or decryption fails.""" -key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (2)
109-116: Rename test to reflect behavior (raises error with wrong key)Name and docstring don’t match behavior. Rename for clarity.
Apply:
- def test_decryption_with_wrong_key_returns_original(self, memory_store: MemoryStore): - """Test that decryption with the wrong key raises an error.""" + def test_decryption_with_wrong_key_raises_error(self, memory_store: MemoryStore): + """Decryption with the wrong key raises an error."""
15-18: Optional: add a test for source_material pathCurrent tests exercise only the fernet instance path. Add a simple round‑trip using source_material to guard the KDF branch.
I can draft the test if you want it included in this PR.
Also applies to: 23-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)key-value/key-value-aio/pyproject.toml(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_encryption.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-sync/src/key_value/sync/wrappers/encryption/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(173-178)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(29-155)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (4)
FernetEncryptionWrapper(8-73)_generate_encryption_key(76-87)encrypt_with_fernet(62-63)decrypt_with_fernet(65-66)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (3)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (1)
fernet(13-14)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(26-159)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/__init__.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(29-155)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(11-65)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(8-73)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
put(145-147)get(125-127)put_many(150-159)get_many(130-132)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (2)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (2)
BaseEncryptionWrapper(26-159)_encrypt_value(66-90)key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(173-178)
key-value/key-value-sync/src/key_value/sync/wrappers/encryption/__init__.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(29-155)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(11-65)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/__init__.py (2)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(26-159)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(8-73)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (2)
BaseEncryptionWrapper(29-155)_encrypt_value(65-86)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (4)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (2)
store(20-21)fernet(13-14)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(11-65)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (4)
put(141-143)get(121-123)put_many(146-155)get_many(126-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
🔇 Additional comments (3)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/__init__.py (1)
4-7: LGTM: public exports match refactorBaseEncryptionWrapper and FernetEncryptionWrapper are correctly exported. No issues.
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/__init__.py (1)
1-4: LGTM: updated exportsExports align with the new base/fernet split.
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
52-60: Constructor validation is goodMutual exclusivity and presence checks are clear and helpful. Nice.
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
key-value/key-value-shared/src/key_value/shared/errors/base.py (1)
8-8: Refine the type annotation for accuracy.The
messageattribute is typed asstr | None, but after__init__completes,self.messagewill always be astr(line 23 sets it to the result of": ".join(message_parts), which returns an empty string when the list is empty, neverNone). Consider updating the type annotation to juststrfor accuracy.- message: str | None = None + message: str = ""key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (4)
29-44: Docstring still references Fernet; generalize to function-based crypto.After the refactor, this wrapper is algorithm-agnostic. Update wording. Apply in the original source.
- This wrapper encrypts the JSON-serialized value using Fernet (symmetric encryption) + This wrapper encrypts the JSON-serialized value using the provided encryption_fn (symmetric encryption) @@ - Note: The encryption key must be kept secret and secure. If the key is lost, - encrypted data cannot be recovered. + Note: Keep encryption material (keys/secrets) secure. If lost, encrypted data cannot be recovered.
46-54: Constructor changes look good; add a quick guard for the version.Validate encryption_version is a positive int at runtime. Apply in the original source.
- self.encryption_version: int = encryption_version + if not isinstance(encryption_version, int) or encryption_version < 1: + raise ValueError("encryption_version must be a positive int") + self.encryption_version: int = encryption_versionAlso applies to: 67-71
86-92: Comments mention Fernet; update to reflect generic hooks.Purely cosmetic, keeps code and docs aligned. Apply in the original source.
- # Encrypt with Fernet + # Encrypt using the provided encryption_fn @@ - # Encode to base64 for storage in dict (though Fernet output is already base64) + # Encode to base64 for storage in dict
118-118: Stricter base64 decoding.Use validate=True to fail fast on non-base64 data. Apply in the original source.
- encrypted_bytes: bytes = base64.b64decode(base64_str) + encrypted_bytes: bytes = base64.b64decode(base64_str, validate=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_encryption.py(5 hunks)key-value/key-value-shared/src/key_value/shared/errors/base.py(2 hunks)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py
🧰 Additional context used
🧬 Code graph analysis (6)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (5)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
EncryptionError(22-23)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
EncryptionError(25-26)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/wrapper.py (1)
EncryptionError(23-24)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/wrapper.py (1)
EncryptionError(20-21)key-value/key-value-shared/src/key_value/shared/errors/key_value.py (3)
SerializationError(10-11)KeyValueOperationError(6-7)DeserializationError(14-15)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (3)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (2)
EncryptionError(22-23)BaseEncryptionWrapper(26-167)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionError(4-5)key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(173-178)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (4)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (1)
fernet(14-15)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(26-167)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(29-167)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(14-76)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (4)
put(153-155)get(133-135)put_many(158-167)get_many(138-140)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(11-86)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
put(153-155)get(133-135)put_many(158-167)get_many(138-140)
🔇 Additional comments (17)
key-value/key-value-shared/src/key_value/shared/errors/base.py (1)
23-25: LGTM!Storing the composed message in
self.messagebefore passing it to the parent exception is a good enhancement. This allows callers to access the final formatted message via themessageattribute while maintaining backward compatibility with standard exception behavior.key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
10-13: LGTM!The new
EncryptionVersionErrorexception follows the established pattern and provides clear semantics for version mismatch errors.key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (3)
55-66: LGTM!The validation logic correctly enforces mutual exclusivity and required parameters with clear error messages.
68-78: LGTM!The encryption and decryption functions correctly implement version checking. The encryption check at lines 69-71 is defensive (since
_encrypt_valuealways passesself.encryption_version), while the decryption check at lines 75-77 is essential for preventing attempts to decrypt data encrypted with newer, potentially incompatible versions.
89-102: LGTM!The HKDF key derivation has been correctly fixed to base64-encode the derived key (line 102), addressing the previous review feedback. Fernet now receives a properly formatted key.
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (3)
18-19: LGTM!The new type aliases improve code readability and make the function signature requirements explicit.
43-69: LGTM!The refactored constructor successfully decouples encryption strategy from the base wrapper, enabling flexible encryption implementations through the function-based interface.
110-121: LGTM!The decryption logic correctly extracts and validates the stored encryption version before passing it to the decryption function, enabling version-aware decryption.
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (3)
16-25: LGTM!The fixtures are well-structured, providing a clean test setup with a Fernet instance for the encryption wrapper.
67-68: LGTM!The test now correctly uses
IsStr(min_length=32)instead of asserting exact Fernet ciphertext, addressing the non-deterministic nature of Fernet tokens and preventing flaky tests.
47-57: LGTM!The test correctly validates that the wrapper cannot decrypt data encrypted with a newer version, ensuring proper version checking behavior.
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (3)
13-22: LGTM!The fixtures are properly structured for async testing, providing a Fernet instance and correctly constructing the async encryption wrapper.
44-70: LGTM!The async tests correctly implement the same validation logic as the sync tests, with proper async/await usage and non-deterministic-safe assertions.
125-137: LGTM!The test and its docstring are now consistent, correctly indicating that decryption with the wrong key raises a
DecryptionError.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (3)
21-22: Type aliases are clear and helpful.Good API surface for injecting crypto strategies.
132-167: Sync wrapper methods look correct.Decrypt-after-read and encrypt-before-write flows are consistent; signatures align with KeyValue protocol.
25-27: Let me verify the generation relationship between aio and sync files:Let me verify the generation relationship:
The review comment is incorrect about which file to edit; redirect to the original source.
The sync file (
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py) is auto-generated from the aio module and carries a "DO NOT CHANGE" warning. Unasync transforms async code into synchronous counterparts by tokenizing code and re-rendering it into a new file. Editing the generated sync file directly won't persist—changes must be made in the original aio file (key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py).Both modules define an unused
EncryptionError(Exception)class (lines 22–24 in aio, lines 25–27 in sync). The actual error raised isDecryptionError, imported fromkey_value.shared.errors.wrappers.encryption. Delete the localEncryptionErrorclass from the aio file, then regenerate the sync variant viabuild_sync_library.py.-class EncryptionError(Exception): - """Exception raised when encryption or decryption fails.""" -Apply this change in
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.pyinstead.Likely an incorrect or invalid review comment.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
68-72: Redundant encryption version check.The version check at line 69 compares
encryption_version(passed fromBaseEncryptionWrapper._encrypt_value) againstself.encryption_version. Since the base class always passesself.encryption_version, this check evaluates toself.encryption_version > self.encryption_version, which is alwaysFalse.The decryption version check (line 75) is useful because the base class passes the version from stored data, which may differ. Consider removing the encryption check or adding a comment explaining it's defensive programming for potential future base class changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(29-167)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
_generate_encryption_key(89-103)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(26-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
🔇 Additional comments (2)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
89-103: LGTM! Key derivation implementation is secure.The PBKDF2 implementation correctly uses 1.2 million iterations (exceeding OWASP's 600k recommendation), SHA-256, and properly base64-encodes the derived key for Fernet compatibility.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
1-90: LGTM! Implementation is correct and secure.The
FernetEncryptionWrapperimplementation is sound:
- Input validation properly enforces mutual exclusivity and required parameters
- PBKDF2 configuration (1.2M iterations, SHA256) exceeds current security recommendations
- The critical bug from the previous review (Fernet key encoding) has been correctly addressed—
base64.urlsafe_b64encode()is now used at line 90- Version checks prevent decryption of data encrypted with unsupported future versions while maintaining backward compatibility
- Closure over
fernetinstance in the inner functions is appropriateNote: Since this is auto-generated code, any future refinements should be made to the source file referenced in the header.
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
108-116: TypeError bypassesraise_on_decryption_errorflag.Lines 108-116 raise
TypeErrordirectly when the encryption version is missing or has the wrong type, but these exceptions are not caught by the try-except block at lines 118-132. This means theraise_on_decryption_errorflag is bypassed for corrupted metadata, inconsistent with the behavior for decryption failures.Based on past review comments, these validation errors should respect the configured error-handling policy.
Apply this diff to route validation errors through the error handler:
- if _ENCRYPTION_VERSION_KEY not in value: - msg = "Corrupted data: missing encryption version" - raise TypeError(msg) - - encryption_version = value[_ENCRYPTION_VERSION_KEY] - if not isinstance(encryption_version, int): - # Corrupted data, return as-is - msg = f"Corrupted data: expected int, got {type(encryption_version)}" - raise TypeError(msg) + if _ENCRYPTION_VERSION_KEY not in value: + msg = "Corrupted data: missing encryption version" + if self.raise_on_decryption_error: + raise DecryptionError(msg) + return None + + encryption_version = value[_ENCRYPTION_VERSION_KEY] + if not isinstance(encryption_version, int): + msg = f"Corrupted data: expected int, got {type(encryption_version)}" + if self.raise_on_decryption_error: + raise DecryptionError(msg) + return Nonekey-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
60-67: Reject whitespace-only source_material/salt (strengthen validation)Whitespace-only inputs currently pass truthiness checks, weakening KDF inputs. Treat them as invalid.
- if fernet is None: - if not source_material: - msg = "Must provide either fernet or source_material" - raise ValueError(msg) - if not salt: - msg = "Must provide a salt" - raise ValueError(msg) - fernet = Fernet(key=_generate_encryption_key(source_material=source_material, salt=salt)) + if fernet is None: + if source_material is None or salt is None: + raise ValueError("Must provide source_material and salt when fernet is not provided") + if not source_material.strip(): + raise ValueError("source_material cannot be empty or whitespace") + if not salt.strip(): + raise ValueError("salt cannot be empty or whitespace") + fernet = Fernet(key=_generate_encryption_key(source_material=source_material, salt=salt))
🧹 Nitpick comments (11)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (1)
48-58: Consider testing version mismatch without mutating wrapper state.The test directly mutates
store.encryption_versionto simulate version incompatibility. While this works, it couples the test to the internal implementation (mutable attribute).A more robust approach would instantiate two wrappers or test with pre-encrypted payloads from different versions.
Example alternative:
def test_encryption_with_wrong_encryption_version(self, memory_store: MemoryStore, fernet: Fernet): """Test that decryption fails when encryption version is not supported.""" store_v2 = FernetEncryptionWrapper(key_value=memory_store, fernet=fernet) store_v2.encryption_version = 2 # Simulate future version original_value = {"test": "value"} store_v2.put(collection="test", key="test", value=original_value) store_v1 = FernetEncryptionWrapper(key_value=memory_store, fernet=fernet) # store_v1 has version 1 by default with pytest.raises(DecryptionError): store_v1.get(collection="test", key="test")key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (2)
55-58: Clarify mixed-argument error messageWording is awkward. Prefer explicitly disallowing fernet alongside either source_material or salt.
- if source_material or salt: - msg = "Cannot provide both fernet and source_material and salt" + if source_material or salt: + msg = "Cannot provide fernet together with source_material or salt" raise ValueError(msg)
8-9: Extract PBKDF2 iteration count into a named constantAvoid magic numbers and make iteration tuning explicit.
ENCRYPTION_VERSION = 1 + +# PBKDF2-HMAC-SHA256 iteration count used for key derivation. +# Keep high enough for security while maintaining acceptable latency. +KDF_ITERATIONS = 1_200_000 @@ - pbkdf2 = PBKDF2HMAC( + pbkdf2 = PBKDF2HMAC( algorithm=hashes.SHA256(), length=32, salt=salt.encode(), - iterations=1_200_000, + iterations=KDF_ITERATIONS, ).derive(key_material=source_material.encode())Also applies to: 87-101
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (6)
29-41: Make docstring algorithm-agnosticBase wrapper is pluggable now; avoid hard-coding “Fernet” in docs.
- This wrapper encrypts the JSON-serialized value using Fernet (symmetric encryption) + This wrapper encrypts the JSON-serialized value using an injected symmetric cipher
83-85: Update inline comment to reflect pluggable encryptionReferences “Fernet” but uses injected callable.
- # Encrypt with Fernet + # Encrypt with provided encryption_fn
122-124: Update inline comment to reflect pluggable decryptionReferences “Fernet” but uses injected callable.
- # Decrypt with Fernet + # Decrypt with provided decryption_fn
118-124: Use strict base64 decoding to catch corruption earlierEnable validation to reject non-base64 characters instead of silently ignoring them.
- encrypted_bytes: bytes = base64.b64decode(base64_str) + encrypted_bytes: bytes = base64.b64decode(base64_str, validate=True)
16-16: Remove unused _ENCRYPTION_VERSION constantDead code after moving to per-instance encryption_version.
-_ENCRYPTION_VERSION = 1
22-24: Remove unused local EncryptionErrorShadowing shared error types; not referenced.
-class EncryptionError(Exception): - """Exception raised when encryption or decryption fails."""key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (2)
45-47: Fix test docstring wordingIt’s testing decryption with mismatched version.
- """Test that encryption fails with the wrong encryption version.""" + """Test that decryption fails with the wrong encryption version."""
141-164: Strengthen key-derivation assertionsAlso verify the key decodes via urlsafe base64 to exactly 32 bytes.
def test_key_generation(): @@ - key = _generate_encryption_key(source_material=source_material, salt=salt) + key = _generate_encryption_key(source_material=source_material, salt=salt) + import base64 + assert len(base64.urlsafe_b64decode(key)) == 32 @@ - key = _generate_encryption_key(source_material=source_material, salt=salt) + key = _generate_encryption_key(source_material=source_material, salt=salt) + assert len(base64.urlsafe_b64decode(key)) == 32 @@ - key = _generate_encryption_key(source_material=source_material, salt=salt) + key = _generate_encryption_key(source_material=source_material, salt=salt) + assert len(base64.urlsafe_b64decode(key)) == 32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py(4 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_encryption.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (2)
FernetEncryptionWrapper(14-74)_generate_encryption_key(77-88)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (4)
put(155-157)get(135-137)put_many(160-169)get_many(140-142)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(29-169)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (4)
FernetEncryptionWrapper(11-84)_generate_encryption_key(87-101)encrypt_with_fernet(69-70)decrypt_with_fernet(72-76)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (3)
EncryptionError(25-26)BaseEncryptionWrapper(29-169)_decrypt_value(94-132)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionError(4-5)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (2)
FernetEncryptionWrapper(11-84)_generate_encryption_key(87-101)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
put(155-157)get(135-137)put_many(160-169)get_many(140-142)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (2)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (2)
BaseEncryptionWrapper(26-169)_decrypt_value(94-132)key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(173-178)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (4)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (1)
fernet(15-16)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(26-169)
🔇 Additional comments (5)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (2)
14-74: LGTM! Clean implementation of two-mode initialization.The wrapper properly validates mutual exclusivity between
fernetandsource_material/saltparameters, and the closure-based encryption/decryption functions correctly defer access toself.encryption_versionuntil runtime (after initialization completes).
77-88: LGTM! Secure key derivation with appropriate iteration count.The function correctly uses PBKDF2-HMAC-SHA256 with 1.2 million iterations (exceeds OWASP 2023 recommendation of 600K+) and properly encodes the output as urlsafe base64 for Fernet compatibility.
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (1)
133-156: LGTM! Comprehensive test of key derivation determinism.The test properly verifies that
_generate_encryption_keyproduces deterministic outputs and that different source materials or salts yield distinct keys.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (2)
21-22: LGTM! Clear type aliases for pluggable encryption.The function type definitions properly capture the encryption/decryption signatures, with
DecryptionFncorrectly including theencryption_versionparameter.
46-72: LGTM! Clean refactoring to function-based encryption.The initialization properly accepts pluggable encryption/decryption functions and stores them for later use, enabling support for different encryption backends beyond Fernet.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
4-5: ImportEncryptionErrorfrom shared module instead of defining locally in aio and sync wrappers.The
key-value-aioandkey-value-syncmodules define their ownEncryptionErrorextendingException(aio/base.py:21, sync/base.py:23), while the shared module definesEncryptionErrorextendingKeyValueOperationError(shared/encryption.py:4). This inconsistency creates conflicting exception hierarchies.Both aio and sync modules already import other error classes from shared (e.g.,
DecryptionError,EncryptionVersionError), so they should importEncryptionErrorfromkey_value.shared.errors.wrappers.encryptionto maintain a consistent exception hierarchy across the codebase.
♻️ Duplicate comments (3)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (2)
105-112: Consider error handling consistency.Same issue as the aio version: structural validation errors (lines 105-112) bypass the
raise_on_decryption_errorflag. Since this is auto-generated, fixing the original will resolve this.
23-24: Import EncryptionError from shared instead of defining locally.Same issue as the aio version: a local
EncryptionErroris defined here (lines 23-24) instead of importing fromkey_value.shared.errors.wrappers.encryptionwhere it's already defined. Since this is auto-generated from the aio version, fixing the originalkey-value-aio/src/key_value/aio/wrappers/encryption/base.pywill resolve this.key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
77-81: Verify version check logic is intentional.The version check uses
>(line 78), meaning it only rejects encryption versions newer than the wrapper's version, but accepts older versions. This allows a version 2 wrapper to decrypt version 1 data (backwards compatibility), but prevents a version 1 wrapper from attempting version 2 data.Is this asymmetric behavior intentional? If so, consider clarifying the error message from "is not supported" to "is newer than supported version".
🧹 Nitpick comments (2)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
16-17: Consider extendingDecryptionErrorfor better semantic hierarchy.Since
CorruptedEncryptionDataErroris raised when corrupted data is detected during decryption, it would be more semantically appropriate to extendDecryptionErrorinstead ofEncryptionErrordirectly.Apply this diff to improve the exception hierarchy:
-class CorruptedEncryptionDataError(EncryptionError): +class CorruptedEncryptionDataError(DecryptionError): """Exception raised when the encrypted data is corrupted."""key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
105-113: Consider error handling consistency.Lines 106-113 raise
CorruptedEncryptionDataErrorfor missing or invalidencryption_versionoutside the try-except block (lines 115-127), which means these errors bypass theraise_on_decryption_errorflag. This is inconsistent with other decryption failures (like corrupted base64 at line 116) that respect the flag.If this is intentional—treating structural corruption differently from decryption failures—consider adding a comment explaining the distinction. Otherwise, these checks could be moved inside the try-except to unify error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_encryption.py(5 hunks)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py(1 hunks)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(25-164)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (3)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (11)
fernet(15-16)store(22-23)test_encryption_encrypts_value(25-43)test_encryption_with_wrong_encryption_version(45-55)test_encryption_with_string_key(57-71)test_encryption_many_operations(73-88)test_decryption_handles_unencrypted_data(90-98)test_decryption_handles_corrupted_data(100-111)test_decryption_ignores_corrupted_data(113-124)test_decryption_with_multi_fernet(126-137)test_decryption_with_wrong_key_raises_error(139-151)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (2)
FernetEncryptionWrapper(16-79)_generate_encryption_key(82-93)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (2)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
EncryptionVersionError(12-13)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
BaseEncryptionWrapper(27-163)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
SerializationError(10-11)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (3)
CorruptedEncryptionDataError(16-17)DecryptionError(8-9)EncryptionError(4-5)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
EncryptionError(21-22)BaseEncryptionWrapper(25-164)_encrypt_value(61-91)_decrypt_value(93-127)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (2)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
EncryptionError(21-22)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (1)
EncryptionError(23-24)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (3)
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
DecryptionError(8-9)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (2)
FernetEncryptionWrapper(13-89)_generate_encryption_key(92-106)key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
put(150-152)get(130-132)put_many(155-164)get_many(135-137)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (4)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
SerializationError(10-11)key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (3)
CorruptedEncryptionDataError(16-17)DecryptionError(8-9)EncryptionError(4-5)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(175-180)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/base.py (4)
EncryptionError(23-24)BaseEncryptionWrapper(27-163)_encrypt_value(63-90)_decrypt_value(92-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
🔇 Additional comments (10)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
86-91: LGTM! Good optimization for storage efficiency.Adding compact separators reduces the serialized JSON size by removing unnecessary whitespace. This is particularly beneficial given the PR's encryption focus, as it minimizes the size of data being encrypted and stored.
key-value/key-value-shared/src/key_value/shared/errors/wrappers/encryption.py (1)
12-14: LGTM!The new exception class is well-defined with a clear docstring. The hierarchy (extending
EncryptionError) is appropriate for version-related encryption errors.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (3)
50-62: LGTM! Validation logic is solid.The mutual exclusivity checks and empty-string validation (using
.strip()) properly prevent misconfiguration.
67-70: Verify version check logic is intentional.The version check uses
>(line 68), meaning it only rejects encryption versions newer than the wrapper's version, but accepts older versions. This allows a version 2 wrapper to decrypt version 1 data (backwards compatibility), but prevents a version 1 wrapper from attempting version 2 data.Is this asymmetric behavior intentional? If so, consider clarifying the error message from "is not supported" to "is newer than supported version".
82-93: LGTM! Key derivation is secure.PBKDF2 with 1.2M iterations and proper base64 encoding for Fernet compatibility.
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py (1)
129-164: LGTM! Delegation pattern is correct.The wrapper methods properly delegate to the underlying store while applying encryption/decryption.
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_encryption.py (1)
17-169: LGTM! Comprehensive test coverage.The tests cover encryption, decryption, version mismatches, MultiFernet support, corrupted data handling, and key generation. Using
IsStr(min_length=32)in snapshots (line 69) correctly avoids hardcoding non-deterministic encrypted values.key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (2)
60-72: LGTM! Validation logic is solid.The mutual exclusivity checks and empty-string validation (using
.strip()) properly prevent misconfiguration.
92-106: LGTM! Key derivation is secure.PBKDF2 with 1.2M iterations and proper base64 encoding for Fernet compatibility.
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (1)
14-177: LGTM! Comprehensive test coverage.The tests cover encryption, decryption, version mismatches, MultiFernet support, corrupted data handling, and key generation. Using
IsStr(min_length=32)in snapshots (line 69) correctly avoids hardcoding non-deterministic encrypted values.
Summary by CodeRabbit
Release Notes
New Features
Documentation