Re: [HACKERS] logical replication access control patches

2017-04-06 Thread Peter Eisentraut 
On 3/29/17 19:01, Petr Jelinek wrote:
>> So this CREATE SUBSCRIPTION priv actually gives you the power to cause
>> the system to open network connections to the outside world.  It's not
>> something you give freely to random strangers -- should be guarded
>> moderately tight, because it could be used as covert channel for data
>> leaking.  However, it's 1000x better than requiring superuser for
>> subscription creation, so +1 for the current approach.
>>
> Plus on the other hand you might want to allow somebody to stream data
> from another server but not necessarily allow said person to create new
> objects in the database which standard CREATE privilege would allow. So
> I think it makes sense to push this approach.

One new concern I just thought about is that having GRANT whatever ON
DATABASE would allow a database owner to assign these privileges, but a
database owner is not necessarily someone highly privileged to make this
decision.

So I'm prepared to set this patch to returned with feedback for now.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-29 Thread Petr Jelinek 
On 29/03/17 20:55, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 3/15/17 21:54, Peter Eisentraut wrote:
> 
>>> 0004 Add subscription apply worker privilege checks
>>> 0005 Add CREATE SUBSCRIPTION privilege on databases
>>
>> It would be nice to reach a conclusion on these (the second one
>> particularly), because otherwise we'll be stuck with only superusers
>> being allowed to create subscriptions.
> 
> I note that the CREATE privilege on databases, which previously only
> enabled schema creation, now also allows to create publications.  I
> wonder what is different about subscriptions that we need a separate
> CREATE SUBSCRIPTION privilege; could we allow the three things under the
> same privilege type?  (I suspect not; why give logical replication
> controls to users who in previous releases were only able to create
> schemas?)  If not, does it make sense to have one privilege for both new
> things, perhaps something like GRANT LOGICAL REPLICATION THINGIES?  If
> not, maybe we should have three separate priv bits: GRANT CREATE for
> schemas, GRANT CREATE PUBLICATION and GRANT CREATE SUBSCRIPTION?
> 
> 
> So this CREATE SUBSCRIPTION priv actually gives you the power to cause
> the system to open network connections to the outside world.  It's not
> something you give freely to random strangers -- should be guarded
> moderately tight, because it could be used as covert channel for data
> leaking.  However, it's 1000x better than requiring superuser for
> subscription creation, so +1 for the current approach.
> 

Plus on the other hand you might want to allow somebody to stream data
from another server but not necessarily allow said person to create new
objects in the database which standard CREATE privilege would allow. So
I think it makes sense to push this approach.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-29 Thread Alvaro Herrera 
Peter Eisentraut wrote:
> On 3/15/17 21:54, Peter Eisentraut wrote:

> > 0004 Add subscription apply worker privilege checks
> > 0005 Add CREATE SUBSCRIPTION privilege on databases
> 
> It would be nice to reach a conclusion on these (the second one
> particularly), because otherwise we'll be stuck with only superusers
> being allowed to create subscriptions.

I note that the CREATE privilege on databases, which previously only
enabled schema creation, now also allows to create publications.  I
wonder what is different about subscriptions that we need a separate
CREATE SUBSCRIPTION privilege; could we allow the three things under the
same privilege type?  (I suspect not; why give logical replication
controls to users who in previous releases were only able to create
schemas?)  If not, does it make sense to have one privilege for both new
things, perhaps something like GRANT LOGICAL REPLICATION THINGIES?  If
not, maybe we should have three separate priv bits: GRANT CREATE for
schemas, GRANT CREATE PUBLICATION and GRANT CREATE SUBSCRIPTION?


So this CREATE SUBSCRIPTION priv actually gives you the power to cause
the system to open network connections to the outside world.  It's not
something you give freely to random strangers -- should be guarded
moderately tight, because it could be used as covert channel for data
leaking.  However, it's 1000x better than requiring superuser for
subscription creation, so +1 for the current approach.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-29 Thread Peter Eisentraut 
On 3/15/17 21:54, Peter Eisentraut wrote:
> 0001 Refine rules for altering publication owner
> 0002 Change logical replication pg_hba.conf use

These two were committed.

> 0003 Add USAGE privilege for publications

I'm withdrawing this one for now, because of some issues that were
discussed in the thread.

> 0004 Add subscription apply worker privilege checks
> 0005 Add CREATE SUBSCRIPTION privilege on databases

It would be nice to reach a conclusion on these (the second one
particularly), because otherwise we'll be stuck with only superusers
being allowed to create subscriptions.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-22 Thread Peter Eisentraut 
On 3/22/17 08:12, Petr Jelinek wrote:
> On 22/03/17 03:38, Peter Eisentraut wrote:
>> On 3/20/17 15:10, Petr Jelinek wrote:
>>> Hmm but REPLICATION role can do basebackup/consume wal, so how does
>>> giving it limited publication access help? Wouldn't we need some
>>> SUBSCRIPTION role/grant used instead for logical replication connections
>>> instead of REPLICATION for this to make sense?
>>
>> Since we're splitting up the pg_hba.conf setup for logical and physical
>> connections, it would probably not matter.
> 
> Hmm yeah I know about that, I am not quite clear on how that change
> affects this.

Well, the explanation for what I had in mind is too complicated to even
put into words, so it's probably not a good idea. ;-)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-22 Thread Peter Eisentraut 
On 3/18/17 09:31, Petr Jelinek wrote:
>> 0001 Refine rules for altering publication owner
>>
>> kind of a bug fix
> 
> Agreed, this can be committed as is.
> 
>>
>> 0002 Change logical replication pg_hba.conf use
>>
>> This was touched upon in the discussion at
>> 
>> and seems to have been viewed favorably there.
> 
> Seems like a good idea and I think can be committed as well.

