PATCH: Configurable file mode mask

On 1 March 2017 at 01:58, David Steele <david@pgmasters.net> wrote:

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

+1

This patch introduces a new initdb param, -u/-file-mode-mask, and a new
GUC, file_mode_mask,

Why both initdb and at server start? Seems like initdb is OK, or in pg_control.

to allow the default mode of files and directories
in the $PGDATA directory to be modified.

Are you saying if this is changed all files/directories will be
changed to the new mode?

It seems like it would be annoying to have some files in one mode,
some in another.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs <simon@2ndquadrant.com> writes:

On 1 March 2017 at 01:58, David Steele <david@pgmasters.net> wrote:

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

+1

I'd ask what is the point, considering that we don't view "cp -a" as a
supported backup technique in the first place.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Mar 6, 2017 at 7:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

On 1 March 2017 at 01:58, David Steele <david@pgmasters.net> wrote:

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

+1

I'd ask what is the point, considering that we don't view "cp -a" as a
supported backup technique in the first place.

/me is confused.

Surely the idea is that you'd like an unprivileged database user to
run pg_start_backup(), an operating system user that can read but not
write the database files to copy them, and then the unprivileged to
then run pg_stop_backup(). I have no opinion on the patch, but I
support the goal. As I said on the surprisingly-controversial thread
about ripping out hard-coded superuser checks, reducing the level of
privilege which someone must have in order to perform a necessary
operation leads to better security. An exclusive backup taken via the
filesystem (probably not via cp, but say via tar or cpio) inevitably
requires the backup user to be able to read the entire cluster
directory, but it doesn't inherently require the backup user to be
able to write the cluster directory.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Greetings,

* Simon Riggs (simon@2ndquadrant.com) wrote:

On 1 March 2017 at 01:58, David Steele <david@pgmasters.net> wrote:

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

+1

This patch introduces a new initdb param, -u/-file-mode-mask, and a new
GUC, file_mode_mask,

Why both initdb and at server start? Seems like initdb is OK, or in pg_control.

One could imagine someone wishing to change their mind regarding the
permissions after initdb, and for existing systems who may wish to move
to allowing group-read in an environment where that can be safely done
but don't wish to re-initdb.

to allow the default mode of files and directories
in the $PGDATA directory to be modified.

Are you saying if this is changed all files/directories will be
changed to the new mode?

No, new files will be created with the new mode and existing files will
be allowed to have the mode set. Changing all of the existing files
didn't seem like something we should be trying to do at server start.

It seems like it would be annoying to have some files in one mode,
some in another.

It's not intended for that to happen, but it is possible for it to. The
alternative is to try and forcibly change all files at server start time
to match what is configured but that didn't seem like a great idea.

Thanks!

Stephen

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

On 1 March 2017 at 01:58, David Steele <david@pgmasters.net> wrote:

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

+1

I'd ask what is the point, considering that we don't view "cp -a" as a
supported backup technique in the first place.

The point is to allow backups to be performed as a user who only has
read-only access to the files and is a non-superuser in the database.
With the changes to allow GRANT'ing of the pg_start/stop_backup and
related functions and these changes to allow the files to be group
readable, that will be possible using a tool such as pgbackrest, not
just with a "cp -a".

Thanks!

Stephen

On 3/6/17 8:50 AM, Stephen Frost wrote:

* Simon Riggs (simon@2ndquadrant.com) wrote:

to allow the default mode of files and directories
in the $PGDATA directory to be modified.

Are you saying if this is changed all files/directories will be
changed to the new mode?

No, new files will be created with the new mode and existing files will
be allowed to have the mode set. Changing all of the existing files
didn't seem like something we should be trying to do at server start.

It seems like it would be annoying to have some files in one mode,
some in another.

It's not intended for that to happen, but it is possible for it to. The
alternative is to try and forcibly change all files at server start time
to match what is configured but that didn't seem like a great idea.

