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

On 12/16/22 10:08 PM, Michael Paquier wrote:
> On Thu, Dec 15, 2022 at 04:59:52AM +0900, Michael Paquier wrote:
>> However, that's only half of the picture. The key length and the hash
>> type (or just the hash type to know what's the digest/key length to
>> use but that's more invasive) still need to be sent across the
>> internal routines of SCRAM and attached to the state data of the
>> frontend and the backend or we won't be able to do the hash and HMAC
>> computations dependent on that.
>
> Attached is a patch to do exactly that, and as a result v2 is half the
> size of v1:
> - SCRAM_KEY_LEN is now named SCRAM_MAX_KEY_LEN, adding a note that
> this should be kept in sync as the maximum digest size of the
> supported hash methods. This is used as the method to size all the
> internal buffers of the SCRAM routines.
> - SCRAM_SHA_256_KEY_LEN is used to track the key length for
> SCRAM-SHA-256, the one initialized with the state data.
> - No changes in the internal, the buffers are just resized based on
> the max defined.

Thanks! I looked through this and ran tests. I like the approach overall
and I think this sets us up pretty well for expanding our SCRAM support.

Only a couple of minor comments:

- I noticed a couple of these in "scram_build_secret" and "scram_mock_salt":

Assert(hash_type == PG_SHA256);

Presumably there to ensure 1/ We're setting a hash_type and 2/ as
possibly a reminder to update the assertions if/when we support more
digests.

With the assertion in "scram_build_secret", that value is set from the
"PG_SHA256" constant anyway, so I don't know if it actually gives us
anything other than a reminder? With "scram_mock"salt" the value
ultimately comes from state (which is currently set from the constant),
so perhaps there is a guard there.

At a minimum, I'd suggest a comment around it, especially if it's set up
to be removed at a future date.

- I do like the "SCRAM_MAX_KEY_LEN" change, and I see we're now passing
"key_length" around to ensure we're only using the desired number of
bytes. I am a little queasy that once we expand "SCRAM_MAX_KEY_LEN" we
run the risk of having the smaller hashes accidentally use the extra
bytes in their calculations. However, I think that's more a fear than
not, an we can mitigate the risk with testing.

> I'd like to move on with that in the next couple of days (still need
> to study more the other areas of the code to see what else could be
> made more pluggable), so let me know if there are any objections..

No objections. I think this decreases the lift to supporting more
variations of SCRAM.

Once committed, I'll rebase the server-side SCRAM functions patch with
this. I may want to rethink the interface for that to allow the digest
to be "selectable" (vs. from the function) but I'll discuss on that
thread[1].

Thanks,

Jonathan

[1]
https://www.postgresql.org/message-id/fce7228e-d0d6-64a1-3dcb-bba85c2fac85@postgresql.org