From: | Ian Barwick <ian(dot)barwick(at)2ndquadrant(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Prevent pg_basebackup running as root |
Date: | 2020-01-30 06:38:54 |
Message-ID: | CABvVfJUmTqUs3My3GBkNKr=4xjMSU-VJVRe5gabsNjieGempTA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2020年1月30日(木) 14:57 Michael Paquier <michael(at)paquier(dot)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
Attachment | Content-Type | Size |
---|---|---|
pg_basebackup-no-root.v2.patch | text/x-patch | 844 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2020-01-30 06:45:39 | Re: Enabling B-Tree deduplication by default |
Previous Message | Amit Langote | 2020-01-30 06:14:31 | Re: Hash join not finding which collation to use for string hashing |