Committed these two.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-22 Thread Petr Jelinek 
On 22/03/17 03:38, Peter Eisentraut wrote:
> On 3/20/17 15:10, Petr Jelinek wrote:
>> Hmm but REPLICATION role can do basebackup/consume wal, so how does
>> giving it limited publication access help? Wouldn't we need some
>> SUBSCRIPTION role/grant used instead for logical replication connections
>> instead of REPLICATION for this to make sense?
> 
> Since we're splitting up the pg_hba.conf setup for logical and physical
> connections, it would probably not matter.

Hmm yeah I know about that, I am not quite clear on how that change
affects this.

> 
> But just to think it through, how could we split this up sensibly?
> 
> Here is the complete list of things that rolreplication allows:
> 
> - create/drop replication slot
> - pg_logical_slot_get_changes() and friends
> - connect to walsender
> 
> For logical replication, we could slice it up this way:
> 
> - new user attribute allowing the creating of logical replication slots
> - store owner of slot, allow drop and get based on ownership
> - allow anyone to connect as walsender
> 

I am not quite sure we can do the owner part. Slots are not usual
catalog and there is this idea that it should be possible to create them
on standby (at least it was reason why our last year proposal to
propagate slot creation/updates via WAL was shot down). So we can't do
any of the dependency stuff for them.

> Another problem is that the walsender command to create a replication
> slot allows you to load an arbitrary plugin.
> 

Yeah I am also not sure what to do with the SQL interface tbh as that
has same problem.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-21 Thread Peter Eisentraut 
On 3/20/17 15:10, Petr Jelinek wrote:
> Hmm but REPLICATION role can do basebackup/consume wal, so how does
> giving it limited publication access help? Wouldn't we need some
> SUBSCRIPTION role/grant used instead for logical replication connections
> instead of REPLICATION for this to make sense?

Since we're splitting up the pg_hba.conf setup for logical and physical
connections, it would probably not matter.

But just to think it through, how could we split this up sensibly?

Here is the complete list of things that rolreplication allows:

- create/drop replication slot
- pg_logical_slot_get_changes() and friends
- connect to walsender

For logical replication, we could slice it up this way:

- new user attribute allowing the creating of logical replication slots
- store owner of slot, allow drop and get based on ownership
- allow anyone to connect as walsender

Another problem is that the walsender command to create a replication
slot allows you to load an arbitrary plugin.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-20 Thread Petr Jelinek 
On 20/03/17 13:32, Peter Eisentraut wrote:
> On 3/18/17 09:31, Petr Jelinek wrote:
>>> 0003 Add USAGE privilege for publications
>>>
>>> a way to control who can subscribe to a publication
>>>
>> Hmm IIUC this removes ability of REPLICATION role to subscribe to
>> publications. I am not quite sure I like that.
> 
> Well, this is kind of the way with all privileges.  They take away
> abilities by default so you can assign them in a more fine-grained manner.
> 
> You can still connect as superuser and do anything you want, if you want
> a "quick start" setup.
> 
> Right now, any replication user connecting can use any publication.
> There is no way to distinguish different table groupings or different
> use cases, such as partial replication of some tables that should go
> over here, or archiving of some other tables that should go over there.
> That's not optimal.
> 

Hmm but REPLICATION role can do basebackup/consume wal, so how does
giving it limited publication access help? Wouldn't we need some
SUBSCRIPTION role/grant used instead for logical replication connections
instead of REPLICATION for this to make sense?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-20 Thread Peter Eisentraut 
On 3/18/17 09:31, Petr Jelinek wrote:
>> 0003 Add USAGE privilege for publications
>>
>> a way to control who can subscribe to a publication
>>
> Hmm IIUC this removes ability of REPLICATION role to subscribe to
> publications. I am not quite sure I like that.

Well, this is kind of the way with all privileges.  They take away
abilities by default so you can assign them in a more fine-grained manner.

You can still connect as superuser and do anything you want, if you want
a "quick start" setup.

Right now, any replication user connecting can use any publication.
There is no way to distinguish different table groupings or different
use cases, such as partial replication of some tables that should go
over here, or archiving of some other tables that should go over there.
That's not optimal.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-18 Thread Petr Jelinek 
Hi,

I went over this patch set, don't really have all that much to say
except it looks good for the most part (details inline).


On 16/03/17 02:54, Peter Eisentraut wrote:
> New patch set based on the discussions.  I have dropped the PUBLICATION
> privilege patch.  The patches are also reordered a bit in approximate
> decreasing priority order.
> 
> 0001 Refine rules for altering publication owner
> 
> kind of a bug fix

Agreed, this can be committed as is.

> 
> 0002 Change logical replication pg_hba.conf use
> 
> This was touched upon in the discussion at
> 
> and seems to have been viewed favorably there.

Seems like a good idea and I think can be committed as well.

> 
> 0003 Add USAGE privilege for publications
> 
> a way to control who can subscribe to a publication
> 

Hmm IIUC this removes ability of REPLICATION role to subscribe to
publications. I am not quite sure I like that.

> 0004 Add subscription apply worker privilege checks
> 
> This is a prerequisite for the next one (or one like it).
> 
> 0005 Add CREATE SUBSCRIPTION privilege on databases
> 
> Need a way to determine which user can create subscriptions.  The
> presented approach made sense to me, but maybe there are other ideas.
> 

The CREATE SUBSCRIPTION as name of privilege is bit weird but something
like SUBSCRIBE would be more fitting for publish side (to which you
subscriber) so don't really have a better name. I like that the patches
cache the acl result so performance impact should be negligible.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-15 Thread Peter Eisentraut 
New patch set based on the discussions.  I have dropped the PUBLICATION
privilege patch.  The patches are also reordered a bit in approximate
decreasing priority order.

0001 Refine rules for altering publication owner

kind of a bug fix

0002 Change logical replication pg_hba.conf use

This was touched upon in the discussion at

and seems to have been viewed favorably there.

0003 Add USAGE privilege for publications

a way to control who can subscribe to a publication

0004 Add subscription apply worker privilege checks

This is a prerequisite for the next one (or one like it).

0005 Add CREATE SUBSCRIPTION privilege on databases

Need a way to determine which user can create subscriptions.  The
presented approach made sense to me, but maybe there are other ideas.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 676e6e26f1b0500907c0cd810969bb015ee548d1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 13 Feb 2017 08:57:45 -0500
Subject: [PATCH v2 1/5] Refine rules for altering publication owner

Previously, the new owner had to be a superuser.  The new rules are more
refined similar to other objects.
---
 doc/src/sgml/ref/alter_publication.sgml   |  7 +--
 src/backend/commands/publicationcmds.c| 34 ++-
 src/test/regress/expected/publication.out |  8 
 src/test/regress/sql/publication.sql  |  4 
 4 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
index 47d83b80be..776661bbeb 100644
--- a/doc/src/sgml/ref/alter_publication.sgml
+++ b/doc/src/sgml/ref/alter_publication.sgml
@@ -48,8 +48,11 @@ Description
   
 
   
-   To alter the owner, you must also be a direct or indirect member of the
-   new owning role. The new owner has to be a superuser
+   To alter the owner, you must also be a direct or indirect member of the new
+   owning role. The new owner must have CREATE privilege on
+   the database.  Also, the new owner of a FOR ALL TABLES
+   publication must be a superuser.  However, a superuser can change the
+   ownership of a publication while circumventing these restrictions.
   
 
   
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 04f83e0a2e..d69e39dc5b 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -670,17 +670,31 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 	if (form->pubowner == newOwnerId)
 		return;
 
