Skip to content

Commit

Permalink
fix password handling in safeURI (#522)
Browse files Browse the repository at this point in the history
* remove PGPASSWORD environment processing from pgsql.c

instead use `pgsql->connectionString` containing the password
and adjust related function comments and removed unused PASSWORD_MASK

* fix mismatched safe{Source,Target}PGURI.password references
  • Loading branch information
acritox authored Nov 2, 2023
1 parent f80ee85 commit 44d6fdf
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 17 deletions.
3 changes: 0 additions & 3 deletions src/bin/pgcopydb/defaults.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,6 @@

#define POSTGRES_PORT 5432

/* masqurade passwords with that value in logs */
#define PASSWORD_MASK "****"

/* default replication slot and origin for logical replication */
#define REPLICATION_ORIGIN "pgcopydb"
#define REPLICATION_PLUGIN "test_decoding"
Expand Down
10 changes: 5 additions & 5 deletions src/bin/pgcopydb/parsing_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -936,9 +936,9 @@ escapeWithPercentEncoding(const char *str, char **dst)


/*
* uri_contains_password takes a Postgres connection string and checks to see
* if it contains a parameter called password. Returns true if a password
* keyword is present in the connection string.
* uri_grab_password takes a Postgres connection string and checks to see
* if it contains a parameter called password and if so stores a copy of it
* in safeURI->password.
*/
static bool
uri_grab_password(const char *pguri, SafeURI *safeURI)
Expand Down Expand Up @@ -984,8 +984,8 @@ uri_grab_password(const char *pguri, SafeURI *safeURI)

/*
* parse_and_scrub_connection_string takes a Postgres connection string and
* populates scrubbedPguri with the password replaced with **** for logging.
* The scrubbedPguri parameter should point to a memory area that has been
* populates safeURI without the password for logging purposes.
* The safeURI parameter should point to a memory area that has been
* allocated by the caller and has at least MAXCONNINFO bytes.
*/
bool
Expand Down
4 changes: 2 additions & 2 deletions src/bin/pgcopydb/pgcmd.c
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ pg_restore_db(PostgresPaths *pgPaths,
setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);

/* override PGPASSWORD environment variable if the pguri contains one */
if (connStrings->safeSourcePGURI.password != NULL)
if (connStrings->safeTargetPGURI.password != NULL)
{
if (pgpassword_found_in_env && !get_env_dup("PGPASSWORD", &PGPASSWORD))
{
Expand Down Expand Up @@ -892,7 +892,7 @@ pg_restore_db(PostgresPaths *pgPaths,

/* make sure to reset the environment PGPASSWORD if we edited it */
if (pgpassword_found_in_env &&
connStrings->safeSourcePGURI.password != NULL)
connStrings->safeTargetPGURI.password != NULL)
{
setenv("PGPASSWORD", PGPASSWORD, 1);
}
Expand Down
8 changes: 1 addition & 7 deletions src/bin/pgcopydb/pgsql.c
Original file line number Diff line number Diff line change
Expand Up @@ -516,12 +516,6 @@ pgsql_open_connection(PGSQL *pgsql)
setenv("PGAPPNAME", PGCOPYDB_PGAPPNAME, 1);
}

/* use the parsed password, unless the environment already is set */
if (!env_exists("PGPASSWORD") && pgsql->safeURI.password != NULL)
{
setenv("PGPASSWORD", pgsql->safeURI.password, 1);
}

/* we implement our own retry strategy */
setenv("PGCONNECT_TIMEOUT", POSTGRES_CONNECT_TIMEOUT, 1);

Expand All @@ -530,7 +524,7 @@ pgsql_open_connection(PGSQL *pgsql)
INSTR_TIME_SET_ZERO(pgsql->retryPolicy.connectTime);

/* Make a connection to the database */
pgsql->connection = PQconnectdb(pgsql->safeURI.pguri);
pgsql->connection = PQconnectdb(pgsql->connectionString);

/* Check to see that the backend connection was successfully made */
if (PQstatus(pgsql->connection) != CONNECTION_OK)
Expand Down

0 comments on commit 44d6fdf

Please sign in to comment.