Re: Special role for subscriptions

2019-03-26 Thread Robert Haas
On Thu, Mar 21, 2019 at 9:28 PM Michael Paquier  wrote:
> By the way, as the commit fest is coming to its end in a couple of
> days, and that we are still discussing how the thing should be shaped,
> I would recommend to mark the patch as returned with feedback.  Any
> objections with that?

+1.  It doesn't even seem that we agree on how it should be designed,
still less the implementation.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: Special role for subscriptions

2019-03-25 Thread Michael Paquier
On Sat, Mar 23, 2019 at 10:52:52AM -0400, Stephen Frost wrote:
> * Petr Jelinek (petr.jeli...@2ndquadrant.com) wrote:
>> I agree the feature is important, it just does not seem like the patch
>> is RFC and given security implications I err on the side of safety here.
> 
> Agreed.

Based on the latest exchanges, I am marking the patch as returned with
feedback.
--
Michael


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-23 Thread Stephen Frost
Greetings,

* Petr Jelinek (petr.jeli...@2ndquadrant.com) wrote:
> On 23/03/2019 02:38, Michael Paquier wrote:
> > On Fri, Mar 22, 2019 at 08:41:06PM +0800, Andrey Borodin wrote:
> >> 22 марта 2019 г., в 19:17, Petr Jelinek  
> >> написал(а):
> >>> I still don't like that we are running the subscription workers as
> >>> superuser even for subscriptions created by regular user. That has
> >>> plenty of privilege escalation issues in terms of how user functions are
> >>> run (we execute triggers, index expressions etc, in that worker).
> >>
> >> Yes, this is important concern, thanks! I think it is not a big deal
> >> to run worker without superuser privileges too.
> 
> Yes we should run without superuser privileges but perhaps more
> importantly we need to so me kind of security checks on tables while
> applying - the fact that the user had access to a table when
> subscription was created does not mean it will have it in 5 minutes and
> given our low level API usage in the worker, there is currently no check
> for that.

Agreed, and that's exactly the same as what I was telling Andrey at
PGConf APAC when he and I were discussing the subscription role.  The
specific suggestion that I had was to check for every transaction,
though that was a pretty off-the-cuff idea and someone might have a
better one, certainly.

> > FWIW, the argument from Petr is very scary.  So please let me think
> > that it is a pretty big deal.
> > 
> >> Yes, this patch is a pure security implication and nothing else.
> > 
> > And this is especially *why* it needs careful screening.
> 
> Yep that was exactly my point.
> 
> I agree the feature is important, it just does not seem like the patch
> is RFC and given security implications I err on the side of safety here.

Agreed.

Thanks!

Stephen


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-23 Thread Petr Jelinek
On 23/03/2019 02:38, Michael Paquier wrote:
> On Fri, Mar 22, 2019 at 08:41:06PM +0800, Andrey Borodin wrote:
>> 22 марта 2019 г., в 19:17, Petr Jelinek  
>> написал(а):
>>> I still don't like that we are running the subscription workers as
>>> superuser even for subscriptions created by regular user. That has
>>> plenty of privilege escalation issues in terms of how user functions are
>>> run (we execute triggers, index expressions etc, in that worker).
>>
>> Yes, this is important concern, thanks! I think it is not a big deal
>> to run worker without superuser privileges too.
> 

Yes we should run without superuser privileges but perhaps more
importantly we need to so me kind of security checks on tables while
applying - the fact that the user had access to a table when
subscription was created does not mean it will have it in 5 minutes and
given our low level API usage in the worker, there is currently no check
for that.

See the 0004 patch in
https://www.postgresql.org/message-id/0b477a34-01c5-ad97-b408-79f4e0e64...@2ndquadrant.com
.

> FWIW, the argument from Petr is very scary.  So please let me think
> that it is a pretty big deal.
> 
>> Yes, this patch is a pure security implication and nothing else.
> 
> And this is especially *why* it needs careful screening.
> 

Yep that was exactly my point.

I agree the feature is important, it just does not seem like the patch
is RFC and given security implications I err on the side of safety here.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services



Re: Special role for subscriptions

2019-03-22 Thread Michael Paquier
On Fri, Mar 22, 2019 at 08:41:06PM +0800, Andrey Borodin wrote:
> 22 марта 2019 г., в 19:17, Petr Jelinek  
> написал(а):
>> I still don't like that we are running the subscription workers as
>> superuser even for subscriptions created by regular user. That has
>> plenty of privilege escalation issues in terms of how user functions are
>> run (we execute triggers, index expressions etc, in that worker).
>
> Yes, this is important concern, thanks! I think it is not a big deal
> to run worker without superuser privileges too.

FWIW, the argument from Petr is very scary.  So please let me think
that it is a pretty big deal.

> Yes, this patch is a pure security implication and nothing else.

And this is especially *why* it needs careful screening.

>> Independently from the willingness of any committer to work on this
>> at current CF, the topic of subscription security relaxation
>> really worth efforts. 

Perhaps, still it seems that we are still discussing about the concept
and that we have no clear agreement on what to do.  This is not a good
sign 8 days before the end of the last commit fest.
--
Michael


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-22 Thread Andrey Borodin
Hi!


> 22 марта 2019 г., в 19:17, Petr Jelinek  
> написал(а):
> 
> I still don't like that we are running the subscription workers as
> superuser even for subscriptions created by regular user. That has
> plenty of privilege escalation issues in terms of how user functions are
> run (we execute triggers, index expressions etc, in that worker).
Yes, this is important concern, thanks! I think it is not a big deal to run 
worker without superuser privileges too.

> Regardless of my complain above, patch with this big security
> implications that has arrived in middle of last CF should not be merged
> in that last CF IMHO.
Yes, this patch is a pure security implication and nothing else.
This thread was started in November with around twenty messages before this CF. 
Our wiki states that "in our community -- if no one objects, then there is 
implicit approval. Within reason!"
I do not really think argument "last version of the patch arrived at last CF" 
applies here. But I understand that it is not easy to setup consensus on the 
problem at hand.
Independently from the willingness of any committer to work on this at current 
CF, the topic of subscription security relaxation really worth efforts.


Best regards, Andrey Borodin.


Re: Special role for subscriptions

2019-03-22 Thread Petr Jelinek
Hi,

