Thread: Special role for subscriptions
Hi hackers! In postgresql 10 and 11 only superuser can create/alter subscriptions. If there was a special role (like pg_monitor), it would be more easy to grant control on subscriptions. I can make a patch if there are no objections against it.
Greetings, * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: > In postgresql 10 and 11 only superuser can create/alter subscriptions. > If there was a special role (like pg_monitor), it would be more easy to grant control on subscriptions. > I can make a patch if there are no objections against it. I think the short answer is 'yes, we should let non-superusers do that', but the longer answer is: What level of access makes sense for managing subscriptions? Should there be a way to say "user X is allowed to create a subscription for remote system Y, but only for tables that exist in schema Q"? My general feeling is 'yes', though, of course, I don't want to say that we have to have all of that before we move forward with allowing non-superusers to create subscriptions, but I do think we want to make sure that we have a well thought-out path for how to get from where we are now to a system which has a lot more granularity, and to do our best to try avoiding any paths that might paint us into a corner. Thanks! Stephen
Attachment
Hi! As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only. 03.11.2018, 19:20, "Stephen Frost" <sfrost@snowman.net>: > Greetings, > > * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: >> In postgresql 10 and 11 only superuser can create/alter subscriptions. >> If there was a special role (like pg_monitor), it would be more easy to grant control on subscriptions. >> I can make a patch if there are no objections against it. > > I think the short answer is 'yes, we should let non-superusers do that', > but the longer answer is: > > What level of access makes sense for managing subscriptions? Should > there be a way to say "user X is allowed to create a subscription for > remote system Y, but only for tables that exist in schema Q"? > > My general feeling is 'yes', though, of course, I don't want to say that > we have to have all of that before we move forward with allowing > non-superusers to create subscriptions, but I do think we want to make > sure that we have a well thought-out path for how to get from where we > are now to a system which has a lot more granularity, and to do our best > to try avoiding any paths that might paint us into a corner. > > Thanks! > > Stephen
Greetings, * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: > As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only. That's a nice idea but seems like we would want to have a way to filter what tables a subscription follows then..? Just failing if the publication publishes tables that we don't have access to or are not the owner of seems like a poor solution.. Thanks! Stephen
Attachment
Hi! I think we can add FOR TABLES clause for create/refresh subscription, for example: CREATE SUBSCRIPTION my_sub CONNECTION... PUBLICATION my_pub [WITH ...] [ FOR TABLES t1, t2 | ALL TABLES ]. ALL TABLES is avalibale only for superuser.FOR TABLES t1, t2 is available to owner of tables and superuser. 07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>: > Greetings, > > * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: >> As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only. > > That's a nice idea but seems like we would want to have a way to filter > what tables a subscription follows then..? Just failing if the > publication publishes tables that we don't have access to or are not the > owner of seems like a poor solution.. > > Thanks! > > Stephen -------- Ефимкин Евгений
Hi!
In order to support create subscription from non-superuser, we need to make it possible to choose tables on the
subscriberside:
1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
```
CREATE SUBSCRIPTION subscription_name
CONNECTION 'conninfo'
PUBLICATION publication_name [, ...]
[ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
[ WITH ( subscription_parameter [= value] [, ... ] ) ]
```
... where `FOR ALL TABLES` is only allowed for superuser.
and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?)
2. Each subscription should have "all tables" attribute.
For example via a new column in pg_subscription "suballtables".
3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
```
ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data];
ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
```
4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. The
checkis ommited if subscription owner is superuser.
5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuser
owner,we should filter tables which owner is not subscription's owner or maybe we need to raise error?
What do you think about it? Any objections?
07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>:
> Greetings,
>
> * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:
>> As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.
>
> That's a nice idea but seems like we would want to have a way to filter
> what tables a subscription follows then..? Just failing if the
> publication publishes tables that we don't have access to or are not the
> owner of seems like a poor solution..
>
> Thanks!
>
> Stephen
--------
Ефимкин Евгений
Hello! I started work on patch (draft attached). Draft has changes related only to `CREATE SUBSCRIPTION`. I also introduce a new status (DEFFERED) for tables in `FOR TABLE` clause (but not in publication). New column in pg_subscription (suballtables) will be used in `REFRESH` clause 09.11.2018, 15:24, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: > Hi! > In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriberside: > 1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`: > ``` > CREATE SUBSCRIPTION subscription_name > CONNECTION 'conninfo' > PUBLICATION publication_name [, ...] > [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ] > [ WITH ( subscription_parameter [= value] [, ... ] ) ] > ``` > ... where `FOR ALL TABLES` is only allowed for superuser. > and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?) > > 2. Each subscription should have "all tables" attribute. > For example via a new column in pg_subscription "suballtables". > > 3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`: > ``` > ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data]; > ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name; > ``` > 4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. The checkis ommited if subscription owner is superuser. > 5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuser owner,we should filter tables which owner is not subscription's owner or maybe we need to raise error? > > What do you think about it? Any objections? > > 07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>: >> Greetings, >> >> * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: >>> As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only. >> >> That's a nice idea but seems like we would want to have a way to filter >> what tables a subscription follows then..? Just failing if the >> publication publishes tables that we don't have access to or are not the >> owner of seems like a poor solution.. >> >> Thanks! >> >> Stephen > > -------- > Ефимкин Евгений -------- Ефимкин Евгений
Attachment
Hello! New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION` forown tables. 14.11.2018, 18:10, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: > Hello! > I started work on patch (draft attached). Draft has changes related only to `CREATE SUBSCRIPTION`. > I also introduce a new status (DEFFERED) for tables in `FOR TABLE` clause (but not in publication). > New column in pg_subscription (suballtables) will be used in `REFRESH` clause > > 09.11.2018, 15:24, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: >> Hi! >> In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriberside: >> 1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`: >> ``` >> CREATE SUBSCRIPTION subscription_name >> CONNECTION 'conninfo' >> PUBLICATION publication_name [, ...] >> [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ] >> [ WITH ( subscription_parameter [= value] [, ... ] ) ] >> ``` >> ... where `FOR ALL TABLES` is only allowed for superuser. >> and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?) >> >> 2. Each subscription should have "all tables" attribute. >> For example via a new column in pg_subscription "suballtables". >> >> 3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`: >> ``` >> ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data]; >> ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name; >> ``` >> 4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. Thecheck is ommited if subscription owner is superuser. >> 5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuserowner, we should filter tables which owner is not subscription's owner or maybe we need to raise error? >> >> What do you think about it? Any objections? >> >> 07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>: >>> Greetings, >>> >>> * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: >>>> As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only. >>> >>> That's a nice idea but seems like we would want to have a way to filter >>> what tables a subscription follows then..? Just failing if the >>> publication publishes tables that we don't have access to or are not the >>> owner of seems like a poor solution.. >>> >>> Thanks! >>> >>> Stephen >> >> -------- >> Ефимкин Евгений > > -------- > Ефимкин Евгений -------- Ефимкин Евгений
Attachment
Hello! I wrote some tests(it's just 01_rep_changes.pl but for non superuser) and fix `DROP TABLE` from subscription. Now old andnew tests pass. 22.11.2018, 16:23, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: > Hello! > New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION` forown tables. > > 14.11.2018, 18:10, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: >> Hello! >> I started work on patch (draft attached). Draft has changes related only to `CREATE SUBSCRIPTION`. >> I also introduce a new status (DEFFERED) for tables in `FOR TABLE` clause (but not in publication). >> New column in pg_subscription (suballtables) will be used in `REFRESH` clause >> >> 09.11.2018, 15:24, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: >>> Hi! >>> In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriberside: >>> 1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`: >>> ``` >>> CREATE SUBSCRIPTION subscription_name >>> CONNECTION 'conninfo' >>> PUBLICATION publication_name [, ...] >>> [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ] >>> [ WITH ( subscription_parameter [= value] [, ... ] ) ] >>> ``` >>> ... where `FOR ALL TABLES` is only allowed for superuser. >>> and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?) >>> >>> 2. Each subscription should have "all tables" attribute. >>> For example via a new column in pg_subscription "suballtables". >>> >>> 3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`: >>> ``` >>> ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data]; >>> ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name; >>> ``` >>> 4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. Thecheck is ommited if subscription owner is superuser. >>> 5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuserowner, we should filter tables which owner is not subscription's owner or maybe we need to raise error? >>> >>> What do you think about it? Any objections? >>> >>> 07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>: >>>> Greetings, >>>> >>>> * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote: >>>>> As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only. >>>> >>>> That's a nice idea but seems like we would want to have a way to filter >>>> what tables a subscription follows then..? Just failing if the >>>> publication publishes tables that we don't have access to or are not the >>>> owner of seems like a poor solution.. >>>> >>>> Thanks! >>>> >>>> Stephen >>> >>> -------- >>> Ефимкин Евгений >> >> -------- >> Ефимкин Евгений > > -------- > Ефимкин Евгений -------- Ефимкин Евгений
Attachment
Hi, Evgeniy! Thanks for working on the feature. > 28 нояб. 2018 г., в 21:41, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): > > Hello! > I wrote some tests(it's just 01_rep_changes.pl but for non superuser) and fix `DROP TABLE` from subscription. Now old andnew tests pass. > > 22.11.2018, 16:23, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: >> Hello! >> New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION`for own tables. I've looked into the patch. The code looks good and coherent to nearby code. There are no docs, obviously, there is WiP. I've got few questions: 1. How will the subscription work for inherited tables? Do we need tests for that? 2. ALTER PUBLICATION has ADD\DROP and SET. Should we add SET too? Or is there a reason not to do that? 3. Message "Must be superuser to create FOR ALL TABLES subscriptions" seems a bit strange to me. Also, this literal is embeddedinto translations. I do not know how we deal with it, how do we deal for example with "måste vara superanvändareför att skapa prenumerationer" or "для создания подписок нужно быть суперпользователем"? Where do we insertFOR ALL TABLES? 4. How does default behavior differ from FOR ALL TABLES? 5. Can we alter subscription FOR ALL TABLES? Drop some tables out of the subscription? Best regards, Andrey Borodin.
Hello! Thank you for questions! > I've got few questions: > 1. How will the subscription work for inherited tables? Do we need tests for that? For subscription created with `FOR TABLE` we can't support inherit tables because subscriber don't know anything about inherit.In new patch i remove `ONLY` for `FOR TABLE` in subscription related statements > 2. ALTER PUBLICATION has ADD\DROP and SET. Should we add SET too? Or is there a reason not to do that? Added it in new patch > 3. Message "Must be superuser to create FOR ALL TABLES subscriptions" seems a bit strange to me. Also, this literal isembedded into translations. I do not know how we deal with it, how do we deal for example with "måste vara superanvändareför att skapa prenumerationer" or "для создания подписок нужно быть суперпользователем"? Where do we insertFOR ALL TABLES? I add hint `Use CREATE SUBSCRIPTION ... FOR TABLE ...` > 4. How does default behavior differ from FOR ALL TABLES? The same with default implementation > 5. Can we alter subscription FOR ALL TABLES? Drop some tables out of the subscription? For subscriptions created with `FOR ALL TABLES` (default), you can't change subscribed tables by `ALTER SUBSCRIPTION ADD/DROP`table, you should use `ALTER SUBSCRIPTION REFRESH PUBLICATION` And i don't know how do export for user created subscriptions, because now non superuser can't select subconninfo column 03.12.2018, 09:06, "Andrey Borodin" <x4mmm@yandex-team.ru>: > Hi, Evgeniy! > > Thanks for working on the feature. >> 28 нояб. 2018 г., в 21:41, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): >> >> Hello! >> I wrote some tests(it's just 01_rep_changes.pl but for non superuser) and fix `DROP TABLE` from subscription. Now oldand new tests pass. >> >> 22.11.2018, 16:23, "Evgeniy Efimkin" <efimkin@yandex-team.ru>: >>> Hello! >>> New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION`for own tables. > > I've looked into the patch. The code looks good and coherent to nearby code. > There are no docs, obviously, there is WiP. > > I've got few questions: > 1. How will the subscription work for inherited tables? Do we need tests for that? > 2. ALTER PUBLICATION has ADD\DROP and SET. Should we add SET too? Or is there a reason not to do that? > 3. Message "Must be superuser to create FOR ALL TABLES subscriptions" seems a bit strange to me. Also, this literal isembedded into translations. I do not know how we deal with it, how do we deal for example with "måste vara superanvändareför att skapa prenumerationer" or "для создания подписок нужно быть суперпользователем"? Where do we insertFOR ALL TABLES? > 4. How does default behavior differ from FOR ALL TABLES? > 5. Can we alter subscription FOR ALL TABLES? Drop some tables out of the subscription? > > Best regards, Andrey Borodin. -------- Ефимкин Евгений
Attachment
Hi! Thanks for working on the patch! > 6 дек. 2018 г., в 21:47, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): > And i don't know how do export for user created subscriptions, because now non superuser can't select subconninfo column BTW, docs say >When dumping logical replication subscriptions, pg_dump will generate CREATE SUBSCRIPTION commands that use the NOCONNECToption But I do not see NOCONNECT anywhere and specifically in CREATE SUBSCRIPTION section. There is only "WITH (connect = false)". And "with (connect = false)" (as in dumpSubscription() now) dump will be successfully restored. pg_dump now checks for superuser at getSubscriptions(), I think you should patch this too. In current form, from my POV, most important issue of this patch is complete lack of doc adjustment. And you can fix "NOCONNECT"thing there too. Please, avoid top-posting (quoting whole message under your reply), this makes harder to read archives at postgresql.org And also please send patches with version number to distinguish old and new versions. Best regards, Andrey Borodin.
Hello! In latest patch i removed `FOR ALL TABLES` clause and `alltables` parameter, now it's look more simple. Add new system view pg_user_subscirption to allow non-superuser use pg_dump and select addition column from pg_subscrption Changes docs. Thanks! -------- Ефимкин Евгений
Attachment
Hi!
> 27 дек. 2018 г., в 12:54, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):
>
> In latest patch i removed `FOR ALL TABLES` clause and `alltables` parameter, now it's look more simple.
> Add new system view pg_user_subscirption to allow non-superuser use pg_dump and select addition column from
pg_subscrption
> Changes docs.
I've reviewed patch again, here are my notes:
1. In create_subscription.sgml and some others. "All tables listed in clause must be present in publication" I think is
betterto write "All tables listed in clause must present in the publication". But I'm not a native speaker, just looks
thatit'd be good if someone proofread docs..
2. New view should be called pg_user_subscription or pg_user_subscriptions? Nearby views are plural e.g.
pg_publication_tables.
3. I do not know how will this view get into the db during pg_upgrade. Is it somehow handled?
4. In subscriptioncmds.c :
>if (stmt->tables&&!connect)
some spaces needed to make AND readable
5. Same file in CreateSubscription() there's foreach collecting tablesiods. This oids are necessary only in on branch
offollowing
>if (stmt->tables)
May be we should refactor this, move tablesiods closer to the place where they are used?
6. In AlterSubscription_set_table() and AlterSubscription_drop_table() palloced memory is not free()'d. Is it OK or is
ita leak?
7.
> errmsg("table \"%s.%s\" not in preset subscription"
Should it be "table does not present in subscription"?
Besides this, patch looks good to me.
Thanks for working on this!
Best regards, Andrey Borodin.
Hi! 1. done 2. rename to pg_user_subscriptions 3. by pg_dump, i checked upgrade from 10 to 12devel, it's work fine 4. done 5. done 6. I took it from AlterSubscription_refresh, in that function no any free() 7. done -------- Ефимкин Евгений
Attachment
Hi!
> 29 янв. 2019 г., в 14:51, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):
>
> <subscription_v2.patch>
Thanks for the next version.
1. In tests the code for "sub reset_pg_hba" is taken from authentication tests (which is fine), but comments are
omitted.Let's take them too?
2. 011_rep_changes_nonsuperuser.pl seems a lot like 001_rep_changes.pl with auth adjustments
I think it is OK, but if you know some clever way to refactor that it would be cool. We could avoid duplication of 200
codeline of tests or so.
3. I'm not sure, but I'd add some articles here: "All tables listed in _a_ clause must be present in _the_
publication."
"clauses will remove ? table from ? subscription"
4.
>qsort(subrel_local_oids, list_length(subrel_states),
sizeof(Oid), oid_cmp);
is indented two times differently, but I think this will be fixed by pg_indent. There are some other indentation
issues.
/* Load the library providing us libpq calls. */
/* Check the connection info string. */
load_file("libpqwalreceiver", false);
walrcv_check_conninfo(stmt->conninfo);
Here 2nd comment jumped a line up. And there are superfluous new line in that code block.
Random newline added after
>errmsg("ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions")));
5. In regression test we only subtract test (fail for nonsuperuser). Can we add some test there too?
All these are merely cosmetical issues. I believe after addressing these we can switch patch to Ready For Committer.
Best regards, Andrey Borodin.
Hi! Thanks for comments 1. fixed 2. in non-superuser we have to use authorization, and FOR table clause, i don't known how merge both files. 3. fixed 4. fixed and run pgindent 5. add some new cases in regression test -------- Efimkin Evgeny
Attachment
Hi! > 11 февр. 2019 г., в 16:30, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): > <subscription_v3.patch> The patch seems good to me but cfbot is not happy. Can you please investigate what's wrong with this build? https://travis-ci.org/postgresql-cfbot/postgresql/builds/492912868 Also, I'm not sure we should drop this lines from docs: > The subscription apply process will run in the local database with the > privileges of a superuser. Thanks! Best regards, Andrey Borodin.
Hi! Thanks for comments! I fixed build and return lines about subscription apply process in doc -------- Efimkin Evgeny
Attachment
Hi! > 14 февр. 2019 г., в 20:04, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): > > <subscription_v4.patch> I've made some more iterations looking for ideas how to improve the patch and found non. Code style, docs, tests, make-check worlds, bit status, everything seems OK. A little bit of copied code from dblink (thereis no problem like CVE-2018-10915, or is it?) and copied tests. I'm inclined to mark the patch as RFC if there is no objections. May be we could also ask some input from cloud managed PostgreSQL providers, what they think about this patch? This patch,actually, is aimed at easing moving to the cloud DB where user has no superuser privileges. Best regards, Andrey Borodin.
On Sun, Feb 17, 2019 at 03:33:12PM +0500, Andrey Borodin wrote:
> I've made some more iterations looking for ideas how to improve the
> patch and found non.
> Code style, docs, tests, make-check worlds, bit status, everything
> seems OK. A little bit of copied code from dblink (there is no
> problem like CVE-2018-10915, or is it?) and copied tests.
> I'm inclined to mark the patch as RFC if there is no objections.
- To create a subscription, the user must be a superuser.
+ To add tables to a subscription, the user must have ownership rights on the
+ table.
[...]
+ /* must have CREATE privilege on database */
+ aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(), ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_DATABASE,
+ get_database_name(MyDatabaseId));
So this means that we degrade subscription creation requirement from
being a superuser to someone having CREATE rights on a given
database? The documentation is inconsistent with what the code does.
And it is possible for a user with CREATE rights on a given database
to add tables if he is an owner of them.
The documentation of GRANT needs to be updated: CREATE rights on a
database allows one to create subscriptions, and not only schemas and
publications.
+static void
+libpqrcv_security_check(WalReceiverConn *conn)
+{
+ if (!superuser())
+ {
+ if (!PQconnectionUsedPassword(conn->streamConn))
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser cannot connect if the
server does not request a password."),
+ errhint("Target server's authentication method
must be changed.")));
I don't understand this requirement. There are a bunch of
password-less, still secured authentication that Postgres can use
depending on the situations. To name one: peer. So this concept
design looks rather broken to me.
+ if (is_superuser(fout) && fout->remoteVersion < 120000)
+ {
+ appendPQExpBuffer(query,
+ "SELECT s.tableoid, s.oid, s.subname,"
+ "(%s s.subowner) AS rolname, "
+ " s.subconninfo, s.subslotname, s.subsynccommit, "
+ " s.subpublications "
+ "FROM pg_subscription s "
+ "WHERE s.subdbid = (SELECT oid FROM pg_database"
+ " WHERE datname = current_database())",
+ username_subquery);
+ }
+ else
+ {
+ appendPQExpBuffer(query,
+ "SELECT s.tableoid, s.oid, s.subname,"
+ "(%s s.subowner) AS rolname, "
+ " us.subconninfo, s.subslotname, s.subsynccommit, "
+ " s.subpublications "
+ "FROM pg_subscription s join pg_user_subscriptions us ON (s.oid=us.oid) "
+ "WHERE s.subdbid = (SELECT oid FROM pg_database"
+ " WHERE datname =
current_database())",
+ username_subquery);
+ }
Access to pg_subcription is still forbidden to non superusers, even
with the patch. Shouldn't a user who has CREATE rights be able to
dump his/her subscriptions?
There is zero documentation about pg_user_subscriptions.
+ if (stmt->tables && !connect)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot create subscription with connect =
false and FOR TABLE")));
+ }
Why? Cannot you store in catalogs the tables which can be used with a
subscription, and then reuse the table list when connecting later.
> May be we could also ask some input from cloud managed PostgreSQL
> providers, what they think about this patch? This patch, actually,
> is aimed at easing moving to the cloud DB where user has no
> superuser privileges.
I find the concept behind this patch a bit confusing, and honestly I
don't think that adding an extra per-relation filtering on the target
server is a concept which compiles well with the existing logical
replication because it is already possible to assign a subset of
tables on the source, and linking a subscription to a publication means
that this subset of tables will be used. Something which is more
simple, and that we could consider is to lower the requirement for
subscription creation to database owners, still this means that we give
a mean to any database owner to trigger connections remote instances
and spawn extra processes. This deserves more discussion, and there
is the dump case which is not really covered.
--
Michael
Attachment
On Fri, 2018-11-09 at 15:24 +0300, Evgeniy Efimkin wrote:
> Hi!
> In order to support create subscription from non-superuser, we need
> to make it possible to choose tables on the subscriber side:
I'd like to know more about the reasoning here. This thread started out
by suggesting a new special role that can be used for subscriptions,
but now it's changing the way subscription happens.
* What are the most important use cases here? Are we just trying to
avoid the unnecessary use of superuser, or is there a real use case for
subscribing to a subset of a publication?
* What are all the reasons CREATE SUBSCRIPTION currently requires
superuser?
* Is the original idea of a special role still viable?
Regards,
Jeff Davis
On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote: > * Is the original idea of a special role still viable? In my opinion, that part may be valuable. The latest patches proposed change the way tables are filtered and listed on the subscription side, lowering the permission to spawn a new thread and to connect to a publication server by just being a database owner instead of being a superuser, and that's quite a gap. -- Michael
Attachment
Hi! Thank you for comments, I’ll fix all inconsistencies in documentation. > I don't understand this requirement. There are a bunch of > password-less, still secured authentication that Postgres can use > depending on the situations. To name one: peer. So this concept > design looks rather broken to me. It does look a bit awkward, maybe — unless you account for details. For instance, you mention peer auth. Suppose we allow it, and there is nothing stopping the user from gaining superuser privilegesby connecting locally. Also, same requirements are in effect for FDW. > Access to pg_subcription is still forbidden to non superusers, even > with the patch. Shouldn't a user who has CREATE rights be able to > dump his/her subscriptions? > > There is zero documentation about pg_user_subscriptions. > Why? Cannot you store in catalogs the tables which can be used with a > subscription, and then reuse the table list when connecting later. That was my first thought too, but that approach made the implementation far too confusing. For insance, it requred an introductionof a new status for sync worker and a new statement to enable sync on particular tables. > I find the concept behind this patch a bit confusing, and honestly I > don't think that adding an extra per-relation filtering on the target > server is a concept which compiles well with the existing logical > replication because it is already possible to assign a subset of > tables on the source, and linking a subscription to a publication means > that this subset of tables will be used. Something which is more > simple, and that we could consider is to lower the requirement for > subscription creation to database owners, still this means that we give > a mean to any database owner to trigger connections remote instances > and spawn extra processes. This deserves more discussion, and there > is the dump case which is not really covered. -------- Efimkin Evgeny
Hi! > * What are the most important use cases here? Are we just trying to > avoid the unnecessary use of superuser, or is there a real use case for > subscribing to a subset of a publication? For instance in target database we do not have permission on some table used in publication, but we still CREATE SUBSCRIPTION for owned tables. > * What are all the reasons CREATE SUBSCRIPTION currently requires > superuser? I'm not sure, but it seems like only superuser have rights on all tables. I can't find any restrictions. > * Is the original idea of a special role still viable? yes, i wrote simple patch. Role create externally, but it can be system role. -------- Efimkin Evgeny
Attachment
Hi! I think it's good idea to able create subscription for database owner, but owner do not have permission on all tables indatabase. At the begging Stephen Frost said about table filter at subscription side. https://www.postgresql.org/message-id/20181106215244.GH18594%40tamriel.snowman.net I can add additional check in create subscription for database owner. -------- Efimkin Evgeny
Em seg, 4 de mar de 2019 às 03:55, Michael Paquier
<michael@paquier.xyz> escreveu:
>
> On Sun, Feb 17, 2019 at 03:33:12PM +0500, Andrey Borodin wrote:
> > I've made some more iterations looking for ideas how to improve the
> > patch and found non.
> > Code style, docs, tests, make-check worlds, bit status, everything
> > seems OK. A little bit of copied code from dblink (there is no
> > problem like CVE-2018-10915, or is it?) and copied tests.
> > I'm inclined to mark the patch as RFC if there is no objections.
>
> - To create a subscription, the user must be a superuser.
> + To add tables to a subscription, the user must have ownership rights on the
> + table.
> [...]
> + /* must have CREATE privilege on database */
> + aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(), ACL_CREATE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, OBJECT_DATABASE,
> + get_database_name(MyDatabaseId));
> So this means that we degrade subscription creation requirement from
> being a superuser to someone having CREATE rights on a given
> database? The documentation is inconsistent with what the code does.
> And it is possible for a user with CREATE rights on a given database
> to add tables if he is an owner of them.
>
> The documentation of GRANT needs to be updated: CREATE rights on a
> database allows one to create subscriptions, and not only schemas and
> publications.
>
> +static void
> +libpqrcv_security_check(WalReceiverConn *conn)
> +{
> + if (!superuser())
> + {
> + if (!PQconnectionUsedPassword(conn->streamConn))
> + ereport(ERROR,
> + (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
> + errmsg("password is required"),
> + errdetail("Non-superuser cannot connect if the
> server does not request a password."),
> + errhint("Target server's authentication method
> must be changed.")));
>
> I don't understand this requirement. There are a bunch of
> password-less, still secured authentication that Postgres can use
> depending on the situations. To name one: peer. So this concept
> design looks rather broken to me.
>
> + if (is_superuser(fout) && fout->remoteVersion < 120000)
> + {
> + appendPQExpBuffer(query,
> + "SELECT s.tableoid, s.oid, s.subname,"
> + "(%s s.subowner) AS rolname, "
> + " s.subconninfo, s.subslotname, s.subsynccommit, "
> + " s.subpublications "
> + "FROM pg_subscription s "
> + "WHERE s.subdbid = (SELECT oid FROM pg_database"
> + " WHERE datname = current_database())",
> + username_subquery);
> + }
> + else
> + {
> + appendPQExpBuffer(query,
> + "SELECT s.tableoid, s.oid, s.subname,"
> + "(%s s.subowner) AS rolname, "
> + " us.subconninfo, s.subslotname, s.subsynccommit, "
> + " s.subpublications "
> + "FROM pg_subscription s join pg_user_subscriptions us ON (s.oid=us.oid) "
> + "WHERE s.subdbid = (SELECT oid FROM pg_database"
> + " WHERE datname =
> current_database())",
> + username_subquery);
> + }
> Access to pg_subcription is still forbidden to non superusers, even
> with the patch. Shouldn't a user who has CREATE rights be able to
> dump his/her subscriptions?
>
> There is zero documentation about pg_user_subscriptions.
>
> + if (stmt->tables && !connect)
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("cannot create subscription with connect =
> false and FOR TABLE")));
> + }
> Why? Cannot you store in catalogs the tables which can be used with a
> subscription, and then reuse the table list when connecting later.
>
> > May be we could also ask some input from cloud managed PostgreSQL
> > providers, what they think about this patch? This patch, actually,
> > is aimed at easing moving to the cloud DB where user has no
> > superuser privileges.
>
> I find the concept behind this patch a bit confusing, and honestly I
> don't think that adding an extra per-relation filtering on the target
> server is a concept which compiles well with the existing logical
> replication because it is already possible to assign a subset of
> tables on the source, and linking a subscription to a publication means
> that this subset of tables will be used. Something which is more
> simple, and that we could consider is to lower the requirement for
> subscription creation to database owners, still this means that we give
> a mean to any database owner to trigger connections remote instances
> and spawn extra processes. This deserves more discussion, and there
> is the dump case which is not really covered.
>
The proposed feature changes from an special role for subscription to
filtering relations on the target server. The publish-subscribe model
was designed in a way that a replication set in the publisher was sent
to subscriber if you provide one or more publications. It was proposed
that some relations will be ignored in the publisher despite of
publication definition if you define a filter in the subscription
(i.e. a subset of the replication set). Why don't you create another
publication with the right relations? How would ALTER SUBSCRIPTION foo
REFRESH PUBLICATION work? I'm not convinced that adding a subset at
the subscriber side is the way to go. It will complicate the
publish-subscribe model and it does not solve the original problem:
relax CREATE SUBSCRIPTION permission.
The original subject (special role for subscription) deserves more
discussion. That role (currently superuser) guarantees that the apply
works and also gives you a sense of security because subscriber will
do critical things in the publisher (like a replication slot that
could lead to an out of space or read sensible data that that table
owner or database owner will not have access to). While in the
physical replication the replication role does not deal directly with
data, logical replication role should have access to data to apply the
modifications. Even if we relax the superuser to unprivileged role for
logical replication, there could be scenarios that that role can be
used to physical replication but not for logical replication because
of some requirements (such as that role could replicate but couldn't
access data); in this case, creating separate roles should work.
Moreover, logical replication role has a special power: modifies
relations that are replicated even if it does not have the GRANT to do
it (only superusers have this). However, if we provide a system role
pg_replication and relax create subscription to it. Then every
pg_replication role member could create subscription but it is up to
DBA granting access to all subscriber relations (permissions should be
checked once replication worker starts).
> Michael
> -----BEGIN PGP SIGNATURE-----
>
> iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlx8y7YACgkQnvQgOdby
> QH121A/+MxqokRFOYlXZAJGYa8BTvzf6qHcojv5MUg+l0r5kzTawaDoIzpVf2MLW
> +w/9GNeanPpFWcOrlZss+4IdzfM3G2/vEKf2bOgKxK4rL5ZSqKf+6KP3R5LzyrCj
> UAmz4/+AzIdNausRzseSH6uQd5aswU6ehpASRAuHNFjL0YoLSwrsQu11uE+y4fbv
> rGttqtsoqDSXPuVrTz1FpiLM7jokdOKTJGERfy5W2ojJeHMSzfC6WOxIt5wLciv7
> MGiOhTRRMliSaYlH2aPKzmrq3YNhr439PX8ApwcLeYkAg1Wlt95TgYdew7nKNakC
> KqMdv2Z5PH+75HRD4pTNDzPj8C1BEy7HGAxgYFtPHck/teE1MO8hKqUCHmNAvF9E
> vDNUG9hgLCs1Sq7/FteJo08sUltIB0dBOtCXYWNJvTKry0y1rkwMbdvBzSyW3MiC
> QGAb9v86IQsHa9gpLGRJjKy9ALt8Y+K2pCQDGGMrtme7gTM5Tvo71ZmNBIIMUAft
> 3VGokTGrscnXEI3BMbJExswOTh+kFwSAUn3rpscFcUiGjDmW5prcMR4afJFARyJs
> FSw3DG3Mgqp88a7GSVYp6QN5yCYmxqVOyddUqTkev4vEAX9CmsiuDeYmWJz9egd1
> BurjiT800yAa97qGGfe5g4YSyDf7VlA0KdPbBmZFCoOyroBtgPQ=
> =TNMF
> -----END PGP SIGNATURE-----
--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
On Mon, Mar 11, 2019 at 10:39 PM Michael Paquier <michael@paquier.xyz> wrote: > On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote: > > * Is the original idea of a special role still viable? > > In my opinion, that part may be valuable. The latest patches proposed > change the way tables are filtered and listed on the subscription > side, lowering the permission to spawn a new thread and to connect to a > publication server by just being a database owner instead of being a > superuser, and that's quite a gap. I agree. I think the original idea was better than what Stephen suggested, and for basically the reasons you mention. However, I'm not sure that you are right when you say "just being a database owner." I think that what's being proposed is that anybody who is a *table* owner could make PostgreSQL run off and try to sync that table from a remote server in perpetuity. That seems like WAY too much access to give an unprivileged user. I don't think we want unprivileged users to be able to launch more or less permanent background processes, nor do we want them to be able to initiate outbound network traffic from the server. Whether we want database owners to be able to do those things is more debatable, but even that would represent a significant expansion of their current rights, IIUC. Just letting the superuser decide who gets to create subscriptions seems good enough from here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 4, 2019 at 1:55 AM Michael Paquier <michael@paquier.xyz> wrote: > - To create a subscription, the user must be a superuser. > + To add tables to a subscription, the user must have ownership rights on the > + table. > [...] > + /* must have CREATE privilege on database */ > + aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(), ACL_CREATE); > + if (aclresult != ACLCHECK_OK) > + aclcheck_error(aclresult, OBJECT_DATABASE, > + get_database_name(MyDatabaseId)); > So this means that we degrade subscription creation requirement from > being a superuser to someone having CREATE rights on a given > database? I find that entirely unacceptable, for reasons which I also talked about on the other thread where this was discussed: https://www.postgresql.org/message-id/CA+TgmoahEoM2zZO71yv4883HFarXcBcOs3if6fEdRcRs8Fs=zA@mail.gmail.com Letting unprivileged users initiate outbound network traffic and more-or-less permanent background processes seems like a terrible idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi! Thanks for comments! > Just letting the superuser decide who gets to create subscriptions > seems good enough from here. I've prepare patch with new system role, i'm not sure about name, called it "pg_subscription_users". In that patch we don't check permissions on target tables, i don't know, should we check it? -------- Efimkin Evgeny
Attachment
Greetings, * Robert Haas (robertmhaas@gmail.com) wrote: > On Mon, Mar 11, 2019 at 10:39 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote: > > > * Is the original idea of a special role still viable? > > > > In my opinion, that part may be valuable. The latest patches proposed > > change the way tables are filtered and listed on the subscription > > side, lowering the permission to spawn a new thread and to connect to a > > publication server by just being a database owner instead of being a > > superuser, and that's quite a gap. > > I agree. I think the original idea was better than what Stephen > suggested, and for basically the reasons you mention. > > However, I'm not sure that you are right when you say "just being a > database owner." I think that what's being proposed is that anybody > who is a *table* owner could make PostgreSQL run off and try to sync > that table from a remote server in perpetuity. That seems like WAY > too much access to give an unprivileged user. I don't think we want > unprivileged users to be able to launch more or less permanent > background processes, nor do we want them to be able to initiate > outbound network traffic from the server. Whether we want database > owners to be able to do those things is more debatable, but even that > would represent a significant expansion of their current rights, IIUC. > > Just letting the superuser decide who gets to create subscriptions > seems good enough from here. It seems I wasn't very clear earlier in the thread- I *definitely* think we need to have a special role which a superuser can grant to certain roles (possibly with the permission to grant further) to allow them to create subscriptions, and that's definitely distinct from "database owner" and shouldn't be combined with that. I view that as the first step towards building a more granular privilege system for subscription creation, and that was the second half of what I was trying to say before- I do think there's value in having something more granular than just "this role can create ANY subscription". As an administrator, I might be fine with subscriptions to system X, but not to system Y, for example. As long as we don't block off the ability to build something finer grained in the future, then having the system role to allow a given role to do create subscription seems fine to me. Thanks! Stephen
Attachment
At Wed, 13 Mar 2019 23:03:26 -0400, Stephen Frost <sfrost@snowman.net> wrote in <20190314030326.GQ6197@tamriel.snowman.net> > Greetings, > > * Robert Haas (robertmhaas@gmail.com) wrote: > > On Mon, Mar 11, 2019 at 10:39 PM Michael Paquier <michael@paquier.xyz> wrote: > > > On Mon, Mar 11, 2019 at 06:32:10PM -0700, Jeff Davis wrote: > > > > * Is the original idea of a special role still viable? > > > > > > In my opinion, that part may be valuable. The latest patches proposed > > > change the way tables are filtered and listed on the subscription > > > side, lowering the permission to spawn a new thread and to connect to a > > > publication server by just being a database owner instead of being a > > > superuser, and that's quite a gap. > > > > I agree. I think the original idea was better than what Stephen > > suggested, and for basically the reasons you mention. > > > > However, I'm not sure that you are right when you say "just being a > > database owner." I think that what's being proposed is that anybody > > who is a *table* owner could make PostgreSQL run off and try to sync > > that table from a remote server in perpetuity. That seems like WAY > > too much access to give an unprivileged user. I don't think we want > > unprivileged users to be able to launch more or less permanent > > background processes, nor do we want them to be able to initiate > > outbound network traffic from the server. Whether we want database > > owners to be able to do those things is more debatable, but even that > > would represent a significant expansion of their current rights, IIUC. > > > > Just letting the superuser decide who gets to create subscriptions > > seems good enough from here. > > It seems I wasn't very clear earlier in the thread- I *definitely* think > we need to have a special role which a superuser can grant to certain > roles (possibly with the permission to grant further) to allow them to > create subscriptions, and that's definitely distinct from "database > owner" and shouldn't be combined with that. > > I view that as the first step towards building a more granular privilege > system for subscription creation, and that was the second half of what I > was trying to say before- I do think there's value in having something > more granular than just "this role can create ANY subscription". As an > administrator, I might be fine with subscriptions to system X, but not > to system Y, for example. As long as we don't block off the ability to > build something finer grained in the future, then having the system role > to allow a given role to do create subscription seems fine to me. The subscription privileges is completely reasonable, but I've heard of users who want to restrict tables on which a role can make subscription. Subscription causes INSERT/UPDATE/DELETE to a table, is it too-much to check the privileges addition to the subscription privileges? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi! > I view that as the first step towards building a more granular privilege > system for subscription creation, and that was the second half of what I > was trying to say before- I do think there's value in having something > more granular than just "this role can create ANY subscription". As an > administrator, I might be fine with subscriptions to system X, but not > to system Y, for example. As long as we don't block off the ability to > build something finer grained in the future, then having the system role > to allow a given role to do create subscription seems fine to me. Do you mean something like `CREATE SERVER` with privileges for each server, which using in CREATE SUBSCRIPTION, very similarway used in foreign data wrapper? -------- Efimkin Evgeny
> 14 марта 2019 г., в 12:56, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): > > Hi! >> I view that as the first step towards building a more granular privilege >> system for subscription creation, and that was the second half of what I >> was trying to say before- I do think there's value in having something >> more granular than just "this role can create ANY subscription". As an >> administrator, I might be fine with subscriptions to system X, but not >> to system Y, for example. As long as we don't block off the ability to >> build something finer grained in the future, then having the system role >> to allow a given role to do create subscription seems fine to me. > Do you mean something like `CREATE SERVER` with privileges for each server, which using in CREATE SUBSCRIPTION, very similarway used in foreign data wrapper? > Let's summarize. To create a subscription into table X user must: 1. be a superuser 2. Or (have role pg_subscription_users 3. and be allowed to write into the table X) 4. Condition 3 can be replaced\extended by "be owner of a the table X". 5. Condition 2 can be replaced\extended by "have privileges for some server remote". Which combination of authorization rules do we want? IMHO 1,2,4 is sufficient. Best regards, Andrey Borodin.
Hi! > 13 марта 2019 г., в 22:55, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а): > > I've prepare patch with new system role, i'm not sure about name, called it "pg_subscription_users". > In that patch we don't check permissions on target tables, i don't know, should we check it? Currently, user with pg_subscription_users can create subscription into any system table, can't they? We certainly need to change it to more secure way. Best regards, Andrey Borodin.
Em qui, 14 de mar de 2019 às 00:03, Stephen Frost <sfrost@snowman.net> escreveu: > > I view that as the first step towards building a more granular privilege > system for subscription creation, and that was the second half of what I > was trying to say before- I do think there's value in having something > more granular than just "this role can create ANY subscription". As an > administrator, I might be fine with subscriptions to system X, but not > to system Y, for example. As long as we don't block off the ability to > build something finer grained in the future, then having the system role > to allow a given role to do create subscription seems fine to me. > Isn't that what HBA rules are for? I don't see a fine grain control if there is no node concept. You need to name the remote replication set(s) to locally control it. Postgres replication is distributed by design (current node doesn't need to store info about all nodes -- just those it is connected to). Node is a centralizing concept (every node has its peers info). Is it worth add complexity to logical replication just to satisfy a fine grain control? In this case, node concept should be adopted in a transparent manner (which means that CREATE PUBLICATION/SUBSCRIPTION should create iif there is no NODE specification) -- old syntax should work but we start to accept node info in both sides. -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Hi! > Currently, user with pg_subscription_users can create subscription into any system table, can't they? > We certainly need to change it to more secure way. No, you can't add system tables to publication. In new patch i add privileges checks on target table, non superuser can'tcreate/refresh subscription if he don't have INSERT, UPDATE, DELETE and TRUNCATE privileges. -------- Efimkin Evgeny
Attachment
On Wed, Mar 20, 2019 at 5:39 AM Evgeniy Efimkin <efimkin@yandex-team.ru> wrote: > Hi! > > Currently, user with pg_subscription_users can create subscription into any system table, can't they? > > We certainly need to change it to more secure way. > No, you can't add system tables to publication. In new patch i add privileges checks on target table, non superuser can'tcreate/refresh subscription if he don't have INSERT, UPDATE, DELETE and TRUNCATE privileges. I don't that's the right approach. That idea kinda makes sense if you think about it as giving permission to publish tables to which they have rights, but that doesn't seem like the right mental model to me. It seems more likely that there is a person whose job it is to set up replication but who doesn't normally interact with the table data itself. In that kind of case, you just want to give the person permission to create subscriptions, without needing to also give them lots of privileges on individual tables (and maybe having whatever they are trying to do fail if you miss a table someplace). But there are some other things that are strange about this too: - If the user's permissions are later revoked, the subscription is unaffected. - If the user creates a subscription that targets a publication which only includes a subset of the insert, update, delete, and truncate operations, they still need all of those permissions on their local table. - We don't typically have operations that require a whole bundle of privileges on the local table -- sometimes you check that you have A on X and B on Y, like for REFERENCES, but needing both A and B on X is somewhat unusual. I think we should view this permission as "you can create subscriptions, plain and simple". -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> 20 марта 2019 г., в 21:46, Robert Haas <robertmhaas@gmail.com> написал(а): > > On Wed, Mar 20, 2019 at 5:39 AM Evgeniy Efimkin <efimkin@yandex-team.ru> wrote: >> Hi! >>> Currently, user with pg_subscription_users can create subscription into any system table, can't they? >>> We certainly need to change it to more secure way. >> No, you can't add system tables to publication. In new patch i add privileges checks on target table, non superuser can'tcreate/refresh subscription if he don't have INSERT, UPDATE, DELETE and TRUNCATE privileges. > > .... > > I think we should view this permission as "you can create > subscriptions, plain and simple". That sounds good. From my POV, the purpose of the patch is to allow users to transfer their database via logical replication. Without superuserprivileges (e.g. to the managed cloud with vanilla postgres). But the role effectively allows inserts to any table, this can be escalated to superuser. What is the best way to deal withit? Best regards, Andrey Borodin.
On Wed, Mar 20, 2019 at 11:58:04PM +0800, Andrey Borodin wrote: >> 20 марта 2019 г., в 21:46, Robert Haas <robertmhaas@gmail.com> написал(а): >> I think we should view this permission as "you can create >> subscriptions, plain and simple". > > That sounds good. > From my POV, the purpose of the patch is to allow users to transfer > their database via logical replication. Without superuser privileges > (e.g. to the managed cloud with vanilla postgres). A system role to be able to create subscriptions is perhaps a too big hammer as that would apply to all databases of a system, still we may be able to live with that. Perhaps we would want something at database level different from GRANT CREATE ON DATABASE, but only for subscriptions? This way, it is possible to have per-database groups having the right to create subscriptions, and I'd like to think that we should not include subcription creation into the existing CREATE rights. It would be kind of funny to not have CREATE include the creation of this specific object though :) -- Michael
Attachment
> 21 марта 2019 г., в 8:56, Michael Paquier <michael@paquier.xyz> написал(а): > > On Wed, Mar 20, 2019 at 11:58:04PM +0800, Andrey Borodin wrote: >>> 20 марта 2019 г., в 21:46, Robert Haas <robertmhaas@gmail.com> написал(а): >>> I think we should view this permission as "you can create >>> subscriptions, plain and simple". >> >> That sounds good. >> From my POV, the purpose of the patch is to allow users to transfer >> their database via logical replication. Without superuser privileges >> (e.g. to the managed cloud with vanilla postgres). > > A system role to be able to create subscriptions is perhaps a too big > hammer as that would apply to all databases of a system, still we may > be able to live with that. > > Perhaps we would want something at database level different from GRANT > CREATE ON DATABASE, but only for subscriptions? This way, it is > possible to have per-database groups having the right to create > subscriptions, and I'd like to think that we should not include > subcription creation into the existing CREATE rights. It would be > kind of funny to not have CREATE include the creation of this specific > object though :) I think that small granularity can lead to unnecessary multiplication of subscription. User need to have sufficient minimumnumber of subscriptions, like they have 1 incoming WAL. If we have per-database permission management, user will decide that it is a good thing to divide one subscription to per-databasesubscriptions. Best regards, Andrey Borodin.
Hi! > - If the user's permissions are later revoked, the subscription is unaffected. Now it work the same, if we revoke superuser, subscription is unaffected and replication still work Don't check grants in target database is very dangerous, i create publication with system tables(it's not difficult) select * from pg_publication_tables ; pubname | schemaname | tablename ---------+------------+-------------------- pub | pg_catalog | pg_authid (1 row) After that i create subscription, in log i see that 2019-03-21 11:19:50.863 MSK [58599] LOG: logical replication table synchronization worker for subscription "sub_nosuper",table "pg_authid" has started 2019-03-21 11:19:51.039 MSK [58599] ERROR: null value in column "oid" violates not-null constraint 2019-03-21 11:19:51.039 MSK [58599] DETAIL: Failing row contains (null, pg_monitor, f, t, f, f, f, f, f, -1, null, null). 2019-03-21 11:19:51.039 MSK [58599] CONTEXT: COPY pg_authid, line 1: "pg_monitor f t f f f f f -1 \N \N" I think it's no problem use it to attack target server after some hack on publication side. -------- Efimkin Evgeny
Hi! > Perhaps we would want something at database level different from GRANT > CREATE ON DATABASE, but only for subscriptions? How about 4 checks to create subscription for nonsuperuser? 1. Special role for create subscription 2. CREATE ON DATABASE privilege 3. INSERT, UPDATE, DELETE, TRUNCATE, REFERENCE privilege on target table 4. target table not in information_schema and pg_catalog -------- Efimkin Evgeny
Em qua, 20 de mar de 2019 às 21:57, Michael Paquier <michael@paquier.xyz> escreveu: > > Perhaps we would want something at database level different from GRANT > CREATE ON DATABASE, but only for subscriptions? This way, it is > possible to have per-database groups having the right to create > subscriptions, and I'd like to think that we should not include > subcription creation into the existing CREATE rights. It would be > kind of funny to not have CREATE include the creation of this specific > object though :) > It will be really strange but I can live with that. Another idea is CREATE bit to create subscriptions (without replicate) and SUBSCRIBE bit to replicate tables. It is not just a privilege to create a subscription but also to modify tables that a role doesn't have explicit permission. Let's allocate another AclItem? -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
On Thu, Mar 21, 2019 at 10:06:03AM -0300, Euler Taveira wrote: > It will be really strange but I can live with that. Another idea is > CREATE bit to create subscriptions (without replicate) and SUBSCRIBE > bit to replicate tables. It is not just a privilege to create a > subscription but also to modify tables that a role doesn't have > explicit permission. Let's allocate another AclItem? By the way, as the commit fest is coming to its end in a couple of days, and that we are still discussing how the thing should be shaped, I would recommend to mark the patch as returned with feedback. Any objections with that? -- Michael
Attachment
> 22 марта 2019 г., в 9:28, Michael Paquier <michael@paquier.xyz> написал(а): > > On Thu, Mar 21, 2019 at 10:06:03AM -0300, Euler Taveira wrote: >> It will be really strange but I can live with that. Another idea is >> CREATE bit to create subscriptions (without replicate) and SUBSCRIBE >> bit to replicate tables. It is not just a privilege to create a >> subscription but also to modify tables that a role doesn't have >> explicit permission. Let's allocate another AclItem? > > By the way, as the commit fest is coming to its end in a couple of > days, and that we are still discussing how the thing should be shaped, > I would recommend to mark the patch as returned with feedback. Any > objections with that? It seems to me that we have consensus that: 1. We need special role to create subscription 2. This role can create subscription with some security checks 3. We have complete list of possible security checks 4. We have code that implements most of these checks (I believe pg_subscription_role_v2.patch is enough, but we can tightenchecks a little more) Do we have any objection on these points? If not, it is RFC, it should not be returned. Best regards, Andrey Borodin.
On Fri, Mar 22, 2019 at 10:15:59AM +0800, Andrey Borodin wrote: > It seems to me that we have consensus that: > 1. We need special role to create subscription > 2. This role can create subscription with some security checks > 3. We have complete list of possible security checks These are basically that the truncate, insert, delete and insert rights for the role creating the subscription. Why would we actually need that? > 4. We have code that implements most of these checks (I believe > pg_subscription_role_v2.patch is enough, but we can tighten checks a > little more) If a unique system role is the conclusion on the matter, it looks so. > If not, it is RFC, it should not be returned. The patch still needs some work before being RFC. From what I can read, pg_dump still ignores roles which are members of the system role pg_subscription_users and these should be able to dump subscriptions, so you have at least one problem. -- Michael
Attachment
Hi! > These are basically that the truncate, insert, delete and insert > rights for the role creating the subscription. Why would we actually > need that? It's for security reasons. Because possible to attack target server. If publication have system tables for instance pg_authid > pg_subscription_users and these should be able to dump subscriptions, > so you have at least one problem. But in system_views.sql we give grant on subconninfo column and pg_dump required superuser privilege only for postgesql under12 version. Old version pg_dump still works but require superuser for dump subscription. -------- Efimkin Evgeny
Hi, On 22/03/2019 03:15, Andrey Borodin wrote: > >> 22 марта 2019 г., в 9:28, Michael Paquier <michael@paquier.xyz> написал(а): >> >> On Thu, Mar 21, 2019 at 10:06:03AM -0300, Euler Taveira wrote: >>> It will be really strange but I can live with that. Another idea is >>> CREATE bit to create subscriptions (without replicate) and SUBSCRIBE >>> bit to replicate tables. It is not just a privilege to create a >>> subscription but also to modify tables that a role doesn't have >>> explicit permission. Let's allocate another AclItem? >> >> By the way, as the commit fest is coming to its end in a couple of >> days, and that we are still discussing how the thing should be shaped, >> I would recommend to mark the patch as returned with feedback. Any >> objections with that? > > It seems to me that we have consensus that: > 1. We need special role to create subscription > 2. This role can create subscription with some security checks > 3. We have complete list of possible security checks > 4. We have code that implements most of these checks (I believe pg_subscription_role_v2.patch is enough, but we can tightenchecks a little more) > I still don't like that we are running the subscription workers as superuser even for subscriptions created by regular user. That has plenty of privilege escalation issues in terms of how user functions are run (we execute triggers, index expressions etc, in that worker). > Do we have any objection on these points? > > If not, it is RFC, it should not be returned. > Regardless of my complain above, patch with this big security implications that has arrived in middle of last CF should not be merged in that last CF IMHO. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Hi! > 22 марта 2019 г., в 19:17, Petr Jelinek <petr.jelinek@2ndquadrant.com> написал(а): > > I still don't like that we are running the subscription workers as > superuser even for subscriptions created by regular user. That has > plenty of privilege escalation issues in terms of how user functions are > run (we execute triggers, index expressions etc, in that worker). Yes, this is important concern, thanks! I think it is not a big deal to run worker without superuser privileges too. > Regardless of my complain above, patch with this big security > implications that has arrived in middle of last CF should not be merged > in that last CF IMHO. Yes, this patch is a pure security implication and nothing else. This thread was started in November with around twenty messages before this CF. Our wiki states that "in our community --if no one objects, then there is implicit approval. Within reason!" I do not really think argument "last version of the patch arrived at last CF" applies here. But I understand that it is noteasy to setup consensus on the problem at hand. Independently from the willingness of any committer to work on this at current CF, the topic of subscription security relaxationreally worth efforts. Best regards, Andrey Borodin.
On Fri, Mar 22, 2019 at 08:41:06PM +0800, Andrey Borodin wrote: > 22 марта 2019 г., в 19:17, Petr Jelinek <petr.jelinek@2ndquadrant.com> написал(а): >> I still don't like that we are running the subscription workers as >> superuser even for subscriptions created by regular user. That has >> plenty of privilege escalation issues in terms of how user functions are >> run (we execute triggers, index expressions etc, in that worker). > > Yes, this is important concern, thanks! I think it is not a big deal > to run worker without superuser privileges too. FWIW, the argument from Petr is very scary. So please let me think that it is a pretty big deal. > Yes, this patch is a pure security implication and nothing else. And this is especially *why* it needs careful screening. >> Independently from the willingness of any committer to work on this >> at current CF, the topic of subscription security relaxation >> really worth efforts. Perhaps, still it seems that we are still discussing about the concept and that we have no clear agreement on what to do. This is not a good sign 8 days before the end of the last commit fest. -- Michael
Attachment
On 23/03/2019 02:38, Michael Paquier wrote: > On Fri, Mar 22, 2019 at 08:41:06PM +0800, Andrey Borodin wrote: >> 22 марта 2019 г., в 19:17, Petr Jelinek <petr.jelinek@2ndquadrant.com> написал(а): >>> I still don't like that we are running the subscription workers as >>> superuser even for subscriptions created by regular user. That has >>> plenty of privilege escalation issues in terms of how user functions are >>> run (we execute triggers, index expressions etc, in that worker). >> >> Yes, this is important concern, thanks! I think it is not a big deal >> to run worker without superuser privileges too. > Yes we should run without superuser privileges but perhaps more importantly we need to so me kind of security checks on tables while applying - the fact that the user had access to a table when subscription was created does not mean it will have it in 5 minutes and given our low level API usage in the worker, there is currently no check for that. See the 0004 patch in https://www.postgresql.org/message-id/0b477a34-01c5-ad97-b408-79f4e0e6414b@2ndquadrant.com . > FWIW, the argument from Petr is very scary. So please let me think > that it is a pretty big deal. > >> Yes, this patch is a pure security implication and nothing else. > > And this is especially *why* it needs careful screening. > Yep that was exactly my point. I agree the feature is important, it just does not seem like the patch is RFC and given security implications I err on the side of safety here. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Greetings, * Petr Jelinek (petr.jelinek@2ndquadrant.com) wrote: > On 23/03/2019 02:38, Michael Paquier wrote: > > On Fri, Mar 22, 2019 at 08:41:06PM +0800, Andrey Borodin wrote: > >> 22 марта 2019 г., в 19:17, Petr Jelinek <petr.jelinek@2ndquadrant.com> написал(а): > >>> I still don't like that we are running the subscription workers as > >>> superuser even for subscriptions created by regular user. That has > >>> plenty of privilege escalation issues in terms of how user functions are > >>> run (we execute triggers, index expressions etc, in that worker). > >> > >> Yes, this is important concern, thanks! I think it is not a big deal > >> to run worker without superuser privileges too. > > Yes we should run without superuser privileges but perhaps more > importantly we need to so me kind of security checks on tables while > applying - the fact that the user had access to a table when > subscription was created does not mean it will have it in 5 minutes and > given our low level API usage in the worker, there is currently no check > for that. Agreed, and that's exactly the same as what I was telling Andrey at PGConf APAC when he and I were discussing the subscription role. The specific suggestion that I had was to check for every transaction, though that was a pretty off-the-cuff idea and someone might have a better one, certainly. > > FWIW, the argument from Petr is very scary. So please let me think > > that it is a pretty big deal. > > > >> Yes, this patch is a pure security implication and nothing else. > > > > And this is especially *why* it needs careful screening. > > Yep that was exactly my point. > > I agree the feature is important, it just does not seem like the patch > is RFC and given security implications I err on the side of safety here. Agreed. Thanks! Stephen
Attachment
On Sat, Mar 23, 2019 at 10:52:52AM -0400, Stephen Frost wrote: > * Petr Jelinek (petr.jelinek@2ndquadrant.com) wrote: >> I agree the feature is important, it just does not seem like the patch >> is RFC and given security implications I err on the side of safety here. > > Agreed. Based on the latest exchanges, I am marking the patch as returned with feedback. -- Michael
Attachment
On Thu, Mar 21, 2019 at 9:28 PM Michael Paquier <michael@paquier.xyz> wrote: > By the way, as the commit fest is coming to its end in a couple of > days, and that we are still discussing how the thing should be shaped, > I would recommend to mark the patch as returned with feedback. Any > objections with that? +1. It doesn't even seem that we agree on how it should be designed, still less the implementation. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company