From: Nathan Bossart <nathandbossart@gmail.com>
To: Gurjeet Singh <gurjeet@singh.im>
Cc: pgsql-hackers@postgresql.org
Subject: Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
Date: Mon, 17 Jul 2023 13:47:44 -0700
Message-ID: <20230717204744.GA890035@nathanxps13> (raw)
In-Reply-To: <20230629052409.GB2049807@nathanxps13>
References: <20230628045741.GA1813397@nathanxps13>
<CABwTF4WB6kYyZzU6Qda_0D7-3SkqzHsVh6q-yscm5FHFQ2F7FQ@mail.gmail.com>
<20230629052409.GB2049807@nathanxps13>
On Wed, Jun 28, 2023 at 10:24:09PM -0700, Nathan Bossart wrote:
> On Wed, Jun 28, 2023 at 09:20:03PM -0700, Gurjeet Singh wrote:
>> The comment on top of connect_utils.c:connectDatabase() seems pertinent:
>>
>>> (Callers should not pass
>>> * allow_password_reuse=true unless reconnecting to the same database+user
>>> * as before, else we might create password exposure hazards.)
>>
>> The callers of {cluster|reindex}_one_database() (which in turn call
>> connectDatabase()) clearly pass different database names in successive
>> calls to these functions. So the patch seems to be in conflict with
>> the recommendation in the comment.
>>
>> I'm not sure if the concern raised in that comment is a legitimate
>> one, though. I mean, if the password is reused to connect to a
>> different database in the same cluster/instance, which I think is
>> always the case with these utilities, the password will exposed in the
>> server logs (if at all). And since the admins of the instance already
>> have full control over the passwords of the user, I don't think this
>> patch will give them any more information than what they can get
>> anyways.
>>
>> It is a valid concern, though, if the utility connects to a different
>> instance in the same run/invocation, and hence exposes the password
>> from the first instance to the admins of the second cluster.
>
> The same commit that added this comment (ff402ae) also set the
> allow_password_reuse parameter to true in vacuumdb's connectDatabase()
> calls. I found a message from the corresponding thread that provides some
> additional detail [0]. I wonder if this comment should instead recommend
> against using the allow_password_reuse flag unless reconnecting to the same
> host/port/user target. Connecting to different databases with the same
> host/port/user information seems okay. Maybe I am missing something...
Here is a new version of the patch in which I've updated this comment as
proposed. Gurjeet, do you have any other concerns about this patch?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.comAttachments:v2-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch (text/x-diff, 3.3K) download
From 85d0a1bbe62c4cc01b3fdba7c653f95b8472cc7a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 27 Jun 2023 21:38:24 -0700
Subject: [PATCH v2 1/1] harmonize password reuse in vacuumdb, clusterdb, and
reindexdb
---
doc/src/sgml/ref/reindexdb.sgml | 14 --------------
doc/src/sgml/ref/vacuumdb.sgml | 13 -------------
src/bin/scripts/clusterdb.c | 2 +-
src/bin/scripts/reindexdb.c | 2 +-
src/fe_utils/connect_utils.c | 2 +-
5 files changed, 3 insertions(+), 30 deletions(-)
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgmlindex 8cb8bf4fa3..8d9ced212f 100644--- a/doc/src/sgml/ref/reindexdb.sgml+++ b/doc/src/sgml/ref/reindexdb.sgml@@ -432,20 +432,6 @@ PostgreSQL documentation
</refsect1>
-- <refsect1>- <title>Notes</title>-- <para>- <application>reindexdb</application> might need to connect several- times to the <productname>PostgreSQL</productname> server, asking- for a password each time. It is convenient to have a- <filename>~/.pgpass</filename> file in such cases. See <xref- linkend="libpq-pgpass"/> for more information.- </para>- </refsect1>--
<refsect1>
<title>Examples</title>
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgmlindex da2393783b..09356ea4fa 100644--- a/doc/src/sgml/ref/vacuumdb.sgml+++ b/doc/src/sgml/ref/vacuumdb.sgml@@ -605,19 +605,6 @@ PostgreSQL documentation
</refsect1>
-- <refsect1>- <title>Notes</title>-- <para>- <application>vacuumdb</application> might need to connect several- times to the <productname>PostgreSQL</productname> server, asking- for a password each time. It is convenient to have a- <filename>~/.pgpass</filename> file in such cases. See <xref- linkend="libpq-pgpass"/> for more information.- </para>- </refsect1>-
<refsect1>
<title>Examples</title>
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.cindex 58a774013b..65428031c7 100644--- a/src/bin/scripts/clusterdb.c+++ b/src/bin/scripts/clusterdb.c@@ -195,7 +195,7 @@ cluster_one_database(const ConnParams *cparams, const char *table,
PGconn *conn;
- conn = connectDatabase(cparams, progname, echo, false, false);+ conn = connectDatabase(cparams, progname, echo, false, true);
initPQExpBuffer(&sql);
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.cindex 5b297d1dc1..002c41f221 100644--- a/src/bin/scripts/reindexdb.c+++ b/src/bin/scripts/reindexdb.c@@ -315,7 +315,7 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
bool failed = false;
int items_count = 0;
- conn = connectDatabase(cparams, progname, echo, false, false);+ conn = connectDatabase(cparams, progname, echo, false, true);
if (concurrently && PQserverVersion(conn) < 120000)
{
diff --git a/src/fe_utils/connect_utils.c b/src/fe_utils/connect_utils.cindex 7a1edea7c8..7d45f5c609 100644--- a/src/fe_utils/connect_utils.c+++ b/src/fe_utils/connect_utils.c@@ -25,7 +25,7 @@
*
* 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+ * allow_password_reuse=true unless reconnecting to the same host+port+user
* as before, else we might create password exposure hazards.)
*/
PGconn *
--
2.25.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, gurjeet@singh.im
Subject: Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
In-Reply-To: <20230717204744.GA890035@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox