Horton

Thread

Commits

  1. Refactor ExecGrant_*() functions

  2. doc: Add note about lack of publication privileges

  1. Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-09T14:09:57Z

    Now that the user can specify rows and columns to be omitted from the logical
replication [1], I suppose hiding rows and columns from the subscriber is an
important use case. However, since the subscription connection user (i.e. the
user specified in the CREATE SUBSCRIPTION ... CONNECTION ... command) needs
SELECT permission on the replicated table (on the publication side), he can
just use another publication (which has different filters or no filters at
all) to get the supposedly-hidden data replicated.

Don't we need privileges on publication (e.g GRANT USAGE ON PUBLICATION ...)
now?

[1] https://www.postgresql.org/docs/devel/sql-createpublication.html

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  2. Re: Privileges on PUBLICATION

    Euler Taveira <euler@eulerto.com> — 2022-05-09T18:44:45Z

    On Mon, May 9, 2022, at 11:09 AM, Antonin Houska wrote:
> Now that the user can specify rows and columns to be omitted from the logical
> replication [1], I suppose hiding rows and columns from the subscriber is an
> important use case. However, since the subscription connection user (i.e. the
> user specified in the CREATE SUBSCRIPTION ... CONNECTION ... command) needs
> SELECT permission on the replicated table (on the publication side), he can
> just use another publication (which has different filters or no filters at
> all) to get the supposedly-hidden data replicated.
The required privileges were not relaxed on publisher after the row filter and 
column list features. It is not just to "create another publication". Create
publications require CREATE privilege on databases (that is *not* granted to
PUBLIC).If you have an untrusted user that could bypass your rules about hidden
data, it is better to review your user privileges.

postgres=# CREATE ROLE foo REPLICATION LOGIN;
CREATE ROLE
postgres=# \c - foo
You are now connected to database "postgres" as user "foo".
postgres=> CREATE PUBLICATION pub1;
ERROR:  permission denied for database postgres

The documentation [1] says

"The role used for the replication connection must have the REPLICATION
attribute (or be a superuser)."

You can use role foo for the replication connection but role foo couldn't be a
superuser. In this case, even if role foo open a connection to database
postgres, a publication cannot be created due to lack of privileges.

> Don't we need privileges on publication (e.g GRANT USAGE ON PUBLICATION ...)
> now?
Maybe. We rely on CREATE privilege on databases right now. If you say that
GRANT USAGE ON PUBLICATION is just a command that will have the same effect as
REPLICATION property [1] has right now, I would say it won't. Are you aiming a
fine-grained access control on publisher?


[1] https://www.postgresql.org/docs/devel/logical-replication-security.html


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  3. Re: Privileges on PUBLICATION

    Amit Kapila <amit.kapila16@gmail.com> — 2022-05-10T03:49:48Z

    On Tue, May 10, 2022 at 12:16 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Mon, May 9, 2022, at 11:09 AM, Antonin Houska wrote:
>
> Now that the user can specify rows and columns to be omitted from the logical
> replication [1], I suppose hiding rows and columns from the subscriber is an
> important use case. However, since the subscription connection user (i.e. the
> user specified in the CREATE SUBSCRIPTION ... CONNECTION ... command) needs
> SELECT permission on the replicated table (on the publication side), he can
> just use another publication (which has different filters or no filters at
> all) to get the supposedly-hidden data replicated.
>
> The required privileges were not relaxed on publisher after the row filter and
> column list features. It is not just to "create another publication". Create
> publications require CREATE privilege on databases (that is *not* granted to
> PUBLIC).If you have an untrusted user that could bypass your rules about hidden
> data, it is better to review your user privileges.
>

Also, to create a subscription (which combines multiple publications
to bypass rules), a user must be a superuser. So, isn't that a
sufficient guarantee that users shouldn't be able to bypass such
rules?

-- 
With Regards,
Amit Kapila.

  4. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-10T08:02:58Z

    Euler Taveira <euler@eulerto.com> wrote:

> On Mon, May 9, 2022, at 11:09 AM, Antonin Houska wrote:
> 
>  Now that the user can specify rows and columns to be omitted from the logical
>  replication [1], I suppose hiding rows and columns from the subscriber is an
>  important use case. However, since the subscription connection user (i.e. the
>  user specified in the CREATE SUBSCRIPTION ... CONNECTION ... command) needs
>  SELECT permission on the replicated table (on the publication side), he can
>  just use another publication (which has different filters or no filters at
>  all) to get the supposedly-hidden data replicated.
> 
> The required privileges were not relaxed on publisher after the row filter and 
> column list features. It is not just to "create another publication". Create
> publications require CREATE privilege on databases (that is *not* granted to
> PUBLIC).If you have an untrusted user that could bypass your rules about hidden
> data, it is better to review your user privileges.
> 
> postgres=# CREATE ROLE foo REPLICATION LOGIN;
> CREATE ROLE
> postgres=# \c - foo
> You are now connected to database "postgres" as user "foo".
> postgres=> CREATE PUBLICATION pub1;
> ERROR:  permission denied for database postgres
> 
> The documentation [1] says
> 
> "The role used for the replication connection must have the REPLICATION
> attribute (or be a superuser)."
> 
> You can use role foo for the replication connection but role foo couldn't be a
> superuser. In this case, even if role foo open a connection to database
> postgres, a publication cannot be created due to lack of privileges.
> 
>  Don't we need privileges on publication (e.g GRANT USAGE ON PUBLICATION ...)
>  now?
> 
> Maybe. We rely on CREATE privilege on databases right now. If you say that
> GRANT USAGE ON PUBLICATION is just a command that will have the same effect as
> REPLICATION property [1] has right now, I would say it won't. Are you aiming a
> fine-grained access control on publisher?