-	if (!pg_publication_ownercheck(HeapTupleGetOid(tup), GetUserId()))
-		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PUBLICATION,
-	   NameStr(form->pubname));
+	if (!superuser())
+	{
+		AclResult	aclresult;
 
-	/* New owner must be a superuser */
-	if (!superuser_arg(newOwnerId))
-		ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of publication \"%s\"",
-		NameStr(form->pubname)),
- errhint("The owner of a publication must be a superuser.")));
+		/* Must be owner */
+		if (!pg_publication_ownercheck(HeapTupleGetOid(tup), GetUserId()))
+			aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PUBLICATION,
+		   NameStr(form->pubname));
+
+		/* Must be able to become new owner */
+		check_is_member_of_role(GetUserId(), newOwnerId);
+
+		/* New owner must have CREATE privilege on database */
+		aclresult = pg_database_aclcheck(MyDatabaseId, newOwnerId, ACL_CREATE);
+		if (aclresult != ACLCHECK_OK)
+			aclcheck_error(aclresult, ACL_KIND_DATABASE,
+		   get_database_name(MyDatabaseId));
+
+		if (form->puballtables && !superuser_arg(newOwnerId))
+			ereport(ERROR,
+	(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+	 errmsg("permission denied to change owner of publication \"%s\"",
+			NameStr(form->pubname)),
+	 errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+	}
 
 	form->pubowner = newOwnerId;
 	CatalogTupleUpdate(rel, >t_self, tup);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 7c4834b213..5a7c0edf7d 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -182,6 +182,14 @@ ALTER PUBLICATION testpub_default RENAME TO testpub_foo;
 
 -- rename back to keep the rest simple
 ALTER PUBLICATION testpub_foo RENAME TO testpub_default;
+ALTER PUBLICATION testpub_default OWNER TO regress_publication_user2;
+\dRp testpub_default
+   List of publications
+  Name   |   Owner   | Inserts | Updates | Deletes 
+-+---+-+-+-
+ testpub_default | regress_publication_user2 | t   | t

Re: [HACKERS] logical replication access control patches

2017-03-15 Thread Peter Eisentraut 
On 3/14/17 14:49, Petr Jelinek wrote:
> Not what I mean - owner should be able to publish table. If you are
> granted role of the owner you can do what owner can no?

I didn't actually know that ownership worked that way.  You can grant
the role of an owner to someone, and then that someone has ownership
rights.  So that should satisfy a pretty good range of use cases for who
can publish what tables.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-15 Thread Peter Eisentraut 
On 3/14/17 15:37, Petr Jelinek wrote:
> Yeah that's rather hard to say in front. Maybe safest action would be to
> give the permission to owners in 10 and revisit special privilege in 11
> based on feedback?

I'm fine with that.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-15 Thread Peter Eisentraut 
On 3/14/17 15:05, Stephen Frost wrote:
> Another approach to solving my concern would be to only allow the
> publishing of tables by non-owner users who have table-level SELECT
> rights

An early version of the logical replication patch set did that.  But the
problem is that this way someone with SELECT privilege could include a
table without replica identity index into a publication and thereby
prevent updates to the table.  There might be ways to tweak things to
make that work better, but right now it works that way.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Robert Haas 
On Tue, Mar 14, 2017 at 3:37 PM, Petr Jelinek
 wrote:
> On 14/03/17 20:09, Robert Haas wrote:
>> On Tue, Mar 14, 2017 at 2:56 PM, Petr Jelinek
>>  wrote:
>>> Note that I am not necessarily saying it's better though, just trying to
>>> explain. It definitely has drawbacks, as in order to grant publish on
>>> one table you might be granting lots of privileges on various objects by
>>> granting the role. So for granularity purposes Peter's PUBLISH privilege
>>> for tables sounds better to me.
>>
>> I get that.  If, without the patch, letting user X do operation Y will
>> require either giving user X membership in a role that has many
>> privileges, and with the patch, will require only granting a specific
>> privilege on a specific object, then the latter is obviously far
>> better from a security point of view.
>>
>> However, what I'm not clear about is whether this is a situation
>> that's likely to come up much in practice.  I would have thought that
>> publications and subscriptions would typically be configured by roles
>> with quite high levels of privilege anyway, in which case the separate
>> PUBLISH privilege would rarely be used in practice, and might
>> therefore fail to be worth using up a bit.  I might be missing a
>> plausible scenario in which that's not the case, though.
>
> Yeah that's rather hard to say in front. Maybe safest action would be to
> give the permission to owners in 10 and revisit special privilege in 11
> based on feedback?

I think that would be entirely sensible.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Petr Jelinek 
On 14/03/17 20:09, Robert Haas wrote:
> On Tue, Mar 14, 2017 at 2:56 PM, Petr Jelinek
>  wrote:
>> Note that I am not necessarily saying it's better though, just trying to
>> explain. It definitely has drawbacks, as in order to grant publish on
>> one table you might be granting lots of privileges on various objects by
>> granting the role. So for granularity purposes Peter's PUBLISH privilege
>> for tables sounds better to me.
> 
> I get that.  If, without the patch, letting user X do operation Y will
> require either giving user X membership in a role that has many
> privileges, and with the patch, will require only granting a specific
> privilege on a specific object, then the latter is obviously far
> better from a security point of view.
> 
> However, what I'm not clear about is whether this is a situation
> that's likely to come up much in practice.  I would have thought that
> publications and subscriptions would typically be configured by roles
> with quite high levels of privilege anyway, in which case the separate
> PUBLISH privilege would rarely be used in practice, and might
> therefore fail to be worth using up a bit.  I might be missing a
> plausible scenario in which that's not the case, though.
> 

Yeah that's rather hard to say in front. Maybe safest action would be to
give the permission to owners in 10 and revisit special privilege in 11
based on feedback?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Stephen Frost 
Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> However, what I'm not clear about is whether this is a situation
> that's likely to come up much in practice.  I would have thought that
> publications and subscriptions would typically be configured by roles
> with quite high levels of privilege anyway, in which case the separate
> PUBLISH privilege would rarely be used in practice, and might
> therefore fail to be worth using up a bit.  I might be missing a
> plausible scenario in which that's not the case, though.

Right, this is part of my concern also.

Further, PUBLISH, as I understand it, is something of a one-time or at
least reasonably rarely done operation.  This is quite different from a
SELECT privilege which is used on every query against the table and
which may be GRANT'd to user X today and user Y tomorrow and perhaps
REVOKE'd from user X the next day.

What happens when the PUBLISH right is REVOKE'd from the user who did
the PUBLISH in the first place, for example..?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Robert Haas 
On Tue, Mar 14, 2017 at 2:56 PM, Petr Jelinek
 wrote:
> Note that I am not necessarily saying it's better though, just trying to
> explain. It definitely has drawbacks, as in order to grant publish on
> one table you might be granting lots of privileges on various objects by
> granting the role. So for granularity purposes Peter's PUBLISH privilege
> for tables sounds better to me.

I get that.  If, without the patch, letting user X do operation Y will
require either giving user X membership in a role that has many
privileges, and with the patch, will require only granting a specific
privilege on a specific object, then the latter is obviously far
better from a security point of view.

However, what I'm not clear about is whether this is a situation
that's likely to come up much in practice.  I would have thought that
publications and subscriptions would typically be configured by roles
with quite high levels of privilege anyway, in which case the separate
PUBLISH privilege would rarely be used in practice, and might
therefore fail to be worth using up a bit.  I might be missing a
plausible scenario in which that's not the case, though.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Stephen Frost 
Greetings,

* Petr Jelinek (petr.jeli...@2ndquadrant.com) wrote:
> On 14/03/17 19:47, Robert Haas wrote:
> > On Tue, Mar 14, 2017 at 2:41 PM, Petr Jelinek
> >  wrote:
> >> My understanding of what Shephen is proposing is, you have "ownerA" of
> >> tableA and "ownerB" of tableB, then you want role "publishe"r to be able
> >> to publish those, so you simply grant it the "ownerA" and "ownerB"
> >> roles. Obviously that might is many situations mean that the "publisher"
> >> role potentially also gets sweeping privileges to other tables which may
> >> not be desirable.
> > 
> > I didn't hear Stephen propose that "publish" should be a
> > role-attribute, and I don't understand why that would be a good idea.
> > Presumably, we don't want unprivileged users to be able to fire up
> > logical replication because that involves making connections to other
> > systems from the PostgreSQL operating system user's account, and that
> > should be a privileged operation.  But that's the subscriber side, not
> > the publisher side.
> > 
> > I don't otherwise follow Stephen's argument.  It seems like he's
> > complaining that PUBLISH might give more access to the relation than
> > SELECT, but, uh, that's what granting additional privileges does in
> > general, by definition.  Mostly we consider that a feature, not a bug.
> 
> Not what I mean - owner should be able to publish table. If you are
> granted role of the owner you can do what owner can no? That's how I
> understand Stephen's proposal.

Exactly correct, yes.  I was not suggesting it be a role attribute.

If we move forward with making PUBLISH a GRANT'able option then I do
worry that people will be surprised that PUBLISH gives you more access
to the table than a straight SELECT does.  We have a good deal of
granularity in what a user can GRANT'd to see and PUBLISH completely
ignores all of that.

The way I see PUBLISH, it's another command which is able to read from a
table, not unlike the way COPY works, but we don't have an independent
COPY privilege and the COPY command does actually respect the SELECT
privileges on the table.

Another approach to solving my concern would be to only allow the
publishing of tables by non-owner users who have table-level SELECT
rights (meaning they can see all columns of the table) and which don't
have RLS enabled.

Frankly, though, I really don't buy the argument that there's a serious
use-case for non-owners to be GRANT'd the PUBLISH capability, which
means we would end up burning one of the few remaining privilege bits
for something that isn't going to be used and I don't care for that
either.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Petr Jelinek 
On 14/03/17 19:49, Petr Jelinek wrote:
> On 14/03/17 19:47, Robert Haas wrote:
>> On Tue, Mar 14, 2017 at 2:41 PM, Petr Jelinek
>>  wrote:
>>> My understanding of what Shephen is proposing is, you have "ownerA" of
>>> tableA and "ownerB" of tableB, then you want role "publishe"r to be able
>>> to publish those, so you simply grant it the "ownerA" and "ownerB"
>>> roles. Obviously that might is many situations mean that the "publisher"
>>> role potentially also gets sweeping privileges to other tables which may
>>> not be desirable.
>>
>> I didn't hear Stephen propose that "publish" should be a
>> role-attribute, and I don't understand why that would be a good idea.
>> Presumably, we don't want unprivileged users to be able to fire up
>> logical replication because that involves making connections to other
>> systems from the PostgreSQL operating system user's account, and that
>> should be a privileged operation.  But that's the subscriber side, not
>> the publisher side.
>>
>> I don't otherwise follow Stephen's argument.  It seems like he's
>> complaining that PUBLISH might give more access to the relation than
>> SELECT, but, uh, that's what granting additional privileges does in
>> general, by definition.  Mostly we consider that a feature, not a bug.
>>
> 
> Not what I mean - owner should be able to publish table. If you are
> granted role of the owner you can do what owner can no? That's how I
> understand Stephen's proposal.
> 

Note that I am not necessarily saying it's better though, just trying to
explain. It definitely has drawbacks, as in order to grant publish on
one table you might be granting lots of privileges on various objects by
granting the role. So for granularity purposes Peter's PUBLISH privilege
for tables sounds better to me.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Petr Jelinek 
On 14/03/17 19:47, Robert Haas wrote:
> On Tue, Mar 14, 2017 at 2:41 PM, Petr Jelinek
>  wrote:
>> My understanding of what Shephen is proposing is, you have "ownerA" of
>> tableA and "ownerB" of tableB, then you want role "publishe"r to be able
>> to publish those, so you simply grant it the "ownerA" and "ownerB"
>> roles. Obviously that might is many situations mean that the "publisher"
>> role potentially also gets sweeping privileges to other tables which may
>> not be desirable.
> 
> I didn't hear Stephen propose that "publish" should be a
> role-attribute, and I don't understand why that would be a good idea.
> Presumably, we don't want unprivileged users to be able to fire up
> logical replication because that involves making connections to other
> systems from the PostgreSQL operating system user's account, and that
> should be a privileged operation.  But that's the subscriber side, not
> the publisher side.
> 
> I don't otherwise follow Stephen's argument.  It seems like he's
> complaining that PUBLISH might give more access to the relation than
> SELECT, but, uh, that's what granting additional privileges does in
> general, by definition.  Mostly we consider that a feature, not a bug.
> 

Not what I mean - owner should be able to publish table. If you are
granted role of the owner you can do what owner can no? That's how I
understand Stephen's proposal.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Robert Haas 
On Tue, Mar 14, 2017 at 2:41 PM, Petr Jelinek
 wrote:
> My understanding of what Shephen is proposing is, you have "ownerA" of
> tableA and "ownerB" of tableB, then you want role "publishe"r to be able
> to publish those, so you simply grant it the "ownerA" and "ownerB"
> roles. Obviously that might is many situations mean that the "publisher"
> role potentially also gets sweeping privileges to other tables which may
> not be desirable.

I didn't hear Stephen propose that "publish" should be a
role-attribute, and I don't understand why that would be a good idea.
Presumably, we don't want unprivileged users to be able to fire up
logical replication because that involves making connections to other
systems from the PostgreSQL operating system user's account, and that
should be a privileged operation.  But that's the subscriber side, not
the publisher side.

I don't otherwise follow Stephen's argument.  It seems like he's
complaining that PUBLISH might give more access to the relation than
SELECT, but, uh, that's what granting additional privileges does in
general, by definition.  Mostly we consider that a feature, not a bug.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-14 Thread Petr Jelinek 
On 10/03/17 20:02, Peter Eisentraut wrote:
> On 2/27/17 22:10, Stephen Frost wrote:
>> Peter,
>>
>> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>>> On 2/18/17 18:06, Stephen Frost wrote:
 I'm not convinced that it really makes sense to have PUBLICATION of a
 table be independent from the rights an owner of a table has.  We don't
 allow other ALTER commands on objects based on GRANT'able rights, in
 general, so I'm not really sure that it makes sense to do so here.
>>>
>>> The REFERENCES and TRIGGER privileges are very similar in principle.
>>
>> TRIGGER is a great example of an, ultimately, poorly chosen privilege to
>> GRANT away, with a good history of discussion about it:
>>
>> https://www.postgresql.org/message-id/21827.1166115978%40sss.pgh.pa.us
>> https://www.postgresql.org/message-id/7389.1405554356%40sss.pgh.pa.us
> 
> Those discussions about the trigger privileges are valid, but the actual
> reason why this is a problem is because our trigger implementation is
> broken by default.  In the SQL standard, triggers are executed as the
> table owner, so the trigger procedure does not have full account access
> to the role that is causing the trigger to fire.  This is an independent
> problem that deserves consideration, but it does not invalidate the kind
> of privilege that can be granted.
> 
>>> Then you couldn't set up a replication structure involving tables owned
>>> by different users without resorting to blunt instruments like having
>>> everything owned by the same user or using superusers.
>>
>> That's not correct- you would simply need a user who is considered an
>> owner for all of the objects which you want to replicate, that doesn't
>> have to be a *direct* owner or a superuser.
>>
>> The tables can have individual roles, with some admin role who is a
>> member of those other roles, or another mechanism (as Simon has proposed
>> not too long ago...) to have a given role be considered equivilant to an
>> owner for all of the relations in a particular database.
> 
> I'm not really following what you are saying here.  It seems to me that
> you are describing a new kind of facility that gives a role a given
> capability with respect to a table.
> 
> If so, we already have that, namely privileges.  If not, please elaborate.
> 

My understanding of what Shephen is proposing is, you have "ownerA" of
tableA and "ownerB" of tableB, then you want role "publishe"r to be able
to publish those, so you simply grant it the "ownerA" and "ownerB"
roles. Obviously that might is many situations mean that the "publisher"
role potentially also gets sweeping privileges to other tables which may
not be desirable.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-10 Thread Peter Eisentraut 
On 3/3/17 10:07, Stephen Frost wrote:
> Will users really understand that the PUBLISH right actually allows
> complete access to the entire relation, rather than just the ability for
> a user to PUBLISH what they are currently about to SELECT?  It certainly
> doesn't seem intuitive to me, which is why I am concerned that it's
> going to lead to confusion and bug/security reports down the road, or,
> worse, poorly configured systems. 

Well, this is a new system with its own properties, which is why I'm
proposing a new way to manage access.  If it were just the same as the
existing stuff, we wouldn't need this conversation.  I'm interested in
other ideas.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-10 Thread Peter Eisentraut 
On 2/27/17 22:10, Stephen Frost wrote:
> Peter,
> 
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 2/18/17 18:06, Stephen Frost wrote:
>>> I'm not convinced that it really makes sense to have PUBLICATION of a
>>> table be independent from the rights an owner of a table has.  We don't
>>> allow other ALTER commands on objects based on GRANT'able rights, in
>>> general, so I'm not really sure that it makes sense to do so here.
>>
>> The REFERENCES and TRIGGER privileges are very similar in principle.
> 
> TRIGGER is a great example of an, ultimately, poorly chosen privilege to
> GRANT away, with a good history of discussion about it:
> 
> https://www.postgresql.org/message-id/21827.1166115978%40sss.pgh.pa.us
> https://www.postgresql.org/message-id/7389.1405554356%40sss.pgh.pa.us

Those discussions about the trigger privileges are valid, but the actual
reason why this is a problem is because our trigger implementation is
broken by default.  In the SQL standard, triggers are executed as the
table owner, so the trigger procedure does not have full account access
to the role that is causing the trigger to fire.  This is an independent
problem that deserves consideration, but it does not invalidate the kind
of privilege that can be granted.

>> Then you couldn't set up a replication structure involving tables owned
>> by different users without resorting to blunt instruments like having
>> everything owned by the same user or using superusers.
> 
> That's not correct- you would simply need a user who is considered an
> owner for all of the objects which you want to replicate, that doesn't
> have to be a *direct* owner or a superuser.
> 
> The tables can have individual roles, with some admin role who is a
> member of those other roles, or another mechanism (as Simon has proposed
> not too long ago...) to have a given role be considered equivilant to an
> owner for all of the relations in a particular database.

I'm not really following what you are saying here.  It seems to me that
you are describing a new kind of facility that gives a role a given
capability with respect to a table.

If so, we already have that, namely privileges.  If not, please elaborate.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-03-03 Thread Stephen Frost 
* Petr Jelinek (petr.jeli...@2ndquadrant.com) wrote:
> On 28/02/17 04:10, Stephen Frost wrote:
> > Peter,
> > 
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> On 2/18/17 18:06, Stephen Frost wrote:
> >>> I'm not convinced that it really makes sense to have PUBLICATION of a
> >>> table be independent from the rights an owner of a table has.  We don't
> >>> allow other ALTER commands on objects based on GRANT'able rights, in
> >>> general, so I'm not really sure that it makes sense to do so here.
> >>
> >> The REFERENCES and TRIGGER privileges are very similar in principle.
> > 
> > TRIGGER is a great example of an, ultimately, poorly chosen privilege to
> > GRANT away, with a good history of discussion about it:
> > 
> > https://www.postgresql.org/message-id/21827.1166115978%40sss.pgh.pa.us
> > https://www.postgresql.org/message-id/7389.1405554356%40sss.pgh.pa.us
> > 
> > Further, how would RLS be handled with publication?  I've been assuming
> > that it's simply ignored, but that's clearly wrong if a non-owner can
> > publish a table that they just have SELECT,PUBLISH on, isn't it?
> 
> I don't see why would PUBLISH care about RLS.

Perhaps I'm missing something here, in which case I would certainly
welcome some clarification, but in curreng PG I can GRANT you access to
a table and then limit what you can see with it using RLS and policies.

If I then GRANT you the PUBLISH right- shouldn't that also respect the
RLS setting and the policies set on the table?  Otherwise, PUBLISH is
allowing you to gain access to the data in the table that you wouldn't
normally be able to see with a simple SELECT.

We don't really even need to get to the RLS level to consider this
situation- what about column-level privileges?

Will users really understand that the PUBLISH right actually allows
complete access to the entire relation, rather than just the ability for
a user to PUBLISH what they are currently about to SELECT?  It certainly
doesn't seem intuitive to me, which is why I am concerned that it's
going to lead to confusion and bug/security reports down the road, or,
worse, poorly configured systems. 

> >>> The downside of adding these privileges is that we're burning through
> >>> the last few bits in the ACLMASK for a privilege that doesn't really
> >>> seem like it's something that would be GRANT'd in general usage.
> >>
> >> I don't see any reason why we couldn't increase the size of AclMode if
> >> it becomes necessary.
> > 
> > I've fought exactly that argument before and there is a good deal of
> > entirely reasonable push-back on increasing our catalogs by so much.
> 
> Hmm to be honest I don't see what's the issue there.

It's a not-insignificant increase in our system catalog data, plus a
bunch of work for whomever ends up being the one to want to push us
over.  The other approach would be to make the ACL system understand
that not every privilege applies to every object, but that's would also
be a bunch of work (most likely more...).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] logical replication access control patches

