Re: Fix md5_password_warnings for role/database settings - Mailing list pgsql-hackers

Hi, Chao

On Wed, 10 Jun 2026 at 14:26, Chao Li <li.evan.chao@gmail.com> wrote:
> Hi,
>
> While testing “[bc60ee860] Warn upon successful MD5 password authentication”, I found a small issue.
>
> This feature emits a warning based on the existing GUC
> md5_password_warnings, but it queues the message in
> md5_crypt_verify(), before GUC values are loaded by
> process_startup_options() and process_settings(). As a result,
> settings loaded later during connection startup, such as startup
> options or ALTER ROLE/ALTER DATABASE settings, are not honored for
> this warning.
>
> Here is a repro:
>
> 1. Edit pg_hba.conf, add this line:
> ```
> local   postgres        md5_role                                md5
> ```
>
> 2. Setup in session 1:
> ```
> evantest=# set password_encryption='md5';
> SET
> evantest=# create role md5_role login password 'pass';
> 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.
> CREATE ROLE
> evantest=#
> evantest=# alter role md5_role set md5_password_warnings =0;
> ALTER ROLE
> evantest=# select pg_reload_conf();  -- reload pg_hba.conf as I didn’t restart the server
>  pg_reload_conf
> ----------------
>  t
> (1 row)
> ```
>
> 3. Connect as md5_role:
> ```
> % PGPASSWORD=pass psql -d postgres -U md5_role -X -qAt -c “show md5_password_warnings"
> WARNING:  authenticated with an MD5-encrypted password
> DETAIL:  MD5 password support is deprecated and will be removed in a future release of PostgreSQL.
> off
> ```
>
> As we can see, although the role’s md5_password_warnings setting is off, the warning message is still shown.
>
> This feature uses the connection warning infrastructure introduced by 1d92e0c2cc, so fixing the problem requires
enhancingthat infrastructure. 
>
> In the current implementation, there are two lists:
> ConnectionWarningMessages and ConnectionWarningDetails. The attached
> patch combines them into one list and adds a filter function to each
> list member, so the filter can be applied in
> EmitConnectionWarnings(). With this mechanism, the warning emitted
> upon successful MD5 authentication is checked against the final value
> of md5_password_warnings, while 1d92e0c2cc’s password expiration
> warning logic remains unchanged.
>

I'm in favor of this idea.

> See the attached patch for details.

A few comments:

1.
 EmitConnectionWarnings(void)
 {
-    ListCell   *lc_msg;
-    ListCell   *lc_detail;
+    ListCell   *lc;

     if (ConnectionWarningsEmitted)
         elog(ERROR, "EmitConnectionWarnings() called more than once");
     else
         ConnectionWarningsEmitted = true;

-    forboth(lc_msg, ConnectionWarningMessages,
-            lc_detail, ConnectionWarningDetails)
+    foreach(lc, ConnectionWarnings)
     {
-        ereport(WARNING,
-                (errmsg("%s", (char *) lfirst(lc_msg)),
-                 errdetail("%s", (char *) lfirst(lc_detail))));
+        ConnectionWarning *warning = lfirst(lc);
+

Perhaps we could use foreach_ptr(ConnectionWarning, warning, ConnectionWarnings)
to simplify the code.

2.
StoreConnectionWarning() states that the caller should ensure the strings are
allocated in a long-lived context.  Since the two existing calls already use
TopMemoryContext, should the function always switch the memory context
internally?


>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.