Обсуждение: password_encryption default

We didn't get anywhere with making the default authentication method in 
a source build anything other than trust.  But perhaps we should change 
the default for password_encryption to nudge people to adopt SCRAM? 
Right now, passwords are still hashed using MD5 by default, unless you 
specify scram-sha-256 using initdb -A or similar.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> We didn't get anywhere with making the default authentication method in 
> a source build anything other than trust.  But perhaps we should change 
> the default for password_encryption to nudge people to adopt SCRAM? 
> Right now, passwords are still hashed using MD5 by default, unless you 
> specify scram-sha-256 using initdb -A or similar.

I think what that was waiting on was for client libraries to become
SCRAM-ready.  Do we have an idea of the state of play on that side?

            regards, tom lane

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:
> On Fri, May 22, 2020 at 4:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > > We didn't get anywhere with making the default authentication method in
> > > a source build anything other than trust.  But perhaps we should change
> > > the default for password_encryption to nudge people to adopt SCRAM?
> > > Right now, passwords are still hashed using MD5 by default, unless you
> > > specify scram-sha-256 using initdb -A or similar.
> >
> > I think what that was waiting on was for client libraries to become
> > SCRAM-ready.  Do we have an idea of the state of play on that side?
> >
>
> If the summary table on the wiki at
> https://wiki.postgresql.org/wiki/List_of_drivers is to be trusted, every
> listed driver except Swift does.

Yes, Katz actually went through and worked with folks to make that
happen.  I'm +1 on moving the default for password_encryption to be
scram.  Even better would be changing the pg_hba.conf default, but I
think we still have concerns about that having problems with the
regression tests and the buildfarm.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Magnus Hagander (magnus@hagander.net) wrote:
>> On Fri, May 22, 2020 at 4:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>>> We didn't get anywhere with making the default authentication method in
>>>> a source build anything other than trust.

> I'm +1 on moving the default for password_encryption to be
> scram.  Even better would be changing the pg_hba.conf default, but I
> think we still have concerns about that having problems with the
> regression tests and the buildfarm.

As far as that last goes, we *did* get the buildfarm fixed to be all
v11 scripts, so I thought we were ready to move forward on trying
09f08930f again.  It's too late to consider that for v13, but
perhaps it'd be reasonable to change the SCRAM default now?  Not sure.
Post-beta1 isn't the best time for such things.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Magnus Hagander (magnus@hagander.net) wrote:
> >> On Fri, May 22, 2020 at 4:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> >>>> We didn't get anywhere with making the default authentication method in
> >>>> a source build anything other than trust.
>
> > I'm +1 on moving the default for password_encryption to be
> > scram.  Even better would be changing the pg_hba.conf default, but I
> > think we still have concerns about that having problems with the
> > regression tests and the buildfarm.
>
> As far as that last goes, we *did* get the buildfarm fixed to be all
> v11 scripts, so I thought we were ready to move forward on trying
> 09f08930f again.  It's too late to consider that for v13, but
> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.

I feel like it is.  I'm not even sure that I agree that it's really too
late to consider 09f08930f considering that's it's a pretty minor code
change and the up-side to that is having reasonable defaults out of the
box, as it were, something we have *long* been derided for.

> Post-beta1 isn't the best time for such things.

It'd be good to be consistent about this between the packagers and the
source builds, imv, and we don't tend to think about that until we have
packages being built and distributed and used and that ends up being
post-beta1.  If we want that changed then we should go back to having
alphas..

In general though, I'm reasonably comfortable with changing of default
values post beta1.  I do appreciate that not everyone would agree with
that, but with all the effort that's put into getting everything working
with SCRAM, it'd be a real shame to keep md5 as the default for yet
another year and a half..

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> As far as that last goes, we *did* get the buildfarm fixed to be all
>> v11 scripts, so I thought we were ready to move forward on trying
>> 09f08930f again.  It's too late to consider that for v13, but
>> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.

