From d63602cc7822eb5f9670d7e7926bb412eba1ff3a Mon Sep 17 00:00:00 2001 From: Conrad Ludgate Date: Wed, 18 Dec 2024 16:03:14 +0000 Subject: [PATCH] chore(proxy): fully remove allow-self-signed-compute flag (#10168) When https://github.com/neondatabase/cloud/pull/21856 is merged, this flag is no longer necessary. --- proxy/src/bin/local_proxy.rs | 1 - proxy/src/bin/proxy.rs | 7 --- proxy/src/cancellation.rs | 44 +++++++---------- proxy/src/compute.rs | 69 +++------------------------ proxy/src/config.rs | 1 - proxy/src/console_redirect_proxy.rs | 1 - proxy/src/control_plane/mod.rs | 5 +- proxy/src/proxy/connect_compute.rs | 9 +--- proxy/src/proxy/mod.rs | 2 - test_runner/fixtures/neon_fixtures.py | 1 - 10 files changed, 25 insertions(+), 115 deletions(-) diff --git a/proxy/src/bin/local_proxy.rs b/proxy/src/bin/local_proxy.rs index 968682cf0f75..56bbd9485011 100644 --- a/proxy/src/bin/local_proxy.rs +++ b/proxy/src/bin/local_proxy.rs @@ -271,7 +271,6 @@ fn build_config(args: &LocalProxyCliArgs) -> anyhow::Result<&'static ProxyConfig Ok(Box::leak(Box::new(ProxyConfig { tls_config: None, metric_collection: None, - allow_self_signed_compute: false, http_config, authentication_config: AuthenticationConfig { jwks_cache: JwkCache::default(), diff --git a/proxy/src/bin/proxy.rs b/proxy/src/bin/proxy.rs index e90555e250b8..3dcf9ca060c0 100644 --- a/proxy/src/bin/proxy.rs +++ b/proxy/src/bin/proxy.rs @@ -129,9 +129,6 @@ struct ProxyCliArgs { /// lock for `connect_compute` api method. example: "shards=32,permits=4,epoch=10m,timeout=1s". (use `permits=0` to disable). #[clap(long, default_value = config::ConcurrencyLockOptions::DEFAULT_OPTIONS_CONNECT_COMPUTE_LOCK)] connect_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 @@ -564,9 +561,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: args.metric_backup_collection_remote_storage.clone(), @@ -641,7 +635,6 @@ fn build_config(args: &ProxyCliArgs) -> anyhow::Result<&'static ProxyConfig> { let config = ProxyConfig { tls_config, metric_collection, - allow_self_signed_compute: args.allow_self_signed_compute, http_config, authentication_config, proxy_protocol_v2: args.proxy_protocol_v2, diff --git a/proxy/src/cancellation.rs b/proxy/src/cancellation.rs index a58e3961da86..ebaea173ae48 100644 --- a/proxy/src/cancellation.rs +++ b/proxy/src/cancellation.rs @@ -4,7 +4,8 @@ use std::sync::Arc; use dashmap::DashMap; use ipnet::{IpNet, Ipv4Net, Ipv6Net}; use once_cell::sync::OnceCell; -use postgres_client::{tls::MakeTlsConnect, CancelToken}; +use postgres_client::tls::MakeTlsConnect; +use postgres_client::CancelToken; use pq_proto::CancelKeyData; use rustls::crypto::ring; use thiserror::Error; @@ -14,17 +15,16 @@ use tracing::{debug, info}; use uuid::Uuid; use crate::auth::{check_peer_addr_is_in_list, IpPattern}; +use crate::compute::load_certs; use crate::error::ReportableError; use crate::ext::LockExt; use crate::metrics::{CancellationRequest, CancellationSource, Metrics}; +use crate::postgres_rustls::MakeRustlsConnect; use crate::rate_limiter::LeakyBucketRateLimiter; use crate::redis::cancellation_publisher::{ CancellationPublisher, CancellationPublisherMut, RedisPublisherClient, }; -use crate::compute::{load_certs, AcceptEverythingVerifier}; -use crate::postgres_rustls::MakeRustlsConnect; - pub type CancelMap = Arc>>; pub type CancellationHandlerMain = CancellationHandler>>>; pub(crate) type CancellationHandlerMainInternal = Option>>; @@ -240,7 +240,6 @@ pub struct CancelClosure { cancel_token: CancelToken, ip_allowlist: Vec, hostname: String, // for pg_sni router - allow_self_signed_compute: bool, } impl CancelClosure { @@ -249,45 +248,34 @@ impl CancelClosure { cancel_token: CancelToken, ip_allowlist: Vec, hostname: String, - allow_self_signed_compute: bool, ) -> Self { Self { socket_addr, cancel_token, ip_allowlist, hostname, - allow_self_signed_compute, } } /// Cancels the query running on user's compute node. pub(crate) async fn try_cancel_query(self) -> Result<(), CancelError> { let socket = TcpStream::connect(self.socket_addr).await?; - let client_config = if self.allow_self_signed_compute { - // Allow all certificates for creating the connection. Used only for tests - let verifier = Arc::new(AcceptEverythingVerifier); - rustls::ClientConfig::builder_with_provider(Arc::new(ring::default_provider())) - .with_safe_default_protocol_versions() - .expect("ring should support the default protocol versions") - .dangerous() - .with_custom_certificate_verifier(verifier) - } else { - let root_store = TLS_ROOTS - .get_or_try_init(load_certs) - .map_err(|_e| { - CancelError::IO(std::io::Error::new( - std::io::ErrorKind::Other, - "TLS root store initialization failed".to_string(), - )) - })? - .clone(); + let root_store = TLS_ROOTS + .get_or_try_init(load_certs) + .map_err(|_e| { + CancelError::IO(std::io::Error::new( + std::io::ErrorKind::Other, + "TLS root store initialization failed".to_string(), + )) + })? + .clone(); + + let client_config = rustls::ClientConfig::builder_with_provider(Arc::new(ring::default_provider())) .with_safe_default_protocol_versions() .expect("ring should support the default protocol versions") .with_root_certificates(root_store) - }; - - let client_config = client_config.with_no_client_auth(); + .with_no_client_auth(); let mut mk_tls = crate::postgres_rustls::MakeRustlsConnect::new(client_config); let tls = >::make_tls_connect( diff --git a/proxy/src/compute.rs b/proxy/src/compute.rs index 42df5ff5e3e5..8dc9b59e81c5 100644 --- a/proxy/src/compute.rs +++ b/proxy/src/compute.rs @@ -10,7 +10,6 @@ use postgres_client::tls::MakeTlsConnect; use postgres_client::{CancelToken, RawConnection}; use postgres_protocol::message::backend::NoticeResponseBody; use pq_proto::StartupMessageParams; -use rustls::client::danger::ServerCertVerifier; use rustls::crypto::ring; use rustls::pki_types::InvalidDnsNameError; use thiserror::Error; @@ -251,7 +250,6 @@ impl ConnCfg { pub(crate) async fn connect( &self, ctx: &RequestContext, - allow_self_signed_compute: bool, aux: MetricsAuxInfo, timeout: Duration, ) -> Result { @@ -259,25 +257,17 @@ impl ConnCfg { let (socket_addr, stream, host) = self.connect_raw(timeout).await?; drop(pause); - let client_config = if allow_self_signed_compute { - // Allow all certificates for creating the connection - let verifier = Arc::new(AcceptEverythingVerifier); - rustls::ClientConfig::builder_with_provider(Arc::new(ring::default_provider())) - .with_safe_default_protocol_versions() - .expect("ring should support the default protocol versions") - .dangerous() - .with_custom_certificate_verifier(verifier) - } else { - let root_store = TLS_ROOTS - .get_or_try_init(load_certs) - .map_err(ConnectionError::TlsCertificateError)? - .clone(); + let root_store = TLS_ROOTS + .get_or_try_init(load_certs) + .map_err(ConnectionError::TlsCertificateError)? + .clone(); + + let client_config = rustls::ClientConfig::builder_with_provider(Arc::new(ring::default_provider())) .with_safe_default_protocol_versions() .expect("ring should support the default protocol versions") .with_root_certificates(root_store) - }; - let client_config = client_config.with_no_client_auth(); + .with_no_client_auth(); let mut mk_tls = crate::postgres_rustls::MakeRustlsConnect::new(client_config); let tls = >::make_tls_connect( @@ -320,7 +310,6 @@ impl ConnCfg { }, vec![], host.to_string(), - allow_self_signed_compute, ); let connection = PostgresConnection { @@ -365,50 +354,6 @@ pub(crate) fn load_certs() -> Result, Vec> = OnceCell::new(); -#[derive(Debug)] -pub(crate) struct AcceptEverythingVerifier; -impl ServerCertVerifier for AcceptEverythingVerifier { - fn supported_verify_schemes(&self) -> Vec { - use rustls::SignatureScheme; - // The schemes for which `SignatureScheme::supported_in_tls13` returns true. - vec![ - SignatureScheme::ECDSA_NISTP521_SHA512, - SignatureScheme::ECDSA_NISTP384_SHA384, - SignatureScheme::ECDSA_NISTP256_SHA256, - SignatureScheme::RSA_PSS_SHA512, - SignatureScheme::RSA_PSS_SHA384, - SignatureScheme::RSA_PSS_SHA256, - SignatureScheme::ED25519, - ] - } - fn verify_server_cert( - &self, - _end_entity: &rustls::pki_types::CertificateDer<'_>, - _intermediates: &[rustls::pki_types::CertificateDer<'_>], - _server_name: &rustls::pki_types::ServerName<'_>, - _ocsp_response: &[u8], - _now: rustls::pki_types::UnixTime, - ) -> Result { - Ok(rustls::client::danger::ServerCertVerified::assertion()) - } - fn verify_tls12_signature( - &self, - _message: &[u8], - _cert: &rustls::pki_types::CertificateDer<'_>, - _dss: &rustls::DigitallySignedStruct, - ) -> Result { - Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) - } - fn verify_tls13_signature( - &self, - _message: &[u8], - _cert: &rustls::pki_types::CertificateDer<'_>, - _dss: &rustls::DigitallySignedStruct, - ) -> Result { - Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/proxy/src/config.rs b/proxy/src/config.rs index debd77ac3296..33d1d2e9e4a0 100644 --- a/proxy/src/config.rs +++ b/proxy/src/config.rs @@ -25,7 +25,6 @@ use crate::types::Host; pub struct ProxyConfig { pub tls_config: Option, pub metric_collection: Option, - pub allow_self_signed_compute: bool, pub http_config: HttpConfig, pub authentication_config: AuthenticationConfig, pub proxy_protocol_v2: ProxyProtocolV2, diff --git a/proxy/src/console_redirect_proxy.rs b/proxy/src/console_redirect_proxy.rs index 02398fb7778c..c477822e853c 100644 --- a/proxy/src/console_redirect_proxy.rs +++ b/proxy/src/console_redirect_proxy.rs @@ -213,7 +213,6 @@ pub(crate) async fn handle_client( params_compat: true, params: ¶ms, locks: &config.connect_compute_locks, - allow_self_signed_compute: config.allow_self_signed_compute, }, &user_info, config.wake_compute_retry_config, diff --git a/proxy/src/control_plane/mod.rs b/proxy/src/control_plane/mod.rs index c0718920b493..0ca1a6aae0eb 100644 --- a/proxy/src/control_plane/mod.rs +++ b/proxy/src/control_plane/mod.rs @@ -73,12 +73,9 @@ impl NodeInfo { pub(crate) async fn connect( &self, ctx: &RequestContext, - allow_self_signed_compute: bool, timeout: Duration, ) -> Result { - self.config - .connect(ctx, allow_self_signed_compute, self.aux.clone(), timeout) - .await + self.config.connect(ctx, self.aux.clone(), timeout).await } pub(crate) fn reuse_settings(&mut self, other: Self) { diff --git a/proxy/src/proxy/connect_compute.rs b/proxy/src/proxy/connect_compute.rs index 6da4c90a535b..4a30d2398558 100644 --- a/proxy/src/proxy/connect_compute.rs +++ b/proxy/src/proxy/connect_compute.rs @@ -73,9 +73,6 @@ pub(crate) struct TcpMechanism<'a> { /// connect_to_compute concurrency lock pub(crate) locks: &'static ApiLocks, - - /// Whether we should accept self-signed certificates (for testing) - pub(crate) allow_self_signed_compute: bool, } #[async_trait] @@ -93,11 +90,7 @@ impl ConnectMechanism for TcpMechanism<'_> { ) -> Result { let host = node_info.config.get_host(); let permit = self.locks.get_permit(&host).await?; - permit.release_result( - node_info - .connect(ctx, self.allow_self_signed_compute, timeout) - .await, - ) + permit.release_result(node_info.connect(ctx, timeout).await) } fn update_connect_config(&self, config: &mut compute::ConnCfg) { diff --git a/proxy/src/proxy/mod.rs b/proxy/src/proxy/mod.rs index 4e5ecda237d5..dbe174cab7d5 100644 --- a/proxy/src/proxy/mod.rs +++ b/proxy/src/proxy/mod.rs @@ -348,8 +348,6 @@ pub(crate) async fn handle_client( params_compat, params: ¶ms, locks: &config.connect_compute_locks, - // only used for console redirect testing. - allow_self_signed_compute: false, }, &user_info, config.wake_compute_retry_config, diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 2553a0c99ab0..9f78ad120b91 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -3222,7 +3222,6 @@ def extra_args(self) -> list[str]: # Link auth backend params *["--auth-backend", "link"], *["--uri", NeonProxy.link_auth_uri], - *["--allow-self-signed-compute", "true"], ] class ProxyV1(AuthBackend):