Password leakage avoidance

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

Dave Cramer
www.postgres.rocks

On Sat, 23 Dec 2023 at 11:00, Tom Lane <tgl@sss.pgh.pa.us> 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:

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

Using the server version has some issues. It's quite possible to encrypt a
user password with md5 when the server version is scram. So if you change
the encryption then pg_hba.conf would have to be updated to allow the user
to log back in.

Dave

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 Wed, 27 Dec 2023 at 16:10, Tom Lane <tgl@sss.pgh.pa.us> 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.

Already have one in the works for JDBC, actually predates this.
https://github.com/pgjdbc/pgjdbc/pull/3067

Dave

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]https://www.postgresql.org/support/security/CVE-2021-23222/ 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/

Having worked on and just about wrapped up the JDBC driver patch for this,
couple thoughts:

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.
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.
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.
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.
5. Our SCRAM specific function allows for customizing the algo iteration
and salt parameters. That topic came up on hackers previously[1]/messages/by-id/1d669d97-86b3-a5dc-9f02-c368bca911f6@iki.fi. 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).
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]/messages/by-id/ZV149Fd2JG_OF7CM@momjian.us on
changing that recently as well but looks like that's not going anywhere.

[1]: /messages/by-id/1d669d97-86b3-a5dc-9f02-c368bca911f6@iki.fi
/messages/by-id/1d669d97-86b3-a5dc-9f02-c368bca911f6@iki.fi
[2]: /messages/by-id/ZV149Fd2JG_OF7CM@momjian.us
/messages/by-id/ZV149Fd2JG_OF7CM@momjian.us

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

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 Wed, 3 Jan 2024 at 08:53, Robert Haas <robertmhaas@gmail.com> wrote:

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

JDBC has it as of yesterday. I would imagine other clients will implement
it.
Dave Cramer

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]/messages/by-id/3a9b7126-01a0-7e1a-1b2a-a76df6176725@postgresql.org. 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]: /messages/by-id/3a9b7126-01a0-7e1a-1b2a-a76df6176725@postgresql.org
/messages/by-id/3a9b7126-01a0-7e1a-1b2a-a76df6176725@postgresql.org

<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">On 1/3/24 7:53 AM, Robert Haas wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CA+TgmoZ=V9t+07LHAhJVU0-F-g+Gmu4eeYi+gFPTf2RuOrMxpQ@mail.gmail.com">
<pre>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.</pre>
</blockquote>
<p>ISTM the only way to really move the needle (short of removing
all SQL support for changing passwords) would be a GUC that allows
disabling the use of SQL for setting passwords. While that doesn't
prevent leakage, it does at least force users to use a secure
method of setting passwords.<br>
</p>
<pre class="moz-signature" cols="72">--
Jim Nasby, Data Architect, Austin TX</pre>
</body>
</html>

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]https://www.postgresql.org/docs/current/libpq-misc.html#LIBPQ-PQENCRYPTPASSWORDCONN, unless I don't understand you
correctly.

[1]: https://www.postgresql.org/docs/current/libpq-misc.html#LIBPQ-PQENCRYPTPASSWORDCONN
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 Sat, Jan 6, 2024 at 11:53 AM Joe Conway <mail@joeconway.com> wrote:

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?

Between writing that and now, we scrapped the "driver picks" variant of
this. Now we only have explicit functions for each of the encoding types
(i.e. one for md5 and one for SCRAM-SHA-256) and an alterUserPassword(...)
method that uses the default for the database via reading the
password_encrypotion GUC. We also have some javadoc comments on the
encoding functions to strongly suggest using the SCRAM functions and only
use the md5 directly for legacy servers.

The "driver picks" one was removed to prevent a situation where an end user
picks the driver default and it's not compatible with their server. The
rationale was if the driver's SCRAM-SHA-256 default is ever replaced with
something else (e.g. SCRAM-SHA-512) we'd end up with an interim state where
an upgraded driver application would try to use that newer encryption
method on an old DB. If a user is going to do that, they would have to be
explicit with their choice of encoding functions (hence removing the
"driver picks" variant).

So the recommendation is to have explicit functions for each variant and
have the end-to-end change password code read from the DB.

My understanding of this patch is that it does exactly that.

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.

Sorry for the confusion. My original list wasn't any specific contrasts
with what libpq is doing. Was more of a summary of thoughts having just
concluded implementing the same type of password change stuff in PGJDBC.

From what I've seen in this patch, it either aligns with how we did things
in PGJDBC or it's something that isn't as relevant in this context (e.g.
generating the SQL text as a public function).

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

On Sat, Jan 6, 2024 at 12:39 PM Joe Conway <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));

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.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Scratch that, rather than memset(...) should be explicit_bzero(...) so it
doesn't get optimized out. Same idea though.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.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