Repository navigation
feat(cli): connection flags + DX (quiet logging, random port, best-effort TLS) - #397
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.
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
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()).
There was a problem hiding this comment.
Still deferred to #402. This limitation already exists on main, and this PR doesn't change it.
Generated by Claude Code
| (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.
There was a problem hiding this comment.
Still deferred to #403. 502e51e makes an invalid certificate fatal when require_tls is set, but the fallback-then-reload case this thread describes is unchanged.
Generated by Claude Code
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: |
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
|
freshtonic
left a comment
There was a problem hiding this comment.
The branch conflicts with main. #443 deleted postgresql/backend.rs and postgresql/messages/error_response.rs, so the error-log change must move to the pg-proto code.
|
Heads-up: we're taking over this branch to get it mergeable. I'm about to push two commits as a fast-forward of
Still open, and not addressed in this push:
Generated by Claude Code |
freshtonic
left a comment
There was a problem hiding this comment.
Re-reviewed head 502e51eb. I read the delta from fde3ad22 and the full diff against main. The merge with #443 is clean, and every fix in the takeover comment is present. For each fix, I removed it and its regression test failed. But two problems remain in the merged code.
Blocking
B1. Proxy no longer logs its own errors at error!, and the official container now uses the quiet default.
- The doc comment at
postgresql/middleware/backend.rs:409and the CHANGELOG (CHANGELOG.md:31) say: "Errors that Proxy itself generates are still logged aterrorlevel". This stopped being true after #443:- The pg-proto frontend logs errors from Query, Parse and Bind with
warn!(middleware/frontend.rs:161,:201,:227-247). - The Execute refusal path (
frontend.rs:178-180) does not log at all. - The
error!that the claim refers to was atpostgresql/frontend.rs:1310onfde3ad22, and #443 deleted it.
- The pg-proto frontend logs errors from Query, Parse and Bind with
proxy.Dockerfileanddocker-entrypoint.shset noCS_LOG__LEVELand ship no config file, so the official image gets the new error-only default. The comment atmain.rs:26-38says that production "sets a level or ships a config", but the image does neither.- Scenario: a container with env-only config. A query fails a mapping or encryption check, or a connection times out. Proxy now logs nothing. On
main, these events appear at warn or info. - Fix:
- Correct the doc comment and the CHANGELOG.
- Then do one of these: log Proxy's own client errors at
error!, or apply the quiet default only when stdout is a TTY.log_formatalready uses the same test.
B2. A SIGHUP can switch a running TLS listener to plaintext.
- The code is
reload_application_config(main.rs:284-305) together withTandemConfig::check_tls(config/tandem.rs:319). - Scenario: TLS is configured with
CS_TLS__CERTIFICATE_PATHandCS_TLS__PRIVATE_KEY_PATH, andrequire_tlsis not set, which is the default.- A certificate rotation leaves a bad file at the same path.
- On SIGHUP,
has_network_config_changedcompares the TLS path config. The paths did not change, so the reload continues. initgetsFallbackToPlaintext, which setstls = Noneon the new config. The newProxythen replaces the old one.- From then on, every new connection is plaintext, because
context/mod.rs:1842readsconfig.tlsfor each connection.
- Clients that use libpq's default
sslmode=preferconnect in plaintext with no error. Onmain, the same event stopped the process. - The takeover fix makes this fatal only when
require_tls=true. #403 does not cover this case, because #403 is about reloads that fail after a fallback at startup. - Fix: in
reload_application_config, treat a fallback result as a reload error and keep the old proxy.
Should fix
S1. Startup fallback when the operator configured TLS (tandem.rs:319). An operator who set CS_TLS__* asked for TLS. Today a bad mount gives a plaintext listener that exits 0, and the only signals are an eprintln! and a warn! that the quiet default hides. The DX case for npx stash proxy is the NotConfigured branch, not this one. I suggest making an invalid certificate fatal whenever TLS config is present, or at least logging it at error!.
S2. --database-url still ignores some options without an error (tandem.rs:475-545).
- A scratch test showed that these are accepted and then ignored:
channel_binding=require, which weakens SCRAM channel bindinghostaddr=10.0.0.9, after which Proxy connects by host nameport=5432,5433, where Proxy takes the first porttarget_session_attrs=read-writeoptions=-csearch_path=fooapplication_name,connect_timeout,load_balance_hostsandsslnegotiation
- Fix: refuse every option except user, password, dbname, host, port and
sslmode=prefer. Each ignored option has a default value intokio_postgres::Config, so the check is easy.
S3. A URL with no port replaces the configured port with 5432 (tandem.rs:518). tokio-postgres fills in the default port, so postgres://u@db/x together with CS_DATABASE__PORT=6543 connects to 5432. This disagrees with the docs, which say that the URL sets only the parts that it contains.
S4. The sslmode error messages are not helpful. sslmode=verify-full, sslmode=allow and sslrootcert= fail with only "Invalid database-url: invalid connection string", without a pointer to CS_DATABASE__WITH_TLS_VERIFICATION. The message for sslmode=disable tells the user to set TLS verification, which is wrong for that case.
Nits
log/targets.rs:40: an explicitCS_LOG__PROTOCOL_LEVEL=infolooks the same as the default, so the quiet default changes it toerror.- No test covers the log-config choice at
main.rs:26-38or theport_configuredlogic atmain.rs:74-76. connect/mod.rs:168usesprintln!, and the TLS notices (main.rs:222) useeprintln!. Both put lines that are not JSON into structured log streams.- Proxy prints the
NotConfigurednotice again on every SIGHUP. - The help text for
--no-tls(cli/mod.rs:61) does not say that--no-tlsalso overridesCS_SERVER__REQUIRE_TLS=true.
Deferred items
- #402 (IPv6 bind): OK to defer.
mainhas the same limitation. - #403 (reloads after a fallback at startup): OK to defer, because it fails closed. But please fix B2 first, or together with it.
- Config file and
--debug: I think--debugshould win over the presence of a config file, because it is an explicit flag on this run. It should lose only to an explicit level, from--log-levelorCS_LOG__LEVEL. This is not a security issue, so it can wait. Either way, the comments atmain.rs:26and incli/mod.rsshould agree.
What I checked and found correct
- The merge:
git show --remerge-diffshows only the expected hunks, nothing is duplicated, and the PR merges cleanly with currentmain. - The new backend debug log records only the code and message fields of the error, at
target: PROTOCOL. Before, it logged every field (including Detail, which can contain key values) aterror!. So it now logs less, and nothing sensitive is added. - The fixes and their tests. For each fix, I removed the fix and its regression test failed:
- An invalid certificate with
require_tlsstops the process. --tlssetsrequire_tls.- The quiet default keeps the loaded log config and the explicit per-target levels.
- The TLS notice prints unless
--no-tlsis given. - The random-port fallback runs only after the bind retries.
--database-urlrefusessslmode, Unix socket hosts and multiple hosts.
- An invalid certificate with
- URL edge cases: percent-encoded socket hosts,
?host=/path,?host=db2,sslmode=disable, and key=value DSNs withsslmodeare refused correctly. - SIGHUP with
require_tls: an invalid certificate stops the process, as onmain. --tlswith no TLS config returnsTlsConfigurationRequired.--db-user:DatabaseConfighasusernameand no serde alias, soCS_DATABASE__USERNAMEis correct.- Log level: an explicit
infofrom the flag or the environment is kept. - CI: all 9 checks pass.
What I ran
cargo test -p cipherstash-proxy --lib: 411 passed. The one failure,invalid_client_id_uuid, is the known environment race. Theconfig::tests with--test-threads=1: 50 of 50 passed.- The bind tests, the mutations above, and a scratch test of URL edge cases. I reverted all changes after the runs.
|
@freshtonic thanks for the re-review. Pushed 99a271b as a fast-forward of 502e51e. Every behaviour change has a regression test that failed before the fix. Blocking
Should fix
Config file and Nits
Generated by Claude Code |
freshtonic
left a comment
There was a problem hiding this comment.
Re-reviewed head 99a271b5. Both blockers are fixed, and so are S1–S4. For each new fix, I removed it and its regression test failed. CI passed on this head. I approve. Two small items about the port are below; they can be fixed in this PR or in a follow-up.
Fixed
-
B2 and S1 (TLS):
- With TLS configured,
resolve_inbound_tls(config/tandem.rs:297) now returns an error for any problem. Proxy logs the error aterror!and exits, at startup and on SIGHUP. - I probed a bad, truncated, missing or swapped cert or key, and an unreadable key (
chmod 000). Withrequire_tlsfalse and withrequire_tlstrue, each one is an error. - The handshake for each connection (
driver.rs:321) does not fall back. - Only
--no-tlsand--tlsinTandemConfig::load(tandem.rs:132-138) writetlsorrequire_tls. A reload builds the current and the new config through the sameload, so the cause of #403 is gone too. --no-tlswithCS_SERVER__REQUIRE_TLS=truegives plaintext, as the help text says.
- With TLS configured,
-
B1 (logging):
- The quiet default applies only when
stdout().is_terminal()(main.rs:35,config/log.rs:103). Under Kubernetes, and under Docker without-t, Proxy keeps the loaded level, so its ownwarn!errors show. - The doc comment (
backend.rs:409) and the CHANGELOG are now correct. - Off a terminal,
report_listeninglogs the bound address atinfo!, so a script that runsnpx stash proxystill gets the port.
- The quiet default applies only when
-
S2 (
--database-urloptions): I probed 41 URL and key=value strings. The equality check refuses no valid URL that I tried, including:- explicit
sslmode=prefer - percent-encoded parts
[::1]and[2001:db8::1]- no user, no dbname, and a bare
postgres:// - options set to their default values
It refuses all nine options from my last review, and also empty
options=,hostaddrin key=value form, and unknown options. - explicit
-
S3 and S4:
- A URL with no port keeps the configured port, except for the case in S-a below.
- The
InvalidDatabaseUrlmessages point to the correct setting. They never contain the password: I tried a password together with bad options, a bad port, bad percent-encoding and broken quoting.
-
--debugnow outranks a config file, and the comments and help text agree.
Should fix (small; a follow-up is OK)
- S-a. A password with an unencoded
/makesurl_has_portwrong (tandem.rs:586).url_has_portcuts the URL at the first/or?before it looks for@. So forpostgres://alice:pa/ss@db1/orders, it sees the authority asalice:paand finds a:. But tokio-postgres parses the password aspa/ssand fills in port 5432, so the configured port is replaced. This is the S3 bug again. Generated base64 passwords often contain/. An empty port (db1:/orders) also sets 5432. Fix: use the last@before the path, or refuse an unencoded/,?or#in the userinfo. - S-b.
postgres://u@db/x?port=6543fails with "multiple ports are not supported" (tandem.rs:512). tokio-postgres adds 5432 for the authority and then 6543. This fails closed, but the message is wrong for this URL.
Nits
- With an explicit
CS_LOG__LEVEL=warnorerrorand no terminal, the fallback port is not visible. Thewarn!atconnect/mod.rs:132-136logs only the configuredport, and the bound address is logged only atinfo!. Please add the bound address to thewarn!. - On a terminal with stderr redirected (
2>file), the TLS-not-configured notice is lost. It is gated onstderr().is_terminal()(main.rs:210), and the quiet level filters out thewarn!. docker run -tand Composetty: truecount as a terminal, so they get the quiet default. That fits "run by hand". But the CHANGELOG (line 25) says "as in a container", which is a little too broad.- The TLS errors do not name the file, for example "peer sent no certificates", "no items found" or "Permission denied". These come from the existing
tls::configure_server, but now every operator with a bad TLS config sees them. - A SIGHUP with a bad certificate stops the whole process, together with its live sessions. This is the same as on
main, and it is not plaintext. Keeping the old proxy after a failed reload would be better, as a follow-up. - Still open, as you said: an explicit per-target
infolooks the same as the default, andport_configuredhas no test. The Execute refusal path (middleware/frontend.rs:178-184) still logs nothing, as onmain.
What I ran
cargo test -p cipherstash-proxy --lib, run outside the sandbox so that the bind tests can bind ports: 418 passed. The 2 failures are the known random environment failures. Theconfig::tests with--test-threads=1: 58 of 58 passed.cargo fmt --checkandcargo clippy -D warningsreport no problems.- One mutation for each new fix:
- the TLS error path
for_startupwith no terminal check--debugchecked after the config file- the equality check removed
- the
url_has_portfilter removed - the
sslmode=disablearm removed
- CI: all 9 checks pass.
Approved.
|
@freshtonic thanks. Pushed a fast-forward of 99a271b with S-a, S-b and two of the nits. S-a and S-b
Nits
Generated by Claude Code |
16ca520 to
7f4ad73
Compare
…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) read from the response's diagnostic fields. Nothing on a SQL error by default; a clean one-line entry under --debug. 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>
…--database-url - An invalid inbound certificate is fatal when require_tls is set, instead of silently falling back to plaintext. The fallback emits a warn! event. - --tls sets require_tls, so clients that skip SSLRequest are rejected. - The quiet default keeps the loaded log config and explicit per-target levels, replacing only levels still at their default. - The "TLS not configured" notice prints regardless of log level unless --no-tls is given. - The random-port fallback runs only after the bind retries are exhausted. - --database-url rejects sslmode, Unix socket hosts and multiple hosts rather than ignoring them. - Password help text recommends CS_DATABASE__PASSWORD only. - CHANGELOG entries for the operator-visible changes. The inbound TLS and quiet-log decisions move out of main.rs into TandemConfig::resolve_inbound_tls and LogConfig::with_default_level so they are covered by unit tests.
Address the second review round on the CLI connection flags. - Inbound TLS that is configured but invalid is fatal again, at startup and on SIGHUP reload. The plaintext fallback silently downgraded sslmode=prefer clients, including on a running listener after a bad certificate rotation. - The quiet error-only default applies only when stdout is a terminal, so containers and services keep their loaded log level. --debug now outranks the presence of a config file; only an explicit level beats it. - --database-url refuses every option Proxy does not apply (for example channel_binding, hostaddr, target_session_attrs, options), keeps the configured port when the URL has none, and points sslmode and ssl* options at CS_DATABASE__WITH_TLS_VERIFICATION, and sslmode=disable at CS_DEVELOPMENT__DISABLE_DATABASE_TLS. Its errors use a new InvalidDatabaseUrl variant documented in docs/errors.md. - Plain-text notices and the listen address print only to a terminal, and the TLS notice prints once per run rather than on every reload. - Correct the doc comment and CHANGELOG claims about error-level logging, and the --tls, --no-tls and --debug help text.
Count the ports tokio-postgres fills in with 5432 the way it parses the URL: credentials end at the first `@`, so an unencoded `/` in the password no longer ends the authority early, and an empty port counts as filled in. Skipping only the filled-in ports also lets `?port=6543` set the port instead of failing as "multiple ports". The port-fallback warning now includes the bound address, so it is visible at warn level without a terminal.
7f4ad73 to
68d6dce
Compare
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.