Re: Non-superuser subscription owners
On Thu, Jun 15, 2023 at 11:18 PM Alvaro Herrera wrote: > > On 2023-Jun-13, Amit Kapila wrote: > > > I'll push this tomorrow unless there are any suggestions or comments. > > Note the proposed commit message is wrong about which commit is to blame > for the original problem -- it mentions e7e7da2f8d57 twice, but one of > them is actually c3afe8cf5a1e. > Right, I also noticed this and changed it before pushing, See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b5c517379a40fa1af84c0852aa3730a5875a6482 -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On 2023-Jun-13, Amit Kapila wrote: > I'll push this tomorrow unless there are any suggestions or comments. Note the proposed commit message is wrong about which commit is to blame for the original problem -- it mentions e7e7da2f8d57 twice, but one of them is actually c3afe8cf5a1e. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
RE: Non-superuser subscription owners
On Wednesday, June 14, 2023 10:11 AM Amit Kapila wrote: > > On Tue, Jun 13, 2023 at 2:25 PM Amit Kapila wrote: > > > > On Fri, May 12, 2023 at 3:28 PM Zhijie Hou (Fujitsu) > > wrote: > > > > > > > > > I can reproduce this via gdb following similar steps in [1]. > > > > > > I think we need to move this call into a transaction as well and > > > here is an attempt to do that. > > > > > > > I am able to reproduce this issue following the steps mentioned by you > > and the proposed patch to fix the issue looks good to me. > > > > Today, again looking at the patch, it seems to me that it would be better if > we > can fix this without starting a new transaction. Won't it be better if we > move this > syscall to a place where we are fetching relstate (GetSubscriptionRelState()) > a > few lines above? I understand by doing that in some cases like when copy_data > = false, we may do this syscall unnecessarily but OTOH, starting a new > transaction just for a syscall (superuser_arg()) also doesn't seem like a good > idea to me. Makes sense to me, here is the updated patch which does the same. Best Regards, Hou zj v2-0001-Fix-possible-logical-replication-table-sync-crash.patch Description: v2-0001-Fix-possible-logical-replication-table-sync-crash.patch
Re: Non-superuser subscription owners
On Tue, Jun 13, 2023 at 2:25 PM Amit Kapila wrote: > > On Fri, May 12, 2023 at 3:28 PM Zhijie Hou (Fujitsu) > wrote: > > > > > > I can reproduce this via gdb following similar steps in [1]. > > > > I think we need to move this call into a transaction as well and here is an > > attempt > > to do that. > > > > I am able to reproduce this issue following the steps mentioned by you > and the proposed patch to fix the issue looks good to me. > Today, again looking at the patch, it seems to me that it would be better if we can fix this without starting a new transaction. Won't it be better if we move this syscall to a place where we are fetching relstate (GetSubscriptionRelState()) a few lines above? I understand by doing that in some cases like when copy_data = false, we may do this syscall unnecessarily but OTOH, starting a new transaction just for a syscall (superuser_arg()) also doesn't seem like a good idea to me. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Tue, Jun 13, 2023 at 2:25 PM Amit Kapila wrote: > > On Fri, May 12, 2023 at 3:28 PM Zhijie Hou (Fujitsu) > wrote: > > > > > > I can reproduce this via gdb following similar steps in [1]. > > > > I think we need to move this call into a transaction as well and here is an > > attempt > > to do that. > > > > I am able to reproduce this issue following the steps mentioned by you > and the proposed patch to fix the issue looks good to me. > I'll push this tomorrow unless there are any suggestions or comments. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Fri, May 12, 2023 at 3:28 PM Zhijie Hou (Fujitsu) wrote: > > > I can reproduce this via gdb following similar steps in [1]. > > I think we need to move this call into a transaction as well and here is an > attempt > to do that. > I am able to reproduce this issue following the steps mentioned by you and the proposed patch to fix the issue looks good to me. -- With Regards, Amit Kapila.
RE: Non-superuser subscription owners
On Tuesday, April 4, 2023 1:57 AM Robert Haas wrote: > > On Sat, Apr 1, 2023 at 12:00 PM Alexander Lakhin > wrote: > > I've managed to reproduce it using the following script: > > for ((i=1;i<=10;i++)); do > > echo "iteration $i" > > echo " > > CREATE ROLE sub_user; > > CREATE SUBSCRIPTION testsub CONNECTION 'dbname=db' > > PUBLICATION testpub WITH (connect = false); ALTER SUBSCRIPTION > > testsub ENABLE; DROP SUBSCRIPTION testsub; SELECT pg_sleep(0.001); > > DROP ROLE sub_user; " | psql psql -c "ALTER SUBSCRIPTION testsub > > DISABLE;" > > psql -c "ALTER SUBSCRIPTION testsub SET (slot_name = NONE);" > > psql -c "DROP SUBSCRIPTION testsub;" > > grep 'TRAP' server.log && break > > done > > After a bit of experimentation this repro worked for me -- I needed > -DRELCACHE_FORCE_RELEASE as well, and a bigger iteration count. I verified > that the patch fixed it, and committed the patch with the addition of a > comment. Thanks for pushing! While testing this, I found a similar problem in table sync worker, as we also invoke superuser_arg() in table sync worker which is not in a transaction. LogicalRepSyncTableStart ... /* Is the use of a password mandatory? */ must_use_password = MySubscription->passwordrequired && !superuser_arg(MySubscription->owner); #0 0x7f18bb55aaff in raise () from /lib64/libc.so.6 #1 0x7f18bb52dea5 in abort () from /lib64/libc.so.6 #2 0x00b69a22 in ExceptionalCondition (conditionName=0xda4338 "IsTransactionState()", fileName=0xda403e "catcache.c", lineNumber=1208) at assert.c:66 #3 0x00b4842a in SearchCatCacheInternal (cache=0x27cab80, nkeys=1, v1=10, v2=0, v3=0, v4=0) at catcache.c:1208 #4 0x00b48329 in SearchCatCache1 (cache=0x27cab80, v1=10) at catcache.c:1162 #5 0x00b630c7 in SearchSysCache1 (cacheId=11, key1=10) at syscache.c:825 #6 0x00b982e3 in superuser_arg (roleid=10) at superuser.c:70 I can reproduce this via gdb following similar steps in [1]. I think we need to move this call into a transaction as well and here is an attempt to do that. [1] https://www.postgresql.org/message-id/OS0PR01MB5716E596E4FB83DE46F592FE948C9%40OS0PR01MB5716.jpnprd01.prod.outlook.com Best Regards, Hou zj 0001-Fix-possible-logical-replication-table-sync-crash.patch Description: 0001-Fix-possible-logical-replication-table-sync-crash.patch
Re: Non-superuser subscription owners
On Fri, Apr 21, 2023 at 6:21 PM Robert Haas wrote: > > On Fri, Apr 21, 2023 at 8:19 AM Amit Kapila wrote: > > LGTM. Let's see if Robert or others have any comments, otherwise, I'll > > push this early next week. > > LGTM too. > Pushed. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Fri, Apr 21, 2023 at 8:19 AM Amit Kapila wrote: > LGTM. Let's see if Robert or others have any comments, otherwise, I'll > push this early next week. LGTM too. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Fri, Apr 21, 2023 at 12:30 PM vignesh C wrote: > > On Fri, 21 Apr 2023 at 01:49, Robert Haas wrote: > > > > On Thu, Apr 20, 2023 at 1:08 AM Amit Kapila wrote: > > > Pushed. I noticed that we didn't display this new subscription option > > > 'password_required' in \dRs+: > > > > > > postgres=# \dRs+ > > > > > > List of subscriptions > > > Name | Owner | Enabled | Publication | Binary | Streaming | > > > Two-phase commit | Disable on error | Origin | Run as Owner? | > > > Synchronous commit |Conninfo | Skip LSN > > > > > > Is that intentional? Sorry, if it was discussed previously because I > > > haven't followed this discussion in detail. > > > > No, I don't think that's intentional. I just didn't think about it. > > Here is a patch to display Password required with \dRs+ command. Also > added one test to describe subscription when password_required is > false, as all the existing tests were there only for password_required > as true. > LGTM. Let's see if Robert or others have any comments, otherwise, I'll push this early next week. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Fri, 21 Apr 2023 at 01:49, Robert Haas wrote:
>
> On Thu, Apr 20, 2023 at 1:08 AM Amit Kapila wrote:
> > Pushed. I noticed that we didn't display this new subscription option
> > 'password_required' in \dRs+:
> >
> > postgres=# \dRs+
> >
> > List of subscriptions
> > Name | Owner | Enabled | Publication | Binary | Streaming |
> > Two-phase commit | Disable on error | Origin | Run as Owner? |
> > Synchronous commit |Conninfo | Skip LSN
> >
> > Is that intentional? Sorry, if it was discussed previously because I
> > haven't followed this discussion in detail.
>
> No, I don't think that's intentional. I just didn't think about it.
Here is a patch to display Password required with \dRs+ command. Also
added one test to describe subscription when password_required is
false, as all the existing tests were there only for password_required
as true.
Regards,
Vignesh
From d0379b915a449d5763bd7665a1ec32edad009bd3 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Fri, 21 Apr 2023 11:54:17 +0530
Subject: [PATCH] Display 'password_required' option for \dRs+ command.
In commit c3afe8cf5a, we added the option 'password_required', this option
should be displayed with \dRs+ command.
---
src/bin/psql/describe.c| 4 +-
src/test/regress/expected/subscription.out | 151 +++--
src/test/regress/sql/subscription.sql | 2 +
3 files changed, 84 insertions(+), 73 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 83a37ee601..058e41e749 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6493,7 +6493,7 @@ describeSubscriptions(const char *pattern, bool verbose)
PGresult *res;
printQueryOpt myopt = pset.popt;
static const bool translate_columns[] = {false, false, false, false,
- false, false, false, false, false, false, false, false, false};
+ false, false, false, false, false, false, false, false, false, false};
if (pset.sversion < 10)
{
@@ -6551,8 +6551,10 @@ describeSubscriptions(const char *pattern, bool verbose)
if (pset.sversion >= 16)
appendPQExpBuffer(,
", suborigin AS \"%s\"\n"
+ ", subpasswordrequired AS \"%s\"\n"
", subrunasowner AS \"%s\"\n",
gettext_noop("Origin"),
+ gettext_noop("Password required"),
gettext_noop("Run as Owner?"));
appendPQExpBuffer(,
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 9c52890f1d..d736246259 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -115,18 +115,18 @@ CREATE SUBSCRIPTION regress_testsub4 CONNECTION 'dbname=regress_doesnotexist' PU
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and refresh the subscription.
\dRs+ regress_testsub4
- List of subscriptions
- Name | Owner | Enabled | Publication | Binary | Streaming | Two-phase commit | Disable on error | Origin | Run as Owner? | Synchronous commit | Conninfo | Skip LSN
---+---+-+-++---+--+--++---++-+--
- regress_testsub4 | regress_subscription_user | f | {testpub} | f | off | d| f| none | f | off| dbname=regress_doesnotexist | 0/0
+ List of subscriptions
+ Name | Owner | Enabled | Publication | Binary | Streaming | Two-phase commit | Disable on error | Origin | Password required | Run as Owner? | Synchronous commit | Conninfo | Skip LSN
+--+---+-+-++---+--+--++---+---++-+--
+ regress_testsub4 | regress_subscription_user | f | {testpub} | f | off | d| f| none | t | f | off| dbname=regress_doesnotexist | 0/0
(1 row)
ALTER SUBSCRIPTION regress_testsub4 SET (origin = any);
\dRs+ regress_testsub4
- List of subscriptions
- Name | Owner | Enabled | Publication | Binary | Streaming | Two-phase commit | Disable on error | Origin | Run as Owner? | Synchronous commit | Conninfo | Skip LSN
Re: Non-superuser subscription owners
On Thu, Apr 20, 2023 at 1:08 AM Amit Kapila wrote: > Pushed. I noticed that we didn't display this new subscription option > 'password_required' in \dRs+: > > postgres=# \dRs+ > > List of subscriptions > Name | Owner | Enabled | Publication | Binary | Streaming | > Two-phase commit | Disable on error | Origin | Run as Owner? | > Synchronous commit |Conninfo | Skip LSN > > Is that intentional? Sorry, if it was discussed previously because I > haven't followed this discussion in detail. No, I don't think that's intentional. I just didn't think about it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Thu, Apr 13, 2023 at 8:02 AM Amit Kapila wrote: > > On Wed, Apr 12, 2023 at 5:50 PM Robert Haas wrote: > > > > On Tue, Apr 11, 2023 at 10:56 PM Amit Kapila > > wrote: > > > Anyway, I don't think as such there is any problem > > > with restarting the worker even when the subscription owner is a > > > superuser, so adjusted the check accordingly. > > > > LGTM. > > > > Thanks. I am away for a few days so can push it only next week. > Pushed. I noticed that we didn't display this new subscription option 'password_required' in \dRs+: postgres=# \dRs+ List of subscriptions Name | Owner | Enabled | Publication | Binary | Streaming | Two-phase commit | Disable on error | Origin | Run as Owner? | Synchronous commit |Conninfo | Skip LSN Is that intentional? Sorry, if it was discussed previously because I haven't followed this discussion in detail. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Wed, Apr 12, 2023 at 5:50 PM Robert Haas wrote: > > On Tue, Apr 11, 2023 at 10:56 PM Amit Kapila wrote: > > Anyway, I don't think as such there is any problem > > with restarting the worker even when the subscription owner is a > > superuser, so adjusted the check accordingly. > > LGTM. > Thanks. I am away for a few days so can push it only next week. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Tue, Apr 11, 2023 at 10:56 PM Amit Kapila wrote: > Anyway, I don't think as such there is any problem > with restarting the worker even when the subscription owner is a > superuser, so adjusted the check accordingly. LGTM. I realize we could do more sophisticated things here, but I think it's better to keep the code simple. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Tue, Apr 11, 2023 at 8:21 PM Robert Haas wrote: > > On Tue, Apr 11, 2023 at 5:53 AM Amit Kapila wrote: > > I think additionally, we should check that the new owner of the > > subscription is not a superuser, otherwise, anyway, this parameter is > > ignored. Please find the attached to add this check. > > I don't see why we should check that. It makes this different from all > the other cases and I don't see any benefit. > I thought it would be better if we don't restart the worker unless it is required. In case, the subscription's owner is a superuser, the 'password_required' is ignored, so why restart the apply worker when somebody changes it in such a case? I understand that there may not be a need to change the 'password_required' option when the subscription's owner is the superuser but one may first choose to change the password_required flag and then the owner of a subscription to a non-superuser. Anyway, I don't think as such there is any problem with restarting the worker even when the subscription owner is a superuser, so adjusted the check accordingly. -- With Regards, Amit Kapila. exit_on_sub_opt_change_2.patch Description: Binary data
Re: Non-superuser subscription owners
On Tue, Apr 11, 2023 at 5:53 AM Amit Kapila wrote: > I think additionally, we should check that the new owner of the > subscription is not a superuser, otherwise, anyway, this parameter is > ignored. Please find the attached to add this check. I don't see why we should check that. It makes this different from all the other cases and I don't see any benefit. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Mon, Apr 10, 2023 at 9:15 PM Robert Haas wrote: > > On Sat, Apr 8, 2023 at 1:35 AM Amit Kapila wrote: > > Do we need to have a check for this new option "password_required" in > > maybe_reread_subscription() where we "Exit if any parameter that > > affects the remote connection was changed."? This new option is > > related to the remote connection so I thought it is worth considering > > whether we want to exit and restart the apply worker when this option > > is changed. > > Hmm, good question. I think that's probably a good idea. If the > current connection is already working, the only possible result of > getting rid of it and trying to create a new one is that it might now > fail instead, but someone might want that behavior. Otherwise, they'd > instead find the failure at a later, maybe less convenient, time. > I think additionally, we should check that the new owner of the subscription is not a superuser, otherwise, anyway, this parameter is ignored. Please find the attached to add this check. -- With Regards, Amit Kapila. exit_on_sub_opt_change_1.patch Description: Binary data
Re: Non-superuser subscription owners
On Sat, Apr 8, 2023 at 1:35 AM Amit Kapila wrote: > Do we need to have a check for this new option "password_required" in > maybe_reread_subscription() where we "Exit if any parameter that > affects the remote connection was changed."? This new option is > related to the remote connection so I thought it is worth considering > whether we want to exit and restart the apply worker when this option > is changed. Hmm, good question. I think that's probably a good idea. If the current connection is already working, the only possible result of getting rid of it and trying to create a new one is that it might now fail instead, but someone might want that behavior. Otherwise, they'd instead find the failure at a later, maybe less convenient, time. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Thu, Mar 30, 2023 at 9:35 PM Robert Haas wrote: > > On Tue, Mar 28, 2023 at 1:52 PM Jeff Davis wrote: > > On Fri, 2023-03-24 at 00:17 -0700, Jeff Davis wrote: > > > The other patch you posted seems like it makes a lot of progress in > > > that direction, and I think that should go in first. That was one of > > > the items I suggested previously[2], so thank you for working on > > > that. > > > > The above is not a hard objection. > > The other patch is starting to go in a direction that is going to have > some conflicts with this one, so I went ahead and committed this one > to avoid rebasing pain. > Do we need to have a check for this new option "password_required" in maybe_reread_subscription() where we "Exit if any parameter that affects the remote connection was changed."? This new option is related to the remote connection so I thought it is worth considering whether we want to exit and restart the apply worker when this option is changed. -- With Regards, Amit Kapila.
Re: Non-superuser subscription owners
On Sat, Apr 1, 2023 at 12:00 PM Alexander Lakhin wrote: > I've managed to reproduce it using the following script: > for ((i=1;i<=10;i++)); do > echo "iteration $i" > echo " > CREATE ROLE sub_user; > CREATE SUBSCRIPTION testsub CONNECTION 'dbname=db' > PUBLICATION testpub WITH (connect = false); > ALTER SUBSCRIPTION testsub ENABLE; > DROP SUBSCRIPTION testsub; > SELECT pg_sleep(0.001); > DROP ROLE sub_user; > " | psql > psql -c "ALTER SUBSCRIPTION testsub DISABLE;" > psql -c "ALTER SUBSCRIPTION testsub SET (slot_name = NONE);" > psql -c "DROP SUBSCRIPTION testsub;" > grep 'TRAP' server.log && break > done After a bit of experimentation this repro worked for me -- I needed -DRELCACHE_FORCE_RELEASE as well, and a bigger iteration count. I verified that the patch fixed it, and committed the patch with the addition of a comment. Thanks very much for this repro, and likewise many thanks to Hou Zhijie for the report and patch. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi,
On April 1, 2023 9:00:00 AM PDT, Alexander Lakhin wrote:
>Hello Robert,
>
>31.03.2023 23:00, Robert Haas wrote:
>> That looks like a reasonable fix but I can't reproduce the problem
>> locally. I thought the reason why that machine sees the problem might
>> be that it uses -DRELCACHE_FORCE_RELEASE, but I tried that option here
>> and the tests still pass. Anyone ideas how to reproduce?
>
>I've managed to reproduce it using the following script:
>for ((i=1;i<=10;i++)); do
>echo "iteration $i"
>echo "
>CREATE ROLE sub_user;
>CREATE SUBSCRIPTION testsub CONNECTION 'dbname=db'
> PUBLICATION testpub WITH (connect = false);
>ALTER SUBSCRIPTION testsub ENABLE;
>DROP SUBSCRIPTION testsub;
>SELECT pg_sleep(0.001);
>DROP ROLE sub_user;
>" | psql
>psql -c "ALTER SUBSCRIPTION testsub DISABLE;"
>psql -c "ALTER SUBSCRIPTION testsub SET (slot_name = NONE);"
>psql -c "DROP SUBSCRIPTION testsub;"
>grep 'TRAP' server.log && break
>done
>
>iteration 3
>CREATE ROLE
>...
>ALTER SUBSCRIPTION
>WARNING: terminating connection because of crash of another server process
>DETAIL: The postmaster has commanded this server process to roll back the
>current transaction and exit, because ano
>ther server process exited abnormally and possibly corrupted shared memory.
>HINT: In a moment you should be able to reconnect to the database and repeat
>your command.
>server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
>connection to server was lost
>TRAP: failed Assert("IsTransactionState()"), File: "catcache.c", Line: 1208,
>PID: 1001242
Errors like that are often easier to reproduce with clobber caches (or whatever
the name is these days) enabled.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Non-superuser subscription owners
Hello Robert,
31.03.2023 23:00, Robert Haas wrote:
That looks like a reasonable fix but I can't reproduce the problem
locally. I thought the reason why that machine sees the problem might
be that it uses -DRELCACHE_FORCE_RELEASE, but I tried that option here
and the tests still pass. Anyone ideas how to reproduce?
I've managed to reproduce it using the following script:
for ((i=1;i<=10;i++)); do
echo "iteration $i"
echo "
CREATE ROLE sub_user;
CREATE SUBSCRIPTION testsub CONNECTION 'dbname=db'
PUBLICATION testpub WITH (connect = false);
ALTER SUBSCRIPTION testsub ENABLE;
DROP SUBSCRIPTION testsub;
SELECT pg_sleep(0.001);
DROP ROLE sub_user;
" | psql
psql -c "ALTER SUBSCRIPTION testsub DISABLE;"
psql -c "ALTER SUBSCRIPTION testsub SET (slot_name = NONE);"
psql -c "DROP SUBSCRIPTION testsub;"
grep 'TRAP' server.log && break
done
iteration 3
CREATE ROLE
...
ALTER SUBSCRIPTION
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the
current transaction and exit, because ano
ther server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat
your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost
TRAP: failed Assert("IsTransactionState()"), File: "catcache.c", Line: 1208,
PID: 1001242
Best regards,
Alexander
RE: Non-superuser subscription owners
On Saturday, April 1, 2023 4:00 AM Robert Haas Hi, > > On Thu, Mar 30, 2023 at 9:49 PM houzj.f...@fujitsu.com > wrote: > > It looks like the super user check is out of a transaction, I haven't > > checked why it only failed on one BF animal, but it seems we can put > > the check into the transaction like the following: > > That looks like a reasonable fix but I can't reproduce the problem locally. I > thought the reason why that machine sees the problem might be that it uses > -DRELCACHE_FORCE_RELEASE, but I tried that option here and the tests still > pass. > Anyone ideas how to reproduce? I think it's a timing problem because superuser_arg() function will cache the roleid that passed in last time, so it might not search the syscache to hit the Assert() check each time. And in the regression test, the roleid cache happened to be invalidated before the superuser_arg() by some concurrently ROLE change( maybe in subscription.sql and publication.sql). I can reproduce it by using gdb and starting another session to change the ROLE. When the apply worker starts, use the gdb to block the apply worker in the transaction before the super user check. Then start another session to ALTER ROLE to invalidate the roleid cache in superuser_arg() which will cause the apply worker to search the syscache and hit the Assert(). -- origin_startpos = replorigin_session_get_progress(false); B* CommitTransactionCommand(); /* Is the use of a password mandatory? */ must_use_password = MySubscription->passwordrequired && ! superuser_arg(MySubscription->owner); -- Best Regards, Hou zj
Re: Non-superuser subscription owners
On Thu, Mar 30, 2023 at 9:49 PM houzj.f...@fujitsu.com wrote: > It looks like the super user check is out of a transaction, I haven't checked > why > it only failed on one BF animal, but it seems we can put the check into the > transaction like the following: That looks like a reasonable fix but I can't reproduce the problem locally. I thought the reason why that machine sees the problem might be that it uses -DRELCACHE_FORCE_RELEASE, but I tried that option here and the tests still pass. Anyone ideas how to reproduce? -- Robert Haas EDB: http://www.enterprisedb.com
RE: Non-superuser subscription owners
On Friday, March 31, 2023 12:05 AM Robert Haas wrote: Hi, > > On Tue, Mar 28, 2023 at 1:52 PM Jeff Davis wrote: > > On Fri, 2023-03-24 at 00:17 -0700, Jeff Davis wrote: > > > The other patch you posted seems like it makes a lot of progress in > > > that direction, and I think that should go in first. That was one of > > > the items I suggested previously[2], so thank you for working on > > > that. > > > > The above is not a hard objection. > > The other patch is starting to go in a direction that is going to have some > conflicts with this one, so I went ahead and committed this one to avoid > rebasing pain. I noticed the BF[1] report a core dump after this commit. #0 0xfd581864 in _lwp_kill () from /usr/lib/libc.so.12 #0 0xfd581864 in _lwp_kill () from /usr/lib/libc.so.12 #1 0xfd5817dc in raise () from /usr/lib/libc.so.12 #2 0xfd581c88 in abort () from /usr/lib/libc.so.12 #3 0x01e6c8d4 in ExceptionalCondition (conditionName=conditionName@entry=0x2007758 "IsTransactionState()", fileName=fileName@entry=0x20565c4 "catcache.c", lineNumber=lineNumber@entry=1208) at assert.c:66 #4 0x01e4e404 in SearchCatCacheInternal (cache=0xfd21e500, nkeys=nkeys@entry=1, v1=v1@entry=28985, v2=v2@entry=0, v3=v3@entry=0, v4=v4@entry=0) at catcache.c:1208 #5 0x01e4eea0 in SearchCatCache1 (cache=, v1=v1@entry=28985) at catcache.c:1162 #6 0x01e66e34 in SearchSysCache1 (cacheId=cacheId@entry=11, key1=key1@entry=28985) at syscache.c:825 #7 0x01e98c40 in superuser_arg (roleid=28985) at superuser.c:70 #8 0x01c657bc in ApplyWorkerMain (main_arg=) at worker.c:4552 #9 0x01c1ceac in StartBackgroundWorker () at bgworker.c:861 #10 0x01c23be0 in do_start_bgworker (rw=) at postmaster.c:5762 #11 maybe_start_bgworkers () at postmaster.c:5986 #12 0x01c2459c in process_pm_pmsignal () at postmaster.c:5149 #13 ServerLoop () at postmaster.c:1770 #14 0x01c26cdc in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0xe0e4) at postmaster.c:1463 #15 0x01ee2c8c in main (argc=4, argv=0xe0e4) at main.c:200 It looks like the super user check is out of a transaction, I haven't checked why it only failed on one BF animal, but it seems we can put the check into the transaction like the following: diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 6fd674b5d6..08f10fc331 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -4545,12 +4545,13 @@ ApplyWorkerMain(Datum main_arg) replorigin_session_setup(originid, 0); replorigin_session_origin = originid; origin_startpos = replorigin_session_get_progress(false); - CommitTransactionCommand(); /* Is the use of a password mandatory? */ must_use_password = MySubscription->passwordrequired && !superuser_arg(MySubscription->owner); + CommitTransactionCommand(); + [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-03-30%2019%3A41%3A08 Best Regards, Hou Zhijie
Re: Non-superuser subscription owners
On Tue, Mar 28, 2023 at 1:52 PM Jeff Davis wrote: > On Fri, 2023-03-24 at 00:17 -0700, Jeff Davis wrote: > > The other patch you posted seems like it makes a lot of progress in > > that direction, and I think that should go in first. That was one of > > the items I suggested previously[2], so thank you for working on > > that. > > The above is not a hard objection. The other patch is starting to go in a direction that is going to have some conflicts with this one, so I went ahead and committed this one to avoid rebasing pain. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Fri, 2023-03-24 at 00:17 -0700, Jeff Davis wrote: > The other patch you posted seems like it makes a lot of progress in > that direction, and I think that should go in first. That was one of > the items I suggested previously[2], so thank you for working on > that. The above is not a hard objection. I still hold the opinion that the non-superuser subscriptions work is feels premature without the apply-as-table-owner work. It would be great if the other patch ends up ready quickly, which would moot the commit-ordering question. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Mon, 2023-03-27 at 14:06 -0400, Robert Haas wrote: > I thought you were asking for those changes to be made before this > patch got committed, so that's what I was responding to. If you're > asking for it not to be committed at all, that's a different > discussion. I separately had a complaint (in a separate subthread) about the scope of the predefined role you are introducing, which I think encompasses two concepts that should be treated differently and I think that may need to be revisited later. If you ignore this complaint it wouldn't be the end of the world. This subthread is about the order in which the patches get committed (which is a topic you brought up), not whether they are ever to be committed. > > I kind of agree with you about the feature itself. Even though the > basic feature works quite well and does something people really want, > there are a lot of loose ends to sort out, and not just about > security. But I also want to make some progress. If there are > problems > with what I'm proposing that will make us regret committing things > right before feature freeze, then we shouldn't. But waiting a whole > additional year to see any kind of improvement is not free; these > issues are serious. The non-superuser-subscription-owner patch without the apply-as-table- owner patch feels like a facade to me, at least right now. Perhaps I can be convinced otherwise, but that's what it looks like to me. > > I think this patch is a lot better-baked and less speculative than > that one. I think that patch is more important, so if they were > equally mature, I'd favor getting that one committed first. But > that's > not the case. You explicitly asked about the order of the patches, which made me think it was more of an option? If the apply-as-table-owner patch gets held up for whatever reason, we might have to make a difficult decision. I'd prefer focus on the apply- as-table-owner patch briefly, and now that it's getting some review attention, we might find out how ready it is quite soon. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Mon, 2023-03-27 at 10:46 -0700, Andres Freund wrote: > > There are some big issues, like the security model for replaying > > changes. > > That seems largely unrelated. They are self-evidently related in a fundamental way. The behavior of the non-superuser-subscription patch depends on the presence of the apply-as-table-owner patch. I think I'd like to understand the apply-as-table-owner patch better to understand the interaction. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Sat, Mar 25, 2023 at 3:16 PM Jeff Davis wrote: > On Fri, 2023-03-24 at 09:24 -0400, Robert Haas wrote: > > I certainly agree that the security model isn't in a reasonable place > > right now. However, I feel that: > > > > (1) adding an extra predefined role > > > (2) even adding the connection string security stuff > > I don't see how these points are related to the question of whether you > should commit your non-superuser-subscription-owners patch or logical- > repl-as-table-owner patch first. I thought you were asking for those changes to be made before this patch got committed, so that's what I was responding to. If you're asking for it not to be committed at all, that's a different discussion. > My perspective is that logical replication is an unfinished feature > with an incomplete design. As I said earlier, that's why I backed away > from trying to do non-superuser subscriptions as a documented feature: > it feels like we need to settle some of the underlying pieces first. I kind of agree with you about the feature itself. Even though the basic feature works quite well and does something people really want, there are a lot of loose ends to sort out, and not just about security. But I also want to make some progress. If there are problems with what I'm proposing that will make us regret committing things right before feature freeze, then we shouldn't. But waiting a whole additional year to see any kind of improvement is not free; these issues are serious. > I don't mean to say all of the above issues are blockers or that they > should all be resolved in my favor. But there are enough issues and > some of those issues are serious enough that I feel like it's premature > to just go ahead with the non-superuser subscriptions and the > predefined role. > > There are already users, which complicates things. And you make a good > point that some important users may be already working around the > flaws. But there's already a patch and discussion going on for some > security model improvements (thanks to you), so let's try to get that > one in first. If we can't, it's probably because we learned something > important. I think this patch is a lot better-baked and less speculative than that one. I think that patch is more important, so if they were equally mature, I'd favor getting that one committed first. But that's not the case. Also, I don't really understand how we could end up not wanting this patch. I mean there's a lot of things I don't understand that are still true anyway, so the mere fact that I don't understand how we could not end up wanting this patch doesn't mean that it couldn't happen. But like, the current state of play is that subscription owners are always going to be superusers at the time the subscription is created, and literally nobody thinks that's a good idea. Some people (like me) think that we ought to assume that subscription owners will be and need to be high-privilege users like superusers, but to my knowledge every such person thinks that it's OK for the subscription owner to be a non-superuser if they have adequate privileges. I just think that's a high amount of privileges, not that it has to be all the privileges i.e. superuser. Other people (like you, AIUI) think that we ought to try to set things up so that subscription owners can be low-privilege users, in which case we, once again, don't want the user who owns the subscription to start out a superuser. I actually can't imagine anyone defending the idea of having the subscription owner always be a superuser at the time they first own the subscription. That's a weird rule that can only serve to reduce security. Nor can I imagine anyone saying that forcing subscriptions to be created only by superusers improves security. I don't think anyone thinks that. If we're going to delay this patch, probably for a full year, because of other ongoing discussions, it should be because there is some outcome of those discussions that would involve deciding that this patch isn't needed or should be significantly redesigned. If this patch is going to end up being desirable no matter how those discussions turn out, and if it's not going to change significantly no matter how those discussions turn out, then those discussions aren't a reason not to get it into this release. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi, On 2023-03-25 12:16:35 -0700, Jeff Davis wrote: > On Fri, 2023-03-24 at 09:24 -0400, Robert Haas wrote: > > I certainly agree that the security model isn't in a reasonable place > > right now. However, I feel that: > > > > (1) adding an extra predefined role > > > (2) even adding the connection string security stuff > > I don't see how these points are related to the question of whether you > should commit your non-superuser-subscription-owners patch or logical- > repl-as-table-owner patch first. > > > My perspective is that logical replication is an unfinished feature > with an incomplete design. I agree with that much. > As I said earlier, that's why I backed away from trying to do non-superuser > subscriptions as a documented feature: it feels like we need to settle some > of the underlying pieces first. I don't agree. The patch allows to use logical rep in a far less dangerous fashion than now. The alternative is to release 16 without a real way to use logical rep less insanely. Which I think is work. > There are some big issues, like the security model for replaying > changes. That seems largely unrelated. > And some smaller issues like feature gaps (RLS doesn't work, > if I remember correctly, and maybe something with partitioning). Entirely unrelated? Greetings, Andres Freund
Re: Non-superuser subscription owners
On Fri, 2023-03-24 at 09:24 -0400, Robert Haas wrote: > I certainly agree that the security model isn't in a reasonable place > right now. However, I feel that: > > (1) adding an extra predefined role > (2) even adding the connection string security stuff I don't see how these points are related to the question of whether you should commit your non-superuser-subscription-owners patch or logical- repl-as-table-owner patch first. My perspective is that logical replication is an unfinished feature with an incomplete design. As I said earlier, that's why I backed away from trying to do non-superuser subscriptions as a documented feature: it feels like we need to settle some of the underlying pieces first. There are some big issues, like the security model for replaying changes. And some smaller issues like feature gaps (RLS doesn't work, if I remember correctly, and maybe something with partitioning). There are potential clashes with other proposals, like the CREATE SUBSCRIPTION ... SERVER, which I hope can be sorted out later. And I don't feel like I have a good handle on the publisher security model and threats, which hopefully is just a matter of documenting some best practices. Each time we dig into one of these issues I learn something, and I think others do, too. If we skip past that process and start adding new features on top of this unfinished design, then I think we are setting ourselves up for trouble that is going to be harder to fix later. I don't mean to say all of the above issues are blockers or that they should all be resolved in my favor. But there are enough issues and some of those issues are serious enough that I feel like it's premature to just go ahead with the non-superuser subscriptions and the predefined role. There are already users, which complicates things. And you make a good point that some important users may be already working around the flaws. But there's already a patch and discussion going on for some security model improvements (thanks to you), so let's try to get that one in first. If we can't, it's probably because we learned something important. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Fri, Mar 24, 2023 at 9:24 AM Robert Haas wrote: > > The other patch you posted seems like it makes a lot of progress in > > that direction, and I think that should go in first. That was one of > > the items I suggested previously[2], so thank you for working on that. > > Perhaps you could review that work? Ah, you already did. Thanks. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Fri, Mar 24, 2023 at 3:17 AM Jeff Davis wrote: > The current patch (non-superuser-subscriptions) is the most user-facing > aspect, and it seems wrong to commit it before we have the security > model in a reasonable place. As you pointed out[1], it's not in a > reasonable place now, so encouraging more use seems like a bad idea. I certainly agree that the security model isn't in a reasonable place right now. However, I feel that: (1) adding an extra predefined role doesn't really help, because it doesn't actually do anything in and of itself, it only prepares for future work, and (2) even adding the connection string security stuff that you're proposing doesn't really help, because (2a) connection string security is almost completely separate from the internal security considerations addressed in the message to which you linked, and (2b) in my opinion, there will be a lot of people who won't use that connection string security stuff even if we had it, possibly even a large majority of people won't use it, because it responds to a specific use case which I think a lot of people don't have, and (3) I don't agree either that this patch would encourage more use of logical replication or that it would be bad if it did. I mean, there could be someone who knows about this patch and will hesitate to deploy logical replication if it doesn't get committed, or maybe slightly more likely, won't be able to do so if this patch doesn't get committed because they're running in a cloud environment. But probably not. Cloud providers are already hacking around this problem, Microsoft included. As a community, we're better off having a standard solution in core than having every vendor hack it their own way. And outside of a cloud environment, there's not really any reason for the lack of this patch to make a potential user hesitate. Also, features getting used is a thing that I think we should all want. If logical replication is in such a bad state that we think people should be using it, we should rip it out until the issues are fixed. I don't think anyone would seriously propose that such a course of action is advisable. So the alternative is to make it better. To reiterate what I think the most important point here is, both Azure and AWS already let you do this. EDB's own cloud offering is also going to let you do this, whether this change goes in or not. But if this patch gets committed, then eventually all of those vendors and whatever others are out there will let you do this in the same way, i.e. pg_create_subscription, instead of every vendor having their own patch to the code that does what this patch does through some method that is specific to that cloud vendor. That sort of fragmentation of the ecosystem is not good for anyone, AFAICS. > The other patch you posted seems like it makes a lot of progress in > that direction, and I think that should go in first. That was one of > the items I suggested previously[2], so thank you for working on that. Perhaps you could review that work? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Wed, 2023-03-22 at 12:16 -0400, Robert Haas wrote: > I've posted a > patch for that at > http://postgr.es/m/CA+TgmoaSCkg9ww9oppPqqs+9RVqCexYCE6Aq=usypfnoode...@mail.gmail.com > and AFAICT everyone agrees with the idea, even if the patch itself > hasn't yet attracted any code reviews. But although the two patches > are fairly closely related, this seems to be a good idea whether that > moves forward or not, and that seems to be a good idea whether this > moves forward or not. As this one has had more review and discussion, > my current thought is to try to get this one committed first. The current patch (non-superuser-subscriptions) is the most user-facing aspect, and it seems wrong to commit it before we have the security model in a reasonable place. As you pointed out[1], it's not in a reasonable place now, so encouraging more use seems like a bad idea. The other patch you posted seems like it makes a lot of progress in that direction, and I think that should go in first. That was one of the items I suggested previously[2], so thank you for working on that. Regards, Jeff Davis [1] https://www.postgresql.org/message-id/CA%2BTgmoavSQVcvEW3ZgZ7a1Q-TJ-fp0%2BNt7W3D7FCawArtTCBCQ%40mail.gmail.com [2] https://www.postgresql.org/message-id/27c557b12a590067c5e00588009447bb5bb2dd42.ca...@j-davis.com
Re: Non-superuser subscription owners
On Thu, Mar 23, 2023 at 1:41 PM Jeff Davis wrote: > Even if my changes don't happen, I would find it less confusing and > more likely that users understand what they're doing. I respectfully but firmly disagree. I think having two predefined roles that are both required to create a subscription and neither of which allows you to do anything other than create a subscription is intrinsically confusing. I'm not willing to commit a patch that works like that, and I will object if someone else wants to do so. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Thu, 2023-03-23 at 11:52 -0400, Robert Haas wrote: > What would this amount to concretely? Also adding a > pg_connection_string predefined role and requiring both that and > pg_create_subscription [to CREATE SUBSCRIPTION] Yes. > If so, I don't think that's a good idea. Maybe for some reason your > proposed changes won't end up happening, and then we've just got a > useless extra thing that makes things confusing. Even if my changes don't happen, I would find it less confusing and more likely that users understand what they're doing. To most users, the consequences of allowing users to write connection strings on the server are far from obvious. Even we, as developers, needed to spend a lot of time discussing the nuances. Someone merely granting the ability to CREATE SUBSCRIPTION would read that page in the docs, which is dominated by the mechanics of a subscription and says little about the connection string, let alone the security nuances of using it on a server. But if there is also a separate connection string privilege required, we can document it better and they are more likely to find it and understand. Beyond that, the connection string and the mechanics of the subscription are really different concepts. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Wed, Mar 22, 2023 at 3:53 PM Jeff Davis wrote: > Is there any chance I can convince you to separate the privileges of > using a connection string and creating a subscription, as I > suggested[1] earlier? What would this amount to concretely? Also adding a pg_connection_string predefined role and requiring both that and pg_create_subscription in all cases until your proposed changes get made? If so, I don't think that's a good idea. Maybe for some reason your proposed changes won't end up happening, and then we've just got a useless extra thing that makes things confusing. I think that adding a pg_connection_string privilege properly belongs to whatever patch makes it possible to separate the connection string from the subscription, and that we probably shouldn't add those even in separate commits, let alone in separate major releases. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Wed, 2023-03-22 at 12:16 -0400, Robert Haas wrote: > If nobody's too unhappy with the idea, I plan to commit this soon, > both because I think that the feature is useful, and also because I > think it's an important security improvement. Is there any chance I can convince you to separate the privileges of using a connection string and creating a subscription, as I suggested[1] earlier? It would be useful for dblink, and I also plan to propose CREATE SUBSCRIPTION ... SERVER for v17 (it was too late for 16), for which it would also be useful to make the distinction. You seemed to generally think it was a reasonable idea, but wanted to wait for the other patch. I think it's the right breakdown of privileges even now, and I don't see a reason to give ourselves a headache later trying to split up the privileges later. Regards, Jeff Davis [1] https://www.postgresql.org/message-id/fa1190c117c2455f2dd968a1a09f796ccef27b29.ca...@j-davis.com
Re: Non-superuser subscription owners
On Wed, Mar 8, 2023 at 2:47 PM Andres Freund wrote: > Hm - it still feels wrong that we error out in case of failure, despite the > comment to the function saying: > * Returns NULL on error and fills the err with palloc'ed error message. I've amended the comment so that it explains why it's done that way. > Other than this, the change looks ready to me. Well, it still needed documentation changes and pg_dump changes. I've added those in the attached version. If nobody's too unhappy with the idea, I plan to commit this soon, both because I think that the feature is useful, and also because I think it's an important security improvement. Since replication is currently run as the subscription owner, any table owner can compromise the subscription owner's account, which is really bad, but if the subscription owner can be a non-superuser, it's a little bit less bad. From a security point of view, I think the right thing to do and what would improve security a lot more is to run replication as the table owner rather than the subscription owner. I've posted a patch for that at http://postgr.es/m/CA+TgmoaSCkg9ww9oppPqqs+9RVqCexYCE6Aq=usypfnoode...@mail.gmail.com and AFAICT everyone agrees with the idea, even if the patch itself hasn't yet attracted any code reviews. But although the two patches are fairly closely related, this seems to be a good idea whether that moves forward or not, and that seems to be a good idea whether this moves forward or not. As this one has had more review and discussion, my current thought is to try to get this one committed first. -- Robert Haas EDB: http://www.enterprisedb.com v5-0001-Add-new-predefined-role-pg_create_subscriptions.patch Description: Binary data
Re: Non-superuser subscription owners
Hi, On 2023-02-07 16:56:55 -0500, Robert Haas wrote: > On Wed, Feb 1, 2023 at 4:02 PM Andres Freund wrote: > > > + /* Is the use of a password mandatory? */ > > > + must_use_password = MySubscription->passwordrequired && > > > + !superuser_arg(MySubscription->owner); > > > > There's a few repetitions of this - perhaps worth putting into a helper? > > I don't think so. It's slightly different each time, because it's > pulling data out of different data structures. > > > This still leaks the connection on error, no? > > I've attempted to fix this in v4, attached. Hm - it still feels wrong that we error out in case of failure, despite the comment to the function saying: * Returns NULL on error and fills the err with palloc'ed error message. Other than this, the change looks ready to me. Greetings, Andres Freund
Re: Non-superuser subscription owners
On Wed, Mar 1, 2023 at 7:34 PM Andres Freund wrote: > ISTM that this would require annotating most functions in the system. There's > many problems besides accessing database contents. Just a few examples: > > - dblink functions to access another system / looping back > - pg_read_file()/pg_file_write() allows almost arbitrary mischief > - pg_stat_reset[_shared]() > - creating/dropping logical replication slots > - use untrusted PL functions > - many more > > A single wrongly annotated function would be sufficient to escape. This > includes user defined functions. > > This basically proposes that we can implement a safe sandbox for executing > arbitrary code in a privileged context. IMO history suggests that that's a > hard thing to do. Yeah, that's true, but I don't think switching users all the time is going to be great either. And it's not like other people haven't gone this way: that's what plperl (not plperlu) is all about, and JavaScript running in your browser, and so on. Those things aren't problem-free, of course, but we're all using them. When I was initially thinking about this, I thought that maybe we could just block access to tables and utility statements. That's got problems in both directions. On the one hand, there are functions like the ones you propose here that have side effects which we might not want to allow, and on the other hand, somebody might have an index expression that does a lookup in a table that they "never change". The latter case is problematic for non-security reasons, because there's an invisible dump-ordering constraint that must be obeyed for dump/restore to work at all, but there's no security issue. Still, I'm not sure this idea is completely dead in the water. It doesn't seem unreasonable to me that if you have that kind of case, you have to somehow opt into the behavior: yeah, I know that index functions I'm executing are going to read from tables, and I consent to that. And similarly, if your index expression calls pg_stat_reset_shared(), that probably ought to be blocked by default too, and if you want to allow it, you have to say so. Yes, that does require labelling functions, or maybe putting run-time checks in them: RequireAvailableCapability(CAP_MODIFY_DATABASE_STATE); If that capability isn't available in the present context, the call errors out. That way, it's possible for the required capabilities to depend on the arguments to the function, and we can change markings in minor releases without needing catalog changes. There's another way of thinking about this problem, which involves supposing that the invoker should only be induced to do things that the definer could also have done. Basically do every privilege check twice, and require that both pass. The problem I have with that is that there are various operations which depend on your identity, not just your privileges. For instance, a GRANT statement records a grantor, and a REVOKE statement infers a grantor whose grant is to be revoked. The operation isn't just allowed or disallowed based on who performed it -- it actually does something different depending on who performs it. I believe we have a number of cases like that, and I think that they suggest that that whole model is pretty flawed. Even if that were no issue, this also seems extremely complex to implement, because we have an absolute crap-ton of places that perform privilege checks and getting all of those places to check privileges as a second user seems nightmarish. I also think that it might be lead to confusing error messages: alice tried to do X but we're not allowing it because bob isn't allowed to do X. Eh, what? As opposed to the sandboxing approach, where I think you get something more like: ERROR: database state cannot be modified now DETAIL: The database system is evaluating an index expression. HINT: Do $SOMETHING to allow this. I don't want to press too hard on my idea here. I'm sure it has a bunch of problems apart from those already mentioned, and those already mentioned are not trivial. However, I do think there might be ways to make it work, and I'm not at all convinced that trying to switch users all over the place is going to be be better, either for security or usability. Is there some other whole kind of approach we can take here that we haven't discussed yet? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi, On 2023-02-28 08:37:02 -0500, Robert Haas wrote: > On Mon, Feb 27, 2023 at 7:37 PM Jeff Davis wrote: > > > Yeah. That's the idea I was floating, at least. > > > > Isn't that a hard problem; maybe impossible? > > It doesn't seem that hard to me; maybe I'm missing something. > > The existing SECURITY_RESTRICTED_OPERATION flag basically prevents you > from tinkering with the session state. If we also had a similar flags > like DATABASE_READS_PROHIBITED and DATABASE_WRITES_PROHIBITED (or just > a combined DATABASE_ACCESS_PROHIBITED flag) I think that would be > pretty close to what we need. The idea would be that, when a user > executes a function or procedure owned by a user that they don't trust > completely, we'd set > SECURITY_RESTRICTED_OPERATION|DATABASE_READS_PROHIBITED|DATABASE_WRITES_PROHIBITED. > And we could provide a user with a way to express the degree of trust > they have in some other user or perhaps even some specific function, > e.g. ISTM that this would require annotating most functions in the system. There's many problems besides accessing database contents. Just a few examples: - dblink functions to access another system / looping back - pg_read_file()/pg_file_write() allows almost arbitrary mischief - pg_stat_reset[_shared]() - creating/dropping logical replication slots - use untrusted PL functions - many more A single wrongly annotated function would be sufficient to escape. This includes user defined functions. This basically proposes that we can implement a safe sandbox for executing arbitrary code in a privileged context. IMO history suggests that that's a hard thing to do. Am I missing something? Greetings, Andres Freund
Re: Non-superuser subscription owners
Hi, On 2023-02-28 12:36:38 -0800, Jeff Davis wrote: > On Tue, 2023-02-28 at 11:28 -0800, Andres Freund wrote: > > I can only repeat myself in stating that SECURITY DEFINER solves none > > of the > > relevant issues. I included several examples of why it doesn't in the > > recent > > thread about "blocking SECURITY INVOKER". E.g. that default arguments > > of > > SECDEF functions are evaluated with the current user's privileges, > > not the > > function owner's privs: > > > > https://postgr.es/m/20230113032943.iyxdu7bnxe4cmbld%40awork3.anarazel.de > > I was speaking a bit loosely, using "SECURITY DEFINER" to mean the > semantics of executing code as the one who wrote it. I didn't > specifically mean the function marker, because as you pointed out in > the other thread, that's not enough. Oh, ok. > From your email it looks like there is still a path forward: > > "The proposal to not trust any expressions controlled by untrusted > users at least allows to prevent execution of code, even if it doesn't > provide a way to execute the code in a safe manner. Given that we > don't have the former, it seems foolish to shoot for the latter." > > And later: > > "I think the combination of > a) a setting that restricts evaluation of any non-trusted expressions, >independent of the origin > b) an easy way to execute arbitrary statements within >SECURITY_RESTRICTED_OPERATION" > > My takeaway from that thread was that we need a mechanism to deal with > non-function code (e.g. default expressions) first; but once we have > that, it opens up the design space to better solutions or at least > mitigations. Is that right? I doubt it's realistic to change the user for all kinds of expressions individually. A query can involve expressions controlled by many users, changing the current user in a super granular way seems undesirable from a performance and complexity pov. Greetings, Andres Freund
Re: Non-superuser subscription owners
On Wed, 2023-03-01 at 16:06 -0500, Robert Haas wrote: > To be fair, it's possible that there's no solution to this class of > problems that *doesn't* suck, but I think we should look a lot harder > before coming to that conclusion. Fair enough. The situation is bad enough that I'm willing to consider a pretty wide range of solutions and mitigations that might otherwise be unappealing. I think there might be something promising in your idea to highly restrict the privileges of code attached to a table. A lot of expressions are really simple and don't need much to be both useful and safe. We may not need the exact same solution for both default expressions and triggers. Some details to work through, though. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Wed, Mar 1, 2023 at 1:13 PM Jeff Davis wrote: > I don't think it's magic, as I said above. But I assume that your more > general point is that if we take some responsibility away from the > invoker and place it on the definer, then it creates room for new kinds > of problems. And I agree. > > The point of moving responsibility to the definer is that the definer > can actually do something to protect themselves (nail down search_path, > restrict USAGE privs, and avoid dynamic SQL); whereas the invoker is > nearly helpless. I think there's some truth to that allegation, but I think it largely comes from the fact that we've given very little thought or attention to this problem. We have a section in the documentation on writing SECURITY DEFINER functions safely because we've known for a long time that it's dangerous and we've provided some (imperfect) tools for dealing with it, like allowing a SET search_path clause to be attached to a function definition. We have no comparable documentation section about SECURITY INVOKER because we haven't historically taken that seriously as a security hazard and we have no tools to make it safe. But we could, as with what I'm proposing here, or the user/function trust mechanism previously proposed by Noah, or various other ideas that we might have. I don't like the idea of saying that we're not going to try to invent anything new and just push people into using the stuff we already have. The stuff we have for security SECURITY DEFINER functions is not very good. True, it's better than what we have for protecting against the risks inherent in SECURITY INVOKER, but that's not saying much: anything at all is better than nothing. But it's easy to forget a SET search_path clause on one of your functions, or to include something in that search path that's not actually safe, or to have a problem that isn't blocked by just setting search_path. Also, not that it's the most important consideration here, but putting a SET clause on your functions is really kind of expensive if what the function does is trivial, which if you're using it in an index expression or a default expression, will often be the case. I don't want to pretend like I have all the answers here, but I find it really hard to believe that pushing people to do the same kind of nonsense that's currently required when writing a SECURITY DEFINER function for a lot of their other functions as well is going to be a win. I think it will probably suck. To be fair, it's possible that there's no solution to this class of problems that *doesn't* suck, but I think we should look a lot harder before coming to that conclusion. I've come to agree with your contention that we're not taking the hazards of SECURITY INVOKER nearly seriously enough, but I think you're underestimating the hazards that SECURITY DEFINER poses, and overestimating how easy it is to avoid them. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Wed, 2023-03-01 at 10:05 -0500, Robert Haas wrote: > For this reason, these expressions are, by > default, restricted from doing . The hard part is defining without resorting to a bunch of special cases, and also in a way that doesn't break a bunch of stuff. > You earlier > mentioned that switching to the table owner seems to be just a way of > turning SECURITY INVOKER into SECURITY DEFINER in random places, or > maybe that's not exactly what you said but that's what I took from > it. Yeah, though I glossed over some details, see below. > If we just slather user context switches > everywhere, I'm not actually very sure that's going to be > comprehensible behavior: if my trigger function is SECURITY INVOKER, > why is it getting executed as me, not the inserting user? Let's consider other expressions first. The proposal is that all expressions attached to a table should run as the table owner (set aside compatibility concerns for a moment). If those expressions call a SECURITY INVOKER function, the invoker would need to be the table owner as well. Users could get confused by that, but I think it's documentable and understandable; and it's really the only thing that makes sense -- otherwise changing the user is completely useless. We should consider triggers as just another expression being executed, and the invoker is the table owner. The problem is that it's a little annoying to users because they probably defined the function for the sole purpose of being a trigger function for a single table, and it might look as though the SECURITY INVOKER label was ignored. But there is a difference, which I glossed over before: SECURITY INVOKER on a trigger function would still have significance, because the function owner (definer) and table owner (invoker) could still be different in the case of a trigger, just like in an expression. This goes back to my point that SECURITY INVOKER is more complex for us to document and for users to understand. The user *must* understand who the invoker is in various contexts. That's the situation today and there's no escaping it. We aren't making things any worse, at least as long as we can sort out compatibility in a reasonable way. (Aside: I'm having some serious concerns about how the invoker of a function called in a view is not the view definer. That's another thing we'll need to fix, because it's another way of putting SECURITY INVOKER code in front of someone without them knowing.) (Aside: We should take a second look at the security invoker views before we release them. I know that I learned some things during this discussion and a fresh look might be useful.) > As soon as you magically turn > that into a SECURITY DEFINER function, you've provided a way for the > users performing DML to attack the table owner. I don't think it's magic, as I said above. But I assume that your more general point is that if we take some responsibility away from the invoker and place it on the definer, then it creates room for new kinds of problems. And I agree. The point of moving responsibility to the definer is that the definer can actually do something to protect themselves (nail down search_path, restrict USAGE privs, and avoid dynamic SQL); whereas the invoker is nearly helpless. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Wed, Mar 1, 2023 at 10:48 AM Stephen Frost wrote: > > Yeah, or any other expressions. Basically impose restrictions when the > > user running the code is not the same as the user who provided the > > code. > > Would this have carve-outs for things like "except if the user providing > the code is trusted/superuser"? Seems like that would be necessary for > the function to be able to do more-or-less anything, but then I worry > that there's superuser-owned code which could leak information or be > used by a malicious owner as that code would still be running as the > invoking user.. Perhaps we could say that the function also has to be > leakproof, but that isn't quite the same issue and therefore it seems > like we'd need to decorate all of the functions with another flag that's > allowed to be run in this manner. Yes, I think there can be carve-outs based on the relationship of the users involved -- if the user who provided the code is the superuser or some other user who can anyway run whatever they want as the user performing the operation, then there's no point in imposing any restrictions -- and I think there can also be some way of setting policy. I proposed a GUC in an earlier email, and you proposed one with somewhat different semantics in this email, and I'm not sure that either of those things in particular is right or that we ought to be using a GUC for this at all. However, there should almost certainly be SOME way for the superuser to turn any new restrictions off, and there should probably also be some way for an unprivileged user to say "you know, I am totally OK with running any code that alice provides -- just go with it." I don't think we're at a point where we can conclude on what those mechanisms should look like just yet, but I think that everyone who has spoken up agrees that they ought to exist, assuming we go in this direction at all. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Feb 28, 2023 at 4:01 PM Jeff Davis wrote: > > Or default expressions, I presume. If we at least agree on this point, > > then I think we should try to find a way to treat these other hunks of > > code in a secure way (which I think is what Andres was suggesting). > > Yeah, or any other expressions. Basically impose restrictions when the > user running the code is not the same as the user who provided the > code. Would this have carve-outs for things like "except if the user providing the code is trusted/superuser"? Seems like that would be necessary for the function to be able to do more-or-less anything, but then I worry that there's superuser-owned code which could leak information or be used by a malicious owner as that code would still be running as the invoking user.. Perhaps we could say that the function also has to be leakproof, but that isn't quite the same issue and therefore it seems like we'd need to decorate all of the functions with another flag that's allowed to be run in this manner. Random thought- what if the function has a NOTIFY in it with a payload of some kind of sensitive information? > > Unless the trusted roles defaults to '*', then I think it will still > > break some things. > > Definitely. IMHO, it's OK to break some things, certainly in a major > release and maybe even in a minor release. But we don't want to break > more things that we really need to break. And as you say, we want the > restrictions to be comprehensible. Really hard to say if whatever this is is OK for back-patching and breaking minor releases without knowing exactly what is getting broken ... but it'd have to be a very clear edge case of what gets broken for it to be sensible for breaking in a minor release without a very clear vulnerability or such that's being fixed with a simple work-around. Just making all auditing triggers break in a minor release certainly wouldn't be acceptable, as an example that I imagine we all agree with. > > Each of these ideas and sub-ideas affect the semantics, and should be > > documented. But how do we document that some code runs as you, some as > > the person who wrote it, sometimes we obey SECURITY INVOKER and > > sometimes we ignore it and use DEFINER semantics, some code is outside > > a function and always executes as the invoker, some code has some > > security flags, and some code has more security flags, code can change > > between the time you look at it and the time it runs, and it's all > > filtered through GUCs with their own privilege sub-language? > > > > OK, let's assume that we have all of that documented, then how do we > > guide users on what reasonable best practices are for the GUC settings, > > etc.? Or do we just say "this is mechanically how all these parts work, > > good luck assembling it into a secure system!". [ Note: I feel like > > this is the state we are in now. Even if technically we don't have live > > security bugs that I'm aware of, we are setting users up for security > > problems. ] > > > > On the other hand, if we focus on executing code as the user who wrote > > it in most places, then the documentation will be something like: "you > > defined the table, you wrote the code, it runs as you, here are some > > best practices for writing secure code". And we have some different > > documentation for writing a cool SECURITY INVOKER function and how to > > get other users to trust you enough to run it. That sounds a LOT more > > understandable for users. > > What I was imagining is that we would document something like: A table > can have executable code associated with it in a variety of ways. For > example, it can have triggers, default expressions, check constraints, > or row-level security filters. In most cases, these expressions are > executed with the privileges of the user performing the operation on > the table, except when SECURITY DEFINER functions are used. Because > these expressions are set by the table owner and executed by the users > accessing the table, there is a risk that the table owner could > include malicious code that usurps the privileges of the user > accessing the table. For this reason, these expressions are, by > default, restricted from doing . If you want to allow those > operations, you can . Well, one possible answer to 'something' might be 'use SECURITY DEFINER functions which are owned by a role allowed to do '. Note that that doesn't have to be the table owner though, it could be a much more constrained role. That approach would allow us to leverage the existing GRANT/RLS/et al system for what's allowed and avoid having to create new things like a complex permission system inside of a GUC for users to have to understand. > I agree that running code as the table owner is helpful in a bunch of > scenarios, but I also don't think it fixes everything. You earlier > mentioned that switching to the table owner seems to be just a
Re: Non-superuser subscription owners
On Tue, Feb 28, 2023 at 4:01 PM Jeff Davis wrote: > You're suggesting using the SECURITY_RESTRICTED_OPERATION flag, along > with the new security flags, but not switch to the table owner, right? Correct. > Or default expressions, I presume. If we at least agree on this point, > then I think we should try to find a way to treat these other hunks of > code in a secure way (which I think is what Andres was suggesting). Yeah, or any other expressions. Basically impose restrictions when the user running the code is not the same as the user who provided the code. > It seems like you're saying to basically just keep the user ID the > same, and maybe keep USAGE privileges, but not be able to do anything > else? Might be useful. Kind of like running it as a nobody user but > without the problems you mentioned. Some details to think about, I'm > sure. Yep. > I'm not very excited about inventing a new privilege language inside a > GUC, but perhaps a simpler form could be a reasonable mitigation (or at > least a starting place). I'm not very sure about this part, either. I think we need some way of shutting off whatever new controls we impose, but the shape of it is unclear to me and I think there are a bunch of problems. > Unless the trusted roles defaults to '*', then I think it will still > break some things. Definitely. IMHO, it's OK to break some things, certainly in a major release and maybe even in a minor release. But we don't want to break more things that we really need to break. And as you say, we want the restrictions to be comprehensible. > Each of these ideas and sub-ideas affect the semantics, and should be > documented. But how do we document that some code runs as you, some as > the person who wrote it, sometimes we obey SECURITY INVOKER and > sometimes we ignore it and use DEFINER semantics, some code is outside > a function and always executes as the invoker, some code has some > security flags, and some code has more security flags, code can change > between the time you look at it and the time it runs, and it's all > filtered through GUCs with their own privilege sub-language? > > OK, let's assume that we have all of that documented, then how do we > guide users on what reasonable best practices are for the GUC settings, > etc.? Or do we just say "this is mechanically how all these parts work, > good luck assembling it into a secure system!". [ Note: I feel like > this is the state we are in now. Even if technically we don't have live > security bugs that I'm aware of, we are setting users up for security > problems. ] > > On the other hand, if we focus on executing code as the user who wrote > it in most places, then the documentation will be something like: "you > defined the table, you wrote the code, it runs as you, here are some > best practices for writing secure code". And we have some different > documentation for writing a cool SECURITY INVOKER function and how to > get other users to trust you enough to run it. That sounds a LOT more > understandable for users. What I was imagining is that we would document something like: A table can have executable code associated with it in a variety of ways. For example, it can have triggers, default expressions, check constraints, or row-level security filters. In most cases, these expressions are executed with the privileges of the user performing the operation on the table, except when SECURITY DEFINER functions are used. Because these expressions are set by the table owner and executed by the users accessing the table, there is a risk that the table owner could include malicious code that usurps the privileges of the user accessing the table. For this reason, these expressions are, by default, restricted from doing . If you want to allow those operations, you can . I agree that running code as the table owner is helpful in a bunch of scenarios, but I also don't think it fixes everything. You earlier mentioned that switching to the table owner seems to be just a way of turning SECURITY INVOKER into SECURITY DEFINER in random places, or maybe that's not exactly what you said but that's what I took from it. And I think that's right. If we just slather user context switches everywhere, I'm not actually very sure that's going to be comprehensible behavior: if my trigger function is SECURITY INVOKER, why is it getting executed as me, not the inserting user? I also think there are plenty of cases where that could just replace the current set of security problems with a new set of security problems. If the trigger function is SECURITY INVOKER, then the user who wrote it doesn't have to worry about securing it against attacks by users accessing the table; it's just running with the permissions of the user performing the DML. Maybe there are correctness issues if you don't lock down search_path etc., but there's no security compromise because there's no user ID switching. As soon as you magically turn that into a SECURITY DEFINER function,
Re: Non-superuser subscription owners
Greetings, * Jeff Davis (pg...@j-davis.com) wrote: > On Tue, 2023-02-28 at 08:37 -0500, Robert Haas wrote: > > The existing SECURITY_RESTRICTED_OPERATION flag basically prevents > > you > > from tinkering with the session state. > > Currently, every time we set that flag we also run all the code as the > table owner. > > You're suggesting using the SECURITY_RESTRICTED_OPERATION flag, along > with the new security flags, but not switch to the table owner, right? I'm having trouble following this too, I have to admit. If we aren't changing who we're running the code under.. but making it so that the code isn't actually able to do anything then that doesn't strike me as likely to actually be useful? Surely things like triggers which are used to update another table or insert into another table what happened on the table with the trigger need to be allowed to modify the database- how do we make that possible while the code runs as the invoker and not the table owner when the table owner is the one who gets to write the code? > > If we also had a similar flags > > like DATABASE_READS_PROHIBITED and DATABASE_WRITES_PROHIBITED (or > > just > > a combined DATABASE_ACCESS_PROHIBITED flag) I think that would be > > pretty close to what we need. The idea would be that, when a user > > executes a function or procedure > > Or default expressions, I presume. If we at least agree on this point, > then I think we should try to find a way to treat these other hunks of > code in a secure way (which I think is what Andres was suggesting). Would need to apply to functions in views and functions in RLS too, along wth default expressions and everything else that could be defined by one person and run by another. > > owned by a user that they don't trust > > completely, we'd set > > SECURITY_RESTRICTED_OPERATION|DATABASE_READS_PROHIBITED|DATABASE_WRIT > > ES_PROHIBITED. > > It seems like you're saying to basically just keep the user ID the > same, and maybe keep USAGE privileges, but not be able to do anything > else? Might be useful. Kind of like running it as a nobody user but > without the problems you mentioned. Some details to think about, I'm > sure. While there's certainly some use-cases where a completely unprivileged user would work, there's certainly an awful lot where it wouldn't. Having that as an option might be interesting for those much more limited use-cases and maybe you could even say "only run functions which are owned by a superuser or X roles" but it's certainly not a general solution to the problem. > > And we could provide a user with a way to express the degree of trust > > they have in some other user or perhaps even some specific function, > > e.g. > > > > SET trusted_roles='alice:read'; > > > > ...could mean that I trust alice to read from the database with my > > permissions, should I happen to run code provided by her in SECURITY > > INVOKER modacke. > > I'm not very excited about inventing a new privilege language inside a > GUC, but perhaps a simpler form could be a reasonable mitigation (or at > least a starting place). I'm pretty far down the path of "wow that looks really difficult to work with", to put it nicely. > > I'm sure there's some details to sort out here, e.g. around security > > related to the trusted_roles GUC itself. But I don't really see a > > fundamental problem. We can invent arbitrary flags that prohibit > > classes of operations that are of concern, set them by default in > > cases where concern is justified, and then give users who want the > > current behavior some kind of escape hatch that causes those flags to > > not get set after all. Not only does such a solution not seem > > impossible, I can possibly even imagine back-patching it, depending > > on > > exactly what the shape of the final solution is, how important we > > think it is to get a fix out there, and how brave I'm feeling that > > day. > > Unless the trusted roles defaults to '*', then I think it will still > break some things. Defaulting to an option that is "don't break anything" while giving users flexibility to test out other, more secure, options seems like it would be a pretty reasonable way forward, generally. That said.. I don't really think this particular approach ends up being a good direction to go in... > One of my key tests for user-facing proposals is whether the > documentation will be reasonable or not. Most of these proposals to > make SECURITY INVOKER less bad fail that test. and this is certainly a very good point as to why. Thanks, Stephen signature.asc Description: PGP signature
Re: Non-superuser subscription owners
On Tue, 2023-02-28 at 08:37 -0500, Robert Haas wrote: > The existing SECURITY_RESTRICTED_OPERATION flag basically prevents > you > from tinkering with the session state. Currently, every time we set that flag we also run all the code as the table owner. You're suggesting using the SECURITY_RESTRICTED_OPERATION flag, along with the new security flags, but not switch to the table owner, right? > If we also had a similar flags > like DATABASE_READS_PROHIBITED and DATABASE_WRITES_PROHIBITED (or > just > a combined DATABASE_ACCESS_PROHIBITED flag) I think that would be > pretty close to what we need. The idea would be that, when a user > executes a function or procedure Or default expressions, I presume. If we at least agree on this point, then I think we should try to find a way to treat these other hunks of code in a secure way (which I think is what Andres was suggesting). > owned by a user that they don't trust > completely, we'd set > SECURITY_RESTRICTED_OPERATION|DATABASE_READS_PROHIBITED|DATABASE_WRIT > ES_PROHIBITED. It seems like you're saying to basically just keep the user ID the same, and maybe keep USAGE privileges, but not be able to do anything else? Might be useful. Kind of like running it as a nobody user but without the problems you mentioned. Some details to think about, I'm sure. > And we could provide a user with a way to express the degree of trust > they have in some other user or perhaps even some specific function, > e.g. > > SET trusted_roles='alice:read'; > > ...could mean that I trust alice to read from the database with my > permissions, should I happen to run code provided by her in SECURITY > INVOKER modacke. I'm not very excited about inventing a new privilege language inside a GUC, but perhaps a simpler form could be a reasonable mitigation (or at least a starting place). > I'm sure there's some details to sort out here, e.g. around security > related to the trusted_roles GUC itself. But I don't really see a > fundamental problem. We can invent arbitrary flags that prohibit > classes of operations that are of concern, set them by default in > cases where concern is justified, and then give users who want the > current behavior some kind of escape hatch that causes those flags to > not get set after all. Not only does such a solution not seem > impossible, I can possibly even imagine back-patching it, depending > on > exactly what the shape of the final solution is, how important we > think it is to get a fix out there, and how brave I'm feeling that > day. Unless the trusted roles defaults to '*', then I think it will still break some things. One of my key tests for user-facing proposals is whether the documentation will be reasonable or not. Most of these proposals to make SECURITY INVOKER less bad fail that test. Each of these ideas and sub-ideas affect the semantics, and should be documented. But how do we document that some code runs as you, some as the person who wrote it, sometimes we obey SECURITY INVOKER and sometimes we ignore it and use DEFINER semantics, some code is outside a function and always executes as the invoker, some code has some security flags, and some code has more security flags, code can change between the time you look at it and the time it runs, and it's all filtered through GUCs with their own privilege sub-language? OK, let's assume that we have all of that documented, then how do we guide users on what reasonable best practices are for the GUC settings, etc.? Or do we just say "this is mechanically how all these parts work, good luck assembling it into a secure system!". [ Note: I feel like this is the state we are in now. Even if technically we don't have live security bugs that I'm aware of, we are setting users up for security problems. ] On the other hand, if we focus on executing code as the user who wrote it in most places, then the documentation will be something like: "you defined the table, you wrote the code, it runs as you, here are some best practices for writing secure code". And we have some different documentation for writing a cool SECURITY INVOKER function and how to get other users to trust you enough to run it. That sounds a LOT more understandable for users. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Tue, 2023-02-28 at 11:28 -0800, Andres Freund wrote: > I can only repeat myself in stating that SECURITY DEFINER solves none > of the > relevant issues. I included several examples of why it doesn't in the > recent > thread about "blocking SECURITY INVOKER". E.g. that default arguments > of > SECDEF functions are evaluated with the current user's privileges, > not the > function owner's privs: > > https://postgr.es/m/20230113032943.iyxdu7bnxe4cmbld%40awork3.anarazel.de I was speaking a bit loosely, using "SECURITY DEFINER" to mean the semantics of executing code as the one who wrote it. I didn't specifically mean the function marker, because as you pointed out in the other thread, that's not enough. >From your email it looks like there is still a path forward: "The proposal to not trust any expressions controlled by untrusted users at least allows to prevent execution of code, even if it doesn't provide a way to execute the code in a safe manner. Given that we don't have the former, it seems foolish to shoot for the latter." And later: "I think the combination of a) a setting that restricts evaluation of any non-trusted expressions, independent of the origin b) an easy way to execute arbitrary statements within SECURITY_RESTRICTED_OPERATION" My takeaway from that thread was that we need a mechanism to deal with non-function code (e.g. default expressions) first; but once we have that, it opens up the design space to better solutions or at least mitigations. Is that right? Regards, Jeff Davis
Re: Non-superuser subscription owners
Hi, On 2023-02-22 09:18:34 -0800, Jeff Davis wrote: > I can't resist mentioning that these are all SECURITY INVOKER problems. > SECURITY INVOKER is insecure unless the invoker absolutely trusts the > definer, and that only really makes sense if the definer is a superuser > (or something very close). That's why we keep adding exceptions with > SECURITY_RESTRICTED_OPERATION, which is really just a way to silently > ignore the SECURITY INVOKER label and use SECURITY DEFINER instead. > > At some point we need to ask: "when is SECURITY INVOKER both safe and > useful?" and contain it to those cases, rather than silently ignoring > it in an expanding list of cases. I can only repeat myself in stating that SECURITY DEFINER solves none of the relevant issues. I included several examples of why it doesn't in the recent thread about "blocking SECURITY INVOKER". E.g. that default arguments of SECDEF functions are evaluated with the current user's privileges, not the function owner's privs: https://postgr.es/m/20230113032943.iyxdu7bnxe4cmbld%40awork3.anarazel.de Greetings, Andres Freund
Re: Non-superuser subscription owners
On Mon, Feb 27, 2023 at 7:37 PM Jeff Davis wrote: > > Yeah. That's the idea I was floating, at least. > > Isn't that a hard problem; maybe impossible? It doesn't seem that hard to me; maybe I'm missing something. The existing SECURITY_RESTRICTED_OPERATION flag basically prevents you from tinkering with the session state. If we also had a similar flags like DATABASE_READS_PROHIBITED and DATABASE_WRITES_PROHIBITED (or just a combined DATABASE_ACCESS_PROHIBITED flag) I think that would be pretty close to what we need. The idea would be that, when a user executes a function or procedure owned by a user that they don't trust completely, we'd set SECURITY_RESTRICTED_OPERATION|DATABASE_READS_PROHIBITED|DATABASE_WRITES_PROHIBITED. And we could provide a user with a way to express the degree of trust they have in some other user or perhaps even some specific function, e.g. SET trusted_roles='alice:read'; ...could mean that I trust alice to read from the database with my permissions, should I happen to run code provided by her in SECURITY INVOKER modacke. I'm sure there's some details to sort out here, e.g. around security related to the trusted_roles GUC itself. But I don't really see a fundamental problem. We can invent arbitrary flags that prohibit classes of operations that are of concern, set them by default in cases where concern is justified, and then give users who want the current behavior some kind of escape hatch that causes those flags to not get set after all. Not only does such a solution not seem impossible, I can possibly even imagine back-patching it, depending on exactly what the shape of the final solution is, how important we think it is to get a fix out there, and how brave I'm feeling that day. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Greetings, * Jeff Davis (pg...@j-davis.com) wrote: > Not all steps would be breaking changes, and a lot of those steps are > things we should do anyway. We could make it easier to write safe > SECURITY DEFINER functions, provide more tools for users to opt-out of > executing SECURITY INVOKER code, provide a way for superusers to safely > drop privileges, document the problems with security invoker and what > to do about them, etc. Agreed. > But we also shouldn't exaggerate it -- for instance, others have > proposed that we run code as the table owner for logical subscriptions, > and that's going to break things in the same way. Arguably, if we are > going to break something, it's better to break it consistently rather > than one subsystem at a time. I tend to agree with this. > Back to the $SUBJECT, if we allow non-superusers to run subscriptions, > and the subscription runs the code as the table owner, that might also > lead to some weird behavior for triggers that rely on SECURITY INVOKER > semantics. Indeed. Thanks, Stephen signature.asc Description: PGP signature
Re: Non-superuser subscription owners
Greetings, * Jeff Davis (pg...@j-davis.com) wrote: > On Mon, 2023-02-27 at 14:10 -0500, Stephen Frost wrote: > > I do think there are some use-cases for it, but agree that it'd be > > better to encourage more use of SECURITY DEFINER and one approach to > > that might be to have a way for users to explicitly say "don't run > > code > > that isn't mine or a superuser's with my privileges." > > I tried that: > > https://www.postgresql.org/message-id/75b0dbb55e9febea54c441efff8012a6d2cb5bd7.ca...@j-davis.com > > but Andres pointed out some problems with my implementation. They > didn't seem easily fixable, but perhaps with more effort it could work > (run all the expressions as security definer, as well?). Presumably. Ultimately, I tend to agree it won't be easy. That doesn't mean it's not a worthwhile effort. > > Of course, we > > need to make sure it's possible to write safe SECURITY DEFINER > > functions > > and to be clear about how to do that to avoid the risk in the other > > direction. > > Agreed. Perhaps we can force search_path to be set for SECURITY > DEFINER, and require that the temp schema be explicitly included rather > than the current "must be at the end". We could also provide a way to > turn public access off in the same statement, so that you don't need to > use a transaction block to keep the function private. We do pretty strongly encourage a search_path setting for SECURITY DEFINER today.. That said, I'm not against pushing on that harder. The issue about temporary schemas is a more difficult issue... but frankly, I'd like an option to say "no temporary schemas should be allowed in my search path" when it comes to a security definer function. > > I don't think we'd be able to get away with just getting rid of > > SECURITY > > INVOKER entirely or even in changing the current way triggers (or > > functions in views, etc) are run by default. > > I didn't propose anything radical. I'm just trying to get some > agreement that SECURITY INVOKER is central to a lot of our security > woes, and that we should be treating it with skepticism on a > fundamental level. Sure, but if we want to make progress then we have to provide a direction for folks to go in that's both secure and convenient. > Individual proposals for how to get away from SECURITY INVOKER should > be evaluated on their merits (i.e. don't break a bunch of stuff). Of course. That said ... we don't want to spend a lot of time going in a direction that won't bear fruit; I'm hopeful that this direction will though. Thanks, Stephen signature.asc Description: PGP signature
Re: Non-superuser subscription owners
On Mon, 2023-02-27 at 16:13 -0500, Robert Haas wrote: > On Mon, Feb 27, 2023 at 1:25 PM Jeff Davis wrote: > > I think you are saying that we should still run Alice's code with > > the > > privileges of Bob, but somehow make that safe(r) for Bob. Is that > > right? > > Yeah. That's the idea I was floating, at least. Isn't that a hard problem; maybe impossible? > > I guess I have a pretty hard time imagining that we can just > obliterate SECURITY INVOKER entirely. Of course not. > It seems fundamentally > reasonable to me that Alice might want to make some code available to > be executed in the form of a function or procedure but without > offering to execute it with her own privileges. It also seems fundamentally reasonable that if someone grants you privileges on one of their tables, it might be safe to access it. I'm sure there are a few use cases for SECURITY INVOKER, but they are quite narrow. Perhaps most frustratingly, even if none of the users on a system has any use case for SECURITY INVOKER, they still must all live in fear of accessing each others' tables, because at any time a SECURITY INVOKER function could be attached to one of the tables. I feel like we are giving up mainstream utility and safety in exchange for contrived or exceptional cases. That's not a good trade. > We already take the position that VACUUM always runs as the > table owner, and while VACUUM runs index expressions but not for > example triggers, why not just be consistent and run all code that is > tied to the table as the table owner, all the time? I'd also extend this to default expressions and other code that can be executed implicitly. > Maybe that's the right thing to do If it's the right place to go, then I think we should consider reasonable steps to take in that direction that don't cause unnecessary breakage. > , but I think it would inevitably > break some things for some users. Not all steps would be breaking changes, and a lot of those steps are things we should do anyway. We could make it easier to write safe SECURITY DEFINER functions, provide more tools for users to opt-out of executing SECURITY INVOKER code, provide a way for superusers to safely drop privileges, document the problems with security invoker and what to do about them, etc. > Alice might choose to write her > triggers or default expressions in ways that rely on them running > with > Bob's permissions in any number of ways. Sure, breakage is possible, and we should mitigate it. But we also shouldn't exaggerate it -- for instance, others have proposed that we run code as the table owner for logical subscriptions, and that's going to break things in the same way. Arguably, if we are going to break something, it's better to break it consistently rather than one subsystem at a time. Back to the $SUBJECT, if we allow non-superusers to run subscriptions, and the subscription runs the code as the table owner, that might also lead to some weird behavior for triggers that rely on SECURITY INVOKER semantics. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Mon, 2023-02-27 at 14:10 -0500, Stephen Frost wrote: > I do think there are some use-cases for it, but agree that it'd be > better to encourage more use of SECURITY DEFINER and one approach to > that might be to have a way for users to explicitly say "don't run > code > that isn't mine or a superuser's with my privileges." I tried that: https://www.postgresql.org/message-id/75b0dbb55e9febea54c441efff8012a6d2cb5bd7.ca...@j-davis.com but Andres pointed out some problems with my implementation. They didn't seem easily fixable, but perhaps with more effort it could work (run all the expressions as security definer, as well?). > Of course, we > need to make sure it's possible to write safe SECURITY DEFINER > functions > and to be clear about how to do that to avoid the risk in the other > direction. Agreed. Perhaps we can force search_path to be set for SECURITY DEFINER, and require that the temp schema be explicitly included rather than the current "must be at the end". We could also provide a way to turn public access off in the same statement, so that you don't need to use a transaction block to keep the function private. > I don't think we'd be able to get away with just getting rid of > SECURITY > INVOKER entirely or even in changing the current way triggers (or > functions in views, etc) are run by default. I didn't propose anything radical. I'm just trying to get some agreement that SECURITY INVOKER is central to a lot of our security woes, and that we should be treating it with skepticism on a fundamental level. Individual proposals for how to get away from SECURITY INVOKER should be evaluated on their merits (i.e. don't break a bunch of stuff). Regards, Jeff Davis
Re: Non-superuser subscription owners
On Mon, Feb 27, 2023 at 1:25 PM Jeff Davis wrote: > I think you are saying that we should still run Alice's code with the > privileges of Bob, but somehow make that safe(r) for Bob. Is that > right? Yeah. That's the idea I was floating, at least. > That sounds hard, and I'm still stuck at the "why" question. Why do we > want to run Alice's code with Bob's permissions? > > The answers I have so far are abstract. For instance, maybe it's a > clever SRF that takes table names as inputs and you want people to only > be able to use the clever SRF with tables they have privileges on. But > that's not what most functions do, and it's certainly not what most > default expressions do. I guess I have a pretty hard time imagining that we can just obliterate SECURITY INVOKER entirely. It seems fundamentally reasonable to me that Alice might want to make some code available to be executed in the form of a function or procedure but without offering to execute it with her own privileges. But I think maybe you're asking a different question, which is whether when the code is attached to a table we ought to categorically switch to the table owner before executing it. I'm less sure about the answer to that question. We already take the position that VACUUM always runs as the table owner, and while VACUUM runs index expressions but not for example triggers, why not just be consistent and run all code that is tied to the table as the table owner, all the time? Maybe that's the right thing to do, but I think it would inevitably break some things for some users. Alice might choose to write her triggers or default expressions in ways that rely on them running with Bob's permissions in any number of ways. For instance, maybe those functions issue a SELECT query against an RLS-enabled table, such that the answer depends on whose privileges are used to run the query. More simply, she might refer to CURRENT_ROLE, say to record who inserted any particular row into her table, which seems like a totally reasonable thing to want to do. If she was feeling really clever, she might even have designed queries that she's using inside those triggers or default expressions to fail if Bob doesn't have enough permissions to do some particular modification that he's attempting, and thus block certain kinds of access to her own tables. That would be pretty weird and perhaps too clever by half, but the point is that the current behavior is probably known to many, many users and we really can't know what they've done that depends on that. If we change any behavior here, some people are going to notice those changes, and they may not like them. To put that another way, we're not talking about a black-and-white security vulnerability here, like a buffer overrun that allows for arbitrary code execution. We're talking about a set of semantics that seem to be somewhat fragile and vulnerable to spawning security problems. Nobody wants those security problems, for sure. But it doesn't follow that nobody is relying on the semantics. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Greetings, * Jeff Davis (pg...@j-davis.com) wrote: > On Mon, 2023-02-27 at 10:45 -0500, Robert Haas wrote: > > Suppose Alice owns a table and attaches a trigger to it. If > > Bob inserts into that table, I think we have to run the trigger, > > because Alice is entitled to assume that, for example, any BEFORE > > triggers she might have defined that block certain kinds of inserts > > are actually going to block those inserts; any constraints that she > > has applied to the table are going to be enforced against all new > > rows; and any default expressions she supplies are actually going to > > work. > > True, but I still find myself suspending my disbelief. Which of these > use cases make sense for SECURITY INVOKER? I do think there are some use-cases for it, but agree that it'd be better to encourage more use of SECURITY DEFINER and one approach to that might be to have a way for users to explicitly say "don't run code that isn't mine or a superuser's with my privileges." Of course, we need to make sure it's possible to write safe SECURITY DEFINER functions and to be clear about how to do that to avoid the risk in the other direction. We also need to provide some additonal functions along the lines of "calling_role()" or similar (so that the function can know who the actual role is that's running the trigger) for the common case of auditing or needing to know the calling role for RLS or similar. I don't think we'd be able to get away with just getting rid of SECURITY INVOKER entirely or even in changing the current way triggers (or functions in views, etc) are run by default. > > I think Bob has to be OK with those things too; otherwise, he > > just shouldn't insert anything into the table. > > Right, but why should Bob's privileges be needed to do any of those > things? Any difference in privileges, for those use cases, could only > either get in the way of achieving Alice's goals, or cause a security > problem for Bob. > > > But Bob doesn't have to be OK with Alice's code changing the session > > state, or executing DML or DDL with his permissions. > > What's left? Should Bob be OK with Alice's code using his permissions > for anything? I don't know about trying to define that X things are ok and Y things are not, that seems like it would be more confusing and difficult to work with. Regular SELECT queries that pull data that Bob has access to but Alice doesn't is a security issue too, were Alice to install a function that Bob calls which writes that data into a place that Alice could then access it. Perhaps if we could allow Bob to say "these things are ok for Alice's code to access" then it could work ... but if that's what is going on then the code could run with Alice's permissions and Bob could use our nice and granular GRANT/RLS system to say what Alice is allowed to access. > > I wonder if > > that's where we should be trying to insert restrictions here. Right > > now, we think of SECURITY_RESTRICTED_OPERATION as a way to prevent a > > function or procedure that runs under a different user ID than the > > session user from poisoning the session state. But I'm thinking that > > maybe the problem isn't really with code running under a different > > user ID. It's with running code *provided by* a different user ID. > > Maybe we should stop thinking about the security context as something > > that you set when you switch to running as a different user ID, and > > start thinking about it as something that needs to be set based on > > the > > relationship between the user that provided the code and the session > > user. If they're not the same, some restrictions are probably > > appropriate, except I think in the case where the user who provided > > the code can become the session user anyway. > > I think you are saying that we should still run Alice's code with the > privileges of Bob, but somehow make that safe(r) for Bob. Is that > right? > > That sounds hard, and I'm still stuck at the "why" question. Why do we > want to run Alice's code with Bob's permissions? > > The answers I have so far are abstract. For instance, maybe it's a > clever SRF that takes table names as inputs and you want people to only > be able to use the clever SRF with tables they have privileges on. But > that's not what most functions do, and it's certainly not what most > default expressions do. current_role / current_user are certainly common as a default expression. I agree that that's more of an edge case that would be nice to solve in a different way though. I do think there's some other use cases for SECURITY INVOKER but not enough folks understand the security risk associated with it and it'd be good for us to improve on that situation. Thanks, Stephen signature.asc Description: PGP signature
Re: Non-superuser subscription owners
On Mon, 2023-02-27 at 10:45 -0500, Robert Haas wrote: > Suppose Alice owns a table and attaches a trigger to it. If > Bob inserts into that table, I think we have to run the trigger, > because Alice is entitled to assume that, for example, any BEFORE > triggers she might have defined that block certain kinds of inserts > are actually going to block those inserts; any constraints that she > has applied to the table are going to be enforced against all new > rows; and any default expressions she supplies are actually going to > work. True, but I still find myself suspending my disbelief. Which of these use cases make sense for SECURITY INVOKER? > I think Bob has to be OK with those things too; otherwise, he > just shouldn't insert anything into the table. Right, but why should Bob's privileges be needed to do any of those things? Any difference in privileges, for those use cases, could only either get in the way of achieving Alice's goals, or cause a security problem for Bob. > But Bob doesn't have to be OK with Alice's code changing the session > state, or executing DML or DDL with his permissions. What's left? Should Bob be OK with Alice's code using his permissions for anything? > I wonder if > that's where we should be trying to insert restrictions here. Right > now, we think of SECURITY_RESTRICTED_OPERATION as a way to prevent a > function or procedure that runs under a different user ID than the > session user from poisoning the session state. But I'm thinking that > maybe the problem isn't really with code running under a different > user ID. It's with running code *provided by* a different user ID. > Maybe we should stop thinking about the security context as something > that you set when you switch to running as a different user ID, and > start thinking about it as something that needs to be set based on > the > relationship between the user that provided the code and the session > user. If they're not the same, some restrictions are probably > appropriate, except I think in the case where the user who provided > the code can become the session user anyway. I think you are saying that we should still run Alice's code with the privileges of Bob, but somehow make that safe(r) for Bob. Is that right? That sounds hard, and I'm still stuck at the "why" question. Why do we want to run Alice's code with Bob's permissions? The answers I have so far are abstract. For instance, maybe it's a clever SRF that takes table names as inputs and you want people to only be able to use the clever SRF with tables they have privileges on. But that's not what most functions do, and it's certainly not what most default expressions do. Regards, Jeff Davis
Re: Non-superuser subscription owners
On Wed, Feb 22, 2023 at 12:18 PM Jeff Davis wrote: > There are two questions: > > 1. Is the security situation with logical replication bad? Yes. You > nicely summarized just how bad. > > 2. Is it the same situation as accessing a table owned by a user you > don't absolutely trust? > > Regardless of how the second question is answered, it won't diminish > your point that logical replication is in a bad state. If another > situation is also bad, we should fix that too. Well said. > So I don't think "shouldn't" is quite good enough. In the first place, > the user needs to know that the risk exists. Second, what if they > actually do want to access a table owned by someone else for whatever > reason -- how do they do that safely? Good question. I don't think we currently have a good answer. > I can't resist mentioning that these are all SECURITY INVOKER problems. > SECURITY INVOKER is insecure unless the invoker absolutely trusts the > definer, and that only really makes sense if the definer is a superuser > (or something very close). That's why we keep adding exceptions with > SECURITY_RESTRICTED_OPERATION, which is really just a way to silently > ignore the SECURITY INVOKER label and use SECURITY DEFINER instead. That's an interesting way to look at it. I think there are perhaps two different possible perspectives here. One possibility is to take the view that you've adopted here, and blame it on SECURITY INVOKER. The other possibility, at least as I see it, is to blame it on the fact that we have so many places to attach executable code to tables and very few ways for people using those tables to limit their exposure to such code. Suppose Alice owns a table and attaches a trigger to it. If Bob inserts into that table, I think we have to run the trigger, because Alice is entitled to assume that, for example, any BEFORE triggers she might have defined that block certain kinds of inserts are actually going to block those inserts; any constraints that she has applied to the table are going to be enforced against all new rows; and any default expressions she supplies are actually going to work. I think Bob has to be OK with those things too; otherwise, he just shouldn't insert anything into the table. But Bob doesn't have to be OK with Alice's code changing the session state, or executing DML or DDL with his permissions. I wonder if that's where we should be trying to insert restrictions here. Right now, we think of SECURITY_RESTRICTED_OPERATION as a way to prevent a function or procedure that runs under a different user ID than the session user from poisoning the session state. But I'm thinking that maybe the problem isn't really with code running under a different user ID. It's with running code *provided by* a different user ID. Maybe we should stop thinking about the security context as something that you set when you switch to running as a different user ID, and start thinking about it as something that needs to be set based on the relationship between the user that provided the code and the session user. If they're not the same, some restrictions are probably appropriate, except I think in the case where the user who provided the code can become the session user anyway. > Another option is having some kind SECURITY NONE that would run the > code as a very limited-privilege user that can basically only access > the catalog. That would be useful for running default expressions and > the like without the definer or invoker needing to be careful. This might be possible, but I have some doubts about how difficult it would be to get all the details right. We'd need to make sure that this limited-privilege user couldn't ever create a table, or own one, or be granted any privileges to do anything other than the minimal set of things it's supposed to be able to do, or poison the session state, etc. And it would have weird results like current_user returning the name of the limited-privilege user rather than any of the users involved in the operation. Maybe that's all OK, but I find it more appealing to try to think about what kinds of operations can be performed in what contexts than to invent entirely new users. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On 2/22/23 14:12, Mark Dilger wrote: On Feb 22, 2023, at 10:49 AM, Jeff Davis wrote: On Wed, 2023-02-22 at 09:27 -0800, Mark Dilger wrote: Another option is to execute under the intersection of their privileges, where both the definer and the invoker need the privileges in order for the action to succeed. That would be more permissive than the proposed SECURITY NONE, while still preventing either party from hijacking privileges of the other. Interesting idea, I haven't heard of something like that being done before. Is there some precedent for that or a use case where it's helpful? > No current use case comes to mind, but I proposed it for event triggers one or two development cycles ago, to allow for non-superuser event trigger owners. The problems associated with allowing non-superusers to create and own event triggers were pretty similar to the problems being discussed in this thread. The intersection of privileges is used, for example, in multi-level security contexts where the intersection of the network-allowed levels and the subject allowed levels is used to bracket what can be accessed and how. Other examples I found with a quick search: https://docs.oracle.com/javase/8/docs/api/java/security/AccessController.html#doPrivileged-java.security.PrivilegedAction-java.security.AccessControlContext- https://learn.microsoft.com/en-us/dotnet/api/system.security.permissions.dataprotectionpermission.intersect?view=dotnet-plat-ext-7.0 -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Non-superuser subscription owners
> On Feb 22, 2023, at 10:49 AM, Jeff Davis wrote: > > On Wed, 2023-02-22 at 09:27 -0800, Mark Dilger wrote: >> Another option is to execute under the intersection of their >> privileges, where both the definer and the invoker need the >> privileges in order for the action to succeed. That would be more >> permissive than the proposed SECURITY NONE, while still preventing >> either party from hijacking privileges of the other. > > Interesting idea, I haven't heard of something like that being done > before. Is there some precedent for that or a use case where it's > helpful? No current use case comes to mind, but I proposed it for event triggers one or two development cycles ago, to allow for non-superuser event trigger owners. The problems associated with allowing non-superusers to create and own event triggers were pretty similar to the problems being discussed in this thread. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Wed, 2023-02-22 at 09:27 -0800, Mark Dilger wrote: > Another option is to execute under the intersection of their > privileges, where both the definer and the invoker need the > privileges in order for the action to succeed. That would be more > permissive than the proposed SECURITY NONE, while still preventing > either party from hijacking privileges of the other. Interesting idea, I haven't heard of something like that being done before. Is there some precedent for that or a use case where it's helpful? Regards, Jeff Davis
Re: Non-superuser subscription owners
> On Feb 22, 2023, at 9:18 AM, Jeff Davis wrote: > > Another option is having some kind SECURITY NONE that would run the > code as a very limited-privilege user that can basically only access > the catalog. That would be useful for running default expressions and > the like without the definer or invoker needing to be careful. Another option is to execute under the intersection of their privileges, where both the definer and the invoker need the privileges in order for the action to succeed. That would be more permissive than the proposed SECURITY NONE, while still preventing either party from hijacking privileges of the other. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Mon, 2023-02-06 at 14:40 -0500, Robert Haas wrote: > On Mon, Feb 6, 2023 at 2:18 PM Andres Freund > wrote: > > It's decidedly not great, yes. I don't know if it's quite a CVE > > type issue, > > after all, the same is true for any other type of query the > > superuser > > executes. But at the very least the documentation needs to be > > better, with a > > big red box making sure the admin is aware of the problem. > > I don't think that's the same thing at all. A superuser executing a > query interactively can indeed cause all sorts of bad things to > happen, but you don't have to log in as superuser and run DML queries > on tables owned by unprivileged users, and you shouldn't. There are two questions: 1. Is the security situation with logical replication bad? Yes. You nicely summarized just how bad. 2. Is it the same situation as accessing a table owned by a user you don't absolutely trust? Regardless of how the second question is answered, it won't diminish your point that logical replication is in a bad state. If another situation is also bad, we should fix that too. And I think the DML situation is really bad, too. Anyone reading our documentation would find extensive explanations about GRANT/REVOKE, and puzzle over the fine details of exactly how much they trust user foo. Do I trust foo enough for WITH GRANT OPTION? Does foo really need to see all of the columns of this table, or just a subset? But there's no obvious mention that user foo must trust you absolutely in order to exercise the GRANT at all, because you (as table owner) can trivially cause foo to execute arbitrary code. There's no warning or hint or suggestion at runtime to know that you are about to execute someone else's code with your privileges or that it might be dangerous. It gets worse. Let's say that user foo figures that out, and they're extra cautious to SET SESSION AUTHORIZATION or SET ROLE to drop their privileges before accessing a table. No good: the table owner can just craft their arbitrary code with a "RESET SESSION AUTHORIZATION" or a "RESET ROLE" at the top, and the code will still execute with the privileges of user foo. So I don't think "shouldn't" is quite good enough. In the first place, the user needs to know that the risk exists. Second, what if they actually do want to access a table owned by someone else for whatever reason -- how do they do that safely? I can't resist mentioning that these are all SECURITY INVOKER problems. SECURITY INVOKER is insecure unless the invoker absolutely trusts the definer, and that only really makes sense if the definer is a superuser (or something very close). That's why we keep adding exceptions with SECURITY_RESTRICTED_OPERATION, which is really just a way to silently ignore the SECURITY INVOKER label and use SECURITY DEFINER instead. At some point we need to ask: "when is SECURITY INVOKER both safe and useful?" and contain it to those cases, rather than silently ignoring it in an expanding list of cases. I know that the response here is that SECURITY DEFINER is somehow worse. Maybe for superuser-defined functions, it is. But basically, the problems with SECURITY DEFINER all amount to "the author of the code needs to be careful", which is a lot more intuitive than the problems with SECURITY INVOKER. Another option is having some kind SECURITY NONE that would run the code as a very limited-privilege user that can basically only access the catalog. That would be useful for running default expressions and the like without the definer or invoker needing to be careful. -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Non-superuser subscription owners
On Wed, Feb 1, 2023 at 4:02 PM Andres Freund wrote: > On 2023-01-30 15:32:34 -0500, Robert Haas wrote: > > I had a long think about what to do with ALTER SUBSCRIPTION ... OWNER > > TO in terms of permissions checks. > > As long as owner and run-as are the same, I think it's strongly > preferrable to *not* require pg_create_subscription. OK. > > Another question around ALTER SUBSCRIPTION ... OWNER TO and also ALTER > > SUBSCRIPTION .. RENAME is whether they ought to fail if you're not a > > superuser and password_required false is set. > > I don't really see a benefit in allowing it, so I'm inclined to go for > the more restrictive option. But this is a really weakly held opinion. I went back and forth on this and ended up with what you propose here. It's simpler to explain this way. > > + /* Is the use of a password mandatory? */ > > + must_use_password = MySubscription->passwordrequired && > > + !superuser_arg(MySubscription->owner); > > There's a few repetitions of this - perhaps worth putting into a helper? I don't think so. It's slightly different each time, because it's pulling data out of different data structures. > This still leaks the connection on error, no? I've attempted to fix this in v4, attached. -- Robert Haas EDB: http://www.enterprisedb.com v4-0001-Add-new-predefined-role-pg_create_subscriptions.patch Description: Binary data
Re: Non-superuser subscription owners
On Mon, Feb 6, 2023 at 2:18 PM Andres Freund wrote: > It's decidedly not great, yes. I don't know if it's quite a CVE type issue, > after all, the same is true for any other type of query the superuser > executes. But at the very least the documentation needs to be better, with a > big red box making sure the admin is aware of the problem. I don't think that's the same thing at all. A superuser executing a query interactively can indeed cause all sorts of bad things to happen, but you don't have to log in as superuser and run DML queries on tables owned by unprivileged users, and you shouldn't. But what we're talking about here is -- the superuser comes along and sets up logical replication in the configuration in what seems to be exactly the way it was intended to be used, and now any user who can log into the subscriber node can become superuser for free whenever they want, without the superuser doing anything at all, even logging in. Saying it's "not ideal" seems like you're putting it in the same category as "the cheese got moldy in the fridge" but to me it sounds more like "the fridge exploded and the house is on fire." If we were to document this, I assume that the warning we would add to the documentation would look like this: <-- begin documentation text --> Pretty much don't ever use logical replication. In any normal configuration, it lets every user on your system escalate to superuser whenever they want. It is possible to make it safe, if you make sure all the tables on the replica are owned by the superuser and none of them have any triggers, defaults, expression indexes, or anything else associated with them that might execute any code while replicating. But notice that this makes logical replication pretty much useless for one of its intended purposes, which is high availability, because if you actually fail over, you're going to then have to change the owners of all of those tables and apply any missing triggers, defaults, expression indexes, or anything like that which you may want to have. And then to fail back you're going to have to remove all of that stuff again and once again make the tables superuser-owned. That's obviously pretty impractical, so you probably shouldn't use logical replication at all until we get around to fixing this. You might wonder why we implemented a feature that can't be used in any kind of normal way without completely and totally breaking your system security -- but don't ask us, we don't know, either! <-- end documentation text --> Honestly, this makes the CREATEROLE exploit that I fixed recently in master look like a walk in the park. Sure, it's a pain for service providers, who might otherwise use it, but most normal users don't and wouldn't no matter how it worked, and really are not going to care. But people do use logical replication, and it seems to me that the issue you're describing here means that approximately 100% of those installations have a vulnerability allowing any local user who owns a table or can create one to escalate to superuser. Far from being not quite a CVE issue, that seems substantially more serious than most things we get CVEs for. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi, On 2023-02-06 14:07:39 -0500, Robert Haas wrote: > On Fri, Feb 3, 2023 at 3:47 AM Andres Freund wrote: > > On 2023-02-02 09:28:03 -0500, Robert Haas wrote: > > > I don't know what you mean by this. DML doesn't confer privileges. If > > > code gets executed and runs with the replication user's credentials, > > > that could lead to privilege escalation, but just moving rows around > > > doesn't, at least not in the database sense. > > > > Executing DML ends up executing code. Think predicated/expression > > indexes, triggers, default expressions etc. If a badly written trigger > > etc can be tricked to do arbitrary code exec, an attack will be able to > > run with the privs of the run-as user. How bad that is is influenced to > > some degree by the amount of privileges that user has. > > I spent some time studying this today. I think you're right. What I'm > confused about is: why do we consider this situation even vaguely > acceptable? Isn't this basically an admission that our logical > replication security model is completely and totally broken and we > need to fix it somehow and file for a CVE number? Like, in released > branches, you can't even have a subscription owned by a non-superuser. > But any non-superuser can set a default expression or create an enable > always trigger and sure enough, if that table is replicated, the > system will run that trigger as the subscription owner, who is a > superuser. Which AFAICS means that if a non-superuser owns a table > that is part of a subscription, they can instantly hack superuser. > Which seems, uh, extremely bad. Am I missing something? It's decidedly not great, yes. I don't know if it's quite a CVE type issue, after all, the same is true for any other type of query the superuser executes. But at the very least the documentation needs to be better, with a big red box making sure the admin is aware of the problem. I think we need some more fundamental ways to deal with this issue, including but not restricted to the replication context. Some potentially relevant discussion is in this thread: https://postgr.es/m/75b0dbb55e9febea54c441efff8012a6d2cb5bd7.camel%40j-davis.com I don't agree with Jeff's proposal, but I think there's some worthwhile ideas in the idea + followups. > And then, in master, where there's some provision for non-superuser > subscription owners, we maybe need to re-think the privileges required > to replicate into a table in the first place. I don't think that > having I/U/D permissions on a table is really sufficient to justify > performing those operations *as the table owner*; perhaps the check > ought to be whether you have the privileges of the table owner. Yes, I think we ought to check role membership, including non-inherited memberships. Greetings, Andres Freund
Re: Non-superuser subscription owners
On Fri, Feb 3, 2023 at 3:47 AM Andres Freund wrote: > On 2023-02-02 09:28:03 -0500, Robert Haas wrote: > > I don't know what you mean by this. DML doesn't confer privileges. If > > code gets executed and runs with the replication user's credentials, > > that could lead to privilege escalation, but just moving rows around > > doesn't, at least not in the database sense. > > Executing DML ends up executing code. Think predicated/expression > indexes, triggers, default expressions etc. If a badly written trigger > etc can be tricked to do arbitrary code exec, an attack will be able to > run with the privs of the run-as user. How bad that is is influenced to > some degree by the amount of privileges that user has. I spent some time studying this today. I think you're right. What I'm confused about is: why do we consider this situation even vaguely acceptable? Isn't this basically an admission that our logical replication security model is completely and totally broken and we need to fix it somehow and file for a CVE number? Like, in released branches, you can't even have a subscription owned by a non-superuser. But any non-superuser can set a default expression or create an enable always trigger and sure enough, if that table is replicated, the system will run that trigger as the subscription owner, who is a superuser. Which AFAICS means that if a non-superuser owns a table that is part of a subscription, they can instantly hack superuser. Which seems, uh, extremely bad. Am I missing something? Based on other remarks you made upthread, it seems like we ought to be doing the actual replication as the table owner, since the table owner has to be prepared for executable code attached to the table to be re-run on rows in the table at any table when somebody does a REINDEX. And then, in master, where there's some provision for non-superuser subscription owners, we maybe need to re-think the privileges required to replicate into a table in the first place. I don't think that having I/U/D permissions on a table is really sufficient to justify performing those operations *as the table owner*; perhaps the check ought to be whether you have the privileges of the table owner. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi, On 2023-02-02 09:28:03 -0500, Robert Haas wrote: > I don't know what you mean by this. DML doesn't confer privileges. If > code gets executed and runs with the replication user's credentials, > that could lead to privilege escalation, but just moving rows around > doesn't, at least not in the database sense. Executing DML ends up executing code. Think predicated/expression indexes, triggers, default expressions etc. If a badly written trigger etc can be tricked to do arbitrary code exec, an attack will be able to run with the privs of the run-as user. How bad that is is influenced to some degree by the amount of privileges that user has. Greetings, Andres Freund
Re: Non-superuser subscription owners
On Wed, Feb 1, 2023 at 3:37 PM Andres Freund wrote: > The general point is that IMO is that in many setups you should use a > user with fewer privileges than a superuser. It doesn't really matter > whether we have an ad-hoc restriction for system catalogs. More often > than not being able to modify other tables will give you a lot of > privileges too. I don't know what you mean by this. DML doesn't confer privileges. If code gets executed and runs with the replication user's credentials, that could lead to privilege escalation, but just moving rows around doesn't, at least not in the database sense. It might confer unanticipated real-world benefits, like if you can update your own salary or something, but in the context of replication you have to have had the ability to do that on some other node anyway. If that change wasn't supposed to get replicated to the local node, then why are we using replication? Or why is that table in the publication? I'm confused. > > The thing I'm struggling to understand is: if you only want to > > replicate into tables that Alice can write, why not just make Alice > > own the subscription? > > Because it implies that the replication happens as a user that's > privileged enough to change the configuration of replication. But again, replication is just about inserting, updating, and deleting rows. To change the replication configuration, you have to be able to parlay that into the ability to execute code. That's why I think trigger security is really important. But I'm wondering if there's some way we can handle that that doesn't require us to make a decision about arun-as user. For instance, if firing triggers as the table owner is an acceptable solution, then the only thing that the run-as user is actually controlling is which tables we're willing to replicate into in the first place (unless there's some other way that logical replication can run arbitrary code). The name almost becomes a misnomer in that case. It's not a run-as user, it's use-this-user's-permissions-to-see-if-I-should-fail-replication user. > > I was thinking a little bit more about that. I > > still maintain that the current system is poorly set up to make that > > work, but suppose we wanted to do better. We could add filtering on > > the subscriber side, like you list schemas or specific relations that > > you are or are not willing to replicate into. > > Isn't that largely a duplication of the ACLs on relations etc? Yeah, maybe. > > I'm not quite sure what we do here now, but I agree that trigger > > firing seems like a problem. It might be that we need to worry about > > the user on the origin server, too. If Alice inserts a row that causes > > a replicated table owned by Bob to fire a trigger or evaluate a > > default expression or whatever due the presence of a subscription > > owned by Charlie, there is a risk that Alice might try to attack > > either Bob or Charlie, or that Bob might try to attack Charlie. > > The attack on Bob exists without logical replication too - a REINDEX or > such is executed as the owner of the relation and re-evaluates index > expressions, constraints etc. Given our security model I don't think we > can protect the relation owner if they trust somebody to insert rows, so > I don't really know what we can do to protect Charlie against Bob. Yikes. > > > > And if we suppose that that already works and is safe, well then > > > > what's the case where I do need a run-as user? > > > > > > It's not at all safe today, IMO. You need to trust that nothing bad will > > > be replicated, otherwise the owner of the subscription has to be > > > considered compromised. > > > > What kinds of things are bad to replicate? > > I think that's unfortunately going to be specific to a setup. Can you give an example? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Wed, Feb 1, 2023 at 1:09 PM Mark Dilger wrote: > > On Feb 1, 2023, at 6:43 AM, Robert Haas wrote: > > The thing I'm > > struggling to understand is: if you only want to replicate into tables > > that Alice can write, why not just make Alice own the subscription? > > For a run-as user to make sense, you need a scenario where we want the > > replication to target only tables that Alice can touch, but we also > > don't want Alice herself to be able to touch the subscription, so you > > make Alice the run-as user and yourself the owner, or something like > > that. But I'm not sure what that scenario is exactly. > > This "run-as" idea came about because we didn't want arbitrary roles to be > able to change the subscription's connection string. A competing idea was to > have a server object rather than a string, with roles like Alice being able > to use the server object if they have been granted usage privilege, and not > otherwise. So the "run-as" and "server" ideas were somewhat competing. As far as not changing the connection string goes, a few more ideas have entered the fray: the current patch uses a password_required property that is modelled on postgres_fdw, and I've elsewhere proposed a reverse pg_hba.conf. IMHO, for the use cases that I can imagine, the reverse pg_hba.conf idea feels better than all competitors, because it's the only idea that lets you define a class of acceptable connection strings. Jeff's idea of a separate connection object is fine if you have a specific, short list of connection strings and you want to allow those and disallow everything else, and there may be cases where people want that, and that's fine, but my guess is that it's overly restrictive in a lot of environments. The password_required property has the virtue of being compatible with what we do in other places right now, and of preventing wraparound-to-superuser attacks effectively, but it's totally unconfigurable and that sucks. The runas user idea gives you some control over who is allowed to set the connection string, but it doesn't help you delegate that to a non-superuser, because the idea there is that you want the non-superuser to be able to set connection strings that are OK with the actual superuser but not others. I think part of my confusion here is that I thought that the point of the runas user was to defend against logical replication itself changing the connection string, and I don't see how it would do that. It's just moving rows around. If the point is that somebody who can log in as the runas user might change the connection string to something we don't like, that makes somewhat more sense. I think I had in my head that you wouldn't use someone's actual login role to run logical replication, but rather some role specifically set up for that purpose. In that scenario, nobody's running SQL commands as the runas user, so even if they also own the subscription, there's no way for it to get modified. > > Mark was postulating a scenario where the publisher and subscriber > > don't trust each other. I was thinking a little bit more about that. I > > still maintain that the current system is poorly set up to make that > > work, but suppose we wanted to do better. We could add filtering on > > the subscriber side, like you list schemas or specific relations that > > you are or are not willing to replicate into. Then you could, for > > example, connect your subscription to a certain remote publication, > > but with the restriction that you're only willing to replicate into > > the "headquarters" schema. Then we'll replicate whatever tables they > > send us, but if the dorks at headquarters mess up the publications on > > their end (intentionally or otherwise) and add some tables from the > > "locally_controlled_stuff" schema, we'll refuse to replicate that into > > our eponymous schema. > > That example is good, though I don't see how "filters" are better than > roles+privileges. Care to elaborate? I'm not sure that they are. Are we assuming that the user who is creating subscriptions is also powerful enough to create roles and give them just the required amount of privilege? If so, it seems like they might as well just do it that way. And maybe we should assume that, because in most cases, a dedication replication role makes more sense to me than running replication under some role that you're also using for other things. On the other hand, I bet a lot of people today are just running replication as a superuser, in which case maybe this could be useful? This whole idea was mostly just me spitballing to see what other people thought. I'm not wild about the complexity involved for what you get out of it, so if we don't need it, that's more than fine with me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi,
On 2023-01-30 15:32:34 -0500, Robert Haas wrote:
> I had a long think about what to do with ALTER SUBSCRIPTION ... OWNER
> TO in terms of permissions checks. The previous version required that
> the new owner have permissions of pg_create_subscription, but there
> seems to be no particular reason for that rule except that it happens
> to be what I made the code do. So I changed it to say that the current
> owner must have CREATE privilege on the database, and must be able to
> SET ROLE to the new owner. This matches the rule for CREATE SCHEMA.
> Possibly we should *additionally* require that the person performing
> the rename still have pg_create_subscription, but that shouldn't be
> the only requirement.
As long as owner and run-as are the same, I think it's strongly
preferrable to *not* require pg_create_subscription.
> There seems to be a good deal of inconsistency here. If you want to
> give someone a schema, YOU need CREATE on the database. But if you
> want to give someone a table, THEY need CREATE on the containing
> schema. It make sense that we check permissions on the containing
> object, which could be a database or a schema depending on what you're
> renaming, but it's unclear to me why we sometimes check on the person
> performing the ALTER command and at other times on the recipient. It's
> also somewhat unclear to me why we are checking CREATE in the first
> place, especially on the donor. It might make sense to have a rule
> that you can't own an object in a place where you couldn't have
> created it, but there is no such rule, because you can give someone
> CREATE on a schema, they can create an object, and they you can take
> CREATE a way and they still own an object there. So it kind of looks
> to me like we made it up as we went along and that the result isn't
> very consistent, but I'm inclined to follow CREATE SCHEMA here unless
> there's some reason to do otherwise.
Yuck. No idea what the best policy around this is.
> Another question around ALTER SUBSCRIPTION ... OWNER TO and also ALTER
> SUBSCRIPTION .. RENAME is whether they ought to fail if you're not a
> superuser and password_required false is set.
I don't really see a benefit in allowing it, so I'm inclined to go for
the more restrictive option. But this is a really weakly held opinion.
> > > If there is, I think we could fix it by moving the LockSharedObject call
> > > up
> > > higher, above object_ownercheck. The only problem with that is it lets you
> > > lock an object on which you have no permissions: see
> > > 2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an
> > > analogue of RangeVarGetRelidExtended.
> >
> > Yea, we really should have something like RangeVarGetRelidExtended() for
> > other
> > kinds of objects. It'd take a fair bit of work / time to use it widely, but
> > it'll take even longer if we start in 5 years ;)
>
> We actually have something sort of like that in the form of
> get_object_address(). It doesn't allow for a callback, but it does
> have a retry loop.
Hm, sure looks like that code doesn't do any privilege checking...
> @@ -1269,13 +1270,19 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
>
> slotname,
>
> NAMEDATALEN);
>
> + /* Is the use of a password mandatory? */
> + must_use_password = MySubscription->passwordrequired &&
> + !superuser_arg(MySubscription->owner);
There's a few repetitions of this - perhaps worth putting into a helper?
> @@ -180,6 +181,13 @@ libpqrcv_connect(const char *conninfo, bool logical,
> const char *appname,
> if (PQstatus(conn->streamConn) != CONNECTION_OK)
> goto bad_connection_errmsg;
>
> + if (must_use_password && !PQconnectionUsedPassword(conn->streamConn))
> + ereport(ERROR,
> +
> (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
> + errmsg("password is required"),
> + errdetail("Non-superuser cannot connect if the
> server does not request a password."),
> + errhint("Target server's authentication method
> must be changed. or set password_required=false in the subscription
> attributes\
.")));
> +
> if (logical)
> {
> PGresult *res;
This still leaks the connection on error, no?
Greetings,
Andres Freund
Re: Non-superuser subscription owners
Hi, On 2023-02-01 09:43:39 -0500, Robert Haas wrote: > On Tue, Jan 31, 2023 at 7:01 PM Andres Freund wrote: > > I don't really understand that - the run-as approach seems like a > > necessary piece of improving the security model. > > > > I think it's perfectly reasonable to want to replicate from one system > > in another, but to not want to allow logical replication to insert into > > pg_class or whatnot. So not using superuser to execute the replication > > makes sense. > > > > This is particularly the case if you're just replicating a small part of > > the tables from one system to another. E.g. in a sharded setup, you may > > want to replicate metadata too servers. > > I don't think that a system catalog should be considered a valid > replication target, no matter who owns the subscription, so ISTM that > writing to pg_class should be blocked regardless. The general point is that IMO is that in many setups you should use a user with fewer privileges than a superuser. It doesn't really matter whether we have an ad-hoc restriction for system catalogs. More often than not being able to modify other tables will give you a lot of privileges too. > The thing I'm struggling to understand is: if you only want to > replicate into tables that Alice can write, why not just make Alice > own the subscription? Because it implies that the replication happens as a user that's privileged enough to change the configuration of replication. > Mark was postulating a scenario where the publisher and subscriber > don't trust each other. FWIW, I don't this this is mainly about "trust", but instead about layering security / the principle of least privilege. The "run-as" user (i.e. currently owner) is constantly performing work on behalf of a remote node, including executing code (default clauses etc). To make it harder to use such a cross-system connection to move from one system to the next, it's a good idea to execute it in the least privileged context possible. And I don't see why it'd need the permission to modify the definition of the subscription and similar "admin" tasks. It's not that such an extra layer would necessarily completely stop an attacker. But it might delay them and make their attack more noisy. Similarly, if I were to operate an important production environment again, I'd not have relations owned by the [pseudo]superuser, but by a user controlled by the [pseudo]superuser. That way somebody tricking the superuser into a REINDEX or such only gets the ability to execute code in a less privileged context. > I was thinking a little bit more about that. I > still maintain that the current system is poorly set up to make that > work, but suppose we wanted to do better. We could add filtering on > the subscriber side, like you list schemas or specific relations that > you are or are not willing to replicate into. Isn't that largely a duplication of the ACLs on relations etc? > > I think we'll need two things to improve upon the current situation: > > > > 1) run-as user, to reduce the scope of potential danger > > > > 2) Option to run the database inserts as the owner of the table, with a > >check that the run-as is actually allowed to perform work as the > >owning role. That prevents escalation from table owner (who could add > >default expressions etc) from gettng the privs of the > >run-as/replication owner. > > I'm not quite sure what we do here now, but I agree that trigger > firing seems like a problem. It might be that we need to worry about > the user on the origin server, too. If Alice inserts a row that causes > a replicated table owned by Bob to fire a trigger or evaluate a > default expression or whatever due the presence of a subscription > owned by Charlie, there is a risk that Alice might try to attack > either Bob or Charlie, or that Bob might try to attack Charlie. The attack on Bob exists without logical replication too - a REINDEX or such is executed as the owner of the relation and re-evaluates index expressions, constraints etc. Given our security model I don't think we can protect the relation owner if they trust somebody to insert rows, so I don't really know what we can do to protect Charlie against Bob. > > > And if we suppose that that already works and is safe, well then > > > what's the case where I do need a run-as user? > > > > It's not at all safe today, IMO. You need to trust that nothing bad will > > be replicated, otherwise the owner of the subscription has to be > > considered compromised. > > What kinds of things are bad to replicate? I think that's unfortunately going to be specific to a setup. Greetings, Andres Freund
Re: Non-superuser subscription owners
> On Feb 1, 2023, at 6:43 AM, Robert Haas wrote: > The thing I'm > struggling to understand is: if you only want to replicate into tables > that Alice can write, why not just make Alice own the subscription? > For a run-as user to make sense, you need a scenario where we want the > replication to target only tables that Alice can touch, but we also > don't want Alice herself to be able to touch the subscription, so you > make Alice the run-as user and yourself the owner, or something like > that. But I'm not sure what that scenario is exactly. This "run-as" idea came about because we didn't want arbitrary roles to be able to change the subscription's connection string. A competing idea was to have a server object rather than a string, with roles like Alice being able to use the server object if they have been granted usage privilege, and not otherwise. So the "run-as" and "server" ideas were somewhat competing. > Mark was postulating a scenario where the publisher and subscriber > don't trust each other. I was thinking a little bit more about that. I > still maintain that the current system is poorly set up to make that > work, but suppose we wanted to do better. We could add filtering on > the subscriber side, like you list schemas or specific relations that > you are or are not willing to replicate into. Then you could, for > example, connect your subscription to a certain remote publication, > but with the restriction that you're only willing to replicate into > the "headquarters" schema. Then we'll replicate whatever tables they > send us, but if the dorks at headquarters mess up the publications on > their end (intentionally or otherwise) and add some tables from the > "locally_controlled_stuff" schema, we'll refuse to replicate that into > our eponymous schema. That example is good, though I don't see how "filters" are better than roles+privileges. Care to elaborate? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Tue, Jan 31, 2023 at 7:01 PM Andres Freund wrote: > I don't really understand that - the run-as approach seems like a > necessary piece of improving the security model. > > I think it's perfectly reasonable to want to replicate from one system > in another, but to not want to allow logical replication to insert into > pg_class or whatnot. So not using superuser to execute the replication > makes sense. > > This is particularly the case if you're just replicating a small part of > the tables from one system to another. E.g. in a sharded setup, you may > want to replicate metadata too servers. I don't think that a system catalog should be considered a valid replication target, no matter who owns the subscription, so ISTM that writing to pg_class should be blocked regardless. The thing I'm struggling to understand is: if you only want to replicate into tables that Alice can write, why not just make Alice own the subscription? For a run-as user to make sense, you need a scenario where we want the replication to target only tables that Alice can touch, but we also don't want Alice herself to be able to touch the subscription, so you make Alice the run-as user and yourself the owner, or something like that. But I'm not sure what that scenario is exactly. Mark was postulating a scenario where the publisher and subscriber don't trust each other. I was thinking a little bit more about that. I still maintain that the current system is poorly set up to make that work, but suppose we wanted to do better. We could add filtering on the subscriber side, like you list schemas or specific relations that you are or are not willing to replicate into. Then you could, for example, connect your subscription to a certain remote publication, but with the restriction that you're only willing to replicate into the "headquarters" schema. Then we'll replicate whatever tables they send us, but if the dorks at headquarters mess up the publications on their end (intentionally or otherwise) and add some tables from the "locally_controlled_stuff" schema, we'll refuse to replicate that into our eponymous schema. I don't think this kind of system is well-suited to environments where people are totally hostile to each other, because you still need to have replication slots on the remote side and stuff. Also, having the remote side decode stuff and ignoring it locally is expensive, and I bet if we add stuff like this then people will misuse it and be sad. But it would make the system easier to reason about: I know for sure that this subscription will only write to these places, because that's all I've given it permission to do. In the sharding scenario you mention, if you want to provide accidental writes to unrelated tables due to the publication being not what we expect, you can either make the subscription owned by the same role that owns the sharded tables, or a special-purpose role that has permission to write to exactly the set of tables that you expect to be touched and no others. Or, if you had something like what I posited in the last paragraph, you could use that instead. But I don't see how a separate run-as user helps. If I'm just being super-dense here, I hope that one of you will explain using short words. :-) > I think we'll need two things to improve upon the current situation: > > 1) run-as user, to reduce the scope of potential danger > > 2) Option to run the database inserts as the owner of the table, with a >check that the run-as is actually allowed to perform work as the >owning role. That prevents escalation from table owner (who could add >default expressions etc) from gettng the privs of the >run-as/replication owner. I'm not quite sure what we do here now, but I agree that trigger firing seems like a problem. It might be that we need to worry about the user on the origin server, too. If Alice inserts a row that causes a replicated table owned by Bob to fire a trigger or evaluate a default expression or whatever due the presence of a subscription owned by Charlie, there is a risk that Alice might try to attack either Bob or Charlie, or that Bob might try to attack Charlie. > > And if we suppose that that already works and is safe, well then > > what's the case where I do need a run-as user? > > It's not at all safe today, IMO. You need to trust that nothing bad will > be replicated, otherwise the owner of the subscription has to be > considered compromised. What kinds of things are bad to replicate? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi, On 2023-01-30 10:44:29 -0500, Robert Haas wrote: > On a technical level, I think that the idea of having a separate > objection for the connection string vs. the subscription itself is > perfectly sound, and to repeat what I said earlier, if someone wants > to implement that, cool. I also agree that it has the advantage that > you specify, namely, that someone can have rights to modify one of > those objects but not the other. What that lets you do is define a > short list of known systems and say, hey, you can replicate whatever > tables you want with whatever options you want, but only between these > systems. I'm not quite sure what problem that solves, though. That does seem somewhat useful, but also fairly limited, at least as long as it's really just a single connection, rather than a "pattern" of safe connections. > Unfortunately, I have even less of an idea about what the run-as > concept is supposed to accomplish. I mean, at one level, I see it > quite clearly: the user creating the subscription wants replication to > have restricted privileges when it's running, and so they make the > run-as user some role with fewer privileges than their own. Brilliant. > But then I get stuck: against what kind of attack does that actually > protect us? If I'm a high privilege user, perhaps even a superuser, > and it's not safe to have logical replication running as me, then it > seems like the security model of logical replication is fundamentally > busted and we need to fix that. I don't really understand that - the run-as approach seems like a necessary piece of improving the security model. I think it's perfectly reasonable to want to replicate from one system in another, but to not want to allow logical replication to insert into pg_class or whatnot. So not using superuser to execute the replication makes sense. This is particularly the case if you're just replicating a small part of the tables from one system to another. E.g. in a sharded setup, you may want to replicate metadata too servers. Even if all the systems are operated by people you trust (including possibly even yourself, if you want to go that far), you may want to reduce the blast radius of privilege escalation, or even just bugs, to a smaller amount of data. I think we'll need two things to improve upon the current situation: 1) run-as user, to reduce the scope of potential danger 2) Option to run the database inserts as the owner of the table, with a check that the run-as is actually allowed to perform work as the owning role. That prevents escalation from table owner (who could add default expressions etc) from gettng the privs of the run-as/replication owner. I think it makes sense for 1) to be a fairly privileged user, but I think it's good practice for that user to not be allowed to change the system configuration etc. > It can't be right to say that if you have 263 users in a database and > you want to replicate the whole database to some other node, you need > 263 different subscriptions with a different run-as user for each. You > need to be able to run all of that logical replication as the > superuser or some other high-privilege user and not end up with a > security compromise. I'm not quite following along here - are you thinking of 263 tables owned by 263 users? If yes, that's why I am thinking that we need the option to perform each table modification as the owner of that table (with the same security restrictions we use for REINDEX etc). > And if we suppose that that already works and is safe, well then > what's the case where I do need a run-as user? It's not at all safe today, IMO. You need to trust that nothing bad will be replicated, otherwise the owner of the subscription has to be considered compromised. Greetings, Andres Freund
Re: Non-superuser subscription owners
> On Jan 30, 2023, at 1:29 PM, Robert Haas wrote: > > I feel like you're accusing me of removing functionality that has > never existed. A subscription doesn't run as the subscription creator. > It runs as the subscription owner. If you or anyone else had added the > capability for it to run as someone other than the subscription owner, > I certainly wouldn't be trying to back that capability out as part of > this patch, and because there isn't, I'm not proposing to add that as > part of this patch. I don't see how that makes me guilty of squashing > anything together. The current state of affairs, where the run-as user > is taken from pg_subscription.subowner, the same field that is updated > by ALTER SUBSCRIPTION ... OWNER TO, is the result of your work, not > anything that I have done or am proposing to do. > > I also *emphatically* disagree with the idea that this undoes what you > worked on. My patch would be *impossible* without your work. Prior to > your work, the run-as user was always, basically, the superuser, and > so the idea of allowing anyone other than a superuser to execute > CREATE SUBSCRIPTION would be flat-out nuts. Because of your work, > that's now a thing that we may be able to reasonably allow, if we can > work through the remaining issues. So I'm grateful to you, and also > sorry to hear that you're annoyed with me. But I still don't think > that the fact that the division you want doesn't exist is somehow my > fault. > > I'm kind of curious why you *didn't* make this distinction at the time > that you were did the other work in this area. Maybe my memory is > playing tricks on me again, but I seem to recall talking about the > idea with you at the time, and I seem to recall thinking that it > sounded like an OK idea. I seem to vaguely recall us discussing > hazards like: well, what if replication causes code to get executed as > the subscription owner that that causes something bad to happen? But I > think the only way that happens is if they put triggers on the tables > that are being replicated, which is their choice, and they can avoid > installing problematic code there if they want. I think there might > have been some other scenarios, too, but I just can't remember. In any > case, I don't think the idea is completely without merit. I think it > could very well be something that we want to have for one reason or > another. But I don't currently understand exactly what those reasons > are, and I don't see any reason why one patch should both split owner > from run-as user and also allow the owner to be a non-superuser. That > seems like two different efforts to me. I don't have a concrete problem with your patch, and wouldn't object if you committed it. My concerns were more how you were phrasing things, but it seems not worth any additional conversation, because it's probably a distinction without a difference. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Mon, Jan 30, 2023 at 3:27 PM Mark Dilger wrote: > That was an aspirational example in which there's infinite daylight between > the publisher and subscriber. I, too, doubt that's ever going to be > possible. But I still think we should aspire to some extra daylight between > the two. Perhaps IANA doesn't publish to the whole world, but instead > publishes only to subscribers who have a contract in place, and have agreed > to monetary penalties should they abuse the publishing server. Whatever. > There's going to be some amount of daylight possible if we design for it, and > none otherwise. > > My real argument here isn't against your goal of having non-superusers who > can create subscriptions. That part seems fine to me. > > Given that my work last year made it possible for subscriptions to run as > somebody other than the subscription creator, it annoys me that you now want > the subscription creator's privileges to be what the subscription runs as. > That seems to undo what I worked on. In my mental model of a > (superuser-creator, non-superuser-owner) pair, it seems you're logically only > touching the lefthand side, so you should then have a (nonsuperuser-creator, > nonsuperuser-owner) pair. But you don't. You go the apparently needless > extra step of just squashing them together. I just don't see why it needs to > be like that. I feel like you're accusing me of removing functionality that has never existed. A subscription doesn't run as the subscription creator. It runs as the subscription owner. If you or anyone else had added the capability for it to run as someone other than the subscription owner, I certainly wouldn't be trying to back that capability out as part of this patch, and because there isn't, I'm not proposing to add that as part of this patch. I don't see how that makes me guilty of squashing anything together. The current state of affairs, where the run-as user is taken from pg_subscription.subowner, the same field that is updated by ALTER SUBSCRIPTION ... OWNER TO, is the result of your work, not anything that I have done or am proposing to do. I also *emphatically* disagree with the idea that this undoes what you worked on. My patch would be *impossible* without your work. Prior to your work, the run-as user was always, basically, the superuser, and so the idea of allowing anyone other than a superuser to execute CREATE SUBSCRIPTION would be flat-out nuts. Because of your work, that's now a thing that we may be able to reasonably allow, if we can work through the remaining issues. So I'm grateful to you, and also sorry to hear that you're annoyed with me. But I still don't think that the fact that the division you want doesn't exist is somehow my fault. I'm kind of curious why you *didn't* make this distinction at the time that you were did the other work in this area. Maybe my memory is playing tricks on me again, but I seem to recall talking about the idea with you at the time, and I seem to recall thinking that it sounded like an OK idea. I seem to vaguely recall us discussing hazards like: well, what if replication causes code to get executed as the subscription owner that that causes something bad to happen? But I think the only way that happens is if they put triggers on the tables that are being replicated, which is their choice, and they can avoid installing problematic code there if they want. I think there might have been some other scenarios, too, but I just can't remember. In any case, I don't think the idea is completely without merit. I think it could very well be something that we want to have for one reason or another. But I don't currently understand exactly what those reasons are, and I don't see any reason why one patch should both split owner from run-as user and also allow the owner to be a non-superuser. That seems like two different efforts to me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Fri, Jan 27, 2023 at 5:00 PM Andres Freund wrote: > > Or, another thought, maybe this should be checking for CREATE on the > > current database + also pg_create_subscription. That seems like it > > might be the right idea, actually. > > Yes, that seems like a good idea. Done in this version. I also changed check_conninfo to take an extra argument instead of returning a Boolean, as per your suggestion. I had a long think about what to do with ALTER SUBSCRIPTION ... OWNER TO in terms of permissions checks. The previous version required that the new owner have permissions of pg_create_subscription, but there seems to be no particular reason for that rule except that it happens to be what I made the code do. So I changed it to say that the current owner must have CREATE privilege on the database, and must be able to SET ROLE to the new owner. This matches the rule for CREATE SCHEMA. Possibly we should *additionally* require that the person performing the rename still have pg_create_subscription, but that shouldn't be the only requirement. This change means that you can't just randomly give your subscription to the superuser (with or without concurrently attempting some other change as per your other comments) which is good because you can't do that with other object types either. There seems to be a good deal of inconsistency here. If you want to give someone a schema, YOU need CREATE on the database. But if you want to give someone a table, THEY need CREATE on the containing schema. It make sense that we check permissions on the containing object, which could be a database or a schema depending on what you're renaming, but it's unclear to me why we sometimes check on the person performing the ALTER command and at other times on the recipient. It's also somewhat unclear to me why we are checking CREATE in the first place, especially on the donor. It might make sense to have a rule that you can't own an object in a place where you couldn't have created it, but there is no such rule, because you can give someone CREATE on a schema, they can create an object, and they you can take CREATE a way and they still own an object there. So it kind of looks to me like we made it up as we went along and that the result isn't very consistent, but I'm inclined to follow CREATE SCHEMA here unless there's some reason to do otherwise. Another question around ALTER SUBSCRIPTION ... OWNER TO and also ALTER SUBSCRIPTION .. RENAME is whether they ought to fail if you're not a superuser and password_required false is set. They are separate code paths from the rest of the ALTER SUBSCRIPTION cases, so if we want that to be a rule we need dedicated code for it. I'm not quite sure what's right. There's no comparable case for ALTER USER MAPPING because a user mapping doesn't have an owner and so can't be reassigned to a new owner. I don't see what the harm is, especially for RENAME, but I might be missing something, and it certainly seems arguable. > > I'm not entirely clear whether there's a hazard there. > > I'm not at all either. It's just a code pattern that makes me anxious - I > suspect there's a few places it makes us more vulnerable. It looks likely to me that it was cut down from the CREATE SCHEMA code, FWIW. > > If there is, I think we could fix it by moving the LockSharedObject call up > > higher, above object_ownercheck. The only problem with that is it lets you > > lock an object on which you have no permissions: see > > 2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an > > analogue of RangeVarGetRelidExtended. > > Yea, we really should have something like RangeVarGetRelidExtended() for other > kinds of objects. It'd take a fair bit of work / time to use it widely, but > it'll take even longer if we start in 5 years ;) We actually have something sort of like that in the form of get_object_address(). It doesn't allow for a callback, but it does have a retry loop. -- Robert Haas EDB: http://www.enterprisedb.com v3-0001-Add-new-predefined-role-pg_create_subscriptions.patch Description: Binary data
Re: Non-superuser subscription owners
> On Jan 30, 2023, at 11:30 AM, Robert Haas wrote: > > CREATE SUBSCRIPTION > provides no tools at all for filtering the data that the subscriber > chooses to send. > > Now that can be changed, I suppose, and a run-as user would be one way > to make progress in that direction. But I'm not sure how viable that > is, because... > >>> In what >>> circumstances would be it be reasonable to give responsibility for >>> those objects to different and especially mutually untrusting users? >> >> When public repositories of data, such as the IANA whois database, publish >> their data via postgres publications. > > ... for that to work, IANA would need to set up the database so that > untrusted parties can create logical replication slots on their > PostgreSQL server. And I think that granting REPLICATION privilege on > your database to random people on the Internet is not really viable, > nor intended to be viable. That was an aspirational example in which there's infinite daylight between the publisher and subscriber. I, too, doubt that's ever going to be possible. But I still think we should aspire to some extra daylight between the two. Perhaps IANA doesn't publish to the whole world, but instead publishes only to subscribers who have a contract in place, and have agreed to monetary penalties should they abuse the publishing server. Whatever. There's going to be some amount of daylight possible if we design for it, and none otherwise. My real argument here isn't against your goal of having non-superusers who can create subscriptions. That part seems fine to me. Given that my work last year made it possible for subscriptions to run as somebody other than the subscription creator, it annoys me that you now want the subscription creator's privileges to be what the subscription runs as. That seems to undo what I worked on. In my mental model of a (superuser-creator, non-superuser-owner) pair, it seems you're logically only touching the lefthand side, so you should then have a (nonsuperuser-creator, nonsuperuser-owner) pair. But you don't. You go the apparently needless extra step of just squashing them together. I just don't see why it needs to be like that. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Mon, Jan 30, 2023 at 1:46 PM Mark Dilger wrote: > I have a grim view of the requirement that publishers and subscribers trust > each other. Even when they do trust each other, they can firewall attacks by > acting as if they do not. I think it's OK if the CREATE PUBLICATION user doesn't particularly trust the CREATE SUBSCRIPTION user, because the publication is just a grouping of tables to which somebody can pay attention or not. The CREATE PUBLICATION user isn't compromised either way. But, at least as things stand, I don't see how the CREATE SUBSCRIPTION user get away with not trusting the CREATE PUBLICATION user. CREATE SUBSCRIPTION provides no tools at all for filtering the data that the subscriber chooses to send. Now that can be changed, I suppose, and a run-as user would be one way to make progress in that direction. But I'm not sure how viable that is, because... > > In what > > circumstances would be it be reasonable to give responsibility for > > those objects to different and especially mutually untrusting users? > > When public repositories of data, such as the IANA whois database, publish > their data via postgres publications. ... for that to work, IANA would need to set up the database so that untrusted parties can create logical replication slots on their PostgreSQL server. And I think that granting REPLICATION privilege on your database to random people on the Internet is not really viable, nor intended to be viable. As the CREATE ROLE documentation says, "A role having the REPLICATION attribute is a very highly privileged role." Concretely, this kind of setup would have the problem that you could kill the IANA database by just creating a replication slot and then not using it (or replicating from it only very very slowly). Eventually, the replication slot would either hold back xmin enough that you got a lot of bloat, or cause enough WAL to be retained that you ran out of disk space. Maybe you could protect yourself against that kind of problem by cutting off users who get too far behind, but that also cuts off people who just have an outage for longer than your cutoff. Also, anyone who can connection to a replication slot can also connect to any other replication slot, and drop any replication slot. So if IANA did grant REPLICATION privilege to random people on the Internet, one of them could jump into the system and screw things up for all the others. This kind of setup just doesn't seem viable to me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
> On Jan 30, 2023, at 9:26 AM, Robert Haas wrote: > > So basically this doesn't really feel like a valid scenario to me. > We're supposed to believe that Alice is hostile to Bob, but the > superuser doesn't seem to have thought very carefully about how Bob is > supposed to defend himself against Alice, and Bob doesn't even seem to > be trying. Maybe we should rename the users to Samson and Delilah? :-) No, Atahualpa and Pizarro. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
> On Jan 30, 2023, at 9:26 AM, Robert Haas wrote: > > First, it doesn't seem to make a lot of sense to have one person > managing the publications and someone else managing the subscriptions, > and especially if those parties are mutually untrusting. I can't think > of any real reason to set things up that way. Sure, you could, but why > would you? You could, equally, decide that one member of your > household was going to decide what's for dinner every night, and some > other member of your household was going to decide what gets purchased > at the grocery store each week. If those two people exercise their > responsibilities without tight coordination, or with hostile intent > toward each other, things are going to go badly, but that's not an > argument for putting a combination lock on the flour canister. It's an > argument for getting along better, or not having such a dumb system in > the first place. I don't quite see how the situation you postulate in > (A) and (B) is any different. Publications and subscriptions are as > closely connected as food purchases and meals. The point of a > publication is for it to connect up to a subscription. I have a grim view of the requirement that publishers and subscribers trust each other. Even when they do trust each other, they can firewall attacks by acting as if they do not. > In what > circumstances would be it be reasonable to give responsibility for > those objects to different and especially mutually untrusting users? When public repositories of data, such as the IANA whois database, publish their data via postgres publications. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Mon, Jan 30, 2023 at 11:11 AM Mark Dilger wrote: > > On Jan 30, 2023, at 7:44 AM, Robert Haas wrote: > > > > And if we suppose that > > that already works and is safe, well then what's the case where I do > > need a run-as user? > > A) Alice publishes tables, and occasionally adds new tables to existing > publications. > > B) Bob manages subscriptions, and periodically runs "refresh publication". > Bob also creates new subscriptions for people when a row is inserted into the > "please create a subscription for me" table which Bob owns, using a trigger > that Bob created on that table. > > C) Alice creates a "please create a subscription for me" table on the > publishing database, adds lots of malicious requests, and adds that table to > the publication. > > D) Bob replicates the table, fires the trigger, creates the malicious > subscriptions, and starts replicating all that stuff, too. > > I think that having Charlie, not Bob, as the "run-as" user helps somewhere > right around (D). I suppose it does, but I have some complaints. First, it doesn't seem to make a lot of sense to have one person managing the publications and someone else managing the subscriptions, and especially if those parties are mutually untrusting. I can't think of any real reason to set things up that way. Sure, you could, but why would you? You could, equally, decide that one member of your household was going to decide what's for dinner every night, and some other member of your household was going to decide what gets purchased at the grocery store each week. If those two people exercise their responsibilities without tight coordination, or with hostile intent toward each other, things are going to go badly, but that's not an argument for putting a combination lock on the flour canister. It's an argument for getting along better, or not having such a dumb system in the first place. I don't quite see how the situation you postulate in (A) and (B) is any different. Publications and subscriptions are as closely connected as food purchases and meals. The point of a publication is for it to connect up to a subscription. In what circumstances would be it be reasonable to give responsibility for those objects to different and especially mutually untrusting users? Second, in step (B), we may ask why Bob is doing this with a trigger. If he's willing to create any subscription for which Alice asks, we could have just given Alice the authority to do those actions herself. Presumably, therefore, Bob is willing to create some subscriptions for which Alice may ask and not others. Perhaps this whole arrangement is just a workaround for the lack of a sensible system for controlling which connection strings Alice can use, in which case what is really needed here might be something like the separate connection object which Jeff postulated or my idea of a reverse pg_hba.conf. That kind of approach would give a better user interface to Alice, who wouldn't have to rephrase all of her CREATE SUBSCRIPTION commands as insert statements. Conversely, if Alice and Bob are truly dedicated to this convoluted system of creating subscriptions, then Bob needs to put logic into his trigger that's smart enough to block any malicious requests that Alice may make. He really brought this problem on himself by not doing that. Third, in step (C), it seems to me that whoever set up Alice's permissions has really messed up. Either the schema Bob is using for his create-me-a-subscription table exists on the primary and Alice has permission to create tables in that schema, or else that schema does not exist on the primary and Alice has permission to create it. Either way, that's a bad setup. Bob's table should be located in a schema for which Alice has only USAGE permissions and shouldn't have excess permissions on the table, either. Then this step can't happen. This step could also be blocked if, instead of using a table with a trigger, Bob wrote a security definer function or procedure and granted EXECUTE permission on that function or procedure to Alice. He's still going to need sanity checks, though, and if the function or procedure inserts into a logging table or something, he'd better make sure that table is adequately secured rather than being, say, a table owned by Alice with malicious triggers on it. So basically this doesn't really feel like a valid scenario to me. We're supposed to believe that Alice is hostile to Bob, but the superuser doesn't seem to have thought very carefully about how Bob is supposed to defend himself against Alice, and Bob doesn't even seem to be trying. Maybe we should rename the users to Samson and Delilah? :-) -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
> On Jan 30, 2023, at 7:44 AM, Robert Haas wrote: > > And if we suppose that > that already works and is safe, well then what's the case where I do > need a run-as user? A) Alice publishes tables, and occasionally adds new tables to existing publications. B) Bob manages subscriptions, and periodically runs "refresh publication". Bob also creates new subscriptions for people when a row is inserted into the "please create a subscription for me" table which Bob owns, using a trigger that Bob created on that table. C) Alice creates a "please create a subscription for me" table on the publishing database, adds lots of malicious requests, and adds that table to the publication. D) Bob replicates the table, fires the trigger, creates the malicious subscriptions, and starts replicating all that stuff, too. I think that having Charlie, not Bob, as the "run-as" user helps somewhere right around (D). — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
On Fri, Jan 27, 2023 at 5:56 PM Mark Dilger wrote: > If the owner cannot modify the subscription, then the owner degenerates into > a mere "run-as" user. Note that this isn't how things work now, and even if > we disallowed owners from modifying the connection string, there would still > be other attributes the owner could modify, such as the set of publications > subscribed. The proposed patch blocks every form of ALTER SUBSCRIPTION if password_required false is set and you aren't a superuser. Is there some other DML command that could be used to modify the set of publications subscribed? > More generally, my thinking on this thread is that there needs to be two > nosuperuser roles: A higher privileged role which can create a subscription, > and a lower privileged role serving the "run-as" function. Those shouldn't > be the same, because the "run-as" concept doesn't logically need to have > subscription creation power, and likely *shouldn't* have that power. > Depending on which sorts of attributes a subscription object has, such as the > connection string, the answer differs for whether the owner/"run-as" user > should get to change those attributes. One advantage of Jeff's idea of using > a server object rather than a string is that it becomes more plausibly safe > to allow the subscription owner to make changes to that attribute of the > subscription. There's some question in my mind about what these different mechanisms are intended to accomplish. On a technical level, I think that the idea of having a separate objection for the connection string vs. the subscription itself is perfectly sound, and to repeat what I said earlier, if someone wants to implement that, cool. I also agree that it has the advantage that you specify, namely, that someone can have rights to modify one of those objects but not the other. What that lets you do is define a short list of known systems and say, hey, you can replicate whatever tables you want with whatever options you want, but only between these systems. I'm not quite sure what problem that solves, though. >From my point of view, the two things that the superuser is most likely to want to do are (1) control the replication setup themselves and delegate nothing to any non-superuser or (2) give a non-superuser pretty much complete control over replication with just enough restrictions to avoid letting them do things that would compromise security, such as hacking the local superuser account. In other words, I expect that delegation of the logical replication configuration is usually going to be all or nothing. Jeff's system allows for a situation where you want to delegate some stuff but not everything, and specifically where you want to dedicate control over the subscription options and the tables being replicated, but not the connection strings. To me, that feels like a bit of an awkward configuration; I don't really understand in what situation that division of responsibility would be particularly useful. I trust that Jeff is proposing it because he knows of such a situation, but I don't know what it is. I feel like, even if I wanted to let people use some connection strings and not others, I'd probably want that control in some form other than listing a specific list of allowable connection strings -- I'd want to say things like "you have to use SSL" or "no connecting back to the local host," because that lets me enforce some general organizational policy without having to care specifically about how each subscription is being set up. Unfortunately, I have even less of an idea about what the run-as concept is supposed to accomplish. I mean, at one level, I see it quite clearly: the user creating the subscription wants replication to have restricted privileges when it's running, and so they make the run-as user some role with fewer privileges than their own. Brilliant. But then I get stuck: against what kind of attack does that actually protect us? If I'm a high privilege user, perhaps even a superuser, and it's not safe to have logical replication running as me, then it seems like the security model of logical replication is fundamentally busted and we need to fix that. It can't be right to say that if you have 263 users in a database and you want to replicate the whole database to some other node, you need 263 different subscriptions with a different run-as user for each. You need to be able to run all of that logical replication as the superuser or some other high-privilege user and not end up with a security compromise. And if we suppose that that already works and is safe, well then what's the case where I do need a run-as user? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
> On Jan 27, 2023, at 1:35 PM, Robert Haas wrote: > >> I started out asking what benefits it provides to own a subscription one >> cannot modify. But I think it is a good capability to have, to restrict the >> set of relations that replication could target. Although perhaps it'd be >> better to set the "replay user" as a separate property on the subscription? > > That's been proposed previously, but for reasons I don't quite > remember it seems not to have happened. I don't think it achieved > consensus. > >> Does owning a subscription one isn't allowed to modify useful outside of >> that? > > Uh, possibly that's a question for Mark or Jeff. I don't know. I can't > see what they would be, but I just work here. If the owner cannot modify the subscription, then the owner degenerates into a mere "run-as" user. Note that this isn't how things work now, and even if we disallowed owners from modifying the connection string, there would still be other attributes the owner could modify, such as the set of publications subscribed. More generally, my thinking on this thread is that there needs to be two nosuperuser roles: A higher privileged role which can create a subscription, and a lower privileged role serving the "run-as" function. Those shouldn't be the same, because the "run-as" concept doesn't logically need to have subscription creation power, and likely *shouldn't* have that power. Depending on which sorts of attributes a subscription object has, such as the connection string, the answer differs for whether the owner/"run-as" user should get to change those attributes. One advantage of Jeff's idea of using a server object rather than a string is that it becomes more plausibly safe to allow the subscription owner to make changes to that attribute of the subscription. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Non-superuser subscription owners
Hi, On 2023-01-27 16:35:11 -0500, Robert Haas wrote: > > Maybe a daft question: > > > > Have we considered using a "normal grant", e.g. on the database, instead of > > a > > role? Could it e.g. be useful to grant a user the permission to create a > > subscription in one database, but not in another? > > Potentially, but I didn't think we'd want to burn through permissions > bits that fast, even given 7b378237aa805711353075de142021b1d40ff3b0. > Still, if the consensus is otherwise, I can change it. I don't really have an opinion on what's better. I looked briefly whether there was discussion around ithis but I didn't see anything. pg_create_subcription feels a bit different than most of the other pg_* roles. For most of those there is no schema object to tie permissions to. But here there is. But I think there's good arguments against a GRANT approach, too. GRANT ALL ON DATABASE would suddenly be dangerous. How does it interact with database ownership? Etc. > Then I guess we'd end up with GRANT CREATE ON DATABASE and GRANT CREATE > SUBSCRIPTION ON DATABASE, which I'm sure wouldn't be confusing at all. Heh. I guess it could just be GRANT SUBSCRIBE. > Or, another thought, maybe this should be checking for CREATE on the > current database + also pg_create_subscription. That seems like it > might be the right idea, actually. Yes, that seems like a good idea. > > The subscription code already does ownership checks before locking and now > > there's also the passwordrequired before. Is it possible that this could > > open > > up some sort of race? Could e.g. the user change the ownership to the > > superuser in one session, do an ALTER in the other? > > > > It looks like your change won't increase the danger of that, as the > > superuser() check just checks the current users permissions. > > I'm not entirely clear whether there's a hazard there. I'm not at all either. It's just a code pattern that makes me anxious - I suspect there's a few places it makes us more vulnerable. > If there is, I think we could fix it by moving the LockSharedObject call up > higher, above object_ownercheck. The only problem with that is it lets you > lock an object on which you have no permissions: see > 2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an > analogue of RangeVarGetRelidExtended. Yea, we really should have something like RangeVarGetRelidExtended() for other kinds of objects. It'd take a fair bit of work / time to use it widely, but it'll take even longer if we start in 5 years ;) Perhaps the bulk of RangeVarGetRelidExtended() could be generalized by having a separate name->oid lookup callback? Greetings, Andres Freund
Re: Non-superuser subscription owners
On Fri, Jan 27, 2023 at 4:09 PM Andres Freund wrote: > Hm, compared to postgres_fdw, the user has far less control over what's > happening using that connection. Is there a way a subscription owner can > trigger evaluation of near-arbitrary SQL on the publisher side? I'm not aware of one, but what I think it would let you do is exfiltrate data you're not entitled to see. > I started out asking what benefits it provides to own a subscription one > cannot modify. But I think it is a good capability to have, to restrict the > set of relations that replication could target. Although perhaps it'd be > better to set the "replay user" as a separate property on the subscription? That's been proposed previously, but for reasons I don't quite remember it seems not to have happened. I don't think it achieved consensus. > Does owning a subscription one isn't allowed to modify useful outside of that? Uh, possibly that's a question for Mark or Jeff. I don't know. I can't see what they would be, but I just work here. > Maybe a daft question: > > Have we considered using a "normal grant", e.g. on the database, instead of a > role? Could it e.g. be useful to grant a user the permission to create a > subscription in one database, but not in another? Potentially, but I didn't think we'd want to burn through permissions bits that fast, even given 7b378237aa805711353075de142021b1d40ff3b0. Still, if the consensus is otherwise, I can change it. Then I guess we'd end up with GRANT CREATE ON DATABASE and GRANT CREATE SUBSCRIPTION ON DATABASE, which I'm sure wouldn't be confusing at all. Or, another thought, maybe this should be checking for CREATE on the current database + also pg_create_subscription. That seems like it might be the right idea, actually. > The subscription code already does ownership checks before locking and now > there's also the passwordrequired before. Is it possible that this could open > up some sort of race? Could e.g. the user change the ownership to the > superuser in one session, do an ALTER in the other? > > It looks like your change won't increase the danger of that, as the > superuser() check just checks the current users permissions. I'm not entirely clear whether there's a hazard there. If there is, I think we could fix it by moving the LockSharedObject call up higher, above object_ownercheck. The only problem with that is it lets you lock an object on which you have no permissions: see 2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an analogue of RangeVarGetRelidExtended. > and throwing an error like that will at the very least leak the connection, > fd, fd reservation. Which I just had fixed :). At the very least you'd need to > copy the stuff that "bad_connection:" does. OK. > I did wonder whether we should make libpqrcv_connect() use errsave() to return > errors. Or whether we should make libpqrcv register a memory context reset > callback that'd close the libpq connection. Yeah. Using errsave() might be better, but not sure I want to tackle that just now. > That is a somewhat odd API. Why does it throw for some things, but not > others? Seems a bit cleaner to pass in a parameter indicating whether it > should throw when not finding a password? Particularly because you already > pass that to walrcv_connect(). Will look into that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
Hi,
On 2023-01-27 14:42:01 -0500, Robert Haas wrote:
> At first, I found it a bit tempting to see this as a further
> indication that the force-a-password approach is not the right idea,
> because the test case clearly memorializes a desire *not* to require a
> password in this situation. However, the loopback-to-superuser attack
> is just as viable for subscription as it in other cases, and my
> previous patch would have done nothing to block it.
Hm, compared to postgres_fdw, the user has far less control over what's
happening using that connection. Is there a way a subscription owner can
trigger evaluation of near-arbitrary SQL on the publisher side?
> So what I did instead is add a password_required attribute, just like what
> postgres_fdw has. As in the case of postgres_fdw, the actual rule is that if
> the attribute is false, a password is not required, and if the attribute is
> true, a password is required unless you are a superuser. If you're a
> superuser, it still isn't. This is a slightly odd set of semantics but it
> has precedent and practical advantages. Also, as in the case of
> postgres_fdw, only a superuser can set password_required=false, and a
> subscription that has that setting can only be modified by a superuser, no
> matter who owns it.
I started out asking what benefits it provides to own a subscription one
cannot modify. But I think it is a good capability to have, to restrict the
set of relations that replication could target. Although perhaps it'd be
better to set the "replay user" as a separate property on the subscription?
Does owning a subscription one isn't allowed to modify useful outside of that?
> Even though I hate the require-a-password stuff with the intensity of
> a thousand suns, I think this is better than the previous patch,
> because it's more consistent with what we do elsewhere and because it
> blocks the loopback-connection-to-superuser attack. I think we
> *really* need to develop a better system for restricting proxied
> connections (no matter how proxied) and I hope that we do that soon.
> But inventing something for this purpose that differs from what we do
> elsewhere will make that task harder, not easier.
>
> Thoughts?
I think it's reasonable to mirror behaviour from elsewhere, and it'd let us
have this feature relatively soon - I think it's a common need to do this as a
non-superuser. It's IMO a very good idea to not subscribe as a superuser, even
if set up by a superuser...
But I also would understand if you / somebody else chose to focus on
implementing a less nasty connection model.
> Subject: [PATCH v2] Add new predefined role pg_create_subscriptions.
Maybe a daft question:
Have we considered using a "normal grant", e.g. on the database, instead of a
role? Could it e.g. be useful to grant a user the permission to create a
subscription in one database, but not in another?
> @@ -1039,6 +1082,16 @@ AlterSubscription(ParseState *pstate,
> AlterSubscriptionStmt *stmt,
>
> sub = GetSubscription(subid, false);
>
> + /*
> + * Don't allow non-superuser modification of a subscription with
> + * password_required=false.
> + */
> + if (!sub->passwordrequired && !superuser())
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +
> errmsg("password_required=false is superuser-only"),
> + errhint("Subscriptions with
> the password_required option set to false may only be created or modified by
> the superuser.")));
> +
> /* Lock the subscription so nobody else can do anything with it. */
> LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);
The subscription code already does ownership checks before locking and now
there's also the passwordrequired before. Is it possible that this could open
up some sort of race? Could e.g. the user change the ownership to the
superuser in one session, do an ALTER in the other?
It looks like your change won't increase the danger of that, as the
superuser() check just checks the current users permissions.
> @@ -180,6 +180,13 @@ libpqrcv_connect(const char *conninfo, bool logical,
> const char *appname,
> if (PQstatus(conn->streamConn) != CONNECTION_OK)
> goto bad_connection_errmsg;
>
> + if (must_use_password && !PQconnectionUsedPassword(conn->streamConn))
> + ereport(ERROR,
> +
> (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
> + errmsg("password is required"),
> + errdetail("Non-superuser cannot connect if the
> server does not request a password."),
> + errhint("Target server's authentication method
> must be changed.")));
> +
The documentation of libpqrcv_connect() says that:
* Returns NULL on error and fills the err with
Re: Non-superuser subscription owners
On Thu, Jan 19, 2023 at 8:46 PM Andres Freund wrote: > > If we already had (or have) that logic someplace else, it would > > probably make sense to reuse it > > We hve. See at least postgres_fdw's check_conn_params(), dblink's > dblink_connstr_check() and dblink_security_check(). In the patch I posted previously, I had some other set of checks, more or less along the lines suggested by Jeff. I looked into revising that approach and making the behavior match exactly what we do in those places instead. I find that it breaks 027_nosuperuser.pl. Specifically, where without the patch I get "ok 6 - nosuperuser admin with all table privileges can replicate into unpartitioned", with the patch it goes boom, because the subscription can't connect any more due to the password requirement. At first, I found it a bit tempting to see this as a further indication that the force-a-password approach is not the right idea, because the test case clearly memorializes a desire *not* to require a password in this situation. However, the loopback-to-superuser attack is just as viable for subscription as it in other cases, and my previous patch would have done nothing to block it. So what I did instead is add a password_required attribute, just like what postgres_fdw has. As in the case of postgres_fdw, the actual rule is that if the attribute is false, a password is not required, and if the attribute is true, a password is required unless you are a superuser. If you're a superuser, it still isn't. This is a slightly odd set of semantics but it has precedent and practical advantages. Also, as in the case of postgres_fdw, only a superuser can set password_required=false, and a subscription that has that setting can only be modified by a superuser, no matter who owns it. Even though I hate the require-a-password stuff with the intensity of a thousand suns, I think this is better than the previous patch, because it's more consistent with what we do elsewhere and because it blocks the loopback-connection-to-superuser attack. I think we *really* need to develop a better system for restricting proxied connections (no matter how proxied) and I hope that we do that soon. But inventing something for this purpose that differs from what we do elsewhere will make that task harder, not easier. Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com v2-0001-Add-new-predefined-role-pg_create_subscriptions.patch Description: Binary data
Re: Non-superuser subscription owners
On Thu, Jan 26, 2023 at 12:36 PM Jeff Davis wrote: > On Thu, 2023-01-26 at 09:43 -0500, Robert Haas wrote: > > I have no issue with that as a long-term plan. However, I think that > > for right now we should just introduce pg_create_subscription. It > > would make sense to add pg_create_connection in the same patch that > > adds a CREATE CONNECTION command (or whatever exact syntax we end up > > with) -- and that patch can also change CREATE SUBSCRIPTION to > > require > > both privileges where a connection string is specified directly. > > I assumed it would be a problem to say that pg_create_subscription was > enough to create a subscription today, and then later require > additional privileges (e.g. pg_create_connection). > > If that's not a problem, then this sounds fine with me. Wonderful! I'm working on a patch, but due to various distractions, it's not done yet. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Thu, 2023-01-26 at 09:43 -0500, Robert Haas wrote: > I have no issue with that as a long-term plan. However, I think that > for right now we should just introduce pg_create_subscription. It > would make sense to add pg_create_connection in the same patch that > adds a CREATE CONNECTION command (or whatever exact syntax we end up > with) -- and that patch can also change CREATE SUBSCRIPTION to > require > both privileges where a connection string is specified directly. I assumed it would be a problem to say that pg_create_subscription was enough to create a subscription today, and then later require additional privileges (e.g. pg_create_connection). If that's not a problem, then this sounds fine with me. -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Non-superuser subscription owners
On Wed, Jan 25, 2023 at 10:45 PM Jeff Davis wrote: > I propose that we have two predefined roles: pg_create_subscription, > and pg_create_connection. If creating a subscription with a connection > string, you'd need to be a member of both roles. But to create a > subscription with a server object, you'd just need to be a member of > pg_create_subscription and have the USAGE privilege on the server > object. I have no issue with that as a long-term plan. However, I think that for right now we should just introduce pg_create_subscription. It would make sense to add pg_create_connection in the same patch that adds a CREATE CONNECTION command (or whatever exact syntax we end up with) -- and that patch can also change CREATE SUBSCRIPTION to require both privileges where a connection string is specified directly. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Non-superuser subscription owners
On Tue, 2023-01-24 at 17:00 -0500, Robert Haas wrote: > It seems to me that the relevant > question isn't "are the servers tightly coupled?" but rather "could > some user make a mess if we let them use any arbitrary connection > string?". The split I created is much easier for an admin to answer: is the list of servers finite, or can users connect to new servers the admin isn't even aware of? If it's a finite list, I feel there's a much better solution with both security and UI benefits. With your question, I'm not entirely clear if that's a question that we already have an answer for (require a password parameter), or that we will answer in this thread, or that the admin will answer. > unless you've got users who are really shady Or compromised. Unfortunately, a role that's creating subscriptions has a lot of surface area for escalation-of-privilege attacks, because they have to trust all the owners of all the tables the subscriptions write to. > I think that you're basically trying to make an argument that some > sort of complex outbound connection filtering is mandatory No, I'm not asking for the validation to be more complex. I believe use case (A) is a substantial use case, and I'd like to leave space in the user interface to solve it a much better way than connection string validation can offer. But to solve use case (A), we need to separate the ability to create a subscription from the ability to create a connection string. Right now you see those as the same because they are done at the same time in the same command; but I don't see it that way, because I had plans to allow a variant of CREATE SUBSCRIPTION that uses foreign servers. That plan would be consistent with dblink and postgres_fdw, which already allow specifying foreign servers. I propose that we have two predefined roles: pg_create_subscription, and pg_create_connection. If creating a subscription with a connection string, you'd need to be a member of both roles. But to create a subscription with a server object, you'd just need to be a member of pg_create_subscription and have the USAGE privilege on the server object. -- Jeff Davis PostgreSQL Contributor Team - AWS
