spp_pii_encryption hardening: deferred design findings from the PR #232 adversarial review

[gonzalesedwin1123]

Tracking issue for the design-level findings from the adversarial security review of #232 (the spp_pii_encryption migration). The must-fix items from that review were applied in #232 itself; everything below was deliberately deferred because the mixin currently has no adopter — no model on 19.0 inherits spp.encrypted.field.mixin.

Gate: items 1–5 must be resolved before the first model adopts the mixin (the PR3 / spp_registry_encryption applier work). They are not cosmetic: each one silently corrupts or leaks data once real records are encrypted.

Blocks first adopter

1. No ciphertext framing — key rotation permanently destroys all encrypted PII, silently. _encrypt_value emits raw b64(nonce||ct||tag) with no version marker, and _get_encryption_key always requests the current key. spp_key_management ships a one-click rotate_key; the moment an operator uses it, every previously encrypted value becomes undecryptable, and per item 4 the failure is silent. The provider retains old key versions, so the fix is framing only (e.g. $spp1$<keyver>$<b64>) plus passing the version to get_key.

2. _apply_encryption_to_vals is not idempotent — copy() double-encrypts. copy_data builds vals from the ORM cache (which holds ciphertext, item 3), and create() re-encrypts it. Duplicating a record produces a double-encrypted value and a blind index computed over ciphertext. Same for any rec.write({"f": rec.f}) normalization pass or import round-trip. Framing (item 1) also gives a cheap "already encrypted" check.

3. Only read() is overridden — every other access path sees ciphertext. Odoo 19's own read() docstring says it is not supposed to be overridden (use _fetch_query/_read_format). Verified consequences: record.field (so all computes, constrains, mail templates, QWeb reports), search_read() (calls _read_format directly — ciphertext over JSON-RPC), export_data (CSV/XLSX exports contain ciphertext, and the audit log's export action never fires), and read_group labels. web_read/web_search_read are covered, which hides the inconsistency until someone exports or hits the API. Correct hook: _read_format, plus a search story (item 5).

4. _decrypt_value returns None and read() silently keeps the ciphertext. The fallback conflates four conditions: legacy plaintext, rotated-away key (item 1), missing key ACL, corrupt/truncated ciphertext. All render as base64 with only a log WARNING, and if the user saves the record, the ciphertext is re-encrypted as if it were plaintext and the original is lost (item 2). Framing makes the cases distinguishable; fail loud on the ones that matter.

5. Plaintext search on an encrypted field silently returns wrong results. search([("national_id", "=", "…")]) hits the ciphertext column and returns zero rows — no error. Concrete failure: a caseworker searches for an existing registrant, finds nothing, creates a duplicate. Either wire the field's search= to the blind index or make a plaintext domain leaf on an encrypted field raise.

Widget honesty

6. The masking widget is cosmetic and inactive on ordinary form fields. It masks only when props.readonly (form views are always in edition, so editable forms show a plain input with full plaintext, no audit), the decrypted value is already in the browser via web_read regardless of mask state, and reveal_group gates a CSS state, not data access. Either add server-side enforcement or rename/document the options as UI de-emphasis, not an access control.

7. Default mask reveals the last 4 characters ("****-****-####") to any user who can read the record — last-4 is a common identity-verification token. Default should be full masking; the short-value guard added in feat(spp_pii_encryption): port PII encryption core from openspp-modules #232 still shows 4 of 5 characters for a 5-char value.

Crypto scope

8. AAD omits the record id — ciphertext transplant defeats record rules. AAD is model.field, identical for every row, so ciphertext copied from an inaccessible record (backup, export, index column) into a writable one decrypts through the normal path. Binding the record id into the AAD has a create-time chicken-and-egg to design around.

9. Blind-index salt is not model-scoped. get_salt("pii", field_name) means res.partner.phone and spp.registry.id.phone produce identical index values for the same plaintext — cross-table linkage for anyone who can read the index columns. Use f"{model}.{field}" like the AAD. (Salt-scope change invalidates existing indexes — do it before first adoption.)

10. Blind-index columns are ordinary readable/groupable/exportable fields. Deterministic HMAC means read_group on national_id_index partitions the registry into equivalence classes ("these 40 share one national ID"), and the column can be exported and correlated offline. Document this property and put a restrictive groups= on index fields in the recommended usage pattern.

Config lifecycle and audit operations

11. Toggling a config off (or archiving it) orphans the data with no warning. One click on the boolean_toggle and read() stops decrypting; the next save writes base64 back as "plaintext". Needs a guard refusing to disable a used config plus a decrypt-migration path.

12. Audit log rate limiting and retention. feat(spp_pii_encryption): port PII encryption core from openspp-modules #232 added target validation to log_field_access, but a user can still create unbounded (valid-looking) rows; there is no ir.cron sweep or retention policy. Also worth noting in docs: ip_address records the proxy IP unless proxy_mode is enabled.

13. UNIQUE(model_id, field_id) ignores active — archive a config and a replacement for the same field is blocked by an invisible archived row. Partial unique index on active.

Performance

14. Per-read/per-record overhead and log amplification. _get_encrypted_fields() runs an uncached search() per read()/write(); _decrypt_value calls get_key per record per field, and each get_key emits a KEY_ACCESS: INFO line and a has_group check — a list view of 80 records logs 80 lines per page load. @ormcache the config lookup (invalidate on config write) and hoist AESGCM(key) out of per-value loops.

Smaller correctness items

15. partial normalization does not .upper() while exact does — _search_by_partial("ab12") misses records stored as AB12.
16. _soundex deviates from standard Soundex: H/W reset the run (they should not), so e.g. "Ashcraft" hashes unexpectedly — false negatives in phonetic matching.
17. except Exception: pass in log_field_access request-metadata capture — at minimum _logger.debug. Also move import re (encrypted_field_mixin.py) and from datetime import timedelta (audit_log.py) to module level.
18. Untranslated strings in the OWL template (t-att-title ternary literals) and dead getMaskPattern branch reading a nonexistent field.mask_pattern key.

Test debt (lands naturally with the first adopter)

19. True end-to-end tests on stored records: encrypt-on-create/decrypt-on-read/blind-index search against a concrete inheriting model, plus copy(), export, search_read, and key-rotation scenarios; and a hoot/tour test that actually mounts masked_char and exercises the reveal→audit flow (the widget is currently wired into no view).

Full review text is preserved in the workspace review dossier; findings were verified against the Odoo 19 source (odoo/orm/models.py, fields_textual.py, addons/web) at review time.