pg_upgrade: warn about roles with md5 passwords
Since MD5 passwords are slated to be marked as deprecated in v18, I figured
it might be a good idea to add a check for roles with MD5 passwords to
pg_upgrade. I'm tempted to suggest that we apply this to v18, but I'm
content to leave it for v19 if nobody feels too strongly about it.
The one thing I don't like about this check is that it's probably not great
from a security standpoint to effectively announce which roles have MD5
passwords. However, pg_upgrade must be run as the bootstrap superuser, and
we'll need to start failing for MD5 passwords at some point, so I'm not
sure how worried to be about that.
One other thing I noticed is that checks that only emit warnings, like
check_for_unicode_update(), require using --retain in order to see the
generated report file. Otherwise, pg_upgrade deletes the files after
successful completion. I don't know how worried to be about this, either,
but I did run into it while testing the attached patch, so it seemed worth
bringing up.
--
nathan
Attachments:
v1-0001-pg_upgrade-Warn-about-roles-with-MD5-passwords.patchtext/plain; charset=us-asciiDownload
From 011fe65c172d8788b1b6f2447b658717b84db6f0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 2 Jun 2025 10:10:03 -0500
Subject: [PATCH v1 1/1] pg_upgrade: Warn about roles with MD5 passwords.
---
src/bin/pg_upgrade/check.c | 66 ++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 940fc77fc2e..9da2b5977e1 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ static void check_new_cluster_logical_replication_slots(void);
static void check_new_cluster_subscription_configuration(void);
static void check_old_cluster_for_valid_slots(void);
static void check_old_cluster_subscription_state(void);
+static void check_for_md5_passwords(ClusterInfo *cluster);
/*
* DataTypesUsageChecks - definitions of data type checks for the old cluster
@@ -685,6 +686,12 @@ check_and_dump_old_cluster(void)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 905)
check_for_pg_role_prefix(&old_cluster);
+ /*
+ * MD5 password support is deprecated. Warn if any roles have MD5
+ * passwords.
+ */
+ check_for_md5_passwords(&old_cluster);
+
/*
* While not a check option, we do this now because this is the only time
* the old server is running.
@@ -2272,3 +2279,62 @@ check_old_cluster_subscription_state(void)
else
check_ok();
}
+
+/*
+ * check_for_md5_passwords()
+ *
+ * As of v18, MD5 password support is marked as deprecated and to-be-removed in
+ * a future major release.
+ */
+static void
+check_for_md5_passwords(ClusterInfo *cluster)
+{
+ PGresult *res;
+ PGconn *conn = connectToServer(cluster, "template1");
+ int ntups;
+ int i_roloid;
+ int i_rolname;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for roles with MD5 passwords");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "roles_with_md5_passwords.txt");
+
+ res = executeQueryOrDie(conn,
+ "SELECT oid AS roloid, rolname "
+ "FROM pg_catalog.pg_authid "
+ "WHERE rolpassword ~ '^md5'");
+
+ ntups = PQntuples(res);
+ i_roloid = PQfnumber(res, "roloid");
+ i_rolname = PQfnumber(res, "rolname");
+ for (int rowno = 0; rowno < ntups; rowno++)
+ {
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", output_path);
+ fprintf(script, "%s (oid=%s)\n",
+ PQgetvalue(res, rowno, i_rolname),
+ PQgetvalue(res, rowno, i_roloid));
+ }
+
+ PQclear(res);
+
+ PQfinish(conn);
+
+ if (script)
+ {
+ fclose(script);
+ report_status(PG_WARNING, "warning");
+ pg_log(PG_WARNING,
+ "Your installation contains roles with MD5 passwords.\n"
+ "Support for MD5-encrypted passwords is deprecated and will be\n"
+ "removed in a future release of PostgreSQL. A list of roles\n"
+ "with MD5 passwords is in the file:\n"
+ " %s", output_path);
+ }
+ else
+ check_ok();
+}
--
2.39.5 (Apple Git-154)
On Mon, 2025-06-02 at 10:32 -0500, Nathan Bossart wrote:
Since MD5 passwords are slated to be marked as deprecated in v18, I
figured
it might be a good idea to add a check for roles with MD5 passwords
to
pg_upgrade. I'm tempted to suggest that we apply this to v18, but
I'm
content to leave it for v19 if nobody feels too strongly about it.
That seems like a reasonable thing to do for v18 to me.
The one thing I don't like about this check is that it's probably not
great
from a security standpoint to effectively announce which roles have
MD5
passwords.
Do you have a specific concern, or is that more of a general concern?
One other thing I noticed is that checks that only emit warnings,
like
check_for_unicode_update(), require using --retain in order to see
the
generated report file.
Should we automatically retain files associated with warnings, or copy
them to a different location?
Regards,
Jeff Davis
On Mon, Jun 02, 2025 at 09:45:55AM -0700, Jeff Davis wrote:
On Mon, 2025-06-02 at 10:32 -0500, Nathan Bossart wrote:
The one thing I don't like about this check is that it's probably not
great
from a security standpoint to effectively announce which roles have
MD5
passwords.Do you have a specific concern, or is that more of a general concern?
General.
One other thing I noticed is that checks that only emit warnings,
like
check_for_unicode_update(), require using --retain in order to see
the
generated report file.Should we automatically retain files associated with warnings, or copy
them to a different location?
That seems worth considering. Another option could be to just document
that files generated for warnings will be lost without --retain. WDYT?
--
nathan
On Mon, 2025-06-02 at 12:04 -0500, Nathan Bossart wrote:
That seems worth considering. Another option could be to just
document
that files generated for warnings will be lost without --retain.
WDYT?
I haven't looked into it yet, but copying the files to an
"upgrade_warnings" directory sounds like a reasonable way to go.
Regards,
Jeff Davis
On Mon, Jun 02, 2025 at 12:41:47PM -0700, Jeff Davis wrote:
On Mon, 2025-06-02 at 12:04 -0500, Nathan Bossart wrote:
That seems worth considering.� Another option could be to just
document
that files generated for warnings will be lost without --retain.�
WDYT?I haven't looked into it yet, but copying the files to an
"upgrade_warnings" directory sounds like a reasonable way to go.
So, right now the upgrade directory will be something like:
./pg_upgrade_output.d/20250602T095620.137
cleanup_output_dirs() recursively deletes everything in the timestamp
directory (and the directory itself), and then it cleans up
pg_upgrade_output.d if it is empty. My first thought would be to teach
cleanup_output_dirs() to delete everything except for files with the ".txt"
suffix (so that future warning files are handled, too).
This is a little weird because users will be forced to delete the leftover
directories and warning files manually, but I'm not sure it's worth adding
different --retain modes for that (e.g., --retain=all, --retain=warnings,
--retain=none).
--
nathan
On Mon, Jun 02, 2025 at 02:55:40PM -0500, Nathan Bossart wrote:
So, right now the upgrade directory will be something like:
./pg_upgrade_output.d/20250602T095620.137
cleanup_output_dirs() recursively deletes everything in the timestamp
directory (and the directory itself), and then it cleans up
pg_upgrade_output.d if it is empty. My first thought would be to teach
cleanup_output_dirs() to delete everything except for files with the ".txt"
suffix (so that future warning files are handled, too).
pg_upgrade has always removed the log and dump files by default if not
specifying --retain, even before 4fff78f00910 that has only made the
base directory name dynamically-generated. Before using the
timestamp-based folder name, note that we've had only one rmtree()
done on log_opts.basedir.
This is a little weird because users will be forced to delete the leftover
directories and warning files manually, but I'm not sure it's worth adding
different --retain modes for that (e.g., --retain=all, --retain=warnings,
--retain=none).
I'm not sure that this is necessary. Only requiring one to use
--retain sounds kind of enough to me.
Saying that, warning users if they have MD5 passwords is a good idea,
because we would already have the code in place to flip it to an error
once/if MD5 is entirely removed. An upgrade failure retains the log
and dump folders around, meaning that users would be able to know the
list of users all the time.
--
Michael
On Mon, 2025-06-02 at 09:45 -0700, Jeff Davis wrote:
On Mon, 2025-06-02 at 10:32 -0500, Nathan Bossart wrote:
Since MD5 passwords are slated to be marked as deprecated in v18, I figured
it might be a good idea to add a check for roles with MD5 passwords to
pg_upgrade. I'm tempted to suggest that we apply this to v18, but I'm
content to leave it for v19 if nobody feels too strongly about it.That seems like a reasonable thing to do for v18 to me.
+1
Laurenz Albe
On Tue, Jun 03, 2025 at 01:38:49PM +0900, Michael Paquier wrote:
I'm not sure that this is necessary. Only requiring one to use
--retain sounds kind of enough to me.
Yeah, maybe we should just leave it alone for now.
Saying that, warning users if they have MD5 passwords is a good idea,
because we would already have the code in place to flip it to an error
once/if MD5 is entirely removed. An upgrade failure retains the log
and dump folders around, meaning that users would be able to know the
list of users all the time.
Right. I'll bring this up with the others on the RMT today.
--
nathan
+1 for this, and +1 for doing this still in v18.
On 03/06/2025 17:12, Nathan Bossart wrote:
On Tue, Jun 03, 2025 at 01:38:49PM +0900, Michael Paquier wrote:
I'm not sure that this is necessary. Only requiring one to use
--retain sounds kind of enough to me.Yeah, maybe we should just leave it alone for now.
I have no direct opinion on how the logging should work, but some thoughts:
- It's better to print a warning somewhere, even if you need to use
--retain to see it, than not doing it at all. At least there's a
fighting chance that someone might see it.
- If we're worried about printing a list of users with md5 passwords, we
could just say "there are users with md5 passwords" without naming them.
I'm not too worried though, pg_upgrade has full access to the data anyway.
--
Heikki Linnakangas
Neon (https://neon.tech)
ISTM that warnings emitted by pg_upgrade will be seen by about
0.1% of users anyway, since packagers typically wrap scripts
around that.
If we really want to be in peoples' face about this, the thing
to do is to print a warning every time they log in with an MD5
password. Also, to Michael's point, that really would be exactly
the same place where the eventual "sorry, not supported anymore"
message will be.
If we're not ready to be in their face that much, maybe the
removal isn't so close after all.
regards, tom lane
On Tue, Jun 03, 2025 at 10:34:06AM -0400, Tom Lane wrote:
If we really want to be in peoples' face about this, the thing
to do is to print a warning every time they log in with an MD5
password. Also, to Michael's point, that really would be exactly
the same place where the eventual "sorry, not supported anymore"
message will be.
I held off on this because I was worried it might be far too noisy. That
does seem like it has the best chance of getting folks' attention, though.
If it's too noisy, users can always turn off the warnings.
If we're not ready to be in their face that much, maybe the
removal isn't so close after all.
I think some hackers would like to see it removed in ~v20. Personally, I'd
rather give it at least a few years. SCRAM was added in v10 and made
default in v14, and MD5 is likely going to be marked deprecated in v18.
So, maybe ~v22 is where we should plan to remove it.
--
nathan
On Tue, Jun 03, 2025 at 09:43:59AM -0500, Nathan Bossart wrote:
On Tue, Jun 03, 2025 at 10:34:06AM -0400, Tom Lane wrote:
If we really want to be in peoples' face about this, the thing
to do is to print a warning every time they log in with an MD5
password. Also, to Michael's point, that really would be exactly
the same place where the eventual "sorry, not supported anymore"
message will be.I held off on this because I was worried it might be far too noisy. That
does seem like it has the best chance of getting folks' attention, though.
If it's too noisy, users can always turn off the warnings.
Here is a draft-grade patch that adds a WARNING upon successful
authentication with an MD5 password. It's a little hacky because AFAICT we
need to wait until well after authentication (for GUCs to be set up, etc.)
before we actually emit the WARNING. When the time comes to remove MD5
password support completely, we'll need to do something like modify
CheckMD5Auth() to always return STATUS_ERROR with an appropriate logdetail
message.
What do folks think about doing this?
--
nathan
Attachments:
v2-0001-pg_upgrade-Warn-about-roles-with-MD5-passwords.patchtext/plain; charset=us-asciiDownload
From ccf33e5f93dd120a0975e7e0039c524ebcca2a3f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 2 Jun 2025 10:10:03 -0500
Subject: [PATCH v2 1/2] pg_upgrade: Warn about roles with MD5 passwords.
---
src/bin/pg_upgrade/check.c | 66 ++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 940fc77fc2e..9da2b5977e1 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ static void check_new_cluster_logical_replication_slots(void);
static void check_new_cluster_subscription_configuration(void);
static void check_old_cluster_for_valid_slots(void);
static void check_old_cluster_subscription_state(void);
+static void check_for_md5_passwords(ClusterInfo *cluster);
/*
* DataTypesUsageChecks - definitions of data type checks for the old cluster
@@ -685,6 +686,12 @@ check_and_dump_old_cluster(void)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 905)
check_for_pg_role_prefix(&old_cluster);
+ /*
+ * MD5 password support is deprecated. Warn if any roles have MD5
+ * passwords.
+ */
+ check_for_md5_passwords(&old_cluster);
+
/*
* While not a check option, we do this now because this is the only time
* the old server is running.
@@ -2272,3 +2279,62 @@ check_old_cluster_subscription_state(void)
else
check_ok();
}
+
+/*
+ * check_for_md5_passwords()
+ *
+ * As of v18, MD5 password support is marked as deprecated and to-be-removed in
+ * a future major release.
+ */
+static void
+check_for_md5_passwords(ClusterInfo *cluster)
+{
+ PGresult *res;
+ PGconn *conn = connectToServer(cluster, "template1");
+ int ntups;
+ int i_roloid;
+ int i_rolname;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for roles with MD5 passwords");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "roles_with_md5_passwords.txt");
+
+ res = executeQueryOrDie(conn,
+ "SELECT oid AS roloid, rolname "
+ "FROM pg_catalog.pg_authid "
+ "WHERE rolpassword ~ '^md5'");
+
+ ntups = PQntuples(res);
+ i_roloid = PQfnumber(res, "roloid");
+ i_rolname = PQfnumber(res, "rolname");
+ for (int rowno = 0; rowno < ntups; rowno++)
+ {
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", output_path);
+ fprintf(script, "%s (oid=%s)\n",
+ PQgetvalue(res, rowno, i_rolname),
+ PQgetvalue(res, rowno, i_roloid));
+ }
+
+ PQclear(res);
+
+ PQfinish(conn);
+
+ if (script)
+ {
+ fclose(script);
+ report_status(PG_WARNING, "warning");
+ pg_log(PG_WARNING,
+ "Your installation contains roles with MD5 passwords.\n"
+ "Support for MD5-encrypted passwords is deprecated and will be\n"
+ "removed in a future release of PostgreSQL. A list of roles\n"
+ "with MD5 passwords is in the file:\n"
+ " %s", output_path);
+ }
+ else
+ check_ok();
+}
--
2.39.5 (Apple Git-154)
v2-0002-WIP-add-warning-upon-authentication-with-MD5-pass.patchtext/plain; charset=us-asciiDownload
From d381840ddc1f80621955c252ede49ad0e0a38608 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 3 Jun 2025 11:52:03 -0500
Subject: [PATCH v2 2/2] [WIP] add warning upon authentication with MD5
password
---
src/backend/libpq/crypt.c | 17 +++++++++++++++++
src/backend/utils/init/postinit.c | 3 +++
src/include/libpq/crypt.h | 1 +
src/test/authentication/t/001_password.pl | 1 +
4 files changed, 22 insertions(+)
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index f6b641e726e..90050dcfc12 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -27,6 +27,8 @@
/* Enables deprecation warnings for MD5 passwords. */
bool md5_password_warnings = true;
+static bool used_md5_auth = false;
+
/*
* Fetch stored password for a user, for authentication.
*
@@ -210,6 +212,8 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
Assert(md5_salt_len > 0);
+ used_md5_auth = true;
+
if (get_password_type(shadow_pass) != PASSWORD_TYPE_MD5)
{
/* incompatible password hash format. */
@@ -242,6 +246,19 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
return retval;
}
+void
+warn_if_md5_auth(void)
+{
+ if (md5_password_warnings && used_md5_auth)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING_DEPRECATED_FEATURE),
+ errmsg("authenticated with an MD5-encrypted password"),
+ errdetail("MD5 password support is deprecated and will be removed in a future release of PostgreSQL."),
+ errhint("Refer to the PostgreSQL documentation for details about migrating to another password type.")));
+
+ used_md5_auth = false;
+}
+
/*
* Check given password for given user, and return STATUS_OK or STATUS_ERROR.
*
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index c86ceefda94..b77c5967468 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -34,6 +34,7 @@
#include "catalog/pg_db_role_setting.h"
#include "catalog/pg_tablespace.h"
#include "libpq/auth.h"
+#include "libpq/crypt.h"
#include "libpq/libpq-be.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -1234,6 +1235,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
/* close the transaction we started above */
if (!bootstrap)
CommitTransactionCommand();
+
+ warn_if_md5_auth();
}
/*
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index a1b4b363143..ca42210aaa3 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -53,6 +53,7 @@ extern char *get_role_password(const char *role, const char **logdetail);
extern int md5_crypt_verify(const char *role, const char *shadow_pass,
const char *client_pass, const uint8 *md5_salt,
int md5_salt_len, const char **logdetail);
+extern void warn_if_md5_auth(void);
extern int plain_crypt_verify(const char *role, const char *shadow_pass,
const char *client_pass,
const char **logdetail);
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 37d96d95a1a..3d94b323c09 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -68,6 +68,7 @@ $node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
# Needed to allow connect_fails to inspect postmaster log:
$node->append_conf('postgresql.conf', "log_min_messages = debug2");
+$node->append_conf('postgresql.conf', "md5_password_warnings = off");
$node->start;
# Test behavior of log_connections GUC
--
2.39.5 (Apple Git-154)
On 02.06.25 17:32, Nathan Bossart wrote:
Since MD5 passwords are slated to be marked as deprecated in v18, I figured
it might be a good idea to add a check for roles with MD5 passwords to
pg_upgrade. I'm tempted to suggest that we apply this to v18, but I'm
content to leave it for v19 if nobody feels too strongly about it.
I tend think pg_upgrade should stick to checking things that are
necessary for the upgrade to succeed. It shouldn't start being an
interactive portal to the release notes for aspects that are merely
recommendations. I'm not necessarily against having such a facility
somewhere. But not everyone uses pg_upgrade, and not every user of
pg_upgrade reads all the messages.
On Wed, Jun 4, 2025 at 10:15:49PM +0200, Peter Eisentraut wrote:
On 02.06.25 17:32, Nathan Bossart wrote:
Since MD5 passwords are slated to be marked as deprecated in v18, I figured
it might be a good idea to add a check for roles with MD5 passwords to
pg_upgrade. I'm tempted to suggest that we apply this to v18, but I'm
content to leave it for v19 if nobody feels too strongly about it.I tend think pg_upgrade should stick to checking things that are necessary
for the upgrade to succeed. It shouldn't start being an interactive portal
to the release notes for aspects that are merely recommendations. I'm not
necessarily against having such a facility somewhere. But not everyone uses
pg_upgrade, and not every user of pg_upgrade reads all the messages.
Yes, combine that with the fact that most people don't see pg_upgrade
output, and the case is even less positive.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Do not let urgent matters crowd out time for investment in the future.
On Wed, Jun 04, 2025 at 04:46:52PM -0400, Bruce Momjian wrote:
On Wed, Jun 4, 2025 at 10:15:49PM +0200, Peter Eisentraut wrote:
I tend think pg_upgrade should stick to checking things that are necessary
for the upgrade to succeed. It shouldn't start being an interactive portal
to the release notes for aspects that are merely recommendations. I'm not
necessarily against having such a facility somewhere. But not everyone uses
pg_upgrade, and not every user of pg_upgrade reads all the messages.Yes, combine that with the fact that most people don't see pg_upgrade
output, and the case is even less positive.
Okay, I'm getting the feeling that we should leave things as-is for v18 and
revisit 0002 (warning every time someone logs in with an MD5 password) down
the road. When we do remove MD5 password support, pg_upgrade will need
this check, but that's probably a few releases away still.
--
nathan
On 4 Jun 2025, at 22:15, Peter Eisentraut <peter@eisentraut.org> wrote:
I tend think pg_upgrade should stick to checking things that are necessary for the upgrade to succeed. It shouldn't start being an interactive portal to the release notes for aspects that are merely recommendations.
Agreed, I was going to support this warning but this comment convinced me
otherwise. pg_upgrade is messy as it is without tasking it with more usecases.
--
Daniel Gustafsson