Обсуждение: Password leakage avoidance

I have recently, once again for the umpteenth time, been involved in 
discussions around (paraphrasing) "why does Postgres leak the passwords 
into the logs when they are changed". I know well that the canonical 
advice is something like "use psql with \password if you care about that".

And while that works, it is a deeply unsatisfying answer for me to give 
and for the OP to receive.

The alternative is something like "...well if you don't like that, use 
PQencryptPasswordConn() to roll your own solution that meets your 
security needs".

Again, not a spectacular answer IMHO. It amounts to "here is a 
do-it-yourself kit, go put it together". It occurred to me that we can, 
and really should, do better.

The attached patch set moves the guts of \password from psql into the 
libpq client side -- PQchangePassword() (patch 0001).

The usage in psql serves as a ready built-in test for the libpq function 
(patch 0002). Docs included too (patch 0003).

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.

I will register this in the upcoming commitfest, but meantime 
thought/comments/etc. would be gratefully received.

Thanks,

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

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:

* 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.

> 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.

            regards, tom lane

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

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

Prior to bikeshedding -- thanks for putting this together. This should 
generally helpful, as it allows libpq-based drivers to adopt this method 
and provide a safer mechanism for setting/changing passwords! (which we 
should also promote once availbale).

>> * 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.)

We're likely to have new algorithms in the future, as there is a draft 
RFC for updating the SCRAM hashes, and already some regulatory bodies 
are looking to deprecate SHA256. My concern with relying on the 
"encrypted_password" GUC (which is why PQencryptPasswordConn takes 
"conn") makes it any easier for users to choose the algorithm, or if 
they need to rely on the server/session setting.

I guess in its current state, it does, and drivers could mask some of 
the complexity.

>>> 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 think this is reasonable to add.

I think this is a good start and adds something that's better than what 
we have today. However, it seems like we also need something for "CREATE 
ROLE", otherwise we're either asking users to set passwords in two 
steps, or allowing for the unencrypted password to leak to the logs via 
CREATE ROLE.

Maybe we need a PQcreaterole that provide the mechanism to set passwords 
safely. It'd likely need to take all the options need for creating a 
role, but that would at least give the underlying mechanism to ensure 
we're always sending a hashed password to the server.

Jonathan

"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> I think this is a good start and adds something that's better than what 
> we have today. However, it seems like we also need something for "CREATE 
> ROLE", otherwise we're either asking users to set passwords in two 
> steps, or allowing for the unencrypted password to leak to the logs via 
> CREATE ROLE.

> Maybe we need a PQcreaterole that provide the mechanism to set passwords 
> safely. It'd likely need to take all the options need for creating a 
> role, but that would at least give the underlying mechanism to ensure 
> we're always sending a hashed password to the server.

I'm kind of down on that, because it seems like it'd be quite hard to
design an easy-to-use C API that doesn't break the next time somebody
adds another option to CREATE USER.  What's so wrong with suggesting
that the password be set in a separate step?  (For comparison, typical
Unix utilities like useradd(8) also tell you to set the password
separately.)

            regards, tom lane

Joe Conway <mail@joeconway.com> writes:
> 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".

As far as that goes, that filename pattern is what is generated by
"git format-patch".  I agree that the digit-count choices are a tad
odd, but they're not so awful as to be worth trying to override.

> The new pattern I picked is "description-vXXX-NN.patch" which fixes all 
> of those issues.

Only if you use the same "description" for all patches of a series,
which seems kind of not the point.  In any case, "git format-patch"
is considered best practice for a multi-patch series AFAIK, so we
have to cope with its ideas about how to name the files.

            regards, tom lane

On 12/24/23 12:22, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> 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".
> 
> As far as that goes, that filename pattern is what is generated by
> "git format-patch".  I agree that the digit-count choices are a tad
> odd, but they're not so awful as to be worth trying to override.


Ah, knew it was something like that. I am still a curmudgeon doing 
things the old way ¯\_(ツ)_/¯


>> The new pattern I picked is "description-vXXX-NN.patch" which fixes all 
>> of those issues.
> 
> Only if you use the same "description" for all patches of a series,
> which seems kind of not the point.  In any case, "git format-patch"
> is considered best practice for a multi-patch series AFAIK, so we
> have to cope with its ideas about how to name the files.
Even if I wanted some differentiating name for the individual patches in 
a set, I still like them to be grouped because it is one unit of work 
from my perspective.

