From 6532c553946856b4acc326f3b9ca6cc6abc718d0 Mon Sep 17 00:00:00 2001 From: John Howard Date: Tue, 28 May 2024 14:36:58 -0700 Subject: [PATCH] Fix regression in source IP spoofing (#1085) * Fix regression in source IP spoofing We were using the `cfg` in pool since #1040. `cfg` is not whether its used, but rather the intent of the user -- we may enable or disable at runtime. This renames the field to make it clear it should not be used like this, and fixes the issue. This went through CI due to https://github.com/istio/ztunnel/issues/1084. * cleanup --- src/config.rs | 7 ++++--- src/proxy.rs | 2 +- src/proxy/inbound.rs | 6 ++---- src/proxy/inbound_passthrough.rs | 6 ++---- src/proxy/outbound.rs | 17 +++++++++++------ src/proxy/pool.rs | 14 ++++++++------ src/proxy/socks5.rs | 8 +++++++- 7 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/config.rs b/src/config.rs index b45935d27..de9f05bae 100644 --- a/src/config.rs +++ b/src/config.rs @@ -203,8 +203,9 @@ pub struct Config { /// Specify the number of worker threads the Tokio Runtime will use. pub num_worker_threads: usize, - // If true, then use original source proxying - pub enable_original_source: Option, + // If set, explicitly configure whether to use original source. + // If unset (recommended), this is automatically detected based on permissions. + pub require_original_source: Option, // CLI args passed to ztunnel at runtime pub proxy_args: String, @@ -435,7 +436,7 @@ pub fn construct_config(pc: ProxyConfig) -> Result { pc.concurrency.unwrap_or(DEFAULT_WORKER_THREADS).into(), )?, - enable_original_source: parse(ENABLE_ORIG_SRC)?, + require_original_source: parse(ENABLE_ORIG_SRC)?, proxy_args: parse_args(), dns_resolver_cfg, dns_resolver_opts, diff --git a/src/proxy.rs b/src/proxy.rs index 44045cf0e..0eb93d994 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -418,7 +418,7 @@ pub(super) fn maybe_set_transparent( pi: &ProxyInputs, listener: &socket::Listener, ) -> Result { - Ok(match pi.cfg.enable_original_source { + Ok(match pi.cfg.require_original_source { Some(true) => { // Explicitly enabled. Return error if we cannot set it. listener.set_transparent()?; diff --git a/src/proxy/inbound.rs b/src/proxy/inbound.rs index de1c58d8c..c68926094 100644 --- a/src/proxy/inbound.rs +++ b/src/proxy/inbound.rs @@ -60,14 +60,12 @@ impl Inbound { .socket_factory .tcp_bind(pi.cfg.inbound_addr) .map_err(|e| Error::Bind(pi.cfg.inbound_addr, e))?; - let transparent = super::maybe_set_transparent(&pi, &listener)?; - // Override with our explicitly configured setting - let enable_orig_src = pi.cfg.enable_original_source.unwrap_or(transparent); + let enable_orig_src = super::maybe_set_transparent(&pi, &listener)?; info!( address=%listener.local_addr(), component="inbound", - transparent, + transparent=enable_orig_src, "listener established", ); Ok(Inbound { diff --git a/src/proxy/inbound_passthrough.rs b/src/proxy/inbound_passthrough.rs index 33bfa44dd..23794b4c9 100644 --- a/src/proxy/inbound_passthrough.rs +++ b/src/proxy/inbound_passthrough.rs @@ -47,14 +47,12 @@ impl InboundPassthrough { .tcp_bind(pi.cfg.inbound_plaintext_addr) .map_err(|e| Error::Bind(pi.cfg.inbound_plaintext_addr, e))?; - let transparent = super::maybe_set_transparent(&pi, &listener)?; - // Override with our explicitly configured setting - let enable_orig_src = pi.cfg.enable_original_source.unwrap_or(transparent); + let enable_orig_src = super::maybe_set_transparent(&pi, &listener)?; info!( address=%listener.local_addr(), component="inbound plaintext", - transparent, + transparent=enable_orig_src, "listener established", ); Ok(InboundPassthrough { diff --git a/src/proxy/outbound.rs b/src/proxy/outbound.rs index a8d97c9d0..ba07cd7f3 100644 --- a/src/proxy/outbound.rs +++ b/src/proxy/outbound.rs @@ -50,14 +50,12 @@ impl Outbound { .socket_factory .tcp_bind(pi.cfg.outbound_addr) .map_err(|e| Error::Bind(pi.cfg.outbound_addr, e))?; - let transparent = super::maybe_set_transparent(&pi, &listener)?; - // Override with our explicitly configured setting - let enable_orig_src = pi.cfg.enable_original_source.unwrap_or(transparent); + let enable_orig_src = super::maybe_set_transparent(&pi, &listener)?; info!( address=%listener.local_addr(), component="outbound", - transparent, + transparent=enable_orig_src, "listener established", ); Ok(Outbound { @@ -81,6 +79,7 @@ impl Outbound { let (sub_drain_signal, sub_drain) = drain::channel(); let pool = proxy::pool::WorkloadHBONEPool::new( self.pi.cfg.clone(), + self.enable_orig_src, self.pi.socket_factory.clone(), self.pi.cert_manager.clone(), ); @@ -557,6 +556,7 @@ mod tests { let sock_fact = std::sync::Arc::new(crate::proxy::DefaultSocketFactory); let cert_mgr = identity::mock::new_secret_manager(Duration::from_secs(10)); + let original_src = false; // for testing, not needed let outbound = OutboundConnection { pi: Arc::new(ProxyInputs { cert_manager: identity::mock::new_secret_manager(Duration::from_secs(10)), @@ -568,8 +568,13 @@ mod tests { connection_manager: ConnectionManager::default(), }), id: TraceParent::new(), - pool: pool::WorkloadHBONEPool::new(cfg.clone(), sock_fact, cert_mgr.clone()), - enable_orig_src: cfg.enable_original_source.unwrap_or_default(), + pool: pool::WorkloadHBONEPool::new( + cfg.clone(), + original_src, + sock_fact, + cert_mgr.clone(), + ), + enable_orig_src: cfg.require_original_source.unwrap_or_default(), hbone_port: cfg.inbound_addr.port(), }; diff --git a/src/proxy/pool.rs b/src/proxy/pool.rs index e12dfef67..b00afbb5a 100644 --- a/src/proxy/pool.rs +++ b/src/proxy/pool.rs @@ -76,6 +76,7 @@ struct PoolState { struct ConnSpawner { cfg: Arc, + original_source: bool, socket_factory: Arc, cert_manager: Arc, timeout_rx: watch::Receiver, @@ -86,15 +87,13 @@ impl ConnSpawner { async fn new_pool_conn(&self, key: WorkloadKey) -> Result { debug!("spawning new pool conn for {}", key); - let local = self - .cfg - .enable_original_source - .unwrap_or_default() - .then_some(key.src); + let local = self.original_source.then_some(key.src); let cert = self.cert_manager.fetch_certificate(&key.src_id).await?; let connector = cert.outbound_connector(key.dst_id.clone())?; let tcp_stream = super::freebind_connect(local, key.dst, self.socket_factory.as_ref()).await?; + + tracing::error!("freebind {:?} {}", local, tcp_stream.local_addr().unwrap()); let tls_stream = connector.connect(tcp_stream).await?; trace!("connector connected, handshaking"); let sender = @@ -337,6 +336,7 @@ impl WorkloadHBONEPool { // Callers should then be safe to drop() the pool instance. pub fn new( cfg: Arc, + original_source: bool, socket_factory: Arc, cert_manager: Arc, ) -> WorkloadHBONEPool { @@ -346,6 +346,7 @@ impl WorkloadHBONEPool { let spawner = ConnSpawner { cfg, + original_source, socket_factory, cert_manager, timeout_rx: timeout_recv.clone(), @@ -992,7 +993,8 @@ mod test { }; let sock_fact = Arc::new(crate::proxy::DefaultSocketFactory); let cert_mgr = identity::mock::new_secret_manager(Duration::from_secs(10)); - let pool = WorkloadHBONEPool::new(Arc::new(cfg), sock_fact, cert_mgr); + let original_src = false; // for testing, not needed + let pool = WorkloadHBONEPool::new(Arc::new(cfg), original_src, sock_fact, cert_mgr); let server = TestServer { conn_counter, drop_rx, diff --git a/src/proxy/socks5.rs b/src/proxy/socks5.rs index 1348d8e6a..803d00ec8 100644 --- a/src/proxy/socks5.rs +++ b/src/proxy/socks5.rs @@ -32,6 +32,7 @@ pub(super) struct Socks5 { pi: Arc, listener: socket::Listener, drain: Watch, + enable_orig_src: bool, } impl Socks5 { @@ -41,9 +42,12 @@ impl Socks5 { .tcp_bind(pi.cfg.socks5_addr.unwrap()) .map_err(|e| Error::Bind(pi.cfg.socks5_addr.unwrap(), e))?; + let enable_orig_src = super::maybe_set_transparent(&pi, &listener)?; + info!( address=%listener.local_addr(), component="socks5", + transparent=enable_orig_src, "listener established", ); @@ -51,6 +55,7 @@ impl Socks5 { pi, listener, drain, + enable_orig_src, }) } @@ -70,6 +75,7 @@ impl Socks5 { // but ProxyInfo is overloaded and only `outbound` should ever use the pool. let pool = crate::proxy::pool::WorkloadHBONEPool::new( self.pi.cfg.clone(), + self.enable_orig_src, self.pi.socket_factory.clone(), self.pi.cert_manager.clone(), ); @@ -80,7 +86,7 @@ impl Socks5 { pi: self.pi.clone(), id: TraceParent::new(), pool, - enable_orig_src: self.pi.cfg.enable_original_source.unwrap_or_default(), + enable_orig_src: self.enable_orig_src, hbone_port: self.pi.cfg.inbound_addr.port(), }; tokio::spawn(async move {