Agreed. It would definitely affect server start time, perhaps
significantly.

I have added a note to the docs that a change in file_mode_mask does not
automatically change the mode of existing files on disk.

This patch applies cleanly on 6f3a13f.

--
-David
david@pgmasters.net

On 3/6/17 8:17 AM, Robert Haas wrote:

On Mon, Mar 6, 2017 at 7:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

On 1 March 2017 at 01:58, David Steele <david@pgmasters.net> wrote:

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

+1

I'd ask what is the point, considering that we don't view "cp -a" as a
supported backup technique in the first place.

/me is confused.

Surely the idea is that you'd like an unprivileged database user to
run pg_start_backup(), an operating system user that can read but not
write the database files to copy them, and then the unprivileged to
then run pg_stop_backup(). I have no opinion on the patch, but I
support the goal. As I said on the surprisingly-controversial thread
about ripping out hard-coded superuser checks, reducing the level of
privilege which someone must have in order to perform a necessary
operation leads to better security. An exclusive backup taken via the
filesystem (probably not via cp, but say via tar or cpio) inevitably
requires the backup user to be able to read the entire cluster
directory, but it doesn't inherently require the backup user to be
able to write the cluster directory.

Limiting privileges also serves to guard against any bugs in tools that
run directly against $PGDATA and do not require write privileges.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/28/17 20:58, David Steele wrote:

This patch introduces a new initdb param, -u/-file-mode-mask, and a new
GUC, file_mode_mask, to allow the default mode of files and directories
in the $PGDATA directory to be modified.

The postmaster.pid file appears not to observe the configured mask.

There ought to be a test, perhaps under src/bin/initdb/, to check for
that kind of thing.

There is no documentation update for initdb.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele
PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively) unprivileged
user to perform the backup at the file system level as well.

I'd like to help review this. First, let me give some questions and comments.

1.What's the concrete use case of this feature? Do you intend to extend the concept of multiple DBAs to the full range of administration of a single database instance, or just multiple OS users for database backup?
If you think that multiple OS user support is desirable to reduce the administration burdon on a single person, then isn't the automated backup sufficient (such as with cron)?

2.Backup should always be considered with recovery. If you allow another OS user to back up the database, can you allow him to recover the database as well?
For example, assume the PostgreSQL user account (the OS user who does initdb and pg_ctl start/stop) is dba1, and dba2 backs up the database using tar or cpio.
When dba2 restores the backup, the owner of the database cluster becomes dba2. If the file permission only allows one user to write the file, then dba1 can't start the instance.

3.The default location of the SSL key file is $PGDATA, so the permission of the key file is likely to become 0640. But the current postgres requires it to be 0600. See src/backend/libpq/be-secure-openssl.c.

4.I've seen a few users to place .pgpass file in $PGDATA and set the environment variable PGPASSFILE to point to it. They expect it to be back up with other database files. So I'm afraid the permission of .pgpass file also becomes 0640 some time. However, the current code requires it to be 0600. See src/interface/libpq/fe-connect.c.

5.I think some explanation about the concept of multiple OS users is necessary, such as here:

16.1. Short Version
https://www.postgresql.org/docs/devel/static/install-short.html

18.2. Creating a Database Cluster
https://www.postgresql.org/docs/devel/static/creating-cluster.html

[FYI]
Oracle instructs the user, during the software installation, to put "umask 022" in ~/.bashrc or so.
MySQL's files in the data directory appears to be 0640.

Regards
Takayuki Tsunakawa

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 2/28/17 20:58, David Steele wrote:

This patch introduces a new initdb param, -u/-file-mode-mask, and a new
GUC, file_mode_mask, to allow the default mode of files and directories
in the $PGDATA directory to be modified.

The postmaster.pid file appears not to observe the configured mask.

Good point, it should.

There ought to be a test, perhaps under src/bin/initdb/, to check for
that kind of thing.