On 22/03/2019 03:15, Andrey Borodin wrote:
> 
>> 22 марта 2019 г., в 9:28, Michael Paquier  написал(а):
>>
>> On Thu, Mar 21, 2019 at 10:06:03AM -0300, Euler Taveira wrote:
>>> It will be really strange but I can live with that. Another idea is
>>> CREATE bit to create subscriptions (without replicate) and SUBSCRIBE
>>> bit to replicate tables. It is not just a privilege to create a
>>> subscription but also to modify tables that a role doesn't have
>>> explicit permission. Let's allocate another AclItem?
>>
>> By the way, as the commit fest is coming to its end in a couple of
>> days, and that we are still discussing how the thing should be shaped,
>> I would recommend to mark the patch as returned with feedback.  Any
>> objections with that?
> 
> It seems to me that we have consensus that:
> 1. We need special role to create subscription
> 2. This role can create subscription with some security checks
> 3. We have complete list of possible security checks
> 4. We have code that implements most of these checks (I believe 
> pg_subscription_role_v2.patch is enough, but we can tighten checks a little 
> more)
> 

I still don't like that we are running the subscription workers as
superuser even for subscriptions created by regular user. That has
plenty of privilege escalation issues in terms of how user functions are
run (we execute triggers, index expressions etc, in that worker).

> Do we have any objection on these points?
> 
> If not, it is RFC, it should not be returned.
> 

Regardless of my complain above, patch with this big security
implications that has arrived in middle of last CF should not be merged
in that last CF IMHO.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services



Re: Special role for subscriptions

2019-03-22 Thread Evgeniy Efimkin
Hi!
> These are basically that the truncate, insert, delete and insert
> rights for the role creating the subscription. Why would we actually
> need that?
It's for security reasons. Because possible to attack target server. If 
publication have system tables for instance pg_authid

> pg_subscription_users and these should be able to dump subscriptions,
> so you have at least one problem.
But in system_views.sql we give grant on subconninfo column and pg_dump 
required superuser privilege only for postgesql under 12 version. Old version 
pg_dump still works but require superuser for dump subscription.

 
Efimkin Evgeny




Re: Special role for subscriptions

2019-03-21 Thread Michael Paquier
On Fri, Mar 22, 2019 at 10:15:59AM +0800, Andrey Borodin wrote:
> It seems to me that we have consensus that:
> 1. We need special role to create subscription
> 2. This role can create subscription with some security checks
> 3. We have complete list of possible security checks

These are basically that the truncate, insert, delete and insert
rights for the role creating the subscription.  Why would we actually
need that?

> 4. We have code that implements most of these checks (I believe
> pg_subscription_role_v2.patch is enough, but we can tighten checks a
> little more)

If a unique system role is the conclusion on the matter, it looks so.

> If not, it is RFC, it should not be returned.

The patch still needs some work before being RFC.  From what I can
read, pg_dump still ignores roles which are members of the system role
pg_subscription_users and these should be able to dump subscriptions,
so you have at least one problem.
--
Michael


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-21 Thread Andrey Borodin



> 22 марта 2019 г., в 9:28, Michael Paquier  написал(а):
> 
> On Thu, Mar 21, 2019 at 10:06:03AM -0300, Euler Taveira wrote:
>> It will be really strange but I can live with that. Another idea is
>> CREATE bit to create subscriptions (without replicate) and SUBSCRIBE
>> bit to replicate tables. It is not just a privilege to create a
>> subscription but also to modify tables that a role doesn't have
>> explicit permission. Let's allocate another AclItem?
> 
> By the way, as the commit fest is coming to its end in a couple of
> days, and that we are still discussing how the thing should be shaped,
> I would recommend to mark the patch as returned with feedback.  Any
> objections with that?

It seems to me that we have consensus that:
1. We need special role to create subscription
2. This role can create subscription with some security checks
3. We have complete list of possible security checks
4. We have code that implements most of these checks (I believe 
pg_subscription_role_v2.patch is enough, but we can tighten checks a little 
more)

Do we have any objection on these points?

If not, it is RFC, it should not be returned.

Best regards, Andrey Borodin.


Re: Special role for subscriptions

2019-03-21 Thread Michael Paquier
On Thu, Mar 21, 2019 at 10:06:03AM -0300, Euler Taveira wrote:
> It will be really strange but I can live with that. Another idea is
> CREATE bit to create subscriptions (without replicate) and SUBSCRIBE
> bit to replicate tables. It is not just a privilege to create a
> subscription but also to modify tables that a role doesn't have
> explicit permission. Let's allocate another AclItem?

By the way, as the commit fest is coming to its end in a couple of
days, and that we are still discussing how the thing should be shaped,
I would recommend to mark the patch as returned with feedback.  Any
objections with that?
--
Michael


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-21 Thread Euler Taveira
Em qua, 20 de mar de 2019 às 21:57, Michael Paquier
 escreveu:
>
> Perhaps we would want something at database level different from GRANT
> CREATE ON DATABASE, but only for subscriptions?  This way, it is
> possible to have per-database groups having the right to create
> subscriptions, and I'd like to think that we should not include
> subcription creation into the existing CREATE rights.  It would be
> kind of funny to not have CREATE include the creation of this specific
> object though :)
>
It will be really strange but I can live with that. Another idea is
CREATE bit to create subscriptions (without replicate) and SUBSCRIBE
bit to replicate tables. It is not just a privilege to create a
subscription but also to modify tables that a role doesn't have
explicit permission. Let's allocate another AclItem?


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento



Re: Special role for subscriptions

2019-03-21 Thread Evgeniy Efimkin
Hi!

> Perhaps we would want something at database level different from GRANT
> CREATE ON DATABASE, but only for subscriptions?
How about 4 checks to create subscription for nonsuperuser?
1. Special role for create subscription 
2. CREATE ON DATABASE privilege 
3. INSERT, UPDATE, DELETE, TRUNCATE, REFERENCE privilege on target table
4. target table not in information_schema and pg_catalog

 
Efimkin Evgeny




Re: Special role for subscriptions

2019-03-21 Thread Evgeniy Efimkin
Hi!
> - If the user's permissions are later revoked, the subscription is unaffected.
Now it work the same, if we revoke superuser, subscription is unaffected and 
replication still work

