On Tue, Jun 27, 2023 at 9:57 PM Nathan Bossart <nathandboss...@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