From bcb8352be2bfc23e21800506499fc6cafd4ee22b Mon Sep 17 00:00:00 2001 From: Dmitry Simonenko Date: Fri, 30 Aug 2019 14:07:58 +0300 Subject: [PATCH] Unset libpq environment variables By default libpq uses environment variables as a fallback way to specify connection options, potentially they could be in a conflict with internal implementation and introduce security risks. Introduce new option `timescaledb.passfile` which specifies the name of the file used to store passwords used for a data node connection. It is intended to be used instead of PGPASSFILE variable, which is no longer accessible. --- src/guc.c | 13 ++++ src/guc.h | 1 + test/postgresql.conf.in | 1 + test/test-defs.cmake | 8 +-- tsl/src/remote/connection.c | 60 +++++++++++++++++-- tsl/test/expected/remote_connection_cache.out | 4 +- tsl/test/postgresql.conf.in | 1 + tsl/test/sql/remote_connection_cache.sql | 5 +- tsl/test/src/remote/connection.c | 3 +- tsl/test/src/remote/connection_cache.c | 2 - 10 files changed, 80 insertions(+), 18 deletions(-) diff --git a/src/guc.c b/src/guc.c index e79be39a2..c9bf0f3dc 100644 --- a/src/guc.c +++ b/src/guc.c @@ -59,6 +59,7 @@ TSDLLEXPORT int ts_guc_max_insert_batch_size = 1000; TSDLLEXPORT bool ts_guc_enable_connection_binary_data; TSDLLEXPORT bool ts_guc_enable_client_ddl_on_data_nodes = false; TSDLLEXPORT char *ts_guc_ssl_dir = NULL; +TSDLLEXPORT char *ts_guc_passfile = NULL; #ifdef TS_DEBUG bool ts_shutdown_bgw = false; @@ -272,6 +273,18 @@ _guc_init(void) NULL, NULL); + DefineCustomStringVariable("timescaledb.passfile", + "TimescaleDB password file path", + "Specifies the name of the file used to store passwords used for " + "data node connections", + &ts_guc_passfile, + NULL, + PGC_SIGHUP, + GUC_SUPERUSER_ONLY, + NULL, + NULL, + NULL); + DefineCustomIntVariable("timescaledb.max_open_chunks_per_insert", "Maximum open chunks per insert", "Maximum number of open chunk tables per insert", diff --git a/src/guc.h b/src/guc.h index b94685f56..343f0007d 100644 --- a/src/guc.h +++ b/src/guc.h @@ -35,6 +35,7 @@ extern TSDLLEXPORT int ts_guc_max_insert_batch_size; extern TSDLLEXPORT bool ts_guc_enable_connection_binary_data; extern TSDLLEXPORT bool ts_guc_enable_client_ddl_on_data_nodes; extern TSDLLEXPORT char *ts_guc_ssl_dir; +extern TSDLLEXPORT char *ts_guc_passfile; #ifdef TS_DEBUG extern bool ts_shutdown_bgw; diff --git a/test/postgresql.conf.in b/test/postgresql.conf.in index 1ee16d20d..3fbf2e3ac 100644 --- a/test/postgresql.conf.in +++ b/test/postgresql.conf.in @@ -15,4 +15,5 @@ log_line_prefix='%u [%p] %d ' # numbers. Setting extra_float_digits=0 retains the old behavior which # is needed to make our tests work for multiple PostgreSQL versions. extra_float_digits=0 +timescaledb.passfile='@TEST_PASSFILE@' hba_file='@TEST_PG_HBA_FILE@' diff --git a/test/test-defs.cmake b/test/test-defs.cmake index b55484273..453d6ee53 100644 --- a/test/test-defs.cmake +++ b/test/test-defs.cmake @@ -21,7 +21,7 @@ set(TEST_PGPORT_TEMP_INSTANCE 15432 CACHE STRING "The port to run a temporary te set(TEST_SCHEDULE ${CMAKE_CURRENT_BINARY_DIR}/test_schedule) set(TEST_SCHEDULE_SHARED ${CMAKE_CURRENT_BINARY_DIR}/shared/test_schedule_shared) set(ISOLATION_TEST_SCHEDULE ${CMAKE_CURRENT_BINARY_DIR}/isolation_test_schedule) -set(TEST_PGPASSFILE ${TEST_OUTPUT_DIR}/pgpass.conf) +set(TEST_PASSFILE ${TEST_OUTPUT_DIR}/pgpass.conf) # Add clustering users with enabled SSL for PG version >= 11. # @@ -109,8 +109,7 @@ if(PG_REGRESS) TEST_OUTPUT_DIR=${TEST_OUTPUT_DIR} TEST_SCHEDULE=${TEST_SCHEDULE} PG_BINDIR=${PG_BINDIR} - PG_REGRESS=${PG_REGRESS} - PGPASSFILE=${TEST_PGPASSFILE}) + PG_REGRESS=${PG_REGRESS}) endif() if(PG_ISOLATION_REGRESS) @@ -128,8 +127,7 @@ if(PG_ISOLATION_REGRESS) TEST_INPUT_DIR=${TEST_INPUT_DIR} TEST_OUTPUT_DIR=${TEST_OUTPUT_DIR} ISOLATION_TEST_SCHEDULE=${ISOLATION_TEST_SCHEDULE} - PG_ISOLATION_REGRESS=${PG_ISOLATION_REGRESS} - PGPASSFILE=${TEST_PGPASSFILE}) + PG_ISOLATION_REGRESS=${PG_ISOLATION_REGRESS}) endif() if (${PG_VERSION_MAJOR} GREATER "9") diff --git a/tsl/src/remote/connection.c b/tsl/src/remote/connection.c index 17b40c4e9..57a82165b 100644 --- a/tsl/src/remote/connection.c +++ b/tsl/src/remote/connection.c @@ -333,6 +333,30 @@ get_libpq_options() return libpq_options; } +static void +unset_libpq_envvar(void) +{ + PQconninfoOption *lopt; + PQconninfoOption *options = PQconndefaults(); + + Assert(options != NULL); + + /* Explicitly unset all libpq environment variables. + * + * By default libpq uses environment variables as a fallback + * to specify connection options, potentially they could be in + * a conflict with PostgreSQL variables and introduce + * security risks. + */ + for (lopt = options; lopt->keyword; lopt++) + { + if (lopt->envvar) + unsetenv(lopt->envvar); + } + + PQconninfoFree(options); +} + static bool is_libpq_option(const char *keyword, char **display_option) { @@ -819,9 +843,33 @@ remote_connection_set_peer_dist_id(TSConnection *conn) /* fallback_application_name, client_encoding, end marker */ #define REMOTE_CONNECTION_SESSION_OPTIONS_N 3 +/* passfile */ +#define REMOTE_CONNECTION_PASSWORD_OPTIONS_N 1 + /* sslmode, sslrootcert, sslcert, sslkey */ #define REMOTE_CONNECTION_SSL_OPTIONS_N 4 +/* default password file basename */ +#define DEFAULT_PASSFILE_NAME "passfile" + +static void +set_password_options(const char **keywords, const char **values, int *option_start) +{ + int option_pos = *option_start; + + /* Set user specified password file path using timescaledb.passfile or + * use default path assuming that the file is stored in the + * data directory */ + keywords[option_pos] = "passfile"; + if (ts_guc_passfile) + values[option_pos] = ts_guc_passfile; + else + values[option_pos] = psprintf("%s/" DEFAULT_PASSFILE_NAME, DataDir); + option_pos++; + + *option_start = option_pos; +} + static void set_ssl_options(const char *user_name, const char **keywords, const char **values, int *option_start) @@ -884,10 +932,10 @@ remote_connection_open_internal(const char *node_name, List *connection_options) * and user. (Some of them might not be libpq options, in * which case we'll just waste a few array slots.) Add 3 extra slots * for fallback_application_name, client_encoding, end marker. - * Add additional 4 slots for ssl options. + * One additional slot to set passfile and 4 slots for ssl options. */ option_count = list_length(connection_options) + REMOTE_CONNECTION_SESSION_OPTIONS_N + - REMOTE_CONNECTION_SSL_OPTIONS_N; + REMOTE_CONNECTION_PASSWORD_OPTIONS_N + REMOTE_CONNECTION_SSL_OPTIONS_N; keywords = (const char **) palloc(option_count * sizeof(char *)); values = (const char **) palloc(option_count * sizeof(char *)); @@ -903,6 +951,9 @@ remote_connection_open_internal(const char *node_name, List *connection_options) values[option_pos] = GetDatabaseEncodingName(); option_pos++; + /* Set passfile options */ + set_password_options(keywords, values, &option_pos); + /* Set client specific SSL connection options */ set_ssl_options(user_name, keywords, values, &option_pos); @@ -927,9 +978,6 @@ remote_connection_open_internal(const char *node_name, List *connection_options) return ts_conn; } -#undef REMOTE_CONNECTION_SSL_OPTIONS -#undef REMOTE_CONNECTION_ADDITIONAL_OPTIONS - /* * Opens a connection. * @@ -1389,6 +1437,8 @@ _remote_connection_init(void) { RegisterXactCallback(remote_connection_xact_end, NULL); RegisterSubXactCallback(remote_connection_subxact_end, NULL); + + unset_libpq_envvar(); } void diff --git a/tsl/test/expected/remote_connection_cache.out b/tsl/test/expected/remote_connection_cache.out index cd96b3cdc..46afd3baf 100644 --- a/tsl/test/expected/remote_connection_cache.out +++ b/tsl/test/expected/remote_connection_cache.out @@ -10,16 +10,16 @@ DO $d$ BEGIN EXECUTE $$CREATE SERVER loopback_1 FOREIGN DATA WRAPPER timescaledb_fdw OPTIONS (dbname '$$||current_database()||$$', + host 'localhost', port '$$||current_setting('port')||$$' )$$; EXECUTE $$CREATE SERVER loopback_2 FOREIGN DATA WRAPPER timescaledb_fdw OPTIONS (dbname '$$||current_database()||$$', + host 'localhost', port '$$||current_setting('port')||$$' )$$; END; $d$; -CREATE USER MAPPING FOR CURRENT_USER SERVER loopback_1; -CREATE USER MAPPING FOR CURRENT_USER SERVER loopback_2; SELECT _timescaledb_internal.test_remote_connection_cache(); test_remote_connection_cache ------------------------------ diff --git a/tsl/test/postgresql.conf.in b/tsl/test/postgresql.conf.in index d52087d79..96cc8d9df 100644 --- a/tsl/test/postgresql.conf.in +++ b/tsl/test/postgresql.conf.in @@ -24,3 +24,4 @@ ssl_ca_file='@TEST_OUTPUT_DIR@/ts_root.crt' ssl_cert_file='@TEST_OUTPUT_DIR@/ts_data_node.crt' ssl_key_file='@TEST_OUTPUT_DIR@/ts_data_node.key' timescaledb.ssl_dir='@TEST_OUTPUT_DIR@' +timescaledb.passfile='@TEST_PASSFILE@' diff --git a/tsl/test/sql/remote_connection_cache.sql b/tsl/test/sql/remote_connection_cache.sql index 87602d597..b02cffae7 100644 --- a/tsl/test/sql/remote_connection_cache.sql +++ b/tsl/test/sql/remote_connection_cache.sql @@ -13,16 +13,15 @@ DO $d$ BEGIN EXECUTE $$CREATE SERVER loopback_1 FOREIGN DATA WRAPPER timescaledb_fdw OPTIONS (dbname '$$||current_database()||$$', + host 'localhost', port '$$||current_setting('port')||$$' )$$; EXECUTE $$CREATE SERVER loopback_2 FOREIGN DATA WRAPPER timescaledb_fdw OPTIONS (dbname '$$||current_database()||$$', + host 'localhost', port '$$||current_setting('port')||$$' )$$; END; $d$; -CREATE USER MAPPING FOR CURRENT_USER SERVER loopback_1; -CREATE USER MAPPING FOR CURRENT_USER SERVER loopback_2; - SELECT _timescaledb_internal.test_remote_connection_cache(); diff --git a/tsl/test/src/remote/connection.c b/tsl/test/src/remote/connection.c index 737e60cb8..2d8f84aec 100644 --- a/tsl/test/src/remote/connection.c +++ b/tsl/test/src/remote/connection.c @@ -34,9 +34,10 @@ get_connection() { return remote_connection_open_with_options( "testdb", - list_make3(makeDefElem("user", + list_make4(makeDefElem("user", (Node *) makeString(GetUserNameFromId(GetUserId(), false)), -1), + makeDefElem("host", (Node *) makeString("localhost"), -1), makeDefElem("dbname", (Node *) makeString(get_database_name(MyDatabaseId)), -1), makeDefElem("port", (Node *) makeString(pstrdup(GetConfigOption("port", false, false))), diff --git a/tsl/test/src/remote/connection_cache.c b/tsl/test/src/remote/connection_cache.c index e850e3256..b5a9a99b6 100644 --- a/tsl/test/src/remote/connection_cache.c +++ b/tsl/test/src/remote/connection_cache.c @@ -28,8 +28,6 @@ #include "test_utils.h" #include "connection.h" -#define NEW_APPLICATION_NAME "app2" - TS_FUNCTION_INFO_V1(tsl_test_remote_connection_cache); static void