Обсуждение: harmonize password reuse in vacuumdb, clusterdb, and reindexdb

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

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

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

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

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

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

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

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