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