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

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

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(at)momjian(dot)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(at)eisentraut(dot)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