Re: Server-side base backup: why superuser, not pg_write_server_files?

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Robert Haas <robertmhaas@gmail.com> writes:
>
>> On Fri, Jan 28, 2022 at 5:58 AM Dagfinn Ilmari Mannsåker
>> <ilmari@ilmari.org> wrote:
>>> I just noticed that the new server-side base backup feature requires
>>> superuser privileges (which is only documented in the pg_basebackup
>>> manual, not in the streaming replication protocol specification).
>>>
>>> Isn't this the kind of thing the pg_write_server_files role was created
>>> for, so that it can be delegated to a non-superuser?
>>
>> That's a good idea. I didn't think of that. Would you like to propose a patch?
>
> Sure, I'll try and whip something up over the weekend.

Or now. Patch attached.

- ilmari

From 2b5f078905fd463fc33d8ef259e93972ea17cd34 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 28 Jan 2022 15:54:07 +0000
Subject: [PATCH] Allow BASE_BACKUP TARGET 'server' to pg_write_server_files
 members

---
 doc/src/sgml/protocol.sgml                  | 5 +++++
 doc/src/sgml/ref/pg_basebackup.sgml         | 3 ++-
 src/backend/replication/basebackup_server.c | 6 ++++--
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 68908dcb7b..24e93f9b28 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2647,6 +2647,11 @@
           <literal>blackhole</literal>, the backup data is not sent
           anywhere; it is simply discarded.
          </para>
+
+         <para>
+          The <literal>server</literal> target requires superuser privilege or
+          being granted the <literal>pg_write_server_files</literal> role.
+         </para>
         </listitem>
        </varlistentry>
 
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index a5e03d2c66..d6b3cb18e3 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -237,7 +237,8 @@
         <literal>server:/some/path</literal>, the backup will be stored on
         the machine where the server is running in the
         <literal>/some/path</literal> directory. Storing a backup on the
-        server requires superuser privileges. If the target is set to
+        server requires superuser privileges or being granted the
+        <literal>pg_write_server_files</literal> role. If the target is set to
         <literal>blackhole</literal>, the contents are discarded and not
         stored anywhere. This should only be used for testing purposes, as you
         will not end up with an actual backup.
diff --git a/src/backend/replication/basebackup_server.c b/src/backend/replication/basebackup_server.c
index ce1b7b4797..18b0e11d90 100644
--- a/src/backend/replication/basebackup_server.c
+++ b/src/backend/replication/basebackup_server.c
@@ -10,10 +10,12 @@
  */
 #include "postgres.h"
 
+#include "catalog/pg_authid.h"
 #include "miscadmin.h"
 #include "replication/basebackup.h"
 #include "replication/basebackup_sink.h"
 #include "storage/fd.h"
+#include "utils/acl.h"
 #include "utils/timestamp.h"
 #include "utils/wait_event.h"
 
@@ -65,10 +67,10 @@ bbsink_server_new(bbsink *next, char *pathname)
     sink->base.bbs_next = next;
 
     /* Replication permission is not sufficient in this case. */
-    if (!superuser())
+    if (!is_member_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
         ereport(ERROR,
                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                 errmsg("must be superuser to create server backup")));
+                 errmsg("must be superuser or a member of the pg_write_server_files role to create server backup")));
 
     /*
      * It's not a good idea to store your backups in the same directory that
-- 
2.30.2