Re: Password leakage avoidance

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