| From: | Japin Li <japinli(at)hotmail(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | PostgreSQL-development <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, Darold Gilles <gilles(at)darold(dot)net> |
| Subject: | Re: Fix md5_password_warnings for role/database settings |
| Date: | 2026-06-10 11:58:14 |
| Message-ID: | SY7PR01MB1092144AACF3AD3A07E246CC8B61A2@SY7PR01MB10921.ausprd01.prod.outlook.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> 在 2026年6月10日,19:39,Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> 写道:
>
>
>
>> On Jun 10, 2026, at 18:22, Japin Li <japinli(at)hotmail(dot)com> wrote:
>>
>>
>> Hi, Chao
>>
>>> On Wed, 10 Jun 2026 at 14:26, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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 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.
I have missed that conversation — thanks for bringing it up.
> PSA v2 - addressed Japin’s first comment.
>
Thanks for updating the patch, LGTM.
> [1] https://postgr.es/m/6E83A384-89B0-4141-887F-E54C02B2CACE@gmail.com
>
> 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>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ewan Young | 2026-06-10 12:16:30 | Discard ORDER BY/DISTINCT when an ANY/IN sublink is pulled up to a join |
| Previous Message | Dean Rasheed | 2026-06-10 11:54:06 | Re: [PATCH v1] [BUG #19516] Skip whole-row projection shortcut for OLD/NEW returning type |