> I feel like it is.  I'm not even sure that I agree that it's really too
> late to consider 09f08930f considering that's it's a pretty minor code
> change and the up-side to that is having reasonable defaults out of the
> box, as it were, something we have *long* been derided for.

Well, the argument against changing right now is that it would invalidate
portability testing done against beta1, which users would be justifiably
upset about.

I'm +1 for changing both of these things as soon as we branch for v14,
but I feel like it's a bit late for v13.  If we aren't feature-frozen
now, when will we be?

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> As far as that last goes, we *did* get the buildfarm fixed to be all
> >> v11 scripts, so I thought we were ready to move forward on trying
> >> 09f08930f again.  It's too late to consider that for v13, but
> >> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.
>
> > I feel like it is.  I'm not even sure that I agree that it's really too
> > late to consider 09f08930f considering that's it's a pretty minor code
> > change and the up-side to that is having reasonable defaults out of the
> > box, as it were, something we have *long* been derided for.
>
> Well, the argument against changing right now is that it would invalidate
> portability testing done against beta1, which users would be justifiably
> upset about.

I don't think we're in complete agreement about the amount of
portability testing that's done with our beta source builds.  To that
point, however, the lack of such testing happening, if there is a lack,
is on us just as much as anyone else- we should be testing, to the
extent possible, as many variations of our configuration options as we
can across as many platforms as we can in the buildfarm.  If a
non-default setting doesn't work on one platform or another, that's a
bug to fix regardless and doesn't really impact the question of "what
should be the default".

> I'm +1 for changing both of these things as soon as we branch for v14,
> but I feel like it's a bit late for v13.  If we aren't feature-frozen
> now, when will we be?

I really don't consider changing of defaults to be on the same level as
implementation of whole features, even if changing those defaults
requires a few lines of code to go with the change.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I'm +1 for changing both of these things as soon as we branch for v14,
>> but I feel like it's a bit late for v13.  If we aren't feature-frozen
>> now, when will we be?

> I really don't consider changing of defaults to be on the same level as
> implementation of whole features, even if changing those defaults
> requires a few lines of code to go with the change.

The buildfarm fiasco with 09f08930f should remind us that changing
defaults *does* break things, even if theoretically it shouldn't.
At this phase of the v13 cycle, we should be looking to fix bugs,
not to break more stuff.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> I'm +1 for changing both of these things as soon as we branch for v14,
> >> but I feel like it's a bit late for v13.  If we aren't feature-frozen
> >> now, when will we be?
>
> > I really don't consider changing of defaults to be on the same level as
> > implementation of whole features, even if changing those defaults
> > requires a few lines of code to go with the change.
>
> The buildfarm fiasco with 09f08930f should remind us that changing
> defaults *does* break things, even if theoretically it shouldn't.
> At this phase of the v13 cycle, we should be looking to fix bugs,
> not to break more stuff.

Sure it does- for the special case of the buildfarm, and that takes
buildfarm code to fix.  Having users make changes to whatever scripts
they're using with PG between major versions is certainly not
unreasonable, or even between beta and final.  These things are not set
in stone at this point, they're the defaults, and it's beta time now,
not post release or RC.

If it breaks for regular users who are using the system properly then we
want to know about that and we'd ideally like to get that fixed before
the release.

Thanks,

Stephen

On 5/22/20 11:34 AM, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> As far as that last goes, we *did* get the buildfarm fixed to be all
>>> v11 scripts, so I thought we were ready to move forward on trying
>>> 09f08930f again.  It's too late to consider that for v13, but
>>> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.
>
>> I feel like it is.  I'm not even sure that I agree that it's really too
>> late to consider 09f08930f considering that's it's a pretty minor code
>> change and the up-side to that is having reasonable defaults out of the
>> box, as it were, something we have *long* been derided for.
>
> Well, the argument against changing right now is that it would invalidate
> portability testing done against beta1, which users would be justifiably
> upset about.
>
> I'm +1 for changing both of these things as soon as we branch for v14,
> but I feel like it's a bit late for v13.  If we aren't feature-frozen
> now, when will we be?

