Fix md5_password_warnings for role and database settings

MD5 authentication warnings are queued during authentication, before
startup options and role/database settings have been applied. The code
checked md5_password_warnings at queue time, so settings such as
ALTER ROLE ... SET md5_password_warnings = off did not suppress the
warning, even though the established session showed the GUC as off.

Keep the connection-warning infrastructure generic by allowing each
queued warning to carry an optional filter callback. Evaluate that
callback when warnings are emitted, after startup options and
role/database settings have been processed.

Use this for MD5 authentication warnings, while leaving password
expiration warnings unchanged. Add test coverage for an MD5-authenticated
role with md5_password_warnings disabled.

Author: Chao Li <lic@highgo.com>
Reviewed-by: Japin Li <japinli@hotmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/AE46E42D-5966-4D76-9E64-95EAB01B9FB5@gmail.com

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 739a2e6fa8bd8cbd9d576d2f6f75d9289d7814cf..28857773d9ccc655d81cb1712d56dab603a310ff 100644 (file)
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -32,6 +32,8 @@ int                   password_expiration_warning_threshold = 604800;
 /* Enables deprecation warnings for MD5 passwords. */
 bool           md5_password_warnings = true;
 
+static bool md5_password_warning_enabled(void);
+
 /*
  * Fetch stored password for a user, for authentication.
  *
@@ -137,7 +139,7 @@ get_role_password(const char *role, const char **logdetail)
                                detail = psprintf(_("The password for role \"%s\" will expire in less than 1 minute."),
                                                                  role);
 
-                       StoreConnectionWarning(warning, detail);
+                       StoreConnectionWarning(warning, detail, NULL);
 
                        MemoryContextSwitchTo(oldcontext);
                }
@@ -296,22 +298,19 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
        if (strlen(client_pass) == strlen(crypt_pwd) &&
                timingsafe_bcmp(client_pass, crypt_pwd, strlen(crypt_pwd)) == 0)
        {
+               MemoryContext oldcontext;
+               char       *warning;
+               char       *detail;
+
                retval = STATUS_OK;
 
-               if (md5_password_warnings)
-               {
-                       MemoryContext oldcontext;
-                       char       *warning;
-                       char       *detail;
+               oldcontext = MemoryContextSwitchTo(TopMemoryContext);
 
-                       oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+               warning = pstrdup(_("authenticated with an MD5-encrypted password"));
+               detail = pstrdup(_("MD5 password support is deprecated and will be removed in a future release of PostgreSQL."));
+               StoreConnectionWarning(warning, detail, md5_password_warning_enabled);
 
-                       warning = pstrdup(_("authenticated with an MD5-encrypted password"));
-                       detail = pstrdup(_("MD5 password support is deprecated and will be removed in a future release of PostgreSQL."));
-                       StoreConnectionWarning(warning, detail);
-
-                       MemoryContextSwitchTo(oldcontext);
-               }
+               MemoryContextSwitchTo(oldcontext);
        }
        else
        {
@@ -323,6 +322,12 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
        return retval;
 }
 
+static bool
+md5_password_warning_enabled(void)
+{
+       return md5_password_warnings;
+}
+
 /*
  * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
  *

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index c1457eb34f02b28dd7ee781b2f5e2a7f917f972c..3d8c9bdebd5594bdada217c5f10acf33adb26edd 100644 (file)
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -74,9 +74,15 @@
 /* has this backend called EmitConnectionWarnings()? */
 static bool ConnectionWarningsEmitted;
 
-/* content of warnings to send via EmitConnectionWarnings() */
-static List *ConnectionWarningMessages;
-static List *ConnectionWarningDetails;
+typedef struct ConnectionWarning
+{
+       char       *message;
+       char       *detail;
+       ConnectionWarningFilter filter;
+} ConnectionWarning;
+
+/* warnings to send via EmitConnectionWarnings() */
+static List *ConnectionWarnings;
 
 static HeapTuple GetDatabaseTuple(const char *dbname);
 static HeapTuple GetDatabaseTupleByOid(Oid dboid);
@@ -1499,15 +1505,19 @@ ThereIsAtLeastOneRole(void)
 
 /*
  * Stores a warning message to be sent later via EmitConnectionWarnings().
- * Both msg and detail must be non-NULL.
+ * Both msg and detail must be non-NULL.  If filter is non-NULL, it is called
+ * just before the warning is emitted, after startup and role/database settings
+ * have been applied.
  *
- * NB: Caller should ensure the strings are allocated in a long-lived context
- * like TopMemoryContext.
+ * NB: Caller should ensure the strings are palloc'd in a long-lived context
+ * like TopMemoryContext.  This function takes ownership of the strings, which
+ * will be pfree'd in EmitConnectionWarnings().
  */
 void
