Re: Refactor SCRAM code to dynamically handle hash type and key length - Mailing list pgsql-hackers
From | Jonathan S. Katz |
---|---|
Subject | Re: Refactor SCRAM code to dynamically handle hash type and key length |
Date | |
Msg-id | e1e4f583-af83-6f44-1d37-ffcfe67f4f1f@postgresql.org Whole thread Raw |
In response to | Re: Refactor SCRAM code to dynamically handle hash type and key length (Michael Paquier <michael@paquier.xyz>) |
Responses |
Re: Refactor SCRAM code to dynamically handle hash type and key length
(Michael Paquier <michael@paquier.xyz>)
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: