Thread

  1. Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2023-12-23T15:13:58Z

    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
  2. Re: Password leakage avoidance

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-23T16:00:10Z

    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
    
    
    
    
  3. Re: Password leakage avoidance

    Dave Cramer <davecramer@postgres.rocks> — 2023-12-24T12:13:13Z

    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
    
  4. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2023-12-24T15:14:27Z

    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
    
  5. Re: Password leakage avoidance

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-12-24T17:06:03Z

    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
    
  6. Re: Password leakage avoidance

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-24T17:15:56Z

    "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
    
    
    
    
  7. Re: Password leakage avoidance

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-24T17:22:01Z

    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
    
    
    
    
  8. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2023-12-24T18:02:01Z

    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
    
    
    
    
    
  9. Re: Password leakage avoidance

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-24T18:11:28Z

    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
    
    
    
    
  10. Re: Password leakage avoidance

    Peter Eisentraut <peter@eisentraut.org> — 2023-12-27T20:39:48Z

    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.
    
    
    
    
    
  11. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2023-12-27T20:53:35Z

    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
    
    
    
    
    
  12. Re: Password leakage avoidance

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-27T21:09:55Z

    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
    
    
    
    
  13. Re: Password leakage avoidance

    Dave Cramer <davecramer@postgres.rocks> — 2023-12-27T21:11:17Z

    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
    
  14. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2023-12-27T21:26:22Z

    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
    
    
    
    
    
  15. Re: Password leakage avoidance

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-12-27T21:31:21Z

    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
    
  16. Re: Password leakage avoidance

    Magnus Hagander <magnus@hagander.net> — 2023-12-31T14:50:05Z

    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/
    
    
    
    
  17. Re: Password leakage avoidance

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-12-31T15:20:53Z

    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/
    
    
  18. Re: Password leakage avoidance

    Sehrope Sarkuni <sehrope@jackdb.com> — 2024-01-02T12:23:31Z

    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]. 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] on
    changing that recently as well but looks like that's not going anywhere.
    
    [1]:
    https://www.postgresql.org/message-id/1d669d97-86b3-a5dc-9f02-c368bca911f6%40iki.fi
    [2]:
    https://www.postgresql.org/message-id/flat/ZV149Fd2JG_OF7CM%40momjian.us#cc97d20ff357a9e9264d4ae14e96e566
    
    Regards,
    -- Sehrope Sarkuni
    Founder & CEO | JackDB, Inc. | https://www.jackdb.com/
    
  19. Re: Password leakage avoidance

    Robert Haas <robertmhaas@gmail.com> — 2024-01-03T13:53:17Z

    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
    
    
    
    
  20. Re: Password leakage avoidance

    Dave Cramer <davecramer@postgres.rocks> — 2024-01-03T13:59:50Z

    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
    
    >
    >
    
  21. Re: Password leakage avoidance

    Jonathan S. Katz <jkatz@postgresql.org> — 2024-01-03T15:33:38Z

    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
    
  22. Re: Password leakage avoidance

    Jim Nasby <jim.nasby@gmail.com> — 2024-01-03T22:43:51Z

    <!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>
    
    
    
    
  23. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2024-01-06T16:53:24Z

    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
    
    
    
    
    
  24. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2024-01-06T17:39:17Z

    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
    
  25. Re: Password leakage avoidance

    Sehrope Sarkuni <sehrope@jackdb.com> — 2024-01-06T18:00:32Z

    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/
    
  26. Re: Password leakage avoidance

    Sehrope Sarkuni <sehrope@jackdb.com> — 2024-01-06T18:16:34Z

    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/
    
  27. Re: Password leakage avoidance

    Sehrope Sarkuni <sehrope@jackdb.com> — 2024-01-06T18:18:29Z

    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/
    
    >
    
  28. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2024-01-06T18:31:22Z

    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
    
    
    
    
    
  29. Re: Password leakage avoidance

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-06T20:10:59Z

    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
    
    
    
    
  30. Re: Password leakage avoidance

    Joe Conway <mail@joeconway.com> — 2024-01-07T18:51:50Z

    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