Re: Password leakage avoidance

On 12/24/23 10:14, Joe Conway wrote:
> 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.

I just read through the thread and my conclusion is that, specifically
related to this patch set (i.e. notwithstanding suggestions for related
features), there is consensus in favor of adding this feature.

The only code specific comments were Tom's above, which have been
addressed. If there are no serious objections I plan to commit this
relatively soon.

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