The configuration I'm thinking of is multiple replicas reading data from the
same master.

For example, consider "foo" and "bar" roles, used by "subscr_foo" and
"subscr_bar" subscriptions respectively. (Therefore, both roles need the
REPLICATION option.) The subscriptions "subscr_foo" and "subscr_bar" are
located in "db_foo" and "db_bar" databases respectively.

On the master side, there are two publications: "pub_foo" and "pub_bar", to be
used by "subscr_foo" and "subscr_bar" subscriptions respectively. The
publications replicate the same table, but each with a different row filter.

The problem is that the admin of "db_foo" can add the "pub_bar" publication to
the "subscr_foo" subscription, and thus get the data that his "pub_foo" would
filter out. Likewise, the admin of "db_bar" can "steal" the data from
"pub_foo" by adding that publication to "subscr_bar".

In this case, the existing publications are misused, so the CREATE PUBLICATION
privileges do not help. Since the REPLICATION option of a role is
cluster-wide, but I need specific roles to be restricted to specific
publications, it can actually be called fine-grained access control as you
say.


> [1] https://www.postgresql.org/docs/devel/logical-replication-security.html

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  5. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-10T08:37:24Z

    Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Tue, May 10, 2022 at 12:16 AM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Mon, May 9, 2022, at 11:09 AM, Antonin Houska wrote:
> >
> > Now that the user can specify rows and columns to be omitted from the logical
> > replication [1], I suppose hiding rows and columns from the subscriber is an
> > important use case. However, since the subscription connection user (i.e. the
> > user specified in the CREATE SUBSCRIPTION ... CONNECTION ... command) needs
> > SELECT permission on the replicated table (on the publication side), he can
> > just use another publication (which has different filters or no filters at
> > all) to get the supposedly-hidden data replicated.
> >
> > The required privileges were not relaxed on publisher after the row filter and
> > column list features. It is not just to "create another publication". Create
> > publications require CREATE privilege on databases (that is *not* granted to
> > PUBLIC).If you have an untrusted user that could bypass your rules about hidden
> > data, it is better to review your user privileges.
> >
> 
> Also, to create a subscription (which combines multiple publications
> to bypass rules), a user must be a superuser. So, isn't that a
> sufficient guarantee that users shouldn't be able to bypass such
> rules?

My understanding is that the rows/columns filtering is a way for the
*publisher* to control which data is available to particular replica. From
this point of view, the publication privileges would just make the control
complete.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  6. Re: Privileges on PUBLICATION

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-05-12T05:46:56Z

    On 10.05.22 10:37, Antonin Houska wrote:
> My understanding is that the rows/columns filtering is a way for the
> *publisher*  to control which data is available to particular replica. From
> this point of view, the publication privileges would just make the control
> complete.

I think privileges on publications would eventually be useful.  But are 
you arguing that we need them for PG15 to make the new features usable 
safely?

  7. Re: Privileges on PUBLICATION

    Euler Taveira <euler@eulerto.com> — 2022-05-12T12:48:28Z

    On Tue, May 10, 2022, at 5:37 AM, Antonin Houska wrote:
> My understanding is that the rows/columns filtering is a way for the
> *publisher* to control which data is available to particular replica. From
> this point of view, the publication privileges would just make the control
> complete.
I agree. IMO it is a new feature. We already require high privilege for logical
replication. Hence, we expect the replication user to have access to all data.
Unfortunately, nobody mentioned about this requirement during the row filter /
column list development; someone could have written a patch for GRANT ... ON
PUBLICATION.

I understand your concern. Like I said in my last sentence in the previous
email: it is a fine-grained access control on the publisher. Keep in mind that
it will *only* work for non-superusers (REPLICATION attribute). It is not
exposing something that we didn't expose before. In this particular case, there
is no mechanism to prevent the subscriber to obtain data provided by the
various row filters if they know the publication names. We could probably add a
sentence to "Logical Replication > Security" section:

There is no privileges for publications. If you have multiple publications in a
database, a subscription can use all publications available.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  8. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-13T06:36:37Z

    Euler Taveira <euler@eulerto.com> wrote:

> On Tue, May 10, 2022, at 5:37 AM, Antonin Houska wrote:
> 
>  My understanding is that the rows/columns filtering is a way for the
>  *publisher* to control which data is available to particular replica. From
>  this point of view, the publication privileges would just make the control
>  complete.
> 
> I agree. IMO it is a new feature. We already require high privilege for logical
> replication. Hence, we expect the replication user to have access to all data.
> Unfortunately, nobody mentioned about this requirement during the row filter /
> column list development; someone could have written a patch for GRANT ... ON
> PUBLICATION.

I can try that for PG 16, unless someone is already working on it.

> I understand your concern. Like I said in my last sentence in the previous
> email: it is a fine-grained access control on the publisher. Keep in mind that
> it will *only* work for non-superusers (REPLICATION attribute). It is not
> exposing something that we didn't expose before. In this particular case, there
> is no mechanism to prevent the subscriber to obtain data provided by the
> various row filters if they know the publication names. We could probably add a
> sentence to "Logical Replication > Security" section:
> 
> There is no privileges for publications. If you have multiple publications in a
> database, a subscription can use all publications available.

