Re: Raising the SCRAM iteration count
> On 17 Dec 2022, at 04:27, Michael Paquier wrote: > > On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote: >>> On 15 Dec 2022, at 00:52, Michael Paquier 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=10; > 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 v4-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
On 2/22/23 8:39 AM, Daniel Gustafsson wrote: On 17 Dec 2022, at 04:27, Michael Paquier 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 OpenPGP_signature Description: OpenPGP digital signature
Re: Raising the SCRAM iteration count
> On 22 Feb 2023, at 18:21, Jonathan S. Katz 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=". 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 ": | " 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 v5-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
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=". 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 ": | " > 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 : 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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 27 Feb 2023, at 08:06, Michael Paquier 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 : 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 v6-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 7 Mar 2023, at 05:53, Michael Paquier 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
Re: Raising the SCRAM iteration count
> On 7 Mar 2023, at 09:26, Daniel Gustafsson 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 v7-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
On Tue, Mar 07, 2023 at 02:03:05PM +0100, Daniel Gustafsson wrote: > On 7 Mar 2023, at 09:26, Daniel Gustafsson 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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 8 Mar 2023, at 08:48, Michael Paquier wrote: > > On Tue, Mar 07, 2023 at 02:03:05PM +0100, Daniel Gustafsson wrote: >> On 7 Mar 2023, at 09:26, Daniel Gustafsson 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
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 9 Mar 2023, at 08:09, Michael Paquier 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 v8-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
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
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
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
Re: Raising the SCRAM iteration count
> On 10 Dec 2022, at 01:15, Andres Freund 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, 10 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/
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
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 1, 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 1, 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-bba85c2fa...@postgresql.org/ OpenPGP_signature Description: OpenPGP digital signature
Re: Raising the SCRAM iteration count
> On 12 Dec 2022, at 15:47, Jonathan S. Katz 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 1, which was 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/
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 14 Dec 2022, at 02:00, Michael Paquier 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/ v2-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
On 12/14/22 6:25 AM, Daniel Gustafsson wrote: On 14 Dec 2022, at 02:00, Michael Paquier 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 4096. 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 OpenPGP_signature Description: OpenPGP digital signature
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
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 OpenPGP_signature Description: OpenPGP digital signature
Re: Raising the SCRAM iteration count
> On 14 Dec 2022, at 19:59, Jonathan S. Katz wrote: > On 12/14/22 6:25 AM, Daniel Gustafsson wrote: >>> On 14 Dec 2022, at 02:00, Michael Paquier 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 4096. 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 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. 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/
Re: Raising the SCRAM iteration count
> On 15 Dec 2022, at 00:52, Michael Paquier 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/ v3-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote: >> On 15 Dec 2022, at 00:52, Michael Paquier 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=10; 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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
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.
Re: Raising the SCRAM iteration count
On Tue, 14 Mar 2023 at 14:54, Gregory Stark (as CFM) 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
Re: Raising the SCRAM iteration count
> On 22 Mar 2023, at 04:14, Gregory Stark (as CFM) wrote: > On Tue, 14 Mar 2023 at 14:54, Gregory Stark (as CFM) > 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/
Re: Raising the SCRAM iteration count
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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 24 Mar 2023, at 00:33, Michael Paquier 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 v9-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
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. + */ +intscram_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 signature.asc Description: PGP signature
Re: Raising the SCRAM iteration count
> On 25 Mar 2023, at 01:56, Michael Paquier 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 v10-0001-Make-SCRAM-iteration-count-configurable.patch Description: Binary data
Re: Raising the SCRAM iteration count
On Sun, Mar 26, 2023 at 11:14:37PM +0200, Daniel Gustafsson wrote: > > On 25 Mar 2023, at 01:56, Michael Paquier 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 signature.asc Description: PGP signature