Re: [17] CREATE SUBSCRIPTION ... SERVER

On Tue, 2024-01-02 at 15:14 +0530, Bharath Rupireddy wrote:
> 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.

Done (see below for details).

> 2. Can one use {FDW, user_mapping, foreign_server} combo other than
> the built-in pg_connection_fdw?

Yes, you can use any FDW for which you have USAGE privileges, passes
the validations, and provides enough of the expected fields to form a
connection string.

There was some discussion on this point already. Initially, I
implemented it with more catalog and grammar support, which improved
error checking, but others objected that the grammar wasn't worth it
and that it was too inflexible. See:

https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us
https://www.postgresql.org/message-id/CAExHW5unvpDv6yMSmqurHP7Du1PqoJFWVxeK-4YNm5EnoNJiSQ%40mail.gmail.com

> If yes, why to allow say oracle_fdw
> foreign server and user mapping with logical replication? Isn't this
> a
> security concern?

A user would need USAGE privileges on that other FDW and also must be a
member of pg_create_subscription.

In v16, a user with such privileges would already be able to create
such connection by specifying the raw connection string, so that's not
a new risk with my proposal.

> 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.

(a) The subscription owner is only relevant if the subscription is
created with run_as_owner=true, in which case the logical worker
applies the changes with the privileges of the subscription owner. [No
change.]
(b) The table owner is only relevant if the subscription is created
with run_as_owner=false (default), in which case the logical worker
applies the changes with the privileges of the table owner. [No
change.]
(c) The FDW owner is irrelevant, though the creator of a foreign server
must have USAGE privileges on it. [No change.]
(d) User mappings do not have owners. [No change.]
(e) The foreign server owner is irrelevant, but USAGE privileges on the
foreign server are needed to create a subscription to it. [New
behavior.]
(f) Not sure what you mean here, but superusers can do anything. [No
change.]
(g) The actual user the process runs as is still the subscription
owner. If run_as_owner=false, the actions are performed as the table
owner; if run_as_owner=true, the actions are performed as the
subscription owner. [No change.]

There are only two actual changes to the model:

1. Users with USAGE privileges on a foreign server can create
subscriptions using that foreign server instead of a connection string
(they still need to be a member of pg_create_subscription).

2. I created a conceptual separation of privileges between
pg_create_subscription and pg_create_connection; though by default
pg_create_subscription has exactly the same capabilities as before.
There is no behavior change unless the administrator revokes
pg_create_connection from pg_create_subscription.

I'd like to also add the capability for subscriptions to a server to
use a passwordless connection as long as the server is trusted somehow.
The password_required subscription option is already fairly complex, so
we'd need to come up with a sensible way for those options to interact.

> What if foreign server owner doesn't have permissions on the table
> being applied by logical replication bg workers?

The owner of the foreign server is irrelevant -- only the USAGE
privileges on that foreign server matter, and only when it comes to
creating subscriptions.

> What if foreign server owner is changed with ALTER SERVER ... OWNER
> TO
> when logical replication is in-progress?

That should have no effect as long as the USAGE priv is still present.

Note that if the owner of the *subscription* changes, it may find a
different user mapping.

> 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?

Yes.

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

If the user mapping or foreign server change, it causes the apply
worker to re-build the connection string from those objects and restart
if something important changed.

If the FDW changes I don't think that matters.

> 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?

Yes. Notice the subscription_change_cb.

That's actually one of the nice features -- if your connection info
changes, update it in one place to affect all subscriptions to that
server.

> 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?

See above.

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

Done.

> 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?

I believe this is addressed after some refactoring -- the FDW itself
doesn't try to validate that a password exists, because we can't rely
on that anyway (someone can use an FDW with no validation or different
validation). Instead, the subscription does this validation.

Note that there is an unrelated hole in the way the subscription does
the validation of password_required, which will be addressed separately
as a part of this other thread:

https://www.postgresql.org/message-id/e5892973ae2a80a1a3e0266806640dae3c428100.camel%40j-davis.com

> 9.
> +{ oid => '6015', oid_symbol => 'PG_CONNECTION_FDW',
> +  descr => 'Pseudo FDW for connections to Postgres',
> +  fdwname => 'pg_connection_fdw', fdwowner => '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?

Thank you, I changed it to use the conventional BKI_DEFAULT(POSTGRES)
instead. (The previous way worked, but was not consistent with existing
patterns.)

> 10.
> +# src/include/catalog/pg_foreign_data_wrapper.dat
> +{ oid => '6015', oid_symbol => 'PG_CONNECTION_FDW',
>
> Do we want to REVOKE USAGE ON FOREIGN DATA WRAPPER pg_connection_fdw
> FROM PUBLIC

The FDW doesn't have USAGE privileges by default so we don't need to
revoke them.

> and REVOKE EXECUTE ON its handler functions?

It has no handler function.

I don't see a reason to restrict privileges on
postgresql_fdw_validator(); it seems useful for testing/debugging.

> 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.

Good suggestion, though I split it a bit differently:

0001: fix postgresql_fdw_validator to use libpq options via walrcv
method. This is appropriate for looser validation that doesn't try to
check for password_required or that a password is set -- that's left up
to the subscription.

0002: built-in pg_connection_fdw, also includes code for validation and
transforming into a connection string. This creates a lot of test diffs
in foreign_data.out because I need to exclude the built in FDW (it's
owned by the bootstrap supseruser which is not a stable username). It
would be nice if there was a way to use a negative-matching regex in a
psql \dew+ command -- something like "(?!pg_)*" -- but I couldn't find
a way to do that because "(?...)" seems to not work in psql. Let me
know if you know a trick to do so.

0003: CREATE SUBSCRIPTION... SERVER.

0004: Add pg_create_connection role.

Regards,
Jeff Davis