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(at)pgmasters(dot)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(at)pgmasters(dot)net