Good idea.

There is no documentation update for initdb.

Right, that needs to be fixed.

Thanks!

Stephen

Greetings,

* Tsunakawa, Takayuki (tsunakawa.takay@jp.fujitsu.com) wrote:

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele
PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively) unprivileged
user to perform the backup at the file system level as well.

I'd like to help review this. First, let me give some questions and comments.

Great!

1.What's the concrete use case of this feature? Do you intend to extend the concept of multiple DBAs to the full range of administration of a single database instance, or just multiple OS users for database backup?

This is to allow a non-postgres user to perform a backup of the
database. Perhaps this could be leveraged for other administration
functions, but it's not clear how off-hand to me and the backup use-case
is the reason for adding this.

If you think that multiple OS user support is desirable to reduce the administration burdon on a single person, then isn't the automated backup sufficient (such as with cron)?

I'm not quite sure what the question here is, but it is desirable to
minimize the amount of access any process requires to only that which is
required to perform its duties. In the case of backup, only read access
to the data directory and access to connect to PG and run certain
functions is required. The ability to run those functions as a
non-superuser was added in 9.6, this continues the work to minimize what
a backup user needs to perform a backup of the system by allowing a user
to have only read-only access to the data directory.

There are multiple reasons why matching the privileges a process has to
only that which is required is good practice. Minimizing impact to
ongoing operations from a compromise of the backup user and reducing the
risk that bugs in backup software could disrupt operations are two of
those.

2.Backup should always be considered with recovery. If you allow another OS user to back up the database, can you allow him to recover the database as well?

That would not be our recommended approach, but it would be possible to
do. Our recommended solution is for the backup user to only be able to
perform the backup. In this use-case, the restore would be run by the
OS user who owns the database (eg: postgres), who would have read-only
access to the backup repository.

To be clear, this is not hypothetical, pgBackrest supports these
configurations and has been tested with this approach. As noted, there
are a few additional items that need to be addressed which weren't
covered in the initial testing (on Debian-based systems, the pid file
and SSL key aren't in the data directory, so they didn't pose a problem,
but they should be addressed so that this can work on other
distributions).

For example, assume the PostgreSQL user account (the OS user who does initdb and pg_ctl start/stop) is dba1, and dba2 backs up the database using tar or cpio.
When dba2 restores the backup, the owner of the database cluster becomes dba2. If the file permission only allows one user to write the file, then dba1 can't start the instance.

If the uesr chose to configure the system with both dba1 and dba2 having
write access to the data directory, performing a restore as dba2 would
be possible and the backup utility could be sure to remove all existing
files and restore them from the backup, ensuring that all of the files
would be owned by a single user (dba2 in this case). Of course, one
would then need to adjust the startup process to run as dba2 instead.

With group write access to all of the files and directories, it may be
possible to actually run with the dba1 user even though the files are
owned by dba2, but we would not recommend it as the result would be a
mix of files owned by one dba or the other. This is not the use-case
this is being developed for.

3.The default location of the SSL key file is $PGDATA, so the permission of the key file is likely to become 0640. But the current postgres requires it to be 0600. See src/backend/libpq/be-secure-openssl.c.

Yes, that needs to be addressed. There was discussion on another thread
that it would be useful to support the SSL key file having group read
access, but since this patch is handling the other files it seems like
it would make sense to do that change here also.

4.I've seen a few users to place .pgpass file in $PGDATA and set the environment variable PGPASSFILE to point to it. They expect it to be back up with other database files. So I'm afraid the permission of .pgpass file also becomes 0640 some time. However, the current code requires it to be 0600. See src/interface/libpq/fe-connect.c.

This is not a configuration which we would recommend (generally
speaking, it's not really a good idea to drop random files into
$PGDATA). That said, there was discussion on another thread about
supporting this with an explicit client-side option to allow it. That
would be independent from this patch, I believe.

5.I think some explanation about the concept of multiple OS users is necessary, such as here:

