Re: Refactor SCRAM code to dynamically handle hash type and key length

On 12/20/22 2:25 AM, Michael Paquier wrote:
> On Tue, Dec 20, 2022 at 08:58:38AM +0900, Michael Paquier wrote:
>> Thanks! I have applied for I have here.. There are other pieces to
>> think about in this area.
>
> FYI, I have spent a few hours looking at the remaining parts of the
> SCRAM code that could be simplified if a new hash method is added, and
> this b3bb7d1 has really made things easier.

Great! Thanks for doing a quick "stress test" on this.

> There are a few things
> that will need more thoughts. Here are my notes, assuming that
> SHA-512 is done:
> 1) HBA entries had better use a new keyword for scram-sha-512, implying
> a new uaSCRAM512 to combine with the existing uaSCRAM. One reason
> behind that it to advertise the mechanisms supported back to the
> client depending on the matching HBA entry.

This does seem like a challenge, particularly if we have to support
multiple different SCRAM hashes.

Perhaps this can be done with an interface change in HBA. For example,
we could rename the auth method from "scram-sha-256" to "scram" and
support an option list of hashes (e.g. "hash=sha-512,sha-256"). We can
then advertise the user-selected hashes as part of the handshake.

For backwards compatibility, we can take an auth method of
"scram-sha-256" to mean "scram" + using a sha-256 hash. Similarly, if no
hash map is defined, we can default to "scram-sha-256".

Anyway, I understand this point would require more discussion, but
perhaps it is a way to simplify the amount of code we would need to
write to support more hashes.

> 2) If a role has a SCRAM-SHA-256 password and the HBA entry matches
> scram-sha-512, the SASL exchange needs to go through the mock process
> with SHA-512 and fail.
> 3) If a role has a SCRAM-SHA-512 password and the HBA entry matches
> scram-sha-256, the SASL exchange needs to go through the mock process
> with SHA-256 and fail.

*nods*

> 4) The case of MD5 is something that looks a bit tricky at quick
> glance. We know that if the role has a MD5 password stored, we will
> fail anyway. So we could just advertise the SHA-256 mechanisms in
> this case and map the mock to that?

Is this the case where we specify "md5" as the auth method but the
user-password is stored in SCRAM?

> 5) The mechanism choice in libpq needs to be reworked a bit based on
> what the backend sends. There may be no point in advertising all the
> SHA-256 and SHA-512 mechanisms at the same time, I guess.

Yeah, I think a user may want to select which ones they want to use
(e.g. they may not want to advertise SHA-256).

> Attached is a WIP patch that I have played with. This shows the parts
> of the code that would need more thoughts if implementing such
> things. This works for the cases 1~3 (see the TAP tests). I've given
> up on the MD5 case 4 for now, but perhaps I just missed a simple trick.
> 5 in libpq uses dirty tricks. I have marked this CF entry as
> committed, and I'll come back to each relevant part on new separate
> threads.

Thanks for starting this.

Jonathan