From 3945b6193240cd7fcb5ad3c67e9dca7f12d68b7e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 23 Dec 2015 15:45:43 -0500 Subject: [PATCH] Improve handling of password reuse in src/bin/scripts programs. This reverts most of commit 83dec5a71 in favor of having connectDatabase() store the possibly-reusable password in a static variable, similar to the coding we've had for a long time in pg_dump's version of that function. To avoid possible problems with unwanted password reuse, make callers specify whether it's reasonable to attempt to re-use the password. This is a wash for cases where re-use isn't needed, but it is far simpler for callers that do want that. Functionally there should be no difference. Even though we're past RC1, it seems like a good idea to back-patch this into 9.5, like the prior commit. Otherwise, if there are any third-party users of connectDatabase(), they'll have to deal with an API change in 9.5 and then another one in 9.6. Michael Paquier --- src/bin/scripts/clusterdb.c | 4 +-- src/bin/scripts/common.c | 48 +++++++++++++------------- src/bin/scripts/common.h | 4 +-- src/bin/scripts/createlang.c | 8 ++--- src/bin/scripts/createuser.c | 4 +-- src/bin/scripts/droplang.c | 8 ++--- src/bin/scripts/dropuser.c | 4 +-- src/bin/scripts/reindexdb.c | 8 ++--- src/bin/scripts/vacuumdb.c | 66 ++++++------------------------------ 9 files changed, 56 insertions(+), 98 deletions(-) diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c index 2f15c82273..f87a9cee29 100644 --- a/src/bin/scripts/clusterdb.c +++ b/src/bin/scripts/clusterdb.c @@ -203,8 +203,8 @@ cluster_one_database(const char *dbname, bool verbose, const char *table, appendPQExpBuffer(&sql, " %s", table); appendPQExpBufferChar(&sql, ';'); - conn = connectDatabase(dbname, host, port, username, NULL, prompt_password, - progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); if (!executeMaintenanceCommand(conn, sql.data, echo)) { if (table) diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c index d26a4edbb6..362e748b9a 100644 --- a/src/bin/scripts/common.c +++ b/src/bin/scripts/common.c @@ -54,22 +54,31 @@ handle_help_version_opts(int argc, char *argv[], /* * Make a database connection with the given parameters. * - * A password can be given, but if not (or if user forces us to) we prompt - * interactively for one, unless caller prohibited us from doing so. + * An interactive password prompt is automatically issued if needed and + * allowed by prompt_password. + * + * If allow_password_reuse is true, we will try to re-use any password + * given during previous calls to this routine. (Callers should not pass + * allow_password_reuse=true unless reconnecting to the same database+user + * as before, else we might create password exposure hazards.) */ PGconn * connectDatabase(const char *dbname, const char *pghost, const char *pgport, - const char *pguser, const char *pgpassword, - enum trivalue prompt_password, const char *progname, - bool fail_ok) + const char *pguser, enum trivalue prompt_password, + const char *progname, bool fail_ok, bool allow_password_reuse) { PGconn *conn; - char *password; + static char *password = NULL; bool new_pass; - password = pgpassword ? strdup(pgpassword) : NULL; + if (!allow_password_reuse) + { + if (password) + free(password); + password = NULL; + } - if (prompt_password == TRI_YES && !pgpassword) + if (password == NULL && prompt_password == TRI_YES) password = simple_prompt("Password: ", 100, false); /* @@ -78,9 +87,8 @@ connectDatabase(const char *dbname, const char *pghost, const char *pgport, */ do { -#define PARAMS_ARRAY_SIZE 7 - const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords)); - const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values)); + const char *keywords[7]; + const char *values[7]; keywords[0] = "host"; values[0] = pghost; @@ -107,9 +115,6 @@ connectDatabase(const char *dbname, const char *pghost, const char *pgport, exit(1); } - pg_free(keywords); - pg_free(values); - /* * No luck? Trying asking (again) for a password. */ @@ -125,9 +130,6 @@ connectDatabase(const char *dbname, const char *pghost, const char *pgport, } } while (new_pass); - if (password) - free(password); - /* check to see that the backend connection was successfully made */ if (PQstatus(conn) == CONNECTION_BAD) { @@ -157,15 +159,15 @@ connectMaintenanceDatabase(const char *maintenance_db, const char *pghost, /* If a maintenance database name was specified, just connect to it. */ if (maintenance_db) - return connectDatabase(maintenance_db, pghost, pgport, pguser, NULL, - prompt_password, progname, false); + return connectDatabase(maintenance_db, pghost, pgport, pguser, + prompt_password, progname, false, false); /* Otherwise, try postgres first and then template1. */ - conn = connectDatabase("postgres", pghost, pgport, pguser, NULL, - prompt_password, progname, true); + conn = connectDatabase("postgres", pghost, pgport, pguser, prompt_password, + progname, true, false); if (!conn) - conn = connectDatabase("template1", pghost, pgport, pguser, NULL, - prompt_password, progname, false); + conn = connectDatabase("template1", pghost, pgport, pguser, + prompt_password, progname, false, false); return conn; } diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h index dafd58f422..9038295918 100644 --- a/src/bin/scripts/common.h +++ b/src/bin/scripts/common.h @@ -31,8 +31,8 @@ extern void handle_help_version_opts(int argc, char *argv[], extern PGconn *connectDatabase(const char *dbname, const char *pghost, const char *pgport, const char *pguser, - const char *pgpassword, enum trivalue prompt_password, - const char *progname, bool fail_ok); + enum trivalue prompt_password, const char *progname, + bool fail_ok, bool allow_password_reuse); extern PGconn *connectMaintenanceDatabase(const char *maintenance_db, const char *pghost, const char *pgport, const char *pguser, diff --git a/src/bin/scripts/createlang.c b/src/bin/scripts/createlang.c index 87cd623d25..f349624092 100644 --- a/src/bin/scripts/createlang.c +++ b/src/bin/scripts/createlang.c @@ -140,8 +140,8 @@ main(int argc, char *argv[]) printQueryOpt popt; static const bool translate_columns[] = {false, true}; - conn = connectDatabase(dbname, host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", " "(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" " @@ -180,8 +180,8 @@ main(int argc, char *argv[]) if (*p >= 'A' && *p <= 'Z') *p += ('a' - 'A'); - conn = connectDatabase(dbname, host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); /* * Make sure the language isn't already installed diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c index 9e7f84d9d5..dc358e40ce 100644 --- a/src/bin/scripts/createuser.c +++ b/src/bin/scripts/createuser.c @@ -250,8 +250,8 @@ main(int argc, char *argv[]) if (login == 0) login = TRI_YES; - conn = connectDatabase("postgres", host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase("postgres", host, port, username, prompt_password, + progname, false, false); initPQExpBuffer(&sql); diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c index fb4025ddf7..e35fdc0092 100644 --- a/src/bin/scripts/droplang.c +++ b/src/bin/scripts/droplang.c @@ -139,8 +139,8 @@ main(int argc, char *argv[]) printQueryOpt popt; static const bool translate_columns[] = {false, true}; - conn = connectDatabase(dbname, host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); printfPQExpBuffer(&sql, "SELECT lanname as \"%s\", " "(CASE WHEN lanpltrusted THEN '%s' ELSE '%s' END) as \"%s\" " @@ -181,8 +181,8 @@ main(int argc, char *argv[]) if (*p >= 'A' && *p <= 'Z') *p += ('a' - 'A'); - conn = connectDatabase(dbname, host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); /* * Force schema search path to be just pg_catalog, so that we don't have diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c index 10d0691c67..1b0cf78f2a 100644 --- a/src/bin/scripts/dropuser.c +++ b/src/bin/scripts/dropuser.c @@ -128,8 +128,8 @@ main(int argc, char *argv[]) appendPQExpBuffer(&sql, "DROP ROLE %s%s;", (if_exists ? "IF EXISTS " : ""), fmtId(dropuser)); - conn = connectDatabase("postgres", host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase("postgres", host, port, username, prompt_password, + progname, false, false); if (echo) printf("%s\n", sql.data); diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index fbf436c6d5..b6f3dcbca0 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -297,8 +297,8 @@ reindex_one_database(const char *name, const char *dbname, const char *type, appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name)); appendPQExpBufferChar(&sql, ';'); - conn = connectDatabase(dbname, host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); if (!executeMaintenanceCommand(conn, sql.data, echo)) { @@ -372,8 +372,8 @@ reindex_system_catalogs(const char *dbname, const char *host, const char *port, appendPQExpBuffer(&sql, " SYSTEM %s;", dbname); - conn = connectDatabase(dbname, host, port, username, NULL, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, false); if (!executeMaintenanceCommand(conn, sql.data, echo)) { fprintf(stderr, _("%s: reindexing of system catalogs failed: %s"), diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 8cbf0392a8..0863c64cbc 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -43,8 +43,7 @@ static void vacuum_one_database(const char *dbname, vacuumingOptions *vacopts, const char *host, const char *port, const char *username, enum trivalue prompt_password, int concurrentCons, - const char *progname, bool echo, bool quiet, - char **password); + const char *progname, bool echo, bool quiet); static void vacuum_all_databases(vacuumingOptions *vacopts, bool analyze_in_stages, @@ -276,8 +275,6 @@ main(int argc, char *argv[]) } else { - char *password = NULL; - if (dbname == NULL) { if (getenv("PGDATABASE")) @@ -299,8 +296,7 @@ main(int argc, char *argv[]) &tables, host, port, username, prompt_password, concurrentCons, - progname, echo, quiet, - &password); + progname, echo, quiet); } } else @@ -309,10 +305,7 @@ main(int argc, char *argv[]) &tables, host, port, username, prompt_password, concurrentCons, - progname, echo, quiet, - &password); - - pg_free(password); + progname, echo, quiet); } exit(0); @@ -330,21 +323,15 @@ main(int argc, char *argv[]) * If concurrentCons is > 1, multiple connections are used to vacuum tables * in parallel. In this case and if the table list is empty, we first obtain * a list of tables from the database. - * - * 'password' is both an input and output parameter. If one is not passed, - * then whatever is used in a connection is returned so that caller can - * reuse it in future connections. */ static void vacuum_one_database(const char *dbname, vacuumingOptions *vacopts, int stage, SimpleStringList *tables, const char *host, const char *port, - const char *username, - enum trivalue prompt_password, + const char *username, enum trivalue prompt_password, int concurrentCons, - const char *progname, bool echo, bool quiet, - char **password) + const char *progname, bool echo, bool quiet) { PQExpBufferData sql; PGconn *conn; @@ -378,15 +365,8 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts, fflush(stdout); } - conn = connectDatabase(dbname, host, port, username, *password, - prompt_password, progname, false); - - /* - * If no password was not specified by caller and the connection required - * one, remember it; this suppresses further password prompts. - */ - if (PQconnectionUsedPassword(conn) && *password == NULL) - *password = pg_strdup(PQpass(conn)); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, true); initPQExpBuffer(&sql); @@ -444,20 +424,10 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts, init_slot(slots, conn); if (parallel) { - const char *pqpass; - - /* - * If a password was supplied for the initial connection, use it for - * subsequent ones too. (Note that since we're connecting to the same - * database with the same user, there's no need to update the stored - * password any further.) - */ - pqpass = PQpass(conn); - for (i = 1; i < concurrentCons; i++) { - conn = connectDatabase(dbname, host, port, username, pqpass, - prompt_password, progname, false); + conn = connectDatabase(dbname, host, port, username, prompt_password, + progname, false, true); init_slot(slots + i, conn); } } @@ -572,23 +542,12 @@ vacuum_all_databases(vacuumingOptions *vacopts, PGresult *result; int stage; int i; - char *password = NULL; conn = connectMaintenanceDatabase(maintenance_db, host, port, username, prompt_password, progname); - result = executeQuery(conn, "SELECT datname FROM pg_database WHERE datallowconn ORDER BY 1;", progname, echo); - - /* - * Remember the password for further connections. If no password was - * required for the maintenance db connection, this gets updated for the - * first connection that does. - */ - if (PQconnectionUsedPassword(conn)) - password = pg_strdup(PQpass(conn)); - PQfinish(conn); if (analyze_in_stages) @@ -613,8 +572,7 @@ vacuum_all_databases(vacuumingOptions *vacopts, NULL, host, port, username, prompt_password, concurrentCons, - progname, echo, quiet, - &password); + progname, echo, quiet); } } } @@ -630,13 +588,11 @@ vacuum_all_databases(vacuumingOptions *vacopts, NULL, host, port, username, prompt_password, concurrentCons, - progname, echo, quiet, - &password); + progname, echo, quiet); } } PQclear(result); - pg_free(password); } /*