Thread: Raising the SCRAM iteration count

In the thread about user space SCRAM functions [0] I mentioned that it might be
wise to consider raising our SCRAM iteration count.  The iteration count is an
important defence against brute-force attacks.

Our current hardcoded value for iteration count is 4096, which is based on a
recommendation from RFC 7677.  This is however the lower end of the scale, and
is related to computing power in 2015 generation handheld devices.  The
relevant paragraph in section 4 of RFC 7677 [1] reads:

   "As a rule of thumb, the hash iteration-count should be such that a modern
   machine will take 0.1 seconds to perform the complete algorithm; however,
   this is unlikely to be practical on mobile devices and other relatively low-
   performance systems.  At the time this was written, the rule of thumb gives
   around 15,000 iterations required; however, a hash iteration- count of 4096
   takes around 0.5 seconds on current mobile handsets."

It goes on to say:

   "..the recommendation of this specification is that the hash iteration- count
   SHOULD be at least 4096, but careful consideration ought to be given to
   using a significantly higher value, particularly where mobile use is less
   important."

Selecting 4096 was thus a conservative take already in 2015, and is now very
much so.  On my 2020-vintage Macbook I need ~200k iterations to consume 0.1
seconds (in a build with assertions).  Calculating tens of thousands of hashes
per second on a consumer laptop at a 4096 iteration count is no stretch.  A
brief look shows that MongoDB has a minimum of 5000 with a default of 15000
[2]; Kafka has a minimum of 4096 [3].

Making the iteration count a configurable setting would allow installations to
raise the iteration count to strengthen against brute force attacks, while
still supporting those with lower end clients who prefer the trade-off of
shorter authentication times.