Don't check grants in target database is very dangerous, i create publication 
with system tables(it's not difficult)

select * from pg_publication_tables ;
 pubname | schemaname | tablename
-++
 pub | pg_catalog | pg_authid
(1 row)

After that i create subscription, in log i see that
2019-03-21 11:19:50.863 MSK [58599] LOG:  logical replication table 
synchronization worker for subscription "sub_nosuper", table "pg_authid" has 
started
2019-03-21 11:19:51.039 MSK [58599] ERROR:  null value in column "oid" violates 
not-null constraint
2019-03-21 11:19:51.039 MSK [58599] DETAIL:  Failing row contains (null, 
pg_monitor, f, t, f, f, f, f, f, -1, null, null).
2019-03-21 11:19:51.039 MSK [58599] CONTEXT:  COPY pg_authid, line 1: 
"pg_monitor   f   t   f   f   f   f   f   -1  \N  \N"

I think it's no problem use it to attack target server after some hack on 
publication side.

 
Efimkin Evgeny




Re: Special role for subscriptions

2019-03-20 Thread Andrey Borodin



> 21 марта 2019 г., в 8:56, Michael Paquier  написал(а):
> 
> On Wed, Mar 20, 2019 at 11:58:04PM +0800, Andrey Borodin wrote:
>>> 20 марта 2019 г., в 21:46, Robert Haas  написал(а):
>>> I think we should view this permission as "you can create
>>> subscriptions, plain and simple".
>> 
>> That sounds good.
>> From my POV, the purpose of the patch is to allow users to transfer
>> their database via logical replication. Without superuser privileges
>> (e.g. to the managed cloud with vanilla postgres).
> 
> A system role to be able to create subscriptions is perhaps a too big
> hammer as that would apply to all databases of a system, still we may
> be able to live with that.
> 
> Perhaps we would want something at database level different from GRANT
> CREATE ON DATABASE, but only for subscriptions?  This way, it is
> possible to have per-database groups having the right to create
> subscriptions, and I'd like to think that we should not include
> subcription creation into the existing CREATE rights.  It would be
> kind of funny to not have CREATE include the creation of this specific
> object though :)

I think that small granularity can lead to unnecessary multiplication of 
subscription. User need to have sufficient minimum number of subscriptions, 
like they have 1 incoming WAL.
If we have per-database permission management, user will decide that it is a 
good thing to divide one subscription to per-database subscriptions.

Best regards, Andrey Borodin.


Re: Special role for subscriptions

2019-03-20 Thread Michael Paquier
On Wed, Mar 20, 2019 at 11:58:04PM +0800, Andrey Borodin wrote:
>> 20 марта 2019 г., в 21:46, Robert Haas  написал(а):
>> I think we should view this permission as "you can create
>> subscriptions, plain and simple".
> 
> That sounds good.
> From my POV, the purpose of the patch is to allow users to transfer
> their database via logical replication. Without superuser privileges
> (e.g. to the managed cloud with vanilla postgres).

A system role to be able to create subscriptions is perhaps a too big
hammer as that would apply to all databases of a system, still we may
be able to live with that.

Perhaps we would want something at database level different from GRANT
CREATE ON DATABASE, but only for subscriptions?  This way, it is
possible to have per-database groups having the right to create
subscriptions, and I'd like to think that we should not include
subcription creation into the existing CREATE rights.  It would be
kind of funny to not have CREATE include the creation of this specific
object though :)
--
Michael


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-20 Thread Andrey Borodin



> 20 марта 2019 г., в 21:46, Robert Haas  написал(а):
> 
> On Wed, Mar 20, 2019 at 5:39 AM Evgeniy Efimkin  
> wrote:
>> Hi!
>>> Currently, user with pg_subscription_users can create subscription into any 
>>> system table, can't they?
>>> We certainly need to change it to more secure way.
>> No, you can't add system tables to publication. In new patch i add 
>> privileges checks on target table, non superuser can't create/refresh 
>> subscription if he don't have INSERT, UPDATE, DELETE and TRUNCATE privileges.
> 
> 
> 
> I think we should view this permission as "you can create
> subscriptions, plain and simple".

That sounds good.
From my POV, the purpose of the patch is to allow users to transfer their 
database via logical replication. Without superuser privileges (e.g. to the 
managed cloud with vanilla postgres).

But the role effectively allows inserts to any table, this can be escalated to 
superuser. What is the best way to deal with it?

Best regards, Andrey Borodin.


Re: Special role for subscriptions

2019-03-20 Thread Robert Haas
On Wed, Mar 20, 2019 at 5:39 AM Evgeniy Efimkin  wrote:
> Hi!
> > Currently, user with pg_subscription_users can create subscription into any 
> > system table, can't they?
> > We certainly need to change it to more secure way.
> No, you can't add system tables to publication. In new patch i add privileges 
> checks on target table, non superuser can't create/refresh subscription if he 
> don't have INSERT, UPDATE, DELETE and TRUNCATE privileges.

I don't that's the right approach.  That idea kinda makes sense if you
think about it as giving permission to publish tables to which they
have rights, but that doesn't seem like the right mental model to me.
It seems more likely that there is a person whose job it is to set up
replication but who doesn't normally interact with the table data
itself.  In that kind of case, you just want to give the person
permission to create subscriptions, without needing to also give them
lots of privileges on individual tables (and maybe having whatever
they are trying to do fail if you miss a table someplace).

But there are some other things that are strange about this too:

- If the user's permissions are later revoked, the subscription is unaffected.

- If the user creates a subscription that targets a publication which
only includes a subset of the insert, update, delete, and truncate
operations, they still need all of those permissions on their local
table.

- We don't typically have operations that require a whole bundle of
privileges on the local table -- sometimes you check that you have A
on X and B on Y, like for REFERENCES, but needing both A and B on X is
somewhat unusual.

I think we should view this permission as "you can create
subscriptions, plain and simple".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: Special role for subscriptions

2019-03-20 Thread Evgeniy Efimkin


Hi!
> Currently, user with pg_subscription_users can create subscription into any 
> system table, can't they?
> We certainly need to change it to more secure way.
No, you can't add system tables to publication. In new patch i add privileges 
checks on target table, non superuser can't create/refresh subscription if he 
don't have INSERT, UPDATE, DELETE and TRUNCATE privileges.


 
Efimkin Evgeny
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index 3f2f674a1a..c2c7241084 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -201,9 +201,10 @@
 
   
Subscriptions are dumped by pg_dump if the current user
-   is a superuser.  Otherwise a warning is written and subscriptions are
-   skipped, because non-superusers cannot read all subscription information
-   from the pg_subscription catalog.
+   is a superuser or member of role pg_subscription_users, other users should
+   use no-subscriptions because non-superusers cannot read
+   all subscription information from the pg_subscription
+   catalog.
   
 
   
@@ -522,7 +523,10 @@
   
 
   
-   To create a subscription, the user must be a superuser.
+   To create a subscription, the user must be a superuser or be member of role
+   pg_subscription_users and have INSERT ,
+   UPDATE,  DELETE, TRUNCATE
+   privileges on target table.
   
 
   
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 6106244d32..9c23a6280d 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -556,6 +556,10 @@ DROP ROLE doomed_role;
pg_read_all_stats and
pg_stat_scan_tables.
   
+  
+   pg_subscription_users
+   Allow create/drop subscriptions and be owner of subscription. Read pg_subscription
+  
  
 

@@ -579,6 +583,12 @@ DROP ROLE doomed_role;
   other system information normally restricted to superusers.
   
 
+  
+  The pg_subscription_users role are intended to allow
+  administrators trusted, but non-superuser, which are able to create/drop subscription.
+  
+
+
   
   Care should be taken when granting these roles to ensure they are only used where
   needed and with the understanding that these roles grant access to privileged
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index afee2838cc..5483a65376 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -242,12 +242,16 @@ AddSubscriptionRelState(Oid subid, Oid relid, char state,
 		XLogRecPtr sublsn)
 {
 	Relation	rel;
+	Relation	target_rel;
 	HeapTuple	tup;
 	bool		nulls[Natts_pg_subscription_rel];
 	Datum		values[Natts_pg_subscription_rel];
+	AclResult	aclresult;
+	AclMode		aclmask;
 
 	LockSharedObject(SubscriptionRelationId, subid, 0, AccessShareLock);
 
+
 	rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);
 
 	/* Try finding existing mapping. */
@@ -258,6 +262,14 @@ AddSubscriptionRelState(Oid subid, Oid relid, char state,
 		elog(ERROR, "subscription table %u in subscription %u already exists",
 			 relid, subid);
 
+	/* Check permission on target table. */
+	aclmask = ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE;
+	target_rel = table_open(relid, NoLock);
+	aclresult = pg_class_aclcheck(RelationGetRelid(target_rel), GetUserId(), aclmask);
+	if (aclresult != ACLCHECK_OK)
+		aclcheck_error(aclresult, get_relkind_objtype(target_rel->rd_rel->relkind),
+			RelationGetRelationName(target_rel));
+
 	/* Form the tuple. */
 	memset(values, 0, sizeof(values));
 	memset(nulls, false, sizeof(nulls));
@@ -278,6 +290,7 @@ AddSubscriptionRelState(Oid subid, Oid relid, char state,
 
 	/* Cleanup. */
 	table_close(rel, NoLock);
+	table_close(target_rel, NoLock);
 }
 
 /*
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index d962648bc5..08e58295bd 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -939,9 +939,12 @@ REVOKE ALL ON pg_replication_origin_status FROM public;
 
 -- All columns of pg_subscription except subconninfo are readable.
 REVOKE ALL ON pg_subscription FROM public;
-GRANT SELECT (subdbid, subname, subowner, subenabled, subslotname, subpublications)
+GRANT SELECT (tableoid, oid, subdbid, subname, subowner, subenabled,
+	subslotname, subsynccommit, subpublications)
 ON pg_subscription TO public;
 
+GRANT SELECT (subconninfo)
+ON pg_subscription TO pg_subscription_users;
 
 --
 -- We have a few function definitions in here, too.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a60a15193a..e1555ed6eb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -26,6 +26,7 @@
 #include "catalog/namespace.h"
 #include "catalog/objectaccess.h"
 #include "catalog/objectaddress.h"
+#include "catalog/pg_authid.h"
 #include 

Re: Special role for subscriptions

2019-03-19 Thread Euler Taveira
Em qui, 14 de mar de 2019 às 00:03, Stephen Frost  escreveu:
>
> I view that as the first step towards building a more granular privilege
> system for subscription creation, and that was the second half of what I
> was trying to say before- I do think there's value in having something
> more granular than just "this role can create ANY subscription".  As an
> administrator, I might be fine with subscriptions to system X, but not
> to system Y, for example.  As long as we don't block off the ability to
> build something finer grained in the future, then having the system role
> to allow a given role to do create subscription seems fine to me.
>
Isn't that what HBA rules are for? I don't see a fine grain control if
there is no node concept. You need to name the remote replication
set(s) to locally control it. Postgres replication is distributed by
design (current node doesn't need to store info about all nodes --
just those it is connected to). Node is a centralizing concept (every
node has its peers info). Is it worth add complexity to logical
replication just to satisfy a fine grain control? In this case, node
concept should be adopted in a transparent manner (which means that
CREATE PUBLICATION/SUBSCRIPTION should create iif there is no NODE
specification) -- old syntax should work but we start to accept node
info in both sides.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento



Re: Special role for subscriptions

2019-03-19 Thread Andrey Borodin
Hi!

> 13 марта 2019 г., в 22:55, Evgeniy Efimkin  
> написал(а):
> 
> I've prepare patch with new system role, i'm not sure about name, called it 
> "pg_subscription_users".
> In that patch we don't check permissions on target tables, i don't know, 
> should we check it?

Currently, user with pg_subscription_users can create subscription into any 
system table, can't they?
We certainly need to change it to more secure way.

Best regards, Andrey Borodin.


Re: Special role for subscriptions

2019-03-14 Thread Andrey Borodin



> 14 марта 2019 г., в 12:56, Evgeniy Efimkin  
> написал(а):
> 
> Hi!
>> I view that as the first step towards building a more granular privilege
>> system for subscription creation, and that was the second half of what I
>> was trying to say before- I do think there's value in having something
>> more granular than just "this role can create ANY subscription". As an
>> administrator, I might be fine with subscriptions to system X, but not
>> to system Y, for example. As long as we don't block off the ability to
>> build something finer grained in the future, then having the system role
>> to allow a given role to do create subscription seems fine to me.
> Do you mean something like `CREATE SERVER` with privileges for each server, 
> which using in CREATE SUBSCRIPTION, very similar way used in foreign data 
> wrapper?
> 

Let's summarize.
To create a subscription into table X user must:
1. be a superuser
2. Or (have role pg_subscription_users 
3. and be allowed to write into the table X)


4. Condition 3 can be replaced\extended by "be owner of a the table X".
5. Condition 2 can be replaced\extended by "have privileges for some server 
remote".

Which combination of authorization rules do we want?

IMHO 1,2,4 is sufficient.

Best regards, Andrey Borodin.


Re: Special role for subscriptions

2019-03-14 Thread Evgeniy Efimkin
Hi!
> I view that as the first step towards building a more granular privilege
> system for subscription creation, and that was the second half of what I
> was trying to say before- I do think there's value in having something
> more granular than just "this role can create ANY subscription". As an
> administrator, I might be fine with subscriptions to system X, but not
> to system Y, for example. As long as we don't block off the ability to
> build something finer grained in the future, then having the system role
> to allow a given role to do create subscription seems fine to me.
Do you mean something like `CREATE SERVER` with privileges for each server, 
which using in CREATE SUBSCRIPTION, very similar way used in foreign data 
wrapper?

 
Efimkin Evgeny




Re: Special role for subscriptions

2019-03-13 Thread Kyotaro HORIGUCHI
At Wed, 13 Mar 2019 23:03:26 -0400, Stephen Frost  wrote in 
<20190314030326.gq6...@tamriel.snowman.net>
> Greetings,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Mon, Mar 11, 2019 at 10:39 PM Michael Paquier  
> > wrote:
> > > On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote:
> > > >  * Is the original idea of a special role still viable?
> > >
> > > In my opinion, that part may be valuable.  The latest patches proposed
> > > change the way tables are filtered and listed on the subscription
> > > side, lowering the permission to spawn a new thread and to connect to a
> > > publication server by just being a database owner instead of being a
> > > superuser, and that's quite a gap.
> > 
> > I agree.  I think the original idea was better than what Stephen
> > suggested, and for basically the reasons you mention.
> > 
> > However, I'm not sure that you are right when you say "just being a
> > database owner."  I think that what's being proposed is that anybody
> > who is a *table* owner could make PostgreSQL run off and try to sync
> > that table from a remote server in perpetuity.  That seems like WAY
> > too much access to give an unprivileged user.  I don't think we want
> > unprivileged users to be able to launch more or less permanent
> > background processes, nor do we want them to be able to initiate
> > outbound network traffic from the server.  Whether we want database
> > owners to be able to do those things is more debatable, but even that
> > would represent a significant expansion of their current rights, IIUC.
> > 
> > Just letting the superuser decide who gets to create subscriptions
> > seems good enough from here.
> 
> It seems I wasn't very clear earlier in the thread- I *definitely* think
> we need to have a special role which a superuser can grant to certain
> roles (possibly with the permission to grant further) to allow them to
> create subscriptions, and that's definitely distinct from "database
> owner" and shouldn't be combined with that.
> 
> I view that as the first step towards building a more granular privilege
> system for subscription creation, and that was the second half of what I
> was trying to say before- I do think there's value in having something
> more granular than just "this role can create ANY subscription".  As an
> administrator, I might be fine with subscriptions to system X, but not
> to system Y, for example.  As long as we don't block off the ability to
> build something finer grained in the future, then having the system role
> to allow a given role to do create subscription seems fine to me.

The subscription privileges is completely reasonable, but I've
heard of users who want to restrict tables on which a role can
make subscription. Subscription causes INSERT/UPDATE/DELETE to a
table, is it too-much to check the privileges addition to the
subscription privileges?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




Re: Special role for subscriptions

2019-03-13 Thread Stephen Frost
Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Mar 11, 2019 at 10:39 PM Michael Paquier  wrote:
> > On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote:
> > >  * Is the original idea of a special role still viable?
> >
> > In my opinion, that part may be valuable.  The latest patches proposed
> > change the way tables are filtered and listed on the subscription
> > side, lowering the permission to spawn a new thread and to connect to a
> > publication server by just being a database owner instead of being a
> > superuser, and that's quite a gap.
> 
> I agree.  I think the original idea was better than what Stephen
> suggested, and for basically the reasons you mention.
> 
> However, I'm not sure that you are right when you say "just being a
> database owner."  I think that what's being proposed is that anybody
> who is a *table* owner could make PostgreSQL run off and try to sync
> that table from a remote server in perpetuity.  That seems like WAY
> too much access to give an unprivileged user.  I don't think we want
> unprivileged users to be able to launch more or less permanent
> background processes, nor do we want them to be able to initiate
> outbound network traffic from the server.  Whether we want database
> owners to be able to do those things is more debatable, but even that
> would represent a significant expansion of their current rights, IIUC.
> 
> Just letting the superuser decide who gets to create subscriptions
> seems good enough from here.

It seems I wasn't very clear earlier in the thread- I *definitely* think
we need to have a special role which a superuser can grant to certain
roles (possibly with the permission to grant further) to allow them to
create subscriptions, and that's definitely distinct from "database
owner" and shouldn't be combined with that.

I view that as the first step towards building a more granular privilege
system for subscription creation, and that was the second half of what I
was trying to say before- I do think there's value in having something
more granular than just "this role can create ANY subscription".  As an
administrator, I might be fine with subscriptions to system X, but not
to system Y, for example.  As long as we don't block off the ability to
build something finer grained in the future, then having the system role
to allow a given role to do create subscription seems fine to me.

Thanks!

Stephen


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-13 Thread Evgeniy Efimkin
Hi!
Thanks for comments!
> Just letting the superuser decide who gets to create subscriptions
> seems good enough from here.
I've prepare patch with new system role, i'm not sure about name, called it 
"pg_subscription_users".
In that patch we don't check permissions on target tables, i don't know, should 
we check it?
 
Efimkin Evgeny
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index 3f2f674a1a..0f7f60fcd0 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -201,9 +201,10 @@
 
   
Subscriptions are dumped by pg_dump if the current user
-   is a superuser.  Otherwise a warning is written and subscriptions are
-   skipped, because non-superusers cannot read all subscription information
-   from the pg_subscription catalog.
+   is a superuser or member of role pg_subscription_users, other users should
+   use no-subscriptions because non-superusers cannot read
+   all subscription information from the pg_subscription
+   catalog.
   
 
   
@@ -522,7 +523,8 @@
   
 
   
-   To create a subscription, the user must be a superuser.
+   To create a subscription, the user must be a superuser or be member of role
+   pg_subscription_users.
   
 
   
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 6106244d32..9c23a6280d 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -556,6 +556,10 @@ DROP ROLE doomed_role;
pg_read_all_stats and
pg_stat_scan_tables.
   
+  
+   pg_subscription_users
+   Allow create/drop subscriptions and be owner of subscription. Read pg_subscription
+  
  
 

@@ -579,6 +583,12 @@ DROP ROLE doomed_role;
   other system information normally restricted to superusers.
   
 
+  
+  The pg_subscription_users role are intended to allow
+  administrators trusted, but non-superuser, which are able to create/drop subscription.
+  
+
+
   
   Care should be taken when granting these roles to ensure they are only used where
   needed and with the understanding that these roles grant access to privileged
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 7723f01327..56bf04631e 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -939,9 +939,12 @@ REVOKE ALL ON pg_replication_origin_status FROM public;
 
 -- All columns of pg_subscription except subconninfo are readable.
 REVOKE ALL ON pg_subscription FROM public;
-GRANT SELECT (subdbid, subname, subowner, subenabled, subslotname, subpublications)
+GRANT SELECT (tableoid, oid, subdbid, subname, subowner, subenabled,
+	subslotname, subsynccommit, subpublications)
 ON pg_subscription TO public;
 
+GRANT SELECT (subconninfo)
+ON pg_subscription TO pg_subscription_users;
 
 --
 -- We have a few function definitions in here, too.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a60a15193a..e1555ed6eb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -26,6 +26,7 @@
 #include "catalog/namespace.h"
 #include "catalog/objectaccess.h"
 #include "catalog/objectaddress.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
 #include "catalog/pg_subscription.h"
 #include "catalog/pg_subscription_rel.h"
@@ -342,10 +343,10 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 	if (create_slot)
 		PreventInTransactionBlock(isTopLevel, "CREATE SUBSCRIPTION ... WITH (create_slot = true)");
 
-	if (!superuser())
+	if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_SUBSCRIPTION_USERS))
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser to create subscriptions";
+ (errmsg("must be member of role \"pg_subscription_users\" to create subscriptions";
 
 	rel = table_open(SubscriptionRelationId, RowExclusiveLock);
 
@@ -1035,12 +1036,12 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 	   NameStr(form->subname));
 
 	/* New owner must be a superuser */
-	if (!superuser_arg(newOwnerId))
+	if (!is_member_of_role(newOwnerId, DEFAULT_ROLE_SUBSCRIPTION_USERS))
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  errmsg("permission denied to change owner of subscription \"%s\"",
 		NameStr(form->subname)),
- errhint("The owner of a subscription must be a superuser.")));
+ errhint("The owner of a subscription must be member of role \"pg_subscription_users\".")));
 
 	form->subowner = newOwnerId;
 	CatalogTupleUpdate(rel, >t_self, tup);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 4c98ae4d7f..8751801b89 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4111,7 +4111,7 @@ getSubscriptions(Archive *fout)
 	if (dopt->no_subscriptions || fout->remoteVersion < 10)
 		return;
 
