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-bba85c2fa...@postgresql.org
OpenPGP_signature
Description: OpenPGP digital signature