16.1. Short Version
https://www.postgresql.org/docs/devel/static/install-short.html

18.2. Creating a Database Cluster
https://www.postgresql.org/docs/devel/static/creating-cluster.html

I agree that we should update the documention for this, including those.

[FYI]
Oracle instructs the user, during the software installation, to put "umask 022" in ~/.bashrc or so.
MySQL's files in the data directory appears to be 0640.

When it comes to MySQL, at least, this may be distribution dependent.
In any case, it's good to see that we are not the only ones doing this.

Thanks!

Stephen

On 3/10/17 8:12 AM, Stephen Frost wrote:

Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 2/28/17 20:58, David Steele wrote:

This patch introduces a new initdb param, -u/-file-mode-mask, and a new
GUC, file_mode_mask, to allow the default mode of files and directories
in the $PGDATA directory to be modified.

The postmaster.pid file appears not to observe the configured mask.

Good point, it should.

Leaving the mask on this file as-is was intentional. At miscinit.c:829:

/* Think not to make the file protection weaker than 0600. See comments
below. */

At miscinit.c:893:

/* We can treat the EPERM-error case as okay because that error implies
that the existing process has a different userid than we do, which means
it cannot be a competing postmaster. A postmaster cannot successfully
attach to a data directory owned by a userid other than its own. (This
is now checked directly in checkDataDir(), but has been true for a long
time because of the restriction that the data directory isn't group- or
world-accessible.) Also, since we create the lockfiles mode 600, we'd
have failed above if the lockfile belonged to another userid --- which
means that whatever process kill() is reporting about isn't the one that
made the lockfile. (NOTE: this last consideration is the only one that
keeps us from blowing away a Unix socket file belonging to an instance
of Postgres being run by someone else, at least on machines where /tmp
hasn't got a stickybit.) */

I can't see why this explanation does not continue to hold even if
permissions for other files are changed. For the use cases I envision,
I don't think being able to read/manipulate postmaster.pid is important,
only to detect that it is present.

There ought to be a test, perhaps under src/bin/initdb/, to check for
that kind of thing.

Good idea.

Agreed, will add to next patch.

There is no documentation update for initdb.

The --file-mode-mask option was added to the option list, but you are
probably referring to a paragraph under description. Will add to the
next patch.

--
-David
david@pgmasters.net

On 3/10/17 8:34 AM, Stephen Frost wrote:

Greetings,

* Tsunakawa, Takayuki (tsunakawa.takay@jp.fujitsu.com) wrote:

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele
PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever). Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively) unprivileged
user to perform the backup at the file system level as well.

I'd like to help review this. First, let me give some questions and comments.

Much appreciated!

3.The default location of the SSL key file is $PGDATA, so the permission of the key file is likely to become 0640. But the current postgres requires it to be 0600. See src/backend/libpq/be-secure-openssl.c.

Yes, that needs to be addressed. There was discussion on another thread
that it would be useful to support the SSL key file having group read
access, but since this patch is handling the other files it seems like
it would make sense to do that change here also.

Perhaps, but since these files are not setup by initdb I'm not sure if
we should be handling their permissions. This seems to be a
distro-specific issue.

It seems to me that it would be best to advise in the docs that these
files should be relocated if they won't be readable by the backup user.
In any event, I'm not convinced that backing up server private keys is a
good idea.

5.I think some explanation about the concept of multiple OS users is necessary, such as here:

16.1. Short Version
https://www.postgresql.org/docs/devel/static/install-short.html

18.2. Creating a Database Cluster
https://www.postgresql.org/docs/devel/static/creating-cluster.html

I agree that we should update the documention for this, including those.

We'll add that to the next patch.

Thanks,
--
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:

At miscinit.c:893:

