From cbd3a32d4d4275338c851dd158e0cb950d64ee91 Mon Sep 17 00:00:00 2001 From: Conrad Ludgate Date: Fri, 9 Feb 2024 19:22:23 +0000 Subject: [PATCH] proxy: decode username and password (#6700) ## Problem usernames and passwords can be URL 'percent' encoded in the connection string URL provided by serverless driver. ## Summary of changes Decode the parameters when getting conn info --- Cargo.lock | 2 ++ Cargo.toml | 1 + proxy/Cargo.toml | 4 +++- proxy/src/serverless/backend.rs | 2 +- proxy/src/serverless/conn_pool.rs | 7 ++++--- proxy/src/serverless/sql_over_http.rs | 10 ++++++++-- test_runner/fixtures/neon_fixtures.py | 6 +++--- test_runner/regress/test_proxy.py | 12 ++++++++++++ 8 files changed, 34 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a2939e6c75..83afdaf66f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4125,6 +4125,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "smallvec", "smol_str", "socket2 0.5.5", "sync_wrapper", @@ -4143,6 +4144,7 @@ dependencies = [ "tracing-subscriber", "tracing-utils", "url", + "urlencoding", "utils", "uuid", "walkdir", diff --git a/Cargo.toml b/Cargo.toml index 6a2c3fa563..ebc3dfa7b1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -171,6 +171,7 @@ tracing-opentelemetry = "0.20.0" tracing-subscriber = { version = "0.3", default_features = false, features = ["smallvec", "fmt", "tracing-log", "std", "env-filter", "json"] } twox-hash = { version = "1.6.3", default-features = false } url = "2.2" +urlencoding = "2.1" uuid = { version = "1.6.1", features = ["v4", "v7", "serde"] } walkdir = "2.3.2" webpki-roots = "0.25" diff --git a/proxy/Cargo.toml b/proxy/Cargo.toml index 83cab381b3..0777d361d2 100644 --- a/proxy/Cargo.toml +++ b/proxy/Cargo.toml @@ -60,6 +60,8 @@ scopeguard.workspace = true serde.workspace = true serde_json.workspace = true sha2.workspace = true +smol_str.workspace = true +smallvec.workspace = true socket2.workspace = true sync_wrapper.workspace = true task-local-extensions.workspace = true @@ -76,6 +78,7 @@ tracing-subscriber.workspace = true tracing-utils.workspace = true tracing.workspace = true url.workspace = true +urlencoding.workspace = true utils.workspace = true uuid.workspace = true webpki-roots.workspace = true @@ -84,7 +87,6 @@ native-tls.workspace = true postgres-native-tls.workspace = true postgres-protocol.workspace = true redis.workspace = true -smol_str.workspace = true workspace_hack.workspace = true diff --git a/proxy/src/serverless/backend.rs b/proxy/src/serverless/backend.rs index 03257e9161..8285da68d7 100644 --- a/proxy/src/serverless/backend.rs +++ b/proxy/src/serverless/backend.rs @@ -48,7 +48,7 @@ impl PoolingBackend { } }; let auth_outcome = - crate::auth::validate_password_and_exchange(conn_info.password.as_bytes(), secret)?; + crate::auth::validate_password_and_exchange(&conn_info.password, secret)?; match auth_outcome { crate::sasl::Outcome::Success(key) => Ok(key), crate::sasl::Outcome::Failure(reason) => { diff --git a/proxy/src/serverless/conn_pool.rs b/proxy/src/serverless/conn_pool.rs index f92793096b..f4e5b145c5 100644 --- a/proxy/src/serverless/conn_pool.rs +++ b/proxy/src/serverless/conn_pool.rs @@ -3,6 +3,7 @@ use futures::{future::poll_fn, Future}; use metrics::IntCounterPairGuard; use parking_lot::RwLock; use rand::Rng; +use smallvec::SmallVec; use smol_str::SmolStr; use std::{collections::HashMap, pin::pin, sync::Arc, sync::Weak, time::Duration}; use std::{ @@ -36,7 +37,7 @@ pub const APP_NAME: SmolStr = SmolStr::new_inline("/sql_over_http"); pub struct ConnInfo { pub user_info: ComputeUserInfo, pub dbname: DbName, - pub password: SmolStr, + pub password: SmallVec<[u8; 16]>, } impl ConnInfo { @@ -731,7 +732,7 @@ mod tests { options: Default::default(), }, dbname: "dbname".into(), - password: "password".into(), + password: "password".as_bytes().into(), }; let ep_pool = Arc::downgrade(&pool.get_or_create_endpoint_pool(&conn_info.endpoint_cache_key())); @@ -788,7 +789,7 @@ mod tests { options: Default::default(), }, dbname: "dbname".into(), - password: "password".into(), + password: "password".as_bytes().into(), }; let ep_pool = Arc::downgrade(&pool.get_or_create_endpoint_pool(&conn_info.endpoint_cache_key())); diff --git a/proxy/src/serverless/sql_over_http.rs b/proxy/src/serverless/sql_over_http.rs index 401022347e..54424360c4 100644 --- a/proxy/src/serverless/sql_over_http.rs +++ b/proxy/src/serverless/sql_over_http.rs @@ -100,6 +100,8 @@ pub enum ConnInfoError { InvalidDbName, #[error("missing username")] MissingUsername, + #[error("invalid username: {0}")] + InvalidUsername(#[from] std::string::FromUtf8Error), #[error("missing password")] MissingPassword, #[error("missing hostname")] @@ -134,7 +136,7 @@ fn get_conn_info( let dbname = url_path.next().ok_or(ConnInfoError::InvalidDbName)?; - let username = RoleName::from(connection_url.username()); + let username = RoleName::from(urlencoding::decode(connection_url.username())?); if username.is_empty() { return Err(ConnInfoError::MissingUsername); } @@ -143,6 +145,7 @@ fn get_conn_info( let password = connection_url .password() .ok_or(ConnInfoError::MissingPassword)?; + let password = urlencoding::decode_binary(password.as_bytes()); let hostname = connection_url .host_str() @@ -172,7 +175,10 @@ fn get_conn_info( Ok(ConnInfo { user_info, dbname: dbname.into(), - password: password.into(), + password: match password { + std::borrow::Cow::Borrowed(b) => b.into(), + std::borrow::Cow::Owned(b) => b.into(), + }, }) } diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 9996853525..231eebff52 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -23,7 +23,7 @@ from itertools import chain, product from pathlib import Path from types import TracebackType from typing import Any, Callable, Dict, Iterator, List, Optional, Tuple, Type, Union, cast -from urllib.parse import urlparse +from urllib.parse import quote, urlparse import asyncpg import backoff @@ -2822,8 +2822,8 @@ class NeonProxy(PgProtocol): def http_query(self, query, args, **kwargs): # TODO maybe use default values if not provided - user = kwargs["user"] - password = kwargs["password"] + user = quote(kwargs["user"]) + password = quote(kwargs["password"]) expected_code = kwargs.get("expected_code") connstr = f"postgresql://{user}:{password}@{self.domain}:{self.proxy_port}/postgres" diff --git a/test_runner/regress/test_proxy.py b/test_runner/regress/test_proxy.py index 49a0450f0c..884643cef0 100644 --- a/test_runner/regress/test_proxy.py +++ b/test_runner/regress/test_proxy.py @@ -462,6 +462,18 @@ def test_sql_over_http_pool(static_proxy: NeonProxy): assert "password authentication failed for user" in res["message"] +def test_sql_over_http_urlencoding(static_proxy: NeonProxy): + static_proxy.safe_psql("create user \"http+auth$$\" with password '%+$^&*@!' superuser") + + static_proxy.http_query( + "select 1", + [], + user="http+auth$$", + password="%+$^&*@!", + expected_code=200, + ) + + # Beginning a transaction should not impact the next query, # which might come from a completely different client. def test_http_pool_begin(static_proxy: NeonProxy):