Subject: Re: [PATCH] Add CHECK_FOR_INTERRUPTS in scram_SaltedPassword loop.

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 23 Nov 2023 11:19:51 +0300

Hi,

> > If we want to add CHECK_FOR_INTERRUPTS inside the loop I think a brief
> > comment would be appropriate.
>
> This has been completed in patch v2 and it's ready for review.

Thanks!

> > I don't think it would be useful to limit this at an arbitrary point, iteration
> > count can be set per password and if someone wants a specific password to be
> > super-hard to brute force then why should we limit that?
> I agree with that. Maybe some users do want a super-hard password.
> RFC 7677 and RFC 5802 don't specify the maximum number of iterations.

That's a fairly good point. However we are not obligated not to
implement everything that is missing in RFC. Also in fact we already
limit the number of iterations to INT_MAX.

If we decide to limit this number even further the actual problem is
to figure out what the new practical limit would be. Regardless of the
chosen number there is a possibility of breaking backward
compatibility for certain users.

For this reason I believe merging the proposed patch would be the
right move at this point. It doesn't make anything worse for the
existing users and solves a potential problem for some of them.

-- 
Best regards,
Aleksander Alekseev

---

From: Michael Paquier <[email-masked]>
Date: Sat, 25 Nov 2023 10:20:00 +0900

> limit the number of iterations to INT_MAX.

INT_MAX, as in the limit that we have for integer GUCs and the
routines building the hashed entry, so the Postgres internals are what
defines the limit here.  I doubt that we'll see cases where somebody
would want more than that, but who knows in 10/20 years.

> compatibility for certain users.

No idea what the limit should be if it were to be lowered down, but
I suspect that even a new lower limit could be an issue for hosts in
the low-end specs when it comes to DOS.  It's not like there are no
ways to eat CPU when you are already logged in.

> existing users and solves a potential problem for some of them.

Yeah, agreed.  Being stuck on a potential large tight loops is
something we tend to avoid in the backend, so I agree that this is a
thing to keep in the backend especially because we have
scram_iterations and that it is user-settable.

I think that we should backpatch that down to v16 at least where the
GUC has been introduced as it's more like a nuisance if one sets the
GUC to an incorrect value, and I'd like to apply the patch this way.
Any objections or comments regarding that?
--
Michael

---

From: Bowen Shi <[email-masked]>
Date: Mon, 27 Nov 2023 11:56:31 +0800

> GUC to an incorrect value, and I'd like to apply the patch this way.

Agreed.

The patch has been submitted in https://commitfest.postgresql.org/46/4671/

Regards
Bowen Shi

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 27 Nov 2023 10:05:49 +0100

> On 25 Nov 2023, at 02:20, Michael Paquier <[email-masked]> wrote:
> 

The whole point of this GUC (and the iteration count construct in the spec) is
to allow hardened setups to make brute forcing passwords as hard as they choose
them to be, setting an upper limit (apart from the INT_MAX implementation
detail) where one isn't even mentioned in the RFC makes little sense when the
loop can be canceled.

On the flip side, setups which have low end clients can choose to reduce it
from the default to make scram an option at all where it previously was too
expensive and less secure schemes had to be used.

> Any objections or comments regarding that?

I don't see any reason to backpatch further down than 16 given how low the
hardcoded value is set there, scanning the archives I see no complaints about
it either.  As a reference, CREATE ROLE using 4096 iterations takes 14ms on my
10 year old laptop (1M iterations, 244x the default, takes less than a second).

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Tue, 28 Nov 2023 08:39:26 +0900

> 10 year old laptop (1M iterations, 244x the default, takes less than a second).

Agreed, so done it this way.  \password has the same problem, where we
could perhaps do something with a callback or something like that, or
perhaps that's just not worth bothering.
--
Michael