public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH v17 09/10] pg_ls_*dir to return all the metadata from pg_stat_file..
2+ messages / 2 participants
[nested] [flat]

* [PATCH v17 09/10] pg_ls_*dir to return all the metadata from pg_stat_file..
@ 2020-03-10 02:56  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 2+ messages in thread

From: Justin Pryzby @ 2020-03-10 02:56 UTC (permalink / raw)

..but it doesn't seem worth factoring out the common bits, since stat_file
doesn't return a name, so all the field numbers are off by one.

NOTE, the atime is now shown where the mtime used to be.

Need catversion bump
---
 doc/src/sgml/func.sgml                       | 36 ++++++++---
 src/backend/utils/adt/genfile.c              | 65 +++++++++-----------
 src/include/catalog/pg_proc.dat              | 34 +++++-----
 src/test/regress/expected/misc_functions.out | 16 ++---
 src/test/regress/output/tablespace.source    |  8 +--
 5 files changed, 85 insertions(+), 74 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 334b6beb3a..c3ec56a5f6 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25700,13 +25700,16 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the server's log directory,
-        return the file's name, size, last modification time (mtime), and a boolean
-        indicating if the file is a directory.
+        return the file's name, along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Filenames beginning with a dot and special file types are excluded.
        </para>
        <para>
@@ -25727,7 +25730,10 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
@@ -25747,7 +25753,10 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
@@ -25779,13 +25788,15 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the server's write-ahead log (WAL) directory, list the
-        file's name, size, last modification time (mtime), and a boolean
-        indicating if the file is a directory.
+        filename along with the metadata columns returned by <function>pg_stat_file</function>.
         Filenames beginning with a dot and special files types are excluded.
        </para>
        <para>
@@ -25804,14 +25815,17 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the server's WAL archive status directory
-        (<filename>pg_wal/archive_status</filename>), list the file's
-        name, size, last modification time (mtime), and a boolean indicating if
-        the file is a directory.
+        (<filename>pg_wal/archive_status</filename>), list the filename
+        along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Filenames beginning with a dot and special file types are excluded.
        </para>
        <para>
@@ -25831,13 +25845,17 @@ LATERAL pg_ls_dir_recurse(dir) AS a;
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the temporary directory within the given
-        <parameter>tablespace</parameter>, list the file's name, size, last
-        modification time (mtime) and a boolean indicating if the file is a directory.
+        <parameter>tablespace</parameter>, list the file's name
+        along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Directories are used for temporary files shared by parallel processes.
         If <parameter>tablespace</parameter> is not provided, the
         <literal>pg_default</literal> tablespace is examined.
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 91bf8c69e9..10780d3fb1 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -36,6 +36,8 @@
 #include "utils/syscache.h"
 #include "utils/timestamp.h"
 
+static void tuple_from_stat(struct stat *fst, const char *path, Datum *values,
+		bool *isnull);
 static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags);
 
 #define	LS_DIR_ISDIR				(1<<0) /* Show column: isdir */
@@ -367,6 +369,28 @@ pg_read_binary_file_all(PG_FUNCTION_ARGS)
 	return pg_read_binary_file(fcinfo);
 }
 
+/*
+ * Populate values and isnull from fst and path.
+ * Used for pg_stat_file() and pg_stat_dir_files()
+ * isnull is assumed to have been zeroed.
+ */
+static void
+tuple_from_stat(struct stat *fst, const char *path, Datum *values, bool *isnull)
+{
+	values[0] = Int64GetDatum((int64) fst->st_size);
+	values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_atime));
+	values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_mtime));
+	/* Unix has file status change time, while Win32 has creation time */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+	values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime));
+	isnull[4] = true;
+#else
+	isnull[3] = true;
+	values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime));
+#endif
+	values[5] = BoolGetDatum(S_ISDIR(fst->st_mode));
+}
+
 /*
  * stat a file
  */
@@ -418,25 +442,7 @@ pg_stat_file(PG_FUNCTION_ARGS)
 	BlessTupleDesc(tupdesc);
 
 	memset(isnull, false, sizeof(isnull));