Oh well, I guess I will get with the program and put every patch-set 
into its own directory.

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

Joe Conway <mail@joeconway.com> writes:
> Oh well, I guess I will get with the program and put every patch-set 
> into its own directory.

Yeah, that's what I've started doing too.  It does have some
advantages, in that you can squirrel away other related files
in the same subdirectory.

            regards, tom lane

On 23.12.23 16:13, Joe Conway wrote:
> I have recently, once again for the umpteenth time, been involved in 
> discussions around (paraphrasing) "why does Postgres leak the passwords 
> into the logs when they are changed". I know well that the canonical 
> advice is something like "use psql with \password if you care about that".
> 
> And while that works, it is a deeply unsatisfying answer for me to give 
> and for the OP to receive.
> 
> The alternative is something like "...well if you don't like that, use 
> PQencryptPasswordConn() to roll your own solution that meets your 
> security needs".
> 
> Again, not a spectacular answer IMHO. It amounts to "here is a 
> do-it-yourself kit, go put it together". It occurred to me that we can, 
> and really should, do better.
> 
> The attached patch set moves the guts of \password from psql into the 
> libpq client side -- PQchangePassword() (patch 0001).
> 
> The usage in psql serves as a ready built-in test for the libpq function 
> (patch 0002). Docs included too (patch 0003).

I don't follow how you get from the problem statement to this solution. 
This proposal doesn't avoid password leakage, does it?  It just provides 
a different way to phrase the existing solution.  Who is a potential 
user of this solution?  Right now it just saves a dozen lines in psql, 
but it's not clear how it improves anything else.

On 12/27/23 15:39, Peter Eisentraut wrote:
> On 23.12.23 16:13, Joe Conway wrote:
>> I have recently, once again for the umpteenth time, been involved in 
>> discussions around (paraphrasing) "why does Postgres leak the passwords 
>> into the logs when they are changed". I know well that the canonical 
>> advice is something like "use psql with \password if you care about that".
>> 
>> And while that works, it is a deeply unsatisfying answer for me to give 
>> and for the OP to receive.
>> 
>> The alternative is something like "...well if you don't like that, use 
>> PQencryptPasswordConn() to roll your own solution that meets your 
>> security needs".
>> 
>> Again, not a spectacular answer IMHO. It amounts to "here is a 
>> do-it-yourself kit, go put it together". It occurred to me that we can, 
>> and really should, do better.
>> 
>> The attached patch set moves the guts of \password from psql into the 
>> libpq client side -- PQchangePassword() (patch 0001).
>> 
>> The usage in psql serves as a ready built-in test for the libpq function 
>> (patch 0002). Docs included too (patch 0003).
> 
> I don't follow how you get from the problem statement to this solution.
> This proposal doesn't avoid password leakage, does it?

Yes, it most certainly does. The plaintext password would never be seen 
by the server and therefore never logged. This is exactly why the 
feature already existed in psql.

>  It just provides a different way to phrase the existing solution.

Yes, a fully built one that is convenient to use, and does not ask 
everyone to roll their own.

> Who is a potential user of this solution? 

Literally every company that has complained that Postgres pollutes their 
logs with plaintext passwords. I have heard the request to provide a 
better solution many times, over many years, while working for three 
different companies.

> Right now it just saves a dozen lines in psql, but it's not clear how
> it improves anything else.

It is to me, and so far no one else has complained about that. More 
opinions would be welcomed of course.

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

Joe Conway <mail@joeconway.com> writes:
> On 12/27/23 15:39, Peter Eisentraut wrote:
>> On 23.12.23 16:13, Joe Conway wrote:
>>> The attached patch set moves the guts of \password from psql into the 
>>> libpq client side -- PQchangePassword() (patch 0001).

>> I don't follow how you get from the problem statement to this solution.
>> This proposal doesn't avoid password leakage, does it?
>> It just provides a different way to phrase the existing solution.

> Yes, a fully built one that is convenient to use, and does not ask 
> everyone to roll their own.

