Re: Password leakage avoidance - Mailing list pgsql-hackers
From | Joe Conway |
---|---|
Subject | Re: Password leakage avoidance |
Date | |
Msg-id | de8a97b4-fc2d-4ee3-8b13-3033aec3c403@joeconway.com Whole thread Raw |
In response to | Re: Password leakage avoidance (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: Password leakage avoidance
Re: Password leakage avoidance Re: Password leakage avoidance |
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: