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