It's convenient for users of libpq, I guess, but it doesn't help
anyone not writing C code directly atop libpq.  If this is the
way forward then we need to also press JDBC and other client
libraries to implement comparable functionality.  That's within
the realm of sanity surely, and having a well-thought-through
reference implementation in libpq would help those authors.
So I don't think this is a strike against the patch; but the answer
to Peter's question has to be that this is just part of the solution.

            regards, tom lane

On 12/27/23 16:09, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> On 12/27/23 15:39, Peter Eisentraut wrote:
>>> On 23.12.23 16:13, Joe Conway wrote:
>>>> The attached patch set moves the guts of \password from psql into the 
>>>> libpq client side -- PQchangePassword() (patch 0001).
> 
>>> I don't follow how you get from the problem statement to this solution.
>>> This proposal doesn't avoid password leakage, does it?
>>> It just provides a different way to phrase the existing solution.
> 
>> Yes, a fully built one that is convenient to use, and does not ask 
>> everyone to roll their own.
> 
> It's convenient for users of libpq, I guess, but it doesn't help
> anyone not writing C code directly atop libpq.  If this is the
> way forward then we need to also press JDBC and other client
> libraries to implement comparable functionality.  That's within
> the realm of sanity surely, and having a well-thought-through
> reference implementation in libpq would help those authors.
> So I don't think this is a strike against the patch; but the answer
> to Peter's question has to be that this is just part of the solution.

As mentioned downthread by Dave Cramer, JDBC is already onboard.

And as Jonathan said in an adjacent part of the thread:
> This should generally helpful, as it allows libpq-based drivers to
> adopt this method and provide a safer mechanism for setting/changing
> passwords! (which we should also promote once availbale).

Which is definitely something I have had in mind all along.

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

On 12/24/23 12:15 PM, Tom Lane wrote:

>> Maybe we need a PQcreaterole that provide the mechanism to set passwords
>> safely. It'd likely need to take all the options need for creating a
>> role, but that would at least give the underlying mechanism to ensure
>> we're always sending a hashed password to the server.
> 
> I'm kind of down on that, because it seems like it'd be quite hard to
> design an easy-to-use C API that doesn't break the next time somebody
> adds another option to CREATE USER.  What's so wrong with suggesting
> that the password be set in a separate step?  (For comparison, typical
> Unix utilities like useradd(8) also tell you to set the password
> separately.)

Modern development frameworks tend to reduce things down to one-step, 
even fairly complex operations. Additionally, a lot of these frameworks 
don't even require a developer to build backend applications that 
involve doing actually work on the backend (e.g. UNIX), so the approach 
of useradd(8) et al. are not familiar. Adding the additional step would 
lead to errors, e.g. the developer not calling the "change password" 
function to create the obfuscated password. Granted, we can push the 
problem down to driver authors to "be better" and simplify the process 
for their end users, but that still can be error prone, having seen this 
with driver authors implementing PostgreSQL SCRAM and having made 
(correctable) mistakes that could have lead to security issues.

That said, I see why trying to keep track of all of the "CREATE ROLE" 
attributes from a C API can be cumbersome, so perhaps we could end up 
adding an API that just does "create-user-with-password" and applies a 
similar method to Joe's patch. That would also align with the developer 
experience above, as in those cases users tend to just be created with a 
password w/o any of the additional role options.

Also open to punting this to a different thread as we can at least make 
things better with the "change password" approach.

Thanks,

Jonathan

On Wed, Dec 27, 2023 at 10:31 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> On 12/24/23 12:15 PM, Tom Lane wrote:
>
> >> Maybe we need a PQcreaterole that provide the mechanism to set passwords
> >> safely. It'd likely need to take all the options need for creating a
> >> role, but that would at least give the underlying mechanism to ensure
> >> we're always sending a hashed password to the server.
> >
> > I'm kind of down on that, because it seems like it'd be quite hard to
> > design an easy-to-use C API that doesn't break the next time somebody
> > adds another option to CREATE USER.  What's so wrong with suggesting
> > that the password be set in a separate step?  (For comparison, typical
> > Unix utilities like useradd(8) also tell you to set the password
> > separately.)
>
> Modern development frameworks tend to reduce things down to one-step,
> even fairly complex operations. Additionally, a lot of these frameworks
> don't even require a developer to build backend applications that
> involve doing actually work on the backend (e.g. UNIX), so the approach
> of useradd(8) et al. are not familiar. Adding the additional step would
> lead to errors, e.g. the developer not calling the "change password"
> function to create the obfuscated password. Granted, we can push the
> problem down to driver authors to "be better" and simplify the process
> for their end users, but that still can be error prone, having seen this
> with driver authors implementing PostgreSQL SCRAM and having made
> (correctable) mistakes that could have lead to security issues.

This seems to confuse "driver" with "framework".

I would say the "two step" approach is perfectly valid for a driver
whereas as you say most people building say webapps or similar on top
of a framework will expect it to handle things for them. But that's
more of a framework thing than a driver thing, depending on
terminology. E.g. it would be up to the "Postgres support driver" in
django/rails/whatnot to reduce it down to one step, not to a low level
driver like libpq (or other low level drivers).

None of those frameworks are likely to want to require direct driver
access anyway, they *want* to take control of that process in my
experience.

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On 12/31/23 9:50 AM, Magnus Hagander wrote:
> On Wed, Dec 27, 2023 at 10:31 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>> On 12/24/23 12:15 PM, Tom Lane wrote:
>>
>>>> Maybe we need a PQcreaterole that provide the mechanism to set passwords
>>>> safely. It'd likely need to take all the options need for creating a
>>>> role, but that would at least give the underlying mechanism to ensure
>>>> we're always sending a hashed password to the server.
>>>
>>> I'm kind of down on that, because it seems like it'd be quite hard to
>>> design an easy-to-use C API that doesn't break the next time somebody
>>> adds another option to CREATE USER.  What's so wrong with suggesting
>>> that the password be set in a separate step?  (For comparison, typical
>>> Unix utilities like useradd(8) also tell you to set the password
>>> separately.)
>>
>> Modern development frameworks tend to reduce things down to one-step,
>> even fairly complex operations. Additionally, a lot of these frameworks
>> don't even require a developer to build backend applications that
>> involve doing actually work on the backend (e.g. UNIX), so the approach
>> of useradd(8) et al. are not familiar. Adding the additional step would
>> lead to errors, e.g. the developer not calling the "change password"
>> function to create the obfuscated password. Granted, we can push the
>> problem down to driver authors to "be better" and simplify the process
>> for their end users, but that still can be error prone, having seen this
>> with driver authors implementing PostgreSQL SCRAM and having made
>> (correctable) mistakes that could have lead to security issues.
> 
> This seems to confuse "driver" with "framework".
> 
> I would say the "two step" approach is perfectly valid for a driver
> whereas as you say most people building say webapps or similar on top
> of a framework will expect it to handle things for them. But that's
> more of a framework thing than a driver thing, depending on
> terminology. E.g. it would be up to the "Postgres support driver" in
> django/rails/whatnot to reduce it down to one step, not to a low level
> driver like libpq (or other low level drivers).
> 
> None of those frameworks are likely to want to require direct driver
> access anyway, they *want* to take control of that process in my
> experience.

Fair point on the framework/driver comparison, but the above still 
applies to drivers. As mentioned above, non-libpq drivers did have 
mistakes that could have lead to security issues while implementing 
PostgreSQL SCRAM. Additionally, CVE-2021-23222[1] presented itself in 
both libpq/non-libpq drivers, either through the issue itself, or 
through implementing the protocol step in a way similar to libpq.

Keeping the implementation surface area simpler for driver maintainers 
does generally help mitigate further issues, though I'd defer to the 
driver maintainers if they agree with that statement.

Thanks,

Jonathan

[1] https://www.postgresql.org/support/security/CVE-2021-23222/

On Sun, Dec 24, 2023 at 12:06 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> We're likely to have new algorithms in the future, as there is a draft
> RFC for updating the SCRAM hashes, and already some regulatory bodies
> are looking to deprecate SHA256. My concern with relying on the
> "encrypted_password" GUC (which is why PQencryptPasswordConn takes
> "conn") makes it any easier for users to choose the algorithm, or if
> they need to rely on the server/session setting.

Yeah, I agree. It doesn't make much sense to me to propose that a GUC,
which is a server-side setting, should control client-side behavior.

