MIME-Version: 1.0
References: <149ff9264db27cdf724b65709fbbaee4bf316835.camel@j-davis.com>
 <CAExHW5vv-78ixJs3arsuZ+12A4AyqehJnDPhejp0m4FAKG6JZA@mail.gmail.com>
 <830a2bc6cbbb2e6e01c6c0d9f31f320822e10603.camel@j-davis.com>
 <CAExHW5sAo4Gv3AwuxQNGSQ2Z45B8_MZEb-SG-B8D-hLMdrquAg@mail.gmail.com>
 <e531ec798298a21d671babd45849092850b8675e.camel@j-davis.com>
 <ac4042d6-204c-9661-9949-79ab8a1f8604@joeconway.com> <CAExHW5unvpDv6yMSmqurHP7Du1PqoJFWVxeK-4YNm5EnoNJiSQ@mail.gmail.com>
 <433d0845248e86c0317d9d396926182cfe157340.camel@j-davis.com>
 <CAExHW5sQVJeCgCGMkum_k4Z4VPaK5GkdAjHW-d+F3MngBZ9qBA@mail.gmail.com>
 <05ae37abb207cd6bf6b126780024692d91402b0b.camel@j-davis.com>
 <93392ffa941ab0d436e19e0ab5d04d0e42c02d3f.camel@j-davis.com> <26be917cb07b6aa3ef5dd15f6b59d1b375ece6e8.camel@j-davis.com>
In-Reply-To: <26be917cb07b6aa3ef5dd15f6b59d1b375ece6e8.camel@j-davis.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 2 Jan 2024 15:14:36 +0530
Message-ID: <CALj2ACXDua2Az15Kj3OZFaRm49G8-faemiEEv8t9GNCcsxv8Hw@mail.gmail.com>
Subject: Re: [17] CREATE SUBSCRIPTION ... SERVER
To: Jeff Davis <pgsql@j-davis.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Joe Conway <mail@joeconway.com>, 
	pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALj2ACXDua2Az15Kj3OZFaRm49G8-faemiEEv8t9GNCcsxv8Hw%40mail.gmail.com>
Precedence: bulk

On Mon, Jan 1, 2024 at 12:29=E2=80=AFAM Jeff Davis <pgsql@j-davis.com> wrot=
e:
>
> On Fri, 2023-12-29 at 15:22 -0800, Jeff Davis wrote:
> > On Tue, 2023-09-05 at 12:08 -0700, Jeff Davis wrote:
> > > OK, so we could have a built-in FDW called pg_connection that would
> > > do
> > > the right kinds of validation; and then also allow other FDWs but
> > > the
> > > subscription would have to do its own validation.
> >
> > Attached a rough rebased version.
>
> Attached a slightly better version which fixes a pg_dump issue and
> improves the documentation.

Hi,  I spent some time today reviewing the v4 patch and below are my
comments. BTW, the patch needs a rebase due to commit 9a17be1e2.

1.
+        /*
+         * We don't want to allow unprivileged users to be able to trigger
+         * attempts to access arbitrary network destinations, so
require the user
+         * to have been specifically authorized to create connections.
+         */
+        if (!has_privs_of_role(owner, ROLE_PG_CREATE_CONNECTION))

Can the pg_create_connection predefined role related code be put into
a separate 0001 patch? I think this can go in a separate commit.

2. Can one use {FDW, user_mapping, foreign_server} combo other than
the built-in pg_connection_fdw? If yes, why to allow say oracle_fdw
foreign server and user mapping with logical replication? Isn't this a
security concern?

3. I'd like to understand how the permission model works with this
feature amidst various users a) subscription owner b) table owner c)
FDW owner d) user mapping owner e) foreign server owner f) superuser
g) user with which logical replication bg workers (table sync,
{parallel} apply workers) are started up and running.
What if foreign server owner doesn't have permissions on the table
being applied by logical replication bg workers?
What if foreign server owner is changed with ALTER SERVER ... OWNER TO
when logical replication is in-progress?
What if the owner of  {FDW, user_mapping, foreign_server} is different
from a subscription owner with USAGE privilege granted? Can the
subscription still use the foreign server?

4. How does the invalidation of {FDW, user_mapping, foreign_server}
affect associated subscription and vice-versa?

5. What if the password is changed in user mapping with ALTER USER
MAPPING? Will it refresh the subscription so that all the logical
replication workers get restarted with new connection info?

6. How does this feature fit if a subscription is created with
run_as_owner? Will it check if the table owner has permissions to use
{FDW, user_mapping, foreign_server} comob?

7.
+            if (strcmp(d->defname, "user") =3D=3D 0 ||
+                strcmp(d->defname, "password") =3D=3D 0 ||
+                strcmp(d->defname, "sslpassword") =3D=3D 0 ||
+                strcmp(d->defname, "password_required") =3D=3D 0)
+                ereport(ERROR,
+                        (errmsg("invalid option \"%s\" for pg_connection_f=
dw",

+                ereport(ERROR,
+                        (errmsg("invalid user mapping option \"%s\"
for pg_connection_fdw",
+                                d->defname)));

Can we emit an informative error message and hint using
initClosestMatch, updateClosestMatch, getClosestMatch similar to other
FDWs elsewhere in the code?

8.
+                     errmsg("password is required"),
+                     errdetail("Non-superusers must provide a
password in the connection string.")));

The error message and detail look generic, can it be improved to
include something about pg_connection_fdw?

9.
+{ oid =3D> '6015', oid_symbol =3D> 'PG_CONNECTION_FDW',
+  descr =3D> 'Pseudo FDW for connections to Postgres',
+  fdwname =3D> 'pg_connection_fdw', fdwowner =3D> 'POSTGRES',

What if the database cluster is initialized with an owner different
than 'POSTGRES' at the time of initdb? Will the fdwowner be correct in
that case?

10.
+# src/include/catalog/pg_foreign_data_wrapper.dat
+{ oid =3D> '6015', oid_symbol =3D> 'PG_CONNECTION_FDW',

Do we want to REVOKE USAGE ON FOREIGN DATA WRAPPER pg_connection_fdw
FROM PUBLIC and REVOKE EXECUTE ON its handler functions? With this,
the permissions are granted explicitly to the foreign server/user
mapping creators.

11. How about splitting patches in the following manner for better
manageability (all of which can go as separate commits) of this
feature?
0001 for pg_create_connection predefined role per comment #1.
0002 for introducing in-built FDW pg_connection_fdw.
0003 utilizing in-built FDW for logical replication to provide CREATE
SUBSCRIPTION ... SERVER.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com