From 7975de2ed10cc92021cb5595d16d398924671a31 Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 11:22:51 -0400 Subject: [PATCH 1/2] 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 2/2] 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