Re: Prevent pg_basebackup running as root

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 :)

> + /*
> + * 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.
*/

> + #ifndef WIN32

Indentation here.

> + 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);

A backpatch could be surprising for some users as that's a behavior
change, so I would recommend not to do a backpatch.
--
Michael