Also, +1 for the general idea. I don't think this is a whole answer to
the problem of passwords appearing in log files because (1) you have
to be using libpq in order to make use of this and (2) you have to
actually use it instead of just doing something else and complaining
about the problem. But neither of those things is a reason not to have
it. There's no reason why a sophisticated user who goes through libpq
shouldn't have an easy way to do this instead of being asked to
reimplement it if they want the functionality.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 1/2/24 7:23 AM, Sehrope Sarkuni wrote:
> Having worked on and just about wrapped up the JDBC driver patch for 
> this, couple thoughts:

> 2. Password encoding should be split out and made available as its own 
> functions. Not just as part of a wider "create _or_ alter a user's 
> password" function attached to a connection. We went a step further and 
> added an intermediate function that generates the "ALTER USER ... 
> PASSWORD" SQL.

I agree with this. It's been a minute, but I had done some refactoring 
on the backend-side to support the "don't need a connection" case for 
SCRAM secret generation functions on the server-side[1]. But I think in 
general we should split out the password generation functions, which 
leads to:

> 5. Our SCRAM specific function allows for customizing the algo iteration 
> and salt parameters. That topic came up on hackers previously[1]. Our 
> high level "alterUserPassword(...)" function does not have those options 
> but it is available as part of our PasswordUtil SCRAM API for advanced 
> users who want to leverage it. The higher level functions have defaults 
> for iteration counts (4096) and salt size (16-bytes).

This seems like a good approach -- the regular function just has the 
defaults (which can be aligned to the PostgreSQL defaults) (or inherit 
from the server configuration, which then requires the connection to be 
present) and then have a more advanced API available.

Thanks,

Jonathan

[1] 
https://www.postgresql.org/message-id/3a9b7126-01a0-7e1a-1b2a-a76df6176725%40postgresql.org

On 1/2/24 07:23, Sehrope Sarkuni wrote:
> 1. There's two sets of defaults, the client program's default and the 
> server's default. Need to pick one for each implemented function. They 
> don't need to be the same across the board.

Is there a concrete recommendation here?

> 2. Password encoding should be split out and made available as its own 
> functions. Not just as part of a wider "create _or_ alter a user's 
> password" function attached to a connection.

It already is split out in libpq[1], unless I don't understand you 
correctly.

[1] 
https://www.postgresql.org/docs/current/libpq-misc.html#LIBPQ-PQENCRYPTPASSWORDCONN

> We went a step further and  added an intermediate function that
> generates the "ALTER USER ... PASSWORD" SQL.

I don't think we want that in libpq, but in any case separate 
patch/discussion IMHO.

> 3. We only added an "ALTER USER ... PASSWORD" function, not anything to 
> create a user. There's way too many options for that and keeping this 
> targeted at just assigning passwords makes it much easier to test.

+1

Also separate patch/discussion, but I don't think the CREATE version is 
needed.

> 4. RE:defaults, the PGJDBC approach is that the encoding-only function 
> uses the driver's default (SCRAM). The "alterUserPassword(...)" uses the 
> server's default (again usually SCRAM for modern installs but could be 
> something else). It's kind of odd that they're different but the use 
> cases are different as well.

Since PQencryptPasswordConn() already exists, and psql's "\password" 
used it with its defaults, I don't think we want to change the behavior. 
The patch as written behaves in a backward compatible way.

> 5. Our SCRAM specific function allows for customizing the algo iteration 
> and salt parameters. That topic came up on hackers previously[1]. Our 
> high level "alterUserPassword(...)" function does not have those options 
> but it is available as part of our PasswordUtil SCRAM API for advanced 
> users who want to leverage it. The higher level functions have defaults 
> for iteration counts (4096) and salt size (16-bytes).

Again separate patch/discussion, IMHO.

> 6. Getting the verbiage right for the PGJDBC version was kind of 
> annoying as we wanted to match the server's wording, e.g. 
> "password_encryption", but it's clearly hashing, not encryption. We 
> settled on "password encoding" for describing the overall task with the 
> comments referencing the server's usage of the term 
> "password_encryption". Found a similar topic[2] on changing that 
> recently as well but looks like that's not going anywhere.

Really this is irrelevant to this discussion, because the new function 
is called PQchangePassword().

The function PQencryptPasswordConn() has been around for a while and the 
horse is out of the gate on that one.

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

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


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