The attached introduces a scram_iteration_count GUC with a default of 15000
(still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
are stored per secret it can be altered with backwards compatibility.

Clientside the count is still at 4096 to limit the scope of this patch a bit.
For psql it would mean adding options to \password which should be a thread of
its own.  For libpq one can imagine specifying this in the algorithm parameter
passed to PQencryptPasswordConn like "scram-sha-256:100000" or something
similar.  It's premature to pursue those without agreement that we should make
the count configurable though.  If this patch is accepted, I'll work on that
next.

Thoughts?

--
Daniel Gustafsson        https://vmware.com/

[0] fce7228e-d0d6-64a1-3dcb-bba85c2fac85@postgresql.org
[1] https://www.rfc-editor.org/rfc/rfc7677#section-4
[2] https://www.mongodb.com/docs/manual/reference/parameters/#mongodb-parameter-param.scramSHA256IterationCount
[3]
https://docs.confluent.io/platform/current/kafka/authentication_sasl/authentication_sasl_scram.html#security-considerations-for-sasl-scram

On 09/12/2022 12:55, Daniel Gustafsson wrote:
> In the thread about user space SCRAM functions [0] I mentioned that it might be
> wise to consider raising our SCRAM iteration count.  The iteration count is an
> important defence against brute-force attacks.
> 
> Our current hardcoded value for iteration count is 4096, which is based on a
> recommendation from RFC 7677.  This is however the lower end of the scale, and
> is related to computing power in 2015 generation handheld devices.  The
> relevant paragraph in section 4 of RFC 7677 [1] reads:
> 
>     "As a rule of thumb, the hash iteration-count should be such that a modern
>     machine will take 0.1 seconds to perform the complete algorithm; however,
>     this is unlikely to be practical on mobile devices and other relatively low-
>     performance systems.  At the time this was written, the rule of thumb gives
>     around 15,000 iterations required; however, a hash iteration- count of 4096
>     takes around 0.5 seconds on current mobile handsets."
> 
> It goes on to say:
> 
>     "..the recommendation of this specification is that the hash iteration- count
>     SHOULD be at least 4096, but careful consideration ought to be given to
>     using a significantly higher value, particularly where mobile use is less
>     important."
> 
> Selecting 4096 was thus a conservative take already in 2015, and is now very
> much so.  On my 2020-vintage Macbook I need ~200k iterations to consume 0.1
> seconds (in a build with assertions).  Calculating tens of thousands of hashes
> per second on a consumer laptop at a 4096 iteration count is no stretch.  A
> brief look shows that MongoDB has a minimum of 5000 with a default of 15000
> [2]; Kafka has a minimum of 4096 [3].
> 
> Making the iteration count a configurable setting would allow installations to
> raise the iteration count to strengthen against brute force attacks, while
> still supporting those with lower end clients who prefer the trade-off of
> shorter authentication times.
> 
> The attached introduces a scram_iteration_count GUC with a default of 15000
> (still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
> are stored per secret it can be altered with backwards compatibility.

We just had a discussion with a colleague about using a *smaller* 
iteration count. Why? To make the connection startup faster. We're 
experimenting with a client that runs in a Cloudflare worker, which is a 
wasm runtime with very small limits on how much CPU time you're allowed 
to use (without paying extra). And we know that the password is randomly 
generated and long enough. If I understand correctly, the point of 
iterations is to slow down brute-force or dictionary attacks, but if the 
password is strong enough to begin with, those attacks are not possible 
regardless of iteration count. So I would actually like to set the 
minimum iteration count all the way down to 1.

- Heikki

On Fri, Dec 09, 2022 at 05:50:00PM +0200, Heikki Linnakangas wrote:
>> The attached introduces a scram_iteration_count GUC with a default of 15000
>> (still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
>> are stored per secret it can be altered with backwards compatibility.
>
> We just had a discussion with a colleague about using a *smaller* iteration
> count. Why? To make the connection startup faster. We're experimenting with
> a client that runs in a Cloudflare worker, which is a wasm runtime with very
> small limits on how much CPU time you're allowed to use (without paying
> extra). And we know that the password is randomly generated and long enough.
> If I understand correctly, the point of iterations is to slow down
> brute-force or dictionary attacks, but if the password is strong enough to
> begin with, those attacks are not possible regardless of iteration count. So
> I would actually like to set the minimum iteration count all the way down to
> 1.

This is the kind of thing that should be easily measurable with
pgbench -C and an empty script.  How much difference are you seeing
with 1, 4096 and more than that?

All that comes down to provide more capability for the existing
routines in my opinion.  So what if we finally extended with a new
flavor PQencryptPasswordConn() able to get a list of options, say
PQencryptPasswordConn() extended that has a string with all the
options?  psql could use for \password a grammar consistent with \g,
as of: \password (iteration=4096, salt_length=123) PASS_STR

Note that scram_build_secret() is already able to handle any iteration
count, even at 1, so IMO it is not a good idea to lower the default to
be so.  I'd agree with Daniel to make it higher by default and follow
the RFCs, though like you I have wanted also in core much more control
over that.
--
Michael

Hi,

On 2022-12-09 11:55:07 +0100, Daniel Gustafsson wrote:
> Our current hardcoded value for iteration count is 4096, which is based on a
> recommendation from RFC 7677.  This is however the lower end of the scale, and
> is related to computing power in 2015 generation handheld devices.  The
> relevant paragraph in section 4 of RFC 7677 [1] reads:
> 
>    "As a rule of thumb, the hash iteration-count should be such that a modern
>    machine will take 0.1 seconds to perform the complete algorithm; however,
>    this is unlikely to be practical on mobile devices and other relatively low-
>    performance systems.  At the time this was written, the rule of thumb gives
>    around 15,000 iterations required; however, a hash iteration- count of 4096
>    takes around 0.5 seconds on current mobile handsets."
> 
> It goes on to say:
> 
>    "..the recommendation of this specification is that the hash iteration- count
>    SHOULD be at least 4096, but careful consideration ought to be given to
>    using a significantly higher value, particularly where mobile use is less
>    important."
> 
> Selecting 4096 was thus a conservative take already in 2015, and is now very
> much so.  On my 2020-vintage Macbook I need ~200k iterations to consume 0.1
> seconds (in a build with assertions).  Calculating tens of thousands of hashes
> per second on a consumer laptop at a 4096 iteration count is no stretch.  A
> brief look shows that MongoDB has a minimum of 5000 with a default of 15000
> [2]; Kafka has a minimum of 4096 [3].
> 
> Making the iteration count a configurable setting would allow installations to
> raise the iteration count to strengthen against brute force attacks, while
> still supporting those with lower end clients who prefer the trade-off of
> shorter authentication times.
>
> The attached introduces a scram_iteration_count GUC with a default of 15000
> (still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
> are stored per secret it can be altered with backwards compatibility.

I am extremely doubtful it's a good idea to increase the default (if anything
the opposite). 0.1 seconds is many times the connection establishment
overhead, even over network.  I've seen users complain about postgres
connection establishment overhead being high, and it just turned out to be due
to scram - yes, they ended up switching to md5, because that was the only
viable alternative.

PGPASSWORD=passme pgbench -n -C -f ~/tmp/select.sql -h 127.0.0.1 -T10 -U passme

md5: tps = 158.577609
scram: tps = 38.196362

Greetings,

Andres Freund

> On 10 Dec 2022, at 01:15, Andres Freund <andres@anarazel.de> wrote:
> On 2022-12-09 11:55:07 +0100, Daniel Gustafsson wrote:

>> The attached introduces a scram_iteration_count GUC with a default of 15000
>> (still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
>> are stored per secret it can be altered with backwards compatibility.
>
> I am extremely doubtful it's a good idea to increase the default (if anything
> the opposite). 0.1 seconds is many times the connection establishment
> overhead, even over network.  I've seen users complain about postgres
> connection establishment overhead being high, and it just turned out to be due
> to scram - yes, they ended up switching to md5, because that was the only
> viable alternative.

That's a fair point.  For the record I don't think we should raise the default
to match 0.1 seconds, but we should make the option available to those who want
it.  If we provide a GUC for the iteration count which has a lower limit than
todays hardcoded value, then maybe we can help workloads with long-lived
connections who want increased on-disk safety as well as workloads where low
connection establishment is critical (or where the env is constrained like in
Heikki's example).

> PGPASSWORD=passme pgbench -n -C -f ~/tmp/select.sql -h 127.0.0.1 -T10 -U passme
>
> md5: tps = 158.577609
> scram: tps = 38.196362

Lowering the minimum for scram_iteration_count I tried out the patch on a set
of iteration counts of interest.  Values are averaged over three runs, using
the same pgbench setup you had above with basically a noop select.sql.  The
relative difference between the values are way off from your results, but I
haven't done much digging to figure that out yet (different OpenSSL versions
might be one factor).

md5: tps = 154.052690
scram 1: tps = 150.060285
scram 1024: tps = 138.191224
scram 4096: tps = 115.197533
scram 15000: tps = 75.156399

For the fun of it, 100000 iterations yields tps = 20.822393.

SCRAM with an iteration count of 1 still provides a lot of benefits over md5,
so if we can make those comparable in performance then that could be a way
forward (with the tradeoffs properly documented).

--
Daniel Gustafsson        https://vmware.com/

On Sun, Dec 11, 2022 at 12:46:23AM +0100, Daniel Gustafsson wrote:
> SCRAM with an iteration count of 1 still provides a lot of benefits over md5,
> so if we can make those comparable in performance then that could be a way
> forward (with the tradeoffs properly documented).

Okay, it looks like there is a wish to make that configurable anyway,
and I have a few comments about that.

       {"scram_iteration_count", PGC_SUSET, CONN_AUTH_AUTH,
+           gettext_noop("Sets the iteration count for SCRAM secret generation."),
+           NULL,
+           GUC_NOT_IN_SAMPLE | GUC_SUPERUSER_ONLY
+       },

Shouldn't this be user-settable as a PGC_USERSET rather than
PGC_SUSET which would limit its updates to superusers?

As shaped, the GUC would not benefit to \password, and we should not
encourage users to give a raw password over the wire if possible if
they wish to compute a verifier with a given interation number.
Hence, wouldn't it be better to mark it as GUC_REPORT, and store its
status in pg_conn@libpq-int.h in the same fashion as
default_transaction_read_only and hot_standby?  This way,
PQencryptPasswordConn() would be able to feed on it automatically
rather than always assume the default implied by
pg_fe_scram_build_secret().
--
Michael

On 12/9/22 7:15 PM, Andres Freund wrote:
> Hi,
> 
> On 2022-12-09 11:55:07 +0100, Daniel Gustafsson wrote:
>> Our current hardcoded value for iteration count is 4096, which is based on a
>> recommendation from RFC 7677.  This is however the lower end of the scale, and
>> is related to computing power in 2015 generation handheld devices.  The
>> relevant paragraph in section 4 of RFC 7677 [1] reads:
>>
>>     "As a rule of thumb, the hash iteration-count should be such that a modern
>>     machine will take 0.1 seconds to perform the complete algorithm; however,
>>     this is unlikely to be practical on mobile devices and other relatively low-
>>     performance systems.  At the time this was written, the rule of thumb gives
>>     around 15,000 iterations required; however, a hash iteration- count of 4096
>>     takes around 0.5 seconds on current mobile handsets."
>>
>> It goes on to say:
>>
>>     "..the recommendation of this specification is that the hash iteration- count
>>     SHOULD be at least 4096, but careful consideration ought to be given to
>>     using a significantly higher value, particularly where mobile use is less
>>     important."
>>
>> Selecting 4096 was thus a conservative take already in 2015, and is now very
>> much so.  On my 2020-vintage Macbook I need ~200k iterations to consume 0.1
>> seconds (in a build with assertions).  Calculating tens of thousands of hashes
>> per second on a consumer laptop at a 4096 iteration count is no stretch.  A
>> brief look shows that MongoDB has a minimum of 5000 with a default of 15000
>> [2]; Kafka has a minimum of 4096 [3].
>>
>> Making the iteration count a configurable setting would allow installations to
>> raise the iteration count to strengthen against brute force attacks, while
>> still supporting those with lower end clients who prefer the trade-off of
>> shorter authentication times.
>>
>> The attached introduces a scram_iteration_count GUC with a default of 15000
>> (still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
>> are stored per secret it can be altered with backwards compatibility.

To throw on a bit of paint, if we do change it, we should likely follow 
what would come out in a RFC.

While the SCRAM-SHA-512 RFC is still in draft[1], the latest draft it 
contains a "SHOULD" recommendation of 10000, which was bumped up from 
4096 in an earlier version of the draft:

==snip==
Therefore, the recommendation of this specification is that the hash 
iteration- count SHOULD be at least 10000, but careful consideration 
ought to be given to using a significantly higher value, particularly 
where mobile use is less important.¶
==snip==

I'm currently ambivalent (+0) on changing the default. I think giving 
the user more control over iterations ([2], and follow up work to make 
it easier to set iteration account via client) can help with this.

However, I do like the idea of a GUC.

> I am extremely doubtful it's a good idea to increase the default (if anything
> the opposite). 0.1 seconds is many times the connection establishment
> overhead, even over network.  I've seen users complain about postgres
> connection establishment overhead being high, and it just turned out to be due
> to scram - yes, they ended up switching to md5, because that was the only
> viable alternative.

Ugh, I'd be curious to know how often that is the case. That said, I 
think some of the above work could help with that.

Thanks,

Jonathan

[1] https://datatracker.ietf.org/doc/html/draft-melnikov-scram-sha-512
[2] https://postgr.es/m/fce7228e-d0d6-64a1-3dcb-bba85c2fac85@postgresql.org/

> On 12 Dec 2022, at 15:47, Jonathan S. Katz <jkatz@postgresql.org> wrote:

> To throw on a bit of paint, if we do change it, we should likely follow what would come out in a RFC.
>
> While the SCRAM-SHA-512 RFC is still in draft[1], the latest draft it contains a "SHOULD" recommendation of 10000,
whichwas bumped up from 4096 in an earlier version of the draft: 

This is however the draft for a different algorithm: SCRAM-SHA-512.  We are
supporting SCRAM-SHA-256 which is defined in RFC7677.  The slightly lower
recommendation there makes sense as SHA-512 is more computationally expensive
than SHA-256.

It does raise an interesting point though, if we in the future add suppprt for
SCRAM-SHA-512 (which seems reasonable to do) it's not good enough to have a
single GUC for SCRAM iterations; we'd need to be able to set the iteration
count per algorithm. I'll account for that when updating the patch downthread.

--
Daniel Gustafsson        https://vmware.com/

On Tue, Dec 13, 2022 at 12:17:58PM +0100, Daniel Gustafsson wrote:
> It does raise an interesting point though, if we in the future add suppprt for
> SCRAM-SHA-512 (which seems reasonable to do) it's not good enough to have a
> single GUC for SCRAM iterations; we'd need to be able to set the iteration
> count per algorithm. I'll account for that when updating the patch downthread.

So, you mean that the GUC should be named like password_iterations,
taking a grammar with a list like 'scram-sha-256=4096,algo2=5000'?
--
Michael

> On 14 Dec 2022, at 02:00, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Dec 13, 2022 at 12:17:58PM +0100, Daniel Gustafsson wrote:
>> It does raise an interesting point though, if we in the future add suppprt for
>> SCRAM-SHA-512 (which seems reasonable to do) it's not good enough to have a
>> single GUC for SCRAM iterations; we'd need to be able to set the iteration
>> count per algorithm. I'll account for that when updating the patch downthread.
>
> So, you mean that the GUC should be named like password_iterations,
> taking a grammar with a list like 'scram-sha-256=4096,algo2=5000'?

I was thinking about it but opted for the simpler approach of a GUC name with
the algorithm baked into it: scram_sha256_iterations.  It doesn't seem all that
likely that we'll have more than two versions of SCRAM (sha256/sha512) so
the additional complexity doesn't seem worth it.

The attached v2 has the GUC rename and a change to GUC_REPORT such that the
frontend can use the real value rather than the default.  I kept it for super
users so far, do you think it should be a user setting being somewhat sensitive?

The default in this version is rolled back to 4096 as there was pushback
against raising it, and the lower limit is one in order to potentially assist
situations like the one Andres mentioned where md5 is used.

--
Daniel Gustafsson        https://vmware.com/

On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>> On 14 Dec 2022, at 02:00, Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Tue, Dec 13, 2022 at 12:17:58PM +0100, Daniel Gustafsson wrote:
>>> It does raise an interesting point though, if we in the future add suppprt for
>>> SCRAM-SHA-512 (which seems reasonable to do) it's not good enough to have a
>>> single GUC for SCRAM iterations; we'd need to be able to set the iteration
>>> count per algorithm. I'll account for that when updating the patch downthread.
>>
>> So, you mean that the GUC should be named like password_iterations,
>> taking a grammar with a list like 'scram-sha-256=4096,algo2=5000'?
> 
> I was thinking about it but opted for the simpler approach of a GUC name with
> the algorithm baked into it: scram_sha256_iterations.  It doesn't seem all that
> likely that we'll have more than two versions of SCRAM (sha256/sha512) so
> the additional complexity doesn't seem worth it.

I would not rule this out. There is a RFC draft for SCRAM-SHA3-512[1].

I do have mixed feelings on the 'x1=y1,x2=y2' style GUC, but we do have 
machinery to handle it and it gives a bit more flexibility over how many 
SCRAM hash methods get added. I'd like to hear more feedback.

(I don't know if there will be a world if we ever let users BYO-hash, 
but that case may force separate GUCs anyway).

[1] https://datatracker.ietf.org/doc/draft-melnikov-scram-sha3-512/

> The attached v2 has the GUC rename and a change to GUC_REPORT such that the
> frontend can use the real value rather than the default.  I kept it for super
> users so far, do you think it should be a user setting being somewhat sensitive?

No, because a user can set the number of iterations today if they build 
their own SCRAM secret. I think it's OK if they change it in a session.

If a superuser wants to enforce a minimum iteration count, they can 
write a password_check_hook. (Or we could add another GUC to enforce that).

> The default in this version is rolled back to 4096 as there was pushback
> against raising it, and the lower limit is one in order to potentially assist
> situations like the one Andres mentioned where md5 is used.

Reviewing patch as is.

Suggestion on text:

==snip==
The number of computational iterations to perform when generating
a SCRAM-SHA-256 secret. The default is <literal>4096</literal>. A
higher number of iterations provides additional protection against
brute-force attacks on stored passwords, but makes authentication
slower. Changing the value has no effect on previously created
SCRAM-SHA-256 secrets as the iteration count at the time of creation
is fixed. A password must be re-hashed to use an updated iteration
value.
==snip==

  /*
- * Default number of iterations when generating secret.  Should be at least
- * 4096 per RFC 7677.
+ * Default number of iterations when generating secret.
   */

I don't think we should remove the RFC 7677 reference entirely. Perhaps:

/*
  * Default number of iterations when generating secret. RFC 7677
  * recommend 4096 for SCRAM-SHA-256, which we set as the default,
  * but we allow users to select their own values.
  */


-pg_fe_scram_build_secret(const char *password, const char **errstr)
+pg_fe_scram_build_secret(const char *password, int iterations, const 
char **errstr)

I have mild worry about changing this function definition for downstream 
usage, esp. for drivers. Perhaps it's not that big of a deal, and 
perhaps this will end up being needed for the work we've discussed 
around "\password" but I do want to note that this could be a breaking 
change.


+    else if (strcmp(name, "scram_sha256_iterations") == 0)
+    {
+        conn->scram_iterations = atoi(value);
+    }

Maybe out of scope for this patch based on what else is in the patch, 
but I was wondering why we don't use a "strncmp" here?

Thanks,

Jonathan

On Wed, Dec 14, 2022 at 01:59:04PM -0500, Jonathan S. Katz wrote:
> On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>> I was thinking about it but opted for the simpler approach of a GUC name with
>> the algorithm baked into it: scram_sha256_iterations.  It doesn't seem all that
>> likely that we'll have more than two versions of SCRAM (sha256/sha512) so
>> the additional complexity doesn't seem worth it.
>
> I would not rule this out. There is a RFC draft for SCRAM-SHA3-512[1].
>
> I do have mixed feelings on the 'x1=y1,x2=y2' style GUC, but we do have
> machinery to handle it and it gives a bit more flexibility over how many
> SCRAM hash methods get added. I'd like to hear more feedback.

Technically, I would put the logic to parse the GUC to scram-common.c
and let libpq and the backend use it.  Saying that, we are just
talking about what looks like one new hashing method, so a separate
GUC is fine by me.

> (I don't know if there will be a world if we ever let users BYO-hash, but
> that case may force separate GUCs anyway).
>
> [1] https://datatracker.ietf.org/doc/draft-melnikov-scram-sha3-512/

Still, the odds is that we are going to see one update to
SCRAM-SHA-256 that we will just need to pick up?

>> The attached v2 has the GUC rename and a change to GUC_REPORT such that the
>> frontend can use the real value rather than the default.  I kept it for super
>> users so far, do you think it should be a user setting being somewhat sensitive?
>
> No, because a user can set the number of iterations today if they build
> their own SCRAM secret. I think it's OK if they change it in a session.
>
> If a superuser wants to enforce a minimum iteration count, they can write a
> password_check_hook. (Or we could add another GUC to enforce that).

Hm?  check_password_hook does not allow one to recompile the password
given by the user, except if I am missing your point?

> -pg_fe_scram_build_secret(const char *password, const char **errstr)
> +pg_fe_scram_build_secret(const char *password, int iterations, const char
> **errstr)
>
> I have mild worry about changing this function definition for downstream
> usage, esp. for drivers. Perhaps it's not that big of a deal, and perhaps
> this will end up being needed for the work we've discussed around
> "\password" but I do want to note that this could be a breaking change.

FWIW, an extension would be required to enforce the type of hash
used, which is an extra parameter on top of the iteration number when
building the SCRAM verifier.

> +    else if (strcmp(name, "scram_sha256_iterations") == 0)
> +    {
> +        conn->scram_iterations = atoi(value);
> +    }
>
> Maybe out of scope for this patch based on what else is in the patch, but I
> was wondering why we don't use a "strncmp" here?

What would that change?  This needs an equal match.

    conn->in_hot_standby = PG_BOOL_UNKNOWN;
+   conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;

s/SCRAM_DEFAULT_ITERATIONS/SCRAM_SHA_256_DEFAULT_ITERATIONS/ and
s/scram_iterations/scram_sha_256_interations/ perhaps?  It does not
look like we'd have the same default across the various SHA variations
if we stick with the RFC definitions..

+#ifndef FRONTEND
+/*
+ * Number of iterations when generating new secrets.
+ */
+extern PGDLLIMPORT int scram_sha256_iterations;
+#endif

It looks like libpq/scram.h, which is backend-only, would be a better
location.

@@ -692,7 +697,7 @@ mock_scram_secret(const char *username, int *iterations, char **salt,
    encoded_salt[encoded_len] = '\0';

    *salt = encoded_salt;
-   *iterations = SCRAM_DEFAULT_ITERATIONS;
+   *iterations = scram_sha256_iterations;

This looks incorrect to me?  The mock authentication is here to
produce a realistic verifier, still it will fail.  It seems to me that
we'd better stick to the default in all the cases.

(FWIW, extending \password with custom options would have the
advantage to allow older server versions to use a custom iteration
number.  Perhaps that's not worth bothering about, just saying as a
separate thing to consider.)
--
Michael

On 12/14/22 6:52 PM, Michael Paquier wrote:
> On Wed, Dec 14, 2022 at 01:59:04PM -0500, Jonathan S. Katz wrote:
HA-256 that we will just need to pick up?
> 
>>> The attached v2 has the GUC rename and a change to GUC_REPORT such that the
>>> frontend can use the real value rather than the default.  I kept it for super
>>> users so far, do you think it should be a user setting being somewhat sensitive?
>>
>> No, because a user can set the number of iterations today if they build
>> their own SCRAM secret. I think it's OK if they change it in a session.
>>
>> If a superuser wants to enforce a minimum iteration count, they can write a
>> password_check_hook. (Or we could add another GUC to enforce that).
> 
> Hm?  check_password_hook does not allow one to recompile the password
> given by the user, except if I am missing your point?
My point is you can write a hook to reject the password if the iteration 
count is "too low". Not to re-hash the password.

Thanks,

Jonathan

> On 14 Dec 2022, at 19:59, Jonathan S. Katz <jkatz@postgresql.org> wrote:
> On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>>> On 14 Dec 2022, at 02:00, Michael Paquier <michael@paquier.xyz> wrote:

>>> So, you mean that the GUC should be named like password_iterations,
>>> taking a grammar with a list like 'scram-sha-256=4096,algo2=5000'?
>> I was thinking about it but opted for the simpler approach of a GUC name with
>> the algorithm baked into it: scram_sha256_iterations.  It doesn't seem all that
>> likely that we'll have more than two versions of SCRAM (sha256/sha512) so
>> the additional complexity doesn't seem worth it.
>
> I would not rule this out. There is a RFC draft for SCRAM-SHA3-512[1].

Note that this draft is very far from RFC status, it has alredy expired twice
and hasn't been updated for a year.  The SCRAM-SHA-512 draft has an almost
identical history and neither are assigned a work group.  The author is also
drafting scram-bis which is setting up more context around these proposals,
this has yet to expire but is also very early.  The work on SCRAM-2FA seems the
most promising right now.

There might be additional versions of SCRAM published but it's looking pretty
distant now.

> Reviewing patch as is.

Thanks for review! Fixes coming downthread in an updated version.

> ==snip==
> The number of computational iterations to perform when generating
> a SCRAM-SHA-256 secret. The default is <literal>4096</literal>. A
> higher number of iterations provides additional protection against
> brute-force attacks on stored passwords, but makes authentication
> slower. Changing the value has no effect on previously created
> SCRAM-SHA-256 secrets as the iteration count at the time of creation
> is fixed. A password must be re-hashed to use an updated iteration
> value.
> ==snip==

I've rewritten to a version of this.  We don't use the terminology "SCRAM
secret" anywhere else so I used password instead.

> /*
> - * Default number of iterations when generating secret.  Should be at least
> - * 4096 per RFC 7677.
> + * Default number of iterations when generating secret.
>  */
>
> I don't think we should remove the RFC 7677 reference entirely.

Fixed.

> -pg_fe_scram_build_secret(const char *password, const char **errstr)
> +pg_fe_scram_build_secret(const char *password, int iterations, const char **errstr)
>
> I have mild worry about changing this function definition for downstream usage, esp. for drivers. Perhaps it's not
thatbig of a deal, and perhaps this will end up being needed for the work we've discussed around "\password" but I do
wantto note that this could be a breaking change. 

Not sure driver authors should be relying on this function..  Code scans
doesn't turn up any public consumers of it right now at least.  If we want to
support multiple SCRAM versions we'd still need to change it though as noted
downthread.

> +    else if (strcmp(name, "scram_sha256_iterations") == 0)
> +    {
> +        conn->scram_iterations = atoi(value);
> +    }
>
> Maybe out of scope for this patch based on what else is in the patch, but I was wondering why we don't use a
"strncmp"here? 

strncmp() would allow scram_sha256_iterations_foo to match, which we don't
want, we want an exact match.

--
Daniel Gustafsson        https://vmware.com/

> On 15 Dec 2022, at 00:52, Michael Paquier <michael@paquier.xyz> wrote:

>    conn->in_hot_standby = PG_BOOL_UNKNOWN;
> +   conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;
>
> s/SCRAM_DEFAULT_ITERATIONS/SCRAM_SHA_256_DEFAULT_ITERATIONS/ and
> s/scram_iterations/scram_sha_256_interations/ perhaps?

Distinct members in the conn object is only of interest if there is a way for
the user to select a different password method in \password right?  I can
rename it now but I think doing too much here is premature, awaiting work on
\password (should that materialize) seems reasonable no?

> +#ifndef FRONTEND
> +/*
> + * Number of iterations when generating new secrets.
> + */
> +extern PGDLLIMPORT int scram_sha256_iterations;
> +#endif
>
> It looks like libpq/scram.h, which is backend-only, would be a better
> location.

Fixed.

> @@ -692,7 +697,7 @@ mock_scram_secret(const char *username, int *iterations, char **salt,
>    encoded_salt[encoded_len] = '\0';
>
>    *salt = encoded_salt;
> -   *iterations = SCRAM_DEFAULT_ITERATIONS;
> +   *iterations = scram_sha256_iterations;
>
> This looks incorrect to me?  The mock authentication is here to
> produce a realistic verifier, still it will fail.  It seems to me that
> we'd better stick to the default in all the cases.

For avoiding revealing anything, I think a case can be argued for both.  I've
reverted back to the default though.

I also renamed the GUC sha_256 to match terminology we use.

--
Daniel Gustafsson        https://vmware.com/

On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote:
>> On 15 Dec 2022, at 00:52, Michael Paquier <michael@paquier.xyz> wrote:
>>    conn->in_hot_standby = PG_BOOL_UNKNOWN;
>> +   conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;
>>
>> s/SCRAM_DEFAULT_ITERATIONS/SCRAM_SHA_256_DEFAULT_ITERATIONS/ and
>> s/scram_iterations/scram_sha_256_interations/ perhaps?
>
> Distinct members in the conn object is only of interest if there is a way for
> the user to select a different password method in \password right?  I can
> rename it now but I think doing too much here is premature, awaiting work on
> \password (should that materialize) seems reasonable no?

You could do that already, somewhat indirectly, with
password_encryption, assuming that it supports more than one mode
whose password build is influenced by it.  If you wish to keep it
named this way, this is no big deal for me either way, so feel free to
use what you think is best based on the state of HEAD.  I think that
I'd value more the consistency with the backend in terms of naming,
though.

>> @@ -692,7 +697,7 @@ mock_scram_secret(const char *username, int *iterations, char **salt,
>>    encoded_salt[encoded_len] = '\0';
>>
>>    *salt = encoded_salt;
>> -   *iterations = SCRAM_DEFAULT_ITERATIONS;
>> +   *iterations = scram_sha256_iterations;
>>
>> This looks incorrect to me?  The mock authentication is here to
>> produce a realistic verifier, still it will fail.  It seems to me that
>> we'd better stick to the default in all the cases.
>
> For avoiding revealing anything, I think a case can be argued for both.  I've
> reverted back to the default though.
>
> I also renamed the GUC sha_256 to match terminology we use.

+   "SET password_encryption='scram-sha-256';
+    SET scram_sha_256_iterations=100000;
Maybe use a lower value to keep the test cheap?

+        time of encryption. In order to make use of a changed value, new
+        password must be set.
"A new password must be set".

Superuser-only GUCs should be documented as such, or do you intend to
make it user-settable like I suggested upthread :) ?
--
Michael

> On 17 Dec 2022, at 04:27, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote:
>>> On 15 Dec 2022, at 00:52, Michael Paquier <michael@paquier.xyz> wrote:
>>>   conn->in_hot_standby = PG_BOOL_UNKNOWN;
>>> +   conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;
>>>
>>> s/SCRAM_DEFAULT_ITERATIONS/SCRAM_SHA_256_DEFAULT_ITERATIONS/ and
>>> s/scram_iterations/scram_sha_256_interations/ perhaps?
>>
>> Distinct members in the conn object is only of interest if there is a way for
>> the user to select a different password method in \password right?  I can
>> rename it now but I think doing too much here is premature, awaiting work on
>> \password (should that materialize) seems reasonable no?
>
> You could do that already, somewhat indirectly, with
> password_encryption, assuming that it supports more than one mode
> whose password build is influenced by it.  If you wish to keep it
> named this way, this is no big deal for me either way, so feel free to
> use what you think is best based on the state of HEAD.  I think that
> I'd value more the consistency with the backend in terms of naming,
> though.

ok, renamed.

>>> @@ -692,7 +697,7 @@ mock_scram_secret(const char *username, int *iterations, char **salt,
>>>   encoded_salt[encoded_len] = '\0';
>>>
>>>   *salt = encoded_salt;
>>> -   *iterations = SCRAM_DEFAULT_ITERATIONS;
>>> +   *iterations = scram_sha256_iterations;
>>>
>>> This looks incorrect to me?  The mock authentication is here to
>>> produce a realistic verifier, still it will fail.  It seems to me that
>>> we'd better stick to the default in all the cases.
>>
>> For avoiding revealing anything, I think a case can be argued for both.  I've
>> reverted back to the default though.
>>
>> I also renamed the GUC sha_256 to match terminology we use.
>
> +   "SET password_encryption='scram-sha-256';
> +    SET scram_sha_256_iterations=100000;
> Maybe use a lower value to keep the test cheap?

Fixed.

> +        time of encryption. In order to make use of a changed value, new
> +        password must be set.
> "A new password must be set".

Fixed.

> Superuser-only GUCs should be documented as such, or do you intend to
> make it user-settable like I suggested upthread :) ?

I don't really have strong feelings, so I reverted to being user-settable since
I can't really present a strong argument for superuser-only.

The attached is a rebase on top of master with no other additional hacking done
on top of the above review comments.

--
Daniel Gustafsson

On 2/22/23 8:39 AM, Daniel Gustafsson wrote:
>> On 17 Dec 2022, at 04:27, Michael Paquier <michael@paquier.xyz> wrote:
>>

>> Superuser-only GUCs should be documented as such, or do you intend to
>> make it user-settable like I suggested upthread :) ?
> 
> I don't really have strong feelings, so I reverted to being user-settable since
> I can't really present a strong argument for superuser-only.

I was going to present some weak arguments, but not worth it. Anything 
around using up CPU cycles would be true of just writing plain old queries.

> The attached is a rebase on top of master with no other additional hacking done
> on top of the above review comments.

Generally LGTM. I read through earlier comments (sorry I missed 
replying) and have nothing to add or object to.

Jonathan

> On 22 Feb 2023, at 18:21, Jonathan S. Katz <jkatz@postgresql.org> wrote:
> On 2/22/23 8:39 AM, Daniel Gustafsson wrote:

>> The attached is a rebase on top of master with no other additional hacking done
>> on top of the above review comments.
>
> Generally LGTM. I read through earlier comments (sorry I missed replying) and have nothing to add or object to.

Thanks for reviewing!

In fixing the CFBot test error in the previous version I realized through
off-list discussion that the GUC name was badly chosen.  Incorporating the
value of another GUC in the name is a bad idea, so the attached version reverts
to "scram_iterations=<int>".  Should there ever be another SCRAM method
standardized (which seems a slim chance to happen before the v17 freeze) we can
make a backwards compatible change to "<method>:<iterations> | <iterations>"
where the latter is a default for all.  Internally the variable contains
sha_256 though, that part I think is fine for readability.

--
Daniel Gustafsson

On Thu, Feb 23, 2023 at 03:10:05PM +0100, Daniel Gustafsson wrote:
> In fixing the CFBot test error in the previous version I realized through
> off-list discussion that the GUC name was badly chosen.  Incorporating the
> value of another GUC in the name is a bad idea, so the attached version reverts
> to "scram_iterations=<int>".  Should there ever be another SCRAM method
> standardized (which seems a slim chance to happen before the v17 freeze) we can
> make a backwards compatible change to "<method>:<iterations> | <iterations>"
> where the latter is a default for all.  Internally the variable contains
> sha_256 though, that part I think is fine for readability.

Okay by me if you want to go this way.  We could always have the
compatibility argument later on if it proves necessary.

Anyway, the patch does that in libpq:
@@ -1181,6 +1181,10 @@ pqSaveParameterStatus(PGconn *conn, const char *name, const char *value)
        conn->in_hot_standby =
            (strcmp(value, "on") == 0) ? PG_BOOL_YES : PG_BOOL_NO;
    }
+   else if (strcmp(name, "scram_sha_256_iterations") == 0)
+   {
+       conn->scram_sha_256_iterations = atoi(value);
+   }
This should match on "scram_iterations", which is the name of the
GUC.  Would the long-term plan be to use multiple variables in conn if
we ever get to <method>:<iterations> that would require more parsing?
This is fine by me, just asking.

Perhaps there should be a test with \password to make sure that libpq
gets the call when the GUC is updated by a SET command?
--
Michael

> On 27 Feb 2023, at 08:06, Michael Paquier <michael@paquier.xyz> wrote:

> +       conn->scram_sha_256_iterations = atoi(value);
> +   }
> This should match on "scram_iterations", which is the name of the
> GUC.

Fixed.

> Would the long-term plan be to use multiple variables in conn if
> we ever get to <method>:<iterations> that would require more parsing?

I personally don't think we'll see more than 2 or at most 3 values so parsing
that format shouldn't be a problem, but it can always be revisited if/when we
get there.

> Perhaps there should be a test with \password to make sure that libpq
> gets the call when the GUC is updated by a SET command?

That would indeed be nice, but is there a way to do this without a complicated
pump TAP expression?  I was unable to think of a way but I might be missing
something?

--
Daniel Gustafsson

On Fri, Mar 03, 2023 at 11:13:36PM +0100, Daniel Gustafsson wrote:
> That would indeed be nice, but is there a way to do this without a complicated
> pump TAP expression?  I was unable to think of a way but I might be missing
> something?

A SET command refreshes immediately the cache information of the
connection in pqSaveParameterStatus()@libpq, so a test in password.sql
with \password would be enough to check the computation happens in
pg_fe_scram_build_secret() with the correct iteration number.  Say
like:
=# SET scram_iterations = 234;
SET
=# \password
Enter new password for user "postgres": TYPEME
Enter it again: TYPEME
=# select substr(rolpassword, 1, 18) from pg_authid
     where oid::regrole::name = current_role;
       substr
--------------------
 SCRAM-SHA-256$234:
(1 row)

Or perhaps I am missing something?

Thanks,
--
Michael

> On 7 Mar 2023, at 05:53, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Mar 03, 2023 at 11:13:36PM +0100, Daniel Gustafsson wrote:
>> That would indeed be nice, but is there a way to do this without a complicated
>> pump TAP expression?  I was unable to think of a way but I might be missing
>> something?
>
> A SET command refreshes immediately the cache information of the
> connection in pqSaveParameterStatus()@libpq, so a test in password.sql
> with \password would be enough to check the computation happens in
> pg_fe_scram_build_secret() with the correct iteration number.  Say
> like:
> =# SET scram_iterations = 234;
> SET
> =# \password
> Enter new password for user "postgres": TYPEME
> Enter it again: TYPEME
> =# select substr(rolpassword, 1, 18) from pg_authid
>     where oid::regrole::name = current_role;
>       substr
> --------------------
> SCRAM-SHA-256$234:
> (1 row)
>
> Or perhaps I am missing something?

Right, what I meant was: can a pg_regress sql/expected test drive a psql
interactive prompt?  Your comments suggested using password.sql so I was
curious if I was missing a neat trick for doing this.

--
Daniel Gustafsson

> On 7 Mar 2023, at 09:26, Daniel Gustafsson <daniel@yesql.se> wrote:

> Right, what I meant was: can a pg_regress sql/expected test drive a psql
> interactive prompt?  Your comments suggested using password.sql so I was
> curious if I was missing a neat trick for doing this.

The attached v7 adds a TAP test for verifying that \password use the changed
SCRAM iteration count setting, and dials back the other added test to use fewer
iterations than the default setting in order to shave (barely noticeable
amounts of) cpu cycles.

Running interactive tests against psql adds a fair bit of complexity and isn't
all that pleasing on the eye, but it can be cleaned up and refactored when
https://commitfest.postgresql.org/42/4228/ is committed.

--
Daniel Gustafsson

On Tue, Mar 07, 2023 at 02:03:05PM +0100, Daniel Gustafsson wrote:
> On 7 Mar 2023, at 09:26, Daniel Gustafsson <daniel@yesql.se> wrote:
>> Right, what I meant was: can a pg_regress sql/expected test drive a psql
>> interactive prompt?  Your comments suggested using password.sql so I was
>> curious if I was missing a neat trick for doing this.

Yes, I meant to rely just on password.sql to do that.  I think that I
see your point now..  You are worried that the SET command changing a
GUC to-be-reported would not affect the client before \password is
done.  That could be possible, I guess.  ReportChangedGUCOptions() is
called before ReadyForQuery() that would tell psql that the backend is
ready to receive the next query.  A trick would be to stick an extra
dummy query between the SET and \password in password.sql?

> Running interactive tests against psql adds a fair bit of complexity and isn't
> all that pleasing on the eye, but it can be cleaned up and refactored when
> https://commitfest.postgresql.org/42/4228/ is committed.

I have not looked at that, so no idea.
--
Michael

> On 8 Mar 2023, at 08:48, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Mar 07, 2023 at 02:03:05PM +0100, Daniel Gustafsson wrote:
>> On 7 Mar 2023, at 09:26, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> Right, what I meant was: can a pg_regress sql/expected test drive a psql
>>> interactive prompt?  Your comments suggested using password.sql so I was
>>> curious if I was missing a neat trick for doing this.
>
> Yes, I meant to rely just on password.sql to do that.  I think that I
> see your point now..  You are worried that the SET command changing a
> GUC to-be-reported would not affect the client before \password is
> done.

No, I just did not think it was possible to feed input to the interactive
\password prompt with a normal pg_regress SQL file test.  If you are able to do
that I'd love to see an example.

AFAIK a TAP test with psql_interactive is the only way to do this so that's
what I've implemented.

--
Daniel Gustafsson

On Wed, Mar 08, 2023 at 09:07:36AM +0100, Daniel Gustafsson wrote:
> No, I just did not think it was possible to feed input to the interactive
> \password prompt with a normal pg_regress SQL file test.  If you are able to do
> that I'd love to see an example.
>
> AFAIK a TAP test with psql_interactive is the only way to do this so that's
> what I've implemented.

Bah, of course.  I was really not following your point here, sorry for
the noise.  Better to call it a day..
--
Michael

On Wed, Mar 08, 2023 at 05:21:20PM +0900, Michael Paquier wrote:
> On Wed, Mar 08, 2023 at 09:07:36AM +0100, Daniel Gustafsson wrote:
>> AFAIK a TAP test with psql_interactive is the only way to do this so that's
>> what I've implemented.

I cannot think of a better idea than what you have here, so I am
marking this patch as ready for committer.  I am wondering how stable
a logic based on a timer of 5s would be..
--
Michael

> On 9 Mar 2023, at 08:09, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Mar 08, 2023 at 05:21:20PM +0900, Michael Paquier wrote:
>> On Wed, Mar 08, 2023 at 09:07:36AM +0100, Daniel Gustafsson wrote:
>>> AFAIK a TAP test with psql_interactive is the only way to do this so that's
>>> what I've implemented.
>
> I cannot think of a better idea than what you have here, so I am
> marking this patch as ready for committer.

Thanks for review!

> I am wondering how stable a logic based on a timer of 5s would be..

Actually that was a bug, it should be using the default timeout and restarting
for each operation to ensure that even overloaded hosts wont time out unless
something is actually broken/incorrect.  I've fixed that in the attached rev
and also renamed the password in the regress test from "raisediterationcount"
as it's now lowering the count in the test.

Unless there objections to this version I plan to commit that during this CF.

--
Daniel Gustafsson

CFBot is failing with this test failure... I'm not sure  if this just
represents a timing dependency or a bad test or what?

[09:44:49.937] --- stdout ---
[09:44:49.937] # executing test in
/tmp/cirrus-ci-build/build-32/testrun/authentication/001_password
group authentication test 001_password
[09:44:49.937] ok 1 - scram_iterations in server side ROLE
[09:44:49.937] # test failed
[09:44:49.937] --- stderr ---
[09:44:49.937] # Tests were run but no plan was declared and
done_testing() was not seen.
[09:44:49.937] # Looks like your test exited with 2 just after 1.
[09:44:49.937]
[09:44:49.937] (test program exited with status code 2)

It looks like perhaps a Perl issue?

# Running: /tmp/cirrus-ci-build/build-32/src/test/regress/pg_regress
--config-auth
/tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/data/t_001_password_primary_data/pgdata
### Starting node "primary"
# Running: pg_ctl -w -D
/tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/data/t_001_password_primary_data/pgdata
-l /tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/log/001_password_primary.log
-o --cluster-name=primary start
waiting for server to start.... done
server started
# Postmaster PID for node "primary" is 66930
[09:44:07.411](1.875s) ok 1 - scram_iterations in server side ROLE
Can't locate IO/Pty.pm in @INC (you may need to install the IO::Pty
module) (@INC contains: /tmp/cirrus-ci-build/src/test/perl
/tmp/cirrus-ci-build/src/test/authentication /etc/perl
/usr/local/lib/i386-linux-gnu/perl/5.32.1 /usr/local/share/perl/5.32.1
/usr/lib/i386-linux-gnu/perl5/5.32 /usr/share/perl5
/usr/lib/i386-linux-gnu/perl/5.32 /usr/share/perl/5.32
/usr/local/lib/site_perl) at /usr/share/perl5/IPC/Run.pm line 1828.
Unexpected SCALAR(0x5814b508) in harness() parameter 3 at
/tmp/cirrus-ci-build/src/test/perl/PostgreSQL/Test/Cluster.pm line
2112.
Can't locate IO/Pty.pm in @INC (you may need to install the IO::Pty
module) (@INC contains: /tmp/cirrus-ci-build/src/test/perl
/tmp/cirrus-ci-build/src/test/authentication /etc/perl
/usr/local/lib/i386-linux-gnu/perl/5.32.1 /usr/local/share/perl/5.32.1
/usr/lib/i386-linux-gnu/perl5/5.32 /usr/share/perl5
/usr/lib/i386-linux-gnu/perl/5.32 /usr/share/perl/5.32
/usr/local/lib/site_perl) at /usr/share/perl5/IPC/Run.pm line 1939.
# Postmaster PID for node "primary" is 66930
### Stopping node "primary" using mode immediate
# Running: pg_ctl -D
/tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/data/t_001_password_primary_data/pgdata
-m immediate stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "primary"
[09:44:07.521](0.110s) # Tests were run but no plan was declared and
done_testing() was not seen.
[09:44:07.521](0.000s) # Looks like your test exited with 2 just after 1.

