pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH v2 1/1] harmonize password reuse in vacuumdb, clusterdb, and reindexdb
11+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2023-07-28 6d982e3b3b Harmonize password reuse in vacuumdb, clusterdb, and reindexdb.
* [PATCH v1 1/1] harmonize password reuse in vacuumdb, clusterdb, and reindexdb
@ 2023-06-28 04:38 Nathan Bossart <nathan@postgresql.org>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-06-28 04:38 UTC (permalink / raw)
---
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 +-
4 files changed, 2 insertions(+), 29 deletions(-)
inline patch 1: doc/src/sgml/ref/reindexdb.sgml, doc/src/sgml/ref/vacuumdb.sgml, src/bin/scripts/clusterdb.c and 1 more (+2/-29 lines) download inline-1.patch
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 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.sgml
index 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.c
index e3585b3272..1a7588d29c 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.c
index 3e8f6aca40..2eee8e351f 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)
{
--
2.25.1
* [PATCH v2 1/1] harmonize password reuse in vacuumdb, clusterdb, and reindexdb
@ 2023-06-28 04:38 Nathan Bossart <nathan@postgresql.org>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-06-28 04:38 UTC (permalink / raw)
---
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(-)
inline patch 1: doc/src/sgml/ref/reindexdb.sgml, doc/src/sgml/ref/vacuumdb.sgml, src/bin/scripts/clusterdb.c and 2 more (+3/-30 lines) download inline-1.patch
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 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.sgml
index 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.c
index 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.c
index 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.c
index 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
* harmonize password reuse in vacuumdb, clusterdb, and reindexdb
@ 2023-06-28 04:57 Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
0 siblings, 1 reply; 11+ messages in thread
From: Nathan Bossart @ 2023-06-28 04:57 UTC (permalink / raw)
To: pgsql-hackers
While looking at something unrelated, I noticed that the vacuumdb docs
mention the following:
vacuumdb might need to connect several times to the PostgreSQL server,
asking for a password each time.
IIUC this has been fixed since 83dec5a from 2015 (which was superceded by
ff402ae), so I think this note (originally added in e0a77f5 from 2002) can
now be removed.
I also found that neither clusterdb nor reindexdb uses the
allow_password_reuse parameter in connectDatabase(), and the reindexdb
documentation contains the same note about repeatedly asking for a
password (originally added in 85e9a5a from 2005). IMO we should allow
password reuse for all three programs, and we should remove the
aforementioned notes in the docs, too. This is what the attached patch
does.
Thoughts?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch (text/x-diff, 2.7K) download
From e6fa53e7a2b0903fc6a498f22f6b436b81b206ed Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 27 Jun 2023 21:38:24 -0700
Subject: [PATCH v1 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 +-
4 files changed, 2 insertions(+), 29 deletions(-)
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 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.sgml
index 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.c
index e3585b3272..1a7588d29c 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.c
index 3e8f6aca40..2eee8e351f 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)
{
--
2.25.1
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-06-29 04:20 ` Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Gurjeet Singh @ 2023-06-29 04:20 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On Tue, Jun 27, 2023 at 9:57 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> While looking at something unrelated, I noticed that the vacuumdb docs
> mention the following:
>
> vacuumdb might need to connect several times to the PostgreSQL server,
> asking for a password each time.
>
> IIUC this has been fixed since 83dec5a from 2015 (which was superceded by
> ff402ae), so I think this note (originally added in e0a77f5 from 2002) can
> now be removed.
>
> I also found that neither clusterdb nor reindexdb uses the
> allow_password_reuse parameter in connectDatabase(), and the reindexdb
> documentation contains the same note about repeatedly asking for a
> password (originally added in 85e9a5a from 2005). IMO we should allow
> password reuse for all three programs, and we should remove the
> aforementioned notes in the docs, too. This is what the attached patch
> does.
>
> Thoughts?
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.
Nitpicking: The patch seems to have Windows line endings, which
explains why my `patch` complained so loudly.
$ patch -p1 < v1-0001-harmonize-....patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file doc/src/sgml/ref/reindexdb.sgml
(Stripping trailing CRs from patch; use --binary to disable.)
patching file doc/src/sgml/ref/vacuumdb.sgml
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/scripts/clusterdb.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/scripts/reindexdb.c
$ file v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch
v1-0001-harmonize-....patch: unified diff output text, ASCII text,
with CRLF line terminators
Best regards,
Gurjeet
http://Gurje.et
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
@ 2023-06-29 05:24 ` Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 21:05 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-07-17 20:47 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-07-19 03:41 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Zhang Mingli <zmlpostgres@gmail.com>
2023-07-19 17:43 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 4 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-06-29 05:24 UTC (permalink / raw)
To: Gurjeet Singh <gurjeet@singh.im>; +Cc: pgsql-hackers
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...
> Nitpicking: The patch seems to have Windows line endings, which
> explains why my `patch` complained so loudly.
>
> $ patch -p1 < v1-0001-harmonize-....patch
> (Stripping trailing CRs from patch; use --binary to disable.)
> patching file doc/src/sgml/ref/reindexdb.sgml
> (Stripping trailing CRs from patch; use --binary to disable.)
> patching file doc/src/sgml/ref/vacuumdb.sgml
> (Stripping trailing CRs from patch; use --binary to disable.)
> patching file src/bin/scripts/clusterdb.c
> (Stripping trailing CRs from patch; use --binary to disable.)
> patching file src/bin/scripts/reindexdb.c
>
> $ file v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch
> v1-0001-harmonize-....patch: unified diff output text, ASCII text,
> with CRLF line terminators
Huh. I didn't write it on a Windows machine. I'll look into it.
[0] https://postgr.es/m/15139.1447357263%40sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-06-29 21:05 ` Nathan Bossart <nathandbossart@gmail.com>
3 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-06-29 21:05 UTC (permalink / raw)
To: Gurjeet Singh <gurjeet@singh.im>; +Cc: pgsql-hackers
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:
>> Nitpicking: The patch seems to have Windows line endings, which
>> explains why my `patch` complained so loudly.
>>
>> $ patch -p1 < v1-0001-harmonize-....patch
>> (Stripping trailing CRs from patch; use --binary to disable.)
>> patching file doc/src/sgml/ref/reindexdb.sgml
>> (Stripping trailing CRs from patch; use --binary to disable.)
>> patching file doc/src/sgml/ref/vacuumdb.sgml
>> (Stripping trailing CRs from patch; use --binary to disable.)
>> patching file src/bin/scripts/clusterdb.c
>> (Stripping trailing CRs from patch; use --binary to disable.)
>> patching file src/bin/scripts/reindexdb.c
>>
>> $ file v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch
>> v1-0001-harmonize-....patch: unified diff output text, ASCII text,
>> with CRLF line terminators
>
> Huh. I didn't write it on a Windows machine. I'll look into it.
I couldn't reproduce this with the patch available in the archives:
$ patch -p1 < v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch
patching file doc/src/sgml/ref/reindexdb.sgml
patching file doc/src/sgml/ref/vacuumdb.sgml
patching file src/bin/scripts/clusterdb.c
patching file src/bin/scripts/reindexdb.c
$ file v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch
v1-0001-harmonize-password-reuse-in-vacuumdb-clusterdb-an.patch: unified diff output, ASCII text
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-07-17 20:47 ` Nathan Bossart <nathandbossart@gmail.com>
2023-07-18 17:05 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
3 siblings, 1 reply; 11+ messages in thread
From: Nathan Bossart @ 2023-07-17 20:47 UTC (permalink / raw)
To: Gurjeet Singh <gurjeet@singh.im>; +Cc: pgsql-hackers
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.com
Attachments:
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.sgml
index 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.sgml
index 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.c
index 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.c
index 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.c
index 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
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-07-17 20:47 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-07-18 17:05 ` Gurjeet Singh <gurjeet@singh.im>
0 siblings, 0 replies; 11+ messages in thread
From: Gurjeet Singh @ 2023-07-18 17:05 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On Mon, Jul 17, 2023 at 1:47 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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?
With the updated comment, the patch looks good to me.
Best regards,
Gurjeet
http://Gurje.et
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-07-19 03:41 ` Zhang Mingli <zmlpostgres@gmail.com>
3 siblings, 0 replies; 11+ messages in thread
From: Zhang Mingli @ 2023-07-19 03:41 UTC (permalink / raw)
To: Gurjeet Singh <gurjeet@singh.im>; Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
HI,
On Jun 29, 2023 at 13:24 +0800, Nathan Bossart <nathandbossart@gmail.com>, wrote:
>
> Connecting to different databases with the same
> host/port/user information seems okay.
Have a look, yeah, cluster_all_databases/vacuum_all_databases/reindex_all_databases will get there.
LGTM.
Regards,
Zhang Mingli
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-07-19 17:43 ` Nathan Bossart <nathandbossart@gmail.com>
2023-07-28 17:14 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
3 siblings, 1 reply; 11+ messages in thread
From: Nathan Bossart @ 2023-07-19 17:43 UTC (permalink / raw)
To: Gurjeet Singh <gurjeet@singh.im>; +Cc: pgsql-hackers; tgl@sss.pgh.pa.us
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.
>>
>> [ ... ]
>
> 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...
I added Tom here since it looks like he was the original author of this
comment. Tom, do you have any concerns with updating the comment for
connectDatabase() in src/fe_utils/connect_utils.c like this?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-07-19 17:43 ` Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
@ 2023-07-28 17:14 ` Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-07-28 17:14 UTC (permalink / raw)
To: Gurjeet Singh <gurjeet@singh.im>; +Cc: pgsql-hackers; tgl@sss.pgh.pa.us
On Wed, Jul 19, 2023 at 10:43:11AM -0700, Nathan Bossart wrote:
> On Wed, Jun 28, 2023 at 10:24:09PM -0700, Nathan Bossart wrote:
>> 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...
>
> I added Tom here since it looks like he was the original author of this
> comment. Tom, do you have any concerns with updating the comment for
> connectDatabase() in src/fe_utils/connect_utils.c like this?
I went ahead and committed this. I'm happy to revisit if there are
concerns.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-07-28 17:14 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-28 04:38 [PATCH v1 1/1] harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathan@postgresql.org>
2023-06-28 04:38 [PATCH v2 1/1] harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathan@postgresql.org>
2023-06-28 04:57 harmonize password reuse in vacuumdb, clusterdb, and reindexdb Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 04:20 ` Gurjeet Singh <gurjeet@singh.im>
2023-06-29 05:24 ` Nathan Bossart <nathandbossart@gmail.com>
2023-06-29 21:05 ` Nathan Bossart <nathandbossart@gmail.com>
2023-07-17 20:47 ` Nathan Bossart <nathandbossart@gmail.com>
2023-07-18 17:05 ` Gurjeet Singh <gurjeet@singh.im>
2023-07-19 03:41 ` Zhang Mingli <zmlpostgres@gmail.com>
2023-07-19 17:43 ` Nathan Bossart <nathandbossart@gmail.com>
2023-07-28 17:14 ` Nathan Bossart <nathandbossart@gmail.com>
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