Re: Password leakage avoidance

On 12/23/23 11:00, Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> The attached patch set moves the guts of \password from psql into the
>> libpq client side -- PQchangePassword() (patch 0001).
>
> Haven't really read the patch, just looked at the docs, but here's
> a bit of bikeshedding:

Thanks!

> * This seems way too eager to promote the use of md5. Surely the
> default ought to be SCRAM, full stop. I question whether we even
> need an algorithm parameter. Perhaps it's a good idea for
> future-proofing, but we could also plan that the function would
> make its own decisions based on noting the server's version.
> (libpq is far more likely to be au courant about what to do than
> the calling application, IMO.)

> * Parameter order seems a bit odd: to me it'd be natural to write
> user before password.

> * Docs claim return type is char *, but then say bool (which is
> also what the code says). We do not use bool in libpq's API;
> the admittedly-hoary convention is to use int with 1/0 values.
> Rather than quibble about that, though, I wonder if we should
> make the result be the PGresult from the command, which the
> caller would be responsible to free. That would document error
> conditions much more flexibly. On the downside, client-side
> errors would be harder to report, particularly OOM, but I think
> we have solutions for that in existing functions.

> * The whole business of nonblock mode seems fairly messy here,
> and I wonder whether it's worth the trouble to support. If we
> do want to claim it works then it ought to be tested.

All of these (except for the doc "char *" cut-n-pasteo) were due to
trying to stay close to the same interface as PQencryptPasswordConn().

But I agree with your assessment and the attached patch series addresses
all of them.

The original use of PQencryptPasswordConn() in psql passed a NULL for
the algorithm, so I dropped that argument entirely. I also swapped user
and password arguments because as you pointed out that is more natural.

This version returns PGresult. As far as special handling for
client-side errors like OOM, I don't see anything beyond returning a
NULL to signify fatal error, e,g,:

8<--------------
PGresult *
PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
{
PGresult *result;

result = (PGresult *) malloc(sizeof(PGresult));
if (!result)
return NULL;
8<--------------

That is the approach I took.

>> One thing I have not done but, considered, is adding an additional
>> optional parameter to allow "VALID UNTIL" to be set. Seems like it would
>> be useful to be able to set an expiration when setting a new password.
>
> No strong opinion about that.

Thanks -- hopefully others will weigh in on that.

Completely unrelated process bikeshedding:
I changed the naming scheme I used for the split patch-set this time. I
don't know if we have a settled/documented pattern for such naming, but
the original pattern which I borrowed from someone else's patches was
"vX-NNNN-description.patch".

The problems I have with that are 1/ there may well be more that 10
versions of a patch-set, 2/ there are probably never going to be more
than 2 digits worth of files in a patch-set, and 3/ the description
coming after the version and file identifiers causes the patches in my
local directory to sort poorly, intermingling several unrelated patch-sets.

The new pattern I picked is "description-vXXX-NN.patch" which fixes all
of those issues. Does that bother anyone? *Should* we try to agree on a
desired pattern (assuming there is not one already)?

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com