From 3569c1bacdaea7b357f927cef32cd657e82af941 Mon Sep 17 00:00:00 2001 From: Dmitry Ivanov Date: Tue, 14 Feb 2023 16:31:05 +0300 Subject: [PATCH] [proxy] Fix: don't cache user & dbname in node info cache Upstream proxy erroneously stores user & dbname in compute node info cache entries, thus causing "funny" connection problems if such an entry is reused while connecting to e.g. a different DB on the same compute node. This PR fixes the problem but doesn't eliminate the root cause just yet. I'll revisit this code and make it more type-safe in the upcoming PR. --- proxy/src/auth/backend/link.rs | 2 ++ proxy/src/auth/credentials.rs | 37 ++++++++++-------------------- proxy/src/compute.rs | 12 ++++++++++ proxy/src/console/provider/mock.rs | 13 ++++------- proxy/src/console/provider/neon.rs | 9 ++++---- 5 files changed, 34 insertions(+), 39 deletions(-) diff --git a/proxy/src/auth/backend/link.rs b/proxy/src/auth/backend/link.rs index ef92b1a444..5d0049c957 100644 --- a/proxy/src/auth/backend/link.rs +++ b/proxy/src/auth/backend/link.rs @@ -78,6 +78,8 @@ pub(super) async fn handle_user( client.write_message_noflush(&Be::NoticeResponse("Connecting to database."))?; + // This config should be self-contained, because we won't + // take username or dbname from client's startup message. let mut config = compute::ConnCfg::new(); config .host(&db_info.host) diff --git a/proxy/src/auth/credentials.rs b/proxy/src/auth/credentials.rs index 66ca8be73e..968104f058 100644 --- a/proxy/src/auth/credentials.rs +++ b/proxy/src/auth/credentials.rs @@ -32,7 +32,6 @@ impl UserFacingError for ClientCredsParseError {} #[derive(Debug, Clone, PartialEq, Eq)] pub struct ClientCredentials<'a> { pub user: &'a str, - pub dbname: &'a str, // TODO: this is a severe misnomer! We should think of a new name ASAP. pub project: Option>, /// If `True`, we'll use the old cleartext password flow. This is used for @@ -59,7 +58,6 @@ impl<'a> ClientCredentials<'a> { // Some parameters are stored in the startup message. let get_param = |key| params.get(key).ok_or(MissingKey(key)); let user = get_param("user")?; - let dbname = get_param("database")?; // Project name might be passed via PG's command-line options. let project_option = params.options_raw().and_then(|mut options| { @@ -100,7 +98,6 @@ impl<'a> ClientCredentials<'a> { info!( user = user, - dbname = dbname, project = project.as_deref(), use_cleartext_password_flow = use_cleartext_password_flow, "credentials" @@ -108,7 +105,6 @@ impl<'a> ClientCredentials<'a> { Ok(Self { user, - dbname, project, use_cleartext_password_flow, }) @@ -131,25 +127,27 @@ mod tests { use ClientCredsParseError::*; #[test] - #[ignore = "TODO: fix how database is handled"] fn parse_bare_minimum() -> anyhow::Result<()> { // According to postgresql, only `user` should be required. let options = StartupMessageParams::new([("user", "john_doe")]); - // TODO: check that `creds.dbname` is None. let creds = ClientCredentials::parse(&options, None, None, false)?; assert_eq!(creds.user, "john_doe"); + assert_eq!(creds.project, None); Ok(()) } #[test] - fn parse_missing_project() -> anyhow::Result<()> { - let options = StartupMessageParams::new([("user", "john_doe"), ("database", "world")]); + fn parse_excessive() -> anyhow::Result<()> { + let options = StartupMessageParams::new([ + ("user", "john_doe"), + ("database", "world"), // should be ignored + ("foo", "bar"), // should be ignored + ]); let creds = ClientCredentials::parse(&options, None, None, false)?; assert_eq!(creds.user, "john_doe"); - assert_eq!(creds.dbname, "world"); assert_eq!(creds.project, None); Ok(()) @@ -157,14 +155,13 @@ mod tests { #[test] fn parse_project_from_sni() -> anyhow::Result<()> { - let options = StartupMessageParams::new([("user", "john_doe"), ("database", "world")]); + let options = StartupMessageParams::new([("user", "john_doe")]); let sni = Some("foo.localhost"); let common_name = Some("localhost"); let creds = ClientCredentials::parse(&options, sni, common_name, false)?; assert_eq!(creds.user, "john_doe"); - assert_eq!(creds.dbname, "world"); assert_eq!(creds.project.as_deref(), Some("foo")); Ok(()) @@ -174,13 +171,11 @@ mod tests { fn parse_project_from_options() -> anyhow::Result<()> { let options = StartupMessageParams::new([ ("user", "john_doe"), - ("database", "world"), ("options", "-ckey=1 project=bar -c geqo=off"), ]); let creds = ClientCredentials::parse(&options, None, None, false)?; assert_eq!(creds.user, "john_doe"); - assert_eq!(creds.dbname, "world"); assert_eq!(creds.project.as_deref(), Some("bar")); Ok(()) @@ -188,18 +183,13 @@ mod tests { #[test] fn parse_projects_identical() -> anyhow::Result<()> { - let options = StartupMessageParams::new([ - ("user", "john_doe"), - ("database", "world"), - ("options", "project=baz"), - ]); + let options = StartupMessageParams::new([("user", "john_doe"), ("options", "project=baz")]); let sni = Some("baz.localhost"); let common_name = Some("localhost"); let creds = ClientCredentials::parse(&options, sni, common_name, false)?; assert_eq!(creds.user, "john_doe"); - assert_eq!(creds.dbname, "world"); assert_eq!(creds.project.as_deref(), Some("baz")); Ok(()) @@ -207,11 +197,8 @@ mod tests { #[test] fn parse_projects_different() { - let options = StartupMessageParams::new([ - ("user", "john_doe"), - ("database", "world"), - ("options", "project=first"), - ]); + let options = + StartupMessageParams::new([("user", "john_doe"), ("options", "project=first")]); let sni = Some("second.localhost"); let common_name = Some("localhost"); @@ -229,7 +216,7 @@ mod tests { #[test] fn parse_inconsistent_sni() { - let options = StartupMessageParams::new([("user", "john_doe"), ("database", "world")]); + let options = StartupMessageParams::new([("user", "john_doe")]); let sni = Some("project.localhost"); let common_name = Some("example.com"); diff --git a/proxy/src/compute.rs b/proxy/src/compute.rs index 0c0cbcde20..3f5eb3caff 100644 --- a/proxy/src/compute.rs +++ b/proxy/src/compute.rs @@ -65,6 +65,18 @@ impl ConnCfg { /// Apply startup message params to the connection config. pub fn set_startup_params(&mut self, params: &StartupMessageParams) { + // Only set `user` if it's not present in the config. + // Link auth flow takes username from the console's response. + if let (None, Some(user)) = (self.get_user(), params.get("user")) { + self.user(user); + } + + // Only set `dbname` if it's not present in the config. + // Link auth flow takes dbname from the console's response. + if let (None, Some(dbname)) = (self.get_dbname(), params.get("database")) { + self.dbname(dbname); + } + if let Some(options) = params.options_raw() { // We must drop all proxy-specific parameters. #[allow(unstable_name_collisions)] diff --git a/proxy/src/console/provider/mock.rs b/proxy/src/console/provider/mock.rs index 301c3be516..eaac9c06d9 100644 --- a/proxy/src/console/provider/mock.rs +++ b/proxy/src/console/provider/mock.rs @@ -82,16 +82,11 @@ impl Api { .await } - async fn do_wake_compute( - &self, - creds: &ClientCredentials<'_>, - ) -> Result { + async fn do_wake_compute(&self) -> Result { let mut config = compute::ConnCfg::new(); config .host(self.endpoint.host_str().unwrap_or("localhost")) - .port(self.endpoint.port().unwrap_or(5432)) - .dbname(creds.dbname) - .user(creds.user); + .port(self.endpoint.port().unwrap_or(5432)); let node = NodeInfo { config, @@ -117,9 +112,9 @@ impl super::Api for Api { async fn wake_compute( &self, _extra: &ConsoleReqExtra<'_>, - creds: &ClientCredentials<'_>, + _creds: &ClientCredentials<'_>, ) -> Result { - self.do_wake_compute(creds) + self.do_wake_compute() .map_ok(CachedNodeInfo::new_uncached) .await } diff --git a/proxy/src/console/provider/neon.rs b/proxy/src/console/provider/neon.rs index 00d3ca8352..4eca025d2d 100644 --- a/proxy/src/console/provider/neon.rs +++ b/proxy/src/console/provider/neon.rs @@ -97,12 +97,11 @@ impl Api { Some(x) => x, }; + // Don't set anything but host and port! This config will be cached. + // We'll set username and such later using the startup message. + // TODO: add more type safety (in progress). let mut config = compute::ConnCfg::new(); - config - .host(host) - .port(port) - .dbname(creds.dbname) - .user(creds.user); + config.host(host).port(port); let node = NodeInfo { config,