Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb

On Tue, Jun 27, 2023 at 9:57 PM Nathan Bossart <nathandbossart(at)gmail(dot)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