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. > 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.