-	if 

Re: Special role for subscriptions

2019-03-13 Thread Robert Haas
On Mon, Mar 11, 2019 at 10:39 PM Michael Paquier  wrote:
> On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote:
> >  * Is the original idea of a special role still viable?
>
> In my opinion, that part may be valuable.  The latest patches proposed
> change the way tables are filtered and listed on the subscription
> side, lowering the permission to spawn a new thread and to connect to a
> publication server by just being a database owner instead of being a
> superuser, and that's quite a gap.

I agree.  I think the original idea was better than what Stephen
suggested, and for basically the reasons you mention.

However, I'm not sure that you are right when you say "just being a
database owner."  I think that what's being proposed is that anybody
who is a *table* owner could make PostgreSQL run off and try to sync
that table from a remote server in perpetuity.  That seems like WAY
too much access to give an unprivileged user.  I don't think we want
unprivileged users to be able to launch more or less permanent
background processes, nor do we want them to be able to initiate
outbound network traffic from the server.  Whether we want database
owners to be able to do those things is more debatable, but even that
would represent a significant expansion of their current rights, IIUC.

Just letting the superuser decide who gets to create subscriptions
seems good enough from here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: Special role for subscriptions

2019-03-12 Thread Evgeniy Efimkin
Hi!
I think it's good idea to able create subscription for database owner, but 
owner do not have permission on all tables in database. 
At the begging Stephen Frost said about table filter at subscription side. 
https://www.postgresql.org/message-id/20181106215244.GH18594%40tamriel.snowman.net
I can add additional check in create subscription for database owner.
 
Efimkin Evgeny




Re: Special role for subscriptions

2019-03-12 Thread Evgeniy Efimkin
Hi!
>  * What are the most important use cases here? Are we just trying to
> avoid the unnecessary use of superuser, or is there a real use case for
> subscribing to a subset of a publication?

For instance in target database we do not have permission on some table used in 
publication,
but we still CREATE SUBSCRIPTION for owned tables.

>  * What are all the reasons CREATE SUBSCRIPTION currently requires
> superuser?
I'm not sure, but it seems like only superuser have rights on all tables. I 
can't find any restrictions.

>  * Is the original idea of a special role still viable?
yes, i wrote simple patch. Role create externally, but it can be system role. 
 
Efimkin Evgeny
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9021463a4c..31b5b9af8c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -322,6 +322,7 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
charoriginname[NAMEDATALEN];
boolcreate_slot;
List   *publications;
+   Oid role;

/*
 * Parse and check options.
@@ -341,11 +342,13 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 */
if (create_slot)
PreventInTransactionBlock(isTopLevel, "CREATE SUBSCRIPTION ... WITH (create_slot = true)");
-
-   if (!superuser())
+   role = get_role_oid("pg_subsciption_users", true);
+   if (!is_member_of_role(GetUserId(), role))
+   {
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-(errmsg("must be superuser to create subscriptions";
+(errmsg("must be pg_subsciption_users to create subscriptions";
+   }

rel = heap_open(SubscriptionRelationId, RowExclusiveLock);

@@ -1023,6 +1026,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 static void
 AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 {
+   Oid role;
Form_pg_subscription form;

form = (Form_pg_subscription) GETSTRUCT(tup);
@@ -1034,13 +1038,16 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION,
   NameStr(form->subname));

+   role = get_role_oid("pg_subsciption_users", true);
/* New owner must be a superuser */
-   if (!superuser_arg(newOwnerId))
+   if (!is_member_of_role(GetUserId(), role))
+   {
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 errmsg("permission denied to change owner of subscription \"%s\"",
NameStr(form->subname)),
-errhint("The owner of a subscription must be a superuser.")));
+errhint("The owner of a subscription must be a pg_subsciption_users.")));
+   }

form->subowner = newOwnerId;
CatalogTupleUpdate(rel, >t_self, tup);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 38ae1b9ab8..fa5d343993 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -754,6 +754,8 @@ copy_table(Relation rel)
CopyState   cstate;
List   *attnamelist;
ParseState *pstate;
+   AclResult   aclresult;
+   AclMode aclmask;

/* Get the publisher relation info. */
fetch_remote_table_info(get_namespace_name(RelationGetNamespace(rel)),
@@ -770,6 +772,13 @@ copy_table(Relation rel)
initStringInfo();
appendStringInfo(, "COPY %s TO STDOUT",
 quote_qualified_identifier(lrel.nspname, lrel.relname));
+   aclmask = ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE;
+   aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
+ aclmask);
+   if (aclresult != ACLCHECK_OK)
+   aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
+  RelationGetRelationName(rel));
+
res = walrcv_exec(wrconn, cmd.data, 0, NULL);
pfree(cmd.data);
if (res->status != WALRCV_OK_COPY_OUT)


Re: Special role for subscriptions

2019-03-11 Thread Michael Paquier
On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote:
>  * Is the original idea of a special role still viable?

In my opinion, that part may be valuable.  The latest patches proposed
change the way tables are filtered and listed on the subscription
side, lowering the permission to spawn a new thread and to connect to a 
publication server by just being a database owner instead of being a
superuser, and that's quite a gap.
--
Michael


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2019-03-11 Thread Jeff Davis
On Fri, 2018-11-09 at 15:24 +0300, Evgeniy Efimkin wrote:
> Hi!
> In order to support create subscription from non-superuser, we need
> to make it possible to choose tables on the subscriber side:

I'd like to know more about the reasoning here. This thread started out
by suggesting a new special role that can be used for subscriptions,
but now it's changing the way subscription happens.

 * What are the most important use cases here? Are we just trying to
avoid the unnecessary use of superuser, or is there a real use case for
subscribing to a subset of a publication?
 * What are all the reasons CREATE SUBSCRIPTION currently requires
superuser?
 * Is the original idea of a special role still viable?

Regards,
Jeff Davis





Re: [WIP] Special role for subscriptions

2018-11-14 Thread Evgeniy Efimkin
Hello!
I started work on patch (draft attached). Draft has changes related only to 
`CREATE SUBSCRIPTION`. 
I also introduce a new status (DEFFERED)  for tables in `FOR TABLE` clause (but 
not in publication).
New column in pg_subscription (suballtables) will be used in `REFRESH` clause

09.11.2018, 15:24, "Evgeniy Efimkin" :
> Hi!
> In order to support create subscription from non-superuser, we need to make 
> it possible to choose tables on the subscriber side:
> 1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
>    ```
> CREATE SUBSCRIPTION subscription_name
> CONNECTION 'conninfo'
> PUBLICATION publication_name [, ...]
> [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
> [ WITH ( subscription_parameter [= value] [, ... ] ) ]
>    ```
>    ... where `FOR ALL TABLES` is only allowed for superuser.
>    and table list in `FOR TABLES` clause will be stored in 
> pg_subscription_rel table (maybe another place?)
>
> 2. Each subscription should have "all tables" attribute.
>    For example via a new column in pg_subscription "suballtables".
>
> 3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
>    ```
> ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name 
> [WITH copy_data];
> ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
>    ```
> 4. On `ALTER SUBSCRIPTION  REFRESH PUBLICATION` should check if 
> table owner equals subscription owner. The check is ommited if subscription 
> owner is superuser.
> 5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on 
> subscription with table list and non-superuser owner, we should filter tables 
> which owner is not subscription's owner or maybe we need to raise error?
>
> What do you think about it? Any objections?
>
> 07.11.2018, 00:52, "Stephen Frost" :
>>  Greetings,
>>
>>  * Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
>>>   As a first step I suggest we allow CREATE SUBSCRIPTION for table owner 
>>> only.
>>
>>  That's a nice idea but seems like we would want to have a way to filter
>>  what tables a subscription follows then..? Just failing if the
>>  publication publishes tables that we don't have access to or are not the
>>  owner of seems like a poor solution..
>>
>>  Thanks!
>>
>>  Stephen
>
> 
> Ефимкин Евгений

 
Ефимкин Евгений
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f138e61a8d..5452bd6a55 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -29,6 +29,7 @@
 #include "catalog/pg_subscription.h"
 #include "catalog/pg_subscription_rel.h"
 
+#include "commands/dbcommands.h"
 #include "commands/defrem.h"
 #include "commands/event_trigger.h"
 #include "commands/subscriptioncmds.h"
@@ -321,6 +322,14 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 	char		originname[NAMEDATALEN];
 	bool		create_slot;
 	List	   *publications;
+	AclResult	aclresult;
+	bool 		alltables;
+
+	/* must have CREATE privilege on database */
+	aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(), ACL_CREATE);
+	if (aclresult != ACLCHECK_OK)
+		aclcheck_error(aclresult, OBJECT_DATABASE,
+	   get_database_name(MyDatabaseId));
 
 	/*
 	 * Parse and check options.
@@ -340,11 +349,13 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 	 */
 	if (create_slot)
 		PreventInTransactionBlock(isTopLevel, "CREATE SUBSCRIPTION ... WITH (create_slot = true)");