As someone who is an unabashed SCRAM fan and was hoping the default
would be up'd for v13, I would actually +1 making it the default in v14,
i.e. because 9.5 will be EOL at that point, and as such we both have
every* driver supporting SCRAM AND every version of PostgreSQL
supporting SCRAM.

(Would I personally love to do it sooner? Yes...but I think the stars
align for doing it in v14).

Jonathan

On 5/22/20 9:09 PM, Jonathan S. Katz wrote:
> As someone who is an unabashed SCRAM fan and was hoping the default
> would be up'd for v13, I would actually +1 making it the default in v14,
> i.e. because 9.5 will be EOL at that point, and as such we both have
> every* driver supporting SCRAM AND every version of PostgreSQL
> supporting SCRAM.

Wasn't SCRAM introduced in 10?
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> On 5/22/20 9:09 PM, Jonathan S. Katz wrote:
>> As someone who is an unabashed SCRAM fan and was hoping the default
>> would be up'd for v13, I would actually +1 making it the default in v14,
>> i.e. because 9.5 will be EOL at that point, and as such we both have
>> every* driver supporting SCRAM AND every version of PostgreSQL
>> supporting SCRAM.

> Wasn't SCRAM introduced in 10?

Yeah.  But there's still something to Jonathan's argument, because 9.6
will go EOL in November 2021, which is pretty close to when v14 will
reach public release (assuming we can hold to the typical schedule).
If we do it in v13, there'll be a full year where still-supported
versions of PG can't do SCRAM, implying that clients would likely
fail to connect to an up-to-date server.

            regards, tom lane

On 5/22/20 5:21 PM, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> On 5/22/20 9:09 PM, Jonathan S. Katz wrote:
>>> As someone who is an unabashed SCRAM fan and was hoping the default
>>> would be up'd for v13, I would actually +1 making it the default in v14,
>>> i.e. because 9.5 will be EOL at that point, and as such we both have
>>> every* driver supporting SCRAM AND every version of PostgreSQL
>>> supporting SCRAM.
>
>> Wasn't SCRAM introduced in 10?
>
> Yeah.  But there's still something to Jonathan's argument, because 9.6
> will go EOL in November 2021, which is pretty close to when v14 will
> reach public release (assuming we can hold to the typical schedule).
> If we do it in v13, there'll be a full year where still-supported
> versions of PG can't do SCRAM, implying that clients would likely
> fail to connect to an up-to-date server.

^ that's what I meant.

Jonathan

On 2020-05-22 23:23, Jonathan S. Katz wrote:
>> Yeah.  But there's still something to Jonathan's argument, because 9.6
>> will go EOL in November 2021, which is pretty close to when v14 will
>> reach public release (assuming we can hold to the typical schedule).
>> If we do it in v13, there'll be a full year where still-supported
>> versions of PG can't do SCRAM, implying that clients would likely
>> fail to connect to an up-to-date server.
> 
> ^ that's what I meant.

Here is a proposed patch for PG14 then.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/25/20 5:45 AM, Peter Eisentraut wrote:
> On 2020-05-22 23:23, Jonathan S. Katz wrote:
>>> Yeah.  But there's still something to Jonathan's argument, because 9.6
>>> will go EOL in November 2021, which is pretty close to when v14 will
>>> reach public release (assuming we can hold to the typical schedule).
>>> If we do it in v13, there'll be a full year where still-supported
>>> versions of PG can't do SCRAM, implying that clients would likely
>>> fail to connect to an up-to-date server.
>>
>> ^ that's what I meant.
>
> Here is a proposed patch for PG14 then.

This makes me happy :D

I took a look over, it looks good. One question on the initdb.c diff:

-    if (strcmp(authmethodlocal, "scram-sha-256") == 0 ||
-        strcmp(authmethodhost, "scram-sha-256") == 0)
-    {
-        conflines = replace_token(conflines,
-                                  "#password_encryption = md5",
-                                  "password_encryption = scram-sha-256");
-    }
-

Would we reverse this, i.e. if someone chooses authmethodlocal to be
"md5", we would then set "password_encryption = md5"?

Thanks,

Jonathan

On 2020-05-25 17:57, Jonathan S. Katz wrote:
> I took a look over, it looks good. One question on the initdb.c diff:
> 
> -    if (strcmp(authmethodlocal, "scram-sha-256") == 0 ||
> -        strcmp(authmethodhost, "scram-sha-256") == 0)
> -    {
> -        conflines = replace_token(conflines,
> -                                  "#password_encryption = md5",
> -                                  "password_encryption = scram-sha-256");
> -    }
> -
> 
> Would we reverse this, i.e. if someone chooses authmethodlocal to be
> "md5", we would then set "password_encryption = md5"?

Yeah, I was too enthusiastic about removing that.  Here is a better patch.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, May 26, 2020 at 10:25:25AM +0200, Peter Eisentraut wrote:
> Yeah, I was too enthusiastic about removing that.  Here is a better patch.

+        as an MD5 hash.  (<literal>on</literal> is also accepted, as an alias
+        for <literal>md5</literal>.)  The default is
+        <literal>scram-sha-256</literal>.
Shouldn't password_encryption = on/true/1/yes be an equivalent of
scram-sha-256 as the default gets changed?
--
Michael

On 2020-05-27 08:00, Michael Paquier wrote:
> On Tue, May 26, 2020 at 10:25:25AM +0200, Peter Eisentraut wrote:
>> Yeah, I was too enthusiastic about removing that.  Here is a better patch.
> 
> +        as an MD5 hash.  (<literal>on</literal> is also accepted, as an alias
> +        for <literal>md5</literal>.)  The default is
> +        <literal>scram-sha-256</literal>.
> Shouldn't password_encryption = on/true/1/yes be an equivalent of
> scram-sha-256 as the default gets changed?

I think these are mostly legacy options anyway, so if we wanted to make 
a change, we should remove them.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, May 27, 2020 at 02:56:34PM +0200, Magnus Hagander wrote:
> Seems like the better choice yeah. Since we're changing the default anyway,
> maybe now is the time to do that? Or if not, maybe have it log an explicit
> deprecation warning when it loads a config with it?

Not sure that's worth it here, so I would just remove the whole.  It
would be confusing to keep the past values and have them map to
something we think is not an appropriate default.
--
Michael

