Skip to content

WIP fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2#382

Open
tobyhede wants to merge 1 commit intomainfrom
proxy-2.1.24-alpha.1
Open

WIP fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2#382
tobyhede wants to merge 1 commit intomainfrom
proxy-2.1.24-alpha.1

Conversation

@tobyhede
Copy link
Contributor

@tobyhede tobyhede commented Mar 23, 2026

Migrate ZeroKMS initialization from config-builder chain (ConsoleConfig/CtsConfig/ZeroKMSConfig/EnvSource) to the new ZeroKMSBuilder + AutoStrategy API.

  • Replace AutoRefresh with AutoStrategy
  • Use ClientKey::from_hex_v1 for client key construction
  • Update error variant: zerokms::Error::Credentials -> Error::Auth
  • Add ZeroKMSBuilderError variant to Error enum
  • Remove build_zerokms_config (replaced by init_zerokms_client)
  • Update cts-common to 0.34.0-alpha.3
  • Pin vitaminc to 0.1.0-pre4.1 (pre4.2 broke backward compat)

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

    • Updated workspace and proxy dependencies and aligned encryption-related crates.
    • Added container env passthrough for CTS and ZeroKMS hosts.
    • Streamlined client construction and removed obsolete test helpers.
  • Bug Fixes

    • Improved ZeroKMS initialization and authentication error handling for more reliable startup and clearer failures.
  • Tests

    • Simplified and relaxed tests around ZeroKMS client initialization.

@coderabbitai
Copy link

coderabbitai bot commented Mar 23, 2026

Warning

Rate limit exceeded

@tobyhede has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 25 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eecd5d3b-55d1-45f6-bfa8-900d2518db4c

📥 Commits

Reviewing files that changed from the base of the PR and between f9db5f1 and a20d56a.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • Cargo.toml
  • docker-compose.yml
  • packages/cipherstash-proxy-integration/src/generate.rs
  • packages/cipherstash-proxy/Cargo.toml
  • packages/cipherstash-proxy/src/error.rs
  • packages/cipherstash-proxy/src/proxy/mod.rs
  • packages/cipherstash-proxy/src/proxy/zerokms/mod.rs
  • packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
  • tests/docker-compose.yml
📝 Walkthrough

Walkthrough

Replaces config-driven ZeroKMS initialization with a builder/AutoStrategy-based flow using client-id/key from env; updates workspace and proxy package dependencies; adds a ZeroKMSBuilder error variant; adjusts error matching and updates related tests and docker env passthrough.

Changes

Cohort / File(s) Summary
Workspace updates
Cargo.toml
Bumped workspace deps: cipherstash-client0.34.0-alpha.2, cts-common0.34.0-alpha.3.
Proxy Cargo changes
packages/cipherstash-proxy/Cargo.toml
Replaced old vitaminc-protected entry with vitaminc, vitaminc-random, vitaminc-protected, vitaminc-encrypt, vitaminc-traits = 0.1.0-pre4.1 and added url = "2".
ZeroKMS init & types
packages/cipherstash-proxy/src/proxy/zerokms/mod.rs, packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
Removed build_zerokms_config; init_zerokms_client now builds a ZerokmsClient via ZeroKMSBuilder/AutoStrategy, parses client_id/client_key from config/env, optionally applies zerokms_host; type aliases updated to AutoStrategy; error mapping changed to return ZeroKMSBuilderError; match arm CredentialsAuth in init_cipher.
Top-level error enum
packages/cipherstash-proxy/src/error.rs
Added Error::ZeroKMSBuilder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError) variant.
Tests & integration
packages/cipherstash-proxy-integration/src/generate.rs, packages/cipherstash-proxy/src/proxy/mod.rs
Tests updated to construct ZeroKMS client from CS_CLIENT_ID/CS_CLIENT_KEY env vars via new builder API; test assertions simplified to check init_zerokms_client returns Ok.
Docker compose
docker-compose.yml
Proxy service now passes CS_CTS_HOST and CS_ZEROKMS_HOST into the container via env var passthrough.

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: AutoStrategy::detect() / select strategy
    Strategy-->>Proxy: AutoStrategy
    Proxy->>Builder: ZeroKMSBuilder::auto().with_client_key(client_key)
    Builder->>ZeroKMS: build()
    ZeroKMS-->>Proxy: ZerokmsClient (ready)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • coderdan
  • auxesis
  • freshtonic