-
-	if (!superuser())
+	alltables = !stmt->tables || !stmt->for_all_tables;
+	/* FOR ALL TABLES requires superuser */
+	if (alltables && !superuser())
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser to create subscriptions";
+ (errmsg("must be superuser to create FOR ALL TABLES publication";
+
 
 	rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
 
@@ -384,6 +395,7 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 		DirectFunctionCall1(namein, CStringGetDatum(stmt->subname));
 	values[Anum_pg_subscription_subowner - 1] = ObjectIdGetDatum(owner);
 	values[Anum_pg_subscription_subenabled - 1] = BoolGetDatum(enabled);
+	values[Anum_pg_subscription_suballtables - 1] = BoolGetDatum(alltables);
 	values[Anum_pg_subscription_subconninfo - 1] =
 		CStringGetTextDatum(conninfo);
 	if (slotname)
@@ -407,6 +419,27 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 	snprintf(originname, sizeof(originname), "pg_%u", subid);
 	replorigin_create(originname);
 
+
+	if (stmt->tables&&!connect)
+	{
+		ListCell   *lc;
+		char		table_state;
+		foreach(lc, stmt->tables)
+		{
+			RangeVar   *rv = (RangeVar *) lfirst(lc);
+			Oid			relid;
+			relid = RangeVarGetRelid(rv, AccessShareLock, false);
+			/* must be owner */
+			if (!pg_class_ownercheck(relid, GetUserId()))
+	

Re: Special role for subscriptions

2018-11-09 Thread Evgeniy Efimkin
Hi!
In order to support create subscription from non-superuser, we need to make it 
possible to choose tables on the subscriber side:
1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
   ```
CREATE SUBSCRIPTION subscription_name
CONNECTION 'conninfo'
PUBLICATION publication_name [, ...]
[ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
[ WITH ( subscription_parameter [= value] [, ... ] ) ]
   ```
   ... where `FOR ALL TABLES` is only allowed for superuser.
   and table list in `FOR TABLES` clause will be stored in 
pg_subscription_rel table (maybe another place?)

2. Each subscription should have "all tables" attribute.
   For example via a new column in pg_subscription "suballtables".

3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
   ```
ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name 
[WITH copy_data];
ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
   ```
4. On `ALTER SUBSCRIPTION  REFRESH PUBLICATION` should check if table 
owner equals subscription owner. The check is ommited if subscription owner is 
superuser.
5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on 
subscription with table list and non-superuser owner, we should filter tables 
which owner is not subscription's owner or maybe we need to raise error?

What do you think about it? Any objections?

07.11.2018, 00:52, "Stephen Frost" :
> Greetings,
>
> * Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
>>  As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.
>
> That's a nice idea but seems like we would want to have a way to filter
> what tables a subscription follows then..? Just failing if the
> publication publishes tables that we don't have access to or are not the
> owner of seems like a poor solution..
>
> Thanks!
>
> Stephen

 
Ефимкин Евгений





Re: Special role for subscriptions

2018-11-08 Thread Evgeniy Efimkin
Hi!
I think we can add FOR TABLES clause for create/refresh subscription, for 
example: CREATE SUBSCRIPTION my_sub CONNECTION ... PUBLICATION my_pub [WITH 
...] [ FOR TABLES t1, t2 | ALL TABLES ]. ALL TABLES is avalibale only for 
superuser. FOR TABLES t1, t2 is available to owner of tables and superuser.

07.11.2018, 00:52, "Stephen Frost" :
> Greetings,
>
> * Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
>>  As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.
>
> That's a nice idea but seems like we would want to have a way to filter
> what tables a subscription follows then..? Just failing if the
> publication publishes tables that we don't have access to or are not the
> owner of seems like a poor solution..
>
> Thanks!
>
> Stephen

 
Ефимкин Евгений





Re: Special role for subscriptions

2018-11-06 Thread Stephen Frost
Greetings,

* Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
> As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

That's a nice idea but seems like we would want to have a way to filter
what tables a subscription follows then..?  Just failing if the
publication publishes tables that we don't have access to or are not the
owner of seems like a poor solution..

Thanks!

Stephen


signature.asc
Description: PGP signature


Re: Special role for subscriptions

2018-11-06 Thread Evgeniy Efimkin
Hi!
As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

03.11.2018, 19:20, "Stephen Frost" :
> Greetings,
>
> * Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
>>  In postgresql 10 and 11 only superuser can create/alter subscriptions.
>>  If there was a special role (like pg_monitor), it would be more easy to 
>> grant control on subscriptions.
>>  I can make a patch if there are no objections against it.
>
> I think the short answer is 'yes, we should let non-superusers do that',
> but the longer answer is:
>
> What level of access makes sense for managing subscriptions? Should
> there be a way to say "user X is allowed to create a subscription for
> remote system Y, but only for tables that exist in schema Q"?
>
> My general feeling is 'yes', though, of course, I don't want to say that
> we have to have all of that before we move forward with allowing
> non-superusers to create subscriptions, but I do think we want to make
> sure that we have a well thought-out path for how to get from where we
> are now to a system which has a lot more granularity, and to do our best
> to try avoiding any paths that might paint us into a corner.
>
> Thanks!
>
> Stephen





Re: Special role for subscriptions

2018-11-03 Thread Stephen Frost
Greetings,

* Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
> In postgresql 10 and 11 only superuser can create/alter subscriptions.
> If there was a special role (like pg_monitor), it would be more easy to grant 
> control on subscriptions.
> I can make a patch if there are no objections against it.

I think the short answer is 'yes, we should let non-superusers do that',
but the longer answer is:

What level of access makes sense for managing subscriptions?  Should
there be a way to say "user X is allowed to create a subscription for
remote system Y, but only for tables that exist in schema Q"?

My general feeling is 'yes', though, of course, I don't want to say that
we have to have all of that before we move forward with allowing
non-superusers to create subscriptions, but I do think we want to make
sure that we have a well thought-out path for how to get from where we
are now to a system which has a lot more granularity, and to do our best
to try avoiding any paths that might paint us into a corner.

Thanks!

Stephen


signature.asc
Description: PGP signature


Special role for subscriptions

2018-11-02 Thread Evgeniy Efimkin
Hi hackers!
In postgresql 10 and 11 only superuser can create/alter subscriptions.
If there was a special role (like pg_monitor), it would be more easy to grant 
control on subscriptions.
I can make a patch if there are no objections against it.