feat(cli): connection flags + DX (quiet logging, random port, best-effort TLS) - #397
feat(cli): connection flags + DX (quiet logging, random port, best-effort TLS)#397coderdan wants to merge 5 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds CLI DB and TLS flags; parses PostgreSQL URLs into CS_DATABASE__* environment variables with CLI overrides; honors --no-tls; enables bind fallback to an OS-assigned port when appropriate; threads a force-TLS flag through init and reloads; lowers some backend error logging; updates related tests. ChangesCLI, config, connect, and init
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds CLI-only database connection configuration and inbound TLS override support for cipherstash-proxy, making the binary easier to run without preconfigured environment variables.
Changes:
- Adds
--database-url,--db-port,--db-password, and--no-tlsCLI flags. - Applies database URL values into
CS_DATABASE__*env vars before layered config loading. - Fixes
--db-userto setCS_DATABASE__USERNAMEinstead of the unusedCS_DATABASE__USER.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/cipherstash-proxy/src/cli/mod.rs |
Adds the new CLI arguments and help text. |
packages/cipherstash-proxy/src/config/tandem.rs |
Applies CLI database overrides, disables inbound TLS when requested, and adds regression coverage for URL parsing/user env var behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(password) = pg.get_password() { | ||
| if let Ok(password) = std::str::from_utf8(password) { | ||
| env::set_var("CS_DATABASE__PASSWORD", password); | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cipherstash-proxy/src/config/tandem.rs (1)
395-398: ⚡ Quick winDoc comment contains stale text from another function.
Lines 395-396 ("Extracts a field name (if present)..." and "This is called in
build...") describeextract_missing_field_and_keybelow, notapply_database_url. This appears to be left over from when the new function was inserted.📝 Proposed fix
-/// Extracts a field name (if present) from a config::ConfigError string -/// This is called in `build` if a ConfigError message contains the string `missing field` /// Parse a PostgreSQL connection string (e.g. /// "postgres://user:pass@host:5432/dbname") and set the corresponding /// CS_DATABASE__* env vars for each component present. Individual --db-* flags /// are applied after this and take precedence. fn apply_database_url(url: &str) -> Result<(), Error> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cipherstash-proxy/src/config/tandem.rs` around lines 395 - 398, The doc comment above apply_database_url is stale and describes extract_missing_field_and_key instead; update the comment for apply_database_url to accurately describe that it parses a PostgreSQL connection string (e.g. "postgres://user:pass@host:5432/dbname"), sets the CS_DATABASE__* environment variables for components present, and notes that individual --db-* flags (applied later) take precedence; remove or relocate the two sentences about extracting field names and the build call so they belong with extract_missing_field_and_key (or delete them if redundant).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Around line 423-427: The code currently drops non-UTF8 passwords silently;
update the pg.get_password() branch to log a warning when
std::str::from_utf8(password) returns Err instead of silently skipping—use your
project's logger (e.g., tracing::warn! or log::warn!) to emit a clear message
that the password contains non-UTF8 bytes and CS_DATABASE__PASSWORD was not set,
and include a safe representation of the raw bytes (hex or base64) for
debugging; keep the existing env::set_var("CS_DATABASE__PASSWORD", password)
path unchanged when from_utf8 succeeds.
---
Nitpick comments:
In `@packages/cipherstash-proxy/src/config/tandem.rs`:
- Around line 395-398: The doc comment above apply_database_url is stale and
describes extract_missing_field_and_key instead; update the comment for
apply_database_url to accurately describe that it parses a PostgreSQL connection
string (e.g. "postgres://user:pass@host:5432/dbname"), sets the CS_DATABASE__*
environment variables for components present, and notes that individual --db-*
flags (applied later) take precedence; remove or relocate the two sentences
about extracting field names and the build call so they belong with
extract_missing_field_and_key (or delete them if redundant).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 240c19cb-ffdf-4e10-b6ca-9aea6530acf7
📒 Files selected for processing (2)
packages/cipherstash-proxy/src/cli/mod.rspackages/cipherstash-proxy/src/config/tandem.rs
coderdan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cipherstash-proxy/src/connect/mod.rs`:
- Around line 67-82: The primary and fallback bind paths both format host:port
with naive string interpolation which breaks on IPv6 literals; update the code
so both TcpListener::bind calls use a proper socket-address builder or
consistently bracket IPv6 hosts (e.g. construct a SocketAddr via
ServerConfig::to_socket_address() or add logic that wraps an IPv6 literal in [ ]
before combining with port) so that the same bracket-handling used for the
primary path is applied to the fallback path (references: TcpListener::bind
calls and ServerConfig::to_socket_address()).
In `@packages/cipherstash-proxy/src/main.rs`:
- Around line 233-240: The reload logic rejects SIGHUP reloads because init may
have downgraded the in-memory TandemConfig (clearing config.tls and setting
config.server.require_tls = false) while a freshly loaded new_config still shows
TLS set, so has_network_config_changed sees a diff and forces a restart; fix by
normalizing both configs to the same effective/network representation before
comparing — e.g., extract the TLS-downgrade/normalization performed in init into
a helper (or call the same downgrade logic) and apply it to new_config inside
reload_application_config (before has_network_config_changed), or change
has_network_config_changed to compare “effective” TLS state instead of raw
config.tls/config.server.require_tls; target symbols: init, TandemConfig::load,
reload_application_config, has_network_config_changed, Proxy { config } and the
fields TandemConfig.tls and TandemConfig.server.require_tls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2214fe66-11bb-48f0-ae08-9fb700a83c51
📒 Files selected for processing (4)
packages/cipherstash-proxy/src/cli/mod.rspackages/cipherstash-proxy/src/config/tandem.rspackages/cipherstash-proxy/src/connect/mod.rspackages/cipherstash-proxy/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cipherstash-proxy/src/config/tandem.rs
| Err(err) if err.kind() == std::io::ErrorKind::AddrInUse && allow_random_fallback => { | ||
| match TcpListener::bind(format!("{}:0", server.host)).await { | ||
| Ok(listener) => { | ||
| println!( | ||
| "Port {} is already in use; listening on an OS-assigned port instead.", | ||
| server.port | ||
| ); | ||
| report_listening(&listener); | ||
| return listener; | ||
| } | ||
| Err(err) => { | ||
| error!(msg = "Error binding connection", error = err.to_string()); | ||
| std::process::exit(exitcode::CONFIG); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how ServerConfig::to_socket_address formats host:port (brackets for IPv6?)
ast-grep --pattern $'fn to_socket_address($$$) {
$$$
}'
echo "----- raw search -----"
rg -nP -C3 '\bto_socket_address\b'Repository: cipherstash/proxy
Length of output: 2830
Fix IPv6-literal concern: primary bind has the same formatting issue as the fallback
The fallback uses format!("{}:0", server.host), but ServerConfig::to_socket_address() (used by the retry/primary path) is also format!("{}:{}", self.host, self.port) with no IPv6-literal bracket handling. So the “primary presumably handles this” distinction doesn’t hold; if IPv6 literals are supported, both paths need consistent bracketed formatting (or a socket-address builder that handles IPv6 correctly).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cipherstash-proxy/src/connect/mod.rs` around lines 67 - 82, The
primary and fallback bind paths both format host:port with naive string
interpolation which breaks on IPv6 literals; update the code so both
TcpListener::bind calls use a proper socket-address builder or consistently
bracket IPv6 hosts (e.g. construct a SocketAddr via
ServerConfig::to_socket_address() or add logic that wraps an IPv6 literal in [ ]
before combining with port) so that the same bracket-handling used for the
primary path is applied to the fallback path (references: TcpListener::bind
calls and ServerConfig::to_socket_address()).
| (Some(_), Some(err), false) => { | ||
| eprintln!( | ||
| "TLS configuration is invalid ({err}); falling back to a plaintext listener. \ | ||
| Pass --tls to make this fatal, or --no-tls to silence this warning." | ||
| ); | ||
| config.tls = None; | ||
| config.server.require_tls = false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm has_network_config_changed compares tls/require_tls and is used on reload
rg -nP -C3 '\bhas_network_config_changed\b'
# Confirm reload uses the live (possibly downgraded) proxy.config
rg -nP -C3 'reload_application_config\s*\('Repository: cipherstash/proxy
Length of output: 2326
🏁 Script executed:
#!/bin/bash
# Inspect TLS validation/downgrade logic around the reported lines
sed -n '200,260p' packages/cipherstash-proxy/src/main.rs
# Inspect reload + change-detection logic around has_network_config_changed usage
sed -n '280,360p' packages/cipherstash-proxy/src/main.rs
# Find where require_tls is set/cleared during init
rg -n --context 2 'require_tls' packages/cipherstash-proxy/src/main.rsRepository: cipherstash/proxy
Length of output: 4552
🏁 Script executed:
#!/bin/bash
# Confirm SIGHUP handler passes &proxy.config into reload_application_config
sed -n '60,130p' packages/cipherstash-proxy/src/main.rs
# Locate Proxy struct definition and how it stores config (look for `config:` field)
rg -n "struct Proxy\b" packages/cipherstash-proxy/src -S
rg -n "\bconfig:\s*TandemConfig\b|\bconfig:\s*.*TandemConfig\b|proxy\.config\b" packages/cipherstash-proxy/src -S
# Locate Proxy::init to see what gets stored on success
rg -n "impl Proxy\b|fn init\(config:|Proxy::init" packages/cipherstash-proxy/src -SRepository: cipherstash/proxy
Length of output: 7142
🏁 Script executed:
#!/bin/bash
# Check whether TandemConfig::load validates TLS (would affect whether new_config.tls stays Some)
rg -n "fn load\\b|impl .*TandemConfig" packages/cipherstash-proxy/src/config/tandem.rs
sed -n '1,260p' packages/cipherstash-proxy/src/config/tandem.rs
rg -n "check_cert|check_private_key|configure_server|tls::configure" packages/cipherstash-proxy/src/config/tandem.rsRepository: cipherstash/proxy
Length of output: 9703
🏁 Script executed:
#!/bin/bash
# Locate ServerConfig definition + defaults for require_tls
rg -n "struct ServerConfig|require_tls" packages/cipherstash-proxy/src/config -S
# Show the portion that defines ServerConfig fields/defaults and how require_tls is derived
fd -e rs packages/cipherstash-proxy/src/config -a | head -n 20 >/dev/null 2>&1 || true
# Print candidate server config files around require_tls
rg -n "require_tls" packages/cipherstash-proxy/src/config -S --files-with-matchesRepository: cipherstash/proxy
Length of output: 966
🏁 Script executed:
#!/bin/bash
set -e
# Show ServerConfig struct + default for require_tls
sed -n '1,120p' packages/cipherstash-proxy/src/config/server.rs
# Find all assignments to require_tls during config build/load
rg -n "require_tls\s*=" packages/cipherstash-proxy/src/config -S
rg -n "require_tls" packages/cipherstash-proxy/src -S
# Inspect any TLS parsing/config wiring in tandem.rs (where tls Some(...) might imply require_tls)
rg -n "tls" packages/cipherstash-proxy/src/config/tandem.rs -S --context 2Repository: cipherstash/proxy
Length of output: 5403
Non-fatal TLS downgrade can prevent SIGHUP reloads indefinitely (until TLS is removed/disabled or the process restarts).
(Some(_), Some(err), false) => {
eprintln!(
"TLS configuration is invalid ({err}); falling back to a plaintext listener. \
Pass --tls to make this fatal, or --no-tls to silence this warning."
);
config.tls = None;
config.server.require_tls = false;
}
When inbound TLS is configured but invalid and --tls is not set, init downgrades in place by clearing config.tls and setting config.server.require_tls = false. That downgraded TandemConfig is what the running proxy stores (Proxy { pub config: Arc<TandemConfig> }). On SIGHUP, reload_application_config(&proxy.config, &args) loads a fresh new_config via TandemConfig::load(args) (which only disables TLS for --no-tls, and does not validate/downgrade invalid certs) and then rejects the reload before calling init if has_network_config_changed(config, &new_config) reports a change. Since has_network_config_changed compares both current.tls and current.server.require_tls, the downgraded current (tls = None) differs from new_config (tls = Some), so reload returns NetworkConfigurationChangeRequiresRestart and the SIGHUP handler keeps the old proxy (unwrap_or(proxy)).
Adjust the reload change detection to compare effective (post-downgrade/normalized) network config, or apply the same downgrade logic to new_config before calling has_network_config_changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cipherstash-proxy/src/main.rs` around lines 233 - 240, The reload
logic rejects SIGHUP reloads because init may have downgraded the in-memory
TandemConfig (clearing config.tls and setting config.server.require_tls = false)
while a freshly loaded new_config still shows TLS set, so
has_network_config_changed sees a diff and forces a restart; fix by normalizing
both configs to the same effective/network representation before comparing —
e.g., extract the TLS-downgrade/normalization performed in init into a helper
(or call the same downgrade logic) and apply it to new_config inside
reload_application_config (before has_network_config_changed), or change
has_network_config_changed to compare “effective” TLS state instead of raw
config.tls/config.server.require_tls; target symbols: init, TandemConfig::load,
reload_application_config, has_network_config_changed, Proxy { config } and the
fields TandemConfig.tls and TandemConfig.server.require_tls.
Review feedback addressed (pushed in c928bdf)Thanks for the reviews. Status: Fixed in this PR
Deferred to follow-up issues (real, but pre-existing / behavioural and out of scope for this CLI-feature PR)
Local validation: |
…db-user Makes the proxy usable as a plain CLI (e.g. `npx stash proxy`) without hand-setting env vars: - --database-url: parse a full postgres:// connection string (host, port, user, password, dbname) via tokio-postgres' parser; individual flags override. - --db-port / --db-password: the missing scalar connection flags. - --no-tls: listen for client connections in plaintext, overriding any CS_TLS__* env/config (inbound only; does not affect the proxy->database connection). Avoids a hard startup error when a stray CS_TLS__* points at a missing cert. Fixes a bug: --db-user set CS_DATABASE__USER, but the config field is `username` (CS_DATABASE__USERNAME), so --db-user was silently a no-op. Adds a regression test for the URL parsing + the USERNAME fix. Validated locally: `npx stash proxy --database-url postgres://.../db --no-tls` connects and listens. Signed-off-by: James Sadler <james@cipherstash.com>
…t TLS DX improvements for running proxy as a CLI (e.g. npx stash proxy): - Quiet by default: log errors only unless --debug, an explicit --log-level / CS_LOG__LEVEL, or a config file is present (production/observability via env or config is unchanged). The listen address always prints. - Random-port fallback: if the listen port is the default (no CS_SERVER__PORT, no config file) and is already in use, bind an OS-assigned port and report it instead of failing. An explicitly configured port is still respected. - Best-effort inbound TLS: a TLS misconfiguration now falls back to a plaintext listener with a notice instead of exiting. --tls forces TLS (fatal if it can't be set up); --no-tls forces plaintext. Adds a regression test for the random-port fallback. Relates to BUG-300-adjacent CLI usability work. Signed-off-by: James Sadler <james@cipherstash.com>
A PostgreSQL error forwarded to the client is normal proxy operation -- the client already surfaces it -- but the backend logged every one at ERROR level as the full Debug-formatted ErrorResponse struct. That is unreadable and, with quiet-by-default logging, was the only thing showing on a failed query (and it bled into --psql sessions). Log it at debug instead, concisely (SQLSTATE code + message) via a new ErrorResponse::field() accessor. Nothing on a SQL error by default; a clean one-line entry under --debug. Adds a unit test for the accessor. Signed-off-by: James Sadler <james@cipherstash.com>
- Add the six new Args fields (database_url, db_port, db_password, no_tls, tls, debug) to the struct literal in the migrate integration test. The proxy crate compiled, but the integration crate's literal was stale, so the full-workspace test build failed E0063 — this is the CI failure. - Reject a non-UTF8 password from --database-url with a clear InvalidParameter error instead of silently dropping it (which would connect with no/stale credentials). Add a regression test; the branch is reachable because tokio-postgres stores the URL password as raw bytes without UTF-8 validation. - Remove a stale doc comment left over from another function above apply_database_url. Signed-off-by: James Sadler <james@cipherstash.com>
Signed-off-by: James Sadler <james@cipherstash.com>
c928bdf to
fde3ad2
Compare
|
Reviewed at Fix before merge1.
Suggest cloning 2. The fallback arm sets Two further notes: this arm emits only The plaintext fallback itself is a reasonable DX default. Discarding an explicit 3. The help text reads "Require inbound (client-facing) TLS", but 4. The fallback guard arm is matched before the retry arm at Suggest falling back only once Question5.
Minor
Already tracked
|
Implements CIP-3823, making the proxy usable as a plain CLI (for example,
npx stash proxy) without hand-setting connection environment variables.Connection flags
--database-url <URL>to parse a PostgreSQL connection string viatokio-postgres; individual--db-*flags take precedence.--db-port(-P) and--db-password(-W).--db-userto setCS_DATABASE__USERNAMEinstead of the unusedCS_DATABASE__USER.CLI behavior
--log-levelandCS_LOG__LEVELvalues, including an explicitinfo; an explicit level or config file takes precedence over--debug.--tlsmakes TLS mandatory, while--no-tlsforces plaintext.Validation
cargo fmt --all -- --checkcargo clippy --all-features -- -D warningscargo check -p cipherstash-proxy-integrationcargo test -p cipherstash-proxy database_url_sets_connection_envcargo test -p cipherstash-proxy bind_falls_back_to_random_port_when_default_in_usecargo test -p cipherstash-proxy log_level_preserves_whether_it_was_explicitly_setcipherstash-proxyunit suite: 136/137 passed in parallel; the remaining environment-mutating config test passed in isolation.The branch is rebased onto current
origin/main, and all five commits include DCO sign-offs.