On Tue, 14 Mar 2023 at 14:54, Gregory Stark (as CFM)
<stark.cfm@gmail.com> wrote:
>
> CFBot is failing with this test failure... I'm not sure  if this just
> represents a timing dependency or a bad test or what?

CFBot is now consistently showing these test failures. I think there
might actually be a problem here?


> [09:44:49.937] --- stdout ---
> [09:44:49.937] # executing test in
> /tmp/cirrus-ci-build/build-32/testrun/authentication/001_password
> group authentication test 001_password
> [09:44:49.937] ok 1 - scram_iterations in server side ROLE
> [09:44:49.937] # test failed
> [09:44:49.937] --- stderr ---
> [09:44:49.937] # Tests were run but no plan was declared and
> done_testing() was not seen.
> [09:44:49.937] # Looks like your test exited with 2 just after 1.
> [09:44:49.937]
> [09:44:49.937] (test program exited with status code 2)
>
> It looks like perhaps a Perl issue?
>
> # Running: /tmp/cirrus-ci-build/build-32/src/test/regress/pg_regress
> --config-auth
/tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/data/t_001_password_primary_data/pgdata
> ### Starting node "primary"
> # Running: pg_ctl -w -D
> /tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/data/t_001_password_primary_data/pgdata
> -l /tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/log/001_password_primary.log
> -o --cluster-name=primary start
> waiting for server to start.... done
> server started
> # Postmaster PID for node "primary" is 66930
> [09:44:07.411](1.875s) ok 1 - scram_iterations in server side ROLE
> Can't locate IO/Pty.pm in @INC (you may need to install the IO::Pty
> module) (@INC contains: /tmp/cirrus-ci-build/src/test/perl
> /tmp/cirrus-ci-build/src/test/authentication /etc/perl
> /usr/local/lib/i386-linux-gnu/perl/5.32.1 /usr/local/share/perl/5.32.1
> /usr/lib/i386-linux-gnu/perl5/5.32 /usr/share/perl5
> /usr/lib/i386-linux-gnu/perl/5.32 /usr/share/perl/5.32
> /usr/local/lib/site_perl) at /usr/share/perl5/IPC/Run.pm line 1828.
> Unexpected SCALAR(0x5814b508) in harness() parameter 3 at
> /tmp/cirrus-ci-build/src/test/perl/PostgreSQL/Test/Cluster.pm line
> 2112.
> Can't locate IO/Pty.pm in @INC (you may need to install the IO::Pty
> module) (@INC contains: /tmp/cirrus-ci-build/src/test/perl
> /tmp/cirrus-ci-build/src/test/authentication /etc/perl
> /usr/local/lib/i386-linux-gnu/perl/5.32.1 /usr/local/share/perl/5.32.1
> /usr/lib/i386-linux-gnu/perl5/5.32 /usr/share/perl5
> /usr/lib/i386-linux-gnu/perl/5.32 /usr/share/perl/5.32
> /usr/local/lib/site_perl) at /usr/share/perl5/IPC/Run.pm line 1939.
> # Postmaster PID for node "primary" is 66930
> ### Stopping node "primary" using mode immediate
> # Running: pg_ctl -D
> /tmp/cirrus-ci-build/build-32/testrun/authentication/001_password/data/t_001_password_primary_data/pgdata
> -m immediate stop
> waiting for server to shut down.... done
> server stopped
> # No postmaster PID for node "primary"
> [09:44:07.521](0.110s) # Tests were run but no plan was declared and
> done_testing() was not seen.
> [09:44:07.521](0.000s) # Looks like your test exited with 2 just after 1.



