fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2#382
Open
fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2#382
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces config-driven ZeroKMS initialization with an AutoStrategy/ZeroKMSBuilder flow, updates workspace and proxy dependencies, adjusts error variants and config typing (client_id → Uuid, client key parsing), removes a large integration test, updates tests, and adds CS_CTS_HOST / CS_ZEROKMS_HOST env passthrough. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment (CS_CLIENT_ID / CS_CLIENT_KEY)
participant Proxy as Proxy init
participant Strategy as AutoStrategy
participant Builder as ZeroKMSBuilder
participant ZeroKMS as ZeroKMS Client
Env->>Proxy: provide CS_CLIENT_ID & CS_CLIENT_KEY
Proxy->>Strategy: detect/select AutoStrategy
Strategy-->>Proxy: AutoStrategy (selected)
Proxy->>Builder: ZeroKMSBuilder::auto().with_client_key(client_key)
Builder->>ZeroKMS: build()
ZeroKMS-->>Proxy: ZerokmsClient (ready)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tobyhede
force-pushed
the
proxy-2.1.24-alpha.1
branch
from
8cb9a16 to
c524252
Compare
tobyhede
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cipherstash-proxy-integration/src/generate.rs`:
- Around line 130-142: The test currently reads local credentials via std::env
(CS_CLIENT_ID/CS_CLIENT_KEY) and calls ZeroKMSBuilder::auto(), making it
host-dependent; move credential acquisition and ZeroKMS instantiation into the
Docker-backed integration test harness and consume them from the harness instead
of environment and auto discovery: replace direct uses of std::env::var and
ClientKey::from_hex_v1(client_id, &client_key_hex) and the
ZeroKMSBuilder::auto().unwrap() chain with injected fixtures or helper functions
provided by the Docker harness (e.g., have the harness supply a ClientKey
instance or a ZeroKMS client via a helper like harness.zero_kms_client() or
ZeroKMSBuilder::from_client(key)), and update the test to use these
harness-provided values so no local env reads or auto discovery occur in
generate.rs.
In `@packages/cipherstash-proxy/src/error.rs`:
- Around line 57-58: Remove the top-level Error::ZeroKMSBuilder variant and
instead surface builder failures under the ZeroKMSError domain: add a Builder
variant (or reuse System) to the ZeroKMSError enum, move the #[from] conversion
for cipherstash_client::zerokms::ZeroKMSBuilderError into that ZeroKMSError
variant, and update Error to hold ZeroKMSError (e.g.,
Error::ZeroKMSError(#[from] ZeroKMSError)); this means deleting the
ZeroKMSBuilder entry from the Error enum and wiring the
From<cipherstash_client::zerokms::ZeroKMSBuilderError> conversion to produce
ZeroKMSError::Builder (or map to ZeroKMSError::System) so all ZeroKMS-related
errors are grouped under ZeroKMSError.
In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs`:
- Around line 26-32: The code currently panics on malformed operator config by
calling expect() when parsing client_id and client_key and when handling
zerokms_host; replace these panics with proper error propagation: validate and
parse the UUID (client_id) and the hex client key (ClientKey::from_hex_v1) and
return a typed error (or map the parsing errors into your function's error type)
instead of calling expect(), and likewise validate/parse zerokms_host and return
an error on failure; alternatively move these validations into the config
loading path so the values are guaranteed valid before reaching the code paths
that use client_id, ClientKey::from_hex_v1, and zerokms_host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 288b8a24-26ba-43f8-bf17-1c654823250b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlpackages/cipherstash-proxy-integration/src/generate.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
💤 Files with no reviewable changes (1)
- packages/cipherstash-proxy/src/proxy/mod.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/cipherstash-proxy/src/error.rs (1)
55-58:⚠️ Potential issue | 🟠 MajorKeep builder failures inside the ZeroKMS domain.
Error::ZeroKMSBuildersplits one problem domain across two top-level variants and leaks the construction mechanism into the public error taxonomy. Foldcipherstash_client::zerokms::ZeroKMSBuilderErrorintoZeroKMSErrorinstead, then keepErrorexposing onlyError::ZeroKMS.Suggested shape
pub enum Error { #[error(transparent)] ZeroKMS(#[from] ZeroKMSError), - - #[error(transparent)] - ZeroKMSBuilder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError), } pub enum ZeroKMSError { #[error("ZeroKMS authentication failed. Check the configured credentials. For help visit {}#zerokms-authentication-failed", ERROR_DOC_BASE_URL)] AuthenticationFailed, + + #[error(transparent)] + Builder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError), #[error(transparent)] System(#[from] cipherstash_client::zerokms::Error), }As per coding guidelines: "All errors must be defined in
packages/cipherstash-proxy/src/error.rsand grouped by problem domain (not module structure). Use descriptive variant names without 'Error' suffix."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/error.rs` around lines 55 - 58, Replace the top-level Error::ZeroKMSBuilder variant by folding cipherstash_client::zerokms::ZeroKMSBuilderError into the ZeroKMSError domain: add a From<ZeroKMSBuilderError> (or #[from]) conversion inside the ZeroKMSError definition and remove the Error::ZeroKMSBuilder variant so the public Error enum only exposes Error::ZeroKMS (which remains #[from] ZeroKMSError); update any code that constructed or matched Error::ZeroKMSBuilder to build or match ZeroKMSError instead so builder failures are contained within the ZeroKMSError type.packages/cipherstash-proxy/src/proxy/zerokms/mod.rs (1)
18-41:⚠️ Potential issue | 🟠 MajorDon't panic on malformed ZeroKMS config.
Line 18 narrows this path to
ZeroKMSBuilderError, and Line 30, Line 32, and Line 41 compensate by callingexpect().packages/cipherstash-proxy/src/config/tandem.rsstill storesclient_idandclient_keyas rawStrings, so bad operator config will abort startup instead of returning a typed config/ZeroKMS error. Please validate these fields during config loading or broaden this function's error type and replace theexpect()calls withmap_err(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs` around lines 18 - 41, The startup currently panics on malformed ZeroKMS config because client_id parse and ClientKey::from_hex_v1 and URL parse use expect(); change these to return proper errors instead: either validate and convert config.encrypt.client_id and config.encrypt.client_key when loading config (so they are typed UUID/ClientKey ahead of time) or broaden the error type of the builder function and replace the expect() calls on config.encrypt.client_id.parse(), ClientKey::from_hex_v1(...), and zerokms_host.parse() with map_err(...) that converts parsing/hex/URL errors into the function's Result (e.g. ZeroKMSBuilderError or a new variant), keeping ZeroKMSBuilder::new and AutoStrategy::builder usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cipherstash-proxy/src/error.rs`:
- Around line 55-58: Replace the top-level Error::ZeroKMSBuilder variant by
folding cipherstash_client::zerokms::ZeroKMSBuilderError into the ZeroKMSError
domain: add a From<ZeroKMSBuilderError> (or #[from]) conversion inside the
ZeroKMSError definition and remove the Error::ZeroKMSBuilder variant so the
public Error enum only exposes Error::ZeroKMS (which remains #[from]
ZeroKMSError); update any code that constructed or matched Error::ZeroKMSBuilder
to build or match ZeroKMSError instead so builder failures are contained within
the ZeroKMSError type.
In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs`:
- Around line 18-41: The startup currently panics on malformed ZeroKMS config
because client_id parse and ClientKey::from_hex_v1 and URL parse use expect();
change these to return proper errors instead: either validate and convert
config.encrypt.client_id and config.encrypt.client_key when loading config (so
they are typed UUID/ClientKey ahead of time) or broaden the error type of the
builder function and replace the expect() calls on
config.encrypt.client_id.parse(), ClientKey::from_hex_v1(...), and
zerokms_host.parse() with map_err(...) that converts parsing/hex/URL errors into
the function's Result (e.g. ZeroKMSBuilderError or a new variant), keeping
ZeroKMSBuilder::new and AutoStrategy::builder usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a62f3288-e41d-4ebe-86c8-c64f3f74b724
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlpackages/cipherstash-proxy-integration/src/generate.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
💤 Files with no reviewable changes (1)
- packages/cipherstash-proxy/src/proxy/mod.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
- packages/cipherstash-proxy/Cargo.toml
- packages/cipherstash-proxy-integration/src/generate.rs
tobyhede
force-pushed
the
proxy-2.1.24-alpha.1
branch
4 times, most recently
from
c25d492 to
ef63279
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Around line 439-440: The tuple array passed to temp_env::with_vars (the
entries for "CS_ENCRYPT__CLIENT_ID" and CS_CLIENT_ID) is misformatted; run cargo
fmt and reformat that hunk so the tuples follow Rust formatting conventions
(align/line breaks) inside the temp_env::with_vars call, then commit the
formatted change.
- Around line 116-124: The client_key hex validation (currently done via
cipherstash_client::zerokms::ClientKey::from_hex_v1 and mapping to
ConfigError::InvalidParameter) must be centralized into a private validation
helper on the config type and invoked from every constructor (load(), build(),
build_path(), for_testing()) before returning the config so invalid keys never
reach init_zerokms_client(); implement a method like validate_client_key(&self)
that calls ClientKey::from_hex_v1(config.encrypt.client_id,
&config.encrypt.client_key) and returns the mapped ConfigError on failure, then
call that helper at the end of each constructor and remove any unchecked expect
paths relying on a later validation.
- Line 45: The UUID parsing error handling currently maps any "UUID parsing
failed" message to ConfigError::InvalidDatasetId; update the match arm that
checks s.contains("UUID parsing failed") to extract the field/key name from the
error string (use the same approach as extract_missing_field_and_key) and branch
on that field: if the failed field is "client_id" (or CS_CLIENT_ID /
encrypt.client_id) return ConfigError::InvalidParameter, otherwise if it's
"default_keyset_id" (or encrypt.default_keyset_id) return
ConfigError::InvalidDatasetId; ensure you reference the EncryptConfig fields
client_id and default_keyset_id and the error-mapping code that currently
produces InvalidDatasetId so the fix targets that match arm.
In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs`:
- Around line 21-24: The current check uses
std::env::var("CS_CTS_HOST").is_err(), which treats an empty CS_CTS_HOST as set
and prevents using config.cts_host(); change the logic to treat empty as unset
by checking the env var's value (via std::env::var or var_os) and only skip
setting CS_CTS_HOST when the existing value is non-empty; update the block that
references config.cts_host(), cts_host, and std::env::var("CS_CTS_HOST") so you
call std::env::set_var("CS_CTS_HOST", cts_host) when no value exists or the
value is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5dad95c-5d11-4084-a9d1-024e1f76dbc9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomldocker-compose.ymlpackages/cipherstash-proxy-integration/src/generate.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/config/tandem.rspackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rstests/docker-compose.yml
💤 Files with no reviewable changes (1)
- packages/cipherstash-proxy-integration/src/generate.rs
✅ Files skipped from review due to trivial changes (3)
- docker-compose.yml
- Cargo.toml
- packages/cipherstash-proxy/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
- packages/cipherstash-proxy/src/error.rs
- packages/cipherstash-proxy/src/proxy/mod.rs
tobyhede
force-pushed
the
proxy-2.1.24-alpha.1
branch
from
ef63279 to
a02bb7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/cipherstash-proxy/src/config/tandem.rs (1)
200-201:⚠️ Potential issue | 🟡 MinorUUID parsing error mapping doesn't distinguish between
client_idanddefault_keyset_id.The blanket mapping
s.contains("UUID parsing failed") => ConfigError::InvalidDatasetIdwill incorrectly label aclient_idparse failure as a dataset error. Consider extracting the field name from the error message (similar toextract_missing_field_and_key) to returnConfigError::InvalidParameterforclient_idfailures.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/config/tandem.rs` around lines 200 - 201, The UUID parsing error branch currently maps any error containing "UUID parsing failed" to ConfigError::InvalidDatasetId; change the logic in the match arm that inspects err.to_string() (the code around match err.to_string() in tandem.rs) to extract the field name from the error text (reuse the existing helper extract_missing_field_and_key or implement the same extraction) and then map the field: if the field is "client_id" return ConfigError::InvalidParameter, if it refers to the dataset id return ConfigError::InvalidDatasetId, otherwise fall back to the previous/default ConfigError; ensure you only change the mapping logic and keep other error handling intact.
🧹 Nitpick comments (1)
packages/cipherstash-proxy/src/config/tandem.rs (1)
305-326:for_testing()uses an invalidclient_keythat will failclient_key()calls.The test fixture at line 316 uses
client_key: "test".to_string(), which is not valid hex and will fail if any test callsconfig.encrypt.client_key(). While this is test-only code (#[cfg(test)]), it could cause confusing failures for future tests that need a workingClientKey.Consider using a valid test key (like the one used in the test env vars at line 586) to make the fixture usable across all test scenarios:
♻️ Suggested fix
encrypt: EncryptConfig { client_id: Uuid::parse_str("00000000-0000-0000-0000-000000000001").unwrap(), - client_key: "test".to_string(), + client_key: "a4627031a16b7065726d75746174696f6e900e05030d0608090007020c04010b0a0f6770325f66726f6da16b7065726d75746174696f6e900608000a0204030f01070d090e0b0c056570325f746fa16b7065726d75746174696f6e90000908060701030a05040e020d0b0c0f627033a16b7065726d75746174696f6e982107181d130d05181f08040a181c1002181e010311181818200b0f0e0915181b0c16171819060012181a14".to_string(), default_keyset_id: Some(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/config/tandem.rs` around lines 305 - 326, The test-only for_testing() fixture sets EncryptConfig.client_key to the non-hex string "test", which will break calls to config.encrypt.client_key(); update for_testing() (the EncryptConfig in the for_testing() constructor) to use a valid hex-encoded test client key (the same valid test key used in the test environment variables elsewhere) or any valid hex string of the expected length so ClientKey parsing succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cipherstash-proxy/tests/config/cipherstash-proxy-with-crn.toml`:
- Around line 10-12: The example config file currently uses string placeholders
that will fail deserialization because EncryptConfig.client_id is typed as Uuid
and ClientKey::from_hex_v1() expects a valid hex key; update
cipherstash-proxy-example.toml to use valid-looking placeholder values (e.g., a
UUID for client_id and a synthetic hex-encoded client_key that matches the
expected format) or add a clear comment above the placeholders stating they are
illustrative only and must be replaced with a real UUID and hex key before use
so deserialization/validation no longer fails.
---
Duplicate comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Around line 200-201: The UUID parsing error branch currently maps any error
containing "UUID parsing failed" to ConfigError::InvalidDatasetId; change the
logic in the match arm that inspects err.to_string() (the code around match
err.to_string() in tandem.rs) to extract the field name from the error text
(reuse the existing helper extract_missing_field_and_key or implement the same
extraction) and then map the field: if the field is "client_id" return
ConfigError::InvalidParameter, if it refers to the dataset id return
ConfigError::InvalidDatasetId, otherwise fall back to the previous/default
ConfigError; ensure you only change the mapping logic and keep other error
handling intact.
---
Nitpick comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Around line 305-326: The test-only for_testing() fixture sets
EncryptConfig.client_key to the non-hex string "test", which will break calls to
config.encrypt.client_key(); update for_testing() (the EncryptConfig in the
for_testing() constructor) to use a valid hex-encoded test client key (the same
valid test key used in the test environment variables elsewhere) or any valid
hex string of the expected length so ClientKey parsing succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 476bf767-0205-4e4a-a81f-7b3a151297f7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
DEVELOPMENT.mdpackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/config/tandem.rspackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/tests/config/cipherstash-proxy-test.tomlpackages/cipherstash-proxy/tests/config/cipherstash-proxy-with-crn.toml
✅ Files skipped from review due to trivial changes (2)
- packages/cipherstash-proxy/tests/config/cipherstash-proxy-test.toml
- DEVELOPMENT.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cipherstash-proxy/Cargo.toml
- packages/cipherstash-proxy/src/proxy/zerokms/mod.rs
tobyhede
changed the title
File was never declared as a module in lib.rs and contained only a scratch-pad test for inspecting STE-Vec encrypted output.
tobyhede
force-pushed
the
proxy-2.1.24-alpha.1
branch
from
0fd8e0e to
0843b5b
Compare
Migrate ZeroKMS initialization from config-builder chain (ConsoleConfig/CtsConfig/ZeroKMSConfig/EnvSource) to the new ZeroKMSBuilder + AutoStrategy API introduced in cipherstash-client 0.34. Key changes: - Replace AutoRefresh<ServiceCredentials> with AutoStrategy - Use ZeroKMSBuilder for client construction - Parse client_id as Uuid and validate client_key at config load - Remove dev endpoint config (now handled by CS_ZEROKMS_HOST env var) - Remove dead error variants from old config API - Add client_id UUID error disambiguation from dataset_id errors - Remove vitaminc pins (fixed in alpha.4) - Update cts-common to 0.34.0-alpha.4
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cipherstash-proxy/src/config/tandem.rs (1)
229-229: Please add coverage for the newInvalidClientKeybranch.
invalid_client_id_uuid()locks in the UUID parsing path, but nothing in the added tests asserts the eagerclient_keyvalidation introduced here. One small env-override test would keepConfigError::InvalidClientKeyfrom regressing.🧪 Example test
#[test] fn invalid_client_key() { with_no_cs_vars(|| { temp_env::with_vars([("CS_CLIENT_KEY", Some("not-hex"))], || { let err = TandemConfig::build_path("tests/config/cipherstash-proxy-test.toml").unwrap_err(); assert!(matches!( err, Error::Config(crate::error::ConfigError::InvalidClientKey(_)) )); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/config/tandem.rs` at line 229, Add a unit test that exercises the new InvalidClientKey branch by forcing client key validation to fail: create a test (e.g., invalid_client_key) that sets the CS_CLIENT_KEY env var to an invalid value (like "not-hex") via temp_env::with_vars while loading the same TandemConfig path used by other tests, call TandemConfig::build_path and assert the returned error matches Error::Config(crate::error::ConfigError::InvalidClientKey(_)); this will exercise the eager validation triggered by config.encrypt.build_client_key() and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Around line 201-209: The fallback branch that currently returns
ConfigError::InvalidDatasetId for UUID parse failures should instead return a
field-accurate error for default_keyset_id; add a new variant
InvalidDefaultKeysetId to the ConfigError enum in
packages/cipherstash-proxy/src/error.rs with the message "default_keyset_id is
not a valid UUID." and update the match arm in tandem.rs (the branch handling
"UUID parsing failed") to return ConfigError::InvalidDefaultKeysetId when the
failing key is not client_id.
---
Nitpick comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Line 229: Add a unit test that exercises the new InvalidClientKey branch by
forcing client key validation to fail: create a test (e.g., invalid_client_key)
that sets the CS_CLIENT_KEY env var to an invalid value (like "not-hex") via
temp_env::with_vars while loading the same TandemConfig path used by other
tests, call TandemConfig::build_path and assert the returned error matches
Error::Config(crate::error::ConfigError::InvalidClientKey(_)); this will
exercise the eager validation triggered by config.encrypt.build_client_key() and
prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9e5ef34-e498-4968-a6b6-56628ad1dec3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlDEVELOPMENT.mddocker-compose.ymlpackages/cipherstash-proxy-integration/src/generate.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/config/tandem.rspackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rspackages/cipherstash-proxy/tests/config/cipherstash-proxy-bad-client-id.tomlpackages/cipherstash-proxy/tests/config/cipherstash-proxy-test.tomlpackages/cipherstash-proxy/tests/config/cipherstash-proxy-with-crn.tomltests/docker-compose.yml
💤 Files with no reviewable changes (1)
- packages/cipherstash-proxy-integration/src/generate.rs
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- tests/docker-compose.yml
- packages/cipherstash-proxy/Cargo.toml
- packages/cipherstash-proxy/src/proxy/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docker-compose.yml
- DEVELOPMENT.md
- packages/cipherstash-proxy/src/proxy/zerokms/mod.rs
tobyhede
force-pushed
the
proxy-2.1.24-alpha.1
branch
from
0843b5b to
0ea1b1f
Compare
…yset error variant Update cipherstash-proxy-example.toml to use valid placeholder UUIDs for client_id and default_keyset_id now that EncryptConfig deserializes these as Uuid types. Rename stale InvalidDatasetId error variant to InvalidDefaultKeysetId with an accurate error message.
The cipherstash-client 0.34 migration removed development.zerokms_host and development.cts_host from DevelopmentConfig. Existing configs with these keys were silently ignored, regressing local and non-prod setups. - Restore zerokms_host and wire through to ZeroKMSBuilder::with_base_url() - Restore cts_host field but warn at startup since the new client library resolves CTS endpoint from credentials automatically - Add url crate dependency for Url::parse
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate ZeroKMS initialization from config-builder chain (ConsoleConfig/CtsConfig/ZeroKMSConfig/EnvSource) to the new ZeroKMSBuilder + AutoStrategy API.
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.
Summary by CodeRabbit
Chores
Bug Fixes
Tests
Documentation