Thread: [HACKERS] PATCH: Configurable file mode mask
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. 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. This obviously required mode changes in a number of places, so at the same time the BasicOpenFile(), OpenTransientFile(), and PathNameOpenFile() have been split into versions that either use the default permissions or allow custom permissions. In the end there was only one call to the custom permission version (be-fsstubs.c:505) for all three variants. The following three calls (at the least) need to be reviewed: bin/pg_dump/pg_backup_directory.c:194 src/port/mkdtemp.c:190 bin/pg_basebackup.c:599:655:1399 And this call needs serious consideration: bin/pg_rewind/file_ops.c:214 Besides that there should be tests to make sure the masks are working as expected and these could be added to the initdb TAP tests, though no mask tests exist at this time. Making sure all file operations produce the correct modes would need to be placed in a new module, perhaps the new backup tests proposed in [1]. Adam Brightwell developed the patch based on an initial concept by me and Stephen Frost. I added the refactoring in fd.c and some additional documentation. This patch applies cleanly on 016c990 but may fare badly over time due to the number of files modified. -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/758e3fd1-45b4-5e28-75cd-e9e7f93a4c02@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
Attachment
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
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
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
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
Attachment
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
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
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 ofadministration 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'tthe 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 allowhim 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 backsup 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 userto 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 thecurrent 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. Theyexpect it to be back up with other database files. So I'm afraid the permission of .pgpass file also becomes 0640 sometime. 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
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 rangeof 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'tthe 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 allowhim 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 backsup 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 oneuser 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. Butthe 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. Theyexpect it to be back up with other database files. So I'm afraid the permission of .pgpass file also becomes 0640 sometime. 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. Butthe 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
... 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
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
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
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
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
David Steele <david@pgmasters.net> writes:
> On 3/13/17 2:16 PM, Tom Lane wrote:
>> 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?
Seems overcomplicated ...
> 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.
Yeah, the Windows port has been weak in this area all along. I don't
think it's incumbent on you to make it better.
>> 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.
If we're only checking file permissions at postmaster start, I think it's
illusory to suppose that we're offering very much protection against
accidental changes. A chmod applied while the postmaster is running
could still break things, and we'd not notice till the next restart.
But it might be worth thinking about whether we want to encourage people
to do manual chmod's at all; that's fairly easy to get wrong, particularly
given the difference in X bits that should be applied to files and
directories. Another approach that could be worth considering is
a PGC_POSTMASTER GUC with just two states (group access or not) and
make it the postmaster's responsibility to do the equivalent of chmod -R
to make the file tree match the GUC. I think we do a tree scan anyway
for other purposes, so correcting any wrong file permissions might not
be much added work in the normal case.
regards, tom lane
From: David Steele [mailto:david@pgmasters.net] > >> 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. Maybe so, but it's convenient to be able to store the key and certificate files in $PGDATA and back them up together. Ifthe database backup were stolen through the compromised backup OS user, then the malicious person can read the data anywayregardless of whether the key file is there or not. That's because the key is not for encrypting data at rest. Related to this, please see: https://www.postgresql.org/docs/devel/static/ssl-tcp.html "On Unix systems, the permissions on server.key must disallow any access to world or group; achieve this by the command chmod0600 server.key. Alternatively, the file can be owned by root and have group read access (that is, 0640 permissions).That setup is intended for installations where certificate and key files are managed by the operating system.The user under which the PostgreSQL server runs should then be made a member of the group that has access to thosecertificate and key files." In the latter case, the file owner is root and the permission is 0640. At first I was a bit confused and misunderstood thatthe PostgreSQL user account and the backup OS user needs to belong to the same OS group. But that's not the case. Thegroup of the key file can be, for example, "ssl_cert", the PostgreSQL user account belongs to the OS group "ssl_cert"and "dba", and the backup OS user only belongs to "backup." This can prevent the backup OS user from reading thekey file. I think it would be better to have some explanation with examples in the above section. Regards Takayuki Tsunakawa
On 3/14/17 4:23 AM, Tsunakawa, Takayuki wrote: > From: David Steele [mailto:david@pgmasters.net] >>>> 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. > > Maybe so, but it's convenient to be able to store the key and certificate files in $PGDATA and back them up together. If the database backup were stolen through the compromised backup OS user, then the malicious person can read the data anywayregardless of whether the key file is there or not. That's because the key is not for encrypting data at rest. Sure, but having the private key may allow them to get new data from the server as well as the data from the backup. To be clear, the default for this patch is to leave permissions exactly as they are now. It also provides alternatives that may or not be useful in all cases. > Related to this, please see: > > https://www.postgresql.org/docs/devel/static/ssl-tcp.html > > "On Unix systems, the permissions on server.key must disallow any access to world or group; achieve this by the commandchmod 0600 server.key. Alternatively, the file can be owned by root and have group read access (that is, 0640 permissions).That setup is intended for installations where certificate and key files are managed by the operating system.The user under which the PostgreSQL server runs should then be made a member of the group that has access to thosecertificate and key files." > > In the latter case, the file owner is root and the permission is 0640. At first I was a bit confused and misunderstoodthat the PostgreSQL user account and the backup OS user needs to belong to the same OS group. But that's notthe case. The group of the key file can be, for example, "ssl_cert", the PostgreSQL user account belongs to the OS group"ssl_cert" and "dba", and the backup OS user only belongs to "backup." This can prevent the backup OS user from readingthe key file. I think it would be better to have some explanation with examples in the above section. If the backup user is in the same group as the postgres user and in the ssl_cert group then backups of the certs would be possible using group reads. Restores will be a little tricky, though, because of the need to set ownership to root. The restore would need to be run as root or the permissions fixed up after the restore completes. -- -David david@pgmasters.net
On 3/13/17 3:03 PM, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 3/13/17 2:16 PM, Tom Lane wrote: >>> 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? > > Seems overcomplicated ... Yeah. It wouldn't be a lot of fun to document, that's for sure. >>> 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. > > If we're only checking file permissions at postmaster start, I think it's > illusory to suppose that we're offering very much protection against > accidental changes. A chmod applied while the postmaster is running > could still break things, and we'd not notice till the next restart. Fair enough. > But it might be worth thinking about whether we want to encourage people > to do manual chmod's at all; that's fairly easy to get wrong, particularly > given the difference in X bits that should be applied to files and > directories. Another approach that could be worth considering is > a PGC_POSTMASTER GUC with just two states (group access or not) and > make it the postmaster's responsibility to do the equivalent of chmod -R > to make the file tree match the GUC. I think we do a tree scan anyway > for other purposes, so correcting any wrong file permissions might not > be much added work in the normal case. The majority of scanning is done in recovery (to find and remove unlogged tables) and I'm not sure we would want to add that overhead to normal startup. -- -David david@pgmasters.net
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele > Sure, but having the private key may allow them to get new data from the > server as well as the data from the backup. You are right. My rough intent was that the data is stolen anyway. So, I thought it might not be so bad to expect to beable to back up the SSL key file in $PGDATA together with the database. If it's bad, then the default value of ssl_key_file(=$PGDATA/ssl.key) should be disallowed. > > https://www.postgresql.org/docs/devel/static/ssl-tcp.html > > > > "On Unix systems, the permissions on server.key must disallow any access > to world or group; achieve this by the command chmod 0600 server.key. > Alternatively, the file can be owned by root and have group read access > (that is, 0640 permissions). That setup is intended for installations where > certificate and key files are managed by the operating system. The user > under which the PostgreSQL server runs should then be made a member of the > group that has access to those certificate and key files." > > > > In the latter case, the file owner is root and the permission is 0640. > At first I was a bit confused and misunderstood that the PostgreSQL user > account and the backup OS user needs to belong to the same OS group. But > that's not the case. The group of the key file can be, for example, > "ssl_cert", the PostgreSQL user account belongs to the OS group "ssl_cert" > and "dba", and the backup OS user only belongs to "backup." This can prevent > the backup OS user from reading the key file. I think it would be better > to have some explanation with examples in the above section. > > If the backup user is in the same group as the postgres user and in the > ssl_cert group then backups of the certs would be possible using group reads. > Restores will be a little tricky, though, because of the need to set > ownership to root. The restore would need to be run as root or the > permissions fixed up after the restore completes. Yes, but I thought, from the following message, that you do not recommend that the backup user be able to read the SSL keyfile. So, I proposed to describe the example configuration to achieve that -- postgres user in dba and ssl_cert group,and a separate backup user in only dba group. > >> 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. > To be clear, the default for this patch is to leave permissions exactly > as they are now. It also provides alternatives that may or not be useful > in all cases. So you think there are configurations that may be useful or not, don't you? Adding a new parameter could possibly complicatewhat users have to consider. Maximal flexibility could lead to insecure misuse. So I think it would be betterto describe secure and practical configuration examples in the SSL section and/or the backup chapter. The configurationfactor includes whether the backup user is different from the postgres user, where the SSL key file is placed,the owner of the SSL key file, whether the backup user can read the SSL key file. Regards Takayuki Tsunakawa
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele > > But it might be worth thinking about whether we want to encourage > > people to do manual chmod's at all; that's fairly easy to get wrong, > > particularly given the difference in X bits that should be applied to > > files and directories. Another approach that could be worth > > considering is a PGC_POSTMASTER GUC with just two states (group access > > or not) and make it the postmaster's responsibility to do the > > equivalent of chmod -R to make the file tree match the GUC. I think > > we do a tree scan anyway for other purposes, so correcting any wrong > > file permissions might not be much added work in the normal case. > > The majority of scanning is done in recovery (to find and remove unlogged > tables) and I'm not sure we would want to add that overhead to normal startup. I'm on David's side, too. I don't postmaster to always scan all files at startup. On the other hand, just doing "chmod -R $PGDATA" is not enough, because chmod doesn't follow the symbolic links. Symboliclinks are used for pg_tblspc/* and pg_wal at least. FYI, MySQL's manual describes the pithole like this: https://dev.mysql.com/doc/refman/8.0/en/changing-mysql-user.html ---------------------------------------- 2. Change the database directories and files so that user_name has privileges to read and write files in them (you mightneed to do this as the Unix root user): shell> chown -R user_name /path/to/mysql/datadir If you do not do this, the server will not be able to access databases or tables when it runs as user_name. If directories or files within the MySQL data directory are symbolic links, chown -R might not follow symbolic links foryou. If it does not, you will also need to follow those links and change the directories and files they point to. ---------------------------------------- I think we also need to describe the procedure carefully. That said, it would be best to make users aware of a configurationalternative (group access) with enough documentation when they first build the database or upgrade the databasecluster. Just describing the alternative only in initdb reference page would result in being unaware of the betterconfiguration, like --data-checksum. Regards Takayuki Tsunakawa
On 3/15/17 1:56 AM, Tsunakawa, Takayuki wrote: > From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele >> Sure, but having the private key may allow them to get new data from the >> server as well as the data from the backup. > > You are right. My rough intent was that the data is stolen anyway. So, I thought it might not be so bad to expect tobe able to back up the SSL key file in $PGDATA together with the database. If it's bad, then the default value of ssl_key_file(=$PGDATA/ssl.key) should be disallowed. I think it really depends on the situation and the user needs to make an evaluation of the security risks for themselves. >> If the backup user is in the same group as the postgres user and in the >> ssl_cert group then backups of the certs would be possible using group reads. >> Restores will be a little tricky, though, because of the need to set >> ownership to root. The restore would need to be run as root or the >> permissions fixed up after the restore completes. > > Yes, but I thought, from the following message, that you do not recommend that the backup user be able to read the SSLkey file. So, I proposed to describe the example configuration to achieve that -- postgres user in dba and ssl_cert group,and a separate backup user in only dba group. That would work as long as the key was not in $PGDATA. Most database backup software does not have a --ignore option because of the obvious dangers. >>>> 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. > >> To be clear, the default for this patch is to leave permissions exactly >> as they are now. It also provides alternatives that may or not be useful >> in all cases. > > So you think there are configurations that may be useful or not, don't you? Adding a new parameter could possibly complicatewhat users have to consider. Maximal flexibility could lead to insecure misuse. So I think it would be betterto describe secure and practical configuration examples in the SSL section and/or the backup chapter. The configurationfactor includes whether the backup user is different from the postgres user, where the SSL key file is placed,the owner of the SSL key file, whether the backup user can read the SSL key file. I think we are more or less on the same page, and you'd like to see documentation that shows examples of secure configurations when group access is allow. I agree and I'm working on documentation for all the sections you recommended. Thanks, -- -David david@pgmasters.net
On 3/15/17 3:00 AM, Tsunakawa, Takayuki wrote: > From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele >>> But it might be worth thinking about whether we want to encourage >>> people to do manual chmod's at all; that's fairly easy to get wrong, >>> particularly given the difference in X bits that should be applied to >>> files and directories. Another approach that could be worth >>> considering is a PGC_POSTMASTER GUC with just two states (group access >>> or not) and make it the postmaster's responsibility to do the >>> equivalent of chmod -R to make the file tree match the GUC. I think >>> we do a tree scan anyway for other purposes, so correcting any wrong >>> file permissions might not be much added work in the normal case. >> >> The majority of scanning is done in recovery (to find and remove unlogged >> tables) and I'm not sure we would want to add that overhead to normal startup. > > I'm on David's side, too. I don't postmaster to always scan all files at startup. > > On the other hand, just doing "chmod -R $PGDATA" is not enough, because chmod doesn't follow the symbolic links. Symboliclinks are used for pg_tblspc/* and pg_wal at least. FYI, MySQL's manual describes the pithole like this: Good point - I think we'll need to add that to the docs as well. > I think we also need to describe the procedure carefully. That said, it would be best to make users aware of a configurationalternative (group access) with enough documentation when they first build the database or upgrade the databasecluster. Just describing the alternative only in initdb reference page would result in being unaware of the betterconfiguration, like --data-checksum. I'm working on a new approach incorporating everybody's suggestions and enhanced documentation. It should be ready on Monday. -- -David david@pgmasters.net
On Wed, Mar 15, 2017 at 3:00 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > I'm on David's side, too. I don't postmaster to always scan all files at startup. +1. Even just doing it during crash recovery, it can take a regrettably long time on machines with tons of relations and a very slow disk. I've been sort of thinking that we should add some logging there so that users know what's happening when that code goes into the tank - I've seen that come up 3 or 4 times now, and I'm getting tired of telling people to run strace to find out. I think Tom's concerns about people doing insecure stuff are excessive. People can do insecure stuff no matter what we do, and trying to prevent them often leads to them doing even-more-insecure stuff. That having been aid, I do wonder whether the idea of allowing group read privileges specifically might be a better concept than a umask, though, because (1) it's not obvious that there's a real use case for anything other than group read privileges, so why not support exactly that to avoid user confusion and (2) umask is a pretty specific concept that may not apply on every platform. For example, AFS has an ACL list instead of using the traditional UNIX permission bits, and I'm not sure Windows has the umask concept exactly either. So wording what we're trying to do a bit more generically might be smart. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 3/18/17 3:57 PM, Robert Haas wrote: > On Wed, Mar 15, 2017 at 3:00 AM, Tsunakawa, Takayuki > <tsunakawa.takay@jp.fujitsu.com> wrote: >> I'm on David's side, too. I don't postmaster to always scan all files at startup. > > +1. Even just doing it during crash recovery, it can take a > regrettably long time on machines with tons of relations and a very > slow disk. I've been sort of thinking that we should add some logging > there so that users know what's happening when that code goes into the > tank - I've seen that come up 3 or 4 times now, and I'm getting tired > of telling people to run strace to find out. Most of this time is spent scanning for unlogged tables. We were working on a patch to put unlogged tables (except for the init fork) in a pg_unlogged directory created for each tablespace, just like pgsql_temp. This would make cleanup a lot faster and make it easier for backup software to skip relations that can't be restored and only take up space. Unfortunately the patch was not ready for the last CF and maybe would not have been a good candidate anyway due to complexity. It's still on our radar, though. > I think Tom's concerns about people doing insecure stuff are > excessive. People can do insecure stuff no matter what we do, and > trying to prevent them often leads to them doing even-more-insecure > stuff. That having been aid, I do wonder whether the idea of allowing > group read privileges specifically might be a better concept than a > umask, though, because (1) it's not obvious that there's a real use > case for anything other than group read privileges, so why not support > exactly that to avoid user confusion and (2) umask is a pretty > specific concept that may not apply on every platform. For example, > AFS has an ACL list instead of using the traditional UNIX permission > bits, and I'm not sure Windows has the umask concept exactly either. > So wording what we're trying to do a bit more generically might be > smart. We took Tom's advice to heart and this is the direction the patch is currently going in. Even the GUC may be too much as there are number of tools that write into PGDATA but don't read postgresql.conf. It looks like using the permissions of PGDATA may be the best way to go. In any case, the changes required have enlarged the size and scope of the patch considerably and we are not confident that it will be done in time to commit for v10. I have marked this submission "Returned with Feedback". -- -David david@pgmasters.net
On 3/21/17 2:02 PM, David Steele wrote: > On 3/18/17 3:57 PM, Robert Haas wrote: > >> I think Tom's concerns about people doing insecure stuff are >> excessive. People can do insecure stuff no matter what we do, and >> trying to prevent them often leads to them doing even-more-insecure >> stuff. That having been aid, I do wonder whether the idea of allowing >> group read privileges specifically might be a better concept than a >> umask, though, because (1) it's not obvious that there's a real use >> case for anything other than group read privileges, so why not support >> exactly that to avoid user confusion and (2) umask is a pretty >> specific concept that may not apply on every platform. For example, >> AFS has an ACL list instead of using the traditional UNIX permission >> bits, and I'm not sure Windows has the umask concept exactly either. >> So wording what we're trying to do a bit more generically might be >> smart. > > We took Tom's advice to heart and this is the direction the patch is > currently going in. Even the GUC may be too much as there are number of > tools that write into PGDATA but don't read postgresql.conf. It looks > like using the permissions of PGDATA may be the best way to go. > > In any case, the changes required have enlarged the size and scope of > the patch considerably and we are not confident that it will be done in > time to commit for v10. > > I have marked this submission "Returned with Feedback". Attached is a new patch set that should address various concerns raised in this thread. 1) group-access-v3-01-mkdir.patch Abstracts all mkdir calls in the backend into a MakeDirectory() function implemented in fd.c. This did not get committed in September as part of 0c5803b450e but I still think it has value. However, I have kept it separate to reduce noise in the second patch. The mkdir() calls could also be modified to use PG_DIR_MODE_DEFAULT with equivalent results. 2) group-access-v3-02-group.patch This is a "GUC-less" implementation of group read access that depends on the mode of the $PGDATA directory to determine which mode to use for subsequent writes. The initdb option is preserved to allow group access to be enabled when the cluster is initialized. Only two modes are allowed (700, 750) and the error message on startup is hard-coded to address translation concerns. I'll add this to the 2018-01 CF. Thanks! -- -David david@pgmasters.net
Attachment
On Thu, Dec 28, 2017 at 2:36 PM, David Steele <david@pgmasters.net> wrote: > Attached is a new patch set that should address various concerns raised in > this thread. > > 1) group-access-v3-01-mkdir.patch > > Abstracts all mkdir calls in the backend into a MakeDirectory() function > implemented in fd.c. This did not get committed in September as part of > 0c5803b450e but I still think it has value. However, I have kept it > separate to reduce noise in the second patch. The mkdir() calls could also > be modified to use PG_DIR_MODE_DEFAULT with equivalent results. +/* + * Default mode for created directories, unless something else is specified + * using the MakeDirectoryPerm() function variant. + */ +#define PG_DIR_MODE_DEFAULT (S_IRWXU) +#define PG_MODE_MASK_DEFAULT (S_IRWXG | S_IRWXO) There's only one comment here, but there are two constants that do different things. Also, this hunk gets removed again by 02, which is probably OK, but 02 adds the replacement stuff in a different part of the file, which seems not as good. I think MakeDirectory() is a good wrapper, but isn't MakeDirectoryPerm() sort of silly? > 2) group-access-v3-02-group.patch > > This is a "GUC-less" implementation of group read access that depends on the > mode of the $PGDATA directory to determine which mode to use for subsequent > writes. The initdb option is preserved to allow group access to be enabled > when the cluster is initialized. > > Only two modes are allowed (700, 750) and the error message on startup is > hard-coded to address translation concerns. + umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT)); Hmm, I wonder if this is going to be 100% portable. Maybe some obscure platform won't like an argument with all the high bits set. Also, why should we break what's now one #if into two? That seems like it's mildly more complicated for no gain. + (These files can confuse <application>pg_ctl</application>.) If group read + access is enabled on the data directory and an unprivileged user in the + <productname>PostgreSQL</productname> group is performing the backup, then + <filename>postmaster.pid</filename> will not be readable and must be + excluded. Is that actually the behavior we want? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Robert, Thanks for looking at the patches. On 12/31/17 1:27 PM, Robert Haas wrote: > On Thu, Dec 28, 2017 at 2:36 PM, David Steele <david@pgmasters.net> wrote: >> Attached is a new patch set that should address various concerns raised in >> this thread. >> >> 1) group-access-v3-01-mkdir.patch >> >> Abstracts all mkdir calls in the backend into a MakeDirectory() function >> implemented in fd.c. This did not get committed in September as part of >> 0c5803b450e but I still think it has value. However, I have kept it >> separate to reduce noise in the second patch. The mkdir() calls could also >> be modified to use PG_DIR_MODE_DEFAULT with equivalent results. > > +/* > + * Default mode for created directories, unless something else is specified > + * using the MakeDirectoryPerm() function variant. > + */ > +#define PG_DIR_MODE_DEFAULT (S_IRWXU) > +#define PG_MODE_MASK_DEFAULT (S_IRWXG | S_IRWXO) > > There's only one comment here, but there are two constants that do > different things. Looks like a copy pasto - I didn't mean PG_MODE_MASK_DEFAULT to be in patch 01 at all. Removed. > Also, this hunk gets removed again by 02, which is probably OK, but 02 > adds the replacement stuff in a different part of the file, which > seems not as good. Agreed. I have moved that. > > I think MakeDirectory() is a good wrapper, but isn't > MakeDirectoryPerm() sort of silly? There's one place in the backend (storage/ipc/ipc.c) that sets non-default directory permissions. This function is intended to support that and any extensions that need to set custom perms. >> 2) group-access-v3-02-group.patch >> >> This is a "GUC-less" implementation of group read access that depends on the >> mode of the $PGDATA directory to determine which mode to use for subsequent >> writes. The initdb option is preserved to allow group access to be enabled >> when the cluster is initialized. >> >> Only two modes are allowed (700, 750) and the error message on startup is >> hard-coded to address translation concerns. > > + umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT)); > > Hmm, I wonder if this is going to be 100% portable. Maybe some > obscure platform won't like an argument with all the high bits set. Sure - I have masked this with 0777 to clear any high bits. Sound OK? > Also, why should we break what's now one #if into two? That seems > like it's mildly more complicated for no gain. There were already two #ifdef blocks so I added a third. I think we should leave it as three or combine them all into one. There are no runtime performance implications so I'm agnostic. > + (These files can confuse <application>pg_ctl</application>.) If group read > + access is enabled on the data directory and an unprivileged user in the > + <productname>PostgreSQL</productname> group is performing the backup, then > + <filename>postmaster.pid</filename> will not be readable and must be > + excluded. > > Is that actually the behavior we want? I think it is. Comments in the code are pretty specific about not changing the permissions on postmaster.pid and I don't see any reason to mess with it. Copying postmaster.pid as part of a backup is not a good idea anyway. New patches attached. Thanks! -- -David david@pgmasters.net
Attachment
On Tue, Jan 2, 2018 at 11:43 AM, David Steele <david@pgmasters.net> wrote: >> > I think MakeDirectory() is a good wrapper, but isn't >> MakeDirectoryPerm() sort of silly? > > There's one place in the backend (storage/ipc/ipc.c) that sets non-default > directory permissions. This function is intended to support that and any > extensions that need to set custom perms. Yeah, but all it does is call mkdir(), which could just as well be called directly. I think there's a pointer to a wrapper when it does something for you -- supply an argument, log something, handle portability concerns -- but this wrapper does exactly nothing. >> + umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT)); >> >> Hmm, I wonder if this is going to be 100% portable. Maybe some >> obscure platform won't like an argument with all the high bits set. > > Sure - I have masked this with 0777 to clear any high bits. Sound OK? Seems a little strange to spell it that way when we're using constants everywhere else. How about writing it like this: umask(PG_DIR_MODE_DEFAULT & ~stat_buf.st_mode); I think that reads as "clear all bits from PG_DIR_MODE_DEFAULT that are not set in stat_buf.st_mode", which sounds like what we want. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 1/3/18 08:11, Robert Haas wrote: > On Tue, Jan 2, 2018 at 11:43 AM, David Steele <david@pgmasters.net> wrote: >>>> I think MakeDirectory() is a good wrapper, but isn't >>> MakeDirectoryPerm() sort of silly? >> >> There's one place in the backend (storage/ipc/ipc.c) that sets non-default >> directory permissions. This function is intended to support that and any >> extensions that need to set custom perms. > > Yeah, but all it does is call mkdir(), which could just as well be > called directly. I think there's a pointer to a wrapper when it does > something for you -- supply an argument, log something, handle > portability concerns -- but this wrapper does exactly nothing. Yeah, I didn't like this aspect when this patch was originally submitted. We want to keep the code legible for future new contributors. Having these generic-sounding but specific-in-purpose wrapper functions can be pretty confusing. Let's use mkdir() when it's the appropriate function, and let's figure out a different name for "make a data directory subdirectory in a secure and robust way". -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/8/18 8:58 PM, Peter Eisentraut wrote: > On 1/3/18 08:11, Robert Haas wrote: >> On Tue, Jan 2, 2018 at 11:43 AM, David Steele <david@pgmasters.net> wrote: >>>>> I think MakeDirectory() is a good wrapper, but isn't >>>> MakeDirectoryPerm() sort of silly? >>> >>> There's one place in the backend (storage/ipc/ipc.c) that sets non-default >>> directory permissions. This function is intended to support that and any >>> extensions that need to set custom perms. >> >> Yeah, but all it does is call mkdir(), which could just as well be >> called directly. I think there's a pointer to a wrapper when it does >> something for you -- supply an argument, log something, handle >> portability concerns -- but this wrapper does exactly nothing. > > Yeah, I didn't like this aspect when this patch was originally > submitted. We want to keep the code legible for future new > contributors. Having these generic-sounding but specific-in-purpose > wrapper functions can be pretty confusing. Let's use mkdir() when it's > the appropriate function, and let's figure out a different name for > "make a data directory subdirectory in a secure and robust way". I think there is value to keeping the function names symmetric to the FileOpen()/FileOpenPerm() variants but I'm clearly in the minority so there's no point in pursuing it. How about MakeDirectoryDefaultPerm()? That's what I'll go with if I don't hear any other ideas. The single call to MakeDirectoryPerm() will be reverted to mkdir() and I'll remove the function. Thanks, -- -David david@pgmasters.net
David, * David Steele (david@pgmasters.net) wrote: > On 1/8/18 8:58 PM, Peter Eisentraut wrote: > > On 1/3/18 08:11, Robert Haas wrote: > >> On Tue, Jan 2, 2018 at 11:43 AM, David Steele <david@pgmasters.net> wrote: > >>>>> I think MakeDirectory() is a good wrapper, but isn't > >>>> MakeDirectoryPerm() sort of silly? > >>> > >>> There's one place in the backend (storage/ipc/ipc.c) that sets non-default > >>> directory permissions. This function is intended to support that and any > >>> extensions that need to set custom perms. > >> > >> Yeah, but all it does is call mkdir(), which could just as well be > >> called directly. I think there's a pointer to a wrapper when it does > >> something for you -- supply an argument, log something, handle > >> portability concerns -- but this wrapper does exactly nothing. > > > > Yeah, I didn't like this aspect when this patch was originally > > submitted. We want to keep the code legible for future new > > contributors. Having these generic-sounding but specific-in-purpose > > wrapper functions can be pretty confusing. Let's use mkdir() when it's > > the appropriate function, and let's figure out a different name for > > "make a data directory subdirectory in a secure and robust way". > > I think there is value to keeping the function names symmetric to the > FileOpen()/FileOpenPerm() variants but I'm clearly in the minority so > there's no point in pursuing it. > > How about MakeDirectoryDefaultPerm()? That's what I'll go with if I > don't hear any other ideas. The single call to MakeDirectoryPerm() will > be reverted to mkdir() and I'll remove the function. I would be sure to include a good comment/explanation about why mkdir() is being used there and not MakeDirectoryDefaultPerm() (unless one exists already), just to avoid later hackers thinking that mkdir() is the way to go in general when, in most cases, MakeDirectoryDefaultPerm() should be used. Thanks! Stephen
Attachment
David Steele wrote: > On 1/8/18 8:58 PM, Peter Eisentraut wrote: > > Yeah, I didn't like this aspect when this patch was originally > > submitted. We want to keep the code legible for future new > > contributors. Having these generic-sounding but specific-in-purpose > > wrapper functions can be pretty confusing. Let's use mkdir() when it's > > the appropriate function, and let's figure out a different name for > > "make a data directory subdirectory in a secure and robust way". > How about MakeDirectoryDefaultPerm()? That's what I'll go with if I > don't hear any other ideas. The single call to MakeDirectoryPerm() will > be reverted to mkdir() and I'll remove the function. I'd go with MakeDirectory, documenting exactly what it does and why, and be done with it. If your new function satisfies future users, great; if not, it can be patched (or not) once we know exactly what these callers need. You know, YAGNI. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/10/18 12:37, David Steele wrote: > How about MakeDirectoryDefaultPerm()? That's what I'll go with if I > don't hear any other ideas. The single call to MakeDirectoryPerm() will > be reverted to mkdir() and I'll remove the function. Works for me. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jan 10, 2018 at 03:19:46PM -0300, Alvaro Herrera wrote: > David Steele wrote: > > On 1/8/18 8:58 PM, Peter Eisentraut wrote: > > > > Yeah, I didn't like this aspect when this patch was originally > > > submitted. We want to keep the code legible for future new > > > contributors. Having these generic-sounding but specific-in-purpose > > > wrapper functions can be pretty confusing. Let's use mkdir() when it's > > > the appropriate function, and let's figure out a different name for > > > "make a data directory subdirectory in a secure and robust way". > > > How about MakeDirectoryDefaultPerm()? That's what I'll go with if I > > don't hear any other ideas. The single call to MakeDirectoryPerm() will > > be reverted to mkdir() and I'll remove the function. > > I'd go with MakeDirectory, documenting exactly what it does and why, and > be done with it. If your new function satisfies future users, great; if > not, it can be patched (or not) once we know exactly what these callers > need. After going through the thread, I would vote for making things simple by just using MakeDirectory() and document precisely what it does. Anyway, there is as well the approach of using MakeDirectoryDefaultPerm(), so I'll be fine with the decision you make, David. The patchis moved to "waiting on author". -- Michael
Attachment
On 1/19/18 3:08 AM, Michael Paquier wrote: > On Wed, Jan 10, 2018 at 03:19:46PM -0300, Alvaro Herrera wrote: >> David Steele wrote: >>> On 1/8/18 8:58 PM, Peter Eisentraut wrote: >> >>>> Yeah, I didn't like this aspect when this patch was originally >>>> submitted. We want to keep the code legible for future new >>>> contributors. Having these generic-sounding but specific-in-purpose >>>> wrapper functions can be pretty confusing. Let's use mkdir() when it's >>>> the appropriate function, and let's figure out a different name for >>>> "make a data directory subdirectory in a secure and robust way". >> >>> How about MakeDirectoryDefaultPerm()? That's what I'll go with if I >>> don't hear any other ideas. The single call to MakeDirectoryPerm() will >>> be reverted to mkdir() and I'll remove the function. >> >> I'd go with MakeDirectory, documenting exactly what it does and why, and >> be done with it. If your new function satisfies future users, great; if >> not, it can be patched (or not) once we know exactly what these callers >> need. > > After going through the thread, I would vote for making things simple by > just using MakeDirectory() and document precisely what it does. Anyway, > there is as well the approach of using MakeDirectoryDefaultPerm(), so > I'll be fine with the decision you make, David. The patchis moved to > "waiting on author". I ended up with MakeDirectoryDefaultPerm(), but I'm flexible. Attached is a new patch set that also adds the following to patch 02. 1) Move permission constants to a new file in common so they can be shared with the front end. 2) Update all client programs that write to PGDATA: initdb, pg_ctl, pg_resetwal, pg_rewind, pg_upgrade. 3) Add tests for initdb, pg_ctl, and pg_rewind. I have yet to add tests for pg_rewindwal and pg_upgrade. pg_rewindwal doesn't *have* any tests as far as I can tell and pg_upgrade has tests in a shell script -- it's not clear how I would extend it or reuse the Perl code for perm testing. Does anyone have suggestions on tests for pg_resetwal and pg_upgrade? Should I start from scratch? Thanks, -- -David david@pgmasters.net
Attachment
On 1/19/18 14:07, David Steele wrote: > I have yet to add tests for pg_rewindwal and pg_upgrade. pg_rewindwal > doesn't *have* any tests as far as I can tell and pg_upgrade has tests > in a shell script -- it's not clear how I would extend it or reuse the > Perl code for perm testing. > > Does anyone have suggestions on tests for pg_resetwal and pg_upgrade? > Should I start from scratch? A test suite for pg_resetwal would be nice. TAP tests for pg_upgrade will create problems with the build farm. There was a recent thread about that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote:
> On 1/19/18 14:07, David Steele wrote:
> > I have yet to add tests for pg_rewindwal and pg_upgrade. pg_rewindwal
> > doesn't *have* any tests as far as I can tell and pg_upgrade has tests
> > in a shell script -- it's not clear how I would extend it or reuse the
> > Perl code for perm testing.
> >
> > Does anyone have suggestions on tests for pg_resetwal and pg_upgrade?
> > Should I start from scratch?
>
> A test suite for pg_resetwal would be nice.
>
> TAP tests for pg_upgrade will create problems with the build farm.
> There was a recent thread about that.
Is this about this commit?
commit 58ffe141eb37c3f027acd25c1fc6b36513bf9380
Author: Peter Eisentraut <peter_e@gmx.net>
AuthorDate: Fri Sep 22 16:34:46 2017 -0400
CommitDate: Fri Sep 22 16:46:56 2017 -0400
Revert "Add basic TAP test setup for pg_upgrade"
This reverts commit f41e56c76e39f02bef7ba002c9de03d62b76de4d.
The build farm client would run the pg_upgrade tests twice, once as part
of the existing pg_upgrade check run and once as part of picking up all
TAP tests by looking for "t" directories. Since the pg_upgrade tests
are pretty slow, we will need a better solution or possibly a build farm
client change before we can proceed with this.
If the only problem is that buildfarm would run tests twice, then I
think we should just press forward with this regardless of that: it
seems a chicken-and-egg problem, because buildfarm cannot be upgraded to
use some new test method if the method doesn't exist yet. As a
solution, let's just live with some run duplication for a period until
the machines are upgraded to a future buildfarm client code.
If there are other problems, let's see what they are so that we can fix
them.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 19, 2018 at 06:54:23PM -0300, Alvaro Herrera wrote: > Peter Eisentraut wrote: > If the only problem is that buildfarm would run tests twice, then I > think we should just press forward with this regardless of that: it > seems a chicken-and-egg problem, because buildfarm cannot be upgraded to > use some new test method if the method doesn't exist yet. As a > solution, let's just live with some run duplication for a period until > the machines are upgraded to a future buildfarm client code. > > If there are other problems, let's see what they are so that we can fix > them. The main complain I received about this move of the pg_upgrade tests to be a TAP one is that they don't support easily cross-major version upgrades, removing some abilities to what a developer or the builfarm can actually do. Making this possible would require first some refactoring of PostgresNode.pm so as a node is aware of the binary paths it uses to be able to manipulate multiple instances with different install paths at the same time. -- Michael
Attachment
On 1/19/18 4:43 PM, Peter Eisentraut wrote: > On 1/19/18 14:07, David Steele wrote: >> I have yet to add tests for pg_rewindwal and pg_upgrade. pg_rewindwal >> doesn't *have* any tests as far as I can tell and pg_upgrade has tests >> in a shell script -- it's not clear how I would extend it or reuse the >> Perl code for perm testing. >> >> Does anyone have suggestions on tests for pg_resetwal and pg_upgrade? >> Should I start from scratch? > > A test suite for pg_resetwal would be nice. It sure would! I'll put together a basic test suite then add perms testing to it. -- -David david@pgmasters.net
On 1/20/18 5:47 PM, Michael Paquier wrote: > On Fri, Jan 19, 2018 at 06:54:23PM -0300, Alvaro Herrera wrote: >> Peter Eisentraut wrote: >> If the only problem is that buildfarm would run tests twice, then I >> think we should just press forward with this regardless of that: it >> seems a chicken-and-egg problem, because buildfarm cannot be upgraded to >> use some new test method if the method doesn't exist yet. As a >> solution, let's just live with some run duplication for a period until >> the machines are upgraded to a future buildfarm client code. It also appears that the TAP tests don't have the infrastructure to be able to run pg_ugrade properly, per below. > The main complain I received about this move of the pg_upgrade tests to > be a TAP one is that they don't support easily cross-major version > upgrades, removing some abilities to what a developer or the builfarm > can actually do. Unless I read it wrong the buildfarm is not doing cross-version upgrades, but a developer/user can do so manually using the same script? > Making this possible would require first some > refactoring of PostgresNode.pm so as a node is aware of the binary paths > it uses to be able to manipulate multiple instances with different > install paths at the same time. Any idea of what the LOE would be for that? Thanks, -- -David david@pgmasters.net
Attachment
David Steele <david@pgmasters.net> writes:
> Unless I read it wrong the buildfarm is not doing cross-version
> upgrades, but a developer/user can do so manually using the same script?
The buildfarm isn't doing that *by default*, but Andrew has at least
one critter configured to do so (crake I think). It found a bug just
a couple days ago too, so losing that capability isn't going to sell.
regards, tom lane
On 1/23/18 9:26 AM, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> Unless I read it wrong the buildfarm is not doing cross-version >> upgrades, but a developer/user can do so manually using the same script? > > The buildfarm isn't doing that *by default*, but Andrew has at least > one critter configured to do so (crake I think). It found a bug just > a couple days ago too, so losing that capability isn't going to sell. Thanks for the clarification, Tom. What if I update pg_upgrade/test.sh to optionally allow group permissions and we configure an animal to test it if it gets committed? It's not ideal, I know, but it would get the permissions patch over the line and is consistent with how we currently test pg_upgrade. Regards, -- -David david@pgmasters.net
On 1/19/18 16:54, Alvaro Herrera wrote: > If the only problem is that buildfarm would run tests twice, then I > think we should just press forward with this regardless of that: it > seems a chicken-and-egg problem, because buildfarm cannot be upgraded to > use some new test method if the method doesn't exist yet. As a > solution, let's just live with some run duplication for a period until > the machines are upgraded to a future buildfarm client code. Well, people protested against that approach. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/19/18 16:54, Alvaro Herrera wrote:
>> If the only problem is that buildfarm would run tests twice, then I
>> think we should just press forward with this regardless of that: it
>> seems a chicken-and-egg problem, because buildfarm cannot be upgraded to
>> use some new test method if the method doesn't exist yet. As a
>> solution, let's just live with some run duplication for a period until
>> the machines are upgraded to a future buildfarm client code.
> Well, people protested against that approach.
I think I was one of the complainers, but my objection was pretty
straightforward. I want the buildfarm script update to be available
more or less contemporaneously with the change going into git.
Some owners might not care if their machines are doing lots of
extra work, but for those that do, we shouldn't leave them stuck
until a buildfarm update appears in some indefinite future.
In short: get Dunstan into the loop. Don't push this till he's
signed off on new buildfarm code.
regards, tom lane
On 1/23/18 09:33, David Steele wrote: > What if I update pg_upgrade/test.sh to optionally allow group > permissions and we configure an animal to test it if it gets committed? > > It's not ideal, I know, but it would get the permissions patch over the > line and is consistent with how we currently test pg_upgrade. Basically, what you'd need is some way to pass some options to the initdb invocations in the pg_upgrade test script, right? That would seem reasonable, and also useful for testing upgrading with other initdb options. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jan 23, 2018 at 09:18:51AM -0500, David Steele wrote: > On 1/20/18 5:47 PM, Michael Paquier wrote: >> Making this possible would require first some >> refactoring of PostgresNode.pm so as a node is aware of the binary paths >> it uses to be able to manipulate multiple instances with different >> install paths at the same time. > > Any idea of what the LOE would be for that? What does LOE means? -- Michael
Attachment
On 1/23/18 9:22 PM, Michael Paquier wrote: > On Tue, Jan 23, 2018 at 09:18:51AM -0500, David Steele wrote: >> On 1/20/18 5:47 PM, Michael Paquier wrote: >>> Making this possible would require first some >>> refactoring of PostgresNode.pm so as a node is aware of the binary paths >>> it uses to be able to manipulate multiple instances with different >>> install paths at the same time. >> >> Any idea of what the LOE would be for that? > > What does LOE means? Sorry - it means "level of effort". I was trying to get an idea if it is something that could be available in the PG11 development cycle, or if I should be looking at other ways to get the testing for this patch completed. -- -David david@pgmasters.net
Attachment
On Tue, Jan 23, 2018 at 10:48:07PM -0500, David Steele wrote: > Sorry - it means "level of effort". I was trying to get an idea if it > is something that could be available in the PG11 development cycle, or > if I should be looking at other ways to get the testing for this patch > completed. I don't think that it would be more than a couple of hours digging things out, but that may be optimistic. What only needs to be done is extending get_new_node with an optional argument to define the bin directory of a PostgresNode and register the path. If the binary directory is undefined, just rely on PATH and call pg_config --bindir to get the information, then register it. What we need to be careful at is that all the binary calls of PostgresNode.pm need to be changed to use the binary path, which is not really complicated, but it can be easy to miss some. There is one small issue with TestLib::check_pg_config but I think that we can live with the existing version relying on PATH. Once this refactoring is done, you need the patch set I sent previously here with some tiny modifications: https://www.postgresql.org/message-id/CAB7nPqRJXz0sEuUL36eBsF7iZtOQGMJoJPGFWxHLuS6TYPxf5w%40mail.gmail.com Those modifications involve just looking at the environment variables specified in pg_upgrade's TESTING and change the node binaries if those are defined. It is also necessary to tweak probin's paths for the dump consistency checks, which is something that my last patch set was not doing. It is tiring to see this topic come up again and again, so you know what, let's bite the bullet. I commit myself into coding this thing with a patch set for the next commit fest. the only thing I want to be sure about is if folks on this list are fine with the plan of this email. -- Michael
Attachment
On 1/19/18 4:43 PM, Peter Eisentraut wrote: > On 1/19/18 14:07, David Steele wrote: >> I have yet to add tests for pg_rewindwal and pg_upgrade. pg_rewindwal >> doesn't *have* any tests as far as I can tell and pg_upgrade has tests >> in a shell script -- it's not clear how I would extend it or reuse the >> Perl code for perm testing. >> >> Does anyone have suggestions on tests for pg_resetwal and pg_upgrade? >> Should I start from scratch? > > A test suite for pg_resetwal would be nice. Agreed. > TAP tests for pg_upgrade will create problems with the build farm. > There was a recent thread about that. OK, that being the case I have piggy-backed on the current pg_upgrade tests in the same way that --wal-segsize did. There are now three patches: 1) 01-pgresetwal-test Adds a *very* basic test suite for pg_resetwal. I was able to make this utility core dump (floating point errors) pretty easily with empty or malformed pg_control files so I focused on WAL reset functionality plus the basic help/command tests that every utility has. 2) 02-mkdir Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original call used default permissions. 3) 03-group Allow group access on PGDATA. This includes backend changes, utility changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility. Regards, -- -David david@pgmasters.net
Attachment
On Mon, Jan 29, 2018 at 04:29:08PM -0500, David Steele wrote:
> OK, that being the case I have piggy-backed on the current pg_upgrade
> tests in the same way that --wal-segsize did.
>
> There are now three patches:
>
> 1) 01-pgresetwal-test
>
> Adds a *very* basic test suite for pg_resetwal. I was able to make this
> utility core dump (floating point errors) pretty easily with empty or
> malformed pg_control files so I focused on WAL reset functionality plus
> the basic help/command tests that every utility has.
A little is better than nothing, so that's an improvement, thanks!
+# Remove WAL from pg_wal and make sure it gets rebuilt
+$node->stop;
+
+unlink("$pgwal/000000010000000000000001") == 1
+ or die("unable to remove 000000010000000000000001");
+
+is_deeply(
+ [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL');
This is_deeply test serves little purpose. The segment gets removed
directly so I would just remove it.
Another test that could be easily included is with -o, then create a
table and check for its OID value. Also for -x, then just use
txid_current to check the value consistency. For -c, with
track_commit_timestamp set to on, you can set a couple of values and
then check for pg_last_committed_xact() which would be NULL if you use
an XID older than the minimum set for example. All those are simple
enough so they could easily be added in the first version of this test
suite.
You need to update src/bin/pg_resetxlog/.gitignore to include
tmp_check/.
> 2) 02-mkdir
>
> Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original
> call used default permissions.
So the only call not converted to that API is in ipc.c... OK, this one
is pretty simple so nothing really to say about it, the real meat comes
with patch 3. I would rather see with a good eye if this patch
introduces file_perm.h which includes both PG_FILE_MODE_DEFAULT and
PG_DIR_MODE_DEFAULT, and have the frontends use those values. This
would make the switch to 0003 a bit easier if you look at pg_resetwal's
diffs for example.
> 3) 03-group
>
> Allow group access on PGDATA. This includes backend changes, utility
> changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.
+++ b/src/include/common/file_perm.h
+ *
+ * This module is located in common so the backend can use the constants.
+ *
This is the case of more or less all the content in src/common/, except
for the things which are frontend-only, so this comment could just be
removed.
+command_ok(
+ ['chmod', "-R", 'g+rX', "$pgdata"],
+ 'add group perms to PGDATA');
That would blow up on Windows. You should replace that by perl's chmod
and look at its return result for sanity checks, likely with ($cnt != 0).
And let's not use icacls please...
+ if ($params{has_group_access})
+ {
+ push(@{$params{extra}}, '-g');
+ }
No need to introduce a new parameter here, please use directly extra =>
[ '-g' ] in the routines calling PostgresNode::init.
The efforts put in the tests are good.
Patch 0003 needs a very careful lookup... We don't want to introduce any
kind of race conditions here. I am not familiar enough with the
proposed patch but the patch is giving me some chills.
You may want to run pgindent once on your patch set.
--
Michael
Attachment
Hi Michael,
Thanks for having a look at the patches.
On 1/30/18 3:01 AM, Michael Paquier wrote:
> On Mon, Jan 29, 2018 at 04:29:08PM -0500, David Steele wrote:
>>
>> Adds a *very* basic test suite for pg_resetwal. I was able to make this
>> utility core dump (floating point errors) pretty easily with empty or
>> malformed pg_control files so I focused on WAL reset functionality plus
>> the basic help/command tests that every utility has.
>
> A little is better than nothing, so that's an improvement, thanks!
>
> +# Remove WAL from pg_wal and make sure it gets rebuilt
> +$node->stop;
> +
> +unlink("$pgwal/000000010000000000000001") == 1
> + or die("unable to remove 000000010000000000000001");
> +
> +is_deeply(
> + [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL');
> This is_deeply test serves little purpose. The segment gets removed
> directly so I would just remove it.
The purpose of this test to be ensure nothing else is in the directory.
As the tests get more complex I think keeping track of the state will be
important. In other words, this is really an assertion that the current
test state is correct, not that the prior command succeeded.
> Another test that could be easily included is with -o, then create a
> table and check for its OID value. Also for -x, then just use
> txid_current to check the value consistency. For -c, with
> track_commit_timestamp set to on, you can set a couple of values and
> then check for pg_last_committed_xact() which would be NULL if you use
> an XID older than the minimum set for example. All those are simple
> enough so they could easily be added in the first version of this test
> suite.
I would prefer to leave this for another patch.
> You need to update src/bin/pg_resetxlog/.gitignore to include
> tmp_check/.
Added.
>> 2) 02-mkdir
>>
>> Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original
>> call used default permissions.
>
> So the only call not converted to that API is in ipc.c... OK, this one
> is pretty simple so nothing really to say about it, the real meat comes
> with patch 3. I would rather see with a good eye if this patch
> introduces file_perm.h which includes both PG_FILE_MODE_DEFAULT and
> PG_DIR_MODE_DEFAULT, and have the frontends use those values. This
> would make the switch to 0003 a bit easier if you look at pg_resetwal's
> diffs for example.
Agreed. I thought about this originally but could not come up with a
good split. I spent some time on it and came up with something that I
think works (and would make sense to commit separately).
>> 3) 03-group
>>
>> Allow group access on PGDATA. This includes backend changes, utility
>> changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.
>
> +++ b/src/include/common/file_perm.h
> + *
> + * This module is located in common so the backend can use the constants.
> + *
> This is the case of more or less all the content in src/common/, except
> for the things which are frontend-only, so this comment could just be
> removed.
Removed.
> +command_ok(
> + ['chmod', "-R", 'g+rX', "$pgdata"],
> + 'add group perms to PGDATA');
>
> That would blow up on Windows. You should replace that by perl's chmod
> and look at its return result for sanity checks, likely with ($cnt != 0).
> And let's not use icacls please...
Fixed with a new function, add_pg_data_group_perm().
> + if ($params{has_group_access})
> + {
> + push(@{$params{extra}}, '-g');
> + }
> No need to introduce a new parameter here, please use directly extra =>
> [ '-g' ] in the routines calling PostgresNode::init.
Other code needs to know if group access is enabled on the node (see
RewindTest.pm) so it's not just a matter of passing the option.
> The efforts put in the tests are good.
Thanks!
New patches are attached. The end result is almost identical to v6 but
code was moved from 03 to 02 as per Michael's suggestion.
1) 01-pgresetwal-test
Adds a very basic test suite for pg_resetwal.
2) 02-file-perm
Adds a new header (file_perm.h) and makes all front-end utilities use
the new constants for setting umask. Converts mkdir() calls in the
backend to MakeDirectoryDefaultPerm() if the original
call used default permissions. Adds tests to make sure permissions in
PGDATA are set correctly by the front-end tools.
3) 03-group
Allow group access on PGDATA. This includes backend changes, utility
changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.
Thanks!
--
-David
david@pgmasters.net
Attachment
On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
> On 1/30/18 3:01 AM, Michael Paquier wrote:
> The purpose of this test to be ensure nothing else is in the directory.
> As the tests get more complex I think keeping track of the state will be
> important. In other words, this is really an assertion that the current
> test state is correct, not that the prior command succeeded.
Good point. Objection removed.
>> Another test that could be easily included is with -o, then create a
>> table and check for its OID value. Also for -x, then just use
>> txid_current to check the value consistency. For -c, with
>> track_commit_timestamp set to on, you can set a couple of values and
>> then check for pg_last_committed_xact() which would be NULL if you use
>> an XID older than the minimum set for example. All those are simple
>> enough so they could easily be added in the first version of this test
>> suite.
>
> I would prefer to leave this for another patch.
OK, no problems with that either.
>>> 2) 02-mkdir
>>>
>>> Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original
>>> call used default permissions.
>>
>> So the only call not converted to that API is in ipc.c... OK, this one
>> is pretty simple so nothing really to say about it, the real meat comes
>> with patch 3. I would rather see with a good eye if this patch
>> introduces file_perm.h which includes both PG_FILE_MODE_DEFAULT and
>> PG_DIR_MODE_DEFAULT, and have the frontends use those values. This
>> would make the switch to 0003 a bit easier if you look at pg_resetwal's
>> diffs for example.
>
> Agreed. I thought about this originally but could not come up with a
> good split. I spent some time on it and came up with something that I
> think works (and would make sense to commit separately).
OK, thanks.
>> +command_ok(
>> + ['chmod', "-R", 'g+rX', "$pgdata"],
>> + 'add group perms to PGDATA');
>>
>> That would blow up on Windows. You should replace that by perl's chmod
>> and look at its return result for sanity checks, likely with ($cnt != 0).
>> And let's not use icacls please...
>
> Fixed with a new function, add_pg_data_group_perm().
That's basically a recursive chmod, so chmod_recursive is more adapted?
I could imagine that this is useful as well for removing group
permissions, so the new mode could be specified as an argument.
>> + if ($params{has_group_access})
>> + {
>> + push(@{$params{extra}}, '-g');
>> + }
>> No need to introduce a new parameter here, please use directly extra =>
>> [ '-g' ] in the routines calling PostgresNode::init.
>
> Other code needs to know if group access is enabled on the node (see
> RewindTest.pm) so it's not just a matter of passing the option.
I don't quite understand here. I have no objection into extending
setup_cluster() with a group_access argument so as the tests run both
the group access and without it, but I'd rather keep the low-level API
clean of anything fancy if we can use the facility which already
exists.
> New patches are attached. The end result is almost identical to v6 but
> code was moved from 03 to 02 as per Michael's suggestion.
Thanks for the updated versions.
> 1) 01-pgresetwal-test
>
> Adds a very basic test suite for pg_resetwal.
Okay. This one looks fine to me. It could be passed down to a
committer.
> 2) 02-file-perm
>
> Adds a new header (file_perm.h) and makes all front-end utilities use
> the new constants for setting umask. Converts mkdir() calls in the
> backend to MakeDirectoryDefaultPerm() if the original
> call used default permissions. Adds tests to make sure permissions in
> PGDATA are set correctly by the front-end tools.
I had a more simple (complicated?) thing in mind for the split, which
would actually cause this patch to be split into two more:
1) Introduce file_perm.h and replace all the existing values for modes
and masks to what the header introduces. This allows to check if the
new header is not forgotten anywhere, especially for the frontends.
2) Introduce MakeDirectoryDefaultPerm, which switches the backend calls
of mkdir() to the new API.
3) Add the grouping facilities, which updates the default masks and
modes of file_perm.h.
Grouping 1) and 2) together as you do makes sense as well I guess, as in
my proposal the mkdir calls in the backend would be touched twice, still
things get more incremental.
+/*
+ * Default mode for created files.
+ */
+#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP)
+
+/*
+ * Default mode for directories.
+ */
+#define PG_DIR_MODE_DEFAULT (S_IRWXU | S_IRGRP | S_IXGRP)
For the first cut, this should not use S_IRGRP in the first declaration,
and (S_IXGRP | S_IXGRP) in the second declaration.
- if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ if (script == NULL && (script = fopen(output_path, "w")) == NULL)
Why are those calls in pg_upgrade changed?
TestLib.pm does not need the group-related extensions, right?
check_pg_data_perm() looks useful even without $allow_group even if the
grouping facility is reverted at the end. S_ISLNK should be considered
as well for tablespaces or symlinks of pg_wal? We have such cases in
the regression tests.
- if (chmod(pg_data, S_IRWXU) != 0)
+ if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
[...]
- if (chmod(xlog_dir, S_IRWXU) != 0)
+ if (chmod(xlog_dir, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
Those seems incorrect, as they consider the default value only.
snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
writefile(path, conflines);
- if (chmod(path, S_IRUSR | S_IWUSR) != 0)
- {
- fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
- progname, path, strerror(errno));
- exit_nicely();
- }
Hm. Why are those removed?
setup_signals();
- umask(S_IRWXG | S_IRWXO);
-
create_data_directory();
This should not be moved around.
> 3) 03-group
>
> Allow group access on PGDATA. This includes backend changes, utility
> changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.
I have not looked much in depth into this one, but I spotted two issues
on a quick read:
+ * Errors are not handled here and should be checked by the frontend
+ * application.
+ */
+#ifdef FRONTEND
What you need to do for frontend-only files in src/common/ is to define
that at the top and not bother about FRONTEND cflags at all:
#ifndef FRONTEND
#error "This file is not expected to be compiled for backend code"
#endif
src/tools/msvc/Mkvcbuild.pm also needs to be updated with this new file,
or you will break MSVC build.
--
Michael
Attachment
Hi, On 2018-02-27 15:52:32 -0500, David Steele wrote: > Thanks for having a look at the patches. I'd personally appreciate if you'd add commit messages to the individual patches in a series. A brief explanation why something is done is good enough. E.g. here it's far from obvious why something is done with pg_resetwal. I'd suggest just using git format-patch -v to format series. Independent of that, I'm not entirely clear on what the status of this patch is, without rereading the thread. Could you summarize? - Andres
Hi Andres, On 3/1/18 9:04 PM, Andres Freund wrote: > On 2018-02-27 15:52:32 -0500, David Steele wrote: >> Thanks for having a look at the patches. > > I'd personally appreciate if you'd add commit messages to the individual > patches in a series. A brief explanation why something is done is good > enough. > > I'd suggest just using git format-patch -v to format series. Sure, I've been meaning to switch over to this format and just haven't gotten around to it yet. I do recognize the value, however, and will do it going forward. > Independent of that, I'm not entirely clear on what the status of this > patch is, without rereading the thread. Could you summarize? Good question. This patch has been around for a year and has gone though a number of iterations. In summary, we started with a more rigid design that required a GUC to enable group privs (actually, at first it was any mode you wanted). Through feedback from Tom and others we realized that the front-end tools could not read the GUCs and it would probably be best based on the perms of PGDATA, as long as they were 700 or 750. We decided the patch was too big, so broke it up a bit and got some of the infrastructure committed in 2017-09 [1]. In 2018-01 we brought in a unified patch set that built on the ideas from 2017-03. During that CF we taught all the front-end tools to check PGDATA for permissions and wrote a lot of tests. The current incarnation is a refinement with input from Michael and a different split in the patches. > E.g. here it's far from obvious why something is done with > pg_resetwal. [reordered by me for clarity] Any front-end tool that writes into PGDATA is affected by this patch because mode must be set correctly. pg_resetwal had no tests, so I wrote a basic test suite to base the group permissions tests on. I included the basic test suite as a separate patch because I felt it should be committed even if the main feature wasn't. It's far from a comprehensive test suite, but certainly better than what we have currently. Does that clear things up? -- -David david@pgmasters.net [1] https://commitfest.postgresql.org/14/1262/
On Thu, Mar 01, 2018 at 09:32:58PM -0500, David Steele wrote: > On 3/1/18 9:04 PM, Andres Freund wrote: >> I'd suggest just using git format-patch -v to format series. > > Sure, I've been meaning to switch over to this format and just haven't > gotten around to it yet. I do recognize the value, however, and will do it > going forward. Simple patches can bypass on that, but once you have at least three patches this helps a lot with reviews. Explaining as well in the email sending the patches what each patch actually does, even roughly helps in focusing on one element or the other. >> E.g. here it's far from obvious why something is done with >> pg_resetwal. > [reordered by me for clarity] > > Any front-end tool that writes into PGDATA is affected by this patch because > mode must be set correctly. > > pg_resetwal had no tests, so I wrote a basic test suite to base the group > permissions tests on. I included the basic test suite as a separate patch > because I felt it should be committed even if the main feature wasn't. It's > far from a comprehensive test suite, but certainly better than what we have > currently. Based on my recent lookup at code level for this feature, the patch for pg_resetwal (which could have been discussed on its own thread as well), would be fine for commit. The thing could be extended a bit more but there is nothing opposing even a basic test suite to be in. Then you have a set of refactoring patches, which still need some work. And finally there is a rather invasive patch on top of the whole thing. The refactoring work shows much more value only after the main feature is in, still I think that unifying the default permissions allowed for files and directories, as well as mkdir() calls has some value in itself to think it as an mergeable, independent, change. I think that it would be hard to get the whole patch set into the tree by the end of the CF though, but cutting the refactoring pieces would be doable. At least it would provide some base for integration in v12. And the refactoring patch has some pieces that would be helpful for TAP tests as well. -- Michael
Attachment
On 2/28/18 2:28 AM, Michael Paquier wrote:
> On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
>> On 1/30/18 3:01 AM, Michael Paquier wrote:
>
>>> +command_ok(
>>> + ['chmod', "-R", 'g+rX', "$pgdata"],
>>> + 'add group perms to PGDATA');
>>>
>>> That would blow up on Windows. You should replace that by perl's chmod
>>> and look at its return result for sanity checks, likely with ($cnt != 0).
>>> And let's not use icacls please...
>>
>> Fixed with a new function, add_pg_data_group_perm().
>
> That's basically a recursive chmod, so chmod_recursive is more adapted?
> I could imagine that this is useful as well for removing group
> permissions, so the new mode could be specified as an argument.
The required package (File::chmod::Recursive) for chmod_recursive is not
in use anywhere else and was not installed when I installed build
dependencies.
I'm not sure what the protocol for introducing a new Perl module is? I
couldn't find packages for the major OSes. Are we OK with using CPAN?
>>> + if ($params{has_group_access})
>>> + {
>>> + push(@{$params{extra}}, '-g');
>>> + }
>>> No need to introduce a new parameter here, please use directly extra =>
>>> [ '-g' ] in the routines calling PostgresNode::init.
>>
>> Other code needs to know if group access is enabled on the node (see
>> RewindTest.pm) so it's not just a matter of passing the option.
>
> I don't quite understand here. I have no objection into extending
> setup_cluster() with a group_access argument so as the tests run both
> the group access and without it, but I'd rather keep the low-level API
> clean of anything fancy if we can use the facility which already
> exists.
I'm not sure what you mean by, "facility that already exists". I think
I implemented this the same as the allows_streaming and has_archiving
flags. The only difference is that this option must be set at initdb
time rather than in postgresql.conf.
Could you be more specific?
>> New patches are attached. The end result is almost identical to v6 but
>> code was moved from 03 to 02 as per Michael's suggestion.
>
> Thanks for the updated versions.
>
>> 1) 01-pgresetwal-test
>>
>> Adds a very basic test suite for pg_resetwal.
>
> Okay. This one looks fine to me. It could be passed down to a
> committer.
Agreed.
>> 2) 02-file-perm
>>
>> Adds a new header (file_perm.h) and makes all front-end utilities use
>> the new constants for setting umask. Converts mkdir() calls in the
>> backend to MakeDirectoryDefaultPerm() if the original
>> call used default permissions. Adds tests to make sure permissions in
>> PGDATA are set correctly by the front-end tools.
>
> I had a more simple (complicated?) thing in mind for the split, which
> would actually cause this patch to be split into two more:
> 1) Introduce file_perm.h and replace all the existing values for modes
> and masks to what the header introduces. This allows to check if the
> new header is not forgotten anywhere, especially for the frontends.
> 2) Introduce MakeDirectoryDefaultPerm, which switches the backend calls
> of mkdir() to the new API.
> 3) Add the grouping facilities, which updates the default masks and
> modes of file_perm.h.
>
> Grouping 1) and 2) together as you do makes sense as well I guess, as in
> my proposal the mkdir calls in the backend would be touched twice, still
> things get more incremental.
I think I prefer grouping 1 and 2. It produces less churn, as you say,
and I don't think MakeDirectoryDefaultPerm really needs its own patch.
Based on comments so far, nobody has an objection to it.
In theory, the first two patches could be applied just for refactoring
without adding group permissions at all. There are a lot of new tests
to make sure permissions are set correctly so it seems like a win right off.
>
> +/*
> + * Default mode for created files.
> + */
> +#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP)
> +
> +/*
> + * Default mode for directories.
> + */
> +#define PG_DIR_MODE_DEFAULT (S_IRWXU | S_IRGRP | S_IXGRP)
> For the first cut, this should not use S_IRGRP in the first declaration,
> and (S_IXGRP | S_IXGRP) in the second declaration.
Done.
>
> - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
> + if (script == NULL && (script = fopen(output_path, "w")) == NULL)
> Why are those calls in pg_upgrade changed?
Done.
Those should have been removed already. Originally I had removed
fopen_priv() because it didn't serve a purpose anymore. In the
meantime, somebody else made it a macro to fopen(). I decided my patch
would be less noisy if I reverted to fopen_priv() but I missed a few places.
> TestLib.pm does not need the group-related extensions, right?
Done.
> check_pg_data_perm() looks useful even without $allow_group even if the
> grouping facility is reverted at the end. S_ISLNK should be considered
> as well for tablespaces or symlinks of pg_wal? We have such cases in
> the regression tests.
Hmmm, the link is just pointing to a directory whose permissions have
been changed. Why do we need to worry about the link?
> - if (chmod(path, S_IRUSR | S_IWUSR) != 0)
> - {
> - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
> - progname, path, strerror(errno));
> - exit_nicely();
> - }
> Hm. Why are those removed?
When I started working on this, pg_upgrade did not set umask and instead
did chmod for each dir created. umask() has since been added (even
before my patch) and so these are now noops. Seemed easier to remove
them than to change them all.
> setup_signals();
>
> - umask(S_IRWXG | S_IRWXO);
> -
> create_data_directory();
> This should not be moved around.
Hmmm - I moved it much earlier in the process which seems like a good
thing. Consider if there was a option to fixup permissions, like there
is to fsync. Isn't it best to set the mode as soon as possible to
prevent code from being inserted before it?
> What you need to do for frontend-only files in src/common/ is to define
> that at the top and not bother about FRONTEND cflags at all:
> #ifndef FRONTEND
> #error "This file is not expected to be compiled for backend code"
> #endif
Fixed.
> src/tools/msvc/Mkvcbuild.pm also needs to be updated with this new file,
> or you will break MSVC build.
Fixed.
New patches attached.
--
-David
david@pgmasters.net
Attachment
On 3/1/18 11:18 PM, Michael Paquier wrote: > > Based on my recent lookup at code level for this feature, the patch for > pg_resetwal (which could have been discussed on its own thread as well), > would be fine for commit. The thing could be extended a bit more but > there is nothing opposing even a basic test suite to be in. There are no core changes, so it doesn't seem like the tests can hurt anything. > Then you > have a set of refactoring patches, which still need some work. New patches posted today, hopefully those address most of your concerns. > And > finally there is a rather invasive patch on top of the whole thing. I'm not sure if I would call it invasive since it's an optional feature that is off by default. Honestly, I think the refactor in 02 is more likely to cause problems even if the goal there is *not* to change the behavior. > The > refactoring work shows much more value only after the main feature is > in, still I think that unifying the default permissions allowed for > files and directories, as well as mkdir() calls has some value in > itself to think it as an mergeable, independent, change. I agree. > I think that > it would be hard to get the whole patch set into the tree by the end of > the CF though I hope it does make it, it's a pretty big win for security. > but cutting the refactoring pieces would be doable. At > least it would provide some base for integration in v12. And the > refactoring patch has some pieces that would be helpful for TAP tests as > well. I've gone pretty big on tests in this patch because I recognize it is a pretty fundamental behavior change. Thanks, -- -David david@pgmasters.net
Attachment
David Steele <david@pgmasters.net> writes:
> On 2/28/18 2:28 AM, Michael Paquier wrote:
>> That's basically a recursive chmod, so chmod_recursive is more adapted?
>> I could imagine that this is useful as well for removing group
>> permissions, so the new mode could be specified as an argument.
> The required package (File::chmod::Recursive) for chmod_recursive is not
> in use anywhere else and was not installed when I installed build
> dependencies.
> I'm not sure what the protocol for introducing a new Perl module is? I
> couldn't find packages for the major OSes. Are we OK with using CPAN?
I don't think that's cool. Anything that's not part of a standard Perl
installation is a bit of a lift already, and if it's not packaged by
major distros then it's really a problem for many people. (Yeah, they
may know what CPAN is, but they might have local policy issues about
installing directly from there.)
regards, tom lane
Hi Tom, On 3/5/18 5:11 PM, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 2/28/18 2:28 AM, Michael Paquier wrote: >>> That's basically a recursive chmod, so chmod_recursive is more adapted? >>> I could imagine that this is useful as well for removing group >>> permissions, so the new mode could be specified as an argument. > >> The required package (File::chmod::Recursive) for chmod_recursive is not >> in use anywhere else and was not installed when I installed build >> dependencies. > >> I'm not sure what the protocol for introducing a new Perl module is? I >> couldn't find packages for the major OSes. Are we OK with using CPAN? > > I don't think that's cool. Anything that's not part of a standard Perl > installation is a bit of a lift already, and if it's not packaged by > major distros then it's really a problem for many people. (Yeah, they > may know what CPAN is, but they might have local policy issues about > installing directly from there.) This is my view as well. I don't think CPAN should ever be on a production box, mostly because of all the other tools that need to be installed to make it work. It's a little different here, because these packages are only used for testing/development. Of course, maybe I have this wrong and Michael will point out my error. If not, I'm happy to rework the function (about 15 lines) to be more generic. -- -David david@pgmasters.net
Tom, all, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > David Steele <david@pgmasters.net> writes: > > On 2/28/18 2:28 AM, Michael Paquier wrote: > >> That's basically a recursive chmod, so chmod_recursive is more adapted? > >> I could imagine that this is useful as well for removing group > >> permissions, so the new mode could be specified as an argument. > > > The required package (File::chmod::Recursive) for chmod_recursive is not > > in use anywhere else and was not installed when I installed build > > dependencies. > > > I'm not sure what the protocol for introducing a new Perl module is? I > > couldn't find packages for the major OSes. Are we OK with using CPAN? > > I don't think that's cool. Anything that's not part of a standard Perl > installation is a bit of a lift already, and if it's not packaged by > major distros then it's really a problem for many people. (Yeah, they > may know what CPAN is, but they might have local policy issues about > installing directly from there.) Agreed. Thanks! Stephen
Attachment
David Steele <david@pgmasters.net> writes:
> On 3/5/18 5:11 PM, Tom Lane wrote:
>> David Steele <david@pgmasters.net> writes:
>>> I'm not sure what the protocol for introducing a new Perl module is? I
>>> couldn't find packages for the major OSes. Are we OK with using CPAN?
>> I don't think that's cool. Anything that's not part of a standard Perl
>> installation is a bit of a lift already, and if it's not packaged by
>> major distros then it's really a problem for many people. (Yeah, they
>> may know what CPAN is, but they might have local policy issues about
>> installing directly from there.)
> It's a little different here, because these packages are only used for
> testing/development.
True, but if we want this test to be part of either check-world or the
buildfarm run, that's still a lot of people we're asking to install
the extra module. If we're talking about saving just a few dozen
lines of code, it ain't worth it.
regards, tom lane
On 3/5/18 5:51 PM, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 3/5/18 5:11 PM, Tom Lane wrote: >>> David Steele <david@pgmasters.net> writes: >>>> I'm not sure what the protocol for introducing a new Perl module is? I >>>> couldn't find packages for the major OSes. Are we OK with using CPAN? > >>> I don't think that's cool. Anything that's not part of a standard Perl >>> installation is a bit of a lift already, and if it's not packaged by >>> major distros then it's really a problem for many people. (Yeah, they >>> may know what CPAN is, but they might have local policy issues about >>> installing directly from there.) > >> It's a little different here, because these packages are only used for >> testing/development. > > True, but if we want this test to be part of either check-world or the > buildfarm run, that's still a lot of people we're asking to install > the extra module. If we're talking about saving just a few dozen > lines of code, it ain't worth it. +1. -- -David david@pgmasters.net
On Mon, Mar 05, 2018 at 05:11:29PM -0500, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 2/28/18 2:28 AM, Michael Paquier wrote: >>> That's basically a recursive chmod, so chmod_recursive is more adapted? >>> I could imagine that this is useful as well for removing group >>> permissions, so the new mode could be specified as an argument. > >> The required package (File::chmod::Recursive) for chmod_recursive is not >> in use anywhere else and was not installed when I installed build >> dependencies. Woah. I didn't even know that chmod_recursive existed and was part of a module. What I commented about here was to rename to a more generic name the routine you are implementing so as other tests could use it. >> I'm not sure what the protocol for introducing a new Perl module is? I >> couldn't find packages for the major OSes. Are we OK with using CPAN? > > I don't think that's cool. Anything that's not part of a standard Perl > installation is a bit of a lift already, and if it's not packaged by > major distros then it's really a problem for many people. (Yeah, they > may know what CPAN is, but they might have local policy issues about > installing directly from there.) Yes, that's not cool. I am not pushing in this direction. Sorry for creating confusion with fuzzy wording. -- Michael
Attachment
On 3/5/18 8:03 PM, Michael Paquier wrote: > On Mon, Mar 05, 2018 at 05:11:29PM -0500, Tom Lane wrote: >> David Steele <david@pgmasters.net> writes: >>> On 2/28/18 2:28 AM, Michael Paquier wrote: >>>> That's basically a recursive chmod, so chmod_recursive is more adapted? >>>> I could imagine that this is useful as well for removing group >>>> permissions, so the new mode could be specified as an argument. >> >>> The required package (File::chmod::Recursive) for chmod_recursive is not >>> in use anywhere else and was not installed when I installed build >>> dependencies. > > Woah. I didn't even know that chmod_recursive existed and was part of a > module. What I commented about here was to rename to a more generic > name the routine you are implementing so as other tests could use it. OK, that is pretty funny. I thought you were directing me to a Perl function I hadn't heard of (but did exist). >>> I'm not sure what the protocol for introducing a new Perl module is? I >>> couldn't find packages for the major OSes. Are we OK with using CPAN? >> >> I don't think that's cool. Anything that's not part of a standard Perl >> installation is a bit of a lift already, and if it's not packaged by >> major distros then it's really a problem for many people. (Yeah, they >> may know what CPAN is, but they might have local policy issues about >> installing directly from there.) > > Yes, that's not cool. I am not pushing in this direction. Sorry for > creating confusion with fuzzy wording. No worries, I'll just make it more generic as suggested. -- -David david@pgmasters.net
Attachment
On Mon, Mar 05, 2018 at 03:07:20PM -0500, David Steele wrote:
> On 2/28/18 2:28 AM, Michael Paquier wrote:
>> On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
>> I don't quite understand here. I have no objection into extending
>> setup_cluster() with a group_access argument so as the tests run both
>> the group access and without it, but I'd rather keep the low-level API
>> clean of anything fancy if we can use the facility which already
>> exists.
>
> I'm not sure what you mean by, "facility that already exists". I think
> I implemented this the same as the allows_streaming and has_archiving
> flags. The only difference is that this option must be set at initdb
> time rather than in postgresql.conf.
>
> Could you be more specific?
Let's remove has_group_access from PostgresNode::init and instead use
existing parameter called "extra". In your patch, this setup:
has_group_access => 1
is equivalent to that:
extra => [ -g ]
You can also guess the value of has_group_access by parsing the
arguments from the array "extra".
> I think I prefer grouping 1 and 2. It produces less churn, as you say,
> and I don't think MakeDirectoryDefaultPerm really needs its own patch.
> Based on comments so far, nobody has an objection to it.
>
> In theory, the first two patches could be applied just for refactoring
> without adding group permissions at all. There are a lot of new tests
> to make sure permissions are set correctly so it seems like a win
> right off.
Okay, that's fine for me.
>> check_pg_data_perm() looks useful even without $allow_group even if the
>> grouping facility is reverted at the end. S_ISLNK should be considered
>> as well for tablespaces or symlinks of pg_wal? We have such cases in
>> the regression tests.
>
> Hmmm, the link is just pointing to a directory whose permissions have
> been changed. Why do we need to worry about the link?
Oh, perhaps I misread your code here, but this ignores symlinks, for
example take an instance where pg_wal is symlinked, we'd likely want to
make sure that at least archive_status is using a correct set of
permissions, no? There is a "follow" option in File::Find which could
help.
>> - if (chmod(path, S_IRUSR | S_IWUSR) != 0)
>> - {
>> - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
>> - progname, path, strerror(errno));
>> - exit_nicely();
>> - }
>> Hm. Why are those removed?
>
> When I started working on this, pg_upgrade did not set umask and instead
> did chmod for each dir created. umask() has since been added (even
> before my patch) and so these are now noops. Seemed easier to remove
> them than to change them all.
>
>> setup_signals();
>>
>> - umask(S_IRWXG | S_IRWXO);
>> -
>> create_data_directory();
>> This should not be moved around.
>
> Hmmm - I moved it much earlier in the process which seems like a good
> thing. Consider if there was a option to fixup permissions, like there
> is to fsync. Isn't it best to set the mode as soon as possible to
> prevent code from being inserted before it?
Those two are separate issues. Could you begin a new thread on the
matter? This will attract more attention.
The indentation in RewindTest.pm is a bit wrong.
- if (chmod(location, S_IRWXU) != 0)
+ current_umask = umask(0);
+ umask(current_umask);
[...]
- if (chmod(pg_data, S_IRWXU) != 0)
+ if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
Such modifications should be part of patch 3, not 2, where the group
features really apply.
my $test_mode = shift;
+ umask(0077);
+
RewindTest::setup_cluster($test_mode);
What's that for? A comment would be welcome.
+# make sure all directories and files have group permissions
+if [ $(find ${PGDATA} -type f ! -perm 600 | wc -l) -ne 0 ]; then
+ echo "files in PGDATA with permission != 600";
+ exit 1;
+fi
Perhaps on hold if we are able to move pg_upgrade tests to a TAP
suite... We'll see what happens.
Perhaps the tests should be cut into a separate patch? Those are not
directly related to the refactoring done in patch 2.
Patch 2 begins to look fine for me. I still need to look at patch 3 in
more details.
--
Michael
Attachment
On 3/5/18 10:46 PM, Michael Paquier wrote: > On Mon, Mar 05, 2018 at 03:07:20PM -0500, David Steele wrote: >> On 2/28/18 2:28 AM, Michael Paquier wrote: >>> On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote: >>> I don't quite understand here. I have no objection into extending >>> setup_cluster() with a group_access argument so as the tests run both >>> the group access and without it, but I'd rather keep the low-level API >>> clean of anything fancy if we can use the facility which already >>> exists. >> >> I'm not sure what you mean by, "facility that already exists". I think >> I implemented this the same as the allows_streaming and has_archiving >> flags. The only difference is that this option must be set at initdb >> time rather than in postgresql.conf. >> >> Could you be more specific? > > Let's remove has_group_access from PostgresNode::init and instead use > existing parameter called "extra". In your patch, this setup: > has_group_access => 1 > is equivalent to that: > extra => [ -g ] > You can also guess the value of has_group_access by parsing the > arguments from the array "extra". OK. extra is used to set -g and the group_access() function checks pgdata directly since this can change after the cluster is created. >>> check_pg_data_perm() looks useful even without $allow_group even if the >>> grouping facility is reverted at the end. S_ISLNK should be considered >>> as well for tablespaces or symlinks of pg_wal? We have such cases in >>> the regression tests. >> >> Hmmm, the link is just pointing to a directory whose permissions have >> been changed. Why do we need to worry about the link? > > Oh, perhaps I misread your code here, but this ignores symlinks, for > example take an instance where pg_wal is symlinked, we'd likely want to > make sure that at least archive_status is using a correct set of > permissions, no? There is a "follow" option in File::Find which could > help. Ah, I see what you mean now. Fixed using follow_fast. This variant claims to be faster and it doesn't matter if files are occasionally checked twice. >> >>> setup_signals(); >>> >>> - umask(S_IRWXG | S_IRWXO); >>> - >>> create_data_directory(); >>> This should not be moved around. >> >> Hmmm - I moved it much earlier in the process which seems like a good >> thing. Consider if there was a option to fixup permissions, like there >> is to fsync. Isn't it best to set the mode as soon as possible to >> prevent code from being inserted before it? > > Those two are separate issues. Could you begin a new thread on the > matter? This will attract more attention. OK, I'll move it back for now, and make a note to raise the position as a separate issue. I'd rather not do it in this fest, though. > The indentation in RewindTest.pm is a bit wrong. Fixed. > - if (chmod(location, S_IRWXU) != 0) > + current_umask = umask(0); > + umask(current_umask); > [...] > - if (chmod(pg_data, S_IRWXU) != 0) > + if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0) > Such modifications should be part of patch 3, not 2, where the group > features really apply. The goal of patch 2 is to refactor the way permissions are set by replacing locally hard-coded values with centralized values, so I think this makes sense here. > my $test_mode = shift; > > + umask(0077); Added. (Ensure that all files are created with default data dir permissions). > RewindTest::setup_cluster($test_mode); > What's that for? A comment would be welcome. Added. (Used to differentiate clusters). > Perhaps the tests should be cut into a separate patch? I prefer tests to be in the same patch as the code they test. > Those are not > directly related to the refactoring done in patch 2. The point of the tests is to be sure there are no regressions in the refactor so they seem directly related to me. Also, the tests themselves were not to good about keeping permissions consistent. > Patch 2 begins to look fine for me. I also made chmod_recursive() generic. New patches are attached. Thanks! -- -David david@pgmasters.net
Attachment
On Tue, Mar 06, 2018 at 01:32:49PM -0500, David Steele wrote:
> On 3/5/18 10:46 PM, Michael Paquier wrote:
> Ah, I see what you mean now. Fixed using follow_fast. This variant
> claims to be faster and it doesn't matter if files are occasionally
> checked twice.
Fine for me. I can see this option present in perl 5.8.8 as well, so we
should be good.
>> Those two are separate issues. Could you begin a new thread on the
>> matter? This will attract more attention.
>
> OK, I'll move it back for now, and make a note to raise the position as
> a separate issue. I'd rather not do it in this fest, though.
Seems like you forgot to re-add the chmod calls in initdb.c.
>> - if (chmod(location, S_IRWXU) != 0)
>> + current_umask = umask(0);
>> + umask(current_umask);
>> [...]
>> - if (chmod(pg_data, S_IRWXU) != 0)
>> + if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
>> Such modifications should be part of patch 3, not 2, where the group
>> features really apply.
>
> The goal of patch 2 is to refactor the way permissions are set by
> replacing locally hard-coded values with centralized values, so I think
> this makes sense here.
Hm. I would have left that out in this first version, now I am fine to
defer that to a committer's opinion as well.
>> my $test_mode = shift;
>>
>> + umask(0077);
>
> Added. (Ensure that all files are created with default data dir
> permissions).
+ # Ensure that all files are created with default data dir permissions
+ umask(0077);
I have stared at this comment two minutes to finally understand that
this refers to the extra files which are part of this test. It may be
better to be a bit more precise here. I thought first that this
referred as well to setup_cluster calls...
>> Patch 2 begins to look fine for me.
>
> I also made chmod_recursive() generic.
Likely this name is not worth worrying with conflicts in CPAN stuff ;)
+# Ensure all permissions in the pg_data directory are
correct. Permissions
+# should be dir = 0700, file = 0600.
+sub check_pg_data_perm
+{
check_permission_recursive() would be a more adapted name. Stuff in
TestLib.pm is not necessarily linked to data folders.
sub clean_rewind_test
{
+ ok (check_pg_data_perm($node_master->data_dir(), 0));
+
$node_master->teardown_node if defined $node_master;
I have also a pending patch for pg_rewind which adds read-only files in
the data folders of a new test, so this would cause this part to blow
up. Testing that for all the test sets does not bring additional value
as well, and doing it at cleanup phase is also confusing. So could you
move that check into only one test's script? You could remove the umask
call in 003_extrafiles.pl as well this way, and reduce the patch diffs.
+ if ($file_mode != 0600)
+ {
+ print(*STDERR, "$File::Find::name mode must be 0600\n");
+
+ $result = 0;
+ return
+ }
0600 should be replaced by $expected_file_perm, and isn't a ';' missing
for each return "clause" (how does this even work..)?
Patch 2 is getting close for a committer lookup I think, still need to
look at patch 3 in details.. And from patch 3:
# Expected permission
- my $expected_file_perm = 0600;
- my $expected_dir_perm = 0700;
+ my $expected_file_perm = $allow_group ? 0640 : 0600;
+ my $expected_dir_perm = $allow_group ? 0750 : 0700;
Or $expected_file_perm and $expected_dir_perm could just be used as
arguments.
--
Michael
Attachment
On 3/6/18 10:04 PM, Michael Paquier wrote:
> On Tue, Mar 06, 2018 at 01:32:49PM -0500, David Steele wrote:
>> On 3/5/18 10:46 PM, Michael Paquier wrote:
>
>>> Those two are separate issues. Could you begin a new thread on the
>>> matter? This will attract more attention.
>>
>> OK, I'll move it back for now, and make a note to raise the position as
>> a separate issue. I'd rather not do it in this fest, though.
>
> Seems like you forgot to re-add the chmod calls in initdb.c.
Hmmm, I thought we were talking about moving the position of umask().
I don't think the chmod() calls are needed because umask() is being set.
The tests show that the config files have the proper permissions after
inidb, so this just looks like redundant code to me.
But, I'm not going to argue if you think they should go back. It would
make the patch less noisy.
>>> - if (chmod(location, S_IRWXU) != 0)
>>> + current_umask = umask(0);
>>> + umask(current_umask);
>>> [...]
>>> - if (chmod(pg_data, S_IRWXU) != 0)
>>> + if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
>>> Such modifications should be part of patch 3, not 2, where the group
>>> features really apply.
>>
>> The goal of patch 2 is to refactor the way permissions are set by
>> replacing locally hard-coded values with centralized values, so I think
>> this makes sense here.
>
> Hm. I would have left that out in this first version, now I am fine to
> defer that to a committer's opinion as well.
OK - I really do think it belongs here. A committer may not agree, of
course.
>>> my $test_mode = shift;
>>>
>>> + umask(0077);
>>
>> Added. (Ensure that all files are created with default data dir
>> permissions).
>
> + # Ensure that all files are created with default data dir permissions
> + umask(0077);
> I have stared at this comment two minutes to finally understand that
> this refers to the extra files which are part of this test. It may be
> better to be a bit more precise here. I thought first that this
> referred as well to setup_cluster calls...
Updated to, "Ensure that all files created in this module for testing
are set with default data dir permissions."
> +# Ensure all permissions in the pg_data directory are
> correct. Permissions
> +# should be dir = 0700, file = 0600.
> +sub check_pg_data_perm
> +{
> check_permission_recursive() would be a more adapted name. Stuff in
> TestLib.pm is not necessarily linked to data folders.
When we add group permissions we need to have special logic around
postmaster.pid. This should be 0600 even if the rest of the files are 0640.
I can certainly remove that special logic in 02 and make this function
more generic, but then I think we would still have to add
check_pg_data_perm() in patch 03.
Another way to do this would be to make the function generic and
stipulate that the postmaster must be shut down before running the
function. We could check postmaster.pid permissions as a separate test.
What do you think?
> sub clean_rewind_test
> {
> + ok (check_pg_data_perm($node_master->data_dir(), 0));
> +
> $node_master->teardown_node if defined $node_master;
> I have also a pending patch for pg_rewind which adds read-only files in
> the data folders of a new test, so this would cause this part to blow
> up. Testing that for all the test sets does not bring additional value
> as well, and doing it at cleanup phase is also confusing. So could you
> move that check into only one test's script? You could remove the umask
> call in 003_extrafiles.pl as well this way, and reduce the patch diffs.
I think I would rather have a way to skip the permission test rather
than disable it for most of the tests. pg_rewind does more writes into
PGDATA that anything other than a backend. Good coverage seems like a plus.
> + if ($file_mode != 0600)
> + {
> + print(*STDERR, "$File::Find::name mode must be 0600\n");
> +
> + $result = 0;
> + return
> + }
> 0600 should be replaced by $expected_file_perm, and isn't a ';' missing
> for each return "clause" (how does this even work..)?
Well, 0600 is a special case -- see above. As for the missing
semi-colon, well that's Perl for you. Fixed.
> Patch 2 is getting close for a committer lookup I think, still need to
> look at patch 3 in details.. And from patch 3:
> # Expected permission
> - my $expected_file_perm = 0600;
> - my $expected_dir_perm = 0700;
> + my $expected_file_perm = $allow_group ? 0640 : 0600;
> + my $expected_dir_perm = $allow_group ? 0750 : 0700;
> Or $expected_file_perm and $expected_dir_perm could just be used as
> arguments.
This gets back to the check_pg_data_perm() discussion above.
I'll hold off on another set of patches until I hear back from you.
There were only trivial changes as noted above.
Thanks,
--
-David
david@pgmasters.net
Attachment
On Wed, Mar 07, 2018 at 10:56:32AM -0500, David Steele wrote:
> On 3/6/18 10:04 PM, Michael Paquier wrote:
>> Seems like you forgot to re-add the chmod calls in initdb.c.
>
> Hmmm, I thought we were talking about moving the position of umask().
>
> I don't think the chmod() calls are needed because umask() is being set.
> The tests show that the config files have the proper permissions after
> inidb, so this just looks like redundant code to me.
Let's discuss that on a separate thread then, there could be something
we are missing, but I agree that those should not be needed. Looking at
the code history, those calls have been around since the beginning of
PG-times.
> Another way to do this would be to make the function generic and
> stipulate that the postmaster must be shut down before running the
> function. We could check postmaster.pid permissions as a separate
> test.
Yeah, that looks like a sensitive approach as this could be run
post-initdb or after taking a backup. This way we would avoid other
similar behaviors in the future... Still postmaster.pid is an
exception.
>> sub clean_rewind_test
>> {
>> + ok (check_pg_data_perm($node_master->data_dir(), 0));
>> +
>> $node_master->teardown_node if defined $node_master;
>> I have also a pending patch for pg_rewind which adds read-only files in
>> the data folders of a new test, so this would cause this part to blow
>> up. Testing that for all the test sets does not bring additional value
>> as well, and doing it at cleanup phase is also confusing. So could you
>> move that check into only one test's script? You could remove the umask
>> call in 003_extrafiles.pl as well this way, and reduce the patch diffs.
>
> I think I would rather have a way to skip the permission test rather
> than disable it for most of the tests. pg_rewind does more writes into
> PGDATA that anything other than a backend. Good coverage seems like a
> plus.
All the tests basically run the same scenario, wiht minimal variations,
so let's do the test once in the test touching the highest amount of
files and call it good.
>> Patch 2 is getting close for a committer lookup I think, still need to
>> look at patch 3 in details.. And from patch 3:
>> # Expected permission
>> - my $expected_file_perm = 0600;
>> - my $expected_dir_perm = 0700;
>> + my $expected_file_perm = $allow_group ? 0640 : 0600;
>> + my $expected_dir_perm = $allow_group ? 0750 : 0700;
>> Or $expected_file_perm and $expected_dir_perm could just be used as
>> arguments.
>
> This gets back to the check_pg_data_perm() discussion above.
>
> I'll hold off on another set of patches until I hear back from you.
> There were only trivial changes as noted above.
OK, let's keep things simple here and assume that the grouping extension
is not available yet. So no extra parameters should be needed.
I have begun to read through patch 3.
- if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+ if (stat_buf.st_mode & PG_MODE_MASK_ALLOW_GROUP)
ereport(FATAL,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has group or world access",
+ errmsg("data directory \"%s\" has invalid permissions",
DataDir),
- errdetail("Permissions should be u=rwx (0700).")));
+ errdetail("Permissions should be u=rwx (0700) or u=rwx,g=rx (0750).")));
Hm. This relaxes the checks and concerns me a lot. What if users want
to keep strict permission all the time and rely on the existing check to
be sure that this gets never changed post-initdb? For such users, we
may want to track if cluster has been initialized with group access, in
which case tracking that in the control file would be more adapted.
Then the startup checks should use this configuration. Another idea
would be a startup option. So, I cannot believe that all users would
like to see such checks relaxed. Some of my users would surely complain
about such sanity checks relaxed unconditionally, so making this
configurable would be fine, and the current approach is not acceptable
in my opinion.
Patch 2 introduces some standards regarding file permissions as those
are copied all over the code tree, which is definitely good in my
opinion. I am rather reserved about patch 3 per what I am seeing now.
Looking in-depth at the security-related requirements would take more
time than I have now.
--
Michael
Attachment
Hi Michael,
On 3/7/18 8:51 PM, Michael Paquier wrote:
> On Wed, Mar 07, 2018 at 10:56:32AM -0500, David Steele wrote:
>> On 3/6/18 10:04 PM, Michael Paquier wrote:
>>> Seems like you forgot to re-add the chmod calls in initdb.c.
>>
>> Hmmm, I thought we were talking about moving the position of umask().
>>
>> I don't think the chmod() calls are needed because umask() is being set.
>> The tests show that the config files have the proper permissions after
>> inidb, so this just looks like redundant code to me.
>
> Let's discuss that on a separate thread then, there could be something
> we are missing, but I agree that those should not be needed. Looking at
> the code history, those calls have been around since the beginning of
> PG-times.
Done.
>> Another way to do this would be to make the function generic and
>> stipulate that the postmaster must be shut down before running the
>> function. We could check postmaster.pid permissions as a separate
>> test.
>
> Yeah, that looks like a sensitive approach as this could be run
> post-initdb or after taking a backup. This way we would avoid other
> similar behaviors in the future... Still postmaster.pid is an
> exception.
Done.
>>> sub clean_rewind_test
>>> {
>>> + ok (check_pg_data_perm($node_master->data_dir(), 0));
>>> +
>>> $node_master->teardown_node if defined $node_master;
>>> I have also a pending patch for pg_rewind which adds read-only files in
>>> the data folders of a new test, so this would cause this part to blow
>>> up. Testing that for all the test sets does not bring additional value
>>> as well, and doing it at cleanup phase is also confusing. So could you
>>> move that check into only one test's script? You could remove the umask
>>> call in 003_extrafiles.pl as well this way, and reduce the patch diffs.
>>
>> I think I would rather have a way to skip the permission test rather
>> than disable it for most of the tests. pg_rewind does more writes into
>> PGDATA that anything other than a backend. Good coverage seems like a
>> plus.
>
> All the tests basically run the same scenario, with minimal variations,
> so let's do the test once in the test touching the highest amount of
> files and call it good.
OK, test 001 is used to check default mode and 002 is used to check
group mode. The rest are left as-is. Does that work for you?
> I have begun to read through patch 3.
>
> - if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
> + if (stat_buf.st_mode & PG_MODE_MASK_ALLOW_GROUP)
> ereport(FATAL,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> - errmsg("data directory \"%s\" has group or world access",
> + errmsg("data directory \"%s\" has invalid permissions",
> DataDir),
> - errdetail("Permissions should be u=rwx (0700).")));
> + errdetail("Permissions should be u=rwx (0700) or u=rwx,g=rx (0750).")));
> Hm. This relaxes the checks and concerns me a lot. What if users want
> to keep strict permission all the time and rely on the existing check to
> be sure that this gets never changed post-initdb? For such users, we
> may want to track if cluster has been initialized with group access, in
> which case tracking that in the control file would be more adapted.
> Then the startup checks should use this configuration. Another idea
> would be a startup option. So, I cannot believe that all users would
> like to see such checks relaxed. Some of my users would surely complain
> about such sanity checks relaxed unconditionally, so making this
> configurable would be fine, and the current approach is not acceptable
> in my opinion.
How about a GUC that enforces one mode or the other on startup? Default
would be 700. The GUC can be set automatically by initdb based on the
-g option. We had this GUC originally, but since the front-end tools
can't read it we abandoned it. Seems like it would be good as an
enforcing mechanism, though.
Thanks,
--
-David
david@pgmasters.net
Attachment
On Fri, Mar 09, 2018 at 01:51:14PM -0500, David Steele wrote:
> How about a GUC that enforces one mode or the other on startup? Default
> would be 700. The GUC can be set automatically by initdb based on the
> -g option. We had this GUC originally, but since the front-end tools
> can't read it we abandoned it. Seems like it would be good as an
> enforcing mechanism, though.
Hm. OK. I can see the whole set of points about that. Please let me
think a bit more about that bit. Do you think that there could be a
pool of users willing to switch from one mode to another? Compared to
your v1, we could indeed have a GUC which enforces a restriction to not
allow group access, and enabled by default. As the commit fest is
running and we don't have a clear picture yet, I am afraid that it may
be better to move that to v12, and focus on getting patches 1 and 2
committed. This will provide a good base for the next move.
There are three places where things are still not correct:
- if (chmod(location, S_IRWXU) != 0)
+ current_umask = umask(0);
+ umask(current_umask);
+
+ if (chmod(location, PG_DIR_MODE_DEFAULT & ~current_umask) != 0)
This is in tablespace.c.
@@ -185,6 +186,9 @@ main(int argc, char **argv)
exit(1);
}
+ /* Set dir/file mode mask */
+ umask(PG_MODE_MASK_DEFAULT);
+
In pg_rewind and pg_resetwal, isn't that also a portion which is not
necessary without the group access feature?
This is all I have basically for patch 2, which would be good for
shipping.
--
Michael
Attachment
Michael, David, * Michael Paquier (michael@paquier.xyz) wrote: > On Fri, Mar 09, 2018 at 01:51:14PM -0500, David Steele wrote: > > How about a GUC that enforces one mode or the other on startup? Default > > would be 700. The GUC can be set automatically by initdb based on the > > -g option. We had this GUC originally, but since the front-end tools > > can't read it we abandoned it. Seems like it would be good as an > > enforcing mechanism, though. > > Hm. OK. I can see the whole set of points about that. Please let me > think a bit more about that bit. Do you think that there could be a > pool of users willing to switch from one mode to another? Compared to > your v1, we could indeed have a GUC which enforces a restriction to not > allow group access, and enabled by default. As the commit fest is > running and we don't have a clear picture yet, I am afraid that it may > be better to move that to v12, and focus on getting patches 1 and 2 > committed. This will provide a good base for the next move. We already had a discussion about having a GUC for this and concluded, rightly in my view, that it's not sensible to have since we don't want all of the various tools having to read and parse out postgresql.conf. I don't see anything in the discussion which has changed that and I don't agree that there's an issue with using the privileges on the data directory for this- it's a simple solution which all of the tools can use and work with easily. I certainly don't agree that it's a serious issue to relax the explicit check- it's just a check, which a user could implement themselves if they wished to and had a concern for. On the other hand, with the explicit check, we are actively preventing an entirely reasonable goal of wanting to use a read-only role to perform a backup of the system. Thanks! Stephen
Attachment
On Mon, Mar 12, 2018 at 03:14:13PM -0400, Stephen Frost wrote: > We already had a discussion about having a GUC for this and concluded, > rightly in my view, that it's not sensible to have since we don't want > all of the various tools having to read and parse out postgresql.conf. If the problem is parsing, it could as well be more portable to put that in the control file, no? I have finished for example finished by implementing my own flavor of pg_controldata to save parsing efforts soas it prints control file fields on a per-call basis using individual fields, which saved also games with locales for as translations of pg_controldata can disturb the parsing logic. > I don't see anything in the discussion which has changed that and I > don't agree that there's an issue with using the privileges on the data > directory for this- it's a simple solution which all of the tools can > use and work with easily. I certainly don't agree that it's a serious > issue to relax the explicit check- it's just a check, which a user could > implement themselves if they wished to and had a concern for. On the > other hand, with the explicit check, we are actively preventing an > entirely reasonable goal of wanting to use a read-only role to perform a > backup of the system. Well, one thing is that the current checks in the postmaster make sure that a data folder is never using anything else than 0700. From a security point of view, making it possible to allow a postmaster to start with 0750 is a step backwards if users don't authorize it explicitely. There are a lot of systems which use a bunch of users with only single group with systemd. So this would remove an existing safeguard. I am not against the idea of this thread, just that I think that secured defaults should be user-enforceable if they want Postgres to behave so. -- Michael
Attachment
Michael, * Michael Paquier (michael@paquier.xyz) wrote: > On Mon, Mar 12, 2018 at 03:14:13PM -0400, Stephen Frost wrote: > > We already had a discussion about having a GUC for this and concluded, > > rightly in my view, that it's not sensible to have since we don't want > > all of the various tools having to read and parse out postgresql.conf. > > If the problem is parsing, it could as well be more portable to put that > in the control file, no? I have finished for example finished by > implementing my own flavor of pg_controldata to save parsing efforts > soas it prints control file fields on a per-call basis using individual > fields, which saved also games with locales for as translations of > pg_controldata can disturb the parsing logic. Then we'd need a tool to allow changing it for people who do want to change it. There's no reason we should have to have an extra tool for this- an administrator who chooses to change the privileges on the data folder should be able to do so and I don't think anyone is going to thank us for requiring them to use some special tool to do so for PostgreSQL. > > I don't see anything in the discussion which has changed that and I > > don't agree that there's an issue with using the privileges on the data > > directory for this- it's a simple solution which all of the tools can > > use and work with easily. I certainly don't agree that it's a serious > > issue to relax the explicit check- it's just a check, which a user could > > implement themselves if they wished to and had a concern for. On the > > other hand, with the explicit check, we are actively preventing an > > entirely reasonable goal of wanting to use a read-only role to perform a > > backup of the system. > > Well, one thing is that the current checks in the postmaster make sure > that a data folder is never using anything else than 0700. From a > security point of view, making it possible to allow a postmaster to > start with 0750 is a step backwards if users don't authorize it > explicitely. There are a lot of systems which use a bunch of users with > only single group with systemd. So this would remove an existing > safeguard. I am not against the idea of this thread, just that I think > that secured defaults should be user-enforceable if they want Postgres > to behave so. I'm aware of what the current one-time check in the postmaster does, and that we don't implement it on all platforms, making me seriously doubt that the level of concern being raised here makes sense. Should we consider it a security issue that the Windows builds don't perform this check, and never has? Further, if the permissions are changed without authorization, it's probably done while the database is running and unlikely to be noticed for days, weeks, or longer, if the administrator is depending on PG to let them know of the change. Considering that the only user who can change the privileges is a database owner or root, it seems even less likely to help (why would an attacker change those privileges when they already have full access?). Lastly, the user *is* able to enforce the privileges on the data directory if they wish to, using tools such as tripwire which are built specifically to provide such checks and to do so regularly instead of the extremely ad-hoc check provided by PG. PostgreSQL should, and does, secure the data directory when it's created by initdb, and subsequent files and directories are similairly secured appropriately. Ultimately, the default which makes sense here isn't a one-size-fits-all but is system dependent and the administrator should be able to choose what permissions they want to have. Thanks! Stephen
Attachment
On 3/13/18 2:46 AM, Michael Paquier wrote: > On Mon, Mar 12, 2018 at 03:14:13PM -0400, Stephen Frost wrote: >> We already had a discussion about having a GUC for this and concluded, >> rightly in my view, that it's not sensible to have since we don't want >> all of the various tools having to read and parse out postgresql.conf. > > If the problem is parsing, it could as well be more portable to put that > in the control file, no? The current approach is based on early discussion of this patch, around [1] and [2] in particular. I proposed an enforcing GUC at that time but there wasn't any interest in the idea. I definitely think it's overkill to put a field in pg_control as that requires more tooling to update values. >> I don't see anything in the discussion which has changed that and I >> don't agree that there's an issue with using the privileges on the data >> directory for this- it's a simple solution which all of the tools can >> use and work with easily. I certainly don't agree that it's a serious >> issue to relax the explicit check- it's just a check, which a user could >> implement themselves if they wished to and had a concern for. On the >> other hand, with the explicit check, we are actively preventing an >> entirely reasonable goal of wanting to use a read-only role to perform a >> backup of the system. > > Well, one thing is that the current checks in the postmaster make sure > that a data folder is never using anything else than 0700. From a > security point of view, making it possible to allow a postmaster to > start with 0750 is a step backwards if users don't authorize it > explicitely. I would argue that changing the mode of PGDATA is explicit, even if it is accidental. To be clear, after a pg_upgrade the behavior of the cluster WRT to setting the mode would be exactly the same as now. The user would need to specify -g at initdb time or explicitly update PGDATA to 750 for group access to be enabled. > There are a lot of systems which use a bunch of users with > only single group with systemd. So this would remove an existing > safeguard. I am not against the idea of this thread, just that I think > that secured defaults should be user-enforceable if they want Postgres > to behave so. As Stephen notes, this can be enforced by the user if they want to, and without much effort (and with better tools). Regards, -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/20526.1489428968%40sss.pgh.pa.us [2] https://www.postgresql.org/message-id/22248.1489431803%40sss.pgh.pa.us
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> * Michael Paquier (michael@paquier.xyz) wrote:
>> If the problem is parsing, it could as well be more portable to put that
>> in the control file, no?
> Then we'd need a tool to allow changing it for people who do want to
> change it. There's no reason we should have to have an extra tool for
> this- an administrator who chooses to change the privileges on the data
> folder should be able to do so and I don't think anyone is going to
> thank us for requiring them to use some special tool to do so for
> PostgreSQL.
FWIW, I took a quick look through this patch and don't have any problem
with the approach, which appears to be "use the data directory's
startup-time permissions as the status indicator". I am kinda wondering
about this though:
+ (These files can confuse <application>pg_ctl</application>.) If group read
+ access is enabled on the data directory and an unprivileged user in the
+ <productname>PostgreSQL</productname> group is performing the backup, then
+ <filename>postmaster.pid</filename> will not be readable and must be
+ excluded.
If we're allowing group read on the data directory, why should that not
include postmaster.pid? There's nothing terribly security-relevant in
there, and being able to find out the postmaster PID seems useful. I can
see the point of restricting server key files, as documented elsewhere,
but it's possible to keep those somewhere else so they don't cause
problems for simple backup software.
Also, in general, this patch desperately needs a round of copy-editing,
particularly with attention to the comments. For instance, there are a
minimum of three grammatical errors in this one comment:
+ * Group read/execute may optional be enabled on PGDATA so any frontend tools
+ * That write into PGDATA must know what mask to set and the permissions to
+ * use for creating files and directories.
In a larger sense, this fails to explain the operating principle,
namely what I stated above, that we allow the existing permissions
on PGDATA to decide what we allow as group permissions. If you
don't already understand that, you're going to have a hard time
extracting it from either file_perm.h or file_perm.c. (The latter
fails to even explain what the argument of DataDirectoryMask is.)
regards, tom lane
On 3/13/18 11:00 AM, Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> writes: >> * Michael Paquier (michael@paquier.xyz) wrote: >>> If the problem is parsing, it could as well be more portable to put that >>> in the control file, no? > >> Then we'd need a tool to allow changing it for people who do want to >> change it. There's no reason we should have to have an extra tool for >> this- an administrator who chooses to change the privileges on the data >> folder should be able to do so and I don't think anyone is going to >> thank us for requiring them to use some special tool to do so for >> PostgreSQL. > > FWIW, I took a quick look through this patch and don't have any problem > with the approach, which appears to be "use the data directory's > startup-time permissions as the status indicator". I am kinda wondering > about this though: > > + (These files can confuse <application>pg_ctl</application>.) If group read > + access is enabled on the data directory and an unprivileged user in the > + <productname>PostgreSQL</productname> group is performing the backup, then > + <filename>postmaster.pid</filename> will not be readable and must be > + excluded. > > If we're allowing group read on the data directory, why should that not > include postmaster.pid? There's nothing terribly security-relevant in > there, and being able to find out the postmaster PID seems useful. I can > see the point of restricting server key files, as documented elsewhere, > but it's possible to keep those somewhere else so they don't cause > problems for simple backup software. I'm OK with that. I had a look at the warnings regarding the required mode of postmaster.pid in miscinit.c (889-911) and it seems to me they still hold true if the mode is 640 instead of 600. Do you agree, Tom? Stephen? If so, I'll make those changes. > Also, in general, this patch desperately needs a round of copy-editing, > particularly with attention to the comments. For instance, there are a > minimum of three grammatical errors in this one comment: > > + * Group read/execute may optional be enabled on PGDATA so any frontend tools > + * That write into PGDATA must know what mask to set and the permissions to > + * use for creating files and directories. > In a larger sense, this fails to explain the operating principle, > namely what I stated above, that we allow the existing permissions > on PGDATA to decide what we allow as group permissions. If you > don't already understand that, you're going to have a hard time > extracting it from either file_perm.h or file_perm.c. (The latter > fails to even explain what the argument of DataDirectoryMask is.) I'll do comment/doc review for the next round of patches. Thanks, -- -David david@pgmasters.net
Greetings, * David Steele (david@pgmasters.net) wrote: > On 3/13/18 11:00 AM, Tom Lane wrote: > > Stephen Frost <sfrost@snowman.net> writes: > >> * Michael Paquier (michael@paquier.xyz) wrote: > >>> If the problem is parsing, it could as well be more portable to put that > >>> in the control file, no? > > > >> Then we'd need a tool to allow changing it for people who do want to > >> change it. There's no reason we should have to have an extra tool for > >> this- an administrator who chooses to change the privileges on the data > >> folder should be able to do so and I don't think anyone is going to > >> thank us for requiring them to use some special tool to do so for > >> PostgreSQL. > > > > FWIW, I took a quick look through this patch and don't have any problem > > with the approach, which appears to be "use the data directory's > > startup-time permissions as the status indicator". I am kinda wondering > > about this though: > > > > + (These files can confuse <application>pg_ctl</application>.) If group read > > + access is enabled on the data directory and an unprivileged user in the > > + <productname>PostgreSQL</productname> group is performing the backup, then > > + <filename>postmaster.pid</filename> will not be readable and must be > > + excluded. > > > > If we're allowing group read on the data directory, why should that not > > include postmaster.pid? There's nothing terribly security-relevant in > > there, and being able to find out the postmaster PID seems useful. I can > > see the point of restricting server key files, as documented elsewhere, > > but it's possible to keep those somewhere else so they don't cause > > problems for simple backup software. > > I'm OK with that. I had a look at the warnings regarding the required > mode of postmaster.pid in miscinit.c (889-911) and it seems to me they > still hold true if the mode is 640 instead of 600. > > Do you agree, Tom? Stephen? > > If so, I'll make those changes. I agree that we can still consider an EPERM-error case as being ok even with the changes proposed, and with the same-user check happening earlier in checkDataDir(), we won't even get to the point of looking at the pid file if the userid's don't match. The historical comment about the old datadir permissions can likely just be removed, perhaps replaced with a bit more commentary above that check in checkDataDir(). The open() call should also fail if we only have group-read privileges on the file (0640), but surely the kill() will in any case. Thanks! Stephen
Attachment
Hi Michael, On 3/12/18 3:28 AM, Michael Paquier wrote: > On Fri, Mar 09, 2018 at 01:51:14PM -0500, David Steele wrote: >> How about a GUC that enforces one mode or the other on startup? Default >> would be 700. The GUC can be set automatically by initdb based on the >> -g option. We had this GUC originally, but since the front-end tools >> can't read it we abandoned it. Seems like it would be good as an >> enforcing mechanism, though. > > Hm. OK. I can see the whole set of points about that. Please let me > think a bit more about that bit. Do you think that there could be a > pool of users willing to switch from one mode to another? Compared to > your v1, we could indeed have a GUC which enforces a restriction to not > allow group access, and enabled by default. As the commit fest is > running and we don't have a clear picture yet, I am afraid that it may > be better to move that to v12, and focus on getting patches 1 and 2 > committed. This will provide a good base for the next move. > > There are three places where things are still not correct: > > - if (chmod(location, S_IRWXU) != 0) > + current_umask = umask(0); > + umask(current_umask); > + > + if (chmod(location, PG_DIR_MODE_DEFAULT & ~current_umask) != 0) > This is in tablespace.c. I have moved this hunk to 03 and used only PG_DIR_MODE_DEFAULT instead. > @@ -185,6 +186,9 @@ main(int argc, char **argv) > exit(1); > } > > + /* Set dir/file mode mask */ > + umask(PG_MODE_MASK_DEFAULT); > + > In pg_rewind and pg_resetwal, isn't that also a portion which is not > necessary without the group access feature? These seem like a good idea to me with or without patch 03. Some of our front-end tools (initdb, pg_upgrade) were setting umask and others weren't. I think it's more consistent (and safer) if they all do, at least if they are writing into PGDATA. > This is all I have basically for patch 2, which would be good for > shipping. Thanks! I'll attach new patches in a reply to [1] once I have made the changes Tom requested. -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/22928.1520953220%40sss.pgh.pa.us
Attachment
David Steele <david@pgmasters.net> writes:
> On 3/12/18 3:28 AM, Michael Paquier wrote:
>> In pg_rewind and pg_resetwal, isn't that also a portion which is not
>> necessary without the group access feature?
> These seem like a good idea to me with or without patch 03. Some of our
> front-end tools (initdb, pg_upgrade) were setting umask and others
> weren't. I think it's more consistent (and safer) if they all do, at
> least if they are writing into PGDATA.
+1 ... see a926eb84e for an example of how easy it is to screw up if
the process's overall umask is permissive.
regards, tom lane
On 03/13/2018 10:40 AM, Stephen Frost wrote: > * Michael Paquier (michael@paquier.xyz) wrote: >> Well, one thing is that the current checks in the postmaster make sure >> that a data folder is never using anything else than 0700. From a >> security point of view, making it possible to allow a postmaster to >> start with 0750 is a step backwards ... > Lastly, the user *is* able to enforce the privileges on the data > directory if they wish to, using tools such as tripwire which are built > specifically to provide such checks and to do so regularly instead of > the extremely ad-hoc check provided by PG. > > ... Ultimately, the default which makes sense here isn't a > one-size-fits-all but is system dependent and the administrator should > be able to choose what permissions they want to have. Hear, hear. Returning for a moment again to https://www.postgresql.org/message-id/8b16cc30-0187-311e-04b5-1f32446d89dd%40anastigmatix.net we see that a stat() returning mode 0750 on a modern system may not even mean there is any group access at all. In that example, the datadir had these permissions: # getfacl . # file: . # owner: postgres # group: postgres user::rwx user:root:r-x group::--- mask::r-x other::--- While PostgreSQL does its stat() and interprets the mode as if it is still on a Unix box from the '80s, two things have changed underneath it: POSIX ACLs and Linux capabilities. Capabilities take the place of the former specialness of root, who now needs to be granted r-x explicitly in the ACL to be able to read stuff there at all, and there is clearly no access to group and no access to other. It would be hard for anybody to call this an insecure configuration. But when you stat() an object with a POSIX ACL, you get the 'mask' value where the 'group' bits used to go, so postgres sees this as 0750, thinks the 5 represents group access, and refuses to start. Purely a mistake. It's the kind of mistake that is inherent in this sort of check, which tries to draw conclusions from the semantics it assumes, while systems evolve and semantics march along. One hypothetical fix would be to add: #ifdef HAS_POSIX_ACLS ... check if there's really an ACL here, and the S_IRWXG bits are really just the mask, or even try to pass judgment on whether the admin's chosen ACL is adequately secure ... #endif but then sooner or later it will still end up making assumptions that aren't true under, say, SELinux, so there's another #ifdef, and where does it end? On 03/13/2018 11:00 AM, Tom Lane wrote: > In a larger sense, this fails to explain the operating principle, > namely what I stated above, that we allow the existing permissions > on PGDATA to decide what we allow as group permissions. I admit I've only skimmed the patches, trying to determine what that will mean in practice. In a case like the ACL example above, does this mean that postgres will stat PGDATA, conclude incorrectly that group access is granted, and then, based on that, actually go granting unwanted group access for real on newly-created files and directories? That doesn't seem ideal. On 03/13/2018 10:45 AM, David Steele wrote: > As Stephen notes, this can be enforced by the user if they want to, > and without much effort (and with better tools). To me, that seems really the key. An --allow-group-access option is nice (but, as we see, misleading if its assumptions are outdated regarding how the filesystem works), but I would plug even harder for a --permission-transparency option, which would just assume that the admin is making security arrangements, through mechanisms that postgres may or may not even understand. The admin can create ACLs with default entries that propagate to newly created objects. SELinux contexts can work in similar ways. The admin controls the umask inherited by the postmaster. A --permission-transparency option would simply use open and mkdir in the traditional ways, allowing the chosen umask, ACL defaults, SELinux contexts, etc., to do their thing, and would avoid then stepping on the results with explicit chmods (and of course skip the I-refuse-to-start-because-I- misunderstand-your-setup check). It wouldn't be for every casual install, but it would be the best way to stay out of the way of an admin securing a system with modern facilities. A lot of the design effort put into debating what postgres itself should or shouldn't insist on could then, perhaps, go into writing postgres-specific configuration rule packages for some of those better configuration-checking tools, and there it might even be possible to write tests that incorporate knowledge of ACLs, SELinux, etc. -Chap
Greetings Chapman, * Chapman Flack (chap@anastigmatix.net) wrote: > On 03/13/2018 10:40 AM, Stephen Frost wrote: > > ... Ultimately, the default which makes sense here isn't a > > one-size-fits-all but is system dependent and the administrator should > > be able to choose what permissions they want to have. > > Hear, hear. Returning for a moment again to > > https://www.postgresql.org/message-id/8b16cc30-0187-311e-04b5-1f32446d89dd%40anastigmatix.net > > we see that a stat() returning mode 0750 on a modern system may not > even mean there is any group access at all. In that example, the > datadir had these permissions: Yes, that's true, but PG has never paid attention to POSIX ACLs or Linux capabilities. Changing it to do so is quite a bit beyond the scope of this particular patch and I don't see anything in what this patch is doing which would preclude someone from putting in that effort in the future. > While PostgreSQL does its stat() and interprets the mode as if it is > still on a Unix box from the '80s, two things have changed underneath > it: POSIX ACLs and Linux capabilities. Capabilities take the place of > the former specialness of root, who now needs to be granted r-x > explicitly in the ACL to be able to read stuff there at all, and there > is clearly no access to group and no access to other. It would be hard > for anybody to call this an insecure configuration. But when you stat() > an object with a POSIX ACL, you get the 'mask' value where the 'group' > bits used to go, so postgres sees this as 0750, thinks the 5 represents > group access, and refuses to start. Purely a mistake. I'll point out that PG does run on quite a few other OS's beyond Linux. > It's the kind of mistake that is inherent in this sort of check, > which tries to draw conclusions from the semantics it assumes, while > systems evolve and semantics march along. One hypothetical fix would > be to add: > > #ifdef HAS_POSIX_ACLS > ... check if there's really an ACL here, and the S_IRWXG bits are > really just the mask, or even try to pass judgment on whether the > admin's chosen ACL is adequately secure ... > #endif > > but then sooner or later it will still end up making assumptions > that aren't true under, say, SELinux, so there's another #ifdef, > and where does it end? I agree with this general concern. > On 03/13/2018 11:00 AM, Tom Lane wrote: > > In a larger sense, this fails to explain the operating principle, > > namely what I stated above, that we allow the existing permissions > > on PGDATA to decide what we allow as group permissions. > > I admit I've only skimmed the patches, trying to determine what > that will mean in practice. In a case like the ACL example above, > does this mean that postgres will stat PGDATA, conclude incorrectly > that group access is granted, and then, based on that, actually go > granting unwanted group access for real on newly-created files > and directories? PG will stat PGDATA and conclude that the system is saying that group access has been granted on PGDATA and will do the same for subsequent files created later on. This is new in PG, so there isn't any concern about this causing problems in an existing environment- you couldn't have had those ACLs on an existing PGDATA dir in the first place, as you note above. The only issue that remains is that PG doesn't understand or work with POSIX ACLs or Linux capabilities, but that's not anything new. > On 03/13/2018 10:45 AM, David Steele wrote: > > As Stephen notes, this can be enforced by the user if they want to, > > and without much effort (and with better tools). > > To me, that seems really the key. An --allow-group-access option is > nice (but, as we see, misleading if its assumptions are outdated > regarding how the filesystem works), but I would plug even harder for > a --permission-transparency option, which would just assume that the > admin is making security arrangements, through mechanisms that > postgres may or may not even understand. The admin can create ACLs > with default entries that propagate to newly created objects. > SELinux contexts can work in similar ways. The admin controls the > umask inherited by the postmaster. A --permission-transparency option > would simply use open and mkdir in the traditional ways, allowing > the chosen umask, ACL defaults, SELinux contexts, etc., to do their > thing, and would avoid then stepping on the results with explicit > chmods (and of course skip the I-refuse-to-start-because-I- > misunderstand-your-setup check). > > It wouldn't be for every casual install, but it would be the best > way to stay out of the way of an admin securing a system with modern > facilities. > > A lot of the design effort put into debating what postgres itself > should or shouldn't insist on could then, perhaps, go into writing > postgres-specific configuration rule packages for some of those > better configuration-checking tools, and there it might even be > possible to write tests that incorporate knowledge of ACLs, SELinux, > etc. I'm a fan of this idea in general, but it's unclear how that --permission-transparency option would work in practice. Are you suggesting that it be a compile-time flag, which would certainly work but also then have to be debated among packagers as to the right setting and if there's any need to be concerned about users misunderstanding it, or a flag for each program, which hardly seems ideal as some invokations of programs might forget to specify that, leading to inconsistent permissions, or something else..? We've pretty well established that there's no particularly good way for PG to just "know" beyond looking at the privileges on the data directory, but we'd have to build in complete support for POSIX ACLs and Linux capabilities if we go down a route where we want to enforce the same POSIX ACLs for every file that exists in the data directory based on the data directory's ACLs. I'm not entirely sure that would even be possible when it comes to capabilities, or if it would make sense.. From what I've seen with SELinux, thankfully, these same issues don't really crop up (probably because PG never tried to force anything SELinux related, and SELinux is largely independent of the POSIX ACLs and Linux capabilities, so it doesn't impact things in this way). Thanks! Stephen
Attachment
On 03/13/2018 01:50 PM, Stephen Frost wrote: > Yes, that's true, but PG has never paid attention to POSIX ACLs or Linux > capabilities. Changing it to do so is quite a bit beyond the scope... I think we're largely in agreement here, as my aim was not to advocate that PG should work harder to understand the subtleties of every system it could be installed on, but rather that it should work less hard at pretending to understand them when it doesn't, and thus avoid obstructing the admin, who presumably does. > I'll point out that PG does run on quite a few other OS's beyond Linux. I'll second that, as I think it is making my argument. When I can supply three or four examples of semantic subtleties that undermine PG's assumptions under Linux alone, the picture when broadened to those quite-a-few other platforms as well certainly doesn't become simpler! >> but then sooner or later it will still end up making assumptions >> that aren't true under, say, SELinux, so there's another #ifdef, >> and where does it end? > > I agree with this general concern. :) That's probably where it became clear that I'm not advocating an add-#ifdefs-for-everything approach. > PG will stat PGDATA and conclude that the system is saying that group > access has been granted on PGDATA and will do the same for subsequent > files created later on. ... The only issue that remains is that PG > doesn't understand or work with POSIX ACLs or Linux capabilities, > but that's not anything new. What's new is that it is now pretending even more extravagantly to understand what it doesn't understand. Where it would previously draw in incorrect conclusion and refuse to start, which is annoying but not very difficult to work around if need be, it would now draw the same incorrect conclusion and then actively go about making the real world embody the incorrect conclusion, contrary to the admin's efforts. >> umask inherited by the postmaster. A --permission-transparency option >> would simply use open and mkdir in the traditional ways, allowing >> the chosen umask, ACL defaults, SELinux contexts, etc., to do their >> thing, and would avoid then stepping on the results with explicit >> chmods (and of course skip the I-refuse-to-start-because-I- >> misunderstand-your-setup check). >> ... > > I'm a fan of this idea in general, but it's unclear how that > --permission-transparency option would work in practice. Are you > suggesting that it be a compile-time flag, which would certainly work > but also then have to be debated among packagers as to the right setting > and if there's any need to be concerned about users misunderstanding it, > or a flag for each program, I was thinking of a command-line option ... > which hardly seems ideal as some invokations > of programs might forget to specify that, leading to inconsistent > permissions, or something else..? ... but I see how that gets complicated with the various other command- line utilities included. > .. we'd have to build in complete > support for POSIX ACLs and Linux capabilities if we go down a route I'm wary of an argument that we can't do better except by building in complete support for POSIX ACLs, and capabilities (and NFSv4 ACLs, and SELinux, and AppArmor, and #ifdef and #ifdef and #ifdef). It seems to me that, in most cases, the designers of these sorts of extensions to old traditional Unix behavior take great pains to design them such that they can still usefully function in the presence of programs that "don't pay attention to or understand or use" them, as long as those programs are in some sense well-behaved, and not going out of their way with active steps that insist on or impose permissions that aren't appropriate under the non-understood circumstances. So my suggestion boils down to PG having at least an option, somehow, to be well-behaved in that sense. -Chap
Chapman Flack <chap@anastigmatix.net> writes:
> On 03/13/2018 01:50 PM, Stephen Frost wrote:
>> I'll point out that PG does run on quite a few other OS's beyond Linux.
> I'll second that, as I think it is making my argument. When I can
> supply three or four examples of semantic subtleties that undermine
> PG's assumptions under Linux alone, the picture when broadened to
> those quite-a-few other platforms as well certainly doesn't become
> simpler!
Well, to be blunt, what we target is POSIX-compatible systems. If
you're telling us to worry about non-POSIX filesystem semantics,
and your name is not Microsoft, it's going to be a hard sell.
We have enough to do just keeping up with that scope of target
systems.
regards, tom lane
Greetings Chapman, * Chapman Flack (chap@anastigmatix.net) wrote: > On 03/13/2018 01:50 PM, Stephen Frost wrote: > > PG will stat PGDATA and conclude that the system is saying that group > > access has been granted on PGDATA and will do the same for subsequent > > files created later on. ... The only issue that remains is that PG > > doesn't understand or work with POSIX ACLs or Linux capabilities, > > but that's not anything new. > > What's new is that it is now pretending even more extravagantly to > understand what it doesn't understand. Where it would previously draw > in incorrect conclusion and refuse to start, which is annoying but > not very difficult to work around if need be, it would now draw the > same incorrect conclusion and then actively go about making the real > world embody the incorrect conclusion, contrary to the admin's efforts. I have to say that I disagree about it being "easy to work-around" PG refusing to start. Also, we're not pretending any more or less, we're sticking to exactly what we do understand- which is the 80's unix permission system, as you put it. The options that I see here are to stick with the user/group system and our naive understanding of it, to go whole-hog and try to completely understand everything (with lots of #ifdef's, as discussed), or to completely remove all checks- but we don't have a clear proposal for that and it strikes me as at least unlikely to go over well anyway, given all of the discussion here about trying to simply change the one check we have. > >> umask inherited by the postmaster. A --permission-transparency option > >> would simply use open and mkdir in the traditional ways, allowing > >> the chosen umask, ACL defaults, SELinux contexts, etc., to do their > >> thing, and would avoid then stepping on the results with explicit > >> chmods (and of course skip the I-refuse-to-start-because-I- > >> misunderstand-your-setup check). > >> ... > > > > I'm a fan of this idea in general, but it's unclear how that > > --permission-transparency option would work in practice. Are you > > suggesting that it be a compile-time flag, which would certainly work > > but also then have to be debated among packagers as to the right setting > > and if there's any need to be concerned about users misunderstanding it, > > or a flag for each program, > > I was thinking of a command-line option ... > > > which hardly seems ideal as some invokations > > of programs might forget to specify that, leading to inconsistent > > permissions, or something else..? > > ... but I see how that gets complicated with the various other command- > line utilities included. Indeed. > > .. we'd have to build in complete > > support for POSIX ACLs and Linux capabilities if we go down a route > > I'm wary of an argument that we can't do better except by building > in complete support for POSIX ACLs, and capabilities (and NFSv4 > ACLs, and SELinux, and AppArmor, and #ifdef and #ifdef and #ifdef). I don't think I meant to imply that we can't do better, I was just trying to enumate what I saw the different options being. > It seems to me that, in most cases, the designers of these sorts of > extensions to old traditional Unix behavior take great pains to design > them such that they can still usefully function in the presence of > programs that "don't pay attention to or understand or use" them, as > long as those programs are in some sense well-behaved, and not going > out of their way with active steps that insist on or impose permissions > that aren't appropriate under the non-understood circumstances. > > So my suggestion boils down to PG having at least an option, somehow, > to be well-behaved in that sense. I'm afraid that we haven't got any great answer to that "somehow". I was hoping you might have some other ideas beyond command-line switches which could leave the system in an inconsistent state a bit too easily. Unless there's a better way then the approach proposed by Tom (originally) and implemented by David seems like the way to go and at least an improvement over the current situation. Thanks! Stephen
Attachment
On 03/13/2018 02:47 PM, Tom Lane wrote: > Well, to be blunt, what we target is POSIX-compatible systems. If > you're telling us to worry about non-POSIX filesystem semantics, > and your name is not Microsoft, it's going to be a hard sell. > We have enough to do just keeping up with that scope of target > systems. So, how many POSIX-compatible systems are available today (if any), where you can actually safely assume that there are no additional security/access-control-related considerations in effect beyond three user bits/three group bits/three other bits, and not be wrong? I'm not advocating the Sisyphean task of having PG incorporate knowledge of all those possibilities. I'm advocating the conservative approach: have PG be that well-behaved application that those extended semantics are generally all designed to play well with, and just not do stuff that obstructs or tramples over what the admin takes care to set up. On 03/13/2018 03:44 PM, Stephen Frost wrote: > * Chapman Flack (chap@anastigmatix.net) wrote: >> So my suggestion boils down to PG having at least an option, somehow, >> to be well-behaved in that sense. > > I'm afraid that we haven't got any great answer to that "somehow". I > was hoping you might have some other ideas beyond command-line > switches which could leave the system in an inconsistent state a bit > too easily. I wonder how complicated it would have to be really. On any system with a POSIX base, I guess it's possible for PGDATA to have an S_ISVTX "sticky" bit in the mode, which does have an official significance (but one that only affects whether non-postgres can rename or unlink things in the directory, which might be of little practical significance). Perhaps its meaning could be overloaded with "the admin is handling the permissions, thank you", and postmaster and various command-line utilities could see it, and refrain from any gratuitous chmods or refusals to function. Or, if overloading S_ISVTX seems in poor taste, what would be wrong with simply checking for an empty file PERMISSIONS-ARE-MANAGED in PGDATA and responding the same way? Or, assuming some form of ACL is available, just let the admin change the owner and group of PGDATA to other than postgres, grant no access to other, and give rwx to postgres in the ACL? PG could then reason as follows: * I do not own this directory. * I am not the group of this directory. * It grants no access to other. * Yet, I find myself listing and accessing files in it without difficulty. * The admin has set this up for me in a way I do not understand. * I will refrain from messing with it. Three ideas off the top of my head. Probably more where they came from. :) -Chap
Chapman, * Chapman Flack (chap@anastigmatix.net) wrote: > I'm not advocating the Sisyphean task of having PG incorporate > knowledge of all those possibilities. I'm advocating the conservative > approach: have PG be that well-behaved application that those extended > semantics are generally all designed to play well with, and just not > do stuff that obstructs or tramples over what the admin takes care > to set up. I think we get that you're advocating removing the checks and permissions-setting that the PG tools do, however... > I wonder how complicated it would have to be really. On any system > with a POSIX base, I guess it's possible for PGDATA to have an S_ISVTX > "sticky" bit in the mode, which does have an official significance (but > one that only affects whether non-postgres can rename or unlink things > in the directory, which might be of little practical significance). > Perhaps its meaning could be overloaded with "the admin is handling > the permissions, thank you", and postmaster and various command-line > utilities could see it, and refrain from any gratuitous chmods or > refusals to function. > > Or, if overloading S_ISVTX seems in poor taste, what would be wrong > with simply checking for an empty file PERMISSIONS-ARE-MANAGED in > PGDATA and responding the same way? > > Or, assuming some form of ACL is available, just let the admin > change the owner and group of PGDATA to other than postgres, > grant no access to other, and give rwx to postgres in the ACL? None of these suggestions really sound workable to me. I certainly don't think we should be overloading the meaning of a specific and entirely independent filesystem permission (and really don't even want to imagine what it would require to do something like that on Windows...), dropping an empty file in the directory strikes me as a ripe target for confusion, and we have specific checks to try and make sure we are running as the owner that owns the directory with some pretty good reasons around that (avoiding multiple postmasters running in the same PGDATA directory, specifically). > PG could then reason as follows: * I do not own this directory. > * I am not the group of this directory. * It grants no access to other. > * Yet, I find myself listing and accessing files in it without > difficulty. * The admin has set this up for me in a way I do not > understand. * I will refrain from messing with it. Removing the check that says "we aren't going to try to run PG in this directory if we aren't the owner of it" also doesn't seem like it's necessairly a great plan. > Three ideas off the top of my head. Probably more where they came from. None of them really seem workable though. On the other hand, let's consider what this patch actually ends up doing when POSIX ACLs are involved. This allows ACLs to be used where they weren't before and with appropriate defaults set even to work for new files being created, which wouldn't work before. Yes, if you create a default ACL which says "grant this other user write access to files in the PG data directory" then that won't actually be honored because we will chmod(640) the file and the POSIX ACL system actually works with the user/group privilege system, like so: Create the "data" dir, as initdb would: ➜ ~ mkdir xyz ➜ ~ chmod 750 xyz ➜ ~ ls -ld xyz drwxr-x--- 2 sfrost sfrost 4096 Mar 13 19:07 xyz Set a default ACL to allow the "daemon" user read/write access to files created: ➜ ~ setfacl -dm u:daemon:rw xyz ➜ ~ getfacl xyz # file: xyz # owner: sfrost # group: sfrost user::rwx group::r-x other::--- default:user::rwx default:user:daemon:rw- default:group::r-x default:mask::rwx default:other::--- Create a file the way PG would: ➜ ~ touch xyz/a ➜ ~ chmod 640 xyz/a ➜ ~ getfacl xyz/a # file: xyz/a # owner: sfrost # group: sfrost user::rw- user:daemon:rw- #effective:r-- group::r-x #effective:r-- mask::r-- other::--- The daemon user ends up with read-only access (note the '#effective', which shows that the POSIX ACL system isn't overriding the "regular" ACLs). ... but that's basically what we want. Multiple users having write access to the data directory could be quite bad as you might possibly get two postmasters running against the same data directory at the same time and there's basically no case where that's a good thing to have happen. This change does let users grant out read access to other users/groups, even beyond what's possible using the traditional user/group system, so this opens up a lot more possible options for advanced users, provided they set the defaults appropriately at the directory level (which, presumably, an administrator versed in POSIX ACLs and wishing to use them would know, or would figure out quickly). Yes, perhaps there's some argument to be made that we should have an option where we don't force any privileges, but that can certainly be considered a future capability and what's being implemented here doesn't actually break use-cases which worked before and allows users more freedom than they had before to use POSIX ACLs if they want to use them, and without having to modify PG to understand POSIX ACLs explicitly. Also, to the point you raise up-thread, where you remove access to the data directory from the group that owns the directory, but grant it to some other user, yes, files inside the data directory would end up with group access for that other group. That won't matter as long as the group doesn't have rights on the directory, though it would certainly be cleaner to just have a dedicated group where it isn't an issue for that group to have read access to the files. I would imagine anyone using POSIX ACLs would understand this without too much difficulty. Of course, POSIX default ACLs would also continue to work for files created under the data directory, as illustrated above. In all, this is looking like a pretty good improvement while also having some caution about what privileges are effectively allowed. Thanks! Stephen
Attachment
On Tue, Mar 13, 2018 at 01:28:17PM -0400, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 3/12/18 3:28 AM, Michael Paquier wrote: >>> In pg_rewind and pg_resetwal, isn't that also a portion which is not >>> necessary without the group access feature? > >> These seem like a good idea to me with or without patch 03. Some of our >> front-end tools (initdb, pg_upgrade) were setting umask and others >> weren't. I think it's more consistent (and safer) if they all do, at >> least if they are writing into PGDATA. > > +1 ... see a926eb84e for an example of how easy it is to screw up if > the process's overall umask is permissive. Okay. A suggestion that I have here would be to split those extra calls into a separate patch. That's a useful self-contained improvement. -- Michael
Attachment
On Tue, Mar 13, 2018 at 12:19:07PM -0400, David Steele wrote: > I'll attach new patches in a reply to [1] once I have made the changes > Tom requested. Cool, thanks for your patience. Looking forward to seeing those. I'll spend time on 0003 at the same time. Let's also put the additional umask calls for pg_rewind and pg_resetwal into a separate patch. -- Michael
Attachment
Hi, On 3/13/18 12:13 PM, Stephen Frost wrote: > * David Steele (david@pgmasters.net) wrote: >> On 3/13/18 11:00 AM, Tom Lane wrote: >>> >>> FWIW, I took a quick look through this patch and don't have any problem >>> with the approach, which appears to be "use the data directory's >>> startup-time permissions as the status indicator". I am kinda wondering >>> about this though: >>> >>> + (These files can confuse <application>pg_ctl</application>.) If group read >>> + access is enabled on the data directory and an unprivileged user in the >>> + <productname>PostgreSQL</productname> group is performing the backup, then >>> + <filename>postmaster.pid</filename> will not be readable and must be >>> + excluded. >>> >>> If we're allowing group read on the data directory, why should that not >>> include postmaster.pid? There's nothing terribly security-relevant in >>> there, and being able to find out the postmaster PID seems useful. I can >>> see the point of restricting server key files, as documented elsewhere, >>> but it's possible to keep those somewhere else so they don't cause >>> problems for simple backup software. >> >> I'm OK with that. I had a look at the warnings regarding the required >> mode of postmaster.pid in miscinit.c (889-911) and it seems to me they >> still hold true if the mode is 640 instead of 600. >> >> Do you agree, Tom? Stephen? >> >> If so, I'll make those changes. > > I agree that we can still consider an EPERM-error case as being ok even > with the changes proposed, and with the same-user check happening > earlier in checkDataDir(), we won't even get to the point of looking at > the pid file if the userid's don't match. The historical comment about > the old datadir permissions can likely just be removed, perhaps replaced > with a bit more commentary above that check in checkDataDir(). The > open() call should also fail if we only have group-read privileges on > the file (0640), but surely the kill() will in any case. OK, that being the case a new patch set is attached that sets the mode of postmaster.pid the same as other files in PGDATA. I also cleaned up / corrected / added comments in various places. Thanks, -- -David david@pgmasters.net
Attachment
Hi Michael, On 3/13/18 9:31 PM, Michael Paquier wrote: > On Tue, Mar 13, 2018 at 12:19:07PM -0400, David Steele wrote: >> I'll attach new patches in a reply to [1] once I have made the changes >> Tom requested. > > Cool, thanks for your patience. Looking forward to seeing those. I'll > spend time on 0003 at the same time. Let's also put the additional > umask calls for pg_rewind and pg_resetwal into a separate patch. Do you mean a separate patch in this patch set, or a separate patch entirely? 02 depends on this logic, so I guess you mean create a new patch between 01 and 02? Are you just trying to make sure it gets in? I'd rather wait a bit - if 02 doesn't look like it will get in, then I'll pull this logic out into a separate patch. Thanks, -- -David david@pgmasters.net
Attachment
On Wed, Mar 14, 2018 at 02:15:03PM -0400, David Steele wrote: > Do you mean a separate patch in this patch set, or a separate patch > entirely? 02 depends on this logic, so I guess you mean create a new > patch between 01 and 02? Yes, one new patch that which can be applied on top of 1. > Are you just trying to make sure it gets in? I'd rather wait a bit - if > 02 doesn't look like it will get in, then I'll pull this logic out into > a separate patch. Okay. -- Michael
Attachment
On Wed, Mar 14, 2018 at 02:08:19PM -0400, David Steele wrote: > OK, that being the case a new patch set is attached that sets the mode > of postmaster.pid the same as other files in PGDATA. +1. > I also cleaned up / corrected / added comments in various places. Patches 1 and 2 look fine to me. The new umask calls in pg_rewind and pg_resetwal could be split into a separate patch but that's a minor issue. When taking a base backup from a data folder which has group access, then the tar data, as well as the untar'ed data, are still using 0600 as umask for files and 0700 for folders. Is that an expected behavior? I would have imagined that sendFileWithContent() and _tarWriteDir() should enforce the file mode to have group access if the cluster has been initialized to work as such. Still as this is a feature aimed at being used for custom backups, that's not really a blocker I guess. Visibly there would be no need for a -g switch in pg_basebackup as it is possible to guess from the received untar'ed files what should be the permissions of the data based on what is received in pg_basebackup.c. It would also be necessary to change the permissions of pg_wal as this is created before receiving any files. Speaking of which, we may want to switch the values used for st_mode to what file_perm.h is giving in basebackup.c? We should also replace the hardcoded 0700 value in pg_backup_directory.c by what file_perm.h offers? I would recommend to not touch at mkdtemp.c as this comes from NetBSD. +=item $node->group_access() + +Does the data dir allow group access? + Nit: s/dir/directory/. Indentation is weird in PostgresNode.pm for some of the chmod calls (tabs not spaces please). -- Michael
Attachment
On 3/15/18 3:17 AM, Michael Paquier wrote: > On Wed, Mar 14, 2018 at 02:08:19PM -0400, David Steele wrote: > > When taking a base backup from a data folder which has group access, > then the tar data, as well as the untar'ed data, are still using > 0600 as umask for files and 0700 for folders. Is that an expected > behavior? I would have imagined that sendFileWithContent() and > _tarWriteDir() should enforce the file mode to have group access if the > cluster has been initialized to work as such. We can certainly make base backup understand the group access mode. Should we continue hard-coding the mode, or use the actual dir/file mode? > Still as this is a > feature aimed at being used for custom backups, that's not really a > blocker I guess. Seems like a good thing to do, though, so I'll have a look for the next patch. > Visibly there would be no need for a -g switch in > pg_basebackup as it is possible to guess from the received untar'ed > files what should be the permissions of the data based on what is > received in pg_basebackup.c. It would also be necessary to change the > permissions of pg_wal as this is created before receiving any files. This part might be trickier. > Speaking of which, we may want to switch the values used for st_mode to > what file_perm.h is giving in basebackup.c? Will do. > We should also replace the hardcoded 0700 value in pg_backup_directory.c > by what file_perm.h offers? I would recommend to not touch at mkdtemp.c > as this comes from NetBSD. Will do. > +=item $node->group_access() > + > +Does the data dir allow group access? > + > Nit: s/dir/directory/. > > Indentation is weird in PostgresNode.pm for some of the chmod calls > (tabs not spaces please). I'll fix these in the next patch as well. Thanks, -- -David david@pgmasters.net
Attachment
Greetings David, Michael, all, * David Steele (david@pgmasters.net) wrote: > On 3/15/18 3:17 AM, Michael Paquier wrote: > > On Wed, Mar 14, 2018 at 02:08:19PM -0400, David Steele wrote: > > > > When taking a base backup from a data folder which has group access, > > then the tar data, as well as the untar'ed data, are still using > > 0600 as umask for files and 0700 for folders. Is that an expected > > behavior? I would have imagined that sendFileWithContent() and > > _tarWriteDir() should enforce the file mode to have group access if the > > cluster has been initialized to work as such. > > We can certainly make base backup understand the group access mode. > Should we continue hard-coding the mode, or use the actual dir/file mode? Given that we're already, as I recall, including the owner/group of the file being backed up through pg_basebackup in the tarball, it seems like we should be including whatever the current dir/file mode is too. Those files may not have anything to do with PostgreSQL, after all, and a 'natural' tar of the directory would capture that information too. > > Still as this is a > > feature aimed at being used for custom backups, that's not really a > > blocker I guess. > > Seems like a good thing to do, though, so I'll have a look for the next > patch. +1. > > Visibly there would be no need for a -g switch in > > pg_basebackup as it is possible to guess from the received untar'ed > > files what should be the permissions of the data based on what is > > received in pg_basebackup.c. It would also be necessary to change the > > permissions of pg_wal as this is created before receiving any files. > > This part might be trickier. This seems like another case where what we should be doing, and what people will be expecting, I'd think, is just what they're used to tar doing in these cases- which would be setting the dir/file mode for each file based on what's in the tarball. Again, the files which are in the data dir are, sadly, not always just those that PG is familiar with. Thanks! Stephen
Attachment
On Fri, Mar 16, 2018 at 11:12:44AM -0400, Stephen Frost wrote: > Given that we're already, as I recall, including the owner/group of the > file being backed up through pg_basebackup in the tarball, it seems like > we should be including whatever the current dir/file mode is too. Those > files may not have anything to do with PostgreSQL, after all, and a > 'natural' tar of the directory would capture that information too. Yes. tar does include this information in the header associated to each file. Do we want an additional switch for pg_receivexlog as well by the way which generates WAL segments with group access? Some people take base backups without slots and rely on an archive to look for remaining segments up to the end-of-backup record. -- Michael
Attachment
On 3/16/18 11:12 AM, Stephen Frost wrote: > >>> Visibly there would be no need for a -g switch in >>> pg_basebackup as it is possible to guess from the received untar'ed >>> files what should be the permissions of the data based on what is >>> received in pg_basebackup.c. It would also be necessary to change the >>> permissions of pg_wal as this is created before receiving any files. >> >> This part might be trickier. > > This seems like another case where what we should be doing, and what > people will be expecting, I'd think, is just what they're used to tar > doing in these cases- which would be setting the dir/file mode for each > file based on what's in the tarball. Again, the files which are in the > data dir are, sadly, not always just those that PG is familiar with. I've been working on this and have become convinced that adding group permissions to files that pg_basebackup writes to disk based on whether group permissions are enabled in PGDATA isn't the right way to go. To be clear, I'm not taking about the permissions set within the tar file - I think it makes sense to use the actual PGDATA permissions in that case. pg_basebackup may not be running as postgres, and even if it is I don't think we can assume that group access is appropriate for the files that it writes. It's a different environment and different security rules may apply. It seems to me that pg_basebackup and pg_receivexlog should have a -g option to control the mode of the files that they write to disk (not including the modes stored in the tar files). Or perhaps we should just update the perms in the tar files for now and leave the rest alone. Thoughts? -- -David david@pgmasters.net
Attachment
David, * David Steele (david@pgmasters.net) wrote: > On 3/16/18 11:12 AM, Stephen Frost wrote: > > > >>> Visibly there would be no need for a -g switch in > >>> pg_basebackup as it is possible to guess from the received untar'ed > >>> files what should be the permissions of the data based on what is > >>> received in pg_basebackup.c. It would also be necessary to change the > >>> permissions of pg_wal as this is created before receiving any files. > >> > >> This part might be trickier. > > > > This seems like another case where what we should be doing, and what > > people will be expecting, I'd think, is just what they're used to tar > > doing in these cases- which would be setting the dir/file mode for each > > file based on what's in the tarball. Again, the files which are in the > > data dir are, sadly, not always just those that PG is familiar with. > > I've been working on this and have become convinced that adding group > permissions to files that pg_basebackup writes to disk based on whether > group permissions are enabled in PGDATA isn't the right way to go. > > To be clear, I'm not taking about the permissions set within the tar > file - I think it makes sense to use the actual PGDATA permissions in > that case. What that implies then, if I'm following, is that the results of a pg_basebackup -Ft && tar -xvf ; would be different from the results of a pg_basebackup -Fp ; . That seems like it would be rather odd and confusing to users and so I have a hard time agreeing that such an inconsistency makes sense. > pg_basebackup may not be running as postgres, and even if it is I don't > think we can assume that group access is appropriate for the files that > it writes. It's a different environment and different security rules > may apply. Sure, and the same security rules may also apply. Consider an environment which is managed through a change management system (as many are these days...) where the pg_basebackup is being run specifically to set up a replica (which is quite commonly done..) and then allow a tool like pgbackrest to use both the primary and the replica for backups, where pgbackrest is run as an independent user which shares the same group as the PG user and needs group-read access on the replica. After building such a replica, the user would have to do a chmod across the entire data directory, even though the primary was initialized with group-read access, or, oddly, perform the pg_basebackup to tar files and then extract those tar files instead of using the plain format. The general pg_basebackup->replica process works great today and the primary and the replica more-or-less match as if they were both initdb'd the same way, or a backup/restore was done, and not preserving the privileges as they exist would end up diverging from that. In these cases we're really talking about the defaults here; as I mention below, I agree that having the ability to control what ends up happening definitely makes sense (as tar does...). > It seems to me that pg_basebackup and pg_receivexlog should have a -g > option to control the mode of the files that they write to disk (not > including the modes stored in the tar files). > > Or perhaps we should just update the perms in the tar files for now and > leave the rest alone. Having options to pg_basebackup to control what's done makes sense to me- but whatever those options do, I'd expect them to apply equally to the tar files and to the files extracted with plain mode. Having those be different really strikes me as very odd. Thanks! Stephen
Attachment
On Tue, Mar 20, 2018 at 05:44:22PM -0400, Stephen Frost wrote: > * David Steele (david@pgmasters.net) wrote: >> On 3/16/18 11:12 AM, Stephen Frost wrote: >> It seems to me that pg_basebackup and pg_receivexlog should have a -g >> option to control the mode of the files that they write to disk (not >> including the modes stored in the tar files). >> >> Or perhaps we should just update the perms in the tar files for now and >> leave the rest alone. > > Having options to pg_basebackup to control what's done makes sense to > me- but whatever those options do, I'd expect them to apply equally to > the tar files and to the files extracted with plain mode. Having those > be different really strikes me as very odd. Agreed for the consistency part, permissions should be applied consistently for the folder and the tar format. Having the option for pg_receivewal definitely makes sense to me, as it is the one in charge of opening and writing the WAL segments. For pg_basebackup, let's not forget that there is one tar file for each tablespace, and that each file is received separately using a COPY stream. There is some logic already which parses the tar header part of an individual file in order to look for recovery.conf (see ReceiveTarFile() in pg_basebackup.c). It would be possible to enforce grouping permissions when receiving each file, and this would be rather low-cost in performance I think. Honestly, my vote would go for having the permissions set correctly by the source server as this brings consistency to the whole experience without complicating the interface of pg_basebackup, and this also makes the footprint of this patch on pg_basebackup way lighter. -- Michael
Attachment
I have committed a basic pg_resetwal test suite as part of another patch, so please adjust accordingly. It looks to me like your pg_resetwal tests contain a bit of useless copy-and-paste. For example, you are not using use Config, nor $tempdir, and you run $node->stop right after $node->start. Please clean that up a bit, or comment, if appropriate. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Peter, On 3/23/18 10:36 AM, Peter Eisentraut wrote: > I have committed a basic pg_resetwal test suite as part of another > patch, so please adjust accordingly. > > It looks to me like your pg_resetwal tests contain a bit of useless > copy-and-paste. For example, you are not using use Config, nor > $tempdir, and you run $node->stop right after $node->start. Please > clean that up a bit, or comment, if appropriate. Yeah, definitely too much copy-paste going on. The various start/stops were intended the ensure that PG actually starts with the reset WAL. I see that these tests don't do them, though, so perhaps that's not a good use of test time. The pg_rewind tests work for my purposes but it seems worth preserving the ones I wrote since there is no overlap. I've attached a patch that integrates my tests with the current tests. If you don't think they are worth adding then I'll just drop them from my patchset. Thanks, -- -David david@pgmasters.net
Attachment
On Fri, Mar 23, 2018 at 12:26:47PM -0400, David Steele wrote: > I've attached a patch that integrates my tests with the current tests. > If you don't think they are worth adding then I'll just drop them from > my patchset. It seems to me that those tests have values (we can add more tests for transaction ID and such in the future), but I would recommend to put them in a separate file named like t/003_reset.pl for tests which execute resets and checks their consistency on the cluster. -- Michael
Attachment
On 3/20/18 11:14 PM, Michael Paquier wrote: > On Tue, Mar 20, 2018 at 05:44:22PM -0400, Stephen Frost wrote: >> * David Steele (david@pgmasters.net) wrote: >>> On 3/16/18 11:12 AM, Stephen Frost wrote: >>> It seems to me that pg_basebackup and pg_receivexlog should have a -g >>> option to control the mode of the files that they write to disk (not >>> including the modes stored in the tar files). >>> >>> Or perhaps we should just update the perms in the tar files for now and >>> leave the rest alone. >> >> Having options to pg_basebackup to control what's done makes sense to >> me- but whatever those options do, I'd expect them to apply equally to >> the tar files and to the files extracted with plain mode. Having those >> be different really strikes me as very odd. > > Agreed for the consistency part, permissions should be applied > consistently for the folder and the tar format. > > Having the option for pg_receivewal definitely makes sense to me, as it > is the one in charge of opening and writing the WAL segments. For > pg_basebackup, let's not forget that there is one tar file for each > tablespace, and that each file is received separately using a COPY > stream. There is some logic already which parses the tar header part of > an individual file in order to look for recovery.conf (see > ReceiveTarFile() in pg_basebackup.c). It would be possible to enforce > grouping permissions when receiving each file, and this would be rather > low-cost in performance I think. Honestly, my vote would go for having > the permissions set correctly by the source server as this brings > consistency to the whole experience without complicating the interface > of pg_basebackup, and this also makes the footprint of this patch on > pg_basebackup way lighter. These updates address Michael's latest review and implement group access for pg_basebackup, pg_receivewal, and pg_recvlogical. A new internal GUC, data_directory_group_access, allows remote processes to determine the correct mode using the existing SHOW protocol command. I have dropped patch 01, which added the pg_resetwal tests. The tests Peter added recently are sufficient for this patch so I'll pursue adding the other tests separately to avoid noise on this thread. Thanks, -- -David david@pgmasters.net
Attachment
On Tue, Mar 27, 2018 at 04:21:09PM -0400, David Steele wrote:
> These updates address Michael's latest review and implement group access
> for pg_basebackup, pg_receivewal, and pg_recvlogical. A new internal
> GUC, data_directory_group_access, allows remote processes to determine
> the correct mode using the existing SHOW protocol command.
Neat idea. This is another use case where the SHOW command in the
replication protocol proves to be useful.
> I have dropped patch 01, which added the pg_resetwal tests. The tests
> Peter added recently are sufficient for this patch so I'll pursue adding
> the other tests separately to avoid noise on this thread.
That's fair.
Some nits from patch 1...
+ ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
[...]
+ ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
Incorrect indentations (space after "ok", yes that's a nit...).
+ /* Set dir/file mode mask */
+ umask(PG_MODE_MASK_DEFAULT);
I would still see that rather committed as a separate patch. I am fine
to delegate the decision to the committer who is hopefully going to pick
up this patch.
And more things about patch 1 which are not nits.
In pg_backup_tar.c, we still have a 0600 hardcoded. You should use
PG_FILE_MODE_DEFAULT there as well.
dsm_impl_posix() can create a new segment with shm_open using as mode
0600. This is present in pg_dynshmem, which would be included in
backups. First, it seems to me that this should use
PG_FILE_MODE_DEFAULT. Then, with patch 2 it seems to me that if an
instance is using DSM then there is a risk of breaking a simple backup
which uses for example "tar" without --exclude filters with group access
(sometimes scripts are not smart enough to skip the same contents as
base backups). So it seems to me that DSM should be also made more
aware of group access to ease the life of users.
And then for patch 2, a couple of issues spotted..
+ /* for previous versions set the default group access */
+ if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_GROUP_ACCESS)
+ {
+ DataDirGroupAccess = false;
+ return false;
+ }
Enforcing DataDirGroupAccess to false for servers older than v11 is
fine, but RetrieveDataDirGroupAccess() should return true. If I read
your patch correctly then support for pg_basebackup on older server
would just fail.
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ if ((stat_buf.st_mode & PG_DIR_MODE_DEFAULT) == PG_DIR_MODE_DEFAULT)
+ {
+ umask(PG_MODE_MASK_ALLOW_GROUP);
+ DataDirGroupAccess = true;
+ }
This should use SetConfigOption() or I am missing something?
+/*
+ * Does the data directory allow group read access? The default is false, i.e.
+ * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750.
+ */
+bool DataDirGroupAccess = false;
Instead of a boolean, I would suggest to use an integer, this is more
consistent with log_file_mode. Actually, about that, should we complain
if log_file_mode is set to a value incompatible?
--
Michael
Attachment
On 3/28/18 1:59 AM, Michael Paquier wrote:
> On Tue, Mar 27, 2018 at 04:21:09PM -0400, David Steele wrote:
>> These updates address Michael's latest review and implement group access
>> for pg_basebackup, pg_receivewal, and pg_recvlogical. A new internal
>> GUC, data_directory_group_access, allows remote processes to determine
>> the correct mode using the existing SHOW protocol command.
>
> Some nits from patch 1...
>
> + ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
> [...]
> + ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
> Incorrect indentations (space after "ok", yes that's a nit...).
Fixed the space, not sure about the indentations?
> And more things about patch 1 which are not nits.
>
> In pg_backup_tar.c, we still have a 0600 hardcoded. You should use
> PG_FILE_MODE_DEFAULT there as well.
I'm starting to wonder if these changes in pg_dump make sense. The
file/tar permissions here do not map directly to anything in the PGDATA
directory (since the dump and restore are logical). Perhaps we should
be adding a -g option for pg_dump (in a separate patch) if we want this
functionality?
> dsm_impl_posix() can create a new segment with shm_open using as mode
> 0600. This is present in pg_dynshmem, which would be included in
> backups. First, it seems to me that this should use
> PG_FILE_MODE_DEFAULT. Then, with patch 2 it seems to me that if an
> instance is using DSM then there is a risk of breaking a simple backup
> which uses for example "tar" without --exclude filters with group access
> (sometimes scripts are not smart enough to skip the same contents as
> base backups). So it seems to me that DSM should be also made more
> aware of group access to ease the life of users.
Done in patch 1. For patch 2, do you see any issues with the shared
memory being group readable from a security perspective? The user can
read everything on disk so it's hard to see how it's a problem. Also,
if these files are ending up in people's backups...
> And then for patch 2, a couple of issues spotted..
>
> + /* for previous versions set the default group access */
> + if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_GROUP_ACCESS)
> + {
> + DataDirGroupAccess = false;
> + return false;
> + }
> Enforcing DataDirGroupAccess to false for servers older than v11 is
> fine, but RetrieveDataDirGroupAccess() should return true. If I read
> your patch correctly then support for pg_basebackup on older server
> would just fail.
You are correct, fixed.
> +#if !defined(WIN32) && !defined(__CYGWIN__)
> + if ((stat_buf.st_mode & PG_DIR_MODE_DEFAULT) == PG_DIR_MODE_DEFAULT)
> + {
> + umask(PG_MODE_MASK_ALLOW_GROUP);
> + DataDirGroupAccess = true;
> + }
> This should use SetConfigOption() or I am missing something?
Done.
> +/*
> + * Does the data directory allow group read access? The default is false, i.e.
> + * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750.
> + */
> +bool DataDirGroupAccess = false;
>
> Instead of a boolean, I would suggest to use an integer, this is more
> consistent with log_file_mode.
Well, the goal was to make this implementation independent, but I'm not
against the idea. Anybody have a preference?
> Actually, about that, should we complain
> if log_file_mode is set to a value incompatible?
I think control of the log file mode should be independent. I usually
don't store log files in PGDATA at all. What if we set log_file_mode
based on the -g option passed to initdb? That will work well for
default installations while providing flexibility to others.
Thanks,
--
-David
david@pgmasters.net
Attachment
On Thu, Mar 29, 2018 at 10:59:59AM -0400, David Steele wrote: > On 3/28/18 1:59 AM, Michael Paquier wrote: >> On Tue, Mar 27, 2018 at 04:21:09PM -0400, David Steele wrote: >>> These updates address Michael's latest review and implement group access >>> for pg_basebackup, pg_receivewal, and pg_recvlogical. A new internal >>> GUC, data_directory_group_access, allows remote processes to determine >>> the correct mode using the existing SHOW protocol command. >> >> Some nits from patch 1... >> >> + ok (check_mode_recursive($node_master->data_dir(), 0700, 0600)); >> [...] >> + ok (check_mode_recursive($node_master->data_dir(), 0700, 0600)); >> Incorrect indentations (space after "ok", yes that's a nit...). > > Fixed the space, not sure about the indentations? That was only the space issues. A nit. >> And more things about patch 1 which are not nits. >> >> In pg_backup_tar.c, we still have a 0600 hardcoded. You should use >> PG_FILE_MODE_DEFAULT there as well. > > I'm starting to wonder if these changes in pg_dump make sense. The > file/tar permissions here do not map directly to anything in the PGDATA > directory (since the dump and restore are logical). Perhaps we should > be adding a -g option for pg_dump (in a separate patch) if we want this > functionality? Yeah. I am having second thoughts on this one actually. pg_dump handles logical backups which require just a connection to Postgres and it does not care about the physical state of the relation files. So I am dropping my comment, and let's not bother about changing things here. >> dsm_impl_posix() can create a new segment with shm_open using as mode >> 0600. This is present in pg_dynshmem, which would be included in >> backups. First, it seems to me that this should use >> PG_FILE_MODE_DEFAULT. Then, with patch 2 it seems to me that if an >> instance is using DSM then there is a risk of breaking a simple backup >> which uses for example "tar" without --exclude filters with group access >> (sometimes scripts are not smart enough to skip the same contents as >> base backups). So it seems to me that DSM should be also made more >> aware of group access to ease the life of users. > > Done in patch 1. For patch 2, do you see any issues with the shared > memory being group readable from a security perspective? The user can > read everything on disk so it's hard to see how it's a problem. Also, > if these files are ending up in people's backups... They would be nuked from the surface of earth when recovery kicks. People should filter this folder, which is why any popular Postgres backup tool I believe does so, and now so do both pg_rewind and pg_basebackup. Still if a user is able to read everything, being able to read as well pg_dynshmem does not change much from a security's point of view. So consistency makes the most sense to me. >> +/* >> + * Does the data directory allow group read access? The default is false, i.e. >> + * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750. >> + */ >> +bool DataDirGroupAccess = false; >> >> Instead of a boolean, I would suggest to use an integer, this is more >> consistent with log_file_mode. > > Well, the goal was to make this implementation independent, but I'm not > against the idea. Anybody have a preference? You mean Windows here? Perhaps you are right and just using a boolean would be fine. When commenting on that I have been likely wondering about potential extensions in the future, like allowing a user to write files in a data folder as well if member of the same group as the user managing the Postgres intance, like for backup deployment? Perhaps that's just a crazy man's thoughts.. >> Actually, about that, should we complain >> if log_file_mode is set to a value incompatible? > > I think control of the log file mode should be independent. I usually > don't store log files in PGDATA at all. What if we set log_file_mode > based on the -g option passed to initdb? That will work well for > default installations while providing flexibility to others. Let's do nothing here as well. This will keep the code more simple, and normally-sane deployments put the log directory in an absolute path out of the data folder. Normally... So it means that if I create a number out of thin air I would imagine that less than 20% of deployments are like that. -- Michael
Attachment
On 3/29/18 11:04 PM, Michael Paquier wrote: > On Thu, Mar 29, 2018 at 10:59:59AM -0400, David Steele wrote: >> On 3/28/18 1:59 AM, Michael Paquier wrote: >> >>> In pg_backup_tar.c, we still have a 0600 hardcoded. You should use >>> PG_FILE_MODE_DEFAULT there as well. >> >> I'm starting to wonder if these changes in pg_dump make sense. The >> file/tar permissions here do not map directly to anything in the PGDATA >> directory (since the dump and restore are logical). Perhaps we should >> be adding a -g option for pg_dump (in a separate patch) if we want this >> functionality? > > Yeah. I am having second thoughts on this one actually. pg_dump > handles logical backups which require just a connection to Postgres and > it does not care about the physical state of the relation files. So I > am dropping my comment, and let's not bother about changing things > here. Glad you agree. I have reverted the mode changes in pg_dump. >>> dsm_impl_posix() can create a new segment with shm_open using as mode >>> 0600. This is present in pg_dynshmem, which would be included in >>> backups. First, it seems to me that this should use >>> PG_FILE_MODE_DEFAULT. Then, with patch 2 it seems to me that if an >>> instance is using DSM then there is a risk of breaking a simple backup >>> which uses for example "tar" without --exclude filters with group access >>> (sometimes scripts are not smart enough to skip the same contents as >>> base backups). So it seems to me that DSM should be also made more >>> aware of group access to ease the life of users. >> >> Done in patch 1. For patch 2, do you see any issues with the shared >> memory being group readable from a security perspective? The user can >> read everything on disk so it's hard to see how it's a problem. Also, >> if these files are ending up in people's backups... > > They would be nuked from the surface of earth when recovery kicks. > People should filter this folder, which is why any popular Postgres > backup tool I believe does so, and now so do both pg_rewind and > pg_basebackup. Still if a user is able to read everything, being able > to read as well pg_dynshmem does not change much from a security's point > of view. So consistency makes the most sense to me. Done. >>> +/* >>> + * Does the data directory allow group read access? The default is false, i.e. >>> + * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750. >>> + */ >>> +bool DataDirGroupAccess = false; >>> >>> Instead of a boolean, I would suggest to use an integer, this is more >>> consistent with log_file_mode. >> >> Well, the goal was to make this implementation independent, but I'm not >> against the idea. Anybody have a preference? > > You mean Windows here? Perhaps you are right and just using a boolean > would be fine. When commenting on that I have been likely wondering > about potential extensions in the future, like allowing a user to write > files in a data folder as well if member of the same group as the user > managing the Postgres intance, like for backup deployment? Perhaps > that's just a crazy man's thoughts.. Haha! But I think you are right that using the mode is more consistent with how other GUCs are done. It hardly makes sense to take a stand on unix-y things here when we use them in other GUCs already. I have replaced data_directory_group_access with data_directory_mode. >>> Actually, about that, should we complain >>> if log_file_mode is set to a value incompatible? >> >> I think control of the log file mode should be independent. I usually >> don't store log files in PGDATA at all. What if we set log_file_mode >> based on the -g option passed to initdb? That will work well for >> default installations while providing flexibility to others. > > Let's do nothing here as well. This will keep the code more simple, and > normally-sane deployments put the log directory in an absolute path out > of the data folder. Normally... So it means that if I create a number > out of thin air I would imagine that less than 20% of deployments are > like that. I decided this made sense to do. It was only a few lines in initdb.c using a very well established pattern. It would be surprising if log files did not follow the mode of the rest of PGDATA after initdb -g, even if it is standard practice to relocate them. Thanks, -- -David david@pgmasters.net
Attachment
On Fri, Mar 30, 2018 at 01:27:11PM -0400, David Steele wrote:
> I have replaced data_directory_group_access with data_directory_mode.
That looks way better. Thanks for considering it.
> I decided this made sense to do. It was only a few lines in initdb.c
> using a very well established pattern. It would be surprising if log
> files did not follow the mode of the rest of PGDATA after initdb -g,
> even if it is standard practice to relocate them.
Okay for me.
@@ -285,7 +286,7 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
* returning.
*/
flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0);
- if ((fd = shm_open(name, flags, 0600)) == -1)
+ if ((fd = shm_open(name, flags, PG_FILE_MODE_DEFAULT)) == -1)
Hm. Sorry for the extra noise. This one is actually incorrect as
shm_open will use the path specified by glibc, which is out of
pg_dynshmem so it does not matter for base backups, and we can keep
0600. pg_dymshmem is used for mmap, still this would map with the umask
setup by the postmaster as OpenTransientFile & friends are used. sysv
uses IPCProtection but there is no need to care about it as well. No
need for a comment perhaps..
pg_basebackup.c creates recovery.conf with 0600 all the time ;)
Except for those two nits, I am fine to pass down to a committer patch
number 1. This has value in my opinion per the refactoring it does and
the umask handling of pg_rewind and pg_resetwal added.
Now for patch 2...
+ <para>
+ If the data directory allows group read access then certificate files may
+ need to be located outside of the data directory in order to conform to the
+ security requirements outlined above. Generally, group access is enabled
+ to allow an unprivileged user to backup the database, and in that case the
+ backup software will not be able to read the certificate files and will
+ likely error.
+ </para
The answer is no for me and likely the same for others, but I am raising
the point for the archives. Should we relax
check_ssl_key_file_permissions() for group permissions by the way from
0600 to 0640 when the file is owned by the user running Postgres? If we
don't do that, then SSL private keys will need to be used with 0600,
potentially breaking backups... At the same time this reduces the
security of private keys but if the administrator is ready to accept
group permissions that should be a risk he is ready to accept?
+ &DataDirMode,
+ 0700, 0000, 0777,
+ NULL, NULL, show_data_directory_mode
Instead of a camel case, renaming that to data_directory_mode would be
nice to ease future greps. I do that all the time for existing code,
pesting when things are not consistent. A nit.
There is a noise diff in miscinit.c.
I am pretty sure that we want more documentation in pg_basebackup,
pg_rewind and pg_resetwal telling that those consider grouping access.
There is no need to include sys/stat.h as this is already part of
file_perm.h as DataDirectoryMask uses mode_t for its definition.
+/*
+ * Mode of the data directory. The default is 0700 but may it be changed in
+ * checkDataDir() to 0750 if the data directory actually has that mode.
+ */
+int DataDirMode = PG_DIR_MODE_NOGROUP
/*
- * Default mode for directories.
+ * Default mode for created files, unless something else is specified using
+ * the *Perm() function variants.
*/
-#define PG_DIR_MODE_DEFAULT S_IRWXU
+#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP)
To reduce the code churn and simplify the logic, I would recommend to
not use variables which have a negative meaning, so PG_DIR_MODE_DEFAULT
should remain the same in patch 2, and there should be PG_DIR_MODE_GROUP
instead of PG_DIR_MODE_NOGROUP. That would be more consistent with the
file modes as well.
Yes, we can.
--
Michael
Attachment
Hi Michael, On 4/2/18 2:28 AM, Michael Paquier wrote: > > @@ -285,7 +286,7 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size, > * returning. > */ > flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0); > - if ((fd = shm_open(name, flags, 0600)) == -1) > + if ((fd = shm_open(name, flags, PG_FILE_MODE_DEFAULT)) == -1) > > Hm. Sorry for the extra noise. This one is actually incorrect as > shm_open will use the path specified by glibc, which is out of > pg_dynshmem so it does not matter for base backups, and we can keep > 0600. pg_dymshmem is used for mmap, still this would map with the umask > setup by the postmaster as OpenTransientFile & friends are used. sysv > uses IPCProtection but there is no need to care about it as well. No > need for a comment perhaps.. Yeah, I think I figured that out when I first went through the code but managed to forget it. Reverted. > pg_basebackup.c creates recovery.conf with 0600 all the time ;) Fixed. > + <para> > + If the data directory allows group read access then certificate files may > + need to be located outside of the data directory in order to conform to the > + security requirements outlined above. Generally, group access is enabled > + to allow an unprivileged user to backup the database, and in that case the > + backup software will not be able to read the certificate files and will > + likely error. > + </para > The answer is no for me and likely the same for others, but I am raising > the point for the archives. Should we relax > check_ssl_key_file_permissions() for group permissions by the way from > 0600 to 0640 when the file is owned by the user running Postgres? If we > don't do that, then SSL private keys will need to be used with 0600, > potentially breaking backups... At the same time this reduces the > security of private keys but if the administrator is ready to accept > group permissions that should be a risk he is ready to accept? I feel this should be considered in a separate patch. These files are not created by initdb so it seems to be an admin/packaging issue. There is always the option to locate the certs outside the data directory and some distros do that by default. > + &DataDirMode, > + 0700, 0000, 0777, > + NULL, NULL, show_data_directory_mode > Instead of a camel case, renaming that to data_directory_mode would be > nice to ease future greps. I do that all the time for existing code, > pesting when things are not consistent. A nit. Done. > There is a noise diff in miscinit.c. Fixed. > I am pretty sure that we want more documentation in pg_basebackup, > pg_rewind and pg_resetwal telling that those consider grouping access. I think this makes sense for pg_basebackup, pg_receivewal, and pg_recvlogical so I have added notes for those. Not sure I'm happy with the language but at least we have something to bikeshed. It seems to me that pg_resetwal and pg_rewind should be expected to not break the permissions in PGDATA, just as they do now. > There is no need to include sys/stat.h as this is already part of > file_perm.h as DataDirectoryMask uses mode_t for its definition. I removed it from file_perm.h instead. With the new variables (see below), most call sites will have no need for the mode constants. > +/* > + * Mode of the data directory. The default is 0700 but may it be changed in > + * checkDataDir() to 0750 if the data directory actually has that mode. > + */ > +int DataDirMode = PG_DIR_MODE_NOGROUP > > /* > - * Default mode for directories. > + * Default mode for created files, unless something else is specified using > + * the *Perm() function variants. > */ > -#define PG_DIR_MODE_DEFAULT S_IRWXU > +#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP) > To reduce the code churn and simplify the logic, I would recommend to > not use variables which have a negative meaning, so PG_DIR_MODE_DEFAULT > should remain the same in patch 2, and there should be PG_DIR_MODE_GROUP > instead of PG_DIR_MODE_NOGROUP. That would be more consistent with the > file modes as well. I decided that the constants were a bit confusing in general. What does "default" mean, anyway? Instead I have created variables in file_perm.c that hold the current file create mode, dir create mode, and mode mask. All call sites use those variables (e.g. pg_dir_create_mode), which I think are much clear. This causes a bit of churn with the constants we added last September but those were added for v11 so it won't create more complications for back-patching. > Yes, we can. Yes! We can! New patches attached. Thanks, -- -David david@pgmasters.net
Attachment
On Wed, Apr 04, 2018 at 08:03:54PM -0400, David Steele wrote: > On 4/2/18 2:28 AM, Michael Paquier wrote: >> The answer is no for me and likely the same for others, but I am raising >> the point for the archives. Should we relax >> check_ssl_key_file_permissions() for group permissions by the way from >> 0600 to 0640 when the file is owned by the user running Postgres? If we >> don't do that, then SSL private keys will need to be used with 0600, >> potentially breaking backups... At the same time this reduces the >> security of private keys but if the administrator is ready to accept >> group permissions that should be a risk he is ready to accept? > > I feel this should be considered in a separate patch. These files are > not created by initdb so it seems to be an admin/packaging issue. There > is always the option to locate the certs outside the data directory and > some distros do that by default. OK, I agree with your final position. Let's treat it later in a separate thread, if need be. However this is not really mandatory. >> I am pretty sure that we want more documentation in pg_basebackup, >> pg_rewind and pg_resetwal telling that those consider grouping access. > > I think this makes sense for pg_basebackup, pg_receivewal, and > pg_recvlogical so I have added notes for those. Not sure I'm happy with > the language but at least we have something to bikeshed. > > It seems to me that pg_resetwal and pg_rewind should be expected to not > break the permissions in PGDATA, just as they do now. OK, I think that I am fine with this logic. > I decided that the constants were a bit confusing in general. What does > "default" mean, anyway? The value that user should have if he decides to not enforce it. > Instead I have created variables in file_perm.c > that hold the current file create mode, dir create mode, and mode mask. > All call sites use those variables (e.g. pg_dir_create_mode), which I > think are much clear. Hm. I personally find that even more confusing, especially with SetDataDirectoryCreatePerm which basically is the same as SetConfigOption for the backend. In your case pg_dir_create_mode is not aimed at remaining a constant as you may change it depending on if grouping is enabled or not... And all the other iterations of such variables in src/common/ are constants (see NumScanKeywords or forkNames[] for example), so I cannot see with a good eye this change. You also forgot a call to SetDataDirectoryCreatePerm or pg_dir_create_mode remains to its default. The interface of file_perm.h that you are introducing is not confusing anymore though.. -- Michael
Attachment
Hi Michael, On 4/5/18 2:55 AM, Michael Paquier wrote: > On Wed, Apr 04, 2018 at 08:03:54PM -0400, David Steele wrote: > >> Instead I have created variables in file_perm.c >> that hold the current file create mode, dir create mode, and mode mask. >> All call sites use those variables (e.g. pg_dir_create_mode), which I >> think are much clear. > > Hm. I personally find that even more confusing, especially with > SetDataDirectoryCreatePerm which basically is the same as > SetConfigOption for the backend. The GUC shows the current mode of the data directory, while the variables in file_perm.c store the mode that should be used to create new dirs/files. One is certainly based on the other but I thought it best to split them for clarity. > In your case pg_dir_create_mode is not > aimed at remaining a constant as you may change it depending on if > grouping is enabled or not... And all the other iterations of such > variables in src/common/ are constants (see NumScanKeywords or > forkNames[] for example), so I cannot see with a good eye this change. The idea is to have a variable that give the fir dir/file create mode without having to trust that umask() is set correctly or do mode masking on chmod(). This makes a number of call sites simpler and, I hope, easier to read. > You also forgot a call to SetDataDirectoryCreatePerm or > pg_dir_create_mode remains to its default. Are saying *if* a call is forgotten? Yes, the file/dir create modes will use the default restrictive permissions unless set otherwise. I see this as a good thing. Worst case, we miss some areas where group access should be enabled and we find those over the next few months. What we don't want is a regression in the current, default behavior. This is design is intended to avoid that outcome. > The interface of file_perm.h that you are introducing is not confusing > anymore though.. Yes, that was the idea. I think it makes it clearer what we are trying to do and centralizes variables related to create modes in one place. I've attached new patches that exclude Windows from permissions tests and deal with bootstrap permissions in a better way. PostgresMain() and AuxiliaryProcessMain() now use checkDataDir() to set permissions. Thanks! -- -David david@pgmasters.net
Attachment
On Thu, Apr 05, 2018 at 12:08:15PM -0400, David Steele wrote: > On 4/5/18 2:55 AM, Michael Paquier wrote: >> On Wed, Apr 04, 2018 at 08:03:54PM -0400, David Steele wrote: >> >>> Instead I have created variables in file_perm.c >>> that hold the current file create mode, dir create mode, and mode mask. >>> All call sites use those variables (e.g. pg_dir_create_mode), which I >>> think are much clear. >> >> Hm. I personally find that even more confusing, especially with >> SetDataDirectoryCreatePerm which basically is the same as >> SetConfigOption for the backend. > > The GUC shows the current mode of the data directory, while the > variables in file_perm.c store the mode that should be used to create > new dirs/files. One is certainly based on the other but I thought it > best to split them for clarity. Yeah, there are arguments to actually have both things split so as they can be concatenated. This makes the code more modular. >> You also forgot a call to SetDataDirectoryCreatePerm or >> pg_dir_create_mode remains to its default. My apologies, I forgot the last two words of this sentence: "for pg_rewind". Still, I am wrong because GetDataDirectoryCreatePerm calls internally SetDataDirectoryCreatePerm. So the API name is confusing because not only the permissions are fetched, but they are also applied. > Are saying *if* a call is forgotten? That applies as well. >> The interface of file_perm.h that you are introducing is not confusing >> anymore though.. > > Yes, that was the idea. I think it makes it clearer what we are trying > to do and centralizes variables related to create modes in one place. > > I've attached new patches that exclude Windows from permissions tests > and deal with bootstrap permissions in a better way. PostgresMain() and > AuxiliaryProcessMain() now use checkDataDir() to set permissions. GetDataDirectoryCreatePerm() is defined in file_perm.h but it is only declared for frontends. At the end, this patch brings in a new abstraction concept to src/common/ to control a set of pre-determined behaviors: - The mode to create directories. - The mode to create files. - The mode number which can be used for umask. I am not really convinced that we need to enforce all of them all the time with a API layer aimed at controlling the behavior of all things at the same time. Getting this abstraction down one level by allowing each frontend to set up things the way they want would be nicer in my opinion. Perhaps others feel differently... It could be also less confusing if the set of variables in file_perm.h was designed in such a way that we have the default, and then we can apply on top of it the set to allow grouping, so as allowing grouping access would be to do the operation of (default mask + group addition). The design on the backend is rather neat however there is also overlapping with SetDataDirectoryCreatePerm() and the GUC data_directory_mode which are heading toward the same types of goals. We could reduce that confusion by designing the interface as follows: - Have read-only GUCs for the directory and file masks on the backend which get set by the postmaster after looking at the permission of the data folder at startup. Then if a file or a folder needs to be created, then look at those values and apply permissions as granted. And also a GUC to decide the umask to apply but that should not be necessary, right? - Frontends are responsible for the decision-making of the permissions. Not all the variables are used for frontends as well. For example pg_resetwal and pg_upgrade only touch files. - For frontends, there are two cases: -- The client needs to connect to a live Postgres instance, in which case it can use the SHOW command to get the wanted information. This applies to pg_rewind with the remote mode (should apply to the second case actually), pg_basebackup, pg_receivexlog, etc. -- Binaries work on a local data folder, so permissions can be guessed from that: pg_rewind, pg_resetwal and pg_upgrade. Having an API in src/common/ which scans for what to apply is neat. This was in v12 and some older versions if I recall correctly. We are two days away from the end of the commit fest, and this patch set if not yet in a committable stagle, so perhaps at this stage we had better just retreat and come back to it in next cycle? -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Thu, Apr 05, 2018 at 12:08:15PM -0400, David Steele wrote: > > On 4/5/18 2:55 AM, Michael Paquier wrote: > >> On Wed, Apr 04, 2018 at 08:03:54PM -0400, David Steele wrote: > >> > >>> Instead I have created variables in file_perm.c > >>> that hold the current file create mode, dir create mode, and mode mask. > >>> All call sites use those variables (e.g. pg_dir_create_mode), which I > >>> think are much clear. > >> > >> Hm. I personally find that even more confusing, especially with > >> SetDataDirectoryCreatePerm which basically is the same as > >> SetConfigOption for the backend. > > > > The GUC shows the current mode of the data directory, while the > > variables in file_perm.c store the mode that should be used to create > > new dirs/files. One is certainly based on the other but I thought it > > best to split them for clarity. > > Yeah, there are arguments to actually have both things split so as they > can be concatenated. This makes the code more modular. I prefer having them split as well. > >> You also forgot a call to SetDataDirectoryCreatePerm or > >> pg_dir_create_mode remains to its default. > > My apologies, I forgot the last two words of this sentence: "for > pg_rewind". Still, I am wrong because GetDataDirectoryCreatePerm calls > internally SetDataDirectoryCreatePerm. So the API name is confusing > because not only the permissions are fetched, but they are also > applied. They're not *actually* applied though- that just sets the variable, and having GetDataDirectoryCreatePerm get what the perm is and then set a variable based off of that, so we know what it's set to later, seems reasonable to me. > >> The interface of file_perm.h that you are introducing is not confusing > >> anymore though.. > > > > Yes, that was the idea. I think it makes it clearer what we are trying > > to do and centralizes variables related to create modes in one place. > > > > I've attached new patches that exclude Windows from permissions tests > > and deal with bootstrap permissions in a better way. PostgresMain() and > > AuxiliaryProcessMain() now use checkDataDir() to set permissions. > > GetDataDirectoryCreatePerm() is defined in file_perm.h but it is only > declared for frontends. Right, the job that GetDataDirectoryCreatePerm() has in the front-ends is more complicated when the backend is starting up because we're also checking who the data directory is owned by and we're setting the internal GUC, all of which is done by checkDataDir(), which also calls SetDataDirectoryCreatePerm() to set the variables. > At the end, this patch brings in a new abstraction concept to > src/common/ to control a set of pre-determined behaviors: > - The mode to create directories. > - The mode to create files. > - The mode number which can be used for umask. > I am not really convinced that we need to enforce all of them all the > time with a API layer aimed at controlling the behavior of all things at > the same time. Getting this abstraction down one level by allowing each > frontend to set up things the way they want would be nicer in my > opinion. Perhaps others feel differently... I don't think we actually *want* the various tools making different decisions about what permissions the files in a given data directory have- that's initdb's job, not the job of pg_resetwal. > It could be also less confusing if the set of variables in file_perm.h > was designed in such a way that we have the default, and then we can > apply on top of it the set to allow grouping, so as allowing grouping > access would be to do the operation of (default mask + group addition). This seems like we're going around-and-around. We had all the different masking things happening previously, but that got rather ugly as there were just small variations between "right" and "wrong". I like this approach which makes it pretty clear that when we start up, we figure out what the perms should be for files in this data dir, and then we set the variables for the permissions and use them. > The design on the backend is rather neat however there is also > overlapping with SetDataDirectoryCreatePerm() and the GUC > data_directory_mode which are heading toward the same types of goals. Just above we discuss how we want those to be seperate, and here it seems you're asking for them to be put together. We really can't have it both ways in this case and I tend to agree with the above discussion where we keep them seperate. > We could reduce that confusion by designing the interface as follows: > - Have read-only GUCs for the directory and file masks on the backend > which get set by the postmaster after looking at the permission of the > data folder at startup. Then if a file or a folder needs to be created, > then look at those values and apply permissions as granted. And also a > GUC to decide the umask to apply but that should not be necessary, > right? This is more-or-less what we've got now- a read-only GUC for the directory and file masks on the backend which gets set by the postmaster after looking at the permission of the data folder on startup- that all happens in checkDataDir() now, which looks good to me. We then have the variables set by SetDataDirectoryCreatePerm() which happens just before the GUC is set in the same area of checkDataDir(). We need to set them both if we're going to keep them seperate, which I believe is what we really want here. > - Frontends are responsible for the decision-making of the permissions. No. Frontends need to follow what the permissions are on the data directory- having them decide independently will just lead to things being inconsistent and that's definitely not something that we want. > Not all the variables are used for frontends as well. For example > pg_resetwal and pg_upgrade only touch files. While true today, I don't think it's an issue and I'd rather keep the directory and file privilege settings in the same place (where else would you put the directory ones?). > - For frontends, there are two cases: > -- The client needs to connect to a live Postgres instance, in which > case it can use the SHOW command to get the wanted information. This > applies to pg_rewind with the remote mode (should apply to the second > case actually), pg_basebackup, pg_receivexlog, etc. Right, that's what pg_basebackup, pg_receivewal, etc, do now with this patch.. > -- Binaries work on a local data folder, so permissions can be guessed > from that: pg_rewind, pg_resetwal and pg_upgrade. Having an API in > src/common/ which scans for what to apply is neat. This was in v12 and > some older versions if I recall correctly. I'm confused here as that's what GetDataDirectoryCreatePerm() specifically does and that's why it's in src/common now for the front-end tools to use, or were you just agreeing that this was in v12 previously and you're happy to see that it's back..? > We are two days away from the end of the commit fest, and this patch set > if not yet in a committable stagle, so perhaps at this stage we had > better just retreat and come back to it in next cycle? I've been watching this and discussing things with David while you and he have been working on it and I think the discussion has generally been good, so I didn't want to step in and disrupt it, but there's a few things here which now seem to be going in circles without a lot of benefit. There's a few comments which I have on the patch after going over it again, but I tend to feel it's actually pretty close as none of the comments I have at this point are serious issues- you and David have addressed those (I'm *very* glad that the pg_dump changes were ripped out, for instance, and having the some of the regression tests committed independently was certainly good...) and I don't see any of the above as being really concerning, but do let me know if you think there's something I'm missing or some other issue. I'll reply to David's last email (where the latest set of patches were included) with my comments/suggestions and I expect we'll be able to get those addressed today and have a final patch to post tonight, with an eye towards committing it tomorrow. Thanks! Stephen
Attachment
On Fri, Apr 06, 2018 at 09:15:15AM -0400, Stephen Frost wrote: > I'll reply to David's last email (where the latest set of patches were > included) with my comments/suggestions and I expect we'll be able to get > those addressed today and have a final patch to post tonight, with an > eye towards committing it tomorrow. The feature freeze is on the 8th, so I am going to have limited room to comment on things until that day. If something gets committed, I am pretty sure that I'll get out of my pocket a couple of things to improve the feature and its interface anyway if of course you are ready to accept that. I have limited my role to be a reviewer, so I refrained myself from writing any code ;) -- Michael
Attachment
David, * David Steele (david@pgmasters.net) wrote: > The GUC shows the current mode of the data directory, while the > variables in file_perm.c store the mode that should be used to create > new dirs/files. One is certainly based on the other but I thought it > best to split them for clarity. Agreed. I did have some other comments about the patches tho- > > You also forgot a call to SetDataDirectoryCreatePerm or > > pg_dir_create_mode remains to its default. > > Are saying *if* a call is forgotten? > > Yes, the file/dir create modes will use the default restrictive > permissions unless set otherwise. I see this as a good thing. Worst > case, we miss some areas where group access should be enabled and we > find those over the next few months. I tend to agree with this, though the space of concern is quite limited- basically between the top of PostmasterMain() and when it calls checkDataDir(), and we really shouldn't be creating any files before we've check that the data dir looks sane. > > The interface of file_perm.h that you are introducing is not confusing > > anymore though.. > > Yes, that was the idea. I think it makes it clearer what we are trying > to do and centralizes variables related to create modes in one place. > > I've attached new patches that exclude Windows from permissions tests > and deal with bootstrap permissions in a better way. PostgresMain() and > AuxiliaryProcessMain() now use checkDataDir() to set permissions. Alright, changes I've made, since I got impatient and it didn't seem to make sense to bounce these back to David instead of just making them (I did discuss them with him on the phone today tho, just to be clear). - MakeDirectoryDefaultPerm() was quite a mouthful, so I've simplified that down to MakePGDirectory(), which I hope is clear in that it's for making directories related to PG (as it isn't a general mkdir() call or just some platform-agnostic mkdir(), which MakeDirectory() might imply, but is specific for working with PG data directories). - The PG_FILE_MODE_DEFAULT, PG_DIR_MODE_DEFAULT, et al, were confusing. Those constants are used for the default file/dir mode, sure, but what that actually *are* is the 'owner'-only style mode, so they've been changed to PG_FILE_MODE_OWNER, PG_DIR_MODE_OWNER, etc. - Where we're intentionally ignoring the MakePGDirectory() result, use (void). - Various comment improvements. - Improved the commit messages a bit. Things that still need doing: - Go back over with pgindent and make sure it's all happy. - Improved documentation - Likely more comments (I don't think I can ever have enough) - Further discussion in the commit messages - Perhaps a bit more review to try to minimize the risk that something breaks on Windows... Looked ok to me, but probably still some risk that the Windows buildfarm members fall over, though I suppose that's also what they're there for. Updated patch attached. David, if you could look through this again and make sure I didn't break anything with the changes made, and perhaps make improvements to the docs/comments/commit messages, that'd be helpful for getting this over the line. Thanks! Stephen
Attachment
Michael, * Michael Paquier (michael@paquier.xyz) wrote: > On Fri, Apr 06, 2018 at 09:15:15AM -0400, Stephen Frost wrote: > > I'll reply to David's last email (where the latest set of patches were > > included) with my comments/suggestions and I expect we'll be able to get > > those addressed today and have a final patch to post tonight, with an > > eye towards committing it tomorrow. > > The feature freeze is on the 8th, so I am going to have limited room to > comment on things until that day. If something gets committed, I am > pretty sure that I'll get out of my pocket a couple of things to improve > the feature and its interface anyway if of course you are ready to > accept that. I have limited my role to be a reviewer, so I refrained > myself from writing any code ;) I'm, of course, happy to accept any further comments or suggestions on it, pre-commit, post-commit, next year, whenever. ;) I've posted an updated patch with comments and next steps, as discussed, which I'm hopeful that David will have a chance to review and comment on, at least, and perhaps help wrap up those last items. This patch has been quite a slog, so thank you again for all of your help reviewing it and moving it forward, it's been really appreciated. Thanks! Stephen
Attachment
Hi Stephen, On 4/6/18 3:02 PM, Stephen Frost wrote: > > Alright, changes I've made, since I got impatient and it didn't seem to > make sense to bounce these back to David instead of just making them (I > did discuss them with him on the phone today tho, just to be clear). > > - The PG_FILE_MODE_DEFAULT, PG_DIR_MODE_DEFAULT, et al, were confusing. > Those constants are used for the default file/dir mode, sure, but what > that actually *are* is the 'owner'-only style mode, so they've been > changed to PG_FILE_MODE_OWNER, PG_DIR_MODE_OWNER, etc. This is definitely better. There were a few missed replacements in 01 so I fixed those. > Things that still need doing: > > - Further discussion in the commit messages Agreed, these need some more work. I'm happy to do that but I'll need a bit more time. Have a look at the new patches and I'll work on some better messages. > - Perhaps a bit more review to try to minimize the risk that something > breaks on Windows... Looked ok to me, but probably still some risk > that the Windows buildfarm members fall over, though I suppose that's > also what they're there for. I did my best on this based on breakage from some of my other patches. > David, if you could look through this again and make sure I didn't break > anything with the changes made, and perhaps make improvements to the > docs/comments/commit messages, that'd be helpful for getting this over > the line. I'm pretty happy with it overall. As you say, there could always be more comments, but I'm not sure what to add without just running on. New patches attached. -- -David david@pgmasters.net
Attachment
On 4/6/18 6:04 PM, David Steele wrote: > On 4/6/18 3:02 PM, Stephen Frost wrote: >> >> - Further discussion in the commit messages > > Agreed, these need some more work. I'm happy to do that but I'll need a > bit more time. Have a look at the new patches and I'll work on some > better messages. I'm sure you'll want to reword some things, but I think these commit messages capture the essential changes for each patch. 01: Refactor file permissions in backend/frontend Consolidate directory and file create permissions by adding a new module (common/file_perm.c) that contains variables (pg_file_create_mode, pg_dir_create_mode) and constants to initialize them (0600 for files and 0700 for directories). Convert mkdir() calls in the backend to MakePGDirectory() if the original call used default permissions (always the case for regular PG directories). Add tests to make sure permissions in PGDATA are set correctly by the front-end tools. Author: David Steele <david@pgmasters.net> Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net 02: Allow group access on PGDATA Allow the cluster to be optionally init'd with read access for the group. This means a relatively non-privileged user can perform a backup of the cluster without requiring write privileges, which enhances security. The mode of PGDATA is used to determine whether group permissions are enabled for directory and file creates. This method was chosen because there are a number of front-end utilities that write into PGDATA but not all of them read pg_control and none of them load GUCS. Changing the mode of PGDATA manually will not automatically change the mode of all the files contained therein. If the user would like to enable group access on an existing cluster then changing the mode of the existing files will be required. Note that pg_upgrade will automatically change the mode of all migrated files if the new cluster is init'd with the -g option. Tests are included for the backend and all front-end utilities to ensure that the correct mode is set based on the PGDATA permissions. Author: David Steele <david@pgmasters.net> Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/https://www.postgresql.org/message-id/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net Thanks! -- -David david@pgmasters.net
David, * David Steele (david@pgmasters.net) wrote: > On 4/6/18 6:04 PM, David Steele wrote: > >On 4/6/18 3:02 PM, Stephen Frost wrote: > >> > >>- Further discussion in the commit messages > > > >Agreed, these need some more work. I'm happy to do that but I'll need a > >bit more time. Have a look at the new patches and I'll work on some > >better messages. > > I'm sure you'll want to reword some things, but I think these commit > messages capture the essential changes for each patch. Thanks much. I've taken (most) of these, adjusting a few bits here and there. I've been back over the patch again, mostly improving the commit messages, comments, and docs. I also looked over the code and tests again and they're looking pretty good to me, so I'll be looking to commit this tomorrow afternoon or so US/Eastern. Attached is v18. Thanks! Stephen
Attachment
Hi Stephen, On 4/6/18 10:22 PM, Stephen Frost wrote: > > * David Steele (david@pgmasters.net) wrote: >> On 4/6/18 6:04 PM, David Steele wrote: >>> On 4/6/18 3:02 PM, Stephen Frost wrote: >>>> >>>> - Further discussion in the commit messages >>> >>> Agreed, these need some more work. I'm happy to do that but I'll need a >>> bit more time. Have a look at the new patches and I'll work on some >>> better messages. >> >> I'm sure you'll want to reword some things, but I think these commit >> messages capture the essential changes for each patch. > > Thanks much. I've taken (most) of these, adjusting a few bits here and > there. > > I've been back over the patch again, mostly improving the commit > messages, comments, and docs. I also looked over the code and tests > again and they're looking pretty good to me, so I'll be looking to > commit this tomorrow afternoon or so US/Eastern. OK, one last review. I did't make any code changes, but I improved some comments, added documentation and fixed a test. 01) Refactor dir/file permissions * Small comment update, replace "such cases-" with "such cases --". 02) Allow group access on PGDATA * Add data_directory_mode guc to "Preset Options" documentation. * Add a section to the documentation that discusses changing the permissions of an existing cluster. * Improved comment on checkControlFile(). * Comment wordsmithing for SetDataDirectoryCreatePerm() and RetrieveDataDirCreatePerm(). * Fixed ordering of -g option in initdb documentation. * Fixed a new test that was "broken" by 032429701e477. It was broken before but the rmtrees added in 032429701e477 exposed the problem. Attached are the v19 patches. Thanks! -- -David david@pgmasters.net
Attachment
David, * David Steele (david@pgmasters.net) wrote: > On 4/6/18 10:22 PM, Stephen Frost wrote: > > * David Steele (david@pgmasters.net) wrote: > >> On 4/6/18 6:04 PM, David Steele wrote: > >>> On 4/6/18 3:02 PM, Stephen Frost wrote: > >>>> > >>>> - Further discussion in the commit messages > >>> > >>> Agreed, these need some more work. I'm happy to do that but I'll need a > >>> bit more time. Have a look at the new patches and I'll work on some > >>> better messages. > >> > >> I'm sure you'll want to reword some things, but I think these commit > >> messages capture the essential changes for each patch. > > > > Thanks much. I've taken (most) of these, adjusting a few bits here and > > there. > > > > I've been back over the patch again, mostly improving the commit > > messages, comments, and docs. I also looked over the code and tests > > again and they're looking pretty good to me, so I'll be looking to > > commit this tomorrow afternoon or so US/Eastern. > > OK, one last review. I did't make any code changes, but I improved some > comments, added documentation and fixed a test. Thanks! I took those and then added a bit more commentary around the umask() calls in the utilities and fixed a typo or two and then pushed it. Time to watch the buildfarm, particularly for Windows hosts just in case there's something in the regression tests which aren't working correctly on that platform. I was able to run the full regression suite locally before committing, though given the recent activity, we may see failures attributed to this patch which are due to unrelated instability. Thanks again! Stephen
Attachment
On 4/7/18 5:49 PM, Stephen Frost wrote: > * David Steele (david@pgmasters.net) wrote: >> >> OK, one last review. I did't make any code changes, but I improved some >> comments, added documentation and fixed a test. > > Thanks! I took those and then added a bit more commentary around the > umask() calls in the utilities and fixed a typo or two and then pushed > it. Excellent, thank you! > Time to watch the buildfarm, particularly for Windows hosts just in case > there's something in the regression tests which aren't working correctly > on that platform. I was able to run the full regression suite locally > before committing, though given the recent activity, we may see failures > attributed to this patch which are due to unrelated instability. I'll have dinner then come back and take a look. -- -David david@pgmasters.net
Attachment
Stephen Frost <sfrost@snowman.net> writes: > Time to watch the buildfarm, That didn't take long. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2018-04-07%2021%3A50%3A02 (I'm really starting to get a bit upset at the amount of stuff that's being pushed in on the very last day.) regards, tom lane
Tom, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > Time to watch the buildfarm, > > That didn't take long. > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2018-04-07%2021%3A50%3A02 Yes, I'm taking a look at it. Thanks! Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2018-04-07%2021%3A50%3A02 > Yes, I'm taking a look at it. Since other animals are coming back successfully, my first suspicion lights on culicidae's use of EXEC_BACKEND. Did you test that case? (If not, this bodes ill for the Windows results.) 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: > >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2018-04-07%2021%3A50%3A02 > > > Yes, I'm taking a look at it. > > Since other animals are coming back successfully, my first suspicion > lights on culicidae's use of EXEC_BACKEND. Did you test that case? > (If not, this bodes ill for the Windows results.) The vast majority of this patch isn't something that's relevant to Windows anyway, so I'm hopeful that those animals will stay green (if not, it's probably that we need to mark some other test as not-on-Windows). We have EXEC_BACKEND on the Unix-y systems too though, so that needs to be fixed. I'm pretty sure the issue here is that SubPostmasterMain() needs to also be checking/updating the umask() based on the data directory, as is done in PostmasterMain. Testing that out now and if that looks good then I'll push that and hopefully it'll make cullcidae green. Thanks! Stephen
Attachment
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > (If not, this bodes ill for the Windows results.) Ah, there go the Windows builds... Missing a #else clause where we should have one for those systems, will fix. Thanks! Stephen
Attachment
Hi Michael, On 4/6/18 10:20 AM, Michael Paquier wrote: > On Fri, Apr 06, 2018 at 09:15:15AM -0400, Stephen Frost wrote: >> I'll reply to David's last email (where the latest set of patches were >> included) with my comments/suggestions and I expect we'll be able to get >> those addressed today and have a final patch to post tonight, with an >> eye towards committing it tomorrow. > > The feature freeze is on the 8th, so I am going to have limited room to > comment on things until that day. If something gets committed, I am > pretty sure that I'll get out of my pocket a couple of things to improve > the feature and its interface anyway if of course you are ready to > accept that. Improvements are always welcome! The core focus of the patch hasn't changed over the last year, but we've found better ways to implement it over time. I'm sure there's more we can do. > I have limited my role to be a reviewer, so I refrained > myself from writing any code ;) Your dedicated and tireless review helped get this patch over the line. Thank you very much for all your hard work. -- -David david@pgmasters.net