-
-	values[0] = Int64GetDatum((int64) fst.st_size);
-	values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_atime));
-	values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_mtime));
-	/* Unix has file status change time, while Win32 has creation time */
-#if !defined(WIN32) && !defined(__CYGWIN__)
-	values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
-	isnull[4] = true;
-#else
-	isnull[3] = true;
-	values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
-#endif
-	values[5] = BoolGetDatum(S_ISDIR(fst.st_mode));
-#ifdef WIN32
-	/* Links should have isdir=false */
-	if (pgwin32_is_junction(filename))
-		values[5] = BoolGetDatum(false);
-#endif
-
+	tuple_from_stat(&fst, filename, values, isnull);
 	tuple = heap_form_tuple(tupdesc, values, isnull);
 
 	pfree(filename);
@@ -578,8 +584,8 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
 
 	while ((de = ReadDir(dirdesc, dir)) != NULL)
 	{
-		Datum		values[4];
-		bool		nulls[4];
+		Datum		values[7];
+		bool		nulls[7];
 		char		path[MAXPGPATH * 2];
 		struct stat attrib;
 
@@ -618,23 +624,10 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
 				continue;
 		}
 
+		memset(nulls, 0, sizeof(nulls));
 		values[0] = CStringGetTextDatum(de->d_name);
 		if (flags & LS_DIR_METADATA)