On 1/6/24 13:16, Sehrope Sarkuni wrote:
> On Sat, Jan 6, 2024 at 12:39 PM Joe Conway <mail@joeconway.com 
> <mailto:mail@joeconway.com>> wrote:
> 
>     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.
> 
> 
> One more thing that we do in pgjdbc is to zero out the input password 
> args so that they don't remain in memory even after being freed. It's 
> kind of odd in Java as it makes the input interface a char[] and we have 
> to convert them to garbage collected Strings internally (which kind of 
> defeats the purpose of the exercise).
> 
> But in libpq could be done via something like:
> 
> memset(pw1, 0, strlen(pw1));
> memset(pw2, 0, strlen(pw2));


That part is in psql not libpq

> There was some debate on our end of where to do that and we settled on 
> doing it inside the encoding functions to ensure it always happens. So 
> the input password char[] always gets wiped regardless of how the 
> encoding functions are invoked.
> 
> Even if it's not added to the password encoding functions (as that kind 
> of changes the after effects if anything was relying on the password 
> still having the password), I think it'd be good to add it to the 
> command.c stuff that has the two copies of the password prior to freeing 
> them.

While that change might or might not be worthwhile, I see it as 
independent of this patch.

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

Joe Conway <mail@joeconway.com> writes:
> 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.

I had not actually read this patchset before, but now I have, and
I have a few minor suggestions:

* The API comment for PQchangePassword should specify that encryption
is done according to the server's password_encryption setting, and
probably the SGML docs should too.  You shouldn't have to read the
code to discover that.

* I don't especially care for the useless initializations of
encrypted_password, fmtpw, and fmtuser.  In all those cases the
initial NULL is immediately replaced by a valid value.  Probably
the compiler will figure out that the initializations are useless,
but human readers have to do so as well.  Moreover, I think this
style is more bug-prone not less so, because if you ever change
the logic in a way that causes some code paths to fail to set
the variables, you won't get use-of-possibly-uninitialized-value
warnings from the compiler.

* Perhaps move the declaration of "buf" to the inner block where
it's actually used?

* This could be shortened to just "return res":

+                 if (!res)
+                     return NULL;
+                 else
+                     return res;

* I'd make the SGML documentation a bit more explicit about the
return value, say

+       Returns a <structname>PGresult</structname> pointer representing
+       the result of the <literal>ALTER USER</literal> command, or
+       a null pointer if the routine failed before issuing any command.

            regards, tom lane

On 1/6/24 15:10, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> 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.
> 
> I had not actually read this patchset before, but now I have, and
> I have a few minor suggestions:

Many thanks!

> * The API comment for PQchangePassword should specify that encryption
> is done according to the server's password_encryption setting, and
> probably the SGML docs should too.  You shouldn't have to read the
> code to discover that.

Check

> * I don't especially care for the useless initializations of
> encrypted_password, fmtpw, and fmtuser.  In all those cases the
> initial NULL is immediately replaced by a valid value.  Probably
> the compiler will figure out that the initializations are useless,
> but human readers have to do so as well.  Moreover, I think this
> style is more bug-prone not less so, because if you ever change
> the logic in a way that causes some code paths to fail to set
> the variables, you won't get use-of-possibly-uninitialized-value
> warnings from the compiler.
> 
> * Perhaps move the declaration of "buf" to the inner block where
> it's actually used?

Makes sense -- fixed

> * This could be shortened to just "return res":
> 
> +                 if (!res)
> +                     return NULL;
> +                 else
> +                     return res;

Heh, apparently I needed more coffee at this point :-)

> * I'd make the SGML documentation a bit more explicit about the
> return value, say
> 
> +       Returns a <structname>PGresult</structname> pointer representing
> +       the result of the <literal>ALTER USER</literal> command, or
> +       a null pointer if the routine failed before issuing any command.

Fixed.

I also ran pgindent. I was kind of surprised/unhappy when it made me 
change this (which aligned the two var names):
8<------------
<tab><tab><tab><tab>PQExpBufferData<tab>buf;
<tab><tab><tab><tab>PGresult<tab><sp><sp><sp>*res;
8<------------

to this (which leaves the var names unaligned):
8<------------
<tab><tab><tab><tab>PQExpBufferData<sp>buf;
<tab><tab><tab><tab>PGresult<sp><sp><sp>*res;
8<------------

Anyway, the resulting adjustments attached.

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