On Fri, Sep 20, 2024 at 10:06:28AM -0400, Jonathan S. Katz wrote:
> On 9/20/24 1:23 AM, Michael Paquier wrote:
>> Not sure. Is this really something we absolutely need? Sure, this
>> generates a better error when inserting a record too long to
>> pg_authid, but removing the toast relation is enough to avoid the
>> problems one would see when authenticating. Not sure if this argument
>> is enough to count as an objection, just sharing some doubts :)
>
> The errors from lack of TOAST are confusing to users. Why can't we have a
> user friendly error here?
If I wanted to argue against adding a user-friendly error, I'd point out
that it's highly unlikely anyone is actually trying to use super long
hashes unless they are trying to break things, and it's just another
arbitrary limit that we'll need to maintain/enforce. But on the off-chance
that someone is building a custom driver that generates long hashes for
whatever reason, I'd imagine that a clear error would be more helpful than
"row is too big."
Here is a v3 patch set that fixes the test comment and a compiler warning
in cfbot.
--
nathan
>From bb3aad9105b1997bc088403437ac6294e22809d9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Thu, 19 Sep 2024 20:59:10 -0500
Subject: [PATCH v3 1/2] place limit on password hash length
---
src/backend/libpq/crypt.c | 60 ++++++++++++++++++--------
src/include/libpq/crypt.h | 10 +++++
src/test/regress/expected/password.out | 7 +++
src/test/regress/sql/password.sql | 4 ++
4 files changed, 63 insertions(+), 18 deletions(-)
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 629e51e00b..753e5c11da 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -116,7 +116,7 @@ encrypt_password(PasswordType target_type, const char *role,
const char *password)
{
PasswordType guessed_type = get_password_type(password);
- char *encrypted_password;
+ char *encrypted_password = NULL;
const char *errstr = NULL;
if (guessed_type != PASSWORD_TYPE_PLAINTEXT)
@@ -125,32 +125,56 @@ encrypt_password(PasswordType target_type, const char
*role,
* Cannot convert an already-encrypted password from one format
to
* another, so return it as it is.
*/
- return pstrdup(password);
+ encrypted_password = pstrdup(password);
}
-
- switch (target_type)
+ else
{
- case PASSWORD_TYPE_MD5:
- encrypted_password = palloc(MD5_PASSWD_LEN + 1);
+ switch (target_type)
+ {
+ case PASSWORD_TYPE_MD5:
+ encrypted_password = palloc(MD5_PASSWD_LEN + 1);
- if (!pg_md5_encrypt(password, role, strlen(role),
-
encrypted_password, &errstr))
- elog(ERROR, "password encryption failed: %s",
errstr);
- return encrypted_password;
+ if (!pg_md5_encrypt(password, role,
strlen(role),
+
encrypted_password, &errstr))
+ elog(ERROR, "password encryption
failed: %s", errstr);
+ break;
- case PASSWORD_TYPE_SCRAM_SHA_256:
- return pg_be_scram_build_secret(password);
+ case PASSWORD_TYPE_SCRAM_SHA_256:
+ encrypted_password =
pg_be_scram_build_secret(password);
+ break;
- case PASSWORD_TYPE_PLAINTEXT:
- elog(ERROR, "cannot encrypt password with 'plaintext'");
+ case PASSWORD_TYPE_PLAINTEXT:
+ elog(ERROR, "cannot encrypt password with
'plaintext'");
+ break;
+ }
}
+ Assert(encrypted_password);
+
/*
- * This shouldn't happen, because the above switch statements should
- * handle every combination of source and target password types.
+ * Valid password hashes may be very long, but we don't want to store
+ * anything that might need out-of-line storage, since de-TOASTing won't
+ * work during authentication because we haven't selected a database yet
+ * and cannot read pg_class. 256 bytes should be more than enough for
all
+ * practical use, so fail for anything longer.
*/
- elog(ERROR, "cannot encrypt password to requested type");
- return NULL; /* keep compiler quiet */
+ if (encrypted_password && /* keep compiler quiet */
+ strlen(encrypted_password) > MAX_ENCRYPTED_PASSWORD_LEN)
+ {
+ /*
+ * We don't expect any of our own hashing routines to produce
hashes
+ * that are too long.
+ */
+ Assert(guessed_type != PASSWORD_TYPE_PLAINTEXT);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("encrypted password is too long"),
+ errdetail("Encrypted passwords must be no
longer than %d characters.",
+
MAX_ENCRYPTED_PASSWORD_LEN)));
+ }
+
+ return encrypted_password;
}
/*
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index f744de4d20..a2c99cd16a 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,6 +15,16 @@
#include "datatype/timestamp.h"
+/*
+ * Valid password hashes may be very long, but we don't want to store anything
+ * that might need out-of-line storage, since de-TOASTing won't work during
+ * authentication because we haven't selected a database yet and cannot read
+ * pg_class. 256 bytes should be more than enough for all practical use, and
+ * our own password encryption routines should never produce hashes longer than
+ * this.
+ */
+#define MAX_ENCRYPTED_PASSWORD_LEN (256)
+
/*
* Types of password hashes or secrets.
*
diff --git a/src/test/regress/expected/password.out
b/src/test/regress/expected/password.out
index 924d6e001d..8ab995a162 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -127,6 +127,13 @@ SELECT rolname, rolpassword not like
'%A6xHKoH/494E941doaPOYg==%' as is_rolpassw
regress_passwd_sha_len2 | t
(3 rows)
+-- Test that valid hashes that are too long are rejected
+CREATE ROLE regress_passwd10 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+ERROR: encrypted password is too long
+DETAIL: Encrypted passwords must be no longer than 256 characters.
+ALTER ROLE regress_passwd9 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+ERROR: encrypted password is too long
+DETAIL: Encrypted passwords must be no longer than 256 characters.
DROP ROLE regress_passwd1;
DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
diff --git a/src/test/regress/sql/password.sql
b/src/test/regress/sql/password.sql
index bb82aa4aa2..442c903c00 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -95,6 +95,10 @@ SELECT rolname, rolpassword not like
'%A6xHKoH/494E941doaPOYg==%' as is_rolpassw
WHERE rolname LIKE 'regress_passwd_sha_len%'
ORDER BY rolname;
+-- Test that valid hashes that are too long are rejected
+CREATE ROLE regress_passwd10 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+ALTER ROLE regress_passwd9 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+
DROP ROLE regress_passwd1;
DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
--
2.39.5 (Apple Git-154)
>From 7e1736701e270d40cc1009ff4760317a8a366663 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Thu, 19 Sep 2024 21:09:30 -0500
Subject: [PATCH v3 2/2] remove pg_authid's TOAST table
XXX: NEEDS CATVERSION BUMP
---
src/backend/catalog/catalog.c | 4 +---
src/include/catalog/pg_authid.h | 2 --
src/test/regress/expected/create_index.out | 14 +++++++-------
src/test/regress/expected/misc_sanity.out | 5 ++++-
src/test/regress/expected/tablespace.out | 12 ++++++------
src/test/regress/sql/create_index.sql | 10 +++++-----
src/test/regress/sql/misc_sanity.sql | 2 ++
src/test/regress/sql/tablespace.sql | 8 ++++----
8 files changed, 29 insertions(+), 28 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 6c39434a30..d6b07a7865 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -300,9 +300,7 @@ IsSharedRelation(Oid relationId)
relationId == TablespaceOidIndexId)
return true;
/* These are their toast tables and toast indexes */
- if (relationId == PgAuthidToastTable ||
- relationId == PgAuthidToastIndex ||
- relationId == PgDatabaseToastTable ||
+ if (relationId == PgDatabaseToastTable ||
relationId == PgDatabaseToastIndex ||
relationId == PgDbRoleSettingToastTable ||
relationId == PgDbRoleSettingToastIndex ||
diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
index e08863f78a..e846d75731 100644
--- a/src/include/catalog/pg_authid.h
+++ b/src/include/catalog/pg_authid.h
@@ -55,8 +55,6 @@ CATALOG(pg_authid,1260,AuthIdRelationId) BKI_SHARED_RELATION
BKI_ROWTYPE_OID(284
*/
typedef FormData_pg_authid *Form_pg_authid;
-DECLARE_TOAST_WITH_MACRO(pg_authid, 4175, 4176, PgAuthidToastTable,
PgAuthidToastIndex);
-
DECLARE_UNIQUE_INDEX(pg_authid_rolname_index, 2676, AuthIdRolnameIndexId,
pg_authid, btree(rolname name_ops));
DECLARE_UNIQUE_INDEX_PKEY(pg_authid_oid_index, 2677, AuthIdOidIndexId,
pg_authid, btree(oid oid_ops));
diff --git a/src/test/regress/expected/create_index.out
b/src/test/regress/expected/create_index.out
index cf6eac5734..d3358dfc39 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2664,10 +2664,10 @@ REINDEX TABLE CONCURRENTLY pg_class; -- no catalog
relation
ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
ERROR: cannot reindex system catalogs concurrently
--- These are the toast table and index of pg_authid.
-REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
+-- These are the toast table and index of pg_database.
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1262; -- no catalog toast table
ERROR: cannot reindex system catalogs concurrently
-REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast
index
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1262_index; -- no catalog toast
index
ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
ERROR: cannot reindex system catalogs concurrently
@@ -2974,10 +2974,10 @@ ERROR: must be owner of schema schema_to_reindex
RESET ROLE;
GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
SET SESSION ROLE regress_reindexuser;
-REINDEX TABLE pg_toast.pg_toast_1260;
-ERROR: permission denied for table pg_toast_1260
-REINDEX INDEX pg_toast.pg_toast_1260_index;
-ERROR: permission denied for index pg_toast_1260_index
+REINDEX TABLE pg_toast.pg_toast_1262;
+ERROR: permission denied for table pg_toast_1262
+REINDEX INDEX pg_toast.pg_toast_1262_index;
+ERROR: permission denied for index pg_toast_1262_index
-- Clean up
RESET ROLE;
REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
diff --git a/src/test/regress/expected/misc_sanity.out
b/src/test/regress/expected/misc_sanity.out
index 45e7492877..b032a3f476 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -40,6 +40,8 @@ WHERE refclassid = 0 OR refobjid = 0 OR
-- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs
-- and toast tables are mutually exclusive and large object data is handled
-- as user data by pg_upgrade, which would cause failures.
+-- 3. pg_authid, since its toast table cannot be accessed when it would be
+-- needed, i.e., during authentication before we've selected a database.
SELECT relname, attname, atttypid::regtype
FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid
WHERE c.oid < 16384 AND
@@ -53,12 +55,13 @@ ORDER BY 1, 2;
pg_attribute | attfdwoptions | text[]
pg_attribute | attmissingval | anyarray
pg_attribute | attoptions | text[]
+ pg_authid | rolpassword | text
pg_class | relacl | aclitem[]
pg_class | reloptions | text[]
pg_class | relpartbound | pg_node_tree
pg_largeobject | data | bytea
pg_largeobject_metadata | lomacl | aclitem[]
-(9 rows)
+(10 rows)
-- system catalogs without primary keys
--
diff --git a/src/test/regress/expected/tablespace.out
b/src/test/regress/expected/tablespace.out
index 9aabb85349..dd535d41a3 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -51,13 +51,13 @@ ERROR: cannot move system relation
"pg_authid_rolname_index"
REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_authid;
ERROR: cannot reindex system catalogs concurrently
-- toast relations, fail
-REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1260_index;
-ERROR: cannot move system relation "pg_toast_1260_index"
-REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1260_index;
+REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1262_index;
+ERROR: cannot move system relation "pg_toast_1262_index"
+REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1262_index;
ERROR: cannot reindex system catalogs concurrently
-REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1260;
-ERROR: cannot move system relation "pg_toast_1260_index"
-REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1260;
+REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1262;
+ERROR: cannot move system relation "pg_toast_1262_index"
+REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1262;
ERROR: cannot reindex system catalogs concurrently
-- system catalog, fail
REINDEX (TABLESPACE pg_global) TABLE pg_authid;
diff --git a/src/test/regress/sql/create_index.sql
b/src/test/regress/sql/create_index.sql
index e296891cab..fe162cc7c3 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1126,9 +1126,9 @@ REINDEX TABLE CONCURRENTLY concur_reindex_tab;
COMMIT;
REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
--- These are the toast table and index of pg_authid.
-REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
-REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast
index
+-- These are the toast table and index of pg_database.
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1262; -- no catalog toast table
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1262_index; -- no catalog toast
index
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
REINDEX (CONCURRENTLY) SYSTEM; -- ditto
@@ -1305,8 +1305,8 @@ REINDEX SCHEMA schema_to_reindex;
RESET ROLE;
GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
SET SESSION ROLE regress_reindexuser;
-REINDEX TABLE pg_toast.pg_toast_1260;
-REINDEX INDEX pg_toast.pg_toast_1260_index;
+REINDEX TABLE pg_toast.pg_toast_1262;
+REINDEX INDEX pg_toast.pg_toast_1262_index;
-- Clean up
RESET ROLE;
diff --git a/src/test/regress/sql/misc_sanity.sql
b/src/test/regress/sql/misc_sanity.sql
index 16f3a7c0c1..135793871b 100644
--- a/src/test/regress/sql/misc_sanity.sql
+++ b/src/test/regress/sql/misc_sanity.sql
@@ -43,6 +43,8 @@ WHERE refclassid = 0 OR refobjid = 0 OR
-- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs
-- and toast tables are mutually exclusive and large object data is handled
-- as user data by pg_upgrade, which would cause failures.
+-- 3. pg_authid, since its toast table cannot be accessed when it would be
+-- needed, i.e., during authentication before we've selected a database.
SELECT relname, attname, atttypid::regtype
FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid
diff --git a/src/test/regress/sql/tablespace.sql
b/src/test/regress/sql/tablespace.sql
index d274d9615e..c8b83788f0 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -40,10 +40,10 @@ REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_am;
REINDEX (TABLESPACE regress_tblspace) TABLE pg_authid;
REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_authid;
-- toast relations, fail
-REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1260_index;
-REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1260_index;
-REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1260;
-REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1260;
+REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1262_index;
+REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1262_index;
+REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1262;
+REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1262;
-- system catalog, fail
REINDEX (TABLESPACE pg_global) TABLE pg_authid;
REINDEX (TABLESPACE pg_global) TABLE CONCURRENTLY pg_authid;
--
2.39.5 (Apple Git-154)
