Thread: Pg14, pg_dumpall and "password_encryption=true"
Greetings
Consider the following:
postgres=# SELECT current_setting('server_version');
current_setting
-----------------
12.5
(1 row)
postgres=# SELECT * FROM pg_db_role_setting WHERE setrole = 10;
setdatabase | setrole | setconfig
-------------+---------+----------------------------
0 | 10 | {password_encryption=true}
(1 row)
$ $PG_14/pg_dumpall --version
pg_dumpall (PostgreSQL) 14devel
$ $PG_14/pg_dumpall -d 'port=5414' --globals-only | grep "password_encryption"
ALTER ROLE postgres SET password_encryption TO 'true';
This command will fail in Pg14, e.g. when running pg_upgrade (which is where
I bumped into the issue):
$ pg_upgrade --whatever
...
Restoring global objects in the new cluster
*failure*
Consult the last few lines of "pg_upgrade_utility.log" for
the probable cause of the failure.
Failure, exiting
$ tail -3 pg_upgrade_utility.log
ALTER ROLE "postgres" SET "password_encryption" TO 'true';
psql:pg_upgrade_dump_globals.sql:75: ERROR: invalid value for parameter "password_encryption": "true"
HINT: Available values: md5, scram-sha-256.
This is a consquence of commit c7eab0e97, which removed support for the legacy
values "on"/"true"/"yes"/"1".
I see following options to resolve this issue.
1. Re-add support for a "true" as an alias for "md5".
------------------------------------------------------------
Easiest option.
The disadvantage is that in Pg10 ~ Pg13, "password_encryption=true"
implictly meant "use the default encryption method", which was "md5",
however the default in Pg14 is "scram-sha-256", so the meaning
of "true" changes to "set to the non-default method".
2. Transparently rewrite "true" to "md5"
----------------------------------------
AFAICS this isn't supported (we can remap parameter names, e.g.
sort_mem -> work_mem, but not enum values), so would require some refactoring.
Adding a 4th field to "config_enum_entry" which, if not NULL, would cause
the setting to be stored as the specified value, e.g.:
{"true", PASSWORD_TYPE_MD5, true, "md5"},
seems like a nice way of defining it, but poking about, it looks non-trivial
to actually implement, and a lot of code for a single workaround.
I suppose, if feasible, it might be useful future, but can't think
of anywhere else it might be useful.
3. Have pg_dumpall map "true" to "md5"
--------------------------------------
This would require adding an exception for "password_encryption" (currently all
settings are dumped as-is).
However this would result in dumps from pre-Pg10 versions which would not be
valid for restoring into those versions (assuming we support later versions of
pg_dump/pg_dumpall being able to produce dumps from older versions which are
valid for those older versions).
4. Add an option to pg_dumpall to specify the target version
------------------------------------------------------------
As (4), but only remap if the target version is 10 or later.
And have pg_upgrade pass the target version option to pg_dumpall.
5. Have pg_upgrade emit a warning/hint
--------------------------------------
This would catch what a common occurrence of this issue.
However it's annoying for the user to have to deal with this during an
upgrade. And it still leaves Pg14's pg_dumpall at risk of creating dumps which
are not actually valid for Pg14, which will cause issues when doing upgrades via
logical replication etc.
6. Document this as a backwards-compatibility thing
----------------------------------------------------
Add an item in the documentation (release notes, pg_upgrade, pg_dumpall) stating
that any occurrences of "password_encryption" which are not valid in Pg14 should
be updated before performing an upgrade.
This has the disadvantage that it's just documentation and as such at risk of
being overlooked, time and time again.
And it still also leaves Pg14's pg_dumpall at risk of creating dumps which are
not actually valid for Pg14.
I'll add to the next CF so this doesn't get lost.
Regards
Ian Barwick
--
Consider the following:
postgres=# SELECT current_setting('server_version');
current_setting
-----------------
12.5
(1 row)
postgres=# SELECT * FROM pg_db_role_setting WHERE setrole = 10;
setdatabase | setrole | setconfig
-------------+---------+----------------------------
0 | 10 | {password_encryption=true}
(1 row)
$ $PG_14/pg_dumpall --version
pg_dumpall (PostgreSQL) 14devel
$ $PG_14/pg_dumpall -d 'port=5414' --globals-only | grep "password_encryption"
ALTER ROLE postgres SET password_encryption TO 'true';
This command will fail in Pg14, e.g. when running pg_upgrade (which is where
I bumped into the issue):
$ pg_upgrade --whatever
...
Restoring global objects in the new cluster
*failure*
Consult the last few lines of "pg_upgrade_utility.log" for
the probable cause of the failure.
Failure, exiting
$ tail -3 pg_upgrade_utility.log
ALTER ROLE "postgres" SET "password_encryption" TO 'true';
psql:pg_upgrade_dump_globals.sql:75: ERROR: invalid value for parameter "password_encryption": "true"
HINT: Available values: md5, scram-sha-256.
This is a consquence of commit c7eab0e97, which removed support for the legacy
values "on"/"true"/"yes"/"1".
I see following options to resolve this issue.
1. Re-add support for a "true" as an alias for "md5".
------------------------------------------------------------
Easiest option.
The disadvantage is that in Pg10 ~ Pg13, "password_encryption=true"
implictly meant "use the default encryption method", which was "md5",
however the default in Pg14 is "scram-sha-256", so the meaning
of "true" changes to "set to the non-default method".
2. Transparently rewrite "true" to "md5"
----------------------------------------
AFAICS this isn't supported (we can remap parameter names, e.g.
sort_mem -> work_mem, but not enum values), so would require some refactoring.
Adding a 4th field to "config_enum_entry" which, if not NULL, would cause
the setting to be stored as the specified value, e.g.:
{"true", PASSWORD_TYPE_MD5, true, "md5"},
seems like a nice way of defining it, but poking about, it looks non-trivial
to actually implement, and a lot of code for a single workaround.
I suppose, if feasible, it might be useful future, but can't think
of anywhere else it might be useful.
3. Have pg_dumpall map "true" to "md5"
--------------------------------------
This would require adding an exception for "password_encryption" (currently all
settings are dumped as-is).
However this would result in dumps from pre-Pg10 versions which would not be
valid for restoring into those versions (assuming we support later versions of
pg_dump/pg_dumpall being able to produce dumps from older versions which are
valid for those older versions).
4. Add an option to pg_dumpall to specify the target version
------------------------------------------------------------
As (4), but only remap if the target version is 10 or later.
And have pg_upgrade pass the target version option to pg_dumpall.
5. Have pg_upgrade emit a warning/hint
--------------------------------------
This would catch what a common occurrence of this issue.
However it's annoying for the user to have to deal with this during an
upgrade. And it still leaves Pg14's pg_dumpall at risk of creating dumps which
are not actually valid for Pg14, which will cause issues when doing upgrades via
logical replication etc.
6. Document this as a backwards-compatibility thing
----------------------------------------------------
Add an item in the documentation (release notes, pg_upgrade, pg_dumpall) stating
that any occurrences of "password_encryption" which are not valid in Pg14 should
be updated before performing an upgrade.
This has the disadvantage that it's just documentation and as such at risk of
being overlooked, time and time again.
And it still also leaves Pg14's pg_dumpall at risk of creating dumps which are
not actually valid for Pg14.
I'll add to the next CF so this doesn't get lost.
Regards
Ian Barwick
--
EnterpriseDB: https://www.enterprisedb.com
On Fri, Jan 15, 2021 at 01:35:50PM +0900, Ian Lawrence Barwick wrote: > $ tail -3 pg_upgrade_utility.log > ALTER ROLE "postgres" SET "password_encryption" TO 'true'; > psql:pg_upgrade_dump_globals.sql:75: ERROR: invalid value for > parameter "password_encryption": "true" > HINT: Available values: md5, scram-sha-256. > > This is a consquence of commit c7eab0e97, which removed support for the > legacy > values "on"/"true"/"yes"/"1". Right. This can happen anytime we reduce the domain of a setting having GucContext PGC_SU_BACKEND, PGC_BACKEND, PGC_SUSET, or PGC_USERSET. > I see following options to resolve this issue. > > 1. Re-add support for a "true" as an alias for "md5". > 2. Transparently rewrite "true" to "md5" > 3. Have pg_dumpall map "true" to "md5" > 4. Add an option to pg_dumpall to specify the target version I expect rather few databases override this particular setting in pg_proc.proconfig or pg_db_role_setting.setconfig. The restore failure has a clear error message, which is enough. Each of the above would be overkill. > 5. Have pg_upgrade emit a warning/hint If done in a way not specific to this one setting, +1 for this. That is to say, do something like the following. Retrieve every pg_proc.proconfig and pg_db_role_setting.setconfig value from the old cluster. Invoke the new postmaster binary to test acceptance of each value. I'm not generally a fan of adding pg_upgrade code to predict whether the dump will fail to restore, because such code will never be as good as just trying the restore. That said, this checking of GUC acceptance could be self-contained and useful for the long term. > 6. Document this as a backwards-compatibility thing > ---------------------------------------------------- > > Add an item in the documentation (release notes, pg_upgrade, pg_dumpall) > stating > that any occurrences of "password_encryption" which are not valid in Pg14 > should > be updated before performing an upgrade. The release notes will document the password_encryption change, though they probably won't specifically mention the interaction with pg_dump. I would not document it elsewhere.
On Sat, Jan 16, 2021 at 8:27 AM Noah Misch <noah@leadboat.com> wrote: > > On Fri, Jan 15, 2021 at 01:35:50PM +0900, Ian Lawrence Barwick wrote: > > $ tail -3 pg_upgrade_utility.log > > ALTER ROLE "postgres" SET "password_encryption" TO 'true'; > > psql:pg_upgrade_dump_globals.sql:75: ERROR: invalid value for > > parameter "password_encryption": "true" > > HINT: Available values: md5, scram-sha-256. > > > > This is a consquence of commit c7eab0e97, which removed support for the > > legacy > > values "on"/"true"/"yes"/"1". > > Right. This can happen anytime we reduce the domain of a setting having > GucContext PGC_SU_BACKEND, PGC_BACKEND, PGC_SUSET, or PGC_USERSET. > > > I see following options to resolve this issue. > > > > 1. Re-add support for a "true" as an alias for "md5". > > 2. Transparently rewrite "true" to "md5" > > 3. Have pg_dumpall map "true" to "md5" > > 4. Add an option to pg_dumpall to specify the target version > > I expect rather few databases override this particular setting in > pg_proc.proconfig or pg_db_role_setting.setconfig. The restore failure has a > clear error message, which is enough. Each of the above would be overkill. Option 3 would be the closest to how other things work though, wuodln't it? Normally, it's the job of pg_dump (or pg_dumpall) to adapt the dump to the new version of PostgreSQL. And pg_dump[all] always generates a dump that should reload in the same version of postgres as the dump program -- not the source postgresql. Thus doing (4) would mean changing that, and would have to teach pg_dump[all] about *all* version differences in all directions -- which would be a huge undertaking. Doing that for just one individual parameter would be very strange. In a lot of ways, think (3) seems like the reasonable ting to do. That's basically what we do when things change in the table creation commands etc, so it seems like the logical place. > > 5. Have pg_upgrade emit a warning/hint > > If done in a way not specific to this one setting, +1 for this. That is to > say, do something like the following. Retrieve every pg_proc.proconfig and > pg_db_role_setting.setconfig value from the old cluster. Invoke the new > postmaster binary to test acceptance of each value. I'm not generally a fan > of adding pg_upgrade code to predict whether the dump will fail to restore, > because such code will never be as good as just trying the restore. That > said, this checking of GUC acceptance could be self-contained and useful for > the long term. Yeah, while I think (3) above would be the better choice in this particular case, I agree something like that would be useful in a longer term. It might not always be possible to declare a rule for mapping values after all.. > > 6. Document this as a backwards-compatibility thing > > ---------------------------------------------------- > > > > Add an item in the documentation (release notes, pg_upgrade, pg_dumpall) > > stating > > that any occurrences of "password_encryption" which are not valid in Pg14 > > should > > be updated before performing an upgrade. > > The release notes will document the password_encryption change, though they > probably won't specifically mention the interaction with pg_dump. I would not > document it elsewhere. +1. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On Sun, Jan 17, 2021 at 01:51:35PM +0100, Magnus Hagander wrote:
> On Sat, Jan 16, 2021 at 8:27 AM Noah Misch <noah@leadboat.com> wrote:
> > On Fri, Jan 15, 2021 at 01:35:50PM +0900, Ian Lawrence Barwick wrote:
> > > $ tail -3 pg_upgrade_utility.log
> > > ALTER ROLE "postgres" SET "password_encryption" TO 'true';
> > > psql:pg_upgrade_dump_globals.sql:75: ERROR: invalid value for
> > > parameter "password_encryption": "true"
> > > HINT: Available values: md5, scram-sha-256.
> > >
> > > This is a consquence of commit c7eab0e97, which removed support for the
> > > legacy
> > > values "on"/"true"/"yes"/"1".
> >
> > Right. This can happen anytime we reduce the domain of a setting having
> > GucContext PGC_SU_BACKEND, PGC_BACKEND, PGC_SUSET, or PGC_USERSET.
> >
> > > I see following options to resolve this issue.
> > >
> > > 1. Re-add support for a "true" as an alias for "md5".
> > > 2. Transparently rewrite "true" to "md5"
> > > 3. Have pg_dumpall map "true" to "md5"
> > > 4. Add an option to pg_dumpall to specify the target version
> >
> > I expect rather few databases override this particular setting in
> > pg_proc.proconfig or pg_db_role_setting.setconfig. The restore failure has a
> > clear error message, which is enough. Each of the above would be overkill.
>
> Option 3 would be the closest to how other things work though,
> wuodln't it? Normally, it's the job of pg_dump (or pg_dumpall) to
> adapt the dump to the new version of PostgreSQL.
I didn't do a precedent search, but I can't think of an instance of those
programs doing (3). We have things like guessConstraintInheritance() that
make up for lack of information in old versions, but dumpFunc() doesn't mutate
any proconfig setting values. Is there a particular pg_dump behavior you had
in mind?
> And pg_dump[all] always generates a dump that should reload in the
> same version of postgres as the dump program -- not the source
> postgresql. Thus doing (4) would mean changing that, and would have to
> teach pg_dump[all] about *all* version differences in all directions
> -- which would be a huge undertaking. Doing that for just one
> individual parameter would be very strange.
Agreed.
> In a lot of ways, think (3) seems like the reasonable ting to do.
> That's basically what we do when things change in the table creation
> commands etc, so it seems like the logical place.
This would be interpreting setconfig='{password_encryption=on}' as "opt out of
future password security increases". I expect that will tend not to match the
intent of the person entering the setting. That said, if v14 were already
behaving this way, I wouldn't dislike it enough to complain.
On Sun, Jan 17, 2021 at 02:20:10PM -0800, Noah Misch wrote:
> On Sun, Jan 17, 2021 at 01:51:35PM +0100, Magnus Hagander wrote:
>> Option 3 would be the closest to how other things work though,
>> wuodln't it? Normally, it's the job of pg_dump (or pg_dumpall) to
>> adapt the dump to the new version of PostgreSQL.
>
> I didn't do a precedent search, but I can't think of an instance of those
> programs doing (3). We have things like guessConstraintInheritance() that
> make up for lack of information in old versions, but dumpFunc() doesn't mutate
> any proconfig setting values. Is there a particular pg_dump behavior you had
> in mind?
I don't recall any code paths in pg_dump touching array parsing that
enforces a value to something else if something is not supported.
>> In a lot of ways, think (3) seems like the reasonable ting to do.
>> That's basically what we do when things change in the table creation
>> commands etc, so it seems like the logical place.
>
> This would be interpreting setconfig='{password_encryption=on}' as "opt out of
> future password security increases". I expect that will tend not to match the
> intent of the person entering the setting. That said, if v14 were already
> behaving this way, I wouldn't dislike it enough to complain.
Hm. Up to 13, "on" is a synonym of "md5", so it seems to me
that we should map "on" to "md5" rather than "scram-sha-256" on the
side of compatibility. But you have a point when it comes to good
security practices. It makes me wonder whether it would be better to
have pg_dumpall complain rather than pg_upgrade if this value is found
in the proconfig items.. pg_upgrade is not the only upgrade path
supported.
--
Michael