-		{
-			values[1] = Int64GetDatum((int64) attrib.st_size);
-			values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
-			if (flags & LS_DIR_ISDIR)
-			{
-				values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode));
-#ifdef WIN32
-				/* Links should have isdir=false */
-				if (pgwin32_is_junction(path))
-					values[3] = BoolGetDatum(false);
-#endif
-			}
-		}
-
-		memset(nulls, 0, sizeof(nulls));
+			tuple_from_stat(&attrib, path, 1+values, 1+nulls);
 
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f94f403475..34ee5a8f97 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10880,48 +10880,48 @@
 { oid => '3353', descr => 'list files in the log directory',
   proname => 'pg_ls_logdir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
-  proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_logdir' },
+  proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+  proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_logdir' },
 { oid => '3354', descr => 'list of files in the WAL directory',
   proname => 'pg_ls_waldir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
-  proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_waldir' },
+  proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+  proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_waldir' },
 { oid => '5031', descr => 'list of files in the archive_status directory',
   proname => 'pg_ls_archive_statusdir', procost => '10', prorows => '20',
   proretset => 't', provolatile => 'v', prorettype => 'record',
-  proargtypes => '', proallargtypes => '{text,int8,timestamptz,bool}',
-  proargmodes => '{o,o,o,o}', proargnames => '{name,size,modification,isdir}',
+  proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}',
+  proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_archive_statusdir' },
 { oid => '5029', descr => 'list files in the pgsql_tmp directory',
   proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
-  proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_tmpdir_noargs' },
+  proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+  proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_tmpdir_noargs' },
 { oid => '5030', descr => 'list files in the pgsql_tmp directory',
   proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'oid',
-  proallargtypes => '{oid,text,int8,timestamptz,bool}', proargmodes => '{i,o,o,o,o}',
-  proargnames => '{tablespace,name,size,modification,isdir}',
+  proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}',
+  proargnames => '{tablespace,name,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_tmpdir_1arg' },
 { oid => '5032', descr => 'list directory with metadata',
   proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'text bool bool',
-  proallargtypes => '{text,bool,bool,text,int8,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o}',
-  proargnames => '{dirname,missing_ok,include_dot_dirs,name,size,modification,isdir}',
+  proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}',
+  proargnames => '{dirname,missing_ok,include_dot_dirs,name,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_dir_metadata' },
 { oid => '5033', descr => 'list directory with metadata',
   proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'text',
-  proallargtypes => '{text,text,int8,timestamptz,bool}', proargmodes => '{i,o,o,o,o}',
-  proargnames => '{dirname,name,size,modification,isdir}',
+  proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}',
+  proargnames => '{dirname,name,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_dir_metadata_1arg' },
 { oid => '5034', descr => 'list all files in a directory recursively',
   proname => 'pg_ls_dir_recurse', prorows => '10000', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'text',
-  proallargtypes => '{text,text,int8,timestamptz,bool}',
-  proargnames => '{dirname,name,size,modification,isdir}', proargmodes => '{i,o,o,o,o}',
-  prolang => 'sql', prosrc => "with recursive ls as (select * from pg_ls_dir_metadata(dirname, true, false) union all select ls.name||'/'||a.name, a.size, a.modification, a.isdir from ls, lateral pg_ls_dir_metadata(dirname||'/'||ls.name, false, false)a where ls.isdir) select * from ls" },
+  proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}',
+  proargnames => '{dirname,name,size,access,modification,change,creation,isdir}', proargmodes => '{i,o,o,o,o,o,o,o}',
+  prolang => 'sql', prosrc => "with recursive ls as (select * from pg_ls_dir_metadata(dirname, true, false) union all select ls.name||'/'||a.name, a.size, a.access, a.modification, a.change, a.creation, a.isdir from ls, lateral pg_ls_dir_metadata(dirname||'/'||ls.name, false, false)a where ls.isdir) select * from ls" },
 
 # hash partitioning constraint function
 { oid => '5028', descr => 'hash partition CHECK constraint',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 4188d684f0..cce84a60a9 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -157,8 +157,8 @@ select count(*) > 0 as ok from (select pg_ls_waldir()) ss;
 
 -- Test not-run-to-completion cases.
 select * from pg_ls_waldir() limit 0;
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 select count(*) > 0 as ok from (select * from pg_ls_waldir() limit 1) ss;
@@ -222,8 +222,8 @@ ERROR:  could not open directory "does not exist": No such file or directory
 -- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet
 -- The name='' condition is never true, so the function runs to completion but returns zero rows.
 select * from pg_ls_tmpdir() where name='Does not exist';
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 select name, isdir from pg_ls_dir_metadata('.') where name='.';
@@ -239,8 +239,8 @@ select name, isdir from pg_ls_dir_metadata('.', false, false) where name='.'; --
 
 -- Check that expected columns are present
 select * from pg_ls_dir_metadata('.') limit 0;
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 -- Check that we at least succeed in recursing once, and that we don't show the leading dir prefix
@@ -253,8 +253,8 @@ SELECT name, isdir FROM pg_ls_dir_recurse('.') WHERE isdir AND name~'^pg_wal';
 
 -- Check that expected columns are present
 SELECT * FROM pg_ls_dir_recurse('.') LIMIT 0;
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 --
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index ba9a3fe29a..1e1e02b589 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -17,15 +17,15 @@ CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
 -- The name='' condition is never true, so the function runs to completion but returns zero rows.
 -- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir()
 SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist';
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 -- This tests the missing_ok parameter.  If that's not functioning, this would ERROR if the logdir doesn't exist yet.
 -- The name='' condition is never true, so the function runs to completion but returns zero rows.
 SELECT * FROM pg_ls_logdir() WHERE name='Does not exist';
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 -- try setting and resetting some properties for the new tablespace
-- 
2.17.0


--4LFBTxd4L5NLO6ly
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v17-0010-pg_ls_-to-show-file-type-and-show-special-files.patch"



^ permalink  raw  reply  [nested|flat] 2+ messages in thread

* Re: Raising the SCRAM iteration count
@ 2023-02-22 13:39  Daniel Gustafsson <[email protected]>
  0 siblings, 0 replies; 2+ messages in thread

From: Daniel Gustafsson @ 2023-02-22 13:39 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Jonathan S. Katz <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>

> On 17 Dec 2022, at 04:27, Michael Paquier <[email protected]> wrote:
> 
> 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.

ok, renamed.

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

Fixed.

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

Fixed.

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

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

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

--
Daniel Gustafsson





Attachments:

  [application/octet-stream] v4-0001-Make-SCRAM-iteration-count-configurable.patch (14.8K, ../../[email protected]/2-v4-0001-Make-SCRAM-iteration-count-configurable.patch)
  download | inline diff:
From d6afaca4da3cd60f837e27c2f371aceeb196c6e2 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Wed, 22 Feb 2023 14:36:36 +0100
Subject: [PATCH v4] 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]>
Reviewed-by: Jonathan S. Katz <[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             |  2 +-
 src/include/libpq/scram.h                     |  3 +++
 src/interfaces/libpq/fe-auth-scram.c          |  4 ++--
 src/interfaces/libpq/fe-auth.c                |  4 +++-
 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        |  7 ++++++-
 src/test/regress/sql/password.sql             |  5 +++++
 16 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e5c41cc6c6..a0eecaaaf0 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1132,6 +1132,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, a 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 4441e0d774..75583dcbf0 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -191,6 +191,11 @@ static char *scram_mock_salt(const char *username,
 							 pg_cryptohash_type hash_type,
 							 int key_length);
 
+/*
+ * 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.
  *
@@ -496,7 +501,7 @@ pg_be_scram_build_secret(const char *password)
 
 	result = scram_build_secret(PG_SHA256, SCRAM_SHA_256_KEY_LEN,
 								saltbuf, SCRAM_DEFAULT_SALT_LEN,
-								SCRAM_DEFAULT_ITERATIONS, password,
+								scram_sha_256_iterations, password,
 								&errstr);
 
 	if (prep_password)
@@ -717,7 +722,7 @@ mock_scram_secret(const char *username, pg_cryptohash_type *hash_type,
 	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_MAX_KEY_LEN);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1c0583fe26..554f706499 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -41,9 +41,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 "nodes/queryjumble.h"
 #include "optimizer/cost.h"
 #include "optimizer/geqo.h"
@@ -3468,6 +3470,17 @@ struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"scram_sha_256_iterations", PGC_USERSET, CONN_AUTH_AUTH,
+			gettext_noop("Sets the iteration count for SCRAM secret generation."),
+			NULL,
+			GUC_REPORT
+		},
+		&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 d06074b86f..9e901fc112 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -95,6 +95,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 bd40d497a9..ef997ef684 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -214,8 +214,7 @@ scram_build_secret(pg_cryptohash_type hash_type, int key_length,
 	/* Only this hash method is supported currently */
 	Assert(hash_type == PG_SHA256);
 
-	if (iterations <= 0)
-		iterations = SCRAM_DEFAULT_ITERATIONS;
+	Assert(iterations > 0);
 
 	/* Calculate StoredKey and ServerKey */
 	if (scram_SaltedPassword(password, hash_type, key_length,
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 0c75df5559..5ccff96ece 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -47,7 +47,7 @@
  * 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,
 								 pg_cryptohash_type hash_type, int key_length,
diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h
index b275e1e87e..310bc36517 100644
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -18,6 +18,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 9c42ea4f81..7162fdbb85 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -894,7 +894,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;
@@ -926,7 +926,7 @@ pg_fe_scram_build_secret(const char *password, const char **errstr)
 
 	result = scram_build_secret(PG_SHA256, SCRAM_SHA_256_KEY_LEN, 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 9afc6f19b9..0935356075 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -1253,7 +1253,9 @@ 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_sha_256_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 1aa556ea2f..124dd5d031 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 50b5df3490..3bf9b08e36 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_sha_256_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_sha_256_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 ec62550e38..8607bc062e 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_sha_256_iterations = atoi(value);
+	}
 }
 
 
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d7ec5ed429..940d1a35a1 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_sha_256_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 a2fde1408b..e0ab39ef52 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=10000;
+	 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..53db07d1d5 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -72,6 +72,9 @@ 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 = 9999;
+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%'
@@ -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$9999:<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..59ef7de21e 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 = 9999;
+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] 2+ messages in thread


end of thread, other threads:[~2023-02-22 13:39 UTC | newest]

Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 02:56 [PATCH v17 09/10] pg_ls_*dir to return all the metadata from pg_stat_file.. Justin Pryzby <[email protected]>
2023-02-22 13:39 Re: Raising the SCRAM iteration count Daniel Gustafsson <[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