Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/cipherstash-proxy-integration/src/migrate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,15 @@ mod tests {
log_level: LogLevel::Debug,
log_format: LogFormat::Pretty,
command: None,
database_url: None,
db_host: None,
db_port: None,
db_name: None,
db_user: None,
db_password: None,
no_tls: false,
tls: false,
debug: false,
};

let config = match TandemConfig::load(&args) {
Expand Down
37 changes: 37 additions & 0 deletions packages/cipherstash-proxy/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,23 @@ const DEFAULT_CONFIG_FILE: &str = "cipherstash-proxy.toml";
/// CipherStash Proxy keeps your sensitive data in PostgreSQL encrypted and searchable, with no changes to SQL.
///
pub struct Args {
/// Optional full PostgreSQL connection string, e.g.
/// "postgres://user:pass@host:5432/dbname".
/// Sets host, port, user, password and database name at once.
/// Individual flags below override matching parts of the URL.
#[arg(long, value_name = "URL", verbatim_doc_comment)]
pub database_url: Option<String>,

/// Optional database host to connect to.
/// Uses env or config file if not specified.
#[arg(short = 'H', long)]
pub db_host: Option<String>,

/// Optional database port to connect to.
/// Uses env or config file if not specified.
#[arg(short = 'P', long)]
pub db_port: Option<u16>,

/// Optional database name to connect to.
/// Uses env or config file if not specified.
#[arg(value_name = "DBNAME")]
Expand All @@ -36,6 +48,31 @@ pub struct Args {
#[arg(short = 'u', long)]
pub db_user: Option<String>,

/// Optional database password.
/// Uses env or config file if not specified.
/// Prefer CS_DATABASE__PASSWORD or --database-url to avoid leaking the
/// password into shell history / the process list.
#[arg(short = 'W', long, verbatim_doc_comment)]
pub db_password: Option<String>,

/// Disable inbound (client-facing) TLS: the proxy listens for client
/// connections in plaintext. Overrides any CS_TLS__* env / config.
/// Use for local development only. Does not affect the connection from the
/// proxy to the database.
#[arg(long, verbatim_doc_comment, conflicts_with = "tls")]
pub no_tls: bool,

/// Require inbound (client-facing) TLS. Startup fails if TLS is not
/// configured or the certificate/key are invalid. Without this flag the
/// proxy uses TLS when configured and falls back to plaintext otherwise.
#[arg(long, verbatim_doc_comment)]
pub tls: bool,

/// Enable verbose (debug) logging. Without it the proxy logs errors only.
/// An explicit --log-level / CS_LOG__LEVEL or a config file takes precedence.
#[arg(long, verbatim_doc_comment)]
pub debug: bool,

/// Optional path to a CipherStash Proxy configuration file.
///
/// Default is "cipherstash-proxy.toml".
Expand Down
142 changes: 137 additions & 5 deletions packages/cipherstash-proxy/src/config/tandem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,31 @@ impl TandemConfig {
config.log.format = args.log_format;
}

// --no-tls forces a plaintext inbound listener, ignoring any CS_TLS__*
// env / config. (Does not affect the proxy -> database connection.)
if args.no_tls {
if config.tls.is_some() {
println!("Disabling inbound TLS (--no-tls): the proxy will listen without TLS");
}
config.tls = None;
config.server.require_tls = false;
}

Ok(config)
}

pub fn build_path(path: &str) -> Result<Self, Error> {
let args = Args {
config_file_path: path.to_string(),
database_url: None,
db_host: None,
db_port: None,
db_name: None,
db_user: None,
db_password: None,
no_tls: false,
tls: false,
debug: false,
log_level: LogConfig::default_log_level(),
log_format: LogConfig::default_log_format(),
command: None,
Expand Down Expand Up @@ -169,20 +185,37 @@ impl TandemConfig {
env
}));

// Command line arguments override env vars
// Command line arguments override env vars.
// A full connection string is applied first; individual flags below
// then override matching parts of it.
if let Some(url) = &args.database_url {
println!("Overriding database connection from --database-url");
apply_database_url(url)?;
}

if let Some(db_host) = &args.db_host {
println!("Overriding database host from command line argument");
env::set_var("CS_DATABASE__HOST", db_host);
}

if let Some(db_port) = &args.db_port {
println!("Overriding database port from command line argument");
env::set_var("CS_DATABASE__PORT", db_port.to_string());
}

if let Some(dbname) = &args.db_name {
println!("Overriding database name from command line argument");
env::set_var("CS_DATABASE__NAME", dbname);
}

if let Some(db_user) = &args.db_user {
println!("Overriding database user from command line argument");
env::set_var("CS_DATABASE__USER", db_user);
env::set_var("CS_DATABASE__USERNAME", db_user);
}

if let Some(db_password) = &args.db_password {
println!("Overriding database password from command line argument");
env::set_var("CS_DATABASE__PASSWORD", db_password);
}

// Source order is important!
Expand Down Expand Up @@ -358,9 +391,53 @@ impl Default for PrometheusConfig {

static RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"`([^`]+)`").unwrap());

///
/// 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> {
use std::str::FromStr;

let pg =
tokio_postgres::Config::from_str(url).map_err(|err| ConfigError::InvalidParameter {
name: "database-url".to_string(),
value: err.to_string(),
})?;

if let Some(host) = pg.get_hosts().iter().find_map(|h| match h {
tokio_postgres::config::Host::Tcp(host) => Some(host.clone()),
_ => None,
}) {
env::set_var("CS_DATABASE__HOST", host);
}

if let Some(port) = pg.get_ports().first() {
env::set_var("CS_DATABASE__PORT", port.to_string());
}

if let Some(user) = pg.get_user() {
env::set_var("CS_DATABASE__USERNAME", user);
}

if let Some(password) = pg.get_password() {
// The password is required downstream as a String. Rather than silently
// dropping a non-UTF8 password (which would leave the proxy connecting
// with no password, or a stale env/config one), fail clearly.
let password =
std::str::from_utf8(password).map_err(|_| ConfigError::InvalidParameter {
name: "database-url".to_string(),
value: "password contains invalid UTF-8".to_string(),
})?;
env::set_var("CS_DATABASE__PASSWORD", password);
}
Comment on lines +422 to +432
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if let Some(dbname) = pg.get_dbname() {
env::set_var("CS_DATABASE__NAME", dbname);
}

Ok(())
}

