Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
Greetings, * Tom Lane (t...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Tom Lane (t...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Stephen Frost writes: > * Tom Lane (t...@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
Re: PATCH: Configurable file mode mask
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost 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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Stephen Frost 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
Re: PATCH: Configurable file mode mask
On 4/7/18 5:49 PM, Stephen Frost wrote: > * David Steele (da...@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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
David, * David Steele (da...@pgmasters.net) wrote: > On 4/6/18 10:22 PM, Stephen Frost wrote: > > * David Steele (da...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Hi Stephen, On 4/6/18 10:22 PM, Stephen Frost wrote: > > * David Steele (da...@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 da...@pgmasters.net From d97495e81bb59beb03941398d7746a22b55d48ec Mon Sep 17 00:00:00 2001 From: David Steele Date: Sat, 7 Apr 2018 09:49:15 -0400 Subject: [PATCH 1/2] Refactor dir/file permissions Consolidate directory and file create permissions for tools which work with the PG data directory 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 tools which modify the PG data directory. Authors: David Steele , Adam Brightwell Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/tablespace.c| 18 --- src/backend/postmaster/postmaster.c | 7 +-- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c| 51 +-- src/backend/storage/ipc/dsm_impl.c | 3 +- src/backend/storage/ipc/ipc.c| 4 ++ src/backend/utils/init/miscinit.c| 5 +- src/bin/initdb/initdb.c | 24 - src/bin/initdb/t/001_initdb.pl | 11 - src/bin/pg_basebackup/pg_basebackup.c| 9 ++-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 14 +- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 14 +- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 18 ++- src/bin/pg_resetwal/pg_resetwal.c| 5 +- src/bin/pg_resetwal/t/001_basic.pl | 12 - src/bin/pg_rewind/RewindTest.pm | 4 ++ src/bin/pg_rewind/file_ops.c | 7 +-- src/bin/pg_rewind/t/001_basic.pl | 11 - src/bin/pg_upgrade/file.c| 5 +- src/bin/pg_upgrade/pg_upgrade.c | 3 +- src/bin/pg_upgrade/test.sh | 11 + src/common/Makefile | 4 +- src/common/file_perm.c | 19 src/include/common/file_perm.h | 34 + src/include/storage/fd.h | 3 ++ src/test/perl/PostgresNode.pm| 3 ++ src/test/perl/TestLib.pm | 73 src/tools/msvc/Mkvcbuild.pm | 4 +- 34 files changed, 330 insertions(+), 75 deletions(-) create mode 100644 src/common/file_perm.c create mode 100644 src/include/common/file_perm.h diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 813b2afaac..4a47395174 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4107,7 +4107,7 @@ ValidateXLOGDirectoryStructure(void
Re: PATCH: Configurable file mode mask
David, * David Steele (da...@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 From 306d152df68e294e056da1b55b33460f5a585063 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 6 Apr 2018 18:10:43 -0400 Subject: [PATCH 1/2] Refactor dir/file permissions Consolidate directory and file create permissions for tools which work with the PG data directory 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 tools which modify the PG data directory. Authors: David Steele , Adam Brightwell Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/tablespace.c| 18 --- src/backend/postmaster/postmaster.c | 7 +-- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c| 51 +-- src/backend/storage/ipc/dsm_impl.c | 3 +- src/backend/storage/ipc/ipc.c| 4 ++ src/backend/utils/init/miscinit.c| 5 +- src/bin/initdb/initdb.c | 24 - src/bin/initdb/t/001_initdb.pl | 11 - src/bin/pg_basebackup/pg_basebackup.c| 9 ++-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 14 +- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 14 +- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 18 ++- src/bin/pg_resetwal/pg_resetwal.c| 5 +- src/bin/pg_resetwal/t/001_basic.pl | 12 - src/bin/pg_rewind/RewindTest.pm | 4 ++ src/bin/pg_rewind/file_ops.c | 7 +-- src/bin/pg_rewind/t/001_basic.pl | 11 - src/bin/pg_upgrade/file.c| 5 +- src/bin/pg_upgrade/pg_upgrade.c | 3 +- src/bin/pg_upgrade/test.sh | 11 + src/common/Makefile | 4 +- src/common/file_perm.c | 19 src/include/common/file_perm.h | 34 + src/include/storage/fd.h | 3 ++ src/test/perl/PostgresNode.pm| 3 ++ src/test/perl/TestLib.pm | 73 src/tools/msvc/Mkvcbuild.pm | 4 +- 34 files changed, 330 insertions(+), 75 deletions(-) create mode 100644 src/common/file_perm.c create mode 100644 src/include/common/file_perm.h diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 813b2afaac..4a47395174 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4107,7 +4107,7 @@ ValidateXLOGDirectoryStructure(void) { ereport(LOG, (errmsg("creating missing WAL directory \"%s\"", path))); - if (mkdir(path, S_IRWXU) < 0) + if (MakePGDirectory(path) < 0) ereport(FATAL, (errmsg("could not create missing directory \"%s\": %m", path))); diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index 5c450caa4e..cfc9fe468c 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -68,6 +68,7 @@ #include "commands/seclabel.h" #include "commands/tablecmds.h" #include "commands/tablespace.h" +#include "common/file_perm.h" #include "miscadmin.h" #include "postmaster/bgwriter.h" #include "storage/fd.h" @@ -151,7 +152,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) else { /* Directory creation failed? */ -if (mkdir(dir, S_IRWXU) < 0) +if (Mak
Re: PATCH: Configurable file mode mask
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 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 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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net >From 792a6809cd3b6e7c006af6221011bfbc2b376601 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 6 Apr 2018 09:29:15 -0400 Subject: [PATCH 1/2] Refactor file permissions in backend/frontend 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 MakePGDirectory() if the original call used default permissions (always the case for regular PG directories). Adds tests to make sure permissions in PGDATA are set correctly by the front-end tools. Author: David Steele Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/tablespace.c| 18 --- src/backend/postmaster/postmaster.c | 7 +-- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c| 38 +-- src/backend/storage/ipc/dsm_impl.c | 3 +- src/backend/storage/ipc/ipc.c| 4 ++ src/backend/utils/init/miscinit.c| 5 +- src/bin/initdb/initdb.c | 24 - src/bin/initdb/t/001_initdb.pl | 11 - src/bin/pg_basebackup/pg_basebackup.c| 9 ++-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 14 +- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 14 +- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 18 ++- src/bin/pg_resetwal/pg_resetwal.c| 5 +- src/bin/pg_resetwal/t/001_basic.pl | 12 - src/bin/pg_rewind/RewindTest.pm | 4 ++ src/bin/pg_rewind/file_ops.c | 7 +-- src/bin/pg_rewind/t/001_basic.pl | 11 - src/bin/pg_upgrade/file.c| 5 +- src/bin/pg_upgrade/pg_upgrade.c | 3 +- src/bin/pg_upgrade/test.sh | 11 + src/common/Makefile | 4 +- src/common/file_perm.c | 19 src/include/common/file_perm.h | 32 src/include/storage/fd.h | 3 ++ src/test/perl/PostgresNode.pm| 3 ++ src/test/perl/TestLib.pm | 73 src/tools/msvc/Mkvcbuild.pm | 4 +- 34 files changed, 316 insertions(+), 74 deletions(-) create mode 100644 src/common/file_perm.c create mode 100644 src/include/common/file_perm.h diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 813b2afaac..4a47395174 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4107,7 +4107,7 @@ ValidateXLOGDirectoryStructure(void) { ereport(LOG, (errmsg("creating missing WAL directory \"%s\"", path))); - if (mkdir(path, S_IRWXU) < 0) + if (MakePGDirectory(path) < 0) ereport(FATAL, (errmsg("cou
Re: PATCH: Configurable file mode mask
Michael, * Michael Paquier (mich...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
David, * David Steele (da...@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 From d3a3f3e4d660e3cfc163a1787d4acb87d6f537e0 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 6 Apr 2018 09:29:15 -0400 Subject: [PATCH 1/2] Refactor file permissions in backend/frontend 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 MakePGDirectory() if the original call used default permissions (always the case for regular PG directories). Adds tests to make sure permissions in PGDATA are set correctly by the front-end tools. Author: David Steele Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/tablespace.c| 18 --- src/backend/postmaster/postmaster.c | 7 +-- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c| 38 +-- src/backend/storage/ipc/dsm_impl.c | 3 +- src/backend/storage/ipc/ipc.c| 4 ++ src/backend/utils/init/miscinit.c| 5 +- src/bin/initdb/initdb.c | 24 - src/bin/initdb/t/001_initdb.pl | 11 - src/bin/pg_basebackup/pg_basebackup.c| 9 ++-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 14 +- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 14 +- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 18 ++- src/bin/
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Greetings, * Michael Paquier (mich...@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-les
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net From 30599b54e0f31e6c6842e028eeca3048b9a371ca Mon Sep 17 00:00:00 2001 From: David Steele Date: Thu, 5 Apr 2018 11:34:58 -0400 Subject: [PATCH 1/2] Refactor file permissions in backend/frontend 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. --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/tablespace.c| 18 --- src/backend/postmaster/postmaster.c | 5 +- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c| 38 +-- src/backend/storage/ipc/dsm_impl.c | 3 +- src/backend/storage/ipc/ipc.c| 4 ++ src/backend/utils/init/miscinit.c| 5 +- src/bin/initdb/initdb.c | 24 - src/bin/initdb/t/001_initdb.pl | 11 - src/bin/pg_basebackup/pg_basebackup.c| 9 ++-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 14 +- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 14 +- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 18 ++- src/bin/pg_resetwal/pg_resetwal.c| 5 +- src/bin/pg_resetwal/t/001_basic.pl | 12 - src/bin/pg_rewind/RewindTest.pm | 4 ++ src/bin/pg_rewind/file_ops.c | 7 +-- src/bin/pg_rewind/t/001_basic.pl | 11 - src/bin/pg_upgrade/file.c| 5 +- src/bin/pg_upgrade/pg_upgrade.c | 3 +- src/bin/pg_upgrade/test.sh | 11 + src/common/Makefile | 4 +- src/common/file_perm.c | 19 src/include/common/file_perm.h | 32 src/include/storage/fd.h | 3 ++ src/test/perl/PostgresNode.pm| 3 ++ src/test/perl/TestLib.pm | 73 src/tools/msvc/Mkvcbuild.pm | 4 +- 34 files changed, 315 insertions(+), 73 deletions(-) create mode 100644 src/common/file_perm.c create mode 100644 src/include/common/file_perm.h diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index b4fd8395b7..5398d2f925 10064
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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. > + > +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. > + 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, , 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_DEFAULTS_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 da...@pgmasters.net From f8c2604ebb6ae16b3b843c2db491d10efe2737fe Mon Sep 17 00:00:00 2001 From: David Steele Date: Wed, 4 Apr 2018 19:17:46 -0400 Subject: [PATCH 1/2] Refactor file permissions in backend/frontend Adds a new header (file_perm.h) and makes all front-end utilities use
Re: PATCH: Configurable file mode mask
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... + +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. + signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net From b48aef8b82742683017bbb7b8ac2f509ed914414 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 30 Mar 2018 13:22:01 -0400 Subject: [PATCH 1/1] Refactor file permissions in backend/frontend 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. --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/table
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 (da...@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 da...@pgmasters.net From 60752c2e12fa2132c24d71ff030cefe4b2b6c502 Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 27 Mar 2018 15:47:15 -0400 Subject: [PATCH 1/2] Refactor file permissions in backend/frontend 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. --- src/backend/access/transam/xlog.c| 2 +- src/backend/commands/tablespace.c| 18 --- src/backend/postmaster/postmaster.c | 5 +- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c| 32 +++- src/backend/storage/ipc/ipc.c| 4 ++ src/backend/utils/init/miscinit.c| 5 +- src/bin/initdb/initdb.c | 24 - src/bin/initdb/t/001_initdb.pl | 4 +- src/bin/pg_basebackup/pg_basebackup.c| 7 +-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 9 +++- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 9 +++- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 13 +++-- src/bin/pg_dump/pg_backup_directory.c| 3 +- src/bin/pg_resetwal/pg_resetwal.c| 8 ++- src/bin/pg_resetwal/t/001_basic.pl | 5 +- src/bin/pg_rewind/RewindTest.pm | 4 ++ src/bin/pg_rewind/file_ops.c | 7 +-- src/bin/pg_rewind/pg_rewind.c| 4 ++ src/bin/pg_rewind/t/001_basic.pl | 3 +- src/bin/pg_upgrade/file.c| 5 +- src/bin/pg_upgrade/pg_upgrade.c | 3 +- src/bin/pg_upgrade/test.sh | 11 + src/include/common/file_perm.h | 32 src/include/storage/fd.h | 3 ++ src/test/perl/PostgresNode.pm| 3 ++ src/test/perl/TestLib.pm | 73 32 files changed, 256 insertions(+), 67 deletions(-) create mode 100644 src/include/common/file_perm.h diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index cb9c2a29cb..95c0fda4e3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4086,7 +4086,7 @@ ValidateXLOGDirectoryStructure(void) {
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl index 1b157cb555..474ece99c7 100644 --- a/src/bin/pg_resetwal/t/001_basic.pl +++ b/src/bin/pg_resetwal/t/001_basic.pl @@ -3,7 +3,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 11; +use Test::More tests => 16; program_help_ok('pg_resetwal'); program_version_ok('pg_resetwal'); @@ -15,3 +15,29 @@ $node->init; command_like([ 'pg_resetwal', '-n', $node->data_dir ], qr/checkpoint/, 'pg_resetwal -n produces output'); + +# Reset WAL after segment has been removed +my $pgwal = $node->data_dir . '/pg_wal'; + +unlink("$pgwal/00010001") == 1 + or BAIL_OUT("unable to remove 00010001"); + +is_deeply( + [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL'); + +$node->command_ok(['pg_resetwal', '-D', $node->data_dir], 'recreate pg_wal'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 00010002))], + 'WAL recreated'); + +# Reset to specific WAL segment +$node->command_ok( + ['pg_resetwal', '-l', '000700070007', '-D', $node->data_dir], + 'set to specific WAL'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 000700070007))], + 'WAL recreated');
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
On Tue, Mar 20, 2018 at 05:44:22PM -0400, Stephen Frost wrote: > * David Steele (da...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
David, * David Steele (da...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Greetings David, Michael, all, * David Steele (da...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
Hi, On 3/13/18 12:13 PM, Stephen Frost wrote: > * David Steele (da...@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 pg_ctl.) If group >>> read >>> +access is enabled on the data directory and an unprivileged user in the >>> +PostgreSQL group is performing the backup, >>> then >>> +postmaster.pid 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 da...@pgmasters.net From 026179cc58166d2b0c6e233ee28ea1d745cffba9 Mon Sep 17 00:00:00 2001 From: David Steele Date: Wed, 14 Mar 2018 13:48:35 -0400 Subject: [PATCH 1/3] pg_resetwal tests. Adds a very basic test suite for pg_resetwal. --- src/bin/pg_resetwal/.gitignore | 1 + src/bin/pg_resetwal/Makefile | 6 + src/bin/pg_resetwal/t/001_basic.pl | 53 ++ 3 files changed, 60 insertions(+) create mode 100644 src/bin/pg_resetwal/t/001_basic.pl diff --git a/src/bin/pg_resetwal/.gitignore b/src/bin/pg_resetwal/.gitignore index 236abb4323..a950255fd7 100644 --- a/src/bin/pg_resetwal/.gitignore +++ b/src/bin/pg_resetwal/.gitignore @@ -1 +1,2 @@ /pg_resetwal +/tmp_check diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile index 5ab7ad33e0..13c9ca6279 100644 --- a/src/bin/pg_resetwal/Makefile +++ b/src/bin/pg_resetwal/Makefile @@ -33,3 +33,9 @@ uninstall: clean distclean maintainer-clean: rm -f pg_resetwal$(X) $(OBJS) + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl new file mode 100644 index 00..b867fe3dba --- /dev/null +++ b/src/bin/pg_resetwal/t/001_basic.pl @@ -0,0 +1,53 @@ +use strict; +use warnings; + +use Config; +use PostgresNode; +use TestLib; +use Test::More tests => 13; + +my $tempdir = TestLib::tempdir; +my $tempdir_short = TestLib::tempdir_short; + +program_help_ok('pg_resetwal'); +program_version_ok('pg_resetwal'); +program_options_handling_ok('pg_resetwal'); + +# Initialize node without replication settings +my $node = get_new_node('main'); +my $pgdata = $node->data_dir; +my $pgwal = "$pgdata/pg_wal"; +$node->init; +$node->start; + +# Remove WAL from pg_wal and make sure it gets rebuilt +$node->stop; + +unlink("$pgwal/00010001") == 1 + or BAIL_OUT("unable to remove 00010001"); + +is_deeply( + [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL'); + +$node->command_ok(['pg_resetwal', '-D', $pgdata], 'recreate pg_wal'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 00010002))], + 'WAL recreated'); + +$node->start; + +# Reset to specific WAL segment +$node->stop; + +$node->command_ok( + ['pg_resetwal', '-l', '000700070007', '-D', $pgdata], + 'set to specific WAL'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 000700070007))], + 'WAL recreated'); + +$node->start; -- 2.14.3 (Apple Git-98) From 7d091c7b46feabd706fd1a564f634bc718d72cc6 Mon Sep 17 00:00:00 2001 From: David Steele Date: Wed, 14 Mar 2018 13:53:03 -0400 Subject: [PATCH 2/3] Refactor file permissions in backend/frontend
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
On Tue, Mar 13, 2018 at 01:28:17PM -0400, Tom Lane wrote: > David Steele 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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Chapman, * Chapman Flack (c...@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
Re: PATCH: Configurable file mode mask
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 (c...@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
Re: PATCH: Configurable file mode mask
Greetings Chapman, * Chapman Flack (c...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Chapman Flack 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
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
Greetings Chapman, * Chapman Flack (c...@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 tha
Re: PATCH: Configurable file mode mask
On 03/13/2018 10:40 AM, Stephen Frost wrote: > * Michael Paquier (mich...@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
Re: PATCH: Configurable file mode mask
David Steele 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
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net [1] https://www.postgresql.org/message-id/22928.1520953220%40sss.pgh.pa.us signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
Greetings, * David Steele (da...@pgmasters.net) wrote: > On 3/13/18 11:00 AM, Tom Lane wrote: > > Stephen Frost writes: > >> * Michael Paquier (mich...@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 pg_ctl.) If group > > read > > +access is enabled on the data directory and an unprivileged user in the > > +PostgreSQL group is performing the backup, > > then > > +postmaster.pid 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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
On 3/13/18 11:00 AM, Tom Lane wrote: > Stephen Frost writes: >> * Michael Paquier (mich...@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 pg_ctl.) If group > read > +access is enabled on the data directory and an unprivileged user in the > +PostgreSQL group is performing the backup, > then > +postmaster.pid 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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
Stephen Frost writes: > * Michael Paquier (mich...@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 pg_ctl.) If group read +access is enabled on the data directory and an unprivileged user in the +PostgreSQL group is performing the backup, then +postmaster.pid 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
Re: PATCH: Configurable file mode mask
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 da...@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 signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
Michael, * Michael Paquier (mich...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Michael, David, * Michael Paquier (mich...@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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net From 19d827b9d9c0285556e977d3962619deb84c3c0e Mon Sep 17 00:00:00 2001 From: David Steele Date: Mon, 5 Mar 2018 14:33:10 -0500 Subject: [PATCH 1/3] pg_resetwal tests. Adds a very basic test suite for pg_resetwal. --- src/bin/pg_resetwal/.gitignore | 1 + src/bin/pg_resetwal/Makefile | 6 + src/bin/pg_resetwal/t/001_basic.pl | 53 ++ 3 files changed, 60 insertions(+) create mode 100644 src/bin/pg_resetwal/t/001_basic.pl diff --git a/src/bin/pg_resetwal/.gitignore b/src/bin/pg_resetwal/.gitignore index 236abb4323..a950255fd7 100644 --- a/src/bin/pg_resetwal/.gitignore +++ b/src/bin/pg_resetwal/.gitignore @@ -1 +1,2 @@ /pg_resetwal +/tmp_check diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile index 5ab7ad33e0..13c9ca6279 100644 --- a/src/bin/pg_resetwal/Makefile +++ b/src/bin/pg_resetwal/Makefile @@ -33,3 +33,9 @@ uninstall: clean distclean maintainer-clean: rm -f p
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net From 2caecfef0cb0b029c439786fe462d69b5caaf67b Mon Sep 17 00:00:00 2001 From: David Steele Date: Mon, 5 Mar 2018 14:33:10 -0500 Subject: [PATCH 1/3] pg_resetwal tests. Adds a very basic test suite for pg_resetwal. --- src/bin/pg_resetwal/.gitignore | 1 + src/bin/pg_resetwal/Makefile | 6 + src/bin/pg_resetwal/t/001_basic.pl | 53 ++ 3 files changed, 60 insertions(+) create mode 100644 src/bin/pg_resetwal/t/001_basic.pl diff --git a/src/bin/pg_resetwal/.gitignore b/src/bin/pg_resetwal/.gitignore index 236abb4323..a950255fd7 100644 --- a/src/bin/pg_resetwal/.gitignore +++ b/src/bin/pg_resetwal/.gitignore @@ -1 +1,2 @@ /pg_resetwal +/tmp_check diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile index 5ab7ad33e0..13c9ca6279 100644 --- a/src/bin/pg_resetwal/Makefile +++ b/src/bin/pg_resetwal/Makefile @@ -
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
On Mon, Mar 05, 2018 at 05:11:29PM -0500, Tom Lane wrote: > David Steele 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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
On 3/5/18 5:51 PM, Tom Lane wrote: > David Steele writes: >> On 3/5/18 5:11 PM, Tom Lane wrote: >>> David Steele 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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
David Steele writes: > On 3/5/18 5:11 PM, Tom Lane wrote: >> David Steele 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
Re: PATCH: Configurable file mode mask
Tom, all, * Tom Lane (t...@sss.pgh.pa.us) wrote: > David Steele 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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Hi Tom, On 3/5/18 5:11 PM, Tom Lane wrote: > David Steele 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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
David Steele 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
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 regressio
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net [1] https://commitfest.postgresql.org/14/1262/
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
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, r
Re: PATCH: Configurable file mode mask
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/00010001") == 1 > + or die("unable to remove 00010001"); > + > +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 da...@pgmasters.net diff --git a/src/bin/pg_resetwal/.gitignore b/src/bin/pg_resetwal/.gitignore index 236abb4323..a950255fd7 100644 --- a/src/bin/pg_resetwal/.gitignore +++ b/src/bin/pg_resetwal/.gitignore @@ -1 +1,2 @@ /pg_resetwal +/tmp_check diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile index 5ab7ad33e0..13c9ca6279 100644 --- a/src/bin/pg_resetwal/Makefile +++ b/src/bin/pg_resetwal/Makefile @@ -33,3 +33,9 @@ uninstall: clean distclean maintai
Re: PATCH: Configurable file mode mask
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/00010001") == 1 + or die("unable to remove 00010001"); + +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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile index 5ab7ad33e0..13c9ca6279 100644 --- a/src/bin/pg_resetwal/Makefile +++ b/src/bin/pg_resetwal/Makefile @@ -33,3 +33,9 @@ uninstall: clean distclean maintainer-clean: rm -f pg_resetwal$(X) $(OBJS) + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl new file mode 100644 index 00..234bd70303 --- /dev/null +++ b/src/bin/pg_resetwal/t/001_basic.pl @@ -0,0 +1,53 @@ +use strict; +use warnings; + +use Config; +use PostgresNode; +use TestLib; +use Test::More tests => 13; + +my $tempdir = TestLib::tempdir; +my $tempdir_short = TestLib::tempdir_short; + +program_help_ok('pg_resetwal'); +program_version_ok('pg_resetwal'); +program_options_handling_ok('pg_resetwal'); + +# Initialize node without replication settings +my $node = get_new_node('main'); +my $pgdata = $node->data_dir; +my $pgwal = "$pgdata/pg_wal"; +$node->init; +$node->start; + +# Remove WAL from pg_wal and make sure it gets rebuilt +$node->stop; + +unlink("$pgwal/00010001") == 1 + or die("unable to remove 00010001"); + +is_deeply( + [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL'); + +$node->command_ok(['pg_resetwal', '-D', $pgdata], 'recreate pg_wal'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 00010002))], + 'WAL recreated'); + +$node->start; + +# Reset to specific WAL segment +$node->stop; + +$node->command_ok( + ['pg_resetwal', '-l', '000700070007', '-D', $pgdata], + 'set to specific WAL'); + +is_deeply( + [sort(slurp_dir($pgwal))], + [sort(qw(. .. archive_status 000700070007))], + 'WAL recreated'); + +$node->start; diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index e42b828edf..9814ac6d3e 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4086,7 +4086,7 @@ ValidateXLOGDirectoryStructure(void) { ereport(LOG, (errmsg("creating missing WAL directory \"%s\"", path))); - if (mkdir(path, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(path) < 0) ereport(FATAL, (errmsg("could not create missing directory \"%s\": %m", path))); diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index 5c450caa4e..746b8c121b 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -151,7 +151,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) else { /* Directory creation failed? */ - if (mkdir(dir, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(dir) < 0) { char *parentdir; @@ -173,7 +173,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) get_parent_directory(parentdir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EE
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
Peter Eisentraut 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
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
On 1/23/18 9:26 AM, Tom Lane wrote: > David Steele 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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
David Steele 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
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
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 da...@pgmasters.net diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index e42b828edf..9814ac6d3e 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4086,7 +4086,7 @@ ValidateXLOGDirectoryStructure(void) { ereport(LOG, (errmsg("creating missing WAL directory \"%s\"", path))); - if (mkdir(path, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(path) < 0) ereport(FATAL, (errmsg("could not create missing directory \"%s\": %m", path))); diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index 8cb834c271..4d836aaeaf 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -151,7 +151,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) else { /* Directory creation failed? */ - if (mkdir(dir, S_IRWXU) < 0) + if (MakeDirectoryDefaultPerm(dir) < 0) { char *parentdir; @@ -173,7 +173,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) get_parent_directory(parentdir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST) + if (MakeDirectoryDefaultPerm(parentdir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -184,7 +184,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) parentdir = pstrdup(dir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST) + if (MakeDirectoryDefaultPerm(parentdir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(),
Re: PATCH: Configurable file mode mask
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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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
Re: PATCH: Configurable file mode mask
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: PATCH: Configurable file mode mask
David, * David Steele (da...@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 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 signature.asc Description: PGP signature
Re: PATCH: Configurable file mode mask
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 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 da...@pgmasters.net
Re: PATCH: Configurable file mode mask
On 1/3/18 08:11, Robert Haas wrote: > On Tue, Jan 2, 2018 at 11:43 AM, David Steele 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