Re: Password leakage avoidance

On 12/23/23 11:00, Tom Lane wrote:
> Joe Conway <mail@joeconway.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