/* We can treat the EPERM-error case as okay because that error implies
that the existing process has a different userid than we do, which means
it cannot be a competing postmaster. A postmaster cannot successfully
attach to a data directory owned by a userid other than its own. (This
is now checked directly in checkDataDir(), but has been true for a long
time because of the restriction that the data directory isn't group- or
world-accessible.) Also, since we create the lockfiles mode 600, we'd
have failed above if the lockfile belonged to another userid --- which
means that whatever process kill() is reporting about isn't the one that
made the lockfile. (NOTE: this last consideration is the only one that
keeps us from blowing away a Unix socket file belonging to an instance
of Postgres being run by someone else, at least on machines where /tmp
hasn't got a stickybit.) */

TBH, the fact that we're relying on 0600 mode for considerations such
as these makes me tremendously afraid of this whole patch. I think that
the claimed advantages are not anywhere near worth the risk that somebody
is going to destroy their database because we weakened some aspect of the
protection against starting multiple postmasters in a database directory.

At the very least, I'd want to see much closer analysis of the safety
issues than I've seen so far in this thread. And since the proposed
patch falsifies the above-quoted comment (and probably others), why
hasn't it updated it?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

... oh, and now that I've actually looked at the patch, I think it's
a seriously bad idea to proceed by removing the mode parameter to
PathNameOpenFile et al. That's basically doubling down on an assumption
that there are NO places in the backend, and never will be any, in which
we want to create files with nondefault permissions. That assumption
seems broken on its face. It also makes the patch exceedingly invasive
for extensions.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Tom,

On 3/13/17 1:13 PM, Tom Lane wrote:

... oh, and now that I've actually looked at the patch, I think it's
a seriously bad idea to proceed by removing the mode parameter to
PathNameOpenFile et al. That's basically doubling down on an assumption
that there are NO places in the backend, and never will be any, in which
we want to create files with nondefault permissions. That assumption
seems broken on its face. It also makes the patch exceedingly invasive
for extensions.

I think it's a bad idea to have the same parameters copied over and over
throughout the code with slight variations (e.g. 0600 vs S_IRUSR |
S_IWUSR) but the same intent.

In all cases there is another version of the function (added by this
patch) that accepts a mode parameter. In practice this was only needed
in one place, be_lo_export(). I think this makes a pretty good argument
for standardization/simplification in other areas.

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Tom,

On 3/13/17 1:03 PM, Tom Lane wrote:

David Steele <david@pgmasters.net> writes:

At miscinit.c:893:

/* We can treat the EPERM-error case as okay because that error implies
that the existing process has a different userid than we do, which means
it cannot be a competing postmaster. A postmaster cannot successfully
attach to a data directory owned by a userid other than its own. (This
is now checked directly in checkDataDir(), but has been true for a long
time because of the restriction that the data directory isn't group- or
world-accessible.) Also, since we create the lockfiles mode 600, we'd
have failed above if the lockfile belonged to another userid --- which
means that whatever process kill() is reporting about isn't the one that
made the lockfile. (NOTE: this last consideration is the only one that
keeps us from blowing away a Unix socket file belonging to an instance
of Postgres being run by someone else, at least on machines where /tmp
hasn't got a stickybit.) */

TBH, the fact that we're relying on 0600 mode for considerations such
as these makes me tremendously afraid of this whole patch. I think that
the claimed advantages are not anywhere near worth the risk that somebody
is going to destroy their database because we weakened some aspect of the
protection against starting multiple postmasters in a database directory.

I don't see a risk if the user uses umask 0027 which is the example
given in the docs. I'm happy to change this example to a strong
recommendation.

At the very least, I'd want to see much closer analysis of the safety
issues than I've seen so far in this thread.

I think it's clear that there would be safety risks with unwise umask
choices. I also think the example/recommended umask is safe.

Running external processes as the postgres user carries serious risks as
well. Not only with regards to data access but the danger of corruption
due to bugs. If a process does not require write access to do its job
then why take that risk?