On 5/26/20 4:25 AM, Peter Eisentraut wrote:
> On 2020-05-25 17:57, Jonathan S. Katz wrote:
>> I took a look over, it looks good. One question on the initdb.c diff:
>>
>> -    if (strcmp(authmethodlocal, "scram-sha-256") == 0 ||
>> -        strcmp(authmethodhost, "scram-sha-256") == 0)
>> -    {
>> -        conflines = replace_token(conflines,
>> -                                  "#password_encryption = md5",
>> -                                  "password_encryption =
>> scram-sha-256");
>> -    }
>> -
>>
>> Would we reverse this, i.e. if someone chooses authmethodlocal to be
>> "md5", we would then set "password_encryption = md5"?
>
> Yeah, I was too enthusiastic about removing that.  Here is a better patch.

Did some testing. Overall it looks good. Here are my test cases and what
happened:

$ initdb -D data

Deferred password_encryption to the default, confirmed it was indeed scram

$ initdb -D data --auth-local=md5


Set password_encryption to md5

$ initdb -D data --auth-host=md5

Set password_encryption to md5

$ initdb -D data --auth-local=md5 --auth-host=scram-sha-256

Got an error message:

initdb: error: must specify a password for the superuser to enable
scram-sha-256 authentication

$ initdb -D data --auth-local=scram-sha-256 --auth-host=md5

Got an error message:

"initdb: error: must specify a password for the superuser to enable md5
authentication"

For the last two, that behavior is to be expected (after all, you've set
the two login vectors to require passwords), but the error message seems
odd now. Perhaps we tweak it to be:


"initdb: error: must specify a password for the superuser when requiring
passwords for both local and host authentication."

Another option could be to set the message based on whether both
local/host have the same setting, and then default to something like the
above when they differ.

Other than that, looks great!

Jonathan

On 5/27/20 9:13 AM, Michael Paquier wrote:
> On Wed, May 27, 2020 at 02:56:34PM +0200, Magnus Hagander wrote:
>> Seems like the better choice yeah. Since we're changing the default anyway,
>> maybe now is the time to do that? Or if not, maybe have it log an explicit
>> deprecation warning when it loads a config with it?
>
> Not sure that's worth it here, so I would just remove the whole.  It
> would be confusing to keep the past values and have them map to
> something we think is not an appropriate default.

+1 to removing the legacy options. It could break some people on legacy
upgrades, but my guess would be that said situations are very small, and
we would document the removal of these as "breaking changes" in the
release notes.

Jonathan

Greetings,

* Jonathan S. Katz (jkatz@postgresql.org) wrote:
> On 5/27/20 9:13 AM, Michael Paquier wrote:
> > On Wed, May 27, 2020 at 02:56:34PM +0200, Magnus Hagander wrote:
> >> Seems like the better choice yeah. Since we're changing the default anyway,
> >> maybe now is the time to do that? Or if not, maybe have it log an explicit
> >> deprecation warning when it loads a config with it?
> >
> > Not sure that's worth it here, so I would just remove the whole.  It
> > would be confusing to keep the past values and have them map to
> > something we think is not an appropriate default.
>
> +1 to removing the legacy options. It could break some people on legacy
> upgrades, but my guess would be that said situations are very small, and
> we would document the removal of these as "breaking changes" in the
> release notes.

Agreed- let's remove the legacy options.  As I've mentioned elsewhere,
distros may manage the issue for us, and if we want to get into it, we
could consider adding support to pg_upgrade to complain if it comes
across a legacy setting that isn't valid.  I'm not sure that's
worthwhile though.

Thanks,

Stephen

On 2020-05-27 15:25, Jonathan S. Katz wrote:
> $ initdb -D data --auth-local=scram-sha-256 --auth-host=md5
> 
> Got an error message:
> 
> "initdb: error: must specify a password for the superuser to enable md5
> authentication"
> 
> For the last two, that behavior is to be expected (after all, you've set
> the two login vectors to require passwords), but the error message seems
> odd now. Perhaps we tweak it to be:
> 
> 
> "initdb: error: must specify a password for the superuser when requiring
> passwords for both local and host authentication."

That message is a bit long.  Maybe just

must specify a password for the superuser to enable password authentication

without reference to the specific method.  I think the idea is clear there.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-05-27 15:59, Stephen Frost wrote:
> Agreed- let's remove the legacy options.  As I've mentioned elsewhere,
> distros may manage the issue for us, and if we want to get into it, we
> could consider adding support to pg_upgrade to complain if it comes
> across a legacy setting that isn't valid.  I'm not sure that's
> worthwhile though.

More along these lines: We could also remove the ENCRYPTED and 
UNENCRYPTED keywords from CREATE and ALTER ROLE.  AFAICT, these have 
never been emitted by pg_dump or psql, so there are no concerns from 
that end.  Thoughts?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/28/20 8:10 AM, Peter Eisentraut wrote:
> On 2020-05-27 15:25, Jonathan S. Katz wrote:
>> $ initdb -D data --auth-local=scram-sha-256 --auth-host=md5
>>
>> Got an error message:
>>
>> "initdb: error: must specify a password for the superuser to enable md5
>> authentication"
>>
>> For the last two, that behavior is to be expected (after all, you've set
>> the two login vectors to require passwords), but the error message seems
>> odd now. Perhaps we tweak it to be:
>>
>>
>> "initdb: error: must specify a password for the superuser when requiring
>> passwords for both local and host authentication."
>
> That message is a bit long.  Maybe just
>
> must specify a password for the superuser to enable password authentication
>
> without reference to the specific method.  I think the idea is clear there.

+1

Jonathan

On Thu, May 28, 2020 at 8:53 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> More along these lines: We could also remove the ENCRYPTED and
> UNENCRYPTED keywords from CREATE and ALTER ROLE.  AFAICT, these have
> never been emitted by pg_dump or psql, so there are no concerns from
> that end.  Thoughts?

I have a question about this. My understanding of this area isn't
great. As I understand it, you can specify a password unencrypted and
let the system compute the validator from it, or you can compute the
validator yourself and then send that as the 'encrypted' password.
But, apparently, CREATE ROLE and ALTER ROLE don't really know which
thing you did. They just examine the string that you passed and decide
whether it looks like a validator. If so, they assume it is; if not,
they assume it's just a password.

But that seems really odd. What if you choose a password that just
happens to look like a validator? Perhaps that's not real likely, but
why do we not permit -- or even require -- the user to specify intent?
It seems out of character for us to, essentially, guess the meaning of
something ambiguous rather than requiring the user to be clear about
it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, May 28, 2020 at 8:53 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > More along these lines: We could also remove the ENCRYPTED and
> > UNENCRYPTED keywords from CREATE and ALTER ROLE.  AFAICT, these have
> > never been emitted by pg_dump or psql, so there are no concerns from
> > that end.  Thoughts?
>
> I have a question about this. My understanding of this area isn't
> great. As I understand it, you can specify a password unencrypted and
> let the system compute the validator from it, or you can compute the
> validator yourself and then send that as the 'encrypted' password.
> But, apparently, CREATE ROLE and ALTER ROLE don't really know which
> thing you did. They just examine the string that you passed and decide
> whether it looks like a validator. If so, they assume it is; if not,
> they assume it's just a password.
>
> But that seems really odd. What if you choose a password that just
> happens to look like a validator? Perhaps that's not real likely, but
> why do we not permit -- or even require -- the user to specify intent?
> It seems out of character for us to, essentially, guess the meaning of
> something ambiguous rather than requiring the user to be clear about
> it.

Indeed, and it's also been a source of bugs...  Watching pgcon atm but
I do recall some history around exactly this.

I'd certainly be in favor of having these things be more explicit-
including doing things like actually splitting out the actual password
validator from the algorithm instead of having them smashed together as
one string as if we don't know what columns are (also recall complaining
about that when scram was first being developed too, though that might
just be in my head).

