OpenFile() Permissions Refactor

On 8/29/17 12:15, David Steele wrote:

While working on the patch to allow group reads of $PGDATA I refactored
the various OpenFile() functions to use default/global permissions
rather than permissions defined in each call.

I think the patch stands on its own regardless of whether we accept the
patch to allow group permissions (which won't make this CF). We had a
couple different ways of defining permissions (e.g. 0600 vs S_IRUSR |
S_IWUSR) and in any case it represented quite a bit of duplication.
This way seems simpler and less error-prone.

Yeah, it would be good to clean some of that up.

I wonder whether we even need that much flexibility. We already set a
global umask, so we could just open all files with 0666 and let the
umask sort it out. Then we don't need all the *Perm() variants.

I don't like the function-like macros in fd.h. We can use proper functions.

I also wonder whether the umask save-and-restore code in copy.c and
be-fsstubs.c is sound. If the AllocateFile() call in between errors
out, then there is nothing that restores the original umask. This might
need a TRY/CATCH block, or maybe just a chmod() afterwards.

The mkdir() calls could perhaps use some further refactoring so you
don't have to specify the mode everywhere.

This kind of code:

-   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
        ereport(FATAL,
                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                 errmsg("data directory \"%s\" has group or world access",
                        DataDir),
-                errdetail("Permissions should be u=rwx (0700).")));
+                errdetail("Permissions should be (%04o).",
PG_DIR_MODE_DEFAULT)));

can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
consistent.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I wonder whether we even need that much flexibility. We already set a
global umask, so we could just open all files with 0666 and let the
umask sort it out. Then we don't need all the *Perm() variants.

-1. What that would mean is that if somebody had a need for a nonstandard
file mask, the path of least resistance would be to bypass fd.c and open
the file directly. We do *not* want to encourage that.

I think David's design with macros inserting the default permission
choices looks fine (but I haven't read the patch completely).

I also wonder whether the umask save-and-restore code in copy.c and
be-fsstubs.c is sound. If the AllocateFile() call in between errors
out, then there is nothing that restores the original umask. This might
need a TRY/CATCH block, or maybe just a chmod() afterwards.

Yeah, this seems like a problem.

This kind of code:

-   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
ereport(FATAL,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("data directory \"%s\" has group or world access",
DataDir),
-                errdetail("Permissions should be u=rwx (0700).")));
+                errdetail("Permissions should be (%04o).",
PG_DIR_MODE_DEFAULT)));

I think this hunk should be left entirely alone. The goal of this
patch should not be "eliminate every reference to S_IRWXO", anyway.
Trying to replace this one seems like it can have no good effect:
it would mean that if anyone ever changes PG_MODE_MASK_DEFAULT,
they've silently introduced either a security hole or a overly
restrictive check.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9/1/17 1:15 PM, Peter Eisentraut wrote:

On 8/29/17 12:15, David Steele wrote:

I wonder whether we even need that much flexibility. We already set a
global umask, so we could just open all files with 0666 and let the
umask sort it out. Then we don't need all the *Perm() variants.

Well, there's one instance where the *Perm is used:

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
-	fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY,
-						   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	fd = OpenTransientFilePerm(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC |
PG_BINARY,
+							   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

I also think it's worth leaving the variants for extensions to use.
Even though there are no calls in the core extensions it's hard to say
what might be out there in the field.

I don't like the function-like macros in fd.h. We can use proper functions.

I had them as functions originally but thought macros might be
friendlier with compilers that don't inline. I'm happy to change them back.

I also wonder whether the umask save-and-restore code in copy.c and
be-fsstubs.c is sound. If the AllocateFile() call in between errors
out, then there is nothing that restores the original umask. This might
need a TRY/CATCH block, or maybe just a chmod() afterwards.

Unless I'm mistaken this is a preexisting issue. Would you prefer I
submit a different patch for that or combine it into this patch?

The chmod() implementation seems the safer option to me and produces
fewer code paths. It also prevents partially-written files from being
accessible to any user but postgres.

The mkdir() calls could perhaps use some further refactoring so you
don't have to specify the mode everywhere.

I thought about that but feared it would be considered an overreach.
Does fd.c seem like a good place for the new function?

This kind of code:

-   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
ereport(FATAL,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("data directory \"%s\" has group or world access",
DataDir),
-                errdetail("Permissions should be u=rwx (0700).")));
+                errdetail("Permissions should be (%04o).",
PG_DIR_MODE_DEFAULT)));

can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
consistent.

Well, the eventual goal is to make the mask/mode configurable - at least
to the extent that group access is allowed. However, I'm happy to leave
that discussion for another day.

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Peter,

Here's a new patch based on your review. Where I had a question I made
a choice as described below:

On 9/1/17 1:58 PM, David Steele wrote:

On 9/1/17 1:15 PM, Peter Eisentraut wrote:

On 8/29/17 12:15, David Steele wrote:

I wonder whether we even need that much flexibility. We already set a
global umask, so we could just open all files with 0666 and let the
umask sort it out. Then we don't need all the *Perm() variants.

Well, there's one instance where the *Perm is used:

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
-	fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY,
-						   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	fd = OpenTransientFilePerm(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC |
PG_BINARY,
+							   S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

I also think it's worth leaving the variants for extensions to use.
Even though there are no calls in the core extensions it's hard to say
what might be out there in the field.

These have been left in.

I don't like the function-like macros in fd.h. We can use proper functions.

I had them as functions originally but thought macros might be
friendlier with compilers that don't inline. I'm happy to change them back.

Macros have been converted to functions.

I also wonder whether the umask save-and-restore code in copy.c and
be-fsstubs.c is sound. If the AllocateFile() call in between errors
out, then there is nothing that restores the original umask. This might
need a TRY/CATCH block, or maybe just a chmod() afterwards.

Unless I'm mistaken this is a preexisting issue. Would you prefer I
submit a different patch for that or combine it into this patch?

The chmod() implementation seems the safer option to me and produces
fewer code paths. It also prevents partially-written files from being
accessible to any user but postgres.

I went with chmod(). The fix is incorporated in this patch but if you
want it broken out let me know.

The mkdir() calls could perhaps use some further refactoring so you
don't have to specify the mode everywhere.

I thought about that but feared it would be considered an overreach.
Does fd.c seem like a good place for the new function?

New functions MakeDirectory() and MakeDirectoryPerm() have been added to
fd.c.

MakeDirectoryPerm() is used in ipc.c.

This kind of code:

-   if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+   if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
ereport(FATAL,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("data directory \"%s\" has group or world access",
DataDir),
-                errdetail("Permissions should be u=rwx (0700).")));
+                errdetail("Permissions should be (%04o).",
PG_DIR_MODE_DEFAULT)));

can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
consistent.

Well, the eventual goal is to make the mask/mode configurable - at least
to the extent that group access is allowed. However, I'm happy to leave
that discussion for another day.

Changes to postmaster.c have been reverted (except to rename mkdir to
MakeDirectory).

Patch v2 is attached.

--
-David
david@pgmasters.net

On 9/13/17 10:26, David Steele wrote:

Here's a new patch based on your review. Where I had a question I made
a choice as described below:

I have committed the changes to the file APIs and a fix for the umask
save/restore issue.

The mkdir changes didn't really inspire me. Replacing mkdir() with
MakeDirectoryPerm() without any change in functionality doesn't really
improve clarity. Maybe we'll revisit that when your next patches arrive.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9/23/17 10:22 AM, Peter Eisentraut wrote:

On 9/13/17 10:26, David Steele wrote:

Here's a new patch based on your review. Where I had a question I made
a choice as described below:

I have committed the changes to the file APIs and a fix for the umask
save/restore issue.

Thank you!

The mkdir changes didn't really inspire me. Replacing mkdir() with
MakeDirectoryPerm() without any change in functionality doesn't really
improve clarity.

OK. I had hoped removing the need to specify the mode at every call
site was functionality enough. Even so, I'm a little surprised you
didn't keep PG_DIR_MODE_DEFAULT.

Maybe we'll revisit that when your next patches arrive.

The next patch set was be this same refactor on the tools (initdb,
pg_rewind, etc), but if you think the mkdir refactor did not add enough
value then I'll rethink my plans.

I may need to present all the patches in one CF so it's clear where all
this is going: allowing group read on $PGDATA.

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers