Обсуждение: Server-side base backup: why superuser, not pg_write_server_files?

Hi Hackers,

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?

- ilmari

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?

--
Robert Haas
EDB: http://www.enterprisedb.com

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.

- ilmari

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

On Fri, Jan 28, 2022 at 12:16 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> Or now. Patch attached.

LGTM. Committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:
> LGTM. Committed.

Thanks!

- ilmari

On Fri, Jan 28, 2022 at 12:35 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:
> > LGTM. Committed.
>
> Thanks!

It appears that neither of us actually tested that this works. For me,
it works when I test as a superuser, but if I test as a non-superuser
with or without pg_write_server_files, it crashes, because we end up
trying to do syscache lookups without a transaction environment. I
*think* that the attached is a sufficient fix; at least, it passes
simple testing.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

> On Fri, Jan 28, 2022 at 12:35 PM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
>> On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:
>> > LGTM. Committed.
>>
>> Thanks!
>
> It appears that neither of us actually tested that this works.

Oops!

> For me, it works when I test as a superuser, but if I test as a
> non-superuser with or without pg_write_server_files, it crashes,
> because we end up trying to do syscache lookups without a transaction
> environment. I *think* that the attached is a sufficient fix; at
> least, it passes simple testing.

Here's a follow-on patch that adds a test for non-superuser server-side
basebackup, which crashes without your patch and passes with it.

- ilmari

From e88af403706338d068a7156d2a9c02e27196ce12 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 2 Feb 2022 15:40:55 +0000
Subject: [PATCH] Test server-side basebackup as non-superuser

---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index a827be5e59..2283a8c42d 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -10,7 +10,7 @@
 use Fcntl qw(:seek);
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
-use Test::More tests => 143;
+use Test::More;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -521,6 +521,15 @@
 ok(-f "$tempdir/backuponserver/base.tar", 'backup tar was created');
 rmtree("$tempdir/backuponserver");
 
+$node->command_ok(
+    [qw(createuser --replication --role=pg_write_server_files backupuser)],
+    'create backup user');
+$node->command_ok(
+    [ @pg_basebackup_defs, '-U', 'backupuser', '--target', "server:$real_tempdir/backuponserver", '-X', 'none' ],
+    'backup target server');
+ok(-f "$tempdir/backuponserver/base.tar", 'backup tar was created as non-superuser');
+rmtree("$tempdir/backuponserver");
+
 $node->command_fails(
     [
         @pg_basebackup_defs,         '-D',
@@ -768,3 +777,5 @@
     rmtree("$tempdir/backup_gzip2");
     rmtree("$tempdir/backup_gzip3");
 }
+
+done_testing();
-- 
2.30.2

On Wed, Feb 2, 2022 at 10:42 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> Here's a follow-on patch that adds a test for non-superuser server-side
> basebackup, which crashes without your patch and passes with it.

This seems like a good idea, but I'm not going to slip a change from
an exact test count to done_testing() into a commit on some other
topic...

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> This seems like a good idea, but I'm not going to slip a change from
> an exact test count to done_testing() into a commit on some other
> topic...

Actually, it seemed that the consensus in the nearby thread [1]
was to start doing exactly that, rather than try to convert them
all in one big push.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/9D4FFB61-392B-4A2C-B7E4-911797B4AC14%40yesql.se

On Wed, Feb 2, 2022 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > This seems like a good idea, but I'm not going to slip a change from
> > an exact test count to done_testing() into a commit on some other
> > topic...
>
> Actually, it seemed that the consensus in the nearby thread [1]
> was to start doing exactly that, rather than try to convert them
> all in one big push.

Urk. Well, OK then.

Such an approach seems to me to have essentially nothing to recommend
it, but I just work here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 2, 2022 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Actually, it seemed that the consensus in the nearby thread [1]
>> was to start doing exactly that, rather than try to convert them
>> all in one big push.

> Urk. Well, OK then.

> Such an approach seems to me to have essentially nothing to recommend
> it, but I just work here.

Well, if someone wants to step up and provide a patch that changes 'em
all at once, that'd be great.  But we've discussed this before and
nothing's happened.

            regards, tom lane

On Wed, Feb 2, 2022 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, if someone wants to step up and provide a patch that changes 'em
> all at once, that'd be great.  But we've discussed this before and
> nothing's happened.

I mean, I don't understand why it's even better. And I would go so far
as to say that if nobody can be bothered to do the work to convert
everything at once, it probably isn't better.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Feb 2, 2022 at 1:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 2, 2022 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Well, if someone wants to step up and provide a patch that changes 'em
> > all at once, that'd be great.  But we've discussed this before and
> > nothing's happened.
>
> I mean, I don't understand why it's even better. And I would go so far
> as to say that if nobody can be bothered to do the work to convert
> everything at once, it probably isn't better.

And one thing that concretely stinks about is the progress reporting
you get while the tests are running:

t/010_pg_basebackup.pl ... 142/?

That's definitely less informative than 142/330 or whatever.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On 2 Feb 2022, at 19:58, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Feb 2, 2022 at 1:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Feb 2, 2022 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Well, if someone wants to step up and provide a patch that changes 'em
>>> all at once, that'd be great.  But we've discussed this before and
>>> nothing's happened.
>>
>> I mean, I don't understand why it's even better. And I would go so far
>> as to say that if nobody can be bothered to do the work to convert
>> everything at once, it probably isn't better.

I personally think it's better, so I went and did the work.  The attached is a
first pass over the tree to see what such a patch would look like.  This should
get a thread of it's own and not be hidden here but as it was discussed I piled
on for now.

> And one thing that concretely stinks about is the progress reporting
> you get while the tests are running:
>
> t/010_pg_basebackup.pl ... 142/?
>
> That's definitely less informative than 142/330 or whatever.

There is that.  That's less informative, but only when looking at the tests
while they are running.  There is no difference once the tests has finished so
CI runs etc are no less informative.  This however is something to consider.

--
Daniel Gustafsson        https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 2 Feb 2022, at 19:58, Robert Haas <robertmhaas@gmail.com> wrote:
>> And one thing that concretely stinks about is the progress reporting
>> you get while the tests are running:
>>
>> t/010_pg_basebackup.pl ... 142/?
>>
>> That's definitely less informative than 142/330 or whatever.

> There is that.  That's less informative, but only when looking at the tests
> while they are running.  There is no difference once the tests has finished so
> CI runs etc are no less informative.  This however is something to consider.

TBH I don't see that that's worth much.  None of our tests run so long
that you'll be sitting there trying to estimate when it'll be done.
I'd rather have the benefit of not having to maintain the test counts.

            regards, tom lane

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Here's a follow-on patch that adds a test for non-superuser server-side
> basebackup, which crashes without your patch and passes with it.

The Windows animals don't like this:

# Running: pg_basebackup --no-sync -cfast -U backupuser --target
server:C:\\prog\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\tmp_check\\tmp_test_VGMM/backuponserver-X none 
pg_basebackup: error: connection to server at "127.0.0.1", port 59539 failed: FATAL:  SSPI authentication failed for
user"backupuser" 
not ok 108 - backup target server

#   Failed test 'backup target server'
#   at t/010_pg_basebackup.pl line 527.

Not sure whether we have a standard method to get around that.

            regards, tom lane

I wrote:
> The Windows animals don't like this:
> pg_basebackup: error: connection to server at "127.0.0.1", port 59539 failed: FATAL:  SSPI authentication failed for
user"backupuser" 

> Not sure whether we have a standard method to get around that.

Ah, right, we do.  Looks like adding something like

auth_extra => [ '--create-role', 'backupuser' ]

to the $node->init call would do it, or you could mess with
invoking pg_regress --config-auth directly.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> I wrote:
>> The Windows animals don't like this:
>> pg_basebackup: error: connection to server at "127.0.0.1", port 59539
>> failed: FATAL: SSPI authentication failed for user "backupuser"
>
>> Not sure whether we have a standard method to get around that.
>
> Ah, right, we do.  Looks like adding something like
>
> auth_extra => [ '--create-role', 'backupuser' ]
>
> to the $node->init call would do it, or you could mess with
> invoking pg_regress --config-auth directly.

This was enough incentive for me to set up Cirrus-CI for my fork on
GitHub, and the auth_extra approach in the attached patch fixed the
test:

https://cirrus-ci.com/task/6578617030279168?logs=test_bin#L21

- ilmari

From fec0e29ac0f57d1449229849b837438dfb0b9a07 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 3 Feb 2022 15:45:50 +0000
Subject: [PATCH] Fix non-superuser server-side basebackup test on Windows

Windows doesn't do trust auth for local users by default, but needs
explicitly allowing the OS user to authenticate as the extra user.
---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 2283a8c42d..f996c6e9b9 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -31,7 +31,7 @@
 umask(0077);
 
 # Initialize node without replication settings
-$node->init(extra => ['--data-checksums']);
+$node->init(extra => ['--data-checksums'], auth_extra => ['--create-role', 'backupuser']);
 $node->start;
 my $pgdata = $node->data_dir;
 
-- 
2.30.2

On 2/2/22 17:52, Tom Lane wrote:
> I wrote:
>> The Windows animals don't like this:
>> pg_basebackup: error: connection to server at "127.0.0.1", port 59539 failed: FATAL:  SSPI authentication failed for
user"backupuser"
 
>> Not sure whether we have a standard method to get around that.
> Ah, right, we do.  Looks like adding something like
>
> auth_extra => [ '--create-role', 'backupuser' ]
>
> to the $node->init call would do it, or you could mess with
> invoking pg_regress --config-auth directly.
>
>             



I've fixed this using the auth_extra method, which avoids a reload.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Feb 3, 2022 at 12:26 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> I've fixed this using the auth_extra method, which avoids a reload.

Thank you much.

-- 
Robert Haas
EDB: http://www.enterprisedb.com