2020年1月30日(木) 14:57 Michael Paquier <mich...@paquier.xyz>:
>
> On Thu, Jan 30, 2020 at 02:29:06PM +0900, Ian Barwick wrote:
> > I can't think of any practical reason why pg_basebackup would ever need to
> > be run as root; we disallow that for initdb, pg_ctl and pg_upgrade, so it
> > seems reasonable to do the same for pg_basebackup. Trivial patch attached,
> > which as with the other cases will allow only the --help/--version options
> > to be executed as root, otherwise nothing else.
>
> My take on the matter is that we should prevent anything creating or
> modifying the data directory to run as root if we finish with
> permissions incompatible with what a postmaster expects. So +1.
>
> > The patch doesn't update the pg_basebackup documentation page; we don't
> > mention it in the pg_ctl and pg_upgrade pages either and it doesn't seem
> > particularly important to mention it explicitly.
>
> We don't mention that in the docs of pg_rewind either. Note also that
> before 5d5aedd pg_rewind printed an error without exiting :)
Ouch.
> > + /*
> > + * Disallow running as root, as PostgreSQL will be unable to start
> > + * with root-owned files.
> > + */
>
> Here is a suggestion:
> /*
> * Don't allow pg_basebackup to be run as root, to avoid creating
> * files in the data directory with ownership rights incompatible
> * with the postmaster. We need only check for root -- any other user
> * won't have sufficient permissions to modify files in the data
> * directory.
> */
I think we can skip the second sentence altogether. It'd be theoretically
easy enough to up with some combination of group permissions,
sticky bits, umask, ACL settings etc/ which would allow one user to
modify the files owned by another user,
> > + #ifndef WIN32
>
> Indentation here.
Whoops, that's what comes from typing on the train ;)
> > + if (geteuid() == 0) /* 0 is root's uid */
> > + {
> > + pg_log_error("cannot be run as root");
> > + fprintf(stderr,
> > + _("Please log in (using, e.g., \"su\") as the
> > (unprivileged) user that will\n"
> > + "own the server process.\n"));
> > + exit(1);
> > + }
> > +#endif
>
> I would recommend to map with the existing message of pg_rewind for
> consistency:
> pg_log_error("cannot be executed by \"root\"");
> fprintf(stderr, _("You must run %s as the PostgreSQL superuser.\n"),
> progname);
Hmm, I was using the existing message from initdb and pg_ctl for consistency:
src/bin/initdb/initdb.c:
if (geteuid() == 0) /* 0 is root's uid */
{
pg_log_error("cannot be run as root");
fprintf(stderr,
_("Please log in (using, e.g., \"su\") as the
(unprivileged) user that will\n"
"own the server process.\n"));
exit(1);
}
src/bin/pg_ctl/pg_ctl.c:
if (geteuid() == 0)
{
write_stderr(_("%s: cannot be run as root\n"
"Please log in (using, e.g., \"su\") as the "
"(unprivileged) user that will\n"
"own the server process.\n"),
progname);
exit(1);
}
src/bin/pg_upgrade/option.c:
if (os_user_effective_id == 0)
pg_fatal("%s: cannot be run as root\n", os_info.progname);
I wonder if it would be worth settling on a common message and way of emitting
it, each utility does it slightly differently.
> A backpatch could be surprising for some users as that's a behavior
> change, so I would recommend not to do a backpatch.
Agreed.
Regards
Ian Barwick
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 238b671f7a..dfa816cbae 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2077,6 +2077,22 @@ main(int argc, char **argv)
}
}
+ /*
+ * Don't allow pg_basebackup to be run as root, to avoid creating
+ * files in the data directory with ownership rights incompatible
+ * with the postmaster.
+ */
+#ifndef WIN32
+ if (geteuid() == 0) /* 0 is root's uid */
+ {
+ pg_log_error("cannot be run as root");
+ fprintf(stderr,
+ _("Please log in (using, e.g., \"su\") as the (unprivileged) user that will\n"
+ "own the server process.\n"));
+ exit(1);
+ }
+#endif
+
atexit(cleanup_directories_atexit);
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