Attached is my proposal. It tries to be more specific and does not mention the
absence of the privileges explicitly.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  9. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-13T06:38:55Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> On 10.05.22 10:37, Antonin Houska wrote:
> > My understanding is that the rows/columns filtering is a way for the
> > *publisher*  to control which data is available to particular replica. From
> > this point of view, the publication privileges would just make the control
> > complete.
> 
> I think privileges on publications would eventually be useful.  But are you
> arguing that we need them for PG15 to make the new features usable safely?

I didn't think that far, but user should be aware of the problem. My proposal
of documentation is in https://www.postgresql.org/message-id/5859.1652423797%40antos

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  10. Re: Privileges on PUBLICATION

    Euler Taveira <euler@eulerto.com> — 2022-05-13T19:28:45Z

    On Fri, May 13, 2022, at 3:36 AM, Antonin Houska wrote:
> Attached is my proposal. It tries to be more specific and does not mention the
> absence of the privileges explicitly.
You explained the current issue but say nothing about the limitation. This
information will trigger a question possibly in one of the MLs. IMO if you say
something like the sentence above at the end, it will make it clear why that
setup expose all data (there is no access control to publications) and
explicitly say there is a TODO here.

Additional privileges might be added to control access to table data in a
future version of <productname>PostgreSQL</productname>.

I also wouldn't use the warning tag because it fits in the same category as the
other restrictions listed in the page.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  11. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-18T09:16:10Z

    Antonin Houska <ah@cybertec.at> wrote:

> Euler Taveira <euler@eulerto.com> wrote:
> 
> > On Tue, May 10, 2022, at 5:37 AM, Antonin Houska wrote:
> > 
> >  My understanding is that the rows/columns filtering is a way for the
> >  *publisher* to control which data is available to particular replica. From
> >  this point of view, the publication privileges would just make the control
> >  complete.
> > 
> > I agree. IMO it is a new feature. We already require high privilege for logical
> > replication. Hence, we expect the replication user to have access to all data.
> > Unfortunately, nobody mentioned about this requirement during the row filter /
> > column list development; someone could have written a patch for GRANT ... ON
> > PUBLICATION.
> 
> I can try that for PG 16, unless someone is already working on it.

The patch is attached to this message.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  12. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-18T09:44:57Z

    Euler Taveira <euler@eulerto.com> wrote:

> On Fri, May 13, 2022, at 3:36 AM, Antonin Houska wrote:
> 
>  Attached is my proposal. It tries to be more specific and does not mention the
>  absence of the privileges explicitly.
> 
> You explained the current issue but say nothing about the limitation. This
> information will trigger a question possibly in one of the MLs. IMO if you say
> something like the sentence above at the end, it will make it clear why that
> setup expose all data (there is no access control to publications) and
> explicitly say there is a TODO here.
> 
> Additional privileges might be added to control access to table data in a
> future version of <productname>PostgreSQL</productname>.

I thought it sound too negative if absence of some feature was mentioned
explicitly. However it makes sense to be clear from technical point of view.

> I also wouldn't use the warning tag because it fits in the same category as the
> other restrictions listed in the page.

ok, please see the next version.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  13. Re: Privileges on PUBLICATION

    Euler Taveira <euler@eulerto.com> — 2022-05-18T18:04:04Z

    On Wed, May 18, 2022, at 6:16 AM, Antonin Houska wrote:
> The patch is attached to this message.
Great. Add it to the next CF. I'll review it when I have some spare time.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  14. Re: Privileges on PUBLICATION

    Euler Taveira <euler@eulerto.com> — 2022-05-18T18:19:29Z

    On Wed, May 18, 2022, at 6:44 AM, Antonin Houska wrote:
> ok, please see the next version.
The new paragraph looks good to me. I'm not sure if the CREATE PUBLICATION is
the right place to provide such information. As I suggested in a previous email
[1], you could add it to "Logical Replication > Security".

[1] https://postgr.es/m/d96103fe-99e2-4119-bd76-952d326b7539@www.fastmail.com


--
Euler Taveira
EDB   https://www.enterprisedb.com/

  15. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-05-19T18:40:55Z

    Euler Taveira <euler@eulerto.com> wrote:

> On Wed, May 18, 2022, at 6:16 AM, Antonin Houska wrote:
> 
>  The patch is attached to this message.
> 
> Great. Add it to the next CF. I'll review it when I have some spare time.

https://commitfest.postgresql.org/38/3641/

Thanks!

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  16. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-06-20T14:01:35Z

    Euler Taveira <euler@eulerto.com> wrote:

> --eeab359ad6094efd84562cddd7fb9e89
> Content-Type: text/plain
> 
> On Wed, May 18, 2022, at 6:44 AM, Antonin Houska wrote:
> > ok, please see the next version.
> The new paragraph looks good to me. I'm not sure if the CREATE PUBLICATION is
> the right place to provide such information. As I suggested in a previous email
> [1], you could add it to "Logical Replication > Security".

ok, I missed that. The next version moves the text there.

> [1] https://postgr.es/m/d96103fe-99e2-4119-bd76-952d326b7539@www.fastmail.com

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  17. Re: Privileges on PUBLICATION

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-01T13:23:51Z

    On 20.06.22 16:01, Antonin Houska wrote:
>> On Wed, May 18, 2022, at 6:44 AM, Antonin Houska wrote:
>>> ok, please see the next version.
>> The new paragraph looks good to me. I'm not sure if the CREATE PUBLICATION is
>> the right place to provide such information. As I suggested in a previous email
>> [1], you could add it to "Logical Replication > Security".
> 
> ok, I missed that. The next version moves the text there.

I have committed this patch that adds the additional documentation.

The CF entry is about privileges on publications.  Please rebase that 
patch and repost it so that the CF app and the CF bot are up to date.

  18. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-11-03T05:43:12Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> The CF entry is about privileges on publications.  Please rebase that patch
> and repost it so that the CF app and the CF bot are up to date.

The rebased patch (with regression tests added) is attached here.

There's still one design issue that I haven't mentioned yet: if the USAGE
privilege on a publication is revoked after the synchronization phase
completed, the missing privilege on a publication causes ERROR in the output
plugin. If the privilege is then granted, the error does not disappear because
the same (historical) snapshot we use to decode the failed data change again
is also used to check the privileges in the catalog, so the output plugin does
not see that the privilege has already been granted.

The only solution seems to be to drop the publication from the subscription
and add it again, or to drop and re-create the whole subscription. I haven't
added a note about this problem to the documentation yet, in case someone has
better idea how to approach the problem.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  19. Re: Privileges on PUBLICATION

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-03T15:49:51Z

    On 03.11.22 01:43, Antonin Houska wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
>> The CF entry is about privileges on publications.  Please rebase that patch
>> and repost it so that the CF app and the CF bot are up to date.
> 
> The rebased patch (with regression tests added) is attached here.

Some preliminary discussion:

What is the upgrade strategy?  I suppose the options are either that 
publications have a default acl that makes them publicly accessible, 
thus preserving the existing behavior by default, or pg_dump would need 
to create additional GRANT statements when upgrading from pre-PG16.  I 
don't see anything like either of these mentioned in the patch.  What is 
your plan?

You might be interested in this patch, which relates to yours: 
https://commitfest.postgresql.org/40/3955/

Looking at your patch, I would also like to find a way to refactor away 
the ExecGrant_Publication() function.  I'll think about that.

I think you should add some tests under src/test/regress/ for the new 
GRANT and REVOKE statements, just to have some basic coverage that it 
works.  sql/publication.sql would be appropriate, I think.

  20. Re: Privileges on PUBLICATION

    Amit Kapila <amit.kapila16@gmail.com> — 2022-11-04T04:29:43Z

    On Thu, Nov 3, 2022 at 11:12 AM Antonin Houska <ah@cybertec.at> wrote:
>
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> > The CF entry is about privileges on publications.  Please rebase that patch
> > and repost it so that the CF app and the CF bot are up to date.
>
> The rebased patch (with regression tests added) is attached here.
>
> There's still one design issue that I haven't mentioned yet: if the USAGE
> privilege on a publication is revoked after the synchronization phase
> completed, the missing privilege on a publication causes ERROR in the output
> plugin. If the privilege is then granted, the error does not disappear because
> the same (historical) snapshot we use to decode the failed data change again
> is also used to check the privileges in the catalog, so the output plugin does
> not see that the privilege has already been granted.
>

We have a similar problem even when publication is dropped/created.
The replication won't be able to proceed.

> The only solution seems to be to drop the publication from the subscription
> and add it again, or to drop and re-create the whole subscription. I haven't
> added a note about this problem to the documentation yet, in case someone has
> better idea how to approach the problem.
>

I think one possibility is that the user advances the slot used in
replication by using pg_replication_slot_advance() at or after the
location where the privilege is granted. Some other ideas have been
discussed in the thread [1], in particular, see email [2] and
discussion after that but we didn't reach any conclusion.

[1] - https://www.postgresql.org/message-id/CAHut%2BPvMbCsL8PAz1Qc6LNoL0Ag0y3YJtPVJ8V0xVXJOPb%2B0xw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAA4eK1JTwOAniPua04o2EcOXfzRa8ANax%3D3bpx4H-8dH7M2p%3DA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  21. Re: Privileges on PUBLICATION

    Amit Kapila <amit.kapila16@gmail.com> — 2022-11-04T04:31:02Z

    On Thu, Nov 3, 2022 at 9:19 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 03.11.22 01:43, Antonin Houska wrote:
> > Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> >
> >> The CF entry is about privileges on publications.  Please rebase that patch
> >> and repost it so that the CF app and the CF bot are up to date.
> >
> > The rebased patch (with regression tests added) is attached here.
>
> Some preliminary discussion:
>
> What is the upgrade strategy?  I suppose the options are either that
> publications have a default acl that makes them publicly accessible,
> thus preserving the existing behavior by default, or pg_dump would need
> to create additional GRANT statements when upgrading from pre-PG16.
>

I think making them publicly accessible is a better option.

-- 
With Regards,
Amit Kapila.

  22. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-11-04T07:28:50Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> On 03.11.22 01:43, Antonin Houska wrote:
> > Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> > 
> >> The CF entry is about privileges on publications.  Please rebase that patch
> >> and repost it so that the CF app and the CF bot are up to date.
> > The rebased patch (with regression tests added) is attached here.
> 
> Some preliminary discussion:
> 
> What is the upgrade strategy?  I suppose the options are either that
> publications have a default acl that makes them publicly accessible, 
> thus preserving the existing behavior by default, or pg_dump would need to
> create additional GRANT statements when upgrading from pre-PG16.  I don't see
> anything like either of these mentioned in the patch.  What is your plan?

So far I considered the first

> You might be interested in this patch, which relates to yours:
> https://commitfest.postgresql.org/40/3955/

ok, I'll check.

> Looking at your patch, I would also like to find a way to refactor away the
> ExecGrant_Publication() function.  I'll think about that.
> 
> I think you should add some tests under src/test/regress/ for the new GRANT
> and REVOKE statements, just to have some basic coverage that it works.
> sql/publication.sql would be appropriate, I think.

I thought about the whole concept a bit more and I doubt if the PUBLICATION
privilege is the best approach. In particular, the user specified in CREATE
SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
privilege on the tables replicated. So if the DBA excludes some columns from
the publication's column list and sets the (publication) privileges in such a
way that the user cannot get the column values via other publications, the
user still can connect to the database directly and get values of the excluded
columns.

As an alternative to the publication privileges, I think that the CREATE
SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
user on the individual columns contained in the publication column list, and
DROP SUBSCRIPTION would revoke that privilege.

Of course a question is what to do if the replication user already has that
privilege on some columns: either the CREATE SUBSCRIPTION command should raise
ERROR, or we should introduce a new privilege (e.g. ACL_SELECT_PUB) for this
purpose, which would effectivelly be ACL_SELECT, but hidden from the users of
GRANT / REVOKE.

If this approach was taken, the USAGE privilege on schema would need to be
granted / revoked in a similar way.

What do you think about that?

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  23. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-11-04T08:37:29Z

    Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Thu, Nov 3, 2022 at 11:12 AM Antonin Houska <ah@cybertec.at> wrote:
> >
> > Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> >
> > > The CF entry is about privileges on publications.  Please rebase that patch
> > > and repost it so that the CF app and the CF bot are up to date.
> >
> > The rebased patch (with regression tests added) is attached here.
> >
> > There's still one design issue that I haven't mentioned yet: if the USAGE
> > privilege on a publication is revoked after the synchronization phase
> > completed, the missing privilege on a publication causes ERROR in the output
> > plugin. If the privilege is then granted, the error does not disappear because
> > the same (historical) snapshot we use to decode the failed data change again
> > is also used to check the privileges in the catalog, so the output plugin does
> > not see that the privilege has already been granted.
> >
> 
> We have a similar problem even when publication is dropped/created.
> The replication won't be able to proceed.
> 
> > The only solution seems to be to drop the publication from the subscription
> > and add it again, or to drop and re-create the whole subscription. I haven't
> > added a note about this problem to the documentation yet, in case someone has
> > better idea how to approach the problem.
> >
> 
> I think one possibility is that the user advances the slot used in
> replication by using pg_replication_slot_advance() at or after the
> location where the privilege is granted. Some other ideas have been
> discussed in the thread [1], in particular, see email [2] and
> discussion after that but we didn't reach any conclusion.

Thanks for feedback. Regarding the publications, I'm not sure what the user
should expect if he revokes the permissions in the middle of processing. In
such a situation, he should be aware that some privileged data could already
have been replicated. My preference in such a situation would be to truncate
the table(s) on the subscriber side and to restart the replication, rather
than fix the replication and keep the subscriber's access to the leaked data.

> [1] - https://www.postgresql.org/message-id/CAHut%2BPvMbCsL8PAz1Qc6LNoL0Ag0y3YJtPVJ8V0xVXJOPb%2B0xw%40mail.gmail.com
> [2] - https://www.postgresql.org/message-id/CAA4eK1JTwOAniPua04o2EcOXfzRa8ANax%3D3bpx4H-8dH7M2p%3DA%40mail.gmail.com

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  24. Re: Privileges on PUBLICATION

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-11-04T17:17:17Z

    
> On Nov 4, 2022, at 12:28 AM, Antonin Houska <ah@cybertec.at> wrote:
> 
> I thought about the whole concept a bit more and I doubt if the PUBLICATION
> privilege is the best approach. In particular, the user specified in CREATE
> SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
> privilege on the tables replicated. So if the DBA excludes some columns from
> the publication's column list and sets the (publication) privileges in such a
> way that the user cannot get the column values via other publications, the
> user still can connect to the database directly and get values of the excluded
> columns.
> 
> As an alternative to the publication privileges, I think that the CREATE
> SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
> user on the individual columns contained in the publication column list, and
> DROP SUBSCRIPTION would revoke that privilege.
> 
> Of course a question is what to do if the replication user already has that
> privilege on some columns: either the CREATE SUBSCRIPTION command should raise
> ERROR, or we should introduce a new privilege (e.g. ACL_SELECT_PUB) for this
> purpose, which would effectivelly be ACL_SELECT, but hidden from the users of
> GRANT / REVOKE.
> 
> If this approach was taken, the USAGE privilege on schema would need to be
> granted / revoked in a similar way.

When you talk about a user needing to have privileges, it sounds like you mean privileges on the publishing database.  But then you talk about CREATE SUBSCRIPTION granting privileges, which would necessarily be on the subscriber database.  Can you clarify what you have in mind?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  25. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-11-04T17:37:31Z

    Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> > On Nov 4, 2022, at 12:28 AM, Antonin Houska <ah@cybertec.at> wrote:
> > 
> > I thought about the whole concept a bit more and I doubt if the PUBLICATION
> > privilege is the best approach. In particular, the user specified in CREATE
> > SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
> > privilege on the tables replicated. So if the DBA excludes some columns from
> > the publication's column list and sets the (publication) privileges in such a
> > way that the user cannot get the column values via other publications, the
> > user still can connect to the database directly and get values of the excluded
> > columns.
> > 
> > As an alternative to the publication privileges, I think that the CREATE
> > SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
> > user on the individual columns contained in the publication column list, and
> > DROP SUBSCRIPTION would revoke that privilege.
> > 
> > Of course a question is what to do if the replication user already has that
> > privilege on some columns: either the CREATE SUBSCRIPTION command should raise
> > ERROR, or we should introduce a new privilege (e.g. ACL_SELECT_PUB) for this
> > purpose, which would effectivelly be ACL_SELECT, but hidden from the users of
> > GRANT / REVOKE.
> > 
> > If this approach was taken, the USAGE privilege on schema would need to be
> > granted / revoked in a similar way.
> 
> When you talk about a user needing to have privileges, it sounds like you
> mean privileges on the publishing database.  But then you talk about CREATE
> SUBSCRIPTION granting privileges, which would necessarily be on the
> subscriber database.  Can you clarify what you have in mind?

Right, the privileges need to be added on the publishing side, but the user
that needs those privileges is specified on the subscription side. I didn't
think much in detail how it would work. The "subscription user" certainly
cannot connect to the publisher database and add grant the privileges to
itself. Perhaps some of the workers on the publisher side could do it on
startup.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  26. Re: Privileges on PUBLICATION

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-11T14:08:16Z

    On 04.11.22 08:28, Antonin Houska wrote:
> I thought about the whole concept a bit more and I doubt if the PUBLICATION
> privilege is the best approach. In particular, the user specified in CREATE
> SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
> privilege on the tables replicated. So if the DBA excludes some columns from
> the publication's column list and sets the (publication) privileges in such a
> way that the user cannot get the column values via other publications, the
> user still can connect to the database directly and get values of the excluded
> columns.

Why are the SELECT privileges needed?  Maybe that's something to think 
about and maybe change.

> As an alternative to the publication privileges, I think that the CREATE
> SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
> user on the individual columns contained in the publication column list, and
> DROP SUBSCRIPTION would revoke that privilege.

I think that approach is weird and unusual.  Privileges and object 
creation should be separate operations.

  27. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-11-14T11:07:48Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> On 04.11.22 08:28, Antonin Houska wrote:
> > I thought about the whole concept a bit more and I doubt if the PUBLICATION
> > privilege is the best approach. In particular, the user specified in CREATE
> > SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
> > privilege on the tables replicated. So if the DBA excludes some columns from
> > the publication's column list and sets the (publication) privileges in such a
> > way that the user cannot get the column values via other publications, the
> > user still can connect to the database directly and get values of the excluded
> > columns.
> 
> Why are the SELECT privileges needed?  Maybe that's something to think about
> and maybe change.

I haven't noticed an explanation in comments nor did I search in the mailing
list archives, but the question makes sense: the REPLICATION attribute of a
role is sufficient for streaming replication, so why should the logical
replication require additional privileges?

Technically the SELECT privilege is needed because the sync worker does
actually execute SELECT query on the published tables. However, I realize now
that it's not checked by the output plugin. Thus if SELECT is revoked from the
"subscription user" after the table has been synchronized, the replication
continues to work. So the necessity for the SELECT privilege might be an
omission rather than a design choice. (Even the documentation says that the
SELECT privilege is needed only for the initial synchronization [1], however
it does not tell why.)

> > As an alternative to the publication privileges, I think that the CREATE
> > SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
> > user on the individual columns contained in the publication column list, and
> > DROP SUBSCRIPTION would revoke that privilege.
> 
> I think that approach is weird and unusual.  Privileges and object creation
> should be separate operations.

ok. Another approach would be to skip the check for the SELECT privilege (as
well as the check for the USAGE privilege on the corresponding schema) if
given column is being accessed via a publication which has it on its column
list and if the subscription user has the USAGE privilege on that publication.

So far I wasn't sure if we can do that because, if pg_upgrade grants the USAGE
privilege on all publications to the "public" role, the DBAs who relied on the
SELECT privileges might not notice that any role having the REPLICATION
attribute can access all the published tables after the upgrade. (pg_upgrade
can hardly do anything else because it has no information on the "subscription
users", so it cannot convert the SELECT privilege on tables to the USAGE
privileges on publications.)

But now that I see that the logical replication doesn't check the SELECT
privilege properly anyway, I think we can get rid of it.


[1] https://www.postgresql.org/docs/current/logical-replication-security.html

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  28. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-11-29T14:35:12Z

    Antonin Houska <ah@cybertec.at> wrote:

> Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> > On 04.11.22 08:28, Antonin Houska wrote:
> > > I thought about the whole concept a bit more and I doubt if the PUBLICATION
> > > privilege is the best approach. In particular, the user specified in CREATE
> > > SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
> > > privilege on the tables replicated. So if the DBA excludes some columns from
> > > the publication's column list and sets the (publication) privileges in such a
> > > way that the user cannot get the column values via other publications, the
> > > user still can connect to the database directly and get values of the excluded
> > > columns.
> > 
> > Why are the SELECT privileges needed?  Maybe that's something to think about
> > and maybe change.
> 
> I haven't noticed an explanation in comments nor did I search in the mailing
> list archives, but the question makes sense: the REPLICATION attribute of a
> role is sufficient for streaming replication, so why should the logical
> replication require additional privileges?
> 
> Technically the SELECT privilege is needed because the sync worker does
> actually execute SELECT query on the published tables. However, I realize now
> that it's not checked by the output plugin. Thus if SELECT is revoked from the
> "subscription user" after the table has been synchronized, the replication
> continues to work. So the necessity for the SELECT privilege might be an
> omission rather than a design choice. (Even the documentation says that the
> SELECT privilege is needed only for the initial synchronization [1], however
> it does not tell why.)
> 
> > > As an alternative to the publication privileges, I think that the CREATE
> > > SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
> > > user on the individual columns contained in the publication column list, and
> > > DROP SUBSCRIPTION would revoke that privilege.
> > 
> > I think that approach is weird and unusual.  Privileges and object creation
> > should be separate operations.
> 
> ok. Another approach would be to skip the check for the SELECT privilege (as
> well as the check for the USAGE privilege on the corresponding schema) if
> given column is being accessed via a publication which has it on its column
> list and if the subscription user has the USAGE privilege on that publication.
> 
> So far I wasn't sure if we can do that because, if pg_upgrade grants the USAGE
> privilege on all publications to the "public" role, the DBAs who relied on the
> SELECT privileges might not notice that any role having the REPLICATION
> attribute can access all the published tables after the upgrade. (pg_upgrade
> can hardly do anything else because it has no information on the "subscription
> users", so it cannot convert the SELECT privilege on tables to the USAGE
> privileges on publications.)
> 
> But now that I see that the logical replication doesn't check the SELECT
> privilege properly anyway, I think we can get rid of it.

The attached version tries to do that - as you can see in 0001, the SELECT
privilege is not required for the walsender process.

I also added PUBLICATION_NAMES option to the COPY TO command so that the
publisher knows which publications are subject to the ACL check. Only data of
those publications are returned to the subscriber. (In the previous patch
version the ACL checks were performed on the subscriber side, but I that's not
ideal in terms of security.)

I also added the regression tests for publications, enhanced psql (the \dRp+
command) so that it displays the publication ACL and added a few missing
pieces of documentation.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  29. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2022-12-16T16:37:07Z

    Antonin Houska <ah@cybertec.at> wrote:

> Antonin Houska <ah@cybertec.at> wrote:
> 
> > Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> > 
> > > On 04.11.22 08:28, Antonin Houska wrote:
> > > > I thought about the whole concept a bit more and I doubt if the PUBLICATION
> > > > privilege is the best approach. In particular, the user specified in CREATE
> > > > SUBSCRIPTION ... CONNECTION ... (say "subscription user") needs to have SELECT
> > > > privilege on the tables replicated. So if the DBA excludes some columns from
> > > > the publication's column list and sets the (publication) privileges in such a
> > > > way that the user cannot get the column values via other publications, the
> > > > user still can connect to the database directly and get values of the excluded
> > > > columns.
> > > 
> > > Why are the SELECT privileges needed?  Maybe that's something to think about
> > > and maybe change.
> > 
> > I haven't noticed an explanation in comments nor did I search in the mailing
> > list archives, but the question makes sense: the REPLICATION attribute of a
> > role is sufficient for streaming replication, so why should the logical
> > replication require additional privileges?
> > 
> > Technically the SELECT privilege is needed because the sync worker does
> > actually execute SELECT query on the published tables. However, I realize now
> > that it's not checked by the output plugin. Thus if SELECT is revoked from the
> > "subscription user" after the table has been synchronized, the replication
> > continues to work. So the necessity for the SELECT privilege might be an
> > omission rather than a design choice. (Even the documentation says that the
> > SELECT privilege is needed only for the initial synchronization [1], however
> > it does not tell why.)
> > 
> > > > As an alternative to the publication privileges, I think that the CREATE
> > > > SUBSCRIPTION command could grant ACL_SELECT automatically to the subscription
> > > > user on the individual columns contained in the publication column list, and
> > > > DROP SUBSCRIPTION would revoke that privilege.
> > > 
> > > I think that approach is weird and unusual.  Privileges and object creation
> > > should be separate operations.
> > 
> > ok. Another approach would be to skip the check for the SELECT privilege (as
> > well as the check for the USAGE privilege on the corresponding schema) if
> > given column is being accessed via a publication which has it on its column
> > list and if the subscription user has the USAGE privilege on that publication.
> > 
> > So far I wasn't sure if we can do that because, if pg_upgrade grants the USAGE
> > privilege on all publications to the "public" role, the DBAs who relied on the
> > SELECT privileges might not notice that any role having the REPLICATION
> > attribute can access all the published tables after the upgrade. (pg_upgrade
> > can hardly do anything else because it has no information on the "subscription
> > users", so it cannot convert the SELECT privilege on tables to the USAGE
> > privileges on publications.)
> > 
> > But now that I see that the logical replication doesn't check the SELECT
> > privilege properly anyway, I think we can get rid of it.
> 
> The attached version tries to do that - as you can see in 0001, the SELECT
> privilege is not required for the walsender process.
> 
> I also added PUBLICATION_NAMES option to the COPY TO command so that the
> publisher knows which publications are subject to the ACL check. Only data of
> those publications are returned to the subscriber. (In the previous patch
> version the ACL checks were performed on the subscriber side, but I that's not
> ideal in terms of security.)
> 
> I also added the regression tests for publications, enhanced psql (the \dRp+
> command) so that it displays the publication ACL and added a few missing
> pieces of documentation.
> 

This is v4. The patch had to be rebased due to the commit 369f09e420.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  30. Re: Privileges on PUBLICATION

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-09T09:23:32Z

    On 16.12.22 17:37, Antonin Houska wrote:
> This is v4. The patch had to be rebased due to the commit 369f09e420.

I think what this patch set needs first of all is a comprehensive 
description of what it is trying to do, exactly what commands and 
behaviors it adds, what are some of the subtleties and corner cases, 
what are open issues and questions.  Some of that can be pieced together 
from this thread, but it should really be put in one place somewhere, 
ideally in the commit message and/or the documentation.  (The main 0002 
patch does not have any documentation.)  It looks like you have a lot of 
bases covered, but without a full description, it's difficult to tell.

Some points on the details:

* You can combine all five patches into one.  I don't think they are 
meant to be applied separately.  The 0001 looks like it was maybe meant 
to be used separately, but it's not clear.  Again, the overall 
description would help.

* There is a lot of code that is contingent on am_db_walsender.  We 
should avoid that.  In most cases, it doesn't seem necessary.  Or at 
least document the reasons.

* The term "aware" (of a publication ACL, of a relation) is used a bunch 
of times.  That's not a technical term, and the meaning of those phrases 
is not clear.  Make sure the documentation/comments are precise.

* I don't think using SPI is warranted here.  You can get the required 
information directly from the underlying functions.

* The places the privileges are ultimately checked is too unprincipled. 
The 0001 patch overrides a very low-level function, but the 0002 on the 
other hand checks the privileges by digging through the query structures 
by hand instead of letting the executor do it.  We need to find ways to 
handle that that is more consistent with what the code is currently 
doing instead of adding more layers to it above and below.

* The misc_sanity.out test output means you need to add a TOAST table to 
pg_publication.

  31. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2023-02-01T12:02:07Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> On 16.12.22 17:37, Antonin Houska wrote:
> > This is v4. The patch had to be rebased due to the commit 369f09e420.
> 
> I think what this patch set needs first of all is a comprehensive description
> of what it is trying to do, exactly what commands and behaviors it adds, what
> are some of the subtleties and corner cases, what are open issues and
> questions.  Some of that can be pieced together from this thread, but it
> should really be put in one place somewhere, ideally in the commit message
> and/or the documentation.  (The main 0002 patch does not have any
> documentation.)  It looks like you have a lot of bases covered, but without a
> full description, it's difficult to tell.
> 
> Some points on the details:
> 
> * You can combine all five patches into one.  I don't think they are meant to
>   be applied separately.  The 0001 looks like it was maybe meant to be used
>   separately, but it's not clear.  Again, the overall description would help.
> 
> * There is a lot of code that is contingent on am_db_walsender.  We should
>   avoid that.  In most cases, it doesn't seem necessary.  Or at least document
>  the reasons.
> 
> * The term "aware" (of a publication ACL, of a relation) is used a bunch of
>   times.  That's not a technical term, and the meaning of those phrases is not
>  clear.  Make sure the documentation/comments are precise.
> 
> * I don't think using SPI is warranted here.  You can get the required
>   information directly from the underlying functions.
> 
> * The places the privileges are ultimately checked is too unprincipled. The
>   0001 patch overrides a very low-level function, but the 0002 on the other
>   hand checks the privileges by digging through the query structures by hand
>   instead of letting the executor do it.  We need to find ways to handle that
>   that is more consistent with what the code is currently doing instead of
>  adding more layers to it above and below.
> 
> * The misc_sanity.out test output means you need to add a TOAST table to
>   pg_publication.
> 

Thanks for your review. Attached is a new version that tries to address your
findings.

I reworked the patch a bit, especially the handling of the PUBLICATION_NAMES
of the COPY TO command, so that compatibility with older subscribers is not
broken. The compatibility is actually the hardest part.

0001 only move some code into functions. I think it's better to do
this kind of thing separate so that the actual changes are easier to read.

The TODO comment in 0002 is related to [1].

[1] https://www.postgresql.org/message-id/3472.1675251957%40antos

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  32. Re: Privileges on PUBLICATION

    Gregory Stark (as CFM) <stark.cfm@gmail.com> — 2023-03-14T18:30:09Z

    FYI this looks like it needs a rebase due to a conflict in copy.c and
an offset in pgoutput.c.

Is there anything specific that still needs review or do you think
you've handled all Peter's concerns? In particular, is there "a
comprehensive description of what it is trying to do"? :)

  33. Re: Privileges on PUBLICATION

    Antonin Houska <ah@cybertec.at> — 2023-03-15T07:42:10Z

    Gregory Stark (as CFM) <stark.cfm@gmail.com> wrote:

> FYI this looks like it needs a rebase due to a conflict in copy.c and
> an offset in pgoutput.c.
> 
> Is there anything specific that still needs review or do you think
> you've handled all Peter's concerns? In particular, is there "a
> comprehensive description of what it is trying to do"? :)

I tried to improve the documentation and commit messages in v05. v06 (just
rebased) is attached.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  34. Re: Privileges on PUBLICATION

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-20T06:17:51Z

    On 14.03.23 19:30, Gregory Stark (as CFM) wrote:
> FYI this looks like it needs a rebase due to a conflict in copy.c and
> an offset in pgoutput.c.
> 
> Is there anything specific that still needs review or do you think
> you've handled all Peter's concerns? In particular, is there "a
> comprehensive description of what it is trying to do"? :)

The latest versions of the patch have pretty much addressed my initial 
comments.  The patch is structured and explained better now.  Most 
extraneous or incomplete changes have been addressed.

The problem now is that it's still a quite complicated patch that 
introduces a security feature.  It still touches a number of subsystems 
on different levels of abstraction.  This functionality is not of the 
kind, "if you don't use it it won't affect you", since it effectively 
pokes holes into the existing privileges checking in order to allow 
publication privileges checking to override it in some cases.  It will 
take significant effort to do a complete analysis and testing on whether 
it is secure and robust.  I don't think I will have time for that, and I 
don't think anyone will want to commit something like this at the last 
moment.

We have already taken a number of things from earlier patches and 
committed them separately as refactorings.  I don't see anything in the 
current patch anymore that we might want to take independently like that.

So in summary I think it would be best to keep this patch around for PG17.