-- 
Gregory Stark
As Commitfest Manager

> On 22 Mar 2023, at 04:14, Gregory Stark (as CFM) <stark.cfm@gmail.com> wrote:
> On Tue, 14 Mar 2023 at 14:54, Gregory Stark (as CFM)
> <stark.cfm@gmail.com> wrote:
>>
>> CFBot is failing with this test failure... I'm not sure  if this just
>> represents a timing dependency or a bad test or what?
>
> CFBot is now consistently showing these test failures. I think there
> might actually be a problem here?

I'm fairly convinced it's a timeout in the interactive psql session.  Given how
ugly the use of that is I'm sort of waiting for Andres' refactoring patch [0] to
commit this such that I can rewrite the test in a saner and more robust way.

--
Daniel Gustafsson

[0] https://commitfest.postgresql.org/42/4228/

On Thu, Mar 23, 2023 at 10:46:56PM +0100, Daniel Gustafsson wrote:
> I'm fairly convinced it's a timeout in the interactive psql session.  Given how
> ugly the use of that is I'm sort of waiting for Andres' refactoring patch [0] to
> commit this such that I can rewrite the test in a saner and more robust way.

FWIW, I'd be OK here even if you don't have a test for libpq in the
first change as what you have sent is already testing for the core
machinery in scram-common.c.  You could always add one later.
--
Michael