Thanks,

Stephen

On Thu, May 28, 2020 at 10:01 AM Stephen Frost <sfrost@snowman.net> wrote:
> as if we don't know what columns are

Amen to that!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, May 28, 2020 at 02:53:17PM +0200, Peter Eisentraut wrote:
> More along these lines: We could also remove the ENCRYPTED and UNENCRYPTED
> keywords from CREATE and ALTER ROLE.  AFAICT, these have never been emitted
> by pg_dump or psql, so there are no concerns from that end.  Thoughts?

+0.5.  I think that you have a good point about the removal of
UNENCRYPTED (one keyword gone!) as we don't support it since 10.  For
ENCRYPTED, I'd rather keep it around for compatibility reasons for a
longer time, just to be on the safe side.
--
Michael

On 5/29/20 3:33 AM, Michael Paquier wrote:
> On Thu, May 28, 2020 at 02:53:17PM +0200, Peter Eisentraut wrote:
>> More along these lines: We could also remove the ENCRYPTED and UNENCRYPTED
>> keywords from CREATE and ALTER ROLE.  AFAICT, these have never been emitted
>> by pg_dump or psql, so there are no concerns from that end.  Thoughts?
>
> +0.5.  I think that you have a good point about the removal of
> UNENCRYPTED (one keyword gone!) as we don't support it since 10.  For
> ENCRYPTED, I'd rather keep it around for compatibility reasons for a
> longer time, just to be on the safe side.

