Thread: Re: pg_upgrade: warn about roles with md5 passwords

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