> On 24 Mar 2023, at 00:33, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Mar 23, 2023 at 10:46:56PM +0100, Daniel Gustafsson wrote:
>> I'm fairly convinced it's a timeout in the interactive psql session.  Given how
>> ugly the use of that is I'm sort of waiting for Andres' refactoring patch [0] to
>> commit this such that I can rewrite the test in a saner and more robust way.
>
> FWIW, I'd be OK here even if you don't have a test for libpq in the
> first change as what you have sent is already testing for the core
> machinery in scram-common.c.  You could always add one later.

Yeah, that's my fallback in case we are unable to get the TAP refactoring done
in time for the end of the CF/feature freeze.

I've actually ripped out the test in question in the attached v9 to have it
ready and building green in CFbot.

--
Daniel Gustafsson

On Fri, Mar 24, 2023 at 09:56:29AM +0100, Daniel Gustafsson wrote:
> I've actually ripped out the test in question in the attached v9 to have it
> ready and building green in CFbot.

While reading through v9, I have noticed a few things.

+-- Changing the SCRAM iteration count
+SET scram_iterations = 1024;
+CREATE ROLE regress_passwd9 PASSWORD 'alterediterationcount';

Perhaps scram_iterations should be reset once this CREATE ROLE is run
to not impact any tests after that?

