From 278ba8f8b5abea8edeed8daa5ee77416838a829d Mon Sep 17 00:00:00 2001 From: Conrad Ludgate Date: Fri, 19 Apr 2024 13:55:51 +0100 Subject: [PATCH] proxy: simplify compute ssl setup --- Cargo.lock | 14 +--------- proxy/Cargo.toml | 5 ++-- proxy/src/auth/backend/link.rs | 1 - proxy/src/bin/proxy.rs | 7 ----- proxy/src/compute.rs | 42 +++++++++++++++++++++--------- proxy/src/config.rs | 1 - proxy/src/console/provider.rs | 13 +-------- proxy/src/console/provider/mock.rs | 1 - proxy/src/console/provider/neon.rs | 1 - proxy/src/proxy.rs | 8 ------ proxy/src/proxy/connect_compute.rs | 3 --- proxy/src/proxy/tests.rs | 15 +++++------ proxy/src/serverless/backend.rs | 1 - 13 files changed, 41 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6faf4b72f0..d961071872 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4009,17 +4009,6 @@ dependencies = [ "tokio-postgres", ] -[[package]] -name = "postgres-native-tls" -version = "0.5.0" -source = "git+https://github.com/neondatabase/rust-postgres.git?branch=neon#20031d7a9ee1addeae6e0968e3899ae6bf01cee2" -dependencies = [ - "native-tls", - "tokio", - "tokio-native-tls", - "tokio-postgres", -] - [[package]] name = "postgres-protocol" version = "0.6.4" @@ -4324,7 +4313,6 @@ dependencies = [ "md5", "measured", "metrics", - "native-tls", "once_cell", "opentelemetry", "parking_lot 0.12.1", @@ -4332,7 +4320,6 @@ dependencies = [ "parquet_derive", "pbkdf2", "pin-project-lite", - "postgres-native-tls", "postgres-protocol", "postgres_backend", "pq_proto", @@ -4351,6 +4338,7 @@ dependencies = [ "rstest", "rustc-hash", "rustls 0.22.2", + "rustls-native-certs 0.7.0", "rustls-pemfile 2.1.1", "scopeguard", "serde", diff --git a/proxy/Cargo.toml b/proxy/Cargo.toml index 6b8f2ecbf4..1ad014878a 100644 --- a/proxy/Cargo.toml +++ b/proxy/Cargo.toml @@ -67,6 +67,7 @@ routerify.workspace = true rustc-hash.workspace = true rustls-pemfile.workspace = true rustls.workspace = true +rustls-native-certs = "0.7.0" scopeguard.workspace = true serde.workspace = true serde_json.workspace = true @@ -81,6 +82,7 @@ thiserror.workspace = true tikv-jemallocator.workspace = true tikv-jemalloc-ctl = { workspace = true, features = ["use_std"] } tokio-postgres.workspace = true +tokio-postgres-rustls.workspace = true tokio-rustls.workspace = true tokio-util.workspace = true tokio = { workspace = true, features = ["signal"] } @@ -94,8 +96,6 @@ utils.workspace = true uuid.workspace = true webpki-roots.workspace = true x509-parser.workspace = true -native-tls.workspace = true -postgres-native-tls.workspace = true postgres-protocol.workspace = true redis.workspace = true @@ -106,6 +106,5 @@ camino-tempfile.workspace = true fallible-iterator.workspace = true rcgen.workspace = true rstest.workspace = true -tokio-postgres-rustls.workspace = true walkdir.workspace = true rand_distr = "0.4" diff --git a/proxy/src/auth/backend/link.rs b/proxy/src/auth/backend/link.rs index 415a4b7d85..06c0cc051e 100644 --- a/proxy/src/auth/backend/link.rs +++ b/proxy/src/auth/backend/link.rs @@ -121,6 +121,5 @@ pub(super) async fn authenticate( Ok(NodeInfo { config, aux: db_info.aux, - allow_self_signed_compute: false, // caller may override }) } diff --git a/proxy/src/bin/proxy.rs b/proxy/src/bin/proxy.rs index b54f8c131c..0d86339f64 100644 --- a/proxy/src/bin/proxy.rs +++ b/proxy/src/bin/proxy.rs @@ -120,9 +120,6 @@ struct ProxyCliArgs { /// lock for `wake_compute` api method. example: "shards=32,permits=4,epoch=10m,timeout=1s". (use `permits=0` to disable). #[clap(long, default_value = config::WakeComputeLockOptions::DEFAULT_OPTIONS_WAKE_COMPUTE_LOCK)] wake_compute_lock: String, - /// Allow self-signed certificates for compute nodes (for testing) - #[clap(long, default_value_t = false, value_parser = clap::builder::BoolishValueParser::new(), action = clap::ArgAction::Set)] - allow_self_signed_compute: bool, #[clap(flatten)] sql_over_http: SqlOverHttpArgs, /// timeout for scram authentication protocol @@ -458,9 +455,6 @@ fn build_config(args: &ProxyCliArgs) -> anyhow::Result<&'static ProxyConfig> { _ => bail!("either both or neither tls-key and tls-cert must be specified"), }; - if args.allow_self_signed_compute { - warn!("allowing self-signed compute certificates"); - } let backup_metric_collection_config = config::MetricBackupCollectionConfig { interval: args.metric_backup_collection_interval, remote_storage_config: remote_storage_from_toml( @@ -575,7 +569,6 @@ fn build_config(args: &ProxyCliArgs) -> anyhow::Result<&'static ProxyConfig> { tls_config, auth_backend, metric_collection, - allow_self_signed_compute: args.allow_self_signed_compute, http_config, authentication_config, require_client_ip: args.require_client_ip, diff --git a/proxy/src/compute.rs b/proxy/src/compute.rs index 149a619316..663e9249f7 100644 --- a/proxy/src/compute.rs +++ b/proxy/src/compute.rs @@ -10,7 +10,12 @@ use crate::{ use futures::{FutureExt, TryFutureExt}; use itertools::Itertools; use pq_proto::StartupMessageParams; -use std::{io, net::SocketAddr, time::Duration}; +use std::{ + io, + net::SocketAddr, + sync::{Arc, OnceLock}, + time::Duration, +}; use thiserror::Error; use tokio::net::TcpStream; use tokio_postgres::tls::MakeTlsConnect; @@ -28,9 +33,6 @@ pub enum ConnectionError { #[error("{COULD_NOT_CONNECT}: {0}")] CouldNotConnect(#[from] io::Error), - #[error("{COULD_NOT_CONNECT}: {0}")] - TlsError(#[from] native_tls::Error), - #[error("{COULD_NOT_CONNECT}: {0}")] WakeComputeError(#[from] WakeComputeError), } @@ -69,7 +71,6 @@ impl ReportableError for ConnectionError { } ConnectionError::Postgres(_) => crate::error::ErrorKind::Compute, ConnectionError::CouldNotConnect(_) => crate::error::ErrorKind::Compute, - ConnectionError::TlsError(_) => crate::error::ErrorKind::Compute, ConnectionError::WakeComputeError(e) => e.get_error_kind(), } } @@ -239,7 +240,7 @@ pub struct PostgresConnection { /// Socket connected to a compute node. pub stream: tokio_postgres::maybe_tls_stream::MaybeTlsStream< tokio::net::TcpStream, - postgres_native_tls::TlsStream, + tokio_postgres_rustls::RustlsStream, >, /// PostgreSQL connection parameters. pub params: std::collections::HashMap, @@ -251,22 +252,39 @@ pub struct PostgresConnection { _guage: NumDbConnectionsGuard<'static>, } +static ROOT_STORE: OnceLock> = OnceLock::new(); + impl ConnCfg { /// Connect to a corresponding compute node. pub async fn connect( &self, ctx: &mut RequestMonitoring, - allow_self_signed_compute: bool, aux: MetricsAuxInfo, timeout: Duration, ) -> Result { let (socket_addr, stream, host) = self.connect_raw(timeout).await?; - let tls_connector = native_tls::TlsConnector::builder() - .danger_accept_invalid_certs(allow_self_signed_compute) - .build() - .unwrap(); - let mut mk_tls = postgres_native_tls::MakeTlsConnector::new(tls_connector); + let root_store = ROOT_STORE.get_or_init(|| { + let mut roots = rustls::RootCertStore::empty(); + + let certs = match rustls_native_certs::load_native_certs() { + Ok(certs) => certs, + Err(e) => { + error!("could not load native ssl certs: {e:?}"); + return Arc::new(roots); + } + }; + + let (added, ignored) = roots.add_parsable_certificates(certs); + info!(added, ignored, "loaded native ssl certifications"); + + Arc::new(roots) + }); + + let client_config = rustls::ClientConfig::builder() + .with_root_certificates(root_store.clone()) + .with_no_client_auth(); + let mut mk_tls = tokio_postgres_rustls::MakeRustlsConnect::new(client_config); let tls = MakeTlsConnect::::make_tls_connect(&mut mk_tls, host)?; // connect_raw() will not use TLS if sslmode is "disable" diff --git a/proxy/src/config.rs b/proxy/src/config.rs index f9519c7645..01a72c2789 100644 --- a/proxy/src/config.rs +++ b/proxy/src/config.rs @@ -24,7 +24,6 @@ pub struct ProxyConfig { pub tls_config: Option, pub auth_backend: auth::BackendType<'static, (), ()>, pub metric_collection: Option, - pub allow_self_signed_compute: bool, pub http_config: HttpConfig, pub authentication_config: AuthenticationConfig, pub require_client_ip: bool, diff --git a/proxy/src/console/provider.rs b/proxy/src/console/provider.rs index aa1800a9da..64f9c748f6 100644 --- a/proxy/src/console/provider.rs +++ b/proxy/src/console/provider.rs @@ -293,9 +293,6 @@ pub struct NodeInfo { /// Labels for proxy's metrics. pub aux: MetricsAuxInfo, - - /// Whether we should accept self-signed certificates (for testing) - pub allow_self_signed_compute: bool, } impl NodeInfo { @@ -304,17 +301,9 @@ impl NodeInfo { ctx: &mut RequestMonitoring, timeout: Duration, ) -> Result { - self.config - .connect( - ctx, - self.allow_self_signed_compute, - self.aux.clone(), - timeout, - ) - .await + self.config.connect(ctx, self.aux.clone(), timeout).await } pub fn reuse_settings(&mut self, other: Self) { - self.allow_self_signed_compute = other.allow_self_signed_compute; self.config.reuse_password(other.config); } diff --git a/proxy/src/console/provider/mock.rs b/proxy/src/console/provider/mock.rs index cfe491f2aa..60d488c973 100644 --- a/proxy/src/console/provider/mock.rs +++ b/proxy/src/console/provider/mock.rs @@ -126,7 +126,6 @@ impl Api { branch_id: (&BranchId::from("branch")).into(), cold_start_info: crate::console::messages::ColdStartInfo::Warm, }, - allow_self_signed_compute: false, }; Ok(node) diff --git a/proxy/src/console/provider/neon.rs b/proxy/src/console/provider/neon.rs index 58b2a1570c..f063b6b01a 100644 --- a/proxy/src/console/provider/neon.rs +++ b/proxy/src/console/provider/neon.rs @@ -175,7 +175,6 @@ impl Api { let node = NodeInfo { config, aux: body.aux, - allow_self_signed_compute: false, }; Ok(node) diff --git a/proxy/src/proxy.rs b/proxy/src/proxy.rs index 4321bad968..2f88c5db5d 100644 --- a/proxy/src/proxy.rs +++ b/proxy/src/proxy.rs @@ -178,13 +178,6 @@ impl ClientMode { } } - pub fn allow_self_signed_compute(&self, config: &ProxyConfig) -> bool { - match self { - ClientMode::Tcp => config.allow_self_signed_compute, - ClientMode::Websockets { .. } => false, - } - } - fn hostname<'a, S>(&'a self, s: &'a Stream) -> Option<&'a str> { match self { ClientMode::Tcp => s.sni_hostname(), @@ -307,7 +300,6 @@ pub async fn handle_client( ctx, &TcpMechanism { params: ¶ms }, &user_info, - mode.allow_self_signed_compute(config), ) .or_else(|e| stream.throw_error(e)) .await?; diff --git a/proxy/src/proxy/connect_compute.rs b/proxy/src/proxy/connect_compute.rs index 33f394c550..0c2db8846a 100644 --- a/proxy/src/proxy/connect_compute.rs +++ b/proxy/src/proxy/connect_compute.rs @@ -92,7 +92,6 @@ pub async fn connect_to_compute( ctx: &mut RequestMonitoring, mechanism: &M, user_info: &B, - allow_self_signed_compute: bool, ) -> Result where M::ConnectError: ShouldRetry + std::fmt::Debug, @@ -103,8 +102,6 @@ where if let Some(keys) = user_info.get_keys() { node_info.set_keys(keys); } - node_info.allow_self_signed_compute = allow_self_signed_compute; - // let mut node_info = credentials.get_node_info(ctx, user_info).await?; mechanism.update_connect_config(&mut node_info.config); // try once diff --git a/proxy/src/proxy/tests.rs b/proxy/src/proxy/tests.rs index 849e9bd33c..8e21588211 100644 --- a/proxy/src/proxy/tests.rs +++ b/proxy/src/proxy/tests.rs @@ -519,7 +519,6 @@ fn helper_create_cached_node_info(cache: &'static NodeInfoCache) -> CachedNodeIn branch_id: (&BranchId::from("branch")).into(), cold_start_info: crate::console::messages::ColdStartInfo::Warm, }, - allow_self_signed_compute: false, }; let (_, node) = cache.insert("key".into(), node); node @@ -549,7 +548,7 @@ async fn connect_to_compute_success() { let mut ctx = RequestMonitoring::test(); let mechanism = TestConnectMechanism::new(vec![Wake, Connect]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap(); mechanism.verify(); @@ -562,7 +561,7 @@ async fn connect_to_compute_retry() { let mut ctx = RequestMonitoring::test(); let mechanism = TestConnectMechanism::new(vec![Wake, Retry, Wake, Connect]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap(); mechanism.verify(); @@ -576,7 +575,7 @@ async fn connect_to_compute_non_retry_1() { let mut ctx = RequestMonitoring::test(); let mechanism = TestConnectMechanism::new(vec![Wake, Retry, Wake, Fail]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap_err(); mechanism.verify(); @@ -590,7 +589,7 @@ async fn connect_to_compute_non_retry_2() { let mut ctx = RequestMonitoring::test(); let mechanism = TestConnectMechanism::new(vec![Wake, Fail, Wake, Connect]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap(); mechanism.verify(); @@ -608,7 +607,7 @@ async fn connect_to_compute_non_retry_3() { Retry, Retry, Retry, Retry, Retry, /* the 17th time */ Retry, ]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap_err(); mechanism.verify(); @@ -622,7 +621,7 @@ async fn wake_retry() { let mut ctx = RequestMonitoring::test(); let mechanism = TestConnectMechanism::new(vec![WakeRetry, Wake, Connect]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap(); mechanism.verify(); @@ -636,7 +635,7 @@ async fn wake_non_retry() { let mut ctx = RequestMonitoring::test(); let mechanism = TestConnectMechanism::new(vec![WakeRetry, WakeFail]); let user_info = helper_create_connect_info(&mechanism); - connect_to_compute(&mut ctx, &mechanism, &user_info, false) + connect_to_compute(&mut ctx, &mechanism, &user_info) .await .unwrap_err(); mechanism.verify(); diff --git a/proxy/src/serverless/backend.rs b/proxy/src/serverless/backend.rs index e74c63599a..d40e1a4830 100644 --- a/proxy/src/serverless/backend.rs +++ b/proxy/src/serverless/backend.rs @@ -107,7 +107,6 @@ impl PoolingBackend { pool: self.pool.clone(), }, &backend, - false, // do not allow self signed compute for http flow ) .await }