To (hopefully) address your concerns, I'll perform an analysis of
starting multiple postmasters with a variety of umask choices and report
the outcomes here.

And since the proposed
patch falsifies the above-quoted comment (and probably others), why
hasn't it updated it?

That was an oversight on my part. I'll update it in the next patch.

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Steele <david@pgmasters.net> writes:

On 3/13/17 1:03 PM, Tom Lane wrote:

TBH, the fact that we're relying on 0600 mode for considerations such
as these makes me tremendously afraid of this whole patch. I think that
the claimed advantages are not anywhere near worth the risk that somebody
is going to destroy their database because we weakened some aspect of the
protection against starting multiple postmasters in a database directory.

I don't see a risk if the user uses umask 0027 which is the example
given in the docs. I'm happy to change this example to a strong
recommendation.

I do not want a "strong recommendation". I want "we don't let you
break this". We don't let you run the postmaster as root either,
even though there have been repeated requests to remove that safety
check.

It might be all right if we forcibly or'd 027 into whatever umask
the user tries to provide; not sure. The existing safety analysis,
such as the cited comment, has all been based on the assumption of
file mode 600 not mode 640. I'm not certain that we always attempt
to open-for-writing files that we expect to be exclusively accessible
by the postmaster.

Anyway, given that we do that analysis, I'd rather not expose this
as a "here, set the umask you want" variable. I think a bool saying
"allow group access" (translating to exactly two supported umasks,
027 and 077) would be simpler from the user's standpoint and much
easier to reason about. I don't see the value in having to think
about what happens if the user supplies a mask like 037 or 067.

I also don't especially want to have to analyze cases like "what
happens if user initdb'd with mask X but then changes the GUC and
restarts the postmaster?". Maybe the right thing is to not expose
this as a GUC at all, but drive it off the permissions observed on
the data directory at postmaster start. Viz:

* $PGDATA has permissions 0700: adopt umask 077

* $PGDATA has permissions 0750: adopt umask 027

* anything else: fail

That way, a "chmod -R" would be the necessary and sufficient procedure
for switching from one case to the other.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/13/17 2:16 PM, Tom Lane wrote:

David Steele <david@pgmasters.net> writes:

On 3/13/17 1:03 PM, Tom Lane wrote:

TBH, the fact that we're relying on 0600 mode for considerations such
as these makes me tremendously afraid of this whole patch. I think that
the claimed advantages are not anywhere near worth the risk that somebody
is going to destroy their database because we weakened some aspect of the
protection against starting multiple postmasters in a database directory.

I don't see a risk if the user uses umask 0027 which is the example
given in the docs. I'm happy to change this example to a strong
recommendation.

<...>

Anyway, given that we do that analysis, I'd rather not expose this
as a "here, set the umask you want" variable. I think a bool saying
"allow group access" (translating to exactly two supported umasks,
027 and 077) would be simpler from the user's standpoint and much
easier to reason about. I don't see the value in having to think
about what happens if the user supplies a mask like 037 or 067.

We debated a flag vs a umask and came down on the side of flexibility.
I'm perfectly happy with using a flag instead.

I also don't especially want to have to analyze cases like "what
happens if user initdb'd with mask X but then changes the GUC and
restarts the postmaster?". Maybe the right thing is to not expose
this as a GUC at all, but drive it off the permissions observed on
the data directory at postmaster start. Viz:

* $PGDATA has permissions 0700: adopt umask 077

* $PGDATA has permissions 0750: adopt umask 027

* anything else: fail

How about a GUC, allow_group_access, that when set will enforce
permissions and set the umask accordingly, and when not set will follow
$PGDATA as proposed above?

Not much we can do for Windows, though. I think it would have to WARN
if the GUC is set and then continue as usual.

That way, a "chmod -R" would be the necessary and sufficient procedure
for switching from one case to the other.

I'm OK with that if you think it's the best course, but perhaps the GUC
would be better because it can detect accidental permission changes.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers