On Mon, Dec 19, 2022 at 02:58:24PM -0500, Jonathan S. Katz wrote:
> 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.
Yes, these mostly act as reminders to anybody touching this code, so
I'd like to keep both. For the mock part, we may also want to use
something different than SHA-256.
> At a minimum, I'd suggest a comment around it, especially if it's set up to
> be removed at a future date.
Okay, sure.
> - 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.
A few code paths relied on the size of these local buffers, now they
just use the passed-in key length from the state.
> 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! I have applied for I have here.. There are other pieces to
think about in this area.
--
Michael