2017-02-27 Thread Petr Jelinek 
On 28/02/17 04:10, Stephen Frost wrote:
> Peter,
> 
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 2/18/17 18:06, Stephen Frost wrote:
>>> I'm not convinced that it really makes sense to have PUBLICATION of a
>>> table be independent from the rights an owner of a table has.  We don't
>>> allow other ALTER commands on objects based on GRANT'able rights, in
>>> general, so I'm not really sure that it makes sense to do so here.
>>
>> The REFERENCES and TRIGGER privileges are very similar in principle.
> 
> TRIGGER is a great example of an, ultimately, poorly chosen privilege to
> GRANT away, with a good history of discussion about it:
> 
> https://www.postgresql.org/message-id/21827.1166115978%40sss.pgh.pa.us
> https://www.postgresql.org/message-id/7389.1405554356%40sss.pgh.pa.us
> 
> Further, how would RLS be handled with publication?  I've been assuming
> that it's simply ignored, but that's clearly wrong if a non-owner can
> publish a table that they just have SELECT,PUBLISH on, isn't it?
> 

I don't see why would PUBLISH care about RLS.

>>> The downside of adding these privileges is that we're burning through
>>> the last few bits in the ACLMASK for a privilege that doesn't really
>>> seem like it's something that would be GRANT'd in general usage.
>>
>> I don't see any reason why we couldn't increase the size of AclMode if
>> it becomes necessary.
> 
> I've fought exactly that argument before and there is a good deal of
> entirely reasonable push-back on increasing our catalogs by so much.
> 