"I hopped through code with keys in paw,
Builders hum where configs once saw,
Env vars whisper id and key,
ZeroKMS wakes — fast and free,
A rabbit's cheer for tidy law 🐇"

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title is overly broad and misleading: it emphasizes the dependency bump as the main change, but the PR's core objective is refactoring ZeroKMS initialization from config-builder to ZeroKMSBuilder + AutoStrategy. The WIP prefix also indicates incomplete work. Remove the WIP prefix and reframe the title to highlight the primary refactoring: e.g., 'refactor: migrate ZeroKMS initialization to ZeroKMSBuilder + AutoStrategy' or similar.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch proxy-2.1.24-alpha.1

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tobyhede tobyhede force-pushed the proxy-2.1.24-alpha.1 branch from 8cb9a16 to c524252 Compare March 23, 2026 04:06
@tobyhede tobyhede changed the title fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2 WIP fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2 Mar 23, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7cab49e and 8cb9a16.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • Cargo.toml
  • packages/cipherstash-proxy-integration/src/generate.rs
  • packages/cipherstash-proxy/Cargo.toml
  • packages/cipherstash-proxy/src/error.rs
  • packages/cipherstash-proxy/src/proxy/mod.rs
  • packages/cipherstash-proxy/src/proxy/zerokms/mod.rs
  • packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
💤 Files with no reviewable changes (1)
  • packages/cipherstash-proxy/src/proxy/mod.rs

Comment on lines +130 to 142
let client_id: uuid::Uuid = std::env::var("CS_CLIENT_ID")
.expect("CS_CLIENT_ID must be set")
.parse()
.expect("CS_CLIENT_ID must be a valid UUID");
let client_key_hex =
std::env::var("CS_CLIENT_KEY").expect("CS_CLIENT_KEY must be set");
let client_key = ClientKey::from_hex_v1(client_id, &client_key_hex)
.expect("CS_CLIENT_KEY must be valid hex");
let zerokms_client = ZeroKMSBuilder::auto()
.unwrap()
.with_client_key(client_key)
.build()
.unwrap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

This integration test is still host-dependent.

The new setup still reads local credentials and performs a live ZeroKMS auth flow, so the test only runs on a preconfigured machine. Please move this behind the Docker-backed integration harness instead of wiring std::env + ZeroKMSBuilder::auto() directly in the test body.

