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

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