On 09/12/2024 15:58, Guillaume Lelarge wrote:
Hi,Le lun. 9 déc. 2024 à 14:40, Daniel Gustafsson <[email protected] <mailto:[email protected]>> a écrit :> On 9 Dec 2024, at 14:26, Greg Sabino Mullane <[email protected] <mailto:[email protected]>> wrote: > -1 to throwing an ERROR - that's not really an error, and not our call to make, so a WARNING is sufficient.Agreed, regardless of how bad it's considered, it's not an error. There aremany ways sensitive data can end up in the logs and offering the impression there is a safety switch offers a false sense of security.I'm fine with adding a test on whether or not we log statements. But that completely hides the fact that people listening on the network could also get to the password if the server doesn't use SSL. Isn't it weird to warn about one potential leak and not the other one?
Here is patch v2. The only addition is a check to warn the user only if the query has been logged:
(log_statement == LOGSTMT_ALL || log_statement == LOGSTMT_DDL || log_min_duration_statement > -1) v2 is attached. Regards. -- Guillaume Lelarge Consultant https://dalibo.com
From 802bd6dfd9c12da032bc307863508c3575bdea19 Mon Sep 17 00:00:00 2001 From: Guillaume Lelarge <[email protected]> Date: Sat, 7 Dec 2024 10:22:37 +0100 Subject: [PATCH v2] Add a warning when using plain text passwords With CREATE/ALTER ROLE... PASSWORD, plain text passwords may be logged and can be discovered if someone is listening on the network. We used to tell users not to use plain text passwords, but it would be better to warn the user of the issue if the user tries to add/change a password using a plain text one in the query. CREATE ROLE and ALTER ROLE now emit warnings when using plain text passwords. The warnings can be disabled by setting the plaintext_password_warnings parameter to "off". --- .../passwordcheck/expected/passwordcheck.out | 1 + .../passwordcheck/expected/passwordcheck_1.out | 1 + contrib/passwordcheck/sql/passwordcheck.sql | 1 + doc/src/sgml/config.sgml | 18 ++++++++++++++++++ src/backend/libpq/crypt.c | 13 +++++++++++++ src/backend/utils/misc/guc_tables.c | 9 +++++++++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/libpq/crypt.h | 3 +++ src/test/regress/expected/create_role.out | 2 ++ src/test/regress/expected/password.out | 4 ++++ src/test/regress/sql/create_role.sql | 2 ++ src/test/regress/sql/password.sql | 5 +++++ 12 files changed, 60 insertions(+) diff --git a/contrib/passwordcheck/expected/passwordcheck.out b/contrib/passwordcheck/expected/passwordcheck.out index 83472c76d2..f3fdf9c03b 100644 --- a/contrib/passwordcheck/expected/passwordcheck.out +++ b/contrib/passwordcheck/expected/passwordcheck.out @@ -1,3 +1,4 @@ +SET plaintext_password_warnings = off; SET md5_password_warnings = off; LOAD 'passwordcheck'; CREATE USER regress_passwordcheck_user1; diff --git a/contrib/passwordcheck/expected/passwordcheck_1.out b/contrib/passwordcheck/expected/passwordcheck_1.out index fb12ec45cc..7db42e831c 100644 --- a/contrib/passwordcheck/expected/passwordcheck_1.out +++ b/contrib/passwordcheck/expected/passwordcheck_1.out @@ -1,3 +1,4 @@ +SET plaintext_password_warnings = off; SET md5_password_warnings = off; LOAD 'passwordcheck'; CREATE USER regress_passwordcheck_user1; diff --git a/contrib/passwordcheck/sql/passwordcheck.sql b/contrib/passwordcheck/sql/passwordcheck.sql index 21ad8d452b..92bc2de632 100644 --- a/contrib/passwordcheck/sql/passwordcheck.sql +++ b/contrib/passwordcheck/sql/passwordcheck.sql @@ -1,3 +1,4 @@ +SET plaintext_password_warnings = off; SET md5_password_warnings = off; LOAD 'passwordcheck'; diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index a782f10998..4493b1a458 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7781,6 +7781,24 @@ log_line_prefix = '%m [%p] %q%u@%d/%a ' </listitem> </varlistentry> + <varlistentry id="guc-plaintext-password-warnings" xreflabel="plaintext_password_warnings"> + <term><varname>plaintext_password_warnings</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>plaintext_password_warnings</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Controls whether a <literal>WARNING</literal> about plaintext password + is produced when a <command>CREATE ROLE</command> or <command>ALTER + ROLE</command> statement uses a plaintext password and if logging is + configured (either with <xref linkend="guc-log-timezone"/> or with <xref + linkend="guc-log-min-duration-statement"/>). The default value is + <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-md5-password-warnings" xreflabel="md5_password_warnings"> <term><varname>md5_password_warnings</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c index cbb85a27cc..8be4730750 100644 --- a/src/backend/libpq/crypt.c +++ b/src/backend/libpq/crypt.c @@ -23,6 +23,11 @@ #include "utils/builtins.h" #include "utils/syscache.h" #include "utils/timestamp.h" +#include "utils/guc.h" +#include "tcop/tcopprot.h" + +/* Enables usage warnings for plaintext passwords. */ +bool plaintext_password_warnings = true; /* Enables deprecation warnings for MD5 passwords. */ bool md5_password_warnings = true; @@ -176,6 +181,14 @@ encrypt_password(PasswordType target_type, const char *role, MAX_ENCRYPTED_PASSWORD_LEN))); } + if (plaintext_password_warnings && + get_password_type(password) == PASSWORD_TYPE_PLAINTEXT && + (log_statement == LOGSTMT_ALL || log_statement == LOGSTMT_DDL || log_min_duration_statement > -1)) + ereport(WARNING, + (errmsg("using a plaintext password in a query"), + errdetail("plaintext password may be logged."), + errhint("Refer to the PostgreSQL documentation for details about using encrypted password in queries."))); + if (md5_password_warnings && get_password_type(encrypted_password) == PASSWORD_TYPE_MD5) ereport(WARNING, diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 71448bb4fd..c033c03c4b 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2087,6 +2087,15 @@ struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, + { + {"plaintext_password_warnings", PGC_USERSET, CONN_AUTH_AUTH, + gettext_noop("Enables usage warnings for plaintext passwords."), + }, + &plaintext_password_warnings, + true, + NULL, NULL, NULL + }, + { {"md5_password_warnings", PGC_USERSET, CONN_AUTH_AUTH, gettext_noop("Enables deprecation warnings for MD5 passwords."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 079efa1baa..7e10b15379 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,6 +96,7 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 +#plaintext_password_warnings = on #md5_password_warnings = on # GSSAPI using Kerberos diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h index dee477428e..fb9f0309f1 100644 --- a/src/include/libpq/crypt.h +++ b/src/include/libpq/crypt.h @@ -25,6 +25,9 @@ */ #define MAX_ENCRYPTED_PASSWORD_LEN (512) +/* Enables usage warnings for plaintext passwords. */ +extern PGDLLIMPORT bool plaintext_password_warnings; + /* Enables deprecation warnings for MD5 passwords. */ extern PGDLLIMPORT bool md5_password_warnings; diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out index 46d4f9efe9..546fa2f086 100644 --- a/src/test/regress/expected/create_role.out +++ b/src/test/regress/expected/create_role.out @@ -63,8 +63,10 @@ GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION; CREATE ROLE regress_login LOGIN; CREATE ROLE regress_inherit INHERIT; CREATE ROLE regress_connection_limit CONNECTION LIMIT 5; +SET plaintext_password_warnings = off; CREATE ROLE regress_encrypted_password ENCRYPTED PASSWORD 'foo'; CREATE ROLE regress_password_null PASSWORD NULL; +SET plaintext_password_warnings = on; -- ok, backwards compatible noise words should be ignored CREATE ROLE regress_noiseword SYSID 12345; NOTICE: SYSID can no longer be specified diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out index 9bb3ab2818..94c4b73cbd 100644 --- a/src/test/regress/expected/password.out +++ b/src/test/regress/expected/password.out @@ -17,6 +17,7 @@ ALTER ROLE regress_passwd1 PASSWORD 'role_pwd1'; WARNING: setting an MD5-encrypted password DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL. HINT: Refer to the PostgreSQL documentation for details about migrating to another password type. +SET plaintext_password_warnings = off; CREATE ROLE regress_passwd2; ALTER ROLE regress_passwd2 PASSWORD 'role_pwd2'; WARNING: setting an MD5-encrypted password @@ -67,11 +68,13 @@ WARNING: setting an MD5-encrypted password DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL. HINT: Refer to the PostgreSQL documentation for details about migrating to another password type. -- already encrypted, use as they are +SET plaintext_password_warnings = on; ALTER ROLE regress_passwd1 PASSWORD 'md5cd3578025fe2c3d7ed1b9a9b26238b70'; WARNING: setting an MD5-encrypted password DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL. HINT: Refer to the PostgreSQL documentation for details about migrating to another password type. ALTER ROLE regress_passwd3 PASSWORD 'SCRAM-SHA-256$4096:VLK4RMaQLCvNtQ==$6YtlR4t69SguDiwFvbVgVZtuz6gpJQQqUMZ7IQJK5yI=:ps75jrHeYU4lXCcXI4O8oIdJ3eO8o2jirjruw9phBTo='; +SET plaintext_password_warnings = off; SET password_encryption = 'scram-sha-256'; -- create SCRAM secret ALTER ROLE regress_passwd4 PASSWORD 'foo'; @@ -171,3 +174,4 @@ SELECT rolname, rolpassword ---------+------------- (0 rows) +SET plaintext_password_warnings = on; diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql index 4491a28a8a..600d428e8e 100644 --- a/src/test/regress/sql/create_role.sql +++ b/src/test/regress/sql/create_role.sql @@ -48,8 +48,10 @@ GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION; CREATE ROLE regress_login LOGIN; CREATE ROLE regress_inherit INHERIT; CREATE ROLE regress_connection_limit CONNECTION LIMIT 5; +SET plaintext_password_warnings = off; CREATE ROLE regress_encrypted_password ENCRYPTED PASSWORD 'foo'; CREATE ROLE regress_password_null PASSWORD NULL; +SET plaintext_password_warnings = on; -- ok, backwards compatible noise words should be ignored CREATE ROLE regress_noiseword SYSID 12345; diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql index e7a9804545..618d5368a8 100644 --- a/src/test/regress/sql/password.sql +++ b/src/test/regress/sql/password.sql @@ -12,6 +12,7 @@ SET password_encryption = 'scram-sha-256'; -- ok SET password_encryption = 'md5'; CREATE ROLE regress_passwd1; ALTER ROLE regress_passwd1 PASSWORD 'role_pwd1'; +SET plaintext_password_warnings = off; CREATE ROLE regress_passwd2; ALTER ROLE regress_passwd2 PASSWORD 'role_pwd2'; SET password_encryption = 'scram-sha-256'; @@ -46,8 +47,10 @@ SET password_encryption = 'md5'; -- encrypt with MD5 ALTER ROLE regress_passwd2 PASSWORD 'foo'; -- already encrypted, use as they are +SET plaintext_password_warnings = on; ALTER ROLE regress_passwd1 PASSWORD 'md5cd3578025fe2c3d7ed1b9a9b26238b70'; ALTER ROLE regress_passwd3 PASSWORD 'SCRAM-SHA-256$4096:VLK4RMaQLCvNtQ==$6YtlR4t69SguDiwFvbVgVZtuz6gpJQQqUMZ7IQJK5yI=:ps75jrHeYU4lXCcXI4O8oIdJ3eO8o2jirjruw9phBTo='; +SET plaintext_password_warnings = off; SET password_encryption = 'scram-sha-256'; -- create SCRAM secret @@ -118,3 +121,5 @@ SELECT rolname, rolpassword FROM pg_authid WHERE rolname LIKE 'regress_passwd%' ORDER BY rolname, rolpassword; + +SET plaintext_password_warnings = on; -- 2.48.1