Hmm to be honest I don't see what's the issue there.

>>> I'm certainly all for removing the need for users to be the superuser
>>> for such commands, just not sure that they should be GRANT'able
>>> privileges instead of privileges which the owner of the relation or
>>> database has.
>>
>> Then you couldn't set up a replication structure involving tables owned
>> by different users without resorting to blunt instruments like having
>> everything owned by the same user or using superusers.
> 
> That's not correct- you would simply need a user who is considered an
> owner for all of the objects which you want to replicate, that doesn't
> have to be a *direct* owner or a superuser.
> 
> The tables can have individual roles, with some admin role who is a
> member of those other roles, or another mechanism (as Simon has proposed
> not too long ago...) to have a given role be considered equivilant to an
> owner for all of the relations in a particular database.
> 

I do agree with this though. And I also agree with the overall sentiment
that we don't need special PUBLICATION privilege on tables.

I do like the rest of the patch.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-02-27 Thread Stephen Frost 
Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2/18/17 18:06, Stephen Frost wrote:
> > I'm not convinced that it really makes sense to have PUBLICATION of a
> > table be independent from the rights an owner of a table has.  We don't
> > allow other ALTER commands on objects based on GRANT'able rights, in
> > general, so I'm not really sure that it makes sense to do so here.
> 
> The REFERENCES and TRIGGER privileges are very similar in principle.

