Repository navigation
feat(python): open and seal the Go SDK's rows, and wipe plaintext copies - #1513
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
This comment has been minimized.
This comment has been minimized.
9ea5ac8 to
a62f310
Compare
Add a deterministic-kms test feature to the extension. In that build, _engine._use_deterministic_keys(seed) makes the process take its keys from stack-kms's DeterministicSource, as the Go SDK's deterministic guest does. A normal build has neither the feature nor the function. tests_deterministic/ runs against the EQL rows the Go SDK stored under that seed: the frozen stored_v4_rows_v1.json, and the ordered, smallint and ledger fixtures. Python opens Go's rows to the same values, and seals the same values to byte-identical equality, order and match terms. Its query terms equal Go's stored terms. Two ledger rows Python cannot hold are refused with decrypt_not_representable. mise run test:deterministic runs them in their own pytest process, and tests-python.yml runs that task with no credentials. Add a heap-hygiene test to the extension: a spy allocator fails when a block freed during encrypt or query still contains the plaintext. It found that the owned copy of a str, which pyo3 makes under abi3-py39, was freed unwiped. Read now wipes it with zeroize. The four native-plan fixtures and the shared conformance corpus are not part of this commit. The fixtures declare plans the Python SDK cannot express. The corpus needs the stack rebased onto next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refuse an opened timestamp with nanoseconds as decrypt_not_representable. to_python rounded it to microseconds, so a Go-written value opened changed, and resealed to another equality term. The ledger test now checks nanoseconds and years on their own: the old test passed only because those rows also had a year out of range. Wipe decrypted plaintext. A Scalar now wipes its text and bytes when dropped, and to_python borrows it. Decimal digits are wiped on read and on to_python. The hygiene test now watches decrypt too, and a decimal by its digits. The hygiene test now checks that the spy fails on a leak of each needle. Each test takes one lock before it attaches to Python: one test could hold the watch lock and wait for the GIL while another held the GIL and waited for the watch lock. In the test build, client::shared() refuses after the switch to the deterministic client, so an operation that skips the check cannot reach ZeroKMS. use_deterministic takes the build lock. CI's clippy step lints every feature. test_go_rows.py no longer imports from conftest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A Scalar now wipes every variant when dropped, not only text and bytes: integers, floats, dates, timestamps and decimal mantissas sit inline in a row's buffer. decrypt also stops moving each value out of its row. A moved value left its bytes in the old slot, and the buffer was freed with them. to_python now borrows the value, so each one is wiped where it lies. The hygiene test now watches a decimal's mantissa as the engine's i128, and fails without either fix. decrypt_not_representable now names the one cause that applies: a year outside 1 to 9999, or a timestamp with nanoseconds. In the test build, shared() checks for the deterministic client again after it takes the build lock. A thread that waited for the lock during the switch built a live client. A --features flag on maturin's command line replaces the features in pyproject.toml. test:deterministic now names pyo3/extension-module again. Maturin 1.10 and later set PYO3_BUILD_EXTENSION_MODULE, but pyproject.toml allows 1.9, where the test build linked libpython. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1393) encrypt, decrypt and query each wrote their call twice: once for the deterministic test client, once for the live one. client::run! now takes the call once and compiles it for both, so the two cannot drift. The clients have different key source types, so a function that returned the current client would still need two calls. encrypt now borrows each column, as decrypt does. A moved passthrough value left its bytes in the row's buffer when the buffer was freed. A decrypted row is now written into one buffer and moved whole. It was a Vec<Option<Scalar>> collected into a Vec<Scalar>, which left no plaintext only because rustc reused the buffer in place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
decrypt chose the cause of decrypt_not_representable from the value before to_python ran, with its own copy of the nanosecond rule. Any other Python error on a date or timestamp was reported as a year out of range, and the real error was lost. to_python now returns the cause itself, as Unfit::Nanoseconds or Unfit::Year. A year is reported only for datetime's own range errors: ValueError from fromordinal, OverflowError from timedelta or the addition. Every other error is Python's own and is raised as it is. The comment in crates/engine/Cargo.toml now names both places that turn on extension-module: pyproject.toml, and the test:deterministic task. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a62f310 to
22f347a
Compare
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 22f347a · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: code-review (medium) · claude-sonnet-5-5
Skills: code-review, review-cross-check, review-post
Findings (1): 1 minor — 1 inline, 0 below.
Seven candidate findings were checked at head. One was confirmed and is the inline comment. Four were refuted. Two were left undecided: the heap-hygiene tests rebuild the encrypt/decrypt call sequence by hand instead of calling the shipped functions, and decrypt now refuses timestamps with nanoseconds, which is a behaviour change with no changeset. No other review feedback existed on the PR.
Generated by Claude Code
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
One change must happen before merging: add tests for the refusals of the test-build switch use_deterministic in client.rs, and for the seed length check. The other points can wait for a follow-up.
Other findings not posted as comments
- Optional:
unsafe impl GlobalAlloc for Spyinheap_hygiene.rshas no// SAFETY:comment. State that each block comes fromSystemwith the caller'sLayout. - Optional: if the closure given to
watchedpanics,WATCHINGstaystruefor that thread. Reset it with a drop guard. - Optional: the hygiene plan has no passthrough field, so the test does not watch the new borrow of
Column::Passthroughinencrypt.rs. - Optional: no test drives a row with a missing or repeated field through
OpenedFields::endindecrypt.rs. - Optional:
test_go_rows.pyleaves out ledger rows 1 and 2 completely. These rows have decimal values at the edge of the range (28-digit maximum, scale 1.50 against 1.5). Add a model with only the fields Python can hold. - Note:
Unfit::Yearcannot come from aScalar::Timestamp, because ani64of nanoseconds covers only 1677 to 2262. The comment onin_rangesays otherwise.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 2 posted |
| claude | claude-opus-5-5 | rust | 3 found, 2 posted |
| claude | claude-opus-5-5 | python | 3 found, 2 posted |
| claude | claude-opus-5-5 | infracode | 3 found, 2 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. 2 comment(s) had a problem that stopped the reader acting; it rewrote 0. It also rewrote the review body.
Stack: position 12 of 15 (#1405, #1406, #1414, #1415, #1433, #1437, #1453, #1499, #1509, #1510, #1511, *️⃣ #1513, #1516, #1517, #1582). *️⃣ marks this pull request.
Context loaded: the description, 2 linked issue(s) and 5 discussion entries.
Summary
The Python SDK now opens the EQL rows that the Go SDK stored, and seals the same values to the same search terms. EQL (Encrypt Query Language) is CipherStash's set of PostgreSQL types for encrypted values. A new test also checks that the extension wipes its own copies of the plaintext before it frees them.
These tests need no credentials. A test build of the extension takes its keys from a fixed seed, in the same way as the Go SDK's test build.
Changes
deterministic-kmsfeature._engine._use_deterministic_keys(seed)switches the process to stack-kms'sDeterministicSource. The Go SDK's test build uses the same key source.client::run!runs each operation on whichever client is in place.encrypt,decrypt, andqueryeach write their call once.tests_deterministic/test_go_rows.pyreads four Go fixtures inpackages/eql/tests/encryption/fixtures/. For each one, Python does three things:Decimal's scale count.c.stored_v4_rows_v1.json. Nothing writes it again. The Go and Rust tests already check it, and now Python does too.datetimesupports neither.decryptnow refuses both withdecrypt_not_representable. The message names the one cause that applies.decryptandencryptnow borrow each value, and no longer move it out of its row. A moved value left its bytes in the row's buffer, and the buffer was freed with them.Decimal.crates/engine/src/heap_hygiene.rsinstalls a spy allocator.encrypt,query, ordecryptstill contains the plaintext.mise run test:deterministicbuilds the test extension, then runstests_deterministic/in its own pytest process.tests-python.ymlruns it in a new step, "Python against the Go SDK's rows". The workflow also runs when the Go fixtures change.--featuresflag replaces the features inpyproject.toml. So the test task namespyo3/extension-moduleagain. Without it, maturin 1.9 links the test build to libpython.Verification
mise run test:cargo: all core and extension tests pass, with the 4 hygiene tests.mise run test:deterministic: 18 passed.gate-probetests.cargo fmt --check: no findings, with and without all features.pnpm run test:scripts: 84 of 84 files pass.assertlines in Python.Related
Refs #1393. Refs #1341.
This PR does not close #1393. These parts of the issue are not in it:
next.Review notes
client.rsfor the switch, thenheap_hygiene.rs.to_pythonpasses such an error on unchanged.🤖 Generated with Claude Code