Re: PATCH: Configurable file mode mask

Hi Robert,

Thanks for looking at the patches.

On 12/31/17 1:27 PM, Robert Haas wrote:
> On Thu, Dec 28, 2017 at 2:36 PM, David Steele <david@pgmasters.net> wrote:
>> Attached is a new patch set that should address various concerns raised in
>> this thread.
>>
>> 1) group-access-v3-01-mkdir.patch
>>
>> Abstracts all mkdir calls in the backend into a MakeDirectory() function
>> implemented in fd.c.  This did not get committed in September as part of
>> 0c5803b450e but I still think it has value.  However, I have kept it
>> separate to reduce noise in the second patch.  The mkdir() calls could also
>> be modified to use PG_DIR_MODE_DEFAULT with equivalent results.
> 
> +/*
> + * Default mode for created directories, unless something else is specified
> + * using the MakeDirectoryPerm() function variant.
> + */
> +#define PG_DIR_MODE_DEFAULT            (S_IRWXU)
> +#define PG_MODE_MASK_DEFAULT   (S_IRWXG | S_IRWXO)
> 
> There's only one comment here, but there are two constants that do
> different things.

Looks like a copy pasto - I didn't mean PG_MODE_MASK_DEFAULT to be in 
patch 01 at all.  Removed.

> Also, this hunk gets removed again by 02, which is probably OK, but 02
> adds the replacement stuff in a different part of the file, which
> seems not as good.

Agreed.  I have moved that.

> > I think MakeDirectory() is a good wrapper, but isn't
> MakeDirectoryPerm() sort of silly?

There's one place in the backend (storage/ipc/ipc.c) that sets 
non-default directory permissions.  This function is intended to support 
that and any extensions that need to set custom perms.

>> 2) group-access-v3-02-group.patch
>>
>> This is a "GUC-less" implementation of group read access that depends on the
>> mode of the $PGDATA directory to determine which mode to use for subsequent
>> writes.  The initdb option is preserved to allow group access to be enabled
>> when the cluster is initialized.
>>
>> Only two modes are allowed (700, 750) and the error message on startup is
>> hard-coded to address translation concerns.
> 
> +       umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT));
> 
> Hmm, I wonder if this is going to be 100% portable.  Maybe some
> obscure platform won't like an argument with all the high bits set.

Sure - I have masked this with 0777 to clear any high bits.  Sound OK?

> Also, why should we break what's now one #if into two?  That seems
> like it's mildly more complicated for no gain.

There were already two #ifdef blocks so I added a third.  I think we 
should leave it as three or combine them all into one.  There are no 
runtime performance implications so I'm agnostic.

> +    (These files can confuse <application>pg_ctl</application>.)  If group read
> +    access is enabled on the data directory and an unprivileged user in the
> +    <productname>PostgreSQL</productname> group is performing the backup, then
> +    <filename>postmaster.pid</filename> will not be readable and must be
> +    excluded.
> 
> Is that actually the behavior we want?

I think it is.  Comments in the code are pretty specific about not 
changing the permissions on postmaster.pid and I don't see any reason to 
mess with it.  Copying postmaster.pid as part of a backup is not a good 
idea anyway.

New patches attached.

Thanks!
-- 
-David
david@pgmasters.net