-StoreConnectionWarning(char *msg, char *detail)
+StoreConnectionWarning(char *msg, char *detail, ConnectionWarningFilter filter)
 {
        MemoryContext oldcontext;
+       ConnectionWarning *warning;
 
        Assert(msg);
        Assert(detail);
@@ -1517,8 +1527,11 @@ StoreConnectionWarning(char *msg, char *detail)
 
        oldcontext = MemoryContextSwitchTo(TopMemoryContext);
 
-       ConnectionWarningMessages = lappend(ConnectionWarningMessages, msg);
-       ConnectionWarningDetails = lappend(ConnectionWarningDetails, detail);
+       warning = palloc_object(ConnectionWarning);
+       warning->message = msg;
+       warning->detail = detail;
+       warning->filter = filter;
+       ConnectionWarnings = lappend(ConnectionWarnings, warning);
 
        MemoryContextSwitchTo(oldcontext);
 }
@@ -1532,22 +1545,23 @@ StoreConnectionWarning(char *msg, char *detail)
 static void
 EmitConnectionWarnings(void)
 {
-       ListCell   *lc_msg;
-       ListCell   *lc_detail;
-
        if (ConnectionWarningsEmitted)
                elog(ERROR, "EmitConnectionWarnings() called more than once");
        else
                ConnectionWarningsEmitted = true;
 
-       forboth(lc_msg, ConnectionWarningMessages,
-                       lc_detail, ConnectionWarningDetails)
+       foreach_ptr(ConnectionWarning, warning, ConnectionWarnings)
        {
-               ereport(WARNING,
-                               (errmsg("%s", (char *) lfirst(lc_msg)),
-                                errdetail("%s", (char *) lfirst(lc_detail))));
+               if (warning->filter == NULL || warning->filter())
+                       ereport(WARNING,
+                                       (errmsg("%s", warning->message),
+                                        errdetail("%s", warning->detail)));
+
+               pfree(warning->message);
+               pfree(warning->detail);
+               pfree(warning);
        }
 
-       list_free_deep(ConnectionWarningMessages);
-       list_free_deep(ConnectionWarningDetails);
+       list_free(ConnectionWarnings);
+       ConnectionWarnings = NIL;
 }

diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 7de0a11540236b66eca5230d486baa71b46db059..7170a4bff9896b3bfc0a68d9d17b4e22de85acfa 100644 (file)
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -516,7 +516,9 @@ extern void InitPostgres(const char *in_dbname, Oid dboid,
                                                 uint32 flags,
                                                 char *out_dbname);
 extern void BaseInit(void);
-extern void StoreConnectionWarning(char *msg, char *detail);
+typedef bool (*ConnectionWarningFilter) (void);
+extern void StoreConnectionWarning(char *msg, char *detail,
+                                                                  ConnectionWarningFilter filter);
 
 /* in utils/init/miscinit.c */
 extern PGDLLIMPORT bool IgnoreSystemIndexes;

diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 69ed4919b16e8996a7a93d2faa2d4610fec65112..fca78fc4d6a3324e5a8970e402b519f0f2560dcd 100644 (file)
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -157,6 +157,14 @@ is( $node->psql(
        ),
        $md5_works ? 0 : 3,
        'created user with md5 password');
+is( $node->psql(
+               'postgres',
+               "SET password_encryption='md5';
+                CREATE ROLE md5_role_no_warnings LOGIN PASSWORD 'pass';
+                ALTER ROLE md5_role_no_warnings SET md5_password_warnings = off;"
+       ),
+       $md5_works ? 0 : 3,
+       'created user with md5 password and MD5 warnings disabled');
 # Set up a table for tests of SYSTEM_USER.
 $node->safe_psql(
        'postgres',
@@ -504,6 +512,15 @@ SKIP:
                expected_stderr => qr/authenticated with an MD5-encrypted password/,
                log_like =>
                  [qr/connection authenticated: identity="md5_role" method=md5/]);
+
+       $node->connect_ok(
+               'user=md5_role_no_warnings',
+               'md5 with warnings disabled',
+               sql => 'SHOW md5_password_warnings',
+               expected_stdout => qr/^off$/,
+               log_like => [
+                       qr/connection authenticated: identity="md5_role_no_warnings" method=md5/
+               ]);
 }
 
 # require_auth succeeds with SCRAM required.

diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 8cf40c87043f28c75e890782b560350933470b10..f9eb23e52c9d7fd4b185c02d4e7559f31b1c403c 100644 (file)
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -524,6 +524,7 @@ ConnStatusType
 ConnType
 ConnectionStateEnum
 ConnectionTiming
+ConnectionWarning
 ConsiderSplitContext
 Const
 ConstrCheck