+/*
+ * The number of iterations to use when generating new secrets.
+ */
+int            scram_sha_256_iterations;

This variable in auth-scram.c should be initialized to
SCRAM_SHA_256_DEFAULT_ITERATIONS.

+use IPC::Run qw(pump finish timer);

This can be removed.
--
Michael

> On 25 Mar 2023, at 01:56, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Mar 24, 2023 at 09:56:29AM +0100, Daniel Gustafsson wrote:
>> I've actually ripped out the test in question in the attached v9 to have it
>> ready and building green in CFbot.
>
> While reading through v9, I have noticed a few things.

The attached rebase fixes all of these comments, and features a slightly
reworded commit message.  I plan to go ahead with this tomorrow to close the CF
patch item.

--
Daniel Gustafsson

On Sun, Mar 26, 2023 at 11:14:37PM +0200, Daniel Gustafsson wrote:
> > On 25 Mar 2023, at 01:56, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Fri, Mar 24, 2023 at 09:56:29AM +0100, Daniel Gustafsson wrote:
> >> I've actually ripped out the test in question in the attached v9 to have it
> >> ready and building green in CFbot.
> >
> > While reading through v9, I have noticed a few things.
>
> The attached rebase fixes all of these comments, and features a slightly
> reworded commit message.  I plan to go ahead with this tomorrow to close the CF
> patch item.

Looks OK by me.

+   "SELECT substr(rolpassword,1,19)
I would have perhaps used a regexp_replace() for that.  What you have
here is of course fine, so feel free to ignore :p
--
Michael