TRIGGER is a great example of an, ultimately, poorly chosen privilege to
GRANT away, with a good history of discussion about it:

https://www.postgresql.org/message-id/21827.1166115978%40sss.pgh.pa.us
https://www.postgresql.org/message-id/7389.1405554356%40sss.pgh.pa.us

Further, how would RLS be handled with publication?  I've been assuming
that it's simply ignored, but that's clearly wrong if a non-owner can
publish a table that they just have SELECT,PUBLISH on, isn't it?

> > The downside of adding these privileges is that we're burning through
> > the last few bits in the ACLMASK for a privilege that doesn't really
> > seem like it's something that would be GRANT'd in general usage.
> 
> I don't see any reason why we couldn't increase the size of AclMode if
> it becomes necessary.

I've fought exactly that argument before and there is a good deal of
entirely reasonable push-back on increasing our catalogs by so much.

> > I'm certainly all for removing the need for users to be the superuser
> > for such commands, just not sure that they should be GRANT'able
> > privileges instead of privileges which the owner of the relation or
> > database has.
> 
> Then you couldn't set up a replication structure involving tables owned
> by different users without resorting to blunt instruments like having
> everything owned by the same user or using superusers.

That's not correct- you would simply need a user who is considered an
owner for all of the objects which you want to replicate, that doesn't
have to be a *direct* owner or a superuser.

The tables can have individual roles, with some admin role who is a
member of those other roles, or another mechanism (as Simon has proposed
not too long ago...) to have a given role be considered equivilant to an
owner for all of the relations in a particular database.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] logical replication access control patches

2017-02-25 Thread Peter Eisentraut 
On 2/18/17 18:06, Stephen Frost wrote:
> I'm not convinced that it really makes sense to have PUBLICATION of a
> table be independent from the rights an owner of a table has.  We don't
> allow other ALTER commands on objects based on GRANT'able rights, in
> general, so I'm not really sure that it makes sense to do so here.

