From 7942d653a6812c5a31b8586232dac94dd13f7339 Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:00:58 -0400 Subject: [PATCH 1/6] Preserve timezone-aware encrypted datetime round trips --- CHANGELOG.md | 2 + tink_fields/fields.py | 24 +++++++++-- tink_fields/test/models.py | 4 ++ tink_fields/test/test_datetimes.py | 67 ++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 tink_fields/test/test_datetimes.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cc35bd..20add07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Restore the database timezone when decrypting naive datetime representations under `USE_TZ=True`, preserving instants across reads, re-saves, and deterministic lookups without rewriting stored ciphertext. + - Reject inherited JSON/date transforms and late-registered plaintext lookups on encrypted columns; keep deterministic exact and SQL null lookups explicit. ## [0.4.0] - 2026-08-01 diff --git a/tink_fields/fields.py b/tink_fields/fields.py index d1cf9e7..d7c9ee0 100644 --- a/tink_fields/fields.py +++ b/tink_fields/fields.py @@ -11,6 +11,7 @@ from collections import OrderedDict from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass +from datetime import datetime from os import PathLike from pathlib import Path from threading import RLock @@ -21,6 +22,7 @@ from django.core.exceptions import FieldError, ImproperlyConfigured from django.db import models from django.db.models.lookups import Exact, IsNull, Lookup +from django.utils import timezone from django.utils.encoding import force_bytes, force_str from django.utils.functional import cached_property from tink import JsonKeysetReader, TinkError, aead, cleartext_keyset_handle, daead, read_keyset_handle @@ -428,9 +430,13 @@ def from_db_value( """ if value is not None: decrypted = self._keyset_manager.aead_primitive.decrypt(bytes(value), self._get_aad()) - return self.to_python(self._to_python_prepare(decrypted)) + return self._convert_decrypted_value(decrypted, connection) return None + def _convert_decrypted_value(self, value: bytes, connection: Any) -> Any: + """Restore the Python value after decrypting a binary database value.""" + return self.to_python(self._to_python_prepare(value)) + @cached_property def validators(self) -> list[Any]: """Get field validators. @@ -496,6 +502,14 @@ def as_sql(self, compiler: Any, connection: Any) -> tuple[str, tuple[Any, ...]]: return f"{lhs_sql} {rhs_sql}", (*lhs_params, *rhs_params) +def _restore_datetime_timezone(value: datetime, connection: Any) -> datetime: + # Binary columns skip the backend's DateTimeField result converters. + # Use the database timezone that was used to prepare existing ciphertext. + if settings.USE_TZ and timezone.is_naive(value): + return timezone.make_aware(value, connection.timezone) + return value + + # Field implementations class EncryptedTextField(EncryptedField, models.TextField): """Encrypted text field.""" @@ -595,7 +609,8 @@ class EncryptedDateField(EncryptedField, models.DateField): class EncryptedDateTimeField(EncryptedField, models.DateTimeField): """Encrypted datetime field.""" - pass + def _convert_decrypted_value(self, value: bytes, connection: Any) -> datetime: + return _restore_datetime_timezone(super()._convert_decrypted_value(value, connection), connection) class EncryptedBinaryField(EncryptedField, models.BinaryField): @@ -676,7 +691,7 @@ def from_db_value( """ if value is not None: decrypted = self._keyset_manager.daead_primitive.decrypt_deterministically(bytes(value), self._get_aad()) - return self.to_python(self._to_python_prepare(decrypted)) + return self._convert_decrypted_value(decrypted, connection) return None @@ -726,4 +741,5 @@ class DeterministicEncryptedDateField(DeterministicEncryptedField, models.DateFi class DeterministicEncryptedDateTimeField(DeterministicEncryptedField, models.DateTimeField): """Deterministic encrypted datetime field.""" - pass + def _convert_decrypted_value(self, value: bytes, connection: Any) -> datetime: + return _restore_datetime_timezone(super()._convert_decrypted_value(value, connection), connection) diff --git a/tink_fields/test/models.py b/tink_fields/test/models.py index ff444a7..5c32012 100644 --- a/tink_fields/test/models.py +++ b/tink_fields/test/models.py @@ -95,3 +95,7 @@ class DeterministicEncryptedExtended(models.Model): class DeterministicEncryptedUnique(models.Model): value = fields.DeterministicEncryptedCharField(max_length=25, keyset="deterministic", unique=True) + + +class DeterministicEncryptedDateTime(models.Model): + value = fields.DeterministicEncryptedDateTimeField(keyset="deterministic") diff --git a/tink_fields/test/test_datetimes.py b/tink_fields/test/test_datetimes.py new file mode 100644 index 0000000..186f328 --- /dev/null +++ b/tink_fields/test/test_datetimes.py @@ -0,0 +1,67 @@ +"""Timezone conversions must survive storage in a binary column.""" + +from datetime import datetime +from unittest.mock import patch +from zoneinfo import ZoneInfo + +import pytest +from django.db import connection +from django.test import override_settings +from django.utils import timezone +from django.utils.encoding import force_bytes + +from tink_fields import DeterministicEncryptedDateTimeField, EncryptedDateTimeField + +from . import models + + +@pytest.mark.django_db +@pytest.mark.parametrize("model", [models.EncryptedDateTime, models.DeterministicEncryptedDateTime]) +@pytest.mark.parametrize("database_timezone", ["UTC", "Asia/Kolkata"]) +@override_settings(USE_TZ=True, TIME_ZONE="America/New_York") +def test_aware_datetime_round_trip_and_resave(model, database_timezone): + original = datetime(2026, 9, 5, 12, 34, 56, 123456, tzinfo=ZoneInfo("Asia/Tokyo")) + with patch.object(connection, "timezone", ZoneInfo(database_timezone)): + instance = model.objects.create(value=original) + with connection.cursor() as cursor: + cursor.execute(f"SELECT value FROM {model._meta.db_table} WHERE id = %s", [instance.pk]) + ciphertext = bytes(cursor.fetchone()[0]) + assert force_bytes(str(original)) not in ciphertext + + instance.refresh_from_db() + assert timezone.is_aware(instance.value) + assert instance.value == original + assert model.objects.values_list("value", flat=True).get(pk=instance.pk) == original + + instance.save() + instance.refresh_from_db() + assert instance.value == original + if model is models.DeterministicEncryptedDateTime: + assert model.objects.get(value=original).pk == instance.pk + assert model.objects.get(value=instance.value).pk == instance.pk + with connection.cursor() as cursor: + cursor.execute(f"SELECT value FROM {model._meta.db_table} WHERE id = %s", [instance.pk]) + assert bytes(cursor.fetchone()[0]) == ciphertext + + +@pytest.mark.parametrize("field_cls", [EncryptedDateTimeField, DeterministicEncryptedDateTimeField]) +@pytest.mark.parametrize("plaintext", ["2026-09-05 03:04:05.123456", "2026-09-05 08:34:05.123456+05:30"]) +@override_settings(USE_TZ=True, TIME_ZONE="America/New_York") +def test_existing_datetime_ciphertext_remains_readable(field_cls, plaintext): + deterministic = field_cls is DeterministicEncryptedDateTimeField + field = field_cls(keyset="deterministic" if deterministic else "default") + if deterministic: + primitive = field._keyset_manager.daead_primitive + ciphertext = primitive.encrypt_deterministically(force_bytes(plaintext), b"") + else: + ciphertext = field._keyset_manager.aead_primitive.encrypt(force_bytes(plaintext), b"") + result = field.from_db_value(ciphertext, None, connection) + assert result == datetime(2026, 9, 5, 3, 4, 5, 123456, tzinfo=ZoneInfo("UTC")) + + +@pytest.mark.parametrize("field_cls", [EncryptedDateTimeField, DeterministicEncryptedDateTimeField]) +@pytest.mark.parametrize("value", [None, datetime(2026, 9, 5, 12, 34, 56)]) +@override_settings(USE_TZ=False) +def test_naive_datetime_and_null_round_trip(field_cls, value): + field = field_cls(keyset="deterministic" if field_cls is DeterministicEncryptedDateTimeField else "default") + assert field.from_db_value(field.get_db_prep_save(value, connection), None, connection) == value From 9942aac46131d5c7d32f3e25280161d88dae6829 Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:05:59 -0400 Subject: [PATCH 2/6] Share cached primitives and synchronize keyset invalidation --- CHANGELOG.md | 6 ++ README.md | 2 + benchmarks/keyset_cache.py | 37 +++++++ tink_fields/fields.py | 149 +++++++++++++------------- tink_fields/test/test_keyset_cache.py | 118 ++++++++++++++++++++ 5 files changed, 240 insertions(+), 72 deletions(-) create mode 100644 benchmarks/keyset_cache.py create mode 100644 tink_fields/test/test_keyset_cache.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 20add07..e8ce014 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Share AEAD and deterministic AEAD primitives across fields using the same cached keyset, avoiding repeated wrapper construction while retaining bounded caching and weak manager tracking. + ### Fixed +- Synchronize primitive construction with cache invalidation so an in-flight load cannot republish a stale primitive after `clear_keyset_cache()`. + - Restore the database timezone when decrypting naive datetime representations under `USE_TZ=True`, preserving instants across reads, re-saves, and deterministic lookups without rewriting stored ciphertext. - Reject inherited JSON/date transforms and late-registered plaintext lookups on encrypted columns; keep deterministic exact and SQL null lookups explicit. diff --git a/README.md b/README.md index 4702815..a400784 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,8 @@ from tink_fields import clear_keyset_cache clear_keyset_cache() ``` +Cache invalidation is synchronized with keyset loading and primitive construction. Operations that already obtained a primitive may finish with the old key; subsequent field operations load the replacement. The cache is local to each process, so reload or restart every worker. + Changing `keyset=` does not re-encrypt existing rows; it only changes how future reads and writes are processed. Likewise, changing an existing plaintext Django field to an encrypted field requires an explicit staged data migration. Back up data and test recovery before any key or ciphertext migration. ## Security limitations diff --git a/benchmarks/keyset_cache.py b/benchmarks/keyset_cache.py new file mode 100644 index 0000000..3dcdbdc --- /dev/null +++ b/benchmarks/keyset_cache.py @@ -0,0 +1,37 @@ +"""Run with `python -m benchmarks.keyset_cache` from the repository root.""" + +from pathlib import Path +from statistics import median +from timeit import repeat + +from django.conf import settings + +from tink_fields.fields import KeysetManager + + +def main() -> None: + settings.configure( + TINK_FIELDS_CONFIG={ + "default": { + "path": Path(__file__).resolve().parents[1] / "tink_fields/test/test_plaintext_keyset.json", + "cleartext": True, + } + } + ) + + def initialize_fields() -> None: + KeysetManager.clear_cache() + managers = [KeysetManager("default") for _ in range(100)] + for manager in managers: + _ = manager.aead_primitive + + cold = median(repeat(initialize_fields, number=20, repeat=7)) / 20 + manager = KeysetManager("default") + _ = manager.aead_primitive + warm = median(repeat(lambda: manager.aead_primitive.encrypt(b"secret", b""), number=100_000, repeat=7)) / 100_000 + print(f"Initialize 100 fields sharing a keyset: {cold * 1_000:.3f} ms") + print(f"Warm primitive access and encryption: {warm * 1_000_000:.3f} us") + + +if __name__ == "__main__": + main() diff --git a/tink_fields/fields.py b/tink_fields/fields.py index d7c9ee0..66fb3ac 100644 --- a/tink_fields/fields.py +++ b/tink_fields/fields.py @@ -10,12 +10,12 @@ import json from collections import OrderedDict from collections.abc import Callable, Mapping, Sequence -from dataclasses import dataclass +from dataclasses import dataclass, field from datetime import datetime from os import PathLike from pathlib import Path from threading import RLock -from typing import Any, ClassVar, cast +from typing import Any, ClassVar, TypeVar, cast from weakref import WeakSet from django.conf import settings @@ -119,6 +119,15 @@ def validate(self) -> None: raise ImproperlyConfigured("Encrypted keysets must specify `master_key_aead`.") +Primitive = TypeVar("Primitive", aead.Aead, daead.DeterministicAead) + + +@dataclass +class _KeysetEntry: + handle: Any + primitives: dict[type[Any], Any] = field(default_factory=dict) + + class KeysetManager: """Manages Tink keyset handles and primitives. @@ -128,7 +137,7 @@ class KeysetManager: _cache_size: ClassVar[int] = 32 _cache_lock: ClassVar[RLock] = RLock() - _handle_cache: ClassVar[OrderedDict[tuple[Any, ...], Any]] = OrderedDict() + _handle_cache: ClassVar[OrderedDict[tuple[Any, ...], _KeysetEntry]] = OrderedDict() _managers: ClassVar[WeakSet[KeysetManager]] = WeakSet() def __init__(self, keyset_name: str, aad_callback: AADCallback = _default_aad_callback) -> None: @@ -140,7 +149,7 @@ def __init__(self, keyset_name: str, aad_callback: AADCallback = _default_aad_ca """ self.keyset_name = keyset_name self.aad_callback = aad_callback - self._keyset_handle = None + self._entry: _KeysetEntry | None = None with self._cache_lock: self._managers.add(self) @@ -183,84 +192,80 @@ def clear_cache(cls) -> None: with cls._cache_lock: cls._handle_cache.clear() for manager in list(cls._managers): - manager._keyset_handle = None - manager.__dict__.pop("aead_primitive", None) - manager.__dict__.pop("daead_primitive", None) + manager._entry = None - def _get_tink_keyset_handle(self) -> Any: - """Read the configuration for the requested keyset and return a keyset handle. + def _get_keyset_entry(self) -> _KeysetEntry: + """Load or reuse a keyset entry while holding the cache lock.""" + if self._entry is not None: + return self._entry - Returns: - KeysetHandle: The configured Tink keyset handle - - Raises: - ImproperlyConfigured: If keyset configuration is invalid or missing - """ - if self._keyset_handle is None: - keyset_config = self._get_keyset_config() - keyset_path = Path(keyset_config.path).expanduser().resolve() - try: - stat = keyset_path.stat() - except OSError as error: - raise ImproperlyConfigured(f"Could not load keyset `{self.keyset_name}`.") from error - cache_key = ( - str(keyset_path), - stat.st_mtime_ns, - stat.st_size, - keyset_config.cleartext, - keyset_config.master_key_aead, - ) + keyset_config = self._get_keyset_config() + keyset_path = Path(keyset_config.path).expanduser().resolve() + try: + stat = keyset_path.stat() + except OSError as error: + raise ImproperlyConfigured(f"Could not load keyset `{self.keyset_name}`.") from error + cache_key = ( + str(keyset_path), + stat.st_mtime_ns, + stat.st_size, + keyset_config.cleartext, + keyset_config.master_key_aead, + ) + try: + hash(cache_key) + except TypeError: + cache_key = () + + cached_entry = self._handle_cache.get(cache_key) if cache_key else None + if cached_entry is not None: + self._handle_cache.move_to_end(cache_key) + self._entry = cached_entry + else: try: - hash(cache_key) - except TypeError: - cache_key = () - - with self._cache_lock: - cached_handle = self._handle_cache.get(cache_key) if cache_key else None - if cached_handle is not None: - self._handle_cache.move_to_end(cache_key) - self._keyset_handle = cached_handle + reader = JsonKeysetReader(keyset_path.read_text(encoding="utf-8")) + if keyset_config.cleartext: + handle = cleartext_keyset_handle.read(reader) else: - try: - reader = JsonKeysetReader(keyset_path.read_text(encoding="utf-8")) - if keyset_config.cleartext: - self._keyset_handle = cleartext_keyset_handle.read(reader) - else: - master_key_aead = keyset_config.master_key_aead - assert master_key_aead is not None - self._keyset_handle = read_keyset_handle(reader, master_key_aead) - except (OSError, TinkError) as error: - raise ImproperlyConfigured(f"Could not load keyset `{self.keyset_name}`.") from error - - if cache_key: - self._handle_cache[cache_key] = self._keyset_handle - self._handle_cache.move_to_end(cache_key) - while len(self._handle_cache) > self._cache_size: - self._handle_cache.popitem(last=False) - - return self._keyset_handle + master_key_aead = keyset_config.master_key_aead + assert master_key_aead is not None + handle = read_keyset_handle(reader, master_key_aead) + except (OSError, TinkError) as error: + raise ImproperlyConfigured(f"Could not load keyset `{self.keyset_name}`.") from error - @cached_property - def aead_primitive(self) -> aead.Aead: - """Get the AEAD primitive for encryption/decryption operations. + self._entry = _KeysetEntry(handle) + if cache_key: + self._handle_cache[cache_key] = self._entry + self._handle_cache.move_to_end(cache_key) + while len(self._handle_cache) > self._cache_size: + self._handle_cache.popitem(last=False) - Returns: - aead.Aead: The AEAD primitive instance - """ - return self._get_tink_keyset_handle().primitive(aead.Aead) + return self._entry - @cached_property - def daead_primitive(self) -> daead.DeterministicAead: - """Get the Deterministic AEAD primitive for encryption/decryption operations. + def _get_tink_keyset_handle(self) -> Any: + """Return this manager's configured handle.""" + with self._cache_lock: + return self._get_keyset_entry().handle - Returns: - daead.DeterministicAead: The Deterministic AEAD primitive instance + def _get_primitive(self, primitive_class: type[Primitive]) -> Primitive: + # Keep construction and publication atomic with respect to clear_cache(). + # cached_property publishes after its getter returns, outside this lock. + with self._cache_lock: + entry = self._get_keyset_entry() + if primitive_class not in entry.primitives: + entry.primitives[primitive_class] = entry.handle.primitive(primitive_class) + return entry.primitives[primitive_class] - Raises: - ImproperlyConfigured: If deterministic AEAD is not available or keyset doesn't support it - """ + @property + def aead_primitive(self) -> aead.Aead: + """Get the AEAD primitive shared by managers using this keyset.""" + return self._get_primitive(aead.Aead) + + @property + def daead_primitive(self) -> daead.DeterministicAead: + """Get the deterministic AEAD primitive shared by this keyset.""" try: - return self._get_tink_keyset_handle().primitive(daead.DeterministicAead) + return self._get_primitive(daead.DeterministicAead) except TinkError as error: raise ImproperlyConfigured( "Current keyset does not support deterministic AEAD. " diff --git a/tink_fields/test/test_keyset_cache.py b/tink_fields/test/test_keyset_cache.py new file mode 100644 index 0000000..ba8c995 --- /dev/null +++ b/tink_fields/test/test_keyset_cache.py @@ -0,0 +1,118 @@ +"""Shared primitive construction, bounded storage, and concurrent invalidation.""" + +import gc +import json +import weakref +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from threading import Event +from unittest.mock import patch + +import pytest +from django.test import override_settings +from tink import KeysetHandle, aead, daead, json_proto_keyset_format, new_keyset_handle, secret_key_access + +from tink_fields import clear_keyset_cache +from tink_fields.fields import KeysetManager + + +@pytest.fixture(autouse=True) +def empty_keyset_cache(): + clear_keyset_cache() + yield + clear_keyset_cache() + + +@pytest.mark.parametrize("keyset, attribute", [("default", "aead_primitive"), ("deterministic", "daead_primitive")]) +def test_managers_share_primitive_construction(keyset, attribute): + managers = [KeysetManager(keyset) for _ in range(100)] + original = KeysetHandle.primitive + with patch.object(KeysetHandle, "primitive", autospec=True, side_effect=original) as construct: + primitives = [getattr(manager, attribute) for manager in managers] + assert construct.call_count == 1 + assert all(primitive is primitives[0] for primitive in primitives) + + +@pytest.mark.parametrize("keyset, attribute", [("default", "aead_primitive"), ("deterministic", "daead_primitive")]) +def test_clear_cache_waits_for_inflight_primitive_construction(keyset, attribute): + manager = KeysetManager(keyset) + building = Event() + release = Event() + clearing = Event() + cleared = Event() + original = KeysetHandle.primitive + + def slow_primitive(handle, primitive_class): + building.set() + assert release.wait(5) + return original(handle, primitive_class) + + def clear(): + clearing.set() + clear_keyset_cache() + cleared.set() + + with ThreadPoolExecutor(max_workers=2) as pool: + with patch.object(KeysetHandle, "primitive", slow_primitive): + constructing = pool.submit(getattr, manager, attribute) + try: + assert building.wait(5) + invalidating = pool.submit(clear) + assert clearing.wait(5) + assert not cleared.wait(0.1), "cache cleared before the old primitive could finish publishing" + finally: + release.set() + old = constructing.result(timeout=5) + invalidating.result(timeout=5) + assert getattr(manager, attribute) is not old + + +@pytest.mark.parametrize( + "template, attribute, encrypt, decrypt", + [ + (aead.aead_key_templates.AES128_GCM, "aead_primitive", "encrypt", "decrypt"), + ( + daead.deterministic_aead_key_templates.AES256_SIV, + "daead_primitive", + "encrypt_deterministically", + "decrypt_deterministically", + ), + ], +) +def test_rotation_reloads_all_active_managers(tmp_path, template, attribute, encrypt, decrypt): + old_keyset = json.loads(json_proto_keyset_format.serialize(new_keyset_handle(template), secret_key_access.TOKEN)) + new_keyset = json.loads(json_proto_keyset_format.serialize(new_keyset_handle(template), secret_key_access.TOKEN)) + new_keyset["key"].extend(old_keyset["key"]) + path = tmp_path / "keys.json" + path.write_text(json.dumps(old_keyset), encoding="utf-8") + with override_settings(TINK_FIELDS_CONFIG={"default": {"path": path, "cleartext": True}}): + managers = [KeysetManager("default") for _ in range(3)] + before = [getattr(manager, attribute) for manager in managers] + ciphertext = getattr(before[0], encrypt)(b"secret", b"aad") + replacement = tmp_path / "replacement.json" + replacement.write_text(json.dumps(new_keyset), encoding="utf-8") + replacement.replace(path) + clear_keyset_cache() + for manager, old_primitive in zip(managers, before, strict=True): + primitive = getattr(manager, attribute) + assert primitive is not old_primitive + assert getattr(primitive, decrypt)(ciphertext, b"aad") == b"secret" + assert getattr(primitive, encrypt)(b"secret", b"aad")[1:5] == new_keyset["primaryKeyId"].to_bytes(4, "big") + + +def test_shared_cache_is_bounded_and_does_not_retain_managers(tmp_path): + config = {} + source = Path(__file__).with_name("test_plaintext_keyset.json").read_text(encoding="utf-8") + for index in range(3): + path = tmp_path / f"keys-{index}.json" + path.write_text(source, encoding="utf-8") + config[str(index)] = {"path": path, "cleartext": True} + with override_settings(TINK_FIELDS_CONFIG=config), patch.object(KeysetManager, "_cache_size", 2): + for name in config: + manager = KeysetManager(name) + _ = manager.aead_primitive + assert len(KeysetManager._handle_cache) == 2 + reference = weakref.ref(manager) + del manager + gc.collect() + assert reference() is None From 0e10776c338d39300fa322b0083728e1b171a86e Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:11:48 -0400 Subject: [PATCH 3/6] Encrypt binary contents before PostgreSQL driver adaptation --- CHANGELOG.md | 2 + pyproject.toml | 1 + tink_fields/fields.py | 7 ++++ tink_fields/test/test_binary_adaptation.py | 46 ++++++++++++++++++++++ 4 files changed, 56 insertions(+) create mode 100644 tink_fields/test/test_binary_adaptation.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e8ce014..576e8d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Encrypt binary buffer contents before driver adaptation, fixing PostgreSQL writes that encrypted the string representation of a `psycopg.Binary` adapter. Previously corrupted values require application-specific recovery; this fix does not rewrite stored rows. + - Synchronize primitive construction with cache invalidation so an in-flight load cannot republish a stale primitive after `clear_keyset_cache()`. - Restore the database timezone when decrypting naive datetime representations under `USE_TZ=True`, preserving instants across reads, re-saves, and deterministic lookups without rewriting stored ciphertext. diff --git a/pyproject.toml b/pyproject.toml index af53b0a..819f40c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,6 +52,7 @@ test = [ "pytest>=8.3", "pytest-cov>=6", "pytest-django>=4.9", + "psycopg[binary]>=3.2", ] [tool.setuptools] diff --git a/tink_fields/fields.py b/tink_fields/fields.py index 66fb3ac..d2d24e3 100644 --- a/tink_fields/fields.py +++ b/tink_fields/fields.py @@ -625,6 +625,13 @@ class EncryptedBinaryField(EncryptedField, models.BinaryField): not be converted to strings during decryption. """ + def _prepare_value_for_database(self, value: Any, connection: Any) -> bytes | None: + """Keep buffer contents as plaintext; adapt only the final ciphertext.""" + value = self.get_prep_value(value) + if value is None: + return None + return bytes(memoryview(value)) + def _to_python_prepare(self, value: bytes) -> bytes: """Prepare decrypted value for to_python conversion. diff --git a/tink_fields/test/test_binary_adaptation.py b/tink_fields/test/test_binary_adaptation.py new file mode 100644 index 0000000..67076e7 --- /dev/null +++ b/tink_fields/test/test_binary_adaptation.py @@ -0,0 +1,46 @@ +"""Use the real PostgreSQL driver adapter without needing a server.""" + +from unittest.mock import patch + +import pytest +from django.db import connection +from django.db.backends.postgresql.base import DatabaseWrapper + +from tink_fields import EncryptedBinaryField + +from . import models + + +@pytest.mark.parametrize("value", [b"", b"binary\x00\xff", bytearray(b"mutable\xff"), memoryview(b"view\x00")]) +def test_postgresql_binary_adapter_wraps_ciphertext_only(value): + postgres = DatabaseWrapper({}) + field = EncryptedBinaryField() + with patch.object(postgres.Database, "Binary", wraps=postgres.Database.Binary) as binary: + adapted = field.get_db_prep_save(value, postgres) + assert field.from_db_value(adapted.obj, None, postgres) == bytes(value) + assert binary.call_count == 1 + assert bytes(adapted.obj) != bytes(value) + + +def test_postgresql_binary_null_skips_adaptation(): + postgres = DatabaseWrapper({}) + with patch.object(postgres.Database, "Binary", wraps=postgres.Database.Binary) as binary: + assert EncryptedBinaryField().get_db_prep_save(None, postgres) is None + binary.assert_not_called() + + +@pytest.mark.django_db +@pytest.mark.parametrize("value", [b"", b"binary\x00\xff", bytearray(b"mutable\xff"), memoryview(b"view\x00")]) +def test_binary_buffer_values_round_trip_in_database(value): + instance = models.EncryptedBinary.objects.create(value=value) + instance.refresh_from_db() + assert instance.value == bytes(value) + with connection.cursor() as cursor: + cursor.execute(f"SELECT value FROM {models.EncryptedBinary._meta.db_table} WHERE id = %s", [instance.pk]) + assert bytes(cursor.fetchone()[0]) != bytes(value) + + +@pytest.mark.parametrize("value", ["not binary", 3]) +def test_non_buffer_values_are_rejected(value): + with pytest.raises(TypeError): + EncryptedBinaryField().get_db_prep_save(value, connection) From a52566415c7656d9765bb7d5fa1a5526dbf821f6 Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:15:29 -0400 Subject: [PATCH 4/6] Validate resolved field options and disable randomized slug indexes --- CHANGELOG.md | 2 + README.md | 2 +- tink_fields/fields.py | 22 +++++++-- tink_fields/test/test_field_options.py | 64 ++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 tink_fields/test/test_field_options.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 576e8d7..2569261 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Validate positional database options after Django resolves them. Randomized slug fields now default to `db_index=False`; existing applications should generate and apply the resulting index-removal migration. + - Encrypt binary buffer contents before driver adaptation, fixing PostgreSQL writes that encrypted the string representation of a `psycopg.Binary` adapter. Previously corrupted values require application-specific recovery; this fix does not rewrite stored rows. - Synchronize primitive construction with cache invalidation so an in-flight load cannot republish a stale primitive after `clear_keyset_cache()`. diff --git a/README.md b/README.md index a400784..17f051b 100644 --- a/README.md +++ b/README.md @@ -93,7 +93,7 @@ Values are ordinary Python objects on model instances. Django validates them usi | `EncryptedURLField` | `URLField` | | `EncryptedUUIDField` | `UUIDField` | -Randomized fields deliberately reject `primary_key`, `unique`, `db_index`, and `db_default`. They support `isnull` queries, including the equivalent `field=None`; every lookup that compares values raises `FieldError`. Inherited JSON key lookups, date transforms, and custom registered lookups are also rejected because they would operate on ciphertext. Database expressions such as `F()` assignments are also rejected because the database cannot encrypt them. +Randomized fields deliberately reject `primary_key`, `unique`, `db_index`, and `db_default`. They support `isnull` queries, including the equivalent `field=None`; every lookup that compares values raises `FieldError`. `EncryptedSlugField` defaults to `db_index=False`, unlike Django's plaintext slug field. Applications upgrading from an earlier version should run `makemigrations` and review the generated index-removal migration. Inherited JSON key lookups, date transforms, and custom registered lookups are also rejected because they would operate on ciphertext. Database expressions such as `F()` assignments are also rejected because the database cannot encrypt them. ### Deterministic fields and exact lookups diff --git a/tink_fields/fields.py b/tink_fields/fields.py index d2d24e3..60b2262 100644 --- a/tink_fields/fields.py +++ b/tink_fields/fields.py @@ -21,6 +21,7 @@ from django.conf import settings from django.core.exceptions import FieldError, ImproperlyConfigured from django.db import models +from django.db.models.fields import NOT_PROVIDED from django.db.models.lookups import Exact, IsNull, Lookup from django.utils import timezone from django.utils.encoding import force_bytes, force_str @@ -305,10 +306,11 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: keyset: Name of the keyset to use (default: "default") aad_callback: Callable for additional authenticated data """ - # Validate unsupported properties - for prop in self._unsupported_properties: - if (prop == "db_default" and prop in kwargs) or kwargs.get(prop): - raise ImproperlyConfigured(f"Field `{self.__class__.__name__}` does not support property `{prop}`.") + # SlugField normally defaults to an index, which randomized ciphertext + # cannot use for meaningful equality queries. + if isinstance(self, models.SlugField) and "db_index" in self._unsupported_properties: + kwargs.setdefault("db_index", False) + has_db_default = "db_default" in kwargs # Extract custom parameters self._keyset = kwargs.pop("keyset", DEFAULT_KEYSET) @@ -322,6 +324,18 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: # Call parent constructor first super().__init__(*args, **kwargs) + # Inspect resolved options so positional arguments and parent defaults + # receive the same checks as keyword arguments. Check primary_key before + # unique, since Django treats all primary keys as unique. + for prop in ("primary_key", "db_index", "unique", "db_default"): + if prop not in self._unsupported_properties: + continue + enabled = ( + (has_db_default or self.db_default is not NOT_PROVIDED) if prop == "db_default" else getattr(self, prop) + ) + if enabled: + raise ImproperlyConfigured(f"Field `{self.__class__.__name__}` does not support property `{prop}`.") + self._keyset_manager = KeysetManager(self._keyset, self._aad_callback) def deconstruct(self) -> tuple[str | None, str, Sequence[Any], dict[str, Any]]: diff --git a/tink_fields/test/test_field_options.py b/tink_fields/test/test_field_options.py new file mode 100644 index 0000000..03ed8bb --- /dev/null +++ b/tink_fields/test/test_field_options.py @@ -0,0 +1,64 @@ +"""Validate field options after Django has resolved arguments and defaults.""" + +from inspect import signature + +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.db import connection +from django.db.models import Field + +from tink_fields import DeterministicEncryptedCharField, EncryptedSlugField, EncryptedTextField + +from . import models + + +@pytest.mark.parametrize( + "args, option", + [ + ((None, None, True), "primary_key"), + ((None, None, False, None, True), "unique"), + ((None, None, False, None, False, False, False, True), "db_index"), + ], +) +def test_randomized_fields_reject_positional_database_options(args, option): + with pytest.raises(ImproperlyConfigured, match=f"property `{option}`"): + EncryptedTextField(*args) + + +def test_deterministic_fields_reject_positional_primary_keys(): + with pytest.raises(ImproperlyConfigured, match="property `primary_key`"): + DeterministicEncryptedCharField(None, None, True, 25) + + +def test_deterministic_fields_keep_positional_indexes_and_uniqueness(): + field = DeterministicEncryptedCharField(None, None, False, 25, True, False, False, True) + assert field.unique is True + assert field.db_index is True + + +def test_randomized_slug_defaults_to_no_index_and_serializes_it(): + field = EncryptedSlugField() + assert field.db_index is False + assert field.clone().db_index is False + assert field.deconstruct()[3]["db_index"] is False + + +def test_randomized_slug_still_rejects_explicit_indexes(): + with pytest.raises(ImproperlyConfigured, match="property `db_index`"): + EncryptedSlugField(db_index=True) + + +@pytest.mark.django_db +def test_randomized_slug_has_no_database_index(): + with connection.cursor() as cursor: + constraints = connection.introspection.get_constraints(cursor, models.EncryptedExtended._meta.db_table) + assert not any(constraint["index"] and constraint["columns"] == ["slug"] for constraint in constraints.values()) + + +@pytest.mark.parametrize("field_cls", [EncryptedTextField, DeterministicEncryptedCharField]) +def test_positional_database_defaults_are_rejected(field_cls): + parameters = signature(Field).parameters + arguments = [parameter.default for parameter in parameters.values()] + arguments[list(parameters).index("db_default")] = None + with pytest.raises(ImproperlyConfigured, match="property `db_default`"): + field_cls(*arguments) From 7975de2ed10cc92021cb5595d16d398924671a31 Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:22:51 -0400 Subject: [PATCH 5/6] Modernize keyset parsing and verify supported dependency bounds --- .github/workflows/ci.yml | 24 +++++ CHANGELOG.md | 5 + README.md | 6 +- docs/code-review-2026-09.md | 119 ++++++++++++++++++++++++ tink_fields/fields.py | 35 ++++--- tink_fields/test/test_keyset_loading.py | 84 +++++++++++++++++ tox.ini | 4 + 7 files changed, 264 insertions(+), 13 deletions(-) create mode 100644 docs/code-review-2026-09.md create mode 100644 tink_fields/test/test_keyset_loading.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 140a378..ed83ef1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,8 +25,12 @@ jobs: django: "5.2" - python: "3.12" django: "6.0" + - python: "3.13" + django: "5.2" - python: "3.13" django: "6.0" + - python: "3.14" + django: "5.2" - python: "3.14" django: "6.0" @@ -53,6 +57,26 @@ jobs: PYTHONWARNINGS: default run: python -m pytest -c example_project/pytest.ini example_project/example_app/tests + minimum-tink: + name: Python 3.10 / Django 5.2 / Tink 1.13.0 + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.10" + cache: pip + cache-dependency-path: pyproject.toml + - name: Install minimum supported Tink + run: python -m pip install -e ".[test]" "Django~=5.2.0" "tink==1.13.0" + - name: Test minimum supported Tink + env: + PYTHONWARNINGS: default + run: | + python -m pytest + python -m pytest -c example_project/pytest.ini example_project/example_app/tests + quality: runs-on: ubuntu-latest timeout-minutes: 10 diff --git a/CHANGELOG.md b/CHANGELOG.md index 2569261..fa53e10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Load existing JSON keysets through Tink's explicit `json_proto_keyset_format` APIs, with typed handles and unchanged encrypted-keyset AAD. +- Exercise all advertised Python/Django combinations and the minimum supported Tink 1.13.0 in CI and tox. + - Share AEAD and deterministic AEAD primitives across fields using the same cached keyset, avoiding repeated wrapper construction while retaining bounded caching and weak manager tracking. ### Fixed +- Expand user-relative keyset paths before validation and report invalid paths, non-UTF-8 keysets, invalid master primitives, and incompatible AEAD keysets as configuration errors. + - Validate positional database options after Django resolves them. Randomized slug fields now default to `db_index=False`; existing applications should generate and apply the resulting index-removal migration. - Encrypt binary buffer contents before driver adaptation, fixing PostgreSQL writes that encrypted the string representation of a `psycopg.Binary` adapter. Previously corrupted values require application-specific recovery; this fix does not rewrite stored rows. diff --git a/README.md b/README.md index 17f051b..8e5f652 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ class Customer(models.Model): birth_date = EncryptedDateField(null=True) ``` -Values are ordinary Python objects on model instances. Django validates them using the corresponding built-in field's validators, encrypts them before database storage, and decrypts them when loading rows. +Values are ordinary Python objects on model instances. The fields use the corresponding built-in field's validators, encrypt values before database storage, and decrypt them when loading rows. As with ordinary Django models, `save()` does not call `full_clean()` automatically; use a validated model form or call `full_clean()` explicitly when validation is required. ### Randomized fields @@ -169,6 +169,8 @@ clear_keyset_cache() Cache invalidation is synchronized with keyset loading and primitive construction. Operations that already obtained a primitive may finish with the old key; subsequent field operations load the replacement. The cache is local to each process, so reload or restart every worker. +For deterministic fields, promoting a new primary key changes the ciphertext used by exact lookups. Retaining old keys permits decryption but does not make new equality queries match rows encrypted under an old primary key, and a unique ciphertext index cannot enforce plaintext uniqueness across key generations. Plan a coordinated data migration before rotating deterministic keys; cache invalidation alone does not solve this. + Changing `keyset=` does not re-encrypt existing rows; it only changes how future reads and writes are processed. Likewise, changing an existing plaintext Django field to an encrypted field requires an explicit staged data migration. Back up data and test recovery before any key or ciphertext migration. ## Security limitations @@ -178,7 +180,7 @@ Changing `keyset=` does not re-encrypt existing rows; it only changes how future - Encryption does not hide row existence, nullness, ciphertext length, access patterns, or—when deterministic encryption is used—equality patterns. - Ordering encrypted columns is permitted by databases but orders ciphertext, not plaintext, and has no useful application meaning. - AAD authenticates context but is not secret and is not stored automatically. -- Validation happens before storage but is not a substitute for authorization, logging controls, backups, or database security. +- Field validation requires a model form or an explicit `full_clean()` call; encryption is not a substitute for application validation, authorization, logging controls, backups, or database security. See [SECURITY.md](SECURITY.md) for vulnerability reporting and supported releases. diff --git a/docs/code-review-2026-09.md b/docs/code-review-2026-09.md new file mode 100644 index 0000000..d7dc946 --- /dev/null +++ b/docs/code-review-2026-09.md @@ -0,0 +1,119 @@ +# Code review and landing plan — 2026-09-05 + +Reviewed the library, tests, example project, packaging, and CI from `main` at +`72c1661`. The original suite passed 76 library tests with 95.28% coverage, but +missed several value-conversion and ORM restriction failures. This stack adds +behavioral regressions instead of treating coverage percentage as proof of +correctness. + +## Changes in this stack + +| Priority | Finding and evidence | Resolution | +| --- | --- | --- | +| High | JSON key lookups, date transforms, and late-registered lookups bypassed the ciphertext restrictions. Twelve new cases failed to raise `FieldError`. | [PR #7](https://github.com/script3r/django-tink-fields/pull/7): explicit lookup allowlist and concrete equality classes. | +| High | With `USE_TZ=True`, SQLite datetime reads lost timezone information. Re-saving under a different default timezone could shift the instant. Six new cases failed. | [PR #8](https://github.com/script3r/django-tink-fields/pull/8): restore the database timezone after decrypting naive datetime representations, preserving serialization. | +| High | An in-flight `cached_property` getter could publish an old primitive after cache invalidation returned. | [PR #9](https://github.com/script3r/django-tink-fields/pull/9): synchronize construction/publication with invalidation; test concurrent loads and actual rotation for both primitive types. | +| Medium | One hundred fields sharing a keyset constructed 100 separate primitives. | PR #9: share one primitive per type per cached keyset, retaining bounded caching and weak manager references. | +| High | PostgreSQL binary writes encrypted the string representation of `psycopg.Binary`, corrupting the original contents. Four real-driver cases failed. | [PR #10](https://github.com/script3r/django-tink-fields/pull/10): convert buffer contents before encryption, and adapt only final ciphertext. | +| Medium | Positional database options bypassed validation; randomized slug fields silently inherited an index. | [PR #11](https://github.com/script3r/django-tink-fields/pull/11): validate resolved options and serialize `db_index=False` for encrypted slugs. | +| Medium | User-relative keyset paths were checked before expansion; invalid encodings and incompatible AEAD primitives escaped as low-level exceptions. | Final keyset-loading PR: normalize paths and improve configuration errors. | +| Medium | CI omitted two advertised Python/Django combinations and never pinned the minimum Tink version. | Final PR: add Python 3.13/3.14 with Django 5.2 and a Tink 1.13.0 job; keep tox aligned. | +| Low | Keyset loading used the older reader/handle API and untyped handles. | Final PR: use Tink's explicit JSON format API and `KeysetHandle` annotations, with legacy keyset interoperability coverage. | + +## Performance evidence + +Run `python -m benchmarks.keyset_cache` from the repository root. The script +reports the median of seven samples, with 20 initializations or 100,000 warm +encryptions per sample. On this Mac with Python 3.14, at the cache PR boundary: + +| Operation | Before | After | +| --- | ---: | ---: | +| Initialize 100 managers sharing one keyset | 2.227 ms | 1.466 ms | +| Warm primitive access and encryption | 0.504 µs | 0.598 µs | +| Primitive constructions for 100 managers | 100 | 1 | + +The improvement is in initialization and wrapper reuse. Locking adds a small +steady-state cost; these measurements do not establish application throughput +or performance under contention. Encryption itself occurs outside the cache +lock. Calls that obtained an old primitive can finish with it, and every worker +process must reload or restart after rotation. + +## Landing and rollout + +Land the PRs in dependency order. Each PR targets its predecessor so its diff +contains only that change. After a parent lands, retarget the next PR to `main` +if GitHub has not done so automatically. Merge commits preserve the stack's +ancestry; squash or rebase merges require rebasing the remaining branches onto +the new `main` before landing them. + +- Existing correctly serialized ciphertext remains readable. No key or payload + format migration is introduced by the lookup, timezone, cache, or loading PRs. +- For encrypted slug fields, generate and apply the index-removal migration. + Positional configurations that violate documented restrictions now fail early. +- PostgreSQL binary rows already corrupted by adapter stringification require + application-specific recovery. In particular, a stored memoryview description + does not contain the original bytes and cannot be repaired by this patch. +- The final README corrects the validation claim: Django model `save()` does not + automatically call `full_clean()`. + +## Follow-up priorities and limits + +1. **High — deterministic rotation needs an explicit migration design.** A new + primary key produces different ciphertext. Retaining old enabled keys allows + decryption but does not make new exact lookups match old rows. A unique index + also cannot enforce plaintext uniqueness across key generations. The README + now explains this limitation. A future implementation needs a coordinated + rewrite, an explicit key-generation strategy, or a separately designed search + index; simply clearing the cache is insufficient. + +2. **High — define canonical deterministic representations before expanding + backend guarantees.** With Django's PostgreSQL adapter, equal aware datetime + instants expressed in Tokyo and UTC produce different encrypted bytes because + the serialized strings retain different offsets. This was reproduced without + a database server using the real PostgreSQL backend. Changing serialization + silently would invalidate equality against existing rows, so it needs a + versioned migration plan. UUID representations can also differ by backend. + +3. **Medium — add real PostgreSQL and MySQL server CI.** This stack tests SQLite + persistence and the real psycopg binary adapter, but does not claim server + integration coverage for PostgreSQL, MySQL, or Oracle. Add service-backed + tests for schema changes, constraints, datetime conversions, and binary + persistence before strengthening the documented backend support. + +4. **Medium — remove temporary field-type mutation during validator creation.** + `EncryptedField.validators` temporarily changes `_internal_type` on a shared + field instance. This is a potential concurrency hazard, not a reproduced + failure in this review. A replacement should construct the concrete field's + validators without changing shared metadata and retain backend range checks. + +5. **Low — improve typing and test organization incrementally.** The older + coverage-focused tests contain duplicate assertions and stale line-number + comments. Consolidate them around observable behavior while retaining the + new regression cases. Package a `py.typed` marker only after testing the + public Django field annotations with downstream type checkers. Module splits + should preserve public field import paths used in existing migrations. + +## Validation + +At the top of the stack, all **137 library tests** and **6 example integration +tests** pass locally on both Python 3.14 / Django 6.0 / Tink 1.16.1 and Python +3.10 / Django 5.2 / Tink 1.13.0. Library coverage is **97.62%**. Ruff lint/format +and Pyright pass. Distribution builds and strict Twine validation pass; the +tested environment has no known vulnerabilities reported by pip-audit, and +Bandit reports no medium/high findings. GitHub CI provides the remaining interpreter combinations; +consult each PR's checks for the status of its exact head commit. + +The tests cover raw ciphertext/SQL NULL storage, tamper detection, migrations, +real driver adaptation, timezone round trips, keyset interoperability, real key +rotation, bounded caching, and concurrent invalidation. They do not establish +formal cryptographic correctness or recovery of previously corrupted data. + +## Upstream references + +Tink's [Python keyset example](https://developers.google.com/tink/generate-plaintext-keyset) +uses `json_proto_keyset_format` with explicit secret-key access. The loading +change retains the empty associated data used for existing encrypted keysets. +[Django's custom field documentation](https://docs.djangoproject.com/en/6.0/howto/custom-model-fields/) +describes the separation between database preparation and conversion on reads; +its [model validation documentation](https://docs.djangoproject.com/en/6.0/ref/models/instances/#validating-objects) +explains that `save()` does not call `full_clean()` automatically. diff --git a/tink_fields/fields.py b/tink_fields/fields.py index 60b2262..b6f5a08 100644 --- a/tink_fields/fields.py +++ b/tink_fields/fields.py @@ -26,7 +26,7 @@ from django.utils import timezone from django.utils.encoding import force_bytes, force_str from django.utils.functional import cached_property -from tink import JsonKeysetReader, TinkError, aead, cleartext_keyset_handle, daead, read_keyset_handle +from tink import KeysetHandle, TinkError, aead, daead, json_proto_keyset_format, secret_key_access def _register_tink_primitives() -> None: @@ -110,14 +110,22 @@ def validate(self) -> None: if not self.path: raise ImproperlyConfigured("Keyset path cannot be None or empty.") - if not Path(self.path).is_file(): + try: + path = Path(self.path).expanduser().resolve() + readable_file = path.is_file() + except (OSError, RuntimeError, TypeError, ValueError) as error: + raise ImproperlyConfigured(f"Keyset `{self.path}` is not a readable file.") from error + if not readable_file: raise ImproperlyConfigured(f"Keyset `{self.path}` is not a readable file.") + object.__setattr__(self, "path", path) if not isinstance(self.cleartext, bool): raise ImproperlyConfigured("Keyset option `cleartext` must be a boolean.") if not self.cleartext and self.master_key_aead is None: raise ImproperlyConfigured("Encrypted keysets must specify `master_key_aead`.") + if not self.cleartext and not isinstance(self.master_key_aead, aead.Aead): + raise ImproperlyConfigured("`master_key_aead` must be a Tink Aead primitive.") Primitive = TypeVar("Primitive", aead.Aead, daead.DeterministicAead) @@ -125,7 +133,7 @@ def validate(self) -> None: @dataclass class _KeysetEntry: - handle: Any + handle: KeysetHandle primitives: dict[type[Any], Any] = field(default_factory=dict) @@ -201,7 +209,7 @@ def _get_keyset_entry(self) -> _KeysetEntry: return self._entry keyset_config = self._get_keyset_config() - keyset_path = Path(keyset_config.path).expanduser().resolve() + keyset_path = Path(keyset_config.path) try: stat = keyset_path.stat() except OSError as error: @@ -224,14 +232,14 @@ def _get_keyset_entry(self) -> _KeysetEntry: self._entry = cached_entry else: try: - reader = JsonKeysetReader(keyset_path.read_text(encoding="utf-8")) + serialized_keyset = keyset_path.read_text(encoding="utf-8") if keyset_config.cleartext: - handle = cleartext_keyset_handle.read(reader) + handle = json_proto_keyset_format.parse(serialized_keyset, secret_key_access.TOKEN) else: master_key_aead = keyset_config.master_key_aead assert master_key_aead is not None - handle = read_keyset_handle(reader, master_key_aead) - except (OSError, TinkError) as error: + handle = json_proto_keyset_format.parse_encrypted(serialized_keyset, master_key_aead, b"") + except (OSError, UnicodeError, TinkError) as error: raise ImproperlyConfigured(f"Could not load keyset `{self.keyset_name}`.") from error self._entry = _KeysetEntry(handle) @@ -243,7 +251,7 @@ def _get_keyset_entry(self) -> _KeysetEntry: return self._entry - def _get_tink_keyset_handle(self) -> Any: + def _get_tink_keyset_handle(self) -> KeysetHandle: """Return this manager's configured handle.""" with self._cache_lock: return self._get_keyset_entry().handle @@ -260,7 +268,12 @@ def _get_primitive(self, primitive_class: type[Primitive]) -> Primitive: @property def aead_primitive(self) -> aead.Aead: """Get the AEAD primitive shared by managers using this keyset.""" - return self._get_primitive(aead.Aead) + try: + return self._get_primitive(aead.Aead) + except TinkError as error: + raise ImproperlyConfigured( + "Current keyset does not support AEAD. Please use a keyset that contains AEAD keys." + ) from error @property def daead_primitive(self) -> daead.DeterministicAead: @@ -378,7 +391,7 @@ def _get_aad(self) -> bytes: return aad @property - def _keyset_handle(self) -> Any: + def _keyset_handle(self) -> KeysetHandle: """Get the keyset handle for backward compatibility. Returns: diff --git a/tink_fields/test/test_keyset_loading.py b/tink_fields/test/test_keyset_loading.py new file mode 100644 index 0000000..fc5e89b --- /dev/null +++ b/tink_fields/test/test_keyset_loading.py @@ -0,0 +1,84 @@ +"""Keyset parsing compatibility and actionable configuration errors.""" + +from pathlib import Path +from tempfile import TemporaryDirectory + +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.test import override_settings +from tink import aead, json_proto_keyset_format, new_keyset_handle + +from tink_fields import clear_keyset_cache +from tink_fields.fields import KeysetConfig, KeysetManager + + +@pytest.fixture(autouse=True) +def empty_keyset_cache(): + clear_keyset_cache() + yield + clear_keyset_cache() + + +def test_user_relative_keyset_path_is_expanded_before_validation(): + source = Path(__file__).with_name("test_plaintext_keyset.json").read_text(encoding="utf-8") + with TemporaryDirectory(prefix=".tink-fields-test-", dir=Path.home()) as directory: + path = Path(directory) / "keys.json" + path.write_text(source, encoding="utf-8") + config = {"default": {"path": f"~/{Path(directory).name}/keys.json", "cleartext": True}} + with override_settings(TINK_FIELDS_CONFIG=config): + primitive = KeysetManager("default").aead_primitive + ciphertext = primitive.encrypt(b"secret", b"context") + assert primitive.decrypt(ciphertext, b"context") == b"secret" + + +def test_non_utf8_keyset_has_a_configuration_error(tmp_path): + path = tmp_path / "keys.json" + path.write_bytes(b"\xff\xfeinvalid") + with ( + override_settings(TINK_FIELDS_CONFIG={"default": {"path": path, "cleartext": True}}), + pytest.raises(ImproperlyConfigured, match="Could not load keyset `default`"), + ): + _ = KeysetManager("default").aead_primitive + + +def test_aead_field_with_deterministic_keyset_has_a_configuration_error(): + with pytest.raises(ImproperlyConfigured, match="does not support AEAD"): + _ = KeysetManager("deterministic").aead_primitive + + +def test_invalid_master_key_has_a_configuration_error(): + with pytest.raises(ImproperlyConfigured, match="must be a Tink Aead"): + KeysetConfig(path=Path(__file__).with_name("test_plaintext_keyset.json"), master_key_aead="invalid") + + +@pytest.mark.parametrize("path", [123, "invalid\x00path"]) +def test_invalid_path_has_a_configuration_error(path): + with pytest.raises(ImproperlyConfigured, match="readable file"): + KeysetConfig(path=path, cleartext=True) + + +def test_unhashable_master_key_still_loads_encrypted_keysets(tmp_path): + primitive = KeysetManager("default").aead_primitive + + class UnhashableAead(aead.Aead): + def __eq__(self, other): + return self is other + + def encrypt(self, plaintext, associated_data): + return primitive.encrypt(plaintext, associated_data) + + def decrypt(self, ciphertext, associated_data): + return primitive.decrypt(ciphertext, associated_data) + + master = UnhashableAead() + path = tmp_path / "encrypted.json" + serialized = json_proto_keyset_format.serialize_encrypted( + new_keyset_handle(aead.aead_key_templates.AES128_GCM), master, b"" + ) + path.write_text(serialized, encoding="utf-8") + config = {"encrypted": {"path": path, "master_key_aead": master, "cleartext": False}} + with override_settings(TINK_FIELDS_CONFIG=config): + first = KeysetManager("encrypted").aead_primitive + second = KeysetManager("encrypted").aead_primitive + ciphertext = first.encrypt(b"secret", b"context") + assert second.decrypt(ciphertext, b"context") == b"secret" diff --git a/tox.ini b/tox.ini index 69e410f..33dc3a8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,10 +1,13 @@ [tox] envlist = py310-django52 + py310-django52-tink113 py311-django52 py312-django52 py312-django60 + py313-django52 py313-django60 + py314-django52 py314-django60 [testenv] @@ -13,6 +16,7 @@ extras = test deps = django52: Django>=5.2,<5.3 django60: Django>=6.0,<6.1 + tink113: tink==1.13.0 setenv = PYTHONWARNINGS = default commands = From 713d0a472bdf5f8f997fd36b5a1d45b2f6eebcad Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:31:35 -0400 Subject: [PATCH 6/6] Clarify integration PR landing order after predecessor merges --- docs/code-review-2026-09.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/code-review-2026-09.md b/docs/code-review-2026-09.md index d7dc946..7172637 100644 --- a/docs/code-review-2026-09.md +++ b/docs/code-review-2026-09.md @@ -40,6 +40,12 @@ process must reload or restart after rotation. ## Landing and rollout +**Current landing order:** [PR #13](https://github.com/script3r/django-tink-fields/pull/13) +then [PR #12](https://github.com/script3r/django-tink-fields/pull/12). PR #7 is in +`main`. PRs #8–#11 were merged into their predecessor branches, so PR #13 carries +those four original commits into `main`. After #13 lands, change #12's base to +`main` before merging it (or verify GitHub has retargeted it automatically). + Land the PRs in dependency order. Each PR targets its predecessor so its diff contains only that change. After a parent lands, retarget the next PR to `main` if GitHub has not done so automatically. Merge commits preserve the stack's