Re: PATCH: Configurable file mode mask - Mailing list pgsql-hackers

On 3/6/18 10:04 PM, Michael Paquier wrote:
> On Tue, Mar 06, 2018 at 01:32:49PM -0500, David Steele wrote:
>> On 3/5/18 10:46 PM, Michael Paquier wrote:
>
>>> Those two are separate issues.  Could you begin a new thread on the
>>> matter?  This will attract more attention.
>>
>> OK, I'll move it back for now, and make a note to raise the position as
>> a separate issue.  I'd rather not do it in this fest, though.
>
> Seems like you forgot to re-add the chmod calls in initdb.c.

Hmmm, I thought we were talking about moving the position of umask().

I don't think the chmod() calls are needed because umask() is being set.
 The tests show that the config files have the proper permissions after
inidb, so this just looks like redundant code to me.

But, I'm not going to argue if you think they should go back.  It would
make the patch less noisy.

>>> -   if (chmod(location, S_IRWXU) != 0)
>>> +   current_umask = umask(0);
>>> +   umask(current_umask);
>>> [...]
>>> -   if (chmod(pg_data, S_IRWXU) != 0)
>>> +   if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
>>> Such modifications should be part of patch 3, not 2, where the group
>>> features really apply.
>>
>> The goal of patch 2 is to refactor the way permissions are set by
>> replacing locally hard-coded values with centralized values, so I think
>> this makes sense here.
>
> Hm.  I would have left that out in this first version, now I am fine to
> defer that to a committer's opinion as well.

OK - I really do think it belongs here.  A committer may not agree, of
course.

>>>     my $test_mode = shift;
>>>
>>> +   umask(0077);
>>
>> Added. (Ensure that all files are created with default data dir
>> permissions).
>
> +   # Ensure that all files are created with default data dir permissions
> +   umask(0077);
> I have stared at this comment two minutes to finally understand that
> this refers to the extra files which are part of this test.  It may be
> better to be a bit more precise here.  I thought first that this
> referred as well to setup_cluster calls...

Updated to, "Ensure that all files created in this module for testing
are set with default data dir permissions."

> +# Ensure all permissions in the pg_data directory are
> correct. Permissions
> +# should be dir = 0700, file = 0600.
> +sub check_pg_data_perm
> +{
> check_permission_recursive() would be a more adapted name.  Stuff in
> TestLib.pm is not necessarily linked to data folders.

When we add group permissions we need to have special logic around
postmaster.pid.  This should be 0600 even if the rest of the files are 0640.

I can certainly remove that special logic in 02 and make this function
more generic, but then I think we would still have to add
check_pg_data_perm() in patch 03.

Another way to do this would be to make the function generic and
stipulate that the postmaster must be shut down before running the
function.  We could check postmaster.pid permissions as a separate test.

What do you think?

> sub clean_rewind_test
> {
> +   ok (check_pg_data_perm($node_master->data_dir(), 0));
> +
>     $node_master->teardown_node  if defined $node_master;
> I have also a pending patch for pg_rewind which adds read-only files in
> the data folders of a new test, so this would cause this part to blow
> up.  Testing that for all the test sets does not bring additional value
> as well, and doing it at cleanup phase is also confusing.  So could you
> move that check into only one test's script?  You could remove the umask
> call in 003_extrafiles.pl as well this way, and reduce the patch diffs.

I think I would rather have a way to skip the permission test rather
than disable it for most of the tests.  pg_rewind does more writes into
PGDATA that anything other than a backend.  Good coverage seems like a plus.

> +     if ($file_mode != 0600)
> +     {
> +         print(*STDERR, "$File::Find::name mode must be 0600\n");
> +
> +         $result = 0;
> +         return
> +     }
> 0600 should be replaced by $expected_file_perm, and isn't a ';' missing
> for each return "clause" (how does this even work..)?

Well, 0600 is a special case -- see above.  As for the missing
semi-colon, well that's Perl for you.  Fixed.

> Patch 2 is getting close for a committer lookup I think, still need to
> look at patch 3 in details..  And from patch 3:
>      # Expected permission
> -    my $expected_file_perm = 0600;
> -    my $expected_dir_perm = 0700;
> +    my $expected_file_perm = $allow_group ? 0640 : 0600;
> +    my $expected_dir_perm = $allow_group ? 0750 : 0700;
> Or $expected_file_perm and $expected_dir_perm could just be used as
> arguments.

This gets back to the check_pg_data_perm() discussion above.

I'll hold off on another set of patches until I hear back from you.
There were only trivial changes as noted above.

Thanks,
--
-David
david@pgmasters.net