password_encryption default

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

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.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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 8:29 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

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.

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?

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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