By that logic, I would +1 removing ENCRYPTED & UNENCRYPTED, given
ENCRYPTED effectively has no meaning either after all this time too. If
it's not emitted by any of our scripts, and it's been effectively moot
for 4 years (by the time of PG14), and we've been saying in the docs "he
ENCRYPTED keyword has no effect, but is accepted for backwards
compatibility" I think we'd be safe with removing it.

Perhaps a stepping stone is to emit a deprecation warning on PG14 and
remove in PG15, but I think it's safe to remove.

Perhaps stating the obvious here, but I also think it's a separate patch
from the $SUBJECT, but glad to see the clean up :)

Jonathan

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Thu, May 28, 2020 at 02:53:17PM +0200, Peter Eisentraut wrote:
> > More along these lines: We could also remove the ENCRYPTED and UNENCRYPTED
> > keywords from CREATE and ALTER ROLE.  AFAICT, these have never been emitted
> > by pg_dump or psql, so there are no concerns from that end.  Thoughts?
>
> +0.5.  I think that you have a good point about the removal of
> UNENCRYPTED (one keyword gone!) as we don't support it since 10.  For
> ENCRYPTED, I'd rather keep it around for compatibility reasons for a
> longer time, just to be on the safe side.

It's both inaccurate and would be completely legacy at that point.

I disagree entirely about keeping it around 'for compatibility'.

Thanks,

Stephen

Greetings,

* Jonathan S. Katz (jkatz@postgresql.org) wrote:
> On 5/29/20 3:33 AM, Michael Paquier wrote:
> > On Thu, May 28, 2020 at 02:53:17PM +0200, Peter Eisentraut wrote:
> >> More along these lines: We could also remove the ENCRYPTED and UNENCRYPTED
> >> keywords from CREATE and ALTER ROLE.  AFAICT, these have never been emitted
> >> by pg_dump or psql, so there are no concerns from that end.  Thoughts?
> >
> > +0.5.  I think that you have a good point about the removal of
> > UNENCRYPTED (one keyword gone!) as we don't support it since 10.  For
> > ENCRYPTED, I'd rather keep it around for compatibility reasons for a
> > longer time, just to be on the safe side.
>
> By that logic, I would +1 removing ENCRYPTED & UNENCRYPTED, given
> ENCRYPTED effectively has no meaning either after all this time too. If
> it's not emitted by any of our scripts, and it's been effectively moot
> for 4 years (by the time of PG14), and we've been saying in the docs "he
> ENCRYPTED keyword has no effect, but is accepted for backwards
> compatibility" I think we'd be safe with removing it.
>
> Perhaps a stepping stone is to emit a deprecation warning on PG14 and
> remove in PG15, but I think it's safe to remove.

We're terrible about that, and people reasonably complain about such
things because we don't *know* we're gonna remove it in 15.

I'll argue again for the approach I mentioned before somewhere: when we
commit the patch for 14, we go back and update the older docs to note
that it's gone as of v14.  Deprecation notices and other such don't work
and we instead end up carrying legacy things on forever.

Thanks,

Stephen

On 5/29/20 9:22 AM, Stephen Frost wrote:
> Greetings,
>
> * Jonathan S. Katz (jkatz@postgresql.org) wrote:
>> On 5/29/20 3:33 AM, Michael Paquier wrote:
>>> On Thu, May 28, 2020 at 02:53:17PM +0200, Peter Eisentraut wrote:
>>>> More along these lines: We could also remove the ENCRYPTED and UNENCRYPTED
>>>> keywords from CREATE and ALTER ROLE.  AFAICT, these have never been emitted
>>>> by pg_dump or psql, so there are no concerns from that end.  Thoughts?
>>>
>>> +0.5.  I think that you have a good point about the removal of
>>> UNENCRYPTED (one keyword gone!) as we don't support it since 10.  For
>>> ENCRYPTED, I'd rather keep it around for compatibility reasons for a
>>> longer time, just to be on the safe side.
>>
>> By that logic, I would +1 removing ENCRYPTED & UNENCRYPTED, given
>> ENCRYPTED effectively has no meaning either after all this time too. If
>> it's not emitted by any of our scripts, and it's been effectively moot
>> for 4 years (by the time of PG14), and we've been saying in the docs "he
>> ENCRYPTED keyword has no effect, but is accepted for backwards
>> compatibility" I think we'd be safe with removing it.
>>
>> Perhaps a stepping stone is to emit a deprecation warning on PG14 and
>> remove in PG15, but I think it's safe to remove.
>
> We're terrible about that, and people reasonably complain about such
> things because we don't *know* we're gonna remove it in 15.
>
> I'll argue again for the approach I mentioned before somewhere: when we
> commit the patch for 14, we go back and update the older docs to note
> that it's gone as of v14.  Deprecation notices and other such don't work
> and we instead end up carrying legacy things on forever.

