Re: [WIP] CREATE SUBSCRIPTION with FOR TABLES clause (table filter) - Mailing list pgsql-hackers

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