/// Expected string is in the forms:
/// "missing field `{field}}` for key `{key}`"
/// "missing field `{field}}`"
Expand Down Expand Up @@ -419,6 +496,61 @@ mod tests {

const CS_PREFIX: &str = "CS_TEST";

#[test]
/// --database-url sets each CS_DATABASE__* var, including USERNAME (not USER
/// -- a previous bug set the wrong var so --db-user was a silent no-op).
fn database_url_sets_connection_env() {
use crate::config::tandem::apply_database_url;
with_no_cs_vars(|| {
temp_env::with_vars_unset(
[
"CS_DATABASE__HOST",
"CS_DATABASE__PORT",
"CS_DATABASE__USERNAME",
"CS_DATABASE__USER",
"CS_DATABASE__PASSWORD",
"CS_DATABASE__NAME",
],
|| {
apply_database_url("postgres://alice:s3cret@db.example.com:5430/orders")
.unwrap();

assert_eq!(
std::env::var("CS_DATABASE__HOST").unwrap(),
"db.example.com"
);
assert_eq!(std::env::var("CS_DATABASE__PORT").unwrap(), "5430");
assert_eq!(std::env::var("CS_DATABASE__USERNAME").unwrap(), "alice");
assert_eq!(std::env::var("CS_DATABASE__PASSWORD").unwrap(), "s3cret");
assert_eq!(std::env::var("CS_DATABASE__NAME").unwrap(), "orders");
// The old (buggy) variable name must not be set.
assert!(std::env::var("CS_DATABASE__USER").is_err());
},
);
});
}

#[test]
/// A non-UTF8 password in --database-url is an error, not a silent no-op.
/// tokio-postgres percent-decodes the password to raw bytes without UTF-8
/// validation, so `%FF` yields invalid UTF-8; we must reject it rather than
/// leave CS_DATABASE__PASSWORD unset (which would connect with no/stale
/// credentials).
fn database_url_rejects_non_utf8_password() {
use crate::config::tandem::apply_database_url;
with_no_cs_vars(|| {
temp_env::with_vars_unset(["CS_DATABASE__PASSWORD"], || {
let result = apply_database_url("postgres://alice:%FF@db.example.com/orders");
assert!(
result.is_err(),
"expected a non-UTF8 password to be rejected"
);
// And it must not have set a password from the bad URL.
assert!(std::env::var("CS_DATABASE__PASSWORD").is_err());
});
});
}

#[test]
/// the env vars from stash setup should be the preferred option
/// File -> extended env (generated by the config struct layout) -> stash setup env
Expand Down
67 changes: 63 additions & 4 deletions packages/cipherstash-proxy/src/connect/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,35 @@ pub async fn database(config: &DatabaseConfig) -> Result<Client, Error> {
Ok(client)
}

pub async fn bind_with_retry(server: &ServerConfig) -> TcpListener {
let address = &server.to_socket_address();
pub async fn bind_with_retry(server: &ServerConfig, allow_random_fallback: bool) -> TcpListener {
let address = server.to_socket_address();
let mut retry_count = 0;

loop {
match TcpListener::bind(address).await {
match TcpListener::bind(&address).await {
Ok(listener) => {
info!(msg = "Server waiting for connections", address);
report_listening(&listener);
return listener;
}
// The configured port is in use, but it's only the default (not
// explicitly set), so fall back to an OS-assigned port and report it
// rather than failing.
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);
}
}
}
Comment on lines +67 to +82
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 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()).

Err(err) => {
if retry_count > MAX_RETRY_COUNT {
error!(
Expand All @@ -80,6 +99,21 @@ pub async fn bind_with_retry(server: &ServerConfig) -> TcpListener {
}
}

/// Report the address the proxy is listening on. Uses `println!` so the
/// listening address (including an OS-assigned fallback port) is always visible,
/// even when logging is quiet.
fn report_listening(listener: &TcpListener) {
match listener.local_addr() {
Ok(addr) => {
println!("CipherStash Proxy listening on {addr}");
info!(msg = "Server waiting for connections", address = %addr);
}
Err(_) => {
info!(msg = "Server waiting for connections");
}
}
}

pub async fn connect_with_retry(addr: &str) -> Result<TcpStream, Error> {
let mut retry_count = 0;

Expand Down Expand Up @@ -157,3 +191,28 @@ pub fn configure(stream: &TcpStream) {
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[tokio::test]
async fn bind_falls_back_to_random_port_when_default_in_use() {
// Occupy a port, then ask bind_with_retry to bind the same one.
let occupied = TcpListener::bind("127.0.0.1:0").await.unwrap();
let occupied_port = occupied.local_addr().unwrap().port();

let server = ServerConfig {
host: "127.0.0.1".to_string(),
port: occupied_port,
..ServerConfig::default()
};

// With fallback allowed, it binds a different OS-assigned port.
let listener = bind_with_retry(&server, true).await;
let bound_port = listener.local_addr().unwrap().port();

assert_ne!(bound_port, occupied_port);
assert_ne!(bound_port, 0);
}
}
Loading
Loading