Yeah, my first preference is to just remove it. I'm ambivalent towards
updating the older docs, but I do think it would be helpful.

Jonathan

Stephen Frost <sfrost@snowman.net> writes:
> * Jonathan S. Katz (jkatz@postgresql.org) wrote:
>> By that logic, I would +1 removing ENCRYPTED & UNENCRYPTED, given
>> ENCRYPTED effectively has no meaning either after all this time too.
>> Perhaps a stepping stone is to emit a deprecation warning on PG14 and
>> remove in PG15, but I think it's safe to remove.

> We're terrible about that, and people reasonably complain about such
> things because we don't *know* we're gonna remove it in 15.

If we're changing associated defaults, there's already some risk of
breaking badly-written applications.  +1 for just removing these
keywords in v14.

            regards, tom lane

On 2020-05-28 15:28, Jonathan S. Katz wrote:
> On 5/28/20 8:10 AM, Peter Eisentraut wrote:
>> On 2020-05-27 15:25, Jonathan S. Katz wrote:
>>> $ initdb -D data --auth-local=scram-sha-256 --auth-host=md5
>>>
>>> Got an error message:
>>>
>>> "initdb: error: must specify a password for the superuser to enable md5
>>> authentication"
>>>
>>> For the last two, that behavior is to be expected (after all, you've set
>>> the two login vectors to require passwords), but the error message seems
>>> odd now. Perhaps we tweak it to be:
>>>
>>>
>>> "initdb: error: must specify a password for the superuser when requiring
>>> passwords for both local and host authentication."
>>
>> That message is a bit long.  Maybe just
>>
>> must specify a password for the superuser to enable password authentication
>>
>> without reference to the specific method.  I think the idea is clear there.
> 
> +1

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 6/10/20 10:47 AM, Peter Eisentraut wrote:
> On 2020-05-28 15:28, Jonathan S. Katz wrote:
>> On 5/28/20 8:10 AM, Peter Eisentraut wrote:
>>> On 2020-05-27 15:25, Jonathan S. Katz wrote:
>>>> $ initdb -D data --auth-local=scram-sha-256 --auth-host=md5
>>>>
>>>> Got an error message:
>>>>
>>>> "initdb: error: must specify a password for the superuser to enable md5
>>>> authentication"
>>>>
>>>> For the last two, that behavior is to be expected (after all, you've
>>>> set
>>>> the two login vectors to require passwords), but the error message
>>>> seems
>>>> odd now. Perhaps we tweak it to be:
>>>>
>>>>
>>>> "initdb: error: must specify a password for the superuser when
>>>> requiring
>>>> passwords for both local and host authentication."
>>>
>>> That message is a bit long.  Maybe just
>>>
>>> must specify a password for the superuser to enable password
>>> authentication
>>>
>>> without reference to the specific method.  I think the idea is clear
>>> there.
>>
>> +1
>
> committed

Yay!!! Thank you!

Jonathan

On Wed, Jun 10, 2020 at 10:51:22AM -0400, Jonathan S. Katz wrote:
> On 6/10/20 10:47 AM, Peter Eisentraut wrote:
>> committed
>
> Yay!!! Thank you!

Thanks, all.
--
Michael