public inbox for [email protected]
help / color / mirror / Atom feedRaising the SCRAM iteration count
18+ messages / 6 participants
[nested] [flat]
* Raising the SCRAM iteration count
@ 2022-12-09 10:55 Daniel Gustafsson <[email protected]>
2022-12-09 15:50 ` Re: Raising the SCRAM iteration count Heikki Linnakangas <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: Daniel Gustafsson @ 2022-12-09 10:55 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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] [email protected]
[1] https://www.rfc-editor.org/rfc/rfc7677#section-4
[2] https://www.mongodb.com/docs/manual/reference/parameters/#mongodb-parameter-param.scramSHA256Iterati...
[3] https://docs.confluent.io/platform/current/kafka/authentication_sasl/authentication_sasl_scram.html#...
Attachments:
[application/octet-stream] v1-0001-Make-SCRAM-iteration-count-configurable.patch (11.5K, ../../[email protected]/2-v1-0001-Make-SCRAM-iteration-count-configurable.patch)
download | inline diff:
From 9de52d4408f26a7758f76ddcff0a629522de4ca0 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Thu, 8 Dec 2022 14:49:42 +0100
Subject: [PATCH v1] Make SCRAM iteration count configurable
The current hardcoded value for SCRAM iteration count is defined to be
4096, which is taken from RFC 7677 chapter 4 where it is cited as the
highest number of iterations for mobile units. This data is however 7
years old by know, and most postgres authentications are made on hand-
held devices.
Replace the hardcoded value with a GUC such that the iteration count
can be raised in order to increase protection against brute-force
attacks.
Discussion: https://postgr.es/m/tbd
---
doc/src/sgml/config.sgml | 17 +++++++++++++
src/backend/libpq/auth-scram.c | 9 +++++--
src/backend/utils/misc/guc_tables.c | 12 +++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/common/scram-common.c | 4 +--
src/include/common/scram-common.h | 17 ++++++++++---
src/test/authentication/t/001_password.pl | 16 ++++++++++++
src/test/regress/expected/password.out | 25 +++++++++++--------
src/test/regress/sql/password.sql | 5 ++++
9 files changed, 89 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index ff6fcd902a..2e2e85f6b2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1098,6 +1098,23 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>scram_iteration_count</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>scram_iteration_count</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ The number of computational iterations to be performed when generating
+ a SCRAM secret. The default is <literal>15000</literal>. A higher
+ number of iterations will provide additional protection against
+ brute-force attacks on stored passwords but will make authentication
+ slower.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-krb-server-keyfile" xreflabel="krb_server_keyfile">
<term><varname>krb_server_keyfile</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index ee7f52218a..0df8a96751 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -184,6 +184,11 @@ static char *sanitize_char(char c);
static char *sanitize_str(const char *s);
static char *scram_mock_salt(const char *username);
+/*
+ * The number of iterations to use when generating new secrets.
+ */
+int scram_iteration_count;
+
/*
* Get a list of SASL mechanisms that this module supports.
*
@@ -483,7 +488,7 @@ pg_be_scram_build_secret(const char *password)
errmsg("could not generate random salt")));
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ scram_iteration_count, password,
&errstr);
if (prep_password)
@@ -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_iteration_count;
/* StoredKey and ServerKey are not used in a doomed authentication */
memset(stored_key, 0, SCRAM_KEY_LEN);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..44e8e39424 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -40,6 +40,7 @@
#include "commands/trigger.h"
#include "commands/user.h"
#include "commands/vacuum.h"
+#include "common/scram-common.h"
#include "jit/jit.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
@@ -3423,6 +3424,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"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
+ },
+ &scram_iteration_count,
+ 15000, SCRAM_MIN_ITERATIONS, INT_MAX,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 043864597f..64b7ad6b7a 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -94,6 +94,7 @@
#authentication_timeout = 1min # 1s-600s
#password_encryption = scram-sha-256 # scram-sha-256 or md5
+#scram_iteration_count = 15000
#db_user_namespace = off
# GSSAPI using Kerberos
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index 1268625929..c5a0de37f2 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -206,8 +206,8 @@ scram_build_secret(const char *salt, int saltlen, int iterations,
int encoded_server_len;
int encoded_result;
- if (iterations <= 0)
- iterations = SCRAM_DEFAULT_ITERATIONS;
+ if (iterations <= SCRAM_MIN_ITERATIONS)
+ iterations = SCRAM_MIN_ITERATIONS;
/* Calculate StoredKey and ServerKey */
if (scram_SaltedPassword(password, salt, saltlen, iterations,
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index e1f5e786e0..0922b10052 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -41,10 +41,21 @@
#define SCRAM_DEFAULT_SALT_LEN 16
/*
- * Default number of iterations when generating secret. Should be at least
- * 4096 per RFC 7677.
+ * The minimum allowed number of iterations when generating new secrets.
*/
-#define SCRAM_DEFAULT_ITERATIONS 4096
+#define SCRAM_MIN_ITERATIONS 4096
+
+/*
+ * Default number of iterations when generating secret.
+ */
+#define SCRAM_DEFAULT_ITERATIONS 15000
+
+#ifndef FRONTEND
+/*
+ * Number of iterations when generating new secrets.
+ */
+extern PGDLLIMPORT int scram_iteration_count;
+#endif
extern int scram_SaltedPassword(const char *password, const char *salt,
int saltlen, int iterations, uint8 *result,
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 42d3d4c79b..8870008f05 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -86,6 +86,14 @@ $node->safe_psql('postgres',
q{SET password_encryption='md5'; CREATE ROLE "md5,role" LOGIN PASSWORD 'pass';}
);
+# Create a role with a non-default iteration count
+$node->safe_psql(
+ 'postgres',
+ "SET password_encryption='scram-sha-256';
+ SET scram_iteration_count=100000;
+ CREATE ROLE scram_role_iter LOGIN PASSWORD 'pass';"
+);
+
# Create a database to test regular expression.
$node->safe_psql('postgres', "CREATE database regex_testdb;");
@@ -134,6 +142,14 @@ test_conn(
log_like => [
qr/connection authenticated: identity="scram_role" method=scram-sha-256/
]);
+test_conn(
+ $node,
+ 'user=scram_role_iter',
+ 'scram-sha-256',
+ 0,
+ log_like => [
+ qr/connection authenticated: identity="scram_role_iter" method=scram-sha-256/
+ ]);
test_conn($node, 'user=md5_role', 'scram-sha-256', 2,
log_unlike => [qr/connection authenticated:/]);
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index 7c84c9da33..7252c78ab2 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -28,11 +28,11 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
ORDER BY rolname, rolpassword;
- rolname | rolpassword_masked
------------------+---------------------------------------------------
+ rolname | rolpassword_masked
+-----------------+----------------------------------------------------
regress_passwd1 | md5783277baca28003b33453252be4dbb34
regress_passwd2 | md54044304ba511dd062133eb5b4b84a2a3
- regress_passwd3 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
+ regress_passwd3 | SCRAM-SHA-256$15000:<salt>$<storedkey>:<serverkey>
regress_passwd4 |
(4 rows)
@@ -72,21 +72,25 @@ CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';
CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_iteration_count = 99999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
ORDER BY rolname, rolpassword;
- rolname | rolpassword_masked
------------------+---------------------------------------------------
+ rolname | rolpassword_masked
+-----------------+----------------------------------------------------
regress_passwd1 | md5cd3578025fe2c3d7ed1b9a9b26238b70
regress_passwd2 | md5dfa155cadd5f4ad57860162f3fab9cdb
regress_passwd3 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
- regress_passwd4 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
+ regress_passwd4 | SCRAM-SHA-256$15000:<salt>$<storedkey>:<serverkey>
regress_passwd5 | md5e73a4b11df52a6068f8b39f90be36023
- regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
- regress_passwd7 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
- regress_passwd8 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
-(8 rows)
+ regress_passwd6 | SCRAM-SHA-256$15000:<salt>$<storedkey>:<serverkey>
+ regress_passwd7 | SCRAM-SHA-256$15000:<salt>$<storedkey>:<serverkey>
+ regress_passwd8 | SCRAM-SHA-256$15000:<salt>$<storedkey>:<serverkey>
+ regress_passwd9 | SCRAM-SHA-256$99999:<salt>$<storedkey>:<serverkey>
+(9 rows)
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
@@ -129,6 +133,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql
index 98f49916e5..6672d58d59 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -63,6 +63,10 @@ CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_iteration_count = 99999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
+
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
@@ -97,6 +101,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
--
2.32.1 (Apple Git-133)
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
@ 2022-12-09 15:50 ` Heikki Linnakangas <[email protected]>
2022-12-09 23:21 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Heikki Linnakangas @ 2022-12-09 15:50 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-09 15:50 ` Re: Raising the SCRAM iteration count Heikki Linnakangas <[email protected]>
@ 2022-12-09 23:21 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Michael Paquier @ 2022-12-09 23:21 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; PostgreSQL Hackers <[email protected]>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../Y5PDD+fNfeBpbqd%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
@ 2022-12-10 00:15 ` Andres Freund <[email protected]>
2022-12-10 23:46 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
1 sibling, 2 replies; 18+ messages in thread
From: Andres Freund @ 2022-12-10 00:15 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
@ 2022-12-10 23:46 ` Daniel Gustafsson <[email protected]>
2022-12-11 03:32 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2022-12-10 23:46 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On 10 Dec 2022, at 01:15, Andres Freund <[email protected]> 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/
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-10 23:46 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
@ 2022-12-11 03:32 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Michael Paquier @ 2022-12-11 03:32 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
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 [email protected] 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../Y5VPVlwE%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
@ 2022-12-12 14:47 ` Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Jonathan S. Katz @ 2022-12-12 14:47 UTC (permalink / raw)
To: Andres Freund <[email protected]>; Daniel Gustafsson <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
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/[email protected]/
Attachments:
[application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
@ 2022-12-13 11:17 ` Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2022-12-13 11:17 UTC (permalink / raw)
To: Jonathan S. Katz <[email protected]>; +Cc: Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
> On 12 Dec 2022, at 15:47, Jonathan S. Katz <[email protected]> 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, 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/
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
@ 2022-12-14 01:00 ` Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2022-12-14 01:00 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Jonathan S. Katz <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
@ 2022-12-14 11:25 ` Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2022-12-14 11:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Jonathan S. Katz <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
> On 14 Dec 2022, at 02:00, Michael Paquier <[email protected]> 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/
Attachments:
[application/octet-stream] v2-0001-Make-SCRAM-iteration-count-configurable.patch (14.8K, ../../[email protected]/2-v2-0001-Make-SCRAM-iteration-count-configurable.patch)
download | inline diff:
From 7187bfd83067f277e3658367a895fc6ac5aa53b0 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Wed, 14 Dec 2022 11:52:36 +0100
Subject: [PATCH v2] Make SCRAM iteration count configurable
Replace the hardcoded value with a GUC such that the iteration count
can be raised in order to increase protection against brute-force
attacks. The hardcoded value for SCRAM iteration count was defined
to be 4096, which is taken from RFC 7677, so set the default for the
GUC to 4096 to match. Raising the iteration count will make stored
passwords more resilient to brute-force attacks at the cost of more
computation performed during connection establishment. Lowering the
count will reduce computational overhead during connections at the
tradeoff of reducing strength against brute-force attacks. When the
alternative is to fall back to md5 instead, SCRAM with a very low
iteration count is still preferrable so allow iteration counts all
the way down to one.
Reviewed-by: Michael Paquier <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/config.sgml | 20 +++++++++++++++++++
src/backend/libpq/auth-scram.c | 9 +++++++--
src/backend/utils/misc/guc_tables.c | 12 +++++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/common/scram-common.c | 3 +--
src/include/common/scram-common.h | 10 ++++++++--
src/interfaces/libpq/fe-auth-scram.c | 4 ++--
src/interfaces/libpq/fe-auth.c | 2 +-
src/interfaces/libpq/fe-auth.h | 1 +
src/interfaces/libpq/fe-connect.c | 2 ++
src/interfaces/libpq/fe-exec.c | 4 ++++
src/interfaces/libpq/libpq-int.h | 1 +
src/test/authentication/t/001_password.pl | 16 +++++++++++++++
src/test/regress/expected/password.out | 11 +++++++---
src/test/regress/sql/password.sql | 5 +++++
15 files changed, 89 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8e4145979d..3da29e5a51 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1098,6 +1098,26 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>scram_sha256_iterations</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>scram_sha256_iterations</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ The number of computational iterations to be performed when generating
+ a SCRAM-SHA-256 password. The default is <literal>4096</literal>. A
+ higher number of iterations will provide additional protection against
+ brute-force attacks on stored passwords but will make authentication
+ slower. Changing the value has no effect on already created secrets,
+ since the iteration count at the time of creation is fixed for the
+ secret. In order to make use of a changed value the password must be
+ altered.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-krb-server-keyfile" xreflabel="krb_server_keyfile">
<term><varname>krb_server_keyfile</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index ee7f52218a..4bf377b189 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -184,6 +184,11 @@ static char *sanitize_char(char c);
static char *sanitize_str(const char *s);
static char *scram_mock_salt(const char *username);
+/*
+ * The number of iterations to use when generating new secrets.
+ */
+int scram_sha256_iterations;
+
/*
* Get a list of SASL mechanisms that this module supports.
*
@@ -483,7 +488,7 @@ pg_be_scram_build_secret(const char *password)
errmsg("could not generate random salt")));
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ scram_sha256_iterations, password,
&errstr);
if (prep_password)
@@ -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;
/* StoredKey and ServerKey are not used in a doomed authentication */
memset(stored_key, 0, SCRAM_KEY_LEN);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..9d6acc7fc5 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -40,6 +40,7 @@
#include "commands/trigger.h"
#include "commands/user.h"
#include "commands/vacuum.h"
+#include "common/scram-common.h"
#include "jit/jit.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
@@ -3423,6 +3424,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"scram_sha256_iterations", PGC_SUSET, CONN_AUTH_AUTH,
+ gettext_noop("Sets the iteration count for SCRAM secret generation."),
+ NULL,
+ GUC_REPORT | GUC_SUPERUSER_ONLY
+ },
+ &scram_sha256_iterations,
+ 4096, 1, INT_MAX,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 043864597f..2b2b8ca86f 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -94,6 +94,7 @@
#authentication_timeout = 1min # 1s-600s
#password_encryption = scram-sha-256 # scram-sha-256 or md5
+#scram_sha256_iterations = 4096
#db_user_namespace = off
# GSSAPI using Kerberos
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index 1268625929..2dc9679a4d 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -206,8 +206,7 @@ scram_build_secret(const char *salt, int saltlen, int iterations,
int encoded_server_len;
int encoded_result;
- if (iterations <= 0)
- iterations = SCRAM_DEFAULT_ITERATIONS;
+ Assert(iterations > 0);
/* Calculate StoredKey and ServerKey */
if (scram_SaltedPassword(password, salt, saltlen, iterations,
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 4acf2a78ad..97c63a7f31 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -38,11 +38,17 @@
#define SCRAM_DEFAULT_SALT_LEN 16
/*
- * Default number of iterations when generating secret. Should be at least
- * 4096 per RFC 7677.
+ * Default number of iterations when generating secret.
*/
#define SCRAM_DEFAULT_ITERATIONS 4096
+#ifndef FRONTEND
+/*
+ * Number of iterations when generating new secrets.
+ */
+extern PGDLLIMPORT int scram_sha256_iterations;
+#endif
+
extern int scram_SaltedPassword(const char *password, const char *salt,
int saltlen, int iterations, uint8 *result,
const char **errstr);
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index c500bea9e7..cf5e8824d3 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -882,7 +882,7 @@ verify_server_signature(fe_scram_state *state, bool *match,
* error details.
*/
char *
-pg_fe_scram_build_secret(const char *password, const char **errstr)
+pg_fe_scram_build_secret(const char *password, int iterations, const char **errstr)
{
char *prep_password;
pg_saslprep_rc rc;
@@ -913,7 +913,7 @@ pg_fe_scram_build_secret(const char *password, const char **errstr)
}
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ iterations, password,
errstr);
free(prep_password);
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 4a6c358bb6..52fee8e579 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -1253,7 +1253,7 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
{
const char *errstr = NULL;
- crypt_pwd = pg_fe_scram_build_secret(passwd, &errstr);
+ crypt_pwd = pg_fe_scram_build_secret(passwd, conn->scram_iterations, &errstr);
if (!crypt_pwd)
libpq_append_conn_error(conn, "could not encrypt password: %s", errstr);
}
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index 049a8bb1a1..e41423a593 100644
--- a/src/interfaces/libpq/fe-auth.h
+++ b/src/interfaces/libpq/fe-auth.h
@@ -26,6 +26,7 @@ extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
/* Mechanisms in fe-auth-scram.c */
extern const pg_fe_sasl_mech pg_scram_mech;
extern char *pg_fe_scram_build_secret(const char *password,
+ int iterations,
const char **errstr);
#endif /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f88d672c6c..1741248f73 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -586,6 +586,7 @@ pqDropServerData(PGconn *conn)
conn->std_strings = false;
conn->default_transaction_read_only = PG_BOOL_UNKNOWN;
conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;
conn->sversion = 0;
/* Drop large-object lookup data */
@@ -3909,6 +3910,7 @@ makeEmptyPGconn(void)
conn->std_strings = false; /* unless server says differently */
conn->default_transaction_read_only = PG_BOOL_UNKNOWN;
conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;
conn->verbosity = PQERRORS_DEFAULT;
conn->show_context = PQSHOW_CONTEXT_ERRORS;
conn->sock = PGINVALID_SOCKET;
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index da229d632a..35d3afbf2a 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -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_sha256_iterations") == 0)
+ {
+ conn->scram_iterations = atoi(value);
+ }
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 512762f999..652df5b6ca 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -515,6 +515,7 @@ struct pg_conn
/* Assorted state for SASL, SSL, GSS, etc */
const pg_fe_sasl_mech *sasl;
void *sasl_state;
+ int scram_iterations;
/* SSL structures */
bool ssl_in_use;
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 42d3d4c79b..a684a93d6e 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -86,6 +86,14 @@ $node->safe_psql('postgres',
q{SET password_encryption='md5'; CREATE ROLE "md5,role" LOGIN PASSWORD 'pass';}
);
+# Create a role with a non-default iteration count
+$node->safe_psql(
+ 'postgres',
+ "SET password_encryption='scram-sha-256';
+ SET scram_sha256_iterations=100000;
+ CREATE ROLE scram_role_iter LOGIN PASSWORD 'pass';"
+);
+
# Create a database to test regular expression.
$node->safe_psql('postgres', "CREATE database regex_testdb;");
@@ -134,6 +142,14 @@ test_conn(
log_like => [
qr/connection authenticated: identity="scram_role" method=scram-sha-256/
]);
+test_conn(
+ $node,
+ 'user=scram_role_iter',
+ 'scram-sha-256',
+ 0,
+ log_like => [
+ qr/connection authenticated: identity="scram_role_iter" method=scram-sha-256/
+ ]);
test_conn($node, 'user=md5_role', 'scram-sha-256', 2,
log_unlike => [qr/connection authenticated:/]);
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index 7c84c9da33..1a3dad23e3 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -72,12 +72,15 @@ CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';
CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_sha256_iterations = 99999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
ORDER BY rolname, rolpassword;
- rolname | rolpassword_masked
------------------+---------------------------------------------------
+ rolname | rolpassword_masked
+-----------------+----------------------------------------------------
regress_passwd1 | md5cd3578025fe2c3d7ed1b9a9b26238b70
regress_passwd2 | md5dfa155cadd5f4ad57860162f3fab9cdb
regress_passwd3 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
@@ -86,7 +89,8 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
regress_passwd7 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
regress_passwd8 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
-(8 rows)
+ regress_passwd9 | SCRAM-SHA-256$99999:<salt>$<storedkey>:<serverkey>
+(9 rows)
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
@@ -129,6 +133,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql
index 98f49916e5..8276afec86 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -63,6 +63,10 @@ CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_sha256_iterations = 99999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
+
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
@@ -97,6 +101,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
--
2.32.1 (Apple Git-133)
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
@ 2022-12-14 18:59 ` Jonathan S. Katz <[email protected]>
2022-12-14 23:52 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-15 11:09 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
0 siblings, 2 replies; 18+ messages in thread
From: Jonathan S. Katz @ 2022-12-14 18:59 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>> On 14 Dec 2022, at 02:00, Michael Paquier <[email protected]> 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
Attachments:
[application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
@ 2022-12-14 23:52 ` Michael Paquier <[email protected]>
2022-12-15 00:00 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-15 11:09 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
1 sibling, 2 replies; 18+ messages in thread
From: Michael Paquier @ 2022-12-14 23:52 UTC (permalink / raw)
To: Jonathan S. Katz <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-14 23:52 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
@ 2022-12-15 00:00 ` Jonathan S. Katz <[email protected]>
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan S. Katz @ 2022-12-15 00:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
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
Attachments:
[application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-14 23:52 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
@ 2022-12-15 11:09 ` Daniel Gustafsson <[email protected]>
2022-12-17 03:27 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
1 sibling, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2022-12-15 11:09 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Jonathan S. Katz <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
> On 15 Dec 2022, at 00:52, Michael Paquier <[email protected]> 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/
Attachments:
[application/octet-stream] v3-0001-Make-SCRAM-iteration-count-configurable.patch (15.3K, ../../[email protected]/2-v3-0001-Make-SCRAM-iteration-count-configurable.patch)
download | inline diff:
From 6bce35e947d23fb4bf8a8e4b3ec5667a1b7d9c44 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Wed, 14 Dec 2022 11:52:36 +0100
Subject: [PATCH v3] Make SCRAM iteration count configurable
Replace the hardcoded value with a GUC such that the iteration count
can be raised in order to increase protection against brute-force
attacks. The hardcoded value for SCRAM iteration count was defined
to be 4096, which is taken from RFC 7677, so set the default for the
GUC to 4096 to match. Raising the iteration count will make stored
passwords more resilient to brute-force attacks at the cost of more
computation performed during connection establishment. Lowering the
count will reduce computational overhead during connections at the
tradeoff of reducing strength against brute-force attacks. When the
alternative is to fall back to md5 instead, SCRAM with a very low
iteration count is still preferrable so allow iteration counts all
the way down to one.
Reviewed-by: Michael Paquier <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/config.sgml | 20 +++++++++++++++++++
src/backend/libpq/auth-scram.c | 9 +++++++--
src/backend/utils/misc/guc_tables.c | 13 ++++++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/common/scram-common.c | 3 +--
src/include/common/scram-common.h | 4 ++--
src/include/libpq/scram.h | 3 +++
src/interfaces/libpq/fe-auth-scram.c | 4 ++--
src/interfaces/libpq/fe-auth.c | 2 +-
src/interfaces/libpq/fe-auth.h | 1 +
src/interfaces/libpq/fe-connect.c | 2 ++
src/interfaces/libpq/fe-exec.c | 4 ++++
src/interfaces/libpq/libpq-int.h | 1 +
src/test/authentication/t/001_password.pl | 16 +++++++++++++++
src/test/regress/expected/password.out | 11 +++++++---
src/test/regress/sql/password.sql | 5 +++++
16 files changed, 87 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8e4145979d..c3f28dae86 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1098,6 +1098,26 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>scram_sha_256_iterations</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>scram_sha_256_iterations</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ The number of computational iterations to be performed when encrypting
+ a password using SCRAM-SHA-256. 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 existing passwords
+ encrypted with SCRAM-SHA-256 as the iteration count is fixed at the
+ time of encryption. In order to make use of a changed value, new
+ password must be set.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-krb-server-keyfile" xreflabel="krb_server_keyfile">
<term><varname>krb_server_keyfile</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index c9bab85e82..d0977ea8c2 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -184,6 +184,11 @@ static char *sanitize_char(char c);
static char *sanitize_str(const char *s);
static char *scram_mock_salt(const char *username);
+/*
+ * The number of iterations to use when generating new secrets.
+ */
+int scram_sha_256_iterations;
+
/*
* Get a list of SASL mechanisms that this module supports.
*
@@ -483,7 +488,7 @@ pg_be_scram_build_secret(const char *password)
errmsg("could not generate random salt")));
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ scram_sha_256_iterations, password,
&errstr);
if (prep_password)
@@ -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_SHA_256_DEFAULT_ITERATIONS;
/* StoredKey and ServerKey are not used in a doomed authentication */
memset(stored_key, 0, SCRAM_KEY_LEN);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..6ba0ece7ec 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -40,9 +40,11 @@
#include "commands/trigger.h"
#include "commands/user.h"
#include "commands/vacuum.h"
+#include "common/scram-common.h"
#include "jit/jit.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
+#include "libpq/scram.h"
#include "optimizer/cost.h"
#include "optimizer/geqo.h"
#include "optimizer/optimizer.h"
@@ -3423,6 +3425,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"scram_sha_256_iterations", PGC_SUSET, CONN_AUTH_AUTH,
+ gettext_noop("Sets the iteration count for SCRAM secret generation."),
+ NULL,
+ GUC_REPORT | GUC_SUPERUSER_ONLY
+ },
+ &scram_sha_256_iterations,
+ SCRAM_SHA_256_DEFAULT_ITERATIONS, 1, INT_MAX,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 043864597f..3ef19f99c0 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -94,6 +94,7 @@
#authentication_timeout = 1min # 1s-600s
#password_encryption = scram-sha-256 # scram-sha-256 or md5
+#scram_sha_256_iterations = 4096
#db_user_namespace = off
# GSSAPI using Kerberos
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index 1268625929..2dc9679a4d 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -206,8 +206,7 @@ scram_build_secret(const char *salt, int saltlen, int iterations,
int encoded_server_len;
int encoded_result;
- if (iterations <= 0)
- iterations = SCRAM_DEFAULT_ITERATIONS;
+ Assert(iterations > 0);
/* Calculate StoredKey and ServerKey */
if (scram_SaltedPassword(password, salt, saltlen, iterations,
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 4acf2a78ad..8032e31a89 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -38,10 +38,10 @@
#define SCRAM_DEFAULT_SALT_LEN 16
/*
- * Default number of iterations when generating secret. Should be at least
+ * Default number of iterations when generating secret. Should be at least
* 4096 per RFC 7677.
*/
-#define SCRAM_DEFAULT_ITERATIONS 4096
+#define SCRAM_SHA_256_DEFAULT_ITERATIONS 4096
extern int scram_SaltedPassword(const char *password, const char *salt,
int saltlen, int iterations, uint8 *result,
diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h
index c51e848c24..10f302aa98 100644
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -17,6 +17,9 @@
#include "libpq/libpq-be.h"
#include "libpq/sasl.h"
+/* Number of iterations when generating new secrets */
+extern PGDLLIMPORT int scram_sha_256_iterations;
+
/* SASL implementation callbacks */
extern PGDLLIMPORT const pg_be_sasl_mech pg_be_scram_mech;
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index c500bea9e7..cf5e8824d3 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -882,7 +882,7 @@ verify_server_signature(fe_scram_state *state, bool *match,
* error details.
*/
char *
-pg_fe_scram_build_secret(const char *password, const char **errstr)
+pg_fe_scram_build_secret(const char *password, int iterations, const char **errstr)
{
char *prep_password;
pg_saslprep_rc rc;
@@ -913,7 +913,7 @@ pg_fe_scram_build_secret(const char *password, const char **errstr)
}
result = scram_build_secret(saltbuf, SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ iterations, password,
errstr);
free(prep_password);
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 4a6c358bb6..52fee8e579 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -1253,7 +1253,7 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
{
const char *errstr = NULL;
- crypt_pwd = pg_fe_scram_build_secret(passwd, &errstr);
+ crypt_pwd = pg_fe_scram_build_secret(passwd, conn->scram_iterations, &errstr);
if (!crypt_pwd)
libpq_append_conn_error(conn, "could not encrypt password: %s", errstr);
}
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index 049a8bb1a1..e41423a593 100644
--- a/src/interfaces/libpq/fe-auth.h
+++ b/src/interfaces/libpq/fe-auth.h
@@ -26,6 +26,7 @@ extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
/* Mechanisms in fe-auth-scram.c */
extern const pg_fe_sasl_mech pg_scram_mech;
extern char *pg_fe_scram_build_secret(const char *password,
+ int iterations,
const char **errstr);
#endif /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f88d672c6c..df9c13138a 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -586,6 +586,7 @@ pqDropServerData(PGconn *conn)
conn->std_strings = false;
conn->default_transaction_read_only = PG_BOOL_UNKNOWN;
conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_iterations = SCRAM_SHA_256_DEFAULT_ITERATIONS;
conn->sversion = 0;
/* Drop large-object lookup data */
@@ -3909,6 +3910,7 @@ makeEmptyPGconn(void)
conn->std_strings = false; /* unless server says differently */
conn->default_transaction_read_only = PG_BOOL_UNKNOWN;
conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_iterations = SCRAM_SHA_256_DEFAULT_ITERATIONS;
conn->verbosity = PQERRORS_DEFAULT;
conn->show_context = PQSHOW_CONTEXT_ERRORS;
conn->sock = PGINVALID_SOCKET;
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index da229d632a..358f6a42eb 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -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_iterations = atoi(value);
+ }
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 512762f999..652df5b6ca 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -515,6 +515,7 @@ struct pg_conn
/* Assorted state for SASL, SSL, GSS, etc */
const pg_fe_sasl_mech *sasl;
void *sasl_state;
+ int scram_iterations;
/* SSL structures */
bool ssl_in_use;
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 42d3d4c79b..f8add4f376 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -86,6 +86,14 @@ $node->safe_psql('postgres',
q{SET password_encryption='md5'; CREATE ROLE "md5,role" LOGIN PASSWORD 'pass';}
);
+# Create a role with a non-default iteration count
+$node->safe_psql(
+ 'postgres',
+ "SET password_encryption='scram-sha-256';
+ SET scram_sha_256_iterations=100000;
+ CREATE ROLE scram_role_iter LOGIN PASSWORD 'pass';"
+);
+
# Create a database to test regular expression.
$node->safe_psql('postgres', "CREATE database regex_testdb;");
@@ -134,6 +142,14 @@ test_conn(
log_like => [
qr/connection authenticated: identity="scram_role" method=scram-sha-256/
]);
+test_conn(
+ $node,
+ 'user=scram_role_iter',
+ 'scram-sha-256',
+ 0,
+ log_like => [
+ qr/connection authenticated: identity="scram_role_iter" method=scram-sha-256/
+ ]);
test_conn($node, 'user=md5_role', 'scram-sha-256', 2,
log_unlike => [qr/connection authenticated:/]);
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index 7c84c9da33..060496b03d 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -72,12 +72,15 @@ CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';
CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_sha_256_iterations = 99999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
ORDER BY rolname, rolpassword;
- rolname | rolpassword_masked
------------------+---------------------------------------------------
+ rolname | rolpassword_masked
+-----------------+----------------------------------------------------
regress_passwd1 | md5cd3578025fe2c3d7ed1b9a9b26238b70
regress_passwd2 | md5dfa155cadd5f4ad57860162f3fab9cdb
regress_passwd3 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
@@ -86,7 +89,8 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
regress_passwd7 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
regress_passwd8 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
-(8 rows)
+ regress_passwd9 | SCRAM-SHA-256$99999:<salt>$<storedkey>:<serverkey>
+(9 rows)
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
@@ -129,6 +133,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql
index 98f49916e5..91c2114e8e 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -63,6 +63,10 @@ CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_sha_256_iterations = 99999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
+
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
@@ -97,6 +101,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
--
2.32.1 (Apple Git-133)
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-14 23:52 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-15 11:09 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
@ 2022-12-17 03:27 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Michael Paquier @ 2022-12-17 03:27 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Jonathan S. Katz <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote:
>> On 15 Dec 2022, at 00:52, Michael Paquier <[email protected]> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Raising the SCRAM iteration count
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-10 00:15 ` Re: Raising the SCRAM iteration count Andres Freund <[email protected]>
2022-12-12 14:47 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Re: Raising the SCRAM iteration count Michael Paquier <[email protected]>
2022-12-14 11:25 ` Re: Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Re: Raising the SCRAM iteration count Jonathan S. Katz <[email protected]>
@ 2022-12-15 11:09 ` Daniel Gustafsson <[email protected]>
1 sibling, 0 replies; 18+ messages in thread
From: Daniel Gustafsson @ 2022-12-15 11:09 UTC (permalink / raw)
To: Jonathan S. Katz <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>
> On 14 Dec 2022, at 19:59, Jonathan S. Katz <[email protected]> wrote:
> On 12/14/22 6:25 AM, Daniel Gustafsson wrote:
>>> On 14 Dec 2022, at 02:00, Michael Paquier <[email protected]> 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 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/
^ permalink raw reply [nested|flat] 18+ messages in thread
* [PATCH 8/9] XXX devel comment: heap_deform_tuple -> slot_deform_tuple?
@ 2026-03-12 15:14 Álvaro Herrera <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Álvaro Herrera @ 2026-03-12 15:14 UTC (permalink / raw)
---
src/backend/replication/pgoutput_repack/pgoutput_repack.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c
index 73aa6f0589c..c77564c4024 100644
--- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c
+++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c
@@ -200,9 +200,14 @@ store_change(LogicalDecodingContext *ctx, Relation relation,
attrs = palloc_array(Datum, desc->natts);
isnull = palloc_array(bool, desc->natts);
+ /*
+ * XXX it might be better to use slot_deform_tuple here, for the case
+ * where atttributes near the end of the tuple don't need to undergo
+ * this procedure.
+ */
heap_deform_tuple(tuple, desc, attrs, isnull);
- /* First, gather and count the "external indirect" attributes. */
+ /* First, gather all the "external indirect" attributes. */
for (int i = 0; i < desc->natts; i++)
{
CompactAttribute *attr = TupleDescCompactAttr(desc, i);
--
2.47.3
--pnppmxqkefjd4hu2
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment;
filename="0009-reuse-existing-variable-by-overwriting-it.nocfbot.txt"
^ permalink raw reply [nested|flat] 18+ messages in thread
* [PATCH 8/9] XXX devel comment: heap_deform_tuple -> slot_deform_tuple?
@ 2026-03-12 15:14 Álvaro Herrera <[email protected]>
0 siblings, 0 replies; 18+ messages in thread
From: Álvaro Herrera @ 2026-03-12 15:14 UTC (permalink / raw)
---
src/backend/replication/pgoutput_repack/pgoutput_repack.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c
index 73aa6f0589c..c77564c4024 100644
--- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c
+++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c
@@ -200,9 +200,14 @@ store_change(LogicalDecodingContext *ctx, Relation relation,
attrs = palloc_array(Datum, desc->natts);
isnull = palloc_array(bool, desc->natts);
+ /*
+ * XXX it might be better to use slot_deform_tuple here, for the case
+ * where atttributes near the end of the tuple don't need to undergo
+ * this procedure.
+ */
heap_deform_tuple(tuple, desc, attrs, isnull);
- /* First, gather and count the "external indirect" attributes. */
+ /* First, gather all the "external indirect" attributes. */
for (int i = 0; i < desc->natts; i++)
{
CompactAttribute *attr = TupleDescCompactAttr(desc, i);
--
2.47.3
--pnppmxqkefjd4hu2
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment;
filename="0009-reuse-existing-variable-by-overwriting-it.nocfbot.txt"
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2026-03-12 15:14 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-12-09 10:55 Raising the SCRAM iteration count Daniel Gustafsson <[email protected]>
2022-12-09 15:50 ` Heikki Linnakangas <[email protected]>
2022-12-09 23:21 ` Michael Paquier <[email protected]>
2022-12-10 00:15 ` Andres Freund <[email protected]>
2022-12-10 23:46 ` Daniel Gustafsson <[email protected]>
2022-12-11 03:32 ` Michael Paquier <[email protected]>
2022-12-12 14:47 ` Jonathan S. Katz <[email protected]>
2022-12-13 11:17 ` Daniel Gustafsson <[email protected]>
2022-12-14 01:00 ` Michael Paquier <[email protected]>
2022-12-14 11:25 ` Daniel Gustafsson <[email protected]>
2022-12-14 18:59 ` Jonathan S. Katz <[email protected]>
2022-12-14 23:52 ` Michael Paquier <[email protected]>
2022-12-15 00:00 ` Jonathan S. Katz <[email protected]>
2022-12-15 11:09 ` Daniel Gustafsson <[email protected]>
2022-12-17 03:27 ` Michael Paquier <[email protected]>
2022-12-15 11:09 ` Daniel Gustafsson <[email protected]>
2026-03-12 15:14 [PATCH 8/9] XXX devel comment: heap_deform_tuple -> slot_deform_tuple? Álvaro Herrera <[email protected]>
2026-03-12 15:14 [PATCH 8/9] XXX devel comment: heap_deform_tuple -> slot_deform_tuple? Álvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox