> On Jun 10, 2026, at 18:22, Japin Li <[email protected]> wrote: > > > Hi, Chao > > On Wed, 10 Jun 2026 at 14:26, Chao Li <[email protected]> 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 enhancing that 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.
Thanks for your review.
>
>> 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.
Agreed.
>
> 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?
>
I raised the same comment when I reviewed the original patch of 1d92e0c2cc, and
the comment was addressed by adding the header comment, see [1] commit 3. So,
I’d not touch this part.
PSA v2 - addressed Japin’s first comment.
[1] https://postgr.es/m/[email protected]
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v2-0001-Fix-md5_password_warnings-for-role-and-database-s.patch
Description: Binary data
