-
Notifications
You must be signed in to change notification settings - Fork 1
chore: update cipherstash-client to 0.32.2 #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Send error response to client on bind errors instead of just returning, preventing indefinite client hangs - Map EqlTermVariant to correct QueryOp for encryption: - JsonPath/JsonAccessor use SteVecSelector for SteVec queries - Tokenized uses Default for LIKE/ILIKE match indexes - Add reusable test:integration:setup:tls task for standalone test runs - Update EQL version to 2.2.1 in example config
Add regression tests to verify that data encrypted by previous proxy versions can still be decrypted after upgrades. This is critical for production deployments. The test framework includes: - Fixture generation (run on main branch with CS_GENERATE_EQL_FIXTURES=1) - Regression tests for all data types (text, int2, int4, int8, float8, bool, jsonb) - JSONB field access and array operation tests on legacy data Tests gracefully skip when no fixtures are present, allowing CI to pass while fixtures are generated separately.
The depends directive on test:integration:without_multitenant caused issues when called via `mise --env tls run` because mise doesn't properly pass the environment context to dependency tasks. For standalone use, run test:integration:setup:tls manually first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR upgrades cipherstash-client from 0.31.1 to 0.32.2 and handles the breaking API changes introduced in the new version. The update includes a critical bug fix for client hanging on bind errors, adds comprehensive regression testing for backwards compatibility, and implements the necessary API migrations.
Changes:
- Updated cipherstash-client dependency to 0.32.2 and migrated from deprecated APIs (EqlEncrypted → EqlCiphertext, ReferencedPendingPipeline → PreparedPlaintext)
- Fixed client hanging issue by ensuring error responses are sent before returning from bind error handler
- Added EQL regression test framework with fixture generation for verifying backwards compatibility of encrypted data across proxy versions
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Updated cipherstash-client to 0.32.2 |
| Cargo.lock | Updated dependency versions including cipherstash-config, cts-common, and zerokms-protocol |
| packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs | Migrated to new PreparedPlaintext/EqlOperation API with proper QueryOp mapping for different term variants |
| packages/cipherstash-proxy/src/proxy/mod.rs | Updated type signatures from EqlEncrypted to EqlCiphertext |
| packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs | Added term_filters field to IndexType::SteVec initialization |
| packages/cipherstash-proxy/src/postgresql/messages/data_row.rs | Updated type references from EqlEncrypted to EqlCiphertext |
| packages/cipherstash-proxy/src/postgresql/messages/bind.rs | Removed unused conversion impl, updated type references |
| packages/cipherstash-proxy/src/postgresql/frontend.rs | Fixed bind error handling to send error response before returning; added performance timing logs |
| packages/cipherstash-proxy/src/postgresql/context/mod.rs | Updated type signatures for encrypt/decrypt methods |
| packages/cipherstash-proxy/src/postgresql/backend.rs | Updated type references and added slow database response logging |
| packages/cipherstash-proxy/src/lib.rs | Updated public API exports with new type names |
| packages/cipherstash-proxy/src/error.rs | Migrated From impl from EncryptError to EqlError with new error variants |
| packages/cipherstash-proxy-integration/src/lib.rs | Added eql_regression module |
| packages/cipherstash-proxy-integration/src/eql_regression.rs | New regression test framework for backwards compatibility verification |
| mise.toml | Added test:integration:setup:tls task and connection string echo for debugging |
| mise.local.example.toml | Updated EQL version to 2.2.1 |
| tests/fixtures/eql_regression/.gitkeep | Placeholder for regression test fixtures directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tobyhede
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the API, tbh. Assuming that because tests pass is correct.
|
|
||
| // JsonPath generates a selector term for SteVec queries | ||
| EqlTermVariant::JsonPath => { | ||
| if let Some(index) = col.config.indexes.first() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the configured column has multiple indexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
| Ok(encrypt_eql(cipher, pipeline, plaintexts, &encryption_specs) | ||
| // JsonAccessor generates a selector term for SteVec field access (-> operator) | ||
| EqlTermVariant::JsonAccessor => { | ||
| if let Some(index) = col.config.indexes.first() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: multiple indexes?
|
|
||
| // Tokenized generates match index terms for LIKE/ILIKE | ||
| EqlTermVariant::Tokenized => { | ||
| if let Some(index) = col.config.indexes.first() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: multiple indexes?
…yption - Tokenized now uses EqlOperation::Store to generate full EQL payload - JsonPath finds SteVec index specifically, uses QueryOp::SteVecSelector - JsonAccessor finds SteVec index specifically, uses QueryOp::SteVecTerm - Fixes bug where .first() could return wrong index type when multiple indexes are configured on a column
Both JsonPath and JsonAccessor specify which field to access in the
JSON structure, so both need SteVecSelector to generate the selector
index ("s" field) that eql_v2."->" expects.
Update cipherstash-client to 0.32.2
Summary
This PR upgrades cipherstash-client from 0.32.0 to 0.32.2 and addresses breaking API changes introduced in the new version. It also fixes a client hanging issue discovered during testing
and adds a regression test framework for backwards compatibility verification.
Changes
Dependency Updates
API Migration
Bug Fixes
Testing Infrastructure
Test Plan
Backwards Compatibility
Data encrypted by previous proxy versions will continue to decrypt correctly. The EqlOperation::Store path used for INSERT operations is unchanged, ensuring stored ciphertext format
compatibility.
To verify backwards compatibility with production data:
Acknowledgment
By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.