The REFERENCES and TRIGGER privileges are very similar in principle.

> The downside of adding these privileges is that we're burning through
> the last few bits in the ACLMASK for a privilege that doesn't really
> seem like it's something that would be GRANT'd in general usage.

I don't see any reason why we couldn't increase the size of AclMode if
it becomes necessary.

> I'm certainly all for removing the need for users to be the superuser
> for such commands, just not sure that they should be GRANT'able
> privileges instead of privileges which the owner of the relation or
> database has.

Then you couldn't set up a replication structure involving tables owned
by different users without resorting to blunt instruments like having
everything owned by the same user or using superusers.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

2017-02-18 Thread Stephen Frost 
Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> 0002 Add PUBLICATION privilege
> 
> Add a new privilege kind to tables to determine whether they can be
> added to a publication.

I'm not convinced that it really makes sense to have PUBLICATION of a
table be independent from the rights an owner of a table has.  We don't
allow other ALTER commands on objects based on GRANT'able rights, in
general, so I'm not really sure that it makes sense to do so here.

The downside of adding these privileges is that we're burning through
the last few bits in the ACLMASK for a privilege that doesn't really
seem like it's something that would be GRANT'd in general usage.

I have similar reservations regarding the proposed SUBSCRIPTION
privilege.

I'm certainly all for removing the need for users to be the superuser
for such commands, just not sure that they should be GRANT'able
privileges instead of privileges which the owner of the relation or
database has.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] logical replication access control patches

2017-02-17 Thread Peter Eisentraut 
Here is a patch set to refine various access control settings in logical
replication.  Currently, you need to be replication or superuser for
most things, and the goal of these patches is to allow ordinary users
equipped with explicit privileges to do most things.  (Btw., current
documentation is here:
https://www.postgresql.org/docs/devel/static/logical-replication-security.html)

0001 Refine rules for altering publication owner

No conceptual changes here, just some fixes to allow altering
publication owner in more cases.

0002 Add PUBLICATION privilege

Add a new privilege kind to tables to determine whether they can be
added to a publication.

0003 Add USAGE privilege for publications

This controls whether a subscription can use the publication.

There is an open issue with this patch:  Since the walsender reads
system catalogs according to what it is currently streaming, you can't
grant this privilege after a subscription has already tried to connect
and failed, because the grant will only appear in the "future" of the
stream.  (You can drop and recreate the subscription, as the test
shows.)  This might need some snapshot trickery around the aclcheck call.

0004 Add CREATE SUBSCRIPTION privilege on databases

New privilege to allow creating a subscription, currently restricted to
superuser.

(We could also add a CREATE PUBLICATION privilege for symmetry.
Currently, publications use the CREATE privilege that schemas also use.)

0005 Add subscription apply worker privilege checks

Makes apply workers check privileges on tables before writing to them.
Currently, all subscription owners are superuser, but 0004 proposes to
change that.

0006 Change logical replication pg_hba.conf use

No longer use the "replication" keyword in pg_hba.conf for logical
replication.  Use the normal database entries instead.

Relates to
https://www.postgresql.org/message-id/flat/CAB7nPqRf8eOv15SPQJbC1npJoDWTNPMTNp6AvMN-XWwB53h2Cg%40mail.gmail.com

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 544bd6e6a88ff8ced8648d4601b5e76ed2d6298c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 13 Feb 2017 08:57:45 -0500
Subject: [PATCH 1/6] Refine rules for altering publication owner

Previously, the new owner had to be a superuser.  The new rules are more
refined similar to other objects.
---
 doc/src/sgml/ref/alter_publication.sgml   |  7 --
 src/backend/commands/publicationcmds.c| 36 +--
 src/test/regress/expected/publication.out | 11 +-
 src/test/regress/sql/publication.sql  |  7 +-
 4 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
index 47d83b80be..776661bbeb 100644
--- a/doc/src/sgml/ref/alter_publication.sgml
+++ b/doc/src/sgml/ref/alter_publication.sgml
@@ -48,8 +48,11 @@ Description
   
 
   
-   To alter the owner, you must also be a direct or indirect member of the
-   new owning role. The new owner has to be a superuser
+   To alter the owner, you must also be a direct or indirect member of the new
+   owning role. The new owner must have CREATE privilege on
+   the database.  Also, the new owner of a FOR ALL TABLES
+   publication must be a superuser.  However, a superuser can change the
+   ownership of a publication while circumventing these restrictions.
   
 
   
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 04f83e0a2e..0e4eb0726d 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -660,7 +660,7 @@ PublicationDropTables(Oid pubid, List *rels, bool missing_ok)
 /*
  * Internal workhorse for changing a publication owner
  */
-	static void
+   static void
 AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 {
 	Form_pg_publication form;
@@ -670,17 +670,31 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 	if (form->pubowner == newOwnerId)
 		return;
 
-	if (!pg_publication_ownercheck(HeapTupleGetOid(tup), GetUserId()))
-		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PUBLICATION,
-	   NameStr(form->pubname));
+	if (!superuser())
+	{
+		AclResult	aclresult;
 
-	/* New owner must be a superuser */
-	if (!superuser_arg(newOwnerId))
-		ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of publication \"%s\"",
-		NameStr(form->pubname)),
- errhint("The owner of a publication must be a superuser.")));
+		/* Must be owner */
+		if (!pg_publication_ownercheck(HeapTupleGetOid(tup), GetUserId()))
+			aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PUBLICATION,
+		   NameStr(form->pubname));
+
+		/* Must be able to become new owner */
+		check_is_member_of_role(GetUserId(), newOwnerId);
+
+		/* New owner must have CREATE privilege on database */
+