Thread
Commits
-
Fix md5_password_warnings for role and database settings
- 44196fd4f378 19 (unreleased) landed
-
Fix md5_password_warnings for role/database settings
Chao Li <li.evan.chao@gmail.com> — 2026-06-10T06:26:19Z
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. See the attached patch for details. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Fix md5_password_warnings for role/database settings
Japin Li <japinli@hotmail.com> — 2026-06-10T10:22:01Z
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 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. -
Re: Fix md5_password_warnings for role/database settings
Chao Li <li.evan.chao@gmail.com> — 2026-06-10T11:38:23Z
> On Jun 10, 2026, at 18:22, Japin Li <japinli@hotmail.com> wrote: > > > 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 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/6E83A384-89B0-4141-887F-E54C02B2CACE@gmail.com Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix md5_password_warnings for role/database settings
Japin Li <japinli@hotmail.com> — 2026-06-10T11:58:14Z
> 在 2026年6月10日，19:39，Chao Li <li.evan.chao@gmail.com> 写道： > > > >> On Jun 10, 2026, at 18:22, Japin Li <japinli@hotmail.com> wrote: >> >> >> 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 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> -
Re: Fix md5_password_warnings for role/database settings
Fujii Masao <masao.fujii@gmail.com> — 2026-06-10T15:12:31Z
On Wed, Jun 10, 2026 at 8:58 PM Japin Li <japinli@hotmail.com> wrote: > > PSA v2 - addressed Japin’s first comment. > > > Thanks for updating the patch, LGTM. The patch basically looks good to me! I just have three minor review comments. + my ($ret, $stdout, $stderr) = $node->psql( + 'postgres', + 'SELECT 1', + connstr => 'user=md5_role_no_warnings', + extra_params => ['-w'], + on_error_stop => 0); + is($ret, 0, 'md5 with warnings disabled'); + unlike( + $stderr, + qr/authenticated with an MD5-encrypted password/, + 'md5 with warnings disabled: no MD5 authentication warning'); For this test, would it be better to use connect_ok() instead of psql(), like this? That would let us verify more behavior at once: the connection succeeds, stderr is empty (i.e., no MD5 warning is emitted), SHOW md5_password_warnings returns off, and the server log still shows 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/]); * NB: Caller should ensure the strings are allocated in a long-lived context - * like TopMemoryContext. + * like TopMemoryContext. This function takes ownership of the strings, which + * will be freed in EmitConnectionWarnings(). Very minor comment: I'd suggest changing "allocated" to "palloc'd" and "freed" to "pfree'd", to avoid future callers passing some non-pfreeable memory unexpectedly. +typedef struct ConnectionWarning Since this patch introduces a new typedef, ConnectionWarning, typedefs.list should probably be updated as well. Even if we forget to do that, it will eventually get updated later, but I think it's better to update it manually when possible so that pgindent works correctly before then. Regards, -- Fujii Masao
-
Re: Fix md5_password_warnings for role/database settings
Chao Li <li.evan.chao@gmail.com> — 2026-06-10T23:56:59Z
> On Jun 10, 2026, at 23:12, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Wed, Jun 10, 2026 at 8:58 PM Japin Li <japinli@hotmail.com> wrote: >>> PSA v2 - addressed Japin’s first comment. >>> >> Thanks for updating the patch, LGTM. > > The patch basically looks good to me! Thanks for your review. > I just have three minor review comments. > > > + my ($ret, $stdout, $stderr) = $node->psql( > + 'postgres', > + 'SELECT 1', > + connstr => 'user=md5_role_no_warnings', > + extra_params => ['-w'], > + on_error_stop => 0); > + is($ret, 0, 'md5 with warnings disabled'); > + unlike( > + $stderr, > + qr/authenticated with an MD5-encrypted password/, > + 'md5 with warnings disabled: no MD5 authentication warning'); > > For this test, would it be better to use connect_ok() instead of > psql(), like this? That would let us verify more behavior at once: > the connection succeeds, stderr is empty (i.e., no MD5 warning is > emitted), SHOW md5_password_warnings returns off, and the server > log still shows 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/]); > Agreed. > > * NB: Caller should ensure the strings are allocated in a long-lived context > - * like TopMemoryContext. > + * like TopMemoryContext. This function takes ownership of the strings, which > + * will be freed in EmitConnectionWarnings(). > > Very minor comment: I'd suggest changing "allocated" to "palloc'd" > and "freed" to "pfree'd", to avoid future callers passing some non-pfreeable > memory unexpectedly. > Okay, changed. > > +typedef struct ConnectionWarning > > Since this patch introduces a new typedef, ConnectionWarning, > typedefs.list should probably be updated as well. Even if we forget > to do that, it will eventually get updated later, but I think it's > better to update it manually when possible so that pgindent works > correctly before then. > Ah, completely forgot that. Added. PFA v3: * Addressed all Fujii’s comments * Changed a palloc to palloc_object Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Fix md5_password_warnings for role/database settings
Fujii Masao <masao.fujii@gmail.com> — 2026-06-11T23:35:02Z
On Thu, Jun 11, 2026 at 8:57 AM Chao Li <li.evan.chao@gmail.com> wrote: > PFA v3: > * Addressed all Fujii’s comments > * Changed a palloc to palloc_object Thanks for updating the patch! LGTM. I've pushed it. Regards, -- Fujii Masao
-
Re: Fix md5_password_warnings for role/database settings
Chao Li <li.evan.chao@gmail.com> — 2026-06-11T23:45:25Z
> On Jun 12, 2026, at 07:35, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Thu, Jun 11, 2026 at 8:57 AM Chao Li <li.evan.chao@gmail.com> wrote: >> PFA v3: >> * Addressed all Fujii’s comments >> * Changed a palloc to palloc_object > > Thanks for updating the patch! LGTM. I've pushed it. > > Regards, > > -- > Fujii Masao Thanks a lot for pushing. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/