As per coding guidelines, "Integration tests should use Docker containers for reproducible environments."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cipherstash-proxy-integration/src/generate.rs` around lines 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.

Comment on lines +57 to +58
#[error(transparent)]
ZeroKMSBuilder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n packages/cipherstash-proxy/src/error.rs

Repository: cipherstash/proxy

Length of output: 22399


🏁 Script executed:

rg -i "zerokmsbuilder" --type rust

Repository: cipherstash/proxy

Length of output: 728


🏁 Script executed:

cat -n packages/cipherstash-proxy/src/proxy/zerokms/mod.rs

Repository: cipherstash/proxy

Length of output: 1879


🏁 Script executed:

rg "init_zerokms_client" --type rust -A 2 -B 2

Repository: cipherstash/proxy

Length of output: 1391


🏁 Script executed:

rg "ZeroKMSBuilderError" --type rust -B 5 -A 5 | head -50

Repository: cipherstash/proxy

Length of output: 2502


Move ZeroKMSBuilder errors into the ZeroKMSError domain enum.

ZeroKMSBuilder exposes an implementation detail (the builder pattern) in the public error taxonomy instead of grouping by problem domain. Builder failures are initialization-phase ZeroKMS errors—the same domain as runtime ZeroKMS failures. Consolidate this into ZeroKMSError (e.g., add a Builder variant or map to the existing System variant), then remove the top-level ZeroKMSBuilder variant from the Error enum. This aligns with the guideline requiring errors to be "grouped by problem domain (not module structure)."

🤖 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 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.

Comment on lines +26 to +32
let client_id: uuid::Uuid = config
.encrypt
.client_id
.parse()
.expect("client_id must be a valid UUID");
let client_key = ClientKey::from_hex_v1(client_id, &config.encrypt.client_key)
.expect("client_key must be valid hex");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the actual code in zerokms/mod.rs
cat -n packages/cipherstash-proxy/src/proxy/zerokms/mod.rs | head -50

Repository: cipherstash/proxy

Length of output: 1879


🏁 Script executed:

# Check if error.rs exists and what error types are defined
cat -n packages/cipherstash-proxy/src/error.rs | head -100

Repository: cipherstash/proxy

Length of output: 3676


🏁 Script executed:

# Search for other parsing operations in the codebase to see how they handle errors
rg -A 3 "\.parse\(\)" packages/cipherstash-proxy/src/ --max-count 10

Repository: cipherstash/proxy

Length of output: 2558


🏁 Script executed:

# Look for config validation or EncryptConfig usage
rg -B 5 -A 5 "EncryptConfig\|client_id\|client_key" packages/cipherstash-proxy/src/ -t rs | head -60

Repository: cipherstash/proxy

Length of output: 87


🏁 Script executed:

# Search for EncryptConfig definition
rg "struct EncryptConfig\|pub struct EncryptConfig" packages/cipherstash-proxy/src/ -A 5

Repository: cipherstash/proxy

Length of output: 43


🏁 Script executed:

# Look for where init_zerokms_client is called and how config is loaded
rg "init_zerokms_client" packages/cipherstash-proxy/src/ -B 3 -A 3

Repository: cipherstash/proxy

Length of output: 1960


🏁 Script executed:

# Look for config loading and validation logic
find packages/cipherstash-proxy/src/config -name "*.rs" -type f

Repository: cipherstash/proxy

Length of output: 392


🏁 Script executed:

# Check the main config module to see how EncryptConfig is defined
grep -n "EncryptConfig" packages/cipherstash-proxy/src/config/mod.rs -A 10 -B 2

Repository: cipherstash/proxy

Length of output: 43


🏁 Script executed:

# Look at tandem.rs for EncryptConfig definition
grep -n "EncryptConfig\|struct.*Encrypt\|pub.*encrypt" packages/cipherstash-proxy/src/config/tandem.rs -A 5 -B 2

Repository: cipherstash/proxy

Length of output: 899


🏁 Script executed:

# Check if there's validation in the config loading
cat -n packages/cipherstash-proxy/src/config/tandem.rs | head -100

Repository: cipherstash/proxy

Length of output: 3499


🏁 Script executed:

# Continue reading tandem.rs to see config loading and validation logic
cat -n packages/cipherstash-proxy/src/config/tandem.rs | sed -n '95,200p'

Repository: cipherstash/proxy

Length of output: 4813


🏁 Script executed:

# Check the ZeroKMS.init function to understand the error flow
cat -n packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs | head -80

Repository: cipherstash/proxy

Length of output: 3350


🏁 Script executed:

# Search for ConfigError definition to understand how config parsing errors are handled
rg "enum ConfigError\|struct ConfigError" packages/cipherstash-proxy/src/ -A 10

Repository: cipherstash/proxy

Length of output: 43


🏁 Script executed:

# Search for ConfigError in error.rs
cat -n packages/cipherstash-proxy/src/error.rs | grep -A 20 "enum ConfigError\|struct ConfigError"

Repository: cipherstash/proxy

Length of output: 834


🏁 Script executed:

# Let's see the full error.rs file structure
wc -l packages/cipherstash-proxy/src/error.rs

Repository: cipherstash/proxy

Length of output: 104


🏁 Script executed:

# Get the second part of error.rs to find ConfigError
cat -n packages/cipherstash-proxy/src/error.rs | sed -n '100,250p'

Repository: cipherstash/proxy

Length of output: 6270


🏁 Script executed:

# Check the complete config building to see if validation happens before init_zerokms_client is called
cat -n packages/cipherstash-proxy/src/config/tandem.rs | sed -n '199,220p'

Repository: cipherstash/proxy

Length of output: 1651


🏁 Script executed:

# Look at the main.rs or lib.rs to understand when ZeroKms::init is called
find packages/cipherstash-proxy/src -name "main.rs" -o -name "lib.rs" | head -5

Repository: cipherstash/proxy

Length of output: 137


🏁 Script executed:

# Check when ZeroKms::init is called
rg "ZeroKms::init\|ZeroKms::new" packages/cipherstash-proxy/src/ -B 3 -A 3

Repository: cipherstash/proxy

Length of output: 43


🏁 Script executed:

# Look at what ZeroKMSBuilderError contains
rg "pub.*struct ZeroKMSBuilderError\|pub.*enum ZeroKMSBuilderError" packages/cipherstash-proxy/src/ -A 10

Repository: cipherstash/proxy

Length of output: 43


🏁 Script executed:

# Check main.rs to see how ZeroKms is initialized
cat -n packages/cipherstash-proxy/src/main.rs | head -100

Repository: cipherstash/proxy

Length of output: 4063


🏁 Script executed:

# Search for ZeroKms initialization anywhere in the codebase
rg "ZeroKms" packages/cipherstash-proxy/src/main.rs packages/cipherstash-proxy/src/lib.rs -B 2 -A 2

Repository: cipherstash/proxy

Length of output: 43


🏁 Script executed:

# Check if there's validation of client_id as UUID format before init_zerokms_client
rg "client_id" packages/cipherstash-proxy/src/config/ -B 2 -A 2

Repository: cipherstash/proxy

Length of output: 2537


🏁 Script executed:

# Check how init_zerokms_client is called from ZeroKms::init
cat -n packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs | sed -n '44,46p'

Repository: cipherstash/proxy

Length of output: 205


🏁 Script executed:

# Check the exact return type and how errors should be handled
cat -n packages/cipherstash-proxy/src/proxy/zerokms/mod.rs | sed -n '16,19p'

Repository: cipherstash/proxy

Length of output: 248


🏁 Script executed:

# Let's verify what happens if we look at how other builders handle parse errors in the codebase
rg "\.parse\(\)" packages/cipherstash-proxy/src/ -B 2 -A 5 | grep -A 10 "Result\|map_err\|expect"

Repository: cipherstash/proxy

Length of output: 2469


Map config parse failures to proper errors instead of panicking.

Lines 26-32 and 39-42 use expect() on values from operator config (client_id, client_key, zerokms_host). These are not validated during config loading, so malformed values will abort startup instead of returning a proper typed error. Either validate these values during config load (preferred), or convert parse failures into appropriate error types returned from this function.

🤖 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 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
packages/cipherstash-proxy/src/error.rs (1)

55-58: ⚠️ Potential issue | 🟠 Major

Keep builder failures inside the ZeroKMS domain.

Error::ZeroKMSBuilder splits one problem domain across two top-level variants and leaks the construction mechanism into the public error taxonomy. Fold cipherstash_client::zerokms::ZeroKMSBuilderError into ZeroKMSError instead, then keep Error exposing only Error::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.rs and 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 | 🟠 Major

Don't panic on malformed ZeroKMS config.

Line 18 narrows this path to ZeroKMSBuilderError, and Line 30, Line 32, and Line 41 compensate by calling expect(). packages/cipherstash-proxy/src/config/tandem.rs still stores client_id and client_key as raw Strings, 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 the expect() calls with map_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

📥 Commits

Reviewing files that changed from the base of the PR and between 8cb9a16 and c524252.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • Cargo.toml
  • packages/cipherstash-proxy-integration/src/generate.rs
  • packages/cipherstash-proxy/Cargo.toml
  • packages/cipherstash-proxy/src/error.rs
  • packages/cipherstash-proxy/src/proxy/mod.rs
  • packages/cipherstash-proxy/src/proxy/zerokms/mod.rs
  • packages/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 tobyhede force-pushed the proxy-2.1.24-alpha.1 branch 2 times, most recently from 32126c1 to f9db5f1 Compare March 23, 2026 05:03
Migrate ZeroKMS initialization from config-builder chain
(ConsoleConfig/CtsConfig/ZeroKMSConfig/EnvSource) to the new
ZeroKMSBuilder + AutoStrategy API.

- Replace AutoRefresh<ServiceCredentials> with AutoStrategy
- Use ClientKey::from_hex_v1 for client key construction
- Update error variant: zerokms::Error::Credentials -> Error::Auth
- Add ZeroKMSBuilderError variant to Error enum
- Remove build_zerokms_config (replaced by init_zerokms_client)
- Update cts-common to 0.34.0-alpha.3
- Pin vitaminc to 0.1.0-pre4.1 (pre4.2 broke backward compat)
@tobyhede tobyhede force-pushed the proxy-2.1.24-alpha.1 branch from f9db5f1 to a20d56a Compare March 23, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant