Re: [WIP] CREATE SUBSCRIPTION with FOR TABLES clause (table filter)

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